Prevent duplicate fields when mixing parent and root nested includes#27072
Prevent duplicate fields when mixing parent and root nested includes#27072jpountz merged 3 commits intoelastic:masterfrom original-brownbear:26990
Conversation
jpountz
left a comment
There was a problem hiding this comment.
I think you found the right place to fix the issue, however I spotted a bug (see comments).
There was a problem hiding this comment.
I don't think it is correct: included should also true if nested.isIncludeInRoot is true? Otherwise we might recurse on a wrong value.
There was a problem hiding this comment.
However I agree that we should test for if (nested.isNested() && nested.isIncludeInParent()) here.
|
@jpountz fixed + added a test that reproduced the bug you found. Also a little more readable now I think/hope, since now it's clearly visible that we're fixing only if included in parent and included in root are true simultaneously :) |
jpountz
left a comment
There was a problem hiding this comment.
I just left a naming suggestion, but the change looks good to me!
There was a problem hiding this comment.
should we rename to something like includeInRootViaParent to make it clearer what this is about?
|
@jpountz I agree renamed that variable :) |
|
@jpountz build failure looks unrelated, should we retrigger Jenkins? |
|
@elasticmachine please test it |
|
@jpountz hmm no seems, there's an actual issue now. Failed the same way for me locally, sorry. Will fix shortly. |
…cludes (follow up)
…cludes (follow up)
|
rebase against |
|
I see the same error here for another PR and it seems to happen randomly locally https://elasticsearch-ci.elastic.co/job/elastic+elasticsearch+pull-request/5141/console so not related it seems. |
|
@elasticmachine please test it |
|
Sure, I'll merge. Thanks @original-brownbear ! |
|
@jpountz np + thanks for the review :) |
|
@jpountz ping :) |
|
Thanks @original-brownbear ! |
|
np @jpountz :) |
* master: Enhances exists queries to reduce need for `_field_names` (elastic#26930) Added new terms_set query Set request body to required to reflect the code base (elastic#27188) Update Docker docs for 6.0.0-rc2 (elastic#27166) Add version 6.0.0 Docs: restore now fails if it encounters incompatible settings (elastic#26933) Convert index blocks to cluster block exceptions (elastic#27050) [DOCS] Link remote info API in Cross Cluster Search docs page Fix Laplace scorer to multiply by alpha (and not add) (elastic#27125) [DOCS] Clarify migrate guide and search request validation Raise IllegalArgumentException if query validation failed (elastic#26811) prevent duplicate fields when mixing parent and root nested includes (elastic#27072) TopHitsAggregator must propagate calls to `setScorer`. (elastic#27138)
* master: Remove checkpoint tracker bit sets setting Fix stable BWC branch detection logic Fix logic detecting unreleased versions Enhances exists queries to reduce need for `_field_names` (elastic#26930) Added new terms_set query Set request body to required to reflect the code base (elastic#27188) Update Docker docs for 6.0.0-rc2 (elastic#27166) Add version 6.0.0 Docs: restore now fails if it encounters incompatible settings (elastic#26933) Convert index blocks to cluster block exceptions (elastic#27050) [DOCS] Link remote info API in Cross Cluster Search docs page Fix Laplace scorer to multiply by alpha (and not add) (elastic#27125) [DOCS] Clarify migrate guide and search request validation Raise IllegalArgumentException if query validation failed (elastic#26811) prevent duplicate fields when mixing parent and root nested includes (elastic#27072) TopHitsAggregator must propagate calls to `setScorer`. (elastic#27138)
* master: Lazy initialize checkpoint tracker bit sets Remove checkpoint tracker bit sets setting Fix stable BWC branch detection logic Fix logic detecting unreleased versions Enhances exists queries to reduce need for `_field_names` (#26930) Added new terms_set query Set request body to required to reflect the code base (#27188) Update Docker docs for 6.0.0-rc2 (#27166) Add version 6.0.0 Docs: restore now fails if it encounters incompatible settings (#26933) Convert index blocks to cluster block exceptions (#27050) [DOCS] Link remote info API in Cross Cluster Search docs page Fix Laplace scorer to multiply by alpha (and not add) (#27125) [DOCS] Clarify migrate guide and search request validation Raise IllegalArgumentException if query validation failed (#26811) prevent duplicate fields when mixing parent and root nested includes (#27072) TopHitsAggregator must propagate calls to `setScorer`. (#27138)
* 6.x: Lazy initialize checkpoint tracker bit sets Remove checkpoint tracker bit sets setting Tests: Fix FullClusterRestartIT.testSnapshotRestore test failing in 6.x (#27218) Fix FullClusterRestartIT using lenient booleans with 6.0 Fix stable BWC branch detection logic Add version 6.0.0 Fix logic detecting unreleased versions Skips exists query tests on unsupported versions Enhances exists queries to reduce need for `_field_names` (#26930) Added new terms_set query Set request body to required to reflect the code base (#27188) Update Docker docs for 6.0.0-rc2 (#27166) Docs: restore now fails if it encounters incompatible settings (#26933) Convert index blocks to cluster block exceptions (#27050) [DOCS] Link remote info API in Cross Cluster Search docs page prevent duplicate fields when mixing parent and root nested includes (#27072) TopHitsAggregator must propagate calls to `setScorer`. (#27138)
Fixes #26990 (I think :))
Fixed this by traversing the tree of
Nestedinstances in theBuilderforRootObjectMappersince it seemed to be the one place before instantiating the mappers where I can mutate builders and have the full tree ofNestedavailable to me.So my logic was:
=> Set all
include_in_roottofalse, that are already covered by transitive chains ofinclude_in_parent == true