Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Include runtime fields in total fields count #89251

Merged
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions docs/changelog/89251.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
pr: 89251
summary: Include runtime fields in total fields count
area: Mapping
type: bug
issues:
- 88265
3 changes: 2 additions & 1 deletion docs/reference/mapping/mapping-settings-limit.asciidoc
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,8 @@ Use the following settings to limit the number of field mappings (created manual

`index.mapping.total_fields.limit`::
The maximum number of fields in an index. Field and object mappings, as well as
field aliases count towards this limit. The default value is `1000`.
field aliases count towards this limit. Mapped runtime fields count towards this
limit as well. The default value is `1000`.
+
[IMPORTANT]
====
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
import org.elasticsearch.action.support.WriteRequest;
import org.elasticsearch.cluster.ClusterState;
import org.elasticsearch.cluster.ClusterStateUpdateTask;
import org.elasticsearch.cluster.metadata.IndexMetadata;
import org.elasticsearch.cluster.metadata.MappingMetadata;
import org.elasticsearch.cluster.service.ClusterService;
import org.elasticsearch.common.Randomness;
Expand Down Expand Up @@ -213,6 +214,69 @@ public void onFailure(Exception e) {
}
}

public void testTotalFieldsLimitWithRunTimefields() {
mayya-sharipova marked this conversation as resolved.
Show resolved Hide resolved
Settings indexSettings = Settings.builder()
.put(IndexMetadata.SETTING_NUMBER_OF_SHARDS, 1)
.put(INDEX_MAPPING_TOTAL_FIELDS_LIMIT_SETTING.getKey(), 4)
.build();

String mapping = """
{
"dynamic":"runtime",
"runtime": {
"my_object.rfield1": {
"type": "keyword"
},
"rfield2": {
"type": "keyword"
}
},
"properties": {
"field3" : {
"type": "keyword"
}
}
}
""";

client().admin().indices().prepareCreate("index1").setSettings(indexSettings).setMapping(mapping).get();
ensureGreen("index1");

{
// introduction of a new object with 2 sub-fields fails
final IndexRequestBuilder indexRequestBuilder = client().prepareIndex("index1")
.setId("1")
.setSource("my_object2", Map.of("field1", "value1", "field2", "value2"));
mayya-sharipova marked this conversation as resolved.
Show resolved Hide resolved
Exception exc = expectThrows(MapperParsingException.class, () -> indexRequestBuilder.get(TimeValue.timeValueSeconds(10)));
assertThat(exc.getMessage(), Matchers.containsString("failed to parse"));
assertThat(exc.getCause(), instanceOf(IllegalArgumentException.class));
assertThat(
exc.getCause().getMessage(),
Matchers.containsString("Limit of total fields [4] has been exceeded while adding new fields [2]")
);
}

{
// introduction of a single field succeeds
client().prepareIndex("index1").setId("2").setSource("field4", 100).get();
}

{
// remove 2 runtime field mappings
assertAcked(client().admin().indices().preparePutMapping("index1").setSource("""
{
"runtime": {
"my_object.rfield1": null,
"rfield2" : null
}
}
""", XContentType.JSON));

// introduction of a new object with 2 sub-fields succeeds
client().prepareIndex("index1").setId("2").setSource("my_object2", Map.of("field1", "value1", "field2", "value2")).get();
}
}

