From be386a2a1668a9fe2884a11f7fc7c5a77e166bcb Mon Sep 17 00:00:00 2001 From: Kostas Krikellas Date: Thu, 8 Feb 2024 19:12:11 +0200 Subject: [PATCH 1/5] Ignore duplicate FieldAliasMappers during building --- .../index/mapper/FieldAliasMapper.java | 17 +++++++++++++++++ .../index/mapper/MappingLookup.java | 3 ++- .../index/mapper/RootObjectMapper.java | 15 ++++++--------- .../index/mapper/FieldAliasMapperTests.java | 10 ++++++++++ 4 files changed, 35 insertions(+), 10 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/index/mapper/FieldAliasMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/FieldAliasMapper.java index 97d1b9368a6c9..83e7540f7ae58 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/FieldAliasMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/FieldAliasMapper.java @@ -64,6 +64,23 @@ public Mapper merge(Mapper mergeWith, MapperMergeContext mapperMergeContext) { return mergeWith; } + @Override + public boolean equals(Object obj) { + if (obj == this) { + return true; + } + if (obj instanceof FieldAliasMapper == false) { + return false; + } + FieldAliasMapper mapper = (FieldAliasMapper) obj; + return simpleName().equals(mapper.simpleName()) && name.equals(mapper.name) && path.equals(mapper.path); + } + + @Override + public int hashCode() { + return Objects.hash(name, path); + } + @Override public Iterator iterator() { return Collections.emptyIterator(); 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 0ae13241b7f56..9afe69a36d0be 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/MappingLookup.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/MappingLookup.java @@ -167,7 +167,8 @@ private MappingLookup( if (objects.containsKey(aliasMapper.name())) { throw new MapperParsingException("Alias [" + aliasMapper.name() + "] is defined both as an object and an alias"); } - if (fieldMappers.put(aliasMapper.name(), aliasMapper) != null) { + Mapper existing = fieldMappers.put(aliasMapper.name(), aliasMapper); + if (existing != null && existing.equals(aliasMapper) == false) { throw new MapperParsingException("Alias [" + aliasMapper.name() + "] is defined both as an alias and a concrete field"); } } 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 2fe8c49df2175..d75383503fe9b 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/RootObjectMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/RootObjectMapper.java @@ -145,15 +145,12 @@ void getAliasMappers(Map mappers, Map aliasMappe // 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." - ); - } + if (conflict != null && conflict.equals(fieldMapper) == false) { + logger.warn( + "Root alias for field " + + fieldMapper.name() + + " conflicts with existing field or alias, skipping alias creation." + ); } else { FieldAliasMapper aliasMapper = new FieldAliasMapper.Builder(fieldMapper.simpleName()).path( fieldMapper.mappedFieldType.name() diff --git a/server/src/test/java/org/elasticsearch/index/mapper/FieldAliasMapperTests.java b/server/src/test/java/org/elasticsearch/index/mapper/FieldAliasMapperTests.java index c20091a308ed8..c89c1af74fec4 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/FieldAliasMapperTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/FieldAliasMapperTests.java @@ -115,4 +115,14 @@ public void testMergeFailure() throws IOException { exception.getMessage() ); } + + public void testEquals() { + FieldAliasMapper mapper = new FieldAliasMapper("simple", "name", "path"); + assertEquals(mapper, mapper); + assertNotEquals(mapper, null); + assertEquals(mapper, new FieldAliasMapper("simple", "name", "path")); + assertNotEquals(mapper, new FieldAliasMapper("simple2", "name", "path")); + assertNotEquals(mapper, new FieldAliasMapper("simple", "name2", "path")); + assertNotEquals(mapper, new FieldAliasMapper("simple", "name", "path2")); + } } From 968dc4479990b23404adb513f9b7f42558757167 Mon Sep 17 00:00:00 2001 From: Kostas Krikellas <131142368+kkrik-es@users.noreply.github.com> Date: Thu, 8 Feb 2024 19:19:52 +0200 Subject: [PATCH 2/5] Update docs/changelog/105298.yaml --- docs/changelog/105298.yaml | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 docs/changelog/105298.yaml diff --git a/docs/changelog/105298.yaml b/docs/changelog/105298.yaml new file mode 100644 index 0000000000000..e0c67a2373f8a --- /dev/null +++ b/docs/changelog/105298.yaml @@ -0,0 +1,5 @@ +pr: 105298 +summary: Ignore duplicate `FieldAliasMappers` +area: TSDB +type: bug +issues: [] From 91a0b737e9f3281f4e322b7844b5adaf9095d96e Mon Sep 17 00:00:00 2001 From: Kostas Krikellas Date: Thu, 8 Feb 2024 19:45:23 +0200 Subject: [PATCH 3/5] revert --- .../index/mapper/FieldAliasMapper.java | 17 ----------------- .../index/mapper/MappingLookup.java | 3 +-- .../index/mapper/ObjectMapper.java | 2 +- .../index/mapper/RootObjectMapper.java | 15 +++++++++------ .../index/mapper/FieldAliasMapperTests.java | 10 ---------- 5 files changed, 11 insertions(+), 36 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/index/mapper/FieldAliasMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/FieldAliasMapper.java index 83e7540f7ae58..97d1b9368a6c9 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/FieldAliasMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/FieldAliasMapper.java @@ -64,23 +64,6 @@ public Mapper merge(Mapper mergeWith, MapperMergeContext mapperMergeContext) { return mergeWith; } - @Override - public boolean equals(Object obj) { - if (obj == this) { - return true; - } - if (obj instanceof FieldAliasMapper == false) { - return false; - } - FieldAliasMapper mapper = (FieldAliasMapper) obj; - return simpleName().equals(mapper.simpleName()) && name.equals(mapper.name) && path.equals(mapper.path); - } - - @Override - public int hashCode() { - return Objects.hash(name, path); - } - @Override public Iterator iterator() { return Collections.emptyIterator(); 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 9afe69a36d0be..0ae13241b7f56 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/MappingLookup.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/MappingLookup.java @@ -167,8 +167,7 @@ private MappingLookup( if (objects.containsKey(aliasMapper.name())) { throw new MapperParsingException("Alias [" + aliasMapper.name() + "] is defined both as an object and an alias"); } - Mapper existing = fieldMappers.put(aliasMapper.name(), aliasMapper); - if (existing != null && existing.equals(aliasMapper) == false) { + if (fieldMappers.put(aliasMapper.name(), aliasMapper) != null) { throw new MapperParsingException("Alias [" + aliasMapper.name() + "] is defined both as an alias and a concrete field"); } } diff --git a/server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java index 0bce02564ef34..7f954a570348a 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java @@ -317,7 +317,7 @@ protected static void parseProperties( throw new MapperParsingException("No handler for type [" + type + "] declared on field [" + fieldName + "]"); } Mapper.Builder fieldBuilder; - if (objBuilder.subobjects.value() == false || type.equals(FieldAliasMapper.CONTENT_TYPE)) { + if (objBuilder.subobjects.value() == false) { fieldBuilder = typeParser.parse(fieldName, propNode, parserContext); } else { String[] fieldNameParts = fieldName.split("\\."); 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 d75383503fe9b..2fe8c49df2175 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/RootObjectMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/RootObjectMapper.java @@ -145,12 +145,15 @@ void getAliasMappers(Map mappers, Map aliasMappe // 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 && conflict.equals(fieldMapper) == false) { - logger.warn( - "Root alias for field " - + fieldMapper.name() - + " conflicts with existing field or alias, skipping alias creation." - ); + 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 { FieldAliasMapper aliasMapper = new FieldAliasMapper.Builder(fieldMapper.simpleName()).path( fieldMapper.mappedFieldType.name() diff --git a/server/src/test/java/org/elasticsearch/index/mapper/FieldAliasMapperTests.java b/server/src/test/java/org/elasticsearch/index/mapper/FieldAliasMapperTests.java index c89c1af74fec4..c20091a308ed8 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/FieldAliasMapperTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/FieldAliasMapperTests.java @@ -115,14 +115,4 @@ public void testMergeFailure() throws IOException { exception.getMessage() ); } - - public void testEquals() { - FieldAliasMapper mapper = new FieldAliasMapper("simple", "name", "path"); - assertEquals(mapper, mapper); - assertNotEquals(mapper, null); - assertEquals(mapper, new FieldAliasMapper("simple", "name", "path")); - assertNotEquals(mapper, new FieldAliasMapper("simple2", "name", "path")); - assertNotEquals(mapper, new FieldAliasMapper("simple", "name2", "path")); - assertNotEquals(mapper, new FieldAliasMapper("simple", "name", "path2")); - } } From 01970b7e91b2d7e4ea6fe5bd933d00fc31101197 Mon Sep 17 00:00:00 2001 From: Kostas Krikellas Date: Thu, 8 Feb 2024 21:45:57 +0200 Subject: [PATCH 4/5] nest alias within objects --- .../index/mapper/RootObjectMapper.java | 32 ++++++++++++++++--- .../index/mapper/RootObjectMapperTests.java | 2 +- 2 files changed, 29 insertions(+), 5 deletions(-) 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 2fe8c49df2175..2fe76f091bd7a 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/RootObjectMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/RootObjectMapper.java @@ -155,10 +155,34 @@ void getAliasMappers(Map mappers, Map aliasMappe ); } } else { - FieldAliasMapper aliasMapper = new FieldAliasMapper.Builder(fieldMapper.simpleName()).path( - fieldMapper.mappedFieldType.name() - ).build(context); - aliasMappers.put(aliasMapper.simpleName(), aliasMapper); + // 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 { + // Nest the alias within object(s). + String realFieldName = fieldNameParts[fieldNameParts.length - 1]; + Mapper.Builder fieldBuilder = new FieldAliasMapper.Builder(realFieldName).path( + fieldMapper.mappedFieldType.name() + ); + for (int i = fieldNameParts.length - 2; i >= 0; --i) { + String intermediateObjectName = fieldNameParts[i]; + ObjectMapper.Builder intermediate = new ObjectMapper.Builder( + intermediateObjectName, + ObjectMapper.Defaults.SUBOBJECTS + ); + intermediate.add(fieldBuilder); + fieldBuilder = intermediate; + } + aliasMappers.put(fieldNameParts[0], fieldBuilder.build(context)); + } } } } 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 b616aa70dafde..4404f4238abeb 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/RootObjectMapperTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/RootObjectMapperTests.java @@ -387,7 +387,7 @@ public void testPassThroughObjectNested() throws IOException { })); assertThat(mapperService.mappingLookup().getMapper("dim"), instanceOf(FieldAliasMapper.class)); assertThat(mapperService.mappingLookup().getMapper("resource.attributes.dim"), instanceOf(KeywordFieldMapper.class)); - assertThat(mapperService.mappingLookup().getMapper("another.dim"), instanceOf(FieldAliasMapper.class)); + assertThat(mapperService.mappingLookup().objectMappers().get("another").getMapper("dim"), instanceOf(FieldAliasMapper.class)); assertThat(mapperService.mappingLookup().getMapper("attributes.another.dim"), instanceOf(KeywordFieldMapper.class)); } From c97256c0e8fcb9641583919df665ce46cda7cdf3 Mon Sep 17 00:00:00 2001 From: Kostas Krikellas <131142368+kkrik-es@users.noreply.github.com> Date: Fri, 9 Feb 2024 10:22:40 +0200 Subject: [PATCH 5/5] Delete docs/changelog/105298.yaml --- docs/changelog/105298.yaml | 5 ----- 1 file changed, 5 deletions(-) delete mode 100644 docs/changelog/105298.yaml diff --git a/docs/changelog/105298.yaml b/docs/changelog/105298.yaml deleted file mode 100644 index e0c67a2373f8a..0000000000000 --- a/docs/changelog/105298.yaml +++ /dev/null @@ -1,5 +0,0 @@ -pr: 105298 -summary: Ignore duplicate `FieldAliasMappers` -area: TSDB -type: bug -issues: []