Rich-T-kid commented on code in PR #12084:
URL: https://github.com/apache/arrow-rs/pull/12084#discussion_r4486462231
##########
arrow-ipc/src/compression.rs:
##########
@@ -275,19 +290,68 @@ impl CompressionCodec {
}
}
+/// Reusable lz4 frame compressor, the lz4 counterpart of
`zstd::bulk::Compressor`.
+///
+/// `lz4_flex` has no bulk frame API, so this wraps a
[`lz4_flex::frame::FrameEncoder`] whose hash
+/// table and block buffers are retained across calls to
[`Self::compress_to_buffer`].
#[cfg(feature = "lz4")]
-fn compress_lz4(input: &[u8], output: &mut Vec<u8>) -> Result<(), ArrowError> {
- use std::io::Write;
- let mut encoder = lz4_flex::frame::FrameEncoder::new(output);
- encoder.write_all(input)?;
- encoder
- .finish()
- .map_err(|e| ArrowError::ExternalError(Box::new(e)))?;
- Ok(())
+struct Lz4Compressor {
+ encoder: lz4_flex::frame::FrameEncoder<Vec<u8>>,
+}
+
+#[cfg(feature = "lz4")]
+impl Lz4Compressor {
+ fn new() -> Self {
+ // Pin the block size: with `BlockSize::Auto` the first buffer written
would fix it for
+ // every subsequent frame of the reused encoder. 256KB keeps large
buffers as fast as
+ // with bigger blocks while the encoder retains only ~0.5MB of buffers.
+ let frame_info =
+
lz4_flex::frame::FrameInfo::new().block_size(lz4_flex::frame::BlockSize::Max256KB);
Review Comment:
maybe we should make this configurable? It shouldn't matter too much since
the allocation will grow to its max size then be re-used.
##########
arrow-ipc/src/compression.rs:
##########
@@ -431,6 +495,27 @@ mod tests {
);
}
+ #[test]
+ #[cfg(feature = "lz4")]
+ fn test_lz4_compression_reuses_context() {
Review Comment:
This verifies correctness across repeated frames, but it would also pass if
every call created a fresh encoder. Is there a stable way to assert that the
same cached compressor is retained across calls?
we could also keep this test and rename it to
`test_lz4_compression_multiple_frames` and create a separate test to assert the
allocation
##########
arrow-ipc/src/compression.rs:
##########
@@ -431,6 +495,27 @@ mod tests {
);
}
+ #[test]
+ #[cfg(feature = "lz4")]
+ fn test_lz4_compression_reuses_context() {
+ let codec = super::CompressionCodec::Lz4Frame;
+ let mut context = super::IpcWriteContext::default();
+ // Spans several 64KB blocks, followed by a smaller frame.
Review Comment:
is this suppose to be
```suggestion
// Spans several 256KB blocks, followed by a smaller frame.
```
--
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]
