From 5b87f776840ed001838dda983aaf5664504ab4f6 Mon Sep 17 00:00:00 2001 From: Luca Cavanna Date: Wed, 2 Jun 2021 12:02:40 +0200 Subject: [PATCH 1/2] Remove getMatchingFieldTypes method FieldTypeLookup and MappingLookup expose the getMatchingFieldTypes method to look up matching field type by a string pattern. We have migrated ExistsQueryBuilder to instead rely on getMatchingFieldNames, hence we can go ahead and remove the remaining usages and the method itself. The remaining usages are to find specific field types from the mappings, specifically to eagerly load global ordinals and for the join field type. These are operations that only needs access to the index time field types, which makes it much simpler to support. We already have a specific index time lookup within MappingLookup, which we can reuse for this. --- .../AggConstructionContentionBenchmark.java | 3 +- .../org/elasticsearch/join/mapper/Joiner.java | 16 ++-- .../join/mapper/ParentJoinFieldMapper.java | 27 +----- .../mapper/ParentJoinFieldMapperTests.java | 6 +- .../index/mapper/FieldTypeLookup.java | 57 +++--------- .../index/mapper/MapperService.java | 5 +- .../index/mapper/MappingLookup.java | 26 ++---- .../index/query/SearchExecutionContext.java | 44 ++-------- .../support/AggregationContext.java | 15 ++-- .../index/mapper/FieldTypeLookupTests.java | 87 ++++++++----------- .../query/SearchExecutionContextTests.java | 20 +++-- .../index/mapper/MapperServiceTestCase.java | 6 +- 12 files changed, 103 insertions(+), 209 deletions(-) diff --git a/benchmarks/src/main/java/org/elasticsearch/benchmark/search/aggregations/AggConstructionContentionBenchmark.java b/benchmarks/src/main/java/org/elasticsearch/benchmark/search/aggregations/AggConstructionContentionBenchmark.java index 5b5efa14ed092..2eee0cb11d16e 100644 --- a/benchmarks/src/main/java/org/elasticsearch/benchmark/search/aggregations/AggConstructionContentionBenchmark.java +++ b/benchmarks/src/main/java/org/elasticsearch/benchmark/search/aggregations/AggConstructionContentionBenchmark.java @@ -72,6 +72,7 @@ import java.util.Optional; import java.util.concurrent.TimeUnit; import java.util.function.Function; +import java.util.function.Predicate; /** * Benchmarks the overhead of constructing {@link Aggregator}s in many @@ -213,7 +214,7 @@ public MappedFieldType getFieldType(String path) { } @Override - public Collection getMatchingFieldTypes(String pattern) { + public Collection getIndexTimeFieldTypes(Predicate predicate) { throw new UnsupportedOperationException(); } diff --git a/modules/parent-join/src/main/java/org/elasticsearch/join/mapper/Joiner.java b/modules/parent-join/src/main/java/org/elasticsearch/join/mapper/Joiner.java index 86805cbb6bcb4..ae4644dc05095 100644 --- a/modules/parent-join/src/main/java/org/elasticsearch/join/mapper/Joiner.java +++ b/modules/parent-join/src/main/java/org/elasticsearch/join/mapper/Joiner.java @@ -24,8 +24,11 @@ import java.util.List; import java.util.Map; import java.util.Objects; +import java.util.Optional; import java.util.Set; import java.util.function.Consumer; +import java.util.function.Function; +import java.util.function.Predicate; /** * Utility class to help build join queries and aggregations, based on a join_field @@ -36,24 +39,24 @@ public final class Joiner { * Get the Joiner for this context, or {@code null} if none is configured */ public static Joiner getJoiner(SearchExecutionContext context) { - return getJoiner(context.getAllFieldTypes()); + return getJoiner(context::getIndexTimeFieldTypes); } /** * Get the Joiner for this context, or {@code null} if none is configured */ public static Joiner getJoiner(AggregationContext context) { - return getJoiner(context.getMatchingFieldTypes("*")); + return getJoiner(context::getIndexTimeFieldTypes); } /** * Get the Joiner for this context, or {@code null} if none is configured */ - static Joiner getJoiner(Collection fieldTypes) { - JoinFieldType ft = ParentJoinFieldMapper.getJoinFieldType(fieldTypes); - return ft != null ? ft.getJoiner() : null; + static Joiner getJoiner(Function, Collection> fieldTypeLookup) { + Optional joinFieldType = fieldTypeLookup.apply(ft -> ft instanceof JoinFieldType) + .stream().map(ft -> (JoinFieldType) ft).findFirst(); + return joinFieldType.map(JoinFieldType::getJoiner).orElse(null); } - private final Map> parentsToChildren = new HashMap<>(); private final Map childrenToParents = new HashMap<>(); @@ -178,5 +181,4 @@ boolean canMerge(Joiner other, Consumer conflicts) { } return conflicted == false; } - } diff --git a/modules/parent-join/src/main/java/org/elasticsearch/join/mapper/ParentJoinFieldMapper.java b/modules/parent-join/src/main/java/org/elasticsearch/join/mapper/ParentJoinFieldMapper.java index 3ed87549076ba..d290d134f94ba 100644 --- a/modules/parent-join/src/main/java/org/elasticsearch/join/mapper/ParentJoinFieldMapper.java +++ b/modules/parent-join/src/main/java/org/elasticsearch/join/mapper/ParentJoinFieldMapper.java @@ -36,7 +36,6 @@ import java.io.IOException; import java.util.ArrayList; import java.util.Arrays; -import java.util.Collection; import java.util.Collections; import java.util.HashMap; import java.util.Iterator; @@ -291,31 +290,11 @@ public FieldMapper.Builder getMergeBuilder() { } @Override - protected void doValidate(MappingLookup mappers) { - List joinFields = getJoinFieldTypes(mappers.getAllFieldTypes()).stream() - .map(JoinFieldType::name) - .collect(Collectors.toList()); + protected void doValidate(MappingLookup mappingLookup) { + List joinFields = mappingLookup.getIndexTimeFieldTypes(ft -> ft instanceof JoinFieldType) + .stream().map(MappedFieldType::name).collect(Collectors.toList()); if (joinFields.size() > 1) { throw new IllegalArgumentException("Only one [parent-join] field can be defined per index, got " + joinFields); } } - - static JoinFieldType getJoinFieldType(Collection fieldTypes) { - for (MappedFieldType ft : fieldTypes) { - if (ft instanceof JoinFieldType) { - return (JoinFieldType) ft; - } - } - return null; - } - - private List getJoinFieldTypes(Collection fieldTypes) { - final List joinFields = new ArrayList<>(); - for (MappedFieldType ft : fieldTypes) { - if (ft instanceof JoinFieldType) { - joinFields.add((JoinFieldType) ft); - } - } - return joinFields; - } } diff --git a/modules/parent-join/src/test/java/org/elasticsearch/join/mapper/ParentJoinFieldMapperTests.java b/modules/parent-join/src/test/java/org/elasticsearch/join/mapper/ParentJoinFieldMapperTests.java index 2a22aa0c78f98..362ec9825d1a4 100644 --- a/modules/parent-join/src/test/java/org/elasticsearch/join/mapper/ParentJoinFieldMapperTests.java +++ b/modules/parent-join/src/test/java/org/elasticsearch/join/mapper/ParentJoinFieldMapperTests.java @@ -47,7 +47,7 @@ public void testSingleLevel() throws Exception { })); DocumentMapper docMapper = mapperService.documentMapper(); - Joiner joiner = Joiner.getJoiner(mapperService.mappingLookup().getAllFieldTypes()); + Joiner joiner = Joiner.getJoiner(mapperService.mappingLookup()::getIndexTimeFieldTypes); assertNotNull(joiner); assertEquals("join_field", joiner.getJoinField()); @@ -233,7 +233,7 @@ public void testUpdateRelations() throws Exception { b.endObject(); })); - Joiner joiner = Joiner.getJoiner(mapperService.mappingLookup().getAllFieldTypes()); + Joiner joiner = Joiner.getJoiner(mapperService.mappingLookup()::getIndexTimeFieldTypes); assertNotNull(joiner); assertEquals("join_field", joiner.getJoinField()); assertTrue(joiner.childTypeExists("child2")); @@ -259,7 +259,7 @@ public void testUpdateRelations() throws Exception { } b.endObject(); })); - joiner = Joiner.getJoiner(mapperService.mappingLookup().getAllFieldTypes()); + joiner = Joiner.getJoiner(mapperService.mappingLookup()::getIndexTimeFieldTypes); assertNotNull(joiner); assertEquals("join_field", joiner.getJoinField()); assertTrue(joiner.childTypeExists("child2")); diff --git a/server/src/main/java/org/elasticsearch/index/mapper/FieldTypeLookup.java b/server/src/main/java/org/elasticsearch/index/mapper/FieldTypeLookup.java index ed78d0d12fd51..c2ec303514860 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/FieldTypeLookup.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/FieldTypeLookup.java @@ -10,15 +10,15 @@ import org.elasticsearch.common.regex.Regex; -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.Objects; import java.util.Set; +import java.util.function.Predicate; +import java.util.stream.Collectors; /** * An immutable container for looking up {@link MappedFieldType}s by their name. @@ -147,49 +147,6 @@ private MappedFieldType getDynamicField(String field) { } } - /** - * Returns all the mapped field types that match a pattern - * - * Note that if a field is aliased and both its actual name and its alias - * match the pattern, the returned collection will contain the field type - * twice. - */ - Collection getMatchingFieldTypes(String pattern) { - if (Regex.isMatchAllPattern(pattern)) { - if (dynamicFieldTypes.isEmpty()) { - return fullNameToFieldType.values(); - } - List fieldTypes = new ArrayList<>(fullNameToFieldType.values()); - for (DynamicFieldType dynamicFieldType : dynamicFieldTypes.values()) { - for (String subfield : dynamicFieldType.getKnownSubfields()) { - fieldTypes.add(dynamicFieldType.getChildFieldType(subfield)); - } - } - return fieldTypes; - } - if (Regex.isSimpleMatchPattern(pattern) == false) { - // no wildcards - MappedFieldType ft = get(pattern); - return ft == null ? Collections.emptySet() : Collections.singleton(ft); - } - List matchingFields = new ArrayList<>(); - for (String field : fullNameToFieldType.keySet()) { - if (Regex.simpleMatch(pattern, field)) { - matchingFields.add(fullNameToFieldType.get(field)); - } - } - for (Map.Entry dynamicFieldTypeEntry : dynamicFieldTypes.entrySet()) { - String parentName = dynamicFieldTypeEntry.getKey(); - DynamicFieldType dynamicFieldType = dynamicFieldTypeEntry.getValue(); - for (String subfield : dynamicFieldType.getKnownSubfields()) { - if (Regex.simpleMatch(pattern, parentName + "." + subfield)) { - matchingFields.add(dynamicFieldType.getChildFieldType(subfield)); - } - } - } - return matchingFields; - } - /** * Returns a set of field names that match a regex-like pattern * @@ -268,4 +225,14 @@ Set sourcePaths(String field) { ? fieldToCopiedFields.get(resolvedField) : Set.of(resolvedField); } + + /** + * Returns the field types that match the provided predicate. + * Note that this is not going to include fields that are dynamically exposed through {@link DynamicFieldType}. + * @param predicate the predicate + * @return the matching field types + */ + Collection getFieldTypes(Predicate predicate) { + return fullNameToFieldType.values().stream().filter(predicate).collect(Collectors.toList()); + } } 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 03d2ae8ef689e..1e22e477bdaa5 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java @@ -48,7 +48,6 @@ import java.util.function.BooleanSupplier; import java.util.function.Function; import java.util.function.Supplier; -import java.util.stream.Collectors; public class MapperService extends AbstractIndexComponent implements Closeable { @@ -373,9 +372,7 @@ public Iterable getEagerGlobalOrdinalsFields() { if (this.mapper == null) { return Collections.emptySet(); } - return this.mapper.mappers().getAllFieldTypes().stream() - .filter(MappedFieldType::eagerGlobalOrdinals) - .collect(Collectors.toList()); + return this.mapper.mappers().getIndexTimeFieldTypes(MappedFieldType::eagerGlobalOrdinals); } /** diff --git a/server/src/main/java/org/elasticsearch/index/mapper/MappingLookup.java b/server/src/main/java/org/elasticsearch/index/mapper/MappingLookup.java index b72f99237d6c9..76ba01e62e851 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/MappingLookup.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/MappingLookup.java @@ -20,6 +20,7 @@ import java.util.Map; import java.util.Set; import java.util.function.Function; +import java.util.function.Predicate; import java.util.stream.Stream; /** @@ -158,9 +159,7 @@ private MappingLookup(Mapping mapping, } this.fieldTypeLookup = new FieldTypeLookup(mappers, aliasMappers, mapping.getRoot().runtimeFields()); - this.indexTimeLookup = indexTimeScriptMappers.isEmpty() - ? null - : new FieldTypeLookup(mappers, aliasMappers, Collections.emptyList()); + this.indexTimeLookup = new FieldTypeLookup(mappers, aliasMappers, Collections.emptyList()); this.fieldMappers = Collections.unmodifiableMap(fieldMappers); this.objectMappers = Collections.unmodifiableMap(objects); } @@ -301,23 +300,14 @@ public Set getMatchingFieldNames(String pattern) { } /** - * Returns all the mapped field types that match a pattern + * Returns the index time field types matching the predicate. This is used to find specific field types among the ones defined + * under the properties section of the mappings. Runtime fields are not included. * - * Note that if a field is aliased and both its actual name and its alias - * match the pattern, the returned collection will contain the field type - * twice. - * - * @param pattern the pattern to match field names against - */ - public Collection getMatchingFieldTypes(String pattern) { - return fieldTypeLookup.getMatchingFieldTypes(pattern); - } - - /** - * @return all mapped field types + * @param predicate the predicate + * @return the matching mapped field types */ - public Collection getAllFieldTypes() { - return fieldTypeLookup.getMatchingFieldTypes("*"); + public Collection getIndexTimeFieldTypes(Predicate predicate) { + return indexTimeLookup.getFieldTypes(predicate); } /** diff --git a/server/src/main/java/org/elasticsearch/index/query/SearchExecutionContext.java b/server/src/main/java/org/elasticsearch/index/query/SearchExecutionContext.java index a6d97aec332b7..f0a679cf7dbbc 100644 --- a/server/src/main/java/org/elasticsearch/index/query/SearchExecutionContext.java +++ b/server/src/main/java/org/elasticsearch/index/query/SearchExecutionContext.java @@ -331,46 +331,14 @@ public Set getMatchingFieldNames(String pattern) { } /** - * @return all mapped field types, including runtime fields defined in the request - */ - public Collection getAllFieldTypes() { - return getMatchingFieldTypes("*"); - } - - /** - * Returns all mapped field types that match a given pattern - * - * Includes any runtime fields that have been defined in the request. Note - * that a runtime field with the same name as a mapped field will override - * the mapped field. + * Returns the index time field types matching the predicate. This is used to find specific field types among the ones defined + * under the properties section of the mappings. Runtime fields are not included. * - * @param pattern the field name pattern + * @param predicate the predicate + * @return the matching mapped field types */ - public Collection getMatchingFieldTypes(String pattern) { - Collection mappedFieldTypes = mappingLookup.getMatchingFieldTypes(pattern); - if (runtimeMappings.isEmpty()) { - return mappedFieldTypes; - } - - Map mappedByName = new HashMap<>(); - mappedFieldTypes.forEach(ft -> mappedByName.put(ft.name(), ft)); - - if ("*".equals(pattern)) { - mappedByName.putAll(runtimeMappings); - } else if (Regex.isSimpleMatchPattern(pattern) == false) { - // no wildcard - if (runtimeMappings.containsKey(pattern) == false) { - return mappedFieldTypes; - } - mappedByName.put(pattern, runtimeMappings.get(pattern)); - } else { - for (String name : runtimeMappings.keySet()) { - if (Regex.simpleMatch(pattern, name)) { - mappedByName.put(name, runtimeMappings.get(name)); - } - } - } - return mappedByName.values(); + public Collection getIndexTimeFieldTypes(Predicate predicate) { + return mappingLookup.getIndexTimeFieldTypes(predicate); } /** diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/support/AggregationContext.java b/server/src/main/java/org/elasticsearch/search/aggregations/support/AggregationContext.java index 8c6459dc0304a..b6e89e9a50a59 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/support/AggregationContext.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/support/AggregationContext.java @@ -24,8 +24,8 @@ import org.elasticsearch.index.mapper.MappedFieldType; import org.elasticsearch.index.mapper.ObjectMapper; import org.elasticsearch.index.query.QueryBuilder; -import org.elasticsearch.index.query.SearchExecutionContext; import org.elasticsearch.index.query.Rewriteable; +import org.elasticsearch.index.query.SearchExecutionContext; import org.elasticsearch.index.query.support.NestedScope; import org.elasticsearch.script.Script; import org.elasticsearch.script.ScriptContext; @@ -46,6 +46,7 @@ import java.util.Optional; import java.util.function.Function; import java.util.function.LongSupplier; +import java.util.function.Predicate; import java.util.function.Supplier; /** @@ -112,9 +113,13 @@ public final FieldContext buildFieldContext(MappedFieldType ft) { public abstract MappedFieldType getFieldType(String path); /** - * Returns the registered mapped field types. + * Returns the index time field types matching the predicate. This is used to find specific field types among the ones defined + * under the properties section of the mappings. Runtime fields are not included. + * + * @param predicate the predicate + * @return the matching mapped field types */ - public abstract Collection getMatchingFieldTypes(String pattern); + public abstract Collection getIndexTimeFieldTypes(Predicate predicate); /** * Returns true if the field identified by the provided name is mapped, false otherwise @@ -346,8 +351,8 @@ public MappedFieldType getFieldType(String path) { } @Override - public Collection getMatchingFieldTypes(String pattern) { - return context.getMatchingFieldTypes(pattern); + public Collection getIndexTimeFieldTypes(Predicate predicate) { + return context.getIndexTimeFieldTypes(predicate); } @Override diff --git a/server/src/test/java/org/elasticsearch/index/mapper/FieldTypeLookupTests.java b/server/src/test/java/org/elasticsearch/index/mapper/FieldTypeLookupTests.java index d3a6f9bbb95ac..ec22330e350ba 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/FieldTypeLookupTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/FieldTypeLookupTests.java @@ -15,14 +15,12 @@ import java.util.Arrays; import java.util.Collection; import java.util.Collections; -import java.util.HashMap; import java.util.List; -import java.util.Map; import java.util.Set; -import java.util.stream.Collectors; import static java.util.Collections.emptyList; import static java.util.Collections.singletonList; +import static org.hamcrest.CoreMatchers.equalTo; import static org.hamcrest.CoreMatchers.instanceOf; import static org.hamcrest.Matchers.contains; import static org.hamcrest.Matchers.containsInAnyOrder; @@ -56,25 +54,41 @@ public void testAddFieldAlias() { assertEquals(field.fieldType(), aliasType); } - public void testMatchingFieldNames() { + public void testGetMatchingFieldNames() { MockFieldMapper field1 = new MockFieldMapper("foo"); MockFieldMapper field2 = new MockFieldMapper("bar"); + MockFieldMapper field3 = new MockFieldMapper("baz"); FieldAliasMapper alias1 = new FieldAliasMapper("food", "food", "foo"); FieldAliasMapper alias2 = new FieldAliasMapper("barometer", "barometer", "bar"); + TestRuntimeField runtimeField = new TestRuntimeField("baz", "type"); TestDynamicRuntimeField dynamicRuntimeField = new TestDynamicRuntimeField("baro", Collections.singletonMap("meter", new TestRuntimeField("meter", "test"))); - FieldTypeLookup lookup = new FieldTypeLookup(List.of(field1, field2), List.of(alias1, alias2), List.of(dynamicRuntimeField)); + FieldTypeLookup lookup = new FieldTypeLookup(List.of(field1, field2, field3), List.of(alias1, alias2), + List.of(runtimeField, dynamicRuntimeField)); - Collection names = lookup.getMatchingFieldNames("b*"); - assertThat(names, containsInAnyOrder("bar", "barometer", "baro.meter")); - - Collection fieldTypes = lookup.getMatchingFieldTypes("b*"); - assertThat(fieldTypes, hasSize(3)); // both "bar" and "barometer" get returned as field types - Set matchedNames = fieldTypes.stream().map(MappedFieldType::name).collect(Collectors.toSet()); - assertThat(matchedNames, containsInAnyOrder("bar", "meter")); + { + Collection names = lookup.getMatchingFieldNames("*"); + assertThat(names, containsInAnyOrder("foo", "food", "bar", "baz", "barometer", "baro.meter")); + } + { + Collection names = lookup.getMatchingFieldNames("b*"); + assertThat(names, containsInAnyOrder("bar", "baz", "barometer", "baro.meter")); + } + { + Collection names = lookup.getMatchingFieldNames("baro.anything"); + assertThat(names, containsInAnyOrder("baro.anything")); + } + { + Collection names = lookup.getMatchingFieldNames("baro.any*"); + assertThat(names, hasSize(0)); + } + { + Collection names = lookup.getMatchingFieldNames("foo*"); + assertThat(names, containsInAnyOrder("foo", "food")); + } } public void testSourcePathWithMultiFields() { @@ -114,51 +128,24 @@ public void testRuntimeFieldsLookup() { assertThat(fieldTypeLookup.get("runtime"), instanceOf(TestRuntimeField.class)); } - public void testRuntimeFieldOverrides() { + public void testRuntimeFieldsOverrideConcreteFields() { + FlattenedFieldMapper flattened = createFlattenedMapper("flattened"); MockFieldMapper field = new MockFieldMapper("field"); MockFieldMapper subfield = new MockFieldMapper("object.subfield"); MockFieldMapper concrete = new MockFieldMapper("concrete"); TestRuntimeField fieldOverride = new TestRuntimeField("field", "type"); TestRuntimeField subfieldOverride = new TestRuntimeField("object.subfield", "type"); TestRuntimeField runtime = new TestRuntimeField("runtime", "type"); + TestDynamicRuntimeField dynamicRuntimeField = new TestDynamicRuntimeField("flattened", + Collections.singletonMap("sub", new TestRuntimeField("sub", "ip"))); - FieldTypeLookup fieldTypeLookup = new FieldTypeLookup(List.of(field, concrete, subfield), emptyList(), - List.of(fieldOverride, runtime, subfieldOverride)); + FieldTypeLookup fieldTypeLookup = new FieldTypeLookup(List.of(field, concrete, subfield, flattened), emptyList(), + List.of(fieldOverride, runtime, subfieldOverride, dynamicRuntimeField)); assertThat(fieldTypeLookup.get("field"), instanceOf(TestRuntimeField.class)); assertThat(fieldTypeLookup.get("object.subfield"), instanceOf(TestRuntimeField.class)); assertThat(fieldTypeLookup.get("concrete"), instanceOf(MockFieldMapper.FakeFieldType.class)); assertThat(fieldTypeLookup.get("runtime"), instanceOf(TestRuntimeField.class)); - } - - public void testRuntimeFieldsGetMatching() { - MockFieldMapper field1 = new MockFieldMapper("field1"); - MockFieldMapper shadowed = new MockFieldMapper("field2"); - MockFieldMapper concrete = new MockFieldMapper("concrete"); - TestRuntimeField field2 = new TestRuntimeField("field2", "type"); - TestRuntimeField subfield = new TestRuntimeField("object.subfield", "type"); - - FieldTypeLookup fieldTypeLookup - = new FieldTypeLookup(List.of(field1, shadowed, concrete), emptyList(), List.of(field2, subfield)); - { - Set matches = fieldTypeLookup.getMatchingFieldNames("fie*"); - assertEquals(2, matches.size()); - assertTrue(matches.contains("field1")); - assertTrue(matches.contains("field2")); - } - { - Collection matches = fieldTypeLookup.getMatchingFieldTypes("fie*"); - assertThat(matches, hasSize(2)); - Map toName = new HashMap<>(); - matches.forEach(m -> toName.put(m.name(), m)); - assertThat(toName.keySet(), hasSize(2)); - assertThat(toName.get("field2"), instanceOf(TestRuntimeField.class)); - assertThat(toName.get("field1"), instanceOf(MockFieldMapper.FakeFieldType.class)); - } - { - Set matches = fieldTypeLookup.getMatchingFieldNames("object.sub*"); - assertEquals(1, matches.size()); - assertTrue(matches.contains("object.subfield")); - } + assertThat(fieldTypeLookup.get("flattened.sub").typeName(), equalTo("ip")); } public void testRuntimeFieldsSourcePaths() { @@ -192,16 +179,12 @@ public void testDynamicRuntimeFields() { Collections.singletonList(new TestDynamicRuntimeField("test"))); assertNull(fieldTypeLookup.get("test")); - assertEquals(0, fieldTypeLookup.getMatchingFieldTypes("test").size()); assertEquals(0, fieldTypeLookup.getMatchingFieldNames("test").size()); String fieldName = "test." + randomAlphaOfLengthBetween(3, 6); assertEquals(KeywordFieldMapper.CONTENT_TYPE, fieldTypeLookup.get(fieldName).typeName()); - Collection matchingFieldTypes = fieldTypeLookup.getMatchingFieldTypes(fieldName); - assertEquals(1, matchingFieldTypes.size()); - assertEquals(KeywordFieldMapper.CONTENT_TYPE, matchingFieldTypes.iterator().next().typeName()); Set matchingFieldNames = fieldTypeLookup.getMatchingFieldNames(fieldName); - assertEquals(1, matchingFieldTypes.size()); + assertEquals(1, matchingFieldNames.size()); assertEquals(fieldName, matchingFieldNames.iterator().next()); } @@ -312,7 +295,7 @@ public void testMaxDynamicKeyDepth() { } } - private FlattenedFieldMapper createFlattenedMapper(String fieldName) { + private static FlattenedFieldMapper createFlattenedMapper(String fieldName) { return new FlattenedFieldMapper.Builder(fieldName).build(new ContentPath()); } } diff --git a/server/src/test/java/org/elasticsearch/index/query/SearchExecutionContextTests.java b/server/src/test/java/org/elasticsearch/index/query/SearchExecutionContextTests.java index df9789e07ce34..49ccc3ff64dfd 100644 --- a/server/src/test/java/org/elasticsearch/index/query/SearchExecutionContextTests.java +++ b/server/src/test/java/org/elasticsearch/index/query/SearchExecutionContextTests.java @@ -346,7 +346,8 @@ public void testSearchRequestRuntimeFields() { SearchExecutionContext context = createSearchExecutionContext( "uuid", null, - createMappingLookup(List.of(new MockFieldMapper.FakeFieldType("pig"), new MockFieldMapper.FakeFieldType("cat")), List.of()), + createMappingLookup(List.of(new MockFieldMapper.FakeFieldType("pig"), new MockFieldMapper.FakeFieldType("cat")), + List.of(new TestRuntimeField("runtime", "long"))), runtimeMappings); assertTrue(context.isFieldMapped("cat")); assertThat(context.getFieldType("cat"), instanceOf(KeywordScriptFieldType.class)); @@ -357,15 +358,18 @@ public void testSearchRequestRuntimeFields() { assertTrue(context.isFieldMapped("pig")); assertThat(context.getFieldType("pig"), instanceOf(MockFieldMapper.FakeFieldType.class)); assertThat(context.getMatchingFieldNames("pig"), equalTo(Set.of("pig"))); - assertThat(context.getMatchingFieldNames("*"), equalTo(Set.of("cat", "dog", "pig"))); + assertThat(context.getMatchingFieldNames("*"), equalTo(Set.of("cat", "dog", "pig", "runtime"))); // test that shadowed fields aren't returned by getMatchingFieldTypes - Collection matches = context.getMatchingFieldTypes("ca*"); - assertThat(matches, hasSize(1)); - assertThat(matches.iterator().next(), instanceOf(KeywordScriptFieldType.class)); - - matches = context.getAllFieldTypes(); - assertThat(matches, hasSize(3)); + Collection matches = context.getIndexTimeFieldTypes(ft -> true); + assertThat(matches, hasSize(2)); + for (MappedFieldType match : matches) { + assertThat(match, instanceOf(MockFieldMapper.FakeFieldType.class)); + } + { + Collection matchingFieldTypes = context.getIndexTimeFieldTypes(ft -> ft.name().equals("dog")); + assertThat(matchingFieldTypes, hasSize(0)); + } } public void testSearchRequestRuntimeFieldsWrongFormat() { diff --git a/test/framework/src/main/java/org/elasticsearch/index/mapper/MapperServiceTestCase.java b/test/framework/src/main/java/org/elasticsearch/index/mapper/MapperServiceTestCase.java index 9696b83d299e3..4dc11c638ba70 100644 --- a/test/framework/src/main/java/org/elasticsearch/index/mapper/MapperServiceTestCase.java +++ b/test/framework/src/main/java/org/elasticsearch/index/mapper/MapperServiceTestCase.java @@ -71,6 +71,7 @@ import java.util.Optional; import java.util.function.BooleanSupplier; import java.util.function.Function; +import java.util.function.Predicate; import static java.util.Collections.emptyList; import static java.util.stream.Collectors.toList; @@ -377,7 +378,7 @@ public MappedFieldType getFieldType(String path) { } @Override - public Collection getMatchingFieldTypes(String pattern) { + public Collection getIndexTimeFieldTypes(Predicate predicate) { throw new UnsupportedOperationException(); } @@ -521,9 +522,6 @@ protected SearchExecutionContext createSearchExecutionContext(MapperService mapp when(searchExecutionContext.getMatchingFieldNames(anyObject())).thenAnswer( inv -> mapperService.mappingLookup().getMatchingFieldNames(inv.getArguments()[0].toString()) ); - when(searchExecutionContext.getMatchingFieldTypes(anyObject())).thenAnswer( - inv -> mapperService.mappingLookup().getMatchingFieldTypes(inv.getArguments()[0].toString()) - ); when(searchExecutionContext.allowExpensiveQueries()).thenReturn(true); when(searchExecutionContext.lookup()).thenReturn(new SearchLookup(mapperService::fieldType, (ft, s) -> { throw new UnsupportedOperationException("search lookup not available"); From 6ba4907b996dbba09bacd1e2c292d577085b1a2f Mon Sep 17 00:00:00 2001 From: Luca Cavanna Date: Wed, 2 Jun 2021 17:56:30 +0200 Subject: [PATCH 2/2] remove getIndexTimeFieldTypes --- .../AggConstructionContentionBenchmark.java | 5 ++--- .../org/elasticsearch/join/mapper/Joiner.java | 15 ++++++--------- .../join/mapper/ParentJoinFieldMapper.java | 4 ++-- .../join/mapper/ParentJoinFieldMapperTests.java | 10 ++++++---- .../index/mapper/FieldTypeLookup.java | 12 ------------ .../index/mapper/MapperService.java | 8 ++++++-- .../index/mapper/MappingLookup.java | 12 ------------ .../index/query/SearchExecutionContext.java | 12 ------------ .../aggregations/support/AggregationContext.java | 16 ++++++---------- .../index/query/SearchExecutionContextTests.java | 13 ------------- .../index/mapper/MapperServiceTestCase.java | 4 ++-- 11 files changed, 30 insertions(+), 81 deletions(-) diff --git a/benchmarks/src/main/java/org/elasticsearch/benchmark/search/aggregations/AggConstructionContentionBenchmark.java b/benchmarks/src/main/java/org/elasticsearch/benchmark/search/aggregations/AggConstructionContentionBenchmark.java index 2eee0cb11d16e..9787cc4baaebf 100644 --- a/benchmarks/src/main/java/org/elasticsearch/benchmark/search/aggregations/AggConstructionContentionBenchmark.java +++ b/benchmarks/src/main/java/org/elasticsearch/benchmark/search/aggregations/AggConstructionContentionBenchmark.java @@ -67,12 +67,11 @@ import java.io.IOException; import java.util.ArrayList; -import java.util.Collection; import java.util.List; import java.util.Optional; +import java.util.Set; import java.util.concurrent.TimeUnit; import java.util.function.Function; -import java.util.function.Predicate; /** * Benchmarks the overhead of constructing {@link Aggregator}s in many @@ -214,7 +213,7 @@ public MappedFieldType getFieldType(String path) { } @Override - public Collection getIndexTimeFieldTypes(Predicate predicate) { + public Set getMatchingFieldNames(String pattern) { throw new UnsupportedOperationException(); } diff --git a/modules/parent-join/src/main/java/org/elasticsearch/join/mapper/Joiner.java b/modules/parent-join/src/main/java/org/elasticsearch/join/mapper/Joiner.java index ae4644dc05095..20019d29742d5 100644 --- a/modules/parent-join/src/main/java/org/elasticsearch/join/mapper/Joiner.java +++ b/modules/parent-join/src/main/java/org/elasticsearch/join/mapper/Joiner.java @@ -19,7 +19,6 @@ import org.elasticsearch.join.mapper.ParentJoinFieldMapper.JoinFieldType; import org.elasticsearch.search.aggregations.support.AggregationContext; -import java.util.Collection; import java.util.HashMap; import java.util.List; import java.util.Map; @@ -27,8 +26,7 @@ import java.util.Optional; import java.util.Set; import java.util.function.Consumer; -import java.util.function.Function; -import java.util.function.Predicate; +import java.util.stream.Stream; /** * Utility class to help build join queries and aggregations, based on a join_field @@ -39,23 +37,22 @@ public final class Joiner { * Get the Joiner for this context, or {@code null} if none is configured */ public static Joiner getJoiner(SearchExecutionContext context) { - return getJoiner(context::getIndexTimeFieldTypes); + return getJoiner(context.getMatchingFieldNames("*").stream().map(context::getFieldType)); } /** * Get the Joiner for this context, or {@code null} if none is configured */ public static Joiner getJoiner(AggregationContext context) { - return getJoiner(context::getIndexTimeFieldTypes); + return getJoiner(context.getMatchingFieldNames("*").stream().map(context::getFieldType)); } /** * Get the Joiner for this context, or {@code null} if none is configured */ - static Joiner getJoiner(Function, Collection> fieldTypeLookup) { - Optional joinFieldType = fieldTypeLookup.apply(ft -> ft instanceof JoinFieldType) - .stream().map(ft -> (JoinFieldType) ft).findFirst(); - return joinFieldType.map(JoinFieldType::getJoiner).orElse(null); + static Joiner getJoiner(Stream fieldTypes) { + Optional joinType = fieldTypes.filter(ft -> ft instanceof JoinFieldType).map(ft -> (JoinFieldType) ft).findFirst(); + return joinType.map(JoinFieldType::getJoiner).orElse(null); } private final Map> parentsToChildren = new HashMap<>(); private final Map childrenToParents = new HashMap<>(); diff --git a/modules/parent-join/src/main/java/org/elasticsearch/join/mapper/ParentJoinFieldMapper.java b/modules/parent-join/src/main/java/org/elasticsearch/join/mapper/ParentJoinFieldMapper.java index d290d134f94ba..aea02019dbd54 100644 --- a/modules/parent-join/src/main/java/org/elasticsearch/join/mapper/ParentJoinFieldMapper.java +++ b/modules/parent-join/src/main/java/org/elasticsearch/join/mapper/ParentJoinFieldMapper.java @@ -291,8 +291,8 @@ public FieldMapper.Builder getMergeBuilder() { @Override protected void doValidate(MappingLookup mappingLookup) { - List joinFields = mappingLookup.getIndexTimeFieldTypes(ft -> ft instanceof JoinFieldType) - .stream().map(MappedFieldType::name).collect(Collectors.toList()); + List joinFields = mappingLookup.getMatchingFieldNames("*").stream().map(mappingLookup::getFieldType) + .filter(ft -> ft instanceof JoinFieldType).map(MappedFieldType::name).collect(Collectors.toList()); if (joinFields.size() > 1) { throw new IllegalArgumentException("Only one [parent-join] field can be defined per index, got " + joinFields); } diff --git a/modules/parent-join/src/test/java/org/elasticsearch/join/mapper/ParentJoinFieldMapperTests.java b/modules/parent-join/src/test/java/org/elasticsearch/join/mapper/ParentJoinFieldMapperTests.java index 362ec9825d1a4..f6a4938082ff8 100644 --- a/modules/parent-join/src/test/java/org/elasticsearch/join/mapper/ParentJoinFieldMapperTests.java +++ b/modules/parent-join/src/test/java/org/elasticsearch/join/mapper/ParentJoinFieldMapperTests.java @@ -46,8 +46,8 @@ public void testSingleLevel() throws Exception { b.endObject(); })); DocumentMapper docMapper = mapperService.documentMapper(); - - Joiner joiner = Joiner.getJoiner(mapperService.mappingLookup()::getIndexTimeFieldTypes); + Joiner joiner = Joiner.getJoiner(mapperService.mappingLookup().getMatchingFieldNames("*").stream() + .map(mapperService.mappingLookup()::getFieldType)); assertNotNull(joiner); assertEquals("join_field", joiner.getJoinField()); @@ -233,7 +233,8 @@ public void testUpdateRelations() throws Exception { b.endObject(); })); - Joiner joiner = Joiner.getJoiner(mapperService.mappingLookup()::getIndexTimeFieldTypes); + Joiner joiner = Joiner.getJoiner(mapperService.mappingLookup().getMatchingFieldNames("*").stream() + .map(mapperService.mappingLookup()::getFieldType)); assertNotNull(joiner); assertEquals("join_field", joiner.getJoinField()); assertTrue(joiner.childTypeExists("child2")); @@ -259,7 +260,8 @@ public void testUpdateRelations() throws Exception { } b.endObject(); })); - joiner = Joiner.getJoiner(mapperService.mappingLookup()::getIndexTimeFieldTypes); + joiner = Joiner.getJoiner(mapperService.mappingLookup().getMatchingFieldNames("*").stream() + .map(mapperService.mappingLookup()::getFieldType)); assertNotNull(joiner); assertEquals("join_field", joiner.getJoinField()); assertTrue(joiner.childTypeExists("child2")); diff --git a/server/src/main/java/org/elasticsearch/index/mapper/FieldTypeLookup.java b/server/src/main/java/org/elasticsearch/index/mapper/FieldTypeLookup.java index c2ec303514860..9e1555a1dcdae 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/FieldTypeLookup.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/FieldTypeLookup.java @@ -17,8 +17,6 @@ import java.util.Map; import java.util.Objects; import java.util.Set; -import java.util.function.Predicate; -import java.util.stream.Collectors; /** * An immutable container for looking up {@link MappedFieldType}s by their name. @@ -225,14 +223,4 @@ Set sourcePaths(String field) { ? fieldToCopiedFields.get(resolvedField) : Set.of(resolvedField); } - - /** - * Returns the field types that match the provided predicate. - * Note that this is not going to include fields that are dynamically exposed through {@link DynamicFieldType}. - * @param predicate the predicate - * @return the matching field types - */ - Collection getFieldTypes(Predicate predicate) { - return fullNameToFieldType.values().stream().filter(predicate).collect(Collectors.toList()); - } } 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 1e22e477bdaa5..289378d29697a 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java @@ -48,6 +48,7 @@ import java.util.function.BooleanSupplier; import java.util.function.Function; import java.util.function.Supplier; +import java.util.stream.Collectors; public class MapperService extends AbstractIndexComponent implements Closeable { @@ -369,10 +370,13 @@ public MappingLookup mappingLookup() { * Returns all mapped field types. */ public Iterable getEagerGlobalOrdinalsFields() { - if (this.mapper == null) { + DocumentMapper mapper = this.mapper; + if (mapper == null) { return Collections.emptySet(); } - return this.mapper.mappers().getIndexTimeFieldTypes(MappedFieldType::eagerGlobalOrdinals); + MappingLookup mappingLookup = mapper.mappers(); + return mappingLookup.getMatchingFieldNames("*").stream().map(mappingLookup::getFieldType) + .filter(MappedFieldType::eagerGlobalOrdinals).collect(Collectors.toList()); } /** diff --git a/server/src/main/java/org/elasticsearch/index/mapper/MappingLookup.java b/server/src/main/java/org/elasticsearch/index/mapper/MappingLookup.java index 76ba01e62e851..093546204b6eb 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/MappingLookup.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/MappingLookup.java @@ -20,7 +20,6 @@ import java.util.Map; import java.util.Set; import java.util.function.Function; -import java.util.function.Predicate; import java.util.stream.Stream; /** @@ -299,17 +298,6 @@ public Set getMatchingFieldNames(String pattern) { return fieldTypeLookup.getMatchingFieldNames(pattern); } - /** - * Returns the index time field types matching the predicate. This is used to find specific field types among the ones defined - * under the properties section of the mappings. Runtime fields are not included. - * - * @param predicate the predicate - * @return the matching mapped field types - */ - public Collection getIndexTimeFieldTypes(Predicate predicate) { - return indexTimeLookup.getFieldTypes(predicate); - } - /** * Returns the mapped field type for the given field name. */ diff --git a/server/src/main/java/org/elasticsearch/index/query/SearchExecutionContext.java b/server/src/main/java/org/elasticsearch/index/query/SearchExecutionContext.java index f0a679cf7dbbc..06fe5a90b3842 100644 --- a/server/src/main/java/org/elasticsearch/index/query/SearchExecutionContext.java +++ b/server/src/main/java/org/elasticsearch/index/query/SearchExecutionContext.java @@ -58,7 +58,6 @@ import org.elasticsearch.transport.RemoteClusterAware; import java.io.IOException; -import java.util.Collection; import java.util.Collections; import java.util.HashMap; import java.util.HashSet; @@ -330,17 +329,6 @@ public Set getMatchingFieldNames(String pattern) { return matches; } - /** - * Returns the index time field types matching the predicate. This is used to find specific field types among the ones defined - * under the properties section of the mappings. Runtime fields are not included. - * - * @param predicate the predicate - * @return the matching mapped field types - */ - public Collection getIndexTimeFieldTypes(Predicate predicate) { - return mappingLookup.getIndexTimeFieldTypes(predicate); - } - /** * Returns the {@link MappedFieldType} for the provided field name. * If the field is not mapped, the behaviour depends on the index.query.parse.allow_unmapped_fields setting, which defaults to true. diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/support/AggregationContext.java b/server/src/main/java/org/elasticsearch/search/aggregations/support/AggregationContext.java index b6e89e9a50a59..6420575da7999 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/support/AggregationContext.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/support/AggregationContext.java @@ -41,12 +41,11 @@ import java.io.IOException; import java.util.ArrayList; -import java.util.Collection; import java.util.List; import java.util.Optional; +import java.util.Set; import java.util.function.Function; import java.util.function.LongSupplier; -import java.util.function.Predicate; import java.util.function.Supplier; /** @@ -113,13 +112,10 @@ public final FieldContext buildFieldContext(MappedFieldType ft) { public abstract MappedFieldType getFieldType(String path); /** - * Returns the index time field types matching the predicate. This is used to find specific field types among the ones defined - * under the properties section of the mappings. Runtime fields are not included. - * - * @param predicate the predicate - * @return the matching mapped field types + * Returns a set of field names that match a regex-like pattern + * All field names in the returned set are guaranteed to resolve to a field */ - public abstract Collection getIndexTimeFieldTypes(Predicate predicate); + public abstract Set getMatchingFieldNames(String pattern); /** * Returns true if the field identified by the provided name is mapped, false otherwise @@ -351,8 +347,8 @@ public MappedFieldType getFieldType(String path) { } @Override - public Collection getIndexTimeFieldTypes(Predicate predicate) { - return context.getIndexTimeFieldTypes(predicate); + public Set getMatchingFieldNames(String pattern) { + return context.getMatchingFieldNames(pattern); } @Override diff --git a/server/src/test/java/org/elasticsearch/index/query/SearchExecutionContextTests.java b/server/src/test/java/org/elasticsearch/index/query/SearchExecutionContextTests.java index 49ccc3ff64dfd..037d8eef9c4c3 100644 --- a/server/src/test/java/org/elasticsearch/index/query/SearchExecutionContextTests.java +++ b/server/src/test/java/org/elasticsearch/index/query/SearchExecutionContextTests.java @@ -75,7 +75,6 @@ import java.io.IOException; import java.util.ArrayList; import java.util.Arrays; -import java.util.Collection; import java.util.Collections; import java.util.HashMap; import java.util.List; @@ -89,7 +88,6 @@ import static java.util.Collections.emptyMap; import static java.util.Collections.singletonMap; import static org.hamcrest.Matchers.equalTo; -import static org.hamcrest.Matchers.hasSize; import static org.hamcrest.Matchers.instanceOf; import static org.hamcrest.Matchers.notNullValue; import static org.hamcrest.Matchers.nullValue; @@ -359,17 +357,6 @@ public void testSearchRequestRuntimeFields() { assertThat(context.getFieldType("pig"), instanceOf(MockFieldMapper.FakeFieldType.class)); assertThat(context.getMatchingFieldNames("pig"), equalTo(Set.of("pig"))); assertThat(context.getMatchingFieldNames("*"), equalTo(Set.of("cat", "dog", "pig", "runtime"))); - - // test that shadowed fields aren't returned by getMatchingFieldTypes - Collection matches = context.getIndexTimeFieldTypes(ft -> true); - assertThat(matches, hasSize(2)); - for (MappedFieldType match : matches) { - assertThat(match, instanceOf(MockFieldMapper.FakeFieldType.class)); - } - { - Collection matchingFieldTypes = context.getIndexTimeFieldTypes(ft -> ft.name().equals("dog")); - assertThat(matchingFieldTypes, hasSize(0)); - } } public void testSearchRequestRuntimeFieldsWrongFormat() { diff --git a/test/framework/src/main/java/org/elasticsearch/index/mapper/MapperServiceTestCase.java b/test/framework/src/main/java/org/elasticsearch/index/mapper/MapperServiceTestCase.java index 4dc11c638ba70..8f9ce586912a0 100644 --- a/test/framework/src/main/java/org/elasticsearch/index/mapper/MapperServiceTestCase.java +++ b/test/framework/src/main/java/org/elasticsearch/index/mapper/MapperServiceTestCase.java @@ -69,9 +69,9 @@ import java.util.List; import java.util.Map; import java.util.Optional; +import java.util.Set; import java.util.function.BooleanSupplier; import java.util.function.Function; -import java.util.function.Predicate; import static java.util.Collections.emptyList; import static java.util.stream.Collectors.toList; @@ -378,7 +378,7 @@ public MappedFieldType getFieldType(String path) { } @Override - public Collection getIndexTimeFieldTypes(Predicate predicate) { + public Set getMatchingFieldNames(String pattern) { throw new UnsupportedOperationException(); }