Repository navigation
perf(arrow-ord): Avoid full index materialization for small-limit lexsorts - #9991
Conversation
eb9a67b to
c3fdb35
Compare
|
@alamb & @etseidl I investigated this integration failure locally since the failure appeared unrelated to the changes in my PR. My PR only modifies
I also checked the local integration corpus and could not find a I also noticed the same |
|
run benchmarks sort_kernel |
I think it was an error in .NET: |
|
🤖 Arrow criterion benchmark running (GKE) | trigger CPU Details (lscpu)Comparing lexsort-small-limit-topk (b7bdb86) to 4b80f0e (merge-base) diff File an issue against this benchmark runner |
|
🤖 Arrow criterion benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usagebase (merge-base)
branch
File an issue against this benchmark runner |
|
@alamb now the errors seem to have gone in the CI checks and all of them pass. Also, for the benchmark, the speed gains are exactly what I expected: >50% gain for limit 10 & >25% gain for limit 100. So could you kindly pls review this PR and approve if everything else seems fine? thnx |
|
run benchmarks sort_kernel |
|
🤖 Arrow criterion benchmark running (GKE) | trigger CPU Details (lscpu)Comparing lexsort-small-limit-topk (ef8f1bf) to da07bce (merge-base) diff File an issue against this benchmark runner |
|
🤖 Arrow criterion benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usagebase (merge-base)
branch
File an issue against this benchmark runner |
Jefffrey
left a comment
There was a problem hiding this comment.
benchmarks show some nice improvements 🚀
i do have some concerns about what this does to our unit tests, as adding this new path at best splits our unit test coverage in half. could we double check the existing coverage and see if we need to modify them to ensure we have sufficient coverage for the non-heap path and the heap path now?
ef8f1bf to
d397853
Compare
d397853 to
b74ad84
Compare
|
Thnx @Jefffrey for the review! I've addressed your comments:
|
|
run benchmarks sort_kernel |
|
🤖 Arrow criterion benchmark running (GKE) | trigger CPU Details (lscpu)Comparing lexsort-small-limit-topk (b74ad84) to fd1c5b3 (merge-base) diff File an issue against this benchmark runner |
|
🤖 Arrow criterion benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usagebase (merge-base)
branch
File an issue against this benchmark runner |
Which issue does this PR close?
Rationale for this change
In
arrow-ord/src/sort.rs,lexsort_to_indicescurrently materializes row indices for the full input before applying the requested lexsort limit.For example, with:
the current implementation still allocates and initializes indices for all 4096 rows:
even though only the first 10 sorted indices are returned.
This PR reduces allocation and sorting work for small-limit lexsorts by avoiding full index materialization when the requested limit is a small fraction(limit < 1/10 th of row count) of the input size.
What changes are included in this PR?
This PR adds a bounded top-k heap path for small-limit lexsorts in
arrow-ord/src/sort.rs.The new path:
The existing partial-sort implementation remains unchanged for larger limits.
To support the bounded heap implementation, this PR also adds:
lexsort_topk_fixedlexsort_topksift_up_worst_heapsift_down_worst_heapAre these changes tested?
Yes.
Existing tests pass:
My local Benchmark results from:
show improvements for small-limit lexsorts such as limits 10, 100.
Are there any user-facing changes?
No.