From 37265dbbdcbd18459b2a5233e946235cc3ba4b79 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christoph=20B=C3=BCscher?= Date: Tue, 3 Nov 2020 15:38:58 +0100 Subject: [PATCH 01/10] Support unmapped fields in search 'fields' option Currently, the 'fields' option only supports fetching mapped fields. Since 'fields' is meant to be the central place to retrieve document content, it should allow for loading unmapped values. This change adds implementation and tests for this addition. Closes #63690 --- .../test/search/330_fetch_fields.yml | 66 +++++++ .../index/query/InnerHitContextBuilder.java | 2 +- .../elasticsearch/search/SearchService.java | 3 +- .../metrics/TopHitsAggregatorFactory.java | 2 +- .../fetch/subphase/FetchFieldsContext.java | 8 +- .../fetch/subphase/FetchFieldsPhase.java | 8 +- .../search/fetch/subphase/FieldFetcher.java | 78 +++++++- .../fetch/subphase/FieldFetcherTests.java | 185 +++++++++++++++++- 8 files changed, 334 insertions(+), 18 deletions(-) diff --git a/rest-api-spec/src/main/resources/rest-api-spec/test/search/330_fetch_fields.yml b/rest-api-spec/src/main/resources/rest-api-spec/test/search/330_fetch_fields.yml index b43a63398d138..2d3cd53c29b1a 100644 --- a/rest-api-spec/src/main/resources/rest-api-spec/test/search/330_fetch_fields.yml +++ b/rest-api-spec/src/main/resources/rest-api-spec/test/search/330_fetch_fields.yml @@ -295,3 +295,69 @@ setup: - is_true: hits.hits.0._id - match: { hits.hits.0.fields.count: [2] } - is_false: hits.hits.0.fields.count_without_dv +--- +Test unmapped field: + - skip: + version: ' - 7.99.99' + reason: support isn't yet backported + - do: + indices.create: + index: test + body: + mappings: + dynamic: false + properties: + f1: + type: keyword + f2: + type: object + enabled: false + f3: + type: object + - do: + index: + index: test + id: 1 + refresh: true + body: + f1: some text + f2: + a: foo + b: bar + f3: + c: baz + f4: some other text + - do: + search: + index: test + body: + fields: + - f1 + - f4 + - match: + hits.hits.0.fields.f1: + - some text + - match: + hits.hits.0.fields.f4: + - some other text + - do: + search: + index: test + body: + fields: + - f* + - match: + hits.hits.0.fields.f1: + - some text + - match: + hits.hits.0.fields.f2\.a: + - foo + - match: + hits.hits.0.fields.f2\.b: + - bar + - match: + hits.hits.0.fields.f3\.c: + - baz + - match: + hits.hits.0.fields.f4: + - some other text diff --git a/server/src/main/java/org/elasticsearch/index/query/InnerHitContextBuilder.java b/server/src/main/java/org/elasticsearch/index/query/InnerHitContextBuilder.java index 26518428763b8..f95bc84cd28cd 100644 --- a/server/src/main/java/org/elasticsearch/index/query/InnerHitContextBuilder.java +++ b/server/src/main/java/org/elasticsearch/index/query/InnerHitContextBuilder.java @@ -93,7 +93,7 @@ protected void setupInnerHitsContext(QueryShardContext queryShardContext, innerHitsContext.docValuesContext(docValuesContext); } if (innerHitBuilder.getFetchFields() != null) { - FetchFieldsContext fieldsContext = new FetchFieldsContext(innerHitBuilder.getFetchFields()); + FetchFieldsContext fieldsContext = new FetchFieldsContext(innerHitBuilder.getFetchFields(), false); innerHitsContext.fetchFieldsContext(fieldsContext); } if (innerHitBuilder.getScriptFields() != null) { diff --git a/server/src/main/java/org/elasticsearch/search/SearchService.java b/server/src/main/java/org/elasticsearch/search/SearchService.java index e8ed6134cb883..8bd8f22f7ef38 100644 --- a/server/src/main/java/org/elasticsearch/search/SearchService.java +++ b/server/src/main/java/org/elasticsearch/search/SearchService.java @@ -973,7 +973,8 @@ private void parseSource(DefaultSearchContext context, SearchSourceBuilder sourc context.docValuesContext(docValuesContext); } if (source.fetchFields() != null) { - FetchFieldsContext fetchFieldsContext = new FetchFieldsContext(source.fetchFields()); + // TODO make "includeUnmapped configurable? + FetchFieldsContext fetchFieldsContext = new FetchFieldsContext(source.fetchFields(), true); context.fetchFieldsContext(fetchFieldsContext); } if (source.highlighter() != null) { diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/TopHitsAggregatorFactory.java b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/TopHitsAggregatorFactory.java index f88dd6ce3793e..9882e9cdef0db 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/TopHitsAggregatorFactory.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/TopHitsAggregatorFactory.java @@ -114,7 +114,7 @@ public Aggregator createInternal(SearchContext searchContext, subSearchContext.docValuesContext(docValuesContext); } if (fetchFields != null) { - FetchFieldsContext fieldsContext = new FetchFieldsContext(fetchFields); + FetchFieldsContext fieldsContext = new FetchFieldsContext(fetchFields, true); subSearchContext.fetchFieldsContext(fieldsContext); } for (ScriptFieldsContext.ScriptField field : scriptFields) { diff --git a/server/src/main/java/org/elasticsearch/search/fetch/subphase/FetchFieldsContext.java b/server/src/main/java/org/elasticsearch/search/fetch/subphase/FetchFieldsContext.java index 29d96495e0a17..098a0660e3479 100644 --- a/server/src/main/java/org/elasticsearch/search/fetch/subphase/FetchFieldsContext.java +++ b/server/src/main/java/org/elasticsearch/search/fetch/subphase/FetchFieldsContext.java @@ -25,12 +25,18 @@ */ public class FetchFieldsContext { private final List fields; + private final boolean includeUnmapped; - public FetchFieldsContext(List fields) { + public FetchFieldsContext(List fields, boolean includeUnmapped) { this.fields = fields; + this.includeUnmapped = includeUnmapped; } public List fields() { return fields; } + + public boolean includeUnmapped() { + return includeUnmapped; + } } diff --git a/server/src/main/java/org/elasticsearch/search/fetch/subphase/FetchFieldsPhase.java b/server/src/main/java/org/elasticsearch/search/fetch/subphase/FetchFieldsPhase.java index c450e61c33ab7..0c4487afe81d5 100644 --- a/server/src/main/java/org/elasticsearch/search/fetch/subphase/FetchFieldsPhase.java +++ b/server/src/main/java/org/elasticsearch/search/fetch/subphase/FetchFieldsPhase.java @@ -53,7 +53,13 @@ public FetchSubPhaseProcessor getProcessor(FetchContext fetchContext) { "in the mappings for index [" + fetchContext.getIndexName() + "]"); } - FieldFetcher fieldFetcher = FieldFetcher.create(fetchContext.getQueryShardContext(), searchLookup, fetchFieldsContext.fields()); + FieldFetcher fieldFetcher = FieldFetcher.create( + fetchContext.getQueryShardContext(), + searchLookup, + fetchFieldsContext.fields(), + fetchFieldsContext.includeUnmapped() + ); + return new FetchSubPhaseProcessor() { @Override public void setNextReader(LeafReaderContext readerContext) { 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 dce613955ca72..f959369678a5a 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 @@ -21,6 +21,7 @@ import org.apache.lucene.index.LeafReaderContext; import org.elasticsearch.common.document.DocumentField; +import org.elasticsearch.common.regex.Regex; import org.elasticsearch.index.mapper.MappedFieldType; import org.elasticsearch.index.mapper.ValueFetcher; import org.elasticsearch.index.query.QueryShardContext; @@ -30,7 +31,9 @@ import java.io.IOException; import java.util.ArrayList; import java.util.Collection; +import java.util.Collections; import java.util.HashMap; +import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.Set; @@ -42,32 +45,48 @@ public class FieldFetcher { public static FieldFetcher create(QueryShardContext context, SearchLookup searchLookup, - Collection fieldAndFormats) { + Collection fieldAndFormats, + boolean includeUnmapped) { List fieldContexts = new ArrayList<>(); + List originalPattern = new ArrayList<>(); + List mappedFields = new ArrayList<>(); for (FieldAndFormat fieldAndFormat : fieldAndFormats) { String fieldPattern = fieldAndFormat.field; String format = fieldAndFormat.format; Collection concreteFields = context.simpleMatchToIndexNames(fieldPattern); + originalPattern.add(fieldAndFormat.field); for (String field : concreteFields) { MappedFieldType ft = context.getFieldType(field); if (ft == null || context.isMetadataField(field)) { continue; } ValueFetcher valueFetcher = ft.valueFetcher(context, searchLookup, format); + mappedFields.add(field); fieldContexts.add(new FieldContext(field, valueFetcher)); } } - return new FieldFetcher(fieldContexts); + return new FieldFetcher(fieldContexts, originalPattern, mappedFields, includeUnmapped); } private final List fieldContexts; - - private FieldFetcher(List fieldContexts) { + private final List originalPattern; + private final List mappedFields; + private final boolean includeUnmapped; + + private FieldFetcher( + List fieldContexts, + List originalPattern, + List mappedFields, + boolean includeUnmapped + ) { this.fieldContexts = fieldContexts; + this.originalPattern = originalPattern; + this.mappedFields = mappedFields; + this.includeUnmapped = includeUnmapped; } public Map fetch(SourceLookup sourceLookup, Set ignoredFields) throws IOException { @@ -85,9 +104,60 @@ public Map fetch(SourceLookup sourceLookup, Set i documentFields.put(field, new DocumentField(field, parsedValues)); } } + if (includeUnmapped) { + // also look up unmapped fields from source + Set allSourcePaths = extractAllLeafPaths(sourceLookup.loadSourceIfNeeded()); + for (String fetchFieldPattern : originalPattern) { + if (Regex.isSimpleMatchPattern(fetchFieldPattern) == false) { + // if pattern has no wildcard, simply look up field if not already present + if (allSourcePaths.contains(fetchFieldPattern)) { + addValueFromSource(sourceLookup, fetchFieldPattern, documentFields); + } + } else { + for (String singlePath : allSourcePaths) { + if (Regex.simpleMatch(fetchFieldPattern, singlePath)) { + addValueFromSource(sourceLookup, singlePath, documentFields); + } + } + } + } + } return documentFields; } + private void addValueFromSource(SourceLookup sourceLookup, String path, Map documentFields) { + // checking mapped fields here again to avoid adding from _source where some value was already added or ignored on purpose before + if (mappedFields.contains(path) == false) { + Object object = sourceLookup.extractValue(path, null); + DocumentField f; + if (object instanceof List) { + f = new DocumentField(path, (List) object); + } else { + f = new DocumentField(path, Collections.singletonList(object)); + } + if (f.getValue() != null) { + documentFields.put(path, f); + } + } + } + + static Set extractAllLeafPaths(Map map) { + Set allPaths = new HashSet<>(); + collectAllPaths(allPaths, "", map); + return allPaths; + } + + private static void collectAllPaths(Set allPaths, String prefix, Map source) { + for (String key : source.keySet()) { + Object value = source.get(key); + if (value instanceof Map) { + collectAllPaths(allPaths, prefix + key + ".", (Map) value); + } else { + allPaths.add(prefix + key); + } + } + } + public void setNextReader(LeafReaderContext readerContext) { for (FieldContext field : fieldContexts) { 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 181f99b982cbf..f17e85893574b 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 @@ -56,7 +56,7 @@ public void testLeafValues() throws IOException { List fieldAndFormats = List.of( new FieldAndFormat("field", null), new FieldAndFormat("object.field", null)); - Map fields = fetchFields(mapperService, source, fieldAndFormats); + Map fields = fetchFields(mapperService, source, fieldAndFormats, false); assertThat(fields.size(), equalTo(2)); DocumentField field = fields.get("field"); @@ -218,7 +218,7 @@ public void testDateFormat() throws IOException { Map fields = fetchFields(mapperService, source, List.of( new FieldAndFormat("field", null), - new FieldAndFormat("date_field", "yyyy/MM/dd"))); + new FieldAndFormat("date_field", "yyyy/MM/dd")), false); assertThat(fields.size(), equalTo(2)); DocumentField field = fields.get("field"); @@ -393,21 +393,188 @@ public void testTextSubFields() throws IOException { } } - private static Map fetchFields(MapperService mapperService, XContentBuilder source, String fieldPattern) - throws IOException { + public void testSimpleUnmappedFields() throws IOException { + MapperService mapperService = createMapperService(); + + XContentBuilder source = XContentFactory.jsonBuilder().startObject() + .field("unmapped_f1", "some text") + .field("unmapped_f2", "some text") + .field("unmapped_f3", "some text") + .field("something_else", "some text") + .nullField("null_value") + .endObject(); + + Map fields = fetchFields(mapperService, source, "unmapped_f*", true); + assertThat(fields.size(), equalTo(3)); + assertThat(fields.keySet(), containsInAnyOrder("unmapped_f1", "unmapped_f2", "unmapped_f3")); + + fields = fetchFields(mapperService, source, "un*1", true); + assertThat(fields.size(), equalTo(1)); + assertThat(fields.keySet(), containsInAnyOrder("unmapped_f1")); + + fields = fetchFields(mapperService, source, "*thing*", true); + assertThat(fields.size(), equalTo(1)); + assertThat(fields.keySet(), containsInAnyOrder("something_else")); + + fields = fetchFields(mapperService, source, "*thing*", true); + assertThat(fields.size(), equalTo(1)); + assertThat(fields.keySet(), containsInAnyOrder("something_else")); + + // TODO discuss wether we want to return something here + fields = fetchFields(mapperService, source, "null*", true); + assertThat(fields.size(), equalTo(0)); + // assertThat(fields.get("null_value").getValue(), equalTo(null)); + } + + public void testSimpleUnmappedArray() throws IOException { + MapperService mapperService = createMapperService(); + + XContentBuilder source = XContentFactory.jsonBuilder().startObject() + .array("unmapped_field", "foo", "bar") + .endObject(); + + Map fields = fetchFields(mapperService, source, "unmapped_field", true); + assertThat(fields.size(), equalTo(1)); + assertThat(fields.keySet(), containsInAnyOrder("unmapped_field")); + + for (DocumentField field : fields.values()) { + assertThat(field.getValues().size(), equalTo(2)); + assertThat(field.getValues().get(0), equalTo("foo")); + assertThat(field.getValues().get(1), equalTo("bar")); + } + } + + public void testUnmappedFieldsInsideObject() throws IOException { + XContentBuilder mapping = XContentFactory.jsonBuilder().startObject() + .startObject("properties") + .startObject("obj") + .field("type", "object") + .field("dynamic", "false") + .startObject("properties") + .startObject("f1").field("type", "keyword").endObject() + .endObject() + .endObject() + .endObject() + .endObject(); + + IndexService indexService = createIndex("index", Settings.EMPTY, mapping); + MapperService mapperService = indexService.mapperService(); + + XContentBuilder source = XContentFactory.jsonBuilder().startObject() + .field("obj.f1", "value1") + .field("obj.f2", "unmapped_value_f2") + .field("obj.innerObj.f3", "unmapped_value_f3") + .field("obj.innerObj.f4", "unmapped_value_f4") + .endObject(); + + Map fields = fetchFields(mapperService, source, "*", false); + + // without unmapped fields this should only return "obj.f1" + assertThat(fields.size(), equalTo(1)); + assertThat(fields.keySet(), containsInAnyOrder("obj.f1")); + + fields = fetchFields(mapperService, source, "*", true); + assertThat(fields.size(), equalTo(4)); + assertThat(fields.keySet(), containsInAnyOrder("obj.f1", "obj.f2", "obj.innerObj.f3", "obj.innerObj.f4")); + } + + /** + * If a mapped field for some reason contains a "_source" value that is not returned by the + * mapped retrieval mechanism (e.g. because its malformed), we don't want to fetch it from _source. + */ + public void testMappedFieldNotOverwritten() throws IOException { + XContentBuilder mapping = XContentFactory.jsonBuilder().startObject() + .startObject("properties") + .startObject("f1") + .field("type", "integer") + .field("ignore_malformed", "true") + .endObject() + .endObject() + .endObject(); + + IndexService indexService = createIndex("index", Settings.EMPTY, mapping); + MapperService mapperService = indexService.mapperService(); + + XContentBuilder source = XContentFactory.jsonBuilder().startObject() + .field("f1", "malformed") + .endObject(); + + // this should not return a field bc. f1 is in the ignored fields + Map fields = fetchFields(mapperService, source, List.of(new FieldAndFormat("*", null)), false, Set.of("f1")); + assertThat(fields.size(), equalTo(0)); + + // and this should neither + fields = fetchFields(mapperService, source, List.of(new FieldAndFormat("*", null)), true, Set.of("f1")); + assertThat(fields.size(), equalTo(0)); + + fields = fetchFields(mapperService, source, List.of(new FieldAndFormat("f1", null)), true, Set.of("f1")); + assertThat(fields.size(), equalTo(0)); + } + + public void testUnmappedFieldsWildcard() throws IOException { + MapperService mapperService = createMapperService(); + XContentBuilder source = XContentFactory.jsonBuilder().startObject() + .startObject("unmapped_object") + .field("a", "foo") + .field("b", "bar") + .endObject() + .endObject(); + + Map fields = fetchFields(mapperService, source, "unmapped_object", true); + assertThat(fields.size(), equalTo(0)); + + fields = fetchFields(mapperService, source, "unmapped_object.*", true); + assertThat(fields.size(), equalTo(2)); + assertThat(fields.keySet(), containsInAnyOrder("unmapped_object.a", "unmapped_object.b")); + + assertThat(fields.get("unmapped_object.a").getValue(), equalTo("foo")); + assertThat(fields.get("unmapped_object.b").getValue(), equalTo("bar")); + } + + private Map fetchFields( + MapperService mapperService, + XContentBuilder source, + String fieldPattern + ) throws IOException { + + List fields = List.of(new FieldAndFormat(fieldPattern, null)); + return fetchFields(mapperService, source, fields, false); + } + + private Map fetchFields( + MapperService mapperService, + XContentBuilder source, + String fieldPattern, + boolean includeUnmapped + ) throws IOException { List fields = List.of(new FieldAndFormat(fieldPattern, null)); - return fetchFields(mapperService, source, fields); + return fetchFields(mapperService, source, fields, includeUnmapped); + } + + private static Map fetchFields( + MapperService mapperService, + XContentBuilder source, + List fields, + boolean includeUnmapped + ) throws IOException { + + return fetchFields(mapperService, source, fields, includeUnmapped, Set.of()); } - private static Map fetchFields(MapperService mapperService, XContentBuilder source, List fields) - throws IOException { + private static Map fetchFields( + MapperService mapperService, + XContentBuilder source, + List fields, + boolean includeUnmapped, + Set ignoreFields + ) throws IOException { SourceLookup sourceLookup = new SourceLookup(); sourceLookup.setSource(BytesReference.bytes(source)); - FieldFetcher fieldFetcher = FieldFetcher.create(createQueryShardContext(mapperService), null, fields); - return fieldFetcher.fetch(sourceLookup, Set.of()); + FieldFetcher fieldFetcher = FieldFetcher.create(createQueryShardContext(mapperService), null, fields, includeUnmapped); + return fieldFetcher.fetch(sourceLookup, ignoreFields); } public MapperService createMapperService() throws IOException { From 33e1a8b68979269182500869cd2450a8f2d8d87a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christoph=20B=C3=BCscher?= Date: Thu, 5 Nov 2020 19:26:29 +0100 Subject: [PATCH 02/10] Alternative impl using source map filtering --- .../search/fetch/subphase/FieldFetcher.java | 84 ++++++++----------- .../fetch/subphase/FieldFetcherTests.java | 12 +++ 2 files changed, 45 insertions(+), 51 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 f959369678a5a..b98f6650d8d3a 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 @@ -21,7 +21,7 @@ import org.apache.lucene.index.LeafReaderContext; import org.elasticsearch.common.document.DocumentField; -import org.elasticsearch.common.regex.Regex; +import org.elasticsearch.common.xcontent.support.XContentMapValues; import org.elasticsearch.index.mapper.MappedFieldType; import org.elasticsearch.index.mapper.ValueFetcher; import org.elasticsearch.index.query.QueryShardContext; @@ -33,10 +33,10 @@ import java.util.Collection; import java.util.Collections; import java.util.HashMap; -import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.Set; +import java.util.function.Function; /** * A helper class to {@link FetchFieldsPhase} that's initialized with a list of field patterns to fetch. @@ -50,7 +50,7 @@ public static FieldFetcher create(QueryShardContext context, List fieldContexts = new ArrayList<>(); List originalPattern = new ArrayList<>(); - List mappedFields = new ArrayList<>(); + List excludeForUnmappedFetch = new ArrayList<>(); for (FieldAndFormat fieldAndFormat : fieldAndFormats) { String fieldPattern = fieldAndFormat.field; @@ -64,29 +64,34 @@ public static FieldFetcher create(QueryShardContext context, continue; } ValueFetcher valueFetcher = ft.valueFetcher(context, searchLookup, format); - mappedFields.add(field); + excludeForUnmappedFetch.add(field); fieldContexts.add(new FieldContext(field, valueFetcher)); } + if (fieldPattern.charAt(fieldPattern.length() - 1) != '*') { + // not a prefix pattern, exclude potential sub-fields when fetching unmapped fields + excludeForUnmappedFetch.add(fieldPattern + ".*"); + } } + Function, Map> filter = XContentMapValues.filter( + originalPattern.toArray(new String[originalPattern.size()]), + excludeForUnmappedFetch.toArray(new String[excludeForUnmappedFetch.size()]) + ); - return new FieldFetcher(fieldContexts, originalPattern, mappedFields, includeUnmapped); + return new FieldFetcher(fieldContexts, includeUnmapped, filter); } private final List fieldContexts; - private final List originalPattern; - private final List mappedFields; private final boolean includeUnmapped; + private final Function, Map> filter; private FieldFetcher( List fieldContexts, - List originalPattern, - List mappedFields, - boolean includeUnmapped + boolean includeUnmapped, + Function, Map> filter ) { this.fieldContexts = fieldContexts; - this.originalPattern = originalPattern; - this.mappedFields = mappedFields; this.includeUnmapped = includeUnmapped; + this.filter = filter; } public Map fetch(SourceLookup sourceLookup, Set ignoredFields) throws IOException { @@ -105,55 +110,32 @@ public Map fetch(SourceLookup sourceLookup, Set i } } if (includeUnmapped) { - // also look up unmapped fields from source - Set allSourcePaths = extractAllLeafPaths(sourceLookup.loadSourceIfNeeded()); - for (String fetchFieldPattern : originalPattern) { - if (Regex.isSimpleMatchPattern(fetchFieldPattern) == false) { - // if pattern has no wildcard, simply look up field if not already present - if (allSourcePaths.contains(fetchFieldPattern)) { - addValueFromSource(sourceLookup, fetchFieldPattern, documentFields); - } - } else { - for (String singlePath : allSourcePaths) { - if (Regex.simpleMatch(fetchFieldPattern, singlePath)) { - addValueFromSource(sourceLookup, singlePath, documentFields); - } - } - } - } + Map unmappedFieldsToAdd = filter.apply(sourceLookup.loadSourceIfNeeded()); + collectLeafValues(unmappedFieldsToAdd, documentFields); } return documentFields; } - private void addValueFromSource(SourceLookup sourceLookup, String path, Map documentFields) { - // checking mapped fields here again to avoid adding from _source where some value was already added or ignored on purpose before - if (mappedFields.contains(path) == false) { - Object object = sourceLookup.extractValue(path, null); - DocumentField f; - if (object instanceof List) { - f = new DocumentField(path, (List) object); - } else { - f = new DocumentField(path, Collections.singletonList(object)); - } - if (f.getValue() != null) { - documentFields.put(path, f); - } - } - } - - static Set extractAllLeafPaths(Map map) { - Set allPaths = new HashSet<>(); - collectAllPaths(allPaths, "", map); - return allPaths; + static void collectLeafValues(Map map, Map documentFields) { + collectAllPaths("", map, documentFields); } - private static void collectAllPaths(Set allPaths, String prefix, Map source) { + private static void collectAllPaths(String prefix, Map source, Map documentFields) { for (String key : source.keySet()) { Object value = source.get(key); + String currentPath = prefix + key; if (value instanceof Map) { - collectAllPaths(allPaths, prefix + key + ".", (Map) value); + collectAllPaths(currentPath + ".", (Map) value, documentFields); } else { - allPaths.add(prefix + key); + DocumentField f; + if (value instanceof List) { + f = new DocumentField(currentPath, (List) value); + } else { + f = new DocumentField(currentPath, Collections.singletonList(value)); + } + if (f.getValue() != null) { + documentFields.put(currentPath, f); + } } } } 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 f17e85893574b..f6c1229364b26 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 @@ -524,12 +524,24 @@ public void testUnmappedFieldsWildcard() throws IOException { Map fields = fetchFields(mapperService, source, "unmapped_object", true); assertThat(fields.size(), equalTo(0)); + fields = fetchFields(mapperService, source, "unmap*object", true); + assertThat(fields.size(), equalTo(0)); + fields = fetchFields(mapperService, source, "unmapped_object.*", true); assertThat(fields.size(), equalTo(2)); assertThat(fields.keySet(), containsInAnyOrder("unmapped_object.a", "unmapped_object.b")); assertThat(fields.get("unmapped_object.a").getValue(), equalTo("foo")); assertThat(fields.get("unmapped_object.b").getValue(), equalTo("bar")); + + fields = fetchFields(mapperService, source, "unmapped_object.a", true); + assertThat(fields.size(), equalTo(1)); + assertThat(fields.get("unmapped_object.a").getValue(), equalTo("foo")); + + fields = fetchFields(mapperService, source, "unmapped_object.b", true); + assertThat(fields.size(), equalTo(1)); + assertThat(fields.get("unmapped_object.b").getValue(), equalTo("bar")); + } private Map fetchFields( From 9f39f914129937255ed63619f8012f2ccdc2499d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christoph=20B=C3=BCscher?= Date: Fri, 6 Nov 2020 16:02:45 +0100 Subject: [PATCH 03/10] Adding bug url --- .../java/org/elasticsearch/index/store/cache/TestUtils.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/x-pack/plugin/searchable-snapshots/src/test/java/org/elasticsearch/index/store/cache/TestUtils.java b/x-pack/plugin/searchable-snapshots/src/test/java/org/elasticsearch/index/store/cache/TestUtils.java index 53095b5879cbe..e120524694f9f 100644 --- a/x-pack/plugin/searchable-snapshots/src/test/java/org/elasticsearch/index/store/cache/TestUtils.java +++ b/x-pack/plugin/searchable-snapshots/src/test/java/org/elasticsearch/index/store/cache/TestUtils.java @@ -52,7 +52,8 @@ static long numberOfRanges(int fileSize, int rangeSize) { } static SortedSet> mergeContiguousRanges(final SortedSet> ranges) { - return ranges.stream().collect(() -> new TreeSet<>(Comparator.comparingLong(Tuple::v1)), (gaps, gap) -> { + // Eclipse needs the TreeSet type to be explicit (see https://bugs.eclipse.org/bugs/show_bug.cgi?id=568600) + return ranges.stream().collect(() -> new TreeSet>(Comparator.comparingLong(Tuple::v1)), (gaps, gap) -> { if (gaps.isEmpty()) { gaps.add(gap); } else { From f50bdc2c4c9234d1036c8f7014ba181374447c7b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christoph=20B=C3=BCscher?= Date: Fri, 6 Nov 2020 19:38:37 +0100 Subject: [PATCH 04/10] Add other collect implementation --- .../search/fetch/subphase/FieldFetcher.java | 102 +++++++++++------- .../fetch/subphase/FieldFetcherTests.java | 42 ++++++++ 2 files changed, 107 insertions(+), 37 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 b98f6650d8d3a..9197b95eb65f0 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 @@ -20,8 +20,9 @@ package org.elasticsearch.search.fetch.subphase; import org.apache.lucene.index.LeafReaderContext; +import org.apache.lucene.util.automaton.CharacterRunAutomaton; import org.elasticsearch.common.document.DocumentField; -import org.elasticsearch.common.xcontent.support.XContentMapValues; +import org.elasticsearch.common.regex.Regex; import org.elasticsearch.index.mapper.MappedFieldType; import org.elasticsearch.index.mapper.ValueFetcher; import org.elasticsearch.index.query.QueryShardContext; @@ -31,12 +32,11 @@ import java.io.IOException; import java.util.ArrayList; import java.util.Collection; -import java.util.Collections; import java.util.HashMap; +import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.Set; -import java.util.function.Function; /** * A helper class to {@link FetchFieldsPhase} that's initialized with a list of field patterns to fetch. @@ -49,49 +49,46 @@ public static FieldFetcher create(QueryShardContext context, boolean includeUnmapped) { List fieldContexts = new ArrayList<>(); - List originalPattern = new ArrayList<>(); - List excludeForUnmappedFetch = new ArrayList<>(); + String[] originalPattern = new String[fieldAndFormats.size()]; + Set mappedToExclude = new HashSet<>(); + int i = 0; for (FieldAndFormat fieldAndFormat : fieldAndFormats) { String fieldPattern = fieldAndFormat.field; + originalPattern[i] = fieldAndFormat.field; + i++; String format = fieldAndFormat.format; Collection concreteFields = context.simpleMatchToIndexNames(fieldPattern); - originalPattern.add(fieldAndFormat.field); for (String field : concreteFields) { MappedFieldType ft = context.getFieldType(field); if (ft == null || context.isMetadataField(field)) { continue; } ValueFetcher valueFetcher = ft.valueFetcher(context, searchLookup, format); - excludeForUnmappedFetch.add(field); + mappedToExclude.add(field); fieldContexts.add(new FieldContext(field, valueFetcher)); } - if (fieldPattern.charAt(fieldPattern.length() - 1) != '*') { - // not a prefix pattern, exclude potential sub-fields when fetching unmapped fields - excludeForUnmappedFetch.add(fieldPattern + ".*"); - } } - Function, Map> filter = XContentMapValues.filter( - originalPattern.toArray(new String[originalPattern.size()]), - excludeForUnmappedFetch.toArray(new String[excludeForUnmappedFetch.size()]) - ); - - return new FieldFetcher(fieldContexts, includeUnmapped, filter); + CharacterRunAutomaton pathAutomaton = new CharacterRunAutomaton(Regex.simpleMatchToAutomaton(originalPattern)); + return new FieldFetcher(fieldContexts, includeUnmapped, pathAutomaton, mappedToExclude); } private final List fieldContexts; private final boolean includeUnmapped; - private final Function, Map> filter; + private final CharacterRunAutomaton pathAutomaton; + private final Set mappedToExclude; private FieldFetcher( List fieldContexts, boolean includeUnmapped, - Function, Map> filter + CharacterRunAutomaton pathAutomaton, + Set mappedToExclude ) { this.fieldContexts = fieldContexts; this.includeUnmapped = includeUnmapped; - this.filter = filter; + this.pathAutomaton = pathAutomaton; + this.mappedToExclude = mappedToExclude; } public Map fetch(SourceLookup sourceLookup, Set ignoredFields) throws IOException { @@ -110,36 +107,67 @@ public Map fetch(SourceLookup sourceLookup, Set i } } if (includeUnmapped) { - Map unmappedFieldsToAdd = filter.apply(sourceLookup.loadSourceIfNeeded()); - collectLeafValues(unmappedFieldsToAdd, documentFields); + collect(documentFields, sourceLookup.loadSourceIfNeeded(), "", 0); } return documentFields; } - static void collectLeafValues(Map map, Map documentFields) { - collectAllPaths("", map, documentFields); - } - - private static void collectAllPaths(String prefix, Map source, Map documentFields) { + private void collect(Map documentFields, Map source, String parentPath, int lastState) { for (String key : source.keySet()) { Object value = source.get(key); - String currentPath = prefix + key; + String currentPath = parentPath + key; + int currentState = step(this.pathAutomaton, key, lastState); + if (currentState == -1) { + // path doesn't match any fields pattern + continue; + } if (value instanceof Map) { - collectAllPaths(currentPath + ".", (Map) value, documentFields); + // one step deeper into source tree + collect(documentFields, (Map) value, currentPath + ".", step(this.pathAutomaton, ".", currentState)); + } else if (value instanceof List) { + // iterate through list values + collect(documentFields, (List) value, currentPath, currentState); } else { - DocumentField f; - if (value instanceof List) { - f = new DocumentField(currentPath, (List) value); - } else { - f = new DocumentField(currentPath, Collections.singletonList(value)); - } - if (f.getValue() != null) { - documentFields.put(currentPath, f); + // we have a leaf value + if (this.pathAutomaton.isAccept(currentState) && this.mappedToExclude.contains(currentPath) == false) { + if (value != null) { + DocumentField currentEntry = documentFields.get(currentPath); + if (currentEntry == null) { + List list = new ArrayList<>(); + list.add(value); + documentFields.put(currentPath, new DocumentField(currentPath, list)); + } else { + currentEntry.getValues().add(value); + } + } } } } } + private void collect(Map documentFields, Iterable iterable, String parentPath, int lastState) { + List list = new ArrayList<>(); + for (Object value : iterable) { + if (value instanceof Map) { + collect(documentFields, (Map) value, parentPath + ".", step(this.pathAutomaton, ".", lastState)); + } else if (value instanceof List) { + // TODO can this happen with Json sources? + } else if (this.pathAutomaton.isAccept(lastState)) { + list.add(value); + } + } + if (list.isEmpty() == false) { + documentFields.put(parentPath, new DocumentField(parentPath, list)); + } + } + + private static int step(CharacterRunAutomaton automaton, String key, int state) { + for (int i = 0; state != -1 && i < key.length(); ++i) { + state = automaton.step(state, key.charAt(i)); + } + return state; + } + public void setNextReader(LeafReaderContext readerContext) { for (FieldContext field : fieldContexts) { 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 f6c1229364b26..c6bc651896246 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 @@ -444,6 +444,48 @@ public void testSimpleUnmappedArray() throws IOException { } } + public void testSimpleUnmappedArrayWithObjects() throws IOException { + MapperService mapperService = createMapperService(); + + XContentBuilder source = XContentFactory.jsonBuilder().startObject() + .startArray("unmapped_field") + .startObject() + .field("f1", "a") + .endObject() + .startObject() + .field("f2", "b") + .endObject() + .endArray() + .endObject(); + + Map fields = fetchFields(mapperService, source, "unmapped_field", true); + assertThat(fields.size(), equalTo(0)); + + fields = fetchFields(mapperService, source, "unmapped_field.f*", true); + assertThat(fields.size(), equalTo(2)); + assertThat(fields.get("unmapped_field.f1").getValue(), equalTo("a")); + assertThat(fields.get("unmapped_field.f2").getValue(), equalTo("b")); + + source = XContentFactory.jsonBuilder().startObject() + .startArray("unmapped_field") + .startObject() + .field("f1", "a") + .endObject() + .startObject() + .field("f1", "b") // same field name, this should result in a list returned + .endObject() + .endArray() + .endObject(); + + fields = fetchFields(mapperService, source, "unmapped_field.f1", true); + assertThat(fields.size(), equalTo(1)); + DocumentField field = fields.get("unmapped_field.f1"); + assertThat(field.getValues().size(), equalTo(2)); + assertThat(field.getValues().get(0), equalTo("a")); + assertThat(field.getValues().get(1), equalTo("b")); + + } + public void testUnmappedFieldsInsideObject() throws IOException { XContentBuilder mapping = XContentFactory.jsonBuilder().startObject() .startObject("properties") From c94b1918803e74114cbc010528c375bf6da70fa8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christoph=20B=C3=BCscher?= Date: Mon, 9 Nov 2020 14:14:38 +0100 Subject: [PATCH 05/10] iter --- .../test/search/330_fetch_fields.yml | 42 ++++++++++++++++ .../search/fetch/subphase/FieldFetcher.java | 14 +++--- .../fetch/subphase/FieldFetcherTests.java | 49 ++++++++++++++++++- 3 files changed, 98 insertions(+), 7 deletions(-) diff --git a/rest-api-spec/src/main/resources/rest-api-spec/test/search/330_fetch_fields.yml b/rest-api-spec/src/main/resources/rest-api-spec/test/search/330_fetch_fields.yml index 2d3cd53c29b1a..3ab32720ed1c6 100644 --- a/rest-api-spec/src/main/resources/rest-api-spec/test/search/330_fetch_fields.yml +++ b/rest-api-spec/src/main/resources/rest-api-spec/test/search/330_fetch_fields.yml @@ -361,3 +361,45 @@ Test unmapped field: - match: hits.hits.0.fields.f4: - some other text +--- +Test unmapped fields inside disabled objects: + - skip: + version: ' - 7.99.99' + reason: support isn't yet backported + - do: + indices.create: + index: test + body: + mappings: + properties: + f1: + type: object + enabled: false + - do: + index: + index: test + id: 1 + refresh: true + body: + f1: + - some text + - a: b + - + - 1 + - 2 + - 3 + - do: + search: + index: test + body: + fields: ["*"] + - match: + hits.hits.0.fields.f1: + - some text + - + - 1 + - 2 + - 3 + - match: + hits.hits.0.fields.f1\.a: + - b 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 9197b95eb65f0..6a42e31d49704 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 @@ -126,7 +126,10 @@ private void collect(Map documentFields, Map) value, currentPath + ".", step(this.pathAutomaton, ".", currentState)); } else if (value instanceof List) { // iterate through list values - collect(documentFields, (List) value, currentPath, currentState); + List list = collectList(documentFields, (List) value, currentPath, currentState); + if (list.isEmpty() == false) { + documentFields.put(currentPath, new DocumentField(currentPath, list)); + } } else { // we have a leaf value if (this.pathAutomaton.isAccept(currentState) && this.mappedToExclude.contains(currentPath) == false) { @@ -145,20 +148,19 @@ private void collect(Map documentFields, Map documentFields, Iterable iterable, String parentPath, int lastState) { + private List collectList(Map documentFields, Iterable iterable, String parentPath, int lastState) { List list = new ArrayList<>(); for (Object value : iterable) { if (value instanceof Map) { collect(documentFields, (Map) value, parentPath + ".", step(this.pathAutomaton, ".", lastState)); } else if (value instanceof List) { - // TODO can this happen with Json sources? + // weird case, but can happen for objects with "enabled" : "false" + list.add(collectList(documentFields, (List) value, parentPath, lastState)); } else if (this.pathAutomaton.isAccept(lastState)) { list.add(value); } } - if (list.isEmpty() == false) { - documentFields.put(parentPath, new DocumentField(parentPath, list)); - } + return list; } private static int step(CharacterRunAutomaton automaton, String key, int state) { 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 c6bc651896246..b703d01681345 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 @@ -41,6 +41,7 @@ import static org.hamcrest.Matchers.containsInAnyOrder; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.hasItems; +import static org.hamcrest.Matchers.instanceOf; public class FieldFetcherTests extends ESSingleNodeTestCase { @@ -520,6 +521,53 @@ public void testUnmappedFieldsInsideObject() throws IOException { assertThat(fields.keySet(), containsInAnyOrder("obj.f1", "obj.f2", "obj.innerObj.f3", "obj.innerObj.f4")); } + public void testUnmappedFieldsInsideDisabledObject() throws IOException { + XContentBuilder mapping = XContentFactory.jsonBuilder().startObject() + .startObject("properties") + .startObject("obj") + .field("type", "object") + .field("enabled", "false") + .endObject() + .endObject() + .endObject(); + + IndexService indexService = createIndex("index", Settings.EMPTY, mapping); + MapperService mapperService = indexService.mapperService(); + + XContentBuilder source = XContentFactory.jsonBuilder().startObject() + .startArray("obj") + .value("string_value") + .startObject() + .field("a", "b") + .endObject() + .startArray() + .value(1).value(2).value(3) + .endArray() + .endArray() + .endObject(); + + Map fields = fetchFields(mapperService, source, "*", false); + + // without unmapped fields this should return nothing + assertThat(fields.size(), equalTo(0)); + + fields = fetchFields(mapperService, source, "*", true); + assertThat(fields.size(), equalTo(2)); + assertThat(fields.keySet(), containsInAnyOrder("obj", "obj.a")); + + List obj = fields.get("obj").getValues(); + assertEquals(2, obj.size()); + assertThat(obj.get(0), instanceOf(String.class)); + assertEquals("string_value", obj.get(0).toString()); + assertThat(obj.get(1), instanceOf(List.class)); + assertEquals(3, ((List) obj.get(1)).size()); + assertEquals("[1, 2, 3]", obj.get(1).toString()); + + List innerObj = fields.get("obj.a").getValues(); + assertEquals(1, innerObj.size()); + assertEquals("b", fields.get("obj.a").getValue()); + } + /** * If a mapped field for some reason contains a "_source" value that is not returned by the * mapped retrieval mechanism (e.g. because its malformed), we don't want to fetch it from _source. @@ -583,7 +631,6 @@ public void testUnmappedFieldsWildcard() throws IOException { fields = fetchFields(mapperService, source, "unmapped_object.b", true); assertThat(fields.size(), equalTo(1)); assertThat(fields.get("unmapped_object.b").getValue(), equalTo("bar")); - } private Map fetchFields( From 976fde1e383e702dcf07ca4f820b0a46a508ee5f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christoph=20B=C3=BCscher?= Date: Mon, 9 Nov 2020 16:54:48 +0100 Subject: [PATCH 06/10] Adapt flattend field yml --- .../test/resources/rest-api-spec/test/flattened/10_basic.yml | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/x-pack/plugin/src/test/resources/rest-api-spec/test/flattened/10_basic.yml b/x-pack/plugin/src/test/resources/rest-api-spec/test/flattened/10_basic.yml index 69729023158b1..c2f4aedc4e5e2 100644 --- a/x-pack/plugin/src/test/resources/rest-api-spec/test/flattened/10_basic.yml +++ b/x-pack/plugin/src/test/resources/rest-api-spec/test/flattened/10_basic.yml @@ -154,5 +154,6 @@ - match: { hits.total.value: 1 } - length: { hits.hits: 1 } - - length: { hits.hits.0.fields: 1 } + - length: { hits.hits.0.fields: 2 } - match: { hits.hits.0.fields.flattened: [ { "some_field": "some_value" } ] } + - match: { hits.hits.0.fields.flattened\.some_field: [ "some_value" ] } From 822fb02e4642e56fb0c5197e45cb1db9cb7edf96 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christoph=20B=C3=BCscher?= Date: Tue, 10 Nov 2020 17:45:37 +0100 Subject: [PATCH 07/10] Add include_unmapped option --- .../action/search/ExpandSearchPhase.java | 2 +- .../action/search/SearchRequestBuilder.java | 7 +- .../index/query/InnerHitBuilder.java | 15 ++++- .../index/query/InnerHitContextBuilder.java | 2 +- .../elasticsearch/search/SearchService.java | 3 +- .../metrics/TopHitsAggregationBuilder.java | 13 +++- .../metrics/TopHitsAggregatorFactory.java | 2 +- .../search/builder/SearchSourceBuilder.java | 8 +-- .../search/fetch/FetchContext.java | 4 +- .../fetch/subphase/FetchDocValuesContext.java | 2 +- .../fetch/subphase/FetchFieldsContext.java | 8 +-- .../fetch/subphase/FetchFieldsPhase.java | 7 +- .../search/fetch/subphase/FieldAndFormat.java | 29 ++++++-- .../search/fetch/subphase/FieldFetcher.java | 67 ++++++++++++------- .../index/query/InnerHitBuilderTests.java | 12 ++-- .../fetch/subphase/FieldFetcherTests.java | 42 ++++++------ 16 files changed, 132 insertions(+), 91 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/action/search/ExpandSearchPhase.java b/server/src/main/java/org/elasticsearch/action/search/ExpandSearchPhase.java index 57082ed450941..27405ccb555e6 100644 --- a/server/src/main/java/org/elasticsearch/action/search/ExpandSearchPhase.java +++ b/server/src/main/java/org/elasticsearch/action/search/ExpandSearchPhase.java @@ -137,7 +137,7 @@ private SearchSourceBuilder buildExpandSearchSourceBuilder(InnerHitBuilder optio } } if (options.getFetchFields() != null) { - options.getFetchFields().forEach(ff -> groupSource.fetchField(ff.field, ff.format)); + options.getFetchFields().forEach(ff -> groupSource.fetchField(ff.field, ff.format, ff.includeUnmapped)); } if (options.getDocValueFields() != null) { options.getDocValueFields().forEach(ff -> groupSource.docValueField(ff.field, ff.format)); diff --git a/server/src/main/java/org/elasticsearch/action/search/SearchRequestBuilder.java b/server/src/main/java/org/elasticsearch/action/search/SearchRequestBuilder.java index 3a02214abb6f0..51c803e3c79cd 100644 --- a/server/src/main/java/org/elasticsearch/action/search/SearchRequestBuilder.java +++ b/server/src/main/java/org/elasticsearch/action/search/SearchRequestBuilder.java @@ -310,7 +310,7 @@ public SearchRequestBuilder addDocValueField(String name) { * @param name The field to load */ public SearchRequestBuilder addFetchField(String name) { - sourceBuilder().fetchField(name, null); + sourceBuilder().fetchField(name, null, null); return this; } @@ -319,9 +319,10 @@ public SearchRequestBuilder addFetchField(String name) { * * @param name The field to load * @param format an optional format string used when formatting values, for example a date format. + * @param includeUnmapped whether this field pattern should also include unmapped fields */ - public SearchRequestBuilder addFetchField(String name, String format) { - sourceBuilder().fetchField(name, format); + public SearchRequestBuilder addFetchField(String name, String format, boolean includeUnmapped) { + sourceBuilder().fetchField(name, format, null); return this; } diff --git a/server/src/main/java/org/elasticsearch/index/query/InnerHitBuilder.java b/server/src/main/java/org/elasticsearch/index/query/InnerHitBuilder.java index 5416395cb7627..61f6bea6c9072 100644 --- a/server/src/main/java/org/elasticsearch/index/query/InnerHitBuilder.java +++ b/server/src/main/java/org/elasticsearch/index/query/InnerHitBuilder.java @@ -356,7 +356,7 @@ public InnerHitBuilder addDocValueField(String field, String format) { if (docValueFields == null || docValueFields.isEmpty()) { docValueFields = new ArrayList<>(); } - docValueFields.add(new FieldAndFormat(field, format)); + docValueFields.add(new FieldAndFormat(field, format, null)); return this; } @@ -393,12 +393,23 @@ public InnerHitBuilder addFetchField(String name) { * Adds a field to load and return as part of the search request. * @param name the field name. * @param format an optional format string used when formatting values, for example a date format. + * @param includeUnmapped whether unmapped fields should be returned as well */ public InnerHitBuilder addFetchField(String name, @Nullable String format) { + return addFetchField(name, format, false); + } + + /** + * Adds a field to load and return as part of the search request. + * @param name the field name. + * @param format an optional format string used when formatting values, for example a date format. + * @param includeUnmapped whether unmapped fields should be returned as well + */ + public InnerHitBuilder addFetchField(String name, @Nullable String format, boolean includeUnmapped) { if (fetchFields == null || fetchFields.isEmpty()) { fetchFields = new ArrayList<>(); } - fetchFields.add(new FieldAndFormat(name, format)); + fetchFields.add(new FieldAndFormat(name, format, includeUnmapped)); return this; } diff --git a/server/src/main/java/org/elasticsearch/index/query/InnerHitContextBuilder.java b/server/src/main/java/org/elasticsearch/index/query/InnerHitContextBuilder.java index f95bc84cd28cd..26518428763b8 100644 --- a/server/src/main/java/org/elasticsearch/index/query/InnerHitContextBuilder.java +++ b/server/src/main/java/org/elasticsearch/index/query/InnerHitContextBuilder.java @@ -93,7 +93,7 @@ protected void setupInnerHitsContext(QueryShardContext queryShardContext, innerHitsContext.docValuesContext(docValuesContext); } if (innerHitBuilder.getFetchFields() != null) { - FetchFieldsContext fieldsContext = new FetchFieldsContext(innerHitBuilder.getFetchFields(), false); + FetchFieldsContext fieldsContext = new FetchFieldsContext(innerHitBuilder.getFetchFields()); innerHitsContext.fetchFieldsContext(fieldsContext); } if (innerHitBuilder.getScriptFields() != null) { diff --git a/server/src/main/java/org/elasticsearch/search/SearchService.java b/server/src/main/java/org/elasticsearch/search/SearchService.java index 29947c66024be..d44445ac811d5 100644 --- a/server/src/main/java/org/elasticsearch/search/SearchService.java +++ b/server/src/main/java/org/elasticsearch/search/SearchService.java @@ -973,8 +973,7 @@ private void parseSource(DefaultSearchContext context, SearchSourceBuilder sourc context.docValuesContext(docValuesContext); } if (source.fetchFields() != null) { - // TODO make "includeUnmapped configurable? - FetchFieldsContext fetchFieldsContext = new FetchFieldsContext(source.fetchFields(), true); + FetchFieldsContext fetchFieldsContext = new FetchFieldsContext(source.fetchFields()); context.fetchFieldsContext(fetchFieldsContext); } if (source.highlighter() != null) { diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/TopHitsAggregationBuilder.java b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/TopHitsAggregationBuilder.java index c1174ac0d1d81..a04379121be6c 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/TopHitsAggregationBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/TopHitsAggregationBuilder.java @@ -424,7 +424,7 @@ public TopHitsAggregationBuilder docValueField(String docValueField, String form if (docValueFields == null) { docValueFields = new ArrayList<>(); } - docValueFields.add(new FieldAndFormat(docValueField, format)); + docValueFields.add(new FieldAndFormat(docValueField, format, null)); return this; } @@ -446,17 +446,24 @@ public List docValueFields() { /** * Adds a field to load and return as part of the search request. */ - public TopHitsAggregationBuilder fetchField(String field, String format) { + public TopHitsAggregationBuilder fetchField(String field, String format, boolean includeUnmapped) { if (field == null) { throw new IllegalArgumentException("[fields] must not be null: [" + name + "]"); } if (fetchFields == null) { fetchFields = new ArrayList<>(); } - fetchFields.add(new FieldAndFormat(field, format)); + fetchFields.add(new FieldAndFormat(field, format, includeUnmapped)); return this; } + /** + * Adds a field to load and return as part of the search request. + */ + public TopHitsAggregationBuilder fetchField(String field, String format) { + return fetchField(field, format, false); + } + /** * Adds a field to load and return as part of the search request. */ diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/TopHitsAggregatorFactory.java b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/TopHitsAggregatorFactory.java index 9882e9cdef0db..f88dd6ce3793e 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/TopHitsAggregatorFactory.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/TopHitsAggregatorFactory.java @@ -114,7 +114,7 @@ public Aggregator createInternal(SearchContext searchContext, subSearchContext.docValuesContext(docValuesContext); } if (fetchFields != null) { - FetchFieldsContext fieldsContext = new FetchFieldsContext(fetchFields, true); + FetchFieldsContext fieldsContext = new FetchFieldsContext(fetchFields); subSearchContext.fetchFieldsContext(fieldsContext); } for (ScriptFieldsContext.ScriptField field : scriptFields) { diff --git a/server/src/main/java/org/elasticsearch/search/builder/SearchSourceBuilder.java b/server/src/main/java/org/elasticsearch/search/builder/SearchSourceBuilder.java index 461cf5456c7e6..e43f8989d2543 100644 --- a/server/src/main/java/org/elasticsearch/search/builder/SearchSourceBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/builder/SearchSourceBuilder.java @@ -828,7 +828,7 @@ public SearchSourceBuilder docValueField(String name, @Nullable String format) { if (docValueFields == null) { docValueFields = new ArrayList<>(); } - docValueFields.add(new FieldAndFormat(name, format)); + docValueFields.add(new FieldAndFormat(name, format, null)); return this; } @@ -851,7 +851,7 @@ public List fetchFields() { * Adds a field to load and return as part of the search request. */ public SearchSourceBuilder fetchField(String name) { - return fetchField(name, null); + return fetchField(name, null, null); } /** @@ -859,11 +859,11 @@ public SearchSourceBuilder fetchField(String name) { * @param name the field name. * @param format an optional format string used when formatting values, for example a date format. */ - public SearchSourceBuilder fetchField(String name, @Nullable String format) { + public SearchSourceBuilder fetchField(String name, @Nullable String format, @Nullable Boolean includeUnmapped) { if (fetchFields == null) { fetchFields = new ArrayList<>(); } - fetchFields.add(new FieldAndFormat(name, format)); + fetchFields.add(new FieldAndFormat(name, format, includeUnmapped)); return this; } diff --git a/server/src/main/java/org/elasticsearch/search/fetch/FetchContext.java b/server/src/main/java/org/elasticsearch/search/fetch/FetchContext.java index 494b90d5a3631..52c6205d36ddc 100644 --- a/server/src/main/java/org/elasticsearch/search/fetch/FetchContext.java +++ b/server/src/main/java/org/elasticsearch/search/fetch/FetchContext.java @@ -135,10 +135,10 @@ public FetchDocValuesContext docValuesContext() { if (dvContext == null) { return new FetchDocValuesContext( searchContext.getQueryShardContext(), - Collections.singletonList(new FieldAndFormat(name, null)) + Collections.singletonList(new FieldAndFormat(name, null, null)) ); } else if (searchContext.docValuesContext().fields().stream().map(ff -> ff.field).anyMatch(name::equals) == false) { - dvContext.fields().add(new FieldAndFormat(name, null)); + dvContext.fields().add(new FieldAndFormat(name, null, null)); } } return dvContext; 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 index b653b9babe7ad..bbe267d31d141 100644 --- a/server/src/main/java/org/elasticsearch/search/fetch/subphase/FetchDocValuesContext.java +++ b/server/src/main/java/org/elasticsearch/search/fetch/subphase/FetchDocValuesContext.java @@ -45,7 +45,7 @@ public FetchDocValuesContext(QueryShardContext shardContext, List fieldNames = shardContext.simpleMatchToIndexNames(field.field); for (String fieldName : fieldNames) { if (shardContext.isFieldMapped(fieldName)) { - fields.add(new FieldAndFormat(fieldName, field.format)); + fields.add(new FieldAndFormat(fieldName, field.format, field.includeUnmapped)); } } } diff --git a/server/src/main/java/org/elasticsearch/search/fetch/subphase/FetchFieldsContext.java b/server/src/main/java/org/elasticsearch/search/fetch/subphase/FetchFieldsContext.java index 098a0660e3479..29d96495e0a17 100644 --- a/server/src/main/java/org/elasticsearch/search/fetch/subphase/FetchFieldsContext.java +++ b/server/src/main/java/org/elasticsearch/search/fetch/subphase/FetchFieldsContext.java @@ -25,18 +25,12 @@ */ public class FetchFieldsContext { private final List fields; - private final boolean includeUnmapped; - public FetchFieldsContext(List fields, boolean includeUnmapped) { + public FetchFieldsContext(List fields) { this.fields = fields; - this.includeUnmapped = includeUnmapped; } public List fields() { return fields; } - - public boolean includeUnmapped() { - return includeUnmapped; - } } diff --git a/server/src/main/java/org/elasticsearch/search/fetch/subphase/FetchFieldsPhase.java b/server/src/main/java/org/elasticsearch/search/fetch/subphase/FetchFieldsPhase.java index 0c4487afe81d5..67c9f614ef913 100644 --- a/server/src/main/java/org/elasticsearch/search/fetch/subphase/FetchFieldsPhase.java +++ b/server/src/main/java/org/elasticsearch/search/fetch/subphase/FetchFieldsPhase.java @@ -53,12 +53,7 @@ public FetchSubPhaseProcessor getProcessor(FetchContext fetchContext) { "in the mappings for index [" + fetchContext.getIndexName() + "]"); } - FieldFetcher fieldFetcher = FieldFetcher.create( - fetchContext.getQueryShardContext(), - searchLookup, - fetchFieldsContext.fields(), - fetchFieldsContext.includeUnmapped() - ); + FieldFetcher fieldFetcher = FieldFetcher.create(fetchContext.getQueryShardContext(), searchLookup, fetchFieldsContext.fields()); return new FetchSubPhaseProcessor() { @Override diff --git a/server/src/main/java/org/elasticsearch/search/fetch/subphase/FieldAndFormat.java b/server/src/main/java/org/elasticsearch/search/fetch/subphase/FieldAndFormat.java index cf4edd13f5cd8..8993e8e240100 100644 --- a/server/src/main/java/org/elasticsearch/search/fetch/subphase/FieldAndFormat.java +++ b/server/src/main/java/org/elasticsearch/search/fetch/subphase/FieldAndFormat.java @@ -19,6 +19,7 @@ package org.elasticsearch.search.fetch.subphase; +import org.elasticsearch.Version; import org.elasticsearch.common.Nullable; import org.elasticsearch.common.ParseField; import org.elasticsearch.common.io.stream.StreamInput; @@ -40,14 +41,16 @@ public final class FieldAndFormat implements Writeable, ToXContentObject { private static final ParseField FIELD_FIELD = new ParseField("field"); private static final ParseField FORMAT_FIELD = new ParseField("format"); + private static final ParseField INCLUDE_UNMAPPED_FIELD = new ParseField("include_unmapped"); private static final ConstructingObjectParser PARSER = new ConstructingObjectParser<>("fetch_field_and_format", - a -> new FieldAndFormat((String) a[0], (String) a[1])); + a -> new FieldAndFormat((String) a[0], (String) a[1], (Boolean) a[2])); static { PARSER.declareString(ConstructingObjectParser.constructorArg(), FIELD_FIELD); PARSER.declareStringOrNull(ConstructingObjectParser.optionalConstructorArg(), FORMAT_FIELD); + PARSER.declareBoolean(ConstructingObjectParser.optionalConstructorArg(), INCLUDE_UNMAPPED_FIELD); } /** @@ -56,7 +59,7 @@ public final class FieldAndFormat implements Writeable, ToXContentObject { public static FieldAndFormat fromXContent(XContentParser parser) throws IOException { XContentParser.Token token = parser.currentToken(); if (token.isValue()) { - return new FieldAndFormat(parser.text(), null); + return new FieldAndFormat(parser.text(), null, null); } else { return PARSER.apply(parser, null); } @@ -69,6 +72,7 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws if (format != null) { builder.field(FORMAT_FIELD.getPreferredName(), format); } + builder.field(INCLUDE_UNMAPPED_FIELD.getPreferredName(), includeUnmapped); builder.endObject(); return builder; } @@ -79,28 +83,45 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws /** The format of the field, or {@code null} if defaults should be used. */ public final String format; + /** Whether to include unmapped fields or not. */ + public final boolean includeUnmapped; + /** Sole constructor. */ - public FieldAndFormat(String field, @Nullable String format) { + public FieldAndFormat(String field, @Nullable String format, Boolean includeUnmapped) { this.field = Objects.requireNonNull(field); this.format = format; + if (includeUnmapped != null) { + this.includeUnmapped = includeUnmapped; + } else { + this.includeUnmapped = false; + } } /** Serialization constructor. */ public FieldAndFormat(StreamInput in) throws IOException { this.field = in.readString(); format = in.readOptionalString(); + if (in.getVersion().onOrAfter(Version.CURRENT)) { + this.includeUnmapped = in.readBoolean(); + } else { + this.includeUnmapped = false; + } } @Override public void writeTo(StreamOutput out) throws IOException { out.writeString(field); out.writeOptionalString(format); + if (out.getVersion().onOrAfter(Version.CURRENT)) { + out.writeBoolean(this.includeUnmapped); + } } @Override public int hashCode() { int h = field.hashCode(); h = 31 * h + Objects.hashCode(format); + h = 31 * h + Boolean.hashCode(this.includeUnmapped); return h; } @@ -110,6 +131,6 @@ public boolean equals(Object obj) { return false; } FieldAndFormat other = (FieldAndFormat) obj; - return field.equals(other.field) && Objects.equals(format, other.format); + return field.equals(other.field) && Objects.equals(format, other.format) && includeUnmapped == other.includeUnmapped; } } 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 6a42e31d49704..9678571b12dde 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 @@ -20,6 +20,7 @@ package org.elasticsearch.search.fetch.subphase; import org.apache.lucene.index.LeafReaderContext; +import org.apache.lucene.util.automaton.Automata; import org.apache.lucene.util.automaton.CharacterRunAutomaton; import org.elasticsearch.common.document.DocumentField; import org.elasticsearch.common.regex.Regex; @@ -45,17 +46,18 @@ public class FieldFetcher { public static FieldFetcher create(QueryShardContext context, SearchLookup searchLookup, - Collection fieldAndFormats, - boolean includeUnmapped) { + Collection fieldAndFormats) { List fieldContexts = new ArrayList<>(); - String[] originalPattern = new String[fieldAndFormats.size()]; + List unmappedFetchPattern = new ArrayList<>(); Set mappedToExclude = new HashSet<>(); int i = 0; for (FieldAndFormat fieldAndFormat : fieldAndFormats) { String fieldPattern = fieldAndFormat.field; - originalPattern[i] = fieldAndFormat.field; + if (fieldAndFormat.includeUnmapped) { + unmappedFetchPattern.add(fieldAndFormat.field); + } i++; String format = fieldAndFormat.format; @@ -70,24 +72,26 @@ public static FieldFetcher create(QueryShardContext context, fieldContexts.add(new FieldContext(field, valueFetcher)); } } - CharacterRunAutomaton pathAutomaton = new CharacterRunAutomaton(Regex.simpleMatchToAutomaton(originalPattern)); - return new FieldFetcher(fieldContexts, includeUnmapped, pathAutomaton, mappedToExclude); + CharacterRunAutomaton unmappedFetchAutomaton = new CharacterRunAutomaton(Automata.makeEmpty()); + if (unmappedFetchPattern.isEmpty() == false) { + unmappedFetchAutomaton = new CharacterRunAutomaton( + Regex.simpleMatchToAutomaton(unmappedFetchPattern.toArray(new String[unmappedFetchPattern.size()])) + ); + } + return new FieldFetcher(fieldContexts, unmappedFetchAutomaton, mappedToExclude); } private final List fieldContexts; - private final boolean includeUnmapped; - private final CharacterRunAutomaton pathAutomaton; + private final CharacterRunAutomaton unmappedFetchAutomaton; private final Set mappedToExclude; private FieldFetcher( List fieldContexts, - boolean includeUnmapped, - CharacterRunAutomaton pathAutomaton, + CharacterRunAutomaton unmappedFetchAutomaton, Set mappedToExclude ) { this.fieldContexts = fieldContexts; - this.includeUnmapped = includeUnmapped; - this.pathAutomaton = pathAutomaton; + this.unmappedFetchAutomaton = unmappedFetchAutomaton; this.mappedToExclude = mappedToExclude; } @@ -106,33 +110,36 @@ public Map fetch(SourceLookup sourceLookup, Set i documentFields.put(field, new DocumentField(field, parsedValues)); } } - if (includeUnmapped) { - collect(documentFields, sourceLookup.loadSourceIfNeeded(), "", 0); - } + collectUnmapped(documentFields, sourceLookup.loadSourceIfNeeded(), "", 0); return documentFields; } - private void collect(Map documentFields, Map source, String parentPath, int lastState) { + private void collectUnmapped(Map documentFields, Map source, String parentPath, int lastState) { for (String key : source.keySet()) { Object value = source.get(key); String currentPath = parentPath + key; - int currentState = step(this.pathAutomaton, key, lastState); + int currentState = step(this.unmappedFetchAutomaton, key, lastState); if (currentState == -1) { - // path doesn't match any fields pattern + // current path doesn't match any fields pattern continue; } if (value instanceof Map) { // one step deeper into source tree - collect(documentFields, (Map) value, currentPath + ".", step(this.pathAutomaton, ".", currentState)); + collectUnmapped( + documentFields, + (Map) value, + currentPath + ".", + step(this.unmappedFetchAutomaton, ".", currentState) + ); } else if (value instanceof List) { // iterate through list values - List list = collectList(documentFields, (List) value, currentPath, currentState); + List list = collectUnmappedList(documentFields, (List) value, currentPath, currentState); if (list.isEmpty() == false) { documentFields.put(currentPath, new DocumentField(currentPath, list)); } } else { // we have a leaf value - if (this.pathAutomaton.isAccept(currentState) && this.mappedToExclude.contains(currentPath) == false) { + if (this.unmappedFetchAutomaton.isAccept(currentState) && this.mappedToExclude.contains(currentPath) == false) { if (value != null) { DocumentField currentEntry = documentFields.get(currentPath); if (currentEntry == null) { @@ -148,15 +155,25 @@ private void collect(Map documentFields, Map collectList(Map documentFields, Iterable iterable, String parentPath, int lastState) { + private List collectUnmappedList( + Map documentFields, + Iterable iterable, + String parentPath, + int lastState + ) { List list = new ArrayList<>(); for (Object value : iterable) { if (value instanceof Map) { - collect(documentFields, (Map) value, parentPath + ".", step(this.pathAutomaton, ".", lastState)); + collectUnmapped( + documentFields, + (Map) value, + parentPath + ".", + step(this.unmappedFetchAutomaton, ".", lastState) + ); } else if (value instanceof List) { // weird case, but can happen for objects with "enabled" : "false" - list.add(collectList(documentFields, (List) value, parentPath, lastState)); - } else if (this.pathAutomaton.isAccept(lastState)) { + list.add(collectUnmappedList(documentFields, (List) value, parentPath, lastState)); + } else if (this.unmappedFetchAutomaton.isAccept(lastState)) { list.add(value); } } diff --git a/server/src/test/java/org/elasticsearch/index/query/InnerHitBuilderTests.java b/server/src/test/java/org/elasticsearch/index/query/InnerHitBuilderTests.java index 8cb4b56cd34c7..8496f8b671b7f 100644 --- a/server/src/test/java/org/elasticsearch/index/query/InnerHitBuilderTests.java +++ b/server/src/test/java/org/elasticsearch/index/query/InnerHitBuilderTests.java @@ -157,9 +157,9 @@ public static InnerHitBuilder randomInnerHits() { innerHits.setStoredFieldNames(randomListStuff(16, () -> randomAlphaOfLengthBetween(1, 16))); } innerHits.setDocValueFields(randomListStuff(16, - () -> new FieldAndFormat(randomAlphaOfLengthBetween(1, 16), null))); + () -> new FieldAndFormat(randomAlphaOfLengthBetween(1, 16), null, null))); innerHits.setFetchFields(randomListStuff(16, - () -> new FieldAndFormat(randomAlphaOfLengthBetween(1, 16), null))); + () -> new FieldAndFormat(randomAlphaOfLengthBetween(1, 16), null, null))); // Random script fields deduped on their field name. Map scriptFields = new HashMap<>(); for (SearchSourceBuilder.ScriptField field: randomListStuff(16, InnerHitBuilderTests::randomScript)) { @@ -201,7 +201,7 @@ static InnerHitBuilder mutate(InnerHitBuilder original) throws IOException { modifiers.add(() -> { if (randomBoolean()) { copy.setDocValueFields(randomValueOtherThan(copy.getDocValueFields(), - () -> randomListStuff(16, () -> new FieldAndFormat(randomAlphaOfLengthBetween(1, 16), null)))); + () -> randomListStuff(16, () -> new FieldAndFormat(randomAlphaOfLengthBetween(1, 16), null, null)))); } else { copy.addDocValueField(randomAlphaOfLengthBetween(1, 16)); } @@ -209,7 +209,7 @@ static InnerHitBuilder mutate(InnerHitBuilder original) throws IOException { modifiers.add(() -> { if (randomBoolean()) { copy.setFetchFields(randomValueOtherThan(copy.getFetchFields(), - () -> randomListStuff(16, () -> new FieldAndFormat(randomAlphaOfLengthBetween(1, 16), null)))); + () -> randomListStuff(16, () -> new FieldAndFormat(randomAlphaOfLengthBetween(1, 16), null, null)))); } else { copy.addFetchField(randomAlphaOfLengthBetween(1, 16)); } @@ -299,7 +299,7 @@ public void testSetDocValueFormat() { innerHit.addDocValueField("foo"); innerHit.addDocValueField("@timestamp", "epoch_millis"); assertEquals( - Arrays.asList(new FieldAndFormat("foo", null), new FieldAndFormat("@timestamp", "epoch_millis")), + Arrays.asList(new FieldAndFormat("foo", null, null), new FieldAndFormat("@timestamp", "epoch_millis", false)), innerHit.getDocValueFields()); } @@ -308,7 +308,7 @@ public void testSetFetchFieldFormat() { innerHit.addFetchField("foo"); innerHit.addFetchField("@timestamp", "epoch_millis"); assertEquals( - Arrays.asList(new FieldAndFormat("foo", null), new FieldAndFormat("@timestamp", "epoch_millis")), + Arrays.asList(new FieldAndFormat("foo", null, null), new FieldAndFormat("@timestamp", "epoch_millis", false)), innerHit.getFetchFields()); } } 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 62de5c80e6c1c..16cf729235c2e 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 @@ -34,6 +34,7 @@ import org.elasticsearch.test.ESSingleNodeTestCase; import java.io.IOException; +import java.util.Collections; import java.util.List; import java.util.Map; import java.util.Set; @@ -55,9 +56,9 @@ public void testLeafValues() throws IOException { .endObject(); List fieldAndFormats = List.of( - new FieldAndFormat("field", null), - new FieldAndFormat("object.field", null)); - Map fields = fetchFields(mapperService, source, fieldAndFormats, false); + new FieldAndFormat("field", null, null), + new FieldAndFormat("object.field", null, null)); + Map fields = fetchFields(mapperService, source, fieldAndFormats); assertThat(fields.size(), equalTo(2)); DocumentField field = fields.get("field"); @@ -218,8 +219,8 @@ public void testDateFormat() throws IOException { .endObject(); Map fields = fetchFields(mapperService, source, List.of( - new FieldAndFormat("field", null), - new FieldAndFormat("date_field", "yyyy/MM/dd")), false); + new FieldAndFormat("field", null, null), + new FieldAndFormat("date_field", "yyyy/MM/dd", null))); assertThat(fields.size(), equalTo(2)); DocumentField field = fields.get("field"); @@ -547,7 +548,6 @@ public void testUnmappedFieldsInsideDisabledObject() throws IOException { .endObject(); Map fields = fetchFields(mapperService, source, "*", false); - // without unmapped fields this should return nothing assertThat(fields.size(), equalTo(0)); @@ -590,14 +590,14 @@ public void testMappedFieldNotOverwritten() throws IOException { .endObject(); // this should not return a field bc. f1 is in the ignored fields - Map fields = fetchFields(mapperService, source, List.of(new FieldAndFormat("*", null)), false, Set.of("f1")); + Map fields = fetchFields(mapperService, source, List.of(new FieldAndFormat("*", null, true)), Set.of("f1")); assertThat(fields.size(), equalTo(0)); // and this should neither - fields = fetchFields(mapperService, source, List.of(new FieldAndFormat("*", null)), true, Set.of("f1")); + fields = fetchFields(mapperService, source, List.of(new FieldAndFormat("*", null, true)), Set.of("f1")); assertThat(fields.size(), equalTo(0)); - fields = fetchFields(mapperService, source, List.of(new FieldAndFormat("f1", null)), true, Set.of("f1")); + fields = fetchFields(mapperService, source, List.of(new FieldAndFormat("f1", null, true)), Set.of("f1")); assertThat(fields.size(), equalTo(0)); } @@ -636,45 +636,41 @@ public void testUnmappedFieldsWildcard() throws IOException { private Map fetchFields( MapperService mapperService, XContentBuilder source, - String fieldPattern + String fieldPattern, + boolean includeUnmapped ) throws IOException { - - List fields = List.of(new FieldAndFormat(fieldPattern, null)); - return fetchFields(mapperService, source, fields, false); + List fields = List.of(new FieldAndFormat(fieldPattern, null, includeUnmapped)); + return fetchFields(mapperService, source, fields, Collections.emptySet()); } private Map fetchFields( MapperService mapperService, XContentBuilder source, - String fieldPattern, - boolean includeUnmapped + String fieldPattern ) throws IOException { - List fields = List.of(new FieldAndFormat(fieldPattern, null)); - return fetchFields(mapperService, source, fields, includeUnmapped); + List fields = List.of(new FieldAndFormat(fieldPattern, null, false)); + return fetchFields(mapperService, source, fields); } private static Map fetchFields( MapperService mapperService, XContentBuilder source, - List fields, - boolean includeUnmapped + List fields ) throws IOException { - - return fetchFields(mapperService, source, fields, includeUnmapped, Set.of()); + return fetchFields(mapperService, source, fields, Collections.emptySet()); } private static Map fetchFields( MapperService mapperService, XContentBuilder source, List fields, - boolean includeUnmapped, Set ignoreFields ) throws IOException { SourceLookup sourceLookup = new SourceLookup(); sourceLookup.setSource(BytesReference.bytes(source)); - FieldFetcher fieldFetcher = FieldFetcher.create(createQueryShardContext(mapperService), null, fields, includeUnmapped); + FieldFetcher fieldFetcher = FieldFetcher.create(createQueryShardContext(mapperService), null, fields); return fieldFetcher.fetch(sourceLookup, ignoreFields); } From 7ec4341843b745c06147a4acc1dcc6ca8bedf8af Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christoph=20B=C3=BCscher?= Date: Thu, 12 Nov 2020 16:41:47 +0100 Subject: [PATCH 08/10] iter flag --- .../test/search/330_fetch_fields.yml | 6 +++--- .../index/query/InnerHitBuilder.java | 3 +-- .../metrics/TopHitsAggregationBuilder.java | 2 +- .../search/builder/SearchSourceBuilder.java | 2 +- .../search/fetch/FetchContext.java | 4 ++-- .../search/fetch/subphase/FieldAndFormat.java | 17 ++++++++--------- .../index/query/InnerHitBuilderTests.java | 12 ++++++------ .../fetch/subphase/FieldFetcherTests.java | 8 ++++---- .../rest-api-spec/test/flattened/10_basic.yml | 2 +- 9 files changed, 27 insertions(+), 29 deletions(-) diff --git a/rest-api-spec/src/main/resources/rest-api-spec/test/search/330_fetch_fields.yml b/rest-api-spec/src/main/resources/rest-api-spec/test/search/330_fetch_fields.yml index 3ab32720ed1c6..ae91f59da41c3 100644 --- a/rest-api-spec/src/main/resources/rest-api-spec/test/search/330_fetch_fields.yml +++ b/rest-api-spec/src/main/resources/rest-api-spec/test/search/330_fetch_fields.yml @@ -333,7 +333,7 @@ Test unmapped field: body: fields: - f1 - - f4 + - { "field" : "f4", "include_unmapped" : true } - match: hits.hits.0.fields.f1: - some text @@ -345,7 +345,7 @@ Test unmapped field: index: test body: fields: - - f* + - { "field" : "f*", "include_unmapped" : true } - match: hits.hits.0.fields.f1: - some text @@ -392,7 +392,7 @@ Test unmapped fields inside disabled objects: search: index: test body: - fields: ["*"] + fields: [ { "field" : "*", "include_unmapped" : true } ] - match: hits.hits.0.fields.f1: - some text diff --git a/server/src/main/java/org/elasticsearch/index/query/InnerHitBuilder.java b/server/src/main/java/org/elasticsearch/index/query/InnerHitBuilder.java index 61f6bea6c9072..89d54cb545e8b 100644 --- a/server/src/main/java/org/elasticsearch/index/query/InnerHitBuilder.java +++ b/server/src/main/java/org/elasticsearch/index/query/InnerHitBuilder.java @@ -356,7 +356,7 @@ public InnerHitBuilder addDocValueField(String field, String format) { if (docValueFields == null || docValueFields.isEmpty()) { docValueFields = new ArrayList<>(); } - docValueFields.add(new FieldAndFormat(field, format, null)); + docValueFields.add(new FieldAndFormat(field, format)); return this; } @@ -393,7 +393,6 @@ public InnerHitBuilder addFetchField(String name) { * Adds a field to load and return as part of the search request. * @param name the field name. * @param format an optional format string used when formatting values, for example a date format. - * @param includeUnmapped whether unmapped fields should be returned as well */ public InnerHitBuilder addFetchField(String name, @Nullable String format) { return addFetchField(name, format, false); diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/TopHitsAggregationBuilder.java b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/TopHitsAggregationBuilder.java index a04379121be6c..4d26d6f55e90e 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/TopHitsAggregationBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/TopHitsAggregationBuilder.java @@ -424,7 +424,7 @@ public TopHitsAggregationBuilder docValueField(String docValueField, String form if (docValueFields == null) { docValueFields = new ArrayList<>(); } - docValueFields.add(new FieldAndFormat(docValueField, format, null)); + docValueFields.add(new FieldAndFormat(docValueField, format)); return this; } diff --git a/server/src/main/java/org/elasticsearch/search/builder/SearchSourceBuilder.java b/server/src/main/java/org/elasticsearch/search/builder/SearchSourceBuilder.java index 116ad341122dd..80e8be64bea45 100644 --- a/server/src/main/java/org/elasticsearch/search/builder/SearchSourceBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/builder/SearchSourceBuilder.java @@ -845,7 +845,7 @@ public SearchSourceBuilder docValueField(String name, @Nullable String format) { if (docValueFields == null) { docValueFields = new ArrayList<>(); } - docValueFields.add(new FieldAndFormat(name, format, null)); + docValueFields.add(new FieldAndFormat(name, format)); return this; } diff --git a/server/src/main/java/org/elasticsearch/search/fetch/FetchContext.java b/server/src/main/java/org/elasticsearch/search/fetch/FetchContext.java index 52c6205d36ddc..494b90d5a3631 100644 --- a/server/src/main/java/org/elasticsearch/search/fetch/FetchContext.java +++ b/server/src/main/java/org/elasticsearch/search/fetch/FetchContext.java @@ -135,10 +135,10 @@ public FetchDocValuesContext docValuesContext() { if (dvContext == null) { return new FetchDocValuesContext( searchContext.getQueryShardContext(), - Collections.singletonList(new FieldAndFormat(name, null, null)) + Collections.singletonList(new FieldAndFormat(name, null)) ); } else if (searchContext.docValuesContext().fields().stream().map(ff -> ff.field).anyMatch(name::equals) == false) { - dvContext.fields().add(new FieldAndFormat(name, null, null)); + dvContext.fields().add(new FieldAndFormat(name, null)); } } return dvContext; diff --git a/server/src/main/java/org/elasticsearch/search/fetch/subphase/FieldAndFormat.java b/server/src/main/java/org/elasticsearch/search/fetch/subphase/FieldAndFormat.java index 8993e8e240100..0a35172fb88e7 100644 --- a/server/src/main/java/org/elasticsearch/search/fetch/subphase/FieldAndFormat.java +++ b/server/src/main/java/org/elasticsearch/search/fetch/subphase/FieldAndFormat.java @@ -45,7 +45,7 @@ public final class FieldAndFormat implements Writeable, ToXContentObject { private static final ConstructingObjectParser PARSER = new ConstructingObjectParser<>("fetch_field_and_format", - a -> new FieldAndFormat((String) a[0], (String) a[1], (Boolean) a[2])); + a -> new FieldAndFormat((String) a[0], (String) a[1], a[2] != null ? (Boolean) a[2] : false)); static { PARSER.declareString(ConstructingObjectParser.constructorArg(), FIELD_FIELD); @@ -59,7 +59,7 @@ public final class FieldAndFormat implements Writeable, ToXContentObject { public static FieldAndFormat fromXContent(XContentParser parser) throws IOException { XContentParser.Token token = parser.currentToken(); if (token.isValue()) { - return new FieldAndFormat(parser.text(), null, null); + return new FieldAndFormat(parser.text(), null, false); } else { return PARSER.apply(parser, null); } @@ -86,15 +86,14 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws /** Whether to include unmapped fields or not. */ public final boolean includeUnmapped; - /** Sole constructor. */ - public FieldAndFormat(String field, @Nullable String format, Boolean includeUnmapped) { + public FieldAndFormat(String field, @Nullable String format) { + this(field, format, false); + } + + public FieldAndFormat(String field, @Nullable String format, boolean includeUnmapped) { this.field = Objects.requireNonNull(field); this.format = format; - if (includeUnmapped != null) { - this.includeUnmapped = includeUnmapped; - } else { - this.includeUnmapped = false; - } + this.includeUnmapped = includeUnmapped; } /** Serialization constructor. */ diff --git a/server/src/test/java/org/elasticsearch/index/query/InnerHitBuilderTests.java b/server/src/test/java/org/elasticsearch/index/query/InnerHitBuilderTests.java index 8496f8b671b7f..8cb4b56cd34c7 100644 --- a/server/src/test/java/org/elasticsearch/index/query/InnerHitBuilderTests.java +++ b/server/src/test/java/org/elasticsearch/index/query/InnerHitBuilderTests.java @@ -157,9 +157,9 @@ public static InnerHitBuilder randomInnerHits() { innerHits.setStoredFieldNames(randomListStuff(16, () -> randomAlphaOfLengthBetween(1, 16))); } innerHits.setDocValueFields(randomListStuff(16, - () -> new FieldAndFormat(randomAlphaOfLengthBetween(1, 16), null, null))); + () -> new FieldAndFormat(randomAlphaOfLengthBetween(1, 16), null))); innerHits.setFetchFields(randomListStuff(16, - () -> new FieldAndFormat(randomAlphaOfLengthBetween(1, 16), null, null))); + () -> new FieldAndFormat(randomAlphaOfLengthBetween(1, 16), null))); // Random script fields deduped on their field name. Map scriptFields = new HashMap<>(); for (SearchSourceBuilder.ScriptField field: randomListStuff(16, InnerHitBuilderTests::randomScript)) { @@ -201,7 +201,7 @@ static InnerHitBuilder mutate(InnerHitBuilder original) throws IOException { modifiers.add(() -> { if (randomBoolean()) { copy.setDocValueFields(randomValueOtherThan(copy.getDocValueFields(), - () -> randomListStuff(16, () -> new FieldAndFormat(randomAlphaOfLengthBetween(1, 16), null, null)))); + () -> randomListStuff(16, () -> new FieldAndFormat(randomAlphaOfLengthBetween(1, 16), null)))); } else { copy.addDocValueField(randomAlphaOfLengthBetween(1, 16)); } @@ -209,7 +209,7 @@ static InnerHitBuilder mutate(InnerHitBuilder original) throws IOException { modifiers.add(() -> { if (randomBoolean()) { copy.setFetchFields(randomValueOtherThan(copy.getFetchFields(), - () -> randomListStuff(16, () -> new FieldAndFormat(randomAlphaOfLengthBetween(1, 16), null, null)))); + () -> randomListStuff(16, () -> new FieldAndFormat(randomAlphaOfLengthBetween(1, 16), null)))); } else { copy.addFetchField(randomAlphaOfLengthBetween(1, 16)); } @@ -299,7 +299,7 @@ public void testSetDocValueFormat() { innerHit.addDocValueField("foo"); innerHit.addDocValueField("@timestamp", "epoch_millis"); assertEquals( - Arrays.asList(new FieldAndFormat("foo", null, null), new FieldAndFormat("@timestamp", "epoch_millis", false)), + Arrays.asList(new FieldAndFormat("foo", null), new FieldAndFormat("@timestamp", "epoch_millis")), innerHit.getDocValueFields()); } @@ -308,7 +308,7 @@ public void testSetFetchFieldFormat() { innerHit.addFetchField("foo"); innerHit.addFetchField("@timestamp", "epoch_millis"); assertEquals( - Arrays.asList(new FieldAndFormat("foo", null, null), new FieldAndFormat("@timestamp", "epoch_millis", false)), + Arrays.asList(new FieldAndFormat("foo", null), new FieldAndFormat("@timestamp", "epoch_millis")), innerHit.getFetchFields()); } } 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 16cf729235c2e..b733f8a810af1 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 @@ -56,8 +56,8 @@ public void testLeafValues() throws IOException { .endObject(); List fieldAndFormats = List.of( - new FieldAndFormat("field", null, null), - new FieldAndFormat("object.field", null, null)); + new FieldAndFormat("field", null), + new FieldAndFormat("object.field", null)); Map fields = fetchFields(mapperService, source, fieldAndFormats); assertThat(fields.size(), equalTo(2)); @@ -219,8 +219,8 @@ public void testDateFormat() throws IOException { .endObject(); Map fields = fetchFields(mapperService, source, List.of( - new FieldAndFormat("field", null, null), - new FieldAndFormat("date_field", "yyyy/MM/dd", null))); + new FieldAndFormat("field", null), + new FieldAndFormat("date_field", "yyyy/MM/dd"))); assertThat(fields.size(), equalTo(2)); DocumentField field = fields.get("field"); diff --git a/x-pack/plugin/src/test/resources/rest-api-spec/test/flattened/10_basic.yml b/x-pack/plugin/src/test/resources/rest-api-spec/test/flattened/10_basic.yml index c2f4aedc4e5e2..29739b1d6a849 100644 --- a/x-pack/plugin/src/test/resources/rest-api-spec/test/flattened/10_basic.yml +++ b/x-pack/plugin/src/test/resources/rest-api-spec/test/flattened/10_basic.yml @@ -150,7 +150,7 @@ search: index: test body: - fields: ["flat*"] + fields: [ { "field" : "flat*", "include_unmapped" : true } ] - match: { hits.total.value: 1 } - length: { hits.hits: 1 } From 4d74f2d6fdfb1202c739131ba88a181e1eb9bbaf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christoph=20B=C3=BCscher?= Date: Thu, 12 Nov 2020 17:18:05 +0100 Subject: [PATCH 09/10] fix test --- docs/reference/sql/endpoints/translate.asciidoc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/docs/reference/sql/endpoints/translate.asciidoc b/docs/reference/sql/endpoints/translate.asciidoc index fdccbf00956b4..4074795addaa5 100644 --- a/docs/reference/sql/endpoints/translate.asciidoc +++ b/docs/reference/sql/endpoints/translate.asciidoc @@ -25,7 +25,8 @@ Which returns: "docvalue_fields": [ { "field": "release_date", - "format": "epoch_millis" + "format": "epoch_millis", + "include_unmapped" : false } ], "_source": { From 4b5232b25a7dd5f15819c84dcadfd1abbfaf34d3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christoph=20B=C3=BCscher?= Date: Thu, 12 Nov 2020 17:40:21 +0100 Subject: [PATCH 10/10] Make flag optional --- .../sql/endpoints/translate.asciidoc | 3 +-- .../action/search/ExpandSearchPhase.java | 2 +- .../index/query/InnerHitBuilder.java | 3 ++- .../metrics/TopHitsAggregationBuilder.java | 2 +- .../search/builder/SearchSourceBuilder.java | 3 ++- .../search/fetch/subphase/FieldAndFormat.java | 25 +++++++++++-------- .../search/fetch/subphase/FieldFetcher.java | 2 +- .../fetch/subphase/FieldFetcherTests.java | 16 ++++++++---- 8 files changed, 33 insertions(+), 23 deletions(-) diff --git a/docs/reference/sql/endpoints/translate.asciidoc b/docs/reference/sql/endpoints/translate.asciidoc index 4074795addaa5..fdccbf00956b4 100644 --- a/docs/reference/sql/endpoints/translate.asciidoc +++ b/docs/reference/sql/endpoints/translate.asciidoc @@ -25,8 +25,7 @@ Which returns: "docvalue_fields": [ { "field": "release_date", - "format": "epoch_millis", - "include_unmapped" : false + "format": "epoch_millis" } ], "_source": { diff --git a/server/src/main/java/org/elasticsearch/action/search/ExpandSearchPhase.java b/server/src/main/java/org/elasticsearch/action/search/ExpandSearchPhase.java index ef09c2e751443..500e9df5c54ac 100644 --- a/server/src/main/java/org/elasticsearch/action/search/ExpandSearchPhase.java +++ b/server/src/main/java/org/elasticsearch/action/search/ExpandSearchPhase.java @@ -138,7 +138,7 @@ private SearchSourceBuilder buildExpandSearchSourceBuilder(InnerHitBuilder optio } } if (options.getFetchFields() != null) { - options.getFetchFields().forEach(ff -> groupSource.fetchField(ff.field, ff.format, ff.includeUnmapped)); + options.getFetchFields().forEach(ff -> groupSource.fetchField(ff.field, ff.format, ff.includeUnmapped.orElse(null))); } if (options.getDocValueFields() != null) { options.getDocValueFields().forEach(ff -> groupSource.docValueField(ff.field, ff.format)); diff --git a/server/src/main/java/org/elasticsearch/index/query/InnerHitBuilder.java b/server/src/main/java/org/elasticsearch/index/query/InnerHitBuilder.java index 89d54cb545e8b..99deb71eb9bd1 100644 --- a/server/src/main/java/org/elasticsearch/index/query/InnerHitBuilder.java +++ b/server/src/main/java/org/elasticsearch/index/query/InnerHitBuilder.java @@ -47,6 +47,7 @@ import java.util.Iterator; import java.util.List; import java.util.Objects; +import java.util.Optional; import java.util.Set; import static org.elasticsearch.common.xcontent.XContentParser.Token.END_OBJECT; @@ -408,7 +409,7 @@ public InnerHitBuilder addFetchField(String name, @Nullable String format, boole if (fetchFields == null || fetchFields.isEmpty()) { fetchFields = new ArrayList<>(); } - fetchFields.add(new FieldAndFormat(name, format, includeUnmapped)); + fetchFields.add(new FieldAndFormat(name, format, Optional.of(includeUnmapped))); return this; } diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/TopHitsAggregationBuilder.java b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/TopHitsAggregationBuilder.java index 4d26d6f55e90e..2125bc5ba809b 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/TopHitsAggregationBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/TopHitsAggregationBuilder.java @@ -453,7 +453,7 @@ public TopHitsAggregationBuilder fetchField(String field, String format, boolean if (fetchFields == null) { fetchFields = new ArrayList<>(); } - fetchFields.add(new FieldAndFormat(field, format, includeUnmapped)); + fetchFields.add(new FieldAndFormat(field, format, Optional.of(includeUnmapped))); return this; } diff --git a/server/src/main/java/org/elasticsearch/search/builder/SearchSourceBuilder.java b/server/src/main/java/org/elasticsearch/search/builder/SearchSourceBuilder.java index 80e8be64bea45..9dfd90eeab6fe 100644 --- a/server/src/main/java/org/elasticsearch/search/builder/SearchSourceBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/builder/SearchSourceBuilder.java @@ -65,6 +65,7 @@ import java.util.List; import java.util.Map; import java.util.Objects; +import java.util.Optional; import static java.util.Collections.emptyMap; import static org.elasticsearch.index.query.AbstractQueryBuilder.parseInnerQueryBuilder; @@ -880,7 +881,7 @@ public SearchSourceBuilder fetchField(String name, @Nullable String format, @Nul if (fetchFields == null) { fetchFields = new ArrayList<>(); } - fetchFields.add(new FieldAndFormat(name, format, includeUnmapped)); + fetchFields.add(new FieldAndFormat(name, format, Optional.ofNullable(includeUnmapped))); return this; } diff --git a/server/src/main/java/org/elasticsearch/search/fetch/subphase/FieldAndFormat.java b/server/src/main/java/org/elasticsearch/search/fetch/subphase/FieldAndFormat.java index 0a35172fb88e7..dd0a3ab5049c1 100644 --- a/server/src/main/java/org/elasticsearch/search/fetch/subphase/FieldAndFormat.java +++ b/server/src/main/java/org/elasticsearch/search/fetch/subphase/FieldAndFormat.java @@ -33,6 +33,7 @@ import java.io.IOException; import java.util.Objects; +import java.util.Optional; /** * Wrapper around a field name and the format that should be used to @@ -45,7 +46,7 @@ public final class FieldAndFormat implements Writeable, ToXContentObject { private static final ConstructingObjectParser PARSER = new ConstructingObjectParser<>("fetch_field_and_format", - a -> new FieldAndFormat((String) a[0], (String) a[1], a[2] != null ? (Boolean) a[2] : false)); + a -> new FieldAndFormat((String) a[0], (String) a[1], Optional.ofNullable((Boolean) a[2]))); static { PARSER.declareString(ConstructingObjectParser.constructorArg(), FIELD_FIELD); @@ -59,7 +60,7 @@ public final class FieldAndFormat implements Writeable, ToXContentObject { public static FieldAndFormat fromXContent(XContentParser parser) throws IOException { XContentParser.Token token = parser.currentToken(); if (token.isValue()) { - return new FieldAndFormat(parser.text(), null, false); + return new FieldAndFormat(parser.text(), null); } else { return PARSER.apply(parser, null); } @@ -72,7 +73,9 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws if (format != null) { builder.field(FORMAT_FIELD.getPreferredName(), format); } - builder.field(INCLUDE_UNMAPPED_FIELD.getPreferredName(), includeUnmapped); + if (this.includeUnmapped.isPresent()) { + builder.field(INCLUDE_UNMAPPED_FIELD.getPreferredName(), includeUnmapped); + } builder.endObject(); return builder; } @@ -84,13 +87,13 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws public final String format; /** Whether to include unmapped fields or not. */ - public final boolean includeUnmapped; + public final Optional includeUnmapped; public FieldAndFormat(String field, @Nullable String format) { - this(field, format, false); + this(field, format, Optional.empty()); } - public FieldAndFormat(String field, @Nullable String format, boolean includeUnmapped) { + public FieldAndFormat(String field, @Nullable String format, Optional includeUnmapped) { this.field = Objects.requireNonNull(field); this.format = format; this.includeUnmapped = includeUnmapped; @@ -101,9 +104,9 @@ public FieldAndFormat(StreamInput in) throws IOException { this.field = in.readString(); format = in.readOptionalString(); if (in.getVersion().onOrAfter(Version.CURRENT)) { - this.includeUnmapped = in.readBoolean(); + this.includeUnmapped = Optional.ofNullable(in.readOptionalBoolean()); } else { - this.includeUnmapped = false; + this.includeUnmapped = Optional.empty(); } } @@ -112,7 +115,7 @@ public void writeTo(StreamOutput out) throws IOException { out.writeString(field); out.writeOptionalString(format); if (out.getVersion().onOrAfter(Version.CURRENT)) { - out.writeBoolean(this.includeUnmapped); + out.writeOptionalBoolean(this.includeUnmapped.orElse(null)); } } @@ -120,7 +123,7 @@ public void writeTo(StreamOutput out) throws IOException { public int hashCode() { int h = field.hashCode(); h = 31 * h + Objects.hashCode(format); - h = 31 * h + Boolean.hashCode(this.includeUnmapped); + h = 31 * h + Boolean.hashCode(this.includeUnmapped.orElse(false)); return h; } @@ -130,6 +133,6 @@ public boolean equals(Object obj) { return false; } FieldAndFormat other = (FieldAndFormat) obj; - return field.equals(other.field) && Objects.equals(format, other.format) && includeUnmapped == other.includeUnmapped; + return field.equals(other.field) && Objects.equals(format, other.format) && Objects.equals(includeUnmapped,other.includeUnmapped); } } 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 9678571b12dde..12f82b15b49ee 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 @@ -55,7 +55,7 @@ public static FieldFetcher create(QueryShardContext context, for (FieldAndFormat fieldAndFormat : fieldAndFormats) { String fieldPattern = fieldAndFormat.field; - if (fieldAndFormat.includeUnmapped) { + if (fieldAndFormat.includeUnmapped.orElse(false)) { unmappedFetchPattern.add(fieldAndFormat.field); } i++; 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 b733f8a810af1..d214f41f6dd78 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 @@ -37,6 +37,7 @@ import java.util.Collections; import java.util.List; import java.util.Map; +import java.util.Optional; import java.util.Set; import static org.hamcrest.Matchers.containsInAnyOrder; @@ -590,14 +591,19 @@ public void testMappedFieldNotOverwritten() throws IOException { .endObject(); // this should not return a field bc. f1 is in the ignored fields - Map fields = fetchFields(mapperService, source, List.of(new FieldAndFormat("*", null, true)), Set.of("f1")); + Map fields = fetchFields( + mapperService, + source, + List.of(new FieldAndFormat("*", null, Optional.of(true))), + Set.of("f1") + ); assertThat(fields.size(), equalTo(0)); // and this should neither - fields = fetchFields(mapperService, source, List.of(new FieldAndFormat("*", null, true)), Set.of("f1")); + fields = fetchFields(mapperService, source, List.of(new FieldAndFormat("*", null, Optional.of(true))), Set.of("f1")); assertThat(fields.size(), equalTo(0)); - fields = fetchFields(mapperService, source, List.of(new FieldAndFormat("f1", null, true)), Set.of("f1")); + fields = fetchFields(mapperService, source, List.of(new FieldAndFormat("f1", null, Optional.of(true))), Set.of("f1")); assertThat(fields.size(), equalTo(0)); } @@ -639,7 +645,7 @@ private Map fetchFields( String fieldPattern, boolean includeUnmapped ) throws IOException { - List fields = List.of(new FieldAndFormat(fieldPattern, null, includeUnmapped)); + List fields = List.of(new FieldAndFormat(fieldPattern, null, Optional.of(includeUnmapped))); return fetchFields(mapperService, source, fields, Collections.emptySet()); } @@ -648,7 +654,7 @@ private Map fetchFields( XContentBuilder source, String fieldPattern ) throws IOException { - List fields = List.of(new FieldAndFormat(fieldPattern, null, false)); + List fields = List.of(new FieldAndFormat(fieldPattern, null)); return fetchFields(mapperService, source, fields); }