This is an automated email from the ASF dual-hosted git repository.
Jefffrey pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/arrow-rs.git
The following commit(s) were added to refs/heads/main by this push:
new d6b0bd7ecc Expose `&Arc[Buffer]` in
`GenericByteViewArray::data_buffers` for cheaper cloning (#10708)
d6b0bd7ecc is described below
commit d6b0bd7ecc857bce0c3b0f97ccae17d5fc3726c4
Author: Yiming Qiao <[email protected]>
AuthorDate: Mon Aug 24 13:05:32 2026 +0800
Expose `&Arc[Buffer]` in `GenericByteViewArray::data_buffers` for cheaper
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](https://github.com/apache/datafusion/issues/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.
---
arrow-array/src/array/byte_view_array.rs | 27 +++++++++++-----------
.../src/builder/generic_bytes_view_builder.rs | 4 ++--
arrow-array/src/ffi.rs | 2 +-
arrow-ipc/src/reader.rs | 2 +-
arrow-row/src/variable.rs | 2 +-
arrow-select/src/coalesce/byte_view.rs | 5 ++--
arrow-select/src/filter.rs | 5 +++-
arrow-select/src/interleave.rs | 2 +-
arrow-select/src/take.rs | 8 ++++---
parquet/src/arrow/buffer/view_buffer.rs | 5 ++--
parquet/tests/arrow_reader/invalid_utf8.rs | 2 +-
11 files changed, 36 insertions(+), 28 deletions(-)
diff --git a/arrow-array/src/array/byte_view_array.rs
b/arrow-array/src/array/byte_view_array.rs
index af390789a2..7a7e94d111 100644
--- a/arrow-array/src/array/byte_view_array.rs
+++ b/arrow-array/src/array/byte_view_array.rs
@@ -238,14 +238,11 @@ impl<T: ByteViewType + ?Sized> GenericByteViewArray<T> {
/// # Safety
///
/// Safe if [`Self::try_new`] would not error
- pub unsafe fn new_unchecked<U>(
+ pub unsafe fn new_unchecked(
views: ScalarBuffer<u128>,
- buffers: U,
+ buffers: Arc<[Buffer]>,
nulls: Option<NullBuffer>,
- ) -> Self
- where
- U: Into<Arc<[Buffer]>>,
- {
+ ) -> Self {
if cfg!(feature = "force_validate") {
return Self::new(views, buffers, nulls);
}
@@ -254,7 +251,7 @@ impl<T: ByteViewType + ?Sized> GenericByteViewArray<T> {
data_type: T::DATA_TYPE,
phantom: Default::default(),
views,
- buffers: buffers.into(),
+ buffers,
nulls,
}
}
@@ -300,9 +297,13 @@ impl<T: ByteViewType + ?Sized> GenericByteViewArray<T> {
&self.views
}
- /// Returns the buffers storing string data
+ /// Returns the shared collection of buffers storing non-inline string or
binary data.
+ ///
+ /// 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`].
#[inline]
- pub fn data_buffers(&self) -> &[Buffer] {
+ pub fn data_buffers(&self) -> &Arc<[Buffer]> {
&self.buffers
}
@@ -543,7 +544,7 @@ impl<T: ByteViewType + ?Sized> GenericByteViewArray<T> {
return unsafe {
GenericByteViewArray::new_unchecked(
self.views().clone(),
- vec![], // empty data blocks
+ Arc::from([]), // empty data blocks
nulls,
)
};
@@ -558,7 +559,7 @@ impl<T: ByteViewType + ?Sized> GenericByteViewArray<T> {
return unsafe {
GenericByteViewArray::new_unchecked(
self.views().clone(),
- vec![], // empty data blocks
+ Arc::from([]), // empty data blocks
nulls,
)
};
@@ -657,7 +658,7 @@ impl<T: ByteViewType + ?Sized> GenericByteViewArray<T> {
let views_scalar = ScalarBuffer::from(views_buf);
// SAFETY: views_scalar, data_blocks, and nulls are correctly aligned
and sized
- unsafe { GenericByteViewArray::new_unchecked(views_scalar,
data_blocks, nulls) }
+ unsafe { GenericByteViewArray::new_unchecked(views_scalar,
data_blocks.into(), nulls) }
}
/// Copy the i‑th view into `data_buf` if it refers to an out‑of‑line
buffer.
@@ -1635,7 +1636,7 @@ mod tests {
gced.data_buffers().len()
);
// No output buffer may exceed the cap.
- for buf in gced.data_buffers() {
+ for buf in gced.data_buffers().iter() {
assert!(buf.len() <= max_buffer_size, "buffer exceeded max size");
}
// Every value (inline, large, and null) is unchanged and in order.
diff --git a/arrow-array/src/builder/generic_bytes_view_builder.rs
b/arrow-array/src/builder/generic_bytes_view_builder.rs
index 12fe7e735b..b68c5d2352 100644
--- a/arrow-array/src/builder/generic_bytes_view_builder.rs
+++ b/arrow-array/src/builder/generic_bytes_view_builder.rs
@@ -500,7 +500,7 @@ impl<T: ByteViewType + ?Sized> GenericByteViewBuilder<T> {
}
let views = std::mem::take(&mut self.views_buffer);
// SAFETY: valid by construction
- unsafe { GenericByteViewArray::new_unchecked(views.into(), completed,
nulls) }
+ unsafe { GenericByteViewArray::new_unchecked(views.into(),
completed.into(), nulls) }
}
/// Builds the [`GenericByteViewArray`] without resetting the builder
@@ -514,7 +514,7 @@ impl<T: ByteViewType + ?Sized> GenericByteViewBuilder<T> {
let views = ScalarBuffer::new(views, 0, len);
let nulls = self.null_buffer_builder.finish_cloned();
// SAFETY: valid by construction
- unsafe { GenericByteViewArray::new_unchecked(views, completed, nulls) }
+ unsafe { GenericByteViewArray::new_unchecked(views, completed.into(),
nulls) }
}
/// Returns the current null buffer as a slice
diff --git a/arrow-array/src/ffi.rs b/arrow-array/src/ffi.rs
index 42c175f33b..d09c077c65 100644
--- a/arrow-array/src/ffi.rs
+++ b/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());
let nulls = empty.nulls().cloned();
// Create a dangling pointer to a view buffer with zero length.
diff --git a/arrow-ipc/src/reader.rs b/arrow-ipc/src/reader.rs
index c42cc5d31c..7d757b534f 100644
--- a/arrow-ipc/src/reader.rs
+++ b/arrow-ipc/src/reader.rs
@@ -3604,7 +3604,7 @@ mod tests {
let array = unsafe {
StringViewArray::new_unchecked(
binary_view_array.views().clone(),
- binary_view_array.data_buffers().to_vec(),
+ Arc::clone(binary_view_array.data_buffers()),
binary_view_array.nulls().cloned(),
)
};
diff --git a/arrow-row/src/variable.rs b/arrow-row/src/variable.rs
index f196e0e220..2dfc1807b6 100644
--- a/arrow-row/src/variable.rs
+++ b/arrow-row/src/variable.rs
@@ -375,7 +375,7 @@ fn decode_binary_view_inner<const VALIDATE_UTF8: bool>(
// SAFETY:
// Valid by construction above
- unsafe { BinaryViewArray::new_unchecked(views.into(), [values.into()],
nulls) }
+ unsafe { BinaryViewArray::new_unchecked(views.into(),
[values.into()].into(), nulls) }
}
/// Decodes a binary view array from `rows` with the provided `options`
diff --git a/arrow-select/src/coalesce/byte_view.rs
b/arrow-select/src/coalesce/byte_view.rs
index feddf29fa7..a6697bfdb5 100644
--- a/arrow-select/src/coalesce/byte_view.rs
+++ b/arrow-select/src/coalesce/byte_view.rs
@@ -501,8 +501,9 @@ impl<B: ByteViewType> InProgressArray for
InProgressByteViewArray<B> {
// Safety: we created valid views and buffers above and the
// input arrays had value data and nulls
- let new_array =
- unsafe { GenericByteViewArray::<B>::new_unchecked(views.into(),
buffers, nulls) };
+ let new_array = unsafe {
+ GenericByteViewArray::<B>::new_unchecked(views.into(),
buffers.into(), nulls)
+ };
Ok(Arc::new(new_array))
}
diff --git a/arrow-select/src/filter.rs b/arrow-select/src/filter.rs
index be2fb18918..80be38d401 100644
--- a/arrow-select/src/filter.rs
+++ b/arrow-select/src/filter.rs
@@ -934,7 +934,7 @@ fn filter_byte_view<T: ByteViewType>(
) -> GenericByteViewArray<T> {
let new_view_buffer = filter_native(array.views(), predicate);
let views = ScalarBuffer::new(new_view_buffer, 0, predicate.count);
- let buffers = array.data_buffers().to_vec();
+ let buffers = Arc::clone(array.data_buffers());
let nulls = predicate.filter_nulls(array.nulls());
// SAFETY: each view is copied unchanged from `array.views()` and `buffers`
@@ -1297,6 +1297,9 @@ mod tests {
let actual = filter(&array, &predicate).unwrap();
assert_eq!(actual.len(), 3);
+ let actual_buffers = actual.as_byte_view::<T>().data_buffers();
+ let input_buffers = array.data_buffers();
+ assert!(Arc::ptr_eq(actual_buffers, input_buffers));
let expected = {
// ["hello", null, "large payload over 12 bytes"]
diff --git a/arrow-select/src/interleave.rs b/arrow-select/src/interleave.rs
index 6d5033b1d4..494fdb2931 100644
--- a/arrow-select/src/interleave.rs
+++ b/arrow-select/src/interleave.rs
@@ -340,7 +340,7 @@ fn interleave_views<T: ByteViewType>(
.collect();
let array = unsafe {
- GenericByteViewArray::<T>::new_unchecked(views.into(), buffers,
interleaved.nulls)
+ GenericByteViewArray::<T>::new_unchecked(views.into(), buffers.into(),
interleaved.nulls)
};
Ok(Arc::new(array))
}
diff --git a/arrow-select/src/take.rs b/arrow-select/src/take.rs
index 36cdb81e76..b66dbf5463 100644
--- a/arrow-select/src/take.rs
+++ b/arrow-select/src/take.rs
@@ -633,10 +633,9 @@ fn take_byte_view<T: ByteViewType, IndexType:
ArrowPrimitiveType>(
) -> Result<GenericByteViewArray<T>, ArrowError> {
let new_views = take_native(array.views(), indices);
let new_nulls = take_nulls(array.nulls(), indices);
+ let buffers = Arc::clone(array.data_buffers());
// Safety: array.views was valid, and take_native copies only valid
values, and verifies bounds
- Ok(unsafe {
- GenericByteViewArray::new_unchecked(new_views,
array.data_buffers().to_vec(), new_nulls)
- })
+ Ok(unsafe { GenericByteViewArray::new_unchecked(new_views, buffers,
new_nulls) })
}
/// `take` implementation for list arrays
@@ -1798,6 +1797,9 @@ mod tests {
let actual = take(&array, &index, None).unwrap();
assert_eq!(actual.len(), index.len());
+ let actual_buffers = actual.as_byte_view::<T>().data_buffers();
+ let input_buffers = array.data_buffers();
+ assert!(Arc::ptr_eq(actual_buffers, input_buffers));
let expected = {
// ["large payload over 12 bytes", null, "world", "large payload
over 12 bytes", "lulu", null]
diff --git a/parquet/src/arrow/buffer/view_buffer.rs
b/parquet/src/arrow/buffer/view_buffer.rs
index 9670d0e9bb..61458eec0b 100644
--- a/parquet/src/arrow/buffer/view_buffer.rs
+++ b/parquet/src/arrow/buffer/view_buffer.rs
@@ -57,14 +57,15 @@ impl ViewBuffer {
let len = self.views.len();
let views = ScalarBuffer::from(self.views);
let nulls = null_buffer.and_then(|b|
NullBuffer::from_unsliced_buffer(b, len));
+ let buffers = self.buffers.into();
match data_type {
ArrowType::Utf8View => {
// Safety: views were created correctly, and checked that the
data is utf8 when building the buffer
- unsafe { Arc::new(StringViewArray::new_unchecked(views,
self.buffers, nulls)) }
+ unsafe { Arc::new(StringViewArray::new_unchecked(views,
buffers, nulls)) }
}
ArrowType::BinaryView => {
// Safety: views were created correctly
- unsafe { Arc::new(BinaryViewArray::new_unchecked(views,
self.buffers, nulls)) }
+ unsafe { Arc::new(BinaryViewArray::new_unchecked(views,
buffers, nulls)) }
}
_ => panic!("Unsupported data type: {data_type}"),
}
diff --git a/parquet/tests/arrow_reader/invalid_utf8.rs
b/parquet/tests/arrow_reader/invalid_utf8.rs
index 1124737a7f..ff1adac374 100644
--- a/parquet/tests/arrow_reader/invalid_utf8.rs
+++ b/parquet/tests/arrow_reader/invalid_utf8.rs
@@ -125,7 +125,7 @@ fn test_invalid_utf8_string_view_array() {
let array = unsafe {
StringViewArray::new_unchecked(
array.views().clone(),
- array.data_buffers().to_vec(),
+ Arc::clone(array.data_buffers()),
array.nulls().cloned(),
)
};
