hudi-agent commented on code in PR #19930:
URL: https://github.com/apache/hudi/pull/19930#discussion_r4081791184
##########
hudi-io/src/main/java/org/apache/hudi/io/hfile/HFileBlock.java:
##########
@@ -232,13 +236,22 @@ public void unpack() throws IOException {
// Copy the block header which is not compressed
System.arraycopy(
compressedByteBuff, startOffsetInCompressedBuff, byteBuff, 0,
HFILEBLOCK_HEADER_SIZE);
+ // onDiskSizeWithoutHeader includes the trailing checksum bytes, while
+ // onDiskDataSizeWithHeader ends immediately after the compressed
payload.
+ // Passing the former to GZIPInputStream lets it interpret checksum
bytes as
+ // another gzip member and can result in ZipException (for example,
during
+ // metadata table compaction).
+ // 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;
try (InputStream byteBuffInputStream = new ByteArrayInputStream(
- compressedByteBuff, startOffsetInCompressedBuff +
HFILEBLOCK_HEADER_SIZE, onDiskSizeWithoutHeader)) {
+ compressedByteBuff, startOffsetInCompressedBuff +
HFILEBLOCK_HEADER_SIZE, compressedDataSize)) {
context.getCompressor().decompress(
Review 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.
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
hudi-io/src/main/java/org/apache/hudi/io/hfile/HFileBlock.java:
##########
@@ -115,6 +116,8 @@ protected HFileBlock(HFileContext context,
this.blockType = blockType;
this.onDiskSizeWithoutHeader = readInt(
byteBuff, startOffsetInBuff +
Header.ON_DISK_SIZE_WITHOUT_HEADER_INDEX);
+ this.onDiskDataSizeWithHeader = readInt(
+ byteBuff, startOffsetInBuff +
Header.ON_DISK_DATA_SIZE_WITH_HEADER_INDEX);
this.uncompressedSizeWithoutHeader = readInt(
byteBuff, startOffsetInBuff +
Header.UNCOMPRESSED_SIZE_WITHOUT_HEADER_INDEX);
this.bytesPerChecksum = readInt(
Review 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`?
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
hudi-io/src/main/java/org/apache/hudi/io/hfile/HFileBlock.java:
##########
@@ -232,13 +236,22 @@ public void unpack() throws IOException {
// Copy the block header which is not compressed
System.arraycopy(
compressedByteBuff, startOffsetInCompressedBuff, byteBuff, 0,
HFILEBLOCK_HEADER_SIZE);
+ // onDiskSizeWithoutHeader includes the trailing checksum bytes, while
+ // onDiskDataSizeWithHeader ends immediately after the compressed
payload.
+ // Passing the former to GZIPInputStream lets it interpret checksum
bytes as
+ // another gzip member and can result in ZipException (for example,
during
+ // metadata table compaction).
+ // 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;
Review 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.
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
--
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]
