-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Orasort based sort kernels #9300
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
base: main
Are you sure you want to change the base?
Conversation
|
Looks interesting - I am wondering if we can't do the same in arrow-rs without relying on a new dependency? |
|
|
||
| impl<'a> KeyAccessor for FixedSizeBinaryAccessor<'a> { | ||
| #[inline(always)] | ||
| fn get_key(&self, index: usize) -> &[u8] { |
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.
Should be marked unsafe
| let mut valids: Vec<(u32, u32, u64)> = value_indices | ||
| .into_iter() | ||
| .map(|idx| unsafe { | ||
| // Build (index, 8-byte prefix) tuples for prefix-accelerated comparison sort |
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.
What is the improvement without orasort on this?
|
run benchmark sort_kernels |
This comment was marked as outdated.
This comment was marked as outdated.
|
🤖 |
|
Benchmark script failed with exit code 101. Last 10 lines of output: Click to expand |
|
run benchmark sort_kernel |
|
🤖 |
|
🤖: Benchmark completed Details
|
|
run benchmark sort_kernel |
|
🤖 |
|
🤖: Benchmark completed Details
|
|
Seems not to reproduce on the VM 🤔 perhaps machine-dependent |
|
I don't understand the benchmarks, can someone explain it to me? |
It seems to me like this PR shows improvements in the sort kernel performance based on the benchmarks |
|
Ah I see the changes are also in the sort string view kernels, not only in the string kernels and they show a modest improvement. Still I wonder, we should be able to improve the performance without resorting to the library? |
|
@vertexclique looks interesting. |
Which issue does this PR close?
Rationale for this change
Orasort is spliced based on prefix and uses radix sort in spliced chunks.
What changes are included in this PR?
Orasort sorting inclusion, adapting to prefix splices for array buffers.
Are these changes tested?
Yes, tests are already covering it.
In addition to that extra benchmarks are added to demonstrate the gain.
Are there any user-facing changes?
No.
Bench Results (main vs this branch)
Orasort core implementation: https://github.com/psila-ai/orasort
Perf defaults of the Orasort: https://github.com/psila-ai/orasort?tab=readme-ov-file#performance