Fix UnsupportedOperationException on nested sort during early termination

[serhiy-bzhezytskyy]

Description

The nested MultiValueMode.select(...parentDocs, childDocs...) variants returned a doc-values instance that overrode only advanceExact, not advance(int). Since #12089 enabled the point-based sort optimization, NumericComparator's competitive iterator calls advance() during early termination (i.e. track_total_hits: false), which hit the base AbstractNumericDocValues.advance() and threw UnsupportedOperationException. With track_total_hits: true there's no early termination, so advance() isn't called — which is why the failure only appeared without it.

This implements advance() on the nested selects:

• long, unsigned-long, binary — advanceExact always returns true (a missing value is emitted when no children match), so every parent doc has a value and advance(target) positions directly on target.
• sorted (keyword) — advanceExact can return false (no missing-value fallback for ords), so values are sparse; advance() walks the parent bitset to the next parent that has an ord.
• double — this select already overrode advance(), but as values.advance(target) (advancing the child values iterator). Aligned it to the same parent-positioning semantics as the others, per the discussion on [Bug]: Intermittent UnsupportedOperationException errors with nested queries #17140.

Tests added to MultiValueModeTests covering the numeric, unsigned-long, and double nested selects. Full MultiValueModeTests and the sort / comparator-source suites pass locally.

Related Issues

Resolves #17140
Resolves #21537

Check List

