Include runtime fields in total fields count #89251
Conversation
We have a check that enforces the total number of fields needs to be below a certain (configurable) threshold. Before runtime fields did not contribute to the count. This patch makes all runtime fields contribute to the count, runtime fields: - that were explicitly defined in mapping by a user - as well as runtime fields that were dynamically created by dynamic mappings Closes elastic#88265
mayya-sharipova
added
:Search Foundations/Mapping
|
Pinging @elastic/es-search (Team:Search) |
|
Hi @mayya-sharipova, I've created a changelog YAML for you. |
| // 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)) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
server/src/internalClusterTest/java/org/elasticsearch/index/mapper/DynamicMappingIT.java
Outdated
Show resolved
Hide resolved
server/src/internalClusterTest/java/org/elasticsearch/index/mapper/DynamicMappingIT.java
Outdated
Show resolved
Hide resolved
|
@javanna Thanks for the initial review. I've tried to address in 1ee4270
I am happy to add extra info in the error message as a part of this PR. I wonder if this constitutes a breaking change, or we are fine to change to expand error messages? |
| @@ -279,6 +279,9 @@ final ObjectMapper getDynamicObjectMapper(String name) { | |||
| * Add a new runtime field dynamically created while parsing. | |||
| */ | |||
| public final void addDynamicRuntimeField(RuntimeField runtimeField) { | |||
| if (newFieldsSeen.add(runtimeField.name())) { | |||
There was a problem hiding this comment.
A runtime field can have the same name as an indexed field, in which case it will shadow the indexed field at search time. I wondered if it's ok to have the same set for both runtime fields and indexed fields and it does look ok because when it comes to dynamic mappings, when we encounter a new field, either we map it as a runtime field or as an indexed field, and never both, so there are never going to be collisions. Shall we add a comment to clarify that?
There was a problem hiding this comment.
@javanna Good suggestion, addressed in dbf2022
Indeed for dynamic updates, a field can be mapped either as runtime field or as indexed field, so no problem here.
For PUT _mapping updates, a user can introduce a new runtime field with the same name as existing indexed field. In this case, this new runtime field will be added to the total count, and can cause an exception for total field counts.
server/src/main/java/org/elasticsearch/index/mapper/DocumentParserContext.java
Outdated
Show resolved
Hide resolved
mayya-sharipova
force-pushed
the
runtime-fields-total-count
branch
from
69582cf to
dbf2022
Compare
We have a check that enforces the total number of fields needs to be below a
certain (configurable) threshold. Before runtime fields did not contribute
to the count. This patch makes all runtime fields contribute to the
count, runtime fields:
mappings
Closes #88265