Skip to content

Commit 3df3199

Browse files
Save heap when dynamically mapping arrays
We can do the merging more eagerly to save on heap at the cost of some cycles (though the overhead should be reasonably limited because we save a little down the line if all the mappers are the same instance). Obviously a bit of a dirty trick but this logic is quite brittle now with things like the vector mapper special case and this seems to me like the shortest path to avoiding runaway heap use. This yet again shows the need to implement actual mapper equality checks and stronger deduplication for them to avoid having mapping merging be our only mechanism for deduplication. closes #117593
1 parent 3057168 commit 3df3199

File tree

3 files changed

+32
-5
lines changed

3 files changed

+32
-5
lines changed

server/src/internalClusterTest/java/org/elasticsearch/index/mapper/DynamicMappingIT.java

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -122,9 +122,9 @@ public void testArrayWithDifferentTypes() {
122122
.get();
123123

124124
assertTrue(bulkResponse.hasFailures());
125-
assertEquals(
126-
"mapper [foo] cannot be changed from type [long] to [text]",
127-
bulkResponse.getItems()[0].getFailure().getCause().getMessage()
125+
assertThat(
126+
bulkResponse.getItems()[0].getFailure().getCause().getMessage(),
127+
containsString("mapper [foo] cannot be changed from type [long] to [text]")
128128
);
129129
}
130130

server/src/main/java/org/elasticsearch/index/mapper/DynamicFieldsBuilder.java

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,8 @@
2121

2222
import java.io.IOException;
2323
import java.time.DateTimeException;
24+
import java.util.Collections;
25+
import java.util.List;
2426
import java.util.Map;
2527

2628
/**
@@ -312,6 +314,11 @@ private static final class Concrete implements Strategy {
312314
boolean createDynamicField(Mapper.Builder builder, DocumentParserContext context, MapperBuilderContext mapperBuilderContext)
313315
throws IOException {
314316
Mapper mapper = builder.build(mapperBuilderContext);
317+
List<Mapper> existing;
318+
if ((existing = context.getDynamicMappers(mapper.fullPath())) != null && existing.isEmpty() == false) {
319+
maybeMergeWithExistingField(context, mapperBuilderContext, mapper, existing);
320+
return true;
321+
}
315322
if (context.addDynamicMapper(mapper)) {
316323
parseField.accept(context, mapper);
317324
return true;
@@ -320,6 +327,26 @@ boolean createDynamicField(Mapper.Builder builder, DocumentParserContext context
320327
}
321328
}
322329

330+
// Attempts to deduplicate arrays of mapper instances my merging with the most recently added mapper for the same path
331+
// if there is one already and then replacing all entries for the field with the merge result
332+
// TODO: this is only as complicated as it is to enable DocumentParser.postProcessDynamicArrayMapping, technically there is no need
333+
// to duplicate the mapper other than to enabled this method's translation of multiple numeric field mappers into a vector mapper
334+
// it seems?
335+
private void maybeMergeWithExistingField(
336+
DocumentParserContext context,
337+
MapperBuilderContext mapperBuilderContext,
338+
Mapper fieldMapper,
339+
List<Mapper> existing
340+
) throws IOException {
341+
var last = existing.getLast();
342+
var merged = last.merge(fieldMapper, MapperMergeContext.from(mapperBuilderContext, Long.MAX_VALUE));
343+
if (merged != last) {
344+
Collections.fill(existing, merged);
345+
}
346+
existing.add(merged);
347+
parseField.accept(context, merged);
348+
}
349+
323350
boolean createDynamicField(Mapper.Builder builder, DocumentParserContext context) throws IOException {
324351
return createDynamicField(builder, context, context.createDynamicMapperBuilderContext());
325352
}

server/src/test/java/org/elasticsearch/index/mapper/ObjectMapperTests.java

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -37,8 +37,8 @@ public class ObjectMapperTests extends MapperServiceTestCase {
3737

3838
public void testDifferentInnerObjectTokenFailure() throws Exception {
3939
DocumentMapper defaultMapper = createDocumentMapper(mapping(b -> {}));
40-
IllegalArgumentException e = expectThrows(
41-
IllegalArgumentException.class,
40+
DocumentParsingException e = expectThrows(
41+
DocumentParsingException.class,
4242
() -> defaultMapper.parse(new SourceToParse("1", new BytesArray("""
4343
{
4444
"object": {

0 commit comments

Comments
 (0)