Improve Buffer documentation, deprecate Buffer::from_bytes add From<Bytes> and From<bytes::Bytes> impls
#6939
Conversation
github-actions
bot
added
parquet
alamb
changed the title
Buffer documentation, add From<Bytes> and From<bytes::Bytes> impls
| /// `Buffer`s can be sliced and cloned without copying the underlying data and can | ||
| /// be created from memory allocated by non-Rust sources such as C/C++. | ||
| /// | ||
| /// # Example: Create a `Buffer` from a `Vec` (without copying) |
There was a problem hiding this comment.
The rest of this PR is driven by the examples: converting to/from bytes::Bytes and Vec. I think it will be easier to do this operation now and not get hung up on Bytes vs bytes::Bytes
They are basically an expansion of the nice example @kylebarron added in #6920
| } | ||
|
|
||
| /// Convert from [`bytes::Bytes`], not internal [`Bytes`] to `Buffer` | ||
| impl From<bytes::Bytes> for Buffer { |
There was a problem hiding this comment.
Here is the From impl proposed by @tustvold in #6930 (comment)
| @@ -28,14 +28,18 @@ use crate::buffer::dangling_ptr; | |||
|
|
|||
| /// A continuous, fixed-size, immutable memory region that knows how to de-allocate itself. | |||
| /// | |||
| /// This structs' API is inspired by the `bytes::Bytes`, but it is not limited to using rust's | |||
| /// global allocator nor u8 alignment. | |||
| /// Note that this structure is an internal implementation detail of the | |||
There was a problem hiding this comment.
Hopefully this will reduce confusion about Bytes (I have also been confused by the name before too, especially when not in an editor/IDE)
| // Then we convert `arrow_buffer::Bytes` into `arrow_buffer:Buffer`, which is also zero copy | ||
| let buf = arrow_buffer::Buffer::from_bytes(self.buf.clone().into()); | ||
| // Zero copy convert `bytes::Bytes` into `arrow_buffer:Buffer` | ||
| let buf = arrow_buffer::Buffer::from(self.buf.clone()); |
There was a problem hiding this comment.
Change to use the From impls
alamb
changed the title
Buffer documentation, add From<Bytes> and From<bytes::Bytes> implsBuffer documentation, deprecate Buffer::from_bytes add From<Bytes> and From<bytes::Bytes> impls
|
After thinking about this more I am also going to propose deprecating |
| } | ||
| } | ||
|
|
||
| /// Convert from [`bytes::Bytes`], not internal `Bytes` to `Buffer` |
There was a problem hiding this comment.
| /// Convert from [`bytes::Bytes`], not internal `Bytes` to `Buffer` | |
| /// Convert from [`bytes::Bytes`] (not internal `Bytes`) to `Buffer` |
Co-authored-by: Jeffrey Vo <[email protected]>
|
Thank you for the review @Jefffrey -- I will leave this PR open for another day to allow time for additional comments |
Which issue does this PR close?
Buffer::from_bytestakes unexportedBytes#6754Buffer::from_bytes#6920 from @kylebarronByteViewArrayDecoderPlain#6930 from @XiangpengHaoRationale for this change
As described by @tv42 #6754 and @kylebarron in #6930 creating
Buffers in a zero copy way is confusing, I have also been personally confused byBytesandbytes::BytesdifferenceAlso, the fact it is not clear how to make a
Bufferin a zero copy manner lead to unintended copies as described by @XiangpengHao inByteViewArrayfrom parquet by removing an implicit copy #6031At least part of this confusion is that
Bytesis an internal (non pub) struct that is named the same asbytes::Bytesbut appears in the public APII would like to make the whole situation less confusing
What changes are included in this PR?
Bytesmaking it clear it is internal and how it is related tobytes::BytesBufferand add examples of how to constructBufferfromVecandbytes::BytesFromimpls to make it easier to createBufferin zero copy manner (used in examples)from_bytesto close Public API using private types:Buffer::from_bytestakes unexportedBytes#6754Are there any user-facing changes?
Fromimpls forBuffer