-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
AVRO-3835: [Rust] Get rid of byteorder and zerocopy dependencies #2455
Conversation
70c6f81
to
e02a65b
Compare
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.
LGTM, Just one suggestion.
lang/rust/avro/src/codec.rs
Outdated
let mut encoded: Vec<u8> = vec![0; snap::raw::max_compress_len(stream.len())]; | ||
let compressed_size = snap::raw::Encoder::new() | ||
.compress(&stream[..], &mut encoded[..]) | ||
.map_err(Error::SnappyCompress)?; | ||
encoded.truncate(compressed_size); |
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.
As we know we will add u32 checksum, wouldn't it be more efficient with
encoded.truncate(compressed_size + 4);
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.
Good suggestion! Thanks!
@@ -28,83 +24,77 @@ pub struct Duration { | |||
} | |||
|
|||
#[derive(Debug, Copy, Clone, Eq, PartialEq)] | |||
pub struct Months(U32<LittleEndian>); | |||
pub struct Months(u32); |
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.
Strange duration concept, that is not even Comparable, and Eq is false between 1 month, 0 day and 0 months 30 days ... And some month are 31 days ...
But this PR indeed enhance this.
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.
The implementation is per the spec - https://avro.apache.org/docs/1.11.1/specification/#duration
Use standard APIs for converting integers to/from byte arrays. Get rid of byteorder and zerocopy dependencies. Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>
e02a65b
to
954a527
Compare
…che#2455) Use standard APIs for converting integers to/from byte arrays. Get rid of byteorder and zerocopy dependencies. Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>
Use standard APIs for converting integers to/from byte arrays. Get rid of byteorder and zerocopy dependencies.
What is the purpose of the change
byteorder
andzerocopy
dependencies with standard APIsVerifying this change
Documentation