-
Notifications
You must be signed in to change notification settings - Fork 24.9k
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 composite agg sort bug #53296
Fix composite agg sort bug #53296
Conversation
When an composite aggregation is run against an index with a sort that *starts* with the "source" fields from the composite but has additional fields it'd blow up in while trying to decide if it could use the sort. This changes it to decide that it *can* use the sort. Closes elastic#52480
Pinging @elastic/es-analytics-geo (:Analytics/Aggregations) |
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.
Good catch @nik9000 . I left one comment regarding the correctness of the fix.
@@ -202,7 +202,8 @@ private Sort buildIndexSortPrefix(LeafReaderContext context) throws IOException | |||
return null; | |||
} | |||
List<SortField> sortFields = new ArrayList<>(); | |||
for (int i = 0; i < indexSort.getSort().length; i++) { | |||
int max = Math.min(indexSort.getSort().length, sourceConfigs.length); |
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.
You can still hit the bug if the source configs is smaller than the index sort ? Since we're looking for the biggest prefix of the index sort maybe we could leave the for loop as it is but add a check that breaks the loop if the source configs length is reached:
if (i >= sourceConfigs.length) {
break;
}
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 think the way I have it covers both cases, right?
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.
oh right, I read the variable named max
and thought that you were extracting the max of the length :(
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.
LGTM, sorry, I checked the logic too quickly
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 a commit that randomly shortens the index sort in addition to my test change in the original commit that randomly lengthens the sort. This should be making sure we're ok with sort that don't line up either way.
This reverts commit 4cd354f.
Woops, ships passing in the night. I'll drop that commit I just mentioned. |
When an composite aggregation is run against an index with a sort that *starts* with the "source" fields from the composite but has additional fields it'd blow up in while trying to decide if it could use the sort. This changes it to decide that it *can* use the sort. Closes elastic#52480
Thankss @jimczi ! |
When an composite aggregation is run against an index with a sort that *starts* with the "source" fields from the composite but has additional fields it'd blow up in while trying to decide if it could use the sort. This changes it to decide that it *can* use the sort. Closes #52480
Now that elastic#53296 is backported to 7.x we can run its tests against 7.7.0.
Now that #53296 is backported to 7.x we can run its tests against 7.7.0.
When an composite aggregation is run against an index with a sort that
starts with the "source" fields from the composite but has additional
fields it'd blow up in while trying to decide if it could use the sort.
This changes it to decide that it can use the sort.
Closes #52480