Skip to content

Commit

Permalink
Improve FieldFetcher retrieval of fields (elastic#66160)
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
Christoph Büscher committed Dec 14, 2020
1 parent a6bad06 commit 968fc72
Show file tree
Hide file tree
Showing 2 changed files with 39 additions and 15 deletions.
27 changes: 12 additions & 15 deletions server/src/main/java/org/elasticsearch/search/fetch/subphase/FieldFetcher.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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) {
Expand All @@ -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());
Expand All @@ -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;
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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);
}
}
Expand All @@ -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);
}
}
Expand Down
27 changes: 27 additions & 0 deletions server/src/test/java/org/elasticsearch/search/fetch/subphase/FieldFetcherTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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<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));
}
Expand Down

0 comments on commit 968fc72

Please sign in to comment.