From f9f19ef0bfc1d00d2532d309cbc2ec500b32e7a0 Mon Sep 17 00:00:00 2001 From: Felix Barnsteiner Date: Fri, 1 Dec 2023 17:53:25 +0100 Subject: [PATCH 1/8] Make field limit more predictable Today, we're counting all mappers, including mappers for subfields that aren't explicitly added to the mapping towards the field limit. This means that some field types, such as search_as_you_type or percolator count as more than one field even though that's not apparent to users as they're just defining them as a single field in the mapping. This change makes it so that each field mapper only counts as one. We're still counting multi-fields. This makes it easier to understand for users why the field limit is hit. --- .../java/org/elasticsearch/index/IndexService.java | 2 +- .../index/mapper/FieldAliasMapper.java | 5 +++++ .../org/elasticsearch/index/mapper/FieldMapper.java | 6 ++++++ .../java/org/elasticsearch/index/mapper/Mapper.java | 6 ++++++ .../elasticsearch/index/mapper/MappingLookup.java | 12 +++++++++++- .../elasticsearch/index/mapper/ObjectMapper.java | 5 +++++ .../index/mapper/RootObjectMapper.java | 5 +++++ .../index/mapper/ObjectMapperTests.java | 13 +++++++++++++ .../index/mapper/MapperServiceTestCase.java | 12 ++++++++++++ .../elasticsearch/index/mapper/MapperTestCase.java | 5 +++++ 10 files changed, 69 insertions(+), 2 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/index/IndexService.java b/server/src/main/java/org/elasticsearch/index/IndexService.java index 2e600bbdc5ed4..06397f1da6540 100644 --- a/server/src/main/java/org/elasticsearch/index/IndexService.java +++ b/server/src/main/java/org/elasticsearch/index/IndexService.java @@ -330,7 +330,7 @@ public NodeMappingStats getNodeMappingStats() { if (mapperService == null) { return null; } - long totalCount = mapperService().mappingLookup().getTotalFieldsCount(); + long totalCount = mapperService().mappingLookup().getTotalMapperCount(); long totalEstimatedOverhead = totalCount * 1024L; // 1KiB estimated per mapping NodeMappingStats indexNodeMappingStats = new NodeMappingStats(totalCount, totalEstimatedOverhead); return indexNodeMappingStats; 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 0654f48d0382e..2cdb7ecbca819 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/FieldAliasMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/FieldAliasMapper.java @@ -113,6 +113,11 @@ public void validate(MappingLookup mappers) { } } + @Override + public int getTotalFieldsCount() { + return 1; + } + public static class TypeParser implements Mapper.TypeParser { @Override public Mapper.Builder parse(String name, Map node, MappingParserContext parserContext) diff --git a/server/src/main/java/org/elasticsearch/index/mapper/FieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/FieldMapper.java index 974d935a35e50..b5e895e70dbbc 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/FieldMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/FieldMapper.java @@ -51,6 +51,7 @@ import java.util.function.Consumer; import java.util.function.Function; import java.util.function.Supplier; +import java.util.stream.Stream; import static org.elasticsearch.core.Strings.format; @@ -428,6 +429,11 @@ protected void doXContentBody(XContentBuilder builder, Params params) throws IOE protected abstract String contentType(); + @Override + public int getTotalFieldsCount() { + return 1 + Stream.of(multiFields.mappers).mapToInt(FieldMapper::getTotalFieldsCount).sum(); + } + public Map indexAnalyzers() { return Map.of(); } diff --git a/server/src/main/java/org/elasticsearch/index/mapper/Mapper.java b/server/src/main/java/org/elasticsearch/index/mapper/Mapper.java index e977b0aac014a..02bab05ac1a90 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/Mapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/Mapper.java @@ -133,4 +133,10 @@ public static FieldType freezeAndDeduplicateFieldType(FieldType fieldType) { } return fieldTypeDeduplicator.computeIfAbsent(fieldType, Function.identity()); } + + /** + * The total number of fields as defined in the mapping. + * Defines how this mapper counts towards {@link MapperService#INDEX_MAPPING_TOTAL_FIELDS_LIMIT_SETTING}. + */ + public abstract int getTotalFieldsCount(); } 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 4880ce5edc204..bd3ce3fc2f7e7 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/MappingLookup.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/MappingLookup.java @@ -55,6 +55,7 @@ private CacheKey() {} private final List indexTimeScriptMappers; private final Mapping mapping; private final Set completionFields; + private final int totalFieldsCount; /** * Creates a new {@link MappingLookup} instance by parsing the provided mapping and extracting its field definitions. @@ -127,6 +128,7 @@ private MappingLookup( Collection objectMappers, Collection aliasMappers ) { + this.totalFieldsCount = mapping.getRoot().getTotalFieldsCount(); this.mapping = mapping; Map fieldMappers = new HashMap<>(); Map objects = new HashMap<>(); @@ -223,6 +225,14 @@ FieldTypeLookup fieldTypesLookup() { * Returns the total number of fields defined in the mappings, including field mappers, object mappers as well as runtime fields. */ public long getTotalFieldsCount() { + return totalFieldsCount; + } + + /** + * Returns the total number of mappers defined in the mappings, including field mappers and their sub-fields + * (which are not explicitly defined in the mappings), multi-fields, object mappers, runtime fields and metadata field mappers. + */ + public long getTotalMapperCount() { return fieldMappers.size() + objectMappers.size() + runtimeFieldMappersCount; } @@ -271,7 +281,7 @@ private void checkFieldLimit(long limit) { } void checkFieldLimit(long limit, int additionalFieldsToAdd) { - if (getTotalFieldsCount() + additionalFieldsToAdd - mapping.getSortedMetadataMappers().length > limit) { + if (totalFieldsCount + additionalFieldsToAdd > limit) { throw new IllegalArgumentException( "Limit of total fields [" + limit 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 d67763879433f..e87274b43036a 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java @@ -187,6 +187,11 @@ public ObjectMapper build(MapperBuilderContext context) { } } + @Override + public int getTotalFieldsCount() { + return 1 + mappers.values().stream().mapToInt(Mapper::getTotalFieldsCount).sum(); + } + public static class TypeParser implements Mapper.TypeParser { @Override public boolean supportsVersion(IndexVersion indexCreatedVersion) { 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 65fce1b69b8cc..80baad50a2761 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/RootObjectMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/RootObjectMapper.java @@ -307,6 +307,11 @@ protected void doXContent(XContentBuilder builder, ToXContent.Params params) thr } } + @Override + public int getTotalFieldsCount() { + return mappers.values().stream().mapToInt(Mapper::getTotalFieldsCount).sum() + runtimeFields.size(); + } + private static void validateDynamicTemplate(MappingParserContext parserContext, DynamicTemplate template) { if (containsSnippet(template.getMapping(), "{name}")) { 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 ec6a9ddd53e2c..0f29d1d8d1b11 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/ObjectMapperTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/ObjectMapperTests.java @@ -8,6 +8,7 @@ package org.elasticsearch.index.mapper; +import org.elasticsearch.common.Explicit; import org.elasticsearch.common.Strings; import org.elasticsearch.common.bytes.BytesArray; import org.elasticsearch.common.bytes.BytesReference; @@ -23,6 +24,7 @@ import java.util.List; import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.instanceOf; import static org.hamcrest.Matchers.nullValue; @@ -522,4 +524,15 @@ public void testSyntheticSourceDocValuesFieldWithout() throws IOException { assertThat(o.syntheticFieldLoader().docValuesLoader(null, null), nullValue()); assertThat(mapper.mapping().getRoot().syntheticFieldLoader().docValuesLoader(null, null), nullValue()); } + + public void testNestedObjectWithMultiFieldsMapperSize() { + ObjectMapper.Builder mapperBuilder = new ObjectMapper.Builder("parent_size_1", Explicit.IMPLICIT_TRUE).add( + new ObjectMapper.Builder("child_size_2", Explicit.IMPLICIT_TRUE).add( + new TextFieldMapper.Builder("grand_child_size_3", createDefaultIndexAnalyzers()).addMultiField( + new KeywordFieldMapper.Builder("multi_field_size_4", IndexVersion.current()) + ).addMultiField(new KeywordFieldMapper.Builder("multi_field_size_5", IndexVersion.current())) + ) + ); + assertThat(mapperBuilder.build(MapperBuilderContext.root(false, false)).getTotalFieldsCount(), equalTo(5)); + } } diff --git a/test/framework/src/main/java/org/elasticsearch/index/mapper/MapperServiceTestCase.java b/test/framework/src/main/java/org/elasticsearch/index/mapper/MapperServiceTestCase.java index aecd81882c108..719f65a3250e7 100644 --- a/test/framework/src/main/java/org/elasticsearch/index/mapper/MapperServiceTestCase.java +++ b/test/framework/src/main/java/org/elasticsearch/index/mapper/MapperServiceTestCase.java @@ -225,6 +225,18 @@ protected final MapperService createMapperService(IndexVersion version, Settings ); } + @SuppressWarnings("unchecked") + protected RootObjectMapper.Builder getRootObjectMapperBuilder(XContentBuilder mapping) throws IOException { + MapperService mapperService = createMapperService(mapping(b -> {})); + Map mappingAsMap = MappingParser.convertToMap(new CompressedXContent(BytesReference.bytes(mapping))); + RootObjectMapper.Builder builder = RootObjectMapper.parse( + "_doc", + (Map) mappingAsMap.get("_doc"), + mapperService.parserContext() + ); + return builder; + } + /** * This is the injection point for tests that require mock scripts. Test cases should override this to return the * mock script factory of their choice. diff --git a/test/framework/src/main/java/org/elasticsearch/index/mapper/MapperTestCase.java b/test/framework/src/main/java/org/elasticsearch/index/mapper/MapperTestCase.java index 44e28132beec0..44a7b291d3004 100644 --- a/test/framework/src/main/java/org/elasticsearch/index/mapper/MapperTestCase.java +++ b/test/framework/src/main/java/org/elasticsearch/index/mapper/MapperTestCase.java @@ -425,6 +425,11 @@ public final void testMinimalToMaximal() throws IOException { assertParseMaximalWarnings(); } + public void testTotalFieldsCount() throws IOException { + RootObjectMapper.Builder builder = getRootObjectMapperBuilder(fieldMapping(this::minimalMapping)); + assertEquals(1, builder.build(MapperBuilderContext.root(false, false)).getTotalFieldsCount()); + } + protected final void assertParseMinimalWarnings() { String[] warnings = getParseMinimalWarnings(); if (warnings.length > 0) { From 7b227bbb0577e5d3cb2d6a159dc3c5ed2bb9860b Mon Sep 17 00:00:00 2001 From: Felix Barnsteiner Date: Fri, 1 Dec 2023 18:09:14 +0100 Subject: [PATCH 2/8] Add test case for multi-field inside a multi-field --- .../elasticsearch/index/mapper/ObjectMapperTests.java | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) 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 0f29d1d8d1b11..3b10b42425a5c 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/ObjectMapperTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/ObjectMapperTests.java @@ -530,9 +530,14 @@ public void testNestedObjectWithMultiFieldsMapperSize() { new ObjectMapper.Builder("child_size_2", Explicit.IMPLICIT_TRUE).add( new TextFieldMapper.Builder("grand_child_size_3", createDefaultIndexAnalyzers()).addMultiField( new KeywordFieldMapper.Builder("multi_field_size_4", IndexVersion.current()) - ).addMultiField(new KeywordFieldMapper.Builder("multi_field_size_5", IndexVersion.current())) + ) + .addMultiField( + new TextFieldMapper.Builder("grand_child_size_5", createDefaultIndexAnalyzers()).addMultiField( + new KeywordFieldMapper.Builder("multi_field_of_multi_field_size_6", IndexVersion.current()) + ) + ) ) ); - assertThat(mapperBuilder.build(MapperBuilderContext.root(false, false)).getTotalFieldsCount(), equalTo(5)); + assertThat(mapperBuilder.build(MapperBuilderContext.root(false, false)).getTotalFieldsCount(), equalTo(6)); } } From e96cb641b35b68e66b87ed7ee8410e8623ec9176 Mon Sep 17 00:00:00 2001 From: Felix Barnsteiner Date: Fri, 1 Dec 2023 18:10:30 +0100 Subject: [PATCH 3/8] Update docs/changelog/102885.yaml --- docs/changelog/102885.yaml | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 docs/changelog/102885.yaml diff --git a/docs/changelog/102885.yaml b/docs/changelog/102885.yaml new file mode 100644 index 0000000000000..7a998c3eb1f66 --- /dev/null +++ b/docs/changelog/102885.yaml @@ -0,0 +1,5 @@ +pr: 102885 +summary: Make field limit more predictable +area: Mapping +type: enhancement +issues: [] From bc3542f9881ef899b27b3ae4b188ea26accd4a14 Mon Sep 17 00:00:00 2001 From: Felix Barnsteiner Date: Fri, 1 Dec 2023 18:37:18 +0100 Subject: [PATCH 4/8] Add assertWarnings to LegacyGeoShapeFieldMapperTests --- .../legacygeo/mapper/LegacyGeoShapeFieldMapperTests.java | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/modules/legacy-geo/src/test/java/org/elasticsearch/legacygeo/mapper/LegacyGeoShapeFieldMapperTests.java b/modules/legacy-geo/src/test/java/org/elasticsearch/legacygeo/mapper/LegacyGeoShapeFieldMapperTests.java index 91a94fe174c21..0a0bb12bedbae 100644 --- a/modules/legacy-geo/src/test/java/org/elasticsearch/legacygeo/mapper/LegacyGeoShapeFieldMapperTests.java +++ b/modules/legacy-geo/src/test/java/org/elasticsearch/legacygeo/mapper/LegacyGeoShapeFieldMapperTests.java @@ -76,6 +76,12 @@ protected boolean supportsStoredFields() { return false; } + @Override + public void testTotalFieldsCount() throws IOException { + super.testTotalFieldsCount(); + assertWarnings("Parameter [strategy] is deprecated and will be removed in a future version"); + } + @Override protected void registerParameters(ParameterChecker checker) throws IOException { From eb9ae17a9054e3a45ab59805611e5a2ffd975556 Mon Sep 17 00:00:00 2001 From: Felix Barnsteiner Date: Wed, 24 Jan 2024 12:39:28 +0100 Subject: [PATCH 5/8] Fix exceedsLimit --- .../main/java/org/elasticsearch/index/mapper/MappingLookup.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 d71d3dd3106c3..e5184f90e2b75 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/MappingLookup.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/MappingLookup.java @@ -292,7 +292,7 @@ void checkFieldLimit(long limit, int additionalFieldsToAdd) { } boolean exceedsLimit(long limit, int additionalFieldsToAdd) { - return getTotalFieldsCount() + additionalFieldsToAdd - mapping.getSortedMetadataMappers().length > limit; + return totalFieldsCount + additionalFieldsToAdd > limit; } private void checkDimensionFieldLimit(long limit) { From 99eb652832ae63d18e506e93361bd9358b0eb699 Mon Sep 17 00:00:00 2001 From: Felix Barnsteiner Date: Wed, 24 Jan 2024 12:39:52 +0100 Subject: [PATCH 6/8] Remove unnecessary getRootObjectMapperBuilder method --- .../index/mapper/MapperServiceTestCase.java | 12 ------------ .../elasticsearch/index/mapper/MapperTestCase.java | 4 ++-- 2 files changed, 2 insertions(+), 14 deletions(-) diff --git a/test/framework/src/main/java/org/elasticsearch/index/mapper/MapperServiceTestCase.java b/test/framework/src/main/java/org/elasticsearch/index/mapper/MapperServiceTestCase.java index bef40c0828982..710c31ed7aee8 100644 --- a/test/framework/src/main/java/org/elasticsearch/index/mapper/MapperServiceTestCase.java +++ b/test/framework/src/main/java/org/elasticsearch/index/mapper/MapperServiceTestCase.java @@ -225,18 +225,6 @@ protected final MapperService createMapperService(IndexVersion version, Settings ); } - @SuppressWarnings("unchecked") - protected RootObjectMapper.Builder getRootObjectMapperBuilder(XContentBuilder mapping) throws IOException { - MapperService mapperService = createMapperService(mapping(b -> {})); - Map mappingAsMap = MappingParser.convertToMap(new CompressedXContent(BytesReference.bytes(mapping))); - RootObjectMapper.Builder builder = RootObjectMapper.parse( - "_doc", - (Map) mappingAsMap.get("_doc"), - mapperService.parserContext() - ); - return builder; - } - /** * This is the injection point for tests that require mock scripts. Test cases should override this to return the * mock script factory of their choice. diff --git a/test/framework/src/main/java/org/elasticsearch/index/mapper/MapperTestCase.java b/test/framework/src/main/java/org/elasticsearch/index/mapper/MapperTestCase.java index e7b2996febdd2..43ac8057a3fc0 100644 --- a/test/framework/src/main/java/org/elasticsearch/index/mapper/MapperTestCase.java +++ b/test/framework/src/main/java/org/elasticsearch/index/mapper/MapperTestCase.java @@ -428,8 +428,8 @@ public final void testMinimalToMaximal() throws IOException { } public void testTotalFieldsCount() throws IOException { - RootObjectMapper.Builder builder = getRootObjectMapperBuilder(fieldMapping(this::minimalMapping)); - assertEquals(1, builder.build(MapperBuilderContext.root(false, false)).getTotalFieldsCount()); + MapperService mapperService = createMapperService(fieldMapping(this::minimalMapping)); + assertEquals(1, mapperService.documentMapper().mapping().getRoot().getTotalFieldsCount()); } protected final void assertParseMinimalWarnings() { From 1f70c52cafbe79746a5b3d1f9e3845d62ede8972 Mon Sep 17 00:00:00 2001 From: Felix Barnsteiner Date: Wed, 24 Jan 2024 13:26:57 +0100 Subject: [PATCH 7/8] Apply spotless suggestions --- .../org/elasticsearch/index/mapper/ObjectMapperTests.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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 6674cd5771ab0..a0560e940eab0 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/ObjectMapperTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/ObjectMapperTests.java @@ -534,8 +534,8 @@ public void testNestedObjectWithMultiFieldsgetTotalFieldsCount() { ObjectMapper.Builder mapperBuilder = new ObjectMapper.Builder("parent_size_1", Explicit.IMPLICIT_TRUE).add( new ObjectMapper.Builder("child_size_2", Explicit.IMPLICIT_TRUE).add( new TextFieldMapper.Builder("grand_child_size_3", createDefaultIndexAnalyzers()).addMultiField( - new KeywordFieldMapper.Builder("multi_field_size_4", IndexVersion.current()) - ) + new KeywordFieldMapper.Builder("multi_field_size_4", IndexVersion.current()) + ) .addMultiField( new TextFieldMapper.Builder("grand_child_size_5", createDefaultIndexAnalyzers()).addMultiField( new KeywordFieldMapper.Builder("multi_field_of_multi_field_size_6", IndexVersion.current()) From fd41e6736640a0bcad5f73378cc0a75afe8322c0 Mon Sep 17 00:00:00 2001 From: Felix Barnsteiner Date: Tue, 6 Feb 2024 11:34:32 +0100 Subject: [PATCH 8/8] Update DocumentParserContext to use new method name --- .../org/elasticsearch/index/mapper/DocumentParserContext.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/src/main/java/org/elasticsearch/index/mapper/DocumentParserContext.java b/server/src/main/java/org/elasticsearch/index/mapper/DocumentParserContext.java index 0a669fb0ade8a..66c5de61bcd92 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/DocumentParserContext.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/DocumentParserContext.java @@ -335,7 +335,7 @@ public final boolean addDynamicMapper(Mapper mapper) { if (mappingLookup.getMapper(mapper.name()) == null && mappingLookup.objectMappers().containsKey(mapper.name()) == false && dynamicMappers.containsKey(mapper.name()) == false) { - int mapperSize = mapper.mapperSize(); + int mapperSize = mapper.getTotalFieldsCount(); int additionalFieldsToAdd = getNewFieldsSize() + mapperSize; if (indexSettings().isIgnoreDynamicFieldsBeyondLimit()) { if (mappingLookup.exceedsLimit(indexSettings().getMappingTotalFieldsLimit(), additionalFieldsToAdd)) {