hudi-agent commented on code in PR #19071: URL: https://github.com/apache/hudi/pull/19071#discussion_r3731080178
########## hudi-io/src/test/java/org/apache/hudi/io/hfile/TestHFileDataBlock.java: ########## @@ -0,0 +1,79 @@ +/* + * 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.junit.jupiter.api.Test; + +import java.io.IOException; +import java.nio.ByteBuffer; +import java.nio.charset.StandardCharsets; + +import static org.junit.jupiter.api.Assertions.assertEquals; + +/** + * Validates the exact on-disk bytes the data block writer emits for each record. A data entry is a + * full HBase KeyValue: {@code [4-byte keyLen][4-byte valueLen][2-byte rowLen][row][1-byte cfLen=0] + * [8-byte ts=LATEST][1-byte type=Put][value][1-byte MVCC=0]}. An HBase reader relies on this exact + * framing, so the test asserts every field rather than a single opaque blob. + */ +class TestHFileDataBlock { + private static final long LATEST_TIMESTAMP = Long.MAX_VALUE; + private static final byte KEY_TYPE_PUT = (byte) 4; + // Column-family length (1) + timestamp (8) + key type (1) + the 2-byte row-length prefix. + private static final int KEY_SUFFIX_AND_PREFIX_LENGTH = 12; + + @Test + void writesFullHBaseKeyValuePerRecord() throws IOException { Review Comment: 🤖 nit: `KEY_SUFFIX_AND_PREFIX_LENGTH` is a bit awkward — "suffix AND prefix" conflates two different parts of the KeyValue key framing (the 10-byte suffix and the 2-byte row-length prefix). The production constant in `HFileBlock` calls just the suffix `KEY_METADATA_SUFFIX_LENGTH`. Could you use a name like `KV_KEY_OVERHEAD` or `KV_KEY_FRAMING_LENGTH` here (and mirror it in `TestHFileRootIndexBlock`) for clarity? <sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag quality.</i></sub> ########## hudi-io/src/test/java/org/apache/hudi/io/hfile/TestHFileWriter.java: ########## @@ -232,6 +250,72 @@ void testLongKeys() throws IOException { } } + /** + * Format lock: with NONE compression and a fixed input the data block and root block-index block + * are deterministic, so their raw bytes are asserted against a golden. The same records written + * by the HBase HFile writer (NONE compression, NULL checksum, latest timestamp, Put type) must + * produce the same two block byte regions, proving the native and HBase writers agree on the + * on-disk encoding. Any change to the encoding (dropping the KeyValue suffix, ts/type, or + * framing) fails here. Neither block holds the file-creation timestamp, so the bytes are stable. + */ + @Test + void writerBlockBytesAreStableFormatLock() throws Exception { + writeTestFile(); + String[] nativeBlocks = dataAndRootIndexBlockHex(Files.readAllBytes(Paths.get(TEST_FILE))); + // Logged so the golden can be regenerated intentionally. + log.info("GOLDEN_DATA_REGION_HEX={}", nativeBlocks[0]); + log.info("GOLDEN_ROOT_INDEX_BLOCK_HEX={}", nativeBlocks[1]); + assertEquals(GOLDEN_DATA_REGION_HEX, nativeBlocks[0], + "native data block bytes changed (storage-format change); review HBase compatibility"); + assertEquals(GOLDEN_ROOT_INDEX_BLOCK_HEX, nativeBlocks[1], + "native root block-index bytes changed (storage-format change); review HBase compatibility"); + + // The HBase writer, given the same records, must produce the same two block byte regions. + String[] hbaseBlocks = dataAndRootIndexBlockHex(writeFixedHBaseFile()); + log.info("HBASE_DATA_REGION_HEX={}", hbaseBlocks[0]); + log.info("HBASE_ROOT_INDEX_BLOCK_HEX={}", hbaseBlocks[1]); + assertEquals(GOLDEN_DATA_REGION_HEX, hbaseBlocks[0], + "HBase writer data block bytes differ from the native writer"); Review Comment: 🤖 nit: `dataAndRootIndexBlockHex` returns a positional `String[]` where callers must remember that `[0]` is the data region and `[1]` is the root index block. A tiny two-field inner record (or even just two separate methods) would make the call sites self-documenting without adding much code. <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: ########## @@ -318,6 +325,41 @@ private byte[] generateChecksumBytes(ChecksumType type, int numChecksumBytes) { throw new HoodieException("Only NULL checksum type is supported"); } + /** + * Returns the serialized length of the KeyValue key for a row: the 2-byte row-length prefix, the + * row, and the 10-byte metadata suffix (column-family length, timestamp, key type). + * + * <p>The data block and the root index block both write the full KeyValue key (not just the row) + * so that a reader can parse and point-look-up either block: a point lookup compares index keys + * against data keys, so the two must use byte-identical key encoding. + * + * @param rowLength length of the row (key content) in bytes. + * @return the KeyValue key length. + */ + protected static int keyValueKeyLength(int rowLength) { Review Comment: 🤖 Confirming the sibling risk is real: `HFileMetaIndexBlock.getUncompressedBlockDataToWrite()` writes bare keys today (`writeVarInt(getLength())` + `getFirstKey().getBytes()`), with an explicit `// Note that: NO two-bytes for encoding key length` comment — so it deliberately diverges from the root-index framing in both the 2-byte prefix and the KeyValue suffix. If these two impls ever get deduped, meta keys would silently gain the KeyValue encoding. Moving the helper to `HFileUtils` (matching the PR description) keeps the encoding opt-in per block type rather than inherited. -- 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]
