yihua commented on code in PR #697:
URL: https://github.com/apache/hudi-rs/pull/697#discussion_r3909463417
##########
crates/core/src/hfile/key.rs:
##########
@@ -37,20 +37,28 @@ pub const KEY_VALUE_HEADER_SIZE: usize = SIZEOF_INT32 * 2;
/// For comparison and hashing, only the key content is used.
#[derive(Debug, Clone)]
pub struct Key {
- /// Raw key bytes including the length prefix
+ /// This key's own bytes, including the length prefix
bytes: Vec<u8>,
- /// Offset to the start of the key within bytes
+ /// Offset to the start of the key within `bytes`; zero when parsed from a
block
offset: usize,
/// Total length of the key part (including length prefix and other info)
length: usize,
}
impl Key {
/// Create a new Key from bytes at the given offset with the specified
length.
+ ///
+ /// Copies the key's own bytes, not the buffer it came from. `bytes` is a
whole
+ /// data block, so copying it here cost the block once per key: parsing a
block
+ /// of N keys copied it N times, which is quadratic in the block's record
count.
pub fn new(bytes: &[u8], offset: usize, length: usize) -> Self {
+ let end = offset.saturating_add(length).min(bytes.len());
+ let start = offset.min(end);
Self {
- bytes: bytes.to_vec(),
- offset,
+ bytes: bytes[start..end].to_vec(),
Review Comment:
non-blocking: with this change every constructor of `Key` and `KeyValue`
sets `offset: 0`, so both fields are now constants. Worth dropping them (and
the `self.offset +` arithmetic) either here or as a follow-up, so the next
reader doesn't assume a nonzero offset is still possible?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
