refactor: remove arrow-ord dependency in arrow-cast
#8716
+502
−12
Conversation
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
Weijun-H
changed the title
arrow_ordarrow-ord dependency in arrow-cast
|
Could you try running the benchmark in this PR #8710 and see what the difference is? I thought |
Weijun-H
force-pushed
the
8708-remove-arrow-ord-in-arrow-cast
branch
from
a6198b8 to
c72af36
Compare
|
I just reviewed the benchmark in and I think it looks good to go. I'll merge it in and then run the benchmarks on this PR |
alamb
reviewed
Oct 27, 2025
| values_indexes.push(0); | ||
| let mut current_data = array.slice(0, 1).to_data(); | ||
| for idx in 1..array.len() { | ||
| let next_data = array.slice(idx, 1).to_data(); |
There was a problem hiding this comment.
I think this is likely to be substantially slower than what partition does, but we can see what the benchmarks show
|
|
||
| // Partition the array to identify runs of consecutive equal values | ||
| let partitions = partition(&[Arc::clone(cast_array)])?; | ||
| let mut run_ends = Vec::new(); |
There was a problem hiding this comment.
I looked briefly at a profile for this function -- I think we could make it substantially faster by reducing allocatiosn with a pre-sized vector here (use partitions.count_ones() to know how many partitions are needed)
There was a problem hiding this comment.
Oh, great idea!
Side note: How did you profile this, using samply (it looks like), cargo build --profile profiling, and ran e.g. a unit test?
There was a problem hiding this comment.
I used Instruments that was part of Mac XCode -- it is pretty sweet as it will do whole system profiling (fire it up and start recording and it gathers the info for all processes)
There was a problem hiding this comment.
Pushed your suggestion as a PR here: #8716, maybe you can run the benchmark on that too? 😇
|
🤖 |
|
🤖: Benchmark completed Details
|
As @vegarsti predicted, this PR appears to be quite a bit slower than using partition |
Weijun-H
force-pushed
the
8708-remove-arrow-ord-in-arrow-cast
branch
from
70b24d1 to
fe208be
Compare
|
FYI @vegarsti , @alamb After several rounds of optimization, the current version delivers significant improvements over the previous one.
|
|
🤖 |
|
🤖: Benchmark completed Details
|
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.
Which issue does this PR close?
Rationale for this change
What changes are included in this PR?
Are these changes tested?
Existing tests cover this change
Are there any user-facing changes?
No