bug/perf: Optimize take for RunEndArrays#10325
Open
Rich-T-kid wants to merge 3 commits into
Open
Conversation
Contributor
Author
Rich-T-kid
changed the title
Rich-T-kid
commented
Jul 11, 2026
Rich-T-kid
left a comment
Rich-T-kid
left a comment
Contributor
Author
There was a problem hiding this comment.
Ill clean this up and push up a revision. Could a maintainer also run the benchmarks to make sure there is a minimal regression in performance.
Comment on lines
+26
to
+29
| //! comparison (e.g. for the run-end-encoded `take` fast path) without taking on | ||
| //! the full ordering kernel suite — which would either create a circular | ||
| //! dependency (`arrow-ord` already depends on `arrow-select`) or force every | ||
| //! downstream user of `arrow-array` to compile the comparator machinery whether |
Contributor
Author
There was a problem hiding this comment.
can remove these comemnts
|
|
||
| #[test] | ||
| fn test_take_run_end_encoded_merges_identical_runs() { | ||
| // https://github.com/apache/arrow-rs/issues/7710 |
Contributor
Author
There was a problem hiding this comment.
we can remove the link to the issue. once this is merged it will be the default behavior
Comment on lines
+2976
to
+2980
| builder.extend( | ||
| ["bob", "bob", "alice", "alice", "bob", "bob", "eve", "eve"] | ||
| .into_iter() | ||
| .map(Some), | ||
| ); |
Contributor
Author
There was a problem hiding this comment.
nit: its more readable to have Some(str) directly
Contributor
|
run benchmarks take_kernels primitive_run_take |
|
🤖 Arrow criterion benchmark running (GKE) | trigger CPU Details (lscpu)Comparing rich-T-kid/combine-overlapping-ree-pt2 (93e9b38) to 970391f (merge-base) diff File an issue against this benchmark runner |
|
🤖 Arrow criterion benchmark running (GKE) | trigger CPU Details (lscpu)Comparing rich-T-kid/combine-overlapping-ree-pt2 (93e9b38) to 970391f (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 |
|
🤖 Arrow criterion benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usagebase (merge-base)
branch
File an issue against this benchmark runner |
Jefffrey
reviewed
Jul 15, 2026
Jefffrey
left a comment
Jefffrey
left a comment
Contributor
There was a problem hiding this comment.
in relation to adding the new arrow-cmp crate, we'll need to:
- add the license + notice symlinks that are present in the other crates, see 0974cff
- add line to release instructions for publishing this crate
arrow-rs/dev/release/README.md
Lines 214 to 240 in a4ac705
- i think between array & select?
Contributor
Author
these results are much better than the initial approach |
Contributor
Author
|
@Jefffrey the latest commit should address all of your comments 🚀 |
Jefffrey
approved these changes
Jul 16, 2026
Jefffrey
left a comment
Jefffrey
left a comment
Contributor
There was a problem hiding this comment.
looks good to me, though would be good to get another set of eyes on this since we're introducing a new crate
Contributor
Author
|
@Weijun-H friendly ping |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The Diff (+2,037 -1,839) makes this PR appear much bigger than it is. this is mostly moving code around & test. most of the logic is under 100 LOC
Which issue does this PR close?
Rationale for this change
for a logical representation of an ree
when the result should be
runs: [4], values: [1]both answers are correct but ree's should be as compact as possible.
see #7710
What changes are included in this PR?
take()onRunEndEncodedarrays now compares values instead of physical indices when deciding run boundaries, producing a more compact run-end representation and fixing cases where identical values across different runs were not merged.arrow-cmp, a minimal crate that extractsmake_comparator/DynComparatorfromarrow-ordsoarrow-selectcan use slot-wise comparison without a circular dependency. Most of the line diff is code moving (+1,889,-1834), not new logicarrow-ordre-exports fromarrow-cmpso its public API is unchanged.Are these changes tested?
yes, I included three test to assert the compaction behavior we expect from a Run-end array.
Are there any user-facing changes?
take()onRunEndEncodedarrays behaves differently. the output may have fewer runs than before. Previously runs were only merged when they hit the same physical index; now runs with equal values are also merged across different physical indices. Any code asserting on the exact run-end structure of take output could break.New
arrow-cmpcrate is published. Users can depend on it directly to getmake_comparator/DynComparatorwithout pulling in all ofarrow-ord.arrow-ordstill re-exports both so its API is unchanged.