linliu-code commented on code in PR #19930: URL: https://github.com/apache/hudi/pull/19930#discussion_r4079894411
########## hudi-io/src/test/java/org/apache/hudi/io/hfile/TestHFileBlockDecompression.java: ########## @@ -0,0 +1,97 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.hudi.io.hfile; + +import org.apache.hudi.io.ByteArraySeekableDataInputStream; +import org.apache.hudi.io.ByteBufferBackedInputStream; +import org.apache.hudi.io.compress.CompressionCodec; + +import org.junit.jupiter.api.Test; + +import java.io.ByteArrayOutputStream; +import java.io.IOException; +import java.util.Arrays; +import java.util.Random; + +import static org.apache.hudi.io.hfile.HFileBlock.HFILEBLOCK_HEADER_SIZE; +import static org.apache.hudi.io.util.IOUtils.readInt; +import static org.junit.jupiter.api.Assertions.assertArrayEquals; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; + +/** Regression tests for compressed HFile block boundaries. */ +class TestHFileBlockDecompression { + + // A valid gzip member header without a body. If checksum bytes are passed to the gzip decoder, + // the decoder treats these bytes as a second member and fails with "invalid block type". + private static final byte[] GZIP_MEMBER_HEADER = { + 0x1f, (byte) 0x8b, 0x08, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x07, 0x00, 0x00, 0x00, 0x00, 0x00 + }; + + @Test + void compressedBlockDoesNotDecodeTrailingChecksums() throws IOException { Review 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. ########## hudi-io/src/main/java/org/apache/hudi/io/hfile/HFileBlock.java: ########## @@ -232,13 +236,19 @@ 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 Review 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. -- 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]
