Improve `Buffer` documentation, deprecate `Buffer::from_bytes` add `From<Bytes>` and `From<bytes::Bytes>` impls #6939

alamb · 2025-01-04T14:34:56Z

Which issue does this PR close?

Closes Public API using private types: Buffer::from_bytes takes unexported Bytes #6754
Follow on to Add doctest example for Buffer::from_bytes #6920 from @kylebarron
Relate to [Parquet] Reuse buffer in ByteViewArrayDecoderPlain #6930 from @XiangpengHao

Rationale 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 by Bytes and bytes::Bytes difference

Also, the fact it is not clear how to make a Buffer in a zero copy manner lead to unintended copies as described by @XiangpengHao in

Improve performance reading ByteViewArray from parquet by removing an implicit copy #6031

At least part of this confusion is that Bytes is an internal (non pub) struct that is named the same as bytes::Bytes but appears in the public API

I would like to make the whole situation less confusing

What changes are included in this PR?

Add documentation to Bytes making it clear it is internal and how it is related to bytes::Bytes
Improve documentation on Buffer and add examples of how to construct Buffer from Vec and bytes::Bytes
Add From impls to make it easier to create Buffer in zero copy manner (used in examples)
Deprecate from_bytes to close Public API using private types: Buffer::from_bytes takes unexported Bytes #6754
Update parquet reader to use new From impls
Misc other documentation improvements

Are there any user-facing changes?

Better Docs
From impls for Buffer

…impls

alamb · 2025-01-04T14:36:44Z

arrow-buffer/src/buffer/immutable.rs

+/// `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)


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

alamb · 2025-01-04T14:37:49Z

arrow-buffer/src/buffer/immutable.rs

+}
+
+/// Convert from [`bytes::Bytes`], not internal [`Bytes`] to `Buffer`
+impl From<bytes::Bytes> for Buffer {


Here is the From impl proposed by @tustvold in #6930 (comment)

alamb · 2025-01-04T14:38:32Z

arrow-buffer/src/bytes.rs

 ///
-/// 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


Hopefully this will reduce confusion about Bytes (I have also been confused by the name before too, especially when not in an editor/IDE)

alamb · 2025-01-04T14:38:50Z

parquet/src/arrow/array_reader/byte_view_array.rs

-        // 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());


Change to use the From impls

alamb · 2025-01-04T17:17:56Z

After thinking about this more I am also going to propose deprecating Buffer::from_bytes in favor of the From impl which I think will close #6754

Jefffrey

Makes sense to me; some minor comments on wording/formatting

arrow-buffer/src/buffer/immutable.rs

Jefffrey · 2025-01-05T04:38:22Z

arrow-buffer/src/buffer/immutable.rs

+    }
+}
+
+/// Convert from [`bytes::Bytes`], not internal `Bytes` to `Buffer`


Suggested change

/// Convert from [`bytes::Bytes`], not internal `Bytes` to `Buffer`

/// Convert from [`bytes::Bytes`] (not internal `Bytes`) to `Buffer`

parquet/src/arrow/array_reader/byte_view_array.rs

arrow-buffer/src/buffer/immutable.rs

Co-authored-by: Jeffrey Vo <[email protected]>

alamb · 2025-01-05T15:10:45Z

Thank you for the review @Jefffrey -- I will leave this PR open for another day to allow time for additional comments

alamb · 2025-01-06T21:03:48Z

Thanks @mbrobbel as well for the look. I am merging this one in to keep the train moving and unblock #6930

…rom<Bytes>` and `From<bytes::Bytes>` impls (apache#6939) * Improve Bytes documentation * Improve Buffer documentation, add From<Bytes> and From<bytes::Bytes> impls * avoid linking to private docs * Deprecate `Buffer::from_bytes` * Apply suggestions from code review Co-authored-by: Jeffrey Vo <[email protected]> --------- Co-authored-by: Jeffrey Vo <[email protected]>

alamb added 2 commits January 4, 2025 08:05

Improve Bytes documentation

57bb533

Improve Buffer documentation, add From<Bytes> and From<bytes::Bytes> …

cae4a7c

…impls

github-actions bot added parquet Changes to the parquet crate arrow Changes to the arrow crate labels Jan 4, 2025

alamb changed the title ~~Alamb/better bytes docs~~ Improve Buffer documentation, add From<Bytes> and From<bytes::Bytes> impls Jan 4, 2025

alamb commented Jan 4, 2025

View reviewed changes

alamb mentioned this pull request Jan 4, 2025

[Parquet] Reuse buffer in ByteViewArrayDecoderPlain #6930

Merged

avoid linking to private docs

247ce3f

alamb changed the title ~~Improve Buffer documentation, add From<Bytes> and From<bytes::Bytes> impls~~ Improve Buffer documentation, deprecate Buffer::from_bytes add From<Bytes> and From<bytes::Bytes> impls Jan 4, 2025

Deprecate Buffer::from_bytes

e8a796a

github-actions bot added the arrow-flight Changes to the arrow-flight crate label Jan 4, 2025

alamb mentioned this pull request Jan 4, 2025

Public API using private types: Buffer::from_bytes takes unexported Bytes #6754

Closed

Jefffrey approved these changes Jan 5, 2025

View reviewed changes

Apply suggestions from code review

1d48865

Co-authored-by: Jeffrey Vo <[email protected]>

mbrobbel approved these changes Jan 6, 2025

View reviewed changes

alamb merged commit fc245b5 into apache:main Jan 6, 2025
28 checks passed

alamb deleted the alamb/better_bytes_docs branch January 6, 2025 21:03

alamb mentioned this pull request Jan 27, 2025

WIP: Update to arrow-rs / parquet 54.1.0 apache/datafusion#14328

Closed

	/// Convert from [`bytes::Bytes`], not internal `Bytes` to `Buffer`
	/// Convert from [`bytes::Bytes`] (not internal `Bytes`) to `Buffer`

Improve Buffer documentation, deprecate Buffer::from_bytes add From<Bytes> and From<bytes::Bytes> impls #6939

Improve Buffer documentation, deprecate Buffer::from_bytes add From<Bytes> and From<bytes::Bytes> impls #6939

Uh oh!

Conversation

alamb commented Jan 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

Uh oh!

alamb Jan 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb Jan 4, 2025

Choose a reason for hiding this comment

Uh oh!

alamb Jan 4, 2025

Choose a reason for hiding this comment

Uh oh!

alamb Jan 4, 2025

Choose a reason for hiding this comment

Uh oh!

alamb commented Jan 4, 2025

Uh oh!

Jefffrey left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Jefffrey Jan 5, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alamb commented Jan 5, 2025

Uh oh!

alamb commented Jan 6, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Improve `Buffer` documentation, deprecate `Buffer::from_bytes` add `From<Bytes>` and `From<bytes::Bytes>` impls #6939

Improve `Buffer` documentation, deprecate `Buffer::from_bytes` add `From<Bytes>` and `From<bytes::Bytes>` impls #6939

alamb commented Jan 4, 2025 •

edited

Loading

alamb Jan 4, 2025 •

edited

Loading