Prevent duplicate fields when mixing parent and root nested includes #27072

original-brownbear · 2017-10-21T16:43:01Z

Fixes #26990 (I think :))

Fixed this by traversing the tree of Nested instances in the Builder for RootObjectMapper since it seemed to be the one place before instantiating the mappers where I can mutate builders and have the full tree of Nested available to me.
So my logic was:

The duplicate fields arise from a child nested by more than one level, that is directly included in the root and also transitively included via a chain of parent(s).
=> Set all include_in_root to false, that are already covered by transitive chains of include_in_parent == true

jpountz

I think you found the right place to fix the issue, however I spotted a bug (see comments).

jpountz · 2017-10-27T07:08:33Z

core/src/main/java/org/elasticsearch/index/mapper/RootObjectMapper.java

+                    Nested nested = child.nested;
+                    boolean included;
+                    if (parentIncluded) {
+                        included = nested.isNested() && nested.isIncludeInParent();


I don't think it is correct: included should also true if nested.isIncludeInRoot is true? Otherwise we might recurse on a wrong value.

jpountz · 2017-10-27T07:09:11Z

core/src/main/java/org/elasticsearch/index/mapper/RootObjectMapper.java

+                    boolean included;
+                    if (parentIncluded) {
+                        included = nested.isNested() && nested.isIncludeInParent();
+                        if (included) {


However I agree that we should test for if (nested.isNested() && nested.isIncludeInParent()) here.

original-brownbear · 2017-10-27T08:39:56Z

@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

I just left a naming suggestion, but the change looks good to me!

jpountz · 2017-10-27T08:55:51Z

core/src/main/java/org/elasticsearch/index/mapper/RootObjectMapper.java

+                    ObjectMapper.Builder child = (ObjectMapper.Builder) mapper;
+                    Nested nested = child.nested;
+                    boolean isNested = nested.isNested();
+                    boolean includedInParent = parentIncluded && isNested && nested.isIncludeInParent();


should we rename to something like includeInRootViaParent to make it clearer what this is about?

original-brownbear · 2017-10-27T09:00:04Z

@jpountz I agree renamed that variable :)

original-brownbear · 2017-10-27T10:04:05Z

@jpountz build failure looks unrelated, should we retrigger Jenkins?

jpountz · 2017-10-27T10:21:12Z

@elasticmachine please test it

original-brownbear · 2017-10-27T10:44:19Z

@jpountz hmm no seems, there's an actual issue now. Failed the same way for me locally, sorry. Will fix shortly.

…ested includes

…sted includes (follow up)

original-brownbear · 2017-10-27T11:07:15Z

rebase against master fixed it locally, let's see what Jenkins says

original-brownbear · 2017-10-27T12:16:13Z

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.

original-brownbear · 2017-10-27T12:16:24Z

@elasticmachine please test it

original-brownbear · 2017-10-27T13:36:57Z

@jpountz instability seems to be handled here #27146 => good to squash + merge right? :)

jpountz · 2017-10-27T13:41:31Z

Sure, I'll merge. Thanks @original-brownbear !

original-brownbear · 2017-10-27T13:48:11Z

@jpountz np + thanks for the review :)

original-brownbear · 2017-10-30T15:20:59Z

@jpountz ping :)

jpountz · 2017-10-31T09:01:44Z

Thanks @original-brownbear !

original-brownbear · 2017-10-31T09:07:04Z

np @jpountz :)

…27072) Closes #26990

* 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)

original-brownbear mentioned this pull request Oct 24, 2017

More than one level of nesting with include_in_parent + include_in_root can lead to duplicate fields #26990

Closed

jpountz self-requested a review October 27, 2017 06:31

jpountz requested changes Oct 27, 2017

View reviewed changes

jpountz approved these changes Oct 27, 2017

View reviewed changes

original-brownbear added 3 commits October 27, 2017 13:05

elastic#26990 prevent duplicate fields when mixing parent and root n…

558ea97

…ested includes

elastic#26990 prevent duplicate fields when mixing parent and root ne…

335a237

…sted includes (follow up)

elastic#26990 prevent duplicate fields when mixing parent and root ne…

a043337

…sted includes (follow up)

original-brownbear force-pushed the 26990 branch from 55fe17c to a043337 Compare October 27, 2017 11:06

jpountz merged commit a4c159e into elastic:master Oct 31, 2017

original-brownbear deleted the 26990 branch October 31, 2017 09:06

jpountz added v6.1.0 v7.0.0 labels Oct 31, 2017

jpountz pushed a commit that referenced this pull request Oct 31, 2017

prevent duplicate fields when mixing parent and root nested includes (#…

1c23877

…27072) Closes #26990

clintongormley changed the title ~~#26990 prevent duplicate fields when mixing parent and root nested includes~~ Prevent duplicate fields when mixing parent and root nested includes Nov 6, 2017

clintongormley added :Nested Docs >bug labels Nov 6, 2017

clintongormley added :Search/Search Search-related issues that do not fall into other categories and removed :Nested Docs labels Feb 14, 2018

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prevent duplicate fields when mixing parent and root nested includes #27072

Prevent duplicate fields when mixing parent and root nested includes #27072

original-brownbear commented Oct 21, 2017

jpountz left a comment

jpountz Oct 27, 2017

jpountz Oct 27, 2017

original-brownbear commented Oct 27, 2017

jpountz left a comment

jpountz Oct 27, 2017

original-brownbear commented Oct 27, 2017 •

edited

Loading

original-brownbear commented Oct 27, 2017

jpountz commented Oct 27, 2017

original-brownbear commented Oct 27, 2017

original-brownbear commented Oct 27, 2017

original-brownbear commented Oct 27, 2017

original-brownbear commented Oct 27, 2017

original-brownbear commented Oct 27, 2017

jpountz commented Oct 27, 2017

original-brownbear commented Oct 27, 2017

original-brownbear commented Oct 30, 2017

jpountz commented Oct 31, 2017

original-brownbear commented Oct 31, 2017

Prevent duplicate fields when mixing parent and root nested includes #27072

Prevent duplicate fields when mixing parent and root nested includes #27072

Conversation

original-brownbear commented Oct 21, 2017

jpountz left a comment

Choose a reason for hiding this comment

jpountz Oct 27, 2017

Choose a reason for hiding this comment

jpountz Oct 27, 2017

Choose a reason for hiding this comment

original-brownbear commented Oct 27, 2017

jpountz left a comment

Choose a reason for hiding this comment

jpountz Oct 27, 2017

Choose a reason for hiding this comment

original-brownbear commented Oct 27, 2017 • edited Loading

original-brownbear commented Oct 27, 2017

jpountz commented Oct 27, 2017

original-brownbear commented Oct 27, 2017

original-brownbear commented Oct 27, 2017

original-brownbear commented Oct 27, 2017

original-brownbear commented Oct 27, 2017

original-brownbear commented Oct 27, 2017

jpountz commented Oct 27, 2017

original-brownbear commented Oct 27, 2017

original-brownbear commented Oct 30, 2017

jpountz commented Oct 31, 2017

original-brownbear commented Oct 31, 2017

original-brownbear commented Oct 27, 2017 •

edited

Loading