public void testMappingVersionAfterDynamicMappingUpdate() throws Exception {
createIndex("test");
final ClusterService clusterService = internalCluster().clusterService();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -226,14 +226,7 @@ public final String documentDescription() {
* Add a new mapper dynamically created while parsing.
*/
public final void addDynamicMapper(Mapper mapper) {
// eagerly check field name limit here to avoid OOM errors
// only check fields that are not already mapped or tracked in order to avoid hitting field limit too early via double-counting
// note that existing fields can also receive dynamic mapping updates (e.g. constant_keyword to fix the value)
if (mappingLookup.getMapper(mapper.name()) == null
&& mappingLookup.objectMappers().containsKey(mapper.name()) == false
&& newFieldsSeen.add(mapper.name())) {
mappingLookup.checkFieldLimit(indexSettings().getMappingTotalFieldsLimit(), newFieldsSeen.size());
}
checkFieldLimit(mapper.name());
if (mapper instanceof ObjectMapper objectMapper) {
dynamicObjectMappers.put(objectMapper.name(), objectMapper);
// dynamic object mappers may have been obtained from applying a dynamic template, in which case their definition may contain
Expand All @@ -254,6 +247,17 @@ public final void addDynamicMapper(Mapper mapper) {
dynamicMappers.add(mapper);
}

private void checkFieldLimit(String fieldName) {
// eagerly check field name limit here to avoid OOM errors
// only check fields that are not already mapped or tracked in order to avoid hitting field limit too early via double-counting
// note that existing fields can also receive dynamic mapping updates (e.g. constant_keyword to fix the value)
if (mappingLookup.getMapper(fieldName) == null
&& mappingLookup.objectMappers().containsKey(fieldName) == false
&& newFieldsSeen.add(fieldName)) {
Copy link
Member

Choose a reason for hiding this comment

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

I had to catch up on the existing logic and why we need to check that the mapper does not exist: don't we call this method only for new fields? There is actually a chance that existing fields receive dynamic updates, it's all in the comment above ( note that existing fields can also receive dynamic mapping updates).

When we are adding a dynamic runtime field these these two checks are redundant, because they will always return false? Do we want to have two different codepaths then for the two scenarios? Though I would not add a check that the runtime field does not yet exist.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, based on the comments, since existing mappers can receive mapping updates, we needed this check.

When we are adding a dynamic runtime field these these two checks are redundant, because they will always return false? Do we want to have two different codepaths then for the two scenarios?

Thanks for explaining this, I did not know this. I've made the tow different codepaths in 1ee4270

mappingLookup.checkFieldLimit(indexSettings().getMappingTotalFieldsLimit(), newFieldsSeen.size());
}
}

/**
* Get dynamic mappers created as a result of parsing an incoming document. Responsible for exposing all the newly created
* fields that need to be merged into the existing mappings. Used to create the required mapping update at the end of document parsing.
Expand All @@ -279,6 +283,7 @@ final ObjectMapper getDynamicObjectMapper(String name) {
* Add a new runtime field dynamically created while parsing.
*/
public final void addDynamicRuntimeField(RuntimeField runtimeField) {
mayya-sharipova marked this conversation as resolved.
Show resolved Hide resolved
checkFieldLimit(runtimeField.name());
dynamicRuntimeFields.add(runtimeField);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ private CacheKey() {}
/** Full field name to mapper */
private final Map<String, Mapper> fieldMappers;
private final Map<String, ObjectMapper> objectMappers;
private final int runtimeFieldMappersCount;
private final NestedLookup nestedLookup;
private final FieldTypeLookup fieldTypeLookup;
private final FieldTypeLookup indexTimeLookup; // for index-time scripts, a lookup that does not include runtime fields
Expand Down Expand Up @@ -180,6 +181,7 @@ private MappingLookup(
// make all fields into compact+fast immutable maps
this.fieldMappers = Map.copyOf(fieldMappers);
this.objectMappers = Map.copyOf(objects);
this.runtimeFieldMappersCount = runtimeFields.size();
this.indexAnalyzersMap = Map.copyOf(indexAnalyzersMap);
this.completionFields = Set.copyOf(completionFields);
this.indexTimeScriptMappers = List.copyOf(indexTimeScriptMappers);
Expand Down Expand Up @@ -262,7 +264,8 @@ private void checkFieldLimit(long limit) {
}

void checkFieldLimit(long limit, int additionalFieldsToAdd) {
if (fieldMappers.size() + objectMappers.size() + additionalFieldsToAdd - mapping.getSortedMetadataMappers().length > limit) {
if (fieldMappers.size() + objectMappers.size() + runtimeFieldMappersCount + additionalFieldsToAdd - mapping
.getSortedMetadataMappers().length > limit) {
throw new IllegalArgumentException(
"Limit of total fields ["
+ limit
Expand Down