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.

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

Drop gap_policy support for bucket_sort #37386

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

liketic
Copy link
Contributor

@liketic liketic commented Jan 12, 2019

Drop gap_policy support for agg bucket_sort.

Closes #36487

@colings86 colings86 added the :Analytics/Aggregations Aggregations label Jan 14, 2019
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-analytics-geo

@polyfractal
Copy link
Contributor

Thanks @liketic!

After you opened the PR I realized there are some backwards compatibility concerns which I completely didn't think about in the original issue. I think we'll need to introduce a new flag (allow_gap_policy: true/false) to allow users to opt out of the gap_policy parameter, so that there is a migration path. Even though this is fixing a bug, it's introducing a breaking change which I didn't think about originally. Sorry :(

I think we'll need to do this:

  • Introduce new allow_gap_policy flag in 6.7. Default to allow_gap_policy: true so that behavior doesn't change. Log a deprecation warning so the user knows it is going away.
  • In 7.0, change default to allow_gap_policy: false, which throws an exception if the user attempts to use gap_policy on the bucket_sort agg. The user can change the flag to opt into the 6.x behavior if they want.
  • In 8.0 we'll remove both allow_gap_policy and gap_policy parameters from bucket_sort

Which makes the change a lot more complicated than before. I understand if you don't want to deal with this, since the original issue looked much simpler and I marked it as a good adopt-me issue. Let me know if you want to continue, or we can close this PR and someone from the ES team can pick it back up.

Thanks, and sorry for the confusion!

@rjernst rjernst added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label May 4, 2020
@elasticsearchmachine elasticsearchmachine changed the base branch from master to main July 22, 2022 23:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Analytics/Aggregations Aggregations Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BucketSort should not support gap_policy
5 participants