Introduces BulkVectorScorer for ExactSearch

[shatejas]

Description

Introduces BulkVectorScorer, a Lucene Scorer implementation that wraps the bulk vector scoring API and encapsulates score filtering logic. This replaces the manual buffer-iteration loops previously spread across ExactSearcher methods, consolidating all exact-search scoring into a single reusable abstraction.

Key changes

• New BulkVectorScorer class — A Scorer backed by VectorScorer.Bulk that iterates documents in batches, applies a score predicate, and exposes standard
• DocIdSetIterator semantics (nextDoc, advance, cost). Two factory methods express intent:
  • forKSearch — accepts all scores (used for top-k collection)
  • forRadialSearch — filters to scores ≥ a minimum threshold
• Simplified ExactSearcher — Removed searchTopK, searchWithMinScore, and the raw buffer loop in scoreAllDocs. All paths now construct the appropriate BulkVectorScorer and pass it to collectTopK or the simplified scoreAllDocs(Scorer). This eliminates duplicated iteration logic and makes the scoring strategy explicit at the call site.

Test plan

• New BulkVectorScorerTests — covers k-search scoring, radial search filtering, filtered doc iteration, advance() semantics (including at/past target),
  cost, initial state, empty docs, and edge cases
• New ExactSearcherTests — unit tests for exact searcher flows
• Existing integration tests (MOSFaissFloatIndexIT, FilteredSearchANNSearchIT) validate end-to-end behavior

Related Issues

Partially Resolves #3348

Check List

