From b600744061d467a5b7ac2209afb58f9a63965141 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christoph=20B=C3=BCscher?= Date: Thu, 10 Dec 2020 11:46:36 +0100 Subject: [PATCH 1/2] Improve FieldFetcher retrieval of fields 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 | 12 ++++----- .../fetch/subphase/FieldFetcherTests.java | 27 +++++++++++++++++++ 2 files changed, 33 insertions(+), 6 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 b9a68a555375b..0247fed961e4a 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 @@ -48,7 +48,7 @@ public static FieldFetcher create(QueryShardContext context, SearchLookup searchLookup, Collection fieldAndFormats) { - List fieldContexts = new ArrayList<>(); + Map fieldContexts = new HashMap<>(); List unmappedFetchPattern = new ArrayList<>(); Set mappedToExclude = new HashSet<>(); boolean includeUnmapped = false; @@ -69,7 +69,7 @@ public static FieldFetcher create(QueryShardContext context, } 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()); @@ -81,13 +81,13 @@ public static FieldFetcher create(QueryShardContext context, return new FieldFetcher(fieldContexts, unmappedFetchAutomaton, mappedToExclude, 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 @@ -100,7 +100,7 @@ private FieldFetcher( 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; @@ -192,7 +192,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 00d17464f4e70..0af074af14c14 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; @@ -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 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)); } From fb1bd84bab7f6ecbb7acf896b6448ab9e54384ad Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christoph=20B=C3=BCscher?= Date: Mon, 14 Dec 2020 12:25:57 +0100 Subject: [PATCH 2/2] iter --- .../search/fetch/subphase/FieldFetcher.java | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 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 0247fed961e4a..81124840ae79b 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) { - Map fieldContexts = new HashMap<>(); + // 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,7 +69,6 @@ public static FieldFetcher create(QueryShardContext context, continue; } ValueFetcher valueFetcher = ft.valueFetcher(context, format); - mappedToExclude.add(field); fieldContexts.put(field, new FieldContext(field, valueFetcher)); } } @@ -78,23 +78,20 @@ 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 Map fieldContexts; private final CharacterRunAutomaton unmappedFetchAutomaton; - private final Set mappedToExclude; private final boolean includeUnmapped; private FieldFetcher( Map fieldContexts, CharacterRunAutomaton unmappedFetchAutomaton, - Set mappedToExclude, boolean includeUnmapped ) { this.fieldContexts = fieldContexts; this.unmappedFetchAutomaton = unmappedFetchAutomaton; - this.mappedToExclude = mappedToExclude; this.includeUnmapped = includeUnmapped; } @@ -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); } }