• Functionality includes testing.
• API changes companion pull request created, if applicable. (n/a — no API change)
• Public documentation issue/PR created, if applicable. (n/a)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit d5278db)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ Recommended focus areas for review Non-parent target in advance() In the dense nested selects (long, unsigned-long, binary, double), advance(target) unconditionally returns target and calls advanceExact(target) regardless of whether target is actually a parent doc. DocIdSetIterator.advance() is contractually expected to return a doc that is a member of the iterator (i.e., a parent doc), and callers may then read a value via longValue()/binaryValue()/doubleValue(). If target is a child doc (not in parentDocs), the iterator will position on a non-parent doc and emit a value computed against child docs that don't belong to that "parent", which can mislead consumers. Consider advancing to parentDocs.nextSetBit(target) as done in the sorted variant, to guarantee the returned doc is a parent. public int advance(int target) throws IOException { // advanceExact always returns true (missing value emitted when no children match), so // every parent doc has a value and advance() positions directly on target. if (target >= maxDoc) { return lastSeenParentDoc = DocIdSetIterator.NO_MORE_DOCS; } advanceExact(target); return target; } Ignored advanceExact return advance() calls advanceExact(target) and discards its return value. While the comment asserts it "always returns true", any future change to advanceExact (e.g., adding a bounds/state guard) would silently produce inconsistent state where docID() reflects target but no value was actually emitted. Consider asserting the returned value or handling the false case explicitly to make the invariant enforceable. advanceExact(target); return target;

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to d5278db
Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Fix advance semantics for sorted ords The sorted (ords) advance iterates only over set bits in parentDocs, but this is a SortedDocValues view where every parent doc is a valid position; the semantics expected by callers (e.g. NumericComparator's competitive iterator) is to return the next doc >= target that has a value, regardless of whether it's a "parent" bit. Additionally, nextSetBit throws if target equals parentDocs.length(); the guard should also skip when there is no set bit at/after target. Consider iterating docs sequentially like the other overrides rather than relying on parentDocs.nextSetBit, or at minimum ensure nextSetBit is only called with valid arguments. server/src/main/java/org/opensearch/search/MultiValueMode.java [1255-1273] @Override public int advance(int target) throws IOException { // advanceExact can return false here (no missing-value fallback for ords), so values are - // sparse: find the next parent doc at or after target that actually has an ord. - if (target >= parentDocs.length()) { - return docID = DocIdSetIterator.NO_MORE_DOCS; - } - int parentDoc = parentDocs.nextSetBit(target); - while (parentDoc != DocIdSetIterator.NO_MORE_DOCS) { - if (advanceExact(parentDoc)) { - return docID = parentDoc; + // sparse: find the next doc at or after target that actually has an ord. + for (int doc = target; doc < parentDocs.length(); doc++) { + if (advanceExact(doc)) { + return docID = doc; } - if (parentDoc + 1 >= parentDocs.length()) { - break; - } - parentDoc = parentDocs.nextSetBit(parentDoc + 1); } return docID = DocIdSetIterator.NO_MORE_DOCS; } Suggestion importance[1-10]: 7 __ Why: Valid concern: advance(target) is expected to find the next doc >= target with a value, and restricting iteration to parent-doc bits could skip valid positions or misalign with caller expectations. Also nextSetBit requires index < length, so the guard needs care. The impact is meaningful for correctness in nested sort with ords.	Medium
	Advance to next parent doc, not target DocIdSetIterator.advance must return the next doc >= target that has a value; here every parent doc has a value but target may be a child (non-parent) doc, in which case advanceExact(target) computes an incorrect emitted value using a prevParentDoc derived from the wrong bit. Consider advancing target to the next parent-doc bit via parentDocs.nextSetBit(target) before calling advanceExact, and return NO_MORE_DOCS if none exists, to preserve correctness when the caller advances mid-block. server/src/main/java/org/opensearch/search/MultiValueMode.java [839-848] @Override public int advance(int target) throws IOException { - // advanceExact always returns true (missing value emitted when no children match), so - // every parent doc has a value and advance() positions directly on target. if (target >= maxDoc) { return lastSeenParentDoc = DocIdSetIterator.NO_MORE_DOCS; } - advanceExact(target); - return target; + int parentDoc = parentDocs.nextSetBit(target); + if (parentDoc == DocIdSetIterator.NO_MORE_DOCS) { + return lastSeenParentDoc = DocIdSetIterator.NO_MORE_DOCS; + } + advanceExact(parentDoc); + return parentDoc; } Suggestion importance[1-10]: 6 __ Why: Raises a legitimate concern about advance(target) when target is a non-parent doc—the derived prevParentDoc and pick logic may yield incorrect values. However, in typical usage callers advance to parent-doc positions, so the practical impact may be limited.	Low

---

Previous suggestions

Suggestions up to commit 0b6e88c

Category	Suggestion	Impact
General	Ensure advance positions on parent docs advance(target) is documented to position on the next doc >= target that has a value, but here it unconditionally returns target even if target isn't a parent doc. Per DocIdSetIterator contract, callers typically iterate through parent docs only; if target may point at a non-parent doc, this returns a value for a child doc, which could produce incorrect sort results. Consider advancing to the next parent doc using the parent BitSet when applicable. server/src/main/java/org/opensearch/search/MultiValueMode.java [839-848] @Override public int advance(int target) throws IOException { - // advanceExact always returns true (missing value emitted when no children match), so - // every parent doc has a value and advance() positions directly on target. if (target >= maxDoc) { return lastSeenParentDoc = DocIdSetIterator.NO_MORE_DOCS; } advanceExact(target); return target; } Suggestion importance[1-10]: 4 __ Why: The concern about advance() returning non-parent docs is potentially valid, as callers using this in nested contexts typically expect parent docs. However, the improved_code is identical to the existing_code (only removes the comment), providing no actual fix.	Low
	Verify advance semantics for sparse iterator The advance method iterates only over set bits in parentDocs, but target itself is passed to nextSetBit which requires target < length. More importantly, when target isn't a parent doc, this skips ahead correctly; however the semantics of DocIdSetIterator.advance require positioning on any doc >= target, not just parent docs. If the iterator is expected to only surface parent docs, ensure target being a non-parent doc is handled — currently nextSetBit(target) returns the next parent doc >= target, which is correct, but consider guarding against a target of a negative value or one already past docID. server/src/main/java/org/opensearch/search/MultiValueMode.java [1255-1273] @Override public int advance(int target) throws IOException { - // advanceExact can return false here (no missing-value fallback for ords), so values are - // sparse: find the next parent doc at or after target that actually has an ord. if (target >= parentDocs.length()) { return docID = DocIdSetIterator.NO_MORE_DOCS; } int parentDoc = parentDocs.nextSetBit(target); while (parentDoc != DocIdSetIterator.NO_MORE_DOCS) { if (advanceExact(parentDoc)) { return docID = parentDoc; } if (parentDoc + 1 >= parentDocs.length()) { break; } parentDoc = parentDocs.nextSetBit(parentDoc + 1); } return docID = DocIdSetIterator.NO_MORE_DOCS; } Suggestion importance[1-10]: 2 __ Why: The suggestion is vague and the improved_code is essentially identical to the existing_code (only removing comments). It raises hypothetical concerns without identifying a concrete bug.	Low

[github-actions]

❌ Gradle check result for 0b6e88c: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[serhiy-bzhezytskyy]

The gradle-check failure is RestoreShallowSnapshotV2IT.testContinuousIndexing — an off-by-one doc-count in an assertBusy (expected:<471> but was:<470>, with a node disconnect / leader failover in the logs). It's a timing flake in the remote-store snapshot path, unrelated to this change (this PR only touches MultiValueMode). It's the known flaky tracked in #16658 (also fails on main in post-merge/timer runs).

Could someone re-run gradle-check when convenient? Thanks!

[github-actions]

Persistent review updated to latest commit d5278db

[github-actions]

❌ Gradle check result for d5278db: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[serhiy-bzhezytskyy]

Merged main in (the branch was 35 commits behind) and re-ran locally: MultiValueModeTests 17/17.

The gradle-check failure on d5278db4 is Netty4HttpRequestSizeLimitIT.testLimitsInFlightRequests, and it is neither this change nor a random flake: Netty 4.2.16 capped HttpContentEncoder's pipeline depth at 128 and that test pipelines 150 requests, so the connection is closed and all 150 responses are lost. #22403 had already capped the sibling test in that class for this reason and left this one at 150. I've put the same cap up as #22636, which CI confirms — the same test passes there.

This PR touches only MultiValueMode.java and its test. Could someone re-run gradle-check once #22636 is in? And it has been waiting on review since 22 July — @msfroh @reta, you asked for a fix on #17140, so is there anything you'd like changed here?

[msfroh]

This is great! Thank you @serhiy-bzhezytskyy.

Out of curiosity, if we remove the advance() method from AbstractNumericDocValues, do we still compile?

Overall, I think with Lucene's changes to sorting logic, we need advance implementations for any subclass of AbstractNumericDocValues. That means we should probably remove the implementation that throws UnsupportedOperationException. That can be a follow-up PR, though.

[github-actions]

✅ Gradle check result for d5278db: SUCCESS

[codecov]

Codecov Report

❌ Patch coverage is 44.44444% with 15 lines in your changes missing coverage. Please review.
✅ Project coverage is 71.40%. Comparing base (bacf3f6) to head (d5278db).
⚠️ Report is 35 commits behind head on main.

Files with missing lines	Patch %	Lines
...ain/java/org/opensearch/search/MultiValueMode.java	44.44%	15 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff            @@
##               main   #22534   +/-   ##
=========================================
  Coverage     71.39%   71.40%           
+ Complexity    76808    76807    -1     
=========================================
  Files          6148     6148           
  Lines        357994   358025   +31     
  Branches      52179    52187    +8     
=========================================
+ Hits         255607   255647   +40     
- Misses        82054    82071   +17     
+ Partials      20333    20307   -26     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[serhiy-bzhezytskyy]

if we remove the advance() method from AbstractNumericDocValues, do we still compile?

No — I tried it. Removing it breaks compilation at 12 subclasses: MultiValueMode ×2 (the non-nested selects), the Diversified*SamplerAggregators ×5, CollapsingDocValuesSource, and 4 in tests.

codecov/patch also caught a real gap: my tests only used numeric values, so the binary and ordinals selectors were never exercised — and the ordinals one is the only non-trivial body here. I've added two tests locally; both fail on main with UnsupportedOperationException. Add them here, or with the follow-up?

[serhiy-bzhezytskyy]

@msfroh fyi pushed two tests covering the ords and binary selects — the paths codecov flagged.