• New functionality includes testing.
• New functionality has been documented.
• Commits are signed per the DCO using --signoff.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 42dc5cb)

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 Possible Issue In nextDoc(), when a batch's maxBatchScore fails the scoreFilter or is below minCompetitiveScore, the batch is skipped by setting currentBatchIdx = buffer.size. However, maxBatchScore is the maximum score for the batch; for radial search this correctly skips only when the max is below minScore, but the scoreFilter.test(maxBatchScore) invocation is semantically checking whether the max score passes the predicate as if it were a per-doc score. For a >= minScore predicate this happens to be correct (if max < minScore, no doc can pass), but the intent is unclear and fragile — any future non-monotonic predicate would silently drop valid docs. public int nextDoc() throws IOException { while (true) { int result = scanBufferForMatch(); if (result != -1) { return result; } float maxBatchScore = bulkScorer.nextDocsAndScores(DocIdSetIterator.NO_MORE_DOCS, null, buffer); currentBatchIdx = 0; if (buffer.size == 0) { return currentDocId = NO_MORE_DOCS; } if (!scoreFilter.test(maxBatchScore) || maxBatchScore < minCompetitiveScore) { currentBatchIdx = buffer.size; } } } Possible Issue getMaxScore(int upTo) returns Float.MAX_VALUE unconditionally. When this scorer is used in a context that leverages getMaxScore for dynamic pruning (e.g., wrapped by a competitive iterator), the value effectively disables all upstream max-score based skipping. If that is intentional, a comment explaining why is warranted; otherwise this could regress performance for callers relying on max-score optimizations. public float getMaxScore(int upTo) { return Float.MAX_VALUE; } Inconsistent Nested Check exactSearchScorer checks context.getParentsFilter() != null to decide whether to null out matchedDocs, while searchLeaf uses an isNested flag derived from the same check. These are equivalent today, but keeping the logic duplicated in two places invites drift. Consider extracting a helper (e.g., isNested(context)) to avoid future inconsistencies where one path handles nested consumption correctly and the other does not. // When nested, matchedDocsIterator is already consumed inside NestedBestChildVectorScorer, // so pass null to avoid double consumption of the same iterator. final DocIdSetIterator matchedDocs = context.getParentsFilter() != null ? null : context.getMatchedDocsIterator();

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 42dc5cb

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Guard against NPE on boxed boolean context.isMemoryOptimizedSearchEnabled is a Boolean (boxed) per the existing assert context.isMemoryOptimizedSearchEnabled != null used in doRadialSearch. Here in exactSearchScorer there is no null-check before auto-unboxing, so a null value will throw a NullPointerException instead of the intended assertion / clear error. Add the same null guard or assertion as in doRadialSearch. src/main/java/org/opensearch/knn/index/query/exactsearch/ExactSearcher.java [138-144] final DocIdSetIterator matchedDocs = context.getParentsFilter() != null ? null : context.getMatchedDocsIterator(); if (context.getRadius() != null) { assert extractKNNEngine(fieldInfo) == KNNEngine.FAISS : "Exact searcher for Radial search is only used by FAISS engine"; + assert context.isMemoryOptimizedSearchEnabled != null; final float minScore = context.isMemoryOptimizedSearchEnabled ? context.getRadius() : KNNEngine.FAISS.score(context.getRadius(), getSpaceType(modelDao, fieldInfo)); Suggestion importance[1-10]: 5 __ Why: Adding the same null-check assertion as in doRadialSearch improves consistency and produces a clearer error, though the impact is limited since assertions are typically disabled in production and the field is likely set by callers.	Low
General	Clarify batch-skip logic semantics The batch-level skip uses scoreFilter.test(maxBatchScore) for radial search, but maxBatchScore is the maximum score in the batch. If the max fails the >= minScore predicate, individual docs in the batch could still pass only if their scores exceed the max, which is impossible — so this is fine for radial. However, applying scoreFilter.test at batch level is semantically only valid for monotonic filters (max-score based). For k-search the filter is always true so it's fine, but relying on this coupling is fragile. Consider using an explicit batch-max threshold check separate from the per-doc predicate to avoid future filter-type mismatches. src/main/java/org/opensearch/knn/index/query/exactsearch/BulkVectorScorer.java [64-77] while (true) { int result = scanBufferForMatch(); if (result != -1) { return result; } float maxBatchScore = bulkScorer.nextDocsAndScores(DocIdSetIterator.NO_MORE_DOCS, null, buffer); currentBatchIdx = 0; if (buffer.size == 0) { return currentDocId = NO_MORE_DOCS; } - if (!scoreFilter.test(maxBatchScore) || maxBatchScore < minCompetitiveScore) { + // Skip batch only if max score cannot satisfy per-doc constraints + if (maxBatchScore < minCompetitiveScore || !scoreFilter.test(maxBatchScore)) { currentBatchIdx = buffer.size; } } Suggestion importance[1-10]: 2 __ Why: The suggestion only reorders the two conditions in an OR expression, which is semantically equivalent. The added comment is minor and offers marginal value.	Low
	Warn on radial heap saturation For radial search (updateMinCompetitiveScore=false), the heap is filled with sentinel entries whose score is -∞. Any doc passing the radial minScore will always be > topDoc.score initially, but once the heap fills, topDoc.score becomes the minimum retained score, and the condition score > topDoc.score will incorrectly evict lower-scoring valid results silently — matching prior behavior. However, collectTopDocs (not shown) must still discard sentinel entries; ensure the invocation with heapSize = maxResultWindow doesn't degrade recall when candidates exceed maxResultWindow. Consider documenting this bounded-window behavior or logging a warning when the heap is saturated for radial search. src/main/java/org/opensearch/knn/index/query/exactsearch/ExactSearcher.java [250-261] for (int doc = iter.nextDoc(); doc != DocIdSetIterator.NO_MORE_DOCS; doc = iter.nextDoc()) { float score = scorer.score(); if (score > topDoc.score) { topDoc.score = score; topDoc.doc = doc; topDoc = queue.updateTop(); collectedCount++; if (updateMinCompetitiveScore && collectedCount >= heapSize) { scorer.setMinCompetitiveScore(topDoc.score); } + } else if (!updateMinCompetitiveScore && collectedCount >= heapSize) { + log.debug("Radial search heap saturated at maxResultWindow={}, some results may be dropped", heapSize); } } Suggestion importance[1-10]: 2 __ Why: Adding a debug log inside a hot per-doc loop is not a strong improvement and could add noise/overhead; the suggestion is more of a documentation concern than a correctness fix.	Low

---

Previous suggestions

Suggestions up to commit 46bea86

Category	Suggestion	Impact
Possible issue	Guard against premature min-score updates collectedCount is incremented on every heap update rather than per distinct document collected, so setMinCompetitiveScore may be called before the heap is actually full (since HitQueue pre-fills with sentinel entries with score -Float.MAX_VALUE, and topDoc.score > -Float.MAX_VALUE triggers updateTop, but collectedCount reaching heapSize doesn't guarantee that all sentinel entries have been displaced). Consider tracking the number of unique docs collected or checking queue.top().score > -Float.MAX_VALUE before setting the min competitive score to avoid setting a too-low threshold prematurely. src/main/java/org/opensearch/knn/index/query/exactsearch/ExactSearcher.java [251-262] for (int doc = iter.nextDoc(); doc != DocIdSetIterator.NO_MORE_DOCS; doc = iter.nextDoc()) { float score = scorer.score(); if (score > topDoc.score) { topDoc.score = score; topDoc.doc = doc; topDoc = queue.updateTop(); collectedCount++; - if (updateMinCompetitiveScore && collectedCount >= heapSize) { + if (updateMinCompetitiveScore && collectedCount >= heapSize && topDoc.score > -Float.MAX_VALUE) { scorer.setMinCompetitiveScore(topDoc.score); } } } Suggestion importance[1-10]: 6 __ Why: Valid concern: HitQueue is pre-filled with sentinel entries, so collectedCount >= heapSize after heapSize updates does guarantee sentinels are displaced, but the additional guard against -Float.MAX_VALUE is a reasonable safety check. Impact is moderate as the current logic is likely correct but the guard adds robustness.	Low
General	Validate batch-skip predicate assumptions The scoreFilter.test(maxBatchScore) check to skip an entire batch is incorrect for radial search. maxBatchScore is the maximum score in the batch; if it fails the >= minScore predicate, individual docs might still fail, but the current logic is only safe when the predicate is monotonic. However, for radial search, if maxBatchScore < minScore, then all docs necessarily fail, which is fine — but predicates like score >= minScore applied to maxBatchScore correctly filter batches. The real issue is that this batch-skip uses the same predicate semantics; ensure batch skipping only occurs when maxBatchScore itself fails the threshold check, which is correct here. Actually the concern: for k-search predicate always returns true, so it's a no-op. Looks OK — but consider that maxBatchScore may not always be a valid upper bound if bulkScorer doesn't guarantee it. Verify that nextDocsAndScores returns a true max-score upper bound. src/main/java/org/opensearch/knn/index/query/exactsearch/BulkVectorScorer.java [64-77] while (true) { int result = scanBufferForMatch(); if (result != -1) { return result; } float maxBatchScore = bulkScorer.nextDocsAndScores(DocIdSetIterator.NO_MORE_DOCS, null, buffer); currentBatchIdx = 0; if (buffer.size == 0) { return currentDocId = NO_MORE_DOCS; } - if (!scoreFilter.test(maxBatchScore) || maxBatchScore < minCompetitiveScore) { + // Skip entire batch only if the batch's max score cannot satisfy either filter + if (maxBatchScore < minCompetitiveScore || !scoreFilter.test(maxBatchScore)) { currentBatchIdx = buffer.size; } } Suggestion importance[1-10]: 2 __ Why: The suggestion is largely a reorder of the existing conditions with no functional change; the original code already correctly skips batches when either filter fails. Marginal at best.	Low
	Avoid redundant iterator allocation for cost Calling vectorScorer.iterator() here just to obtain cost() may allocate a separate iterator that is unused after this line, and depending on implementation it can conflict with the iterator already consumed by vectorScorer.bulk(matchedDocs). Consider caching the cost lazily or ensuring iterator() invocation is safe post-bulk(). Also, matchedDocs.cost() might have already been consumed if the iterator was advanced elsewhere; document the precondition. src/main/java/org/opensearch/knn/index/query/exactsearch/BulkVectorScorer.java [32-37] private BulkVectorScorer(final VectorScorer vectorScorer, final DocIdSetIterator matchedDocs, final Predicate<Float> scoreFilter) throws IOException { this.bulkScorer = vectorScorer.bulk(matchedDocs); this.scoreFilter = scoreFilter; this.cost = matchedDocs != null ? matchedDocs.cost() : vectorScorer.iterator().cost(); + assert scoreFilter != null : "scoreFilter must not be null"; } Suggestion importance[1-10]: 2 __ Why: The suggestion mainly adds an assertion without addressing the raised concern, and the improved code is essentially the same as the existing code. Low impact.	Low

Suggestions up to commit 8a22b0f

Category	Suggestion	Impact
Possible issue	Replace assertion with explicit null check Using assert for null-check on vectorScorer is unsafe because assertions are disabled by default in production JVMs. If createVectorScorer returns null, this will pass through and cause a NullPointerException on the subsequent vectorScorer.bulk(...) call inside BulkVectorScorer. Replace the assertion with an explicit null check returning null (matching the behavior of searchLeaf). src/main/java/org/opensearch/knn/index/query/exactsearch/ExactSearcher.java [130-131] final VectorScorer vectorScorer = createVectorScorer(reader, fieldInfo, leafReaderContext, context); -assert vectorScorer != null; +if (vectorScorer == null) { + return null; +} Suggestion importance[1-10]: 6 __ Why: Valid point that assertions are disabled in production. However, searchLeaf also uses similar patterns and createVectorScorer may not actually return null in practice, but matching searchLeaf's null-handling improves robustness.	Low
	Fix advance() to honor Lucene's forward-progress contract The advance method may incorrectly return the current doc when currentDocId is already past target but the caller expects forward progress. Per Lucene contract, advance(target) must return a doc >= target AND must advance past the current position. The early-return when currentDocId >= target could return a stale doc that was already consumed, breaking downstream collectors. Remove the early return or ensure proper semantics. src/main/java/org/opensearch/knn/index/query/exactsearch/BulkVectorScorer.java [81-94] @Override public int advance(int target) throws IOException { - if (currentDocId >= target) { - return currentDocId; - } while (true) { int doc = nextDoc(); if (doc == NO_MORE_DOCS) { return NO_MORE_DOCS; } if (doc >= target) { return doc; } } } Suggestion importance[1-10]: 3 __ Why: The early-return is actually consistent with Lucene's DocIdSetIterator.advance contract when currentDocId >= target (the iterator must not go backwards). The suggestion's claim about "stale doc" is incorrect since currentDocId reflects the last returned doc, not a consumed one.	Low
General	Clarify batch-level pruning semantics Using scoreFilter.test(maxBatchScore) to skip an entire batch is incorrect for k-search where the filter is score -> true (fine), but conceptually it conflates per-doc filtering with batch-level max-score pruning. For radial search, skipping the whole batch when maxBatchScore < minScore is correct, but using the same predicate for per-doc filtering can be misleading. More importantly, if maxBatchScore is somehow returned as a sentinel or NaN, the predicate could incorrectly skip valid batches. Consider explicitly comparing maxBatchScore against minScore for batch pruning only. src/main/java/org/opensearch/knn/index/query/exactsearch/BulkVectorScorer.java [69-76] float maxBatchScore = bulkScorer.nextDocsAndScores(DocIdSetIterator.NO_MORE_DOCS, null, buffer); currentBatchIdx = 0; if (buffer.size == 0) { return currentDocId = NO_MORE_DOCS; } -if (!scoreFilter.test(maxBatchScore) || maxBatchScore < minCompetitiveScore) { +if (maxBatchScore < minCompetitiveScore || !scoreFilter.test(maxBatchScore)) { currentBatchIdx = buffer.size; } Suggestion importance[1-10]: 2 __ Why: The suggestion only reorders the conditions in the boolean expression without changing the logic. The change is essentially cosmetic with marginal benefit.	Low
	Verify sentinel filtering in collectTopDocs In radial search (updateMinCompetitiveScore=false), the heap is sized to maxResultWindow. When the candidate set exceeds maxResultWindow, results are correctly bounded, but the final collectTopDocs(queue) call (not shown) likely returns sentinel entries since score > topDoc.score may never displace initial sentinels with score -Infinity. Ensure collectTopDocs filters out sentinel entries to avoid returning bogus docs, especially when fewer than heapSize docs pass the filter. src/main/java/org/opensearch/knn/index/query/exactsearch/ExactSearcher.java [247-258] +for (int doc = iter.nextDoc(); doc != DocIdSetIterator.NO_MORE_DOCS; doc = iter.nextDoc()) { + float score = scorer.score(); + if (score > topDoc.score) { + topDoc.score = score; + topDoc.doc = doc; + topDoc = queue.updateTop(); + collectedCount++; + if (updateMinCompetitiveScore && collectedCount >= heapSize) { + scorer.setMinCompetitiveScore(topDoc.score); + } + } +} - Suggestion importance[1-10]: 2 __ Why: The suggestion only asks to verify behavior in collectTopDocs (not shown) and the existing_code is identical to improved_code. It is a verification request without a concrete change.	Low

Suggestions up to commit d4d608d

Category	Suggestion	Impact
Possible issue	Replace assertion with proper null check Using assert for null-checking vectorScorer is unsafe because assertions are disabled in production by default. If createVectorScorer returns null (e.g., when there are no vector files in the segment, as handled in searchLeaf), this will lead to a NullPointerException downstream. Replace the assertion with an explicit null check that returns null, mirroring the behavior of searchLeaf. src/main/java/org/opensearch/knn/index/query/exactsearch/ExactSearcher.java [130-131] final VectorScorer vectorScorer = createVectorScorer(reader, fieldInfo, leafReaderContext, context); -assert vectorScorer != null; +if (vectorScorer == null) { + return null; +} Suggestion importance[1-10]: 7 __ Why: Valid concern: assertions are disabled in production, so a null vectorScorer would cause an NPE. Adding a proper null check aligns with the behavior in searchLeaf and improves robustness.	Medium
	Fix min competitive score trigger condition collectedCount is incremented every time a doc replaces the heap top, not when a doc is collected. This means for k-search with k results, collectedCount may exceed heapSize long before the heap is actually full of real (non-sentinel) entries, or it may reach heapSize only after heapSize replacements. The intent appears to be "once heap is full, start using min competitive score", but the current condition triggers based on number of replacements. Track the actual number of distinct docs inserted, or simply always call setMinCompetitiveScore(topDoc.score) after each updateTop once at least heapSize docs have been seen by the iterator. src/main/java/org/opensearch/knn/index/query/exactsearch/ExactSearcher.java [246-257] +int seenDocs = 0; for (int doc = iter.nextDoc(); doc != DocIdSetIterator.NO_MORE_DOCS; doc = iter.nextDoc()) { + seenDocs++; float score = scorer.score(); if (score > topDoc.score) { topDoc.score = score; topDoc.doc = doc; topDoc = queue.updateTop(); - collectedCount++; - if (updateMinCompetitiveScore && collectedCount >= heapSize) { + if (updateMinCompetitiveScore && seenDocs >= heapSize) { scorer.setMinCompetitiveScore(topDoc.score); } } } Suggestion importance[1-10]: 6 __ Why: Valid observation: collectedCount increments only on heap replacement, so setMinCompetitiveScore may be triggered later than intended. The fix improves correctness of competitive score pruning, though impact is moderate.	Low
General	Avoid autoboxing in hot scoring path Using Predicate causes autoboxing of every score on each call to test, which can be a measurable performance hit in a tight scoring loop over many documents. Define and use a primitive FloatPredicate functional interface (or use DoublePredicate with cast) to avoid boxing in the hot path. src/main/java/org/opensearch/knn/index/query/exactsearch/BulkVectorScorer.java [24] -private final Predicate<Float> scoreFilter; +@FunctionalInterface +private interface FloatPredicate { + boolean test(float value); +} +private final FloatPredicate scoreFilter; Suggestion importance[1-10]: 5 __ Why: Valid performance concern: Predicate<Float> causes autoboxing per score check in a hot loop. Using a primitive FloatPredicate would avoid this, though the actual performance impact may be modest depending on JIT optimizations.	Low
	Guard buffer scan against empty buffer scanBufferForMatch is called before the buffer is ever filled, which on the first call returns -1 correctly but on subsequent iterations may re-scan a stale buffer if the previous batch was fully consumed. More importantly, when the loop falls through after skipping a batch (setting currentBatchIdx = buffer.size), the next iteration calls scanBufferForMatch which returns -1, then refills - this is fine, but be aware that buffer.size == 0 check should also reset currentDocId only once. Additionally, the initial state has an uninitialized buffer; ensure scanBufferForMatch safely handles buffer.size == 0 initially (it does since the while loop condition fails). Consider also fetching the next batch before scanning to avoid an unnecessary first call returning -1. src/main/java/org/opensearch/knn/index/query/exactsearch/BulkVectorScorer.java [63-78] @Override public int nextDoc() throws IOException { while (true) { - int result = scanBufferForMatch(); - if (result != -1) { - return result; + if (currentBatchIdx < buffer.size) { + int result = scanBufferForMatch(); + if (result != -1) { + return result; + } } float maxBatchScore = bulkScorer.nextDocsAndScores(DocIdSetIterator.NO_MORE_DOCS, null, buffer); currentBatchIdx = 0; if (buffer.size == 0) { return currentDocId = NO_MORE_DOCS; } if (!scoreFilter.test(maxBatchScore) || maxBatchScore < minCompetitiveScore) { currentBatchIdx = buffer.size; } } } Suggestion importance[1-10]: 2 __ Why: The existing code already handles an empty buffer correctly via the while (currentBatchIdx < buffer.size) check in scanBufferForMatch. The suggestion adds a redundant guard without fixing an actual bug.	Low

Suggestions up to commit 9e44a73

Category	Suggestion	Impact
Possible issue	Handle null scorer without assertion Relying on assert vectorScorer != null means in production (assertions disabled) a null scorer will throw an opaque NPE on the next line instead of being handled gracefully. searchLeaf returns EMPTY_TOPDOCS in this case; exactSearchScorer should similarly return null to keep behavior consistent and avoid NPEs. src/main/java/org/opensearch/knn/index/query/exactsearch/ExactSearcher.java [130-135] final VectorScorer vectorScorer = createVectorScorer(reader, fieldInfo, leafReaderContext, context); -assert vectorScorer != null; +if (vectorScorer == null) { + return null; +} // When nested, matchedDocsIterator is already consumed inside NestedBestChildVectorScorer, // so pass null to avoid double consumption of the same iterator. final DocIdSetIterator matchedDocs = context.getParentsFilter() != null ? null : context.getMatchedDocsIterator(); Suggestion importance[1-10]: 6 __ Why: Replacing the assertion with a null check provides safer production behavior consistent with searchLeaf, preventing potential NPEs when assertions are disabled.	Low
	Pass min competitive score for pruning The batch-level pruning relies on maxBatchScore representing the maximum score in the buffer, but Lucene's nextDocsAndScores contract only guarantees it as an upper bound when a minCompetitiveScore is supplied (here it's null). For radial search, passing minScore as the competitive threshold would enable proper pruning; otherwise the batch-skip branch can incorrectly discard a buffer where individual doc scores actually meet the filter. src/main/java/org/opensearch/knn/index/query/exactsearch/BulkVectorScorer.java [68-75] -float maxBatchScore = bulkScorer.nextDocsAndScores(DocIdSetIterator.NO_MORE_DOCS, null, buffer); +Float minCompetitive = scoreFilter.test(Float.NEGATIVE_INFINITY) ? null : getMinScore(); +float maxBatchScore = bulkScorer.nextDocsAndScores(DocIdSetIterator.NO_MORE_DOCS, minCompetitive, buffer); currentBatchIdx = 0; if (buffer.size == 0) { return currentDocId = NO_MORE_DOCS; } if (!scoreFilter.test(maxBatchScore)) { currentBatchIdx = buffer.size; } Suggestion importance[1-10]: 5 __ Why: The observation about maxBatchScore semantics depending on the minCompetitiveScore argument is reasonable, but the improved_code references a non-existent getMinScore() and uses a confused predicate logic, making the suggestion not directly applicable.	Low
General	Improve advance performance over buffered docs advance performs a linear scan by repeatedly calling nextDoc, which defeats the purpose of advance and can be very slow when callers skip large doc ranges. Since the underlying bulkScorer typically supports advancing its iterator efficiently, consider delegating to it (or at least documenting the linear behavior) to avoid performance regressions in skip-heavy use cases. src/main/java/org/opensearch/knn/index/query/exactsearch/BulkVectorScorer.java [80-93] @Override public int advance(int target) throws IOException { if (currentDocId >= target) { return currentDocId; } + // Skip buffered docs below target before fetching new batches + while (currentBatchIdx < buffer.size && buffer.docs[currentBatchIdx] < target) { + currentBatchIdx++; + } while (true) { int doc = nextDoc(); - if (doc == NO_MORE_DOCS) { - return NO_MORE_DOCS; - } - if (doc >= target) { + if (doc == NO_MORE_DOCS || doc >= target) { return doc; } } } Suggestion importance[1-10]: 4 __ Why: Skipping already-buffered docs below target is a minor optimization, but the suggestion doesn't fully address the larger inefficiency of not delegating to the underlying iterator's advance, and impact is limited.	Low

Suggestions up to commit 86b889b

Category	Suggestion	Impact
Possible issue	Guard against null boxed Boolean unboxing The isMemoryOptimizedSearchEnabled field is Boolean (boxed) and may be null; using it directly in a ternary expression will trigger a NullPointerException via auto-unboxing. The original doRadialSearch has an assert context.isMemoryOptimizedSearchEnabled != null; guard which is missing here. Add the same null guard or use Boolean.TRUE.equals(...). src/main/java/org/opensearch/knn/index/query/exactsearch/ExactSearcher.java [137-144] if (context.getRadius() != null) { assert extractKNNEngine(fieldInfo) == KNNEngine.FAISS : "Exact searcher for Radial search is only used by FAISS engine"; + assert context.isMemoryOptimizedSearchEnabled != null; final float minScore = context.isMemoryOptimizedSearchEnabled ? context.getRadius() : KNNEngine.FAISS.score(context.getRadius(), getSpaceType(modelDao, fieldInfo)); return BulkVectorScorer.forRadialSearch(vectorScorer, matchedDocs, minScore); } Suggestion importance[1-10]: 6 __ Why: Valid observation: the Boolean field can be null and would NPE on auto-unboxing. Adding the assert mirrors the existing pattern in doRadialSearch and improves robustness, though in practice callers may always set it.	Low
	Fix advance early-return for initial state The advance method's early-return condition currentDocId >= target is incorrect when currentDocId == -1 (initial state) and target <= -1, but more importantly it short-circuits even when the iterator hasn't started. The check should be currentDocId != -1 && currentDocId >= target, or better, only short-circuit when already positioned at or past target after iteration has begun. Otherwise calling advance(0) on a fresh iterator could behave incorrectly in certain edge cases. src/main/java/org/opensearch/knn/index/query/exactsearch/BulkVectorScorer.java [80-93] @Override public int advance(int target) throws IOException { - if (currentDocId >= target) { + if (currentDocId != -1 && currentDocId >= target) { return currentDocId; } while (true) { int doc = nextDoc(); if (doc == NO_MORE_DOCS) { return NO_MORE_DOCS; } if (doc >= target) { return doc; } } } Suggestion importance[1-10]: 3 __ Why: The standard DocIdSetIterator.advance contract requires target > docID(), so callers shouldn't invoke advance(0) on a fresh iterator. The suggested change is defensive but not clearly necessary, and the original behavior matches typical Lucene patterns.	Low
General	Preserve tie-handling semantics in heap update For radial search, the old searchWithMinScore checked score >= minScore && score > topDoc.score, so any doc passing the radius threshold was eligible for the heap even when the heap was not full (sentinel score is -MAX_VALUE). The unified collectTopK now relies on BulkVectorScorer to filter by minScore, which is correct; however, the score > topDoc.score check skips valid radial matches whose scores equal the current heap top. Consider using >= to preserve original semantics for ties when the heap is not yet full. src/main/java/org/opensearch/knn/index/query/exactsearch/ExactSearcher.java [236-248] private static TopDocs collectTopK(final Scorer scorer, final int heapSize) throws IOException { final HitQueue queue = new HitQueue(heapSize, true); ScoreDoc topDoc = queue.top(); DocIdSetIterator iter = scorer.iterator(); for (int doc = iter.nextDoc(); doc != DocIdSetIterator.NO_MORE_DOCS; doc = iter.nextDoc()) { float score = scorer.score(); - if (score > topDoc.score) { + if (score >= topDoc.score) { topDoc.score = score; topDoc.doc = doc; topDoc = queue.updateTop(); } } return collectTopDocs(queue); Suggestion importance[1-10]: 2 __ Why: The original searchTopK also used > for comparison, so the new code preserves k-search semantics. Changing to >= would actually alter behavior and could cause unnecessary heap churn; the claimed behavior change for radial search is minor and the original > is consistent.	Low

[github-actions]

Persistent review updated to latest commit 31cb7d3

[github-actions]

Persistent review updated to latest commit 49f2752

[github-actions]

Persistent review updated to latest commit c9600a2

[github-actions]

Persistent review updated to latest commit 9ccc599

[codecov]

Codecov Report

❌ Patch coverage is 96.51163% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.86%. Comparing base (d256ce9) to head (42dc5cb).
⚠️ Report is 4 commits behind head on main.

Files with missing lines	Patch %	Lines
...rch/knn/index/query/exactsearch/ExactSearcher.java	92.30%	0 Missing and 3 partials ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #3361      +/-   ##
============================================
+ Coverage     83.77%   83.86%   +0.08%     
- Complexity     4371     4418      +47     
============================================
  Files           454      456       +2     
  Lines         15789    15961     +172     
  Branches       2057     2110      +53     
============================================
+ Hits          13228    13386     +158     
- Misses         1777     1778       +1     
- Partials        784      797      +13     

☔ 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.

[github-actions]

Persistent review updated to latest commit 3d58da6

[Vikasht34]

The refactoring is clean and the BulkVectorScorer abstraction makes the code much more readable Thanks @shatejas , I have one concern though , our Old code https://github.com/opensearch-project/k-NN/blob/main/src/main/java/org/opensearch/knn/index/query/exactsearch/ExactSearcher.java#L227-L248 , we had optimization to skip block if score is less than max score

if (maxScore < topDoc.score) {
    continue;  
}

Seems like now we are just using wrapper againt Lucene VectorScorer.Bulk interface that logic is missed . @navneet1v that skiping logic for meant for optimizating latency if I am nit wrong?

[github-actions]

Persistent review updated to latest commit 86b889b

[github-actions]

Persistent review updated to latest commit 9e44a73

[github-actions]

Persistent review updated to latest commit d4d608d

[github-actions]

Persistent review updated to latest commit 8a22b0f

[github-actions]

Persistent review updated to latest commit 46bea86

[navneet1v]

@shatejas can you please use ./gradlew spotlessApply

[github-actions]

Persistent review updated to latest commit 42dc5cb