From 2ee5a768c5f81ac61083bfc6ed6c012c34ecac29 Mon Sep 17 00:00:00 2001 From: markharwood Date: Tue, 5 Oct 2021 15:18:29 +0100 Subject: [PATCH 1/9] Search - return ignored field values from fields api. Since Kibana's Discover switched to retrieving values via the fields API rather than source there have been gaps in the display caused by "ignored" fields (those that fall foul of ignore_above and ignore_malformed size and formatting rules). This PR returns ignored values from source when a user-requested field fails to be parsed for a document. In these cases the corresponding hit adds a new ignored_field_values section in the response. Closes #74121 --- .../retrieve-selected-fields.asciidoc | 87 ++++++++++++++ .../extras/MatchOnlyTextFieldMapper.java | 3 +- .../mapper/extras/TokenCountFieldMapper.java | 2 +- .../join/mapper/ParentIdFieldMapper.java | 2 +- .../index/mapper/size/SizeFieldMapper.java | 2 +- .../test/search/200_ignore_malformed.yml | 17 +++ .../common/document/DocumentField.java | 108 +++++++++++++++--- .../elasticsearch/index/get/GetResult.java | 2 +- .../index/mapper/ArraySourceValueFetcher.java | 4 +- .../index/mapper/DocValueFetcher.java | 2 +- .../index/mapper/NestedValueFetcher.java | 2 +- .../index/mapper/SourceValueFetcher.java | 5 +- .../index/mapper/ValueFetcher.java | 3 +- .../index/termvectors/TermVectorsService.java | 3 +- .../org/elasticsearch/search/SearchHit.java | 15 ++- .../fetch/subphase/FetchDocValuesPhase.java | 6 +- .../search/fetch/subphase/FieldFetcher.java | 7 +- .../subphase/highlight/HighlightUtils.java | 3 +- .../search/lookup/FieldValues.java | 10 +- .../index/get/DocumentFieldTests.java | 10 +- .../KeyedFlattenedFieldTypeTests.java | 4 +- .../index/mapper/FieldTypeTestCase.java | 5 +- .../index/mapper/MapperTestCase.java | 6 +- .../mapper/DataTierFieldMapper.java | 4 +- .../mapper/DataTierFieldTypeTests.java | 7 +- .../xpack/eql/action/EqlSearchResponse.java | 2 +- .../mapper/ConstantKeywordFieldMapper.java | 4 +- .../mapper/ConstantKeywordFieldTypeTests.java | 10 +- 28 files changed, 272 insertions(+), 63 deletions(-) diff --git a/docs/reference/search/search-your-data/retrieve-selected-fields.asciidoc b/docs/reference/search/search-your-data/retrieve-selected-fields.asciidoc index 97d020e71ccf3..44a2adf45fb5e 100644 --- a/docs/reference/search/search-your-data/retrieve-selected-fields.asciidoc +++ b/docs/reference/search/search-your-data/retrieve-selected-fields.asciidoc @@ -376,6 +376,93 @@ won't be included in the response because `include_unmapped` isn't set to // TESTRESPONSE[s/"max_score" : 1.0/"max_score" : $body.hits.max_score/] // TESTRESPONSE[s/"_score" : 1.0/"_score" : $body.hits.hits.0._score/] +[discrete] +[[Ignored-field values]] +==== Ignored field values +The `fields` option only returns values that were valid when indexed. +If your search request asks for values from a field that ignored certain +because they were malformed or too large these values are returned +separately in an `ignored_field_values` section. + +In this example we index a document that has a value which is ignored and +not added to the index so is shown separately in search results: + +[source,console] +---- +PUT my-index-000001 +{ + "mappings": { + "properties": { + "my-small" : { "type" : "keyword", "ignore_above": 1 }, <1> + "my-large" : { "type" : "keyword" } + } + } +} + +PUT my-index-000001/_doc/1?refresh=true +{ + "my-small": "bad content", <2> + "my-large": "ok content" +} + +POST my-index-000001/_search +{ + "fields": ["my-*"], + "_source": false +} +---- + +<1> This field has a size restriction +<2> This document field exceeds the size restriction so is ignored and not indexed + +The response will contain ignored field results under the `ignored_field_values` path. +These values are retrieved from the document's original JSON source and are raw so will +not be formatted or treated in any way, unlike the successfully indexed fields which are +returned in the `fields` section. + +[source,console-result] +---- +{ + "took" : 2, + "timed_out" : false, + "_shards" : { + "total" : 1, + "successful" : 1, + "skipped" : 0, + "failed" : 0 + }, + "hits" : { + "total" : { + "value" : 1, + "relation" : "eq" + }, + "max_score" : 1.0, + "hits" : [ + { + "_index" : "my-index-000001", + "_id" : "1", + "_score" : 1.0, + "_ignored" : [ "my-small"], + "fields" : { + "my-large": [ + "ok content" + ] + }, + "ignored_field_values" : { + "my-small": [ + "bad content" + ] + } + } + ] + } +} +---- +// TESTRESPONSE[s/"took" : 2/"took": $body.took/] +// TESTRESPONSE[s/"max_score" : 1.0/"max_score" : $body.hits.max_score/] +// TESTRESPONSE[s/"_score" : 1.0/"_score" : $body.hits.hits.0._score/] + + [discrete] [[source-filtering]] === The `_source` option diff --git a/modules/mapper-extras/src/main/java/org/elasticsearch/index/mapper/extras/MatchOnlyTextFieldMapper.java b/modules/mapper-extras/src/main/java/org/elasticsearch/index/mapper/extras/MatchOnlyTextFieldMapper.java index b9f2a5ad9b506..cf39b9d4429bc 100644 --- a/modules/mapper-extras/src/main/java/org/elasticsearch/index/mapper/extras/MatchOnlyTextFieldMapper.java +++ b/modules/mapper-extras/src/main/java/org/elasticsearch/index/mapper/extras/MatchOnlyTextFieldMapper.java @@ -48,6 +48,7 @@ import java.io.IOException; import java.io.UncheckedIOException; +import java.util.ArrayList; import java.util.Collections; import java.util.List; import java.util.Map; @@ -178,7 +179,7 @@ private Function, IOException> return docID -> { try { sourceLookup.setSegmentAndDocument(context, docID); - return valueFetcher.fetchValues(sourceLookup); + return valueFetcher.fetchValues(sourceLookup, new ArrayList<>()); } catch (IOException e) { throw new UncheckedIOException(e); } diff --git a/modules/mapper-extras/src/main/java/org/elasticsearch/index/mapper/extras/TokenCountFieldMapper.java b/modules/mapper-extras/src/main/java/org/elasticsearch/index/mapper/extras/TokenCountFieldMapper.java index 385fcc7973ec4..6ec62adac0673 100644 --- a/modules/mapper-extras/src/main/java/org/elasticsearch/index/mapper/extras/TokenCountFieldMapper.java +++ b/modules/mapper-extras/src/main/java/org/elasticsearch/index/mapper/extras/TokenCountFieldMapper.java @@ -109,7 +109,7 @@ static class TokenCountFieldType extends NumberFieldMapper.NumberFieldType { @Override public ValueFetcher valueFetcher(SearchExecutionContext context, String format) { if (hasDocValues() == false) { - return lookup -> List.of(); + return (lookup, ignoredValues) -> List.of(); } return new DocValueFetcher(docValueFormat(format, null), context.getForField(this)); } diff --git a/modules/parent-join/src/main/java/org/elasticsearch/join/mapper/ParentIdFieldMapper.java b/modules/parent-join/src/main/java/org/elasticsearch/join/mapper/ParentIdFieldMapper.java index 87635140f6f5d..e9edb8ca1bc09 100644 --- a/modules/parent-join/src/main/java/org/elasticsearch/join/mapper/ParentIdFieldMapper.java +++ b/modules/parent-join/src/main/java/org/elasticsearch/join/mapper/ParentIdFieldMapper.java @@ -76,7 +76,7 @@ public IndexFieldData.Builder fielddataBuilder(String fullyQualifiedIndexName, S public ValueFetcher valueFetcher(SearchExecutionContext context, String format) { // Although this is an internal field, we return it in the list of all field types. So we // provide an empty value fetcher here instead of throwing an error. - return lookup -> List.of(); + return (lookup, ignoredValues) -> List.of(); } @Override diff --git a/plugins/mapper-size/src/main/java/org/elasticsearch/index/mapper/size/SizeFieldMapper.java b/plugins/mapper-size/src/main/java/org/elasticsearch/index/mapper/size/SizeFieldMapper.java index f9334ba7c5a74..d2e3c6e8b0e0d 100644 --- a/plugins/mapper-size/src/main/java/org/elasticsearch/index/mapper/size/SizeFieldMapper.java +++ b/plugins/mapper-size/src/main/java/org/elasticsearch/index/mapper/size/SizeFieldMapper.java @@ -57,7 +57,7 @@ private static class SizeFieldType extends NumberFieldType { @Override public ValueFetcher valueFetcher(SearchExecutionContext context, String format) { if (hasDocValues() == false) { - return lookup -> List.of(); + return (lookup, ignoredValues) -> List.of(); } return new DocValueFetcher(docValueFormat(format, null), context.getForField(this)); } diff --git a/rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/search/200_ignore_malformed.yml b/rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/search/200_ignore_malformed.yml index 162da4fc3c9e5..e48289a53909d 100644 --- a/rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/search/200_ignore_malformed.yml +++ b/rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/search/200_ignore_malformed.yml @@ -88,3 +88,20 @@ setup: - length: { hits.hits: 1 } - is_true: hits.hits.0._ignored + + +--- +"ignored_field_values is returned by default": + - skip: + version: " - 7.99.99" + reason: ignored_field_values was added in 8.0 + + - do: + search: + rest_total_hits_as_int: true + body: { query: { ids: { "values": [ "3" ] } },"_source":false, "fields":["my*"]} + + - length: { hits.hits: 1 } + - length: { hits.hits.0._ignored: 2} + - length: { hits.hits.0.ignored_field_values.my_date: 1} + - length: { hits.hits.0.ignored_field_values.my_ip: 1} diff --git a/server/src/main/java/org/elasticsearch/common/document/DocumentField.java b/server/src/main/java/org/elasticsearch/common/document/DocumentField.java index e6140c2e5ee5b..08380ceb9723d 100644 --- a/server/src/main/java/org/elasticsearch/common/document/DocumentField.java +++ b/server/src/main/java/org/elasticsearch/common/document/DocumentField.java @@ -8,6 +8,7 @@ package org.elasticsearch.common.document; +import org.elasticsearch.Version; import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.common.io.stream.StreamOutput; import org.elasticsearch.common.io.stream.Writeable; @@ -19,6 +20,7 @@ import java.io.IOException; import java.util.ArrayList; +import java.util.Collections; import java.util.Iterator; import java.util.List; import java.util.Objects; @@ -32,21 +34,34 @@ * @see SearchHit * @see GetResult */ -public class DocumentField implements Writeable, ToXContentFragment, Iterable { +//public class DocumentField implements Writeable, ToXContentFragment, Iterable { +public class DocumentField implements Writeable, Iterable { private final String name; private final List values; - + private List ignoredValues; + public DocumentField(StreamInput in) throws IOException { name = in.readString(); values = in.readList(StreamInput::readGenericValue); + if (in.getVersion().onOrAfter(Version.V_8_0_0)) { + ignoredValues = in.readList(StreamInput::readGenericValue); + } else { + ignoredValues = Collections.emptyList(); + } } public DocumentField(String name, List values) { + this(name, values, null); + } + + public DocumentField(String name, List values, List ignoredValues) { this.name = Objects.requireNonNull(name, "name must not be null"); this.values = Objects.requireNonNull(values, "values must not be null"); + this.ignoredValues = ignoredValues == null ? Collections.emptyList(): ignoredValues; } + /** * The name of the field. */ @@ -76,26 +91,82 @@ public List getValues() { public Iterator iterator() { return values.iterator(); } + + /** + * The field's ignored values as an immutable list. + */ + public List getIgnoredValues() { + return ignoredValues == Collections.emptyList() ? ignoredValues : Collections.unmodifiableList(ignoredValues); + } + /** + * The field's ignored values as an immutable list. + */ + public void addIgnoredValues(List additions) { + if (ignoredValues.isEmpty()) { + return; + } + if (ignoredValues == Collections.emptyList()) { + ignoredValues = new ArrayList<>(additions); + } else { + ignoredValues.addAll(additions); + } + } + @Override public void writeTo(StreamOutput out) throws IOException { out.writeString(name); out.writeCollection(values, StreamOutput::writeGenericValue); - } - - @Override - public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { - builder.startArray(name); - for (Object value : values) { - // This call doesn't really need to support writing any kind of object, since the values - // here are always serializable to xContent. Each value could be a leaf types like a string, - // number, or boolean, a list of such values, or a map of such values with string keys. - builder.value(value); + if (out.getVersion().onOrAfter(Version.V_8_0_0)) { + out.writeCollection(ignoredValues, StreamOutput::writeGenericValue); } - builder.endArray(); - return builder; + } - + + public ToXContentFragment getValidValuesWriter(){ + return new ToXContentFragment() { + + @Override + public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { + builder.startArray(name); + for (Object value : values) { + // This call doesn't really need to support writing any kind of object, since the values + // here are always serializable to xContent. Each value could be a leaf types like a string, + // number, or boolean, a list of such values, or a map of such values with string keys. + builder.value(value); + } + builder.endArray(); + return builder; + } + }; + } + public ToXContentFragment getIgnoredValuesWriter(){ + return new ToXContentFragment() { + + @Override + public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { + builder.startArray(name); + for (Object value : ignoredValues) { + builder.value(value); + } + builder.endArray(); + return builder; + } + }; + } + +// public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { +// builder.startArray(name); +// for (Object value : values) { +// // This call doesn't really need to support writing any kind of object, since the values +// // here are always serializable to xContent. Each value could be a leaf types like a string, +// // number, or boolean, a list of such values, or a map of such values with string keys. +// builder.value(value); +// } +// builder.endArray(); +// return builder; +// } +// public static DocumentField fromXContent(XContentParser parser) throws IOException { ensureExpectedToken(XContentParser.Token.FIELD_NAME, parser.currentToken(), parser); String fieldName = parser.currentName(); @@ -117,12 +188,13 @@ public boolean equals(Object o) { return false; } DocumentField objects = (DocumentField) o; - return Objects.equals(name, objects.name) && Objects.equals(values, objects.values); + return Objects.equals(name, objects.name) && Objects.equals(values, objects.values) && + Objects.equals(ignoredValues, objects.ignoredValues); } @Override public int hashCode() { - return Objects.hash(name, values); + return Objects.hash(name, values, ignoredValues); } @Override @@ -130,6 +202,8 @@ public String toString() { return "DocumentField{" + "name='" + name + '\'' + ", values=" + values + + ", ignoredValues=" + ignoredValues + '}'; } + } diff --git a/server/src/main/java/org/elasticsearch/index/get/GetResult.java b/server/src/main/java/org/elasticsearch/index/get/GetResult.java index 1bc6b20321a2e..de743730d2c15 100644 --- a/server/src/main/java/org/elasticsearch/index/get/GetResult.java +++ b/server/src/main/java/org/elasticsearch/index/get/GetResult.java @@ -275,7 +275,7 @@ public XContentBuilder toXContentEmbedded(XContentBuilder builder, Params params if (documentFields.isEmpty() == false) { builder.startObject(FIELDS); for (DocumentField field : documentFields.values()) { - field.toXContent(builder, params); + field.getValidValuesWriter().toXContent(builder, params); } builder.endObject(); } diff --git a/server/src/main/java/org/elasticsearch/index/mapper/ArraySourceValueFetcher.java b/server/src/main/java/org/elasticsearch/index/mapper/ArraySourceValueFetcher.java index ba655769533f3..4fd2979123076 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/ArraySourceValueFetcher.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/ArraySourceValueFetcher.java @@ -43,7 +43,7 @@ public ArraySourceValueFetcher(String fieldName, SearchExecutionContext context, } @Override - public List fetchValues(SourceLookup lookup) { + public List fetchValues(SourceLookup lookup, List ignoredValues) { List values = new ArrayList<>(); for (String path : sourcePaths) { Object sourceValue = lookup.extractValue(path, nullValue); @@ -55,7 +55,7 @@ public List fetchValues(SourceLookup lookup) { } catch (Exception e) { // if parsing fails here then it would have failed at index time // as well, meaning that we must be ignoring malformed values. - // So ignore it here too. + ignoredValues.add(sourceValue); } } return values; diff --git a/server/src/main/java/org/elasticsearch/index/mapper/DocValueFetcher.java b/server/src/main/java/org/elasticsearch/index/mapper/DocValueFetcher.java index 21f5766c58eff..465ca06c522e8 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/DocValueFetcher.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/DocValueFetcher.java @@ -39,7 +39,7 @@ public void setNextReader(LeafReaderContext context) { } @Override - public List fetchValues(SourceLookup lookup) throws IOException { + public List fetchValues(SourceLookup lookup, List ignoredValues) throws IOException { if (false == formattedDocValues.advanceExact(lookup.docId())) { return emptyList(); } diff --git a/server/src/main/java/org/elasticsearch/index/mapper/NestedValueFetcher.java b/server/src/main/java/org/elasticsearch/index/mapper/NestedValueFetcher.java index 65f0003a67d8d..f12669ded80ed 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/NestedValueFetcher.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/NestedValueFetcher.java @@ -39,7 +39,7 @@ public NestedValueFetcher(String nestedField, FieldFetcher nestedFieldFetcher) { } @Override - public List fetchValues(SourceLookup lookup) throws IOException { + public List fetchValues(SourceLookup lookup, List includedValues) throws IOException { List nestedEntriesToReturn = new ArrayList<>(); Map filteredSource = new HashMap<>(); Map stub = createSourceMapStub(filteredSource); diff --git a/server/src/main/java/org/elasticsearch/index/mapper/SourceValueFetcher.java b/server/src/main/java/org/elasticsearch/index/mapper/SourceValueFetcher.java index b23ae682c14ca..2469abce25c14 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/SourceValueFetcher.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/SourceValueFetcher.java @@ -51,7 +51,7 @@ public SourceValueFetcher(Set sourcePaths, Object nullValue) { } @Override - public List fetchValues(SourceLookup lookup) { + public List fetchValues(SourceLookup lookup, List ignoredValues) { List values = new ArrayList<>(); for (String path : sourcePaths) { Object sourceValue = lookup.extractValue(path, nullValue); @@ -76,8 +76,11 @@ public List fetchValues(SourceLookup lookup) { Object parsedValue = parseSourceValue(value); if (parsedValue != null) { values.add(parsedValue); + } else { + ignoredValues.add(value); } } catch (Exception e) { + ignoredValues.add(value); // if we get a parsing exception here, that means that the // value in _source would have also caused a parsing // exception at index time and the value ignored. diff --git a/server/src/main/java/org/elasticsearch/index/mapper/ValueFetcher.java b/server/src/main/java/org/elasticsearch/index/mapper/ValueFetcher.java index 70f4a08da238d..9227056daad83 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/ValueFetcher.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/ValueFetcher.java @@ -31,9 +31,10 @@ public interface ValueFetcher { * should not be relied on. * * @param lookup a lookup structure over the document's source. + * @param ignoredValues a mutable list to collect any ignored values as they were originally presented in source * @return a list a standardized field values. */ - List fetchValues(SourceLookup lookup) throws IOException; + List fetchValues(SourceLookup lookup, List ignoredValues) throws IOException; /** * Update the leaf reader used to fetch values. diff --git a/server/src/main/java/org/elasticsearch/index/termvectors/TermVectorsService.java b/server/src/main/java/org/elasticsearch/index/termvectors/TermVectorsService.java index 4afa98edc7f30..a87f746d5a602 100644 --- a/server/src/main/java/org/elasticsearch/index/termvectors/TermVectorsService.java +++ b/server/src/main/java/org/elasticsearch/index/termvectors/TermVectorsService.java @@ -256,7 +256,8 @@ private static Fields generateTermVectors(IndexShard indexShard, for (String field : fields) { if (values.containsKey(field) == false) { SourceValueFetcher valueFetcher = SourceValueFetcher.toString(mappingLookup.sourcePaths(field)); - List v = valueFetcher.fetchValues(sourceLookup); + List ignoredValues = new ArrayList<>(); + List v = valueFetcher.fetchValues(sourceLookup, ignoredValues); if (v.isEmpty() == false) { values.put(field, v); } diff --git a/server/src/main/java/org/elasticsearch/search/SearchHit.java b/server/src/main/java/org/elasticsearch/search/SearchHit.java index d14167d635be6..282fe196cc520 100644 --- a/server/src/main/java/org/elasticsearch/search/SearchHit.java +++ b/server/src/main/java/org/elasticsearch/search/SearchHit.java @@ -582,6 +582,7 @@ public static class Fields { static final String _PRIMARY_TERM = "_primary_term"; static final String _SCORE = "_score"; static final String FIELDS = "fields"; + static final String IGNORED_FIELD_VALUES = "ignored_field_values"; static final String HIGHLIGHT = "highlight"; static final String SORT = "sort"; static final String MATCHED_QUERIES = "matched_queries"; @@ -666,7 +667,19 @@ public XContentBuilder toInnerXContent(XContentBuilder builder, Params params) t builder.startObject(Fields.FIELDS); for (DocumentField field : documentFields.values()) { if (field.getValues().size() > 0) { - field.toXContent(builder, params); + field.getValidValuesWriter().toXContent(builder, params); + } + } + builder.endObject(); + } + //ignored field values + if (documentFields.isEmpty() == false && + // omit ignored_field_values all together if there are none + documentFields.values().stream().anyMatch(df -> df.getIgnoredValues().size() > 0)) { + builder.startObject(Fields.IGNORED_FIELD_VALUES); + for (DocumentField field : documentFields.values()) { + if (field.getIgnoredValues().size() > 0) { + field.getIgnoredValuesWriter().toXContent(builder, params); } } builder.endObject(); diff --git a/server/src/main/java/org/elasticsearch/search/fetch/subphase/FetchDocValuesPhase.java b/server/src/main/java/org/elasticsearch/search/fetch/subphase/FetchDocValuesPhase.java index 8aee2b676925b..ff63913599bda 100644 --- a/server/src/main/java/org/elasticsearch/search/fetch/subphase/FetchDocValuesPhase.java +++ b/server/src/main/java/org/elasticsearch/search/fetch/subphase/FetchDocValuesPhase.java @@ -69,7 +69,11 @@ public void process(HitContext hit) throws IOException { // docValues fields will still be document fields, and put under "fields" section of a hit. hit.hit().setDocumentField(f.field, hitField); } - hitField.getValues().addAll(f.fetcher.fetchValues(hit.sourceLookup())); + List ignoredValues = new ArrayList<>(); + hitField.getValues().addAll(f.fetcher.fetchValues(hit.sourceLookup(), ignoredValues)); + // ignored fields are the exception so DocumentField normally returns emptyList which isn't + // mutable. We have to use the add method here to set any exceptional + hitField.addIgnoredValues(ignoredValues); } } }; 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 62fafa7f0caf7..c5bcf1773c831 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 @@ -167,9 +167,10 @@ public Map fetch(SourceLookup sourceLookup) throws IOExce String field = context.fieldName; ValueFetcher valueFetcher = context.valueFetcher; - List parsedValues = valueFetcher.fetchValues(sourceLookup); - if (parsedValues.isEmpty() == false) { - documentFields.put(field, new DocumentField(field, parsedValues)); + List ignoredValues = new ArrayList<>(); + List parsedValues = valueFetcher.fetchValues(sourceLookup, ignoredValues); + if (parsedValues.isEmpty() == false || ignoredValues.isEmpty() == false) { + documentFields.put(field, new DocumentField(field, parsedValues, ignoredValues)); } } collectUnmapped(documentFields, sourceLookup.source(), "", 0); diff --git a/server/src/main/java/org/elasticsearch/search/fetch/subphase/highlight/HighlightUtils.java b/server/src/main/java/org/elasticsearch/search/fetch/subphase/highlight/HighlightUtils.java index d3c930b021fba..d71b8c8282892 100644 --- a/server/src/main/java/org/elasticsearch/search/fetch/subphase/highlight/HighlightUtils.java +++ b/server/src/main/java/org/elasticsearch/search/fetch/subphase/highlight/HighlightUtils.java @@ -17,6 +17,7 @@ import org.elasticsearch.search.fetch.FetchSubPhase; import java.io.IOException; +import java.util.ArrayList; import java.util.Collections; import java.util.List; import java.util.Objects; @@ -48,7 +49,7 @@ public static List loadFieldValues(MappedFieldType fieldType, } ValueFetcher fetcher = fieldType.valueFetcher(searchContext, null); fetcher.setNextReader(hitContext.readerContext()); - return fetcher.fetchValues(hitContext.sourceLookup()); + return fetcher.fetchValues(hitContext.sourceLookup(), new ArrayList()); } public static class Encoders { diff --git a/server/src/main/java/org/elasticsearch/search/lookup/FieldValues.java b/server/src/main/java/org/elasticsearch/search/lookup/FieldValues.java index 638ae5706897d..21a3134367ece 100644 --- a/server/src/main/java/org/elasticsearch/search/lookup/FieldValues.java +++ b/server/src/main/java/org/elasticsearch/search/lookup/FieldValues.java @@ -58,13 +58,12 @@ public void setNextReader(LeafReaderContext context) { } @Override - public List fetchValues(SourceLookup lookup) { + public List fetchValues(SourceLookup lookup, List ignoredValues) { List values = new ArrayList<>(); try { fieldValues.valuesForDoc(context.lookup(), ctx, lookup.docId(), v -> values.add(formatter.apply(v))); } catch (Exception e) { - // ignore errors - if they exist here then they existed at index time - // and so on_script_error must have been set to `ignore` + ignoredValues.addAll(values); } return values; } @@ -89,13 +88,12 @@ public void setNextReader(LeafReaderContext context) { } @Override - public List fetchValues(SourceLookup lookup) { + public List fetchValues(SourceLookup lookup, List ignoredValues) { List values = new ArrayList<>(); try { fieldValues.valuesForDoc(context.lookup(), ctx, lookup.docId(), v -> values.add(v)); } catch (Exception e) { - // ignore errors - if they exist here then they existed at index time - // and so on_script_error must have been set to `ignore` + ignoredValues.addAll(values); } return formatter.apply(values); } diff --git a/server/src/test/java/org/elasticsearch/index/get/DocumentFieldTests.java b/server/src/test/java/org/elasticsearch/index/get/DocumentFieldTests.java index 826e8d4b6ab09..5a864d2fb114f 100644 --- a/server/src/test/java/org/elasticsearch/index/get/DocumentFieldTests.java +++ b/server/src/test/java/org/elasticsearch/index/get/DocumentFieldTests.java @@ -36,9 +36,11 @@ public class DocumentFieldTests extends ESTestCase { public void testToXContent() { - DocumentField documentField = new DocumentField("field", Arrays.asList("value1", "value2")); - String output = Strings.toString(documentField); + DocumentField documentField = new DocumentField("field", Arrays.asList("value1", "value2"), Arrays.asList("ignored1", "ignored2")); + String output = Strings.toString(documentField.getValidValuesWriter()); assertEquals("{\"field\":[\"value1\",\"value2\"]}", output); + String ignoredOutput = Strings.toString(documentField.getIgnoredValuesWriter()); + assertEquals("{\"field\":[\"ignored1\",\"ignored2\"]}", ignoredOutput); } public void testEqualsAndHashcode() { @@ -52,7 +54,7 @@ public void testToAndFromXContent() throws Exception { DocumentField documentField = tuple.v1(); DocumentField expectedDocumentField = tuple.v2(); boolean humanReadable = randomBoolean(); - BytesReference originalBytes = toShuffledXContent(documentField, xContentType, ToXContent.EMPTY_PARAMS, humanReadable); + BytesReference originalBytes = toShuffledXContent(documentField.getValidValuesWriter(), xContentType, ToXContent.EMPTY_PARAMS, humanReadable); //test that we can parse what we print out DocumentField parsedDocumentField; try (XContentParser parser = createParser(xContentType.xContent(), originalBytes)) { @@ -65,7 +67,7 @@ public void testToAndFromXContent() throws Exception { assertNull(parser.nextToken()); } assertEquals(expectedDocumentField, parsedDocumentField); - BytesReference finalBytes = toXContent(parsedDocumentField, xContentType, humanReadable); + BytesReference finalBytes = toXContent(parsedDocumentField.getValidValuesWriter(), xContentType, humanReadable); assertToXContentEquivalent(originalBytes, finalBytes, xContentType); } diff --git a/server/src/test/java/org/elasticsearch/index/mapper/flattened/KeyedFlattenedFieldTypeTests.java b/server/src/test/java/org/elasticsearch/index/mapper/flattened/KeyedFlattenedFieldTypeTests.java index a9e7c77f32c00..b10e415acc7da 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/flattened/KeyedFlattenedFieldTypeTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/flattened/KeyedFlattenedFieldTypeTests.java @@ -178,9 +178,9 @@ public void testFetchSourceValue() throws IOException { SourceLookup lookup = new SourceLookup(); lookup.setSource(Collections.singletonMap("field", sourceValue)); - assertEquals(List.of("value"), fetcher.fetchValues(lookup)); + assertEquals(List.of("value"), fetcher.fetchValues(lookup, new ArrayList())); lookup.setSource(Collections.singletonMap("field", null)); - assertEquals(List.of(), fetcher.fetchValues(lookup)); + assertEquals(List.of(), fetcher.fetchValues(lookup, new ArrayList())); IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> ft.valueFetcher(searchExecutionContext, "format")); assertEquals("Field [field.key] of type [flattened] doesn't support formats.", e.getMessage()); diff --git a/test/framework/src/main/java/org/elasticsearch/index/mapper/FieldTypeTestCase.java b/test/framework/src/main/java/org/elasticsearch/index/mapper/FieldTypeTestCase.java index 173431b8f48b4..c85c944f3d7f6 100644 --- a/test/framework/src/main/java/org/elasticsearch/index/mapper/FieldTypeTestCase.java +++ b/test/framework/src/main/java/org/elasticsearch/index/mapper/FieldTypeTestCase.java @@ -13,6 +13,7 @@ import org.elasticsearch.test.ESTestCase; import java.io.IOException; +import java.util.ArrayList; import java.util.Collections; import java.util.List; import java.util.Set; @@ -53,7 +54,7 @@ public static List fetchSourceValue(MappedFieldType fieldType, Object sourceV ValueFetcher fetcher = fieldType.valueFetcher(searchExecutionContext, format); SourceLookup lookup = new SourceLookup(); lookup.setSource(Collections.singletonMap(field, sourceValue)); - return fetcher.fetchValues(lookup); + return fetcher.fetchValues(lookup, new ArrayList<>()); } public static List fetchSourceValues(MappedFieldType fieldType, Object... values) throws IOException { @@ -64,6 +65,6 @@ public static List fetchSourceValues(MappedFieldType fieldType, Object... val ValueFetcher fetcher = fieldType.valueFetcher(searchExecutionContext, null); SourceLookup lookup = new SourceLookup(); lookup.setSource(Collections.singletonMap(field, List.of(values))); - return fetcher.fetchValues(lookup); + return fetcher.fetchValues(lookup, new ArrayList<>()); } } diff --git a/test/framework/src/main/java/org/elasticsearch/index/mapper/MapperTestCase.java b/test/framework/src/main/java/org/elasticsearch/index/mapper/MapperTestCase.java index 47d097c72870a..244aa80ab62d4 100644 --- a/test/framework/src/main/java/org/elasticsearch/index/mapper/MapperTestCase.java +++ b/test/framework/src/main/java/org/elasticsearch/index/mapper/MapperTestCase.java @@ -335,7 +335,7 @@ protected final List fetchFromDocValues(MapperService mapperService, MappedFi LeafReaderContext context = searcher.getIndexReader().leaves().get(0); lookup.source().setSegmentAndDocument(context, 0); valueFetcher.setNextReader(context); - result.set(valueFetcher.fetchValues(lookup.source())); + result.set(valueFetcher.fetchValues(lookup.source(), new ArrayList<>())); }); return result.get(); } @@ -605,8 +605,8 @@ protected void assertFetch(MapperService mapperService, String field, Object val sourceLookup.setSegmentAndDocument(ir.leaves().get(0), 0); docValueFetcher.setNextReader(ir.leaves().get(0)); nativeFetcher.setNextReader(ir.leaves().get(0)); - List fromDocValues = docValueFetcher.fetchValues(sourceLookup); - List fromNative = nativeFetcher.fetchValues(sourceLookup); + List fromDocValues = docValueFetcher.fetchValues(sourceLookup, new ArrayList<>()); + List fromNative = nativeFetcher.fetchValues(sourceLookup, new ArrayList<>()); /* * The native fetcher uses byte, short, etc but doc values always * uses long or double. This difference is fine because on the outside diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/cluster/routing/allocation/mapper/DataTierFieldMapper.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/cluster/routing/allocation/mapper/DataTierFieldMapper.java index fb77ae0f30463..6e8f073bf4a0f 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/cluster/routing/allocation/mapper/DataTierFieldMapper.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/cluster/routing/allocation/mapper/DataTierFieldMapper.java @@ -79,8 +79,8 @@ public ValueFetcher valueFetcher(SearchExecutionContext context, String format) String tierPreference = getTierPreference(context); return tierPreference == null - ? lookup -> List.of() - : lookup -> List.of(tierPreference); + ? (lookup, ignoredValues) -> List.of() + : (lookup, ignoredValues) -> List.of(tierPreference); } /** diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/cluster/routing/allocation/mapper/DataTierFieldTypeTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/cluster/routing/allocation/mapper/DataTierFieldTypeTests.java index e336d3b3e36ed..6a0a1efa23f45 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/cluster/routing/allocation/mapper/DataTierFieldTypeTests.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/cluster/routing/allocation/mapper/DataTierFieldTypeTests.java @@ -23,7 +23,9 @@ import org.elasticsearch.search.lookup.SourceLookup; import java.io.IOException; +import java.util.ArrayList; import java.util.Arrays; +import java.util.List; import java.util.function.Predicate; import static java.util.Collections.emptyMap; @@ -88,11 +90,12 @@ public void testFetchValue() throws IOException { MappedFieldType ft = DataTierFieldMapper.DataTierFieldType.INSTANCE; SourceLookup lookup = new SourceLookup(); + List ignoredValues = new ArrayList<>(); ValueFetcher valueFetcher = ft.valueFetcher(createContext(), null); - assertEquals(singletonList("data_warm"), valueFetcher.fetchValues(lookup)); + assertEquals(singletonList("data_warm"), valueFetcher.fetchValues(lookup, ignoredValues)); ValueFetcher emptyValueFetcher = ft.valueFetcher(createContextWithoutSetting(), null); - assertTrue(emptyValueFetcher.fetchValues(lookup).isEmpty()); + assertTrue(emptyValueFetcher.fetchValues(lookup, ignoredValues).isEmpty()); } private SearchExecutionContext createContext() { diff --git a/x-pack/plugin/eql/src/main/java/org/elasticsearch/xpack/eql/action/EqlSearchResponse.java b/x-pack/plugin/eql/src/main/java/org/elasticsearch/xpack/eql/action/EqlSearchResponse.java index 693cf3bafdd15..1078c2abc09a7 100644 --- a/x-pack/plugin/eql/src/main/java/org/elasticsearch/xpack/eql/action/EqlSearchResponse.java +++ b/x-pack/plugin/eql/src/main/java/org/elasticsearch/xpack/eql/action/EqlSearchResponse.java @@ -279,7 +279,7 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws builder.startObject(Fields.FIELDS); for (DocumentField field : fetchFields.values()) { if (field.getValues().size() > 0) { - field.toXContent(builder, params); + field.getValidValuesWriter().toXContent(builder, params); } } builder.endObject(); diff --git a/x-pack/plugin/mapper-constant-keyword/src/main/java/org/elasticsearch/xpack/constantkeyword/mapper/ConstantKeywordFieldMapper.java b/x-pack/plugin/mapper-constant-keyword/src/main/java/org/elasticsearch/xpack/constantkeyword/mapper/ConstantKeywordFieldMapper.java index c8acbc7237b35..7dac799216556 100644 --- a/x-pack/plugin/mapper-constant-keyword/src/main/java/org/elasticsearch/xpack/constantkeyword/mapper/ConstantKeywordFieldMapper.java +++ b/x-pack/plugin/mapper-constant-keyword/src/main/java/org/elasticsearch/xpack/constantkeyword/mapper/ConstantKeywordFieldMapper.java @@ -141,8 +141,8 @@ public ValueFetcher valueFetcher(SearchExecutionContext context, String format) } return value == null - ? lookup -> List.of() - : lookup -> List.of(value); + ? (lookup, ignoredValues) -> List.of() + : (lookup, ignoredValues) -> List.of(value); } diff --git a/x-pack/plugin/mapper-constant-keyword/src/test/java/org/elasticsearch/xpack/constantkeyword/mapper/ConstantKeywordFieldTypeTests.java b/x-pack/plugin/mapper-constant-keyword/src/test/java/org/elasticsearch/xpack/constantkeyword/mapper/ConstantKeywordFieldTypeTests.java index db9ed9667722e..c376b9c10ea12 100644 --- a/x-pack/plugin/mapper-constant-keyword/src/test/java/org/elasticsearch/xpack/constantkeyword/mapper/ConstantKeywordFieldTypeTests.java +++ b/x-pack/plugin/mapper-constant-keyword/src/test/java/org/elasticsearch/xpack/constantkeyword/mapper/ConstantKeywordFieldTypeTests.java @@ -17,6 +17,7 @@ import org.elasticsearch.search.lookup.SourceLookup; import org.elasticsearch.xpack.constantkeyword.mapper.ConstantKeywordFieldMapper.ConstantKeywordFieldType; +import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; import java.util.List; @@ -114,13 +115,14 @@ public void testFetchValue() throws Exception { SourceLookup nullValueLookup = new SourceLookup(); nullValueLookup.setSource(Collections.singletonMap("field", null)); - assertTrue(fetcher.fetchValues(missingValueLookup).isEmpty()); - assertTrue(fetcher.fetchValues(nullValueLookup).isEmpty()); + List ignoredValues = new ArrayList<>(); + assertTrue(fetcher.fetchValues(missingValueLookup, ignoredValues).isEmpty()); + assertTrue(fetcher.fetchValues(nullValueLookup, ignoredValues).isEmpty()); MappedFieldType valued = new ConstantKeywordFieldMapper.ConstantKeywordFieldType("field", "foo"); fetcher = valued.valueFetcher(null, null); - assertEquals(List.of("foo"), fetcher.fetchValues(missingValueLookup)); - assertEquals(List.of("foo"), fetcher.fetchValues(nullValueLookup)); + assertEquals(List.of("foo"), fetcher.fetchValues(missingValueLookup, ignoredValues)); + assertEquals(List.of("foo"), fetcher.fetchValues(nullValueLookup, ignoredValues)); } } From 908a2b7fa4305a92094860c2b44e90719296a521 Mon Sep 17 00:00:00 2001 From: markharwood Date: Tue, 5 Oct 2021 15:27:37 +0100 Subject: [PATCH 2/9] Removed commented-out code, checkstyle fix --- .../elasticsearch/common/document/DocumentField.java | 12 ------------ .../elasticsearch/index/get/DocumentFieldTests.java | 7 ++++++- 2 files changed, 6 insertions(+), 13 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/common/document/DocumentField.java b/server/src/main/java/org/elasticsearch/common/document/DocumentField.java index 08380ceb9723d..1529bd8978cf6 100644 --- a/server/src/main/java/org/elasticsearch/common/document/DocumentField.java +++ b/server/src/main/java/org/elasticsearch/common/document/DocumentField.java @@ -155,18 +155,6 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws }; } -// public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { -// builder.startArray(name); -// for (Object value : values) { -// // This call doesn't really need to support writing any kind of object, since the values -// // here are always serializable to xContent. Each value could be a leaf types like a string, -// // number, or boolean, a list of such values, or a map of such values with string keys. -// builder.value(value); -// } -// builder.endArray(); -// return builder; -// } -// public static DocumentField fromXContent(XContentParser parser) throws IOException { ensureExpectedToken(XContentParser.Token.FIELD_NAME, parser.currentToken(), parser); String fieldName = parser.currentName(); diff --git a/server/src/test/java/org/elasticsearch/index/get/DocumentFieldTests.java b/server/src/test/java/org/elasticsearch/index/get/DocumentFieldTests.java index 5a864d2fb114f..44d1cb1c30d00 100644 --- a/server/src/test/java/org/elasticsearch/index/get/DocumentFieldTests.java +++ b/server/src/test/java/org/elasticsearch/index/get/DocumentFieldTests.java @@ -54,7 +54,12 @@ public void testToAndFromXContent() throws Exception { DocumentField documentField = tuple.v1(); DocumentField expectedDocumentField = tuple.v2(); boolean humanReadable = randomBoolean(); - BytesReference originalBytes = toShuffledXContent(documentField.getValidValuesWriter(), xContentType, ToXContent.EMPTY_PARAMS, humanReadable); + BytesReference originalBytes = toShuffledXContent( + documentField.getValidValuesWriter(), + xContentType, + ToXContent.EMPTY_PARAMS, + humanReadable + ); //test that we can parse what we print out DocumentField parsedDocumentField; try (XContentParser parser = createParser(xContentType.xContent(), originalBytes)) { From 1d9dc8d44184743a61f99bb646c03071422aa8f0 Mon Sep 17 00:00:00 2001 From: markharwood Date: Tue, 5 Oct 2021 16:50:37 +0100 Subject: [PATCH 3/9] Change test expectations to match new DocumentField behaviour --- .../search/fetch/subphase/FieldFetcherTests.java | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) 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 87cdfd8d1de92..afcf623bd189a 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 @@ -349,7 +349,8 @@ public void testIgnoreAbove() throws IOException { .array("field", "really_really_long_value") .endObject(); fields = fetchFields(mapperService, source, "field"); - assertFalse(fields.containsKey("field")); + assertThat(fields.get("field").getValues().size(), equalTo(0)); + assertThat(fields.get("field").getIgnoredValues().size(), equalTo(1)); } public void testFieldAliases() throws IOException { @@ -827,14 +828,17 @@ public void testMappedFieldNotOverwritten() throws IOException { // this should not return a field bc. f1 is malformed Map fields = fetchFields(mapperService, source, List.of(new FieldAndFormat("*", null, true))); - assertThat(fields.size(), equalTo(0)); + assertThat(fields.get("f1").getValues().size(), equalTo(0)); + assertThat(fields.get("f1").getIgnoredValues().size(), equalTo(1)); // and this should neither fields = fetchFields(mapperService, source, List.of(new FieldAndFormat("*", null, true))); - assertThat(fields.size(), equalTo(0)); + assertThat(fields.get("f1").getValues().size(), equalTo(0)); + assertThat(fields.get("f1").getIgnoredValues().size(), equalTo(1)); fields = fetchFields(mapperService, source, List.of(new FieldAndFormat("f1", null, true))); - assertThat(fields.size(), equalTo(0)); + assertThat(fields.get("f1").getValues().size(), equalTo(0)); + assertThat(fields.get("f1").getIgnoredValues().size(), equalTo(1)); // check this also does not overwrite with arrays source = XContentFactory.jsonBuilder().startObject() @@ -842,7 +846,8 @@ public void testMappedFieldNotOverwritten() throws IOException { .endObject(); fields = fetchFields(mapperService, source, List.of(new FieldAndFormat("f1", null, true))); - assertThat(fields.size(), equalTo(0)); + assertThat(fields.get("f1").getValues().size(), equalTo(0)); + assertThat(fields.get("f1").getIgnoredValues().size(), equalTo(1)); } public void testUnmappedFieldsWildcard() throws IOException { From 84744299f0814134529204d0987172b102e1a196 Mon Sep 17 00:00:00 2001 From: markharwood Date: Mon, 11 Oct 2021 12:13:08 +0100 Subject: [PATCH 4/9] Addressing review comments (thanks @jimczi !) --- .../retrieve-selected-fields.asciidoc | 13 ++++++++----- .../common/document/DocumentField.java | 1 - .../search/fetch/subphase/FetchDocValuesPhase.java | 5 ++--- 3 files changed, 10 insertions(+), 9 deletions(-) diff --git a/docs/reference/search/search-your-data/retrieve-selected-fields.asciidoc b/docs/reference/search/search-your-data/retrieve-selected-fields.asciidoc index 44a2adf45fb5e..73a56a769fc98 100644 --- a/docs/reference/search/search-your-data/retrieve-selected-fields.asciidoc +++ b/docs/reference/search/search-your-data/retrieve-selected-fields.asciidoc @@ -393,7 +393,7 @@ PUT my-index-000001 { "mappings": { "properties": { - "my-small" : { "type" : "keyword", "ignore_above": 1 }, <1> + "my-small" : { "type" : "keyword", "ignore_above": 2 }, <1> "my-large" : { "type" : "keyword" } } } @@ -401,7 +401,7 @@ PUT my-index-000001 PUT my-index-000001/_doc/1?refresh=true { - "my-small": "bad content", <2> + "my-small": ["ok", "bad"], <2> "my-large": "ok content" } @@ -413,9 +413,9 @@ POST my-index-000001/_search ---- <1> This field has a size restriction -<2> This document field exceeds the size restriction so is ignored and not indexed +<2> This document field has a value that exceeds the size restriction so is ignored and not indexed -The response will contain ignored field results under the `ignored_field_values` path. +The response will contain ignored field values under the `ignored_field_values` path. These values are retrieved from the document's original JSON source and are raw so will not be formatted or treated in any way, unlike the successfully indexed fields which are returned in the `fields` section. @@ -446,11 +446,14 @@ returned in the `fields` section. "fields" : { "my-large": [ "ok content" + ], + "my-small": [ + "ok" ] }, "ignored_field_values" : { "my-small": [ - "bad content" + "bad" ] } } diff --git a/server/src/main/java/org/elasticsearch/common/document/DocumentField.java b/server/src/main/java/org/elasticsearch/common/document/DocumentField.java index 1529bd8978cf6..af124ae950e84 100644 --- a/server/src/main/java/org/elasticsearch/common/document/DocumentField.java +++ b/server/src/main/java/org/elasticsearch/common/document/DocumentField.java @@ -34,7 +34,6 @@ * @see SearchHit * @see GetResult */ -//public class DocumentField implements Writeable, ToXContentFragment, Iterable { public class DocumentField implements Writeable, Iterable { private final String name; diff --git a/server/src/main/java/org/elasticsearch/search/fetch/subphase/FetchDocValuesPhase.java b/server/src/main/java/org/elasticsearch/search/fetch/subphase/FetchDocValuesPhase.java index ff63913599bda..cb7ecbe0ff984 100644 --- a/server/src/main/java/org/elasticsearch/search/fetch/subphase/FetchDocValuesPhase.java +++ b/server/src/main/java/org/elasticsearch/search/fetch/subphase/FetchDocValuesPhase.java @@ -71,9 +71,8 @@ public void process(HitContext hit) throws IOException { } List ignoredValues = new ArrayList<>(); hitField.getValues().addAll(f.fetcher.fetchValues(hit.sourceLookup(), ignoredValues)); - // ignored fields are the exception so DocumentField normally returns emptyList which isn't - // mutable. We have to use the add method here to set any exceptional - hitField.addIgnoredValues(ignoredValues); + // Doc value fetches should not return any ignored values + assert ignoredValues.isEmpty(); } } }; From 137682b45c6ba304da44f1dc4771640f01157533 Mon Sep 17 00:00:00 2001 From: markharwood Date: Mon, 11 Oct 2021 15:21:51 +0100 Subject: [PATCH 5/9] Address review comments This reverts commit 1f82ab33d96d9e6ed247f75892362b9faba387cb. --- .../common/document/DocumentField.java | 13 +++---------- 1 file changed, 3 insertions(+), 10 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/common/document/DocumentField.java b/server/src/main/java/org/elasticsearch/common/document/DocumentField.java index af124ae950e84..291f485451957 100644 --- a/server/src/main/java/org/elasticsearch/common/document/DocumentField.java +++ b/server/src/main/java/org/elasticsearch/common/document/DocumentField.java @@ -99,17 +99,10 @@ public List getIgnoredValues() { } /** - * The field's ignored values as an immutable list. + * Set the field's ignored values. */ - public void addIgnoredValues(List additions) { - if (ignoredValues.isEmpty()) { - return; - } - if (ignoredValues == Collections.emptyList()) { - ignoredValues = new ArrayList<>(additions); - } else { - ignoredValues.addAll(additions); - } + public void setIgnoredValues(List additions) { + ignoredValues = List.copyOf(additions); } @Override From 7130834e9457e9f068ef3ebe8f541bca62dfc6e9 Mon Sep 17 00:00:00 2001 From: markharwood Date: Tue, 12 Oct 2021 10:58:36 +0100 Subject: [PATCH 6/9] Compilation fix --- .../org/elasticsearch/index/mapper/StoredValueFetcher.java | 2 +- .../org/elasticsearch/index/mapper/IdFieldMapperTests.java | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/index/mapper/StoredValueFetcher.java b/server/src/main/java/org/elasticsearch/index/mapper/StoredValueFetcher.java index 77ceae54e6297..993a85f28ae99 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/StoredValueFetcher.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/StoredValueFetcher.java @@ -36,7 +36,7 @@ public void setNextReader(LeafReaderContext context) { } @Override - public List fetchValues(SourceLookup lookup) throws IOException { + public List fetchValues(SourceLookup lookup, List ignoredValues) throws IOException { leafSearchLookup.setDocument(lookup.docId()); return leafSearchLookup.fields().get(fieldname).getValues(); } diff --git a/server/src/test/java/org/elasticsearch/index/mapper/IdFieldMapperTests.java b/server/src/test/java/org/elasticsearch/index/mapper/IdFieldMapperTests.java index 8990fd470d5ed..a725d6847783c 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/IdFieldMapperTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/IdFieldMapperTests.java @@ -17,6 +17,7 @@ import org.elasticsearch.search.lookup.SearchLookup; import java.io.IOException; +import java.util.ArrayList; import java.util.List; import static org.elasticsearch.index.mapper.IdFieldMapper.ID_FIELD_DATA_DEPRECATION_MESSAGE; @@ -85,7 +86,7 @@ public void testFetchIdFieldValue() throws IOException { LeafReaderContext context = searcher.getIndexReader().leaves().get(0); lookup.source().setSegmentAndDocument(context, 0); valueFetcher.setNextReader(context); - assertEquals(List.of(id), valueFetcher.fetchValues(lookup.source())); + assertEquals(List.of(id), valueFetcher.fetchValues(lookup.source(), new ArrayList<>())); }); } From 18949454594d722e86eb58fd738f566e9ef0c4a3 Mon Sep 17 00:00:00 2001 From: markharwood Date: Tue, 12 Oct 2021 14:52:51 +0100 Subject: [PATCH 7/9] Addressing review comments --- .../elasticsearch/common/document/DocumentField.java | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/common/document/DocumentField.java b/server/src/main/java/org/elasticsearch/common/document/DocumentField.java index 291f485451957..1ddd0879e0fbf 100644 --- a/server/src/main/java/org/elasticsearch/common/document/DocumentField.java +++ b/server/src/main/java/org/elasticsearch/common/document/DocumentField.java @@ -51,13 +51,13 @@ public DocumentField(StreamInput in) throws IOException { } public DocumentField(String name, List values) { - this(name, values, null); + this(name, values, Collections.emptyList()); } public DocumentField(String name, List values, List ignoredValues) { this.name = Objects.requireNonNull(name, "name must not be null"); this.values = Objects.requireNonNull(values, "values must not be null"); - this.ignoredValues = ignoredValues == null ? Collections.emptyList(): ignoredValues; + this.ignoredValues = Objects.requireNonNull(ignoredValues, "ignoredValues must not be null"); } @@ -98,13 +98,6 @@ public List getIgnoredValues() { return ignoredValues == Collections.emptyList() ? ignoredValues : Collections.unmodifiableList(ignoredValues); } - /** - * Set the field's ignored values. - */ - public void setIgnoredValues(List additions) { - ignoredValues = List.copyOf(additions); - } - @Override public void writeTo(StreamOutput out) throws IOException { out.writeString(name); From d7f0b92dee591336ac9f41b4787c7bb037afbed9 Mon Sep 17 00:00:00 2001 From: markharwood Date: Tue, 12 Oct 2021 15:40:20 +0100 Subject: [PATCH 8/9] Make parsed and ignored value lists both immutable in constructor --- .../common/document/DocumentField.java | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/common/document/DocumentField.java b/server/src/main/java/org/elasticsearch/common/document/DocumentField.java index 1ddd0879e0fbf..5110d9624de2d 100644 --- a/server/src/main/java/org/elasticsearch/common/document/DocumentField.java +++ b/server/src/main/java/org/elasticsearch/common/document/DocumentField.java @@ -56,8 +56,17 @@ public DocumentField(String name, List values) { public DocumentField(String name, List values, List ignoredValues) { this.name = Objects.requireNonNull(name, "name must not be null"); - this.values = Objects.requireNonNull(values, "values must not be null"); - this.ignoredValues = Objects.requireNonNull(ignoredValues, "ignoredValues must not be null"); + this.values = asUnmodifiableList(values, "values must not be null"); + this.ignoredValues = asUnmodifiableList(ignoredValues, "ignoredValues must not be null"); + } + + private static List asUnmodifiableList(List list, String ifNullMessage) { + list = Objects.requireNonNull(list, ifNullMessage); + if (list == Collections.emptyList()) { + // already immutable + return list; + } + return Collections.unmodifiableList(list); } @@ -92,10 +101,10 @@ public Iterator iterator() { } /** - * The field's ignored values as an immutable list. + * The field's ignored values */ public List getIgnoredValues() { - return ignoredValues == Collections.emptyList() ? ignoredValues : Collections.unmodifiableList(ignoredValues); + return ignoredValues; } @Override From f2188f7ad811efbd24591e4f3668b1f5bc6e2397 Mon Sep 17 00:00:00 2001 From: markharwood Date: Tue, 12 Oct 2021 16:39:34 +0100 Subject: [PATCH 9/9] Revert "Make parsed and ignored value lists both immutable in constructor" This reverts commit 13a5062cfd081d823635eac87f6ed062c126a9f6. --- .../common/document/DocumentField.java | 17 ++++------------- 1 file changed, 4 insertions(+), 13 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/common/document/DocumentField.java b/server/src/main/java/org/elasticsearch/common/document/DocumentField.java index 5110d9624de2d..1ddd0879e0fbf 100644 --- a/server/src/main/java/org/elasticsearch/common/document/DocumentField.java +++ b/server/src/main/java/org/elasticsearch/common/document/DocumentField.java @@ -56,17 +56,8 @@ public DocumentField(String name, List values) { public DocumentField(String name, List values, List ignoredValues) { this.name = Objects.requireNonNull(name, "name must not be null"); - this.values = asUnmodifiableList(values, "values must not be null"); - this.ignoredValues = asUnmodifiableList(ignoredValues, "ignoredValues must not be null"); - } - - private static List asUnmodifiableList(List list, String ifNullMessage) { - list = Objects.requireNonNull(list, ifNullMessage); - if (list == Collections.emptyList()) { - // already immutable - return list; - } - return Collections.unmodifiableList(list); + this.values = Objects.requireNonNull(values, "values must not be null"); + this.ignoredValues = Objects.requireNonNull(ignoredValues, "ignoredValues must not be null"); } @@ -101,10 +92,10 @@ public Iterator iterator() { } /** - * The field's ignored values + * The field's ignored values as an immutable list. */ public List getIgnoredValues() { - return ignoredValues; + return ignoredValues == Collections.emptyList() ? ignoredValues : Collections.unmodifiableList(ignoredValues); } @Override