Conversation
Exclude trailing checksum bytes from the native GZIP input stream and add a regression test for Hudi issue apache#19929.
hudi-agent
left a comment
hudi-agent
left a comment
There was a problem hiding this comment.
Thanks for the contribution! This PR bounds native HFile block decompression to the compressed payload (using onDiskDataSizeWithHeader from the block header) so trailing checksum bytes are no longer fed to the gzip decoder, and limits the output to uncompressedSizeWithoutHeader. I traced the writer (serialize), the airlift decompressor / readFully semantics, and the buffer sizing, and the new bounds are consistent with both the Hudi writer and the HBase v3 header layout. No issues flagged from this automated pass — a Hudi committer or PMC member can take it from here for a final review.
cc @yihua
|
cc @linliu-code for the code reviewing. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #19930 +/- ##
============================================
+ Coverage 80.23% 80.36% +0.13%
- Complexity 34747 34792 +45
============================================
Files 2546 2545 -1
Lines 142608 142706 +98
Branches 17362 17671 +309
============================================
+ Hits 114419 114689 +270
+ Misses 20277 20109 -168
+ Partials 7912 7908 -4
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
@linliu-code Gentle follow-up on this PR. The latest Azure CI and Codecov checks are green, and the modified lines are fully covered. The HFile regression test and full hudi-io test suite also pass locally. When you have a chance, could you please take a look? Happy to address any feedback. Thanks! |
linliu-code
left a comment
linliu-code
left a comment
There was a problem hiding this comment.
Thanks for working on this! The PR limits native HFile block decompression to the compressed payload and the uncompressed size, so checksum bytes can no longer be read as a second gzip stream. I checked locally that the new test fails with invalid block type on the pre-fix code with the current Aircompressor 2.0.3, so the description's note that 2.0.3 can hide the over-read may not be needed. The full hudi-io suite passes, including the HBase-written GZ fixtures. Two small inline comments, mainly a suggestion to make the test cover the HBase-written case and the EOFException variant. This is one review pass, not an approval.
| }; | ||
|
|
||
| @Test | ||
| void compressedBlockDoesNotDecodeTrailingChecksums() throws IOException { |
There was a problem hiding this comment.
Would you consider generalizing this into a property: for an HBase-written GZ fixture with real CRC32C checksums (e.g. hudi_1_0_hbase_2_4_9_512KB_GZ_20000.hfile, whose first block has 12 checksum bytes), overwrite the checksum bytes and assert all rows read back identically to the unmodified file? Parameterized over real CRCs, zeros, a gzip header + 07, a gzip header + truncated data, and ~20 seeded random tails starting with 1f 8b 08. I tried it locally: all pass with this change, while the pre-fix code fails on the two header cases and ~6% of the random tails, mostly with EOFException: Unexpected end of ZLIB input stream, which the current test doesn't reach.
| // Copy the block header which is not compressed | ||
| System.arraycopy( | ||
| compressedByteBuff, startOffsetInCompressedBuff, byteBuff, 0, HFILEBLOCK_HEADER_SIZE); | ||
| // onDiskSizeWithoutHeader includes the trailing checksum bytes, while |
There was a problem hiding this comment.
Could the comment also mention the output side? byteBuff is sized with sizeCheckSum included, so asking for byteBuff.length - HFILEBLOCK_HEADER_SIZE made readFully keep reading past the end of the first gzip stream. Worth noting so both limits stay.
hudi-agent
left a comment
hudi-agent
left a comment
There was a problem hiding this comment.
Thanks for working on this! The PR narrows the native HFile reader's GZIP decompression to the block's compressed payload (onDiskDataSizeWithHeader) and caps output at uncompressedSizeWithoutHeader, with a parameterized regression test over an HBase-written GZ fixture. The bounds line up with what the writer side in the same class emits; the main thing worth double-checking is the unvalidated decompress return value, which can now silently zero-fill a block instead of erroring. Please take a look at any inline comments, and this should be ready for a Hudi committer or PMC member to take it from here.
| try (InputStream byteBuffInputStream = new ByteArrayInputStream( | ||
| compressedByteBuff, startOffsetInCompressedBuff + HFILEBLOCK_HEADER_SIZE, onDiskSizeWithoutHeader)) { | ||
| compressedByteBuff, startOffsetInCompressedBuff + HFILEBLOCK_HEADER_SIZE, compressedDataSize)) { | ||
| context.getCompressor().decompress( |
There was a problem hiding this comment.
🤖 Now that the output limit is exactly uncompressedSizeWithoutHeader, would it be worth checking the returned count? IOUtils.readFully breaks silently on read() == -1, so a block whose gzip payload decodes to fewer bytes than the header claims would leave the tail of byteBuff zero-filled and be parsed as key-values instead of failing loudly. Something like if (decompressed != uncompressedSizeWithoutHeader) throw new IOException(...) would turn that into a clear error.
| // byteBuff also reserves sizeCheckSum bytes, so limit output to uncompressedSizeWithoutHeader. | ||
| // Using byteBuff.length - HFILEBLOCK_HEADER_SIZE makes readFully keep reading beyond | ||
| // the first gzip stream to fill that checksum space. | ||
| int compressedDataSize = onDiskDataSizeWithHeader - HFILEBLOCK_HEADER_SIZE; |
There was a problem hiding this comment.
🤖 Is onDiskDataSizeWithHeader guaranteed sane for every HFile the native reader accepts? If a block ever carries 0 (or a value larger than the bytes actually present), compressedDataSize goes negative / out of range and ByteArrayInputStream throws an unchecked IndexOutOfBoundsException instead of an IOException. A bounds check with a descriptive IOException might make corrupt blocks easier to diagnose.
| byteBuff, startOffsetInBuff + Header.ON_DISK_DATA_SIZE_WITH_HEADER_INDEX); | ||
| this.uncompressedSizeWithoutHeader = readInt( | ||
| byteBuff, startOffsetInBuff + Header.UNCOMPRESSED_SIZE_WITHOUT_HEADER_INDEX); | ||
| this.bytesPerChecksum = readInt( |
There was a problem hiding this comment.
🤖 [Line 125] Slightly related: now that the real checksum length is derivable as onDiskSizeWithoutHeader + HFILEBLOCK_HEADER_SIZE - onDiskDataSizeWithHeader (exactly what the new test computes), sizeCheckSum here is computed from getOnDiskSizeWithHeader(), which already includes the checksum bytes and so can over-count chunks. Harmless today since it only over-allocates byteBuff, but would it be cleaner to derive it from onDiskDataSizeWithHeader?
| import static org.junit.jupiter.api.Assertions.assertTrue; | ||
|
|
||
| /** Regression tests for compressed HFile block boundaries. */ | ||
| class TestHFileBlockDecompression { |
There was a problem hiding this comment.
Instead of adding a new test class and new tests, could you adjust existing test artifacts that are used for comprehensive reader testing?
github-actions
Bot
added
size:S
This branch has not been deployed
7 participants
Describe the issue this Pull Request addresses
The native HFile reader can fail with
java.util.zip.ZipException: invalid block typeorjava.io.EOFException: Unexpected end of ZLIB input streamwhen trailing checksum bytes are passed to the GZIP decoder. This addresses #19929.Summary and Changelog
hudi_1_0_hbase_2_4_9_512KB_GZ_20000.hfiletest artifact so its first two data-block checksum tails resemble a gzip member with an invalid DEFLATE block type and one with truncated data. The existingTestHFileReadertests cover all 20,000 rows, point lookups, and prefix lookups without a new test class or test method.Only those two 12-byte checksum regions are modified in the HBase-written fixture; headers, compressed payloads, block offsets, and the remaining file contents are unchanged. These deliberately substituted bytes are no longer valid CRCs. The native reader does not validate CRCs, and must exclude checksum bytes from decompression.
The reader uses
onDiskDataSizeWithHeader - HFILEBLOCK_HEADER_SIZEfor decoder input anduncompressedSizeWithoutHeaderfor the output limit.onDiskSizeWithoutHeaderincludes trailing checksum bytes, and the output buffer also reserves checksum space; requesting that extra output space makesreadFullycontinue beyond the first gzip stream.Validation on Aircompressor 2.0.3:
ZipException: invalid block type. Temporarily restoring only the first block's original checksum exposes the second block'sEOFException: Unexpected end of ZLIB input stream. Both modified tails and the fix were restored before the full-suite run.Impact
No public API, storage format, writer serialization, metadata logic, or block navigation changes. The fix only narrows the native reader's decompression input and output bounds.
Risk Level
low. The change is limited to decompression boundaries and is covered by the existing comprehensive reader tests using the adjusted fixture and the full hudi-io test suite.
Documentation Update
none
Contributor's checklist