diff --git a/docs/changelog/127439.yaml b/docs/changelog/127439.yaml new file mode 100644 index 0000000000000..b2dd324284017 --- /dev/null +++ b/docs/changelog/127439.yaml @@ -0,0 +1,6 @@ +pr: 127439 +summary: Save heap when dynamically mapping arrays +area: Mapping +type: bug +issues: + - 117593 diff --git a/server/src/internalClusterTest/java/org/elasticsearch/index/mapper/DynamicMappingIT.java b/server/src/internalClusterTest/java/org/elasticsearch/index/mapper/DynamicMappingIT.java index 71b5ba58b5789..628345c85d643 100644 --- a/server/src/internalClusterTest/java/org/elasticsearch/index/mapper/DynamicMappingIT.java +++ b/server/src/internalClusterTest/java/org/elasticsearch/index/mapper/DynamicMappingIT.java @@ -122,9 +122,9 @@ public void testArrayWithDifferentTypes() { .get(); assertTrue(bulkResponse.hasFailures()); - assertEquals( - "mapper [foo] cannot be changed from type [long] to [text]", - bulkResponse.getItems()[0].getFailure().getCause().getMessage() + assertThat( + bulkResponse.getItems()[0].getFailure().getCause().getMessage(), + containsString("mapper [foo] cannot be changed from type [long] to [text]") ); } diff --git a/server/src/main/java/org/elasticsearch/index/mapper/DynamicFieldsBuilder.java b/server/src/main/java/org/elasticsearch/index/mapper/DynamicFieldsBuilder.java index fc0b0d864547b..91563bd86dff1 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/DynamicFieldsBuilder.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/DynamicFieldsBuilder.java @@ -21,6 +21,8 @@ import java.io.IOException; import java.time.DateTimeException; +import java.util.Collections; +import java.util.List; import java.util.Map; /** @@ -312,6 +314,11 @@ private static final class Concrete implements Strategy { boolean createDynamicField(Mapper.Builder builder, DocumentParserContext context, MapperBuilderContext mapperBuilderContext) throws IOException { Mapper mapper = builder.build(mapperBuilderContext); + List existing; + if ((existing = context.getDynamicMappers(mapper.fullPath())) != null && existing.isEmpty() == false) { + maybeMergeWithExistingField(context, mapperBuilderContext, mapper, existing); + return true; + } if (context.addDynamicMapper(mapper)) { parseField.accept(context, mapper); return true; @@ -320,6 +327,26 @@ boolean createDynamicField(Mapper.Builder builder, DocumentParserContext context } } + // Attempts to deduplicate arrays of mapper instances my merging with the most recently added mapper for the same path + // if there is one already and then replacing all entries for the field with the merge result + // TODO: this is only as complicated as it is to enable DocumentParser.postProcessDynamicArrayMapping, technically there is no need + // to duplicate the mapper other than to enabled this method's translation of multiple numeric field mappers into a vector mapper + // it seems? + private void maybeMergeWithExistingField( + DocumentParserContext context, + MapperBuilderContext mapperBuilderContext, + Mapper fieldMapper, + List existing + ) throws IOException { + var last = existing.getLast(); + var merged = last.merge(fieldMapper, MapperMergeContext.from(mapperBuilderContext, Long.MAX_VALUE)); + if (merged != last) { + Collections.fill(existing, merged); + } + existing.add(merged); + parseField.accept(context, merged); + } + boolean createDynamicField(Mapper.Builder builder, DocumentParserContext context) throws IOException { return createDynamicField(builder, context, context.createDynamicMapperBuilderContext()); } diff --git a/server/src/test/java/org/elasticsearch/index/mapper/ObjectMapperTests.java b/server/src/test/java/org/elasticsearch/index/mapper/ObjectMapperTests.java index 6dfcf5d7399d2..92adbfc2a7611 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/ObjectMapperTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/ObjectMapperTests.java @@ -37,8 +37,8 @@ public class ObjectMapperTests extends MapperServiceTestCase { public void testDifferentInnerObjectTokenFailure() throws Exception { DocumentMapper defaultMapper = createDocumentMapper(mapping(b -> {})); - IllegalArgumentException e = expectThrows( - IllegalArgumentException.class, + DocumentParsingException e = expectThrows( + DocumentParsingException.class, () -> defaultMapper.parse(new SourceToParse("1", new BytesArray(""" { "object": {