-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Use SortPreservingMerge for in memory sort #5851
Conversation
tracking_metrics, | ||
))) | ||
if buffered_batches.len() < 2 { | ||
let batches: Vec<_> = buffered_batches.drain(..).collect(); |
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.
It is worth highlighting why this is important to the benchmarks, as there was much discussion of this on #5230
The way the "preserve partitioning" benchmarks are setup is they yield a single RecordBatch per partition, they're effectively a special case where the in_mem_partial_sort is a no-op.
Regressions look very significant in some cases, let's see if the proposed next step mitigates that. |
#5854 contains an extremely hacky POC of special casing the cursor implementation, and eliminates the regression for "sort i64". |
Using the benchmarks in #5881
|
Closing in favour of #6163 |
Which issue does this PR close?
Closes #5879
Rationale for this change
Investigating #5292 and thought I would try to break it into smaller pieces to make it easier to see what is going on. In particular this PR just switches
ExternalSorter
to usingSortPreservingMerge
.This results in better performance, apart from in the case of a single sort column. This is not hugely surprising, as lexsort_to_indices has optimised kernels for when sorting by a single column.
I think the next step is to find a way to make single-column sort expressions perform better in SortPreservingMerge, likely by providing specialized implementations of
SortPreservingMergeStream
's inner cursor loop. I intend to have a play around with doing this.What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?