Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix multi level nested sort #32204

Merged
merged 5 commits into from
Jul 20, 2018
Merged

Fix multi level nested sort #32204

merged 5 commits into from
Jul 20, 2018

Conversation

jimczi
Copy link
Contributor

@jimczi jimczi commented Jul 19, 2018

Multi level nested sort produces an invalid parent filter that contains a required clause at the wrong level. This can lead to document picking values from other documents. This change fixes this bug by removing the child query entirely from the parent filter.
Multi level nested sort now creates a single parent query that match the level of documents that needs to be sorted and a child query that contains the nested filters at the deepest level (where the sort field exists). This change also ensures that we cache only nested type filters in the bitset filter cache.(#31783)

Supersedes #31776
Closes #31554
Closes #32130
Closes #31783

dbevacqua and others added 3 commits July 19, 2018 17:44
@dbevacqua @jimczi
Fix sort values coming from unrelated documents in certain multi-leve…
edac443 
…l sorts elastic#31554
@martijnvg @jimczi
added a better unit test
cd69c52
@jimczi
Simplify parent filter for nested sort
8ecc35b 
The parent filter for nested sort should always match **all** parents regardless
of the child queries. It is used to find the boundaries of a single parent and we use
the child query to match all the filters set in the nested tree so there is no need to
repeat the nested filters.
With this change we ensure that we build bitset filters
only to find the root docs (or the docs at the level where the sort applies) that can be reused
among queries.

Closes elastic#31783
@jimczi jimczi added >bug :Search/Search Search-related issues that do not fall into other categories v7.0.0 v6.4.0 v6.3.2 labels Jul 19, 2018
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search-aggs

@jimczi jimczi mentioned this pull request Jul 19, 2018
Closed
martijnvg
martijnvg approved these changes Jul 20, 2018
View reviewed changes
Copy link
Member

@martijnvg martijnvg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @jimczi for fixing! LGTM

@jimczi jimczi merged commit 6ed1ad0 into elastic:master Jul 20, 2018
@jimczi jimczi deleted the multi_level_nested_sort branch July 20, 2018 14:55
jimczi added a commit that referenced this pull request Jul 20, 2018
@jimczi @dbevacqua
Fix multi level nested sort (#32204)
9bf6112 
The parent filter for nested sort should always match **all** parents regardless
of the child queries. It is used to find the boundaries of a single parent and we use
the child query to match all the filters set in the nested tree so there is no need to
repeat the nested filters.
With this change we ensure that we build bitset filters
only to find the root docs (or the docs at the level where the sort applies) that can be reused
among queries.

Closes #31554
Closes #32130
Closes #31783

Co-authored-by: Dominic Bevacqua <bev@treatwell.com>
jimczi added a commit that referenced this pull request Jul 20, 2018
@jimczi @dbevacqua
Fix multi level nested sort (#32204)
913c1a9 
The parent filter for nested sort should always match **all** parents regardless
of the child queries. It is used to find the boundaries of a single parent and we use
the child query to match all the filters set in the nested tree so there is no need to
repeat the nested filters.
With this change we ensure that we build bitset filters
only to find the root docs (or the docs at the level where the sort applies) that can be reused
among queries.

Closes #31554
Closes #32130
Closes #31783

Co-authored-by: Dominic Bevacqua <bev@treatwell.com>
martijnvg added a commit that referenced this pull request Jul 21, 2018
@martijnvg
Merge remote-tracking branch 'es/6.x' into ccr-6.x
6663e24 
* es/6.x: (24 commits)
  Fix broken backport
  Switch full-cluster-restart to new style Requests (#32140)
  Fix multi level nested sort (#32204)
  MINOR: Remove unused `IndexDynamicSettings` (#32237) (#32248)
  [Tests] Remove QueryStringQueryBuilderTests#toQuery class assertions (#32236)
  Switch rolling restart to new style Requests (#32147)
  Enhance Parent circuit breaker error message (#32056)
  [ML] Use default request durability for .ml-state index (#32233)
  Enable testing in FIPS140 JVM (#31666) (#32231)
  Remove indices stats timeout from monitoring docs
  TESTS: Check for Netty resource leaks (#31861) (#32225)
  Rename ranking evaluation response section (#32166)
  Dependencies: Upgrade to joda time 2.10 (#32160)
  Backport SSL context names (#30953) to 6.x (#32223)
  Require Gradle 4.9  as minimum version (#32200)
  Detect old trial licenses and mimic behaviour (#32209)
  Painless: Simplify Naming in Lookup Package (#32177)
  add support for write index resolution when creating/updating documents (#31520)
  A replica can be promoted and started in one cluster state update (#32042)
  Rest test - allow for snapshots to take 0 milliseconds
  ...
martijnvg added a commit that referenced this pull request Jul 21, 2018
@martijnvg
Merge remote-tracking branch 'es/master' into ccr
8ed3624 
* es/master: (23 commits)
  Switch full-cluster-restart to new style Requests (#32140)
  [DOCS] Clarified that you must remove X-Pack plugin when upgrading from pre-6.3. (#32016)
  Remove BouncyCastle dependency from runtime (#32193)
  INGEST: Extend KV Processor (#31789) (#32232)
  INGEST: Make a few Processors callable by Painless (#32170)
  Add region ISO code to GeoIP Ingest plugin (#31669)
  [Tests] Remove QueryStringQueryBuilderTests#toQuery class assertions (#32236)
  Make sure that field aliases count towards the total fields limit. (#32222)
  Switch rolling restart to new style Requests (#32147)
  muting failing test for internal auto date histogram to avoid failure before fix is merged
  MINOR: Remove unused `IndexDynamicSettings` (#32237)
  Fix multi level nested sort (#32204)
  Enhance Parent circuit breaker error message (#32056)
  [ML] Use default request durability for .ml-state index (#32233)
  Remove indices stats timeout from monitoring docs
  Rename ranking evaluation response section (#32166)
  Dependencies: Upgrade to joda time 2.10 (#32160)
  Remove aliases resolution limitations when security is enabled (#31952)
  Ensure that field aliases cannot be used in multi-fields. (#32219)
  TESTS: Check for Netty resource leaks (#31861)
  ...
@JulienColin
Copy link

Hello,
I've tested the 6.3.2 release against the #32130 issue, and from what I see this is not fixed. I still run into the same issue when using bulk indexing. Is it possible to re-open the issue ?

Thank you

@JulienColin JulienColin mentioned this pull request Jul 25, 2018
Closed
@jimczi jimczi added v6.3.3 and removed v6.3.2 labels Jul 25, 2018
@jimczi
Copy link
Contributor Author

jimczi commented Jul 25, 2018 •
edited
Loading

@JulienColin the fix is not in the 6.3.2 release, I thought that it would be part of it but the merge was too late. I'll update the release notes and bump the fixed version to 6.3.3. Thanks for checking.

jimczi added a commit that referenced this pull request Jul 25, 2018
@jimczi
Remove invalid entry from 6.3.2 release notes
c0978c8 
#32204 is not part of the 6.3.2 release.
jimczi added a commit that referenced this pull request Jul 25, 2018
@jimczi
Remove invalid entry from 6.3.2 release notes
37b503f 
#32204 is not part of the 6.3.2 release.
@JulienColin
Copy link

Alright @jimczi , indeed I was mislead by the release not. Thank you very much !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@martijnvg martijnvg martijnvg approved these changes

Assignees
No one assigned
Labels
>bug :Search/Search Search-related issues that do not fall into other categories v6.3.3 v6.4.0 v7.0.0-beta1
Projects
None yet
Milestone
No milestone
6 participants
@jimczi @elasticmachine @JulienColin @martijnvg @colings86 @dbevacqua