diff --git a/CHANGELOG.md b/CHANGELOG.md index 279c912c05..d1b9ea68b0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,6 +17,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), * Re-enable flaky BWC test testKNNWarmupCustomLegacyFieldMapping and restore legacy index settings in BWC test fixtures [#2415](https://github.com/opensearch-project/k-NN/issues/2415) ### Bug Fixes +* Return a non-matching explanation from `KNNWeight.explain()` for documents that are not nearest neighbors [#3480](https://github.com/opensearch-project/k-NN/pull/3480) * Preserve raw non-XContent `_source` fields when derived source is enabled [#3402](https://github.com/opensearch-project/k-NN/pull/3402) * Fix dimension-based oversampling not applying for 32x compression [#3455](https://github.com/opensearch-project/k-NN/pull/3455) * Fix NPE in nested kNN search when index contains documents without nested object [#3368](https://github.com/opensearch-project/k-NN/pull/3368) diff --git a/src/main/java/org/opensearch/knn/index/query/KNNWeight.java b/src/main/java/org/opensearch/knn/index/query/KNNWeight.java index 675f19211f..0207a4fae6 100644 --- a/src/main/java/org/opensearch/knn/index/query/KNNWeight.java +++ b/src/main/java/org/opensearch/knn/index/query/KNNWeight.java @@ -133,7 +133,13 @@ public Explanation explain(LeafReaderContext context, int doc, float score) { // calculate score only when its 0 as for disk-based search, // score will be passed from the caller and there is no need to re-compute the score if (score == 0) { - score = getKnnScore(knnScorer, doc); + // explain() can be called for a document this query does not match, for instance when the knn clause is + // one of several should clauses. Reporting a match for such a document breaks the contract that + // Weight.explain() and the scorer of the same Weight have to agree on which documents match. + if (knnScorer.iterator().advance(doc) != doc) { + return Explanation.noMatch("the document is not a nearest neighbor result for the field [" + knnQuery.getField() + "]"); + } + score = knnScorer.score(); } } catch (IOException e) { throw new RuntimeException(String.format("Error while explaining KNN score for doc [%d], score [%f]", doc, score), e); @@ -273,10 +279,6 @@ private Scorer getOrCreateKnnScorer(LeafReaderContext context) throws IOExceptio return scorer; } - private float getKnnScore(Scorer knnScorer, int doc) throws IOException { - return (knnScorer.iterator().advance(doc) == doc) ? knnScorer.score() : 0; - } - @Override public ScorerSupplier scorerSupplier(LeafReaderContext context) { return new ScorerSupplier() { diff --git a/src/test/java/org/opensearch/knn/index/query/ExplainTests.java b/src/test/java/org/opensearch/knn/index/query/ExplainTests.java index 3eac94cbd5..d05a7ad324 100644 --- a/src/test/java/org/opensearch/knn/index/query/ExplainTests.java +++ b/src/test/java/org/opensearch/knn/index/query/ExplainTests.java @@ -37,6 +37,7 @@ import java.io.IOException; import java.util.ArrayList; +import java.util.Collections; import java.util.Comparator; import java.util.List; import java.util.Locale; @@ -376,6 +377,54 @@ public void testDefaultANNSearch() { assertTrue(Comparators.isInOrder(actualDocIds, Comparator.naturalOrder())); } + @SneakyThrows + public void testExplain_whenDocIsNotANearestNeighbor_thenNoMatch() { + // Given + int k = 3; + jniServiceMockedStatic.when( + () -> JNIService.queryIndex(anyLong(), eq(QUERY_VECTOR), eq(k), eq(HNSW_METHOD_PARAMETERS), any(), eq(null), anyInt(), any()) + ).thenReturn(getFilteredKNNQueryResults()); + + final int[] filterDocIds = new int[] { 0, 1, 2, 3, 4, 5 }; + final Map attributesMap = ImmutableMap.of( + KNN_ENGINE, + KNNEngine.FAISS.getName(), + SPACE_TYPE, + SpaceType.L2.getValue() + ); + + setupTest(filterDocIds, attributesMap); + + final KNNQuery query = KNNQuery.builder() + .field(FIELD_NAME) + .queryVector(QUERY_VECTOR) + .k(k) + .indexName(INDEX_NAME) + .filterQuery(FILTER_QUERY) + .methodParameters(HNSW_METHOD_PARAMETERS) + .vectorDataType(VectorDataType.FLOAT) + .explain(true) + .build(); + query.setExplain(true); + + final KNNWeight knnWeight = new DefaultKNNWeight(query, 1f, filterQueryWeight); + final KNNScorer knnScorer = (KNNScorer) knnWeight.scorer(leafReaderContext); + assertNotNull(knnScorer); + knnWeight.getKnnExplanation().addKnnScorer(leafReaderContext, knnScorer); + + // The query returns FILTERED_DOC_ID_TO_SCORES, so this document is not one of the nearest neighbors. Advancing + // the scorer to it lands on the next matching document instead. + final int docIdWithoutMatch = Collections.min(FILTERED_DOC_ID_TO_SCORES.keySet()) - 1; + + // When + final Explanation explanation = knnWeight.explain(leafReaderContext, docIdWithoutMatch); + + // Then + assertNotNull(explanation); + assertFalse(explanation.isMatch()); + assertTrue(explanation.getDescription().contains(FIELD_NAME)); + } + @SneakyThrows public void testANN_FilteredExactSearchAfterANN() { ExactSearcher mockedExactSearcher = mock(ExactSearcher.class);