Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make field limit more predictable #102885

5 changes: 5 additions & 0 deletions docs/changelog/102885.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
pr: 102885
summary: Make field limit more predictable
area: Mapping
type: enhancement
issues: []
Original file line number Diff line number Diff line change
Expand Up @@ -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 {

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,11 @@ public void validate(MappingLookup mappers) {
}
}

@Override
public int getTotalFieldsCount() {
return 1;
Copy link
Contributor

@salvatore-campagna salvatore-campagna Feb 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see here that we are counting aliases as one...wouldn't it make sense to skip aliases not counting them?
Considering also that we might use (extensively) passthrough fields #103648
probably it might make sense not to count them.

}

public static class TypeParser implements Mapper.TypeParser {
@Override
public Mapper.Builder parse(String name, Map<String, Object> node, MappingParserContext parserContext)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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<String, NamedAnalyzer> indexAnalyzers() {
return Map.of();
}
Expand Down Expand Up @@ -455,7 +461,7 @@ private void add(FieldMapper mapper) {

private void update(FieldMapper toMerge, MapperMergeContext context) {
if (mapperBuilders.containsKey(toMerge.simpleName()) == false) {
if (context.decrementFieldBudgetIfPossible(toMerge.mapperSize())) {
if (context.decrementFieldBudgetIfPossible(toMerge.getTotalFieldsCount())) {
add(toMerge);
}
} else {
Expand Down
14 changes: 3 additions & 11 deletions server/src/main/java/org/elasticsearch/index/mapper/Mapper.java
Original file line number Diff line number Diff line change
Expand Up @@ -137,16 +137,8 @@ public static FieldType freezeAndDeduplicateFieldType(FieldType fieldType) {
}

/**
* Returns the size this mapper counts against the {@linkplain MapperService#INDEX_MAPPING_TOTAL_FIELDS_LIMIT_SETTING field limit}.
* <p>
* Needs to be in sync with {@link MappingLookup#getTotalFieldsCount()}.
* 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 int mapperSize() {
int size = 1;
for (Mapper mapper : this) {
size += mapper.mapperSize();
}
return size;
}

public abstract int getTotalFieldsCount();
}
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ private CacheKey() {}
private final List<FieldMapper> indexTimeScriptMappers;
private final Mapping mapping;
private final Set<String> completionFields;
private final int totalFieldsCount;

/**
* Creates a new {@link MappingLookup} instance by parsing the provided mapping and extracting its field definitions.
Expand Down Expand Up @@ -127,6 +128,7 @@ private MappingLookup(
Collection<ObjectMapper> objectMappers,
Collection<FieldAliasMapper> aliasMappers
) {
this.totalFieldsCount = mapping.getRoot().getTotalFieldsCount();
this.mapping = mapping;
Map<String, Mapper> fieldMappers = new HashMap<>();
Map<String, ObjectMapper> objects = new HashMap<>();
Expand Down Expand Up @@ -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() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I can no longer tell the difference between getTotalFieldsCount and getTotalMapperCount . It's very subtle, or is there any difference at all?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return fieldMappers.size() + objectMappers.size() + runtimeFieldMappersCount;
}

Expand Down Expand Up @@ -282,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) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -182,6 +182,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) {
Expand Down Expand Up @@ -552,7 +557,7 @@ private static Map<String, Mapper> buildMergedMappers(

Mapper merged = null;
if (mergeIntoMapper == null) {
if (objectMergeContext.decrementFieldBudgetIfPossible(mergeWithMapper.mapperSize())) {
if (objectMergeContext.decrementFieldBudgetIfPossible(mergeWithMapper.getTotalFieldsCount())) {
merged = mergeWithMapper;
} else if (mergeWithMapper instanceof ObjectMapper om) {
merged = truncateObjectMapper(reason, objectMergeContext, om);
Expand Down Expand Up @@ -586,7 +591,7 @@ private static ObjectMapper truncateObjectMapper(MergeReason reason, MapperMerge
// there's not enough capacity for the whole object mapper,
// so we're just trying to add the shallow object, without it's sub-fields
ObjectMapper shallowObjectMapper = objectMapper.withoutMappers();
if (context.decrementFieldBudgetIfPossible(shallowObjectMapper.mapperSize())) {
if (context.decrementFieldBudgetIfPossible(shallowObjectMapper.getTotalFieldsCount())) {
// now trying to add the sub-fields one by one via a merge, until we hit the limit
return shallowObjectMapper.merge(objectMapper, reason, context);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -523,11 +523,7 @@ private static boolean processField(
}

@Override
public int mapperSize() {
int size = runtimeFields().size();
for (Mapper mapper : this) {
size += mapper.mapperSize();
}
return size;
public int getTotalFieldsCount() {
return mappers.values().stream().mapToInt(Mapper::getTotalFieldsCount).sum() + runtimeFields.size();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ public void testParsing() throws IOException {
);
DocumentMapper mapper = createDocumentMapper(mapping);
assertEquals(mapping, mapper.mappingSource().toString());
assertEquals(2, mapper.mapping().getRoot().mapperSize());
assertEquals(2, mapper.mapping().getRoot().getTotalFieldsCount());
}

public void testParsingWithMissingPath() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -213,10 +213,10 @@ public void testMergeWithLimit() {
final ObjectMapper mergedAdd1 = rootObjectMapper.merge(mergeWith, MapperMergeContext.root(false, false, 1));

// THEN "baz" new field is added to merged mapping
assertEquals(3, rootObjectMapper.mapperSize());
assertEquals(4, mergeWith.mapperSize());
assertEquals(3, mergedAdd0.mapperSize());
assertEquals(4, mergedAdd1.mapperSize());
assertEquals(3, rootObjectMapper.getTotalFieldsCount());
assertEquals(4, mergeWith.getTotalFieldsCount());
assertEquals(3, mergedAdd0.getTotalFieldsCount());
assertEquals(4, mergedAdd1.getTotalFieldsCount());
}

public void testMergeWithLimitTruncatedObjectField() {
Expand All @@ -231,11 +231,11 @@ public void testMergeWithLimitTruncatedObjectField() {
ObjectMapper mergedAdd1 = root.merge(mergeWith, MapperMergeContext.root(false, false, 1));
ObjectMapper mergedAdd2 = root.merge(mergeWith, MapperMergeContext.root(false, false, 2));
ObjectMapper mergedAdd3 = root.merge(mergeWith, MapperMergeContext.root(false, false, 3));
assertEquals(0, root.mapperSize());
assertEquals(0, mergedAdd0.mapperSize());
assertEquals(1, mergedAdd1.mapperSize());
assertEquals(2, mergedAdd2.mapperSize());
assertEquals(3, mergedAdd3.mapperSize());
assertEquals(0, root.getTotalFieldsCount());
assertEquals(0, mergedAdd0.getTotalFieldsCount());
assertEquals(1, mergedAdd1.getTotalFieldsCount());
assertEquals(2, mergedAdd2.getTotalFieldsCount());
assertEquals(3, mergedAdd3.getTotalFieldsCount());

ObjectMapper parent1 = (ObjectMapper) mergedAdd1.getMapper("parent");
assertNull(parent1.getMapper("child1"));
Expand All @@ -262,9 +262,9 @@ public void testMergeSameObjectDifferentFields() {

ObjectMapper mergedAdd0 = root.merge(mergeWith, MapperMergeContext.root(false, false, 0));
ObjectMapper mergedAdd1 = root.merge(mergeWith, MapperMergeContext.root(false, false, 1));
assertEquals(2, root.mapperSize());
assertEquals(2, mergedAdd0.mapperSize());
assertEquals(3, mergedAdd1.mapperSize());
assertEquals(2, root.getTotalFieldsCount());
assertEquals(2, mergedAdd0.getTotalFieldsCount());
assertEquals(3, mergedAdd1.getTotalFieldsCount());

ObjectMapper parent0 = (ObjectMapper) mergedAdd0.getMapper("parent");
assertNotNull(parent0.getMapper("child1"));
Expand All @@ -285,13 +285,13 @@ public void testMergeWithLimitMultiField() {
createTextKeywordMultiField("text", "keyword2")
).build(MapperBuilderContext.root(false, false));

assertEquals(2, mergeInto.mapperSize());
assertEquals(2, mergeWith.mapperSize());
assertEquals(2, mergeInto.getTotalFieldsCount());
assertEquals(2, mergeWith.getTotalFieldsCount());

ObjectMapper mergedAdd0 = mergeInto.merge(mergeWith, MapperMergeContext.root(false, false, 0));
ObjectMapper mergedAdd1 = mergeInto.merge(mergeWith, MapperMergeContext.root(false, false, 1));
assertEquals(2, mergedAdd0.mapperSize());
assertEquals(3, mergedAdd1.mapperSize());
assertEquals(2, mergedAdd0.getTotalFieldsCount());
assertEquals(3, mergedAdd1.getTotalFieldsCount());
}

public void testMergeWithLimitRuntimeField() {
Expand All @@ -302,13 +302,13 @@ public void testMergeWithLimitRuntimeField() {
new TestRuntimeField("existing_runtime_field", "keyword")
).addRuntimeField(new TestRuntimeField("new_runtime_field", "keyword")).build(MapperBuilderContext.root(false, false));

assertEquals(3, mergeInto.mapperSize());
assertEquals(2, mergeWith.mapperSize());
assertEquals(3, mergeInto.getTotalFieldsCount());
assertEquals(2, mergeWith.getTotalFieldsCount());

ObjectMapper mergedAdd0 = mergeInto.merge(mergeWith, MapperMergeContext.root(false, false, 0));
ObjectMapper mergedAdd1 = mergeInto.merge(mergeWith, MapperMergeContext.root(false, false, 1));
assertEquals(3, mergedAdd0.mapperSize());
assertEquals(4, mergedAdd1.mapperSize());
assertEquals(3, mergedAdd0.getTotalFieldsCount());
assertEquals(4, mergedAdd1.getTotalFieldsCount());
}

private static RootObjectMapper createRootSubobjectFalseLeafWithDots() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -530,15 +530,20 @@ public void testSyntheticSourceDocValuesFieldWithout() throws IOException {
assertThat(mapper.mapping().getRoot().syntheticFieldLoader().docValuesLoader(null, null), nullValue());
}

public void testNestedObjectWithMultiFieldsMapperSize() throws IOException {
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())
).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)).mapperSize(), equalTo(5));
assertThat(mapperBuilder.build(MapperBuilderContext.root(false, false)).getTotalFieldsCount(), equalTo(6));
}

public void testWithoutMappers() throws IOException {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,7 @@ public void testRuntimeSection() throws IOException {
}));
MapperService mapperService = createMapperService(mapping);
assertEquals(mapping, mapperService.documentMapper().mappingSource().toString());
assertEquals(3, mapperService.documentMapper().mapping().getRoot().mapperSize());
assertEquals(3, mapperService.documentMapper().mapping().getRoot().getTotalFieldsCount());
}

public void testRuntimeSectionRejectedUpdate() throws IOException {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -427,6 +427,11 @@ public final void testMinimalToMaximal() throws IOException {
assertParseMaximalWarnings();
}

public void testTotalFieldsCount() throws IOException {
MapperService mapperService = createMapperService(fieldMapping(this::minimalMapping));
assertEquals(1, mapperService.documentMapper().mapping().getRoot().getTotalFieldsCount());
}

protected final void assertParseMinimalWarnings() {
String[] warnings = getParseMinimalWarnings();
if (warnings.length > 0) {
Expand Down