Skip to content

Rescore radial search quantized complete - #3337

Merged
0ctopus13prime merged 2 commits into
opensearch-project:rescore-radial-quantizedfrom
0ctopus13prime:rescore-radial-search-quantized-complete
May 28, 2026
Merged

Rescore radial search quantized complete#3337
0ctopus13prime merged 2 commits into
opensearch-project:rescore-radial-quantizedfrom
0ctopus13prime:rescore-radial-search-quantized-complete

Conversation

@0ctopus13prime

Copy link
Copy Markdown
Collaborator

Description

Radial search (max_distance/min_score) was previously blocked for all quantized indices due to scoring inaccuracy from quantization error. This PR enables radial search on 1-bit SQ (32x compression) indices by introducing RescoreRadialSearchQuery — a wrapper query that runs quantized radial search as a first pass, then rescores
candidates using full-precision vectors to filter out false positives (vectors whose quantized score fell within the radius but
whose true score does not).
Results are capped at 10k per segment. Supports both Faiss and Lucene engines across all space types (L2, cosine, inner product).

Related Issues

Resolves #[Issue number to be closed when this PR is merged]
N/A

Check List

  • [O] New functionality includes testing.
  • [O] New functionality has been documented.
  • [O] API changes companion pull request created.
  • [O] 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.

Signed-off-by: Dooyong Kim <kdooyong@amazon.com>
@github-actions

github-actions Bot commented May 20, 2026

Copy link
Copy Markdown

PR Reviewer Guide 🔍

(Review updated until commit 3146536)

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

The singleton EXACT_SEARCHER_SINGLETON is checked for null in the constructor but can be set to null via initialize(null) in tests. If a test sets it to null and another test or production code attempts to construct RescoreRadialSearchQuery concurrently or afterward without re-initialization, a NullPointerException will be thrown. This creates a fragile test dependency where test execution order or parallelism can cause failures.

private static ExactSearcher EXACT_SEARCHER_SINGLETON;

/** The inner radial search query that operates on quantized vectors. */
private final Query innerQuery;

/** The name of the knn_vector field being searched. */
private final String field;

/** The original query vector provided by the user. */
private final float[] queryVector;

/**
 * The engine-specific radius threshold.
 * For Faiss, this is a raw distance converted via {@code KNNEngine.distanceToRadialThreshold()}.
 * For Lucene, this is a similarity value converted via {@code KNNEngine.scoreToRadialThreshold()}.
 */
private final float radius;

/**
 * Whether memory-optimized search is enabled for this field.
 * Determines how {@code radius} is interpreted during rescoring:
 * when true, radius is already a Lucene-normalized score;
 * when false, radius is a raw distance requiring conversion via {@code KNNEngine.score()}.
 */
private final boolean memoryOptimizedSearchEnabled;

/**
 * Maximum number of results to retain after rescoring.
 * Derived from the index-level {@code max_result_window} setting when available,
 * otherwise defaults to {@code MAX_RESULTS_RADIAL_RESCORING}.
 * All first-pass candidates are still scored, but only the top results up to this cap are kept.
 */
private final int maxResultsSize;

/**
 * Constructs a new rescoring wrapper for radial search on a quantized index.
 *
 * @param innerQuery                   the inner radial search query (must not be null)
 * @param field                        the knn_vector field name (must not be null)
 * @param queryVector                  the query vector (must not be null)
 * @param radius                       the radius threshold for the search
 * @param memoryOptimizedSearchEnabled whether memory-optimized search is enabled
 */
public RescoreRadialSearchQuery(
    final Query innerQuery,
    final String field,
    final float[] queryVector,
    float radius,
    final boolean memoryOptimizedSearchEnabled,
    final int maxResultsSize
) {
    this.innerQuery = Objects.requireNonNull(innerQuery);
    this.field = Objects.requireNonNull(field);
    this.queryVector = Objects.requireNonNull(queryVector);
    this.radius = radius;
    this.memoryOptimizedSearchEnabled = memoryOptimizedSearchEnabled;
    this.maxResultsSize = maxResultsSize;
    Objects.requireNonNull(EXACT_SEARCHER_SINGLETON, "Exact searcher was not initialized.");
}
Possible Issue

In collectTopDocs, the assertion assert (iterator.cost() > maxResultsSize); will fail if cost() equals maxResultsSize. The condition at line 242 checks > maxResultsSize, so when cost() == maxResultsSize, the method should not be called. However, if the cost estimate is inaccurate or changes between the check and the call, the assertion will fail in production builds with assertions enabled.

private TopDocs collectTopDocs(final Scorer scorer) throws IOException {
    final TopKnnCollector collector = new TopKnnCollector(maxResultsSize, Integer.MAX_VALUE);
    final DocIdSetIterator iterator = scorer.iterator();
    assert (iterator.cost() > maxResultsSize);
    int docId;
    while ((docId = iterator.nextDoc()) != DocIdSetIterator.NO_MORE_DOCS) {
        collector.collect(docId, scorer.score());
    }
    return collector.topDocs();
}
Possible Issue

When createQueryRequest.getContext() is absent, maxResultsSize defaults to MAX_RESULTS_RADIAL_RESCORING (10000). However, the actual index setting max_result_window might be lower (e.g., 100). This mismatch means the rescore layer may retain up to 10000 results when the index setting only allows 100, potentially causing downstream issues or inconsistent behavior when the collector or other components enforce the index-level limit.

    // Honor the index-level max_result_window setting to cap the number of results retained
    // after rescoring. Falls back to MAX_RESULTS_RADIAL_RESCORING if context is unavailable.
    final int maxResultsSize;
    if (createQueryRequest.getContext().isPresent()) {
        maxResultsSize = createQueryRequest.getContext().get().getIndexSettings().getMaxResultWindow();
    } else {
        maxResultsSize = MAX_RESULTS_RADIAL_RESCORING;
    }
    return new RescoreRadialSearchQuery(
        innerQuery,
        fieldName,
        vector,
        radius,
        createQueryRequest.isMemoryOptimizedSearchEnabled(),
        maxResultsSize
    );
}

