fix(puffin): implement LZ4 footer compression - #2921
Open
1fanwang wants to merge 4 commits into
Open
Conversation
The PuffinWriter exposed a `compress_footer: bool` flag that set the
FooterPayloadCompressed bit in the file footer and recorded LZ4 as the
codec, but `CompressionCodec::Lz4.compress` returned `FeatureUnsupported`,
so calling `PuffinWriter::new(_, _, true)` always failed at close time:
thread '...' panicked at crates/iceberg/src/puffin/writer.rs:350:
called `Result::unwrap()` on an `Err` value:
FeatureUnsupported => LZ4 compression is not supported currently
The reader side had the same hole: a Puffin file written by another
implementation with FooterPayloadCompressed=1 was unreadable.
Wire LZ4 through `CompressionCodec::{compress,decompress}` using
`lz4_flex`'s frame encoder/decoder, matching the Puffin spec requirement
of "LZ4 single compression frame with content size present" by setting
`FrameInfo::content_size(Some(len))` on the encoder. The reader path is
symmetric — `FrameDecoder::read_to_end` consumes the same frame.
Tests:
- `test_compress_empty_footer_lz4_succeeds` — direct reproducer from
the issue (close() with no blobs and compress_footer=true).
- `test_compress_footer_lz4_round_trips` — full encode/decode loop with
one blob and file properties.
- `test_write_lz4_compressed_metric_data` — was previously asserting the
error string; now asserts round-trip of two blobs.
- `test_lz4_compressed_footer_is_decoded` (metadata) — verifies the
reader honors the FooterPayloadCompressed flag.
- `test_compression_codec_lz4_roundtrip` — round-trip on empty + mixed
payloads, and asserts the frame begins with the LZ4 magic 0x184D2204.
Closes apache#2419.
Signed-off-by: 1fanwang <1fannnw@gmail.com>
- generalize the multi-codec compress test to cover empty and less-compressible payloads for every codec; split the LZ4 frame-magic-number assertion into its own test - rename the Snappy case to test_snappy_compression_is_unsupported - fix the Puffin footer test docs: pre-fix, close() failed with FeatureUnsupported rather than writing raw JSON, and these are feature tests, not regressions; drop issue-number references Signed-off-by: 1fanwang <1fannnw@gmail.com>
Signed-off-by: 1fanwang <1fannnw@gmail.com>
1fanwang
force-pushed
the
fix/puffin-footer-lz4-compression
branch
from
August 24, 2026 06:25
2fc0bff to
86dfdfb
Compare
There was a problem hiding this comment.
Pull request overview
Implements end-to-end LZ4 support needed for Puffin footer compression/decompression so PuffinWriter can successfully close when compress_footer=true, and FileMetadata reading can open Puffin files with an LZ4-compressed footer per the Puffin spec.
Changes:
- Implement LZ4 frame compression/decompression via
lz4_flexinCompressionCodec. - Update Puffin writer/reader tests to validate LZ4-compressed blobs and LZ4-compressed footers round-trip.
- Add
lz4_flexas a direct workspace dependency (and in theicebergcrate) to make the codec implementation explicit.
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| crates/iceberg/src/puffin/writer.rs | Updates tests to expect LZ4 compression to work and adds coverage for compressed footers. |
| crates/iceberg/src/puffin/metadata.rs | Updates footer-metadata test to ensure LZ4-compressed footer payloads decode successfully. |
| crates/iceberg/src/compression.rs | Implements LZ4 frame compress/decompress and expands codec tests (including frame magic assertion). |
| crates/iceberg/Cargo.toml | Adds lz4_flex as a direct dependency for the iceberg crate. |
| Cargo.toml | Adds lz4_flex to workspace dependencies to share the version across crates. |
| Cargo.lock | Records lz4_flex as a direct dependency of iceberg. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+327
to
+331
| let input_file = output_file.to_input_file(); | ||
| let metadata = read_file_metadata(&input_file).await.unwrap(); | ||
| assert_eq!(metadata.properties, file_properties()); | ||
| assert_eq!(metadata.blobs.len(), 1); | ||
| assert_eq!(read_all_blobs_from_puffin_file(input_file).await, vec
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Fixes #2419
Supersedes #2438, which was closed as stale before review finished.
What changes are included in this PR?
PuffinWriterexposes footer compression, but closing a writer with it enabled fails with:The reader also cannot open a Puffin file with an LZ4-compressed footer. LZ4 now uses a single frame with content size in its header, as required by the Puffin spec. Writers close successfully, and readers open the compressed footer. The crate was already present transitively through Parquet.
Are these changes tested?
The tests drive the real Puffin writer and reader for empty and populated compressed footers.
Raw before/after output
Hygiene:
cargo fmt --check, Clippy with all targets and features, andcargo machetepass.