-
Notifications
You must be signed in to change notification settings - Fork 35
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
Adds lazy reader support for blobs #629
Conversation
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.
🗺️ PR tour
pub struct BytesRef<'data> { | ||
data: Cow<'data, [u8]>, | ||
} |
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.
🗺️ When there was only a binary reader, methods that read blobs could always return a &[u8]
--a slice of the input buffer. Now that we also have a text reader, we need to accommodate base64-encoded blobs, which always require a new Vec
to be allocated to hold the decoded data. BytesRef
can hold either a borrowed &[u8]
or an owned Vec<u8>
, allowing it to be used in either situation.
This type is analogous to StrRef
and SymbolRef
but for blobs.
@@ -18,7 +19,7 @@ pub enum RawValueRef<'data, D: LazyDecoder<'data>> { | |||
Timestamp(Timestamp), | |||
String(StrRef<'data>), | |||
Symbol(RawSymbolTokenRef<'data>), | |||
Blob(&'data [u8]), | |||
Blob(BytesRef<'data>), |
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.
🗺️ RawValueRef
now returns a BytesRef
instead of a &[u8]
so it has the option to allocate a Vec<u8>
when the input encoding is base64 text. The binary reader can still return a slice of the input buffer.
@@ -23,7 +24,7 @@ pub enum ValueRef<'top, 'data, D: LazyDecoder<'data>> { | |||
Timestamp(Timestamp), | |||
String(StrRef<'data>), | |||
Symbol(SymbolRef<'top>), | |||
Blob(&'data [u8]), | |||
Blob(BytesRef<'data>), |
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.
🗺️ Like RawValueRef
, ValueRef
now returns a BytesRef
instead of a &[u8]
so it has the option to allocate a Vec<u8>
when the input encoding is base64 text. The binary reader can still return a slice of the input buffer.
Codecov ReportPatch coverage is
📢 Thoughts on this report? Let us know!. |
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.
It looks like there are some edge cases that might need to be fixed, but I might be wrong.
Builds on outstanding PRs #612, #613, #614, #616, #617, #619, #620, #621, #622, #623, #627, and #628.
Adds lazy reader support for blobs.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.