From 0aa542d979186655825e8d8822061b53a03cd3eb Mon Sep 17 00:00:00 2001 From: Felix Barnsteiner Date: Mon, 4 Dec 2023 18:07:02 +0100 Subject: [PATCH 01/40] Add ability to limit fields added during Mapper#merge This is extracting a piece of independent functionality from https://github.com/elastic/elasticsearch/pull/96235 --- .../LegacyGeoShapeFieldMapperTests.java | 12 +++ .../extras/SearchAsYouTypeFieldMapper.java | 6 ++ .../join/mapper/ParentJoinFieldMapper.java | 5 ++ .../percolator/PercolatorFieldMapper.java | 10 +++ .../metadata/MetadataMappingService.java | 2 +- .../index/mapper/DocumentParser.java | 2 +- .../index/mapper/FieldAliasMapper.java | 7 +- .../index/mapper/FieldMapper.java | 23 +++-- .../elasticsearch/index/mapper/Mapper.java | 29 ++++++- .../index/mapper/MapperMergeContext.java | 84 +++++++++++++++++++ .../index/mapper/MapperService.java | 27 ++++-- .../elasticsearch/index/mapper/Mapping.java | 7 +- .../index/mapper/MappingLookup.java | 6 +- .../index/mapper/NestedObjectMapper.java | 26 +++++- .../index/mapper/ObjectMapper.java | 65 +++++++------- .../index/mapper/ParsedDocument.java | 2 +- .../index/mapper/PlaceHolderFieldMapper.java | 2 +- .../index/mapper/RootObjectMapper.java | 41 +++++++-- .../index/mapper/DocumentMapperTests.java | 16 ++-- .../index/mapper/DocumentParserTests.java | 2 +- .../index/mapper/FieldAliasMapperTests.java | 34 ++++---- .../mapper/GeoShapeFieldMapperTests.java | 6 ++ .../index/mapper/MapperMergeContextTests.java | 69 +++++++++++++++ .../index/mapper/NestedObjectMapperTests.java | 4 +- .../index/mapper/ObjectMapperMergeTests.java | 30 ++++--- .../index/mapper/ObjectMapperTests.java | 20 ++++- .../index/mapper/ParametrizedMapperTests.java | 19 +++-- .../index/mapper/RootObjectMapperTests.java | 9 ++ .../flattened/FlattenedFieldMapperTests.java | 3 + .../index/mapper/MapperServiceTestCase.java | 12 +++ .../index/mapper/MapperTestCase.java | 26 ++++++ .../AggregateDoubleMetricFieldMapper.java | 5 ++ .../mapper/ConstantKeywordFieldMapper.java | 5 ++ .../CountedKeywordFieldMapper.java | 5 ++ ...GeoShapeWithDocValuesFieldMapperTests.java | 6 ++ .../index/mapper/PointFieldMapperTests.java | 6 ++ .../index/mapper/ShapeFieldMapperTests.java | 6 ++ 37 files changed, 525 insertions(+), 114 deletions(-) create mode 100644 server/src/main/java/org/elasticsearch/index/mapper/MapperMergeContext.java create mode 100644 server/src/test/java/org/elasticsearch/index/mapper/MapperMergeContextTests.java 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..ae979448357ee 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 @@ -104,6 +104,18 @@ protected void registerParameters(ParameterChecker checker) throws IOException { checker.registerUpdateCheck(b -> b.field("distance_error_pct", 0.8), m -> {}); } + @Override + public void testMapperBuilderSize() throws IOException { + super.testMapperBuilderSize(); + assertWarnings("Parameter [strategy] is deprecated and will be removed in a future version"); + } + + @Override + public void testMapperBuilderSizeMultiField() throws IOException { + super.testMapperBuilderSizeMultiField(); + assertWarnings("Parameter [strategy] is deprecated and will be removed in a future version"); + } + @Override protected Collection getPlugins() { return List.of(new TestLegacyGeoShapeFieldMapperPlugin()); diff --git a/modules/mapper-extras/src/main/java/org/elasticsearch/index/mapper/extras/SearchAsYouTypeFieldMapper.java b/modules/mapper-extras/src/main/java/org/elasticsearch/index/mapper/extras/SearchAsYouTypeFieldMapper.java index ca8231c46736f..a76e54b3580ce 100644 --- a/modules/mapper-extras/src/main/java/org/elasticsearch/index/mapper/extras/SearchAsYouTypeFieldMapper.java +++ b/modules/mapper-extras/src/main/java/org/elasticsearch/index/mapper/extras/SearchAsYouTypeFieldMapper.java @@ -270,6 +270,12 @@ public SearchAsYouTypeFieldMapper build(MapperBuilderContext context) { this ); } + + @Override + public int mapperSize() { + return super.mapperSize() + 1 // prefixField + + maxShingleSize.getValue() - 1; + } } private static int countPosition(TokenStream stream) throws IOException { diff --git a/modules/parent-join/src/main/java/org/elasticsearch/join/mapper/ParentJoinFieldMapper.java b/modules/parent-join/src/main/java/org/elasticsearch/join/mapper/ParentJoinFieldMapper.java index 2bbd5e81444b7..6412e40b651cf 100644 --- a/modules/parent-join/src/main/java/org/elasticsearch/join/mapper/ParentJoinFieldMapper.java +++ b/modules/parent-join/src/main/java/org/elasticsearch/join/mapper/ParentJoinFieldMapper.java @@ -127,6 +127,11 @@ public ParentJoinFieldMapper build(MapperBuilderContext context) { relations.get() ); } + + @Override + public int mapperSize() { + return 1 + relations.get().size(); + } } public static final TypeParser PARSER = new TypeParser((n, c) -> { diff --git a/modules/percolator/src/main/java/org/elasticsearch/percolator/PercolatorFieldMapper.java b/modules/percolator/src/main/java/org/elasticsearch/percolator/PercolatorFieldMapper.java index e212264287937..85aaa22d33f79 100644 --- a/modules/percolator/src/main/java/org/elasticsearch/percolator/PercolatorFieldMapper.java +++ b/modules/percolator/src/main/java/org/elasticsearch/percolator/PercolatorFieldMapper.java @@ -178,6 +178,16 @@ public PercolatorFieldMapper build(MapperBuilderContext context) { ); } + @Override + public int mapperSize() { + return 1 // this + + 1 // queryTermsField, + + 1 // extractionResultField, + + 1 // queryBuilderField, + + 1 // minimumShouldMatchFieldMapper, + + 1; // rangeFieldMapper + } + static KeywordFieldMapper createExtractQueryFieldBuilder( String name, MapperBuilderContext context, diff --git a/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataMappingService.java b/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataMappingService.java index 7a2d20d042f84..0e627456c8409 100644 --- a/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataMappingService.java +++ b/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataMappingService.java @@ -148,7 +148,7 @@ private static ClusterState applyRequest( // try and parse it (no need to add it here) so we can bail early in case of parsing exception // first, simulate: just call merge and ignore the result Mapping mapping = mapperService.parseMapping(MapperService.SINGLE_MAPPING_NAME, mappingUpdateSource); - MapperService.mergeMappings(mapperService.documentMapper(), mapping, MergeReason.MAPPING_UPDATE); + mapperService.mergeMappings(mapping, MergeReason.MAPPING_UPDATE); } Metadata.Builder builder = Metadata.builder(metadata); boolean updated = false; diff --git a/server/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java b/server/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java index 17af6259ca27c..cbbcda1601070 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java @@ -830,7 +830,7 @@ private static class NoOpObjectMapper extends ObjectMapper { } @Override - public ObjectMapper merge(Mapper mergeWith, MapperBuilderContext mapperBuilderContext) { + public ObjectMapper merge(Mapper mergeWith, MapperMergeContext mapperBuilderContext) { return this; } } 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..4fb59474862b2 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/FieldAliasMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/FieldAliasMapper.java @@ -55,7 +55,7 @@ public String path() { } @Override - public Mapper merge(Mapper mergeWith, MapperBuilderContext mapperBuilderContext) { + public Mapper merge(Mapper mergeWith, MapperMergeContext mapperMergeContext) { if ((mergeWith instanceof FieldAliasMapper) == false) { throw new IllegalArgumentException( "Cannot merge a field alias mapping [" + name() + "] with a mapping that is not for a field alias." @@ -155,6 +155,11 @@ public FieldAliasMapper build(MapperBuilderContext context) { String fullName = context.buildFullName(name); return new FieldAliasMapper(name, fullName, path); } + + @Override + public int mapperSize() { + return 1; + } } @Override 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..900f229aa399e 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/FieldMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/FieldMapper.java @@ -372,7 +372,7 @@ private static void checkNestedScopeCompatibility(String source, String target) public abstract Builder getMergeBuilder(); @Override - public final FieldMapper merge(Mapper mergeWith, MapperBuilderContext mapperBuilderContext) { + public final FieldMapper merge(Mapper mergeWith, MapperMergeContext mapperBuilderContext) { if (mergeWith == this) { return this; } @@ -396,7 +396,7 @@ public final FieldMapper merge(Mapper mergeWith, MapperBuilderContext mapperBuil Conflicts conflicts = new Conflicts(name()); builder.merge((FieldMapper) mergeWith, conflicts, mapperBuilderContext); conflicts.check(); - return builder.build(mapperBuilderContext); + return builder.build(mapperBuilderContext.getMapperBuilderContext()); } protected void checkIncomingMergeType(FieldMapper mergeWith) { @@ -454,11 +454,11 @@ public Builder add(FieldMapper mapper) { return this; } - public Builder update(FieldMapper toMerge, MapperBuilderContext context) { + public Builder update(FieldMapper toMerge, MapperMergeContext context) { if (mapperBuilders.containsKey(toMerge.simpleName()) == false) { - add(toMerge); + context.addFieldIfPossible(toMerge, () -> add(toMerge)); } else { - FieldMapper existing = mapperBuilders.get(toMerge.simpleName()).apply(context); + FieldMapper existing = mapperBuilders.get(toMerge.simpleName()).apply(context.getMapperBuilderContext()); add(existing.merge(toMerge, context)); } return this; @@ -481,6 +481,10 @@ public MultiFields build(Mapper.Builder mainFieldBuilder, MapperBuilderContext c return new MultiFields(mappers); } } + + public int mapperSize() { + return mapperBuilders.size(); + } } private final FieldMapper[] mappers; @@ -1220,11 +1224,11 @@ public Builder init(FieldMapper initializer) { return this; } - protected void merge(FieldMapper in, Conflicts conflicts, MapperBuilderContext mapperBuilderContext) { + protected void merge(FieldMapper in, Conflicts conflicts, MapperMergeContext mapperBuilderContext) { for (Parameter param : getParameters()) { param.merge(in, conflicts); } - MapperBuilderContext childContext = mapperBuilderContext.createChildContext(in.simpleName()); + MapperMergeContext childContext = mapperBuilderContext.createChildContext(in.simpleName()); for (FieldMapper newSubField : in.multiFields.mappers) { multiFieldsBuilder.update(newSubField, childContext); } @@ -1407,6 +1411,11 @@ private static boolean isDeprecatedParameter(String propName, IndexVersion index } return DEPRECATED_PARAMS.contains(propName); } + + @Override + public int mapperSize() { + return 1 + multiFieldsBuilder.mapperSize(); + } } public static BiConsumer notInMultiFields(String type) { 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..ef90f7f7da20c 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/Mapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/Mapper.java @@ -37,6 +37,13 @@ public String name() { /** Returns a newly built mapper. */ public abstract Mapper build(MapperBuilderContext context); + + /** + * Returns the size this mapper counts against the {@linkplain MapperService#INDEX_MAPPING_TOTAL_FIELDS_LIMIT_SETTING field limit}. + *

