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
SQL: Fix wrong results when sorting on aggregate #43154
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -28,8 +28,8 @@ class CompositeAggsRowSet extends ResultRowSet<BucketExtractor> { | |
private final int size; | ||
private int row = 0; | ||
|
||
CompositeAggsRowSet(List<BucketExtractor> exts, BitSet mask, SearchResponse response, int limit, byte[] next, | ||
boolean includeFrozen, String... indices) { | ||
CompositeAggsRowSet(List<BucketExtractor> exts, BitSet mask, SearchResponse response, | ||
int limit, byte[] next, boolean includeFrozen, String... indices) { | ||
super(exts, mask); | ||
|
||
CompositeAggregation composite = CompositeAggregationCursor.getComposite(response); | ||
|
@@ -40,18 +40,22 @@ class CompositeAggsRowSet extends ResultRowSet<BucketExtractor> { | |
} | ||
|
||
// page size | ||
size = limit < 0 ? buckets.size() : Math.min(buckets.size(), limit); | ||
size = limit == -1 ? buckets.size() : Math.min(buckets.size(), limit); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why the original code wasn't good? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It will also work, but we introduced the |
||
|
||
if (next == null) { | ||
cursor = Cursor.EMPTY; | ||
} else { | ||
// compute remaining limit | ||
int remainingLimit = limit - size; | ||
// Compute remaining limit | ||
|
||
// If the limit is -1 then we have a local sorting (sort on aggregate function) that requires all the buckets | ||
// to be processed so we stop only when all data is exhausted. | ||
int remainingLimit = (limit == -1) ? limit : ((limit - size) >= 0 ? (limit - size) : 0); | ||
|
||
// if the computed limit is zero, or the size is zero it means either there's nothing left or the limit has been reached | ||
// note that a composite agg might be valid but return zero groups (since these can be filtered with HAVING/bucket selector) | ||
// however the Querier takes care of that and keeps making requests until either the query is invalid or at least one response | ||
// is returned | ||
if (next == null || size == 0 || remainingLimit == 0) { | ||
// is returned. | ||
if (size == 0 || remainingLimit == 0) { | ||
cursor = Cursor.EMPTY; | ||
} else { | ||
cursor = new CompositeAggregationCursor(next, exts, mask, remainingLimit, includeFrozen, indices); | ||
|
@@ -92,4 +96,4 @@ public int size() { | |
public Cursor nextPageCursor() { | ||
return cursor; | ||
} | ||
} | ||
} |
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.
Unrelated to this pr but I don't think we should treat ordered GROUP_BY differently than natural GROUP_BY (sorted by keys). We should use the same hard limit for both, the cost of an ordered GROUP_BY depends greatly on the cardinality of the groups and not so much on the size of the priority queue (assuming it remains reasonable ;)).
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 don't think I agree here. When we don't have an order by on aggregate then we don't need to keep this extra buffer in memory (PriorityQueue) in the SQL module, instead we have a cursor from ES to paginate the results, thus we don't need to apply a hard limit. Or maybe I'm missing something?
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.
The memory to keep the best buckets in the PriorityQueue should be identical to the one that is used in ES when we compute the top hits so it's only a question on where this memory is held. I understand that the priority queue in the client can increase the memory but it shouldn't make a big difference if the size is 512 or 1000 or even 10,000. Most of the time will be spent on paginating the buckets and a limit of 1 or 512 shouldn't make any difference.
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.
Thx, opened an issue to track it: #43168
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.
My thinking here is the other way around, we should increase the hard limit of the ordered group_by to match the one that we set to "normal" group_by.
10,000
seems a reasonable value IMO.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.
+1, I misunderstood you here. Changed the issue. Thx!