Repository navigation
Conversation
No unit test read or wrote a snappy or bzip2 Avro block, so a broken snappy checksum (fixed in apache#2252) went unnoticed. Exchanging files with fastavro checks each codec's framing against an independent implementation of the Avro spec.
There was a problem hiding this comment.
🟢 Approval recommended
There are no active review issues for the added Avro codec interoperability tests.
No open issues 🎉
What changed in this PR
Adds Avro codec interoperability tests against fastavro to help catch block-framing regressions like the snappy checksum bug fixed in #2252.
Changes:
- Covers
null,deflate,bzip2, andsnappycodecs. - Verifies that PyIceberg reads fastavro-written files and fastavro reads PyIceberg-written files.
- Checks record contents in both directions and codec metadata on PyIceberg output.
| File | Description |
|---|---|
tests/avro/test_file.py |
Adds parameterized, bidirectional codec interoperability coverage using fastavro. |
Review effort: Balanced
Fokko
left a comment
Fokko
left a comment
There was a problem hiding this comment.
Thanks @chenjian2664! 🙌 Nice — a bidirectional check against fastavro would have caught the snappy framing bug from #2252. Ran the suite locally, all green.
One small thing: could we also cover zstandard? It's a first-class codec for us (in KNOWN_CODECS, and a core dep) and fastavro writes it too — verified locally that it passes. See the suggestion below :)
| assert list(avro_reader) == records | ||
|
|
||
|
|
||
| @pytest.mark.parametrize("codec", ["null", "deflate", "bzip2", "snappy"]) |
There was a problem hiding this comment.
zstandard is also a first-class codec (a core dependency, no extra gating needed) and fastavro supports it — verified locally that this passes:
| @pytest.mark.parametrize("codec", ["null", "deflate", "bzip2", "snappy"]) | |
| @pytest.mark.parametrize("codec", ["null", "deflate", "bzip2", "snappy", "zstandard"]) |
Fokko
left a comment
Fokko
left a comment
There was a problem hiding this comment.
One suggestion, apart from that, LGTM
|
@Fokko Thank you for the review and help! |
Rationale for this change
No unit test reads or writes a snappy or bzip2 Avro block. The snappy checksum bug fixed in #2252 made every snappy-compressed Avro file unreadable, and nothing caught it. Exchanging files with fastavro in both directions checks each codec's block framing against an independent implementation of the Avro spec. A test that only compresses and decompresses with PyIceberg would miss a framing error that both sides share.
Are these changes tested?
Are there any user-facing changes?