Fix automatic tracking of collapse with docvalue_fields (#110103) (#1…

…10154) There were some optimizations that broke collapse fields automatically being added to `docvalue_fields` during the fetch phase. Consequently, users will get really weird errors like `unsupported_operation_exception`. This commit corrects the intended behavior of automatically including the collapse field in the docvalue_fields context during fetch if it isn't already included. closes: #96510
elastic · Jun 26, 2024 · d3efd51 · d3efd51
1 parent 25b5660
commit d3efd51
Show file tree

Hide file tree

Showing 4 changed files with 32 additions and 4 deletions.
diff --git a/docs/changelog/110103.yaml b/docs/changelog/110103.yaml
@@ -0,0 +1,5 @@
+pr: 110103
+summary: Fix automatic tracking of collapse with `docvalue_fields`
+area: Search
+type: bug
+issues: []
diff --git a/server/src/internalClusterTest/java/org/elasticsearch/search/CollapseSearchResultsIT.java b/server/src/internalClusterTest/java/org/elasticsearch/search/CollapseSearchResultsIT.java
@@ -39,4 +39,26 @@ public void testCollapse() {
             }
         );
     }
+
+    public void testCollapseWithDocValueFields() {
+        final String indexName = "test_collapse";
+        createIndex(indexName);
+        final String collapseField = "collapse_field";
+        final String otherField = "other_field";
+        assertAcked(indicesAdmin().preparePutMapping(indexName).setSource(collapseField, "type=keyword", otherField, "type=keyword"));
+        index(indexName, "id_1_0", Map.of(collapseField, "value1", otherField, "other_value1"));
+        index(indexName, "id_1_1", Map.of(collapseField, "value1", otherField, "other_value2"));
+        index(indexName, "id_2_0", Map.of(collapseField, "value2", otherField, "other_value3"));
+        refresh(indexName);
+
+        assertNoFailuresAndResponse(
+            prepareSearch(indexName).setQuery(new MatchAllQueryBuilder())
+                .addDocValueField(otherField)
+                .setCollapse(new CollapseBuilder(collapseField).setInnerHits(new InnerHitBuilder("ih").setSize(2))),
+            searchResponse -> {
+                assertEquals(collapseField, searchResponse.getHits().getCollapseField());
+                assertEquals(Set.of(new BytesRef("value1"), new BytesRef("value2")), Set.of(searchResponse.getHits().getCollapseValues()));
+            }
+        );
+    }
 }
diff --git a/server/src/main/java/org/elasticsearch/search/fetch/FetchContext.java b/server/src/main/java/org/elasticsearch/search/fetch/FetchContext.java
@@ -175,7 +175,7 @@ public FetchDocValuesContext docValuesContext() {
                     searchContext.getSearchExecutionContext(),
                     Collections.singletonList(new FieldAndFormat(name, null))
                 );
-            } else if (searchContext.docValuesContext().fields().stream().map(ff -> ff.field).anyMatch(name::equals) == false) {
+            } else if (searchContext.docValuesContext().fields().stream().map(ff -> ff.field).noneMatch(name::equals)) {
                 dvContext.fields().add(new FieldAndFormat(name, null));
             }
         }

diff --git a/server/src/main/java/org/elasticsearch/search/fetch/subphase/FetchDocValuesContext.java b/server/src/main/java/org/elasticsearch/search/fetch/subphase/FetchDocValuesContext.java
@@ -10,6 +10,7 @@
 import org.elasticsearch.index.IndexSettings;
 import org.elasticsearch.index.query.SearchExecutionContext;
 
+import java.util.ArrayList;
 import java.util.Collection;
 import java.util.LinkedHashMap;
 import java.util.List;
@@ -24,7 +25,7 @@
  */
 public class FetchDocValuesContext {
 
-    private final Collection<FieldAndFormat> fields;
+    private final List<FieldAndFormat> fields;
 
     /**
      * Create a new FetchDocValuesContext using the provided input list.
@@ -40,7 +41,7 @@ public FetchDocValuesContext(SearchExecutionContext searchExecutionContext, List
                 fieldToFormats.put(fieldName, new FieldAndFormat(fieldName, field.format, field.includeUnmapped));
             }
         }
-        this.fields = fieldToFormats.values();
+        this.fields = new ArrayList<>(fieldToFormats.values());
         int maxAllowedDocvalueFields = searchExecutionContext.getIndexSettings().getMaxDocvalueFields();
         if (fields.size() > maxAllowedDocvalueFields) {
             throw new IllegalArgumentException(
@@ -58,7 +59,7 @@ public FetchDocValuesContext(SearchExecutionContext searchExecutionContext, List
     /**
      * Returns the required docvalue fields.
      */
-    public Collection<FieldAndFormat> fields() {
+    public List<FieldAndFormat> fields() {
         return this.fields;
     }
 }