You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "YimingQiao (via GitHub)" <gi...@apache.org> on 2026/08/16 15:33:46 UTC
[PR] perf: share ByteView buffer lists in take and filter [arrow-rs]
YimingQiao opened a new pull request, #10708:
URL: https://github.com/apache/arrow-rs/pull/10708
# Which issue does this PR close?
- Part of #10692.
- Related to apache/datafusion#16206.
# Rationale for this change
DataFusion hash joins commonly produce a `take` + `BatchCoalescer` pattern. In the high-fanout reproduction from #10692, a `ByteView` array can carry thousands of buffer handles. `take_byte_view` and `filter_byte_view` currently call `data_buffers().to_vec()`, so every selection clones and later drops every `Buffer`, even though `GenericByteViewArray` has stored the collection as `Arc<[Buffer]>` since #9016.
This can be viewed through the same lens as our SIGMOD 2025 paper, [*Data Chunk Compaction in Vectorized Execution*](https://yimingqiao.github.io/files/data-chunk-compaction-sigmod25.pdf). The paper studies when compaction is worth its copying cost because fragmented chunks make downstream interpretation expensive. `ByteView` exposes a reference-level variant: the rows and string payload can both be dense, while the indirect buffer table is fragmented and expensive to repeatedly interpret. This PR removes the repeated ownership work in selection kernels; a follow-up will canonicalize duplicate buffer references in `BatchCoalescer` itself.
# What changes are included in this PR?
- Add `GenericByteViewArray::data_buffers_ref` to expose the existing shared `Arc<[Buffer]>`.
- Make `take_byte_view` and `filter_byte_view` clone that `Arc` in O(1), rather than allocate a new list and clone every `Buffer` in O(number of buffers).
- Verify that StringView and BinaryView selection outputs share the input buffer list.
This does not retain any payload that was not already retained: the old implementation cloned every input `Buffer` into the output. Values, null handling, GC policy, and buffer indexes are unchanged.
# Are these changes tested?
```shell
cargo fmt --all -- --check
cargo test -p arrow-array -p arrow-select
cargo clippy -p arrow-array -p arrow-select --all-targets --all-features -- -D warnings
```
On an Intel Xeon Platinum 8474C, the existing `take_kernels` benchmark changed as follows:
| Benchmark | main | this PR | Change |
|---|---:|---:|---:|
| `take stringview 512` | 519.00 ns | 370.31 ns | -28.7% |
| `take stringview 1024` | 717.62 ns | 637.13 ns | -11.3% |
| `take stringview null indices 512` | 541.42 ns | 541.85 ns | no change |
I also ran 2,000 takes of 8,192 rows while varying the number of entries in a shared-payload buffer table:
| Buffer entries | main | this PR | Speedup |
|---:|---:|---:|---:|
| 0 | 8.39 ms | 8.32 ms | 1.01x |
| 1 | 8.40 ms | 8.33 ms | 1.01x |
| 64 | 10.29 ms | 8.31 ms | 1.24x |
| 4,096 | 128.79 ms | 8.39 ms | 15.35x |
The existing mixed StringView filter benchmarks showed no regression; the low-selectivity case improved by 1.8%.
# Are there any user-facing changes?
There is one non-breaking public accessor, `data_buffers_ref`. Selection results now share the same immutable buffer collection rather than independently allocating an equivalent collection.
# AI assistance
I used OpenAI Codex to help inspect the related implementation history, draft the initial 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 is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] perf: avoid cloning ByteView buffer lists in take and filter [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #10708:
URL: https://github.com/apache/arrow-rs/pull/10708#discussion_r3806756603
##########
arrow-array/src/array/byte_view_array.rs:
##########
@@ -306,6 +306,23 @@ impl<T: ByteViewType + ?Sized> GenericByteViewArray<T> {
&self.buffers
}
+ /// Returns a cloned `Arc` of the buffers storing non-inline string or binary data.
Review Comment:
I wonder if we should just change the other APIs be consistent -- like make `data_buffers` return `&Arc<[Buffer]>` and change `new_unchecked` to take Arc<[Buffer]> directly (rather than `Into<Arc<[Buffer]>>` )
That way we could find more places that unecessairly copy these buffers 🤔
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] perf: avoid cloning ByteView buffer lists in take and filter [arrow-rs]
Posted by "YimingQiao (via GitHub)" <gi...@apache.org>.
YimingQiao commented on code in PR #10708:
URL: https://github.com/apache/arrow-rs/pull/10708#discussion_r3794759792
##########
arrow/benches/take_kernels.rs:
##########
@@ -71,6 +72,26 @@ fn bench_take_bounds_check(values: &dyn Array, indices: &UInt32Array) {
hint::black_box(take(values, indices, Some(TakeOptions { check_bounds: true })).unwrap());
}
+/// Creates an array whose buffer entries all share the same payload allocation.
+///
+/// Views reference every entry in turn, isolating the cost of cloning the buffer
+/// collection from the size of the underlying string payload.
+fn create_string_view_array_with_buffers(size: usize, buffer_count: usize) -> StringViewArray {
Review Comment:
Yep, makes sense. I removed the benchmark from the patch. I left the numbers in the PR description as a temporary local check, but there is no new benchmark to maintain.
##########
arrow-array/src/array/byte_view_array.rs:
##########
@@ -306,6 +306,16 @@ impl<T: ByteViewType + ?Sized> GenericByteViewArray<T> {
&self.buffers
}
+ /// Returns shared ownership of the buffers storing non-inline values
+ ///
+ /// This operation is `O(1)` and does not clone the individual buffers or
+ /// their contents. See [`Self::data_buffers`] to inspect the buffers
+ /// without taking shared ownership.
+ #[inline]
+ pub fn data_buffers_shared(&self) -> Arc<[Buffer]> {
+ Arc::clone(&self.buffers)
Review Comment:
Thanks, this is clearer. I renamed it to `data_buffers_cloned` and used the suggested docs. I only changed `string data` to `non-inline string or binary data`, since the API is shared by StringView and BinaryView, and kept the O(1) note.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] perf: avoid cloning ByteView buffer lists in take and filter [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #10708:
URL: https://github.com/apache/arrow-rs/pull/10708#discussion_r3830556306
##########
arrow-array/src/array/byte_view_array.rs:
##########
@@ -300,27 +297,14 @@ impl<T: ByteViewType + ?Sized> GenericByteViewArray<T> {
&self.views
}
- /// Returns the buffers storing string data
- #[inline]
- pub fn data_buffers(&self) -> &[Buffer] {
- &self.buffers
- }
-
- /// Returns a cloned `Arc` of the buffers storing non-inline string or binary data.
+ /// Returns the shared collection of buffers storing non-inline string or binary data.
///
- /// This is useful when needing to construct a new byte view array from this existing
- /// array, but [`into_parts`] is not feasible (e.g. need to keep both arrays around),
- /// and trying to reconstruct the buffers from [`data_buffers`] would require a
- /// `Vec` allocation and cloning of each buffer element, which can be expensive
- /// if there is a large number of buffers.
- ///
- /// This operation is `O(1)` and clones only the collection's `Arc`.
- ///
- /// [`into_parts`]: Self::into_parts
- /// [`data_buffers`]: Self::data_buffers
+ /// The returned `Arc` can be cloned to share the buffers with another array without
+ /// allocating a new collection or cloning the individual buffers. To consume this
+ /// array and take ownership of its buffers, use [`Self::into_parts`].
Review Comment:
👍
##########
arrow-array/src/ffi.rs:
##########
@@ -1815,7 +1815,7 @@ mod tests_from_ffi {
#[cfg(not(feature = "force_validate"))]
fn test_utf8_view_ffi_from_dangling_pointer() {
let empty = GenericByteViewBuilder::<StringViewType>::new().finish();
- let buffers = empty.data_buffers().to_vec();
+ let buffers = Arc::clone(empty.data_buffers());
Review Comment:
it is only a test, but this is a nice improvement
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] perf: avoid cloning ByteView buffer lists in take and filter [arrow-rs]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on code in PR #10708:
URL: https://github.com/apache/arrow-rs/pull/10708#discussion_r3794669903
##########
arrow-array/src/array/byte_view_array.rs:
##########
@@ -306,6 +306,16 @@ impl<T: ByteViewType + ?Sized> GenericByteViewArray<T> {
&self.buffers
}
+ /// Returns shared ownership of the buffers storing non-inline values
+ ///
+ /// This operation is `O(1)` and does not clone the individual buffers or
+ /// their contents. See [`Self::data_buffers`] to inspect the buffers
+ /// without taking shared ownership.
+ #[inline]
+ pub fn data_buffers_shared(&self) -> Arc<[Buffer]> {
+ Arc::clone(&self.buffers)
Review Comment:
```suggestion
/// Returns a cloned `Arc` of the buffers storing string data.
///
/// This is useful when needing to construct a new byte view array from this existing
/// array, but [`into_parts`] is not feasible (e.g. need to keep both arrays around),
/// and trying to reconstruct the buffers from [`data_buffers`] would require a
/// a `Vec` allocation and cloning of each buffer element, which can be expensive
/// if there is a large number of buffers.
///
/// [`into_parts`]: Self::into_parts
/// [`data_buffers`]: Self::data_buffers
#[inline]
pub fn data_buffers_cloned(&self) -> Arc<[Buffer]> {
```
thoughts on adjusting it like so? makes it more clear what the use case is
##########
arrow/benches/take_kernels.rs:
##########
@@ -71,6 +72,26 @@ fn bench_take_bounds_check(values: &dyn Array, indices: &UInt32Array) {
hint::black_box(take(values, indices, Some(TakeOptions { check_bounds: true })).unwrap());
}
+/// Creates an array whose buffer entries all share the same payload allocation.
+///
+/// Views reference every entry in turn, isolating the cost of cloning the buffer
+/// collection from the size of the underlying string payload.
+fn create_string_view_array_with_buffers(size: usize, buffer_count: usize) -> StringViewArray {
Review Comment:
personally i would say we can omit these benchmarks; the change is obvious enough and the improvement is only for a very niche case
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] perf: avoid cloning ByteView buffer lists in take and filter [arrow-rs]
Posted by "YimingQiao (via GitHub)" <gi...@apache.org>.
YimingQiao commented on code in PR #10708:
URL: https://github.com/apache/arrow-rs/pull/10708#discussion_r3826984164
##########
arrow-array/src/array/byte_view_array.rs:
##########
@@ -306,6 +306,23 @@ impl<T: ByteViewType + ?Sized> GenericByteViewArray<T> {
&self.buffers
}
+ /// Returns a cloned `Arc` of the buffers storing non-inline string or binary data.
Review Comment:
That worked nicely. I changed `data_buffers()` to return `&Arc<[Buffer]>` and made `new_unchecked` take `Arc<[Buffer]>` directly. Letting the compiler drive the update found three more `.to_vec()` call sites in the FFI, IPC, and Parquet tests; those now use `Arc::clone`, while callers that genuinely construct new buffer lists convert them explicitly. The workspace checks, relevant tests, clippy, and rustdoc all pass locally. This changes two public signatures, and I do not have permission to add labels here, so could you add `api-change` if appropriate?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Expose `&Arc[Buffer]` in `GenericByteViewArray::data_buffers` for cheaper cloning [arrow-rs]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on PR #10708:
URL: https://github.com/apache/arrow-rs/pull/10708#issuecomment-5390965434
thanks @YimingQiao & @alamb
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Expose `&Arc[Buffer]` in `GenericByteViewArray::data_buffers` for cheaper cloning [arrow-rs]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey merged PR #10708:
URL: https://github.com/apache/arrow-rs/pull/10708
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org