Add safe zero-copy conversion from bytes::Bytes (#4254) #4260
Conversation
github-actions
bot
added
arrow
tustvold
force-pushed
the
zero-copy-bytes
branch
from
f9c66ff
to
7c3ba73
Compare
| @@ -258,7 +259,7 @@ impl FlightDataDecoder { | |||
| )); | |||
| }; | |||
|
|
|||
| let buffer: arrow_buffer::Buffer = data.data_body.into(); | |||
| let buffer = Buffer::from_bytes(data.data_body.into()); | |||
There was a problem hiding this comment.
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
There was a problem hiding this comment.
👍 I think we can use this API in IOx as well, FWIW.
| @@ -34,6 +34,7 @@ path = "src/lib.rs" | |||
| bench = false | |||
|
|
|||
| [dependencies] | |||
| bytes = { version = "1.4" } | |||
There was a problem hiding this comment.
This is such a common dependency that I don't see this being overly burdensome
There was a problem hiding this comment.
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
There was a problem hiding this comment.
if it's so common, why do we have our own Bytes types in the first place?
There was a problem hiding this comment.
Because the bytes crate doesn't allow foreign allocations
There was a problem hiding this comment.
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+
There was a problem hiding this comment.
tokio-rs/bytes#437 is the upstream ticket
tustvold
force-pushed
the
zero-copy-bytes
branch
from
7c3ba73
to
2d669f9
Compare
tustvold
changed the title
| @@ -34,6 +34,7 @@ path = "src/lib.rs" | |||
| bench = false | |||
|
|
|||
| [dependencies] | |||
| bytes = { version = "1.4" } | |||
There was a problem hiding this comment.
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
| @@ -258,7 +259,7 @@ impl FlightDataDecoder { | |||
| )); | |||
| }; | |||
|
|
|||
| let buffer: arrow_buffer::Buffer = data.data_body.into(); | |||
| let buffer = Buffer::from_bytes(data.data_body.into()); | |||
There was a problem hiding this comment.
👍 I think we can use this API in IOx as well, FWIW.
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?