Fix negative scores returned from multi_match query with cross_fields (#13829) (#13983)

msfroh · web-flow · commit 4e95e695f757 · 2024-06-06T03:07:03.000Z
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -45,6 +45,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 - Fix get field mapping API returns 404 error in mixed cluster with multiple versions ([#13624](https://github.com/opensearch-project/OpenSearch/pull/13624))
 - Allow clearing `remote_store.compatibility_mode` setting ([#13646](https://github.com/opensearch-project/OpenSearch/pull/13646))
 - Painless: ensure type "UnmodifiableMap" for params ([#13885](https://github.com/opensearch-project/OpenSearch/pull/13885))
+- Don't return negative scores from `multi_match` query with `cross_fields` type  ([#13829](https://github.com/opensearch-project/OpenSearch/pull/13829))
 - Pass parent filter to inner hit query ([#13903](https://github.com/opensearch-project/OpenSearch/pull/13903))
 - Fix NPE on restore searchable snapshot ([#13911](https://github.com/opensearch-project/OpenSearch/pull/13911))
 
diff --git a/rest-api-spec/src/main/resources/rest-api-spec/test/search/50_multi_match.yml b/rest-api-spec/src/main/resources/rest-api-spec/test/search/50_multi_match.yml
@@ -0,0 +1,35 @@
+"Cross fields do not return negative scores":
+  - skip:
+      version: " - 2.99.99"
+      reason: "This fix is in 2.15. Until we do the BWC dance, we need to skip all pre-3.0, though."
+  - do:
+      index:
+        index: test
+        id: 1
+        body: { "color" : "orange red yellow" }
+  - do:
+      index:
+        index: test
+        id: 2
+        body: { "color": "orange red purple", "shape": "red square" }
+  - do:
+      index:
+        index: test
+        id: 3
+        body: { "color" : "orange red yellow purple" }
+  - do:
+      indices.refresh: { }
+  - do:
+      search:
+        index: test
+        body:
+          query:
+            multi_match:
+              query: "red"
+              type: "cross_fields"
+              fields: [ "color", "shape^100"]
+              tie_breaker: 0.1
+          explain: true
+  - match: { hits.total.value: 3 }
+  - match: { hits.hits.0._id: "2" }
+  - gt: { hits.hits.2._score: 0.0 }
diff --git a/server/src/main/java/org/apache/lucene/queries/BlendedTermQuery.java b/server/src/main/java/org/apache/lucene/queries/BlendedTermQuery.java
@@ -120,6 +120,7 @@ protected void blend(final TermStates[] contexts, int maxDoc, IndexReader reader
         }
         int max = 0;
         long minSumTTF = Long.MAX_VALUE;
+        int[] docCounts = new int[contexts.length];
         for (int i = 0; i < contexts.length; i++) {
             TermStates ctx = contexts[i];
             int df = ctx.docFreq();
@@ -133,6 +134,7 @@ protected void blend(final TermStates[] contexts, int maxDoc, IndexReader reader
                 // we need to find out the minimum sumTTF to adjust the statistics
                 // otherwise the statistics don't match
                 minSumTTF = Math.min(minSumTTF, reader.getSumTotalTermFreq(terms[i].field()));
+                docCounts[i] = reader.getDocCount(terms[i].field());
             }
         }
         if (maxDoc > minSumTTF) {
@@ -175,7 +177,11 @@ protected int compare(int i, int j) {
             if (prev > current) {
                 actualDf++;
             }
-            contexts[i] = ctx = adjustDF(reader.getContext(), ctx, Math.min(maxDoc, actualDf));
+            // Per field, we want to guarantee that the adjusted df does not exceed the number of docs with the field.
+            // That is, in the IDF formula (log(1 + (N - n + 0.5) / (n + 0.5))), we need to make sure that n (the
+            // adjusted df) is never bigger than N (the number of docs with the field).
+            int fieldMaxDoc = Math.min(maxDoc, docCounts[i]);
+            contexts[i] = ctx = adjustDF(reader.getContext(), ctx, Math.min(fieldMaxDoc, actualDf));
             prev = current;
             sumTTF += ctx.totalTermFreq();
         }
diff --git a/test/framework/src/main/java/org/opensearch/test/rest/yaml/section/Assertion.java b/test/framework/src/main/java/org/opensearch/test/rest/yaml/section/Assertion.java
@@ -37,6 +37,8 @@
 import java.io.IOException;
 import java.util.Map;
 
+import static org.junit.Assert.fail;
+
 /**
  * Base class for executable sections that hold assertions
  */
@@ -79,6 +81,41 @@ protected final Object getActualValue(ClientYamlTestExecutionContext executionCo
         return executionContext.response(field);
     }
 
+    static Object convertActualValue(Object actualValue, Object expectedValue) {
+        if (actualValue == null || expectedValue.getClass().isAssignableFrom(actualValue.getClass())) {
+            return actualValue;
+        }
+        if (actualValue instanceof Number && expectedValue instanceof Number) {
+            if (expectedValue instanceof Float) {
+                return Float.parseFloat(actualValue.toString());
+            } else if (expectedValue instanceof Double) {
+                return Double.parseDouble(actualValue.toString());
+            } else if (expectedValue instanceof Integer) {
+                return Integer.parseInt(actualValue.toString());
+            } else if (expectedValue instanceof Long) {
+                return Long.parseLong(actualValue.toString());
+            }
+        }
+        // Force a class cast exception here, so developers can flesh out the above logic as needed.
+        try {
+            expectedValue.getClass().cast(actualValue);
+        } catch (ClassCastException e) {
+            fail(
+                "Type mismatch: Expected value ("
+                    + expectedValue
+                    + ") has type "
+                    + expectedValue.getClass()
+                    + ". "
+                    + "Actual value ("
+                    + actualValue
+                    + ") has type "
+                    + actualValue.getClass()
+                    + "."
+            );
+        }
+        return actualValue;
+    }
+
     @Override
     public XContentLocation getLocation() {
         return location;
diff --git a/test/framework/src/main/java/org/opensearch/test/rest/yaml/section/GreaterThanAssertion.java b/test/framework/src/main/java/org/opensearch/test/rest/yaml/section/GreaterThanAssertion.java
@@ -71,6 +71,7 @@ public GreaterThanAssertion(XContentLocation location, String field, Object expe
     @Override
     protected void doAssert(Object actualValue, Object expectedValue) {
         logger.trace("assert that [{}] is greater than [{}] (field: [{}])", actualValue, expectedValue, getField());
+        actualValue = convertActualValue(actualValue, expectedValue);
         assertThat(
             "value of [" + getField() + "] is not comparable (got [" + safeClass(actualValue) + "])",
             actualValue,
diff --git a/test/framework/src/main/java/org/opensearch/test/rest/yaml/section/GreaterThanEqualToAssertion.java b/test/framework/src/main/java/org/opensearch/test/rest/yaml/section/GreaterThanEqualToAssertion.java
@@ -72,6 +72,7 @@ public GreaterThanEqualToAssertion(XContentLocation location, String field, Obje
     @Override
     protected void doAssert(Object actualValue, Object expectedValue) {
         logger.trace("assert that [{}] is greater than or equal to [{}] (field: [{}])", actualValue, expectedValue, getField());
+        actualValue = convertActualValue(actualValue, expectedValue);
         assertThat(
             "value of [" + getField() + "] is not comparable (got [" + safeClass(actualValue) + "])",
             actualValue,
diff --git a/test/framework/src/main/java/org/opensearch/test/rest/yaml/section/LessThanAssertion.java b/test/framework/src/main/java/org/opensearch/test/rest/yaml/section/LessThanAssertion.java
@@ -72,6 +72,7 @@ public LessThanAssertion(XContentLocation location, String field, Object expecte
     @Override
     protected void doAssert(Object actualValue, Object expectedValue) {
         logger.trace("assert that [{}] is less than [{}] (field: [{}])", actualValue, expectedValue, getField());
+        actualValue = convertActualValue(actualValue, expectedValue);
         assertThat(
             "value of [" + getField() + "] is not comparable (got [" + safeClass(actualValue) + "])",
             actualValue,
diff --git a/test/framework/src/main/java/org/opensearch/test/rest/yaml/section/LessThanOrEqualToAssertion.java b/test/framework/src/main/java/org/opensearch/test/rest/yaml/section/LessThanOrEqualToAssertion.java
@@ -72,6 +72,7 @@ public LessThanOrEqualToAssertion(XContentLocation location, String field, Objec
     @Override
     protected void doAssert(Object actualValue, Object expectedValue) {
         logger.trace("assert that [{}] is less than or equal to [{}] (field: [{}])", actualValue, expectedValue, getField());
+        actualValue = convertActualValue(actualValue, expectedValue);
         assertThat(
             "value of [" + getField() + "] is not comparable (got [" + safeClass(actualValue) + "])",
             actualValue,

Original file line number	Diff line number	Diff line change
`@@ -120,6 +120,7 @@ protected void blend(final TermStates[] contexts, int maxDoc, IndexReader reader`
`120`	`120`	`}`
`121`	`121`	`int max = 0;`
`122`	`122`	`long minSumTTF = Long.MAX_VALUE;`
	`123`	`+ int[] docCounts = new int[contexts.length];`
`123`	`124`	`for (int i = 0; i < contexts.length; i++) {`
`124`	`125`	`TermStates ctx = contexts[i];`
`125`	`126`	`int df = ctx.docFreq();`
`@@ -133,6 +134,7 @@ protected void blend(final TermStates[] contexts, int maxDoc, IndexReader reader`
`133`	`134`	`// we need to find out the minimum sumTTF to adjust the statistics`
`134`	`135`	`// otherwise the statistics don't match`
`135`	`136`	`minSumTTF = Math.min(minSumTTF, reader.getSumTotalTermFreq(terms[i].field()));`
	`137`	`+ docCounts[i] = reader.getDocCount(terms[i].field());`
`136`	`138`	`}`
`137`	`139`	`}`
`138`	`140`	`if (maxDoc > minSumTTF) {`
`@@ -175,7 +177,11 @@ protected int compare(int i, int j) {`
`175`	`177`	`if (prev > current) {`
`176`	`178`	`actualDf++;`
`177`	`179`	`}`
`178`		`- contexts[i] = ctx = adjustDF(reader.getContext(), ctx, Math.min(maxDoc, actualDf));`
	`180`	`+ // Per field, we want to guarantee that the adjusted df does not exceed the number of docs with the field.`
	`181`	`+ // That is, in the IDF formula (log(1 + (N - n + 0.5) / (n + 0.5))), we need to make sure that n (the`
	`182`	`+ // adjusted df) is never bigger than N (the number of docs with the field).`
	`183`	`+ int fieldMaxDoc = Math.min(maxDoc, docCounts[i]);`
	`184`	`+ contexts[i] = ctx = adjustDF(reader.getContext(), ctx, Math.min(fieldMaxDoc, actualDf));`
`179`	`185`	`prev = current;`
`180`	`186`	`sumTTF += ctx.totalTermFreq();`
`181`	`187`	`}`