Don't run include_in_parent when in copy_to context#87123
Don't run include_in_parent when in copy_to context#87123romseygeek merged 6 commits intoelastic:masterfrom
Conversation
|
Pinging @elastic/es-search (Team:Search) |
|
Hi @romseygeek, I've created a changelog YAML for you. |
ywelsch
left a comment
There was a problem hiding this comment.
Thanks for looking into this, @romseygeek! I've left one question, looking good o.w.
| String nestedPathFieldName = NestedPathFieldMapper.name(indexCreatedVersion); | ||
| for (IndexableField field : nestedDoc.getFields()) { | ||
| if (field.name().equals(nestedPathFieldName) == false) { | ||
| if (field.name().equals(nestedPathFieldName) == false && field.name().equals(IdFieldMapper.NAME) == false) { |
There was a problem hiding this comment.
Should we move the logic for adding _id fields to nested docs to the IdFieldMapper.postParse method, similar as we have it for other metadata fields? Then we wouldn't need this exception here?
There was a problem hiding this comment.
That sounds like a good plan, but I think it might end up being a fairly big change as there are a few places that refer directly to the id field scattered around the place. It also interacts with TSDB in a slightly complicated way - are you OK with doing this as a follow-up?
| @@ -345,6 +345,13 @@ private static void throwEOF(ObjectMapper mapper, String currentFieldName) { | |||
| } | |||
|
|
|||
| private static void nested(DocumentParserContext context, NestedObjectMapper nested) { | |||
There was a problem hiding this comment.
This method name does not suggest at all that this method is about adding fields to parent/root doc. Should this be renamed to make the purpose clearer?
There was a problem hiding this comment.
++ have renamed to copyNestedFields
… into mapper/copy-to-transitivity
|
Thanks for the review @ywelsch, this is ready for another go-around. |
ywelsch
left a comment
There was a problem hiding this comment.
LGTM. Thank you for digging into this bug, and the fix!
|
@elasticmachine update branch |
We changed how copy_to is implemented in elastic#79922, which moved the handling of dots in field names into a specialised parser. Unfortunately, while doing this we added a bug whereby every time a copy_to directive is processed for a nested field, the nested field's include_in_parent logic would be run, meaning that the parent would end up with multiple copies of the nested child's fields. This commit fixes this by only running include_in_parent when the parser is not in a copy_to context. It also fixes another bug that meant the parent document would contain multiple copies of the ID field. Fixes elastic#87036
💚 Backport successful
|
We changed how copy_to is implemented in #79922, which moved the handling of dots in field names into a specialised parser. Unfortunately, while doing this we added a bug whereby every time a copy_to directive is processed for a nested field, the nested field's include_in_parent logic would be run, meaning that the parent would end up with multiple copies of the nested child's fields. This commit fixes this by only running include_in_parent when the parser is not in a copy_to context. It also fixes another bug that meant the parent document would contain multiple copies of the ID field. Fixes #87036
We changed how copy_to is implemented in #79922, which moved
the handling of dots in field names into a specialised parser. Unfortunately,
while doing this we added a bug whereby every time a copy_to directive
is processed for a nested field, the nested field's include_in_parent logic
would be run, meaning that the parent would end up with multiple copies
of the nested child's fields.
This commit fixes this by only running include_in_parent when the parser
is not in a copy_to context. It also fixes another bug that meant the parent
document would contain multiple copies of the ID field.
Fixes #87036