Jefffrey commented on code in PR #10345:
URL: https://github.com/apache/arrow-rs/pull/10345#discussion_r3607639796
##########
parquet/src/compression.rs:
##########
@@ -534,22 +535,31 @@ mod zstd_codec {
output_buf: &mut Vec<u8>,
uncompress_size: Option<usize>,
) -> Result<usize> {
- let capacity = uncompress_size.unwrap_or_else(|| {
- // Get the decompressed size from the zstd frame header
- zstd::zstd_safe::get_frame_content_size(input_buf)
- .ok()
- .flatten()
- .unwrap_or(input_buf.len() as u64 * 4) as usize
+ let offset = output_buf.len();
+ let len = uncompress_size.unwrap_or_else(|| {
+ // Get the decompressed size from the zstd frame header.
+ // See doc of upper_bound about "experimental" feature.
+ zstd::bulk::Decompressor::upper_bound(input_buf)
+ .unwrap_or(input_buf.len().saturating_mul(4))
});
- let decompressed = self.decompressor.decompress(input_buf,
capacity)?;
- let len = decompressed.len();
- output_buf.extend_from_slice(&decompressed);
+ output_buf.reserve(len);
+
+ let mut cursor = Cursor::new(output_buf);
+ cursor.set_position(offset as u64);
+ let len = self
+ .decompressor
+ .decompress_to_buffer(input_buf, &mut cursor)?;
Ok(len)
}
fn compress(&mut self, input_buf: &[u8], output_buf: &mut Vec<u8>) ->
Result<()> {
- let compressed = self.compressor.compress(input_buf)?;
- output_buf.extend_from_slice(&compressed);
+ let offset = output_buf.len();
+ let len = zstd::zstd_safe::compress_bound(input_buf.len());
Review Comment:
these changes make sense since this is essentially what `compress()` already
does internally, we're just pulling it out here 👍
##########
parquet/src/compression.rs:
##########
@@ -534,22 +535,31 @@ mod zstd_codec {
output_buf: &mut Vec<u8>,
uncompress_size: Option<usize>,
) -> Result<usize> {
- let capacity = uncompress_size.unwrap_or_else(|| {
- // Get the decompressed size from the zstd frame header
- zstd::zstd_safe::get_frame_content_size(input_buf)
- .ok()
- .flatten()
- .unwrap_or(input_buf.len() as u64 * 4) as usize
+ let offset = output_buf.len();
+ let len = uncompress_size.unwrap_or_else(|| {
Review Comment:
for here, the previous logic for the capacity (including the internals of
`decompress()` was essentially like:
```rust
let capacity = uncompress_size.unwrap_or_else(|| {
// Get the decompressed size from the zstd frame header
zstd::zstd_safe::get_frame_content_size(input_buf)
.ok()
.flatten()
.unwrap_or(input_buf.len() as u64 * 4) as usize
});
let capacity = zstd::bulk::Decompressor::upper_bound(input_buf)
.unwrap_or(capacity)
.min(capacity);
```
which compared to this new change:
```rust
let capacity = uncompress_size.unwrap_or_else(|| {
// Get the decompressed size from the zstd frame header.
// See doc of upper_bound about "experimental" feature.
zstd::bulk::Decompressor::upper_bound(input_buf)
.unwrap_or(input_buf.len().saturating_mul(4))
});
```
is there significance to omitting the need for `get_frame_content_size()`
now? internally `decompress()` was already making some use of `upper_bound()`,
but i dont fully know the implications (if any) of this simplification 🤔
--
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]
