From 9e52513c7bf435ec2487dda100aac81070be18dc Mon Sep 17 00:00:00 2001 From: Julie Tibshirani Date: Mon, 12 Oct 2020 17:34:21 -0700 Subject: [PATCH] Add support for missing value fetchers. (#63585) This PR implements value fetching for the following field types: * `text` phrase and prefix subfields * `search_as_you_type`, plus its subfields * `token_count`, which is implemented by fetching doc values Supporting these types helps ensure that retrieving all fields through `"fields": ["*"]` doesn't fail because of unsupported value fetchers. --- .../mapper/SearchAsYouTypeFieldMapper.java | 10 ++++-- .../index/mapper/TokenCountFieldMapper.java | 21 +++++++++-- .../mapper/SearchAsYouTypeFieldTypeTests.java | 22 ++++++++++++ rest-api-spec/build.gradle | 4 +++ .../test/search/330_fetch_fields.yml | 36 +++++++++++++++++++ .../index/mapper/MappedFieldType.java | 4 +++ .../index/mapper/NumberFieldMapper.java | 2 +- .../index/mapper/TextFieldMapper.java | 12 ++++--- .../index/mapper/TextFieldTypeTests.java | 23 +++++++----- .../fetch/subphase/FieldFetcherTests.java | 29 +++++++++++++++ .../index/mapper/FieldTypeTestCase.java | 2 -- 11 files changed, 143 insertions(+), 22 deletions(-) diff --git a/modules/mapper-extras/src/main/java/org/elasticsearch/index/mapper/SearchAsYouTypeFieldMapper.java b/modules/mapper-extras/src/main/java/org/elasticsearch/index/mapper/SearchAsYouTypeFieldMapper.java index 114759cd57b..9596a3acb20 100644 --- a/modules/mapper-extras/src/main/java/org/elasticsearch/index/mapper/SearchAsYouTypeFieldMapper.java +++ b/modules/mapper-extras/src/main/java/org/elasticsearch/index/mapper/SearchAsYouTypeFieldMapper.java @@ -265,7 +265,7 @@ public class SearchAsYouTypeFieldMapper extends ParametrizedFieldMapper { @Override public ValueFetcher valueFetcher(MapperService mapperService, SearchLookup searchLookup, String format) { - throw new UnsupportedOperationException(); + return SourceValueFetcher.toString(name(), mapperService, format); } @Override @@ -376,7 +376,9 @@ public class SearchAsYouTypeFieldMapper extends ParametrizedFieldMapper { @Override public ValueFetcher valueFetcher(MapperService mapperService, SearchLookup searchLookup, String format) { - throw new UnsupportedOperationException(); + // Because this internal field is modelled as a multi-field, SourceValueFetcher will look up its + // parent field in _source. So we don't need to use the parent field name here. + return SourceValueFetcher.toString(name(), mapperService, format); } @Override @@ -480,7 +482,9 @@ public class SearchAsYouTypeFieldMapper extends ParametrizedFieldMapper { @Override public ValueFetcher valueFetcher(MapperService mapperService, SearchLookup searchLookup, String format) { - throw new UnsupportedOperationException(); + // Because this internal field is modelled as a multi-field, SourceValueFetcher will look up its + // parent field in _source. So we don't need to use the parent field name here. + return SourceValueFetcher.toString(name(), mapperService, format); } @Override diff --git a/modules/mapper-extras/src/main/java/org/elasticsearch/index/mapper/TokenCountFieldMapper.java b/modules/mapper-extras/src/main/java/org/elasticsearch/index/mapper/TokenCountFieldMapper.java index e09808a81a7..0413ed50a3e 100644 --- a/modules/mapper-extras/src/main/java/org/elasticsearch/index/mapper/TokenCountFieldMapper.java +++ b/modules/mapper-extras/src/main/java/org/elasticsearch/index/mapper/TokenCountFieldMapper.java @@ -23,6 +23,7 @@ import org.apache.lucene.analysis.Analyzer; import org.apache.lucene.analysis.TokenStream; import org.apache.lucene.analysis.tokenattributes.PositionIncrementAttribute; import org.elasticsearch.index.analysis.NamedAnalyzer; +import org.elasticsearch.search.lookup.SearchLookup; import java.io.IOException; import java.util.Arrays; @@ -72,19 +73,33 @@ public class TokenCountFieldMapper extends ParametrizedFieldMapper { if (analyzer.getValue() == null) { throw new MapperParsingException("Analyzer must be set for field [" + name + "] but wasn't."); } - MappedFieldType ft = new NumberFieldMapper.NumberFieldType( + MappedFieldType ft = new TokenCountFieldType( buildFullName(context), - NumberFieldMapper.NumberType.INTEGER, index.getValue(), store.getValue(), hasDocValues.getValue(), - false, nullValue.getValue(), meta.getValue()); return new TokenCountFieldMapper(name, ft, multiFieldsBuilder.build(this, context), copyTo.build(), this); } } + static class TokenCountFieldType extends NumberFieldMapper.NumberFieldType { + + TokenCountFieldType(String name, boolean isSearchable, boolean isStored, + boolean hasDocValues, Number nullValue, Map meta) { + super(name, NumberFieldMapper.NumberType.INTEGER, isSearchable, isStored, hasDocValues, false, nullValue, meta); + } + + @Override + public ValueFetcher valueFetcher(MapperService mapperService, SearchLookup searchLookup, String format) { + if (hasDocValues() == false) { + return lookup -> org.elasticsearch.common.collect.List.of(); + } + return new DocValueFetcher(docValueFormat(format, null), searchLookup.doc().getForField(this)); + } + } + public static TypeParser PARSER = new TypeParser((n, c) -> new Builder(n)); private final boolean index; diff --git a/modules/mapper-extras/src/test/java/org/elasticsearch/index/mapper/SearchAsYouTypeFieldTypeTests.java b/modules/mapper-extras/src/test/java/org/elasticsearch/index/mapper/SearchAsYouTypeFieldTypeTests.java index de5cdaa2a36..e6a15dfe290 100644 --- a/modules/mapper-extras/src/test/java/org/elasticsearch/index/mapper/SearchAsYouTypeFieldTypeTests.java +++ b/modules/mapper-extras/src/test/java/org/elasticsearch/index/mapper/SearchAsYouTypeFieldTypeTests.java @@ -34,6 +34,7 @@ import org.elasticsearch.index.mapper.SearchAsYouTypeFieldMapper.PrefixFieldType import org.elasticsearch.index.mapper.SearchAsYouTypeFieldMapper.SearchAsYouTypeFieldType; import org.elasticsearch.index.mapper.SearchAsYouTypeFieldMapper.ShingleFieldType; +import java.io.IOException; import java.util.Collections; import static java.util.Arrays.asList; @@ -109,4 +110,25 @@ public class SearchAsYouTypeFieldTypeTests extends FieldTypeTestCase { assertEquals("[prefix] queries cannot be executed when 'search.allow_expensive_queries' is set to false. " + "For optimised prefix queries on text fields please enable [index_prefixes].", ee.getMessage()); } + + public void testFetchSourceValue() throws IOException { + SearchAsYouTypeFieldType fieldType = createFieldType(); + fieldType.setIndexAnalyzer(Lucene.STANDARD_ANALYZER); + + assertEquals(org.elasticsearch.common.collect.List.of("value"), fetchSourceValue(fieldType, "value")); + assertEquals(org.elasticsearch.common.collect.List.of("42"), fetchSourceValue(fieldType, 42L)); + assertEquals(org.elasticsearch.common.collect.List.of("true"), fetchSourceValue(fieldType, true)); + + SearchAsYouTypeFieldMapper.PrefixFieldType prefixFieldType = new SearchAsYouTypeFieldMapper.PrefixFieldType( + fieldType.name(), fieldType.getTextSearchInfo(), 2, 10); + assertEquals(org.elasticsearch.common.collect.List.of("value"), fetchSourceValue(prefixFieldType, "value")); + assertEquals(org.elasticsearch.common.collect.List.of("42"), fetchSourceValue(prefixFieldType, 42L)); + assertEquals(org.elasticsearch.common.collect.List.of("true"), fetchSourceValue(prefixFieldType, true)); + + SearchAsYouTypeFieldMapper.ShingleFieldType shingleFieldType = new SearchAsYouTypeFieldMapper.ShingleFieldType( + fieldType.name(), 5, fieldType.getTextSearchInfo()); + assertEquals(org.elasticsearch.common.collect.List.of("value"), fetchSourceValue(shingleFieldType, "value")); + assertEquals(org.elasticsearch.common.collect.List.of("42"), fetchSourceValue(shingleFieldType, 42L)); + assertEquals(org.elasticsearch.common.collect.List.of("true"), fetchSourceValue(shingleFieldType, true)); + } } diff --git a/rest-api-spec/build.gradle b/rest-api-spec/build.gradle index 76bc0daa2e1..a8fed883c2d 100644 --- a/rest-api-spec/build.gradle +++ b/rest-api-spec/build.gradle @@ -15,5 +15,9 @@ artifacts { restTests(new File(projectDir, "src/main/resources/rest-api-spec/test")) } +testClusters.all { + module ':modules:mapper-extras' +} + test.enabled = false jarHell.enabled = false 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 6233fce7040..b43a63398d1 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 @@ -259,3 +259,39 @@ setup: - is_true: hits.hits.0._id - is_true: hits.hits.0._source - match: { hits.hits.0.fields.date.0: "1990/12/29" } + +--- +"Test token count": + - skip: + version: " - 7.99.99" + reason: "support for token_count isn't yet backported" + - do: + indices.create: + index: test + body: + mappings: + properties: + count: + type: token_count + analyzer: standard + count_without_dv: + type: token_count + analyzer: standard + doc_values: false + + - do: + index: + index: test + id: 1 + refresh: true + body: + count: "some text" + - do: + search: + index: test + body: + fields: [count, count_without_dv] + + - is_true: hits.hits.0._id + - match: { hits.hits.0.fields.count: [2] } + - is_false: hits.hits.0.fields.count_without_dv diff --git a/server/src/main/java/org/elasticsearch/index/mapper/MappedFieldType.java b/server/src/main/java/org/elasticsearch/index/mapper/MappedFieldType.java index bbde6516fbd..5965ec74a70 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/MappedFieldType.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/MappedFieldType.java @@ -102,6 +102,10 @@ public abstract class MappedFieldType { /** * Create a helper class to fetch field values during the {@link FetchFieldsPhase}. + * + * New field types must implement this method in order to support the search 'fields' option. Except + * for metadata fields, field types should not throw {@link UnsupportedOperationException} since this + * could cause a search retrieving multiple fields (like "fields": ["*"]) to fail. */ public abstract ValueFetcher valueFetcher(MapperService mapperService, SearchLookup searchLookup, @Nullable String format); diff --git a/server/src/main/java/org/elasticsearch/index/mapper/NumberFieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/NumberFieldMapper.java index da7b9e1b84b..bf9d82b7e62 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/NumberFieldMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/NumberFieldMapper.java @@ -889,7 +889,7 @@ public class NumberFieldMapper extends ParametrizedFieldMapper { } } - public static final class NumberFieldType extends SimpleMappedFieldType { + public static class NumberFieldType extends SimpleMappedFieldType { private final NumberType type; private final boolean coerce; diff --git a/server/src/main/java/org/elasticsearch/index/mapper/TextFieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/TextFieldMapper.java index f92c0619ca3..be78777640c 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/TextFieldMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/TextFieldMapper.java @@ -447,11 +447,11 @@ public class TextFieldMapper extends ParametrizedFieldMapper { } } - private static final class PhraseFieldType extends StringFieldType { + static final class PhraseFieldType extends StringFieldType { final TextFieldType parent; - private PhraseFieldType(TextFieldType parent) { + PhraseFieldType(TextFieldType parent) { super(parent.name() + FAST_PHRASE_SUFFIX, true, false, false, parent.getTextSearchInfo(), Collections.emptyMap()); setAnalyzer(parent.indexAnalyzer().name(), parent.indexAnalyzer().analyzer()); this.parent = parent; @@ -468,7 +468,9 @@ public class TextFieldMapper extends ParametrizedFieldMapper { @Override public ValueFetcher valueFetcher(MapperService mapperService, SearchLookup searchLookup, String format) { - throw new UnsupportedOperationException(); + // Because this internal field is modelled as a multi-field, SourceValueFetcher will look up its + // parent field in _source. So we don't need to use the parent field name here. + return SourceValueFetcher.toString(name(), mapperService, format); } @Override @@ -496,7 +498,9 @@ public class TextFieldMapper extends ParametrizedFieldMapper { @Override public ValueFetcher valueFetcher(MapperService mapperService, SearchLookup searchLookup, String format) { - throw new UnsupportedOperationException(); + // Because this internal field is modelled as a multi-field, SourceValueFetcher will look up its + // parent field in _source. So we don't need to use the parent field name here. + return SourceValueFetcher.toString(name(), mapperService, format); } void setAnalyzer(NamedAnalyzer delegate) { diff --git a/server/src/test/java/org/elasticsearch/index/mapper/TextFieldTypeTests.java b/server/src/test/java/org/elasticsearch/index/mapper/TextFieldTypeTests.java index 7388728c36e..277d6b8101d 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/TextFieldTypeTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/TextFieldTypeTests.java @@ -35,12 +35,9 @@ import org.apache.lucene.util.automaton.Automata; import org.apache.lucene.util.automaton.Automaton; import org.apache.lucene.util.automaton.Operations; import org.elasticsearch.ElasticsearchException; -import org.elasticsearch.Version; -import org.elasticsearch.cluster.metadata.IndexMetadata; import org.elasticsearch.common.lucene.BytesRefs; import org.elasticsearch.common.lucene.Lucene; import org.elasticsearch.common.lucene.search.AutomatonQueries; -import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.unit.Fuzziness; import org.elasticsearch.index.mapper.TextFieldMapper.TextFieldType; @@ -167,13 +164,21 @@ public class TextFieldTypeTests extends FieldTypeTestCase { } public void testFetchSourceValue() throws IOException { - Settings settings = Settings.builder().put(IndexMetadata.SETTING_VERSION_CREATED, Version.CURRENT.id).build(); - Mapper.BuilderContext context = new Mapper.BuilderContext(settings, new ContentPath()); + TextFieldType fieldType = createFieldType(); + fieldType.setIndexAnalyzer(Lucene.STANDARD_ANALYZER); - MappedFieldType mapper = new TextFieldMapper.Builder("field", () -> Lucene.STANDARD_ANALYZER).build(context).fieldType(); + assertEquals(org.elasticsearch.common.collect.List.of("value"), fetchSourceValue(fieldType, "value")); + assertEquals(org.elasticsearch.common.collect.List.of("42"), fetchSourceValue(fieldType, 42L)); + assertEquals(org.elasticsearch.common.collect.List.of("true"), fetchSourceValue(fieldType, true)); - assertEquals(Collections.singletonList("value"), fetchSourceValue(mapper, "value")); - assertEquals(Collections.singletonList("42"), fetchSourceValue(mapper, 42L)); - assertEquals(Collections.singletonList("true"), fetchSourceValue(mapper, true)); + TextFieldMapper.PrefixFieldType prefixFieldType = new TextFieldMapper.PrefixFieldType(fieldType, "field._index_prefix", 2, 10); + assertEquals(org.elasticsearch.common.collect.List.of("value"), fetchSourceValue(prefixFieldType, "value")); + assertEquals(org.elasticsearch.common.collect.List.of("42"), fetchSourceValue(prefixFieldType, 42L)); + assertEquals(org.elasticsearch.common.collect.List.of("true"), fetchSourceValue(prefixFieldType, true)); + + TextFieldMapper.PhraseFieldType phraseFieldType = new TextFieldMapper.PhraseFieldType(fieldType); + assertEquals(org.elasticsearch.common.collect.List.of("value"), fetchSourceValue(phraseFieldType, "value")); + assertEquals(org.elasticsearch.common.collect.List.of("42"), fetchSourceValue(phraseFieldType, 42L)); + assertEquals(org.elasticsearch.common.collect.List.of("true"), fetchSourceValue(phraseFieldType, true)); } } 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 6c8f2e2a026..c3d8f83c2bf 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 @@ -33,6 +33,7 @@ import java.io.IOException; import java.util.List; import java.util.Map; +import static org.hamcrest.Matchers.containsInAnyOrder; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.hasItems; @@ -359,6 +360,34 @@ public class FieldFetcherTests extends ESSingleNodeTestCase { assertFalse(fields.containsKey("object")); } + public void testTextSubFields() throws IOException { + XContentBuilder mapping = XContentFactory.jsonBuilder().startObject() + .startObject("properties") + .startObject("field") + .field("type", "text") + .startObject("index_prefixes").endObject() + .field("index_phrases", true) + .endObject() + .endObject() + .endObject(); + + IndexService indexService = createIndex("index", Settings.EMPTY, MapperService.SINGLE_MAPPING_NAME, mapping); + MapperService mapperService = indexService.mapperService(); + + XContentBuilder source = XContentFactory.jsonBuilder().startObject() + .array("field", "some text") + .endObject(); + + Map fields = fetchFields(mapperService, source, "*"); + assertThat(fields.size(), equalTo(3)); + assertThat(fields.keySet(), containsInAnyOrder("field", "field._index_prefix", "field._index_phrase")); + + for (DocumentField field : fields.values()) { + assertThat(field.getValues().size(), equalTo(1)); + assertThat(field.getValue(), equalTo("some text")); + } + } + private Map fetchFields(MapperService mapperService, XContentBuilder source, String fieldPattern) throws IOException { 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 24aeb8ed8c4..b837b2095d3 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 @@ -51,7 +51,6 @@ public abstract class FieldTypeTestCase extends ESTestCase { public static List fetchSourceValue(MappedFieldType fieldType, Object sourceValue, String format) throws IOException { String field = fieldType.name(); - MapperService mapperService = mock(MapperService.class); when(mapperService.sourcePath(field)).thenReturn(org.elasticsearch.common.collect.Set.of(field)); @@ -60,5 +59,4 @@ public abstract class FieldTypeTestCase extends ESTestCase { lookup.setSource(Collections.singletonMap(field, sourceValue)); return fetcher.fetchValues(lookup); } - }