+ * Needs to be in sync with {@link Mapper#mapperSize()} which is based on {@link Mapper#iterator()} + */ + public abstract int mapperSize(); } public interface TypeParser { @@ -73,9 +80,11 @@ public final String simpleName() { */ public abstract String typeName(); - /** Return the merge of {@code mergeWith} into this. - * Both {@code this} and {@code mergeWith} will be left unmodified. */ - public abstract Mapper merge(Mapper mergeWith, MapperBuilderContext mapperBuilderContext); + /** + * Return the merge of {@code mergeWith} into this. + * Both {@code this} and {@code mergeWith} will be left unmodified. + */ + public abstract Mapper merge(Mapper mergeWith, MapperMergeContext mapperBuilderContext); /** * Validate any cross-field references made by this mapper @@ -133,4 +142,18 @@ public static FieldType freezeAndDeduplicateFieldType(FieldType fieldType) { } return fieldTypeDeduplicator.computeIfAbsent(fieldType, Function.identity()); } + + /** + * Returns the size this mapper counts against the {@linkplain MapperService#INDEX_MAPPING_TOTAL_FIELDS_LIMIT_SETTING field limit}. + *

+ * Needs to be in sync with {@link Mapper.Builder#mapperSize()} and {@link MappingLookup#getTotalFieldsCount()}. + */ + public int mapperSize() { + int size = 1; + for (Mapper mapper : this) { + size += mapper.mapperSize(); + } + return size; + } + } diff --git a/server/src/main/java/org/elasticsearch/index/mapper/MapperMergeContext.java b/server/src/main/java/org/elasticsearch/index/mapper/MapperMergeContext.java new file mode 100644 index 0000000000000..b6130a9e5d1f2 --- /dev/null +++ b/server/src/main/java/org/elasticsearch/index/mapper/MapperMergeContext.java @@ -0,0 +1,84 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0 and the Server Side Public License, v 1; you may not use this file except + * in compliance with, at your election, the Elastic License 2.0 or the Server + * Side Public License, v 1. + */ + +package org.elasticsearch.index.mapper; + +import java.util.Map; +import java.util.concurrent.atomic.AtomicLong; + +public class MapperMergeContext { + + private final MapperBuilderContext mapperBuilderContext; + private final AtomicLong remainingFieldsUntilLimit; + + public static MapperMergeContext root(boolean isSourceSynthetic, boolean isDataStream, long maxFieldsToAddDuringMerge) { + return new MapperMergeContext( + MapperBuilderContext.root(isSourceSynthetic, isDataStream), + new AtomicLong(maxFieldsToAddDuringMerge) + ); + } + + public static MapperMergeContext from(MapperBuilderContext mapperBuilderContext, long maxFieldsToAddDuringMerge) { + return new MapperMergeContext(mapperBuilderContext, new AtomicLong(maxFieldsToAddDuringMerge)); + } + + private MapperMergeContext(MapperBuilderContext mapperBuilderContext, AtomicLong remainingFieldsUntilLimit) { + this.mapperBuilderContext = mapperBuilderContext; + this.remainingFieldsUntilLimit = remainingFieldsUntilLimit; + } + + public MapperMergeContext createChildContext(String name) { + return createChildContext(mapperBuilderContext.createChildContext(name)); + } + + public MapperMergeContext createChildContext(MapperBuilderContext childContext) { + return new MapperMergeContext(childContext, remainingFieldsUntilLimit); + } + + public MapperBuilderContext getMapperBuilderContext() { + return mapperBuilderContext; + } + + public void removeRuntimeField(Map runtimeFields, String name) { + if (runtimeFields.containsKey(name)) { + runtimeFields.remove(name); + if (remainingFieldsUntilLimit.get() != Long.MAX_VALUE) { + remainingFieldsUntilLimit.incrementAndGet(); + } + } + } + + public void addRuntimeFieldIfPossible(Map runtimeFields, RuntimeField runtimeField) { + if (runtimeFields.containsKey(runtimeField.name())) { + runtimeFields.put(runtimeField.name(), runtimeField); + } else if (canAddField(1)) { + remainingFieldsUntilLimit.decrementAndGet(); + runtimeFields.put(runtimeField.name(), runtimeField); + } + } + + public boolean addFieldIfPossible(Map mappers, Mapper mapper) { + if (canAddField(mapper.mapperSize())) { + remainingFieldsUntilLimit.getAndAdd(mapper.mapperSize() * -1); + mappers.put(mapper.simpleName(), mapper); + return true; + } + return false; + } + + public void addFieldIfPossible(Mapper mapper, Runnable addField) { + if (canAddField(mapper.mapperSize())) { + remainingFieldsUntilLimit.getAndAdd(mapper.mapperSize() * -1); + addField.run(); + } + } + + public boolean canAddField(int fieldSize) { + return remainingFieldsUntilLimit.get() >= fieldSize; + } +} diff --git a/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java b/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java index cbf2dd872da2f..5e37bb33fcb23 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java @@ -296,7 +296,7 @@ public void updateMapping(final IndexMetadata currentIndexMetadata, final IndexM DocumentMapper previousMapper; synchronized (this) { previousMapper = this.mapper; - assert assertRefreshIsNotNeeded(previousMapper, type, incomingMapping); + assert assertRefreshIsNotNeeded(type, incomingMapping); this.mapper = newDocumentMapper(incomingMapping, MergeReason.MAPPING_RECOVERY, incomingMappingSource); } String op = previousMapper != null ? "updated" : "added"; @@ -310,8 +310,8 @@ public void updateMapping(final IndexMetadata currentIndexMetadata, final IndexM } } - private boolean assertRefreshIsNotNeeded(DocumentMapper currentMapper, String type, Mapping incomingMapping) { - Mapping mergedMapping = mergeMappings(currentMapper, incomingMapping, MergeReason.MAPPING_RECOVERY); + private boolean assertRefreshIsNotNeeded(String type, Mapping incomingMapping) { + Mapping mergedMapping = mergeMappings(incomingMapping, MergeReason.MAPPING_RECOVERY); // skip the runtime section or removed runtime fields will make the assertion fail ToXContent.MapParams params = new ToXContent.MapParams(Collections.singletonMap(RootObjectMapper.TOXCONTENT_SKIP_RUNTIME, "true")); CompressedXContent mergedMappingSource; @@ -515,7 +515,7 @@ public DocumentMapper merge(String type, CompressedXContent mappingSource, Merge private synchronized DocumentMapper doMerge(String type, MergeReason reason, Map mappingSourceAsMap) { Mapping incomingMapping = parseMapping(type, mappingSourceAsMap); - Mapping mapping = mergeMappings(this.mapper, incomingMapping, reason); + Mapping mapping = mergeMappings(incomingMapping, reason); // TODO: In many cases the source here is equal to mappingSource so we need not serialize again. // We should identify these cases reliably and save expensive serialization here DocumentMapper newMapper = newDocumentMapper(mapping, reason, mapping.toCompressedXContent()); @@ -556,12 +556,25 @@ public Mapping parseMapping(String mappingType, Map mappingSourc } } - public static Mapping mergeMappings(DocumentMapper currentMapper, Mapping incomingMapping, MergeReason reason) { + public Mapping mergeMappings(Mapping incomingMapping, MergeReason reason) { + return mergeMappings(this.mapper, incomingMapping, reason, Long.MAX_VALUE); + } + + static Mapping mergeMappings( + DocumentMapper currentMapper, + Mapping incomingMapping, + MergeReason reason, + long maxFieldsToAddDuringMerge + ) { Mapping newMapping; if (currentMapper == null) { - newMapping = incomingMapping; + if (MappingLookup.fromMapping(incomingMapping).exceedsLimit(maxFieldsToAddDuringMerge, 0)) { + newMapping = Mapping.EMPTY.merge(incomingMapping, reason, maxFieldsToAddDuringMerge); + } else { + newMapping = incomingMapping; + } } else { - newMapping = currentMapper.mapping().merge(incomingMapping, reason); + newMapping = currentMapper.mapping().merge(incomingMapping, reason, maxFieldsToAddDuringMerge); } return newMapping; } diff --git a/server/src/main/java/org/elasticsearch/index/mapper/Mapping.java b/server/src/main/java/org/elasticsearch/index/mapper/Mapping.java index fb07ddbc56d83..6469dc58e4784 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/Mapping.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/Mapping.java @@ -135,8 +135,9 @@ public SourceLoader.SyntheticFieldLoader syntheticFieldLoader() { * @param reason the reason this merge was initiated. * @return the resulting merged mapping. */ - Mapping merge(Mapping mergeWith, MergeReason reason) { - RootObjectMapper mergedRoot = root.merge(mergeWith.root, reason, MapperBuilderContext.root(isSourceSynthetic(), false)); + Mapping merge(Mapping mergeWith, MergeReason reason, long maxFieldsToAddDuringMerge) { + MapperMergeContext mergeContext = MapperMergeContext.root(isSourceSynthetic(), false, maxFieldsToAddDuringMerge); + RootObjectMapper mergedRoot = root.merge(mergeWith.root, reason, mergeContext); // When merging metadata fields as part of applying an index template, new field definitions // completely overwrite existing ones instead of being merged. This behavior matches how we @@ -148,7 +149,7 @@ Mapping merge(Mapping mergeWith, MergeReason reason) { if (mergeInto == null || reason == MergeReason.INDEX_TEMPLATE) { merged = metaMergeWith; } else { - merged = (MetadataFieldMapper) mergeInto.merge(metaMergeWith, MapperBuilderContext.root(isSourceSynthetic(), false)); + merged = (MetadataFieldMapper) mergeInto.merge(metaMergeWith, mergeContext); } mergedMetadataMappers.put(merged.getClass(), merged); } 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..0172c22c0b176 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/MappingLookup.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/MappingLookup.java @@ -271,7 +271,7 @@ private void checkFieldLimit(long limit) { } void checkFieldLimit(long limit, int additionalFieldsToAdd) { - if (getTotalFieldsCount() + additionalFieldsToAdd - mapping.getSortedMetadataMappers().length > limit) { + if (exceedsLimit(limit, additionalFieldsToAdd)) { throw new IllegalArgumentException( "Limit of total fields [" + limit @@ -281,6 +281,10 @@ void checkFieldLimit(long limit, int additionalFieldsToAdd) { } } + boolean exceedsLimit(long limit, int additionalFieldsToAdd) { + return getTotalFieldsCount() + additionalFieldsToAdd - mapping.getSortedMetadataMappers().length > limit; + } + private void checkDimensionFieldLimit(long limit) { long dimensionFieldCount = fieldMappers.values() .stream() diff --git a/server/src/main/java/org/elasticsearch/index/mapper/NestedObjectMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/NestedObjectMapper.java index 257b2270176bc..507ab3fe52366 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/NestedObjectMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/NestedObjectMapper.java @@ -186,6 +186,20 @@ public ObjectMapper.Builder newBuilder(IndexVersion indexVersionCreated) { return builder; } + public NestedObjectMapper withoutMappers() { + return new NestedObjectMapper( + simpleName(), + fullPath(), + null, + enabled, + dynamic, + includeInParent, + includeInRoot, + nestedTypePath, + nestedTypeFilter + ); + } + @Override public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { builder.startObject(simpleName()); @@ -207,12 +221,12 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws } @Override - public ObjectMapper merge(Mapper mergeWith, MapperService.MergeReason reason, MapperBuilderContext parentBuilderContext) { + public ObjectMapper merge(Mapper mergeWith, MapperService.MergeReason reason, MapperMergeContext parentMergeContext) { if ((mergeWith instanceof NestedObjectMapper) == false) { MapperErrors.throwNestedMappingConflictError(mergeWith.name()); } NestedObjectMapper mergeWithObject = (NestedObjectMapper) mergeWith; - var mergeResult = MergeResult.build(this, mergeWith, reason, parentBuilderContext); + var mergeResult = MergeResult.build(this, mergeWith, reason, parentMergeContext); Explicit incInParent = this.includeInParent; Explicit incInRoot = this.includeInRoot; if (reason == MapperService.MergeReason.INDEX_TEMPLATE) { @@ -230,6 +244,7 @@ public ObjectMapper merge(Mapper mergeWith, MapperService.MergeReason reason, Ma throw new MapperException("the [include_in_root] parameter can't be updated on a nested object mapping"); } } + MapperBuilderContext parentBuilderContext = parentMergeContext.getMapperBuilderContext(); if (parentBuilderContext instanceof NestedMapperBuilderContext nc) { if (nc.parentIncludedInRoot && incInParent.value()) { incInRoot = Explicit.IMPLICIT_FALSE; @@ -253,12 +268,15 @@ public ObjectMapper merge(Mapper mergeWith, MapperService.MergeReason reason, Ma } @Override - protected MapperBuilderContext createChildContext(MapperBuilderContext mapperBuilderContext, String name) { + protected MapperMergeContext createChildContext(MapperMergeContext mapperMergeContext, String name) { + MapperBuilderContext mapperBuilderContext = mapperMergeContext.getMapperBuilderContext(); boolean parentIncludedInRoot = this.includeInRoot.value(); if (mapperBuilderContext instanceof NestedMapperBuilderContext == false) { parentIncludedInRoot |= this.includeInParent.value(); } - return new NestedMapperBuilderContext(mapperBuilderContext.buildFullName(name), parentIncludedInRoot); + return mapperMergeContext.createChildContext( + new NestedMapperBuilderContext(mapperBuilderContext.buildFullName(name), parentIncludedInRoot) + ); } @Override 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..664c2f54ed195 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java @@ -24,10 +24,11 @@ import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; +import java.util.Collections; import java.util.Comparator; -import java.util.HashMap; import java.util.HashSet; import java.util.Iterator; +import java.util.LinkedHashMap; import java.util.List; import java.util.Locale; import java.util.Map; @@ -156,7 +157,7 @@ private ObjectMapper.Builder findObjectBuilder(String leafName, String fullName, } protected final Map buildMappers(MapperBuilderContext mapperBuilderContext) { - Map mappers = new HashMap<>(); + Map mappers = new LinkedHashMap<>(); for (Mapper.Builder builder : mappersBuilders) { Mapper mapper = builder.build(mapperBuilderContext); assert mapper instanceof ObjectMapper == false || subobjects.value() : "unexpected object while subobjects are disabled"; @@ -167,7 +168,7 @@ protected final Map buildMappers(MapperBuilderContext mapperBuil // This can also happen due to multiple index templates being merged into a single mappings definition using // XContentHelper#mergeDefaults, again in case some index templates contained mappings for the same field using a // mix of object notation and dot notation. - mapper = existing.merge(mapper, mapperBuilderContext); + mapper = existing.merge(mapper, MapperMergeContext.from(mapperBuilderContext, Long.MAX_VALUE)); } mappers.put(mapper.simpleName(), mapper); } @@ -185,6 +186,11 @@ public ObjectMapper build(MapperBuilderContext context) { buildMappers(context.createChildContext(name)) ); } + + @Override + public int mapperSize() { + return 1 + mappersBuilders.stream().mapToInt(Mapper.Builder::mapperSize).sum(); + } } public static class TypeParser implements Mapper.TypeParser { @@ -394,7 +400,7 @@ private static void validateFieldName(String fieldName, IndexVersion indexCreate if (mappers == null) { this.mappers = Map.of(); } else { - this.mappers = Map.copyOf(mappers); + this.mappers = Collections.unmodifiableMap(mappers); } } @@ -408,6 +414,10 @@ public ObjectMapper.Builder newBuilder(IndexVersion indexVersionCreated) { return builder; } + public ObjectMapper withoutMappers() { + return new ObjectMapper(simpleName(), fullPath, enabled, subobjects, dynamic, null); + } + @Override public String name() { return this.fullPath; @@ -448,7 +458,7 @@ public final boolean subobjects() { } @Override - public ObjectMapper merge(Mapper mergeWith, MapperBuilderContext mapperBuilderContext) { + public ObjectMapper merge(Mapper mergeWith, MapperMergeContext mapperBuilderContext) { return merge(mergeWith, MergeReason.MAPPING_UPDATE, mapperBuilderContext); } @@ -459,11 +469,11 @@ public void validate(MappingLookup mappers) { } } - protected MapperBuilderContext createChildContext(MapperBuilderContext mapperBuilderContext, String name) { + protected MapperMergeContext createChildContext(MapperMergeContext mapperBuilderContext, String name) { return mapperBuilderContext.createChildContext(name); } - public ObjectMapper merge(Mapper mergeWith, MergeReason reason, MapperBuilderContext parentBuilderContext) { + public ObjectMapper merge(Mapper mergeWith, MergeReason reason, MapperMergeContext parentBuilderContext) { var mergeResult = MergeResult.build(this, mergeWith, reason, parentBuilderContext); return new ObjectMapper( simpleName(), @@ -486,7 +496,7 @@ public static MergeResult build( ObjectMapper existing, Mapper mergeWith, MergeReason reason, - MapperBuilderContext parentBuilderContext + MapperMergeContext parentMergeContext ) { if ((mergeWith instanceof ObjectMapper) == false) { MapperErrors.throwObjectMappingConflictError(mergeWith.name()); @@ -522,8 +532,8 @@ public static MergeResult build( } else { subObjects = existing.subobjects; } - MapperBuilderContext objectBuilderContext = existing.createChildContext(parentBuilderContext, existing.simpleName()); - Map mergedMappers = buildMergedMappers(existing, mergeWith, reason, objectBuilderContext); + MapperMergeContext objectMergeContext = existing.createChildContext(parentMergeContext, existing.simpleName()); + Map mergedMappers = buildMergedMappers(existing, mergeWith, reason, objectMergeContext); return new MergeResult( enabled, subObjects, @@ -536,17 +546,25 @@ private static Map buildMergedMappers( ObjectMapper existing, Mapper mergeWith, MergeReason reason, - MapperBuilderContext objectBuilderContext + MapperMergeContext objectMergeContext ) { - Map mergedMappers = null; + // need to preserve order to make it deterministic which fields are ignored + Map mergedMappers = new LinkedHashMap<>(existing.mappers); for (Mapper mergeWithMapper : mergeWith) { - Mapper mergeIntoMapper = (mergedMappers == null ? existing.mappers : mergedMappers).get(mergeWithMapper.simpleName()); + Mapper mergeIntoMapper = mergedMappers.get(mergeWithMapper.simpleName()); - Mapper merged; if (mergeIntoMapper == null) { - merged = mergeWithMapper; + if (mergeWithMapper instanceof ObjectMapper om) { + ObjectMapper withoutMappers = om.withoutMappers(); + boolean added = objectMergeContext.addFieldIfPossible(mergedMappers, withoutMappers); + if (added) { + mergedMappers.put(om.simpleName(), withoutMappers.merge(mergeWithMapper, reason, objectMergeContext)); + } + } else { + objectMergeContext.addFieldIfPossible(mergedMappers, mergeWithMapper); + } } else if (mergeIntoMapper instanceof ObjectMapper objectMapper) { - merged = objectMapper.merge(mergeWithMapper, reason, objectBuilderContext); + mergedMappers.put(objectMapper.simpleName(), objectMapper.merge(mergeWithMapper, reason, objectMergeContext)); } else { assert mergeIntoMapper instanceof FieldMapper || mergeIntoMapper instanceof FieldAliasMapper; if (mergeWithMapper instanceof NestedObjectMapper) { @@ -558,22 +576,13 @@ private static Map buildMergedMappers( // If we're merging template mappings when creating an index, then a field definition always // replaces an existing one. if (reason == MergeReason.INDEX_TEMPLATE) { - merged = mergeWithMapper; + mergedMappers.put(mergeWithMapper.simpleName(), mergeWithMapper); } else { - merged = mergeIntoMapper.merge(mergeWithMapper, objectBuilderContext); + mergedMappers.put(mergeWithMapper.simpleName(), mergeIntoMapper.merge(mergeWithMapper, objectMergeContext)); } } - if (mergedMappers == null) { - mergedMappers = new HashMap<>(existing.mappers); - } - mergedMappers.put(merged.simpleName(), merged); - } - if (mergedMappers != null) { - mergedMappers = Map.copyOf(mergedMappers); - } else { - mergedMappers = Map.copyOf(existing.mappers); } - return mergedMappers; + return Collections.unmodifiableMap(mergedMappers); } } diff --git a/server/src/main/java/org/elasticsearch/index/mapper/ParsedDocument.java b/server/src/main/java/org/elasticsearch/index/mapper/ParsedDocument.java index 05f05dd5be941..014be192a4133 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/ParsedDocument.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/ParsedDocument.java @@ -160,7 +160,7 @@ public void addDynamicMappingsUpdate(Mapping update) { if (dynamicMappingsUpdate == null) { dynamicMappingsUpdate = update; } else { - dynamicMappingsUpdate = dynamicMappingsUpdate.merge(update, MergeReason.MAPPING_UPDATE); + dynamicMappingsUpdate = dynamicMappingsUpdate.merge(update, MergeReason.MAPPING_UPDATE, Long.MAX_VALUE); } } diff --git a/server/src/main/java/org/elasticsearch/index/mapper/PlaceHolderFieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/PlaceHolderFieldMapper.java index 4ad0873b66d50..b1226e0bee41d 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/PlaceHolderFieldMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/PlaceHolderFieldMapper.java @@ -63,7 +63,7 @@ public FieldMapper.Builder init(FieldMapper initializer) { } @Override - protected void merge(FieldMapper in, Conflicts conflicts, MapperBuilderContext mapperBuilderContext) { + protected void merge(FieldMapper in, Conflicts conflicts, MapperMergeContext mapperBuilderContext) { assert in instanceof PlaceHolderFieldMapper; unknownParams.putAll(((PlaceHolderFieldMapper) in).unknownParams); super.merge(in, conflicts, mapperBuilderContext); 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..05e8f850ecb5f 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/RootObjectMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/RootObjectMapper.java @@ -119,6 +119,11 @@ public RootObjectMapper build(MapperBuilderContext context) { numericDetection ); } + + @Override + public int mapperSize() { + return mappersBuilders.stream().mapToInt(Mapper.Builder::mapperSize).sum() + runtimeFields.size(); + } } private final Explicit dynamicDateTimeFormatters; @@ -155,6 +160,21 @@ public RootObjectMapper.Builder newBuilder(IndexVersion indexVersionCreated) { return builder; } + public RootObjectMapper withoutMappers() { + return new RootObjectMapper( + simpleName(), + enabled, + subobjects, + dynamic, + null, + null, + dynamicDateTimeFormatters, + dynamicTemplates, + dateDetection, + numericDetection + ); + } + /** * Public API */ @@ -192,13 +212,13 @@ RuntimeField getRuntimeField(String name) { } @Override - protected MapperBuilderContext createChildContext(MapperBuilderContext mapperBuilderContext, String name) { - assert Objects.equals(mapperBuilderContext.buildFullName("foo"), "foo"); - return mapperBuilderContext; + protected MapperMergeContext createChildContext(MapperMergeContext mapperMergeContext, String name) { + assert Objects.equals(mapperMergeContext.getMapperBuilderContext().buildFullName("foo"), "foo"); + return mapperMergeContext; } @Override - public RootObjectMapper merge(Mapper mergeWith, MergeReason reason, MapperBuilderContext parentBuilderContext) { + public RootObjectMapper merge(Mapper mergeWith, MergeReason reason, MapperMergeContext parentBuilderContext) { final var mergeResult = MergeResult.build(this, mergeWith, reason, parentBuilderContext); final Explicit numericDetection; RootObjectMapper mergeWithObject = (RootObjectMapper) mergeWith; @@ -245,9 +265,9 @@ public RootObjectMapper merge(Mapper mergeWith, MergeReason reason, MapperBuilde assert this.runtimeFields != mergeWithObject.runtimeFields; for (Map.Entry runtimeField : mergeWithObject.runtimeFields.entrySet()) { if (runtimeField.getValue() == null) { - runtimeFields.remove(runtimeField.getKey()); + parentBuilderContext.removeRuntimeField(runtimeFields, runtimeField.getKey()); } else { - runtimeFields.put(runtimeField.getKey(), runtimeField.getValue()); + parentBuilderContext.addRuntimeFieldIfPossible(runtimeFields, runtimeField.getValue()); } } @@ -502,4 +522,13 @@ private static boolean processField( } return false; } + + @Override + public int mapperSize() { + int size = 0; + for (Mapper mapper : this) { + size += mapper.mapperSize(); + } + return size; + } } diff --git a/server/src/test/java/org/elasticsearch/index/mapper/DocumentMapperTests.java b/server/src/test/java/org/elasticsearch/index/mapper/DocumentMapperTests.java index f0458add93c78..e935ae2431131 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/DocumentMapperTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/DocumentMapperTests.java @@ -63,7 +63,7 @@ public void testAddFields() throws Exception { })); MergeReason reason = randomFrom(MergeReason.MAPPING_UPDATE, MergeReason.INDEX_TEMPLATE); - Mapping merged = MapperService.mergeMappings(stage1, stage2.mapping(), reason); + Mapping merged = MapperService.mergeMappings(stage1, stage2.mapping(), reason, Long.MAX_VALUE); // stage1 mapping should not have been modified assertThat(stage1.mappers().getMapper("age"), nullValue()); assertThat(stage1.mappers().getMapper("obj1.prop1"), nullValue()); @@ -81,7 +81,7 @@ public void testMergeObjectDynamic() throws Exception { DocumentMapper withDynamicMapper = createDocumentMapper(topMapping(b -> b.field("dynamic", "false"))); assertThat(withDynamicMapper.mapping().getRoot().dynamic(), equalTo(ObjectMapper.Dynamic.FALSE)); - Mapping merged = MapperService.mergeMappings(mapper, withDynamicMapper.mapping(), MergeReason.MAPPING_UPDATE); + Mapping merged = MapperService.mergeMappings(mapper, withDynamicMapper.mapping(), MergeReason.MAPPING_UPDATE, Long.MAX_VALUE); assertThat(merged.getRoot().dynamic(), equalTo(ObjectMapper.Dynamic.FALSE)); } @@ -93,14 +93,14 @@ public void testMergeObjectAndNested() throws Exception { { IllegalArgumentException e = expectThrows( IllegalArgumentException.class, - () -> MapperService.mergeMappings(objectMapper, nestedMapper.mapping(), reason) + () -> MapperService.mergeMappings(objectMapper, nestedMapper.mapping(), reason, Long.MAX_VALUE) ); assertThat(e.getMessage(), containsString("can't merge a non-nested mapping [obj] with a nested mapping")); } { IllegalArgumentException e = expectThrows( IllegalArgumentException.class, - () -> MapperService.mergeMappings(nestedMapper, objectMapper.mapping(), reason) + () -> MapperService.mergeMappings(nestedMapper, objectMapper.mapping(), reason, Long.MAX_VALUE) ); assertThat(e.getMessage(), containsString("can't merge a non-nested mapping [obj] with a nested mapping")); } @@ -235,11 +235,11 @@ public void testMergeMeta() throws IOException { DocumentMapper updatedMapper = createDocumentMapper(fieldMapping(b -> b.field("type", "text"))); - Mapping merged = MapperService.mergeMappings(initMapper, updatedMapper.mapping(), MergeReason.MAPPING_UPDATE); + Mapping merged = MapperService.mergeMappings(initMapper, updatedMapper.mapping(), MergeReason.MAPPING_UPDATE, Long.MAX_VALUE); assertThat(merged.getMeta().get("foo"), equalTo("bar")); updatedMapper = createDocumentMapper(topMapping(b -> b.startObject("_meta").field("foo", "new_bar").endObject())); - merged = MapperService.mergeMappings(initMapper, updatedMapper.mapping(), MergeReason.MAPPING_UPDATE); + merged = MapperService.mergeMappings(initMapper, updatedMapper.mapping(), MergeReason.MAPPING_UPDATE, Long.MAX_VALUE); assertThat(merged.getMeta().get("foo"), equalTo("new_bar")); } @@ -262,7 +262,7 @@ public void testMergeMetaForIndexTemplate() throws IOException { assertThat(initMapper.mapping().getMeta(), equalTo(expected)); DocumentMapper updatedMapper = createDocumentMapper(fieldMapping(b -> b.field("type", "text"))); - Mapping merged = MapperService.mergeMappings(initMapper, updatedMapper.mapping(), MergeReason.INDEX_TEMPLATE); + Mapping merged = MapperService.mergeMappings(initMapper, updatedMapper.mapping(), MergeReason.INDEX_TEMPLATE, Long.MAX_VALUE); assertThat(merged.getMeta(), equalTo(expected)); updatedMapper = createDocumentMapper(topMapping(b -> { @@ -278,7 +278,7 @@ public void testMergeMetaForIndexTemplate() throws IOException { } b.endObject(); })); - merged = merged.merge(updatedMapper.mapping(), MergeReason.INDEX_TEMPLATE); + merged = merged.merge(updatedMapper.mapping(), MergeReason.INDEX_TEMPLATE, Long.MAX_VALUE); expected = Map.of("field", "value", "object", Map.of("field1", "value1", "field2", "new_value", "field3", "value3")); assertThat(merged.getMeta(), equalTo(expected)); diff --git a/server/src/test/java/org/elasticsearch/index/mapper/DocumentParserTests.java b/server/src/test/java/org/elasticsearch/index/mapper/DocumentParserTests.java index a3706b7ddab18..5bad55ecbe57e 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/DocumentParserTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/DocumentParserTests.java @@ -2580,7 +2580,7 @@ same name need to be part of the same mappings (hence the same document). If th assertArrayEquals(new String[] { "LongField ", "LongField " }, fieldStrings); // merge without going through toXContent and reparsing, otherwise the potential leaf path issue gets fixed on its own - Mapping newMapping = MapperService.mergeMappings(mapperService.documentMapper(), mapping, MapperService.MergeReason.MAPPING_UPDATE); + Mapping newMapping = mapperService.mergeMappings(mapping, MapperService.MergeReason.MAPPING_UPDATE); DocumentMapper newDocMapper = new DocumentMapper( mapperService.documentParser(), newMapping, 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 07f4c3c1346c4..52dd48ea3e6a2 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/FieldAliasMapperTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/FieldAliasMapperTests.java @@ -9,6 +9,7 @@ package org.elasticsearch.index.mapper; import org.elasticsearch.common.Strings; +import org.elasticsearch.xcontent.XContentBuilder; import org.elasticsearch.xcontent.XContentFactory; import java.io.IOException; @@ -16,24 +17,25 @@ public class FieldAliasMapperTests extends MapperServiceTestCase { public void testParsing() throws IOException { - String mapping = Strings.toString( - XContentFactory.jsonBuilder() - .startObject() - .startObject("_doc") - .startObject("properties") - .startObject("alias-field") - .field("type", "alias") - .field("path", "concrete-field") - .endObject() - .startObject("concrete-field") - .field("type", "keyword") - .endObject() - .endObject() - .endObject() - .endObject() - ); + XContentBuilder mappingXContent = XContentFactory.jsonBuilder() + .startObject() + .startObject("_doc") + .startObject("properties") + .startObject("alias-field") + .field("type", "alias") + .field("path", "concrete-field") + .endObject() + .startObject("concrete-field") + .field("type", "keyword") + .endObject() + .endObject() + .endObject() + .endObject(); + String mapping = Strings.toString(mappingXContent); DocumentMapper mapper = createDocumentMapper(mapping); assertEquals(mapping, mapper.mappingSource().toString()); + RootObjectMapper.Builder builder = getRootObjectMapperBuilder(mappingXContent); + assertEquals(2, builder.mapperSize()); } public void testParsingWithMissingPath() { diff --git a/server/src/test/java/org/elasticsearch/index/mapper/GeoShapeFieldMapperTests.java b/server/src/test/java/org/elasticsearch/index/mapper/GeoShapeFieldMapperTests.java index 92da99bc059a2..bae6f9cb08ca1 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/GeoShapeFieldMapperTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/GeoShapeFieldMapperTests.java @@ -63,6 +63,12 @@ protected Object getSampleValueForDocument() { return "POINT (14.0 15.0)"; } + @Override + public void testMapperBuilderSizeMultiField() throws IOException { + super.testMapperBuilderSizeMultiField(); + assertWarnings("Adding multifields to [geo_shape] mappers has no effect and will be forbidden in future"); + } + public void testDefaultConfiguration() throws IOException { DocumentMapper mapper = createDocumentMapper(fieldMapping(this::minimalMapping)); Mapper fieldMapper = mapper.mappers().getMapper("field"); diff --git a/server/src/test/java/org/elasticsearch/index/mapper/MapperMergeContextTests.java b/server/src/test/java/org/elasticsearch/index/mapper/MapperMergeContextTests.java new file mode 100644 index 0000000000000..18e34963d4a9e --- /dev/null +++ b/server/src/test/java/org/elasticsearch/index/mapper/MapperMergeContextTests.java @@ -0,0 +1,69 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0 and the Server Side Public License, v 1; you may not use this file except + * in compliance with, at your election, the Elastic License 2.0 or the Server + * Side Public License, v 1. + */ + +package org.elasticsearch.index.mapper; + +import org.elasticsearch.index.IndexVersion; +import org.elasticsearch.test.ESTestCase; + +import java.util.HashMap; + +public class MapperMergeContextTests extends ESTestCase { + + public void testAddFieldIfPossibleUnderLimit() { + MapperMergeContext context = MapperMergeContext.root(false, false, 1); + assertTrue(context.canAddField(1)); + HashMap mappers = new HashMap<>(); + context.addFieldIfPossible(mappers, getKeywordFieldMapper()); + assertEquals(1, mappers.size()); + assertFalse(context.canAddField(1)); + } + + public void testAddFieldIfPossibleAtLimit() { + MapperMergeContext context = MapperMergeContext.root(false, false, 0); + assertFalse(context.canAddField(1)); + HashMap mappers = new HashMap<>(); + context.addFieldIfPossible(mappers, getKeywordFieldMapper()); + assertEquals(0, mappers.size()); + assertFalse(context.canAddField(1)); + } + + public void testAddRuntimeFieldIfPossibleUnderLimit() { + MapperMergeContext context = MapperMergeContext.root(false, false, 1); + assertTrue(context.canAddField(1)); + HashMap runtimeFields = new HashMap<>(); + context.addRuntimeFieldIfPossible(runtimeFields, new TestRuntimeField("foo", "keyword")); + assertEquals(1, runtimeFields.size()); + assertFalse(context.canAddField(1)); + } + + public void testAddRuntimeFieldIfPossibleAtLimit() { + MapperMergeContext context = MapperMergeContext.root(false, false, 0); + assertFalse(context.canAddField(1)); + HashMap runtimeFields = new HashMap<>(); + context.addRuntimeFieldIfPossible(runtimeFields, new TestRuntimeField("foo", "keyword")); + assertEquals(0, runtimeFields.size()); + assertFalse(context.canAddField(1)); + } + + public void testRemoveRuntimeField() { + MapperMergeContext context = MapperMergeContext.root(false, false, 1); + HashMap runtimeFields = new HashMap<>(); + context.addRuntimeFieldIfPossible(runtimeFields, new TestRuntimeField("foo", "keyword")); + assertEquals(1, runtimeFields.size()); + assertFalse(context.canAddField(1)); + + context.removeRuntimeField(runtimeFields, "foo"); + assertTrue(context.canAddField(1)); + } + + private static KeywordFieldMapper getKeywordFieldMapper() { + return new KeywordFieldMapper.Builder("foo", IndexVersion.current()).build(MapperBuilderContext.root(false, false)); + } + +} diff --git a/server/src/test/java/org/elasticsearch/index/mapper/NestedObjectMapperTests.java b/server/src/test/java/org/elasticsearch/index/mapper/NestedObjectMapperTests.java index 621f03813b1ff..f4ed4b8620306 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/NestedObjectMapperTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/NestedObjectMapperTests.java @@ -1508,14 +1508,14 @@ public void testMergeNested() { MapperException e = expectThrows( MapperException.class, - () -> firstMapper.merge(secondMapper, MapperBuilderContext.root(false, false)) + () -> firstMapper.merge(secondMapper, MapperMergeContext.root(false, false, Long.MAX_VALUE)) ); assertThat(e.getMessage(), containsString("[include_in_parent] parameter can't be updated on a nested object mapping")); NestedObjectMapper result = (NestedObjectMapper) firstMapper.merge( secondMapper, MapperService.MergeReason.INDEX_TEMPLATE, - MapperBuilderContext.root(false, false) + MapperMergeContext.root(false, false, Long.MAX_VALUE) ); assertFalse(result.isIncludeInParent()); assertTrue(result.isIncludeInRoot()); diff --git a/server/src/test/java/org/elasticsearch/index/mapper/ObjectMapperMergeTests.java b/server/src/test/java/org/elasticsearch/index/mapper/ObjectMapperMergeTests.java index 559eb4712d7c1..45d0ea4c2eed1 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/ObjectMapperMergeTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/ObjectMapperMergeTests.java @@ -41,7 +41,7 @@ public void testMerge() { ObjectMapper mergeWith = createMapping(false, true, true, true); // WHEN merging mappings - final ObjectMapper merged = rootObjectMapper.merge(mergeWith, MapperBuilderContext.root(false, false)); + final ObjectMapper merged = rootObjectMapper.merge(mergeWith, MapperMergeContext.root(false, false, Long.MAX_VALUE)); // THEN "baz" new field is added to merged mapping final ObjectMapper mergedFoo = (ObjectMapper) merged.getMapper("foo"); @@ -63,7 +63,7 @@ public void testMergeWhenDisablingField() { // THEN a MapperException is thrown with an excepted message MapperException e = expectThrows( MapperException.class, - () -> rootObjectMapper.merge(mergeWith, MapperBuilderContext.root(false, false)) + () -> rootObjectMapper.merge(mergeWith, MapperMergeContext.root(false, false, Long.MAX_VALUE)) ); assertEquals("the [enabled] parameter can't be updated for the object mapping [foo]", e.getMessage()); } @@ -75,7 +75,10 @@ public void testMergeDisabledField() { new ObjectMapper.Builder("disabled", Explicit.IMPLICIT_TRUE) ).build(MapperBuilderContext.root(false, false)); - RootObjectMapper merged = (RootObjectMapper) rootObjectMapper.merge(mergeWith, MapperBuilderContext.root(false, false)); + RootObjectMapper merged = (RootObjectMapper) rootObjectMapper.merge( + mergeWith, + MapperMergeContext.root(false, false, Long.MAX_VALUE) + ); assertFalse(((ObjectMapper) merged.getMapper("disabled")).isEnabled()); } @@ -84,14 +87,14 @@ public void testMergeEnabled() { MapperException e = expectThrows( MapperException.class, - () -> rootObjectMapper.merge(mergeWith, MapperBuilderContext.root(false, false)) + () -> rootObjectMapper.merge(mergeWith, MapperMergeContext.root(false, false, Long.MAX_VALUE)) ); assertEquals("the [enabled] parameter can't be updated for the object mapping [disabled]", e.getMessage()); ObjectMapper result = rootObjectMapper.merge( mergeWith, MapperService.MergeReason.INDEX_TEMPLATE, - MapperBuilderContext.root(false, false) + MapperMergeContext.root(false, false, Long.MAX_VALUE) ); assertTrue(result.isEnabled()); } @@ -106,14 +109,14 @@ public void testMergeEnabledForRootMapper() { MapperException e = expectThrows( MapperException.class, - () -> firstMapper.merge(secondMapper, MapperBuilderContext.root(false, false)) + () -> firstMapper.merge(secondMapper, MapperMergeContext.root(false, false, Long.MAX_VALUE)) ); assertEquals("the [enabled] parameter can't be updated for the object mapping [" + type + "]", e.getMessage()); ObjectMapper result = firstMapper.merge( secondMapper, MapperService.MergeReason.INDEX_TEMPLATE, - MapperBuilderContext.root(false, false) + MapperMergeContext.root(false, false, Long.MAX_VALUE) ); assertFalse(result.isEnabled()); } @@ -128,7 +131,10 @@ public void testMergeDisabledRootMapper() { Collections.singletonMap("test", new TestRuntimeField("test", "long")) ).build(MapperBuilderContext.root(false, false)); - RootObjectMapper merged = (RootObjectMapper) rootObjectMapper.merge(mergeWith, MapperBuilderContext.root(false, false)); + RootObjectMapper merged = (RootObjectMapper) rootObjectMapper.merge( + mergeWith, + MapperMergeContext.root(false, false, Long.MAX_VALUE) + ); assertFalse(merged.isEnabled()); assertEquals(1, merged.runtimeFields().size()); assertEquals("test", merged.runtimeFields().iterator().next().name()); @@ -138,7 +144,7 @@ public void testMergedFieldNamesFieldWithDotsSubobjectsFalseAtRoot() { RootObjectMapper mergeInto = createRootSubobjectFalseLeafWithDots(); RootObjectMapper mergeWith = createRootSubobjectFalseLeafWithDots(); - final ObjectMapper merged = mergeInto.merge(mergeWith, MapperBuilderContext.root(false, false)); + final ObjectMapper merged = mergeInto.merge(mergeWith, MapperMergeContext.root(false, false, Long.MAX_VALUE)); final KeywordFieldMapper keywordFieldMapper = (KeywordFieldMapper) merged.getMapper("host.name"); assertEquals("host.name", keywordFieldMapper.name()); @@ -153,7 +159,7 @@ public void testMergedFieldNamesFieldWithDotsSubobjectsFalse() { createObjectSubobjectsFalseLeafWithDots() ).build(MapperBuilderContext.root(false, false)); - final ObjectMapper merged = mergeInto.merge(mergeWith, MapperBuilderContext.root(false, false)); + final ObjectMapper merged = mergeInto.merge(mergeWith, MapperMergeContext.root(false, false, Long.MAX_VALUE)); ObjectMapper foo = (ObjectMapper) merged.getMapper("foo"); ObjectMapper metrics = (ObjectMapper) foo.getMapper("metrics"); @@ -168,7 +174,7 @@ public void testMergedFieldNamesMultiFields() { RootObjectMapper mergeWith = new RootObjectMapper.Builder("_doc", Explicit.IMPLICIT_TRUE).add(createTextKeywordMultiField("text")) .build(MapperBuilderContext.root(false, false)); - final ObjectMapper merged = mergeInto.merge(mergeWith, MapperBuilderContext.root(false, false)); + final ObjectMapper merged = mergeInto.merge(mergeWith, MapperMergeContext.root(false, false, Long.MAX_VALUE)); TextFieldMapper text = (TextFieldMapper) merged.getMapper("text"); assertEquals("text", text.name()); @@ -186,7 +192,7 @@ public void testMergedFieldNamesMultiFieldsWithinSubobjectsFalse() { createObjectSubobjectsFalseLeafWithMultiField() ).build(MapperBuilderContext.root(false, false)); - final ObjectMapper merged = mergeInto.merge(mergeWith, MapperBuilderContext.root(false, false)); + final ObjectMapper merged = mergeInto.merge(mergeWith, MapperMergeContext.root(false, false, Long.MAX_VALUE)); ObjectMapper foo = (ObjectMapper) merged.getMapper("foo"); ObjectMapper metrics = (ObjectMapper) foo.getMapper("metrics"); 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..3c47b8159fb26 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; @@ -120,7 +122,7 @@ public void testMerge() throws IOException { "_doc", new CompressedXContent(BytesReference.bytes(topMapping(b -> b.field("dynamic", "strict")))) ); - Mapping merged = mapper.mapping().merge(mergeWith, reason); + Mapping merged = mapper.mapping().merge(mergeWith, reason, Long.MAX_VALUE); assertEquals(Dynamic.STRICT, merged.getRoot().dynamic()); } @@ -465,7 +467,7 @@ public void testSubobjectsCannotBeUpdated() throws IOException { })))); MapperException exception = expectThrows( MapperException.class, - () -> mapper.mapping().merge(mergeWith, MergeReason.MAPPING_UPDATE) + () -> mapper.mapping().merge(mergeWith, MergeReason.MAPPING_UPDATE, Long.MAX_VALUE) ); assertEquals("the [subobjects] parameter can't be updated for the object mapping [field]", exception.getMessage()); } @@ -479,7 +481,7 @@ public void testSubobjectsCannotBeUpdatedOnRoot() throws IOException { })))); MapperException exception = expectThrows( MapperException.class, - () -> mapper.mapping().merge(mergeWith, MergeReason.MAPPING_UPDATE) + () -> mapper.mapping().merge(mergeWith, MergeReason.MAPPING_UPDATE, Long.MAX_VALUE) ); assertEquals("the [subobjects] parameter can't be updated for the object mapping [_doc]", exception.getMessage()); } @@ -522,4 +524,16 @@ public void testSyntheticSourceDocValuesFieldWithout() throws IOException { assertThat(o.syntheticFieldLoader().docValuesLoader(null, null), nullValue()); assertThat(mapper.mapping().getRoot().syntheticFieldLoader().docValuesLoader(null, null), nullValue()); } + + public void testNestedObjectWithMultiFieldsMapperSize() throws IOException { + 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.mapperSize(), equalTo(5)); + assertThat(mapperBuilder.build(MapperBuilderContext.root(false, false)).mapperSize(), equalTo(5)); + } } diff --git a/server/src/test/java/org/elasticsearch/index/mapper/ParametrizedMapperTests.java b/server/src/test/java/org/elasticsearch/index/mapper/ParametrizedMapperTests.java index 9b447d0727152..562a30ba4f389 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/ParametrizedMapperTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/ParametrizedMapperTests.java @@ -345,7 +345,7 @@ public void testMerging() { {"type":"test_mapper","fixed":true,"fixed2":true,"required":"value"}"""); IllegalArgumentException e = expectThrows( IllegalArgumentException.class, - () -> mapper.merge(badMerge, MapperBuilderContext.root(false, false)) + () -> mapper.merge(badMerge, MapperMergeContext.root(false, false, Long.MAX_VALUE)) ); String expectedError = """ Mapper for [field] conflicts with existing mapper: @@ -358,7 +358,7 @@ public void testMerging() { // TODO: should we have to include 'fixed' here? Or should updates take as 'defaults' the existing values? TestMapper goodMerge = fromMapping(""" {"type":"test_mapper","fixed":false,"variable":"updated","required":"value"}"""); - TestMapper merged = (TestMapper) mapper.merge(goodMerge, MapperBuilderContext.root(false, false)); + TestMapper merged = (TestMapper) mapper.merge(goodMerge, MapperMergeContext.root(false, false, Long.MAX_VALUE)); assertEquals("{\"field\":" + mapping + "}", Strings.toString(mapper)); // original mapping is unaffected assertEquals(""" @@ -376,7 +376,7 @@ public void testMultifields() throws IOException { String addSubField = """ {"type":"test_mapper","variable":"foo","required":"value","fields":{"sub2":{"type":"keyword"}}}"""; TestMapper toMerge = fromMapping(addSubField); - TestMapper merged = (TestMapper) mapper.merge(toMerge, MapperBuilderContext.root(false, false)); + TestMapper merged = (TestMapper) mapper.merge(toMerge, MapperMergeContext.root(false, false, Long.MAX_VALUE)); assertEquals(XContentHelper.stripWhitespace(""" { "field": { @@ -399,7 +399,7 @@ public void testMultifields() throws IOException { TestMapper badToMerge = fromMapping(badSubField); IllegalArgumentException e = expectThrows( IllegalArgumentException.class, - () -> merged.merge(badToMerge, MapperBuilderContext.root(false, false)) + () -> merged.merge(badToMerge, MapperMergeContext.root(false, false, Long.MAX_VALUE)) ); assertEquals("mapper [field.sub2] cannot be changed from type [keyword] to [binary]", e.getMessage()); } @@ -415,13 +415,13 @@ public void testCopyTo() { TestMapper toMerge = fromMapping(""" {"type":"test_mapper","variable":"updated","required":"value","copy_to":["foo","bar"]}"""); - TestMapper merged = (TestMapper) mapper.merge(toMerge, MapperBuilderContext.root(false, false)); + TestMapper merged = (TestMapper) mapper.merge(toMerge, MapperMergeContext.root(false, false, Long.MAX_VALUE)); assertEquals(""" {"field":{"type":"test_mapper","variable":"updated","required":"value","copy_to":["foo","bar"]}}""", Strings.toString(merged)); TestMapper removeCopyTo = fromMapping(""" {"type":"test_mapper","variable":"updated","required":"value"}"""); - TestMapper noCopyTo = (TestMapper) merged.merge(removeCopyTo, MapperBuilderContext.root(false, false)); + TestMapper noCopyTo = (TestMapper) merged.merge(removeCopyTo, MapperMergeContext.root(false, false, Long.MAX_VALUE)); assertEquals(""" {"field":{"type":"test_mapper","variable":"updated","required":"value"}}""", Strings.toString(noCopyTo)); } @@ -487,7 +487,7 @@ public void testCustomSerialization() { TestMapper toMerge = fromMapping(conflict); IllegalArgumentException e = expectThrows( IllegalArgumentException.class, - () -> mapper.merge(toMerge, MapperBuilderContext.root(false, false)) + () -> mapper.merge(toMerge, MapperMergeContext.root(false, false, Long.MAX_VALUE)) ); assertEquals( "Mapper for [field] conflicts with existing mapper:\n" @@ -576,7 +576,10 @@ public void testAnalyzers() { TestMapper original = mapper; TestMapper toMerge = fromMapping(mapping); - e = expectThrows(IllegalArgumentException.class, () -> original.merge(toMerge, MapperBuilderContext.root(false, false))); + e = expectThrows( + IllegalArgumentException.class, + () -> original.merge(toMerge, MapperMergeContext.root(false, false, Long.MAX_VALUE)) + ); assertEquals( "Mapper for [field] conflicts with existing mapper:\n" + "\tCannot update parameter [analyzer] from [default] to [_standard]", e.getMessage() 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 5bd85a6dcdea7..ea2e2df844d0c 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/RootObjectMapperTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/RootObjectMapperTests.java @@ -162,6 +162,15 @@ public void testRuntimeSection() throws IOException { assertEquals(mapping, mapperService.documentMapper().mappingSource().toString()); } + public void testRuntimeMappingBuilderSize() throws IOException { + RootObjectMapper.Builder builder = getRootObjectMapperBuilder(runtimeMapping(b -> { + b.startObject("field1").field("type", "double").endObject(); + b.startObject("field2").field("type", "date").endObject(); + b.startObject("field3").field("type", "ip").endObject(); + })); + assertEquals(3, builder.mapperSize()); + } + public void testRuntimeSectionRejectedUpdate() throws IOException { MapperService mapperService; { diff --git a/server/src/test/java/org/elasticsearch/index/mapper/flattened/FlattenedFieldMapperTests.java b/server/src/test/java/org/elasticsearch/index/mapper/flattened/FlattenedFieldMapperTests.java index e4ea78f3b7a0e..f825bf70db3e4 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/flattened/FlattenedFieldMapperTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/flattened/FlattenedFieldMapperTests.java @@ -92,6 +92,9 @@ protected boolean supportsIgnoreMalformed() { return false; } + @Override + public void testMapperBuilderSizeMultiField() {} + public void testDefaults() throws Exception { DocumentMapper mapper = createDocumentMapper(fieldMapping(this::minimalMapping)); ParsedDocument parsedDoc = mapper.parse(source(b -> b.startObject("field").field("key", "value").endObject())); 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 63a726d83f79e..dfd6ac9848e2b 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..2f5d039a83f45 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,32 @@ public final void testMinimalToMaximal() throws IOException { assertParseMaximalWarnings(); } + public void testMapperBuilderSize() throws IOException { + RootObjectMapper.Builder builder = getRootObjectMapperBuilder(fieldMapping(this::minimalMapping)); + assertEquals(builder.build(MapperBuilderContext.root(false, false)).mapperSize(), builder.mapperSize()); + } + + public void testMapperBuilderSizeMultiField() throws IOException { + RootObjectMapper.Builder builder = getRootObjectMapperBuilder(fieldMapping(b -> { + minimalMapping(b); + b.startObject("fields"); + { + b.startObject("multi_field_1"); + { + b.field("type", "keyword"); + } + b.endObject(); + b.startObject("multi_field_2"); + { + b.field("type", "keyword"); + } + b.endObject(); + } + b.endObject(); + })); + assertEquals(builder.build(MapperBuilderContext.root(false, false)).mapperSize(), builder.mapperSize()); + } + protected final void assertParseMinimalWarnings() { String[] warnings = getParseMinimalWarnings(); if (warnings.length > 0) { diff --git a/x-pack/plugin/mapper-aggregate-metric/src/main/java/org/elasticsearch/xpack/aggregatemetric/mapper/AggregateDoubleMetricFieldMapper.java b/x-pack/plugin/mapper-aggregate-metric/src/main/java/org/elasticsearch/xpack/aggregatemetric/mapper/AggregateDoubleMetricFieldMapper.java index b5c35e758a65c..a2508c528cfe6 100644 --- a/x-pack/plugin/mapper-aggregate-metric/src/main/java/org/elasticsearch/xpack/aggregatemetric/mapper/AggregateDoubleMetricFieldMapper.java +++ b/x-pack/plugin/mapper-aggregate-metric/src/main/java/org/elasticsearch/xpack/aggregatemetric/mapper/AggregateDoubleMetricFieldMapper.java @@ -254,6 +254,11 @@ public AggregateDoubleMetricFieldMapper build(MapperBuilderContext context) { return new AggregateDoubleMetricFieldMapper(name, metricFieldType, metricMappers, this); } + + @Override + public int mapperSize() { + return 1; + } } public static final FieldMapper.TypeParser PARSER = new TypeParser( diff --git a/x-pack/plugin/mapper-constant-keyword/src/main/java/org/elasticsearch/xpack/constantkeyword/mapper/ConstantKeywordFieldMapper.java b/x-pack/plugin/mapper-constant-keyword/src/main/java/org/elasticsearch/xpack/constantkeyword/mapper/ConstantKeywordFieldMapper.java index ebe25ea1da1d9..a01a9cf81dd88 100644 --- a/x-pack/plugin/mapper-constant-keyword/src/main/java/org/elasticsearch/xpack/constantkeyword/mapper/ConstantKeywordFieldMapper.java +++ b/x-pack/plugin/mapper-constant-keyword/src/main/java/org/elasticsearch/xpack/constantkeyword/mapper/ConstantKeywordFieldMapper.java @@ -102,6 +102,11 @@ public ConstantKeywordFieldMapper build(MapperBuilderContext context) { new ConstantKeywordFieldType(context.buildFullName(name), value.getValue(), meta.getValue()) ); } + + @Override + public int mapperSize() { + return 1; + } } public static final TypeParser PARSER = new TypeParser((n, c) -> new Builder(n)); diff --git a/x-pack/plugin/mapper-counted-keyword/src/main/java/org/elasticsearch/xpack/countedkeyword/CountedKeywordFieldMapper.java b/x-pack/plugin/mapper-counted-keyword/src/main/java/org/elasticsearch/xpack/countedkeyword/CountedKeywordFieldMapper.java index d04bb88325cc7..b7b38d9b6a88e 100644 --- a/x-pack/plugin/mapper-counted-keyword/src/main/java/org/elasticsearch/xpack/countedkeyword/CountedKeywordFieldMapper.java +++ b/x-pack/plugin/mapper-counted-keyword/src/main/java/org/elasticsearch/xpack/countedkeyword/CountedKeywordFieldMapper.java @@ -294,6 +294,11 @@ public FieldMapper build(MapperBuilderContext context) { countFieldMapper ); } + + @Override + public int mapperSize() { + return super.mapperSize() + 1; // countFieldMapper + } } public static TypeParser PARSER = new TypeParser((n, c) -> new CountedKeywordFieldMapper.Builder(n)); diff --git a/x-pack/plugin/spatial/src/test/java/org/elasticsearch/xpack/spatial/index/mapper/GeoShapeWithDocValuesFieldMapperTests.java b/x-pack/plugin/spatial/src/test/java/org/elasticsearch/xpack/spatial/index/mapper/GeoShapeWithDocValuesFieldMapperTests.java index 0ddb38ea500f1..dd5b06ae311b1 100644 --- a/x-pack/plugin/spatial/src/test/java/org/elasticsearch/xpack/spatial/index/mapper/GeoShapeWithDocValuesFieldMapperTests.java +++ b/x-pack/plugin/spatial/src/test/java/org/elasticsearch/xpack/spatial/index/mapper/GeoShapeWithDocValuesFieldMapperTests.java @@ -66,6 +66,12 @@ protected void registerParameters(ParameterChecker checker) throws IOException { }); } + @Override + public void testMapperBuilderSizeMultiField() throws IOException { + super.testMapperBuilderSizeMultiField(); + assertWarnings("Adding multifields to [geo_shape] mappers has no effect and will be forbidden in future"); + } + protected AbstractShapeGeometryFieldType fieldType(Mapper fieldMapper) { AbstractShapeGeometryFieldMapper shapeFieldMapper = (AbstractShapeGeometryFieldMapper) fieldMapper; return (AbstractShapeGeometryFieldType) shapeFieldMapper.fieldType(); diff --git a/x-pack/plugin/spatial/src/test/java/org/elasticsearch/xpack/spatial/index/mapper/PointFieldMapperTests.java b/x-pack/plugin/spatial/src/test/java/org/elasticsearch/xpack/spatial/index/mapper/PointFieldMapperTests.java index 0e26d9ba0aaf2..3680be6c7bd4f 100644 --- a/x-pack/plugin/spatial/src/test/java/org/elasticsearch/xpack/spatial/index/mapper/PointFieldMapperTests.java +++ b/x-pack/plugin/spatial/src/test/java/org/elasticsearch/xpack/spatial/index/mapper/PointFieldMapperTests.java @@ -63,6 +63,12 @@ protected void registerParameters(ParameterChecker checker) throws IOException { }); } + @Override + public void testMapperBuilderSizeMultiField() throws IOException { + super.testMapperBuilderSizeMultiField(); + assertWarnings("Adding multifields to [point] mappers has no effect and will be forbidden in future"); + } + public void testAggregationsDocValuesDisabled() throws IOException { MapperService mapperService = createMapperService(fieldMapping(b -> { minimalMapping(b); diff --git a/x-pack/plugin/spatial/src/test/java/org/elasticsearch/xpack/spatial/index/mapper/ShapeFieldMapperTests.java b/x-pack/plugin/spatial/src/test/java/org/elasticsearch/xpack/spatial/index/mapper/ShapeFieldMapperTests.java index 26d349a7ee5a6..aa5e1507c7786 100644 --- a/x-pack/plugin/spatial/src/test/java/org/elasticsearch/xpack/spatial/index/mapper/ShapeFieldMapperTests.java +++ b/x-pack/plugin/spatial/src/test/java/org/elasticsearch/xpack/spatial/index/mapper/ShapeFieldMapperTests.java @@ -87,6 +87,12 @@ protected void registerParameters(ParameterChecker checker) throws IOException { }); } + @Override + public void testMapperBuilderSizeMultiField() throws IOException { + super.testMapperBuilderSizeMultiField(); + assertWarnings("Adding multifields to [shape] mappers has no effect and will be forbidden in future"); + } + protected AbstractShapeGeometryFieldType fieldType(Mapper fieldMapper) { AbstractShapeGeometryFieldMapper shapeFieldMapper = (AbstractShapeGeometryFieldMapper) fieldMapper; return (AbstractShapeGeometryFieldType) shapeFieldMapper.fieldType(); From 044835b89dd18d6d8032aa74aa716fc10ae54ab0 Mon Sep 17 00:00:00 2001 From: Felix Barnsteiner Date: Tue, 5 Dec 2023 13:19:32 +0100 Subject: [PATCH 02/40] Add unit tests --- .../index/mapper/ObjectMapperMergeTests.java | 98 ++++++++++++++++++- 1 file changed, 97 insertions(+), 1 deletion(-) diff --git a/server/src/test/java/org/elasticsearch/index/mapper/ObjectMapperMergeTests.java b/server/src/test/java/org/elasticsearch/index/mapper/ObjectMapperMergeTests.java index 45d0ea4c2eed1..d27420a531034 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/ObjectMapperMergeTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/ObjectMapperMergeTests.java @@ -204,6 +204,98 @@ public void testMergedFieldNamesMultiFieldsWithinSubobjectsFalse() { assertEquals("keyword", fieldMapper.simpleName()); } + public void testMergeWithLimit() { + // GIVEN an enriched mapping with "baz" new field + ObjectMapper mergeWith = createMapping(false, true, true, true); + + // WHEN merging mappings + final ObjectMapper mergedAdd0 = rootObjectMapper.merge(mergeWith, MapperMergeContext.root(false, false, 0)); + 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()); + } + + public void testMergeWithLimitPreserveOrder() { + RootObjectMapper root = new RootObjectMapper.Builder("_doc", Explicit.IMPLICIT_TRUE).build(MapperBuilderContext.root(false, false)); + RootObjectMapper mergeWith = new RootObjectMapper.Builder("_doc", Explicit.IMPLICIT_TRUE).add( + new KeywordFieldMapper.Builder("field1", IndexVersion.current()) + ) + .add(new KeywordFieldMapper.Builder("field2", IndexVersion.current())) + .add(new KeywordFieldMapper.Builder("field3", IndexVersion.current())) + .add(new KeywordFieldMapper.Builder("field4", IndexVersion.current())) + .add(new KeywordFieldMapper.Builder("field5", IndexVersion.current())) + .add(new KeywordFieldMapper.Builder("field6", IndexVersion.current())) + .add(new KeywordFieldMapper.Builder("field7", IndexVersion.current())) + .add(new KeywordFieldMapper.Builder("field8", IndexVersion.current())) + .add(new KeywordFieldMapper.Builder("field9", IndexVersion.current())) + .add(new KeywordFieldMapper.Builder("field10", IndexVersion.current())) + .build(MapperBuilderContext.root(false, false)); + + ObjectMapper merged = root.merge(mergeWith, MapperMergeContext.root(false, false, 5)); + assertEquals(0, root.mapperSize()); + assertEquals(5, merged.mapperSize()); + + assertNotNull(merged.getMapper("field1")); + assertNotNull(merged.getMapper("field2")); + assertNotNull(merged.getMapper("field3")); + assertNotNull(merged.getMapper("field4")); + assertNotNull(merged.getMapper("field5")); + assertNull(merged.getMapper("field6")); + assertNull(merged.getMapper("field7")); + assertNull(merged.getMapper("field8")); + assertNull(merged.getMapper("field9")); + assertNull(merged.getMapper("field10")); + } + + public void testMergeWithLimitObjectField() { + RootObjectMapper root = new RootObjectMapper.Builder("_doc", Explicit.IMPLICIT_TRUE).build(MapperBuilderContext.root(false, false)); + RootObjectMapper mergeWith = new RootObjectMapper.Builder("_doc", Explicit.IMPLICIT_TRUE).add( + new ObjectMapper.Builder("parent", Explicit.IMPLICIT_FALSE) + .add(new KeywordFieldMapper.Builder("child1", IndexVersion.current())) + .add(new KeywordFieldMapper.Builder("child2", IndexVersion.current())) + ).build(MapperBuilderContext.root(false, false)); + + 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(1, mergedAdd1.mapperSize()); + assertEquals(2, mergedAdd2.mapperSize()); + + ObjectMapper parent1 = (ObjectMapper) mergedAdd1.getMapper("parent"); + assertNull(parent1.getMapper("child1")); + assertNull(parent1.getMapper("child2")); + + ObjectMapper parent2 = (ObjectMapper) mergedAdd2.getMapper("parent"); + assertNotNull(parent2.getMapper("child1")); + assertNull(parent2.getMapper("child2")); + + ObjectMapper parent3 = (ObjectMapper) mergedAdd3.getMapper("parent"); + assertNotNull(parent3.getMapper("child1")); + assertNotNull(parent3.getMapper("child2")); + } + + public void testMergeWithLimitMultiField() { + RootObjectMapper mergeInto = new RootObjectMapper.Builder("_doc", Explicit.IMPLICIT_TRUE).add( + createTextKeywordMultiField("text", "keyword1") + ).build(MapperBuilderContext.root(false, false)); + RootObjectMapper mergeWith = new RootObjectMapper.Builder("_doc", Explicit.IMPLICIT_TRUE).add( + createTextKeywordMultiField("text", "keyword2") + ).build(MapperBuilderContext.root(false, false)); + + assertEquals(2, mergeInto.mapperSize()); + assertEquals(2, mergeWith.mapperSize()); + + 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()); + } + private static RootObjectMapper createRootSubobjectFalseLeafWithDots() { FieldMapper.Builder fieldBuilder = new KeywordFieldMapper.Builder("host.name", IndexVersion.current()); FieldMapper fieldMapper = fieldBuilder.build(MapperBuilderContext.root(false, false)); @@ -237,8 +329,12 @@ private ObjectMapper.Builder createObjectSubobjectsFalseLeafWithMultiField() { } private TextFieldMapper.Builder createTextKeywordMultiField(String name) { + return createTextKeywordMultiField(name, "keyword"); + } + + private TextFieldMapper.Builder createTextKeywordMultiField(String name, String multiFieldName) { TextFieldMapper.Builder builder = new TextFieldMapper.Builder(name, createDefaultIndexAnalyzers()); - builder.multiFieldsBuilder.add(new KeywordFieldMapper.Builder("keyword", IndexVersion.current())); + builder.multiFieldsBuilder.add(new KeywordFieldMapper.Builder(multiFieldName, IndexVersion.current())); return builder; } } From 1d92b115794a5a3cc374aee0907d01ec08e06451 Mon Sep 17 00:00:00 2001 From: Felix Barnsteiner Date: Tue, 5 Dec 2023 13:51:16 +0100 Subject: [PATCH 03/40] Don't preserve order in mappers --- .../index/mapper/ObjectMapper.java | 11 +++--- .../index/mapper/ObjectMapperMergeTests.java | 36 ++----------------- 2 files changed, 7 insertions(+), 40 deletions(-) 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 664c2f54ed195..6c1f2f9c8db9c 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java @@ -24,11 +24,10 @@ import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; -import java.util.Collections; import java.util.Comparator; +import java.util.HashMap; import java.util.HashSet; import java.util.Iterator; -import java.util.LinkedHashMap; import java.util.List; import java.util.Locale; import java.util.Map; @@ -157,7 +156,7 @@ private ObjectMapper.Builder findObjectBuilder(String leafName, String fullName, } protected final Map buildMappers(MapperBuilderContext mapperBuilderContext) { - Map mappers = new LinkedHashMap<>(); + Map mappers = new HashMap<>(); for (Mapper.Builder builder : mappersBuilders) { Mapper mapper = builder.build(mapperBuilderContext); assert mapper instanceof ObjectMapper == false || subobjects.value() : "unexpected object while subobjects are disabled"; @@ -400,7 +399,7 @@ private static void validateFieldName(String fieldName, IndexVersion indexCreate if (mappers == null) { this.mappers = Map.of(); } else { - this.mappers = Collections.unmodifiableMap(mappers); + this.mappers = Map.copyOf(mappers); } } @@ -549,7 +548,7 @@ private static Map buildMergedMappers( MapperMergeContext objectMergeContext ) { // need to preserve order to make it deterministic which fields are ignored - Map mergedMappers = new LinkedHashMap<>(existing.mappers); + Map mergedMappers = new HashMap<>(existing.mappers); for (Mapper mergeWithMapper : mergeWith) { Mapper mergeIntoMapper = mergedMappers.get(mergeWithMapper.simpleName()); @@ -582,7 +581,7 @@ private static Map buildMergedMappers( } } } - return Collections.unmodifiableMap(mergedMappers); + return Map.copyOf(mergedMappers); } } diff --git a/server/src/test/java/org/elasticsearch/index/mapper/ObjectMapperMergeTests.java b/server/src/test/java/org/elasticsearch/index/mapper/ObjectMapperMergeTests.java index d27420a531034..390337dead859 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/ObjectMapperMergeTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/ObjectMapperMergeTests.java @@ -219,38 +219,6 @@ public void testMergeWithLimit() { assertEquals(4, mergedAdd1.mapperSize()); } - public void testMergeWithLimitPreserveOrder() { - RootObjectMapper root = new RootObjectMapper.Builder("_doc", Explicit.IMPLICIT_TRUE).build(MapperBuilderContext.root(false, false)); - RootObjectMapper mergeWith = new RootObjectMapper.Builder("_doc", Explicit.IMPLICIT_TRUE).add( - new KeywordFieldMapper.Builder("field1", IndexVersion.current()) - ) - .add(new KeywordFieldMapper.Builder("field2", IndexVersion.current())) - .add(new KeywordFieldMapper.Builder("field3", IndexVersion.current())) - .add(new KeywordFieldMapper.Builder("field4", IndexVersion.current())) - .add(new KeywordFieldMapper.Builder("field5", IndexVersion.current())) - .add(new KeywordFieldMapper.Builder("field6", IndexVersion.current())) - .add(new KeywordFieldMapper.Builder("field7", IndexVersion.current())) - .add(new KeywordFieldMapper.Builder("field8", IndexVersion.current())) - .add(new KeywordFieldMapper.Builder("field9", IndexVersion.current())) - .add(new KeywordFieldMapper.Builder("field10", IndexVersion.current())) - .build(MapperBuilderContext.root(false, false)); - - ObjectMapper merged = root.merge(mergeWith, MapperMergeContext.root(false, false, 5)); - assertEquals(0, root.mapperSize()); - assertEquals(5, merged.mapperSize()); - - assertNotNull(merged.getMapper("field1")); - assertNotNull(merged.getMapper("field2")); - assertNotNull(merged.getMapper("field3")); - assertNotNull(merged.getMapper("field4")); - assertNotNull(merged.getMapper("field5")); - assertNull(merged.getMapper("field6")); - assertNull(merged.getMapper("field7")); - assertNull(merged.getMapper("field8")); - assertNull(merged.getMapper("field9")); - assertNull(merged.getMapper("field10")); - } - public void testMergeWithLimitObjectField() { RootObjectMapper root = new RootObjectMapper.Builder("_doc", Explicit.IMPLICIT_TRUE).build(MapperBuilderContext.root(false, false)); RootObjectMapper mergeWith = new RootObjectMapper.Builder("_doc", Explicit.IMPLICIT_TRUE).add( @@ -271,8 +239,8 @@ public void testMergeWithLimitObjectField() { assertNull(parent1.getMapper("child2")); ObjectMapper parent2 = (ObjectMapper) mergedAdd2.getMapper("parent"); - assertNotNull(parent2.getMapper("child1")); - assertNull(parent2.getMapper("child2")); + // the order is not deterministic, but we expect one to be null and the other to be non-null + assertTrue(parent2.getMapper("child1") == null ^ parent2.getMapper("child2") == null); ObjectMapper parent3 = (ObjectMapper) mergedAdd3.getMapper("parent"); assertNotNull(parent3.getMapper("child1")); From e9944c7d58889cc99cd155f7a6228397b70fe550 Mon Sep 17 00:00:00 2001 From: Felix Barnsteiner Date: Tue, 5 Dec 2023 13:55:53 +0100 Subject: [PATCH 04/40] Apply spotless suggestions --- .../elasticsearch/index/mapper/ObjectMapperMergeTests.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/server/src/test/java/org/elasticsearch/index/mapper/ObjectMapperMergeTests.java b/server/src/test/java/org/elasticsearch/index/mapper/ObjectMapperMergeTests.java index 390337dead859..8f95b169ede5f 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/ObjectMapperMergeTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/ObjectMapperMergeTests.java @@ -222,9 +222,9 @@ public void testMergeWithLimit() { public void testMergeWithLimitObjectField() { RootObjectMapper root = new RootObjectMapper.Builder("_doc", Explicit.IMPLICIT_TRUE).build(MapperBuilderContext.root(false, false)); RootObjectMapper mergeWith = new RootObjectMapper.Builder("_doc", Explicit.IMPLICIT_TRUE).add( - new ObjectMapper.Builder("parent", Explicit.IMPLICIT_FALSE) - .add(new KeywordFieldMapper.Builder("child1", IndexVersion.current())) - .add(new KeywordFieldMapper.Builder("child2", IndexVersion.current())) + new ObjectMapper.Builder("parent", Explicit.IMPLICIT_FALSE).add( + new KeywordFieldMapper.Builder("child1", IndexVersion.current()) + ).add(new KeywordFieldMapper.Builder("child2", IndexVersion.current())) ).build(MapperBuilderContext.root(false, false)); ObjectMapper mergedAdd1 = root.merge(mergeWith, MapperMergeContext.root(false, false, 1)); From 9c49a87e450ad88bc058204b167fa640ed568503 Mon Sep 17 00:00:00 2001 From: Felix Barnsteiner Date: Wed, 6 Dec 2023 17:14:09 +0100 Subject: [PATCH 05/40] Comment out new tests in ObjectMapperMergeTests to check if they're the cause of the segfault --- .../elasticsearch/index/mapper/ObjectMapperMergeTests.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/server/src/test/java/org/elasticsearch/index/mapper/ObjectMapperMergeTests.java b/server/src/test/java/org/elasticsearch/index/mapper/ObjectMapperMergeTests.java index 8f95b169ede5f..355ffc6e40d54 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/ObjectMapperMergeTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/ObjectMapperMergeTests.java @@ -184,7 +184,7 @@ public void testMergedFieldNamesMultiFields() { assertEquals("keyword", keyword.simpleName()); } - public void testMergedFieldNamesMultiFieldsWithinSubobjectsFalse() { + /*public void testMergedFieldNamesMultiFieldsWithinSubobjectsFalse() { RootObjectMapper mergeInto = new RootObjectMapper.Builder("_doc", Explicit.IMPLICIT_TRUE).add( createObjectSubobjectsFalseLeafWithMultiField() ).build(MapperBuilderContext.root(false, false)); @@ -245,7 +245,7 @@ public void testMergeWithLimitObjectField() { ObjectMapper parent3 = (ObjectMapper) mergedAdd3.getMapper("parent"); assertNotNull(parent3.getMapper("child1")); assertNotNull(parent3.getMapper("child2")); - } + }*/ public void testMergeWithLimitMultiField() { RootObjectMapper mergeInto = new RootObjectMapper.Builder("_doc", Explicit.IMPLICIT_TRUE).add( From 3244ec5e49cb176218aab049afd64739dff14fca Mon Sep 17 00:00:00 2001 From: Felix Barnsteiner Date: Wed, 6 Dec 2023 17:42:45 +0100 Subject: [PATCH 06/40] Revert "Comment out new tests in ObjectMapperMergeTests to check if they're the cause of the segfault" This reverts commit 9c49a87e450ad88bc058204b167fa640ed568503. --- .../elasticsearch/index/mapper/ObjectMapperMergeTests.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/server/src/test/java/org/elasticsearch/index/mapper/ObjectMapperMergeTests.java b/server/src/test/java/org/elasticsearch/index/mapper/ObjectMapperMergeTests.java index 355ffc6e40d54..8f95b169ede5f 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/ObjectMapperMergeTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/ObjectMapperMergeTests.java @@ -184,7 +184,7 @@ public void testMergedFieldNamesMultiFields() { assertEquals("keyword", keyword.simpleName()); } - /*public void testMergedFieldNamesMultiFieldsWithinSubobjectsFalse() { + public void testMergedFieldNamesMultiFieldsWithinSubobjectsFalse() { RootObjectMapper mergeInto = new RootObjectMapper.Builder("_doc", Explicit.IMPLICIT_TRUE).add( createObjectSubobjectsFalseLeafWithMultiField() ).build(MapperBuilderContext.root(false, false)); @@ -245,7 +245,7 @@ public void testMergeWithLimitObjectField() { ObjectMapper parent3 = (ObjectMapper) mergedAdd3.getMapper("parent"); assertNotNull(parent3.getMapper("child1")); assertNotNull(parent3.getMapper("child2")); - }*/ + } public void testMergeWithLimitMultiField() { RootObjectMapper mergeInto = new RootObjectMapper.Builder("_doc", Explicit.IMPLICIT_TRUE).add( From 84ab9ae3b3badb02db1d8765a921eab0bb79417d Mon Sep 17 00:00:00 2001 From: Felix Barnsteiner Date: Tue, 2 Jan 2024 09:06:22 +0100 Subject: [PATCH 07/40] Consider runtime fields for root mapper size --- .../java/org/elasticsearch/index/mapper/RootObjectMapper.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 05e8f850ecb5f..8c294d4ce93f6 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/RootObjectMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/RootObjectMapper.java @@ -525,7 +525,7 @@ private static boolean processField( @Override public int mapperSize() { - int size = 0; + int size = runtimeFields().size(); for (Mapper mapper : this) { size += mapper.mapperSize(); } From befd7f6802ed2ec2eb1e05d16a9f64c0c46b971e Mon Sep 17 00:00:00 2001 From: Felix Barnsteiner Date: Tue, 2 Jan 2024 09:12:19 +0100 Subject: [PATCH 08/40] Assert mapper size equal to builder size --- .../main/java/org/elasticsearch/index/mapper/ObjectMapper.java | 2 ++ 1 file changed, 2 insertions(+) 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 6c1f2f9c8db9c..486f61bd148c1 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java @@ -159,6 +159,8 @@ protected final Map buildMappers(MapperBuilderContext mapperBuil Map mappers = new HashMap<>(); for (Mapper.Builder builder : mappersBuilders) { Mapper mapper = builder.build(mapperBuilderContext); + assert mapper instanceof FieldMapper == false || mapper.mapperSize() == builder.mapperSize() + : "size of mapper differs from it's builder"; assert mapper instanceof ObjectMapper == false || subobjects.value() : "unexpected object while subobjects are disabled"; Mapper existing = mappers.get(mapper.simpleName()); if (existing != null) { From dbee72bbc852dbd46d80a3bc87b7bf7734f9cc34 Mon Sep 17 00:00:00 2001 From: Felix Barnsteiner Date: Tue, 2 Jan 2024 11:59:31 +0100 Subject: [PATCH 09/40] Fix SemanticTextFieldMapperTests --- .../org/elasticsearch/index/mapper/FieldMapper.java | 4 ++++ .../xpack/ml/mapper/SemanticTextFieldMapper.java | 12 ++++++++++++ .../ml/mapper/SemanticTextFieldMapperTests.java | 6 ++++++ 3 files changed, 22 insertions(+) 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 900f229aa399e..909f720d446a2 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/FieldMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/FieldMapper.java @@ -485,6 +485,10 @@ public MultiFields build(Mapper.Builder mainFieldBuilder, MapperBuilderContext c public int mapperSize() { return mapperBuilders.size(); } + + public void clear() { + mapperBuilders.clear(); + } } private final FieldMapper[] mappers; diff --git a/x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/mapper/SemanticTextFieldMapper.java b/x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/mapper/SemanticTextFieldMapper.java index cf713546a071a..21864ce91fca9 100644 --- a/x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/mapper/SemanticTextFieldMapper.java +++ b/x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/mapper/SemanticTextFieldMapper.java @@ -9,6 +9,8 @@ import org.apache.lucene.search.Query; import org.elasticsearch.common.Strings; +import org.elasticsearch.common.logging.DeprecationCategory; +import org.elasticsearch.common.logging.DeprecationLogger; import org.elasticsearch.index.fielddata.FieldDataContext; import org.elasticsearch.index.fielddata.IndexFieldData; import org.elasticsearch.index.mapper.DocumentParserContext; @@ -34,6 +36,8 @@ */ public class SemanticTextFieldMapper extends FieldMapper { + private static final DeprecationLogger DEPRECATION_LOGGER = DeprecationLogger.getLogger(SemanticTextFieldMapper.class); + public static final String CONTENT_TYPE = "semantic_text"; private static SemanticTextFieldMapper toType(FieldMapper in) { @@ -89,6 +93,14 @@ protected Parameter[] getParameters() { @Override public SemanticTextFieldMapper build(MapperBuilderContext context) { + if (multiFieldsBuilder.hasMultiFields()) { + DEPRECATION_LOGGER.warn( + DeprecationCategory.MAPPINGS, + "semantic_text_multifields", + "Adding multifields to [semantic_text] mappers has no effect and will be forbidden in future" + ); + multiFieldsBuilder.clear(); + } return new SemanticTextFieldMapper(name(), new SemanticTextFieldType(name(), modelId.getValue(), meta.getValue()), copyTo); } } diff --git a/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/mapper/SemanticTextFieldMapperTests.java b/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/mapper/SemanticTextFieldMapperTests.java index ccb8f106e4945..05f019594d9cb 100644 --- a/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/mapper/SemanticTextFieldMapperTests.java +++ b/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/mapper/SemanticTextFieldMapperTests.java @@ -30,6 +30,12 @@ public class SemanticTextFieldMapperTests extends MapperTestCase { + @Override + public void testMapperBuilderSizeMultiField() throws IOException { + super.testMapperBuilderSizeMultiField(); + assertWarnings("Adding multifields to [semantic_text] mappers has no effect and will be forbidden in future"); + } + public void testDefaults() throws Exception { DocumentMapper mapper = createDocumentMapper(fieldMapping(this::minimalMapping)); assertEquals(Strings.toString(fieldMapping(this::minimalMapping)), mapper.mappingSource().toString()); From fd524dc085f2f40cb25dbbcfd25e8db59159c51a Mon Sep 17 00:00:00 2001 From: Felix Barnsteiner Date: Tue, 2 Jan 2024 11:59:51 +0100 Subject: [PATCH 10/40] Fix multi-field mapper builder size calculation --- .../index/mapper/FieldMapper.java | 52 ++++++++++++++++--- 1 file changed, 45 insertions(+), 7 deletions(-) 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 909f720d446a2..54f6f447338b0 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/FieldMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/FieldMapper.java @@ -442,15 +442,49 @@ public static MultiFields empty() { public static class Builder { - private final Map> mapperBuilders = new HashMap<>(); + private interface FieldMapperOrBuilder { + int mapperSize(); + + FieldMapper build(MapperBuilderContext context); + + static FieldMapperOrBuilder of(FieldMapper mapper) { + return new FieldMapperOrBuilder() { + @Override + public int mapperSize() { + return mapper.mapperSize(); + } + + @Override + public FieldMapper build(MapperBuilderContext context) { + return mapper; + } + }; + } + + static FieldMapperOrBuilder of(FieldMapper.Builder builder) { + return new FieldMapperOrBuilder() { + @Override + public int mapperSize() { + return builder.mapperSize(); + } + + @Override + public FieldMapper build(MapperBuilderContext context) { + return builder.build(context); + } + }; + } + } + + private final Map mapperBuilders = new HashMap<>(); public Builder add(FieldMapper.Builder builder) { - mapperBuilders.put(builder.name(), builder::build); + mapperBuilders.put(builder.name(), FieldMapperOrBuilder.of(builder)); return this; } public Builder add(FieldMapper mapper) { - mapperBuilders.put(mapper.simpleName(), context -> mapper); + mapperBuilders.put(mapper.simpleName(), FieldMapperOrBuilder.of(mapper)); return this; } @@ -458,7 +492,7 @@ public Builder update(FieldMapper toMerge, MapperMergeContext context) { if (mapperBuilders.containsKey(toMerge.simpleName()) == false) { context.addFieldIfPossible(toMerge, () -> add(toMerge)); } else { - FieldMapper existing = mapperBuilders.get(toMerge.simpleName()).apply(context.getMapperBuilderContext()); + FieldMapper existing = mapperBuilders.get(toMerge.simpleName()).build(context.getMapperBuilderContext()); add(existing.merge(toMerge, context)); } return this; @@ -475,15 +509,19 @@ public MultiFields build(Mapper.Builder mainFieldBuilder, MapperBuilderContext c FieldMapper[] mappers = new FieldMapper[mapperBuilders.size()]; context = context.createChildContext(mainFieldBuilder.name()); int i = 0; - for (Map.Entry> entry : this.mapperBuilders.entrySet()) { - mappers[i++] = entry.getValue().apply(context); + for (Map.Entry entry : this.mapperBuilders.entrySet()) { + mappers[i++] = entry.getValue().build(context); } return new MultiFields(mappers); } } public int mapperSize() { - return mapperBuilders.size(); + int size = 0; + for (FieldMapperOrBuilder mapper : mapperBuilders.values()) { + size += mapper.mapperSize(); + } + return size; } public void clear() { From 2e0f00ca07f1723b8b45b6c32d337843f85b5a4d Mon Sep 17 00:00:00 2001 From: Felix Barnsteiner Date: Tue, 2 Jan 2024 12:36:37 +0100 Subject: [PATCH 11/40] Apply spotless suggestions --- .../main/java/org/elasticsearch/index/mapper/FieldMapper.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 54f6f447338b0..24e1b0ce1666c 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/FieldMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/FieldMapper.java @@ -479,7 +479,7 @@ public FieldMapper build(MapperBuilderContext context) { private final Map mapperBuilders = new HashMap<>(); public Builder add(FieldMapper.Builder builder) { - mapperBuilders.put(builder.name(), FieldMapperOrBuilder.of(builder)); + mapperBuilders.put(builder.name(), FieldMapperOrBuilder.of(builder)); return this; } From cc69edfde9bb8a7d1a8151737fc42e7a6f18ec53 Mon Sep 17 00:00:00 2001 From: Felix Barnsteiner Date: Thu, 4 Jan 2024 09:29:14 +0100 Subject: [PATCH 12/40] Fixes and polishing --- .../percolator/PercolatorFieldMapper.java | 2 +- .../elasticsearch/index/mapper/FieldMapper.java | 4 ---- .../index/mapper/ObjectMapper.java | 1 - .../ml/mapper/SemanticTextFieldMapper.java | 17 +++++------------ 4 files changed, 6 insertions(+), 18 deletions(-) diff --git a/modules/percolator/src/main/java/org/elasticsearch/percolator/PercolatorFieldMapper.java b/modules/percolator/src/main/java/org/elasticsearch/percolator/PercolatorFieldMapper.java index dddb2db00b66c..9f7af939a25cf 100644 --- a/modules/percolator/src/main/java/org/elasticsearch/percolator/PercolatorFieldMapper.java +++ b/modules/percolator/src/main/java/org/elasticsearch/percolator/PercolatorFieldMapper.java @@ -180,7 +180,7 @@ public PercolatorFieldMapper build(MapperBuilderContext context) { @Override public int mapperSize() { - return 1 // this + return super.mapperSize() // this + multi fields + 1 // queryTermsField, + 1 // extractionResultField, + 1 // queryBuilderField, 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 24e1b0ce1666c..29d8ef89fa62e 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/FieldMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/FieldMapper.java @@ -523,10 +523,6 @@ public int mapperSize() { } return size; } - - public void clear() { - mapperBuilders.clear(); - } } private final FieldMapper[] mappers; 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 486f61bd148c1..0e29d3eb68cec 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java @@ -549,7 +549,6 @@ private static Map buildMergedMappers( MergeReason reason, MapperMergeContext objectMergeContext ) { - // need to preserve order to make it deterministic which fields are ignored Map mergedMappers = new HashMap<>(existing.mappers); for (Mapper mergeWithMapper : mergeWith) { Mapper mergeIntoMapper = mergedMappers.get(mergeWithMapper.simpleName()); diff --git a/x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/mapper/SemanticTextFieldMapper.java b/x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/mapper/SemanticTextFieldMapper.java index 21864ce91fca9..6f291ded61f1f 100644 --- a/x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/mapper/SemanticTextFieldMapper.java +++ b/x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/mapper/SemanticTextFieldMapper.java @@ -9,8 +9,6 @@ import org.apache.lucene.search.Query; import org.elasticsearch.common.Strings; -import org.elasticsearch.common.logging.DeprecationCategory; -import org.elasticsearch.common.logging.DeprecationLogger; import org.elasticsearch.index.fielddata.FieldDataContext; import org.elasticsearch.index.fielddata.IndexFieldData; import org.elasticsearch.index.mapper.DocumentParserContext; @@ -36,8 +34,6 @@ */ public class SemanticTextFieldMapper extends FieldMapper { - private static final DeprecationLogger DEPRECATION_LOGGER = DeprecationLogger.getLogger(SemanticTextFieldMapper.class); - public static final String CONTENT_TYPE = "semantic_text"; private static SemanticTextFieldMapper toType(FieldMapper in) { @@ -93,16 +89,13 @@ protected Parameter[] getParameters() { @Override public SemanticTextFieldMapper build(MapperBuilderContext context) { - if (multiFieldsBuilder.hasMultiFields()) { - DEPRECATION_LOGGER.warn( - DeprecationCategory.MAPPINGS, - "semantic_text_multifields", - "Adding multifields to [semantic_text] mappers has no effect and will be forbidden in future" - ); - multiFieldsBuilder.clear(); - } return new SemanticTextFieldMapper(name(), new SemanticTextFieldType(name(), modelId.getValue(), meta.getValue()), copyTo); } + + @Override + public int mapperSize() { + return 1; + } } public static class SemanticTextFieldType extends SimpleMappedFieldType implements InferenceModelFieldType { From c648c69b31e42c8186a383552d23daff039f047a Mon Sep 17 00:00:00 2001 From: Felix Barnsteiner Date: Thu, 4 Jan 2024 09:31:47 +0100 Subject: [PATCH 13/40] Fix SemanticTextFieldMapperTests --- .../xpack/ml/mapper/SemanticTextFieldMapperTests.java | 6 ------ 1 file changed, 6 deletions(-) diff --git a/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/mapper/SemanticTextFieldMapperTests.java b/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/mapper/SemanticTextFieldMapperTests.java index 05f019594d9cb..ccb8f106e4945 100644 --- a/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/mapper/SemanticTextFieldMapperTests.java +++ b/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/mapper/SemanticTextFieldMapperTests.java @@ -30,12 +30,6 @@ public class SemanticTextFieldMapperTests extends MapperTestCase { - @Override - public void testMapperBuilderSizeMultiField() throws IOException { - super.testMapperBuilderSizeMultiField(); - assertWarnings("Adding multifields to [semantic_text] mappers has no effect and will be forbidden in future"); - } - public void testDefaults() throws Exception { DocumentMapper mapper = createDocumentMapper(fieldMapping(this::minimalMapping)); assertEquals(Strings.toString(fieldMapping(this::minimalMapping)), mapper.mappingSource().toString()); From 49e4902591d77ccadb9fe8ea7df1180e0d69d6a8 Mon Sep 17 00:00:00 2001 From: Felix Barnsteiner Date: Thu, 4 Jan 2024 18:36:59 +0100 Subject: [PATCH 14/40] Revert "Apply spotless suggestions" This reverts commit 2e0f00ca07f1723b8b45b6c32d337843f85b5a4d. --- .../main/java/org/elasticsearch/index/mapper/FieldMapper.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 29d8ef89fa62e..5feac4518acef 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/FieldMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/FieldMapper.java @@ -479,7 +479,7 @@ public FieldMapper build(MapperBuilderContext context) { private final Map mapperBuilders = new HashMap<>(); public Builder add(FieldMapper.Builder builder) { - mapperBuilders.put(builder.name(), FieldMapperOrBuilder.of(builder)); + mapperBuilders.put(builder.name(), FieldMapperOrBuilder.of(builder)); return this; } From 024b689d91d210d8cd961e9a2c316dc8bf46fe4a Mon Sep 17 00:00:00 2001 From: Felix Barnsteiner Date: Thu, 4 Jan 2024 18:37:03 +0100 Subject: [PATCH 15/40] Revert "Fix multi-field mapper builder size calculation" This reverts commit fd524dc085f2f40cb25dbbcfd25e8db59159c51a. --- .../index/mapper/FieldMapper.java | 52 +++---------------- 1 file changed, 7 insertions(+), 45 deletions(-) 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 5feac4518acef..900f229aa399e 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/FieldMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/FieldMapper.java @@ -442,49 +442,15 @@ public static MultiFields empty() { public static class Builder { - private interface FieldMapperOrBuilder { - int mapperSize(); - - FieldMapper build(MapperBuilderContext context); - - static FieldMapperOrBuilder of(FieldMapper mapper) { - return new FieldMapperOrBuilder() { - @Override - public int mapperSize() { - return mapper.mapperSize(); - } - - @Override - public FieldMapper build(MapperBuilderContext context) { - return mapper; - } - }; - } - - static FieldMapperOrBuilder of(FieldMapper.Builder builder) { - return new FieldMapperOrBuilder() { - @Override - public int mapperSize() { - return builder.mapperSize(); - } - - @Override - public FieldMapper build(MapperBuilderContext context) { - return builder.build(context); - } - }; - } - } - - private final Map mapperBuilders = new HashMap<>(); + private final Map> mapperBuilders = new HashMap<>(); public Builder add(FieldMapper.Builder builder) { - mapperBuilders.put(builder.name(), FieldMapperOrBuilder.of(builder)); + mapperBuilders.put(builder.name(), builder::build); return this; } public Builder add(FieldMapper mapper) { - mapperBuilders.put(mapper.simpleName(), FieldMapperOrBuilder.of(mapper)); + mapperBuilders.put(mapper.simpleName(), context -> mapper); return this; } @@ -492,7 +458,7 @@ public Builder update(FieldMapper toMerge, MapperMergeContext context) { if (mapperBuilders.containsKey(toMerge.simpleName()) == false) { context.addFieldIfPossible(toMerge, () -> add(toMerge)); } else { - FieldMapper existing = mapperBuilders.get(toMerge.simpleName()).build(context.getMapperBuilderContext()); + FieldMapper existing = mapperBuilders.get(toMerge.simpleName()).apply(context.getMapperBuilderContext()); add(existing.merge(toMerge, context)); } return this; @@ -509,19 +475,15 @@ public MultiFields build(Mapper.Builder mainFieldBuilder, MapperBuilderContext c FieldMapper[] mappers = new FieldMapper[mapperBuilders.size()]; context = context.createChildContext(mainFieldBuilder.name()); int i = 0; - for (Map.Entry entry : this.mapperBuilders.entrySet()) { - mappers[i++] = entry.getValue().build(context); + for (Map.Entry> entry : this.mapperBuilders.entrySet()) { + mappers[i++] = entry.getValue().apply(context); } return new MultiFields(mappers); } } public int mapperSize() { - int size = 0; - for (FieldMapperOrBuilder mapper : mapperBuilders.values()) { - size += mapper.mapperSize(); - } - return size; + return mapperBuilders.size(); } } From 54f033810788cd35732335f727059cb0965c22a8 Mon Sep 17 00:00:00 2001 From: Felix Barnsteiner Date: Thu, 4 Jan 2024 18:38:45 +0100 Subject: [PATCH 16/40] Revert "Assert mapper size equal to builder size" This reverts commit befd7f6802ed2ec2eb1e05d16a9f64c0c46b971e. --- .../main/java/org/elasticsearch/index/mapper/ObjectMapper.java | 2 -- 1 file changed, 2 deletions(-) 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 6d0c0d1ccd61a..8557e754fb42b 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java @@ -154,8 +154,6 @@ protected final Map buildMappers(MapperBuilderContext mapperBuil Map mappers = new HashMap<>(); for (Mapper.Builder builder : mappersBuilders) { Mapper mapper = builder.build(mapperBuilderContext); - assert mapper instanceof FieldMapper == false || mapper.mapperSize() == builder.mapperSize() - : "size of mapper differs from it's builder"; assert mapper instanceof ObjectMapper == false || subobjects.value() : "unexpected object while subobjects are disabled"; Mapper existing = mappers.get(mapper.simpleName()); if (existing != null) { From a716549a7a13e74a4274024fcba1ad9c635e2906 Mon Sep 17 00:00:00 2001 From: Felix Barnsteiner Date: Fri, 5 Jan 2024 08:40:13 +0100 Subject: [PATCH 17/40] Remove Mapper.Builder#mapperSize Because of https://github.com/elastic/elasticsearch/pull/103865, DocumentParserContext#addDynamicMapper receives a Mapper, not a Mapper.Builder again. Therefore, we don't need a mapperSize method for the builder. This simplifies things a lot. --- .../LegacyGeoShapeFieldMapperTests.java | 12 ------- .../extras/SearchAsYouTypeFieldMapper.java | 6 ---- .../join/mapper/ParentJoinFieldMapper.java | 5 --- .../percolator/PercolatorFieldMapper.java | 10 ------ .../index/mapper/FieldAliasMapper.java | 5 --- .../index/mapper/FieldMapper.java | 5 --- .../elasticsearch/index/mapper/Mapper.java | 9 +---- .../index/mapper/ObjectMapper.java | 5 --- .../index/mapper/RootObjectMapper.java | 5 --- .../index/mapper/FieldAliasMapperTests.java | 34 +++++++++---------- .../mapper/GeoShapeFieldMapperTests.java | 6 ---- .../index/mapper/ObjectMapperTests.java | 1 - .../index/mapper/RootObjectMapperTests.java | 10 +----- .../flattened/FlattenedFieldMapperTests.java | 3 -- .../index/mapper/MapperServiceTestCase.java | 12 ------- .../index/mapper/MapperTestCase.java | 26 -------------- .../AggregateDoubleMetricFieldMapper.java | 5 --- .../mapper/ConstantKeywordFieldMapper.java | 5 --- .../CountedKeywordFieldMapper.java | 5 --- .../ml/mapper/SemanticTextFieldMapper.java | 5 --- ...GeoShapeWithDocValuesFieldMapperTests.java | 6 ---- .../index/mapper/PointFieldMapperTests.java | 6 ---- .../index/mapper/ShapeFieldMapperTests.java | 6 ---- 23 files changed, 19 insertions(+), 173 deletions(-) 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 ae979448357ee..91a94fe174c21 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 @@ -104,18 +104,6 @@ protected void registerParameters(ParameterChecker checker) throws IOException { checker.registerUpdateCheck(b -> b.field("distance_error_pct", 0.8), m -> {}); } - @Override - public void testMapperBuilderSize() throws IOException { - super.testMapperBuilderSize(); - assertWarnings("Parameter [strategy] is deprecated and will be removed in a future version"); - } - - @Override - public void testMapperBuilderSizeMultiField() throws IOException { - super.testMapperBuilderSizeMultiField(); - assertWarnings("Parameter [strategy] is deprecated and will be removed in a future version"); - } - @Override protected Collection getPlugins() { return List.of(new TestLegacyGeoShapeFieldMapperPlugin()); diff --git a/modules/mapper-extras/src/main/java/org/elasticsearch/index/mapper/extras/SearchAsYouTypeFieldMapper.java b/modules/mapper-extras/src/main/java/org/elasticsearch/index/mapper/extras/SearchAsYouTypeFieldMapper.java index a76e54b3580ce..ca8231c46736f 100644 --- a/modules/mapper-extras/src/main/java/org/elasticsearch/index/mapper/extras/SearchAsYouTypeFieldMapper.java +++ b/modules/mapper-extras/src/main/java/org/elasticsearch/index/mapper/extras/SearchAsYouTypeFieldMapper.java @@ -270,12 +270,6 @@ public SearchAsYouTypeFieldMapper build(MapperBuilderContext context) { this ); } - - @Override - public int mapperSize() { - return super.mapperSize() + 1 // prefixField - + maxShingleSize.getValue() - 1; - } } private static int countPosition(TokenStream stream) throws IOException { diff --git a/modules/parent-join/src/main/java/org/elasticsearch/join/mapper/ParentJoinFieldMapper.java b/modules/parent-join/src/main/java/org/elasticsearch/join/mapper/ParentJoinFieldMapper.java index 6412e40b651cf..2bbd5e81444b7 100644 --- a/modules/parent-join/src/main/java/org/elasticsearch/join/mapper/ParentJoinFieldMapper.java +++ b/modules/parent-join/src/main/java/org/elasticsearch/join/mapper/ParentJoinFieldMapper.java @@ -127,11 +127,6 @@ public ParentJoinFieldMapper build(MapperBuilderContext context) { relations.get() ); } - - @Override - public int mapperSize() { - return 1 + relations.get().size(); - } } public static final TypeParser PARSER = new TypeParser((n, c) -> { diff --git a/modules/percolator/src/main/java/org/elasticsearch/percolator/PercolatorFieldMapper.java b/modules/percolator/src/main/java/org/elasticsearch/percolator/PercolatorFieldMapper.java index 9f7af939a25cf..be8d342254afd 100644 --- a/modules/percolator/src/main/java/org/elasticsearch/percolator/PercolatorFieldMapper.java +++ b/modules/percolator/src/main/java/org/elasticsearch/percolator/PercolatorFieldMapper.java @@ -178,16 +178,6 @@ public PercolatorFieldMapper build(MapperBuilderContext context) { ); } - @Override - public int mapperSize() { - return super.mapperSize() // this + multi fields - + 1 // queryTermsField, - + 1 // extractionResultField, - + 1 // queryBuilderField, - + 1 // minimumShouldMatchFieldMapper, - + 1; // rangeFieldMapper - } - static KeywordFieldMapper createExtractQueryFieldBuilder( String name, MapperBuilderContext context, 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 4fb59474862b2..c24ff9bb9c277 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/FieldAliasMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/FieldAliasMapper.java @@ -155,11 +155,6 @@ public FieldAliasMapper build(MapperBuilderContext context) { String fullName = context.buildFullName(name); return new FieldAliasMapper(name, fullName, path); } - - @Override - public int mapperSize() { - return 1; - } } @Override 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 900f229aa399e..81e7fcaa02b5b 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/FieldMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/FieldMapper.java @@ -1411,11 +1411,6 @@ private static boolean isDeprecatedParameter(String propName, IndexVersion index } return DEPRECATED_PARAMS.contains(propName); } - - @Override - public int mapperSize() { - return 1 + multiFieldsBuilder.mapperSize(); - } } public static BiConsumer notInMultiFields(String type) { 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 ef90f7f7da20c..7cb4d721cb746 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/Mapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/Mapper.java @@ -37,13 +37,6 @@ public String name() { /** Returns a newly built mapper. */ public abstract Mapper build(MapperBuilderContext context); - - /** - * Returns the size this mapper counts against the {@linkplain MapperService#INDEX_MAPPING_TOTAL_FIELDS_LIMIT_SETTING field limit}. - *

- * Needs to be in sync with {@link Mapper#mapperSize()} which is based on {@link Mapper#iterator()} - */ - public abstract int mapperSize(); } public interface TypeParser { @@ -146,7 +139,7 @@ public static FieldType freezeAndDeduplicateFieldType(FieldType fieldType) { /** * Returns the size this mapper counts against the {@linkplain MapperService#INDEX_MAPPING_TOTAL_FIELDS_LIMIT_SETTING field limit}. *

- * Needs to be in sync with {@link Mapper.Builder#mapperSize()} and {@link MappingLookup#getTotalFieldsCount()}. + * Needs to be in sync with {@link MappingLookup#getTotalFieldsCount()}. */ public int mapperSize() { int size = 1; 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 8557e754fb42b..ac2ca037c29b2 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java @@ -180,11 +180,6 @@ public ObjectMapper build(MapperBuilderContext context) { buildMappers(context.createChildContext(name)) ); } - - @Override - public int mapperSize() { - return 1 + mappersBuilders.stream().mapToInt(Mapper.Builder::mapperSize).sum(); - } } public static class TypeParser implements Mapper.TypeParser { 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 8c294d4ce93f6..dfe302352e3de 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/RootObjectMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/RootObjectMapper.java @@ -119,11 +119,6 @@ public RootObjectMapper build(MapperBuilderContext context) { numericDetection ); } - - @Override - public int mapperSize() { - return mappersBuilders.stream().mapToInt(Mapper.Builder::mapperSize).sum() + runtimeFields.size(); - } } private final Explicit dynamicDateTimeFormatters; 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 52dd48ea3e6a2..b163d6553477b 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/FieldAliasMapperTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/FieldAliasMapperTests.java @@ -17,25 +17,25 @@ public class FieldAliasMapperTests extends MapperServiceTestCase { public void testParsing() throws IOException { - XContentBuilder mappingXContent = XContentFactory.jsonBuilder() - .startObject() - .startObject("_doc") - .startObject("properties") - .startObject("alias-field") - .field("type", "alias") - .field("path", "concrete-field") - .endObject() - .startObject("concrete-field") - .field("type", "keyword") - .endObject() - .endObject() - .endObject() - .endObject(); - String mapping = Strings.toString(mappingXContent); + String mapping = Strings.toString( + XContentFactory.jsonBuilder() + .startObject() + .startObject("_doc") + .startObject("properties") + .startObject("alias-field") + .field("type", "alias") + .field("path", "concrete-field") + .endObject() + .startObject("concrete-field") + .field("type", "keyword") + .endObject() + .endObject() + .endObject() + .endObject() + ); DocumentMapper mapper = createDocumentMapper(mapping); assertEquals(mapping, mapper.mappingSource().toString()); - RootObjectMapper.Builder builder = getRootObjectMapperBuilder(mappingXContent); - assertEquals(2, builder.mapperSize()); + assertEquals(2, mapper.mapping().getRoot().mapperSize()); } public void testParsingWithMissingPath() { diff --git a/server/src/test/java/org/elasticsearch/index/mapper/GeoShapeFieldMapperTests.java b/server/src/test/java/org/elasticsearch/index/mapper/GeoShapeFieldMapperTests.java index bae6f9cb08ca1..92da99bc059a2 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/GeoShapeFieldMapperTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/GeoShapeFieldMapperTests.java @@ -63,12 +63,6 @@ protected Object getSampleValueForDocument() { return "POINT (14.0 15.0)"; } - @Override - public void testMapperBuilderSizeMultiField() throws IOException { - super.testMapperBuilderSizeMultiField(); - assertWarnings("Adding multifields to [geo_shape] mappers has no effect and will be forbidden in future"); - } - public void testDefaultConfiguration() throws IOException { DocumentMapper mapper = createDocumentMapper(fieldMapping(this::minimalMapping)); Mapper fieldMapper = mapper.mappers().getMapper("field"); 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 ee3cd8010cda6..97d4b01675c3f 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/ObjectMapperTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/ObjectMapperTests.java @@ -536,7 +536,6 @@ public void testNestedObjectWithMultiFieldsMapperSize() throws IOException { ).addMultiField(new KeywordFieldMapper.Builder("multi_field_size_5", IndexVersion.current())) ) ); - assertThat(mapperBuilder.mapperSize(), equalTo(5)); assertThat(mapperBuilder.build(MapperBuilderContext.root(false, false)).mapperSize(), equalTo(5)); } } 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 ea2e2df844d0c..03bc835f3bf33 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/RootObjectMapperTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/RootObjectMapperTests.java @@ -160,15 +160,7 @@ public void testRuntimeSection() throws IOException { })); MapperService mapperService = createMapperService(mapping); assertEquals(mapping, mapperService.documentMapper().mappingSource().toString()); - } - - public void testRuntimeMappingBuilderSize() throws IOException { - RootObjectMapper.Builder builder = getRootObjectMapperBuilder(runtimeMapping(b -> { - b.startObject("field1").field("type", "double").endObject(); - b.startObject("field2").field("type", "date").endObject(); - b.startObject("field3").field("type", "ip").endObject(); - })); - assertEquals(3, builder.mapperSize()); + assertEquals(3, mapperService.documentMapper().mapping().getRoot().mapperSize()); } public void testRuntimeSectionRejectedUpdate() throws IOException { diff --git a/server/src/test/java/org/elasticsearch/index/mapper/flattened/FlattenedFieldMapperTests.java b/server/src/test/java/org/elasticsearch/index/mapper/flattened/FlattenedFieldMapperTests.java index f825bf70db3e4..e4ea78f3b7a0e 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/flattened/FlattenedFieldMapperTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/flattened/FlattenedFieldMapperTests.java @@ -92,9 +92,6 @@ protected boolean supportsIgnoreMalformed() { return false; } - @Override - public void testMapperBuilderSizeMultiField() {} - public void testDefaults() throws Exception { DocumentMapper mapper = createDocumentMapper(fieldMapping(this::minimalMapping)); ParsedDocument parsedDoc = mapper.parse(source(b -> b.startObject("field").field("key", "value").endObject())); 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 dfd6ac9848e2b..63a726d83f79e 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 47af594b99310..5bbd3736bc7b6 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,32 +425,6 @@ public final void testMinimalToMaximal() throws IOException { assertParseMaximalWarnings(); } - public void testMapperBuilderSize() throws IOException { - RootObjectMapper.Builder builder = getRootObjectMapperBuilder(fieldMapping(this::minimalMapping)); - assertEquals(builder.build(MapperBuilderContext.root(false, false)).mapperSize(), builder.mapperSize()); - } - - public void testMapperBuilderSizeMultiField() throws IOException { - RootObjectMapper.Builder builder = getRootObjectMapperBuilder(fieldMapping(b -> { - minimalMapping(b); - b.startObject("fields"); - { - b.startObject("multi_field_1"); - { - b.field("type", "keyword"); - } - b.endObject(); - b.startObject("multi_field_2"); - { - b.field("type", "keyword"); - } - b.endObject(); - } - b.endObject(); - })); - assertEquals(builder.build(MapperBuilderContext.root(false, false)).mapperSize(), builder.mapperSize()); - } - protected final void assertParseMinimalWarnings() { String[] warnings = getParseMinimalWarnings(); if (warnings.length > 0) { diff --git a/x-pack/plugin/mapper-aggregate-metric/src/main/java/org/elasticsearch/xpack/aggregatemetric/mapper/AggregateDoubleMetricFieldMapper.java b/x-pack/plugin/mapper-aggregate-metric/src/main/java/org/elasticsearch/xpack/aggregatemetric/mapper/AggregateDoubleMetricFieldMapper.java index a2508c528cfe6..b5c35e758a65c 100644 --- a/x-pack/plugin/mapper-aggregate-metric/src/main/java/org/elasticsearch/xpack/aggregatemetric/mapper/AggregateDoubleMetricFieldMapper.java +++ b/x-pack/plugin/mapper-aggregate-metric/src/main/java/org/elasticsearch/xpack/aggregatemetric/mapper/AggregateDoubleMetricFieldMapper.java @@ -254,11 +254,6 @@ public AggregateDoubleMetricFieldMapper build(MapperBuilderContext context) { return new AggregateDoubleMetricFieldMapper(name, metricFieldType, metricMappers, this); } - - @Override - public int mapperSize() { - return 1; - } } public static final FieldMapper.TypeParser PARSER = new TypeParser( diff --git a/x-pack/plugin/mapper-constant-keyword/src/main/java/org/elasticsearch/xpack/constantkeyword/mapper/ConstantKeywordFieldMapper.java b/x-pack/plugin/mapper-constant-keyword/src/main/java/org/elasticsearch/xpack/constantkeyword/mapper/ConstantKeywordFieldMapper.java index ae5f793529fc3..cc819c353f69c 100644 --- a/x-pack/plugin/mapper-constant-keyword/src/main/java/org/elasticsearch/xpack/constantkeyword/mapper/ConstantKeywordFieldMapper.java +++ b/x-pack/plugin/mapper-constant-keyword/src/main/java/org/elasticsearch/xpack/constantkeyword/mapper/ConstantKeywordFieldMapper.java @@ -103,11 +103,6 @@ public ConstantKeywordFieldMapper build(MapperBuilderContext context) { new ConstantKeywordFieldType(context.buildFullName(name), value.getValue(), meta.getValue()) ); } - - @Override - public int mapperSize() { - return 1; - } } public static final TypeParser PARSER = new TypeParser((n, c) -> new Builder(n)); diff --git a/x-pack/plugin/mapper-counted-keyword/src/main/java/org/elasticsearch/xpack/countedkeyword/CountedKeywordFieldMapper.java b/x-pack/plugin/mapper-counted-keyword/src/main/java/org/elasticsearch/xpack/countedkeyword/CountedKeywordFieldMapper.java index 373571fc23870..ad5e224efd5db 100644 --- a/x-pack/plugin/mapper-counted-keyword/src/main/java/org/elasticsearch/xpack/countedkeyword/CountedKeywordFieldMapper.java +++ b/x-pack/plugin/mapper-counted-keyword/src/main/java/org/elasticsearch/xpack/countedkeyword/CountedKeywordFieldMapper.java @@ -309,11 +309,6 @@ public FieldMapper build(MapperBuilderContext context) { countFieldMapper ); } - - @Override - public int mapperSize() { - return super.mapperSize() + 1; // countFieldMapper - } } public static TypeParser PARSER = new TypeParser((n, c) -> new CountedKeywordFieldMapper.Builder(n)); diff --git a/x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/mapper/SemanticTextFieldMapper.java b/x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/mapper/SemanticTextFieldMapper.java index 6f291ded61f1f..cf713546a071a 100644 --- a/x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/mapper/SemanticTextFieldMapper.java +++ b/x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/mapper/SemanticTextFieldMapper.java @@ -91,11 +91,6 @@ protected Parameter[] getParameters() { public SemanticTextFieldMapper build(MapperBuilderContext context) { return new SemanticTextFieldMapper(name(), new SemanticTextFieldType(name(), modelId.getValue(), meta.getValue()), copyTo); } - - @Override - public int mapperSize() { - return 1; - } } public static class SemanticTextFieldType extends SimpleMappedFieldType implements InferenceModelFieldType { diff --git a/x-pack/plugin/spatial/src/test/java/org/elasticsearch/xpack/spatial/index/mapper/GeoShapeWithDocValuesFieldMapperTests.java b/x-pack/plugin/spatial/src/test/java/org/elasticsearch/xpack/spatial/index/mapper/GeoShapeWithDocValuesFieldMapperTests.java index dd5b06ae311b1..0ddb38ea500f1 100644 --- a/x-pack/plugin/spatial/src/test/java/org/elasticsearch/xpack/spatial/index/mapper/GeoShapeWithDocValuesFieldMapperTests.java +++ b/x-pack/plugin/spatial/src/test/java/org/elasticsearch/xpack/spatial/index/mapper/GeoShapeWithDocValuesFieldMapperTests.java @@ -66,12 +66,6 @@ protected void registerParameters(ParameterChecker checker) throws IOException { }); } - @Override - public void testMapperBuilderSizeMultiField() throws IOException { - super.testMapperBuilderSizeMultiField(); - assertWarnings("Adding multifields to [geo_shape] mappers has no effect and will be forbidden in future"); - } - protected AbstractShapeGeometryFieldType fieldType(Mapper fieldMapper) { AbstractShapeGeometryFieldMapper shapeFieldMapper = (AbstractShapeGeometryFieldMapper) fieldMapper; return (AbstractShapeGeometryFieldType) shapeFieldMapper.fieldType(); diff --git a/x-pack/plugin/spatial/src/test/java/org/elasticsearch/xpack/spatial/index/mapper/PointFieldMapperTests.java b/x-pack/plugin/spatial/src/test/java/org/elasticsearch/xpack/spatial/index/mapper/PointFieldMapperTests.java index 3680be6c7bd4f..0e26d9ba0aaf2 100644 --- a/x-pack/plugin/spatial/src/test/java/org/elasticsearch/xpack/spatial/index/mapper/PointFieldMapperTests.java +++ b/x-pack/plugin/spatial/src/test/java/org/elasticsearch/xpack/spatial/index/mapper/PointFieldMapperTests.java @@ -63,12 +63,6 @@ protected void registerParameters(ParameterChecker checker) throws IOException { }); } - @Override - public void testMapperBuilderSizeMultiField() throws IOException { - super.testMapperBuilderSizeMultiField(); - assertWarnings("Adding multifields to [point] mappers has no effect and will be forbidden in future"); - } - public void testAggregationsDocValuesDisabled() throws IOException { MapperService mapperService = createMapperService(fieldMapping(b -> { minimalMapping(b); diff --git a/x-pack/plugin/spatial/src/test/java/org/elasticsearch/xpack/spatial/index/mapper/ShapeFieldMapperTests.java b/x-pack/plugin/spatial/src/test/java/org/elasticsearch/xpack/spatial/index/mapper/ShapeFieldMapperTests.java index aa5e1507c7786..26d349a7ee5a6 100644 --- a/x-pack/plugin/spatial/src/test/java/org/elasticsearch/xpack/spatial/index/mapper/ShapeFieldMapperTests.java +++ b/x-pack/plugin/spatial/src/test/java/org/elasticsearch/xpack/spatial/index/mapper/ShapeFieldMapperTests.java @@ -87,12 +87,6 @@ protected void registerParameters(ParameterChecker checker) throws IOException { }); } - @Override - public void testMapperBuilderSizeMultiField() throws IOException { - super.testMapperBuilderSizeMultiField(); - assertWarnings("Adding multifields to [shape] mappers has no effect and will be forbidden in future"); - } - protected AbstractShapeGeometryFieldType fieldType(Mapper fieldMapper) { AbstractShapeGeometryFieldMapper shapeFieldMapper = (AbstractShapeGeometryFieldMapper) fieldMapper; return (AbstractShapeGeometryFieldType) shapeFieldMapper.fieldType(); From 2518e33f300e9e8e7b3a5ef21b1f080a83541435 Mon Sep 17 00:00:00 2001 From: Felix Barnsteiner Date: Fri, 5 Jan 2024 08:47:33 +0100 Subject: [PATCH 18/40] Remove unused import --- .../org/elasticsearch/index/mapper/FieldAliasMapperTests.java | 1 - 1 file changed, 1 deletion(-) 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 b163d6553477b..f816f403be89f 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/FieldAliasMapperTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/FieldAliasMapperTests.java @@ -9,7 +9,6 @@ package org.elasticsearch.index.mapper; import org.elasticsearch.common.Strings; -import org.elasticsearch.xcontent.XContentBuilder; import org.elasticsearch.xcontent.XContentFactory; import java.io.IOException; From 72c55827399047c38ebe5ae4d0e8f2161cf9f20d Mon Sep 17 00:00:00 2001 From: Felix Barnsteiner Date: Thu, 18 Jan 2024 11:42:01 +0100 Subject: [PATCH 19/40] Update parameter names to mapperMergeContext --- .../elasticsearch/index/mapper/DocumentParser.java | 2 +- .../org/elasticsearch/index/mapper/FieldMapper.java | 10 +++++----- .../java/org/elasticsearch/index/mapper/Mapper.java | 2 +- .../org/elasticsearch/index/mapper/ObjectMapper.java | 12 ++++++------ .../index/mapper/PlaceHolderFieldMapper.java | 4 ++-- .../elasticsearch/index/mapper/RootObjectMapper.java | 8 ++++---- 6 files changed, 19 insertions(+), 19 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java b/server/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java index 3d10372723a6d..e551c4ee05269 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java @@ -822,7 +822,7 @@ private static class NoOpObjectMapper extends ObjectMapper { } @Override - public ObjectMapper merge(Mapper mergeWith, MapperMergeContext mapperBuilderContext) { + public ObjectMapper merge(Mapper mergeWith, MapperMergeContext mapperMergeContext) { return this; } } 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 81e7fcaa02b5b..d72b7c2277f06 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/FieldMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/FieldMapper.java @@ -372,7 +372,7 @@ private static void checkNestedScopeCompatibility(String source, String target) public abstract Builder getMergeBuilder(); @Override - public final FieldMapper merge(Mapper mergeWith, MapperMergeContext mapperBuilderContext) { + public final FieldMapper merge(Mapper mergeWith, MapperMergeContext mapperMergeContext) { if (mergeWith == this) { return this; } @@ -394,9 +394,9 @@ public final FieldMapper merge(Mapper mergeWith, MapperMergeContext mapperBuilde return (FieldMapper) mergeWith; } Conflicts conflicts = new Conflicts(name()); - builder.merge((FieldMapper) mergeWith, conflicts, mapperBuilderContext); + builder.merge((FieldMapper) mergeWith, conflicts, mapperMergeContext); conflicts.check(); - return builder.build(mapperBuilderContext.getMapperBuilderContext()); + return builder.build(mapperMergeContext.getMapperBuilderContext()); } protected void checkIncomingMergeType(FieldMapper mergeWith) { @@ -1224,11 +1224,11 @@ public Builder init(FieldMapper initializer) { return this; } - protected void merge(FieldMapper in, Conflicts conflicts, MapperMergeContext mapperBuilderContext) { + protected void merge(FieldMapper in, Conflicts conflicts, MapperMergeContext mapperMergeContext) { for (Parameter param : getParameters()) { param.merge(in, conflicts); } - MapperMergeContext childContext = mapperBuilderContext.createChildContext(in.simpleName()); + MapperMergeContext childContext = mapperMergeContext.createChildContext(in.simpleName()); for (FieldMapper newSubField : in.multiFields.mappers) { multiFieldsBuilder.update(newSubField, childContext); } 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 7cb4d721cb746..ca15248c037bc 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/Mapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/Mapper.java @@ -77,7 +77,7 @@ public final String simpleName() { * Return the merge of {@code mergeWith} into this. * Both {@code this} and {@code mergeWith} will be left unmodified. */ - public abstract Mapper merge(Mapper mergeWith, MapperMergeContext mapperBuilderContext); + public abstract Mapper merge(Mapper mergeWith, MapperMergeContext mapperMergeContext); /** * Validate any cross-field references made by this mapper 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 ac2ca037c29b2..5c41e5e66eef3 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java @@ -447,8 +447,8 @@ public final boolean subobjects() { } @Override - public ObjectMapper merge(Mapper mergeWith, MapperMergeContext mapperBuilderContext) { - return merge(mergeWith, MergeReason.MAPPING_UPDATE, mapperBuilderContext); + public ObjectMapper merge(Mapper mergeWith, MapperMergeContext mapperMergeContext) { + return merge(mergeWith, MergeReason.MAPPING_UPDATE, mapperMergeContext); } @Override @@ -458,12 +458,12 @@ public void validate(MappingLookup mappers) { } } - protected MapperMergeContext createChildContext(MapperMergeContext mapperBuilderContext, String name) { - return mapperBuilderContext.createChildContext(name); + protected MapperMergeContext createChildContext(MapperMergeContext mapperMergeContext, String name) { + return mapperMergeContext.createChildContext(name); } - public ObjectMapper merge(Mapper mergeWith, MergeReason reason, MapperMergeContext parentBuilderContext) { - var mergeResult = MergeResult.build(this, mergeWith, reason, parentBuilderContext); + public ObjectMapper merge(Mapper mergeWith, MergeReason reason, MapperMergeContext parentMergeContext) { + var mergeResult = MergeResult.build(this, mergeWith, reason, parentMergeContext); return new ObjectMapper( simpleName(), fullPath, diff --git a/server/src/main/java/org/elasticsearch/index/mapper/PlaceHolderFieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/PlaceHolderFieldMapper.java index b1226e0bee41d..98f8f21be704a 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/PlaceHolderFieldMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/PlaceHolderFieldMapper.java @@ -63,10 +63,10 @@ public FieldMapper.Builder init(FieldMapper initializer) { } @Override - protected void merge(FieldMapper in, Conflicts conflicts, MapperMergeContext mapperBuilderContext) { + protected void merge(FieldMapper in, Conflicts conflicts, MapperMergeContext mapperMergeContext) { assert in instanceof PlaceHolderFieldMapper; unknownParams.putAll(((PlaceHolderFieldMapper) in).unknownParams); - super.merge(in, conflicts, mapperBuilderContext); + super.merge(in, conflicts, mapperMergeContext); } @Override 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 dfe302352e3de..958a5825d19d8 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/RootObjectMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/RootObjectMapper.java @@ -213,8 +213,8 @@ protected MapperMergeContext createChildContext(MapperMergeContext mapperMergeCo } @Override - public RootObjectMapper merge(Mapper mergeWith, MergeReason reason, MapperMergeContext parentBuilderContext) { - final var mergeResult = MergeResult.build(this, mergeWith, reason, parentBuilderContext); + public RootObjectMapper merge(Mapper mergeWith, MergeReason reason, MapperMergeContext parentMergeContext) { + final var mergeResult = MergeResult.build(this, mergeWith, reason, parentMergeContext); final Explicit numericDetection; RootObjectMapper mergeWithObject = (RootObjectMapper) mergeWith; if (mergeWithObject.numericDetection.explicit()) { @@ -260,9 +260,9 @@ public RootObjectMapper merge(Mapper mergeWith, MergeReason reason, MapperMergeC assert this.runtimeFields != mergeWithObject.runtimeFields; for (Map.Entry runtimeField : mergeWithObject.runtimeFields.entrySet()) { if (runtimeField.getValue() == null) { - parentBuilderContext.removeRuntimeField(runtimeFields, runtimeField.getKey()); + parentMergeContext.removeRuntimeField(runtimeFields, runtimeField.getKey()); } else { - parentBuilderContext.addRuntimeFieldIfPossible(runtimeFields, runtimeField.getValue()); + parentMergeContext.addRuntimeFieldIfPossible(runtimeFields, runtimeField.getValue()); } } From a8aa0e8fb035c2e55758c77b4699deb447deceed Mon Sep 17 00:00:00 2001 From: Felix Barnsteiner Date: Thu, 18 Jan 2024 12:15:43 +0100 Subject: [PATCH 20/40] Add Javadoc to MapperMergeContext and make it final --- .../org/elasticsearch/index/mapper/MapperMergeContext.java | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/server/src/main/java/org/elasticsearch/index/mapper/MapperMergeContext.java b/server/src/main/java/org/elasticsearch/index/mapper/MapperMergeContext.java index b6130a9e5d1f2..d3ebc2dbe3522 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/MapperMergeContext.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/MapperMergeContext.java @@ -11,7 +11,12 @@ import java.util.Map; import java.util.concurrent.atomic.AtomicLong; -public class MapperMergeContext { +/** + * Holds context used when merging mappings. + * As the merge process also involves building merged {@link Mapper.Builder}s, + * this also contains a {@link MapperBuilderContext}. + */ +public final class MapperMergeContext { private final MapperBuilderContext mapperBuilderContext; private final AtomicLong remainingFieldsUntilLimit; From d3c962b3d7d35e39c07cb9fb33a171ffe145afc2 Mon Sep 17 00:00:00 2001 From: Felix Barnsteiner Date: Thu, 18 Jan 2024 12:29:52 +0100 Subject: [PATCH 21/40] Make MapperService#mergeMappings static again --- .../cluster/metadata/MetadataMappingService.java | 2 +- .../elasticsearch/index/mapper/MapperService.java | 12 ++++++------ .../index/mapper/DocumentParserTests.java | 2 +- 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataMappingService.java b/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataMappingService.java index 0e627456c8409..7a2d20d042f84 100644 --- a/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataMappingService.java +++ b/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataMappingService.java @@ -148,7 +148,7 @@ private static ClusterState applyRequest( // try and parse it (no need to add it here) so we can bail early in case of parsing exception // first, simulate: just call merge and ignore the result Mapping mapping = mapperService.parseMapping(MapperService.SINGLE_MAPPING_NAME, mappingUpdateSource); - mapperService.mergeMappings(mapping, MergeReason.MAPPING_UPDATE); + MapperService.mergeMappings(mapperService.documentMapper(), mapping, MergeReason.MAPPING_UPDATE); } Metadata.Builder builder = Metadata.builder(metadata); boolean updated = false; diff --git a/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java b/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java index f1e7d1385e889..ce332d138ddc4 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java @@ -297,7 +297,7 @@ public void updateMapping(final IndexMetadata currentIndexMetadata, final IndexM DocumentMapper previousMapper; synchronized (this) { previousMapper = this.mapper; - assert assertRefreshIsNotNeeded(type, incomingMapping); + assert assertRefreshIsNotNeeded(previousMapper, type, incomingMapping); this.mapper = newDocumentMapper(incomingMapping, MergeReason.MAPPING_RECOVERY, incomingMappingSource); this.mappingVersion = newIndexMetadata.getMappingVersion(); } @@ -312,8 +312,8 @@ public void updateMapping(final IndexMetadata currentIndexMetadata, final IndexM } } - private boolean assertRefreshIsNotNeeded(String type, Mapping incomingMapping) { - Mapping mergedMapping = mergeMappings(incomingMapping, MergeReason.MAPPING_RECOVERY); + private boolean assertRefreshIsNotNeeded(DocumentMapper currentMapper, String type, Mapping incomingMapping) { + Mapping mergedMapping = mergeMappings(currentMapper, incomingMapping, MergeReason.MAPPING_RECOVERY); // skip the runtime section or removed runtime fields will make the assertion fail ToXContent.MapParams params = new ToXContent.MapParams(Collections.singletonMap(RootObjectMapper.TOXCONTENT_SKIP_RUNTIME, "true")); CompressedXContent mergedMappingSource; @@ -517,7 +517,7 @@ public DocumentMapper merge(String type, CompressedXContent mappingSource, Merge private synchronized DocumentMapper doMerge(String type, MergeReason reason, Map mappingSourceAsMap) { Mapping incomingMapping = parseMapping(type, mappingSourceAsMap); - Mapping mapping = mergeMappings(incomingMapping, reason); + Mapping mapping = mergeMappings(this.mapper, incomingMapping, reason); // TODO: In many cases the source here is equal to mappingSource so we need not serialize again. // We should identify these cases reliably and save expensive serialization here DocumentMapper newMapper = newDocumentMapper(mapping, reason, mapping.toCompressedXContent()); @@ -558,8 +558,8 @@ public Mapping parseMapping(String mappingType, Map mappingSourc } } - public Mapping mergeMappings(Mapping incomingMapping, MergeReason reason) { - return mergeMappings(this.mapper, incomingMapping, reason, Long.MAX_VALUE); + public static Mapping mergeMappings(DocumentMapper currentMapper, Mapping incomingMapping, MergeReason reason) { + return mergeMappings(currentMapper, incomingMapping, reason, Long.MAX_VALUE); } static Mapping mergeMappings( diff --git a/server/src/test/java/org/elasticsearch/index/mapper/DocumentParserTests.java b/server/src/test/java/org/elasticsearch/index/mapper/DocumentParserTests.java index 5bad55ecbe57e..a3706b7ddab18 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/DocumentParserTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/DocumentParserTests.java @@ -2580,7 +2580,7 @@ same name need to be part of the same mappings (hence the same document). If th assertArrayEquals(new String[] { "LongField ", "LongField " }, fieldStrings); // merge without going through toXContent and reparsing, otherwise the potential leaf path issue gets fixed on its own - Mapping newMapping = mapperService.mergeMappings(mapping, MapperService.MergeReason.MAPPING_UPDATE); + Mapping newMapping = MapperService.mergeMappings(mapperService.documentMapper(), mapping, MapperService.MergeReason.MAPPING_UPDATE); DocumentMapper newDocMapper = new DocumentMapper( mapperService.documentParser(), newMapping, From 6574d6d7eb418e664233710620a0a2c0cc31c61a Mon Sep 17 00:00:00 2001 From: Felix Barnsteiner Date: Thu, 18 Jan 2024 15:49:32 +0100 Subject: [PATCH 22/40] Make field adding methods in MapperMergeContext package-private --- .../elasticsearch/index/mapper/MapperMergeContext.java | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/index/mapper/MapperMergeContext.java b/server/src/main/java/org/elasticsearch/index/mapper/MapperMergeContext.java index 9758e288f168f..5b2fe8830dc48 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/MapperMergeContext.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/MapperMergeContext.java @@ -68,7 +68,7 @@ MapperBuilderContext getMapperBuilderContext() { return mapperBuilderContext; } - public void removeRuntimeField(Map runtimeFields, String name) { + void removeRuntimeField(Map runtimeFields, String name) { if (runtimeFields.containsKey(name)) { runtimeFields.remove(name); if (remainingFieldsUntilLimit.get() != Long.MAX_VALUE) { @@ -77,7 +77,7 @@ public void removeRuntimeField(Map runtimeFields, String n } } - public void addRuntimeFieldIfPossible(Map runtimeFields, RuntimeField runtimeField) { + void addRuntimeFieldIfPossible(Map runtimeFields, RuntimeField runtimeField) { if (runtimeFields.containsKey(runtimeField.name())) { runtimeFields.put(runtimeField.name(), runtimeField); } else if (canAddField(1)) { @@ -86,7 +86,7 @@ public void addRuntimeFieldIfPossible(Map runtimeFields, R } } - public boolean addFieldIfPossible(Map mappers, Mapper mapper) { + boolean addFieldIfPossible(Map mappers, Mapper mapper) { if (canAddField(mapper.mapperSize())) { remainingFieldsUntilLimit.getAndAdd(mapper.mapperSize() * -1); mappers.put(mapper.simpleName(), mapper); @@ -95,14 +95,14 @@ public boolean addFieldIfPossible(Map mappers, Mapper mapper) { return false; } - public void addFieldIfPossible(Mapper mapper, Runnable addField) { + void addFieldIfPossible(Mapper mapper, Runnable addField) { if (canAddField(mapper.mapperSize())) { remainingFieldsUntilLimit.getAndAdd(mapper.mapperSize() * -1); addField.run(); } } - public boolean canAddField(int fieldSize) { + boolean canAddField(int fieldSize) { return remainingFieldsUntilLimit.get() >= fieldSize; } } From 274aff0d35914d0ffb3329936b6efa93abad3ebc Mon Sep 17 00:00:00 2001 From: Felix Barnsteiner Date: Fri, 19 Jan 2024 09:31:18 +0100 Subject: [PATCH 23/40] Apply feedback from review --- .../index/mapper/FieldMapper.java | 7 +--- .../index/mapper/MapperMergeContext.java | 33 +++++++++---------- .../index/mapper/MapperService.java | 15 ++------- .../elasticsearch/index/mapper/Mapping.java | 5 +-- .../index/mapper/NestedObjectMapper.java | 3 +- .../index/mapper/ObjectMapper.java | 2 +- .../index/mapper/RootObjectMapper.java | 3 +- 7 files changed, 27 insertions(+), 41 deletions(-) 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 d72b7c2277f06..db7d14646e23f 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/FieldMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/FieldMapper.java @@ -454,14 +454,13 @@ public Builder add(FieldMapper mapper) { return this; } - public Builder update(FieldMapper toMerge, MapperMergeContext context) { + private void update(FieldMapper toMerge, MapperMergeContext context) { if (mapperBuilders.containsKey(toMerge.simpleName()) == false) { context.addFieldIfPossible(toMerge, () -> add(toMerge)); } else { FieldMapper existing = mapperBuilders.get(toMerge.simpleName()).apply(context.getMapperBuilderContext()); add(existing.merge(toMerge, context)); } - return this; } public boolean hasMultiFields() { @@ -481,10 +480,6 @@ public MultiFields build(Mapper.Builder mainFieldBuilder, MapperBuilderContext c return new MultiFields(mappers); } } - - public int mapperSize() { - return mapperBuilders.size(); - } } private final FieldMapper[] mappers; diff --git a/server/src/main/java/org/elasticsearch/index/mapper/MapperMergeContext.java b/server/src/main/java/org/elasticsearch/index/mapper/MapperMergeContext.java index 5b2fe8830dc48..4680fd87afdc9 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/MapperMergeContext.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/MapperMergeContext.java @@ -19,31 +19,28 @@ public final class MapperMergeContext { private final MapperBuilderContext mapperBuilderContext; - private final AtomicLong remainingFieldsUntilLimit; + private final AtomicLong remainingFieldsBudget; /** * The root context, to be used when merging a tree of mappers */ - public static MapperMergeContext root(boolean isSourceSynthetic, boolean isDataStream, long maxFieldsToAddDuringMerge) { - return new MapperMergeContext( - MapperBuilderContext.root(isSourceSynthetic, isDataStream), - new AtomicLong(maxFieldsToAddDuringMerge) - ); + public static MapperMergeContext root(boolean isSourceSynthetic, boolean isDataStream, long newFieldsBudget) { + return new MapperMergeContext(MapperBuilderContext.root(isSourceSynthetic, isDataStream), new AtomicLong(newFieldsBudget)); } /** * Creates a new {@link MapperMergeContext} from a {@link MapperBuilderContext} * @param mapperBuilderContext the {@link MapperBuilderContext} for this {@link MapperMergeContext} - * @param maxFieldsToAddDuringMerge limits how many fields can be added during the merge process + * @param newFieldsBudget limits how many fields can be added during the merge process * @return a new {@link MapperMergeContext}, wrapping the provided {@link MapperBuilderContext} */ - public static MapperMergeContext from(MapperBuilderContext mapperBuilderContext, long maxFieldsToAddDuringMerge) { - return new MapperMergeContext(mapperBuilderContext, new AtomicLong(maxFieldsToAddDuringMerge)); + public static MapperMergeContext from(MapperBuilderContext mapperBuilderContext, long newFieldsBudget) { + return new MapperMergeContext(mapperBuilderContext, new AtomicLong(newFieldsBudget)); } - private MapperMergeContext(MapperBuilderContext mapperBuilderContext, AtomicLong remainingFieldsUntilLimit) { + private MapperMergeContext(MapperBuilderContext mapperBuilderContext, AtomicLong remainingFieldsBudget) { this.mapperBuilderContext = mapperBuilderContext; - this.remainingFieldsUntilLimit = remainingFieldsUntilLimit; + this.remainingFieldsBudget = remainingFieldsBudget; } /** @@ -61,7 +58,7 @@ public MapperMergeContext createChildContext(String name) { * @return a new {@link MapperMergeContext}, wrapping the provided {@link MapperBuilderContext} */ public MapperMergeContext createChildContext(MapperBuilderContext childContext) { - return new MapperMergeContext(childContext, remainingFieldsUntilLimit); + return new MapperMergeContext(childContext, remainingFieldsBudget); } MapperBuilderContext getMapperBuilderContext() { @@ -71,8 +68,8 @@ MapperBuilderContext getMapperBuilderContext() { void removeRuntimeField(Map runtimeFields, String name) { if (runtimeFields.containsKey(name)) { runtimeFields.remove(name); - if (remainingFieldsUntilLimit.get() != Long.MAX_VALUE) { - remainingFieldsUntilLimit.incrementAndGet(); + if (remainingFieldsBudget.get() != Long.MAX_VALUE) { + remainingFieldsBudget.incrementAndGet(); } } } @@ -81,14 +78,14 @@ void addRuntimeFieldIfPossible(Map runtimeFields, RuntimeF if (runtimeFields.containsKey(runtimeField.name())) { runtimeFields.put(runtimeField.name(), runtimeField); } else if (canAddField(1)) { - remainingFieldsUntilLimit.decrementAndGet(); + remainingFieldsBudget.decrementAndGet(); runtimeFields.put(runtimeField.name(), runtimeField); } } boolean addFieldIfPossible(Map mappers, Mapper mapper) { if (canAddField(mapper.mapperSize())) { - remainingFieldsUntilLimit.getAndAdd(mapper.mapperSize() * -1); + remainingFieldsBudget.getAndAdd(mapper.mapperSize() * -1); mappers.put(mapper.simpleName(), mapper); return true; } @@ -97,12 +94,12 @@ boolean addFieldIfPossible(Map mappers, Mapper mapper) { void addFieldIfPossible(Mapper mapper, Runnable addField) { if (canAddField(mapper.mapperSize())) { - remainingFieldsUntilLimit.getAndAdd(mapper.mapperSize() * -1); + remainingFieldsBudget.getAndAdd(mapper.mapperSize() * -1); addField.run(); } } boolean canAddField(int fieldSize) { - return remainingFieldsUntilLimit.get() >= fieldSize; + return remainingFieldsBudget.get() >= fieldSize; } } diff --git a/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java b/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java index ce332d138ddc4..c94d1050cbb19 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java @@ -562,21 +562,12 @@ public static Mapping mergeMappings(DocumentMapper currentMapper, Mapping incomi return mergeMappings(currentMapper, incomingMapping, reason, Long.MAX_VALUE); } - static Mapping mergeMappings( - DocumentMapper currentMapper, - Mapping incomingMapping, - MergeReason reason, - long maxFieldsToAddDuringMerge - ) { + static Mapping mergeMappings(DocumentMapper currentMapper, Mapping incomingMapping, MergeReason reason, long newFieldsBudget) { Mapping newMapping; if (currentMapper == null) { - if (MappingLookup.fromMapping(incomingMapping).exceedsLimit(maxFieldsToAddDuringMerge, 0)) { - newMapping = Mapping.EMPTY.merge(incomingMapping, reason, maxFieldsToAddDuringMerge); - } else { - newMapping = incomingMapping; - } + newMapping = Mapping.EMPTY.merge(incomingMapping, reason, newFieldsBudget); } else { - newMapping = currentMapper.mapping().merge(incomingMapping, reason, maxFieldsToAddDuringMerge); + newMapping = currentMapper.mapping().merge(incomingMapping, reason, newFieldsBudget); } return newMapping; } diff --git a/server/src/main/java/org/elasticsearch/index/mapper/Mapping.java b/server/src/main/java/org/elasticsearch/index/mapper/Mapping.java index 6469dc58e4784..4b8b5225f6b66 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/Mapping.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/Mapping.java @@ -133,10 +133,11 @@ public SourceLoader.SyntheticFieldLoader syntheticFieldLoader() { * * @param mergeWith the new mapping to merge into this one. * @param reason the reason this merge was initiated. + * @param newFieldsBudget how many new fields can be added during the merge process * @return the resulting merged mapping. */ - Mapping merge(Mapping mergeWith, MergeReason reason, long maxFieldsToAddDuringMerge) { - MapperMergeContext mergeContext = MapperMergeContext.root(isSourceSynthetic(), false, maxFieldsToAddDuringMerge); + Mapping merge(Mapping mergeWith, MergeReason reason, long newFieldsBudget) { + MapperMergeContext mergeContext = MapperMergeContext.root(isSourceSynthetic(), false, newFieldsBudget); RootObjectMapper mergedRoot = root.merge(mergeWith.root, reason, mergeContext); // When merging metadata fields as part of applying an index template, new field definitions diff --git a/server/src/main/java/org/elasticsearch/index/mapper/NestedObjectMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/NestedObjectMapper.java index 507ab3fe52366..1ee8a0b1bf80a 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/NestedObjectMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/NestedObjectMapper.java @@ -186,7 +186,8 @@ public ObjectMapper.Builder newBuilder(IndexVersion indexVersionCreated) { return builder; } - public NestedObjectMapper withoutMappers() { + @Override + NestedObjectMapper withoutMappers() { return new NestedObjectMapper( simpleName(), fullPath(), 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 5c41e5e66eef3..4cb3366eb5178 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java @@ -403,7 +403,7 @@ public ObjectMapper.Builder newBuilder(IndexVersion indexVersionCreated) { return builder; } - public ObjectMapper withoutMappers() { + ObjectMapper withoutMappers() { return new ObjectMapper(simpleName(), fullPath, enabled, subobjects, dynamic, null); } 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 958a5825d19d8..3e464de0de91b 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/RootObjectMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/RootObjectMapper.java @@ -155,7 +155,8 @@ public RootObjectMapper.Builder newBuilder(IndexVersion indexVersionCreated) { return builder; } - public RootObjectMapper withoutMappers() { + @Override + RootObjectMapper withoutMappers() { return new RootObjectMapper( simpleName(), enabled, From 08f9c5cbac4cdbb091603c1dcfaa6273525e17fc Mon Sep 17 00:00:00 2001 From: Felix Barnsteiner Date: Fri, 19 Jan 2024 10:52:41 +0100 Subject: [PATCH 24/40] Use Consumer in addFieldIfPossible Avoid passing in Mappers map into addFieldIfPossible, instead rely on the consumer to do that Added some comments to the merge process --- .../index/mapper/FieldMapper.java | 2 +- .../index/mapper/MapperMergeContext.java | 22 ++++++------------- .../index/mapper/ObjectMapper.java | 11 ++++++++-- .../index/mapper/RootObjectMapper.java | 4 +++- .../index/mapper/MapperMergeContextTests.java | 13 ++++++----- 5 files changed, 28 insertions(+), 24 deletions(-) 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 db7d14646e23f..a6b4d927fa0ed 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/FieldMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/FieldMapper.java @@ -456,7 +456,7 @@ public Builder add(FieldMapper mapper) { private void update(FieldMapper toMerge, MapperMergeContext context) { if (mapperBuilders.containsKey(toMerge.simpleName()) == false) { - context.addFieldIfPossible(toMerge, () -> add(toMerge)); + context.addFieldIfPossible(toMerge, this::add); } else { FieldMapper existing = mapperBuilders.get(toMerge.simpleName()).apply(context.getMapperBuilderContext()); add(existing.merge(toMerge, context)); diff --git a/server/src/main/java/org/elasticsearch/index/mapper/MapperMergeContext.java b/server/src/main/java/org/elasticsearch/index/mapper/MapperMergeContext.java index 4680fd87afdc9..54f9dfb4b7980 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/MapperMergeContext.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/MapperMergeContext.java @@ -10,6 +10,7 @@ import java.util.Map; import java.util.concurrent.atomic.AtomicLong; +import java.util.function.Consumer; /** * Holds context used when merging mappings. @@ -74,28 +75,19 @@ void removeRuntimeField(Map runtimeFields, String name) { } } - void addRuntimeFieldIfPossible(Map runtimeFields, RuntimeField runtimeField) { - if (runtimeFields.containsKey(runtimeField.name())) { - runtimeFields.put(runtimeField.name(), runtimeField); - } else if (canAddField(1)) { - remainingFieldsBudget.decrementAndGet(); - runtimeFields.put(runtimeField.name(), runtimeField); - } - } - - boolean addFieldIfPossible(Map mappers, Mapper mapper) { + boolean addFieldIfPossible(M mapper, Consumer addField) { if (canAddField(mapper.mapperSize())) { remainingFieldsBudget.getAndAdd(mapper.mapperSize() * -1); - mappers.put(mapper.simpleName(), mapper); + addField.accept(mapper); return true; } return false; } - void addFieldIfPossible(Mapper mapper, Runnable addField) { - if (canAddField(mapper.mapperSize())) { - remainingFieldsBudget.getAndAdd(mapper.mapperSize() * -1); - addField.run(); + void addRuntimeFieldIfPossible(RuntimeField runtimeField, Consumer addField) { + if (canAddField(1)) { + remainingFieldsBudget.decrementAndGet(); + addField.accept(runtimeField); } } 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 4cb3366eb5178..e8d10f724f5fd 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java @@ -543,13 +543,20 @@ private static Map buildMergedMappers( if (mergeIntoMapper == null) { if (mergeWithMapper instanceof ObjectMapper om) { + // We may not be able to fully add the object due to the field budget, + // so we're just adding the object, without it's sub-fields. ObjectMapper withoutMappers = om.withoutMappers(); - boolean added = objectMergeContext.addFieldIfPossible(mergedMappers, withoutMappers); + boolean added = objectMergeContext.addFieldIfPossible(withoutMappers, o -> mergedMappers.put(o.simpleName(), o)); if (added) { + // If we were able to add the empty object, + // we're then adding the sub-fields one by one via a merge mergedMappers.put(om.simpleName(), withoutMappers.merge(mergeWithMapper, reason, objectMergeContext)); } } else { - objectMergeContext.addFieldIfPossible(mergedMappers, mergeWithMapper); + objectMergeContext.addFieldIfPossible( + mergeWithMapper, + m -> mergedMappers.put(mergeWithMapper.simpleName(), mergeWithMapper) + ); } } else if (mergeIntoMapper instanceof ObjectMapper objectMapper) { mergedMappers.put(objectMapper.simpleName(), objectMapper.merge(mergeWithMapper, reason, objectMergeContext)); 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 3e464de0de91b..b3e592de320e2 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/RootObjectMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/RootObjectMapper.java @@ -262,8 +262,10 @@ public RootObjectMapper merge(Mapper mergeWith, MergeReason reason, MapperMergeC for (Map.Entry runtimeField : mergeWithObject.runtimeFields.entrySet()) { if (runtimeField.getValue() == null) { parentMergeContext.removeRuntimeField(runtimeFields, runtimeField.getKey()); + } else if (runtimeFields.containsKey(runtimeField.getKey())) { + runtimeFields.put(runtimeField.getKey(), runtimeField.getValue()); } else { - parentMergeContext.addRuntimeFieldIfPossible(runtimeFields, runtimeField.getValue()); + parentMergeContext.addRuntimeFieldIfPossible(runtimeField.getValue(), r -> runtimeFields.put(r.name(), r)); } } diff --git a/server/src/test/java/org/elasticsearch/index/mapper/MapperMergeContextTests.java b/server/src/test/java/org/elasticsearch/index/mapper/MapperMergeContextTests.java index 18e34963d4a9e..3ec4f2c6b9cf9 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/MapperMergeContextTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/MapperMergeContextTests.java @@ -12,6 +12,7 @@ import org.elasticsearch.test.ESTestCase; import java.util.HashMap; +import java.util.Map; public class MapperMergeContextTests extends ESTestCase { @@ -19,7 +20,8 @@ public void testAddFieldIfPossibleUnderLimit() { MapperMergeContext context = MapperMergeContext.root(false, false, 1); assertTrue(context.canAddField(1)); HashMap mappers = new HashMap<>(); - context.addFieldIfPossible(mappers, getKeywordFieldMapper()); + Mapper mapper = getKeywordFieldMapper(); + context.addFieldIfPossible(mapper, m -> ((Map) mappers).put(mapper.simpleName(), mapper)); assertEquals(1, mappers.size()); assertFalse(context.canAddField(1)); } @@ -28,7 +30,8 @@ public void testAddFieldIfPossibleAtLimit() { MapperMergeContext context = MapperMergeContext.root(false, false, 0); assertFalse(context.canAddField(1)); HashMap mappers = new HashMap<>(); - context.addFieldIfPossible(mappers, getKeywordFieldMapper()); + Mapper mapper = getKeywordFieldMapper(); + context.addFieldIfPossible(mapper, m -> ((Map) mappers).put(mapper.simpleName(), mapper)); assertEquals(0, mappers.size()); assertFalse(context.canAddField(1)); } @@ -37,7 +40,7 @@ public void testAddRuntimeFieldIfPossibleUnderLimit() { MapperMergeContext context = MapperMergeContext.root(false, false, 1); assertTrue(context.canAddField(1)); HashMap runtimeFields = new HashMap<>(); - context.addRuntimeFieldIfPossible(runtimeFields, new TestRuntimeField("foo", "keyword")); + context.addRuntimeFieldIfPossible(new TestRuntimeField("foo", "keyword"), r -> runtimeFields.put(r.name(), r)); assertEquals(1, runtimeFields.size()); assertFalse(context.canAddField(1)); } @@ -46,7 +49,7 @@ public void testAddRuntimeFieldIfPossibleAtLimit() { MapperMergeContext context = MapperMergeContext.root(false, false, 0); assertFalse(context.canAddField(1)); HashMap runtimeFields = new HashMap<>(); - context.addRuntimeFieldIfPossible(runtimeFields, new TestRuntimeField("foo", "keyword")); + context.addRuntimeFieldIfPossible(new TestRuntimeField("foo", "keyword"), r -> runtimeFields.put(r.name(), r)); assertEquals(0, runtimeFields.size()); assertFalse(context.canAddField(1)); } @@ -54,7 +57,7 @@ public void testAddRuntimeFieldIfPossibleAtLimit() { public void testRemoveRuntimeField() { MapperMergeContext context = MapperMergeContext.root(false, false, 1); HashMap runtimeFields = new HashMap<>(); - context.addRuntimeFieldIfPossible(runtimeFields, new TestRuntimeField("foo", "keyword")); + context.addRuntimeFieldIfPossible(new TestRuntimeField("foo", "keyword"), r -> runtimeFields.put(r.name(), r)); assertEquals(1, runtimeFields.size()); assertFalse(context.canAddField(1)); From cf8a6ba883b7db7e480f2ae5c3ef89208c89d0ee Mon Sep 17 00:00:00 2001 From: Felix Barnsteiner Date: Mon, 22 Jan 2024 08:51:59 +0100 Subject: [PATCH 25/40] Extract ObjectMapper#addNewObjectMapper As an additional optimization, try to add the full object first. Only merge sub-mappers one by one in case there wasn't enough budget for the full object mappper. --- .../index/mapper/ObjectMapper.java | 41 ++++++++++++++----- 1 file changed, 31 insertions(+), 10 deletions(-) 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 e8d10f724f5fd..9d586ce6764da 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java @@ -537,21 +537,18 @@ private static Map buildMergedMappers( MergeReason reason, MapperMergeContext objectMergeContext ) { + Iterator iterator = mergeWith.iterator(); + if (iterator.hasNext() == false) { + return Map.copyOf(existing.mappers); + } Map mergedMappers = new HashMap<>(existing.mappers); - for (Mapper mergeWithMapper : mergeWith) { + while (iterator.hasNext()) { + Mapper mergeWithMapper = iterator.next(); Mapper mergeIntoMapper = mergedMappers.get(mergeWithMapper.simpleName()); if (mergeIntoMapper == null) { if (mergeWithMapper instanceof ObjectMapper om) { - // We may not be able to fully add the object due to the field budget, - // so we're just adding the object, without it's sub-fields. - ObjectMapper withoutMappers = om.withoutMappers(); - boolean added = objectMergeContext.addFieldIfPossible(withoutMappers, o -> mergedMappers.put(o.simpleName(), o)); - if (added) { - // If we were able to add the empty object, - // we're then adding the sub-fields one by one via a merge - mergedMappers.put(om.simpleName(), withoutMappers.merge(mergeWithMapper, reason, objectMergeContext)); - } + addNewObjectMapper(reason, objectMergeContext, mergedMappers, om); } else { objectMergeContext.addFieldIfPossible( mergeWithMapper, @@ -579,6 +576,30 @@ private static Map buildMergedMappers( } return Map.copyOf(mergedMappers); } + + private static void addNewObjectMapper( + MergeReason reason, + MapperMergeContext objectMergeContext, + Map mergedMappers, + ObjectMapper objectMapper + ) { + // first try to add the full object, with all it's sub-fields + boolean fullObjectAdded = objectMergeContext.addFieldIfPossible(objectMapper, o -> mergedMappers.put(o.simpleName(), o)); + if (fullObjectAdded) { + return; + } + // 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(); + boolean shallowObjectAdded = objectMergeContext.addFieldIfPossible( + shallowObjectMapper, + o -> mergedMappers.put(o.simpleName(), o) + ); + if (shallowObjectAdded) { + // now trying to add the sub-fields one by one via a merge, until we hit the limit + mergedMappers.put(objectMapper.simpleName(), shallowObjectMapper.merge(objectMapper, reason, objectMergeContext)); + } + } } @Override From 56d7cf1cdd6e8abc17fc8281e7f4641019acdeb3 Mon Sep 17 00:00:00 2001 From: Felix Barnsteiner Date: Mon, 22 Jan 2024 08:56:30 +0100 Subject: [PATCH 26/40] Make `FieldMapper.MultiFields.Builder.add(FieldMapper)` private --- .../main/java/org/elasticsearch/index/mapper/FieldMapper.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) 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 a6b4d927fa0ed..0a3ab0a31557a 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/FieldMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/FieldMapper.java @@ -449,9 +449,8 @@ public Builder add(FieldMapper.Builder builder) { return this; } - public Builder add(FieldMapper mapper) { + private void add(FieldMapper mapper) { mapperBuilders.put(mapper.simpleName(), context -> mapper); - return this; } private void update(FieldMapper toMerge, MapperMergeContext context) { From 48e2f33ebf798b703e8d0c62101d12639ab548d4 Mon Sep 17 00:00:00 2001 From: Felix Barnsteiner Date: Mon, 22 Jan 2024 10:24:45 +0100 Subject: [PATCH 27/40] Fix merge with empty mapping --- .../org/elasticsearch/index/mapper/MapperService.java | 2 +- .../java/org/elasticsearch/index/mapper/Mapping.java | 9 +++++++++ .../org/elasticsearch/index/mapper/RootObjectMapper.java | 4 ++-- 3 files changed, 12 insertions(+), 3 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java b/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java index c94d1050cbb19..61dd444be34b1 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java @@ -565,7 +565,7 @@ public static Mapping mergeMappings(DocumentMapper currentMapper, Mapping incomi static Mapping mergeMappings(DocumentMapper currentMapper, Mapping incomingMapping, MergeReason reason, long newFieldsBudget) { Mapping newMapping; if (currentMapper == null) { - newMapping = Mapping.EMPTY.merge(incomingMapping, reason, newFieldsBudget); + newMapping = incomingMapping.withFieldsBudget(newFieldsBudget); } else { newMapping = currentMapper.mapping().merge(incomingMapping, reason, newFieldsBudget); } diff --git a/server/src/main/java/org/elasticsearch/index/mapper/Mapping.java b/server/src/main/java/org/elasticsearch/index/mapper/Mapping.java index 4b8b5225f6b66..a3ac7c5a5bced 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/Mapping.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/Mapping.java @@ -171,6 +171,15 @@ Mapping merge(Mapping mergeWith, MergeReason reason, long newFieldsBudget) { return new Mapping(mergedRoot, mergedMetadataMappers.values().toArray(new MetadataFieldMapper[0]), mergedMeta); } + public Mapping withFieldsBudget(long fieldsBudget) { + MapperMergeContext mergeContext = MapperMergeContext.root(isSourceSynthetic(), false, fieldsBudget); + return new Mapping( + root.withoutMappers().merge(root, MergeReason.MAPPING_RECOVERY, mergeContext), + Arrays.copyOf(metadataMappers, metadataMappers.length), + meta == null ? null : Map.copyOf(meta) + ); + } + @Override public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { root.toXContent(builder, params, (b, params1) -> { 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 b3e592de320e2..32b1825377703 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/RootObjectMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/RootObjectMapper.java @@ -162,8 +162,8 @@ RootObjectMapper withoutMappers() { enabled, subobjects, dynamic, - null, - null, + Map.of(), + Map.of(), dynamicDateTimeFormatters, dynamicTemplates, dateDetection, From 3e5d65b5aff85f75ca51a7ec5ef4e4a50e58da69 Mon Sep 17 00:00:00 2001 From: Felix Barnsteiner Date: Mon, 22 Jan 2024 10:59:54 +0100 Subject: [PATCH 28/40] Attempt to fix mapper serialization assertion --- .../src/main/java/org/elasticsearch/index/mapper/Mapping.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/index/mapper/Mapping.java b/server/src/main/java/org/elasticsearch/index/mapper/Mapping.java index a3ac7c5a5bced..a7ec5de59738d 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/Mapping.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/Mapping.java @@ -175,8 +175,8 @@ public Mapping withFieldsBudget(long fieldsBudget) { MapperMergeContext mergeContext = MapperMergeContext.root(isSourceSynthetic(), false, fieldsBudget); return new Mapping( root.withoutMappers().merge(root, MergeReason.MAPPING_RECOVERY, mergeContext), - Arrays.copyOf(metadataMappers, metadataMappers.length), - meta == null ? null : Map.copyOf(meta) + metadataMappers, + meta ); } From 1751c708f39dbb1a3c88244aca5e9fa3c3beca9f Mon Sep 17 00:00:00 2001 From: Felix Barnsteiner Date: Mon, 22 Jan 2024 12:09:04 +0100 Subject: [PATCH 29/40] Apply spotless suggestions --- .../main/java/org/elasticsearch/index/mapper/Mapping.java | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/index/mapper/Mapping.java b/server/src/main/java/org/elasticsearch/index/mapper/Mapping.java index a7ec5de59738d..489e31e6f2a68 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/Mapping.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/Mapping.java @@ -173,11 +173,7 @@ Mapping merge(Mapping mergeWith, MergeReason reason, long newFieldsBudget) { public Mapping withFieldsBudget(long fieldsBudget) { MapperMergeContext mergeContext = MapperMergeContext.root(isSourceSynthetic(), false, fieldsBudget); - return new Mapping( - root.withoutMappers().merge(root, MergeReason.MAPPING_RECOVERY, mergeContext), - metadataMappers, - meta - ); + return new Mapping(root.withoutMappers().merge(root, MergeReason.MAPPING_RECOVERY, mergeContext), metadataMappers, meta); } @Override From a6d0e2f8bd67cd0bc15a2ee401f261a60ba787a5 Mon Sep 17 00:00:00 2001 From: Felix Barnsteiner Date: Mon, 22 Jan 2024 13:02:16 +0100 Subject: [PATCH 30/40] Add NewFieldsBudget class --- .../index/mapper/MapperMergeContext.java | 102 ++++++++++++++---- .../index/mapper/MapperService.java | 10 +- .../elasticsearch/index/mapper/Mapping.java | 5 +- .../index/mapper/ParsedDocument.java | 3 +- .../index/mapper/DocumentMapperTests.java | 32 ++++-- .../index/mapper/MapperMergeContextTests.java | 31 +++--- .../index/mapper/NestedObjectMapperTests.java | 4 +- .../index/mapper/ObjectMapperMergeTests.java | 39 +++---- .../index/mapper/ObjectMapperTests.java | 7 +- .../index/mapper/ParametrizedMapperTests.java | 17 +-- 10 files changed, 171 insertions(+), 79 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/index/mapper/MapperMergeContext.java b/server/src/main/java/org/elasticsearch/index/mapper/MapperMergeContext.java index 54f9dfb4b7980..679dafc467f07 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/MapperMergeContext.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/MapperMergeContext.java @@ -9,7 +9,6 @@ package org.elasticsearch.index.mapper; import java.util.Map; -import java.util.concurrent.atomic.AtomicLong; import java.util.function.Consumer; /** @@ -20,13 +19,18 @@ public final class MapperMergeContext { private final MapperBuilderContext mapperBuilderContext; - private final AtomicLong remainingFieldsBudget; + private final NewFieldsBudget remainingFieldsBudget; + + private MapperMergeContext(MapperBuilderContext mapperBuilderContext, NewFieldsBudget remainingFieldsBudget) { + this.mapperBuilderContext = mapperBuilderContext; + this.remainingFieldsBudget = remainingFieldsBudget; + } /** * The root context, to be used when merging a tree of mappers */ - public static MapperMergeContext root(boolean isSourceSynthetic, boolean isDataStream, long newFieldsBudget) { - return new MapperMergeContext(MapperBuilderContext.root(isSourceSynthetic, isDataStream), new AtomicLong(newFieldsBudget)); + public static MapperMergeContext root(boolean isSourceSynthetic, boolean isDataStream, NewFieldsBudget newFieldsBudget) { + return new MapperMergeContext(MapperBuilderContext.root(isSourceSynthetic, isDataStream), newFieldsBudget); } /** @@ -36,12 +40,7 @@ public static MapperMergeContext root(boolean isSourceSynthetic, boolean isDataS * @return a new {@link MapperMergeContext}, wrapping the provided {@link MapperBuilderContext} */ public static MapperMergeContext from(MapperBuilderContext mapperBuilderContext, long newFieldsBudget) { - return new MapperMergeContext(mapperBuilderContext, new AtomicLong(newFieldsBudget)); - } - - private MapperMergeContext(MapperBuilderContext mapperBuilderContext, AtomicLong remainingFieldsBudget) { - this.mapperBuilderContext = mapperBuilderContext; - this.remainingFieldsBudget = remainingFieldsBudget; + return new MapperMergeContext(mapperBuilderContext, NewFieldsBudget.of(newFieldsBudget)); } /** @@ -69,15 +68,12 @@ MapperBuilderContext getMapperBuilderContext() { void removeRuntimeField(Map runtimeFields, String name) { if (runtimeFields.containsKey(name)) { runtimeFields.remove(name); - if (remainingFieldsBudget.get() != Long.MAX_VALUE) { - remainingFieldsBudget.incrementAndGet(); - } + remainingFieldsBudget.increment(1); } } boolean addFieldIfPossible(M mapper, Consumer addField) { - if (canAddField(mapper.mapperSize())) { - remainingFieldsBudget.getAndAdd(mapper.mapperSize() * -1); + if (remainingFieldsBudget.decrementIfPossible(mapper.mapperSize())) { addField.accept(mapper); return true; } @@ -85,13 +81,81 @@ boolean addFieldIfPossible(M mapper, Consumer addField) { } void addRuntimeFieldIfPossible(RuntimeField runtimeField, Consumer addField) { - if (canAddField(1)) { - remainingFieldsBudget.decrementAndGet(); + if (remainingFieldsBudget.decrementIfPossible(1)) { addField.accept(runtimeField); } } - boolean canAddField(int fieldSize) { - return remainingFieldsBudget.get() >= fieldSize; + boolean hasRemainingBudget() { + return remainingFieldsBudget.hasRemainingBudget(); + } + + /** + * Keeps track of now many new fields can be added during mapper merge + */ + public interface NewFieldsBudget { + static NewFieldsBudget unlimited() { + return Unlimited.INSTANCE; + } + + static NewFieldsBudget of(long fieldsBudget) { + return new Limited(fieldsBudget); + } + + boolean decrementIfPossible(long fieldSize); + + void increment(long fieldSize); + + boolean hasRemainingBudget(); + + class Unlimited implements NewFieldsBudget { + + private static final Unlimited INSTANCE = new Unlimited(); + + private Unlimited() {} + + @Override + public boolean decrementIfPossible(long fieldSize) { + return true; + } + + @Override + public void increment(long fieldSize) { + // noop + } + + @Override + public boolean hasRemainingBudget() { + return true; + } + } + + class Limited implements NewFieldsBudget { + + private long fieldsBudget; + + public Limited(long fieldsBudget) { + this.fieldsBudget = fieldsBudget; + } + + @Override + public boolean decrementIfPossible(long fieldSize) { + if (fieldsBudget >= fieldSize) { + fieldsBudget -= fieldSize; + return true; + } + return false; + } + + @Override + public void increment(long fieldSize) { + fieldsBudget++; + } + + @Override + public boolean hasRemainingBudget() { + return fieldsBudget >= 1; + } + } } } diff --git a/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java b/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java index 61dd444be34b1..9d8c8ab3d8352 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java @@ -25,6 +25,7 @@ import org.elasticsearch.index.analysis.AnalysisRegistry; import org.elasticsearch.index.analysis.IndexAnalyzers; import org.elasticsearch.index.analysis.NamedAnalyzer; +import org.elasticsearch.index.mapper.MapperMergeContext.NewFieldsBudget; import org.elasticsearch.index.query.SearchExecutionContext; import org.elasticsearch.index.similarity.SimilarityService; import org.elasticsearch.indices.IndicesModule; @@ -559,10 +560,15 @@ public Mapping parseMapping(String mappingType, Map mappingSourc } public static Mapping mergeMappings(DocumentMapper currentMapper, Mapping incomingMapping, MergeReason reason) { - return mergeMappings(currentMapper, incomingMapping, reason, Long.MAX_VALUE); + return mergeMappings(currentMapper, incomingMapping, reason, NewFieldsBudget.unlimited()); } - static Mapping mergeMappings(DocumentMapper currentMapper, Mapping incomingMapping, MergeReason reason, long newFieldsBudget) { + static Mapping mergeMappings( + DocumentMapper currentMapper, + Mapping incomingMapping, + MergeReason reason, + NewFieldsBudget newFieldsBudget + ) { Mapping newMapping; if (currentMapper == null) { newMapping = incomingMapping.withFieldsBudget(newFieldsBudget); diff --git a/server/src/main/java/org/elasticsearch/index/mapper/Mapping.java b/server/src/main/java/org/elasticsearch/index/mapper/Mapping.java index 489e31e6f2a68..4ead2350fe6f4 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/Mapping.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/Mapping.java @@ -12,6 +12,7 @@ import org.elasticsearch.common.Strings; import org.elasticsearch.common.compress.CompressedXContent; import org.elasticsearch.common.xcontent.XContentHelper; +import org.elasticsearch.index.mapper.MapperMergeContext.NewFieldsBudget; import org.elasticsearch.index.mapper.MapperService.MergeReason; import org.elasticsearch.xcontent.ToXContentFragment; import org.elasticsearch.xcontent.XContentBuilder; @@ -136,7 +137,7 @@ public SourceLoader.SyntheticFieldLoader syntheticFieldLoader() { * @param newFieldsBudget how many new fields can be added during the merge process * @return the resulting merged mapping. */ - Mapping merge(Mapping mergeWith, MergeReason reason, long newFieldsBudget) { + Mapping merge(Mapping mergeWith, MergeReason reason, NewFieldsBudget newFieldsBudget) { MapperMergeContext mergeContext = MapperMergeContext.root(isSourceSynthetic(), false, newFieldsBudget); RootObjectMapper mergedRoot = root.merge(mergeWith.root, reason, mergeContext); @@ -171,7 +172,7 @@ Mapping merge(Mapping mergeWith, MergeReason reason, long newFieldsBudget) { return new Mapping(mergedRoot, mergedMetadataMappers.values().toArray(new MetadataFieldMapper[0]), mergedMeta); } - public Mapping withFieldsBudget(long fieldsBudget) { + public Mapping withFieldsBudget(NewFieldsBudget fieldsBudget) { MapperMergeContext mergeContext = MapperMergeContext.root(isSourceSynthetic(), false, fieldsBudget); return new Mapping(root.withoutMappers().merge(root, MergeReason.MAPPING_RECOVERY, mergeContext), metadataMappers, meta); } diff --git a/server/src/main/java/org/elasticsearch/index/mapper/ParsedDocument.java b/server/src/main/java/org/elasticsearch/index/mapper/ParsedDocument.java index 014be192a4133..ed5431541a2f0 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/ParsedDocument.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/ParsedDocument.java @@ -13,6 +13,7 @@ import org.apache.lucene.util.BytesRef; import org.elasticsearch.common.bytes.BytesArray; import org.elasticsearch.common.bytes.BytesReference; +import org.elasticsearch.index.mapper.MapperMergeContext.NewFieldsBudget; import org.elasticsearch.index.mapper.MapperService.MergeReason; import org.elasticsearch.xcontent.XContentType; @@ -160,7 +161,7 @@ public void addDynamicMappingsUpdate(Mapping update) { if (dynamicMappingsUpdate == null) { dynamicMappingsUpdate = update; } else { - dynamicMappingsUpdate = dynamicMappingsUpdate.merge(update, MergeReason.MAPPING_UPDATE, Long.MAX_VALUE); + dynamicMappingsUpdate = dynamicMappingsUpdate.merge(update, MergeReason.MAPPING_UPDATE, NewFieldsBudget.unlimited()); } } diff --git a/server/src/test/java/org/elasticsearch/index/mapper/DocumentMapperTests.java b/server/src/test/java/org/elasticsearch/index/mapper/DocumentMapperTests.java index e935ae2431131..66af997a637d8 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/DocumentMapperTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/DocumentMapperTests.java @@ -20,6 +20,7 @@ import org.elasticsearch.index.analysis.AnalyzerScope; import org.elasticsearch.index.analysis.IndexAnalyzers; import org.elasticsearch.index.analysis.NamedAnalyzer; +import org.elasticsearch.index.mapper.MapperMergeContext.NewFieldsBudget; import org.elasticsearch.index.mapper.MapperService.MergeReason; import org.elasticsearch.plugins.internal.DocumentParsingObserver; import org.elasticsearch.xcontent.XContentBuilder; @@ -63,7 +64,7 @@ public void testAddFields() throws Exception { })); MergeReason reason = randomFrom(MergeReason.MAPPING_UPDATE, MergeReason.INDEX_TEMPLATE); - Mapping merged = MapperService.mergeMappings(stage1, stage2.mapping(), reason, Long.MAX_VALUE); + Mapping merged = MapperService.mergeMappings(stage1, stage2.mapping(), reason, NewFieldsBudget.unlimited()); // stage1 mapping should not have been modified assertThat(stage1.mappers().getMapper("age"), nullValue()); assertThat(stage1.mappers().getMapper("obj1.prop1"), nullValue()); @@ -81,7 +82,12 @@ public void testMergeObjectDynamic() throws Exception { DocumentMapper withDynamicMapper = createDocumentMapper(topMapping(b -> b.field("dynamic", "false"))); assertThat(withDynamicMapper.mapping().getRoot().dynamic(), equalTo(ObjectMapper.Dynamic.FALSE)); - Mapping merged = MapperService.mergeMappings(mapper, withDynamicMapper.mapping(), MergeReason.MAPPING_UPDATE, Long.MAX_VALUE); + Mapping merged = MapperService.mergeMappings( + mapper, + withDynamicMapper.mapping(), + MergeReason.MAPPING_UPDATE, + NewFieldsBudget.unlimited() + ); assertThat(merged.getRoot().dynamic(), equalTo(ObjectMapper.Dynamic.FALSE)); } @@ -93,14 +99,14 @@ public void testMergeObjectAndNested() throws Exception { { IllegalArgumentException e = expectThrows( IllegalArgumentException.class, - () -> MapperService.mergeMappings(objectMapper, nestedMapper.mapping(), reason, Long.MAX_VALUE) + () -> MapperService.mergeMappings(objectMapper, nestedMapper.mapping(), reason, NewFieldsBudget.unlimited()) ); assertThat(e.getMessage(), containsString("can't merge a non-nested mapping [obj] with a nested mapping")); } { IllegalArgumentException e = expectThrows( IllegalArgumentException.class, - () -> MapperService.mergeMappings(nestedMapper, objectMapper.mapping(), reason, Long.MAX_VALUE) + () -> MapperService.mergeMappings(nestedMapper, objectMapper.mapping(), reason, NewFieldsBudget.unlimited()) ); assertThat(e.getMessage(), containsString("can't merge a non-nested mapping [obj] with a nested mapping")); } @@ -235,11 +241,16 @@ public void testMergeMeta() throws IOException { DocumentMapper updatedMapper = createDocumentMapper(fieldMapping(b -> b.field("type", "text"))); - Mapping merged = MapperService.mergeMappings(initMapper, updatedMapper.mapping(), MergeReason.MAPPING_UPDATE, Long.MAX_VALUE); + Mapping merged = MapperService.mergeMappings( + initMapper, + updatedMapper.mapping(), + MergeReason.MAPPING_UPDATE, + NewFieldsBudget.unlimited() + ); assertThat(merged.getMeta().get("foo"), equalTo("bar")); updatedMapper = createDocumentMapper(topMapping(b -> b.startObject("_meta").field("foo", "new_bar").endObject())); - merged = MapperService.mergeMappings(initMapper, updatedMapper.mapping(), MergeReason.MAPPING_UPDATE, Long.MAX_VALUE); + merged = MapperService.mergeMappings(initMapper, updatedMapper.mapping(), MergeReason.MAPPING_UPDATE, NewFieldsBudget.unlimited()); assertThat(merged.getMeta().get("foo"), equalTo("new_bar")); } @@ -262,7 +273,12 @@ public void testMergeMetaForIndexTemplate() throws IOException { assertThat(initMapper.mapping().getMeta(), equalTo(expected)); DocumentMapper updatedMapper = createDocumentMapper(fieldMapping(b -> b.field("type", "text"))); - Mapping merged = MapperService.mergeMappings(initMapper, updatedMapper.mapping(), MergeReason.INDEX_TEMPLATE, Long.MAX_VALUE); + Mapping merged = MapperService.mergeMappings( + initMapper, + updatedMapper.mapping(), + MergeReason.INDEX_TEMPLATE, + NewFieldsBudget.unlimited() + ); assertThat(merged.getMeta(), equalTo(expected)); updatedMapper = createDocumentMapper(topMapping(b -> { @@ -278,7 +294,7 @@ public void testMergeMetaForIndexTemplate() throws IOException { } b.endObject(); })); - merged = merged.merge(updatedMapper.mapping(), MergeReason.INDEX_TEMPLATE, Long.MAX_VALUE); + merged = merged.merge(updatedMapper.mapping(), MergeReason.INDEX_TEMPLATE, NewFieldsBudget.unlimited()); expected = Map.of("field", "value", "object", Map.of("field1", "value1", "field2", "new_value", "field3", "value3")); assertThat(merged.getMeta(), equalTo(expected)); diff --git a/server/src/test/java/org/elasticsearch/index/mapper/MapperMergeContextTests.java b/server/src/test/java/org/elasticsearch/index/mapper/MapperMergeContextTests.java index 3ec4f2c6b9cf9..8f41727d7b2a5 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/MapperMergeContextTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/MapperMergeContextTests.java @@ -9,6 +9,7 @@ package org.elasticsearch.index.mapper; import org.elasticsearch.index.IndexVersion; +import org.elasticsearch.index.mapper.MapperMergeContext.NewFieldsBudget; import org.elasticsearch.test.ESTestCase; import java.util.HashMap; @@ -17,52 +18,52 @@ public class MapperMergeContextTests extends ESTestCase { public void testAddFieldIfPossibleUnderLimit() { - MapperMergeContext context = MapperMergeContext.root(false, false, 1); - assertTrue(context.canAddField(1)); + MapperMergeContext context = MapperMergeContext.root(false, false, NewFieldsBudget.of(1)); + assertTrue(context.hasRemainingBudget()); HashMap mappers = new HashMap<>(); Mapper mapper = getKeywordFieldMapper(); context.addFieldIfPossible(mapper, m -> ((Map) mappers).put(mapper.simpleName(), mapper)); assertEquals(1, mappers.size()); - assertFalse(context.canAddField(1)); + assertFalse(context.hasRemainingBudget()); } public void testAddFieldIfPossibleAtLimit() { - MapperMergeContext context = MapperMergeContext.root(false, false, 0); - assertFalse(context.canAddField(1)); + MapperMergeContext context = MapperMergeContext.root(false, false, NewFieldsBudget.of(0)); + assertFalse(context.hasRemainingBudget()); HashMap mappers = new HashMap<>(); Mapper mapper = getKeywordFieldMapper(); context.addFieldIfPossible(mapper, m -> ((Map) mappers).put(mapper.simpleName(), mapper)); assertEquals(0, mappers.size()); - assertFalse(context.canAddField(1)); + assertFalse(context.hasRemainingBudget()); } public void testAddRuntimeFieldIfPossibleUnderLimit() { - MapperMergeContext context = MapperMergeContext.root(false, false, 1); - assertTrue(context.canAddField(1)); + MapperMergeContext context = MapperMergeContext.root(false, false, NewFieldsBudget.of(1)); + assertTrue(context.hasRemainingBudget()); HashMap runtimeFields = new HashMap<>(); context.addRuntimeFieldIfPossible(new TestRuntimeField("foo", "keyword"), r -> runtimeFields.put(r.name(), r)); assertEquals(1, runtimeFields.size()); - assertFalse(context.canAddField(1)); + assertFalse(context.hasRemainingBudget()); } public void testAddRuntimeFieldIfPossibleAtLimit() { - MapperMergeContext context = MapperMergeContext.root(false, false, 0); - assertFalse(context.canAddField(1)); + MapperMergeContext context = MapperMergeContext.root(false, false, NewFieldsBudget.of(0)); + assertFalse(context.hasRemainingBudget()); HashMap runtimeFields = new HashMap<>(); context.addRuntimeFieldIfPossible(new TestRuntimeField("foo", "keyword"), r -> runtimeFields.put(r.name(), r)); assertEquals(0, runtimeFields.size()); - assertFalse(context.canAddField(1)); + assertFalse(context.hasRemainingBudget()); } public void testRemoveRuntimeField() { - MapperMergeContext context = MapperMergeContext.root(false, false, 1); + MapperMergeContext context = MapperMergeContext.root(false, false, NewFieldsBudget.of(1)); HashMap runtimeFields = new HashMap<>(); context.addRuntimeFieldIfPossible(new TestRuntimeField("foo", "keyword"), r -> runtimeFields.put(r.name(), r)); assertEquals(1, runtimeFields.size()); - assertFalse(context.canAddField(1)); + assertFalse(context.hasRemainingBudget()); context.removeRuntimeField(runtimeFields, "foo"); - assertTrue(context.canAddField(1)); + assertTrue(context.hasRemainingBudget()); } private static KeywordFieldMapper getKeywordFieldMapper() { diff --git a/server/src/test/java/org/elasticsearch/index/mapper/NestedObjectMapperTests.java b/server/src/test/java/org/elasticsearch/index/mapper/NestedObjectMapperTests.java index f4ed4b8620306..7a2016afb5e40 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/NestedObjectMapperTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/NestedObjectMapperTests.java @@ -1508,14 +1508,14 @@ public void testMergeNested() { MapperException e = expectThrows( MapperException.class, - () -> firstMapper.merge(secondMapper, MapperMergeContext.root(false, false, Long.MAX_VALUE)) + () -> firstMapper.merge(secondMapper, MapperMergeContext.root(false, false, MapperMergeContext.NewFieldsBudget.unlimited())) ); assertThat(e.getMessage(), containsString("[include_in_parent] parameter can't be updated on a nested object mapping")); NestedObjectMapper result = (NestedObjectMapper) firstMapper.merge( secondMapper, MapperService.MergeReason.INDEX_TEMPLATE, - MapperMergeContext.root(false, false, Long.MAX_VALUE) + MapperMergeContext.root(false, false, MapperMergeContext.NewFieldsBudget.unlimited()) ); assertFalse(result.isIncludeInParent()); assertTrue(result.isIncludeInRoot()); diff --git a/server/src/test/java/org/elasticsearch/index/mapper/ObjectMapperMergeTests.java b/server/src/test/java/org/elasticsearch/index/mapper/ObjectMapperMergeTests.java index 8f95b169ede5f..67ac998be138f 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/ObjectMapperMergeTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/ObjectMapperMergeTests.java @@ -9,6 +9,7 @@ import org.elasticsearch.common.Explicit; import org.elasticsearch.index.IndexVersion; +import org.elasticsearch.index.mapper.MapperMergeContext.NewFieldsBudget; import org.elasticsearch.test.ESTestCase; import java.util.Collections; @@ -41,7 +42,7 @@ public void testMerge() { ObjectMapper mergeWith = createMapping(false, true, true, true); // WHEN merging mappings - final ObjectMapper merged = rootObjectMapper.merge(mergeWith, MapperMergeContext.root(false, false, Long.MAX_VALUE)); + final ObjectMapper merged = rootObjectMapper.merge(mergeWith, MapperMergeContext.root(false, false, NewFieldsBudget.unlimited())); // THEN "baz" new field is added to merged mapping final ObjectMapper mergedFoo = (ObjectMapper) merged.getMapper("foo"); @@ -63,7 +64,7 @@ public void testMergeWhenDisablingField() { // THEN a MapperException is thrown with an excepted message MapperException e = expectThrows( MapperException.class, - () -> rootObjectMapper.merge(mergeWith, MapperMergeContext.root(false, false, Long.MAX_VALUE)) + () -> rootObjectMapper.merge(mergeWith, MapperMergeContext.root(false, false, NewFieldsBudget.unlimited())) ); assertEquals("the [enabled] parameter can't be updated for the object mapping [foo]", e.getMessage()); } @@ -77,7 +78,7 @@ public void testMergeDisabledField() { RootObjectMapper merged = (RootObjectMapper) rootObjectMapper.merge( mergeWith, - MapperMergeContext.root(false, false, Long.MAX_VALUE) + MapperMergeContext.root(false, false, NewFieldsBudget.unlimited()) ); assertFalse(((ObjectMapper) merged.getMapper("disabled")).isEnabled()); } @@ -87,14 +88,14 @@ public void testMergeEnabled() { MapperException e = expectThrows( MapperException.class, - () -> rootObjectMapper.merge(mergeWith, MapperMergeContext.root(false, false, Long.MAX_VALUE)) + () -> rootObjectMapper.merge(mergeWith, MapperMergeContext.root(false, false, NewFieldsBudget.unlimited())) ); assertEquals("the [enabled] parameter can't be updated for the object mapping [disabled]", e.getMessage()); ObjectMapper result = rootObjectMapper.merge( mergeWith, MapperService.MergeReason.INDEX_TEMPLATE, - MapperMergeContext.root(false, false, Long.MAX_VALUE) + MapperMergeContext.root(false, false, NewFieldsBudget.unlimited()) ); assertTrue(result.isEnabled()); } @@ -109,14 +110,14 @@ public void testMergeEnabledForRootMapper() { MapperException e = expectThrows( MapperException.class, - () -> firstMapper.merge(secondMapper, MapperMergeContext.root(false, false, Long.MAX_VALUE)) + () -> firstMapper.merge(secondMapper, MapperMergeContext.root(false, false, NewFieldsBudget.unlimited())) ); assertEquals("the [enabled] parameter can't be updated for the object mapping [" + type + "]", e.getMessage()); ObjectMapper result = firstMapper.merge( secondMapper, MapperService.MergeReason.INDEX_TEMPLATE, - MapperMergeContext.root(false, false, Long.MAX_VALUE) + MapperMergeContext.root(false, false, NewFieldsBudget.unlimited()) ); assertFalse(result.isEnabled()); } @@ -133,7 +134,7 @@ public void testMergeDisabledRootMapper() { RootObjectMapper merged = (RootObjectMapper) rootObjectMapper.merge( mergeWith, - MapperMergeContext.root(false, false, Long.MAX_VALUE) + MapperMergeContext.root(false, false, NewFieldsBudget.unlimited()) ); assertFalse(merged.isEnabled()); assertEquals(1, merged.runtimeFields().size()); @@ -144,7 +145,7 @@ public void testMergedFieldNamesFieldWithDotsSubobjectsFalseAtRoot() { RootObjectMapper mergeInto = createRootSubobjectFalseLeafWithDots(); RootObjectMapper mergeWith = createRootSubobjectFalseLeafWithDots(); - final ObjectMapper merged = mergeInto.merge(mergeWith, MapperMergeContext.root(false, false, Long.MAX_VALUE)); + final ObjectMapper merged = mergeInto.merge(mergeWith, MapperMergeContext.root(false, false, NewFieldsBudget.unlimited())); final KeywordFieldMapper keywordFieldMapper = (KeywordFieldMapper) merged.getMapper("host.name"); assertEquals("host.name", keywordFieldMapper.name()); @@ -159,7 +160,7 @@ public void testMergedFieldNamesFieldWithDotsSubobjectsFalse() { createObjectSubobjectsFalseLeafWithDots() ).build(MapperBuilderContext.root(false, false)); - final ObjectMapper merged = mergeInto.merge(mergeWith, MapperMergeContext.root(false, false, Long.MAX_VALUE)); + final ObjectMapper merged = mergeInto.merge(mergeWith, MapperMergeContext.root(false, false, NewFieldsBudget.unlimited())); ObjectMapper foo = (ObjectMapper) merged.getMapper("foo"); ObjectMapper metrics = (ObjectMapper) foo.getMapper("metrics"); @@ -174,7 +175,7 @@ public void testMergedFieldNamesMultiFields() { RootObjectMapper mergeWith = new RootObjectMapper.Builder("_doc", Explicit.IMPLICIT_TRUE).add(createTextKeywordMultiField("text")) .build(MapperBuilderContext.root(false, false)); - final ObjectMapper merged = mergeInto.merge(mergeWith, MapperMergeContext.root(false, false, Long.MAX_VALUE)); + final ObjectMapper merged = mergeInto.merge(mergeWith, MapperMergeContext.root(false, false, NewFieldsBudget.unlimited())); TextFieldMapper text = (TextFieldMapper) merged.getMapper("text"); assertEquals("text", text.name()); @@ -192,7 +193,7 @@ public void testMergedFieldNamesMultiFieldsWithinSubobjectsFalse() { createObjectSubobjectsFalseLeafWithMultiField() ).build(MapperBuilderContext.root(false, false)); - final ObjectMapper merged = mergeInto.merge(mergeWith, MapperMergeContext.root(false, false, Long.MAX_VALUE)); + final ObjectMapper merged = mergeInto.merge(mergeWith, MapperMergeContext.root(false, false, NewFieldsBudget.unlimited())); ObjectMapper foo = (ObjectMapper) merged.getMapper("foo"); ObjectMapper metrics = (ObjectMapper) foo.getMapper("metrics"); @@ -209,8 +210,8 @@ public void testMergeWithLimit() { ObjectMapper mergeWith = createMapping(false, true, true, true); // WHEN merging mappings - final ObjectMapper mergedAdd0 = rootObjectMapper.merge(mergeWith, MapperMergeContext.root(false, false, 0)); - final ObjectMapper mergedAdd1 = rootObjectMapper.merge(mergeWith, MapperMergeContext.root(false, false, 1)); + final ObjectMapper mergedAdd0 = rootObjectMapper.merge(mergeWith, MapperMergeContext.root(false, false, NewFieldsBudget.of(0))); + final ObjectMapper mergedAdd1 = rootObjectMapper.merge(mergeWith, MapperMergeContext.root(false, false, NewFieldsBudget.of(1))); // THEN "baz" new field is added to merged mapping assertEquals(3, rootObjectMapper.mapperSize()); @@ -227,9 +228,9 @@ public void testMergeWithLimitObjectField() { ).add(new KeywordFieldMapper.Builder("child2", IndexVersion.current())) ).build(MapperBuilderContext.root(false, false)); - 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)); + ObjectMapper mergedAdd1 = root.merge(mergeWith, MapperMergeContext.root(false, false, NewFieldsBudget.of(1))); + ObjectMapper mergedAdd2 = root.merge(mergeWith, MapperMergeContext.root(false, false, NewFieldsBudget.of(2))); + ObjectMapper mergedAdd3 = root.merge(mergeWith, MapperMergeContext.root(false, false, NewFieldsBudget.of(3))); assertEquals(0, root.mapperSize()); assertEquals(1, mergedAdd1.mapperSize()); assertEquals(2, mergedAdd2.mapperSize()); @@ -258,8 +259,8 @@ public void testMergeWithLimitMultiField() { assertEquals(2, mergeInto.mapperSize()); assertEquals(2, mergeWith.mapperSize()); - ObjectMapper mergedAdd0 = mergeInto.merge(mergeWith, MapperMergeContext.root(false, false, 0)); - ObjectMapper mergedAdd1 = mergeInto.merge(mergeWith, MapperMergeContext.root(false, false, 1)); + ObjectMapper mergedAdd0 = mergeInto.merge(mergeWith, MapperMergeContext.root(false, false, NewFieldsBudget.of(0))); + ObjectMapper mergedAdd1 = mergeInto.merge(mergeWith, MapperMergeContext.root(false, false, NewFieldsBudget.of(1))); assertEquals(2, mergedAdd0.mapperSize()); assertEquals(3, mergedAdd1.mapperSize()); } 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 97d4b01675c3f..8a53ca06d470f 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/ObjectMapperTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/ObjectMapperTests.java @@ -15,6 +15,7 @@ import org.elasticsearch.common.compress.CompressedXContent; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.index.IndexVersion; +import org.elasticsearch.index.mapper.MapperMergeContext.NewFieldsBudget; import org.elasticsearch.index.mapper.MapperService.MergeReason; import org.elasticsearch.index.mapper.ObjectMapper.Dynamic; import org.elasticsearch.xcontent.XContentFactory; @@ -125,7 +126,7 @@ public void testMerge() throws IOException { "_doc", new CompressedXContent(BytesReference.bytes(topMapping(b -> b.field("dynamic", "strict")))) ); - Mapping merged = mapper.mapping().merge(mergeWith, reason, Long.MAX_VALUE); + Mapping merged = mapper.mapping().merge(mergeWith, reason, NewFieldsBudget.unlimited()); assertEquals(Dynamic.STRICT, merged.getRoot().dynamic()); } @@ -470,7 +471,7 @@ public void testSubobjectsCannotBeUpdated() throws IOException { })))); MapperException exception = expectThrows( MapperException.class, - () -> mapper.mapping().merge(mergeWith, MergeReason.MAPPING_UPDATE, Long.MAX_VALUE) + () -> mapper.mapping().merge(mergeWith, MergeReason.MAPPING_UPDATE, NewFieldsBudget.unlimited()) ); assertEquals("the [subobjects] parameter can't be updated for the object mapping [field]", exception.getMessage()); } @@ -484,7 +485,7 @@ public void testSubobjectsCannotBeUpdatedOnRoot() throws IOException { })))); MapperException exception = expectThrows( MapperException.class, - () -> mapper.mapping().merge(mergeWith, MergeReason.MAPPING_UPDATE, Long.MAX_VALUE) + () -> mapper.mapping().merge(mergeWith, MergeReason.MAPPING_UPDATE, NewFieldsBudget.unlimited()) ); assertEquals("the [subobjects] parameter can't be updated for the object mapping [_doc]", exception.getMessage()); } diff --git a/server/src/test/java/org/elasticsearch/index/mapper/ParametrizedMapperTests.java b/server/src/test/java/org/elasticsearch/index/mapper/ParametrizedMapperTests.java index 562a30ba4f389..df8b2dcd0d593 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/ParametrizedMapperTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/ParametrizedMapperTests.java @@ -23,6 +23,7 @@ import org.elasticsearch.index.analysis.IndexAnalyzers; import org.elasticsearch.index.analysis.NamedAnalyzer; import org.elasticsearch.index.mapper.FieldMapper.Parameter; +import org.elasticsearch.index.mapper.MapperMergeContext.NewFieldsBudget; import org.elasticsearch.plugins.MapperPlugin; import org.elasticsearch.plugins.Plugin; import org.elasticsearch.script.ScriptCompiler; @@ -345,7 +346,7 @@ public void testMerging() { {"type":"test_mapper","fixed":true,"fixed2":true,"required":"value"}"""); IllegalArgumentException e = expectThrows( IllegalArgumentException.class, - () -> mapper.merge(badMerge, MapperMergeContext.root(false, false, Long.MAX_VALUE)) + () -> mapper.merge(badMerge, MapperMergeContext.root(false, false, NewFieldsBudget.unlimited())) ); String expectedError = """ Mapper for [field] conflicts with existing mapper: @@ -358,7 +359,7 @@ public void testMerging() { // TODO: should we have to include 'fixed' here? Or should updates take as 'defaults' the existing values? TestMapper goodMerge = fromMapping(""" {"type":"test_mapper","fixed":false,"variable":"updated","required":"value"}"""); - TestMapper merged = (TestMapper) mapper.merge(goodMerge, MapperMergeContext.root(false, false, Long.MAX_VALUE)); + TestMapper merged = (TestMapper) mapper.merge(goodMerge, MapperMergeContext.root(false, false, NewFieldsBudget.unlimited())); assertEquals("{\"field\":" + mapping + "}", Strings.toString(mapper)); // original mapping is unaffected assertEquals(""" @@ -376,7 +377,7 @@ public void testMultifields() throws IOException { String addSubField = """ {"type":"test_mapper","variable":"foo","required":"value","fields":{"sub2":{"type":"keyword"}}}"""; TestMapper toMerge = fromMapping(addSubField); - TestMapper merged = (TestMapper) mapper.merge(toMerge, MapperMergeContext.root(false, false, Long.MAX_VALUE)); + TestMapper merged = (TestMapper) mapper.merge(toMerge, MapperMergeContext.root(false, false, NewFieldsBudget.unlimited())); assertEquals(XContentHelper.stripWhitespace(""" { "field": { @@ -399,7 +400,7 @@ public void testMultifields() throws IOException { TestMapper badToMerge = fromMapping(badSubField); IllegalArgumentException e = expectThrows( IllegalArgumentException.class, - () -> merged.merge(badToMerge, MapperMergeContext.root(false, false, Long.MAX_VALUE)) + () -> merged.merge(badToMerge, MapperMergeContext.root(false, false, NewFieldsBudget.unlimited())) ); assertEquals("mapper [field.sub2] cannot be changed from type [keyword] to [binary]", e.getMessage()); } @@ -415,13 +416,13 @@ public void testCopyTo() { TestMapper toMerge = fromMapping(""" {"type":"test_mapper","variable":"updated","required":"value","copy_to":["foo","bar"]}"""); - TestMapper merged = (TestMapper) mapper.merge(toMerge, MapperMergeContext.root(false, false, Long.MAX_VALUE)); + TestMapper merged = (TestMapper) mapper.merge(toMerge, MapperMergeContext.root(false, false, NewFieldsBudget.unlimited())); assertEquals(""" {"field":{"type":"test_mapper","variable":"updated","required":"value","copy_to":["foo","bar"]}}""", Strings.toString(merged)); TestMapper removeCopyTo = fromMapping(""" {"type":"test_mapper","variable":"updated","required":"value"}"""); - TestMapper noCopyTo = (TestMapper) merged.merge(removeCopyTo, MapperMergeContext.root(false, false, Long.MAX_VALUE)); + TestMapper noCopyTo = (TestMapper) merged.merge(removeCopyTo, MapperMergeContext.root(false, false, NewFieldsBudget.unlimited())); assertEquals(""" {"field":{"type":"test_mapper","variable":"updated","required":"value"}}""", Strings.toString(noCopyTo)); } @@ -487,7 +488,7 @@ public void testCustomSerialization() { TestMapper toMerge = fromMapping(conflict); IllegalArgumentException e = expectThrows( IllegalArgumentException.class, - () -> mapper.merge(toMerge, MapperMergeContext.root(false, false, Long.MAX_VALUE)) + () -> mapper.merge(toMerge, MapperMergeContext.root(false, false, NewFieldsBudget.unlimited())) ); assertEquals( "Mapper for [field] conflicts with existing mapper:\n" @@ -578,7 +579,7 @@ public void testAnalyzers() { TestMapper toMerge = fromMapping(mapping); e = expectThrows( IllegalArgumentException.class, - () -> original.merge(toMerge, MapperMergeContext.root(false, false, Long.MAX_VALUE)) + () -> original.merge(toMerge, MapperMergeContext.root(false, false, NewFieldsBudget.unlimited())) ); assertEquals( "Mapper for [field] conflicts with existing mapper:\n" + "\tCannot update parameter [analyzer] from [default] to [_standard]", From 8fae92bd9ddec9fd7f31c4a4a6937498ecb352da Mon Sep 17 00:00:00 2001 From: Felix Barnsteiner Date: Mon, 22 Jan 2024 14:30:42 +0100 Subject: [PATCH 31/40] Relax assertion in RootObjectMapper#merge --- .../java/org/elasticsearch/index/mapper/RootObjectMapper.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 32b1825377703..6318e507e4342 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/RootObjectMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/RootObjectMapper.java @@ -258,7 +258,7 @@ public RootObjectMapper merge(Mapper mergeWith, MergeReason reason, MapperMergeC dynamicTemplates = this.dynamicTemplates; } final Map runtimeFields = new HashMap<>(this.runtimeFields); - assert this.runtimeFields != mergeWithObject.runtimeFields; + assert this.runtimeFields != mergeWithObject.runtimeFields || this.runtimeFields == Map.of(); for (Map.Entry runtimeField : mergeWithObject.runtimeFields.entrySet()) { if (runtimeField.getValue() == null) { parentMergeContext.removeRuntimeField(runtimeFields, runtimeField.getKey()); From 0218d5bd207d39d99682224b74bd4020e911da08 Mon Sep 17 00:00:00 2001 From: Felix Barnsteiner Date: Tue, 23 Jan 2024 09:31:48 +0100 Subject: [PATCH 32/40] Apply feedback from review --- .../index/mapper/FieldMapper.java | 4 +- .../index/mapper/MapperMergeContext.java | 90 ++++++------------- .../index/mapper/MapperService.java | 10 +-- .../elasticsearch/index/mapper/Mapping.java | 10 ++- .../index/mapper/ObjectMapper.java | 24 ++--- .../index/mapper/ParsedDocument.java | 3 +- .../index/mapper/RootObjectMapper.java | 6 +- .../index/mapper/DocumentMapperTests.java | 32 ++----- .../index/mapper/MapperMergeContextTests.java | 59 ++---------- .../index/mapper/NestedObjectMapperTests.java | 4 +- .../index/mapper/ObjectMapperMergeTests.java | 39 ++++---- .../index/mapper/ObjectMapperTests.java | 7 +- .../index/mapper/ParametrizedMapperTests.java | 17 ++-- 13 files changed, 97 insertions(+), 208 deletions(-) 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 0a3ab0a31557a..5a57d1927efe1 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/FieldMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/FieldMapper.java @@ -455,7 +455,9 @@ private void add(FieldMapper mapper) { private void update(FieldMapper toMerge, MapperMergeContext context) { if (mapperBuilders.containsKey(toMerge.simpleName()) == false) { - context.addFieldIfPossible(toMerge, this::add); + if (context.decrementIfPossible(toMerge.mapperSize())) { + add(toMerge); + } } else { FieldMapper existing = mapperBuilders.get(toMerge.simpleName()).apply(context.getMapperBuilderContext()); add(existing.merge(toMerge, context)); diff --git a/server/src/main/java/org/elasticsearch/index/mapper/MapperMergeContext.java b/server/src/main/java/org/elasticsearch/index/mapper/MapperMergeContext.java index 679dafc467f07..98e30df618ee4 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/MapperMergeContext.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/MapperMergeContext.java @@ -8,9 +8,6 @@ package org.elasticsearch.index.mapper; -import java.util.Map; -import java.util.function.Consumer; - /** * Holds context used when merging mappings. * As the merge process also involves building merged {@link Mapper.Builder}s, @@ -19,18 +16,18 @@ public final class MapperMergeContext { private final MapperBuilderContext mapperBuilderContext; - private final NewFieldsBudget remainingFieldsBudget; + private final NewFieldsBudget newFieldsBudget; - private MapperMergeContext(MapperBuilderContext mapperBuilderContext, NewFieldsBudget remainingFieldsBudget) { + private MapperMergeContext(MapperBuilderContext mapperBuilderContext, NewFieldsBudget newFieldsBudget) { this.mapperBuilderContext = mapperBuilderContext; - this.remainingFieldsBudget = remainingFieldsBudget; + this.newFieldsBudget = newFieldsBudget; } /** * The root context, to be used when merging a tree of mappers */ - public static MapperMergeContext root(boolean isSourceSynthetic, boolean isDataStream, NewFieldsBudget newFieldsBudget) { - return new MapperMergeContext(MapperBuilderContext.root(isSourceSynthetic, isDataStream), newFieldsBudget); + public static MapperMergeContext root(boolean isSourceSynthetic, boolean isDataStream, long newFieldsBudget) { + return new MapperMergeContext(MapperBuilderContext.root(isSourceSynthetic, isDataStream), NewFieldsBudget.of(newFieldsBudget)); } /** @@ -44,71 +41,52 @@ public static MapperMergeContext from(MapperBuilderContext mapperBuilderContext, } /** - * Creates a new {@link MapperMergeContext} that is a child of this context + * Creates a new {@link MapperMergeContext} with a child {@link MapperBuilderContext}. + * The child {@link MapperMergeContext} context will share the same field limit. * @param name the name of the child context * @return a new {@link MapperMergeContext} with this context as its parent */ - public MapperMergeContext createChildContext(String name) { + MapperMergeContext createChildContext(String name) { return createChildContext(mapperBuilderContext.createChildContext(name)); } /** - * Creates a new {@link MapperMergeContext} with a given {@link MapperBuilderContext} + * Creates a new {@link MapperMergeContext} with a given child {@link MapperBuilderContext} + * The child {@link MapperMergeContext} context will share the same field limit. * @param childContext the child {@link MapperBuilderContext} * @return a new {@link MapperMergeContext}, wrapping the provided {@link MapperBuilderContext} */ - public MapperMergeContext createChildContext(MapperBuilderContext childContext) { - return new MapperMergeContext(childContext, remainingFieldsBudget); + MapperMergeContext createChildContext(MapperBuilderContext childContext) { + return new MapperMergeContext(childContext, newFieldsBudget); } MapperBuilderContext getMapperBuilderContext() { return mapperBuilderContext; } - void removeRuntimeField(Map runtimeFields, String name) { - if (runtimeFields.containsKey(name)) { - runtimeFields.remove(name); - remainingFieldsBudget.increment(1); - } - } - - boolean addFieldIfPossible(M mapper, Consumer addField) { - if (remainingFieldsBudget.decrementIfPossible(mapper.mapperSize())) { - addField.accept(mapper); - return true; - } - return false; - } - - void addRuntimeFieldIfPossible(RuntimeField runtimeField, Consumer addField) { - if (remainingFieldsBudget.decrementIfPossible(1)) { - addField.accept(runtimeField); - } - } - - boolean hasRemainingBudget() { - return remainingFieldsBudget.hasRemainingBudget(); + boolean decrementIfPossible(int fieldSize) { + return newFieldsBudget.decrementIfPossible(fieldSize); } /** - * Keeps track of now many new fields can be added during mapper merge + * Keeps track of how many new fields can be added during mapper merge. + * The field budget is shared across instances of {@link MapperMergeContext} that are created via + * {@link MapperMergeContext#createChildContext}. + * This ensures that fields that are consumed by one child object mapper also decrement the budget for another child object. + * Not thread safe.The same instance may not be modified by multiple threads. */ - public interface NewFieldsBudget { - static NewFieldsBudget unlimited() { - return Unlimited.INSTANCE; - } + private interface NewFieldsBudget { static NewFieldsBudget of(long fieldsBudget) { + if (fieldsBudget == Long.MAX_VALUE) { + return Unlimited.INSTANCE; + } return new Limited(fieldsBudget); } boolean decrementIfPossible(long fieldSize); - void increment(long fieldSize); - - boolean hasRemainingBudget(); - - class Unlimited implements NewFieldsBudget { + final class Unlimited implements NewFieldsBudget { private static final Unlimited INSTANCE = new Unlimited(); @@ -119,18 +97,9 @@ public boolean decrementIfPossible(long fieldSize) { return true; } - @Override - public void increment(long fieldSize) { - // noop - } - - @Override - public boolean hasRemainingBudget() { - return true; - } } - class Limited implements NewFieldsBudget { + final class Limited implements NewFieldsBudget { private long fieldsBudget; @@ -147,15 +116,6 @@ public boolean decrementIfPossible(long fieldSize) { return false; } - @Override - public void increment(long fieldSize) { - fieldsBudget++; - } - - @Override - public boolean hasRemainingBudget() { - return fieldsBudget >= 1; - } } } } diff --git a/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java b/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java index 9d8c8ab3d8352..61dd444be34b1 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java @@ -25,7 +25,6 @@ import org.elasticsearch.index.analysis.AnalysisRegistry; import org.elasticsearch.index.analysis.IndexAnalyzers; import org.elasticsearch.index.analysis.NamedAnalyzer; -import org.elasticsearch.index.mapper.MapperMergeContext.NewFieldsBudget; import org.elasticsearch.index.query.SearchExecutionContext; import org.elasticsearch.index.similarity.SimilarityService; import org.elasticsearch.indices.IndicesModule; @@ -560,15 +559,10 @@ public Mapping parseMapping(String mappingType, Map mappingSourc } public static Mapping mergeMappings(DocumentMapper currentMapper, Mapping incomingMapping, MergeReason reason) { - return mergeMappings(currentMapper, incomingMapping, reason, NewFieldsBudget.unlimited()); + return mergeMappings(currentMapper, incomingMapping, reason, Long.MAX_VALUE); } - static Mapping mergeMappings( - DocumentMapper currentMapper, - Mapping incomingMapping, - MergeReason reason, - NewFieldsBudget newFieldsBudget - ) { + static Mapping mergeMappings(DocumentMapper currentMapper, Mapping incomingMapping, MergeReason reason, long newFieldsBudget) { Mapping newMapping; if (currentMapper == null) { newMapping = incomingMapping.withFieldsBudget(newFieldsBudget); diff --git a/server/src/main/java/org/elasticsearch/index/mapper/Mapping.java b/server/src/main/java/org/elasticsearch/index/mapper/Mapping.java index 4ead2350fe6f4..2114fdc3a11c8 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/Mapping.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/Mapping.java @@ -12,7 +12,6 @@ import org.elasticsearch.common.Strings; import org.elasticsearch.common.compress.CompressedXContent; import org.elasticsearch.common.xcontent.XContentHelper; -import org.elasticsearch.index.mapper.MapperMergeContext.NewFieldsBudget; import org.elasticsearch.index.mapper.MapperService.MergeReason; import org.elasticsearch.xcontent.ToXContentFragment; import org.elasticsearch.xcontent.XContentBuilder; @@ -137,7 +136,7 @@ public SourceLoader.SyntheticFieldLoader syntheticFieldLoader() { * @param newFieldsBudget how many new fields can be added during the merge process * @return the resulting merged mapping. */ - Mapping merge(Mapping mergeWith, MergeReason reason, NewFieldsBudget newFieldsBudget) { + Mapping merge(Mapping mergeWith, MergeReason reason, long newFieldsBudget) { MapperMergeContext mergeContext = MapperMergeContext.root(isSourceSynthetic(), false, newFieldsBudget); RootObjectMapper mergedRoot = root.merge(mergeWith.root, reason, mergeContext); @@ -172,8 +171,13 @@ Mapping merge(Mapping mergeWith, MergeReason reason, NewFieldsBudget newFieldsBu return new Mapping(mergedRoot, mergedMetadataMappers.values().toArray(new MetadataFieldMapper[0]), mergedMeta); } - public Mapping withFieldsBudget(NewFieldsBudget fieldsBudget) { + /** + * Returns a copy of this mapper that ensures that the number of fields isn't greater than the provided fields budget. + * @param fieldsBudget the maximum number of fields this mapping may have + */ + public Mapping withFieldsBudget(long fieldsBudget) { MapperMergeContext mergeContext = MapperMergeContext.root(isSourceSynthetic(), false, fieldsBudget); + // calling merge to ensure we're only adding as many fields as allowed by the fields budget return new Mapping(root.withoutMappers().merge(root, MergeReason.MAPPING_RECOVERY, mergeContext), metadataMappers, meta); } 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 9d586ce6764da..fb833557a619b 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java @@ -547,13 +547,10 @@ private static Map buildMergedMappers( Mapper mergeIntoMapper = mergedMappers.get(mergeWithMapper.simpleName()); if (mergeIntoMapper == null) { - if (mergeWithMapper instanceof ObjectMapper om) { - addNewObjectMapper(reason, objectMergeContext, mergedMappers, om); - } else { - objectMergeContext.addFieldIfPossible( - mergeWithMapper, - m -> mergedMappers.put(mergeWithMapper.simpleName(), mergeWithMapper) - ); + if (objectMergeContext.decrementIfPossible(mergeWithMapper.mapperSize())) { + mergedMappers.put(mergeWithMapper.simpleName(), mergeWithMapper); + } else if (mergeWithMapper instanceof ObjectMapper om) { + addPartialObjectMapper(reason, objectMergeContext, mergedMappers, om); } } else if (mergeIntoMapper instanceof ObjectMapper objectMapper) { mergedMappers.put(objectMapper.simpleName(), objectMapper.merge(mergeWithMapper, reason, objectMergeContext)); @@ -577,25 +574,16 @@ private static Map buildMergedMappers( return Map.copyOf(mergedMappers); } - private static void addNewObjectMapper( + private static void addPartialObjectMapper( MergeReason reason, MapperMergeContext objectMergeContext, Map mergedMappers, ObjectMapper objectMapper ) { - // first try to add the full object, with all it's sub-fields - boolean fullObjectAdded = objectMergeContext.addFieldIfPossible(objectMapper, o -> mergedMappers.put(o.simpleName(), o)); - if (fullObjectAdded) { - return; - } // 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(); - boolean shallowObjectAdded = objectMergeContext.addFieldIfPossible( - shallowObjectMapper, - o -> mergedMappers.put(o.simpleName(), o) - ); - if (shallowObjectAdded) { + if (objectMergeContext.decrementIfPossible(shallowObjectMapper.mapperSize())) { // now trying to add the sub-fields one by one via a merge, until we hit the limit mergedMappers.put(objectMapper.simpleName(), shallowObjectMapper.merge(objectMapper, reason, objectMergeContext)); } diff --git a/server/src/main/java/org/elasticsearch/index/mapper/ParsedDocument.java b/server/src/main/java/org/elasticsearch/index/mapper/ParsedDocument.java index ed5431541a2f0..014be192a4133 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/ParsedDocument.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/ParsedDocument.java @@ -13,7 +13,6 @@ import org.apache.lucene.util.BytesRef; import org.elasticsearch.common.bytes.BytesArray; import org.elasticsearch.common.bytes.BytesReference; -import org.elasticsearch.index.mapper.MapperMergeContext.NewFieldsBudget; import org.elasticsearch.index.mapper.MapperService.MergeReason; import org.elasticsearch.xcontent.XContentType; @@ -161,7 +160,7 @@ public void addDynamicMappingsUpdate(Mapping update) { if (dynamicMappingsUpdate == null) { dynamicMappingsUpdate = update; } else { - dynamicMappingsUpdate = dynamicMappingsUpdate.merge(update, MergeReason.MAPPING_UPDATE, NewFieldsBudget.unlimited()); + dynamicMappingsUpdate = dynamicMappingsUpdate.merge(update, MergeReason.MAPPING_UPDATE, Long.MAX_VALUE); } } 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 6318e507e4342..6371e36b09626 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/RootObjectMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/RootObjectMapper.java @@ -261,11 +261,13 @@ public RootObjectMapper merge(Mapper mergeWith, MergeReason reason, MapperMergeC assert this.runtimeFields != mergeWithObject.runtimeFields || this.runtimeFields == Map.of(); for (Map.Entry runtimeField : mergeWithObject.runtimeFields.entrySet()) { if (runtimeField.getValue() == null) { - parentMergeContext.removeRuntimeField(runtimeFields, runtimeField.getKey()); + runtimeFields.remove(runtimeField.getKey()); } else if (runtimeFields.containsKey(runtimeField.getKey())) { runtimeFields.put(runtimeField.getKey(), runtimeField.getValue()); } else { - parentMergeContext.addRuntimeFieldIfPossible(runtimeField.getValue(), r -> runtimeFields.put(r.name(), r)); + if (parentMergeContext.decrementIfPossible(1)) { + runtimeFields.put(runtimeField.getValue().name(), runtimeField.getValue()); + } } } diff --git a/server/src/test/java/org/elasticsearch/index/mapper/DocumentMapperTests.java b/server/src/test/java/org/elasticsearch/index/mapper/DocumentMapperTests.java index 66af997a637d8..e935ae2431131 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/DocumentMapperTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/DocumentMapperTests.java @@ -20,7 +20,6 @@ import org.elasticsearch.index.analysis.AnalyzerScope; import org.elasticsearch.index.analysis.IndexAnalyzers; import org.elasticsearch.index.analysis.NamedAnalyzer; -import org.elasticsearch.index.mapper.MapperMergeContext.NewFieldsBudget; import org.elasticsearch.index.mapper.MapperService.MergeReason; import org.elasticsearch.plugins.internal.DocumentParsingObserver; import org.elasticsearch.xcontent.XContentBuilder; @@ -64,7 +63,7 @@ public void testAddFields() throws Exception { })); MergeReason reason = randomFrom(MergeReason.MAPPING_UPDATE, MergeReason.INDEX_TEMPLATE); - Mapping merged = MapperService.mergeMappings(stage1, stage2.mapping(), reason, NewFieldsBudget.unlimited()); + Mapping merged = MapperService.mergeMappings(stage1, stage2.mapping(), reason, Long.MAX_VALUE); // stage1 mapping should not have been modified assertThat(stage1.mappers().getMapper("age"), nullValue()); assertThat(stage1.mappers().getMapper("obj1.prop1"), nullValue()); @@ -82,12 +81,7 @@ public void testMergeObjectDynamic() throws Exception { DocumentMapper withDynamicMapper = createDocumentMapper(topMapping(b -> b.field("dynamic", "false"))); assertThat(withDynamicMapper.mapping().getRoot().dynamic(), equalTo(ObjectMapper.Dynamic.FALSE)); - Mapping merged = MapperService.mergeMappings( - mapper, - withDynamicMapper.mapping(), - MergeReason.MAPPING_UPDATE, - NewFieldsBudget.unlimited() - ); + Mapping merged = MapperService.mergeMappings(mapper, withDynamicMapper.mapping(), MergeReason.MAPPING_UPDATE, Long.MAX_VALUE); assertThat(merged.getRoot().dynamic(), equalTo(ObjectMapper.Dynamic.FALSE)); } @@ -99,14 +93,14 @@ public void testMergeObjectAndNested() throws Exception { { IllegalArgumentException e = expectThrows( IllegalArgumentException.class, - () -> MapperService.mergeMappings(objectMapper, nestedMapper.mapping(), reason, NewFieldsBudget.unlimited()) + () -> MapperService.mergeMappings(objectMapper, nestedMapper.mapping(), reason, Long.MAX_VALUE) ); assertThat(e.getMessage(), containsString("can't merge a non-nested mapping [obj] with a nested mapping")); } { IllegalArgumentException e = expectThrows( IllegalArgumentException.class, - () -> MapperService.mergeMappings(nestedMapper, objectMapper.mapping(), reason, NewFieldsBudget.unlimited()) + () -> MapperService.mergeMappings(nestedMapper, objectMapper.mapping(), reason, Long.MAX_VALUE) ); assertThat(e.getMessage(), containsString("can't merge a non-nested mapping [obj] with a nested mapping")); } @@ -241,16 +235,11 @@ public void testMergeMeta() throws IOException { DocumentMapper updatedMapper = createDocumentMapper(fieldMapping(b -> b.field("type", "text"))); - Mapping merged = MapperService.mergeMappings( - initMapper, - updatedMapper.mapping(), - MergeReason.MAPPING_UPDATE, - NewFieldsBudget.unlimited() - ); + Mapping merged = MapperService.mergeMappings(initMapper, updatedMapper.mapping(), MergeReason.MAPPING_UPDATE, Long.MAX_VALUE); assertThat(merged.getMeta().get("foo"), equalTo("bar")); updatedMapper = createDocumentMapper(topMapping(b -> b.startObject("_meta").field("foo", "new_bar").endObject())); - merged = MapperService.mergeMappings(initMapper, updatedMapper.mapping(), MergeReason.MAPPING_UPDATE, NewFieldsBudget.unlimited()); + merged = MapperService.mergeMappings(initMapper, updatedMapper.mapping(), MergeReason.MAPPING_UPDATE, Long.MAX_VALUE); assertThat(merged.getMeta().get("foo"), equalTo("new_bar")); } @@ -273,12 +262,7 @@ public void testMergeMetaForIndexTemplate() throws IOException { assertThat(initMapper.mapping().getMeta(), equalTo(expected)); DocumentMapper updatedMapper = createDocumentMapper(fieldMapping(b -> b.field("type", "text"))); - Mapping merged = MapperService.mergeMappings( - initMapper, - updatedMapper.mapping(), - MergeReason.INDEX_TEMPLATE, - NewFieldsBudget.unlimited() - ); + Mapping merged = MapperService.mergeMappings(initMapper, updatedMapper.mapping(), MergeReason.INDEX_TEMPLATE, Long.MAX_VALUE); assertThat(merged.getMeta(), equalTo(expected)); updatedMapper = createDocumentMapper(topMapping(b -> { @@ -294,7 +278,7 @@ public void testMergeMetaForIndexTemplate() throws IOException { } b.endObject(); })); - merged = merged.merge(updatedMapper.mapping(), MergeReason.INDEX_TEMPLATE, NewFieldsBudget.unlimited()); + merged = merged.merge(updatedMapper.mapping(), MergeReason.INDEX_TEMPLATE, Long.MAX_VALUE); expected = Map.of("field", "value", "object", Map.of("field1", "value1", "field2", "new_value", "field3", "value3")); assertThat(merged.getMeta(), equalTo(expected)); diff --git a/server/src/test/java/org/elasticsearch/index/mapper/MapperMergeContextTests.java b/server/src/test/java/org/elasticsearch/index/mapper/MapperMergeContextTests.java index 8f41727d7b2a5..59e92238f7c9c 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/MapperMergeContextTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/MapperMergeContextTests.java @@ -8,66 +8,25 @@ package org.elasticsearch.index.mapper; -import org.elasticsearch.index.IndexVersion; -import org.elasticsearch.index.mapper.MapperMergeContext.NewFieldsBudget; import org.elasticsearch.test.ESTestCase; -import java.util.HashMap; -import java.util.Map; - public class MapperMergeContextTests extends ESTestCase { public void testAddFieldIfPossibleUnderLimit() { - MapperMergeContext context = MapperMergeContext.root(false, false, NewFieldsBudget.of(1)); - assertTrue(context.hasRemainingBudget()); - HashMap mappers = new HashMap<>(); - Mapper mapper = getKeywordFieldMapper(); - context.addFieldIfPossible(mapper, m -> ((Map) mappers).put(mapper.simpleName(), mapper)); - assertEquals(1, mappers.size()); - assertFalse(context.hasRemainingBudget()); + MapperMergeContext context = MapperMergeContext.root(false, false, 1); + assertTrue(context.decrementIfPossible(1)); + assertFalse(context.decrementIfPossible(1)); } public void testAddFieldIfPossibleAtLimit() { - MapperMergeContext context = MapperMergeContext.root(false, false, NewFieldsBudget.of(0)); - assertFalse(context.hasRemainingBudget()); - HashMap mappers = new HashMap<>(); - Mapper mapper = getKeywordFieldMapper(); - context.addFieldIfPossible(mapper, m -> ((Map) mappers).put(mapper.simpleName(), mapper)); - assertEquals(0, mappers.size()); - assertFalse(context.hasRemainingBudget()); - } - - public void testAddRuntimeFieldIfPossibleUnderLimit() { - MapperMergeContext context = MapperMergeContext.root(false, false, NewFieldsBudget.of(1)); - assertTrue(context.hasRemainingBudget()); - HashMap runtimeFields = new HashMap<>(); - context.addRuntimeFieldIfPossible(new TestRuntimeField("foo", "keyword"), r -> runtimeFields.put(r.name(), r)); - assertEquals(1, runtimeFields.size()); - assertFalse(context.hasRemainingBudget()); - } - - public void testAddRuntimeFieldIfPossibleAtLimit() { - MapperMergeContext context = MapperMergeContext.root(false, false, NewFieldsBudget.of(0)); - assertFalse(context.hasRemainingBudget()); - HashMap runtimeFields = new HashMap<>(); - context.addRuntimeFieldIfPossible(new TestRuntimeField("foo", "keyword"), r -> runtimeFields.put(r.name(), r)); - assertEquals(0, runtimeFields.size()); - assertFalse(context.hasRemainingBudget()); - } - - public void testRemoveRuntimeField() { - MapperMergeContext context = MapperMergeContext.root(false, false, NewFieldsBudget.of(1)); - HashMap runtimeFields = new HashMap<>(); - context.addRuntimeFieldIfPossible(new TestRuntimeField("foo", "keyword"), r -> runtimeFields.put(r.name(), r)); - assertEquals(1, runtimeFields.size()); - assertFalse(context.hasRemainingBudget()); - - context.removeRuntimeField(runtimeFields, "foo"); - assertTrue(context.hasRemainingBudget()); + MapperMergeContext context = MapperMergeContext.root(false, false, 0); + assertFalse(context.decrementIfPossible(1)); } - private static KeywordFieldMapper getKeywordFieldMapper() { - return new KeywordFieldMapper.Builder("foo", IndexVersion.current()).build(MapperBuilderContext.root(false, false)); + public void testAddFieldIfPossibleUnlimited() { + MapperMergeContext context = MapperMergeContext.root(false, false, Long.MAX_VALUE); + assertTrue(context.decrementIfPossible(Integer.MAX_VALUE)); + assertTrue(context.decrementIfPossible(Integer.MAX_VALUE)); } } diff --git a/server/src/test/java/org/elasticsearch/index/mapper/NestedObjectMapperTests.java b/server/src/test/java/org/elasticsearch/index/mapper/NestedObjectMapperTests.java index 7a2016afb5e40..f4ed4b8620306 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/NestedObjectMapperTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/NestedObjectMapperTests.java @@ -1508,14 +1508,14 @@ public void testMergeNested() { MapperException e = expectThrows( MapperException.class, - () -> firstMapper.merge(secondMapper, MapperMergeContext.root(false, false, MapperMergeContext.NewFieldsBudget.unlimited())) + () -> firstMapper.merge(secondMapper, MapperMergeContext.root(false, false, Long.MAX_VALUE)) ); assertThat(e.getMessage(), containsString("[include_in_parent] parameter can't be updated on a nested object mapping")); NestedObjectMapper result = (NestedObjectMapper) firstMapper.merge( secondMapper, MapperService.MergeReason.INDEX_TEMPLATE, - MapperMergeContext.root(false, false, MapperMergeContext.NewFieldsBudget.unlimited()) + MapperMergeContext.root(false, false, Long.MAX_VALUE) ); assertFalse(result.isIncludeInParent()); assertTrue(result.isIncludeInRoot()); diff --git a/server/src/test/java/org/elasticsearch/index/mapper/ObjectMapperMergeTests.java b/server/src/test/java/org/elasticsearch/index/mapper/ObjectMapperMergeTests.java index 67ac998be138f..8f95b169ede5f 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/ObjectMapperMergeTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/ObjectMapperMergeTests.java @@ -9,7 +9,6 @@ import org.elasticsearch.common.Explicit; import org.elasticsearch.index.IndexVersion; -import org.elasticsearch.index.mapper.MapperMergeContext.NewFieldsBudget; import org.elasticsearch.test.ESTestCase; import java.util.Collections; @@ -42,7 +41,7 @@ public void testMerge() { ObjectMapper mergeWith = createMapping(false, true, true, true); // WHEN merging mappings - final ObjectMapper merged = rootObjectMapper.merge(mergeWith, MapperMergeContext.root(false, false, NewFieldsBudget.unlimited())); + final ObjectMapper merged = rootObjectMapper.merge(mergeWith, MapperMergeContext.root(false, false, Long.MAX_VALUE)); // THEN "baz" new field is added to merged mapping final ObjectMapper mergedFoo = (ObjectMapper) merged.getMapper("foo"); @@ -64,7 +63,7 @@ public void testMergeWhenDisablingField() { // THEN a MapperException is thrown with an excepted message MapperException e = expectThrows( MapperException.class, - () -> rootObjectMapper.merge(mergeWith, MapperMergeContext.root(false, false, NewFieldsBudget.unlimited())) + () -> rootObjectMapper.merge(mergeWith, MapperMergeContext.root(false, false, Long.MAX_VALUE)) ); assertEquals("the [enabled] parameter can't be updated for the object mapping [foo]", e.getMessage()); } @@ -78,7 +77,7 @@ public void testMergeDisabledField() { RootObjectMapper merged = (RootObjectMapper) rootObjectMapper.merge( mergeWith, - MapperMergeContext.root(false, false, NewFieldsBudget.unlimited()) + MapperMergeContext.root(false, false, Long.MAX_VALUE) ); assertFalse(((ObjectMapper) merged.getMapper("disabled")).isEnabled()); } @@ -88,14 +87,14 @@ public void testMergeEnabled() { MapperException e = expectThrows( MapperException.class, - () -> rootObjectMapper.merge(mergeWith, MapperMergeContext.root(false, false, NewFieldsBudget.unlimited())) + () -> rootObjectMapper.merge(mergeWith, MapperMergeContext.root(false, false, Long.MAX_VALUE)) ); assertEquals("the [enabled] parameter can't be updated for the object mapping [disabled]", e.getMessage()); ObjectMapper result = rootObjectMapper.merge( mergeWith, MapperService.MergeReason.INDEX_TEMPLATE, - MapperMergeContext.root(false, false, NewFieldsBudget.unlimited()) + MapperMergeContext.root(false, false, Long.MAX_VALUE) ); assertTrue(result.isEnabled()); } @@ -110,14 +109,14 @@ public void testMergeEnabledForRootMapper() { MapperException e = expectThrows( MapperException.class, - () -> firstMapper.merge(secondMapper, MapperMergeContext.root(false, false, NewFieldsBudget.unlimited())) + () -> firstMapper.merge(secondMapper, MapperMergeContext.root(false, false, Long.MAX_VALUE)) ); assertEquals("the [enabled] parameter can't be updated for the object mapping [" + type + "]", e.getMessage()); ObjectMapper result = firstMapper.merge( secondMapper, MapperService.MergeReason.INDEX_TEMPLATE, - MapperMergeContext.root(false, false, NewFieldsBudget.unlimited()) + MapperMergeContext.root(false, false, Long.MAX_VALUE) ); assertFalse(result.isEnabled()); } @@ -134,7 +133,7 @@ public void testMergeDisabledRootMapper() { RootObjectMapper merged = (RootObjectMapper) rootObjectMapper.merge( mergeWith, - MapperMergeContext.root(false, false, NewFieldsBudget.unlimited()) + MapperMergeContext.root(false, false, Long.MAX_VALUE) ); assertFalse(merged.isEnabled()); assertEquals(1, merged.runtimeFields().size()); @@ -145,7 +144,7 @@ public void testMergedFieldNamesFieldWithDotsSubobjectsFalseAtRoot() { RootObjectMapper mergeInto = createRootSubobjectFalseLeafWithDots(); RootObjectMapper mergeWith = createRootSubobjectFalseLeafWithDots(); - final ObjectMapper merged = mergeInto.merge(mergeWith, MapperMergeContext.root(false, false, NewFieldsBudget.unlimited())); + final ObjectMapper merged = mergeInto.merge(mergeWith, MapperMergeContext.root(false, false, Long.MAX_VALUE)); final KeywordFieldMapper keywordFieldMapper = (KeywordFieldMapper) merged.getMapper("host.name"); assertEquals("host.name", keywordFieldMapper.name()); @@ -160,7 +159,7 @@ public void testMergedFieldNamesFieldWithDotsSubobjectsFalse() { createObjectSubobjectsFalseLeafWithDots() ).build(MapperBuilderContext.root(false, false)); - final ObjectMapper merged = mergeInto.merge(mergeWith, MapperMergeContext.root(false, false, NewFieldsBudget.unlimited())); + final ObjectMapper merged = mergeInto.merge(mergeWith, MapperMergeContext.root(false, false, Long.MAX_VALUE)); ObjectMapper foo = (ObjectMapper) merged.getMapper("foo"); ObjectMapper metrics = (ObjectMapper) foo.getMapper("metrics"); @@ -175,7 +174,7 @@ public void testMergedFieldNamesMultiFields() { RootObjectMapper mergeWith = new RootObjectMapper.Builder("_doc", Explicit.IMPLICIT_TRUE).add(createTextKeywordMultiField("text")) .build(MapperBuilderContext.root(false, false)); - final ObjectMapper merged = mergeInto.merge(mergeWith, MapperMergeContext.root(false, false, NewFieldsBudget.unlimited())); + final ObjectMapper merged = mergeInto.merge(mergeWith, MapperMergeContext.root(false, false, Long.MAX_VALUE)); TextFieldMapper text = (TextFieldMapper) merged.getMapper("text"); assertEquals("text", text.name()); @@ -193,7 +192,7 @@ public void testMergedFieldNamesMultiFieldsWithinSubobjectsFalse() { createObjectSubobjectsFalseLeafWithMultiField() ).build(MapperBuilderContext.root(false, false)); - final ObjectMapper merged = mergeInto.merge(mergeWith, MapperMergeContext.root(false, false, NewFieldsBudget.unlimited())); + final ObjectMapper merged = mergeInto.merge(mergeWith, MapperMergeContext.root(false, false, Long.MAX_VALUE)); ObjectMapper foo = (ObjectMapper) merged.getMapper("foo"); ObjectMapper metrics = (ObjectMapper) foo.getMapper("metrics"); @@ -210,8 +209,8 @@ public void testMergeWithLimit() { ObjectMapper mergeWith = createMapping(false, true, true, true); // WHEN merging mappings - final ObjectMapper mergedAdd0 = rootObjectMapper.merge(mergeWith, MapperMergeContext.root(false, false, NewFieldsBudget.of(0))); - final ObjectMapper mergedAdd1 = rootObjectMapper.merge(mergeWith, MapperMergeContext.root(false, false, NewFieldsBudget.of(1))); + final ObjectMapper mergedAdd0 = rootObjectMapper.merge(mergeWith, MapperMergeContext.root(false, false, 0)); + final ObjectMapper mergedAdd1 = rootObjectMapper.merge(mergeWith, MapperMergeContext.root(false, false, 1)); // THEN "baz" new field is added to merged mapping assertEquals(3, rootObjectMapper.mapperSize()); @@ -228,9 +227,9 @@ public void testMergeWithLimitObjectField() { ).add(new KeywordFieldMapper.Builder("child2", IndexVersion.current())) ).build(MapperBuilderContext.root(false, false)); - ObjectMapper mergedAdd1 = root.merge(mergeWith, MapperMergeContext.root(false, false, NewFieldsBudget.of(1))); - ObjectMapper mergedAdd2 = root.merge(mergeWith, MapperMergeContext.root(false, false, NewFieldsBudget.of(2))); - ObjectMapper mergedAdd3 = root.merge(mergeWith, MapperMergeContext.root(false, false, NewFieldsBudget.of(3))); + 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(1, mergedAdd1.mapperSize()); assertEquals(2, mergedAdd2.mapperSize()); @@ -259,8 +258,8 @@ public void testMergeWithLimitMultiField() { assertEquals(2, mergeInto.mapperSize()); assertEquals(2, mergeWith.mapperSize()); - ObjectMapper mergedAdd0 = mergeInto.merge(mergeWith, MapperMergeContext.root(false, false, NewFieldsBudget.of(0))); - ObjectMapper mergedAdd1 = mergeInto.merge(mergeWith, MapperMergeContext.root(false, false, NewFieldsBudget.of(1))); + 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()); } 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 8a53ca06d470f..97d4b01675c3f 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/ObjectMapperTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/ObjectMapperTests.java @@ -15,7 +15,6 @@ import org.elasticsearch.common.compress.CompressedXContent; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.index.IndexVersion; -import org.elasticsearch.index.mapper.MapperMergeContext.NewFieldsBudget; import org.elasticsearch.index.mapper.MapperService.MergeReason; import org.elasticsearch.index.mapper.ObjectMapper.Dynamic; import org.elasticsearch.xcontent.XContentFactory; @@ -126,7 +125,7 @@ public void testMerge() throws IOException { "_doc", new CompressedXContent(BytesReference.bytes(topMapping(b -> b.field("dynamic", "strict")))) ); - Mapping merged = mapper.mapping().merge(mergeWith, reason, NewFieldsBudget.unlimited()); + Mapping merged = mapper.mapping().merge(mergeWith, reason, Long.MAX_VALUE); assertEquals(Dynamic.STRICT, merged.getRoot().dynamic()); } @@ -471,7 +470,7 @@ public void testSubobjectsCannotBeUpdated() throws IOException { })))); MapperException exception = expectThrows( MapperException.class, - () -> mapper.mapping().merge(mergeWith, MergeReason.MAPPING_UPDATE, NewFieldsBudget.unlimited()) + () -> mapper.mapping().merge(mergeWith, MergeReason.MAPPING_UPDATE, Long.MAX_VALUE) ); assertEquals("the [subobjects] parameter can't be updated for the object mapping [field]", exception.getMessage()); } @@ -485,7 +484,7 @@ public void testSubobjectsCannotBeUpdatedOnRoot() throws IOException { })))); MapperException exception = expectThrows( MapperException.class, - () -> mapper.mapping().merge(mergeWith, MergeReason.MAPPING_UPDATE, NewFieldsBudget.unlimited()) + () -> mapper.mapping().merge(mergeWith, MergeReason.MAPPING_UPDATE, Long.MAX_VALUE) ); assertEquals("the [subobjects] parameter can't be updated for the object mapping [_doc]", exception.getMessage()); } diff --git a/server/src/test/java/org/elasticsearch/index/mapper/ParametrizedMapperTests.java b/server/src/test/java/org/elasticsearch/index/mapper/ParametrizedMapperTests.java index df8b2dcd0d593..562a30ba4f389 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/ParametrizedMapperTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/ParametrizedMapperTests.java @@ -23,7 +23,6 @@ import org.elasticsearch.index.analysis.IndexAnalyzers; import org.elasticsearch.index.analysis.NamedAnalyzer; import org.elasticsearch.index.mapper.FieldMapper.Parameter; -import org.elasticsearch.index.mapper.MapperMergeContext.NewFieldsBudget; import org.elasticsearch.plugins.MapperPlugin; import org.elasticsearch.plugins.Plugin; import org.elasticsearch.script.ScriptCompiler; @@ -346,7 +345,7 @@ public void testMerging() { {"type":"test_mapper","fixed":true,"fixed2":true,"required":"value"}"""); IllegalArgumentException e = expectThrows( IllegalArgumentException.class, - () -> mapper.merge(badMerge, MapperMergeContext.root(false, false, NewFieldsBudget.unlimited())) + () -> mapper.merge(badMerge, MapperMergeContext.root(false, false, Long.MAX_VALUE)) ); String expectedError = """ Mapper for [field] conflicts with existing mapper: @@ -359,7 +358,7 @@ public void testMerging() { // TODO: should we have to include 'fixed' here? Or should updates take as 'defaults' the existing values? TestMapper goodMerge = fromMapping(""" {"type":"test_mapper","fixed":false,"variable":"updated","required":"value"}"""); - TestMapper merged = (TestMapper) mapper.merge(goodMerge, MapperMergeContext.root(false, false, NewFieldsBudget.unlimited())); + TestMapper merged = (TestMapper) mapper.merge(goodMerge, MapperMergeContext.root(false, false, Long.MAX_VALUE)); assertEquals("{\"field\":" + mapping + "}", Strings.toString(mapper)); // original mapping is unaffected assertEquals(""" @@ -377,7 +376,7 @@ public void testMultifields() throws IOException { String addSubField = """ {"type":"test_mapper","variable":"foo","required":"value","fields":{"sub2":{"type":"keyword"}}}"""; TestMapper toMerge = fromMapping(addSubField); - TestMapper merged = (TestMapper) mapper.merge(toMerge, MapperMergeContext.root(false, false, NewFieldsBudget.unlimited())); + TestMapper merged = (TestMapper) mapper.merge(toMerge, MapperMergeContext.root(false, false, Long.MAX_VALUE)); assertEquals(XContentHelper.stripWhitespace(""" { "field": { @@ -400,7 +399,7 @@ public void testMultifields() throws IOException { TestMapper badToMerge = fromMapping(badSubField); IllegalArgumentException e = expectThrows( IllegalArgumentException.class, - () -> merged.merge(badToMerge, MapperMergeContext.root(false, false, NewFieldsBudget.unlimited())) + () -> merged.merge(badToMerge, MapperMergeContext.root(false, false, Long.MAX_VALUE)) ); assertEquals("mapper [field.sub2] cannot be changed from type [keyword] to [binary]", e.getMessage()); } @@ -416,13 +415,13 @@ public void testCopyTo() { TestMapper toMerge = fromMapping(""" {"type":"test_mapper","variable":"updated","required":"value","copy_to":["foo","bar"]}"""); - TestMapper merged = (TestMapper) mapper.merge(toMerge, MapperMergeContext.root(false, false, NewFieldsBudget.unlimited())); + TestMapper merged = (TestMapper) mapper.merge(toMerge, MapperMergeContext.root(false, false, Long.MAX_VALUE)); assertEquals(""" {"field":{"type":"test_mapper","variable":"updated","required":"value","copy_to":["foo","bar"]}}""", Strings.toString(merged)); TestMapper removeCopyTo = fromMapping(""" {"type":"test_mapper","variable":"updated","required":"value"}"""); - TestMapper noCopyTo = (TestMapper) merged.merge(removeCopyTo, MapperMergeContext.root(false, false, NewFieldsBudget.unlimited())); + TestMapper noCopyTo = (TestMapper) merged.merge(removeCopyTo, MapperMergeContext.root(false, false, Long.MAX_VALUE)); assertEquals(""" {"field":{"type":"test_mapper","variable":"updated","required":"value"}}""", Strings.toString(noCopyTo)); } @@ -488,7 +487,7 @@ public void testCustomSerialization() { TestMapper toMerge = fromMapping(conflict); IllegalArgumentException e = expectThrows( IllegalArgumentException.class, - () -> mapper.merge(toMerge, MapperMergeContext.root(false, false, NewFieldsBudget.unlimited())) + () -> mapper.merge(toMerge, MapperMergeContext.root(false, false, Long.MAX_VALUE)) ); assertEquals( "Mapper for [field] conflicts with existing mapper:\n" @@ -579,7 +578,7 @@ public void testAnalyzers() { TestMapper toMerge = fromMapping(mapping); e = expectThrows( IllegalArgumentException.class, - () -> original.merge(toMerge, MapperMergeContext.root(false, false, NewFieldsBudget.unlimited())) + () -> original.merge(toMerge, MapperMergeContext.root(false, false, Long.MAX_VALUE)) ); assertEquals( "Mapper for [field] conflicts with existing mapper:\n" + "\tCannot update parameter [analyzer] from [default] to [_standard]", From 678c4162724ef41903d8aaddf0d9538d8eb1889d Mon Sep 17 00:00:00 2001 From: Felix Barnsteiner Date: Tue, 23 Jan 2024 10:13:30 +0100 Subject: [PATCH 33/40] Fix checkstyle issue --- .../java/org/elasticsearch/index/mapper/MapperMergeContext.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/src/main/java/org/elasticsearch/index/mapper/MapperMergeContext.java b/server/src/main/java/org/elasticsearch/index/mapper/MapperMergeContext.java index 98e30df618ee4..89e50344b8850 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/MapperMergeContext.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/MapperMergeContext.java @@ -103,7 +103,7 @@ final class Limited implements NewFieldsBudget { private long fieldsBudget; - public Limited(long fieldsBudget) { + Limited(long fieldsBudget) { this.fieldsBudget = fieldsBudget; } From 3edf677493eb87158ac266b06d50c72e0cab158c Mon Sep 17 00:00:00 2001 From: Felix Barnsteiner Date: Tue, 23 Jan 2024 10:16:50 +0100 Subject: [PATCH 34/40] Rename method to MapperMergeContext#decrementFieldBudgetIfPossible --- .../org/elasticsearch/index/mapper/FieldMapper.java | 2 +- .../elasticsearch/index/mapper/MapperMergeContext.java | 2 +- .../org/elasticsearch/index/mapper/ObjectMapper.java | 4 ++-- .../elasticsearch/index/mapper/RootObjectMapper.java | 2 +- .../index/mapper/MapperMergeContextTests.java | 10 +++++----- 5 files changed, 10 insertions(+), 10 deletions(-) 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 5a57d1927efe1..9ed23f61bf0ea 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/FieldMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/FieldMapper.java @@ -455,7 +455,7 @@ private void add(FieldMapper mapper) { private void update(FieldMapper toMerge, MapperMergeContext context) { if (mapperBuilders.containsKey(toMerge.simpleName()) == false) { - if (context.decrementIfPossible(toMerge.mapperSize())) { + if (context.decrementFieldBudgetIfPossible(toMerge.mapperSize())) { add(toMerge); } } else { diff --git a/server/src/main/java/org/elasticsearch/index/mapper/MapperMergeContext.java b/server/src/main/java/org/elasticsearch/index/mapper/MapperMergeContext.java index 89e50344b8850..79adaf5966c5b 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/MapperMergeContext.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/MapperMergeContext.java @@ -64,7 +64,7 @@ MapperBuilderContext getMapperBuilderContext() { return mapperBuilderContext; } - boolean decrementIfPossible(int fieldSize) { + boolean decrementFieldBudgetIfPossible(int fieldSize) { return newFieldsBudget.decrementIfPossible(fieldSize); } 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 fb833557a619b..ec200e0ea00fa 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java @@ -547,7 +547,7 @@ private static Map buildMergedMappers( Mapper mergeIntoMapper = mergedMappers.get(mergeWithMapper.simpleName()); if (mergeIntoMapper == null) { - if (objectMergeContext.decrementIfPossible(mergeWithMapper.mapperSize())) { + if (objectMergeContext.decrementFieldBudgetIfPossible(mergeWithMapper.mapperSize())) { mergedMappers.put(mergeWithMapper.simpleName(), mergeWithMapper); } else if (mergeWithMapper instanceof ObjectMapper om) { addPartialObjectMapper(reason, objectMergeContext, mergedMappers, om); @@ -583,7 +583,7 @@ private static void addPartialObjectMapper( // 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 (objectMergeContext.decrementIfPossible(shallowObjectMapper.mapperSize())) { + if (objectMergeContext.decrementFieldBudgetIfPossible(shallowObjectMapper.mapperSize())) { // now trying to add the sub-fields one by one via a merge, until we hit the limit mergedMappers.put(objectMapper.simpleName(), shallowObjectMapper.merge(objectMapper, reason, objectMergeContext)); } 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 6371e36b09626..45ae186b51b2b 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/RootObjectMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/RootObjectMapper.java @@ -265,7 +265,7 @@ public RootObjectMapper merge(Mapper mergeWith, MergeReason reason, MapperMergeC } else if (runtimeFields.containsKey(runtimeField.getKey())) { runtimeFields.put(runtimeField.getKey(), runtimeField.getValue()); } else { - if (parentMergeContext.decrementIfPossible(1)) { + if (parentMergeContext.decrementFieldBudgetIfPossible(1)) { runtimeFields.put(runtimeField.getValue().name(), runtimeField.getValue()); } } diff --git a/server/src/test/java/org/elasticsearch/index/mapper/MapperMergeContextTests.java b/server/src/test/java/org/elasticsearch/index/mapper/MapperMergeContextTests.java index 59e92238f7c9c..9c38487dbdf7b 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/MapperMergeContextTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/MapperMergeContextTests.java @@ -14,19 +14,19 @@ public class MapperMergeContextTests extends ESTestCase { public void testAddFieldIfPossibleUnderLimit() { MapperMergeContext context = MapperMergeContext.root(false, false, 1); - assertTrue(context.decrementIfPossible(1)); - assertFalse(context.decrementIfPossible(1)); + assertTrue(context.decrementFieldBudgetIfPossible(1)); + assertFalse(context.decrementFieldBudgetIfPossible(1)); } public void testAddFieldIfPossibleAtLimit() { MapperMergeContext context = MapperMergeContext.root(false, false, 0); - assertFalse(context.decrementIfPossible(1)); + assertFalse(context.decrementFieldBudgetIfPossible(1)); } public void testAddFieldIfPossibleUnlimited() { MapperMergeContext context = MapperMergeContext.root(false, false, Long.MAX_VALUE); - assertTrue(context.decrementIfPossible(Integer.MAX_VALUE)); - assertTrue(context.decrementIfPossible(Integer.MAX_VALUE)); + assertTrue(context.decrementFieldBudgetIfPossible(Integer.MAX_VALUE)); + assertTrue(context.decrementFieldBudgetIfPossible(Integer.MAX_VALUE)); } } From c1b4258613cd5366b6d699b342e670112080fab4 Mon Sep 17 00:00:00 2001 From: Felix Barnsteiner Date: Tue, 23 Jan 2024 10:26:15 +0100 Subject: [PATCH 35/40] Move put method out of the if/else again --- .../index/mapper/ObjectMapper.java | 26 +++++++++---------- 1 file changed, 13 insertions(+), 13 deletions(-) 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 ec200e0ea00fa..e5c44e9d86499 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java @@ -546,14 +546,15 @@ private static Map buildMergedMappers( Mapper mergeWithMapper = iterator.next(); Mapper mergeIntoMapper = mergedMappers.get(mergeWithMapper.simpleName()); + Mapper merged = null; if (mergeIntoMapper == null) { if (objectMergeContext.decrementFieldBudgetIfPossible(mergeWithMapper.mapperSize())) { - mergedMappers.put(mergeWithMapper.simpleName(), mergeWithMapper); + merged = mergeWithMapper; } else if (mergeWithMapper instanceof ObjectMapper om) { - addPartialObjectMapper(reason, objectMergeContext, mergedMappers, om); + merged = truncateObjectMapper(reason, objectMergeContext, om); } } else if (mergeIntoMapper instanceof ObjectMapper objectMapper) { - mergedMappers.put(objectMapper.simpleName(), objectMapper.merge(mergeWithMapper, reason, objectMergeContext)); + merged = objectMapper.merge(mergeWithMapper, reason, objectMergeContext); } else { assert mergeIntoMapper instanceof FieldMapper || mergeIntoMapper instanceof FieldAliasMapper; if (mergeWithMapper instanceof NestedObjectMapper) { @@ -565,28 +566,27 @@ private static Map buildMergedMappers( // If we're merging template mappings when creating an index, then a field definition always // replaces an existing one. if (reason == MergeReason.INDEX_TEMPLATE) { - mergedMappers.put(mergeWithMapper.simpleName(), mergeWithMapper); + merged = mergeWithMapper; } else { - mergedMappers.put(mergeWithMapper.simpleName(), mergeIntoMapper.merge(mergeWithMapper, objectMergeContext)); + merged = mergeIntoMapper.merge(mergeWithMapper, objectMergeContext); } } + if (merged != null) { + mergedMappers.put(merged.simpleName(), merged); + } } return Map.copyOf(mergedMappers); } - private static void addPartialObjectMapper( - MergeReason reason, - MapperMergeContext objectMergeContext, - Map mergedMappers, - ObjectMapper objectMapper - ) { + private static ObjectMapper truncateObjectMapper(MergeReason reason, MapperMergeContext context, ObjectMapper objectMapper) { // 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 (objectMergeContext.decrementFieldBudgetIfPossible(shallowObjectMapper.mapperSize())) { + if (context.decrementFieldBudgetIfPossible(shallowObjectMapper.mapperSize())) { // now trying to add the sub-fields one by one via a merge, until we hit the limit - mergedMappers.put(objectMapper.simpleName(), shallowObjectMapper.merge(objectMapper, reason, objectMergeContext)); + return shallowObjectMapper.merge(objectMapper, reason, context); } + return null; } } From 4ab47e73b2425d19cb0b2569b55388297c2dff54 Mon Sep 17 00:00:00 2001 From: Felix Barnsteiner Date: Tue, 23 Jan 2024 14:19:48 +0100 Subject: [PATCH 36/40] Enhance javadoc --- .../main/java/org/elasticsearch/index/mapper/Mapping.java | 6 ++++-- .../java/org/elasticsearch/index/mapper/ObjectMapper.java | 4 ++++ 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/index/mapper/Mapping.java b/server/src/main/java/org/elasticsearch/index/mapper/Mapping.java index 2114fdc3a11c8..903e4e5da5b29 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/Mapping.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/Mapping.java @@ -177,8 +177,10 @@ Mapping merge(Mapping mergeWith, MergeReason reason, long newFieldsBudget) { */ public Mapping withFieldsBudget(long fieldsBudget) { MapperMergeContext mergeContext = MapperMergeContext.root(isSourceSynthetic(), false, fieldsBudget); - // calling merge to ensure we're only adding as many fields as allowed by the fields budget - return new Mapping(root.withoutMappers().merge(root, MergeReason.MAPPING_RECOVERY, mergeContext), metadataMappers, meta); + // get a copy of the root mapper, without any fields + RootObjectMapper shallowRoot = root.withoutMappers(); + // calling merge on the shallow root to ensure we're only adding as many fields as allowed by the fields budget + return new Mapping(shallowRoot.merge(root, MergeReason.MAPPING_RECOVERY, mergeContext), metadataMappers, meta); } @Override 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 e5c44e9d86499..177f00d9263ef 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java @@ -403,6 +403,10 @@ public ObjectMapper.Builder newBuilder(IndexVersion indexVersionCreated) { return builder; } + /** + * Returns a copy of this object mapper that doesn't have any fields and runtime fields. + * This is typically used in the context of a mapper merge when there's not enough budget to add the entire object. + */ ObjectMapper withoutMappers() { return new ObjectMapper(simpleName(), fullPath, enabled, subobjects, dynamic, null); } From 3aca9d94fa02afc27af27dd3b9dc92538a36f3c7 Mon Sep 17 00:00:00 2001 From: Felix Barnsteiner Date: Tue, 23 Jan 2024 14:20:07 +0100 Subject: [PATCH 37/40] Enhance testMergeWithLimitTruncatedObjectField --- .../elasticsearch/index/mapper/ObjectMapperMergeTests.java | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/server/src/test/java/org/elasticsearch/index/mapper/ObjectMapperMergeTests.java b/server/src/test/java/org/elasticsearch/index/mapper/ObjectMapperMergeTests.java index 8f95b169ede5f..8218dc5ff650d 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/ObjectMapperMergeTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/ObjectMapperMergeTests.java @@ -219,7 +219,7 @@ public void testMergeWithLimit() { assertEquals(4, mergedAdd1.mapperSize()); } - public void testMergeWithLimitObjectField() { + public void testMergeWithLimitTruncatedObjectField() { RootObjectMapper root = new RootObjectMapper.Builder("_doc", Explicit.IMPLICIT_TRUE).build(MapperBuilderContext.root(false, false)); RootObjectMapper mergeWith = new RootObjectMapper.Builder("_doc", Explicit.IMPLICIT_TRUE).add( new ObjectMapper.Builder("parent", Explicit.IMPLICIT_FALSE).add( @@ -227,12 +227,15 @@ public void testMergeWithLimitObjectField() { ).add(new KeywordFieldMapper.Builder("child2", IndexVersion.current())) ).build(MapperBuilderContext.root(false, false)); + ObjectMapper mergedAdd0 = root.merge(mergeWith, MapperMergeContext.root(false, false, 0)); 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()); ObjectMapper parent1 = (ObjectMapper) mergedAdd1.getMapper("parent"); assertNull(parent1.getMapper("child1")); From 7e9d8d0134f391dbb47266e89b11f8d2678d65eb Mon Sep 17 00:00:00 2001 From: Felix Barnsteiner Date: Tue, 23 Jan 2024 14:21:55 +0100 Subject: [PATCH 38/40] Remove assertion --- .../java/org/elasticsearch/index/mapper/RootObjectMapper.java | 1 - 1 file changed, 1 deletion(-) 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 45ae186b51b2b..82cda33aa77b6 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/RootObjectMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/RootObjectMapper.java @@ -258,7 +258,6 @@ public RootObjectMapper merge(Mapper mergeWith, MergeReason reason, MapperMergeC dynamicTemplates = this.dynamicTemplates; } final Map runtimeFields = new HashMap<>(this.runtimeFields); - assert this.runtimeFields != mergeWithObject.runtimeFields || this.runtimeFields == Map.of(); for (Map.Entry runtimeField : mergeWithObject.runtimeFields.entrySet()) { if (runtimeField.getValue() == null) { runtimeFields.remove(runtimeField.getKey()); From 1dc26dae60ecff451269404ec1f4d23e597e241f Mon Sep 17 00:00:00 2001 From: Felix Barnsteiner Date: Tue, 23 Jan 2024 17:11:30 +0100 Subject: [PATCH 39/40] Add more cases to ObjectMapperMergeTests --- .../index/mapper/ObjectMapperMergeTests.java | 44 +++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/server/src/test/java/org/elasticsearch/index/mapper/ObjectMapperMergeTests.java b/server/src/test/java/org/elasticsearch/index/mapper/ObjectMapperMergeTests.java index 8218dc5ff650d..8eb824884a591 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/ObjectMapperMergeTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/ObjectMapperMergeTests.java @@ -250,6 +250,33 @@ public void testMergeWithLimitTruncatedObjectField() { assertNotNull(parent3.getMapper("child2")); } + public void testMergeSameObjectDifferentFields() { + RootObjectMapper root = new RootObjectMapper.Builder("_doc", Explicit.IMPLICIT_TRUE).add( + new ObjectMapper.Builder("parent", Explicit.IMPLICIT_TRUE).add(new KeywordFieldMapper.Builder("child1", IndexVersion.current())) + ).build(MapperBuilderContext.root(false, false)); + RootObjectMapper mergeWith = new RootObjectMapper.Builder("_doc", Explicit.IMPLICIT_TRUE).add( + new ObjectMapper.Builder("parent", Explicit.IMPLICIT_TRUE).add( + new KeywordFieldMapper.Builder("child1", IndexVersion.current()).ignoreAbove(42) + ).add(new KeywordFieldMapper.Builder("child2", IndexVersion.current())) + ).build(MapperBuilderContext.root(false, false)); + + 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()); + + ObjectMapper parent0 = (ObjectMapper) mergedAdd0.getMapper("parent"); + assertNotNull(parent0.getMapper("child1")); + assertEquals(42, ((KeywordFieldMapper) parent0.getMapper("child1")).fieldType().ignoreAbove()); + assertNull(parent0.getMapper("child2")); + + ObjectMapper parent1 = (ObjectMapper) mergedAdd1.getMapper("parent"); + assertNotNull(parent1.getMapper("child1")); + assertEquals(42, ((KeywordFieldMapper) parent1.getMapper("child1")).fieldType().ignoreAbove()); + assertNotNull(parent1.getMapper("child2")); + } + public void testMergeWithLimitMultiField() { RootObjectMapper mergeInto = new RootObjectMapper.Builder("_doc", Explicit.IMPLICIT_TRUE).add( createTextKeywordMultiField("text", "keyword1") @@ -267,6 +294,23 @@ public void testMergeWithLimitMultiField() { assertEquals(3, mergedAdd1.mapperSize()); } + public void testMergeWithLimitRuntimeField() { + RootObjectMapper mergeInto = new RootObjectMapper.Builder("_doc", Explicit.IMPLICIT_TRUE).addRuntimeField( + new TestRuntimeField("existing_runtime_field", "keyword") + ).add(createTextKeywordMultiField("text", "keyword1")).build(MapperBuilderContext.root(false, false)); + RootObjectMapper mergeWith = new RootObjectMapper.Builder("_doc", Explicit.IMPLICIT_TRUE).addRuntimeField( + 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()); + + 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()); + } + private static RootObjectMapper createRootSubobjectFalseLeafWithDots() { FieldMapper.Builder fieldBuilder = new KeywordFieldMapper.Builder("host.name", IndexVersion.current()); FieldMapper fieldMapper = fieldBuilder.build(MapperBuilderContext.root(false, false)); From 6383ee619f4491676dd77adbe13eacae735bbdea Mon Sep 17 00:00:00 2001 From: Felix Barnsteiner Date: Tue, 23 Jan 2024 17:14:29 +0100 Subject: [PATCH 40/40] Add test cases for ObjectMapper#withoutMappers --- .../index/mapper/NestedObjectMapper.java | 2 +- .../index/mapper/ObjectMapper.java | 2 +- .../index/mapper/NestedObjectMapperTests.java | 32 ++++++++++++ .../index/mapper/ObjectMapperTests.java | 32 ++++++++++++ .../index/mapper/RootObjectMapperTests.java | 49 +++++++++++++++++++ 5 files changed, 115 insertions(+), 2 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/index/mapper/NestedObjectMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/NestedObjectMapper.java index 1ee8a0b1bf80a..dd2d4407bed03 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/NestedObjectMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/NestedObjectMapper.java @@ -191,7 +191,7 @@ NestedObjectMapper withoutMappers() { return new NestedObjectMapper( simpleName(), fullPath(), - null, + Map.of(), enabled, dynamic, includeInParent, 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 177f00d9263ef..2c89f60f40975 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java @@ -408,7 +408,7 @@ public ObjectMapper.Builder newBuilder(IndexVersion indexVersionCreated) { * This is typically used in the context of a mapper merge when there's not enough budget to add the entire object. */ ObjectMapper withoutMappers() { - return new ObjectMapper(simpleName(), fullPath, enabled, subobjects, dynamic, null); + return new ObjectMapper(simpleName(), fullPath, enabled, subobjects, dynamic, Map.of()); } @Override diff --git a/server/src/test/java/org/elasticsearch/index/mapper/NestedObjectMapperTests.java b/server/src/test/java/org/elasticsearch/index/mapper/NestedObjectMapperTests.java index f4ed4b8620306..61d62c1e41969 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/NestedObjectMapperTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/NestedObjectMapperTests.java @@ -13,6 +13,7 @@ import org.elasticsearch.common.bytes.BytesReference; import org.elasticsearch.common.compress.CompressedXContent; import org.elasticsearch.common.settings.Settings; +import org.elasticsearch.core.CheckedConsumer; import org.elasticsearch.index.IndexVersion; import org.elasticsearch.index.IndexVersions; import org.elasticsearch.index.mapper.MapperService.MergeReason; @@ -1520,4 +1521,35 @@ public void testMergeNested() { assertFalse(result.isIncludeInParent()); assertTrue(result.isIncludeInRoot()); } + + public void testWithoutMappers() throws IOException { + ObjectMapper shallowObject = createNestedObjectMapperWithAllParametersSet(b -> {}); + ObjectMapper object = createNestedObjectMapperWithAllParametersSet(b -> { + b.startObject("keyword"); + { + b.field("type", "keyword"); + } + b.endObject(); + }); + assertThat(object.withoutMappers().toString(), equalTo(shallowObject.toString())); + } + + private NestedObjectMapper createNestedObjectMapperWithAllParametersSet(CheckedConsumer propertiesBuilder) + throws IOException { + DocumentMapper mapper = createDocumentMapper(mapping(b -> { + b.startObject("nested_object"); + { + b.field("type", "nested"); + b.field("enabled", false); + b.field("dynamic", false); + b.field("include_in_parent", true); + b.field("include_in_root", true); + b.startObject("properties"); + propertiesBuilder.accept(b); + b.endObject(); + } + b.endObject(); + })); + return (NestedObjectMapper) mapper.mapping().getRoot().getMapper("nested_object"); + } } 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 97d4b01675c3f..6e958ddbea904 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/ObjectMapperTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/ObjectMapperTests.java @@ -14,9 +14,11 @@ import org.elasticsearch.common.bytes.BytesReference; import org.elasticsearch.common.compress.CompressedXContent; import org.elasticsearch.common.settings.Settings; +import org.elasticsearch.core.CheckedConsumer; import org.elasticsearch.index.IndexVersion; import org.elasticsearch.index.mapper.MapperService.MergeReason; import org.elasticsearch.index.mapper.ObjectMapper.Dynamic; +import org.elasticsearch.xcontent.XContentBuilder; import org.elasticsearch.xcontent.XContentFactory; import org.elasticsearch.xcontent.XContentType; @@ -538,4 +540,34 @@ public void testNestedObjectWithMultiFieldsMapperSize() throws IOException { ); assertThat(mapperBuilder.build(MapperBuilderContext.root(false, false)).mapperSize(), equalTo(5)); } + + public void testWithoutMappers() throws IOException { + ObjectMapper shallowObject = createObjectMapperWithAllParametersSet(b -> {}); + ObjectMapper object = createObjectMapperWithAllParametersSet(b -> { + b.startObject("keyword"); + { + b.field("type", "keyword"); + } + b.endObject(); + }); + assertThat(object.withoutMappers().toString(), equalTo(shallowObject.toString())); + } + + private ObjectMapper createObjectMapperWithAllParametersSet(CheckedConsumer propertiesBuilder) + throws IOException { + DocumentMapper mapper = createDocumentMapper(mapping(b -> { + b.startObject("object"); + { + b.field("type", "object"); + b.field("subobjects", false); + b.field("enabled", false); + b.field("dynamic", false); + b.startObject("properties"); + propertiesBuilder.accept(b); + b.endObject(); + } + b.endObject(); + })); + return (ObjectMapper) mapper.mapping().getRoot().getMapper("object"); + } } 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 03bc835f3bf33..b2a6651142181 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/RootObjectMapperTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/RootObjectMapperTests.java @@ -10,6 +10,7 @@ import org.elasticsearch.common.Strings; import org.elasticsearch.common.xcontent.XContentHelper; +import org.elasticsearch.core.CheckedConsumer; import org.elasticsearch.index.mapper.MapperService.MergeReason; import org.elasticsearch.xcontent.XContentBuilder; import org.elasticsearch.xcontent.XContentFactory; @@ -19,6 +20,7 @@ import java.util.Collections; import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.instanceOf; public class RootObjectMapperTests extends MapperServiceTestCase { @@ -359,4 +361,51 @@ public void testEmptyType() throws Exception { assertThat(e.getMessage(), containsString("type cannot be an empty string")); } + public void testWithoutMappers() throws IOException { + RootObjectMapper shallowRoot = createRootObjectMapperWithAllParametersSet(b -> {}, b -> {}); + RootObjectMapper root = createRootObjectMapperWithAllParametersSet(b -> { + b.startObject("keyword"); + { + b.field("type", "keyword"); + } + b.endObject(); + }, b -> { + b.startObject("runtime"); + b.startObject("field").field("type", "keyword").endObject(); + b.endObject(); + }); + assertThat(root.withoutMappers().toString(), equalTo(shallowRoot.toString())); + } + + private RootObjectMapper createRootObjectMapperWithAllParametersSet( + CheckedConsumer buildProperties, + CheckedConsumer buildRuntimeFields + ) throws IOException { + DocumentMapper mapper = createDocumentMapper(topMapping(b -> { + b.field("enabled", false); + b.field("subobjects", false); + b.field("dynamic", false); + b.field("date_detection", false); + b.field("numeric_detection", false); + b.field("dynamic_date_formats", Collections.singletonList("yyyy-MM-dd")); + b.startArray("dynamic_templates"); + { + b.startObject(); + { + b.startObject("my_template"); + { + b.startObject("mapping").field("type", "keyword").endObject(); + } + b.endObject(); + } + b.endObject(); + } + b.endArray(); + b.startObject("properties"); + buildProperties.accept(b); + b.endObject(); + })); + return mapper.mapping().getRoot(); + } + }