Simplify adding dynamic sub-fields to their dynamic parent object#87866
Simplify adding dynamic sub-fields to their dynamic parent object#87866javanna merged 2 commits intoelastic:masterfrom
Conversation
DocumentParserContext holds all the dynamic mappers created while parsing a document, as well as dynamic object mappers on a separate map so they can be looked up by name quickly when parsing their sub-fields and looking for their parent field. For every sub-field, so far we looked up the corresponding object mapper and then call .newBuilder on it. Calling newBuilder can be done already when the object mapper is added to the map, which simplifies the logic down the line as instead of creating a new builde for each sub-field, we re-use the same builder and add sub-fields to it. This has no effect on the resulting dynamic update, but it simplifies the code and it should have a positive impact on performance.
javanna
added
the
:Search Foundations/Mapping
|
Pinging @elastic/es-search (Team:Search) |
romseygeek
left a comment
romseygeek
left a comment
There was a problem hiding this comment.
LGTM.
One question, does this mean that we can get rid of the object merging when we build the final dynamic update now? Because we should only have a single builder per object, right?
| if (parentMapper == null) { | ||
| ObjectMapper.Builder dynamicObjectMapperBuilder = context.getDynamicObjectMapperBuilder(parentName); | ||
| if (dynamicObjectMapperBuilder == null) { | ||
| // it can still happen that the path is ambiguous and we are not able to locate the parent |
There was a problem hiding this comment.
I wonder if this is still true? I guess it doesn't matter so much.
There was a problem hiding this comment.
we are very much aligned :) I want to to do something about this in a follow-up. I think probably have an assertion that this never happens. If it does happen I would like to know why...
Not yet, but that would be the plan more or less... I haven't figured out exactly how to get there but I am on it. |
|
Heads, up this was reverted due to a considerable performance regression when building the root object mapper as part of creating the dynamic mapping update. I will dig to understand what caused that and eventually reopen. |
4 participants
DocumentParserContext holds all the dynamic mappers created while parsing a document, as well as dynamic object mappers on a separate map so they can be looked up by name quickly when parsing their sub-fields and looking for their parent field. For every sub-field, so far we looked up the corresponding object mapper and then call .newBuilder on it. Calling newBuilder can be done already when the object mapper is added to the map, which simplifies the logic down the line as instead of creating a new builde for each sub-field, we re-use the same builder and add sub-fields to it. This has no effect on the resulting dynamic update, but it simplifies the code and it should have a positive impact on performance.