Repository navigation
[arrow] Minimize allocation in GenericViewArray::slice() - #9016
Merged
Merged
Conversation
mhilton
reviewed
Dec 19, 2025
| nulls.shrink_to_fit(); | ||
| } | ||
| */ | ||
| todo!() |
Contributor
There was a problem hiding this comment.
Did you mean to leave this todo here? It seems to me that it might be important to finish this part. If it isn't necessary could you add a comment explaining why.
Contributor
There was a problem hiding this comment.
it is also bad we have no test coverage for this. we should make a PR to add some
Contributor
Author
There was a problem hiding this comment.
Nope! I didn't mean to do that. I've pushed a fix. But at least it caught this test coverage miss? 😅
Contributor
There was a problem hiding this comment.
Nope! I didn't mean to do that. I've pushed a fix. But at least it caught this test coverage miss? 😅
Yes indeed -- it is bad we don't have a test for this
alamb
reviewed
Dec 19, 2025
alamb
left a comment
alamb
left a comment
Contributor
There was a problem hiding this comment.
Thank you @maxburke and @mhilton -- this is a really cool PR. I like it a lot
I think once we sort out the "shrink_to_fit" bit it will be ready to go
cc @XiangpengHao and @Dandandan
| } | ||
|
|
||
| fn shrink_to_fit(&mut self) { | ||
| /* |
Contributor
There was a problem hiding this comment.
I think you can use Arc::make_mut here to modify (or clone) the buffers here as needed
fn shrink_to_fit(&mut self) {
self.views.shrink_to_fit();
Arc::make_mut(&mut self.buffers).iter_mut().for_each(|b| b.shrink_to_fit());
if let Some(nulls) = &mut self.nulls {
nulls.shrink_to_fit();
}
}I think the call to
self.buffers.shrink_to_fit();which would shrink the Vec today doesn't really have an analog when the view has an Arc of the slice -- we could potentially call shrink_to_fit that prior to creating the Arc but I think that might be unecessary.
I would personally suggest not messing with the actual self.buffers call
Contributor
Author
There was a problem hiding this comment.
I think I just committed a fix before you posted this 😅
Is shrink_to_fit a best-effort operation? If so, it's probably not necessary to try to shrink self.buffers...?
Contributor
There was a problem hiding this comment.
yeah my review overlapped with your push 🏃
Contributor
There was a problem hiding this comment.
Is shrink_to_fit a best-effort operation? If so, it's probably not necessary to try to shrink self.buffers...?
I think it is reasonable behavior. Maybe we can leave a comment explaining the rationale ('best effort' or something) -- the memory that will be saved from a few elements in a Vec is not likely high -- especially as slicing a generic view array doesn't adjust the buffers anyways
| let len = array.len(); | ||
| array.buffers.insert(0, array.views.into_inner()); | ||
|
|
||
| let mut buffers = array.buffers.iter().cloned().collect::<Vec<_>>(); |
Contributor
There was a problem hiding this comment.
I think you could call to_vec here
Suggested change
| let mut buffers = array.buffers.iter().cloned().collect::<Vec<_>>(); | |
| let mut buffers = array.buffers.into_vec(); |
alamb
added
the
api-change
Contributor
|
Since buffers is exposed I think this is an API change that will have to wait for the next major release (in Feb) |
alamb
approved these changes
Dec 19, 2025
| /// | ||
| /// Panics if [`GenericByteViewArray::try_new`] returns an error | ||
| pub fn new(views: ScalarBuffer<u128>, buffers: Vec<Buffer>, nulls: Option<NullBuffer>) -> Self { | ||
| pub fn new<U>(views: ScalarBuffer<u128>, buffers: U, nulls: Option<NullBuffer>) -> Self |
Contributor
There was a problem hiding this comment.
This is very elegant. It is nice to keep this API backwards compatible (anything that used to compile still compiles).
|
|
||
| /// Deconstruct this array into its constituent parts | ||
| pub fn into_parts(self) -> (ScalarBuffer<u128>, Vec<Buffer>, Option<NullBuffer>) { | ||
| pub fn into_parts(self) -> (ScalarBuffer<u128>, Arc<[Buffer]>, Option<NullBuffer>) { |
Contributor
There was a problem hiding this comment.
this I think is a breaking API change
Contributor
Author
There was a problem hiding this comment.
I've tested it with Datafusion and it dropped right in (both the mainline version and the hacked up and patched version we're using).
But, yeah, I can't speak for other users of the package.
| self.buffers.iter_mut().for_each(|b| b.shrink_to_fit()); | ||
| self.buffers.shrink_to_fit(); | ||
|
|
||
| if let Some(buffers) = Arc::get_mut(&mut self.buffers) { |
Contributor
There was a problem hiding this comment.
I think this will only shrink the buffers when there are no other outstanding references to this code. I think it would be better to call Arc::make_mut here to ensure that the buffers get shrunken
Contributor
Author
There was a problem hiding this comment.
So the issue I have with Arc::make_mut is that if I slice array I now have two references to the underlying buffers. If I call shrink_to_fit on one of them, Arc::make_mut will clone the buffers and then shrink_to_fit will be called on one of them. But because the underlying buffers end up being cloned, it doesn't make the original allocation shrink and in the end it'll end up using more memory, until the other reference is dropped.
Additionally it'll create more allocator pressure because the buffer cloning will duplicate the buffer at it's pre-shrunken size before it's shrunk.
Contributor
There was a problem hiding this comment.
That makes sense -- let's keep it this way then. I do think it is worth a comment explaining the rationale, though, for future readers that may wonder the same thing
alamb
added
the
next-major-release
Contributor
|
run benchmark view_types |
|
🤖 |
|
🤖: Benchmark completed Details
|
Use the suggested Arc<[Buffer]> storage for ViewArray storage instead of an owned Vec<Buffer> so that the slice clone does not allocate.
Contributor
Not bad 😎 |
Contributor
|
run benchmark view_types |
|
🤖 |
|
🤖: Benchmark completed Details
|
Contributor
|
🤔 it actually looks like GC'ing is slowing down -- probably from the need to do an extra allocation for new buffers |
Contributor
Upon some more thought I think an extra allocation in GC is a better tradeoff, especially since a lot of GC'ing in downstream systems like DataFusion actually happens as part of concat and coalesce kernels @XiangpengHao and @ctsk do you haven any thoughts on this tradeoff? |
Contributor
|
run benchmark coalesce_kernels concatenate_kernel |
|
🤖 |
|
🤖: Benchmark completed Details
|
|
🤖 |
|
🤖: Benchmark completed Details
|
Contributor
|
run benchmark coalesce_kernels concatenate_kernel view_types |
|
🤖 |
|
🤖: Benchmark completed Details
|
|
🤖 |
|
🤖: Benchmark completed Details
|
|
🤖 |
|
🤖: Benchmark completed Details
|
Contributor
Author
|
Is there anything else I need to do for this PR? |
Contributor
Author
|
Also do you know why the Archery test is failing? I don't appear to see any failures in the output? |
Contributor
Thanks @maxburke No there is nothing else you need to do. I am just waiting to merge it until the nxt major release window (after we release arrow 57.2.0, which will likely be sometime in the next few weeks
I don't -- but it has been happening regularly I filed a ticket to track it |
Contributor
|
Main is now open for breaking changes. I tried merging this PR up from main and it now gets a logical error (due to some specializations for the zip kernel). I made a PR to this branch to fix it -- perhaps you can review and merge it if it is acceptable @maxburke: |
Merge up from main and fix logical conflict in zip kernel
Contributor
|
Let's do it -- this has been outstanding too long |
Contributor
|
Thanks again @maxburke @ctsk and @XiangpengHao and everyone else to help |
Dandandan
pushed a commit
to Dandandan/arrow-rs
that referenced
this pull request
Jan 15, 2026
Use the suggested Arc<[Buffer]> storage for ViewArray storage instead of an owned Vec<Buffer> so that the slice clone does not allocate. # Which issue does this PR close? - Closes apache#6408 . --------- Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
Jefffrey
pushed a commit
that referenced
this pull request
Aug 24, 2026
…aper cloning (#10708) # Which issue does this PR close? - Related to #10692. # Rationale for this change This change follows directly from the existing ByteView design history: - #6408 traced slow ByteView slicing to allocating and cloning the backing-buffer list. The discussion specifically identified `take` as another operation that would benefit from sharing this list. - #6427 distinguished two related but separate concerns: very large buffer lists may indicate missing GC or deduplication, but avoiding allocation when an operation retains the complete list is independently worthwhile. - #6808 demonstrated why eager deduplication in a general-purpose path is a different tradeoff: it added substantial overhead to common operations, and review favored treating ByteView compaction more holistically. - #9016 subsequently changed `GenericByteViewArray` to store the collection as `Arc<[Buffer]>`, making it possible to share the complete list without allocation. The downstream impact is concrete. [apache/datafusion#16206](apache/datafusion#16206) describes hash joins where concatenating build-side batches produces ByteView payload columns with many backing buffers, and constructing join output repeatedly calls `take`. In that pattern, cloning and later dropping every buffer handle can become a significant part of execution time. Arrow issue #10692 provides a compact multi-stage `take` and `BatchCoalescer` reproduction of the same ownership-metadata amplification. `take_byte_view` and `filter_byte_view` do not yet use this shared representation. They rebuild the collection with `data_buffers().to_vec()`, allocating a new collection and cloning every `Buffer`. This makes the ownership bookkeeping for selection O(number of backing buffers), despite retaining exactly the same complete buffer list. Selection already copies the chosen views while leaving their payloads zero-copy. The cost of retaining the unchanged backing-buffer collection should therefore not grow with the number of entries in that collection. This PR completes that narrow part of the earlier design while leaving buffer canonicalization as a separate problem. # What changes are included in this PR? - Change `GenericByteViewArray::data_buffers()` to return `&Arc<[Buffer]>`, allowing callers to inspect the buffers as before or clone the collection's `Arc` in O(1). - Change `GenericByteViewArray::new_unchecked` to accept `Arc<[Buffer]>` directly, making shared versus newly constructed buffer-list ownership explicit at each call site. - Use `Arc::clone` in the ByteView `take` and `filter` kernels instead of rebuilding the collection. - Replace three additional `data_buffers().to_vec()` call sites found by the stricter constructor signature with shared `Arc` clones. - Verify for both StringView and BinaryView that selection results share the input buffer collection. This PR only removes repeated ownership-metadata cloning. It retains the same complete collection of backing buffers as before. It does not prune or deduplicate buffer entries, remap views, run GC, or copy string/binary payloads. The broader buffer-fragmentation problem described in #10692 remains separate. # Are these changes tested? ```shell cargo fmt --all -- --check cargo check --workspace --all-targets cargo test -p arrow-array -p arrow-select -p arrow-row -p arrow-ipc cargo test -p parquet --test arrow_reader invalid_utf8 cargo clippy -p arrow-array -p arrow-select -p arrow-row -p arrow-ipc -p parquet --all-targets --all-features -- -D warnings cargo doc -p arrow-array --no-deps ``` A temporary local Criterion benchmark was used to validate the asymptotic behavior. It takes 8,192 views distributed across a varying number of buffer entries. The entries share one immutable payload allocation, isolating collection-ownership cost from payload size. `main` and this PR were built in separate Cargo target directories on an Intel Xeon Platinum 8474C: | Buffer entries | `main` | This PR | Speedup | |---:|---:|---:|---:| | 1 | 5.4900 us | 5.4619 us | 1.01x | | 16 | 5.7342 us | 5.4577 us | 1.05x | | 256 | 9.2806 us | 5.4518 us | 1.70x | | 4,096 | 65.774 us | 5.4584 us | 12.05x | Existing normal-size benchmarks did not regress: | Benchmark | `main` | This PR | |---|---:|---:| | `take stringview 512` | 504.73 ns | 439.40 ns | | `take stringview 1024` | 872.36 ns | 773.48 ns | | `filter context mixed string view (kept 1/2)` | 73.406 us | 72.083 us | # Are there any user-facing changes? There are two public signature changes. `data_buffers()` now returns `&Arc<[Buffer]>` instead of `&[Buffer]`; slice methods remain available through deref, while direct `for` iteration can use `.iter()`. `new_unchecked` now requires `Arc<[Buffer]>`; callers constructing a `Vec<Buffer>` can convert it with `.into()`. ByteView selection results now share the immutable backing-buffer collection instead of allocating an equivalent collection. Logical values, null handling, buffer indexes, payload lifetimes, and GC behavior are unchanged. # AI assistance I used OpenAI Codex to help inspect the relevant implementation history, draft the patch and tests, and prepare the benchmark harness and PR text. I reviewed the implementation and benchmark methodology and ran the checks above locally.
Rich-T-kid
pushed a commit
to Rich-T-kid/arrow-rs
that referenced
this pull request
Aug 26, 2026
…aper cloning (apache#10708) # Which issue does this PR close? - Related to apache#10692. # Rationale for this change This change follows directly from the existing ByteView design history: - apache#6408 traced slow ByteView slicing to allocating and cloning the backing-buffer list. The discussion specifically identified `take` as another operation that would benefit from sharing this list. - apache#6427 distinguished two related but separate concerns: very large buffer lists may indicate missing GC or deduplication, but avoiding allocation when an operation retains the complete list is independently worthwhile. - apache#6808 demonstrated why eager deduplication in a general-purpose path is a different tradeoff: it added substantial overhead to common operations, and review favored treating ByteView compaction more holistically. - apache#9016 subsequently changed `GenericByteViewArray` to store the collection as `Arc<[Buffer]>`, making it possible to share the complete list without allocation. The downstream impact is concrete. [apache/datafusion#16206](apache/datafusion#16206) describes hash joins where concatenating build-side batches produces ByteView payload columns with many backing buffers, and constructing join output repeatedly calls `take`. In that pattern, cloning and later dropping every buffer handle can become a significant part of execution time. Arrow issue apache#10692 provides a compact multi-stage `take` and `BatchCoalescer` reproduction of the same ownership-metadata amplification. `take_byte_view` and `filter_byte_view` do not yet use this shared representation. They rebuild the collection with `data_buffers().to_vec()`, allocating a new collection and cloning every `Buffer`. This makes the ownership bookkeeping for selection O(number of backing buffers), despite retaining exactly the same complete buffer list. Selection already copies the chosen views while leaving their payloads zero-copy. The cost of retaining the unchanged backing-buffer collection should therefore not grow with the number of entries in that collection. This PR completes that narrow part of the earlier design while leaving buffer canonicalization as a separate problem. # What changes are included in this PR? - Change `GenericByteViewArray::data_buffers()` to return `&Arc<[Buffer]>`, allowing callers to inspect the buffers as before or clone the collection's `Arc` in O(1). - Change `GenericByteViewArray::new_unchecked` to accept `Arc<[Buffer]>` directly, making shared versus newly constructed buffer-list ownership explicit at each call site. - Use `Arc::clone` in the ByteView `take` and `filter` kernels instead of rebuilding the collection. - Replace three additional `data_buffers().to_vec()` call sites found by the stricter constructor signature with shared `Arc` clones. - Verify for both StringView and BinaryView that selection results share the input buffer collection. This PR only removes repeated ownership-metadata cloning. It retains the same complete collection of backing buffers as before. It does not prune or deduplicate buffer entries, remap views, run GC, or copy string/binary payloads. The broader buffer-fragmentation problem described in apache#10692 remains separate. # Are these changes tested? ```shell cargo fmt --all -- --check cargo check --workspace --all-targets cargo test -p arrow-array -p arrow-select -p arrow-row -p arrow-ipc cargo test -p parquet --test arrow_reader invalid_utf8 cargo clippy -p arrow-array -p arrow-select -p arrow-row -p arrow-ipc -p parquet --all-targets --all-features -- -D warnings cargo doc -p arrow-array --no-deps ``` A temporary local Criterion benchmark was used to validate the asymptotic behavior. It takes 8,192 views distributed across a varying number of buffer entries. The entries share one immutable payload allocation, isolating collection-ownership cost from payload size. `main` and this PR were built in separate Cargo target directories on an Intel Xeon Platinum 8474C: | Buffer entries | `main` | This PR | Speedup | |---:|---:|---:|---:| | 1 | 5.4900 us | 5.4619 us | 1.01x | | 16 | 5.7342 us | 5.4577 us | 1.05x | | 256 | 9.2806 us | 5.4518 us | 1.70x | | 4,096 | 65.774 us | 5.4584 us | 12.05x | Existing normal-size benchmarks did not regress: | Benchmark | `main` | This PR | |---|---:|---:| | `take stringview 512` | 504.73 ns | 439.40 ns | | `take stringview 1024` | 872.36 ns | 773.48 ns | | `filter context mixed string view (kept 1/2)` | 73.406 us | 72.083 us | # Are there any user-facing changes? There are two public signature changes. `data_buffers()` now returns `&Arc<[Buffer]>` instead of `&[Buffer]`; slice methods remain available through deref, while direct `for` iteration can use `.iter()`. `new_unchecked` now requires `Arc<[Buffer]>`; callers constructing a `Vec<Buffer>` can convert it with `.into()`. ByteView selection results now share the immutable backing-buffer collection instead of allocating an equivalent collection. Logical values, null handling, buffer indexes, payload lifetimes, and GC behavior are unchanged. # AI assistance I used OpenAI Codex to help inspect the relevant implementation history, draft the patch and tests, and prepare the benchmark harness and PR text. I reviewed the implementation and benchmark methodology and ran the checks above locally.
Rich-T-kid
pushed a commit
to Rich-T-kid/arrow-rs
that referenced
this pull request
Aug 28, 2026
…aper cloning (apache#10708) # Which issue does this PR close? - Related to apache#10692. # Rationale for this change This change follows directly from the existing ByteView design history: - apache#6408 traced slow ByteView slicing to allocating and cloning the backing-buffer list. The discussion specifically identified `take` as another operation that would benefit from sharing this list. - apache#6427 distinguished two related but separate concerns: very large buffer lists may indicate missing GC or deduplication, but avoiding allocation when an operation retains the complete list is independently worthwhile. - apache#6808 demonstrated why eager deduplication in a general-purpose path is a different tradeoff: it added substantial overhead to common operations, and review favored treating ByteView compaction more holistically. - apache#9016 subsequently changed `GenericByteViewArray` to store the collection as `Arc<[Buffer]>`, making it possible to share the complete list without allocation. The downstream impact is concrete. [apache/datafusion#16206](apache/datafusion#16206) describes hash joins where concatenating build-side batches produces ByteView payload columns with many backing buffers, and constructing join output repeatedly calls `take`. In that pattern, cloning and later dropping every buffer handle can become a significant part of execution time. Arrow issue apache#10692 provides a compact multi-stage `take` and `BatchCoalescer` reproduction of the same ownership-metadata amplification. `take_byte_view` and `filter_byte_view` do not yet use this shared representation. They rebuild the collection with `data_buffers().to_vec()`, allocating a new collection and cloning every `Buffer`. This makes the ownership bookkeeping for selection O(number of backing buffers), despite retaining exactly the same complete buffer list. Selection already copies the chosen views while leaving their payloads zero-copy. The cost of retaining the unchanged backing-buffer collection should therefore not grow with the number of entries in that collection. This PR completes that narrow part of the earlier design while leaving buffer canonicalization as a separate problem. # What changes are included in this PR? - Change `GenericByteViewArray::data_buffers()` to return `&Arc<[Buffer]>`, allowing callers to inspect the buffers as before or clone the collection's `Arc` in O(1). - Change `GenericByteViewArray::new_unchecked` to accept `Arc<[Buffer]>` directly, making shared versus newly constructed buffer-list ownership explicit at each call site. - Use `Arc::clone` in the ByteView `take` and `filter` kernels instead of rebuilding the collection. - Replace three additional `data_buffers().to_vec()` call sites found by the stricter constructor signature with shared `Arc` clones. - Verify for both StringView and BinaryView that selection results share the input buffer collection. This PR only removes repeated ownership-metadata cloning. It retains the same complete collection of backing buffers as before. It does not prune or deduplicate buffer entries, remap views, run GC, or copy string/binary payloads. The broader buffer-fragmentation problem described in apache#10692 remains separate. # Are these changes tested? ```shell cargo fmt --all -- --check cargo check --workspace --all-targets cargo test -p arrow-array -p arrow-select -p arrow-row -p arrow-ipc cargo test -p parquet --test arrow_reader invalid_utf8 cargo clippy -p arrow-array -p arrow-select -p arrow-row -p arrow-ipc -p parquet --all-targets --all-features -- -D warnings cargo doc -p arrow-array --no-deps ``` A temporary local Criterion benchmark was used to validate the asymptotic behavior. It takes 8,192 views distributed across a varying number of buffer entries. The entries share one immutable payload allocation, isolating collection-ownership cost from payload size. `main` and this PR were built in separate Cargo target directories on an Intel Xeon Platinum 8474C: | Buffer entries | `main` | This PR | Speedup | |---:|---:|---:|---:| | 1 | 5.4900 us | 5.4619 us | 1.01x | | 16 | 5.7342 us | 5.4577 us | 1.05x | | 256 | 9.2806 us | 5.4518 us | 1.70x | | 4,096 | 65.774 us | 5.4584 us | 12.05x | Existing normal-size benchmarks did not regress: | Benchmark | `main` | This PR | |---|---:|---:| | `take stringview 512` | 504.73 ns | 439.40 ns | | `take stringview 1024` | 872.36 ns | 773.48 ns | | `filter context mixed string view (kept 1/2)` | 73.406 us | 72.083 us | # Are there any user-facing changes? There are two public signature changes. `data_buffers()` now returns `&Arc<[Buffer]>` instead of `&[Buffer]`; slice methods remain available through deref, while direct `for` iteration can use `.iter()`. `new_unchecked` now requires `Arc<[Buffer]>`; callers constructing a `Vec<Buffer>` can convert it with `.into()`. ByteView selection results now share the immutable backing-buffer collection instead of allocating an equivalent collection. Logical values, null handling, buffer indexes, payload lifetimes, and GC behavior are unchanged. # AI assistance I used OpenAI Codex to help inspect the relevant implementation history, draft the patch and tests, and prepare the benchmark harness and PR text. I reviewed the implementation and benchmark methodology and ran the checks above locally.
Rich-T-kid
pushed a commit
to Rich-T-kid/arrow-rs
that referenced
this pull request
Sep 2, 2026
…aper cloning (apache#10708) # Which issue does this PR close? - Related to apache#10692. # Rationale for this change This change follows directly from the existing ByteView design history: - apache#6408 traced slow ByteView slicing to allocating and cloning the backing-buffer list. The discussion specifically identified `take` as another operation that would benefit from sharing this list. - apache#6427 distinguished two related but separate concerns: very large buffer lists may indicate missing GC or deduplication, but avoiding allocation when an operation retains the complete list is independently worthwhile. - apache#6808 demonstrated why eager deduplication in a general-purpose path is a different tradeoff: it added substantial overhead to common operations, and review favored treating ByteView compaction more holistically. - apache#9016 subsequently changed `GenericByteViewArray` to store the collection as `Arc<[Buffer]>`, making it possible to share the complete list without allocation. The downstream impact is concrete. [apache/datafusion#16206](apache/datafusion#16206) describes hash joins where concatenating build-side batches produces ByteView payload columns with many backing buffers, and constructing join output repeatedly calls `take`. In that pattern, cloning and later dropping every buffer handle can become a significant part of execution time. Arrow issue apache#10692 provides a compact multi-stage `take` and `BatchCoalescer` reproduction of the same ownership-metadata amplification. `take_byte_view` and `filter_byte_view` do not yet use this shared representation. They rebuild the collection with `data_buffers().to_vec()`, allocating a new collection and cloning every `Buffer`. This makes the ownership bookkeeping for selection O(number of backing buffers), despite retaining exactly the same complete buffer list. Selection already copies the chosen views while leaving their payloads zero-copy. The cost of retaining the unchanged backing-buffer collection should therefore not grow with the number of entries in that collection. This PR completes that narrow part of the earlier design while leaving buffer canonicalization as a separate problem. # What changes are included in this PR? - Change `GenericByteViewArray::data_buffers()` to return `&Arc<[Buffer]>`, allowing callers to inspect the buffers as before or clone the collection's `Arc` in O(1). - Change `GenericByteViewArray::new_unchecked` to accept `Arc<[Buffer]>` directly, making shared versus newly constructed buffer-list ownership explicit at each call site. - Use `Arc::clone` in the ByteView `take` and `filter` kernels instead of rebuilding the collection. - Replace three additional `data_buffers().to_vec()` call sites found by the stricter constructor signature with shared `Arc` clones. - Verify for both StringView and BinaryView that selection results share the input buffer collection. This PR only removes repeated ownership-metadata cloning. It retains the same complete collection of backing buffers as before. It does not prune or deduplicate buffer entries, remap views, run GC, or copy string/binary payloads. The broader buffer-fragmentation problem described in apache#10692 remains separate. # Are these changes tested? ```shell cargo fmt --all -- --check cargo check --workspace --all-targets cargo test -p arrow-array -p arrow-select -p arrow-row -p arrow-ipc cargo test -p parquet --test arrow_reader invalid_utf8 cargo clippy -p arrow-array -p arrow-select -p arrow-row -p arrow-ipc -p parquet --all-targets --all-features -- -D warnings cargo doc -p arrow-array --no-deps ``` A temporary local Criterion benchmark was used to validate the asymptotic behavior. It takes 8,192 views distributed across a varying number of buffer entries. The entries share one immutable payload allocation, isolating collection-ownership cost from payload size. `main` and this PR were built in separate Cargo target directories on an Intel Xeon Platinum 8474C: | Buffer entries | `main` | This PR | Speedup | |---:|---:|---:|---:| | 1 | 5.4900 us | 5.4619 us | 1.01x | | 16 | 5.7342 us | 5.4577 us | 1.05x | | 256 | 9.2806 us | 5.4518 us | 1.70x | | 4,096 | 65.774 us | 5.4584 us | 12.05x | Existing normal-size benchmarks did not regress: | Benchmark | `main` | This PR | |---|---:|---:| | `take stringview 512` | 504.73 ns | 439.40 ns | | `take stringview 1024` | 872.36 ns | 773.48 ns | | `filter context mixed string view (kept 1/2)` | 73.406 us | 72.083 us | # Are there any user-facing changes? There are two public signature changes. `data_buffers()` now returns `&Arc<[Buffer]>` instead of `&[Buffer]`; slice methods remain available through deref, while direct `for` iteration can use `.iter()`. `new_unchecked` now requires `Arc<[Buffer]>`; callers constructing a `Vec<Buffer>` can convert it with `.into()`. ByteView selection results now share the immutable backing-buffer collection instead of allocating an equivalent collection. Logical values, null handling, buffer indexes, payload lifetimes, and GC behavior are unchanged. # AI assistance I used OpenAI Codex to help inspect the relevant implementation history, draft the patch and tests, and prepare the benchmark harness and PR text. I reviewed the implementation and benchmark methodology and ran the checks above locally.
Rich-T-kid
pushed a commit
to Rich-T-kid/arrow-rs
that referenced
this pull request
Sep 18, 2026
…aper cloning (apache#10708) # Which issue does this PR close? - Related to apache#10692. # Rationale for this change This change follows directly from the existing ByteView design history: - apache#6408 traced slow ByteView slicing to allocating and cloning the backing-buffer list. The discussion specifically identified `take` as another operation that would benefit from sharing this list. - apache#6427 distinguished two related but separate concerns: very large buffer lists may indicate missing GC or deduplication, but avoiding allocation when an operation retains the complete list is independently worthwhile. - apache#6808 demonstrated why eager deduplication in a general-purpose path is a different tradeoff: it added substantial overhead to common operations, and review favored treating ByteView compaction more holistically. - apache#9016 subsequently changed `GenericByteViewArray` to store the collection as `Arc<[Buffer]>`, making it possible to share the complete list without allocation. The downstream impact is concrete. [apache/datafusion#16206](apache/datafusion#16206) describes hash joins where concatenating build-side batches produces ByteView payload columns with many backing buffers, and constructing join output repeatedly calls `take`. In that pattern, cloning and later dropping every buffer handle can become a significant part of execution time. Arrow issue apache#10692 provides a compact multi-stage `take` and `BatchCoalescer` reproduction of the same ownership-metadata amplification. `take_byte_view` and `filter_byte_view` do not yet use this shared representation. They rebuild the collection with `data_buffers().to_vec()`, allocating a new collection and cloning every `Buffer`. This makes the ownership bookkeeping for selection O(number of backing buffers), despite retaining exactly the same complete buffer list. Selection already copies the chosen views while leaving their payloads zero-copy. The cost of retaining the unchanged backing-buffer collection should therefore not grow with the number of entries in that collection. This PR completes that narrow part of the earlier design while leaving buffer canonicalization as a separate problem. # What changes are included in this PR? - Change `GenericByteViewArray::data_buffers()` to return `&Arc<[Buffer]>`, allowing callers to inspect the buffers as before or clone the collection's `Arc` in O(1). - Change `GenericByteViewArray::new_unchecked` to accept `Arc<[Buffer]>` directly, making shared versus newly constructed buffer-list ownership explicit at each call site. - Use `Arc::clone` in the ByteView `take` and `filter` kernels instead of rebuilding the collection. - Replace three additional `data_buffers().to_vec()` call sites found by the stricter constructor signature with shared `Arc` clones. - Verify for both StringView and BinaryView that selection results share the input buffer collection. This PR only removes repeated ownership-metadata cloning. It retains the same complete collection of backing buffers as before. It does not prune or deduplicate buffer entries, remap views, run GC, or copy string/binary payloads. The broader buffer-fragmentation problem described in apache#10692 remains separate. # Are these changes tested? ```shell cargo fmt --all -- --check cargo check --workspace --all-targets cargo test -p arrow-array -p arrow-select -p arrow-row -p arrow-ipc cargo test -p parquet --test arrow_reader invalid_utf8 cargo clippy -p arrow-array -p arrow-select -p arrow-row -p arrow-ipc -p parquet --all-targets --all-features -- -D warnings cargo doc -p arrow-array --no-deps ``` A temporary local Criterion benchmark was used to validate the asymptotic behavior. It takes 8,192 views distributed across a varying number of buffer entries. The entries share one immutable payload allocation, isolating collection-ownership cost from payload size. `main` and this PR were built in separate Cargo target directories on an Intel Xeon Platinum 8474C: | Buffer entries | `main` | This PR | Speedup | |---:|---:|---:|---:| | 1 | 5.4900 us | 5.4619 us | 1.01x | | 16 | 5.7342 us | 5.4577 us | 1.05x | | 256 | 9.2806 us | 5.4518 us | 1.70x | | 4,096 | 65.774 us | 5.4584 us | 12.05x | Existing normal-size benchmarks did not regress: | Benchmark | `main` | This PR | |---|---:|---:| | `take stringview 512` | 504.73 ns | 439.40 ns | | `take stringview 1024` | 872.36 ns | 773.48 ns | | `filter context mixed string view (kept 1/2)` | 73.406 us | 72.083 us | # Are there any user-facing changes? There are two public signature changes. `data_buffers()` now returns `&Arc<[Buffer]>` instead of `&[Buffer]`; slice methods remain available through deref, while direct `for` iteration can use `.iter()`. `new_unchecked` now requires `Arc<[Buffer]>`; callers constructing a `Vec<Buffer>` can convert it with `.into()`. ByteView selection results now share the immutable backing-buffer collection instead of allocating an equivalent collection. Logical values, null handling, buffer indexes, payload lifetimes, and GC behavior are unchanged. # AI assistance I used OpenAI Codex to help inspect the relevant implementation history, draft the patch and tests, and prepare the benchmark harness and PR text. I reviewed the implementation and benchmark methodology and ran the checks above locally.
Rich-T-kid
pushed a commit
to Rich-T-kid/arrow-rs
that referenced
this pull request
Sep 21, 2026
…aper cloning (apache#10708) # Which issue does this PR close? - Related to apache#10692. # Rationale for this change This change follows directly from the existing ByteView design history: - apache#6408 traced slow ByteView slicing to allocating and cloning the backing-buffer list. The discussion specifically identified `take` as another operation that would benefit from sharing this list. - apache#6427 distinguished two related but separate concerns: very large buffer lists may indicate missing GC or deduplication, but avoiding allocation when an operation retains the complete list is independently worthwhile. - apache#6808 demonstrated why eager deduplication in a general-purpose path is a different tradeoff: it added substantial overhead to common operations, and review favored treating ByteView compaction more holistically. - apache#9016 subsequently changed `GenericByteViewArray` to store the collection as `Arc<[Buffer]>`, making it possible to share the complete list without allocation. The downstream impact is concrete. [apache/datafusion#16206](apache/datafusion#16206) describes hash joins where concatenating build-side batches produces ByteView payload columns with many backing buffers, and constructing join output repeatedly calls `take`. In that pattern, cloning and later dropping every buffer handle can become a significant part of execution time. Arrow issue apache#10692 provides a compact multi-stage `take` and `BatchCoalescer` reproduction of the same ownership-metadata amplification. `take_byte_view` and `filter_byte_view` do not yet use this shared representation. They rebuild the collection with `data_buffers().to_vec()`, allocating a new collection and cloning every `Buffer`. This makes the ownership bookkeeping for selection O(number of backing buffers), despite retaining exactly the same complete buffer list. Selection already copies the chosen views while leaving their payloads zero-copy. The cost of retaining the unchanged backing-buffer collection should therefore not grow with the number of entries in that collection. This PR completes that narrow part of the earlier design while leaving buffer canonicalization as a separate problem. # What changes are included in this PR? - Change `GenericByteViewArray::data_buffers()` to return `&Arc<[Buffer]>`, allowing callers to inspect the buffers as before or clone the collection's `Arc` in O(1). - Change `GenericByteViewArray::new_unchecked` to accept `Arc<[Buffer]>` directly, making shared versus newly constructed buffer-list ownership explicit at each call site. - Use `Arc::clone` in the ByteView `take` and `filter` kernels instead of rebuilding the collection. - Replace three additional `data_buffers().to_vec()` call sites found by the stricter constructor signature with shared `Arc` clones. - Verify for both StringView and BinaryView that selection results share the input buffer collection. This PR only removes repeated ownership-metadata cloning. It retains the same complete collection of backing buffers as before. It does not prune or deduplicate buffer entries, remap views, run GC, or copy string/binary payloads. The broader buffer-fragmentation problem described in apache#10692 remains separate. # Are these changes tested? ```shell cargo fmt --all -- --check cargo check --workspace --all-targets cargo test -p arrow-array -p arrow-select -p arrow-row -p arrow-ipc cargo test -p parquet --test arrow_reader invalid_utf8 cargo clippy -p arrow-array -p arrow-select -p arrow-row -p arrow-ipc -p parquet --all-targets --all-features -- -D warnings cargo doc -p arrow-array --no-deps ``` A temporary local Criterion benchmark was used to validate the asymptotic behavior. It takes 8,192 views distributed across a varying number of buffer entries. The entries share one immutable payload allocation, isolating collection-ownership cost from payload size. `main` and this PR were built in separate Cargo target directories on an Intel Xeon Platinum 8474C: | Buffer entries | `main` | This PR | Speedup | |---:|---:|---:|---:| | 1 | 5.4900 us | 5.4619 us | 1.01x | | 16 | 5.7342 us | 5.4577 us | 1.05x | | 256 | 9.2806 us | 5.4518 us | 1.70x | | 4,096 | 65.774 us | 5.4584 us | 12.05x | Existing normal-size benchmarks did not regress: | Benchmark | `main` | This PR | |---|---:|---:| | `take stringview 512` | 504.73 ns | 439.40 ns | | `take stringview 1024` | 872.36 ns | 773.48 ns | | `filter context mixed string view (kept 1/2)` | 73.406 us | 72.083 us | # Are there any user-facing changes? There are two public signature changes. `data_buffers()` now returns `&Arc<[Buffer]>` instead of `&[Buffer]`; slice methods remain available through deref, while direct `for` iteration can use `.iter()`. `new_unchecked` now requires `Arc<[Buffer]>`; callers constructing a `Vec<Buffer>` can convert it with `.into()`. ByteView selection results now share the immutable backing-buffer collection instead of allocating an equivalent collection. Logical values, null handling, buffer indexes, payload lifetimes, and GC behavior are unchanged. # AI assistance I used OpenAI Codex to help inspect the relevant implementation history, draft the patch and tests, and prepare the benchmark harness and PR text. I reviewed the implementation and benchmark methodology and ran the checks above locally.
Rich-T-kid
pushed a commit
to Rich-T-kid/arrow-rs
that referenced
this pull request
Sep 25, 2026
…aper cloning (apache#10708) # Which issue does this PR close? - Related to apache#10692. # Rationale for this change This change follows directly from the existing ByteView design history: - apache#6408 traced slow ByteView slicing to allocating and cloning the backing-buffer list. The discussion specifically identified `take` as another operation that would benefit from sharing this list. - apache#6427 distinguished two related but separate concerns: very large buffer lists may indicate missing GC or deduplication, but avoiding allocation when an operation retains the complete list is independently worthwhile. - apache#6808 demonstrated why eager deduplication in a general-purpose path is a different tradeoff: it added substantial overhead to common operations, and review favored treating ByteView compaction more holistically. - apache#9016 subsequently changed `GenericByteViewArray` to store the collection as `Arc<[Buffer]>`, making it possible to share the complete list without allocation. The downstream impact is concrete. [apache/datafusion#16206](apache/datafusion#16206) describes hash joins where concatenating build-side batches produces ByteView payload columns with many backing buffers, and constructing join output repeatedly calls `take`. In that pattern, cloning and later dropping every buffer handle can become a significant part of execution time. Arrow issue apache#10692 provides a compact multi-stage `take` and `BatchCoalescer` reproduction of the same ownership-metadata amplification. `take_byte_view` and `filter_byte_view` do not yet use this shared representation. They rebuild the collection with `data_buffers().to_vec()`, allocating a new collection and cloning every `Buffer`. This makes the ownership bookkeeping for selection O(number of backing buffers), despite retaining exactly the same complete buffer list. Selection already copies the chosen views while leaving their payloads zero-copy. The cost of retaining the unchanged backing-buffer collection should therefore not grow with the number of entries in that collection. This PR completes that narrow part of the earlier design while leaving buffer canonicalization as a separate problem. # What changes are included in this PR? - Change `GenericByteViewArray::data_buffers()` to return `&Arc<[Buffer]>`, allowing callers to inspect the buffers as before or clone the collection's `Arc` in O(1). - Change `GenericByteViewArray::new_unchecked` to accept `Arc<[Buffer]>` directly, making shared versus newly constructed buffer-list ownership explicit at each call site. - Use `Arc::clone` in the ByteView `take` and `filter` kernels instead of rebuilding the collection. - Replace three additional `data_buffers().to_vec()` call sites found by the stricter constructor signature with shared `Arc` clones. - Verify for both StringView and BinaryView that selection results share the input buffer collection. This PR only removes repeated ownership-metadata cloning. It retains the same complete collection of backing buffers as before. It does not prune or deduplicate buffer entries, remap views, run GC, or copy string/binary payloads. The broader buffer-fragmentation problem described in apache#10692 remains separate. # Are these changes tested? ```shell cargo fmt --all -- --check cargo check --workspace --all-targets cargo test -p arrow-array -p arrow-select -p arrow-row -p arrow-ipc cargo test -p parquet --test arrow_reader invalid_utf8 cargo clippy -p arrow-array -p arrow-select -p arrow-row -p arrow-ipc -p parquet --all-targets --all-features -- -D warnings cargo doc -p arrow-array --no-deps ``` A temporary local Criterion benchmark was used to validate the asymptotic behavior. It takes 8,192 views distributed across a varying number of buffer entries. The entries share one immutable payload allocation, isolating collection-ownership cost from payload size. `main` and this PR were built in separate Cargo target directories on an Intel Xeon Platinum 8474C: | Buffer entries | `main` | This PR | Speedup | |---:|---:|---:|---:| | 1 | 5.4900 us | 5.4619 us | 1.01x | | 16 | 5.7342 us | 5.4577 us | 1.05x | | 256 | 9.2806 us | 5.4518 us | 1.70x | | 4,096 | 65.774 us | 5.4584 us | 12.05x | Existing normal-size benchmarks did not regress: | Benchmark | `main` | This PR | |---|---:|---:| | `take stringview 512` | 504.73 ns | 439.40 ns | | `take stringview 1024` | 872.36 ns | 773.48 ns | | `filter context mixed string view (kept 1/2)` | 73.406 us | 72.083 us | # Are there any user-facing changes? There are two public signature changes. `data_buffers()` now returns `&Arc<[Buffer]>` instead of `&[Buffer]`; slice methods remain available through deref, while direct `for` iteration can use `.iter()`. `new_unchecked` now requires `Arc<[Buffer]>`; callers constructing a `Vec<Buffer>` can convert it with `.into()`. ByteView selection results now share the immutable backing-buffer collection instead of allocating an equivalent collection. Logical values, null handling, buffer indexes, payload lifetimes, and GC behavior are unchanged. # AI assistance I used OpenAI Codex to help inspect the relevant implementation history, draft the patch and tests, and prepare the benchmark harness and PR text. I reviewed the implementation and benchmark methodology and ran the checks above locally.
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.
Use the suggested Arc<[Buffer]> storage for ViewArray storage instead of an owned Vec so that the slice clone does not allocate.
Which issue does this PR close?
StringViewArray::slice()andBinaryViewArray::slice()are slow (they allocate) #6408 .