Skip to content

Add safe zero-copy conversion from bytes::Bytes (#4254)#4260

Merged
tustvold merged 1 commit intoapache:masterfrom
tustvold:zero-copy-bytes
Aug 16, 2023
Merged

Add safe zero-copy conversion from bytes::Bytes (#4254)#4260
tustvold merged 1 commit intoapache:masterfrom
tustvold:zero-copy-bytes

Conversation

@tustvold
Copy link
Copy Markdown
Contributor

@tustvold tustvold commented May 22, 2023

Which issue does this PR close?

Closes #4254

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

@github-actions github-actions bot added arrow Changes to the arrow crate arrow-flight Changes to the arrow-flight crate labels May 22, 2023
Copy link
Copy Markdown
Contributor Author

@tustvold tustvold Aug 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This API is kind of unfortunate, but I've not been able to find a good way to workaround the blanket From<AsRef<[u8]>> impl for Buffer

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 I think we can use this API in IOx as well, FWIW.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is such a common dependency that I don't see this being overly burdensome

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://crates.io/crates/bytes/reverse_dependencies -- crates.io agrees with you ✅

If it causes problems for anyone, we could also put it behind a feature flag, but perhaps we can wait to see if anyone needs that before doing it

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if it's so common, why do we have our own Bytes types in the first place?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because the bytes crate doesn't allow foreign allocations

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps we could file a request upstream? I don't see anything related to this functionality in the currently open issues

https://github.com/tokio-rs/bytes/issues?q=is%3Aissue+is%3Aopen+allocation+

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tokio-rs/bytes#437 is the upstream ticket

@tustvold tustvold marked this pull request as ready for review August 16, 2023 10:57
@tustvold tustvold changed the title Add safe zero-copy converion from bytes::Bytes (#4254) Add safe zero-copy conversion from bytes::Bytes (#4254) Aug 16, 2023
Copy link
Copy Markdown
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM -- thank you @tustvold

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://crates.io/crates/bytes/reverse_dependencies -- crates.io agrees with you ✅

If it causes problems for anyone, we could also put it behind a feature flag, but perhaps we can wait to see if anyone needs that before doing it

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 I think we can use this API in IOx as well, FWIW.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

arrow Changes to the arrow crate arrow-flight Changes to the arrow-flight crate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Safe Zero-Copy Buffer from bytes::Bytes

3 participants