[BUG] SSDV.nextOrd() should not make use of NO_MORE_DOCS

[rishabhmaurya]

Describe the bug

Lucene 10 makes it illegal to call SortedSetDocValues#nextOrd() more than#docValueCount() times for the currently-positioned doc.
https://github.com/apache/lucene/blob/e7f9bc837e419672d9bfc829d01e643df667e9d4/lucene/core/src/java/org/apache/lucene/index/SortedSetDocValues.java#L36

Remove all occurrences of SSDV.nextOrd() != NO_MORE_DOCS and replace with use of SSDV.docValuesCount() like in #17626

I consider this as a blocker to OpenSearch 3.0 which is making use of lucene 10.

Related component

Search

To Reproduce

This was causing flakiness in MultiOrdinalsTests as encountered in PR #17446 with failure https://build.ci.opensearch.org/job/gradle-check/54686/testReport/junit/org.opensearch.index.fielddata.ordinals/MultiOrdinalsTests/testRandomValues/

Expected behavior

Remove all occurrences of SSDV.nextOrd() != NO_MORE_DOCS and replace with use of SSDV.docValuesCount()

Additional Details

Plugins
Please list all plugins currently enabled.

Screenshots
If applicable, add screenshots to help explain your problem.

Host/Environment (please complete the following information):

• OS: [e.g. iOS]
• Version [e.g. 22]

Additional context
Add any other context about the problem here.

[rishabhmaurya]

@expani

[expani]

Thanks for opening the issue @rishabhmaurya

Please assign this issue to me.

Was able to reproduce with

./gradlew ':server:test' --tests "org.opensearch.index.fielddata.ordinals.MultiOrdinalsTests.testRandomValues" -Dtests.seed=E4F93E607FE81E76 -Dtests.security.manager=true -Dtests.jvm.argline="-XX:TieredStopAtLevel=1 -XX:ReservedCodeCacheSize=64m" -Dtests.locale=en-AU -Dtests.timezone=America/Indiana/Vevay -Druntime.java=21

Error

  2> java.lang.AssertionError: expected:<2147483647> but was:<0>
        at __randomizedtesting.SeedInfo.seed([E4F93E607FE81E76:9A5E63094A6A3168]:0)
        at org.junit.Assert.fail(Assert.java:89)
        at org.junit.Assert.failNotEquals(Assert.java:835)
        at org.junit.Assert.assertEquals(Assert.java:647)
        at org.junit.Assert.assertEquals(Assert.java:633)
        at org.opensearch.index.fielddata.ordinals.MultiOrdinalsTests.testRandomValues(MultiOrdinalsTests.java:134)
  2> NOTE: leaving temporary files on disk at: /Users/anijainc/Desktop/GithubWorkspaces/OS_30_Aplha1/OpenSearch/server/build/testrun/test/temp/org.opensearch.index.fielddata.ordinals.MultiOrdinalsTests_E4F93E607FE81E76-001
  2> NOTE: test params are: codec=Asserting(Lucene101): {}, docValues:{}, maxPointsInLeafNode=446, maxMBSortInHeap=5.21462929723478, sim=Asserting(RandomSimilarity(queryNorm=false): {}), locale=en-AU, timezone=America/Indiana/Vevay
  2> NOTE: Mac OS X 15.3.1 aarch64/Amazon.com Inc. 21.0.5 (64-bit)/cpus=10,threads=2,free=436266416,total=536870912
  2> NOTE: All tests run in this JVM: [MultiOrdinalsTests]

[msfroh]

Looks like there's also an occurrence in MissingValuesTests

I think the implementation of MissingValues may need some work. Specifically, it pretends that a doc that has no values instead has 1 value. Unfortunately, it always passes through values.docValueCount(), which may be incorrect if the call to advanceExact() failed:

OpenSearch/server/src/main/java/org/opensearch/search/aggregations/support/MissingValues.java

Lines 317 to 328 in 8447737

	public boolean advanceExact(int doc) throws IOException {
	hasOrds = values.advanceExact(doc);
	nextMissingOrd = missingOrd;
	// always return true because we want to return a value even if
	// the document does not have a value
	return true;
	}
	@Override
	public int docValueCount() {
	return values.docValueCount();
	}

I believe the correct implementation of docValuesCount() will return 1 if hasOrd == false.

We also have some redundant checks where we're checking both docValueCount() and NO_MORE_DOCS:

• OpenSearch/server/src/main/java/org/opensearch/search/aggregations/bucket/terms/GlobalOrdinalsStringTermsAggregator.java

  Line 311 in 218f353

  while ((count-- > 0) && (globalOrd = globalOrds.nextOrd()) != SortedSetDocValues.NO_MORE_DOCS) {

• OpenSearch/server/src/main/java/org/opensearch/search/aggregations/metrics/CardinalityAggregator.java

  Line 565 in e3a6cca

  while ((count-- > 0) && (ord = values.nextOrd()) != SortedSetDocValues.NO_MORE_DOCS) {

• OpenSearch/server/src/main/java/org/opensearch/index/mapper/IpFieldMapper.java

  Line 457 in 7c46f8f

  for (long ord = in.nextOrd(); ord != SortedSetDocValues.NO_MORE_DOCS && count < in.docValueCount(); ord = in

• OpenSearch/server/src/test/java/org/opensearch/search/MultiValueModeTests.java

  Line 887 in 7c46f8f

  for (long ord = values.nextOrd(); ord != SortedSetDocValues.NO_MORE_DOCS

Those redundant checks aren't a problem AFAIK, but we should probably just check docValueCount().

[msfroh]

Oh -- sorry, @expani, I just fixed MissingValues: #17647

[expani]

Thanks @msfroh opening the PR with fix and this draft PR #17649 to check if any callers are still misbehaving by throwing exceptions instead of NO_MORE_DOCS