Skip to content

Fix UnsupportedOperationException on nested sort during early termination#22534

Open
serhiy-bzhezytskyy wants to merge 1 commit into
opensearch-project:mainfrom
serhiy-bzhezytskyy:fix/17140-nested-select-advance
Open

Fix UnsupportedOperationException on nested sort during early termination#22534
serhiy-bzhezytskyy wants to merge 1 commit into
opensearch-project:mainfrom
serhiy-bzhezytskyy:fix/17140-nested-select-advance

Conversation

@serhiy-bzhezytskyy

Copy link
Copy Markdown

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, binaryadvanceExact 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.

…tion (opensearch-project#17140, opensearch-project#21537)

The nested MultiValueMode.select(...parentDocs, childDocs...) variants returned an
AbstractNumericDocValues/AbstractBinaryDocValues/AbstractSortedDocValues that overrode
only advanceExact, not advance(int). Since opensearch-project#12089 enabled point-based sort optimization,
NumericComparator's competitive iterator calls advance() during early termination
(track_total_hits:false), hitting the base UnsupportedOperationException.

Implements advance() on all four nested selects that lacked it (long, unsigned-long,
binary, sorted/keyword). For the numeric/binary selects advanceExact always returns true
(missing-value fallback), so advance(target) positions on target. The sorted select is
sparse (no missing-value fallback for ords), so advance() walks the parent bitset to the
next parent that has an ord. Adds MultiValueModeTests coverage.

Signed-off-by: serhiy-bzhezytskyy <me@serhiy-bzhezytskyy.com>
@github-actions

Copy link
Copy Markdown
Contributor

PR Reviewer Guide 🔍

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

Semantics of advance()

For the dense long/unsigned-long/binary/double selects, advance(target) returns target unconditionally after calling advanceExact(target). The DocIdSetIterator.advance contract requires positioning on the next doc >= target that has a value, and target may be less than the current docID (though typically not). More importantly, if target is not a parent doc, this still returns target and sets lastSeenParentDoc = target, which may confuse callers that iterate via advance then docID() expecting only parent docs. Confirm the caller (NumericComparator competitive iterator) only advances to parent docs; otherwise the returned docID may not correspond to a valid parent, leading to incorrect sort values.

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;
}
Potential infinite/incorrect loop bound

In the sorted (keyword) advance(), the loop uses parentDocs.length() as the bound, but parentDocs is a BitSet whose length may exceed maxDoc. If advanceExact is called with a parent index beyond maxDoc, downstream child iteration may misbehave. Consider bounding with maxDoc consistently, as done in the other selects.

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;
}

@github-actions

Copy link
Copy Markdown
Contributor

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    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

Copy link
Copy Markdown
Contributor

❌ 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

Copy link
Copy Markdown
Author

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working lucene Search Search query, autocomplete ...etc

Projects

None yet

1 participant