This is an automated email from the ASF dual-hosted git repository.
alamb 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 4e5583273f fix: `Buffer::into_mutable` return error instead of panic
for converting owned sliced when not start at 0 and fix returned Mutable length
(#10118)
4e5583273f is described below
commit 4e5583273fb8f308eeb7d21dab979cd4268a502f
Author: Raz Luvaton <[email protected]>
AuthorDate: Wed Jun 24 21:52:05 2026 +0300
fix: `Buffer::into_mutable` return error instead of panic for converting
owned sliced when not start at 0 and fix returned Mutable length (#10118)
# Which issue does this PR close?
- Closes #10117
# Rationale for this change
This include 2 fixes:
1. Calling `Buffer::into_mutable` on owned sliced that
`Buffer::ptr_offset() > 0` panicked before, now returning error
2. Calling `Buffer::into_mutable` on owned slice that
`Buffer::ptr_offset() == 0` returned `MutableBuffer` with length greater
than the original buffer length
# What changes are included in this PR?
1. Updating `Buffer::into_mutable` to return error when `ptr_offset > 0`
2. Set the returned `MutableBuffer` length to the original buffer length
so length is the same
# Are these changes tested?
yes
# Are there any user-facing changes?
not really, except error instead of panic and correct mutable length
---
arrow-buffer/src/buffer/immutable.rs | 81 ++++++++++++++++++++++++++++++------
1 file changed, 68 insertions(+), 13 deletions(-)
diff --git a/arrow-buffer/src/buffer/immutable.rs
b/arrow-buffer/src/buffer/immutable.rs
index fc001afe5a..9c7fc1a901 100644
--- a/arrow-buffer/src/buffer/immutable.rs
+++ b/arrow-buffer/src/buffer/immutable.rs
@@ -358,8 +358,8 @@ impl Buffer {
UnalignedBitChunk::new(self.as_slice(), offset, len).count_ones()
}
- /// Returns `MutableBuffer` for mutating the buffer if this buffer is not
shared.
- /// Returns `Err` if this is shared or its allocation is from an external
source or
+ /// Returns `MutableBuffer` for mutating the buffer if this buffer is not
shared or sliced.
+ /// Returns `Err` if this is shared or the [`Self::ptr_offset`] is greater
than 0 or its allocation is from an external source or
/// it is not allocated with alignment [`ALIGNMENT`]
///
/// # Example: Creating a [`MutableBuffer`] from a [`Buffer`]
@@ -383,17 +383,30 @@ impl Buffer {
pub fn into_mutable(self) -> Result<MutableBuffer, Self> {
let ptr = self.ptr;
let length = self.length;
- Arc::try_unwrap(self.data)
- .and_then(|bytes| {
- // The pointer of underlying buffer should not be offset.
- assert_eq!(ptr, bytes.ptr().as_ptr());
- MutableBuffer::from_bytes(bytes).map_err(Arc::new)
- })
- .map_err(|bytes| Buffer {
- data: bytes,
- ptr,
- length,
- })
+
+ // Disallow converting when the offset is not 0 because we can't start
a mutable from offset
+ let res = if self.ptr_offset() > 0 {
+ Err(self.data)
+ } else {
+ Arc::try_unwrap(self.data)
+ };
+
+ res.and_then(|bytes| {
+ // The pointer of underlying buffer should not be offset.
+ assert_eq!(ptr, bytes.ptr().as_ptr());
+ MutableBuffer::from_bytes(bytes).map_err(Arc::new)
+ })
+ .map_err(|bytes| Buffer {
+ data: bytes,
+ ptr,
+ length,
+ })
+ .map(|mut mutable| {
+ // We need to update the length since in case we are converting
sliced buffer we need to return MutableBuffer with the same length
+ // SAFETY: this is safe as the length is coming from valid Buffer
(this) and it is guaranteed to be less than the underlying data buffer
+ unsafe { mutable.set_len(length) };
+ mutable
+ })
}
/// Converts self into a `Vec`, if possible.
@@ -1080,4 +1093,46 @@ mod tests {
drop(capture);
assert_eq!(buffer2.strong_count(), 1);
}
+
+ #[test]
+ fn
into_mutable_should_return_error_for_sliced_owned_buffer_when_ptr_offset_is_not_0()
{
+ let original_buffer_data = [1_u8, 2, 3, 4, 5, 6, 7, 8];
+ for (slice_from, slice_length) in [
+ (original_buffer_data.len(), 0),
+ (2, 4),
+ (2, original_buffer_data.len() - 2),
+ ] {
+ let buffer = Buffer::from(original_buffer_data);
+ let sliced = buffer.slice_with_length(slice_from, slice_length);
+ drop(buffer); // Keep only 1 owner
+ assert_ne!(sliced.ptr_offset(), 0);
+
+ sliced
+ .into_mutable()
+ .expect_err("should not convert sliced buffer when ptr_offset
is not 0");
+ }
+ }
+
+ #[test]
+ fn
into_mutable_should_allow_converting_sliced_owned_buffer_when_ptr_offset_is_0()
{
+ let original_buffer_data = [1_u8, 2, 3, 4, 5, 6, 7, 8];
+ for slice_length in [0, original_buffer_data.len() - 2] {
+ let buffer = Buffer::from(original_buffer_data);
+ let sliced = buffer.slice_with_length(0, slice_length);
+ drop(buffer); // Keep only 1 owner
+ assert_eq!(sliced.ptr_offset(), 0);
+
+ let original_ptr = sliced.data_ptr();
+
+ let mutable = sliced
+ .into_mutable()
+ .unwrap_or_else(|_| panic!("should allow converting to
mutable when not sliced from end when slice_length is {slice_length}"));
+
+ let buffer_back: Buffer = mutable.into();
+ assert_eq!(buffer_back.data_ptr(), original_ptr);
+
+ let expected =
Buffer::from(original_buffer_data).slice_with_length(0, slice_length);
+ assert_eq!(buffer_back.as_slice(), expected.as_slice());
+ }
+ }
}
