This is an automated email from the ASF dual-hosted git repository.
alamb pushed a commit to branch 59_maintenance
in repository https://gitbox.apache.org/repos/asf/arrow-rs.git
The following commit(s) were added to refs/heads/59_maintenance by this push:
new 5cd40db1f5 [59_maintenance] fix(arrow-array): avoid returning corrupt
array in `GenericByteArray::into_builder` (#11446)
5cd40db1f5 is described below
commit 5cd40db1f5ab200fb6144deb8a818274df633f6c
Author: Jeffrey Vo <[email protected]>
AuthorDate: Fri Oct 9 03:28:05 2026 +0900
[59_maintenance] fix(arrow-array): avoid returning corrupt array in
`GenericByteArray::into_builder` (#11446)
- Backport https://github.com/apache/arrow-rs/pull/11311
- For inclusion in https://github.com/apache/arrow-rs/issues/11352
Cherry-pick of
https://github.com/apache/arrow-rs/commit/c02a2aa78912678e962e07e0ee3764609dfe9e6e
Co-authored-by: Neil Conway <[email protected]>
---
arrow-array/src/array/byte_array.rs | 115 +++++++++--------------
arrow-array/src/array/string_array.rs | 62 +++++++++++-
arrow-array/src/builder/generic_bytes_builder.rs | 9 +-
3 files changed, 111 insertions(+), 75 deletions(-)
diff --git a/arrow-array/src/array/byte_array.rs
b/arrow-array/src/array/byte_array.rs
index 93924ac76b..2f5a0bf19d 100644
--- a/arrow-array/src/array/byte_array.rs
+++ b/arrow-array/src/array/byte_array.rs
@@ -368,87 +368,56 @@ impl<T: ByteArrayType> GenericByteArray<T> {
}
}
- /// Returns `GenericByteBuilder` of this byte array for mutating its
values if the underlying
- /// offset and data buffers are not shared by others.
+ /// Converts this array into a [`GenericByteBuilder`] that reuses its
buffers,
+ /// if possible.
+ ///
+ /// Returns `Err(self)` if the offsets or values cannot be reused, for
+ /// example because they are shared with another array, or because this is
a
+ /// sliced array with a non-zero first offset.
+ ///
+ /// The null buffer is copied if it cannot be reused.
pub fn into_builder(self) -> Result<GenericByteBuilder<T>, Self> {
- let len = self.len();
- let value_len = T::Offset::as_usize(self.value_offsets()[len] -
self.value_offsets()[0]);
-
- let data = self.into_data();
- let null_bit_buffer = data.nulls().map(|b| b.inner().sliced());
-
- let element_len = std::mem::size_of::<T::Offset>();
- let offset_buffer = data.buffers()[0]
- .slice_with_length(data.offset() * element_len, (len + 1) *
element_len);
+ let (offsets, values, nulls) = self.into_parts();
- let element_len = std::mem::size_of::<u8>();
- let value_buffer = data.buffers()[1]
- .slice_with_length(data.offset() * element_len, value_len *
element_len);
-
- drop(data);
-
- let try_mutable_null_buffer = match null_bit_buffer {
- None => Ok(None),
- Some(null_buffer) => {
- // Null buffer exists, tries to make it mutable
- null_buffer.into_mutable().map(Some)
+ // Puts the parts back together into the original array
+ let rebuild = |offsets: Buffer, values: Buffer, nulls:
Option<NullBuffer>| {
+ // SAFETY: the parts come unchanged from `self`
+ unsafe {
+
Self::new_unchecked(OffsetBuffer::new_unchecked(offsets.into()), values, nulls)
}
};
- let try_mutable_buffers = match try_mutable_null_buffer {
- Ok(mutable_null_buffer) => {
- // Got mutable null buffer, tries to get mutable value buffer
- let try_mutable_offset_buffer = offset_buffer.into_mutable();
- let try_mutable_value_buffer = value_buffer.into_mutable();
-
- // try_mutable_offset_buffer.map(...).map_err(...) doesn't
work as the compiler complains
- // mutable_null_buffer is moved into map closure.
- match (try_mutable_offset_buffer, try_mutable_value_buffer) {
- (Ok(mutable_offset_buffer), Ok(mutable_value_buffer)) =>
unsafe {
- Ok(GenericByteBuilder::<T>::new_from_buffer(
- mutable_offset_buffer,
- mutable_value_buffer,
- mutable_null_buffer,
- ))
- },
- (Ok(mutable_offset_buffer), Err(value_buffer)) => Err((
- mutable_offset_buffer.into(),
- value_buffer,
- mutable_null_buffer.map(|b| b.into()),
- )),
- (Err(offset_buffer), Ok(mutable_value_buffer)) => Err((
- offset_buffer,
- mutable_value_buffer.into(),
- mutable_null_buffer.map(|b| b.into()),
- )),
- (Err(offset_buffer), Err(value_buffer)) => Err((
- offset_buffer,
- value_buffer,
- mutable_null_buffer.map(|b| b.into()),
- )),
- }
- }
- Err(mutable_null_buffer) => {
- // Unable to get mutable null buffer
- Err((offset_buffer, value_buffer, Some(mutable_null_buffer)))
- }
+ // A buffer can be reused only if this array is its only owner and it
+ // starts at the beginning of its allocation. Otherwise, the original
+ // array is returned.
+ let offsets = match offsets.into_inner().into_inner().into_mutable() {
+ Ok(offsets) => offsets,
+ Err(offsets) => return Err(rebuild(offsets, values, nulls)),
+ };
+ let values = match values.into_mutable() {
+ Ok(values) => values,
+ Err(values) => return Err(rebuild(offsets.into(), values, nulls)),
};
- match try_mutable_buffers {
- Ok(builder) => Ok(builder),
- Err((offset_buffer, value_buffer, null_bit_buffer)) => {
- let builder = ArrayData::builder(T::DATA_TYPE)
- .len(len)
- .add_buffer(offset_buffer)
- .add_buffer(value_buffer)
- .null_bit_buffer(null_bit_buffer);
-
- let array_data = unsafe { builder.build_unchecked() };
- let array = GenericByteArray::<T>::from(array_data);
-
- Err(array)
+ // The builder needs a null buffer whose first bit is for the first
row.
+ // That is the case when the bit offset is 0, and then the buffer is
reused
+ // if this array is its only owner. Otherwise, the bits are copied,
which is
+ // cheap as there is one bit per row.
+ let nulls = nulls.map(|nulls| {
+ let bits = nulls.into_inner();
+ if bits.offset() == 0 {
+ let len = bits.len();
+ bits.into_inner()
+ .into_mutable()
+ .unwrap_or_else(|buffer|
buffer[..len.div_ceil(8)].to_vec().into())
+ } else {
+ // `sliced` returns the bits starting at the first row
+ bits.sliced().to_vec().into()
}
- }
+ });
+
+ // SAFETY: the buffers come from a valid array
+ Ok(unsafe { GenericByteBuilder::new_from_buffer(offsets, values,
nulls) })
}
}
diff --git a/arrow-array/src/array/string_array.rs
b/arrow-array/src/array/string_array.rs
index b9f06c2a80..c98b2a1b2a 100644
--- a/arrow-array/src/array/string_array.rs
+++ b/arrow-array/src/array/string_array.rs
@@ -159,7 +159,7 @@ mod tests {
use crate::Array;
use crate::builder::{ListBuilder, PrimitiveBuilder, StringBuilder};
use crate::types::UInt8Type;
- use arrow_buffer::Buffer;
+ use arrow_buffer::{Buffer, NullBuffer, OffsetBuffer};
use arrow_data::ArrayData;
use arrow_schema::{DataType, Field};
use std::sync::Arc;
@@ -550,6 +550,66 @@ mod tests {
assert_eq!(&err_return, &shared_array);
}
+ #[test]
+ fn test_into_builder_non_zero_first_offset() {
+ // Sliced, and the only owner of its buffers
+ let array: StringArray = vec!["abcde", "fgh", "ij"].into();
+ let sliced = array.slice(1, 2);
+ drop(array);
+ let err_return = sliced.into_builder().unwrap_err();
+ assert_eq!(err_return, StringArray::from(vec!["fgh", "ij"]));
+
+ // Not sliced, but there are unused bytes before the first offset and
+ // after the last offset
+ let array = StringArray::new(
+ OffsetBuffer::new(vec![3, 5, 8].into()),
+ Buffer::from(b"xxxabcdeyy"),
+ None,
+ );
+ let mut builder = array.into_builder().unwrap();
+ builder.append_value("fg");
+ assert_eq!(builder.finish(), StringArray::from(vec!["ab", "cde",
"fg"]));
+ }
+
+ #[test]
+ fn test_into_builder_err_returns_original_buffers() {
+ // The offsets could be reused, but the values buffer is shared
+ let values = Buffer::from(b"abxyz");
+ let array = StringArray::new(
+ OffsetBuffer::new(vec![0, 2, 2].into()),
+ values.clone(),
+ None,
+ );
+ let offsets_ptr = array.offsets().as_ptr();
+
+ let err_return = array.into_builder().unwrap_err();
+ assert_eq!(err_return.offsets().as_ptr(), offsets_ptr);
+ assert_eq!(err_return.values(), &values);
+ }
+
+ #[test]
+ fn test_into_builder_nulls() {
+ // Reused when this array is the only owner
+ let array = StringArray::from(vec![Some("ab"), None]);
+ let nulls_ptr = array.nulls().unwrap().buffer().as_ptr();
+ let builder = array.into_builder().unwrap();
+ assert_eq!(builder.validity_slice().unwrap().as_ptr(), nulls_ptr);
+
+ // Copied when shared
+ let nulls = NullBuffer::from(vec![true, false]);
+ let array = StringArray::new(
+ OffsetBuffer::new(vec![0, 2, 2].into()),
+ Buffer::from(b"ab"),
+ Some(nulls.clone()),
+ );
+ let mut builder = array.into_builder().unwrap();
+ builder.append_value("c");
+ assert_eq!(
+ builder.finish(),
+ StringArray::from(vec![Some("ab"), None, Some("c")])
+ );
+ }
+
#[test]
fn test_non_null_string_array_equal() {
let a = StringArray::from(vec![Some("ab"), Some("c")]);
diff --git a/arrow-array/src/builder/generic_bytes_builder.rs
b/arrow-array/src/builder/generic_bytes_builder.rs
index a2d044e808..11b0306f85 100644
--- a/arrow-array/src/builder/generic_bytes_builder.rs
+++ b/arrow-array/src/builder/generic_bytes_builder.rs
@@ -58,17 +58,24 @@ impl<T: ByteArrayType> GenericByteBuilder<T> {
/// Creates a new [`GenericByteBuilder`] from buffers.
///
+ /// Any bytes in `value_buffer` after the last offset are dropped. The
first
+ /// bit of `null_buffer` is for the first row.
+ ///
/// # Safety
///
/// This doesn't verify buffer contents as it assumes the buffers are from
/// existing and valid [`GenericByteArray`].
pub unsafe fn new_from_buffer(
offsets_buffer: MutableBuffer,
- value_buffer: MutableBuffer,
+ mut value_buffer: MutableBuffer,
null_buffer: Option<MutableBuffer>,
) -> Self {
let offsets_builder: Vec<T::Offset> =
ScalarBuffer::<T::Offset>::from(offsets_buffer).into();
+
+ // New values are appended at the end of the values buffer, so it must
end
+ // at the last offset
+ value_buffer.truncate(offsets_builder.last().unwrap().as_usize());
let value_builder: Vec<u8> =
ScalarBuffer::<u8>::from(value_buffer).into();
let null_buffer_builder = null_buffer
