From 009ee70f56cd39271c77fe14fa88f450b925ba64 Mon Sep 17 00:00:00 2001 From: Kostas Krikellas Date: Wed, 27 Mar 2024 18:43:15 +0200 Subject: [PATCH 01/14] Alias PassThroughObjectMapper subfields in FieldTypeLookup --- .../TSDBPassthroughIndexingIT.java | 41 ++- .../DataStreamGetWriteIndexTests.java | 8 +- .../index/mapper/FieldTypeLookup.java | 24 ++ .../index/mapper/MappingLookup.java | 40 +-- .../index/mapper/RootObjectMapper.java | 128 +-------- .../FieldAliasMapperValidationTests.java | 2 +- .../mapper/FieldNamesFieldTypeTests.java | 2 +- .../index/mapper/FieldTypeLookupTests.java | 71 +++-- .../index/mapper/MappingLookupTests.java | 2 +- .../index/mapper/RootObjectMapperTests.java | 260 ------------------ .../query/SearchExecutionContextTests.java | 2 +- .../indices/IndicesRequestCacheTests.java | 23 +- .../AbstractSuggestionBuilderTestCase.java | 2 +- .../metadata/DataStreamTestHelper.java | 2 +- .../aggregations/AggregatorTestCase.java | 5 +- .../DocumentSubsetBitsetCacheTests.java | 3 +- ...ityIndexReaderWrapperIntegrationTests.java | 2 +- 17 files changed, 137 insertions(+), 480 deletions(-) diff --git a/modules/data-streams/src/internalClusterTest/java/org/elasticsearch/datastreams/TSDBPassthroughIndexingIT.java b/modules/data-streams/src/internalClusterTest/java/org/elasticsearch/datastreams/TSDBPassthroughIndexingIT.java index aa3fa2a730be3..a8b6834b11801 100644 --- a/modules/data-streams/src/internalClusterTest/java/org/elasticsearch/datastreams/TSDBPassthroughIndexingIT.java +++ b/modules/data-streams/src/internalClusterTest/java/org/elasticsearch/datastreams/TSDBPassthroughIndexingIT.java @@ -17,6 +17,8 @@ import org.elasticsearch.action.admin.indices.template.put.TransportPutComposableIndexTemplateAction; import org.elasticsearch.action.bulk.BulkRequest; import org.elasticsearch.action.delete.DeleteRequest; +import org.elasticsearch.action.fieldcaps.FieldCapabilitiesRequest; +import org.elasticsearch.action.fieldcaps.FieldCapabilitiesResponse; import org.elasticsearch.action.get.GetRequest; import org.elasticsearch.action.index.IndexRequest; import org.elasticsearch.action.search.SearchRequest; @@ -197,31 +199,20 @@ public void testIndexingGettingAndSearching() throws Exception { assertMap(attributes.get("pod.ip"), matchesMap().entry("type", "ip").entry("time_series_dimension", true)); assertMap(attributes.get("pod.uid"), matchesMap().entry("type", "keyword").entry("time_series_dimension", true)); assertMap(attributes.get("pod.name"), matchesMap().entry("type", "keyword").entry("time_series_dimension", true)); - // alias field mappers: - assertMap( - ObjectPath.eval("properties.metricset", mapping), - matchesMap().entry("type", "alias").entry("path", "attributes.metricset") - ); - assertMap( - ObjectPath.eval("properties.number.properties.long", mapping), - matchesMap().entry("type", "alias").entry("path", "attributes.number.long") - ); - assertMap( - ObjectPath.eval("properties.number.properties.double", mapping), - matchesMap().entry("type", "alias").entry("path", "attributes.number.double") - ); - assertMap( - ObjectPath.eval("properties.pod.properties", mapping), - matchesMap().extraOk().entry("name", matchesMap().entry("type", "alias").entry("path", "attributes.pod.name")) - ); - assertMap( - ObjectPath.eval("properties.pod.properties", mapping), - matchesMap().extraOk().entry("uid", matchesMap().entry("type", "alias").entry("path", "attributes.pod.uid")) - ); - assertMap( - ObjectPath.eval("properties.pod.properties", mapping), - matchesMap().extraOk().entry("ip", matchesMap().entry("type", "alias").entry("path", "attributes.pod.ip")) - ); + + FieldCapabilitiesResponse fieldCaps = client().fieldCaps(new FieldCapabilitiesRequest().fields("*").indices("k8s")).actionGet(); + assertTrue(fieldCaps.getField("attributes.metricset").get("keyword").isDimension()); + assertTrue(fieldCaps.getField("metricset").get("keyword").isDimension()); + assertTrue(fieldCaps.getField("attributes.number.long").get("long").isDimension()); + assertTrue(fieldCaps.getField("number.long").get("long").isDimension()); + assertTrue(fieldCaps.getField("attributes.number.double").get("float").isDimension()); + assertTrue(fieldCaps.getField("number.double").get("float").isDimension()); + assertTrue(fieldCaps.getField("attributes.pod.ip").get("ip").isDimension()); + assertTrue(fieldCaps.getField("pod.ip").get("ip").isDimension()); + assertTrue(fieldCaps.getField("attributes.pod.uid").get("keyword").isDimension()); + assertTrue(fieldCaps.getField("pod.uid").get("keyword").isDimension()); + assertTrue(fieldCaps.getField("attributes.pod.name").get("keyword").isDimension()); + assertTrue(fieldCaps.getField("pod.name").get("keyword").isDimension()); } public void testIndexingGettingAndSearchingShrunkIndex() throws Exception { diff --git a/modules/data-streams/src/test/java/org/elasticsearch/datastreams/DataStreamGetWriteIndexTests.java b/modules/data-streams/src/test/java/org/elasticsearch/datastreams/DataStreamGetWriteIndexTests.java index b61cbdc837010..52022b34901c8 100644 --- a/modules/data-streams/src/test/java/org/elasticsearch/datastreams/DataStreamGetWriteIndexTests.java +++ b/modules/data-streams/src/test/java/org/elasticsearch/datastreams/DataStreamGetWriteIndexTests.java @@ -239,7 +239,13 @@ public void setup() throws Exception { new MetadataFieldMapper[] { dtfm }, Collections.emptyMap() ); - MappingLookup mappingLookup = MappingLookup.fromMappers(mapping, List.of(dtfm, dateFieldMapper), List.of(), List.of()); + MappingLookup mappingLookup = MappingLookup.fromMappers( + mapping, + List.of(dtfm, dateFieldMapper), + List.of(), + List.of(), + List.of() + ); indicesService = DataStreamTestHelper.mockIndicesServices(mappingLookup); } 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 5e3dbe9590b99..c80002e3114ed 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/FieldTypeLookup.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/FieldTypeLookup.java @@ -14,6 +14,7 @@ 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; @@ -38,9 +39,14 @@ final class FieldTypeLookup { private final int maxParentPathDots; + FieldTypeLookup(Collection fieldMappers, Collection fieldAliasMappers) { + this(fieldMappers, fieldAliasMappers, List.of(), List.of()); + } + FieldTypeLookup( Collection fieldMappers, Collection fieldAliasMappers, + Collection passThroughMappers, Collection runtimeFields ) { @@ -86,6 +92,24 @@ final class FieldTypeLookup { } } + for (FieldMapper fieldMapper : fieldMappers) { + String fieldName = fieldMapper.name(); + for (PassThroughObjectMapper mapper : passThroughMappers) { + if (fieldName.startsWith(mapper.name())) { + String name = fieldName.substring(mapper.name().length() + 1); + // Check if there's an existing field or alias for the same field. + if (fullNameToFieldType.containsKey(name)) { + continue; + } + MappedFieldType fieldType = fieldMapper.fieldType(); + fullNameToFieldType.put(name, fieldType); + if (fieldType instanceof DynamicFieldType) { + dynamicFieldTypes.put(name, (DynamicFieldType) fieldType); + } + } + } + } + for (MappedFieldType fieldType : RuntimeField.collectFieldTypes(runtimeFields).values()) { // this will override concrete fields with runtime fields that have the same name fullNameToFieldType.put(fieldType.name(), fieldType); 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 673593cc6e240..d6f82e865fc18 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/MappingLookup.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/MappingLookup.java @@ -40,7 +40,7 @@ private CacheKey() {} * A lookup representing an empty mapping. It can be used to look up fields, although it won't hold any, but it does not * hold a valid {@link DocumentParser}, {@link IndexSettings} or {@link IndexAnalyzers}. */ - public static final MappingLookup EMPTY = fromMappers(Mapping.EMPTY, List.of(), List.of(), List.of()); + public static final MappingLookup EMPTY = fromMappers(Mapping.EMPTY, List.of(), List.of(), List.of(), List.of()); private final CacheKey cacheKey = new CacheKey(); @@ -67,35 +67,40 @@ public static MappingLookup fromMapping(Mapping mapping) { List newObjectMappers = new ArrayList<>(); List newFieldMappers = new ArrayList<>(); List newFieldAliasMappers = new ArrayList<>(); + List newPassThroughMappers = new ArrayList<>(); for (MetadataFieldMapper metadataMapper : mapping.getSortedMetadataMappers()) { if (metadataMapper != null) { newFieldMappers.add(metadataMapper); } } for (Mapper child : mapping.getRoot()) { - collect(child, newObjectMappers, newFieldMappers, newFieldAliasMappers); + collect(child, newObjectMappers, newFieldMappers, newFieldAliasMappers, newPassThroughMappers); } - return new MappingLookup(mapping, newFieldMappers, newObjectMappers, newFieldAliasMappers); + return new MappingLookup(mapping, newFieldMappers, newObjectMappers, newFieldAliasMappers, newPassThroughMappers); } private static void collect( Mapper mapper, Collection objectMappers, Collection fieldMappers, - Collection fieldAliasMappers + Collection fieldAliasMappers, + Collection passThroughMappers ) { - if (mapper instanceof ObjectMapper) { - objectMappers.add((ObjectMapper) mapper); - } else if (mapper instanceof FieldMapper) { - fieldMappers.add((FieldMapper) mapper); - } else if (mapper instanceof FieldAliasMapper) { - fieldAliasMappers.add((FieldAliasMapper) mapper); + if (mapper instanceof PassThroughObjectMapper passThroughObjectMapper) { + passThroughMappers.add(passThroughObjectMapper); + objectMappers.add(passThroughObjectMapper); + } else if (mapper instanceof ObjectMapper objectMapper) { + objectMappers.add(objectMapper); + } else if (mapper instanceof FieldMapper fieldMapper) { + fieldMappers.add(fieldMapper); + } else if (mapper instanceof FieldAliasMapper fieldAliasMapper) { + fieldAliasMappers.add(fieldAliasMapper); } else { throw new IllegalStateException("Unrecognized mapper type [" + mapper.getClass().getSimpleName() + "]."); } for (Mapper child : mapper) { - collect(child, objectMappers, fieldMappers, fieldAliasMappers); + collect(child, objectMappers, fieldMappers, fieldAliasMappers, passThroughMappers); } } @@ -111,22 +116,25 @@ private static void collect( * @param mappers the field mappers * @param objectMappers the object mappers * @param aliasMappers the field alias mappers + * @param passThroughMappers the pass-through mappers * @return the newly created lookup instance */ public static MappingLookup fromMappers( Mapping mapping, Collection mappers, Collection objectMappers, - Collection aliasMappers + Collection aliasMappers, + Collection passThroughMappers ) { - return new MappingLookup(mapping, mappers, objectMappers, aliasMappers); + return new MappingLookup(mapping, mappers, objectMappers, aliasMappers, passThroughMappers); } private MappingLookup( Mapping mapping, Collection mappers, Collection objectMappers, - Collection aliasMappers + Collection aliasMappers, + Collection passThroughMappers ) { this.totalFieldsCount = mapping.getRoot().getTotalFieldsCount(); this.mapping = mapping; @@ -173,12 +181,12 @@ private MappingLookup( } final Collection runtimeFields = mapping.getRoot().runtimeFields(); - this.fieldTypeLookup = new FieldTypeLookup(mappers, aliasMappers, runtimeFields); + this.fieldTypeLookup = new FieldTypeLookup(mappers, aliasMappers, passThroughMappers, runtimeFields); if (runtimeFields.isEmpty()) { // without runtime fields this is the same as the field type lookup this.indexTimeLookup = fieldTypeLookup; } else { - this.indexTimeLookup = new FieldTypeLookup(mappers, aliasMappers, Collections.emptyList()); + this.indexTimeLookup = new FieldTypeLookup(mappers, aliasMappers, passThroughMappers, Collections.emptyList()); } // make all fields into compact+fast immutable maps this.fieldMappers = Map.copyOf(fieldMappers); diff --git a/server/src/main/java/org/elasticsearch/index/mapper/RootObjectMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/RootObjectMapper.java index 90d9c879c57e1..6ccff0f52cdf2 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/RootObjectMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/RootObjectMapper.java @@ -111,14 +111,12 @@ public RootObjectMapper.Builder addRuntimeFields(Map runti @Override public RootObjectMapper build(MapperBuilderContext context) { - Map mappers = buildMappers(context.createChildContext(null, dynamic)); - mappers.putAll(getAliasMappers(mappers, context)); return new RootObjectMapper( name(), enabled, subobjects, dynamic, - mappers, + buildMappers(context.createChildContext(null, dynamic)), new HashMap<>(runtimeFields), dynamicDateTimeFormatters, dynamicTemplates, @@ -126,130 +124,6 @@ public RootObjectMapper build(MapperBuilderContext context) { numericDetection ); } - - Map getAliasMappers(Map mappers, MapperBuilderContext context) { - Map newMappers = new HashMap<>(); - Map objectIntermediates = new HashMap<>(1); - Map objectIntermediatesFullName = new HashMap<>(1); - getAliasMappers(mappers, mappers, newMappers, objectIntermediates, objectIntermediatesFullName, context, 0); - for (var entry : objectIntermediates.entrySet()) { - newMappers.put(entry.getKey(), entry.getValue().build(context)); - } - return newMappers; - } - - void getAliasMappers( - Map mappers, - Map topLevelMappers, - Map aliasMappers, - Map objectIntermediates, - Map objectIntermediatesFullName, - MapperBuilderContext context, - int level - ) { - if (level >= MAX_NESTING_LEVEL_FOR_PASS_THROUGH_OBJECTS) { - logger.warn("Exceeded maximum nesting level for searching for pass-through object fields within object fields."); - return; - } - for (Mapper mapper : mappers.values()) { - // Create aliases for all fields in child passthrough mappers and place them under the root object. - if (mapper instanceof PassThroughObjectMapper passthroughMapper) { - for (Mapper internalMapper : passthroughMapper.mappers.values()) { - if (internalMapper instanceof FieldMapper fieldMapper) { - // If there's a conflicting alias with the same name at the root level, we don't want to throw an error - // to avoid indexing disruption. - // TODO: record an error without affecting document indexing, so that it can be investigated later. - Mapper conflict = mappers.get(fieldMapper.simpleName()); - if (conflict != null) { - if (conflict.typeName().equals(FieldAliasMapper.CONTENT_TYPE) == false - || ((FieldAliasMapper) conflict).path().equals(fieldMapper.mappedFieldType.name()) == false) { - logger.warn( - "Root alias for field " - + fieldMapper.name() - + " conflicts with existing field or alias, skipping alias creation." - ); - } - } else { - // Check if the field name contains dots, as aliases require nesting within objects in this case. - String[] fieldNameParts = fieldMapper.simpleName().split("\\."); - if (fieldNameParts.length == 0) { - throw new IllegalArgumentException("field name cannot contain only dots"); - } - if (fieldNameParts.length == 1) { - // No nesting required, add the alias directly to the root. - FieldAliasMapper aliasMapper = new FieldAliasMapper.Builder(fieldMapper.simpleName()).path( - fieldMapper.mappedFieldType.name() - ).build(context); - aliasMappers.put(aliasMapper.simpleName(), aliasMapper); - } else { - conflict = topLevelMappers.get(fieldNameParts[0]); - if (conflict != null) { - if (isConflictingObject(conflict, fieldNameParts)) { - throw new IllegalArgumentException( - "Conflicting objects created during alias generation for pass-through field: [" - + conflict.name() - + "]" - ); - } - } - - // Nest the alias within object(s). - String realFieldName = fieldNameParts[fieldNameParts.length - 1]; - Mapper.Builder fieldBuilder = new FieldAliasMapper.Builder(realFieldName).path( - fieldMapper.mappedFieldType.name() - ); - ObjectMapper.Builder intermediate = null; - for (int i = fieldNameParts.length - 2; i >= 0; --i) { - String intermediateObjectName = fieldNameParts[i]; - intermediate = objectIntermediatesFullName.computeIfAbsent( - concatStrings(fieldNameParts, i), - s -> new ObjectMapper.Builder(intermediateObjectName, ObjectMapper.Defaults.SUBOBJECTS) - ); - intermediate.add(fieldBuilder); - fieldBuilder = intermediate; - } - objectIntermediates.putIfAbsent(fieldNameParts[0], intermediate); - } - } - } - } - } else if (mapper instanceof ObjectMapper objectMapper) { - // Call recursively to check child fields. The level guards against long recursive call sequences. - getAliasMappers( - objectMapper.mappers, - topLevelMappers, - aliasMappers, - objectIntermediates, - objectIntermediatesFullName, - context, - level + 1 - ); - } - } - } - } - - private static String concatStrings(String[] parts, int last) { - StringBuilder builder = new StringBuilder(); - for (int i = 0; i <= last; i++) { - builder.append('.'); - builder.append(parts[i]); - } - return builder.toString(); - } - - private static boolean isConflictingObject(Mapper mapper, String[] parts) { - for (int i = 0; i < parts.length - 1; i++) { - if (mapper == null) { - return true; - } - if (mapper instanceof ObjectMapper objectMapper) { - mapper = objectMapper.getMapper(parts[i + 1]); - } else { - return true; - } - } - return mapper == null; } private final Explicit dynamicDateTimeFormatters; diff --git a/server/src/test/java/org/elasticsearch/index/mapper/FieldAliasMapperValidationTests.java b/server/src/test/java/org/elasticsearch/index/mapper/FieldAliasMapperValidationTests.java index 6df9fd1f35f52..403a9471225ff 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/FieldAliasMapperValidationTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/FieldAliasMapperValidationTests.java @@ -185,6 +185,6 @@ private static MappingLookup createMappingLookup( new MetadataFieldMapper[0], Collections.emptyMap() ); - return MappingLookup.fromMappers(mapping, fieldMappers, objectMappers, fieldAliasMappers); + return MappingLookup.fromMappers(mapping, fieldMappers, objectMappers, fieldAliasMappers, emptyList()); } } diff --git a/server/src/test/java/org/elasticsearch/index/mapper/FieldNamesFieldTypeTests.java b/server/src/test/java/org/elasticsearch/index/mapper/FieldNamesFieldTypeTests.java index 9417bd924e221..b3d0c4041bf6d 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/FieldNamesFieldTypeTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/FieldNamesFieldTypeTests.java @@ -35,7 +35,7 @@ public void testTermQuery() { settings ); List mappers = Stream.of(fieldNamesFieldType, fieldType).map(MockFieldMapper::new).toList(); - MappingLookup mappingLookup = MappingLookup.fromMappers(Mapping.EMPTY, mappers, emptyList(), emptyList()); + MappingLookup mappingLookup = MappingLookup.fromMappers(Mapping.EMPTY, mappers, emptyList(), emptyList(), emptyList()); SearchExecutionContext searchExecutionContext = SearchExecutionContextHelper.createSimple(indexSettings, null, null); Query termQuery = fieldNamesFieldType.termQuery("field_name", searchExecutionContext); assertEquals(new TermQuery(new Term(FieldNamesFieldMapper.CONTENT_TYPE, "field_name")), termQuery); 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 3f50b9fdf6621..aa9708dba0add 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/FieldTypeLookupTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/FieldTypeLookupTests.java @@ -30,7 +30,7 @@ public class FieldTypeLookupTests extends ESTestCase { public void testEmpty() { - FieldTypeLookup lookup = new FieldTypeLookup(Collections.emptyList(), Collections.emptyList(), Collections.emptyList()); + FieldTypeLookup lookup = new FieldTypeLookup(emptyList(), emptyList()); assertNull(lookup.get("foo")); Collection names = lookup.getMatchingFieldNames("foo"); assertNotNull(names); @@ -39,7 +39,7 @@ public void testEmpty() { public void testAddNewField() { MockFieldMapper f = new MockFieldMapper("foo"); - FieldTypeLookup lookup = new FieldTypeLookup(Collections.singletonList(f), emptyList(), Collections.emptyList()); + FieldTypeLookup lookup = new FieldTypeLookup(Collections.singletonList(f), emptyList()); assertNull(lookup.get("bar")); assertEquals(f.fieldType(), lookup.get("foo")); } @@ -48,11 +48,7 @@ public void testAddFieldAlias() { MockFieldMapper field = new MockFieldMapper("foo"); FieldAliasMapper alias = new FieldAliasMapper("alias", "alias", "foo"); - FieldTypeLookup lookup = new FieldTypeLookup( - Collections.singletonList(field), - Collections.singletonList(alias), - Collections.emptyList() - ); + FieldTypeLookup lookup = new FieldTypeLookup(Collections.singletonList(field), Collections.singletonList(alias)); MappedFieldType aliasType = lookup.get("alias"); assertEquals(field.fieldType(), aliasType); @@ -79,6 +75,7 @@ public void testGetMatchingFieldNames() { FieldTypeLookup lookup = new FieldTypeLookup( List.of(field1, field2, field3, flattened), List.of(alias1, alias2), + List.of(), List.of(runtimeField, multi) ); @@ -124,7 +121,7 @@ public void testSourcePathWithMultiFields() { // Adding a subfield that is not multi-field MockFieldMapper subfield = new MockFieldMapper.Builder("field.subfield4").build(MapperBuilderContext.root(false, false)); - FieldTypeLookup lookup = new FieldTypeLookup(List.of(field, subfield), emptyList(), emptyList()); + FieldTypeLookup lookup = new FieldTypeLookup(List.of(field, subfield), emptyList()); assertEquals(Set.of("field"), lookup.sourcePaths("field")); assertEquals(Set.of("field"), lookup.sourcePaths("field.subfield1")); @@ -148,11 +145,7 @@ public void testSourcePathsWithCopyTo() { .copyTo("field.nested") .build(MapperBuilderContext.root(false, false)); - FieldTypeLookup lookup = new FieldTypeLookup( - Arrays.asList(field, nestedField, otherField, otherNestedField), - emptyList(), - emptyList() - ); + FieldTypeLookup lookup = new FieldTypeLookup(Arrays.asList(field, nestedField, otherField, otherNestedField), emptyList()); assertEquals(Set.of("other_field", "other_field.nested", "field"), lookup.sourcePaths("field")); assertEquals(Set.of("other_field", "other_field.nested", "field"), lookup.sourcePaths("field.subfield1")); @@ -172,7 +165,12 @@ public void testRuntimeFieldsLookup() { ) ); - FieldTypeLookup fieldTypeLookup = new FieldTypeLookup(List.of(concrete), emptyList(), List.of(runtime, runtimeLong, multi)); + FieldTypeLookup fieldTypeLookup = new FieldTypeLookup( + List.of(concrete), + emptyList(), + emptyList(), + List.of(runtime, runtimeLong, multi) + ); assertThat(fieldTypeLookup.get("concrete"), instanceOf(MockFieldMapper.FakeFieldType.class)); assertThat(fieldTypeLookup.get("string"), instanceOf(TestRuntimeField.TestRuntimeFieldType.class)); assertThat(fieldTypeLookup.get("string").typeName(), equalTo("type")); @@ -202,6 +200,7 @@ public void testRuntimeFieldsOverrideConcreteFields() { FieldTypeLookup fieldTypeLookup = new FieldTypeLookup( List.of(field, concrete, subfield, flattened), emptyList(), + emptyList(), List.of(fieldOverride, runtime, subfieldOverride, flattenedRuntime) ); assertThat(fieldTypeLookup.get("field"), instanceOf(TestRuntimeField.TestRuntimeFieldType.class)); @@ -223,7 +222,12 @@ public void testRuntimeFieldsSourcePaths() { TestRuntimeField field2 = new TestRuntimeField("field2", "type"); TestRuntimeField subfield = new TestRuntimeField("object.subfield", "type"); - FieldTypeLookup fieldTypeLookup = new FieldTypeLookup(List.of(field1, concrete), emptyList(), List.of(field2, subfield)); + FieldTypeLookup fieldTypeLookup = new FieldTypeLookup( + List.of(field1, concrete), + emptyList(), + emptyList(), + List.of(field2, subfield) + ); { Set sourcePaths = fieldTypeLookup.sourcePaths("field1"); assertEquals(1, sourcePaths.size()); @@ -245,7 +249,7 @@ public void testFlattenedLookup() { String fieldName = "object1.object2.field"; FlattenedFieldMapper mapper = createFlattenedMapper(fieldName); - FieldTypeLookup lookup = new FieldTypeLookup(singletonList(mapper), emptyList(), emptyList()); + FieldTypeLookup lookup = new FieldTypeLookup(singletonList(mapper), emptyList()); assertEquals(mapper.fieldType(), lookup.get(fieldName)); String objectKey = "key1.key2"; @@ -271,7 +275,7 @@ public void testFlattenedLookupWithAlias() { String aliasName = "alias"; FieldAliasMapper alias = new FieldAliasMapper(aliasName, aliasName, fieldName); - FieldTypeLookup lookup = new FieldTypeLookup(singletonList(mapper), singletonList(alias), emptyList()); + FieldTypeLookup lookup = new FieldTypeLookup(singletonList(mapper), singletonList(alias), emptyList(), emptyList()); assertEquals(mapper.fieldType(), lookup.get(aliasName)); String objectKey = "key1.key2"; @@ -293,35 +297,31 @@ public void testFlattenedLookupWithMultipleFields() { FlattenedFieldMapper mapper2 = createFlattenedMapper(field2); FlattenedFieldMapper mapper3 = createFlattenedMapper(field3); - FieldTypeLookup lookup = new FieldTypeLookup(Arrays.asList(mapper1, mapper2), emptyList(), emptyList()); + FieldTypeLookup lookup = new FieldTypeLookup(Arrays.asList(mapper1, mapper2), emptyList()); assertNotNull(lookup.get(field1 + ".some.key")); assertNotNull(lookup.get(field2 + ".some.key")); - lookup = new FieldTypeLookup(Arrays.asList(mapper1, mapper2, mapper3), emptyList(), emptyList()); + lookup = new FieldTypeLookup(Arrays.asList(mapper1, mapper2, mapper3), emptyList()); assertNotNull(lookup.get(field1 + ".some.key")); assertNotNull(lookup.get(field2 + ".some.key")); assertNotNull(lookup.get(field3 + ".some.key")); } public void testUnmappedLookupWithDots() { - FieldTypeLookup lookup = new FieldTypeLookup(emptyList(), emptyList(), emptyList()); + FieldTypeLookup lookup = new FieldTypeLookup(emptyList(), emptyList()); assertNull(lookup.get("object.child")); } public void testMaxDynamicKeyDepth() { { - FieldTypeLookup lookup = new FieldTypeLookup(Collections.emptyList(), Collections.emptyList(), Collections.emptyList()); + FieldTypeLookup lookup = new FieldTypeLookup(emptyList(), emptyList()); assertEquals(0, lookup.getMaxParentPathDots()); } // Add a flattened object field. { String name = "object1.object2.field"; - FieldTypeLookup lookup = new FieldTypeLookup( - Collections.singletonList(createFlattenedMapper(name)), - Collections.emptyList(), - Collections.emptyList() - ); + FieldTypeLookup lookup = new FieldTypeLookup(Collections.singletonList(createFlattenedMapper(name)), emptyList()); assertEquals(2, lookup.getMaxParentPathDots()); } @@ -330,8 +330,7 @@ public void testMaxDynamicKeyDepth() { String name = "object1.object2.field"; FieldTypeLookup lookup = new FieldTypeLookup( Collections.singletonList(createFlattenedMapper(name)), - Collections.singletonList(new FieldAliasMapper("alias", "alias", "object1.object2.field")), - Collections.emptyList() + Collections.singletonList(new FieldAliasMapper("alias", "alias", "object1.object2.field")) ); assertEquals(2, lookup.getMaxParentPathDots()); } @@ -341,8 +340,7 @@ public void testMaxDynamicKeyDepth() { String name = "object1.object2.field"; FieldTypeLookup lookup = new FieldTypeLookup( Collections.singletonList(createFlattenedMapper(name)), - Collections.singletonList(new FieldAliasMapper("alias", "object1.object2.object3.alias", "object1.object2.field")), - Collections.emptyList() + Collections.singletonList(new FieldAliasMapper("alias", "object1.object2.object3.alias", "object1.object2.field")) ); assertEquals(2, lookup.getMaxParentPathDots()); } @@ -353,8 +351,9 @@ public void testRuntimeFieldNameClashes() { IllegalArgumentException iae = expectThrows( IllegalArgumentException.class, () -> new FieldTypeLookup( - Collections.emptySet(), - Collections.emptySet(), + emptyList(), + emptyList(), + emptyList(), List.of(new TestRuntimeField("field", "type"), new TestRuntimeField("field", "long")) ) ); @@ -368,7 +367,7 @@ public void testRuntimeFieldNameClashes() { TestRuntimeField runtime = new TestRuntimeField("multi.first", "runtime"); IllegalArgumentException iae = expectThrows( IllegalArgumentException.class, - () -> new FieldTypeLookup(Collections.emptySet(), Collections.emptySet(), List.of(multi, runtime)) + () -> new FieldTypeLookup(emptyList(), emptyList(), emptyList(), List.of(multi, runtime)) ); assertEquals(iae.getMessage(), "Found two runtime fields with same name [multi.first]"); } @@ -383,7 +382,7 @@ public void testRuntimeFieldNameClashes() { IllegalArgumentException iae = expectThrows( IllegalArgumentException.class, - () -> new FieldTypeLookup(Collections.emptySet(), Collections.emptySet(), List.of(multi)) + () -> new FieldTypeLookup(emptyList(), emptyList(), emptyList(), List.of(multi)) ); assertEquals(iae.getMessage(), "Found two runtime fields with same name [multi]"); } @@ -401,7 +400,7 @@ public void testRuntimeFieldNameOutsideContext() { ); IllegalStateException ise = expectThrows( IllegalStateException.class, - () -> new FieldTypeLookup(Collections.emptySet(), Collections.emptySet(), Collections.singletonList(multi)) + () -> new FieldTypeLookup(emptyList(), emptyList(), emptyList(), Collections.singletonList(multi)) ); assertEquals("Found sub-fields with name not belonging to the parent field they are part of [first, second]", ise.getMessage()); } @@ -415,7 +414,7 @@ public void testRuntimeFieldNameOutsideContext() { ); IllegalStateException ise = expectThrows( IllegalStateException.class, - () -> new FieldTypeLookup(Collections.emptySet(), Collections.emptySet(), Collections.singletonList(multi)) + () -> new FieldTypeLookup(emptyList(), emptyList(), emptyList(), Collections.singletonList(multi)) ); assertEquals("Found sub-fields with name not belonging to the parent field they are part of [multi.]", ise.getMessage()); } diff --git a/server/src/test/java/org/elasticsearch/index/mapper/MappingLookupTests.java b/server/src/test/java/org/elasticsearch/index/mapper/MappingLookupTests.java index 0308dac5fa216..843ff497d639c 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/MappingLookupTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/MappingLookupTests.java @@ -47,7 +47,7 @@ private static MappingLookup createMappingLookup( new MetadataFieldMapper[0], Collections.emptyMap() ); - return MappingLookup.fromMappers(mapping, fieldMappers, objectMappers, emptyList()); + return MappingLookup.fromMappers(mapping, fieldMappers, objectMappers, emptyList(), emptyList()); } public void testOnlyRuntimeField() { diff --git a/server/src/test/java/org/elasticsearch/index/mapper/RootObjectMapperTests.java b/server/src/test/java/org/elasticsearch/index/mapper/RootObjectMapperTests.java index 7a7f1668b4636..80f3d00da34a2 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/RootObjectMapperTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/RootObjectMapperTests.java @@ -8,11 +8,9 @@ package org.elasticsearch.index.mapper; -import org.elasticsearch.common.Explicit; import org.elasticsearch.common.Strings; import org.elasticsearch.common.xcontent.XContentHelper; import org.elasticsearch.core.CheckedConsumer; -import org.elasticsearch.index.IndexVersion; import org.elasticsearch.index.mapper.MapperService.MergeReason; import org.elasticsearch.xcontent.XContentBuilder; import org.elasticsearch.xcontent.XContentFactory; @@ -20,8 +18,6 @@ import java.io.IOException; import java.util.Arrays; import java.util.Collections; -import java.util.HashMap; -import java.util.Map; import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.equalTo; @@ -346,262 +342,6 @@ public void testRuntimeSectionRemainingField() throws IOException { assertEquals("Failed to parse mapping: unknown parameter [unsupported] on runtime field [field] of type [keyword]", e.getMessage()); } - public void testPassThroughObjectWithAliases() throws IOException { - MapperService mapperService = createMapperService(mapping(b -> { - b.startObject("labels").field("type", "passthrough"); - { - b.startObject("properties"); - b.startObject("dim").field("type", "keyword").endObject(); - b.endObject(); - } - b.endObject(); - })); - assertThat(mapperService.mappingLookup().getMapper("dim"), instanceOf(FieldAliasMapper.class)); - assertThat(mapperService.mappingLookup().getMapper("labels.dim"), instanceOf(KeywordFieldMapper.class)); - } - - public void testPassThroughObjectNested() throws IOException { - MapperService mapperService = createMapperService(mapping(b -> { - b.startObject("resource").field("type", "object"); - { - b.startObject("properties"); - { - b.startObject("attributes").field("type", "passthrough"); - { - b.startObject("properties"); - b.startObject("dim").field("type", "keyword").endObject(); - b.endObject(); - } - b.endObject(); - } - b.endObject(); - } - b.endObject(); - b.startObject("attributes").field("type", "passthrough"); - { - b.startObject("properties"); - b.startObject("another.dim").field("type", "keyword").endObject(); - b.endObject(); - } - b.endObject(); - })); - assertThat(mapperService.mappingLookup().getMapper("dim"), instanceOf(FieldAliasMapper.class)); - assertThat(mapperService.mappingLookup().getMapper("resource.attributes.dim"), instanceOf(KeywordFieldMapper.class)); - assertThat(mapperService.mappingLookup().objectMappers().get("another").getMapper("dim"), instanceOf(FieldAliasMapper.class)); - assertThat(mapperService.mappingLookup().getMapper("attributes.another.dim"), instanceOf(KeywordFieldMapper.class)); - } - - public void testPassThroughObjectNestedWithDuplicateNames() throws IOException { - MapperService mapperService = createMapperService(mapping(b -> { - b.startObject("resource").field("type", "object"); - { - b.startObject("properties"); - { - b.startObject("attributes").field("type", "passthrough"); - { - b.startObject("properties"); - b.startObject("dim").field("type", "keyword").endObject(); - b.startObject("more.attributes.another.dimA").field("type", "keyword").endObject(); - b.startObject("more.attributes.another.dimB").field("type", "keyword").endObject(); - b.endObject(); - } - b.endObject(); - } - b.endObject(); - } - b.endObject(); - b.startObject("attributes").field("type", "passthrough"); - { - b.startObject("properties"); - b.startObject("another.dim").field("type", "keyword").endObject(); - b.startObject("more.attributes.another.dimC").field("type", "keyword").endObject(); - b.startObject("more.attributes.another.dimD").field("type", "keyword").endObject(); - b.endObject(); - } - b.endObject(); - })); - - assertThat(mapperService.mappingLookup().getMapper("dim"), instanceOf(FieldAliasMapper.class)); - assertThat(mapperService.mappingLookup().getMapper("resource.attributes.dim"), instanceOf(KeywordFieldMapper.class)); - assertThat( - mapperService.mappingLookup().objectMappers().get("more.attributes.another").getMapper("dimA"), - instanceOf(FieldAliasMapper.class) - ); - assertThat( - mapperService.mappingLookup().getMapper("resource.attributes.more.attributes.another.dimA"), - instanceOf(KeywordFieldMapper.class) - ); - assertThat( - mapperService.mappingLookup().objectMappers().get("more.attributes.another").getMapper("dimB"), - instanceOf(FieldAliasMapper.class) - ); - assertThat( - mapperService.mappingLookup().getMapper("resource.attributes.more.attributes.another.dimB"), - instanceOf(KeywordFieldMapper.class) - ); - - assertThat(mapperService.mappingLookup().objectMappers().get("another").getMapper("dim"), instanceOf(FieldAliasMapper.class)); - assertThat(mapperService.mappingLookup().getMapper("attributes.another.dim"), instanceOf(KeywordFieldMapper.class)); - assertThat( - mapperService.mappingLookup().objectMappers().get("more.attributes.another").getMapper("dimC"), - instanceOf(FieldAliasMapper.class) - ); - assertThat( - mapperService.mappingLookup().getMapper("attributes.more.attributes.another.dimC"), - instanceOf(KeywordFieldMapper.class) - ); - assertThat( - mapperService.mappingLookup().objectMappers().get("more.attributes.another").getMapper("dimD"), - instanceOf(FieldAliasMapper.class) - ); - assertThat( - mapperService.mappingLookup().getMapper("attributes.more.attributes.another.dimD"), - instanceOf(KeywordFieldMapper.class) - ); - } - - public void testPassThroughObjectNestedWithConflictingNames() throws IOException { - MapperParsingException e = expectThrows(MapperParsingException.class, () -> createMapperService(mapping(b -> { - b.startObject("resource").field("type", "object"); - { - b.startObject("properties"); - { - b.startObject("attributes").field("type", "passthrough"); - { - b.startObject("properties"); - b.startObject("dim").field("type", "keyword").endObject(); - b.startObject("resource.attributes.another.dim").field("type", "keyword").endObject(); - b.endObject(); - } - b.endObject(); - } - b.endObject(); - } - b.endObject(); - }))); - assertEquals( - "Failed to parse mapping: Conflicting objects created during alias generation for pass-through field: [resource]", - e.getMessage() - ); - } - - public void testAliasMappersCreatesAlias() throws Exception { - var context = MapperBuilderContext.root(false, false); - Map aliases = new RootObjectMapper.Builder("root", Explicit.EXPLICIT_FALSE).getAliasMappers( - Map.of( - "labels", - new PassThroughObjectMapper( - "labels", - "labels", - Explicit.EXPLICIT_TRUE, - ObjectMapper.Dynamic.FALSE, - Map.of("host", new KeywordFieldMapper.Builder("host", IndexVersion.current()).build(context)), - Explicit.EXPLICIT_FALSE - ) - ), - context - ); - assertEquals(1, aliases.size()); - assertThat(aliases.get("host"), instanceOf(FieldAliasMapper.class)); - } - - public void testAliasMappersCreatesAliasNested() throws Exception { - var context = MapperBuilderContext.root(false, false); - Map aliases = new RootObjectMapper.Builder("root", Explicit.EXPLICIT_FALSE).getAliasMappers( - Map.of( - "outer", - new ObjectMapper( - "outer", - "outer", - Explicit.EXPLICIT_TRUE, - Explicit.EXPLICIT_TRUE, - ObjectMapper.Dynamic.FALSE, - Map.of( - "inner", - new PassThroughObjectMapper( - "inner", - "outer.inner", - Explicit.EXPLICIT_TRUE, - ObjectMapper.Dynamic.FALSE, - Map.of("host", new KeywordFieldMapper.Builder("host", IndexVersion.current()).build(context)), - Explicit.EXPLICIT_FALSE - ) - ) - ) - ), - context - ); - assertEquals(1, aliases.size()); - assertThat(aliases.get("host"), instanceOf(FieldAliasMapper.class)); - } - - public void testAliasMappersExitsInDeepNesting() throws Exception { - var context = MapperBuilderContext.root(false, false); - Map aliases = new HashMap<>(); - var objectIntermediates = new HashMap(1); - var objectIntermediatesFullPath = new HashMap(1); - new RootObjectMapper.Builder("root", Explicit.EXPLICIT_FALSE).getAliasMappers( - Map.of( - "labels", - new PassThroughObjectMapper( - "labels", - "labels", - Explicit.EXPLICIT_TRUE, - ObjectMapper.Dynamic.FALSE, - Map.of("host", new KeywordFieldMapper.Builder("host", IndexVersion.current()).build(context)), - Explicit.EXPLICIT_FALSE - ) - ), - Map.of(), - aliases, - objectIntermediates, - objectIntermediatesFullPath, - context, - 1_000_000 - ); - assertTrue(aliases.isEmpty()); - } - - public void testAliasMappersCreatesNoAliasForRegularObject() throws Exception { - var context = MapperBuilderContext.root(false, false); - Map aliases = new RootObjectMapper.Builder("root", Explicit.EXPLICIT_FALSE).getAliasMappers( - Map.of( - "labels", - new ObjectMapper( - "labels", - "labels", - Explicit.EXPLICIT_TRUE, - Explicit.EXPLICIT_FALSE, - ObjectMapper.Dynamic.FALSE, - Map.of("host", new KeywordFieldMapper.Builder("host", IndexVersion.current()).build(context)) - ) - ), - context - ); - assertTrue(aliases.isEmpty()); - } - - public void testAliasMappersConflictingField() throws Exception { - var context = MapperBuilderContext.root(false, false); - Map aliases = new RootObjectMapper.Builder("root", Explicit.EXPLICIT_FALSE).getAliasMappers( - Map.of( - "labels", - new PassThroughObjectMapper( - "labels", - "labels", - Explicit.EXPLICIT_TRUE, - ObjectMapper.Dynamic.FALSE, - Map.of("host", new KeywordFieldMapper.Builder("host", IndexVersion.current()).build(context)), - Explicit.EXPLICIT_FALSE - ), - "host", - new KeywordFieldMapper.Builder("host", IndexVersion.current()).build(context) - ), - context - ); - assertTrue(aliases.isEmpty()); - } - public void testEmptyType() throws Exception { String mapping = Strings.toString( XContentFactory.jsonBuilder() 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 6d671a258c26a..1c3b9c3ba3d66 100644 --- a/server/src/test/java/org/elasticsearch/index/query/SearchExecutionContextTests.java +++ b/server/src/test/java/org/elasticsearch/index/query/SearchExecutionContextTests.java @@ -294,7 +294,7 @@ private static MappingLookup createMappingLookup(List concreteF new MetadataFieldMapper[0], Collections.emptyMap() ); - return MappingLookup.fromMappers(mapping, mappers, Collections.emptyList(), Collections.emptyList()); + return MappingLookup.fromMappers(mapping, mappers, Collections.emptyList(), Collections.emptyList(), Collections.emptyList()); } public void testSearchRequestRuntimeFields() { diff --git a/server/src/test/java/org/elasticsearch/indices/IndicesRequestCacheTests.java b/server/src/test/java/org/elasticsearch/indices/IndicesRequestCacheTests.java index 590dc72e2a72b..2225a2da21f5f 100644 --- a/server/src/test/java/org/elasticsearch/indices/IndicesRequestCacheTests.java +++ b/server/src/test/java/org/elasticsearch/indices/IndicesRequestCacheTests.java @@ -203,7 +203,8 @@ public void testCacheDifferentReaders() throws Exception { public void testCacheDifferentMapping() throws Exception { IndicesRequestCache cache = new IndicesRequestCache(Settings.EMPTY); MappingLookup.CacheKey mappingKey1 = MappingLookup.EMPTY.cacheKey(); - MappingLookup.CacheKey mappingKey2 = MappingLookup.fromMappers(Mapping.EMPTY, emptyList(), emptyList(), emptyList()).cacheKey(); + MappingLookup.CacheKey mappingKey2 = MappingLookup.fromMappers(Mapping.EMPTY, emptyList(), emptyList(), emptyList(), emptyList()) + .cacheKey(); AtomicBoolean indexShard = new AtomicBoolean(true); ShardRequestCache requestCacheStats = new ShardRequestCache(); Directory dir = newDirectory(); @@ -363,14 +364,25 @@ public void testClearAllEntityIdentity() throws Exception { writer.updateDocument(new Term("id", "0"), newDoc(0, "bar")); DirectoryReader secondReader = ElasticsearchDirectoryReader.wrap(DirectoryReader.open(writer), new ShardId("foo", "bar", 1)); - MappingLookup.CacheKey secondMappingKey = MappingLookup.fromMappers(Mapping.EMPTY, emptyList(), emptyList(), emptyList()) - .cacheKey(); + MappingLookup.CacheKey secondMappingKey = MappingLookup.fromMappers( + Mapping.EMPTY, + emptyList(), + emptyList(), + emptyList(), + emptyList() + ).cacheKey(); TestEntity secondEntity = new TestEntity(requestCacheStats, indexShard); Loader secondLoader = new Loader(secondReader, 0); writer.updateDocument(new Term("id", "0"), newDoc(0, "baz")); DirectoryReader thirdReader = ElasticsearchDirectoryReader.wrap(DirectoryReader.open(writer), new ShardId("foo", "bar", 1)); - MappingLookup.CacheKey thirdMappingKey = MappingLookup.fromMappers(Mapping.EMPTY, emptyList(), emptyList(), emptyList()).cacheKey(); + MappingLookup.CacheKey thirdMappingKey = MappingLookup.fromMappers( + Mapping.EMPTY, + emptyList(), + emptyList(), + emptyList(), + emptyList() + ).cacheKey(); AtomicBoolean differentIdentity = new AtomicBoolean(true); TestEntity thirdEntity = new TestEntity(requestCacheStats, differentIdentity); Loader thirdLoader = new Loader(thirdReader, 0); @@ -506,7 +518,8 @@ public void testKeyEqualsAndHashCode() throws IOException { AtomicBoolean trueBoolean = new AtomicBoolean(true); AtomicBoolean falseBoolean = new AtomicBoolean(false); MappingLookup.CacheKey mKey1 = MappingLookup.EMPTY.cacheKey(); - MappingLookup.CacheKey mKey2 = MappingLookup.fromMappers(Mapping.EMPTY, emptyList(), emptyList(), emptyList()).cacheKey(); + MappingLookup.CacheKey mKey2 = MappingLookup.fromMappers(Mapping.EMPTY, emptyList(), emptyList(), emptyList(), emptyList()) + .cacheKey(); Directory dir = newDirectory(); IndexWriterConfig config = newIndexWriterConfig(); IndexWriter writer = new IndexWriter(dir, config); diff --git a/server/src/test/java/org/elasticsearch/search/suggest/AbstractSuggestionBuilderTestCase.java b/server/src/test/java/org/elasticsearch/search/suggest/AbstractSuggestionBuilderTestCase.java index 928cc53751545..2cebbb7c44ac1 100644 --- a/server/src/test/java/org/elasticsearch/search/suggest/AbstractSuggestionBuilderTestCase.java +++ b/server/src/test/java/org/elasticsearch/search/suggest/AbstractSuggestionBuilderTestCase.java @@ -167,7 +167,7 @@ public void testBuild() throws IOException { invocation -> new TestTemplateService.MockTemplateScript.Factory(((Script) invocation.getArguments()[0]).getIdOrCode()) ); List mappers = Collections.singletonList(new MockFieldMapper(fieldType)); - MappingLookup lookup = MappingLookup.fromMappers(Mapping.EMPTY, mappers, emptyList(), emptyList()); + MappingLookup lookup = MappingLookup.fromMappers(Mapping.EMPTY, mappers, emptyList(), emptyList(), emptyList()); SearchExecutionContext mockContext = new SearchExecutionContext( 0, 0, diff --git a/test/framework/src/main/java/org/elasticsearch/cluster/metadata/DataStreamTestHelper.java b/test/framework/src/main/java/org/elasticsearch/cluster/metadata/DataStreamTestHelper.java index 4cc019a300e8b..a1ab76a817e39 100644 --- a/test/framework/src/main/java/org/elasticsearch/cluster/metadata/DataStreamTestHelper.java +++ b/test/framework/src/main/java/org/elasticsearch/cluster/metadata/DataStreamTestHelper.java @@ -654,7 +654,7 @@ public static MetadataRolloverService getMetadataRolloverService( new MetadataFieldMapper[] { dtfm }, Collections.emptyMap() ); - mappingLookup = MappingLookup.fromMappers(mapping, List.of(dtfm, dateFieldMapper), List.of(), List.of()); + mappingLookup = MappingLookup.fromMappers(mapping, List.of(dtfm, dateFieldMapper), List.of(), List.of(), List.of()); } IndicesService indicesService = mockIndicesServices(mappingLookup); diff --git a/test/framework/src/main/java/org/elasticsearch/search/aggregations/AggregatorTestCase.java b/test/framework/src/main/java/org/elasticsearch/search/aggregations/AggregatorTestCase.java index 1787638f9fdf3..c4582a9a178f2 100644 --- a/test/framework/src/main/java/org/elasticsearch/search/aggregations/AggregatorTestCase.java +++ b/test/framework/src/main/java/org/elasticsearch/search/aggregations/AggregatorTestCase.java @@ -347,7 +347,8 @@ private AggregationContext createAggregationContext( // Alias all fields to -alias to test aliases Arrays.stream(fieldTypes) .map(ft -> new FieldAliasMapper(ft.name() + "-alias", ft.name() + "-alias", ft.name())) - .collect(toList()) + .collect(toList()), + List.of() ); BiFunction> fieldDataBuilder = (fieldType, context) -> fieldType .fielddataBuilder( @@ -459,7 +460,7 @@ private SubSearchContext buildSubSearchContext( * of stuff. */ SearchExecutionContext subContext = spy(searchExecutionContext); - MappingLookup disableNestedLookup = MappingLookup.fromMappers(Mapping.EMPTY, Set.of(), Set.of(), Set.of()); + MappingLookup disableNestedLookup = MappingLookup.fromMappers(Mapping.EMPTY, Set.of(), Set.of(), Set.of(), Set.of()); doReturn(new NestedDocuments(disableNestedLookup, bitsetFilterCache::getBitSetProducer, indexSettings.getIndexVersionCreated())) .when(subContext) .getNestedDocuments(); diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authz/accesscontrol/DocumentSubsetBitsetCacheTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authz/accesscontrol/DocumentSubsetBitsetCacheTests.java index aac6e17cd6ac2..426bc3eb08b6e 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authz/accesscontrol/DocumentSubsetBitsetCacheTests.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authz/accesscontrol/DocumentSubsetBitsetCacheTests.java @@ -66,6 +66,7 @@ import static java.util.Collections.emptyList; import static java.util.Collections.emptyMap; +import static org.hamcrest.Matchers.empty; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.is; import static org.hamcrest.Matchers.not; @@ -645,7 +646,7 @@ private void runTestOnIndices(int numberIndices, CheckedConsumer concreteFields) { List mappers = concreteFields.stream().map(MockFieldMapper::new).collect(Collectors.toList()); - return MappingLookup.fromMappers(Mapping.EMPTY, mappers, emptyList(), emptyList()); + return MappingLookup.fromMappers(Mapping.EMPTY, mappers, emptyList(), emptyList(), emptyList()); } } From c54fd29aeaf5b9dc2cb7ef8c7a396dd3a2e81fc1 Mon Sep 17 00:00:00 2001 From: Kostas Krikellas Date: Wed, 27 Mar 2024 19:05:13 +0200 Subject: [PATCH 02/14] small refactor --- .../index/mapper/MappingLookup.java | 6 ++++- .../mapper/FieldNamesFieldTypeTests.java | 2 +- .../index/mapper/MappingLookupTests.java | 2 +- .../query/SearchExecutionContextTests.java | 2 +- .../indices/IndicesRequestCacheTests.java | 22 ++++--------------- .../AbstractSuggestionBuilderTestCase.java | 2 +- .../metadata/DataStreamTestHelper.java | 2 +- .../aggregations/AggregatorTestCase.java | 2 +- .../DocumentSubsetBitsetCacheTests.java | 3 +-- ...ityIndexReaderWrapperIntegrationTests.java | 2 +- 10 files changed, 17 insertions(+), 28 deletions(-) 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 d6f82e865fc18..adde9aff12b4e 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/MappingLookup.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/MappingLookup.java @@ -40,7 +40,7 @@ private CacheKey() {} * A lookup representing an empty mapping. It can be used to look up fields, although it won't hold any, but it does not * hold a valid {@link DocumentParser}, {@link IndexSettings} or {@link IndexAnalyzers}. */ - public static final MappingLookup EMPTY = fromMappers(Mapping.EMPTY, List.of(), List.of(), List.of(), List.of()); + public static final MappingLookup EMPTY = fromMappers(Mapping.EMPTY, List.of(), List.of()); private final CacheKey cacheKey = new CacheKey(); @@ -129,6 +129,10 @@ public static MappingLookup fromMappers( return new MappingLookup(mapping, mappers, objectMappers, aliasMappers, passThroughMappers); } + public static MappingLookup fromMappers(Mapping mapping, Collection mappers, Collection objectMappers) { + return new MappingLookup(mapping, mappers, objectMappers, List.of(), List.of()); + } + private MappingLookup( Mapping mapping, Collection mappers, diff --git a/server/src/test/java/org/elasticsearch/index/mapper/FieldNamesFieldTypeTests.java b/server/src/test/java/org/elasticsearch/index/mapper/FieldNamesFieldTypeTests.java index b3d0c4041bf6d..31736d7ff9b0f 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/FieldNamesFieldTypeTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/FieldNamesFieldTypeTests.java @@ -35,7 +35,7 @@ public void testTermQuery() { settings ); List mappers = Stream.of(fieldNamesFieldType, fieldType).map(MockFieldMapper::new).toList(); - MappingLookup mappingLookup = MappingLookup.fromMappers(Mapping.EMPTY, mappers, emptyList(), emptyList(), emptyList()); + MappingLookup mappingLookup = MappingLookup.fromMappers(Mapping.EMPTY, mappers, emptyList()); SearchExecutionContext searchExecutionContext = SearchExecutionContextHelper.createSimple(indexSettings, null, null); Query termQuery = fieldNamesFieldType.termQuery("field_name", searchExecutionContext); assertEquals(new TermQuery(new Term(FieldNamesFieldMapper.CONTENT_TYPE, "field_name")), termQuery); diff --git a/server/src/test/java/org/elasticsearch/index/mapper/MappingLookupTests.java b/server/src/test/java/org/elasticsearch/index/mapper/MappingLookupTests.java index 843ff497d639c..b97c912e523be 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/MappingLookupTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/MappingLookupTests.java @@ -47,7 +47,7 @@ private static MappingLookup createMappingLookup( new MetadataFieldMapper[0], Collections.emptyMap() ); - return MappingLookup.fromMappers(mapping, fieldMappers, objectMappers, emptyList(), emptyList()); + return MappingLookup.fromMappers(mapping, fieldMappers, objectMappers); } public void testOnlyRuntimeField() { 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 1c3b9c3ba3d66..bbe29d37572af 100644 --- a/server/src/test/java/org/elasticsearch/index/query/SearchExecutionContextTests.java +++ b/server/src/test/java/org/elasticsearch/index/query/SearchExecutionContextTests.java @@ -294,7 +294,7 @@ private static MappingLookup createMappingLookup(List concreteF new MetadataFieldMapper[0], Collections.emptyMap() ); - return MappingLookup.fromMappers(mapping, mappers, Collections.emptyList(), Collections.emptyList(), Collections.emptyList()); + return MappingLookup.fromMappers(mapping, mappers, Collections.emptyList()); } public void testSearchRequestRuntimeFields() { diff --git a/server/src/test/java/org/elasticsearch/indices/IndicesRequestCacheTests.java b/server/src/test/java/org/elasticsearch/indices/IndicesRequestCacheTests.java index 2225a2da21f5f..e8f6061efb8d9 100644 --- a/server/src/test/java/org/elasticsearch/indices/IndicesRequestCacheTests.java +++ b/server/src/test/java/org/elasticsearch/indices/IndicesRequestCacheTests.java @@ -203,8 +203,7 @@ public void testCacheDifferentReaders() throws Exception { public void testCacheDifferentMapping() throws Exception { IndicesRequestCache cache = new IndicesRequestCache(Settings.EMPTY); MappingLookup.CacheKey mappingKey1 = MappingLookup.EMPTY.cacheKey(); - MappingLookup.CacheKey mappingKey2 = MappingLookup.fromMappers(Mapping.EMPTY, emptyList(), emptyList(), emptyList(), emptyList()) - .cacheKey(); + MappingLookup.CacheKey mappingKey2 = MappingLookup.fromMappers(Mapping.EMPTY, emptyList(), emptyList()).cacheKey(); AtomicBoolean indexShard = new AtomicBoolean(true); ShardRequestCache requestCacheStats = new ShardRequestCache(); Directory dir = newDirectory(); @@ -364,25 +363,13 @@ public void testClearAllEntityIdentity() throws Exception { writer.updateDocument(new Term("id", "0"), newDoc(0, "bar")); DirectoryReader secondReader = ElasticsearchDirectoryReader.wrap(DirectoryReader.open(writer), new ShardId("foo", "bar", 1)); - MappingLookup.CacheKey secondMappingKey = MappingLookup.fromMappers( - Mapping.EMPTY, - emptyList(), - emptyList(), - emptyList(), - emptyList() - ).cacheKey(); + MappingLookup.CacheKey secondMappingKey = MappingLookup.fromMappers(Mapping.EMPTY, emptyList(), emptyList()).cacheKey(); TestEntity secondEntity = new TestEntity(requestCacheStats, indexShard); Loader secondLoader = new Loader(secondReader, 0); writer.updateDocument(new Term("id", "0"), newDoc(0, "baz")); DirectoryReader thirdReader = ElasticsearchDirectoryReader.wrap(DirectoryReader.open(writer), new ShardId("foo", "bar", 1)); - MappingLookup.CacheKey thirdMappingKey = MappingLookup.fromMappers( - Mapping.EMPTY, - emptyList(), - emptyList(), - emptyList(), - emptyList() - ).cacheKey(); + MappingLookup.CacheKey thirdMappingKey = MappingLookup.fromMappers(Mapping.EMPTY, emptyList(), emptyList()).cacheKey(); AtomicBoolean differentIdentity = new AtomicBoolean(true); TestEntity thirdEntity = new TestEntity(requestCacheStats, differentIdentity); Loader thirdLoader = new Loader(thirdReader, 0); @@ -518,8 +505,7 @@ public void testKeyEqualsAndHashCode() throws IOException { AtomicBoolean trueBoolean = new AtomicBoolean(true); AtomicBoolean falseBoolean = new AtomicBoolean(false); MappingLookup.CacheKey mKey1 = MappingLookup.EMPTY.cacheKey(); - MappingLookup.CacheKey mKey2 = MappingLookup.fromMappers(Mapping.EMPTY, emptyList(), emptyList(), emptyList(), emptyList()) - .cacheKey(); + MappingLookup.CacheKey mKey2 = MappingLookup.fromMappers(Mapping.EMPTY, emptyList(), emptyList()).cacheKey(); Directory dir = newDirectory(); IndexWriterConfig config = newIndexWriterConfig(); IndexWriter writer = new IndexWriter(dir, config); diff --git a/server/src/test/java/org/elasticsearch/search/suggest/AbstractSuggestionBuilderTestCase.java b/server/src/test/java/org/elasticsearch/search/suggest/AbstractSuggestionBuilderTestCase.java index 2cebbb7c44ac1..4c911559df287 100644 --- a/server/src/test/java/org/elasticsearch/search/suggest/AbstractSuggestionBuilderTestCase.java +++ b/server/src/test/java/org/elasticsearch/search/suggest/AbstractSuggestionBuilderTestCase.java @@ -167,7 +167,7 @@ public void testBuild() throws IOException { invocation -> new TestTemplateService.MockTemplateScript.Factory(((Script) invocation.getArguments()[0]).getIdOrCode()) ); List mappers = Collections.singletonList(new MockFieldMapper(fieldType)); - MappingLookup lookup = MappingLookup.fromMappers(Mapping.EMPTY, mappers, emptyList(), emptyList(), emptyList()); + MappingLookup lookup = MappingLookup.fromMappers(Mapping.EMPTY, mappers, emptyList()); SearchExecutionContext mockContext = new SearchExecutionContext( 0, 0, diff --git a/test/framework/src/main/java/org/elasticsearch/cluster/metadata/DataStreamTestHelper.java b/test/framework/src/main/java/org/elasticsearch/cluster/metadata/DataStreamTestHelper.java index a1ab76a817e39..5c95c82fa9876 100644 --- a/test/framework/src/main/java/org/elasticsearch/cluster/metadata/DataStreamTestHelper.java +++ b/test/framework/src/main/java/org/elasticsearch/cluster/metadata/DataStreamTestHelper.java @@ -654,7 +654,7 @@ public static MetadataRolloverService getMetadataRolloverService( new MetadataFieldMapper[] { dtfm }, Collections.emptyMap() ); - mappingLookup = MappingLookup.fromMappers(mapping, List.of(dtfm, dateFieldMapper), List.of(), List.of(), List.of()); + mappingLookup = MappingLookup.fromMappers(mapping, List.of(dtfm, dateFieldMapper), List.of()); } IndicesService indicesService = mockIndicesServices(mappingLookup); diff --git a/test/framework/src/main/java/org/elasticsearch/search/aggregations/AggregatorTestCase.java b/test/framework/src/main/java/org/elasticsearch/search/aggregations/AggregatorTestCase.java index c4582a9a178f2..d8443ab6e778f 100644 --- a/test/framework/src/main/java/org/elasticsearch/search/aggregations/AggregatorTestCase.java +++ b/test/framework/src/main/java/org/elasticsearch/search/aggregations/AggregatorTestCase.java @@ -460,7 +460,7 @@ private SubSearchContext buildSubSearchContext( * of stuff. */ SearchExecutionContext subContext = spy(searchExecutionContext); - MappingLookup disableNestedLookup = MappingLookup.fromMappers(Mapping.EMPTY, Set.of(), Set.of(), Set.of(), Set.of()); + MappingLookup disableNestedLookup = MappingLookup.fromMappers(Mapping.EMPTY, Set.of(), Set.of()); doReturn(new NestedDocuments(disableNestedLookup, bitsetFilterCache::getBitSetProducer, indexSettings.getIndexVersionCreated())) .when(subContext) .getNestedDocuments(); diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authz/accesscontrol/DocumentSubsetBitsetCacheTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authz/accesscontrol/DocumentSubsetBitsetCacheTests.java index 426bc3eb08b6e..2c6a962758cba 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authz/accesscontrol/DocumentSubsetBitsetCacheTests.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authz/accesscontrol/DocumentSubsetBitsetCacheTests.java @@ -66,7 +66,6 @@ import static java.util.Collections.emptyList; import static java.util.Collections.emptyMap; -import static org.hamcrest.Matchers.empty; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.is; import static org.hamcrest.Matchers.not; @@ -646,7 +645,7 @@ private void runTestOnIndices(int numberIndices, CheckedConsumer concreteFields) { List mappers = concreteFields.stream().map(MockFieldMapper::new).collect(Collectors.toList()); - return MappingLookup.fromMappers(Mapping.EMPTY, mappers, emptyList(), emptyList(), emptyList()); + return MappingLookup.fromMappers(Mapping.EMPTY, mappers, emptyList()); } } From 6ec86995f9e182d2bb6207fc253549291c29fc75 Mon Sep 17 00:00:00 2001 From: Kostas Krikellas Date: Wed, 27 Mar 2024 22:36:11 +0200 Subject: [PATCH 03/14] conflict resolution --- .../test/data_stream/150_tsdb.yml | 5 +- .../index/mapper/FieldTypeLookup.java | 14 ++++-- .../index/mapper/PassThroughObjectMapper.java | 46 +++++++++++++++++-- 3 files changed, 56 insertions(+), 9 deletions(-) diff --git a/modules/data-streams/src/yamlRestTest/resources/rest-api-spec/test/data_stream/150_tsdb.yml b/modules/data-streams/src/yamlRestTest/resources/rest-api-spec/test/data_stream/150_tsdb.yml index 683cf675cda8e..454f578b0e908 100644 --- a/modules/data-streams/src/yamlRestTest/resources/rest-api-spec/test/data_stream/150_tsdb.yml +++ b/modules/data-streams/src/yamlRestTest/resources/rest-api-spec/test/data_stream/150_tsdb.yml @@ -354,6 +354,7 @@ dynamic templates - conflicting aliases: type: passthrough dynamic: true time_series_dimension: true + superseded_by: attributes dynamic_templates: - counter_metric: mapping: @@ -391,7 +392,7 @@ dynamic templates - conflicting aliases: filterA: filter: term: - dim: "C" + dim: A aggs: tsids: terms: @@ -410,7 +411,7 @@ dynamic templates - conflicting aliases: filterA: filter: term: - attributes.dim: A + resource_attributes.dim: C aggs: tsids: terms: 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 c80002e3114ed..a01e8374f39c3 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/FieldTypeLookup.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/FieldTypeLookup.java @@ -92,13 +92,21 @@ final class FieldTypeLookup { } } + Map passThroughFieldAliases = new HashMap<>(); for (FieldMapper fieldMapper : fieldMappers) { String fieldName = fieldMapper.name(); for (PassThroughObjectMapper mapper : passThroughMappers) { if (fieldName.startsWith(mapper.name())) { - String name = fieldName.substring(mapper.name().length() + 1); - // Check if there's an existing field or alias for the same field. - if (fullNameToFieldType.containsKey(name)) { + String name = fieldMapper.simpleName(); + // Check for conflict between PassThroughObjectMapper subfields. + PassThroughObjectMapper conflict = passThroughFieldAliases.put(name, mapper); + if (conflict != null) { + if (mapper.supersededBy().contains(conflict.name())) { + passThroughFieldAliases.put(name, conflict); + continue; + } + } else if (fullNameToFieldType.containsKey(name)) { + // There's an existing field or alias for the same field. continue; } MappedFieldType fieldType = fieldMapper.fieldType(); diff --git a/server/src/main/java/org/elasticsearch/index/mapper/PassThroughObjectMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/PassThroughObjectMapper.java index 16b4d0b49917f..abb3298b84373 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/PassThroughObjectMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/PassThroughObjectMapper.java @@ -14,10 +14,14 @@ import org.elasticsearch.xcontent.XContentBuilder; import java.io.IOException; +import java.util.Arrays; import java.util.Locale; import java.util.Map; +import java.util.Set; +import java.util.TreeSet; import static org.elasticsearch.common.xcontent.support.XContentMapValues.nodeBooleanValue; +import static org.elasticsearch.common.xcontent.support.XContentMapValues.nodeStringArrayValue; /** * Mapper for pass-through objects. @@ -28,12 +32,16 @@ */ public class PassThroughObjectMapper extends ObjectMapper { public static final String CONTENT_TYPE = "passthrough"; + public static final String SUPERSEDED_BY_PARAM = "superseded_by"; public static class Builder extends ObjectMapper.Builder { // Controls whether subfields are configured as time-series dimensions. protected Explicit timeSeriesDimensionSubFields = Explicit.IMPLICIT_FALSE; + // Controls which pass-through fields take precedence in case of conflicting aliases. + protected Set supersededBy = Set.of(); + public Builder(String name) { // Subobjects are not currently supported. super(name, Explicit.IMPLICIT_FALSE); @@ -61,7 +69,8 @@ public PassThroughObjectMapper build(MapperBuilderContext context) { enabled, dynamic, buildMappers(context.createChildContext(name(), timeSeriesDimensionSubFields.value(), dynamic)), - timeSeriesDimensionSubFields + timeSeriesDimensionSubFields, + supersededBy ); } } @@ -69,34 +78,51 @@ public PassThroughObjectMapper build(MapperBuilderContext context) { // If set, its subfields are marked as time-series dimensions (for the types supporting this). private final Explicit timeSeriesDimensionSubFields; + private final Set supersededBy; + PassThroughObjectMapper( String name, String fullPath, Explicit enabled, Dynamic dynamic, Map mappers, - Explicit timeSeriesDimensionSubFields + Explicit timeSeriesDimensionSubFields, + Set supersededBy ) { // Subobjects are not currently supported. super(name, fullPath, enabled, Explicit.IMPLICIT_FALSE, dynamic, mappers); this.timeSeriesDimensionSubFields = timeSeriesDimensionSubFields; + this.supersededBy = supersededBy; } @Override PassThroughObjectMapper withoutMappers() { - return new PassThroughObjectMapper(simpleName(), fullPath(), enabled, dynamic, Map.of(), timeSeriesDimensionSubFields); + return new PassThroughObjectMapper( + simpleName(), + fullPath(), + enabled, + dynamic, + Map.of(), + timeSeriesDimensionSubFields, + supersededBy + ); } public boolean containsDimensions() { return timeSeriesDimensionSubFields.value(); } + public Set supersededBy() { + return supersededBy; + } + @Override public PassThroughObjectMapper.Builder newBuilder(IndexVersion indexVersionCreated) { PassThroughObjectMapper.Builder builder = new PassThroughObjectMapper.Builder(simpleName()); builder.enabled = enabled; builder.dynamic = dynamic; builder.timeSeriesDimensionSubFields = timeSeriesDimensionSubFields; + builder.supersededBy = supersededBy; return builder; } @@ -108,13 +134,17 @@ public PassThroughObjectMapper merge(ObjectMapper mergeWith, MergeReason reason, ? mergeWithObject.timeSeriesDimensionSubFields : this.timeSeriesDimensionSubFields; + Set mergedSupersededBy = new TreeSet<>(supersededBy); + mergedSupersededBy.addAll(mergeWithObject.supersededBy); + return new PassThroughObjectMapper( simpleName(), fullPath(), mergeResult.enabled(), mergeResult.dynamic(), mergeResult.mappers(), - containsDimensions + containsDimensions, + mergedSupersededBy ); } @@ -125,6 +155,9 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws if (timeSeriesDimensionSubFields.explicit()) { builder.field(TimeSeriesParams.TIME_SERIES_DIMENSION_PARAM, timeSeriesDimensionSubFields.value()); } + if (supersededBy.isEmpty() == false) { + builder.field(SUPERSEDED_BY_PARAM, supersededBy); + } if (dynamic != null) { builder.field("dynamic", dynamic.name().toLowerCase(Locale.ROOT)); } @@ -153,6 +186,11 @@ protected static void parsePassthrough(String name, Map node, Pa ); node.remove(TimeSeriesParams.TIME_SERIES_DIMENSION_PARAM); } + fieldNode = node.get(SUPERSEDED_BY_PARAM); + if (fieldNode != null) { + builder.supersededBy = new TreeSet<>(Arrays.asList(nodeStringArrayValue(fieldNode))); + node.remove(SUPERSEDED_BY_PARAM); + } } } } From 05a601615de2e2ee7359e77450e113d93d74a7a0 Mon Sep 17 00:00:00 2001 From: Kostas Krikellas Date: Thu, 28 Mar 2024 15:15:14 +0200 Subject: [PATCH 04/14] add check for circular deps --- .../test/data_stream/150_tsdb.yml | 32 ++++- .../index/mapper/FieldTypeLookup.java | 3 + .../index/mapper/MappingLookup.java | 9 ++ .../index/mapper/PassThroughObjectMapper.java | 62 ++++++++ .../index/mapper/RootObjectMapper.java | 4 - .../index/mapper/FieldTypeLookupTests.java | 74 ++++++++++ .../mapper/PassThroughObjectMapperTests.java | 135 ++++++++++++++++++ .../index/mapper/MockFieldMapper.java | 4 + 8 files changed, 317 insertions(+), 6 deletions(-) diff --git a/modules/data-streams/src/yamlRestTest/resources/rest-api-spec/test/data_stream/150_tsdb.yml b/modules/data-streams/src/yamlRestTest/resources/rest-api-spec/test/data_stream/150_tsdb.yml index 454f578b0e908..f65a458755c22 100644 --- a/modules/data-streams/src/yamlRestTest/resources/rest-api-spec/test/data_stream/150_tsdb.yml +++ b/modules/data-streams/src/yamlRestTest/resources/rest-api-spec/test/data_stream/150_tsdb.yml @@ -834,11 +834,39 @@ enable subobjects in passthrough object: index: number_of_shards: 1 mode: time_series - time_series: - start_time: 2023-08-31T13:03:08.138Z mappings: properties: attributes: type: passthrough subobjects: true + +--- +passthrough objects with circular deps: + - skip: + version: " - 8.12.99" + reason: "Support for passthrough fields was added in 8.13" + - do: + catch: /Circular dependency between pass-through objects/ + indices.put_index_template: + name: my-dynamic-template + body: + index_patterns: [k9s*] + data_stream: {} + template: + settings: + index: + number_of_shards: 1 + mode: time_series + + mappings: + properties: + attributes: + type: passthrough + superseded_by: system.attributes + resource.attributes: + type: passthrough + superseded_by: attributes + system.attributes: + type: passthrough + superseded_by: resource.attributes 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 a01e8374f39c3..46bd492a64c4e 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/FieldTypeLookup.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/FieldTypeLookup.java @@ -92,6 +92,9 @@ final class FieldTypeLookup { } } + // Pass-though subfields can be referenced without the prefix corresponding to the + // PassThroughObjectMapper name. This is achieved by adding a second reference to their + // MappedFieldType using the remaining suffix. Map passThroughFieldAliases = new HashMap<>(); for (FieldMapper fieldMapper : fieldMappers) { String fieldName = fieldMapper.name(); 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 adde9aff12b4e..7949237463be4 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/MappingLookup.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/MappingLookup.java @@ -23,6 +23,7 @@ import java.util.Map; import java.util.Set; import java.util.function.Function; +import java.util.stream.Collectors; /** * A (mostly) immutable snapshot of the current mapping of an index with @@ -184,6 +185,14 @@ private MappingLookup( } } + List sequence = PassThroughObjectMapper.checkSupersedesForCircularDeps(passThroughMappers); + if (sequence.isEmpty() == false) { + throw new MapperParsingException( + "Circular dependency between pass-through objects: " + + sequence.stream().map(PassThroughObjectMapper::name).collect(Collectors.joining(" => ")) + ); + } + final Collection runtimeFields = mapping.getRoot().runtimeFields(); this.fieldTypeLookup = new FieldTypeLookup(mappers, aliasMappers, passThroughMappers, runtimeFields); if (runtimeFields.isEmpty()) { diff --git a/server/src/main/java/org/elasticsearch/index/mapper/PassThroughObjectMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/PassThroughObjectMapper.java index abb3298b84373..d4f84005f2757 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/PassThroughObjectMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/PassThroughObjectMapper.java @@ -15,10 +15,16 @@ import java.io.IOException; import java.util.Arrays; +import java.util.Collection; +import java.util.HashMap; +import java.util.List; import java.util.Locale; import java.util.Map; import java.util.Set; +import java.util.Stack; import java.util.TreeSet; +import java.util.function.Function; +import java.util.stream.Collectors; import static org.elasticsearch.common.xcontent.support.XContentMapValues.nodeBooleanValue; import static org.elasticsearch.common.xcontent.support.XContentMapValues.nodeStringArrayValue; @@ -34,6 +40,8 @@ public class PassThroughObjectMapper extends ObjectMapper { public static final String CONTENT_TYPE = "passthrough"; public static final String SUPERSEDED_BY_PARAM = "superseded_by"; + private static final int CIRCULAR_DEPENDENCY_MAX_OPS = 10_000; + public static class Builder extends ObjectMapper.Builder { // Controls whether subfields are configured as time-series dimensions. @@ -93,6 +101,12 @@ public PassThroughObjectMapper build(MapperBuilderContext context) { super(name, fullPath, enabled, Explicit.IMPLICIT_FALSE, dynamic, mappers); this.timeSeriesDimensionSubFields = timeSeriesDimensionSubFields; this.supersededBy = supersededBy; + + if (supersededBy.contains(fullPath)) { + throw new MapperException( + "Mapping definition for [" + fullPath + "] contains a self-reference in param [" + SUPERSEDED_BY_PARAM + "]" + ); + } } @Override @@ -193,4 +207,52 @@ protected static void parsePassthrough(String name, Map node, Pa } } } + + /** + * Checks the passed objects for circular dependencies on the superseded_by cross-dependencies. + * @param passThroughMappers objects to check + * @return A list containing the circular dependency chain, or an empty list if no circular dependency is found + */ + public static List checkSupersedesForCircularDeps(Collection passThroughMappers) { + if (passThroughMappers.size() < 2) { + return List.of(); + } + + // Perform DFS. + // The implementation below assumes that the graph is rather small and sparse. Further optimizations are + // required (e.g. use sets for lookups) if production systems end up with hundreds of tightly-coupled objects. + int i = 0; + Map lookupByName = passThroughMappers.stream() + .collect(Collectors.toMap(PassThroughObjectMapper::name, Function.identity())); + for (PassThroughObjectMapper start : passThroughMappers) { + Stack sequence = new Stack<>(); + Map reverseDeps = new HashMap<>(); + Stack pending = new Stack<>(); + pending.push(start); + while (pending.empty() == false) { + if (++i > CIRCULAR_DEPENDENCY_MAX_OPS) { // Defensive check. + break; + } + PassThroughObjectMapper current = pending.pop(); + // If we reached the end of a branch, unwind the stack till the beginning of the next branch. + while (sequence.isEmpty() == false && reverseDeps.get(current) != sequence.peek()) { + sequence.pop(); + } + // Check if the branch contains the same element twice. + if (sequence.contains(current)) { + sequence.push(current); + return sequence; + } + sequence.push(current); + for (String dep : current.supersededBy()) { + PassThroughObjectMapper mapper = lookupByName.get(dep); + if (mapper != null) { + pending.push(mapper); + reverseDeps.put(mapper, current); + } + } + } + } + return List.of(); + } } diff --git a/server/src/main/java/org/elasticsearch/index/mapper/RootObjectMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/RootObjectMapper.java index 6ccff0f52cdf2..d42ad02cc0939 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/RootObjectMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/RootObjectMapper.java @@ -18,8 +18,6 @@ import org.elasticsearch.index.IndexVersions; import org.elasticsearch.index.mapper.DynamicTemplate.XContentFieldType; import org.elasticsearch.index.mapper.MapperService.MergeReason; -import org.elasticsearch.logging.LogManager; -import org.elasticsearch.logging.Logger; import org.elasticsearch.xcontent.ToXContent; import org.elasticsearch.xcontent.XContentBuilder; @@ -77,8 +75,6 @@ public static class Builder extends ObjectMapper.Builder { protected Explicit dateDetection = Defaults.DATE_DETECTION; protected Explicit numericDetection = Defaults.NUMERIC_DETECTION; - private static final Logger logger = LogManager.getLogger(RootObjectMapper.Builder.class); - public Builder(String name, Explicit subobjects) { super(name, subobjects); } 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 aa9708dba0add..7dba80698b35a 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/FieldTypeLookupTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/FieldTypeLookupTests.java @@ -8,6 +8,7 @@ package org.elasticsearch.index.mapper; +import org.elasticsearch.common.Explicit; import org.elasticsearch.index.mapper.flattened.FlattenedFieldMapper; import org.elasticsearch.test.ESTestCase; import org.hamcrest.Matchers; @@ -16,6 +17,7 @@ import java.util.Collection; import java.util.Collections; import java.util.List; +import java.util.Map; import java.util.Set; import static java.util.Collections.emptyList; @@ -423,4 +425,76 @@ public void testRuntimeFieldNameOutsideContext() { private static FlattenedFieldMapper createFlattenedMapper(String fieldName) { return new FlattenedFieldMapper.Builder(fieldName).build(MapperBuilderContext.root(false, false)); } + + private PassThroughObjectMapper createPassThroughMapper(String name, Set supersededBy) { + return new PassThroughObjectMapper( + name, + name, + Explicit.EXPLICIT_TRUE, + ObjectMapper.Dynamic.FALSE, + Map.of(), + Explicit.EXPLICIT_FALSE, + supersededBy + ); + } + + public void testAddRootAliasesForPassThroughFields() { + MockFieldMapper foo = new MockFieldMapper("attributes.foo"); + MockFieldMapper bar = new MockFieldMapper( + new MockFieldMapper.FakeFieldType("attributes.much.more.deeply.nested.bar"), + "much.more.deeply.nested.bar" + ); + PassThroughObjectMapper attributes = createPassThroughMapper("attributes", Set.of()); + + MockFieldMapper baz = new MockFieldMapper("resource.attributes.baz"); + MockFieldMapper bag = new MockFieldMapper( + new MockFieldMapper.FakeFieldType("resource.attributes.much.more.deeply.nested.bag"), + "much.more.deeply.nested.bag" + ); + PassThroughObjectMapper resourceAttributes = createPassThroughMapper("resource.attributes", Set.of("")); + + FieldTypeLookup lookup = new FieldTypeLookup( + List.of(foo, bar, baz, bag), + List.of(), + List.of(attributes, resourceAttributes), + List.of() + ); + assertEquals(foo.fieldType(), lookup.get("foo")); + assertEquals(bar.fieldType(), lookup.get("much.more.deeply.nested.bar")); + assertEquals(baz.fieldType(), lookup.get("baz")); + assertEquals(bag.fieldType(), lookup.get("much.more.deeply.nested.bag")); + } + + public void testNoPassThroughField() { + MockFieldMapper field = new MockFieldMapper("labels.foo"); + PassThroughObjectMapper attributes = createPassThroughMapper("attributes", Set.of()); + + FieldTypeLookup lookup = new FieldTypeLookup(List.of(field), List.of(), List.of(attributes), List.of()); + assertNull(lookup.get("foo")); + } + + public void testAddRootAliasForConflictingPassThroughFields() { + MockFieldMapper attributeField = new MockFieldMapper("attributes.foo"); + PassThroughObjectMapper attributes = createPassThroughMapper("attributes", Set.of()); + + MockFieldMapper resourceAttributeField = new MockFieldMapper("resource.attributes.foo"); + PassThroughObjectMapper resourceAttributes = createPassThroughMapper("resource.attributes", Set.of("attributes")); + + FieldTypeLookup lookup = new FieldTypeLookup( + List.of(attributeField, resourceAttributeField), + List.of(), + List.of(attributes, resourceAttributes), + List.of() + ); + assertEquals(attributeField.fieldType(), lookup.get("foo")); + } + + public void testNoRootAliasForPassThroughFieldOnConflictingField() { + MockFieldMapper attributeFoo = new MockFieldMapper("attributes.foo"); + MockFieldMapper foo = new MockFieldMapper("foo"); + PassThroughObjectMapper attributes = createPassThroughMapper("attributes", Set.of()); + + FieldTypeLookup lookup = new FieldTypeLookup(List.of(foo, attributeFoo), List.of(), List.of(attributes), List.of()); + assertEquals(foo.fieldType(), lookup.get("foo")); + } } diff --git a/server/src/test/java/org/elasticsearch/index/mapper/PassThroughObjectMapperTests.java b/server/src/test/java/org/elasticsearch/index/mapper/PassThroughObjectMapperTests.java index b49ed2cf99df6..cdf22271a8f90 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/PassThroughObjectMapperTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/PassThroughObjectMapperTests.java @@ -8,8 +8,16 @@ package org.elasticsearch.index.mapper; +import org.elasticsearch.common.Explicit; + import java.io.IOException; +import java.util.List; +import java.util.Map; +import java.util.Set; +import java.util.stream.Collectors; +import static org.hamcrest.Matchers.contains; +import static org.hamcrest.Matchers.containsInAnyOrder; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.instanceOf; @@ -45,6 +53,53 @@ public void testKeywordDimension() throws IOException { assertTrue(((KeywordFieldMapper) mapper).fieldType().isDimension()); } + public void testSupersededByOne() throws IOException { + MapperService mapperService = createMapperService(mapping(b -> { + b.startObject("labels").field("type", "passthrough").field("superseded_by", "foo"); + { + b.startObject("properties"); + b.startObject("dim").field("type", "keyword").endObject(); + b.endObject(); + } + b.endObject(); + })); + Mapper mapper = mapperService.mappingLookup().objectMappers().get("labels"); + assertThat(mapper, instanceOf(PassThroughObjectMapper.class)); + assertThat(((PassThroughObjectMapper) mapper).supersededBy(), contains("foo")); + } + + public void testSupersededByMany() throws IOException { + MapperService mapperService = createMapperService(mapping(b -> { + b.startObject("labels").field("type", "passthrough").field("superseded_by", "far,too,many,deps,so,many"); + { + b.startObject("properties"); + b.startObject("dim").field("type", "keyword").endObject(); + b.endObject(); + } + b.endObject(); + })); + Mapper mapper = mapperService.mappingLookup().objectMappers().get("labels"); + assertThat(mapper, instanceOf(PassThroughObjectMapper.class)); + assertThat(((PassThroughObjectMapper) mapper).supersededBy(), containsInAnyOrder("far", "too", "many", "deps", "so")); + } + + public void testSupersededBySelfReference() throws IOException { + MapperException exception = expectThrows(MapperException.class, () -> createMapperService(mapping(b -> { + b.startObject("labels").field("type", "passthrough").field("superseded_by", "labels"); + { + b.startObject("properties"); + b.startObject("dim").field("type", "keyword").endObject(); + b.endObject(); + } + b.endObject(); + }))); + + assertEquals( + "Failed to parse mapping: Mapping definition for [labels] contains a self-reference in param [superseded_by]", + exception.getMessage() + ); + } + public void testDynamic() throws IOException { MapperService mapperService = createMapperService(mapping(b -> { b.startObject("labels").field("type", "passthrough").field("dynamic", "true"); @@ -131,4 +186,84 @@ public void testWithoutMappers() throws IOException { var shallow = mapperService.mappingLookup().objectMappers().get("shallow"); assertThat(labels.withoutMappers().toString(), equalTo(shallow.toString().replace("shallow", "labels"))); } + + public void testCheckSupersedesForCircularDepsEmpty() throws IOException { + assertTrue(PassThroughObjectMapper.checkSupersedesForCircularDeps(List.of()).isEmpty()); + } + + private PassThroughObjectMapper create(String name, Set supersededBy) { + return new PassThroughObjectMapper( + name, + name, + Explicit.EXPLICIT_TRUE, + ObjectMapper.Dynamic.FALSE, + Map.of(), + Explicit.EXPLICIT_FALSE, + supersededBy + ); + } + + public void testCheckSupersedesForCircularDepsOneElement() throws IOException { + assertTrue(PassThroughObjectMapper.checkSupersedesForCircularDeps(List.of(create("foo", Set.of("bar")))).isEmpty()); + } + + public void testCheckSupersedesForCircularDepsTwoElementsNoDep() throws IOException { + assertTrue( + PassThroughObjectMapper.checkSupersedesForCircularDeps( + List.of(create("foo", Set.of("A", "B", "C")), create("bar", Set.of("D", "E"))) + ).isEmpty() + ); + } + + public void testCheckSupersedesForCircularDepsTwoElementsOneDep() throws IOException { + assertTrue( + PassThroughObjectMapper.checkSupersedesForCircularDeps( + List.of(create("foo", Set.of("A", "B", "C")), create("bar", Set.of("foo", "D", "E"))) + ).isEmpty() + ); + } + + public void testCheckSupersedesForCircularDepsTwoElementsCrossDep() throws IOException { + assertThat( + PassThroughObjectMapper.checkSupersedesForCircularDeps( + List.of(create("foo", Set.of("A", "B", "bar")), create("bar", Set.of("foo", "D", "E"))) + ).stream().map(PassThroughObjectMapper::name).collect(Collectors.toList()), + contains("foo", "bar", "foo") + ); + } + + public void testCheckSupersedesForCircularDepsManyElementsWithCircle() throws IOException { + assertThat( + PassThroughObjectMapper.checkSupersedesForCircularDeps( + List.of( + create("A", Set.of("B", "D")), + create("B", Set.of("E", "G")), + create("C", Set.of()), + create("D", Set.of("C")), + create("E", Set.of("C")), + create("F", Set.of("A", "B")), + create("G", Set.of("C", "H")), + create("H", Set.of("A", "C")) + ) + ).stream().map(PassThroughObjectMapper::name).collect(Collectors.toList()), + contains("A", "B", "G", "H", "A") + ); + } + + public void testCheckSupersedesForCircularDepsManyElementsNoCircle() throws IOException { + assertTrue( + PassThroughObjectMapper.checkSupersedesForCircularDeps( + List.of( + create("A", Set.of("B", "D")), + create("B", Set.of("E", "G")), + create("C", Set.of()), + create("D", Set.of("C")), + create("E", Set.of("C")), + create("F", Set.of("A", "B")), + create("G", Set.of("C", "H")), + create("H", Set.of("C", "E")) + ) + ).isEmpty() + ); + } } diff --git a/test/framework/src/main/java/org/elasticsearch/index/mapper/MockFieldMapper.java b/test/framework/src/main/java/org/elasticsearch/index/mapper/MockFieldMapper.java index cb028c746a8cd..b95619602573c 100644 --- a/test/framework/src/main/java/org/elasticsearch/index/mapper/MockFieldMapper.java +++ b/test/framework/src/main/java/org/elasticsearch/index/mapper/MockFieldMapper.java @@ -24,6 +24,10 @@ public MockFieldMapper(MappedFieldType fieldType) { this(findSimpleName(fieldType.name()), fieldType, MultiFields.empty(), CopyTo.empty()); } + public MockFieldMapper(MappedFieldType fieldType, String simpleName) { + super(simpleName, fieldType, MultiFields.empty(), CopyTo.empty(), false, null); + } + public MockFieldMapper(String fullName, MappedFieldType fieldType, MultiFields multifields, CopyTo copyTo) { super(findSimpleName(fullName), fieldType, multifields, copyTo, false, null); } From c084c316c7b2cdededb2fdf9d5ad4f0aa8dde9e2 Mon Sep 17 00:00:00 2001 From: Kostas Krikellas Date: Thu, 28 Mar 2024 15:51:45 +0200 Subject: [PATCH 05/14] use lexicographical ordering for conflicting aliases --- .../index/mapper/FieldTypeLookupTests.java | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) 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 7dba80698b35a..d3a12b145b6f8 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/FieldTypeLookupTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/FieldTypeLookupTests.java @@ -478,7 +478,7 @@ public void testAddRootAliasForConflictingPassThroughFields() { PassThroughObjectMapper attributes = createPassThroughMapper("attributes", Set.of()); MockFieldMapper resourceAttributeField = new MockFieldMapper("resource.attributes.foo"); - PassThroughObjectMapper resourceAttributes = createPassThroughMapper("resource.attributes", Set.of("attributes")); + PassThroughObjectMapper resourceAttributes = createPassThroughMapper("resource.attributes", Set.of()); FieldTypeLookup lookup = new FieldTypeLookup( List.of(attributeField, resourceAttributeField), @@ -489,6 +489,22 @@ public void testAddRootAliasForConflictingPassThroughFields() { assertEquals(attributeField.fieldType(), lookup.get("foo")); } + public void testAddRootAliasForConflictingPassThroughFieldsSuperseded() { + MockFieldMapper attributeField = new MockFieldMapper("attributes.foo"); + PassThroughObjectMapper attributes = createPassThroughMapper("attributes", Set.of("resource.attributes")); + + MockFieldMapper resourceAttributeField = new MockFieldMapper("resource.attributes.foo"); + PassThroughObjectMapper resourceAttributes = createPassThroughMapper("resource.attributes", Set.of()); + + FieldTypeLookup lookup = new FieldTypeLookup( + List.of(attributeField, resourceAttributeField), + List.of(), + List.of(attributes, resourceAttributes), + List.of() + ); + assertEquals(resourceAttributeField.fieldType(), lookup.get("foo")); + } + public void testNoRootAliasForPassThroughFieldOnConflictingField() { MockFieldMapper attributeFoo = new MockFieldMapper("attributes.foo"); MockFieldMapper foo = new MockFieldMapper("foo"); From 5109d56c7882370e54c2dd63c200fba1b6efaae3 Mon Sep 17 00:00:00 2001 From: Kostas Krikellas Date: Thu, 28 Mar 2024 15:51:55 +0200 Subject: [PATCH 06/14] use lexicographical ordering for conflicting aliases --- .../datastreams/DataStreamGetWriteIndexTests.java | 8 +------- .../org/elasticsearch/index/mapper/FieldTypeLookup.java | 4 +++- .../index/mapper/PassThroughObjectMapper.java | 4 ++++ 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/modules/data-streams/src/test/java/org/elasticsearch/datastreams/DataStreamGetWriteIndexTests.java b/modules/data-streams/src/test/java/org/elasticsearch/datastreams/DataStreamGetWriteIndexTests.java index 52022b34901c8..a877466cd30f2 100644 --- a/modules/data-streams/src/test/java/org/elasticsearch/datastreams/DataStreamGetWriteIndexTests.java +++ b/modules/data-streams/src/test/java/org/elasticsearch/datastreams/DataStreamGetWriteIndexTests.java @@ -239,13 +239,7 @@ public void setup() throws Exception { new MetadataFieldMapper[] { dtfm }, Collections.emptyMap() ); - MappingLookup mappingLookup = MappingLookup.fromMappers( - mapping, - List.of(dtfm, dateFieldMapper), - List.of(), - List.of(), - List.of() - ); + MappingLookup mappingLookup = MappingLookup.fromMappers(mapping, List.of(dtfm, dateFieldMapper), List.of()); indicesService = DataStreamTestHelper.mockIndicesServices(mappingLookup); } 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 46bd492a64c4e..f3d04de037d80 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/FieldTypeLookup.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/FieldTypeLookup.java @@ -104,7 +104,9 @@ final class FieldTypeLookup { // Check for conflict between PassThroughObjectMapper subfields. PassThroughObjectMapper conflict = passThroughFieldAliases.put(name, mapper); if (conflict != null) { - if (mapper.supersededBy().contains(conflict.name())) { + // Check if there's a "superseded_by" param covering this conflict, otherwise use lexicographical ordering. + if (mapper.supersededBy().contains(conflict.name()) + || (conflict.supersededBy().contains(mapper.name()) == false && name.compareTo(conflict.name()) > 0)) { passThroughFieldAliases.put(name, conflict); continue; } diff --git a/server/src/main/java/org/elasticsearch/index/mapper/PassThroughObjectMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/PassThroughObjectMapper.java index d4f84005f2757..50e68a5f9f46c 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/PassThroughObjectMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/PassThroughObjectMapper.java @@ -35,6 +35,10 @@ * Pass-through objects allow creating fields inside them that can also be referenced directly in search queries. * They also include parameters that affect how nested fields get configured. For instance, if parameter "time_series_dimension" * is set, eligible subfields are marked as dimensions and keyword fields are additionally included in routing and tsid calculations. + * + * In case different pass-through objects contain subfields with the same name (excluding the pass-through prefix), their aliases conflict. + * To resolve this, the pass-through spec can specify which object takes precedence by including it in the "superseded_by" parameter. + * Otherwise, precedence is based on lexicographical ordering. */ public class PassThroughObjectMapper extends ObjectMapper { public static final String CONTENT_TYPE = "passthrough"; From d867e307d76decbe556f9e39341d0d997e336eaf Mon Sep 17 00:00:00 2001 From: Kostas Krikellas Date: Thu, 4 Apr 2024 15:31:05 +0300 Subject: [PATCH 07/14] simplify loop producing aliases --- .../index/mapper/FieldTypeLookup.java | 14 +++---- .../index/mapper/FieldTypeLookupTests.java | 40 ++++++++++++++----- 2 files changed, 37 insertions(+), 17 deletions(-) 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 f3d04de037d80..5d43e96749f0f 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/FieldTypeLookup.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/FieldTypeLookup.java @@ -96,17 +96,17 @@ final class FieldTypeLookup { // PassThroughObjectMapper name. This is achieved by adding a second reference to their // MappedFieldType using the remaining suffix. Map passThroughFieldAliases = new HashMap<>(); - for (FieldMapper fieldMapper : fieldMappers) { - String fieldName = fieldMapper.name(); - for (PassThroughObjectMapper mapper : passThroughMappers) { - if (fieldName.startsWith(mapper.name())) { + for (PassThroughObjectMapper passThroughMapper : passThroughMappers) { + for (Mapper subfield : passThroughMapper.mappers.values()) { + if (subfield instanceof FieldMapper fieldMapper) { String name = fieldMapper.simpleName(); // Check for conflict between PassThroughObjectMapper subfields. - PassThroughObjectMapper conflict = passThroughFieldAliases.put(name, mapper); + PassThroughObjectMapper conflict = passThroughFieldAliases.put(name, passThroughMapper); if (conflict != null) { // Check if there's a "superseded_by" param covering this conflict, otherwise use lexicographical ordering. - if (mapper.supersededBy().contains(conflict.name()) - || (conflict.supersededBy().contains(mapper.name()) == false && name.compareTo(conflict.name()) > 0)) { + if (passThroughMapper.supersededBy().contains(conflict.name()) + || (conflict.supersededBy().contains(passThroughMapper.name()) == false + && name.compareTo(conflict.name()) > 0)) { passThroughFieldAliases.put(name, conflict); continue; } 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 d3a12b145b6f8..9334dd5e56a84 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/FieldTypeLookupTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/FieldTypeLookupTests.java @@ -426,13 +426,13 @@ private static FlattenedFieldMapper createFlattenedMapper(String fieldName) { return new FlattenedFieldMapper.Builder(fieldName).build(MapperBuilderContext.root(false, false)); } - private PassThroughObjectMapper createPassThroughMapper(String name, Set supersededBy) { + private PassThroughObjectMapper createPassThroughMapper(String name, Map mappers, Set supersededBy) { return new PassThroughObjectMapper( name, name, Explicit.EXPLICIT_TRUE, ObjectMapper.Dynamic.FALSE, - Map.of(), + mappers, Explicit.EXPLICIT_FALSE, supersededBy ); @@ -444,14 +444,22 @@ public void testAddRootAliasesForPassThroughFields() { new MockFieldMapper.FakeFieldType("attributes.much.more.deeply.nested.bar"), "much.more.deeply.nested.bar" ); - PassThroughObjectMapper attributes = createPassThroughMapper("attributes", Set.of()); + PassThroughObjectMapper attributes = createPassThroughMapper( + "attributes", + Map.of("foo", foo, "much.more.deeply.nested.bar", bar), + Set.of() + ); MockFieldMapper baz = new MockFieldMapper("resource.attributes.baz"); MockFieldMapper bag = new MockFieldMapper( new MockFieldMapper.FakeFieldType("resource.attributes.much.more.deeply.nested.bag"), "much.more.deeply.nested.bag" ); - PassThroughObjectMapper resourceAttributes = createPassThroughMapper("resource.attributes", Set.of("")); + PassThroughObjectMapper resourceAttributes = createPassThroughMapper( + "resource.attributes", + Map.of("baz", baz, "much.more.deeply.nested.bag", bag), + Set.of("") + ); FieldTypeLookup lookup = new FieldTypeLookup( List.of(foo, bar, baz, bag), @@ -467,7 +475,7 @@ public void testAddRootAliasesForPassThroughFields() { public void testNoPassThroughField() { MockFieldMapper field = new MockFieldMapper("labels.foo"); - PassThroughObjectMapper attributes = createPassThroughMapper("attributes", Set.of()); + PassThroughObjectMapper attributes = createPassThroughMapper("attributes", Map.of(), Set.of()); FieldTypeLookup lookup = new FieldTypeLookup(List.of(field), List.of(), List.of(attributes), List.of()); assertNull(lookup.get("foo")); @@ -475,10 +483,14 @@ public void testNoPassThroughField() { public void testAddRootAliasForConflictingPassThroughFields() { MockFieldMapper attributeField = new MockFieldMapper("attributes.foo"); - PassThroughObjectMapper attributes = createPassThroughMapper("attributes", Set.of()); + PassThroughObjectMapper attributes = createPassThroughMapper("attributes", Map.of("foo", attributeField), Set.of()); MockFieldMapper resourceAttributeField = new MockFieldMapper("resource.attributes.foo"); - PassThroughObjectMapper resourceAttributes = createPassThroughMapper("resource.attributes", Set.of()); + PassThroughObjectMapper resourceAttributes = createPassThroughMapper( + "resource.attributes", + Map.of("foo", resourceAttributeField), + Set.of() + ); FieldTypeLookup lookup = new FieldTypeLookup( List.of(attributeField, resourceAttributeField), @@ -491,10 +503,18 @@ public void testAddRootAliasForConflictingPassThroughFields() { public void testAddRootAliasForConflictingPassThroughFieldsSuperseded() { MockFieldMapper attributeField = new MockFieldMapper("attributes.foo"); - PassThroughObjectMapper attributes = createPassThroughMapper("attributes", Set.of("resource.attributes")); + PassThroughObjectMapper attributes = createPassThroughMapper( + "attributes", + Map.of("foo", attributeField), + Set.of("resource.attributes") + ); MockFieldMapper resourceAttributeField = new MockFieldMapper("resource.attributes.foo"); - PassThroughObjectMapper resourceAttributes = createPassThroughMapper("resource.attributes", Set.of()); + PassThroughObjectMapper resourceAttributes = createPassThroughMapper( + "resource.attributes", + Map.of("foo", resourceAttributeField), + Set.of() + ); FieldTypeLookup lookup = new FieldTypeLookup( List.of(attributeField, resourceAttributeField), @@ -508,7 +528,7 @@ public void testAddRootAliasForConflictingPassThroughFieldsSuperseded() { public void testNoRootAliasForPassThroughFieldOnConflictingField() { MockFieldMapper attributeFoo = new MockFieldMapper("attributes.foo"); MockFieldMapper foo = new MockFieldMapper("foo"); - PassThroughObjectMapper attributes = createPassThroughMapper("attributes", Set.of()); + PassThroughObjectMapper attributes = createPassThroughMapper("attributes", Map.of("foo", attributeFoo), Set.of()); FieldTypeLookup lookup = new FieldTypeLookup(List.of(foo, attributeFoo), List.of(), List.of(attributes), List.of()); assertEquals(foo.fieldType(), lookup.get("foo")); From 0f9b08d28f8712e78132259d3f31ebd30450d068 Mon Sep 17 00:00:00 2001 From: Kostas Krikellas Date: Fri, 5 Apr 2024 09:58:23 +0300 Subject: [PATCH 08/14] use priorities for passthrough deduplication --- .../index/mapper/FieldTypeLookup.java | 6 +- .../index/mapper/MappingLookup.java | 10 +- .../index/mapper/PassThroughObjectMapper.java | 130 ++++++------------ .../index/mapper/FieldTypeLookupTests.java | 40 ++---- .../mapper/PassThroughObjectMapperTests.java | 127 ++++++----------- 5 files changed, 94 insertions(+), 219 deletions(-) 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 5d43e96749f0f..698dd02a9dda4 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/FieldTypeLookup.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/FieldTypeLookup.java @@ -103,10 +103,8 @@ final class FieldTypeLookup { // Check for conflict between PassThroughObjectMapper subfields. PassThroughObjectMapper conflict = passThroughFieldAliases.put(name, passThroughMapper); if (conflict != null) { - // Check if there's a "superseded_by" param covering this conflict, otherwise use lexicographical ordering. - if (passThroughMapper.supersededBy().contains(conflict.name()) - || (conflict.supersededBy().contains(passThroughMapper.name()) == false - && name.compareTo(conflict.name()) > 0)) { + // Check the priorities of the conflicting objects. + if (conflict.priority() > passThroughMapper.priority()) { passThroughFieldAliases.put(name, conflict); continue; } 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 7949237463be4..85f51035bcd0c 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/MappingLookup.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/MappingLookup.java @@ -23,7 +23,6 @@ import java.util.Map; import java.util.Set; import java.util.function.Function; -import java.util.stream.Collectors; /** * A (mostly) immutable snapshot of the current mapping of an index with @@ -185,14 +184,7 @@ private MappingLookup( } } - List sequence = PassThroughObjectMapper.checkSupersedesForCircularDeps(passThroughMappers); - if (sequence.isEmpty() == false) { - throw new MapperParsingException( - "Circular dependency between pass-through objects: " - + sequence.stream().map(PassThroughObjectMapper::name).collect(Collectors.joining(" => ")) - ); - } - + PassThroughObjectMapper.checkForInvalidPriorities(passThroughMappers); final Collection runtimeFields = mapping.getRoot().runtimeFields(); this.fieldTypeLookup = new FieldTypeLookup(mappers, aliasMappers, passThroughMappers, runtimeFields); if (runtimeFields.isEmpty()) { diff --git a/server/src/main/java/org/elasticsearch/index/mapper/PassThroughObjectMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/PassThroughObjectMapper.java index 50e68a5f9f46c..94f9516cc2a62 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/PassThroughObjectMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/PassThroughObjectMapper.java @@ -14,20 +14,13 @@ import org.elasticsearch.xcontent.XContentBuilder; import java.io.IOException; -import java.util.Arrays; import java.util.Collection; import java.util.HashMap; -import java.util.List; import java.util.Locale; import java.util.Map; -import java.util.Set; -import java.util.Stack; -import java.util.TreeSet; -import java.util.function.Function; -import java.util.stream.Collectors; import static org.elasticsearch.common.xcontent.support.XContentMapValues.nodeBooleanValue; -import static org.elasticsearch.common.xcontent.support.XContentMapValues.nodeStringArrayValue; +import static org.elasticsearch.common.xcontent.support.XContentMapValues.nodeIntegerValue; /** * Mapper for pass-through objects. @@ -37,14 +30,12 @@ * is set, eligible subfields are marked as dimensions and keyword fields are additionally included in routing and tsid calculations. * * In case different pass-through objects contain subfields with the same name (excluding the pass-through prefix), their aliases conflict. - * To resolve this, the pass-through spec can specify which object takes precedence by including it in the "superseded_by" parameter. - * Otherwise, precedence is based on lexicographical ordering. + * To resolve this, the pass-through spec specifies which object takes precedence through required parameter "priority"; non-negative + * integer values are accepted, with the highest priority value winning in case of conflicting aliases. */ public class PassThroughObjectMapper extends ObjectMapper { public static final String CONTENT_TYPE = "passthrough"; - public static final String SUPERSEDED_BY_PARAM = "superseded_by"; - - private static final int CIRCULAR_DEPENDENCY_MAX_OPS = 10_000; + public static final String PRIORITY_PARAM_NAME = "priority"; public static class Builder extends ObjectMapper.Builder { @@ -52,7 +43,7 @@ public static class Builder extends ObjectMapper.Builder { protected Explicit timeSeriesDimensionSubFields = Explicit.IMPLICIT_FALSE; // Controls which pass-through fields take precedence in case of conflicting aliases. - protected Set supersededBy = Set.of(); + protected int priority = 0; public Builder(String name) { // Subobjects are not currently supported. @@ -82,7 +73,7 @@ public PassThroughObjectMapper build(MapperBuilderContext context) { dynamic, buildMappers(context.createChildContext(name(), timeSeriesDimensionSubFields.value(), dynamic)), timeSeriesDimensionSubFields, - supersededBy + priority ); } } @@ -90,7 +81,7 @@ public PassThroughObjectMapper build(MapperBuilderContext context) { // If set, its subfields are marked as time-series dimensions (for the types supporting this). private final Explicit timeSeriesDimensionSubFields; - private final Set supersededBy; + private final int priority; PassThroughObjectMapper( String name, @@ -99,39 +90,30 @@ public PassThroughObjectMapper build(MapperBuilderContext context) { Dynamic dynamic, Map mappers, Explicit timeSeriesDimensionSubFields, - Set supersededBy + int priority ) { // Subobjects are not currently supported. super(name, fullPath, enabled, Explicit.IMPLICIT_FALSE, dynamic, mappers); this.timeSeriesDimensionSubFields = timeSeriesDimensionSubFields; - this.supersededBy = supersededBy; - - if (supersededBy.contains(fullPath)) { - throw new MapperException( - "Mapping definition for [" + fullPath + "] contains a self-reference in param [" + SUPERSEDED_BY_PARAM + "]" - ); - } + this.priority = priority; } @Override PassThroughObjectMapper withoutMappers() { - return new PassThroughObjectMapper( - simpleName(), - fullPath(), - enabled, - dynamic, - Map.of(), - timeSeriesDimensionSubFields, - supersededBy - ); + return new PassThroughObjectMapper(simpleName(), fullPath(), enabled, dynamic, Map.of(), timeSeriesDimensionSubFields, priority); + } + + @Override + public String typeName() { + return CONTENT_TYPE; } public boolean containsDimensions() { return timeSeriesDimensionSubFields.value(); } - public Set supersededBy() { - return supersededBy; + public int priority() { + return priority; } @Override @@ -140,7 +122,7 @@ public PassThroughObjectMapper.Builder newBuilder(IndexVersion indexVersionCreat builder.enabled = enabled; builder.dynamic = dynamic; builder.timeSeriesDimensionSubFields = timeSeriesDimensionSubFields; - builder.supersededBy = supersededBy; + builder.priority = priority; return builder; } @@ -152,9 +134,6 @@ public PassThroughObjectMapper merge(ObjectMapper mergeWith, MergeReason reason, ? mergeWithObject.timeSeriesDimensionSubFields : this.timeSeriesDimensionSubFields; - Set mergedSupersededBy = new TreeSet<>(supersededBy); - mergedSupersededBy.addAll(mergeWithObject.supersededBy); - return new PassThroughObjectMapper( simpleName(), fullPath(), @@ -162,7 +141,7 @@ public PassThroughObjectMapper merge(ObjectMapper mergeWith, MergeReason reason, mergeResult.dynamic(), mergeResult.mappers(), containsDimensions, - mergedSupersededBy + Math.max(priority, mergeWithObject.priority) ); } @@ -173,8 +152,8 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws if (timeSeriesDimensionSubFields.explicit()) { builder.field(TimeSeriesParams.TIME_SERIES_DIMENSION_PARAM, timeSeriesDimensionSubFields.value()); } - if (supersededBy.isEmpty() == false) { - builder.field(SUPERSEDED_BY_PARAM, supersededBy); + if (priority >= 0) { + builder.field(PRIORITY_PARAM_NAME, priority); } if (dynamic != null) { builder.field("dynamic", dynamic.name().toLowerCase(Locale.ROOT)); @@ -204,59 +183,38 @@ protected static void parsePassthrough(String name, Map node, Pa ); node.remove(TimeSeriesParams.TIME_SERIES_DIMENSION_PARAM); } - fieldNode = node.get(SUPERSEDED_BY_PARAM); + fieldNode = node.get(PRIORITY_PARAM_NAME); if (fieldNode != null) { - builder.supersededBy = new TreeSet<>(Arrays.asList(nodeStringArrayValue(fieldNode))); - node.remove(SUPERSEDED_BY_PARAM); + builder.priority = nodeIntegerValue(fieldNode); + node.remove(PRIORITY_PARAM_NAME); } } } /** - * Checks the passed objects for circular dependencies on the superseded_by cross-dependencies. + * Checks the passed objects for duplicate or negative priorities. * @param passThroughMappers objects to check - * @return A list containing the circular dependency chain, or an empty list if no circular dependency is found */ - public static List checkSupersedesForCircularDeps(Collection passThroughMappers) { - if (passThroughMappers.size() < 2) { - return List.of(); - } - - // Perform DFS. - // The implementation below assumes that the graph is rather small and sparse. Further optimizations are - // required (e.g. use sets for lookups) if production systems end up with hundreds of tightly-coupled objects. - int i = 0; - Map lookupByName = passThroughMappers.stream() - .collect(Collectors.toMap(PassThroughObjectMapper::name, Function.identity())); - for (PassThroughObjectMapper start : passThroughMappers) { - Stack sequence = new Stack<>(); - Map reverseDeps = new HashMap<>(); - Stack pending = new Stack<>(); - pending.push(start); - while (pending.empty() == false) { - if (++i > CIRCULAR_DEPENDENCY_MAX_OPS) { // Defensive check. - break; - } - PassThroughObjectMapper current = pending.pop(); - // If we reached the end of a branch, unwind the stack till the beginning of the next branch. - while (sequence.isEmpty() == false && reverseDeps.get(current) != sequence.peek()) { - sequence.pop(); - } - // Check if the branch contains the same element twice. - if (sequence.contains(current)) { - sequence.push(current); - return sequence; - } - sequence.push(current); - for (String dep : current.supersededBy()) { - PassThroughObjectMapper mapper = lookupByName.get(dep); - if (mapper != null) { - pending.push(mapper); - reverseDeps.put(mapper, current); - } - } + public static void checkForInvalidPriorities(Collection passThroughMappers) { + Map seen = new HashMap<>(); + for (PassThroughObjectMapper mapper : passThroughMappers) { + if (mapper.priority < 0) { + throw new MapperException( + "Pass-through object [" + mapper.name() + "] has a negative value for parameter [priority=" + mapper.priority + "]" + ); + } + String conflict = seen.put(mapper.priority, mapper.name()); + if (conflict != null) { + throw new MapperException( + "Pass-through object [" + + mapper.name() + + "] has a conflicting param [priority=" + + mapper.priority + + "] with object [" + + conflict + + "]" + ); } } - return List.of(); } } 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 9334dd5e56a84..04013bf01d57c 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/FieldTypeLookupTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/FieldTypeLookupTests.java @@ -426,7 +426,7 @@ private static FlattenedFieldMapper createFlattenedMapper(String fieldName) { return new FlattenedFieldMapper.Builder(fieldName).build(MapperBuilderContext.root(false, false)); } - private PassThroughObjectMapper createPassThroughMapper(String name, Map mappers, Set supersededBy) { + private PassThroughObjectMapper createPassThroughMapper(String name, Map mappers, int priority) { return new PassThroughObjectMapper( name, name, @@ -434,7 +434,7 @@ private PassThroughObjectMapper createPassThroughMapper(String name, Map { - b.startObject("labels").field("type", "passthrough").field("superseded_by", "foo"); + b.startObject("labels").field("type", "passthrough"); { b.startObject("properties"); b.startObject("dim").field("type", "keyword").endObject(); @@ -65,12 +61,12 @@ public void testSupersededByOne() throws IOException { })); Mapper mapper = mapperService.mappingLookup().objectMappers().get("labels"); assertThat(mapper, instanceOf(PassThroughObjectMapper.class)); - assertThat(((PassThroughObjectMapper) mapper).supersededBy(), contains("foo")); + assertEquals(0, ((PassThroughObjectMapper) mapper).priority()); } - public void testSupersededByMany() throws IOException { + public void testPriorityParamSet() throws IOException { MapperService mapperService = createMapperService(mapping(b -> { - b.startObject("labels").field("type", "passthrough").field("superseded_by", "far,too,many,deps,so,many"); + b.startObject("labels").field("type", "passthrough").field("priority", "10"); { b.startObject("properties"); b.startObject("dim").field("type", "keyword").endObject(); @@ -80,24 +76,7 @@ public void testSupersededByMany() throws IOException { })); Mapper mapper = mapperService.mappingLookup().objectMappers().get("labels"); assertThat(mapper, instanceOf(PassThroughObjectMapper.class)); - assertThat(((PassThroughObjectMapper) mapper).supersededBy(), containsInAnyOrder("far", "too", "many", "deps", "so")); - } - - public void testSupersededBySelfReference() throws IOException { - MapperException exception = expectThrows(MapperException.class, () -> createMapperService(mapping(b -> { - b.startObject("labels").field("type", "passthrough").field("superseded_by", "labels"); - { - b.startObject("properties"); - b.startObject("dim").field("type", "keyword").endObject(); - b.endObject(); - } - b.endObject(); - }))); - - assertEquals( - "Failed to parse mapping: Mapping definition for [labels] contains a self-reference in param [superseded_by]", - exception.getMessage() - ); + assertEquals(10, ((PassThroughObjectMapper) mapper).priority()); } public void testDynamic() throws IOException { @@ -171,27 +150,26 @@ public void testAddSubobjectAutoFlatten() throws IOException { public void testWithoutMappers() throws IOException { MapperService mapperService = createMapperService(mapping(b -> { - b.startObject("labels").field("type", "passthrough"); + b.startObject("labels").field("type", "passthrough").field("priority", "1"); { b.startObject("properties"); b.startObject("dim").field("type", "keyword").endObject(); b.endObject(); } b.endObject(); - b.startObject("shallow").field("type", "passthrough"); + b.startObject("shallow").field("type", "passthrough").field("priority", "2"); b.endObject(); })); - var labels = mapperService.mappingLookup().objectMappers().get("labels"); - var shallow = mapperService.mappingLookup().objectMappers().get("shallow"); - assertThat(labels.withoutMappers().toString(), equalTo(shallow.toString().replace("shallow", "labels"))); + assertEquals("passthrough", mapperService.mappingLookup().objectMappers().get("labels").typeName()); + assertEquals("passthrough", mapperService.mappingLookup().objectMappers().get("shallow").typeName()); } - public void testCheckSupersedesForCircularDepsEmpty() throws IOException { - assertTrue(PassThroughObjectMapper.checkSupersedesForCircularDeps(List.of()).isEmpty()); + public void testCheckForInvalidPrioritiesEmpty() throws IOException { + PassThroughObjectMapper.checkForInvalidPriorities(List.of()); } - private PassThroughObjectMapper create(String name, Set supersededBy) { + private PassThroughObjectMapper create(String name, int priority) { return new PassThroughObjectMapper( name, name, @@ -199,71 +177,44 @@ private PassThroughObjectMapper create(String name, Set supersededBy) { ObjectMapper.Dynamic.FALSE, Map.of(), Explicit.EXPLICIT_FALSE, - supersededBy + priority ); } - public void testCheckSupersedesForCircularDepsOneElement() throws IOException { - assertTrue(PassThroughObjectMapper.checkSupersedesForCircularDeps(List.of(create("foo", Set.of("bar")))).isEmpty()); + public void testCheckForInvalidPrioritiesOneElement() throws IOException { + PassThroughObjectMapper.checkForInvalidPriorities(List.of(create("foo", 0))); + PassThroughObjectMapper.checkForInvalidPriorities(List.of(create("foo", 10))); } - public void testCheckSupersedesForCircularDepsTwoElementsNoDep() throws IOException { - assertTrue( - PassThroughObjectMapper.checkSupersedesForCircularDeps( - List.of(create("foo", Set.of("A", "B", "C")), create("bar", Set.of("D", "E"))) - ).isEmpty() + public void testCheckForInvalidPrioritiesNegativePriority() throws IOException { + MapperException e = expectThrows( + MapperException.class, + () -> PassThroughObjectMapper.checkForInvalidPriorities(List.of(create("foo", -1))) ); + assertThat(e.getMessage(), containsString("Pass-through object [foo] has a negative value for parameter [priority=-1]")); } - public void testCheckSupersedesForCircularDepsTwoElementsOneDep() throws IOException { - assertTrue( - PassThroughObjectMapper.checkSupersedesForCircularDeps( - List.of(create("foo", Set.of("A", "B", "C")), create("bar", Set.of("foo", "D", "E"))) - ).isEmpty() - ); - } - - public void testCheckSupersedesForCircularDepsTwoElementsCrossDep() throws IOException { - assertThat( - PassThroughObjectMapper.checkSupersedesForCircularDeps( - List.of(create("foo", Set.of("A", "B", "bar")), create("bar", Set.of("foo", "D", "E"))) - ).stream().map(PassThroughObjectMapper::name).collect(Collectors.toList()), - contains("foo", "bar", "foo") - ); + public void testCheckForInvalidPrioritiesManyValidElements() throws IOException { + PassThroughObjectMapper.checkForInvalidPriorities(List.of(create("foo", 1), create("bar", 2), create("baz", 3), create("bar", 4))); } - public void testCheckSupersedesForCircularDepsManyElementsWithCircle() throws IOException { - assertThat( - PassThroughObjectMapper.checkSupersedesForCircularDeps( - List.of( - create("A", Set.of("B", "D")), - create("B", Set.of("E", "G")), - create("C", Set.of()), - create("D", Set.of("C")), - create("E", Set.of("C")), - create("F", Set.of("A", "B")), - create("G", Set.of("C", "H")), - create("H", Set.of("A", "C")) - ) - ).stream().map(PassThroughObjectMapper::name).collect(Collectors.toList()), - contains("A", "B", "G", "H", "A") + public void testCheckForInvalidPrioritiesManyElementsInvalidPriority() throws IOException { + MapperException e = expectThrows( + MapperException.class, + () -> PassThroughObjectMapper.checkForInvalidPriorities( + List.of(create("foo", 1), create("bar", 2), create("baz", 3), create("bar", -4)) + ) ); + assertThat(e.getMessage(), containsString("Pass-through object [bar] has a negative value for parameter [priority=-4]")); } - public void testCheckSupersedesForCircularDepsManyElementsNoCircle() throws IOException { - assertTrue( - PassThroughObjectMapper.checkSupersedesForCircularDeps( - List.of( - create("A", Set.of("B", "D")), - create("B", Set.of("E", "G")), - create("C", Set.of()), - create("D", Set.of("C")), - create("E", Set.of("C")), - create("F", Set.of("A", "B")), - create("G", Set.of("C", "H")), - create("H", Set.of("C", "E")) - ) - ).isEmpty() + public void testCheckForInvalidPrioritiesManyElementsDuplicatePriority() throws IOException { + MapperException e = expectThrows( + MapperException.class, + () -> PassThroughObjectMapper.checkForInvalidPriorities( + List.of(create("foo", 1), create("bar", 1), create("baz", 3), create("bar", 4)) + ) ); + assertThat(e.getMessage(), containsString("Pass-through object [bar] has a conflicting param [priority=1] with object [foo]")); } } From 7f2dc2a3009ec6683579b0d7aa8cf0ac6f7fd32e Mon Sep 17 00:00:00 2001 From: Kostas Krikellas Date: Fri, 5 Apr 2024 16:06:19 +0300 Subject: [PATCH 09/14] make priority param required --- .../TSDBPassthroughIndexingIT.java | 1 + .../DataStreamIndexSettingsProviderTests.java | 6 +- .../test/data_stream/150_tsdb.yml | 44 ++++++------ .../index/mapper/MappingLookup.java | 2 +- .../index/mapper/PassThroughObjectMapper.java | 17 +++-- .../mapper/DynamicFieldsBuilderTests.java | 2 +- .../mapper/PassThroughObjectMapperTests.java | 69 +++++++++---------- 7 files changed, 73 insertions(+), 68 deletions(-) diff --git a/modules/data-streams/src/internalClusterTest/java/org/elasticsearch/datastreams/TSDBPassthroughIndexingIT.java b/modules/data-streams/src/internalClusterTest/java/org/elasticsearch/datastreams/TSDBPassthroughIndexingIT.java index a8b6834b11801..b8d7d18dec475 100644 --- a/modules/data-streams/src/internalClusterTest/java/org/elasticsearch/datastreams/TSDBPassthroughIndexingIT.java +++ b/modules/data-streams/src/internalClusterTest/java/org/elasticsearch/datastreams/TSDBPassthroughIndexingIT.java @@ -72,6 +72,7 @@ public class TSDBPassthroughIndexingIT extends ESSingleNodeTestCase { }, "attributes": { "type": "passthrough", + "priority": 0, "dynamic": true, "time_series_dimension": true }, diff --git a/modules/data-streams/src/test/java/org/elasticsearch/datastreams/DataStreamIndexSettingsProviderTests.java b/modules/data-streams/src/test/java/org/elasticsearch/datastreams/DataStreamIndexSettingsProviderTests.java index 01ad1bb09b20f..c96469ea4ac37 100644 --- a/modules/data-streams/src/test/java/org/elasticsearch/datastreams/DataStreamIndexSettingsProviderTests.java +++ b/modules/data-streams/src/test/java/org/elasticsearch/datastreams/DataStreamIndexSettingsProviderTests.java @@ -647,10 +647,12 @@ public void testGenerateRoutingPathFromPassThroughObject() throws Exception { "properties": { "labels": { "type": "passthrough", - "time_series_dimension": true + "time_series_dimension": true, + "priority": 2 }, "metrics": { - "type": "passthrough" + "type": "passthrough", + "priority": 1 }, "another_field": { "type": "keyword" diff --git a/modules/data-streams/src/yamlRestTest/resources/rest-api-spec/test/data_stream/150_tsdb.yml b/modules/data-streams/src/yamlRestTest/resources/rest-api-spec/test/data_stream/150_tsdb.yml index f65a458755c22..84108cca514b1 100644 --- a/modules/data-streams/src/yamlRestTest/resources/rest-api-spec/test/data_stream/150_tsdb.yml +++ b/modules/data-streams/src/yamlRestTest/resources/rest-api-spec/test/data_stream/150_tsdb.yml @@ -195,8 +195,8 @@ index without timestamp with pipeline: --- dynamic templates: - skip: - version: " - 8.12.99" - reason: "Support for dynamic fields was added in 8.13" + version: " - 8.13.99" + reason: "Support for priority in passthrough objects was added in 8.14" - do: allowed_warnings: - "index template [my-dynamic-template] has index patterns [k9s*] matching patterns from existing older templates [global] with patterns (global => [*]); this template [my-dynamic-template] will take precedence during new index creation" @@ -219,6 +219,7 @@ dynamic templates: type: passthrough dynamic: true time_series_dimension: true + priority: 0 dynamic_templates: - counter_metric: mapping: @@ -326,8 +327,8 @@ dynamic templates: --- dynamic templates - conflicting aliases: - skip: - version: " - 8.12.99" - reason: "Support for dynamic fields was added in 8.13" + version: " - 8.13.99" + reason: "Support for priority in passthrough objects was added in 8.14" - do: allowed_warnings: - "index template [my-dynamic-template] has index patterns [k9s*] matching patterns from existing older templates [global] with patterns (global => [*]); this template [my-dynamic-template] will take precedence during new index creation" @@ -350,11 +351,12 @@ dynamic templates - conflicting aliases: type: passthrough dynamic: true time_series_dimension: true + priority: 2 resource_attributes: type: passthrough dynamic: true time_series_dimension: true - superseded_by: attributes + priority: 1 dynamic_templates: - counter_metric: mapping: @@ -424,8 +426,8 @@ dynamic templates - conflicting aliases: --- dynamic templates with nesting: - skip: - version: " - 8.12.99" - reason: "Support for dynamic fields was added in 8.13" + version: " - 8.13.99" + reason: "Support for priority in passthrough objects was added in 8.14" - do: allowed_warnings: - "index template [my-dynamic-template] has index patterns [k9s*] matching patterns from existing older templates [global] with patterns (global => [*]); this template [my-dynamic-template] will take precedence during new index creation" @@ -448,6 +450,7 @@ dynamic templates with nesting: type: passthrough dynamic: true time_series_dimension: true + priority: 2 resource: type: object properties: @@ -455,6 +458,7 @@ dynamic templates with nesting: type: passthrough dynamic: true time_series_dimension: true + priority: 1 dynamic_templates: - counter_metric: mapping: @@ -581,8 +585,8 @@ dynamic templates with nesting: --- dynamic templates with incremental indexing: - skip: - version: " - 8.12.99" - reason: "Support for dynamic fields was added in 8.13" + version: " - 8.13.99" + reason: "Support for priority in passthrough objects was added in 8.14" - do: allowed_warnings: - "index template [my-dynamic-template] has index patterns [k9s*] matching patterns from existing older templates [global] with patterns (global => [*]); this template [my-dynamic-template] will take precedence during new index creation" @@ -605,6 +609,7 @@ dynamic templates with incremental indexing: type: passthrough dynamic: true time_series_dimension: true + priority: 2 resource: type: object properties: @@ -612,6 +617,7 @@ dynamic templates with incremental indexing: type: passthrough dynamic: true time_series_dimension: true + priority: 1 dynamic_templates: - counter_metric: mapping: @@ -775,8 +781,8 @@ dynamic templates with incremental indexing: --- subobject in passthrough object auto flatten: - skip: - version: " - 8.12.99" - reason: "Support for passthrough fields was added in 8.13" + version: " - 8.13.99" + reason: "Support for priority in passthrough objects was added in 8.14" - do: allowed_warnings: - "index template [my-passthrough-template] has index patterns [k9s*] matching patterns from existing older templates [global] with patterns (global => [*]); this template [my-passthrough-template] will take precedence during new index creation" @@ -795,6 +801,7 @@ subobject in passthrough object auto flatten: attributes: type: passthrough time_series_dimension: true + priority: 0 properties: subcategory: type: object @@ -842,12 +849,12 @@ enable subobjects in passthrough object: subobjects: true --- -passthrough objects with circular deps: +passthrough objects with duplicate priority: - skip: - version: " - 8.12.99" - reason: "Support for passthrough fields was added in 8.13" + version: " - 8.13.99" + reason: "Support for priority in passthrough objects was added in 8.14" - do: - catch: /Circular dependency between pass-through objects/ + catch: /has a conflicting param/ indices.put_index_template: name: my-dynamic-template body: @@ -863,10 +870,7 @@ passthrough objects with circular deps: properties: attributes: type: passthrough - superseded_by: system.attributes + priority: 1 resource.attributes: type: passthrough - superseded_by: attributes - system.attributes: - type: passthrough - superseded_by: resource.attributes + priority: 1 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 85f51035bcd0c..f1770f9bd040a 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/MappingLookup.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/MappingLookup.java @@ -184,7 +184,7 @@ private MappingLookup( } } - PassThroughObjectMapper.checkForInvalidPriorities(passThroughMappers); + PassThroughObjectMapper.checkForDuplicatePriorities(passThroughMappers); final Collection runtimeFields = mapping.getRoot().runtimeFields(); this.fieldTypeLookup = new FieldTypeLookup(mappers, aliasMappers, passThroughMappers, runtimeFields); if (runtimeFields.isEmpty()) { diff --git a/server/src/main/java/org/elasticsearch/index/mapper/PassThroughObjectMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/PassThroughObjectMapper.java index 94f9516cc2a62..0cd8d816dfb0f 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/PassThroughObjectMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/PassThroughObjectMapper.java @@ -43,7 +43,7 @@ public static class Builder extends ObjectMapper.Builder { protected Explicit timeSeriesDimensionSubFields = Explicit.IMPLICIT_FALSE; // Controls which pass-through fields take precedence in case of conflicting aliases. - protected int priority = 0; + protected int priority = -1; public Builder(String name) { // Subobjects are not currently supported. @@ -64,6 +64,11 @@ public PassThroughObjectMapper.Builder setContainsDimensions() { return this; } + public PassThroughObjectMapper.Builder setPriority(int priority) { + this.priority = priority; + return this; + } + @Override public PassThroughObjectMapper build(MapperBuilderContext context) { return new PassThroughObjectMapper( @@ -96,6 +101,9 @@ public PassThroughObjectMapper build(MapperBuilderContext context) { super(name, fullPath, enabled, Explicit.IMPLICIT_FALSE, dynamic, mappers); this.timeSeriesDimensionSubFields = timeSeriesDimensionSubFields; this.priority = priority; + if (priority < 0) { + throw new MapperException("Pass-through object [" + fullPath + "] is missing a non-negative value for parameter [priority]"); + } } @Override @@ -195,14 +203,9 @@ protected static void parsePassthrough(String name, Map node, Pa * Checks the passed objects for duplicate or negative priorities. * @param passThroughMappers objects to check */ - public static void checkForInvalidPriorities(Collection passThroughMappers) { + public static void checkForDuplicatePriorities(Collection passThroughMappers) { Map seen = new HashMap<>(); for (PassThroughObjectMapper mapper : passThroughMappers) { - if (mapper.priority < 0) { - throw new MapperException( - "Pass-through object [" + mapper.name() + "] has a negative value for parameter [priority=" + mapper.priority + "]" - ); - } String conflict = seen.put(mapper.priority, mapper.name()); if (conflict != null) { throw new MapperException( diff --git a/server/src/test/java/org/elasticsearch/index/mapper/DynamicFieldsBuilderTests.java b/server/src/test/java/org/elasticsearch/index/mapper/DynamicFieldsBuilderTests.java index 329d8a795732f..3714e603a5778 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/DynamicFieldsBuilderTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/DynamicFieldsBuilderTests.java @@ -69,7 +69,7 @@ public void testCreateDynamicStringFieldAsKeywordForDimension() throws IOExcepti SourceFieldMapper sourceMapper = new SourceFieldMapper.Builder(null).setSynthetic().build(); RootObjectMapper root = new RootObjectMapper.Builder("_doc", Explicit.IMPLICIT_TRUE).add( - new PassThroughObjectMapper.Builder("labels").setContainsDimensions().dynamic(ObjectMapper.Dynamic.TRUE) + new PassThroughObjectMapper.Builder("labels").setPriority(0).setContainsDimensions().dynamic(ObjectMapper.Dynamic.TRUE) ).build(MapperBuilderContext.root(false, false)); Mapping mapping = new Mapping(root, new MetadataFieldMapper[] { sourceMapper }, Map.of()); diff --git a/server/src/test/java/org/elasticsearch/index/mapper/PassThroughObjectMapperTests.java b/server/src/test/java/org/elasticsearch/index/mapper/PassThroughObjectMapperTests.java index bf63d22c81b5a..32899375dfaf0 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/PassThroughObjectMapperTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/PassThroughObjectMapperTests.java @@ -21,7 +21,7 @@ public class PassThroughObjectMapperTests extends MapperServiceTestCase { public void testSimpleKeyword() throws IOException { MapperService mapperService = createMapperService(mapping(b -> { - b.startObject("labels").field("type", "passthrough"); + b.startObject("labels").field("type", "passthrough").field("priority", "0"); { b.startObject("properties"); b.startObject("dim").field("type", "keyword").endObject(); @@ -36,7 +36,7 @@ public void testSimpleKeyword() throws IOException { public void testKeywordDimension() throws IOException { MapperService mapperService = createMapperService(mapping(b -> { - b.startObject("labels").field("type", "passthrough").field("time_series_dimension", "true"); + b.startObject("labels").field("type", "passthrough").field("priority", "0").field("time_series_dimension", "true"); { b.startObject("properties"); b.startObject("dim").field("type", "keyword").endObject(); @@ -49,8 +49,8 @@ public void testKeywordDimension() throws IOException { assertTrue(((KeywordFieldMapper) mapper).fieldType().isDimension()); } - public void testDefaultPriority() throws IOException { - MapperService mapperService = createMapperService(mapping(b -> { + public void testMissingPriority() throws IOException { + MapperException e = expectThrows(MapperException.class, () -> createMapperService(mapping(b -> { b.startObject("labels").field("type", "passthrough"); { b.startObject("properties"); @@ -58,10 +58,21 @@ public void testDefaultPriority() throws IOException { b.endObject(); } b.endObject(); - })); - Mapper mapper = mapperService.mappingLookup().objectMappers().get("labels"); - assertThat(mapper, instanceOf(PassThroughObjectMapper.class)); - assertEquals(0, ((PassThroughObjectMapper) mapper).priority()); + }))); + assertThat(e.getMessage(), containsString("Pass-through object [labels] is missing a non-negative value for parameter [priority]")); + } + + public void testNegativePriority() throws IOException { + MapperException e = expectThrows(MapperException.class, () -> createMapperService(mapping(b -> { + b.startObject("labels").field("type", "passthrough").field("priority", "-1"); + { + b.startObject("properties"); + b.startObject("dim").field("type", "keyword").endObject(); + b.endObject(); + } + b.endObject(); + }))); + assertThat(e.getMessage(), containsString("Pass-through object [labels] is missing a non-negative value for parameter [priority]")); } public void testPriorityParamSet() throws IOException { @@ -81,7 +92,7 @@ public void testPriorityParamSet() throws IOException { public void testDynamic() throws IOException { MapperService mapperService = createMapperService(mapping(b -> { - b.startObject("labels").field("type", "passthrough").field("dynamic", "true"); + b.startObject("labels").field("type", "passthrough").field("priority", "0").field("dynamic", "true"); { b.startObject("properties"); b.startObject("dim").field("type", "keyword").endObject(); @@ -95,7 +106,7 @@ public void testDynamic() throws IOException { public void testEnabled() throws IOException { MapperService mapperService = createMapperService(mapping(b -> { - b.startObject("labels").field("type", "passthrough").field("enabled", "false"); + b.startObject("labels").field("type", "passthrough").field("priority", "0").field("enabled", "false"); { b.startObject("properties"); b.startObject("dim").field("type", "keyword").endObject(); @@ -126,7 +137,7 @@ public void testSubobjectsThrows() throws IOException { public void testAddSubobjectAutoFlatten() throws IOException { MapperService mapperService = createMapperService(mapping(b -> { - b.startObject("labels").field("type", "passthrough").field("time_series_dimension", "true"); + b.startObject("labels").field("type", "passthrough").field("priority", "0").field("time_series_dimension", "true"); { b.startObject("properties"); { @@ -165,8 +176,8 @@ public void testWithoutMappers() throws IOException { assertEquals("passthrough", mapperService.mappingLookup().objectMappers().get("shallow").typeName()); } - public void testCheckForInvalidPrioritiesEmpty() throws IOException { - PassThroughObjectMapper.checkForInvalidPriorities(List.of()); + public void testCheckForDuplicatePrioritiesEmpty() throws IOException { + PassThroughObjectMapper.checkForDuplicatePriorities(List.of()); } private PassThroughObjectMapper create(String name, int priority) { @@ -181,37 +192,21 @@ private PassThroughObjectMapper create(String name, int priority) { ); } - public void testCheckForInvalidPrioritiesOneElement() throws IOException { - PassThroughObjectMapper.checkForInvalidPriorities(List.of(create("foo", 0))); - PassThroughObjectMapper.checkForInvalidPriorities(List.of(create("foo", 10))); - } - - public void testCheckForInvalidPrioritiesNegativePriority() throws IOException { - MapperException e = expectThrows( - MapperException.class, - () -> PassThroughObjectMapper.checkForInvalidPriorities(List.of(create("foo", -1))) - ); - assertThat(e.getMessage(), containsString("Pass-through object [foo] has a negative value for parameter [priority=-1]")); - } - - public void testCheckForInvalidPrioritiesManyValidElements() throws IOException { - PassThroughObjectMapper.checkForInvalidPriorities(List.of(create("foo", 1), create("bar", 2), create("baz", 3), create("bar", 4))); + public void testCheckForDuplicatePrioritiesOneElement() throws IOException { + PassThroughObjectMapper.checkForDuplicatePriorities(List.of(create("foo", 0))); + PassThroughObjectMapper.checkForDuplicatePriorities(List.of(create("foo", 10))); } - public void testCheckForInvalidPrioritiesManyElementsInvalidPriority() throws IOException { - MapperException e = expectThrows( - MapperException.class, - () -> PassThroughObjectMapper.checkForInvalidPriorities( - List.of(create("foo", 1), create("bar", 2), create("baz", 3), create("bar", -4)) - ) + public void testCheckForDuplicatePrioritiesManyValidElements() throws IOException { + PassThroughObjectMapper.checkForDuplicatePriorities( + List.of(create("foo", 1), create("bar", 2), create("baz", 3), create("bar", 4)) ); - assertThat(e.getMessage(), containsString("Pass-through object [bar] has a negative value for parameter [priority=-4]")); } - public void testCheckForInvalidPrioritiesManyElementsDuplicatePriority() throws IOException { + public void testCheckForDuplicatePrioritiesManyElementsDuplicatePriority() throws IOException { MapperException e = expectThrows( MapperException.class, - () -> PassThroughObjectMapper.checkForInvalidPriorities( + () -> PassThroughObjectMapper.checkForDuplicatePriorities( List.of(create("foo", 1), create("bar", 1), create("baz", 3), create("bar", 4)) ) ); From 45aa214a3345973ebfbab0bce71b4de8ae2e313f Mon Sep 17 00:00:00 2001 From: Kostas Krikellas Date: Mon, 8 Apr 2024 15:12:26 +0300 Subject: [PATCH 10/14] add passthrough disclaimer --- .../elasticsearch/index/mapper/PassThroughObjectMapper.java | 3 +++ 1 file changed, 3 insertions(+) diff --git a/server/src/main/java/org/elasticsearch/index/mapper/PassThroughObjectMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/PassThroughObjectMapper.java index 0cd8d816dfb0f..884d008eae1eb 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/PassThroughObjectMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/PassThroughObjectMapper.java @@ -32,6 +32,9 @@ * In case different pass-through objects contain subfields with the same name (excluding the pass-through prefix), their aliases conflict. * To resolve this, the pass-through spec specifies which object takes precedence through required parameter "priority"; non-negative * integer values are accepted, with the highest priority value winning in case of conflicting aliases. + * + * Note that this is an experimental, undocumented mapper type, currently intended for prototyping purposes only. + * It has not been vetted for use in production systems. */ public class PassThroughObjectMapper extends ObjectMapper { public static final String CONTENT_TYPE = "passthrough"; From 3cd0f1c140bd94d42bb64295ce6b1312cbfbfb72 Mon Sep 17 00:00:00 2001 From: Kostas Krikellas Date: Mon, 8 Apr 2024 15:20:24 +0300 Subject: [PATCH 11/14] small refactor --- .../elasticsearch/index/mapper/FieldTypeLookup.java | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) 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 698dd02a9dda4..a242d15d10a65 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/FieldTypeLookup.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/FieldTypeLookup.java @@ -102,12 +102,10 @@ final class FieldTypeLookup { String name = fieldMapper.simpleName(); // Check for conflict between PassThroughObjectMapper subfields. PassThroughObjectMapper conflict = passThroughFieldAliases.put(name, passThroughMapper); - if (conflict != null) { - // Check the priorities of the conflicting objects. - if (conflict.priority() > passThroughMapper.priority()) { - passThroughFieldAliases.put(name, conflict); - continue; - } + if (conflict != null && conflict.priority() > passThroughMapper.priority()) { + // Keep the conflicting field if it has higher priority. + passThroughFieldAliases.put(name, conflict); + continue; } else if (fullNameToFieldType.containsKey(name)) { // There's an existing field or alias for the same field. continue; From ba3aeccd995724c0d8d7c080d50cdca9fe1f095b Mon Sep 17 00:00:00 2001 From: Kostas Krikellas Date: Mon, 8 Apr 2024 15:34:27 +0300 Subject: [PATCH 12/14] small refactor --- .../index/mapper/FieldTypeLookup.java | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) 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 a242d15d10a65..6fd78d1c24746 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/FieldTypeLookup.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/FieldTypeLookup.java @@ -105,15 +105,12 @@ final class FieldTypeLookup { if (conflict != null && conflict.priority() > passThroughMapper.priority()) { // Keep the conflicting field if it has higher priority. passThroughFieldAliases.put(name, conflict); - continue; - } else if (fullNameToFieldType.containsKey(name)) { - // There's an existing field or alias for the same field. - continue; - } - MappedFieldType fieldType = fieldMapper.fieldType(); - fullNameToFieldType.put(name, fieldType); - if (fieldType instanceof DynamicFieldType) { - dynamicFieldTypes.put(name, (DynamicFieldType) fieldType); + } else if (fullNameToFieldType.containsKey(name) == false) { // No existing field or alias with the same name. + MappedFieldType fieldType = fieldMapper.fieldType(); + fullNameToFieldType.put(name, fieldType); + if (fieldType instanceof DynamicFieldType) { + dynamicFieldTypes.put(name, (DynamicFieldType) fieldType); + } } } } From a72f16dd833a56ab8a58fefdaea8ab4d9ba3a473 Mon Sep 17 00:00:00 2001 From: Kostas Krikellas Date: Mon, 8 Apr 2024 16:12:45 +0300 Subject: [PATCH 13/14] revert refactor --- .../index/mapper/FieldTypeLookup.java | 21 ++++++++++++------- 1 file changed, 13 insertions(+), 8 deletions(-) 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 6fd78d1c24746..7070c387fbb97 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/FieldTypeLookup.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/FieldTypeLookup.java @@ -102,15 +102,20 @@ final class FieldTypeLookup { String name = fieldMapper.simpleName(); // Check for conflict between PassThroughObjectMapper subfields. PassThroughObjectMapper conflict = passThroughFieldAliases.put(name, passThroughMapper); - if (conflict != null && conflict.priority() > passThroughMapper.priority()) { - // Keep the conflicting field if it has higher priority. - passThroughFieldAliases.put(name, conflict); - } else if (fullNameToFieldType.containsKey(name) == false) { // No existing field or alias with the same name. - MappedFieldType fieldType = fieldMapper.fieldType(); - fullNameToFieldType.put(name, fieldType); - if (fieldType instanceof DynamicFieldType) { - dynamicFieldTypes.put(name, (DynamicFieldType) fieldType); + if (conflict != null) { + if (conflict.priority() > passThroughMapper.priority()) { + // Keep the conflicting field if it has higher priority. + passThroughFieldAliases.put(name, conflict); + continue; } + } else if (fullNameToFieldType.containsKey(name)) { + // There's an existing field or alias for the same field. + continue; + } + MappedFieldType fieldType = fieldMapper.fieldType(); + fullNameToFieldType.put(name, fieldType); + if (fieldType instanceof DynamicFieldType) { + dynamicFieldTypes.put(name, (DynamicFieldType) fieldType); } } } From 20ed13d87b1a31dd23d26145b5ba417258ca8323 Mon Sep 17 00:00:00 2001 From: Kostas Krikellas Date: Mon, 29 Apr 2024 14:43:52 +0300 Subject: [PATCH 14/14] node feature for pass-through priotity --- .../test/data_stream/150_tsdb.yml | 24 +++++++++---------- .../index/mapper/MapperFeatures.java | 2 +- .../index/mapper/PassThroughObjectMapper.java | 3 +++ 3 files changed, 16 insertions(+), 13 deletions(-) diff --git a/modules/data-streams/src/yamlRestTest/resources/rest-api-spec/test/data_stream/150_tsdb.yml b/modules/data-streams/src/yamlRestTest/resources/rest-api-spec/test/data_stream/150_tsdb.yml index b36f909c7aec9..a1ded40ce1852 100644 --- a/modules/data-streams/src/yamlRestTest/resources/rest-api-spec/test/data_stream/150_tsdb.yml +++ b/modules/data-streams/src/yamlRestTest/resources/rest-api-spec/test/data_stream/150_tsdb.yml @@ -195,8 +195,8 @@ index without timestamp with pipeline: --- dynamic templates: - requires: - cluster_features: ["gte_v8.13.0"] - reason: "Support for priority in passthrough objects was added in 8.14" + cluster_features: ["mapper.pass_through_priority"] + reason: support for priority in passthrough objects - do: allowed_warnings: - "index template [my-dynamic-template] has index patterns [k9s*] matching patterns from existing older templates [global] with patterns (global => [*]); this template [my-dynamic-template] will take precedence during new index creation" @@ -327,8 +327,8 @@ dynamic templates: --- dynamic templates - conflicting aliases: - requires: - cluster_features: ["gte_v8.13.0"] - reason: "Support for priority in passthrough objects was added in 8.14" + cluster_features: ["mapper.pass_through_priority"] + reason: support for priority in passthrough objects - do: allowed_warnings: - "index template [my-dynamic-template] has index patterns [k9s*] matching patterns from existing older templates [global] with patterns (global => [*]); this template [my-dynamic-template] will take precedence during new index creation" @@ -426,8 +426,8 @@ dynamic templates - conflicting aliases: --- dynamic templates with nesting: - requires: - cluster_features: ["gte_v8.13.0"] - reason: "Support for priority in passthrough objects was added in 8.14" + cluster_features: ["mapper.pass_through_priority"] + reason: support for priority in passthrough objects - do: allowed_warnings: - "index template [my-dynamic-template] has index patterns [k9s*] matching patterns from existing older templates [global] with patterns (global => [*]); this template [my-dynamic-template] will take precedence during new index creation" @@ -585,8 +585,8 @@ dynamic templates with nesting: --- dynamic templates with incremental indexing: - requires: - cluster_features: ["gte_v8.13.0"] - reason: "Support for priority in passthrough objects was added in 8.14" + cluster_features: ["mapper.pass_through_priority"] + reason: support for priority in passthrough objects - do: allowed_warnings: @@ -782,8 +782,8 @@ dynamic templates with incremental indexing: --- subobject in passthrough object auto flatten: - requires: - cluster_features: ["gte_v8.13.0"] - reason: "Support for priority in passthrough objects was added in 8.14" + cluster_features: ["mapper.pass_through_priority"] + reason: support for priority in passthrough objects - do: allowed_warnings: - "index template [my-passthrough-template] has index patterns [k9s*] matching patterns from existing older templates [global] with patterns (global => [*]); this template [my-passthrough-template] will take precedence during new index creation" @@ -852,8 +852,8 @@ enable subobjects in passthrough object: --- passthrough objects with duplicate priority: - requires: - cluster_features: ["gte_v8.13.0"] - reason: "Support for priority in passthrough objects was added in 8.14" + cluster_features: ["mapper.pass_through_priority"] + reason: support for priority in passthrough objects - do: catch: /has a conflicting param/ indices.put_index_template: diff --git a/server/src/main/java/org/elasticsearch/index/mapper/MapperFeatures.java b/server/src/main/java/org/elasticsearch/index/mapper/MapperFeatures.java index dc189aecab01c..e0407db814f2d 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/MapperFeatures.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/MapperFeatures.java @@ -19,6 +19,6 @@ public class MapperFeatures implements FeatureSpecification { @Override public Set getFeatures() { - return Set.of(IgnoredSourceFieldMapper.TRACK_IGNORED_SOURCE); + return Set.of(IgnoredSourceFieldMapper.TRACK_IGNORED_SOURCE, PassThroughObjectMapper.PASS_THROUGH_PRIORITY); } } diff --git a/server/src/main/java/org/elasticsearch/index/mapper/PassThroughObjectMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/PassThroughObjectMapper.java index 6841b2ba8e003..77333f6aedb88 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/PassThroughObjectMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/PassThroughObjectMapper.java @@ -9,6 +9,7 @@ package org.elasticsearch.index.mapper; import org.elasticsearch.common.Explicit; +import org.elasticsearch.features.NodeFeature; import org.elasticsearch.index.IndexVersion; import org.elasticsearch.xcontent.XContentBuilder; @@ -39,6 +40,8 @@ public class PassThroughObjectMapper extends ObjectMapper { public static final String CONTENT_TYPE = "passthrough"; public static final String PRIORITY_PARAM_NAME = "priority"; + static final NodeFeature PASS_THROUGH_PRIORITY = new NodeFeature("mapper.pass_through_priority"); + public static class Builder extends ObjectMapper.Builder { // Controls whether subfields are configured as time-series dimensions.