refactor: remove arrow-ord dependency in arrow-cast#8716
Open
Weijun-H wants to merge 4 commits intoapache:mainfrom
Open
refactor: remove arrow-ord dependency in arrow-cast#8716Weijun-H wants to merge 4 commits intoapache:mainfrom
arrow-ord dependency in arrow-cast#8716Weijun-H wants to merge 4 commits intoapache:mainfrom
Conversation
Weijun-H
changed the title
arrow_ordarrow-ord dependency in arrow-cast
Contributor
|
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
Contributor
|
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(); |
Contributor
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
arrow-cast/src/cast/run_array.rs
Outdated
| @@ -134,16 +134,8 @@ pub(crate) fn cast_to_run_end_encoded<K: RunEndIndexType>( | |||
| )); | |||
| } | |||
|
|
|||
| // Partition the array to identify runs of consecutive equal values | |||
| let partitions = partition(&[Arc::clone(cast_array)])?; | |||
| let mut run_ends = Vec::new(); | |||
Contributor
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)
Contributor
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?
Contributor
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)
Contributor
There was a problem hiding this comment.
Pushed your suggestion as a PR here: #8716, maybe you can run the benchmark on that too? 😇
Contributor
|
🤖 |
Contributor
|
🤖: Benchmark completed Details
|
Contributor
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
Member
Author
|
FYI @vegarsti , @alamb After several rounds of optimization, the current version delivers significant improvements over the previous one.
|
Contributor
|
🤖 |
Contributor
|
🤖: Benchmark completed Details
|
alamb
pushed a commit
that referenced
this pull request
Oct 30, 2025
Related to #8707. Inspired by #8716 (comment), a follow up improvement to #8589: We already know what the length of the two vectors will be, so we can create them with that capacity.
Weijun-H
force-pushed
the
8708-remove-arrow-ord-in-arrow-cast
branch
from
f9fc4fe to
4fd7761
Compare
Jefffrey
reviewed
Feb 6, 2026
Contributor
Jefffrey
left a comment
Jefffrey
left a comment
There was a problem hiding this comment.
Are we still interested in getting this through? Could we run bot benchmarks again (I don't think I have the permission)
| match array.data_type() { | ||
| Null => (vec![array.len()], vec![0]), | ||
| Boolean => runs_for_boolean(array.as_boolean()), | ||
| Int8 => runs_for_primitive(array.as_primitive::<Int8Type>()), |
Contributor
There was a problem hiding this comment.
Could probably use downcast_primitive_array to cut down some of the boilerplate here?
| ) | ||
| } | ||
|
|
||
| fn runs_for_string_view(array: &StringViewArray) -> (Vec<usize>, Vec<usize>) { |
Contributor
There was a problem hiding this comment.
Probably remove these thin wrappers since we have a catch-all for non-specialized types in the big match above?
| finalize_runs(run_boundaries, len) | ||
| } | ||
|
|
||
| fn runs_for_string_i32(array: &GenericStringArray<i32>) -> (Vec<usize>, Vec<usize>) { |
Contributor
There was a problem hiding this comment.
fn runs_for_string<O: OffsetSizeTrait>(array: &GenericStringArray<O>) -> (Vec<usize>, Vec<usize>) {
let mut to_usize = |v: O| v.as_usize();
runs_for_binary_like(
array.len(),
array.null_count(),
array.value_offsets(),
array.value_data(),
|idx| array.is_valid(idx),
&mut to_usize,
)
}Nice deduplication
Contributor
I added you here: alamb/datafusion-benchmarking@1f1e8b2 (though beware the benchmark machine is non ideal in that it is a shared VM and thus is prone to workload variations) |
Contributor
|
run benchmark cast_kernels |
1 similar comment
Contributor
|
run benchmark cast_kernels |
|
🤖 |
Contributor
It seems to me like a good idea in theory -- I just didnt' have the time to follow it through and complete a review 😢 |
|
🤖: Benchmark completed Details
|
|
🤖 |
|
🤖: Benchmark completed Details
|
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
5 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