Improve FieldFetcher retrieval of fields (#66160)

Currently FieldFetcher stores all the FieldContexts that are later used to
retrieve the fields in a List. This has the disadvantage that the same field
path can be retrieved several times (e.g. if multiple patterns match the same
path or if similar paths are defined several times e.g. with different formats).
Currently the last value to be retrieved "wins" and gets returned. We might as
well de-duplicate the FieldContexts by using a map internally, keyed by the
field path that is going to be retrieved, to avoid more work later.

Author: Christoph Büscher

server/src/main/java/org/elasticsearch/search/fetch/subphase/FieldFetcher.java

@@ -34,7 +34,7 @@
 import java.util.ArrayList;
 import java.util.Collection;
 import java.util.HashMap;
-import java.util.HashSet;
+import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
@@ -48,9 +48,10 @@ public static FieldFetcher create(QueryShardContext context,
                                       SearchLookup searchLookup,
                                       Collection<FieldAndFormat> fieldAndFormats) {
 
-        List<FieldContext> fieldContexts = new ArrayList<>();
+        // Using a LinkedHashMap so fields are returned in the order requested.
+        // We won't formally guarantee this but but its good for readability of the response
+        Map<String, FieldContext> fieldContexts = new LinkedHashMap<>();
         List<String> unmappedFetchPattern = new ArrayList<>();
-        Set<String> mappedToExclude = new HashSet<>();
         boolean includeUnmapped = false;
 
         for (FieldAndFormat fieldAndFormat : fieldAndFormats) {
@@ -68,8 +69,7 @@ public static FieldFetcher create(QueryShardContext context,
                     continue;
                 }
                 ValueFetcher valueFetcher = ft.valueFetcher(context, format);
-                mappedToExclude.add(field);
-                fieldContexts.add(new FieldContext(field, valueFetcher));
+                fieldContexts.put(field, new FieldContext(field, valueFetcher));
             }
         }
         CharacterRunAutomaton unmappedFetchAutomaton = new CharacterRunAutomaton(Automata.makeEmpty());
@@ -78,29 +78,26 @@ public static FieldFetcher create(QueryShardContext context,
                 Regex.simpleMatchToAutomaton(unmappedFetchPattern.toArray(new String[unmappedFetchPattern.size()]))
             );
         }
-        return new FieldFetcher(fieldContexts, unmappedFetchAutomaton, mappedToExclude, includeUnmapped);
+        return new FieldFetcher(fieldContexts, unmappedFetchAutomaton, includeUnmapped);
     }
 
-    private final List<FieldContext> fieldContexts;
+    private final Map<String, FieldContext> fieldContexts;
     private final CharacterRunAutomaton unmappedFetchAutomaton;
-    private final Set<String> mappedToExclude;
     private final boolean includeUnmapped;
 
     private FieldFetcher(
-        List<FieldContext> fieldContexts,
+        Map<String, FieldContext> fieldContexts,
         CharacterRunAutomaton unmappedFetchAutomaton,
-        Set<String> mappedToExclude,
         boolean includeUnmapped
     ) {
         this.fieldContexts = fieldContexts;
         this.unmappedFetchAutomaton = unmappedFetchAutomaton;
-        this.mappedToExclude = mappedToExclude;
         this.includeUnmapped = includeUnmapped;
     }
 
     public Map<String, DocumentField> fetch(SourceLookup sourceLookup, Set<String> ignoredFields) throws IOException {
         Map<String, DocumentField> documentFields = new HashMap<>();
-        for (FieldContext context : fieldContexts) {
+        for (FieldContext context : fieldContexts.values()) {
             String field = context.fieldName;
             if (ignoredFields.contains(field)) {
                 continue;
@@ -141,7 +138,7 @@ private void collectUnmapped(Map<String, DocumentField> documentFields, Map<Stri
                 collectUnmappedList(documentFields, (List<?>) value, currentPath, currentState);
             } else {
                 // we have a leaf value
-                if (this.unmappedFetchAutomaton.isAccept(currentState) && this.mappedToExclude.contains(currentPath) == false) {
+                if (this.unmappedFetchAutomaton.isAccept(currentState) && this.fieldContexts.containsKey(currentPath) == false) {
                     if (value != null) {
                         DocumentField currentEntry = documentFields.get(currentPath);
                         if (currentEntry == null) {
@@ -170,7 +167,7 @@ private void collectUnmappedList(Map<String, DocumentField> documentFields, Iter
             } else if (value instanceof List) {
                 // weird case, but can happen for objects with "enabled" : "false"
                 collectUnmappedList(documentFields, (List<?>) value, parentPath, lastState);
-            } else if (this.unmappedFetchAutomaton.isAccept(lastState) && this.mappedToExclude.contains(parentPath) == false) {
+            } else if (this.unmappedFetchAutomaton.isAccept(lastState) && this.fieldContexts.containsKey(parentPath) == false) {
                 list.add(value);
             }
         }
@@ -192,7 +189,7 @@ private static int step(CharacterRunAutomaton automaton, String key, int state)
     }
 
     public void setNextReader(LeafReaderContext readerContext) {
-        for (FieldContext field : fieldContexts) {
+        for (FieldContext field : fieldContexts.values()) {
             field.valueFetcher.setNextReader(readerContext);
         }
     }

server/src/test/java/org/elasticsearch/search/fetch/subphase/FieldFetcherTests.java

@@ -36,6 +36,7 @@
 import org.elasticsearch.search.lookup.SourceLookup;
 
 import java.io.IOException;
+import java.util.ArrayList;
 import java.util.Collections;
 import java.util.List;
 import java.util.Map;
@@ -658,6 +659,32 @@ public void testUnmappedFieldsWildcard() throws IOException {
         assertThat(fields.get("unmapped_object.b").getValue(), equalTo("bar"));
     }
 
+    public void testLastFormatWins() throws IOException {
+        MapperService mapperService = createMapperService();
+
+        XContentBuilder source = XContentFactory.jsonBuilder().startObject()
+            .startArray("date_field")
+                .value("2011-11-11T11:11:11")
+                .value("2012-12-12T12:12:12")
+            .endArray()
+            .endObject();
+
+        List<FieldAndFormat> ff = new ArrayList<>();
+        ff.add(new FieldAndFormat("date_field", "year", false));
+        Map<String, DocumentField> fields = fetchFields(mapperService, source, ff, null);
+        assertThat(fields.size(), equalTo(1));
+        assertThat(fields.get("date_field").getValues().size(), equalTo(2));
+        assertThat(fields.get("date_field").getValues().get(0), equalTo("2011"));
+        assertThat(fields.get("date_field").getValues().get(1), equalTo("2012"));
+
+        ff.add(new FieldAndFormat("date_field", "hour", false));
+        fields = fetchFields(mapperService, source, ff, null);
+        assertThat(fields.size(), equalTo(1));
+        assertThat(fields.get("date_field").getValues().size(), equalTo(2));
+        assertThat(fields.get("date_field").getValues().get(0), equalTo("11"));
+        assertThat(fields.get("date_field").getValues().get(1), equalTo("12"));
+    }
+
     private List<FieldAndFormat> fieldAndFormatList(String name, String format, boolean includeUnmapped) {
         return Collections.singletonList(new FieldAndFormat(name, format, includeUnmapped));
     }