Performance: ListViewArray zero-copyable to ListArray optimizations#5129
Merged
connortsui20 merged 2 commits intodevelopfrom Nov 3, 2025
Merged
Performance: ListViewArray zero-copyable to ListArray optimizations#5129connortsui20 merged 2 commits intodevelopfrom
ListViewArray zero-copyable to ListArray optimizations#5129connortsui20 merged 2 commits intodevelopfrom
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
connortsui20
force-pushed
the
ct/list-view-zctl
branch
2 times, most recently
from
04f18cc to
5155c0d
Compare
connortsui20
added
the
action/benchmark
github-actions
bot
removed
the
action/benchmark
Contributor
Benchmarks: Random AccessSummary
|
Contributor
Benchmarks: TPC-H SF=1 on NVMESummary
Detailed Results Table
|
Contributor
Benchmarks: FineWeb NVMeSummary
Detailed Results Table
|
Contributor
Benchmarks: FineWeb S3Summary
Detailed Results Table
|
Contributor
Benchmarks: TPC-H SF=1 on S3Summary
Detailed Results Table
|
Contributor
Benchmarks: TPC-H SF=10 on NVMESummary
Detailed Results Table
|
Contributor
Benchmarks: CompressionSummary
Detailed Results Table
|
Contributor
Benchmarks: TPC-DS SF=1 on NVMESummary
Detailed Results Table
|
Contributor
Benchmarks: GitHub Archive (NVMe)Summary
Detailed Results Table
|
Contributor
Benchmarks: GitHub Archive (S3)Summary
Detailed Results Table
|
Contributor
Benchmarks: TPC-H SF=10 on S3Summary
Detailed Results Table
|
Contributor
Benchmarks: Clickbench on NVMESummary
Detailed Results Table
|
Contributor
Benchmarks: Statistical and Population GeneticsSummary
Detailed Results Table
|
connortsui20
added
the
action/benchmark
github-actions
bot
removed
the
action/benchmark
Contributor
|
What perf changes did you expect from the benchmarks? |
Contributor
Author
|
@joseph-isaacs none since none of our benchmarks really test the conversion between list and listview (we have a test file that didn't finish on develop but finishes in 3-4 sec on this branch). Maybe we can add that as a be regression benchmark? |
fuzz/src/array/mask.rs
Outdated
| array.offsets().clone(), | ||
| array.sizes().clone(), | ||
| new_validity, | ||
| array.is_zero_copy_to_list(), |
Contributor
There was a problem hiding this comment.
This feels leeky here
Contributor
Author
There was a problem hiding this comment.
Leaky in what way? This is basically identical to the mask compute function impl
Contributor
joseph-isaacs
left a comment
joseph-isaacs
left a comment
There was a problem hiding this comment.
Its nice we can avoid changing serialised array. Its telling that we always have a is_zctl comments everywhere
gatesn
reviewed
Nov 3, 2025
connortsui20
force-pushed
the
ct/list-view-zctl
branch
3 times, most recently
from
ca5a1d4 to
f27a6d9
Compare
connortsui20
force-pushed
the
ct/list-view-zctl
branch
from
f27a6d9 to
c60e14b
Compare
github-actions
bot
removed
the
action/benchmark
connortsui20
force-pushed
the
ct/list-view-zctl
branch
from
c60e14b to
8e30bf2
Compare
Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>
connortsui20
force-pushed
the
ct/list-view-zctl
branch
from
8e30bf2 to
0c1203e
Compare
a10y
reviewed
Nov 3, 2025
Comment on lines
112
to
114
| let start = if self.is_zero_copy_to_list() { | ||
| // If offsets are sorted, then the minimum offset is the first offset. | ||
| self.offset_at(0) |
Contributor
There was a problem hiding this comment.
does this hold if the first item is null?
Contributor
Author
There was a problem hiding this comment.
We have the constraint in ListArray where even if there are null elements defined by the validity mask, the offsets still must be sorted. So if the ListViewArray came from a ListArray that had a null first element, this is still valid.
Also when we validate offsets and sizes we completely ignore nulls since for ListViewArray we have the constraint that the views must be valid even if the validity mask specifies the view is null.
See https://arrow.apache.org/docs/format/Columnar.html#listview-layout, specifically:
Every list-view value, including null values, has to guarantee the following invariants:
This does deserve a comment though
Edit: Added the comment in 258a35a
Instead of taking a boolean flag inside `new_unchecked`, it is better to use a builder-like unsafe method that adds the flag. This separates the SAFETY comment out. Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>
connortsui20
force-pushed
the
ct/list-view-zctl
branch
from
0c1203e to
258a35a
Compare
a10y
approved these changes
Nov 3, 2025
Contributor
a10y
left a comment
a10y
left a comment
There was a problem hiding this comment.
Looks good, also ran it again with our broken test case file and perf on the last commit is back to acceptable. Ran the fuzzer for a few minutes and made sure it didn't catch issues either.
Let's ship it
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.
Tracking Issue: #4699
Closes: #5053
This PR introduces a performance optimization for
ListViewArrayby adding anis_zero_copy_to_listflag that tracks whether theListViewArraycan be (near) constant-time converted to aListArraywithout copying data.When this flag is true (indicating sorted offsets with no gaps and no overlaps), conversions can bypass the very expensive rebuild process (which just calls
append_scalarin a loop).Additionally, this PR refactors all
ListViewArrayconstructor call sites and compute operations throughout the codebase to properly maintain this new invariant. The only time you can set this flag is via thenew_uncheckedconstructor which is unsafe, which means the caller better be careful to know what they are doing.Some things that are not included in this PR:
offsetsslot for ann+1th offset inListViewArray. This is becauseListArrayhasn+1offsets, and from a plainListViewArraywe have to reallocate the offsets to make space.reset_offsetsright now is probably more expensive than it could be since it uses thesub_scalarcompute function. We know in advance thatreset_offsetsshould never fail in the non-recursive case.is_zctlflag is SUPER expensive, but it can be made a bit less expensive by using aBitBufferto track references + a flag for overlaps.check_is_zero_copy_to_listfunction onListViewArraythat sets the flag if it detects it is zero-copyable to aListArray. This would be nice when we are reading from disk or converting from an ArrowGenericListView.is_sortedstatistic, but at the same time I feel like it is not correct to store this as a statistic? Not sure.