From 27f7f4c4fe4405eebfef73da1987077b60adbef1 Mon Sep 17 00:00:00 2001 From: Luca Cavanna Date: Wed, 7 Oct 2020 16:10:59 +0200 Subject: [PATCH 1/2] Move unmapped type handling to QueryShardContext `MapperService` exposes an `unmappedFieldType` method that builds and caches anonoymous field types created when sorting on unmapped fields, based on the unmapped type being specified in the search request. Such method is only called through `QueryShardContext`, hence it can be moved out of `MapperService` and its logic can be simplified by removing the caching of unmapped field types. --- .../index/mapper/MapperService.java | 32 ------------------- .../index/query/QueryShardContext.java | 21 ++++++++++++ .../search/sort/FieldSortBuilder.java | 2 +- .../search/sort/GeoDistanceSortBuilder.java | 3 +- .../index/mapper/MapperServiceTests.java | 9 ------ .../index/query/QueryShardContextTests.java | 18 +++++++++++ 6 files changed, 42 insertions(+), 43 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java b/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java index 2c714e2882803..54edc56ab03f9 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java @@ -46,7 +46,6 @@ import org.elasticsearch.index.analysis.ReloadableCustomAnalyzer; import org.elasticsearch.index.analysis.TokenFilterFactory; import org.elasticsearch.index.analysis.TokenizerFactory; -import org.elasticsearch.index.mapper.Mapper.BuilderContext; import org.elasticsearch.index.query.QueryShardContext; import org.elasticsearch.index.similarity.SimilarityService; import org.elasticsearch.indices.IndicesModule; @@ -57,7 +56,6 @@ import java.io.IOException; import java.util.ArrayList; import java.util.Collections; -import java.util.HashMap; import java.util.List; import java.util.Map; import java.util.Set; @@ -65,9 +63,6 @@ import java.util.function.Function; import java.util.function.Supplier; -import static java.util.Collections.emptyMap; -import static java.util.Collections.unmodifiableMap; - public class MapperService extends AbstractIndexComponent implements Closeable { /** @@ -118,8 +113,6 @@ public enum MergeReason { private final MapperAnalyzerWrapper searchAnalyzer; private final MapperAnalyzerWrapper searchQuoteAnalyzer; - private volatile Map unmappedFieldTypes = emptyMap(); - final MapperRegistry mapperRegistry; private final BooleanSupplier idFieldDataEnabled; @@ -428,30 +421,6 @@ public ObjectMapper getObjectMapper(String name) { return this.mapper == null ? null : this.mapper.objectMappers().get(name); } - /** - * Given a type (eg. long, string, ...), return an anonymous field mapper that can be used for search operations. - */ - public MappedFieldType unmappedFieldType(String type) { - MappedFieldType fieldType = unmappedFieldTypes.get(type); - if (fieldType == null) { - final Mapper.TypeParser.ParserContext parserContext = documentMapperParser().parserContext(); - Mapper.TypeParser typeParser = parserContext.typeParser(type); - if (typeParser == null) { - throw new IllegalArgumentException("No mapper found for type [" + type + "]"); - } - final Mapper.Builder builder = typeParser.parse("__anonymous_" + type, emptyMap(), parserContext); - final BuilderContext builderContext = new BuilderContext(indexSettings.getSettings(), new ContentPath(1)); - fieldType = ((FieldMapper)builder.build(builderContext)).fieldType(); - - // There is no need to synchronize writes here. In the case of concurrent access, we could just - // compute some mappers several times, which is not a big deal - Map newUnmappedFieldTypes = new HashMap<>(unmappedFieldTypes); - newUnmappedFieldTypes.put(type, fieldType); - unmappedFieldTypes = unmodifiableMap(newUnmappedFieldTypes); - } - return fieldType; - } - public Analyzer indexAnalyzer() { return this.indexAnalyzer; } @@ -544,5 +513,4 @@ public synchronized List reloadSearchAnalyzers(AnalysisRegistry registry } return reloadedAnalyzers; } - } diff --git a/server/src/main/java/org/elasticsearch/index/query/QueryShardContext.java b/server/src/main/java/org/elasticsearch/index/query/QueryShardContext.java index 4f79550307482..33f3adfb4c743 100644 --- a/server/src/main/java/org/elasticsearch/index/query/QueryShardContext.java +++ b/server/src/main/java/org/elasticsearch/index/query/QueryShardContext.java @@ -44,6 +44,7 @@ import org.elasticsearch.index.cache.bitset.BitsetFilterCache; import org.elasticsearch.index.fielddata.IndexFieldData; import org.elasticsearch.index.mapper.ContentPath; +import org.elasticsearch.index.mapper.FieldMapper; import org.elasticsearch.index.mapper.MappedFieldType; import org.elasticsearch.index.mapper.Mapper; import org.elasticsearch.index.mapper.MapperService; @@ -61,6 +62,7 @@ import org.elasticsearch.transport.RemoteClusterAware; import java.io.IOException; +import java.util.Collections; import java.util.HashMap; import java.util.List; import java.util.Map; @@ -276,6 +278,25 @@ public Analyzer getSearchQuoteAnalyzer(MappedFieldType fieldType) { return mapperService.searchQuoteAnalyzer(); } + /** + * Given a type (eg. long, string, ...), returns an anonymous field type that can be used for search operations. + * Generally used to handle unmapped fields in the context of sorting. + */ + public MappedFieldType buildAnonymousFieldType(String type) { + final Mapper.TypeParser.ParserContext parserContext = mapperService.documentMapperParser().parserContext(); + Mapper.TypeParser typeParser = parserContext.typeParser(type); + if (typeParser == null) { + throw new IllegalArgumentException("No mapper found for type [" + type + "]"); + } + final Mapper.Builder builder = typeParser.parse("__anonymous_" + type, Collections.emptyMap(), parserContext); + final Mapper.BuilderContext builderContext = new Mapper.BuilderContext(indexSettings.getSettings(), new ContentPath(1)); + Mapper mapper = builder.build(builderContext); + if (mapper instanceof FieldMapper) { + return ((FieldMapper)mapper).fieldType(); + } + throw new IllegalArgumentException("Mapper for type [" + type + "] must be a leaf field"); + } + public IndexAnalyzers getIndexAnalyzers() { return mapperService.getIndexAnalyzers(); } diff --git a/server/src/main/java/org/elasticsearch/search/sort/FieldSortBuilder.java b/server/src/main/java/org/elasticsearch/search/sort/FieldSortBuilder.java index 756d7fd320198..694188094a041 100644 --- a/server/src/main/java/org/elasticsearch/search/sort/FieldSortBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/sort/FieldSortBuilder.java @@ -449,7 +449,7 @@ private MappedFieldType resolveUnmappedType(QueryShardContext context) { if (unmappedType == null) { throw new QueryShardException(context, "No mapping found for [" + fieldName + "] in order to sort on"); } - return context.getMapperService().unmappedFieldType(unmappedType); + return context.buildAnonymousFieldType(unmappedType); } private MultiValueMode localSortMode() { diff --git a/server/src/main/java/org/elasticsearch/search/sort/GeoDistanceSortBuilder.java b/server/src/main/java/org/elasticsearch/search/sort/GeoDistanceSortBuilder.java index 634c752599244..18565a3aadbf1 100644 --- a/server/src/main/java/org/elasticsearch/search/sort/GeoDistanceSortBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/sort/GeoDistanceSortBuilder.java @@ -57,6 +57,7 @@ import org.elasticsearch.index.query.QueryShardContext; import org.elasticsearch.search.DocValueFormat; import org.elasticsearch.search.MultiValueMode; +import org.elasticsearch.search.aggregations.support.CoreValuesSourceType; import java.io.IOException; import java.util.ArrayList; @@ -569,7 +570,7 @@ private IndexGeoPointFieldData fieldData(QueryShardContext context) { MappedFieldType fieldType = context.getFieldType(fieldName); if (fieldType == null) { if (ignoreUnmapped) { - fieldType = context.getMapperService().unmappedFieldType("geo_point"); + return new LatLonPointIndexFieldData(fieldName, CoreValuesSourceType.GEOPOINT); } else { throw new IllegalArgumentException("failed to find mapper for [" + fieldName + "] for geo distance based sort"); } diff --git a/server/src/test/java/org/elasticsearch/index/mapper/MapperServiceTests.java b/server/src/test/java/org/elasticsearch/index/mapper/MapperServiceTests.java index e0b193b989042..f4fd5267399a1 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/MapperServiceTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/MapperServiceTests.java @@ -36,9 +36,7 @@ import org.elasticsearch.index.analysis.NamedAnalyzer; import org.elasticsearch.index.analysis.ReloadableCustomAnalyzer; import org.elasticsearch.index.analysis.TokenFilterFactory; -import org.elasticsearch.index.mapper.KeywordFieldMapper.KeywordFieldType; import org.elasticsearch.index.mapper.MapperService.MergeReason; -import org.elasticsearch.index.mapper.NumberFieldMapper.NumberFieldType; import org.elasticsearch.indices.analysis.AnalysisModule.AnalysisProvider; import org.elasticsearch.plugins.AnalysisPlugin; import org.elasticsearch.plugins.Plugin; @@ -52,7 +50,6 @@ import java.util.Map; import static org.hamcrest.CoreMatchers.containsString; -import static org.hamcrest.Matchers.instanceOf; import static org.hamcrest.Matchers.notNullValue; import static org.hamcrest.Matchers.nullValue; @@ -126,12 +123,6 @@ public void testMappingDepthExceedsLimit() throws Throwable { assertThat(e.getMessage(), containsString("Limit of mapping depth [1] has been exceeded")); } - public void testUnmappedFieldType() { - MapperService mapperService = createIndex("index").mapperService(); - assertThat(mapperService.unmappedFieldType("keyword"), instanceOf(KeywordFieldType.class)); - assertThat(mapperService.unmappedFieldType("long"), instanceOf(NumberFieldType.class)); - } - public void testPartitionedConstraints() { // partitioned index must have routing IllegalArgumentException noRoutingException = expectThrows(IllegalArgumentException.class, () -> { diff --git a/server/src/test/java/org/elasticsearch/index/query/QueryShardContextTests.java b/server/src/test/java/org/elasticsearch/index/query/QueryShardContextTests.java index 8750f6ad7e05d..6a42e5449b2eb 100644 --- a/server/src/test/java/org/elasticsearch/index/query/QueryShardContextTests.java +++ b/server/src/test/java/org/elasticsearch/index/query/QueryShardContextTests.java @@ -50,10 +50,15 @@ import org.elasticsearch.index.fielddata.LeafFieldData; import org.elasticsearch.index.fielddata.ScriptDocValues; import org.elasticsearch.index.fielddata.plain.AbstractLeafOrdinalsFieldData; +import org.elasticsearch.index.mapper.DocumentMapperParser; import org.elasticsearch.index.mapper.IndexFieldMapper; +import org.elasticsearch.index.mapper.KeywordFieldMapper; import org.elasticsearch.index.mapper.MappedFieldType; +import org.elasticsearch.index.mapper.Mapper; import org.elasticsearch.index.mapper.MapperService; +import org.elasticsearch.index.mapper.NumberFieldMapper; import org.elasticsearch.index.mapper.TextFieldMapper; +import org.elasticsearch.indices.IndicesModule; import org.elasticsearch.search.lookup.LeafDocLookup; import org.elasticsearch.search.lookup.LeafSearchLookup; import org.elasticsearch.search.lookup.SearchLookup; @@ -63,6 +68,7 @@ import java.util.ArrayList; import java.util.Collections; import java.util.List; +import java.util.Map; import java.util.function.BiFunction; import java.util.function.Supplier; @@ -102,6 +108,12 @@ public void testFailIfFieldMappingNotFound() { assertThat(result.name(), equalTo("name")); } + public void testUnmappedFieldType() { + QueryShardContext context = createQueryShardContext("uuid", null); + assertThat(context.buildAnonymousFieldType("keyword"), instanceOf(KeywordFieldMapper.KeywordFieldType.class)); + assertThat(context.buildAnonymousFieldType("long"), instanceOf(NumberFieldMapper.NumberFieldType.class)); + } + public void testToQueryFails() { QueryShardContext context = createQueryShardContext(IndexMetadata.INDEX_UUID_NA_VALUE, null); Exception exc = expectThrows(Exception.class, @@ -300,6 +312,12 @@ private static QueryShardContext createQueryShardContext(String indexUuid, Strin when(mapperService.getIndexSettings()).thenReturn(indexSettings); when(mapperService.index()).thenReturn(indexMetadata.getIndex()); when(mapperService.getIndexAnalyzers()).thenReturn(indexAnalyzers); + DocumentMapperParser documentMapperParser = mock(DocumentMapperParser.class); + Map typeParserMap = IndicesModule.getMappers(Collections.emptyList()); + Mapper.TypeParser.ParserContext parserContext = new Mapper.TypeParser.ParserContext(name -> null, mapperService, + typeParserMap::get, Version.CURRENT, () -> null, null, null); + when(documentMapperParser.parserContext()).thenReturn(parserContext); + when(mapperService.documentMapperParser()).thenReturn(documentMapperParser); if (runtimeDocValues != null) { when(mapperService.fieldType(any())).thenAnswer(fieldTypeInv -> { String fieldName = (String)fieldTypeInv.getArguments()[0]; From ae517e20ebee9a7b42bffc3c56ca57c49a450a71 Mon Sep 17 00:00:00 2001 From: Luca Cavanna Date: Wed, 7 Oct 2020 16:13:59 +0200 Subject: [PATCH 2/2] iter --- .../org/elasticsearch/index/query/QueryShardContextTests.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/src/test/java/org/elasticsearch/index/query/QueryShardContextTests.java b/server/src/test/java/org/elasticsearch/index/query/QueryShardContextTests.java index 6a42e5449b2eb..8ec0a9065346e 100644 --- a/server/src/test/java/org/elasticsearch/index/query/QueryShardContextTests.java +++ b/server/src/test/java/org/elasticsearch/index/query/QueryShardContextTests.java @@ -108,7 +108,7 @@ public void testFailIfFieldMappingNotFound() { assertThat(result.name(), equalTo("name")); } - public void testUnmappedFieldType() { + public void testBuildAnonymousFieldType() { QueryShardContext context = createQueryShardContext("uuid", null); assertThat(context.buildAnonymousFieldType("keyword"), instanceOf(KeywordFieldMapper.KeywordFieldType.class)); assertThat(context.buildAnonymousFieldType("long"), instanceOf(NumberFieldMapper.NumberFieldType.class));