@github-actions

github-actions Bot commented May 20, 2026

Copy link
Copy Markdown

PR Code Suggestions ✨

Latest suggestions up to 3146536

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Add thread-safe singleton initialization

The singleton pattern with a mutable static field is not thread-safe and can lead to
race conditions during initialization. If multiple threads call the constructor
before initialize() completes, they may see a null value. Consider using a
thread-safe initialization pattern or making the field volatile.

src/main/java/org/opensearch/knn/index/query/RescoreRadialSearchQuery.java [52]

-private static ExactSearcher EXACT_SEARCHER_SINGLETON;
+private static volatile ExactSearcher EXACT_SEARCHER_SINGLETON;
 ...
 Objects.requireNonNull(EXACT_SEARCHER_SINGLETON, "Exact searcher was not initialized.");
Suggestion importance[1-10]: 7

__

Why: The singleton field EXACT_SEARCHER_SINGLETON should be volatile to ensure thread-safe visibility across threads. Without volatile, threads may see stale values during initialization, though the requireNonNull check at line 109 provides some protection.

Medium
General
Remove unreliable cost assertion

The assertion iterator.cost() > maxResultsSize may fail if the iterator's cost
estimate is inaccurate or if the iterator was already partially consumed. Remove the
assertion or replace it with a runtime check that handles the case gracefully.

src/main/java/org/opensearch/knn/index/query/RescoreRadialSearchQuery.java [295-304]

 private TopDocs collectTopDocs(final Scorer scorer) throws IOException {
     final TopKnnCollector collector = new TopKnnCollector(maxResultsSize, Integer.MAX_VALUE);
     final DocIdSetIterator iterator = scorer.iterator();
-    assert (iterator.cost() > maxResultsSize);
     int docId;
     while ((docId = iterator.nextDoc()) != DocIdSetIterator.NO_MORE_DOCS) {
         collector.collect(docId, scorer.score());
     }
     return collector.topDocs();
 }
Suggestion importance[1-10]: 5

__

Why: The assertion at line 298 assumes iterator.cost() > maxResultsSize, but this method is only called when that condition is true (line 245). However, assertions can be disabled at runtime, and the cost estimate may be inaccurate. Removing the assertion makes the code more robust without changing functionality.

Low

Previous suggestions

Suggestions up to commit b43bbc5
CategorySuggestion                                                                                                                                    Impact
General
Remove unreliable assertion on iterator cost

The assertion assert (iterator.cost() > maxResultsSize) may not hold true if the
iterator's cost estimate is inaccurate or if documents are filtered during
iteration. This could cause the assertion to fail in production when assertions are
enabled, leading to unexpected crashes.

src/main/java/org/opensearch/knn/index/query/RescoreRadialSearchQuery.java [295-304]

 private TopDocs collectTopDocs(final Scorer scorer) throws IOException {
     final TopKnnCollector collector = new TopKnnCollector(maxResultsSize, Integer.MAX_VALUE);
     final DocIdSetIterator iterator = scorer.iterator();
-    assert (iterator.cost() > maxResultsSize);
     int docId;
     while ((docId = iterator.nextDoc()) != DocIdSetIterator.NO_MORE_DOCS) {
         collector.collect(docId, scorer.score());
     }
     return collector.topDocs();
 }
Suggestion importance[1-10]: 5

__

Why: The assertion at line 298 (assert (iterator.cost() > maxResultsSize)) could fail if the cost estimate is inaccurate. While assertions are typically disabled in production, removing this assertion improves robustness. However, the assertion serves as documentation of the expected precondition, so removing it has a moderate impact.

Low
Add error handling for collectTopDocs

The collectTopDocs method consumes the innerScorer iterator, but if it throws an
exception or returns fewer docs than expected, the original matchedDocs iterator is
already exhausted and cannot be reused. This could lead to data loss or incorrect
results in edge cases.

src/main/java/org/opensearch/knn/index/query/RescoreRadialSearchQuery.java [245-252]

+final DocIdSetIterator docsToRescore;
+final long numDocsToRescore;
 if (matchedDocs.cost() > maxResultsSize) {
-    final TopDocs topCandidates = collectTopDocs(innerScorer);
-    docsToRescore = new TopDocsDISI(topCandidates);
-    numDocsToRescore = topCandidates.scoreDocs.length;
+    try {
+        final TopDocs topCandidates = collectTopDocs(innerScorer);
+        if (topCandidates.scoreDocs.length == 0) {
+            return KNNScorer.emptyScorer();
+        }
+        docsToRescore = new TopDocsDISI(topCandidates);
+        numDocsToRescore = topCandidates.scoreDocs.length;
+    } catch (IOException e) {
+        throw new RuntimeException("Failed to collect top candidates for rescoring", e);
+    }
 } else {
     docsToRescore = matchedDocs;
     numDocsToRescore = matchedDocs.cost();
 }
Suggestion importance[1-10]: 3

__

Why: The suggestion adds error handling for collectTopDocs, but the concern about iterator exhaustion is not valid since collectTopDocs is only called when the cost exceeds maxResultsSize, and the iterator is consumed intentionally. The empty check is redundant as ExactSearcher handles empty results. The try-catch adds minimal value since IOException would propagate anyway.

