Store dynamic mapping updates as builders #90674
Conversation
romseygeek
added
:Search Foundations/Mapping
|
Pinging @elastic/es-search (Team:Search) |
There was a problem hiding this comment.
This reminds me of #87866 . I am rusty on mappings code, and remember vaguely some performance regression I introduced with #87866 but I can't remember the root cause.
There are some subtleties that I'd like to better understand. It feels like a good change, I am just a bit paranoid that it may break something. I'd like to have a deeper look.
| } | ||
| } | ||
| if (dynamicMappers.containsKey(fullName)) { | ||
| // we've got the same dynamic mapper added in two ways, maybe due to a difference | ||
| // in dot notation: { "a" : { "b" : "value1" }, "a.b" : "value2" } |
There was a problem hiding this comment.
is this possible in practice? we either expand dots or subobjects are disallowed? Would it be possible to instead replace this with an assertion to enforce that it never really happens?
| ObjectMapper.Builder builder = context.getDynamicObjectMapper(fullName); | ||
| if (builder != null) { | ||
| // we re-use builder instances so if the builder has already been | ||
| // added we don't need to do so again |
There was a problem hiding this comment.
what would cause this situation in practice?
There was a problem hiding this comment.
If the input document has mixed object and dot notation for fields then we can end up creating the same parent object builder twice.
| for (Mapper mapper : context.getDynamicMappers()) { | ||
| rootBuilder.addDynamic(mapper.name(), null, mapper, context); | ||
| } | ||
| context.getDynamicMappers().forEach((name, builder) -> rootBuilder.addDynamic(name, null, builder, context)); |
| // we could potentially skip the step of adding these to the dynamic mappers, because their parent is already added to | ||
| // that list, and what is important is that all of the intermediate objects are added to the dynamic object mappers so that | ||
| // they can be looked up once sub-fields need to be added to them. For simplicity, we treat these like any other object | ||
| addDynamicMapper(submapper); | ||
| addDynamicMapper(fullName + "." + submapper.name, submapper); |
There was a problem hiding this comment.
so the full name is actually the path? How does this work with subobjects false? Is it then empty and it's all in the leaf name?
There was a problem hiding this comment.
This never gets called with subobjects:false because we're explicitly dealing with subobjects here.
Note this breaks sodding everything
|
@elasticmachine update branch |
|
This conflicts with #96235. I'm trying to get the size of all dynamic mappers in |
|
Maybe it's not as difficult to implement |
Reverts elastic#90674 The revert is not perfectly clean as there are some minor adjustments to account for later changes. This is in contrast with: elastic#103858 closes: elastic#103011
Reverts elastic#90674 The revert is not perfectly clean as there are some minor adjustments to account for later changes. This is in contrast with: elastic#103858 closes: elastic#103011
Currently, newly created dynamic mappers as stored on the parser context as
instantiated mappers. This means that when we build the mapping update from
the collected dynamic mappers, we need to convert them back into builders
again, and immediately re-build them. This commit changes the context to
instead store just the builders, which results in a small simplification to the
dynamic update build logic and removes another method on ObjectMapper.Builder.