Skip to content

Conversation

@jimczi
Copy link
Contributor

@jimczi jimczi commented Dec 21, 2017

This change fixes the deferring collector when it is executed in a global context
with a sub collector thats requires to access scores (e.g. top_hits aggregation).
The deferring collector replays the best buckets for each document and re-executes the original query
if scores are needed. When executed in a global context, the query to replay is a simple match_all
query and not the original query.

Closes #22321
Closes #27928

This change fixes the deferring collector when it is executed in a global context
with a sub collector thats requires to access scores (e.g. top_hits aggregation).
The deferring collector replays the best buckets for each document and re-executes the original query
if scores are needed. When executed in a global context, the query to replay is a simple match_all
 query and not the original query.

Closes elastic#22321
Closes elastic#27928
@jpountz
Copy link
Contributor

jpountz commented Dec 21, 2017

We might need better validation too (I'm happy if this is a follow-up). It feels buggy to me to use scores under a global aggregation.

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.

Thx @jimczi! LGTM

@jimczi jimczi merged commit cb783bc into elastic:master Jan 5, 2018
@jimczi jimczi deleted the bug/global_agg_with_scores branch January 5, 2018 10:41
jimczi added a commit that referenced this pull request Jan 5, 2018
* Fix global aggregation that requires breadth first and scores

This change fixes the deferring collector when it is executed in a global context
with a sub collector thats requires to access scores (e.g. top_hits aggregation).
The deferring collector replays the best buckets for each document and re-executes the original query
if scores are needed. When executed in a global context, the query to replay is a simple match_all
 query and not the original query.

Closes #22321
Closes #27928
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Jan 5, 2018
* master:
  test: replaced try-catch statements with expectThrows(...)
  Add getWarmer and getTranslog method to NodeIndicesStats (elastic#28092)
  fix doc mistake
  Added ASN support for Ingest GeoIP plugin.
  Fix global aggregation that requires breadth first and scores (elastic#27942)
  Introduce Gradle wrapper
  Ignore GIT_COMMIT when calculating commit hash
  Re-enable bwc tests after elastic#27881 was backported
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Jan 6, 2018
* master: (25 commits)
  Remove Gradle cheatsheet
  Fix reproduction info to point to Gradle wrapper
  Update platforms tests to use Gradle wrapper
  Update testing docs to reflect Gradle wrapper
  Painless: Modify Loader to Load Classes Directly from Definition (elastic#28088)
  Update contributing docs to use the Gradle wrapper
  Create nio-transport plugin for NioTransport (elastic#27949)
  test: replaced try-catch statements with expectThrows(...)
  Add getWarmer and getTranslog method to NodeIndicesStats (elastic#28092)
  fix doc mistake
  Added ASN support for Ingest GeoIP plugin.
  Fix global aggregation that requires breadth first and scores (elastic#27942)
  Introduce Gradle wrapper
  Ignore GIT_COMMIT when calculating commit hash
  Re-enable bwc tests after elastic#27881 was backported
  Set the elasticsearch-nio codebase for tests (elastic#28067)
  Bump compat version for local depdendent test to 6.2.0
  Pass `java.locale.providers=COMPAT` to Java 9 onwards (elastic#28080)
  Allow shrinking of indices from a previous major (elastic#28076)
  Remove deprecated exceptions (elastic#28059)
  ...
jasontedor added a commit that referenced this pull request Jan 6, 2018
* master:
  Remove out-of-date projectile file
  Remove Gradle cheatsheet
  Fix reproduction info to point to Gradle wrapper
  Update platforms tests to use Gradle wrapper
  Update testing docs to reflect Gradle wrapper
  Painless: Modify Loader to Load Classes Directly from Definition (#28088)
  Update contributing docs to use the Gradle wrapper
  Create nio-transport plugin for NioTransport (#27949)
  test: replaced try-catch statements with expectThrows(...)
  Add getWarmer and getTranslog method to NodeIndicesStats (#28092)
  fix doc mistake
  Added ASN support for Ingest GeoIP plugin.
  Fix global aggregation that requires breadth first and scores (#27942)
jasontedor added a commit that referenced this pull request Jan 6, 2018
* 6.x:
  Remove out-of-date projectile file
  Remove Gradle cheatsheet
  Fix reproduction info to point to Gradle wrapper
  Update platforms tests to use Gradle wrapper
  Update testing docs to reflect Gradle wrapper
  Update contributing docs to use the Gradle wrapper
  test: replaced try-catch statements with expectThrows(...)
  Add getWarmer and getTranslog method to NodeIndicesStats (#28092)
  fix doc mistake
  Added ASN support for Ingest GeoIP plugin.
  Fix global aggregation that requires breadth first and scores (#27942)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

4 participants