Low
Suggestions up to commit eedbae2
CategorySuggestion                                                                                                                                    Impact
Possible issue
Add thread-safety to singleton initialization

The static singleton EXACT_SEARCHER_SINGLETON is not thread-safe and can cause race
conditions during initialization. If multiple threads call initialize()
concurrently, or if one thread reads while another writes, the singleton may be in
an inconsistent state. Use volatile keyword or proper synchronization to ensure
thread-safe initialization and visibility across threads.

src/main/java/org/opensearch/knn/index/query/RescoreRadialSearchQuery.java [57-109]

-private static ExactSearcher EXACT_SEARCHER_SINGLETON;
+private static volatile ExactSearcher EXACT_SEARCHER_SINGLETON;
 ...
 @VisibleForTesting
-public static void initialize(final ExactSearcher exactSearcher) {
+public static synchronized void initialize(final ExactSearcher exactSearcher) {
     EXACT_SEARCHER_SINGLETON = exactSearcher;
 }
Suggestion importance[1-10]: 8

__

Why: The static singleton EXACT_SEARCHER_SINGLETON lacks thread-safety mechanisms, which could lead to race conditions during concurrent initialization or access. Adding volatile and synchronized ensures proper visibility and prevents inconsistent state.

Medium
Prevent NPE from uninitialized singleton

If EXACT_SEARCHER_SINGLETON is null (not initialized), this will throw a
NullPointerException. Add a null check before using the singleton to prevent runtime
failures and provide a meaningful error message if the singleton hasn't been
initialized properly.

src/main/java/org/opensearch/knn/index/query/RescoreRadialSearchQuery.java [250]

+if (EXACT_SEARCHER_SINGLETON == null) {
+    throw new IllegalStateException("ExactSearcher singleton not initialized");
+}
 final TopDocs rescored = EXACT_SEARCHER_SINGLETON.searchLeaf(context, exactSearcherContext);
Suggestion importance[1-10]: 8

__

Why: If EXACT_SEARCHER_SINGLETON is not initialized before use, a NullPointerException will occur at runtime. Adding a null check with a meaningful error message prevents this critical failure and improves debuggability.

Medium
General
Fix incorrect TotalHits relation reporting

The method caps collection at MAX_RESULTS_RADIAL_RESCORING but reports
TotalHits.Relation.EQUAL_TO, which is incorrect when more documents exist. If the
iterator has more docs than the cap, the relation should be GREATER_THAN_OR_EQUAL_TO
to accurately reflect that results were truncated.

src/main/java/org/opensearch/knn/index/query/RescoreRadialSearchQuery.java [278-290]

 private TopDocs collectTopDocs(final Scorer scorer) throws IOException {
-    // Preallocate array to prevent array doubling
     final int maxCapacity = (int) Math.min(scorer.iterator().cost(), MAX_RESULTS_RADIAL_RESCORING);
     final List<ScoreDoc> scoreDocs = new ArrayList<>(maxCapacity);
 
-    // Pull score docs and create TopDocs
     final DocIdSetIterator iterator = scorer.iterator();
     int docId;
     while ((docId = iterator.nextDoc()) != DocIdSetIterator.NO_MORE_DOCS && scoreDocs.size() < MAX_RESULTS_RADIAL_RESCORING) {
         scoreDocs.add(new ScoreDoc(docId, scorer.score()));
     }
-    return new TopDocs(new TotalHits(scoreDocs.size(), TotalHits.Relation.EQUAL_TO), scoreDocs.toArray(new ScoreDoc[0]));
+    TotalHits.Relation relation = iterator.nextDoc() == DocIdSetIterator.NO_MORE_DOCS 
+        ? TotalHits.Relation.EQUAL_TO 
+        : TotalHits.Relation.GREATER_THAN_OR_EQUAL_TO;
+    return new TopDocs(new TotalHits(scoreDocs.size(), relation), scoreDocs.toArray(new ScoreDoc[0]));
 }
Suggestion importance[1-10]: 7

__

Why: The method reports TotalHits.Relation.EQUAL_TO even when results are capped at MAX_RESULTS_RADIAL_RESCORING, which is misleading when more documents exist. Using GREATER_THAN_OR_EQUAL_TO when truncation occurs provides accurate metadata about result completeness.

Medium
Suggestions up to commit bccd36f
CategorySuggestion                                                                                                                                    Impact
Possible issue
Prevent null pointer dereference

The code dereferences EXACT_SEARCHER_SINGLETON without checking if it's null. If
initialize() hasn't been called yet or fails, this will throw a
NullPointerException. Add a null check before usage and throw a descriptive
exception if uninitialized.

src/main/java/org/opensearch/knn/index/query/RescoreRadialSearchQuery.java [250]

+if (EXACT_SEARCHER_SINGLETON == null) {
+    throw new IllegalStateException("RescoreRadialSearchQuery not initialized");
+}
 final TopDocs rescored = EXACT_SEARCHER_SINGLETON.searchLeaf(context, exactSearcherContext);
Suggestion importance[1-10]: 8

__

Why: The code dereferences EXACT_SEARCHER_SINGLETON without null checking. If initialize() hasn't been called, this causes a NullPointerException. Adding a null check with a descriptive exception prevents runtime failures.

Medium
Add thread-safety to singleton field

The static singleton EXACT_SEARCHER_SINGLETON is not thread-safe and can be accessed
before initialization. This creates a race condition where multiple threads might
call initialize() concurrently, or scorerSupplier() might access a null singleton.
Add synchronization or use a volatile field with null checks.

src/main/java/org/opensearch/knn/index/query/RescoreRadialSearchQuery.java [57]

-private static ExactSearcher EXACT_SEARCHER_SINGLETON;
+private static volatile ExactSearcher EXACT_SEARCHER_SINGLETON;
Suggestion importance[1-10]: 7

__

Why: The static singleton EXACT_SEARCHER_SINGLETON lacks thread-safety guarantees. Using volatile ensures visibility across threads and prevents potential race conditions during initialization or access.

Medium
General
Fix incorrect total hits relation

The method caps collection at MAX_RESULTS_RADIAL_RESCORING but doesn't track whether
more documents exist beyond this limit. This causes the TotalHits.Relation to always
be EQUAL_TO, which is incorrect when documents are truncated. Track if iteration
stopped early and set relation to GREATER_THAN_OR_EQUAL_TO when capped.

src/main/java/org/opensearch/knn/index/query/RescoreRadialSearchQuery.java [278-290]

 private TopDocs collectTopDocs(final Scorer scorer) throws IOException {
-    // Preallocate array to prevent array doubling
     final int maxCapacity = (int) Math.min(scorer.iterator().cost(), MAX_RESULTS_RADIAL_RESCORING);
     final List<ScoreDoc> scoreDocs = new ArrayList<>(maxCapacity);
-
-    // Pull score docs and create TopDocs
     final DocIdSetIterator iterator = scorer.iterator();
     int docId;
     while ((docId = iterator.nextDoc()) != DocIdSetIterator.NO_MORE_DOCS && scoreDocs.size() < MAX_RESULTS_RADIAL_RESCORING) {
         scoreDocs.add(new ScoreDoc(docId, scorer.score()));
     }
-    return new TopDocs(new TotalHits(scoreDocs.size(), TotalHits.Relation.EQUAL_TO), scoreDocs.toArray(new ScoreDoc[0]));
+    TotalHits.Relation relation = (iterator.nextDoc() != DocIdSetIterator.NO_MORE_DOCS) 
+        ? TotalHits.Relation.GREATER_THAN_OR_EQUAL_TO 
+        : TotalHits.Relation.EQUAL_TO;
+    return new TopDocs(new TotalHits(scoreDocs.size(), relation), scoreDocs.toArray(new ScoreDoc[0]));
 }
Suggestion importance[1-10]: 4

__

Why: The suggestion correctly identifies that TotalHits.Relation should be GREATER_THAN_OR_EQUAL_TO when results are capped. However, the proposed fix has a flaw: calling iterator.nextDoc() after the loop consumes the iterator state, which may cause issues. A better approach would check if the loop terminated due to the cap rather than exhaustion.

Low
Suggestions up to commit 7cab05e
CategorySuggestion                                                                                                                                    Impact
General
Simplify rewrite logic for clarity

The rewrite method creates a new RescoreRadialSearchQuery with rewritten but the
condition checks rewritten != innerQuery. If innerQuery.rewrite() returns a
different instance that is semantically equivalent, this creates unnecessary new
query objects. Consider checking if rewriting actually changed the query
semantically.

src/main/java/org/opensearch/knn/index/query/RescoreRadialSearchQuery.java [123-130]

 public Query rewrite(final IndexSearcher indexSearcher) throws IOException {
     final Query rewritten = innerQuery.rewrite(indexSearcher);
-    if (rewritten != innerQuery) {
-        return new RescoreRadialSearchQuery(rewritten, field, queryVector, radius, memoryOptimizedSearchEnabled);
-    } else {
+    if (rewritten == innerQuery) {
         return this;
     }
+    return new RescoreRadialSearchQuery(rewritten, field, queryVector, radius, memoryOptimizedSearchEnabled);
 }
Suggestion importance[1-10]: 4

__

Why: The suggestion inverts the condition for slightly better readability by checking equality first. However, both versions are functionally equivalent and correct. The improvement is minimal and purely stylistic.

Low
Handle inaccurate cost estimates safely

The scorer.iterator().cost() may return an inaccurate estimate, potentially causing
unnecessary memory allocation or array resizing. Consider using a more conservative
initial capacity or handling the case where cost() returns a very large or
inaccurate value.

src/main/java/org/opensearch/knn/index/query/RescoreRadialSearchQuery.java [275-276]

-final int initialCapacity = (int) Math.min(scorer.iterator().cost(), MAX_RESULTS_RADIAL_RESCORING);
+final long estimatedCost = scorer.iterator().cost();
+final int initialCapacity = (int) Math.min(Math.max(estimatedCost, 100), MAX_RESULTS_RADIAL_RESCORING);
 final List<ScoreDoc> scoreDocs = new ArrayList<>(initialCapacity);
Suggestion importance[1-10]: 3

__

Why: The suggestion adds a minimum capacity of 100, but this is an arbitrary optimization that doesn't address a real bug. The Math.min already caps the allocation at MAX_RESULTS_RADIAL_RESCORING, preventing excessive memory use. The Math.max with 100 is unnecessary since ArrayList handles small initial capacities efficiently.

Low
Suggestions up to commit a86e85c
CategorySuggestion                                                                                                                                    Impact
General
Remove redundant nested Math.min call

The nested Math.min calls are redundant since matchedDocs.cost() returns a long and
Integer.MAX_VALUE is already the maximum int value. The outer Math.min with
MAX_RESULTS_RADIAL_RESCORING is sufficient. Simplify to improve readability.

src/main/java/org/opensearch/knn/index/query/RescoreRadialSearchQuery.java [236]

-.maxResultWindow((int) Math.min(MAX_RESULTS_RADIAL_RESCORING, Math.min(matchedDocs.cost(), Integer.MAX_VALUE)))
+.maxResultWindow((int) Math.min(MAX_RESULTS_RADIAL_RESCORING, matchedDocs.cost()))
Suggestion importance[1-10]: 6

__

Why: The suggestion correctly identifies redundant nested Math.min calls. The inner Math.min(matchedDocs.cost(), Integer.MAX_VALUE) is unnecessary since the outer Math.min with MAX_RESULTS_RADIAL_RESCORING already handles the capping. Simplifying improves code readability.

Low
Initialize ArrayList with estimated capacity

The ArrayList grows dynamically without an initial capacity hint, which can cause
multiple reallocations when collecting large result sets. Consider initializing with
an estimated capacity based on scorer.iterator().cost() to reduce memory allocations
and improve performance.

src/main/java/org/opensearch/knn/index/query/RescoreRadialSearchQuery.java [273-281]

 private TopDocs collectTopDocs(final Scorer scorer) throws IOException {
-    final List<ScoreDoc> scoreDocs = new ArrayList<>();
+    final long estimatedSize = scorer.iterator().cost();
+    final int initialCapacity = (int) Math.min(estimatedSize, 10000);
+    final List<ScoreDoc> scoreDocs = new ArrayList<>(initialCapacity);
     final DocIdSetIterator iterator = scorer.iterator();
     int docId;
     while ((docId = iterator.nextDoc()) != DocIdSetIterator.NO_MORE_DOCS) {
         scoreDocs.add(new ScoreDoc(docId, scorer.score()));
     }
     return new TopDocs(new TotalHits(scoreDocs.size(), TotalHits.Relation.EQUAL_TO), scoreDocs.toArray(new ScoreDoc[0]));
 }
Suggestion importance[1-10]: 5

__

Why: The suggestion provides a valid performance optimization by initializing ArrayList with an estimated capacity based on scorer.iterator().cost(). This reduces memory reallocations when collecting large result sets, though the impact is moderate since the list grows efficiently anyway.

Low

@0ctopus13prime
0ctopus13prime force-pushed the rescore-radial-search-quantized-complete branch from 348d623 to 1760bb0 Compare May 21, 2026 17:35
@github-actions

Copy link
Copy Markdown

Persistent review updated to latest commit 1760bb0

@0ctopus13prime
0ctopus13prime force-pushed the rescore-radial-search-quantized-complete branch from 1760bb0 to a86e85c Compare May 21, 2026 17:48
@github-actions

Copy link
Copy Markdown

Persistent review updated to latest commit a86e85c

@0ctopus13prime
0ctopus13prime force-pushed the rescore-radial-search-quantized-complete branch from a86e85c to 7cab05e Compare May 21, 2026 17:56
@github-actions

Copy link
Copy Markdown

Persistent review updated to latest commit 7cab05e

@codecov

codecov Bot commented May 21, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 94.00000% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.56%. Comparing base (872daa9) to head (3146536).

Files with missing lines Patch % Lines
...arch/knn/index/query/RescoreRadialSearchQuery.java 93.02% 1 Missing and 2 partials ⚠️
Additional details and impacted files
@@                      Coverage Diff                       @@
##             rescore-radial-quantized    #3337      +/-   ##
==============================================================
+ Coverage                       83.52%   83.56%   +0.04%     
- Complexity                       4294     4297       +3     
==============================================================
  Files                             451      451              
  Lines                           15564    15611      +47     
  Branches                         2016     2022       +6     
==============================================================
+ Hits                            13000    13046      +46     
- Misses                           1774     1775       +1     
  Partials                          790      790              

☔ View full report in Codecov by Sentry.
📢 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.

Comment thread src/main/java/org/opensearch/knn/index/query/RescoreRadialSearchQuery.java Outdated
@0ctopus13prime
0ctopus13prime force-pushed the rescore-radial-search-quantized-complete branch from 7cab05e to bccd36f Compare May 22, 2026 00:23
@github-actions

Copy link
Copy Markdown

Persistent review updated to latest commit bccd36f

@0ctopus13prime
0ctopus13prime force-pushed the rescore-radial-search-quantized-complete branch from bccd36f to eedbae2 Compare May 22, 2026 17:36
@github-actions

Copy link
Copy Markdown

Persistent review updated to latest commit eedbae2

@Vikasht34

Copy link
Copy Markdown
Collaborator

Is this PR is ready for Review , I see in Draft? @0ctopus13prime

@0ctopus13prime

Copy link
Copy Markdown
Collaborator Author

@Vikasht34
Yeah, overall it's ready!

Comment thread src/main/java/org/opensearch/knn/index/query/RescoreRadialSearchQuery.java Outdated
Comment thread src/main/java/org/opensearch/knn/index/query/RescoreRadialSearchQuery.java Outdated
Comment thread src/main/java/org/opensearch/knn/index/query/RescoreRadialSearchQuery.java Outdated
@0ctopus13prime
0ctopus13prime force-pushed the rescore-radial-search-quantized-complete branch from eedbae2 to b43bbc5 Compare May 27, 2026 19:34
@github-actions

Copy link
Copy Markdown

Persistent review updated to latest commit b43bbc5

Signed-off-by: Dooyong Kim <kdooyong@amazon.com>
@0ctopus13prime
0ctopus13prime force-pushed the rescore-radial-search-quantized-complete branch from b43bbc5 to 3146536 Compare May 27, 2026 22:06
@github-actions

Copy link
Copy Markdown

Persistent review updated to latest commit 3146536

@Vikasht34 Vikasht34 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks Good !!

final TopDocs rescored = EXACT_SEARCHER_SINGLETON.searchLeaf(context, exactSearcherContext);

// 6. Return scorer over rescored results
return new KNNScorer(rescored, boost);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One of the potential optimizations here is to have exact searcher return a scorer rather than topdocs. Returning topDocs here is redundant because TopScoreDocCollector will do it again. Collecting topDocs forces to loop through the entire results unnecessarily. This breaks the lazy behavior of lucene and bypasses early termination and leap frogging if at all its needed.

The idea is simple here, wrap the bulkScoring logic in an iterator and return a scorer from exact search level removing the redundant loop for creating TopDocs and packaging it again in KNNScorer. I have been thinking through this idea and see where this scorer is beneficial apart from radial search because exactsearch does not need to have topk either

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree. We can directly pull and sort docs from min-heap without having to convert them to TopDocs since as you mentioned, TopScoreDocCollector will do it again anyway.
And I think the lazy behavior will only be matter when it was used as a sub query, like Conjunction(TermQuery, Radial) for example. And in most case I believe radial search will be used as a top level query.

I believe we should find a way to get the entire query tree then decide whether to return iterator wrapper having exact searcher in it. But for that, we probably need core's support.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can directly pull and sort docs from min-heap without having to convert them to TopDocs

We don't even need a minHeap its additional JVM heap use followed by GC cleanup. Just a scorer which tells how to compute should be enough. We just need to make sure that the DocIdSetIterator of the scorer iterates the docs in sequence. No sorting, No looping through

And in most case I believe radial search will be used as a top level query.

Correct, it will still avoid the extra minHeap and looping through all results to get TopDocs again to hold those in memory, I understand these are minor things but might matter in a high throughput environments

I believe we should find a way to get the entire query tree then decide whether to return iterator wrapper having exact searcher in it. But for that, we probably need core's support.

Not necessarily if we abide by the lucene scorer contract, traversing a query tree is not needed.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, I think it's better to have a quick sync on this

But I think believe we still need the min heap when the intermediate result size > 10k though to extract top-10k vectors. If the result from 1st phase < 10k, then yes, it's not needed and this PR already has it.

Also be able to look at the entire query tree is essential in the optimization, as in my view, if radial search is at the top level, then we should run exact search aggressively otherwise we should get benefit from lazy evaluation.

@shatejas shatejas left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good for now, we can take scorer optimization as followup

@0ctopus13prime
0ctopus13prime merged commit e981427 into opensearch-project:rescore-radial-quantized May 28, 2026
51 of 68 checks passed
0ctopus13prime added a commit that referenced this pull request May 29, 2026
* Rescoring after radial search on quantized index. [Task 1 - 4] (#3300)

* Bumped gradle to 9.4.1 and jacoco to 0.8.14 (#3308)

Signed-off-by: Andrew Klepchick <aklepchi@amazon.com>

* Use KNN1040ScalarQuantizedVectorsFormat for Faiss SQ flat format (#3302)

The Faiss SQ format was using Lucene's Lucene104ScalarQuantizedVectorsFormat
directly, which lacks the prefetch-enabled raw vector reader that
KNN1040ScalarQuantizedVectorsFormat provides. This meant exact search
rescoring was missing I/O prefetch during graph traversal.

Changes:
- Switch faissSqFlatFormat from Lucene104ScalarQuantizedVectorsFormat to
  KNN1040ScalarQuantizedVectorsFormat in Faiss1040ScalarQuantizedKnnVectorsFormat
- Add @VisibleForTesting getFlatVectorsReader() to
  Faiss1040ScalarQuantizedKnnVectorsReader to replace reflection in tests
- Add testGetRandomVectorScorer_returnsPrefetchableScorer in
  KNN1040ScalarQuantizedVectorsFormatTests verifying the scorer is
  PrefetchableRandomVectorScorer via a real write/read cycle
- Replace reflection with getter in
  Faiss1040ScalarQuantizedKnnVectorsFormatTests.testFieldsReader_thenWrapsFlatReaderWithPrefetchSupport

Signed-off-by: Vijayan Balasubramanian <balasvij@amazon.com>

* Allow minScore, maxDistance for 32x SQ index.

Signed-off-by: Dooyong Kim <kdooyong@amazon.com>

Pass compression and quantization config to RNN query builder.

Signed-off-by: Dooyong Kim <kdooyong@amazon.com>

Added RescoreRadialSearchQuery.

Signed-off-by: Dooyong Kim <kdooyong@amazon.com>

Wiring `RescoreRadialSearchQuery` wrapper in `RNNQueryFactory`

Signed-off-by: Dooyong Kim <kdooyong@amazon.com>

---------

Signed-off-by: Andrew Klepchick <aklepchi@amazon.com>
Signed-off-by: Vijayan Balasubramanian <balasvij@amazon.com>
Signed-off-by: Dooyong Kim <kdooyong@amazon.com>
Co-authored-by: Andrew Klepchick <aklepchi@amazon.com>
Co-authored-by: Vijayan Balasubramanian <balasvij@amazon.com>

* Rescore radial search quantized complete (#3337)

* Added exact search logic after radial.

Signed-off-by: Dooyong Kim <kdooyong@amazon.com>

* Adding 2nd rescoring after radial search on quantized index.

Signed-off-by: Dooyong Kim <kdooyong@amazon.com>

---------

Signed-off-by: Dooyong Kim <kdooyong@amazon.com>

* Update changelog

Signed-off-by: Dooyong Kim <kdooyong@amazon.com>

---------

Signed-off-by: Andrew Klepchick <aklepchi@amazon.com>
Signed-off-by: Vijayan Balasubramanian <balasvij@amazon.com>
Signed-off-by: Dooyong Kim <kdooyong@amazon.com>
Co-authored-by: Andrew Klepchick <aklepchi@amazon.com>
Co-authored-by: Vijayan Balasubramanian <balasvij@amazon.com>
@0ctopus13prime
0ctopus13prime deleted the rescore-radial-search-quantized-complete branch June 11, 2026 21:53
naveentatikonda pushed a commit that referenced this pull request Jun 18, 2026
* Enhance unit test coverage for 32x defaults

Signed-off-by: Kunal Kotwani <kkotwani@amazon.com>

* Add BwC test coverage (#3329)

Signed-off-by: Kunal Kotwani <kkotwani@amazon.com>

* Add base64 binary encoding as default format for knn_vector docvalue_fields (#3324)

Signed-off-by: Navneet Verma <navneev@amazon.com>

* Add issues write permission to untriaged label workflow (#3332)

Signed-off-by: shreyah963 <shreyab963@gmail.com>

* Fix score to radius conversion for IP with faiss (#3336)

Signed-off-by: Kunal Kotwani <kkotwani@amazon.com>
Co-authored-by: Tejas Shah <shatejas@amazon.com>

* Add ci.opensearch.org maven2 mirror to avoid throttling (#3345)

Signed-off-by: Sayali Gaikawad <gaiksaya@amazon.com>

* [AUTO] Add release notes for 3.7.0 (#3342)

Signed-off-by: opensearch-ci-bot <opensearch-infra@amazon.com>

* Fix derived source for mixed-case vector fields (#3313)

* Fix derived source for mixed-case vector fields

Signed-off-by: Wonjae Lee <wonjae.lee@dremio.com>

* Add BWC coverage for derived source field casing

Signed-off-by: Wonjae Lee <wonjae.lee@dremio.com>

* Add changelog entry for mixed-case derived source fix

Signed-off-by: Wonjae Lee <wonjae.lee@dremio.com>

* Handle case-insensitive conflicts by preferring vector field

Signed-off-by: Wonjae Lee <wonjae.lee@dremio.com>

* Avoid stream wrappers for derived field lookup

Signed-off-by: Wonjae Lee <wonjae.lee@dremio.com>

* Handle ambiguous case-insensitive matches without vector hints

Signed-off-by: Wonjae Lee <wonjae.lee@dremio.com>

* Update src/main/java/org/opensearch/knn/index/codec/KNN10010Codec/KNN10010DerivedSourceStoredFieldsFormat.java

Co-authored-by: Tejas Shah <shatejas@amazon.com>
Signed-off-by: Wonjae Lee <38933452+leewjae@users.noreply.github.com>

* Apply spotless formatting for derived source field resolution

Signed-off-by: Wonjae Lee <wonjae.lee@dremio.com>

* Avoid guessing when case-insensitive matches lack vector hints

Signed-off-by: Wonjae Lee <wonjae.lee@dremio.com>

* Simplify case-insensitive derived field matching

Signed-off-by: Wonjae Lee <wonjae.lee@dremio.com>

* Trigger CI rerun for BWC investigation

Signed-off-by: Wonjae Lee <wonjae.lee@dremio.com>

* Add native engine field info coverage

Signed-off-by: Wonjae Lee <wonjae.lee@dremio.com>

---------

Signed-off-by: Wonjae Lee <wonjae.lee@dremio.com>
Signed-off-by: Wonjae Lee <38933452+leewjae@users.noreply.github.com>
Signed-off-by: Tejas Shah <shatejas@amazon.com>
Co-authored-by: Tejas Shah <shatejas@amazon.com>
Co-authored-by: Navneet Verma <navneev@amazon.com>

* Fixes RescoreParser to pass the rescore flag (#3343)

* Fixes RescoreParser to pass the rescore flag

For multinode or coordinator-data node setup, rescore set to false is
not passed through streams. This causes rescoring to execute even when
its not disabled explicitly by user

Signed-off-by: Tejas Shah <shatejas@amazon.com>

* Updates Changelogs, improves code cov

Signed-off-by: Tejas Shah <shatejas@amazon.com>

* Makes the coordinator port dynamic

Signed-off-by: Tejas Shah <shatejas@amazon.com>

* Adds BWC test for mode and compression

Signed-off-by: Tejas Shah <shatejas@amazon.com>

* Does not create compressed indices before 2.18

Signed-off-by: Tejas Shah <shatejas@amazon.com>

* Fixes bwc

Signed-off-by: Tejas Shah <shatejas@amazon.com>

---------

Signed-off-by: Tejas Shah <shatejas@amazon.com>

* Merge rescore-radial-quantized feature branch to main (#3347)

* Rescoring after radial search on quantized index. [Task 1 - 4] (#3300)

* Bumped gradle to 9.4.1 and jacoco to 0.8.14 (#3308)

Signed-off-by: Andrew Klepchick <aklepchi@amazon.com>

* Use KNN1040ScalarQuantizedVectorsFormat for Faiss SQ flat format (#3302)

The Faiss SQ format was using Lucene's Lucene104ScalarQuantizedVectorsFormat
directly, which lacks the prefetch-enabled raw vector reader that
KNN1040ScalarQuantizedVectorsFormat provides. This meant exact search
rescoring was missing I/O prefetch during graph traversal.

Changes:
- Switch faissSqFlatFormat from Lucene104ScalarQuantizedVectorsFormat to
  KNN1040ScalarQuantizedVectorsFormat in Faiss1040ScalarQuantizedKnnVectorsFormat
- Add @VisibleForTesting getFlatVectorsReader() to
  Faiss1040ScalarQuantizedKnnVectorsReader to replace reflection in tests
- Add testGetRandomVectorScorer_returnsPrefetchableScorer in
  KNN1040ScalarQuantizedVectorsFormatTests verifying the scorer is
  PrefetchableRandomVectorScorer via a real write/read cycle
- Replace reflection with getter in
  Faiss1040ScalarQuantizedKnnVectorsFormatTests.testFieldsReader_thenWrapsFlatReaderWithPrefetchSupport

Signed-off-by: Vijayan Balasubramanian <balasvij@amazon.com>

* Allow minScore, maxDistance for 32x SQ index.

Signed-off-by: Dooyong Kim <kdooyong@amazon.com>

Pass compression and quantization config to RNN query builder.

Signed-off-by: Dooyong Kim <kdooyong@amazon.com>

Added RescoreRadialSearchQuery.

Signed-off-by: Dooyong Kim <kdooyong@amazon.com>

Wiring `RescoreRadialSearchQuery` wrapper in `RNNQueryFactory`

Signed-off-by: Dooyong Kim <kdooyong@amazon.com>

---------

Signed-off-by: Andrew Klepchick <aklepchi@amazon.com>
Signed-off-by: Vijayan Balasubramanian <balasvij@amazon.com>
Signed-off-by: Dooyong Kim <kdooyong@amazon.com>
Co-authored-by: Andrew Klepchick <aklepchi@amazon.com>
Co-authored-by: Vijayan Balasubramanian <balasvij@amazon.com>

* Rescore radial search quantized complete (#3337)

* Added exact search logic after radial.

Signed-off-by: Dooyong Kim <kdooyong@amazon.com>

* Adding 2nd rescoring after radial search on quantized index.

Signed-off-by: Dooyong Kim <kdooyong@amazon.com>

---------

Signed-off-by: Dooyong Kim <kdooyong@amazon.com>

* Update changelog

Signed-off-by: Dooyong Kim <kdooyong@amazon.com>

---------

Signed-off-by: Andrew Klepchick <aklepchi@amazon.com>
Signed-off-by: Vijayan Balasubramanian <balasvij@amazon.com>
Signed-off-by: Dooyong Kim <kdooyong@amazon.com>
Co-authored-by: Andrew Klepchick <aklepchi@amazon.com>
Co-authored-by: Vijayan Balasubramanian <balasvij@amazon.com>

* Add support for binary and byte field support in doc_values (#3340)

Signed-off-by: Navneet Verma <navneev@amazon.com>

* Pin GitHub Actions to commit SHAs (#3339)

Signed-off-by: Divya Madala <divyaasm@amazon.com>
Co-authored-by: Tejas Shah <shatejas@amazon.com>

* Turn off ACORN for MOS (#3346)

Signed-off-by: Andrew Klepchick <aklepchi@amazon.com>

* Add base64 encoded vector indexing support for knn_vector fields (#3350)

Vectors can now be indexed as base64-encoded strings in addition to JSON
arrays. Float vectors use little-endian byte encoding (symmetric with
the doc_values binary output format), while byte/binary vectors use raw
byte encoding. This enables efficient bulk ingestion pipelines that
avoid JSON array serialization overhead.

Signed-off-by: Navneet Verma <navneev@amazon.com>

* Made MemoryOptimizedSearchWarmup skip MemoryOptimizedSearchOldIndicesNotSupportedException. (#3344)

Signed-off-by: Dooyong Kim <kdooyong@amazon.com>
Signed-off-by: Doo Yong Kim <kdooyong@amazon.com>

* Integrated proper ef_search functionality into MOS and Lucene with oversample_factor (#3331)

* Check to see if Lucene's search budget has exhausted when deciding to exact search (#3354)

* Update opensearch-build workflow references from commit SHA to main (#3363)

Signed-off-by: Divya Madala <divyaasm@amazon.com>

* Pinned the commit for tj-actions/changed-files for version v47.0.0 (#3367)

Signed-off-by: Navneet Verma <navneev@amazon.com>

---------

Signed-off-by: Kunal Kotwani <kkotwani@amazon.com>
Signed-off-by: Navneet Verma <navneev@amazon.com>
Signed-off-by: shreyah963 <shreyab963@gmail.com>
Signed-off-by: Sayali Gaikawad <gaiksaya@amazon.com>
Signed-off-by: opensearch-ci-bot <opensearch-infra@amazon.com>
Signed-off-by: Wonjae Lee <wonjae.lee@dremio.com>
Signed-off-by: Wonjae Lee <38933452+leewjae@users.noreply.github.com>
Signed-off-by: Tejas Shah <shatejas@amazon.com>
Signed-off-by: Andrew Klepchick <aklepchi@amazon.com>
Signed-off-by: Vijayan Balasubramanian <balasvij@amazon.com>
Signed-off-by: Dooyong Kim <kdooyong@amazon.com>
Signed-off-by: Divya Madala <divyaasm@amazon.com>
Signed-off-by: Doo Yong Kim <kdooyong@amazon.com>
Co-authored-by: Navneet Verma <navneev@amazon.com>
Co-authored-by: Shreya Bhatta <shreyab963@gmail.com>
Co-authored-by: Tejas Shah <shatejas@amazon.com>
Co-authored-by: Sayali Gaikawad <gaiksaya@amazon.com>
Co-authored-by: opensearch-ci <83309141+opensearch-ci-bot@users.noreply.github.com>
Co-authored-by: Wonjae Lee <38933452+leewjae@users.noreply.github.com>
Co-authored-by: Doo Yong Kim <kdooyong@amazon.com>
Co-authored-by: Andrew Klepchick <aklepchi@amazon.com>
Co-authored-by: Vijayan Balasubramanian <balasvij@amazon.com>
Co-authored-by: Divya Madala <113469545+Divyaasm@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants