From 9fe6f933b2f2fadd0fa65887b0ce1c99a7853ece Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christoph=20B=C3=BCscher?= Date: Mon, 14 Dec 2020 20:59:11 +0100 Subject: [PATCH] Improve FieldFetcher retrieval of fields (#66160) (#66284) 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. --- .../search/fetch/subphase/FieldFetcher.java | 27 +++++++++---------- .../fetch/subphase/FieldFetcherTests.java | 27 +++++++++++++++++++ 2 files changed, 39 insertions(+), 15 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/search/fetch/subphase/FieldFetcher.java b/server/src/main/java/org/elasticsearch/search/fetch/subphase/FieldFetcher.java index b9a68a555375..81124840ae79 100644 --- a/server/src/main/java/org/elasticsearch/search/fetch/subphase/FieldFetcher.java +++ b/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 fieldAndFormats) { - List 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 fieldContexts = new LinkedHashMap<>(); List unmappedFetchPattern = new ArrayList<>(); - Set 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 fieldContexts; + private final Map fieldContexts; private final CharacterRunAutomaton unmappedFetchAutomaton; - private final Set mappedToExclude; private final boolean includeUnmapped; private FieldFetcher( - List fieldContexts, + Map fieldContexts, CharacterRunAutomaton unmappedFetchAutomaton, - Set mappedToExclude, boolean includeUnmapped ) { this.fieldContexts = fieldContexts; this.unmappedFetchAutomaton = unmappedFetchAutomaton; - this.mappedToExclude = mappedToExclude; this.includeUnmapped = includeUnmapped; } public Map fetch(SourceLookup sourceLookup, Set ignoredFields) throws IOException { Map 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 documentFields, Map) 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 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); } } diff --git a/server/src/test/java/org/elasticsearch/search/fetch/subphase/FieldFetcherTests.java b/server/src/test/java/org/elasticsearch/search/fetch/subphase/FieldFetcherTests.java index 0178d8cf38e5..1f92c773021f 100644 --- a/server/src/test/java/org/elasticsearch/search/fetch/subphase/FieldFetcherTests.java +++ b/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; @@ -659,6 +660,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 ff = new ArrayList<>(); + ff.add(new FieldAndFormat("date_field", "year", false)); + Map 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 fieldAndFormatList(String name, String format, boolean includeUnmapped) { return Collections.singletonList(new FieldAndFormat(name, format, includeUnmapped)); }