arrow-select: implement specialized interleave_list#8953
Merged
alamb merged 1 commit intoapache:mainfrom Dec 13, 2025
Merged
Conversation
Contributor
Author
This was referenced Dec 5, 2025
Contributor
|
run benchmark interleave_kernels |
|
🤖 |
|
🤖: Benchmark completed Details
|
Contributor
|
I noticed there were no benchmarks for interleaving ListArrays, so I made a PR to add some |
alamb
reviewed
Dec 10, 2025
Contributor
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
Thank you @asubiotto -- the code looks good to me, but I don't think we should merge this in without some results showing it make things faster
I made a PR to add some benchmark coverage -- once that is merged i can test this pR again.
I also verified coverage using
cargo llvm-cov --html -p arrow-selectIt looks like we don't have coverage for interleaving LargeLists
However as this is the same code for normal lists I think it is ok (though it would be nice to add more coverage)
asubiotto
force-pushed
the
asubiotto/interleavelist
branch
from
36392c1 to
1281cf3
Compare
Contributor
Author
Added coverage for large lists by making the existing test generic over the offset type and running it for both list and largelist. Thanks for the review! This is ready for another look. |
Dandandan
reviewed
Dec 11, 2025
asubiotto
force-pushed
the
asubiotto/interleavelist
branch
from
1281cf3 to
a291eec
Compare
Dandandan
pushed a commit
that referenced
this pull request
Dec 11, 2025
# Which issue does this PR close? - Part of #8953 # Rationale for this change While reviewing #8953 from @asubiotto I noticed there was no benchmark for interleave with ListArray. Let's add some so we can evaluate the performance impact of ttps://github.com//pull/8953 and future changes. # What changes are included in this PR? Add benchmark for list interleaving # Are these changes tested? I ran the bechmarks manually ```shell cargo bench --bench interleave_kernels -- list ``` # Are there any user-facing changes? No
Contributor
|
run benchmark interleave_kernels |
|
🤖 |
|
🤖: Benchmark completed Details
|
Contributor
|
(I think we need to merge/rebase this branch with the latest main so it also contains the list benchmarks) |
asubiotto
force-pushed
the
asubiotto/interleavelist
branch
from
a291eec to
be7904c
Compare
Contributor
Author
|
Triggered an automatic rebase with Github. |
Contributor
|
run benchmark interleave_kernels |
|
🤖 |
|
🤖: Benchmark completed Details
|
Contributor
Looks good to me 👍 |
alamb
approved these changes
Dec 11, 2025
Contributor
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
Thank you @asubiotto -- this looks great to me ❤️
asubiotto
force-pushed
the
asubiotto/interleavelist
branch
from
be7904c to
a1674ad
Compare
Contributor
|
run benchmark interleave_kernels |
Dandandan
reviewed
Dec 11, 2025
|
🤖 |
|
🤖: Benchmark completed Details
|
Previously, List and LargeList would fall through to the interleave_fallback match arm, which is inefficient. This commit implements interleave_list, which interleaves a list's child arrays and rebuilds the offsets buffer. Running it on production tests reduced memory by 80%. Signed-off-by: Alfonso Subiotto Marques <alfonso.subiotto@polarsignals.com>
asubiotto
force-pushed
the
asubiotto/interleavelist
branch
from
a1674ad to
5b441ec
Compare
Contributor
Author
|
Addressed the latest comment and rebased. This should be ready to merge. |
Contributor
|
run benchmark interleave_kernels |
This comment was marked as outdated.
This comment was marked as outdated.
Contributor
|
Thank you for this PR @asubiotto and all the help @Dandandan |
|
🤖 |
|
🤖: 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
4 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.
Previously, List and LargeList would fall through to the interleave_fallback match arm, which is inefficient. This commit implements interleave_list, which interleaves a list's child arrays and rebuilds the offsets buffer. Running it on production tests reduced memory by 80%.
Which issue does this PR close?
interleavefor List/LargeList #8952Rationale for this change
Performance and memory usage when interleaving List/LargeList
Are these changes tested?
This PR does not include tests because interleave tests for Lists already exist
Are there any user-facing changes?
No, purely performance