-
Notifications
You must be signed in to change notification settings - Fork 24.4k
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
Fix merging of terms aggregation with compound order #64469
Conversation
This change fixes a bug introduced in elastic#61779 that uses a compound order to compare buckets when merging. The bug is triggered when the compound order uses a primary sort ordered by key (asc or desc). This commit ensures that we always extract the primary sort when comparing keys during merging. The PR is marked as no-issue since the bug has not been released in any official version.
Pinging @elastic/es-analytics-geo (:Analytics/Aggregations) |
|
||
@Override | ||
protected InternalTerms<?, ?> createTestInstance(String name, | ||
Map<String, Object> metadata, | ||
InternalAggregations aggregations, | ||
boolean showTermDocCountError, | ||
long docCountError) { | ||
generateRandomDict(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can dict
be a local variable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I want to ensure that there are some equal keys in the instances that we try to merge so I build a single dictionary for the entire test suite. It could be easier if the class was not used by the AggregationTests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you override randomResultsToReduce
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
++, I pushed 3777760
This change fixes a bug introduced in #61779 that uses a compound order to compare buckets when merging. The bug is triggered when the compound order uses a primary sort ordered by key (asc or desc). This commit ensures that we always extract the primary sort when comparing keys during merging. The PR is marked as no-issue since the bug has not been released in any official version.
This change fixes a bug introduced in #61779 that uses a compound order to compare buckets when merging. The bug is triggered when the compound order uses a primary sort ordered by key (asc or desc). This commit ensures that we always extract the primary sort when comparing keys during merging. The PR is marked as no-issue since the bug has not been released in any official version.
This change fixes a bug introduced in #61779 that uses a compound order to
compare buckets when merging. The bug is triggered when the compound order
uses a primary sort ordered by key (asc or desc).
This commit ensures that we always extract the primary sort when comparing keys
during merging.
The PR is marked as no-issue since the bug has not been released in any official version.