-
Notifications
You must be signed in to change notification settings - Fork 924
Add safe zero-copy conversion from bytes::Bytes (#4254) #4260
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,6 +17,7 @@ | |
|
||
use crate::{utils::flight_data_to_arrow_batch, FlightData}; | ||
use arrow_array::{ArrayRef, RecordBatch}; | ||
use arrow_buffer::Buffer; | ||
use arrow_schema::{Schema, SchemaRef}; | ||
use bytes::Bytes; | ||
use futures::{ready, stream::BoxStream, Stream, StreamExt}; | ||
|
@@ -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. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 I think we can use this API in IOx as well, FWIW. |
||
let dictionary_batch = | ||
message.header_as_dictionary_batch().ok_or_else(|| { | ||
FlightError::protocol( | ||
|
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?There was a problem hiding this comment.
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
There was a problem hiding this comment.
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+
There was a problem hiding this comment.
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