alamb commented on code in PR #10118:
URL: https://github.com/apache/arrow-rs/pull/10118#discussion_r3398475209
##########
arrow-buffer/src/buffer/immutable.rs:
##########
@@ -381,14 +390,56 @@ impl Buffer {
///
/// [`ALIGNMENT`]: crate::alloc::ALIGNMENT
pub fn into_mutable(self) -> Result<MutableBuffer, Self> {
+ // Disallow converting sliced buffer into mutable to avoid pitfall of
doing a roundtrip from sliced owned buffer will result in a different length:
+ // ```
+ // fn example(owned_sliced_buffer: Buffer) -> Buffer {
+ // let og_len = owned_sliced_buffer.len();
+ // let roundtrip =
Buffer::from(owned_sliced_buffer.into_mutable().unwrap());
+ // assert_eq!(og_len, roundtrip.len()); // <-- this will have
different length for sliced buffer, causing confusion
+ // }
+ // ```
+ if self.is_sliced() {
+ return Err(self);
+ }
+ self.into_mutable_unsliced()
+ }
+
+ /// Returns `MutableBuffer` for mutating the buffer if this buffer is not
shared.
+ /// This is the same as [`Self::into_mutable`] but allow sliced buffer and
will the entire unsliced data as [`MutableBuffer`]
+ ///
+ /// when the buffer is sliced, the returned [`MutableBuffer`] will contain
the **entire** unsliced data, so calling `Buffer::from` on the resulting
[`MutableBuffer`] from this function
+ /// will be the unsliced buffer
+ /// ```
+ /// # use arrow_buffer::buffer::{Buffer, MutableBuffer};
+ /// let buffer: Buffer = Buffer::from(&[1u8, 2, 3, 4][..]);
+ /// let sliced: Buffer = buffer.slice_with_length(1, 2);
+ /// drop(buffer); // <- keep `sliced` the only reference
+ ///
+ /// assert_eq!(sliced.as_slice(), &[2, 3]);
+ /// let back = Buffer::from(sliced.into_mutable_unsliced().unwrap());
+ /// assert_eq!(back.as_slice(), &[1, 2, 3, 4]);
+ /// ```
+ ///
+ /// # Example: Creating a [`MutableBuffer`] from a [`Buffer`]
+ /// ```
+ /// # use arrow_buffer::buffer::{Buffer, MutableBuffer};
+ /// let buffer: Buffer = Buffer::from(&[1u8, 2, 3, 4][..]);
+ /// let sliced: Buffer = buffer.slice_with_length(1, 2);
+ /// drop(buffer); // <- keep `sliced` the only reference
+ /// assert_eq!(sliced.as_slice(), &[2, 3]);
+ ///
+ /// let mutable = sliced.into_mutable_unsliced().unwrap(); // <- this will
succeed as the buffer is not shared and is not sliced
+ /// assert_eq!(mutable.as_slice(), &[1, 2, 3, 4]);
+ ///
+ /// let buffer_back = Buffer::from(mutable);
+ /// assert_eq!(buffer_back.as_slice(), &[1, 2, 3, 4]);
+ /// ```
+ pub fn into_mutable_unsliced(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());
Review Comment:
why remove this assert? Is it the confusing error?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
