[DNM, stacked] fix(storage-format): fix multi-block previous-block offset and add byte-level HFile writer tests#19083
[DNM, stacked] fix(storage-format): fix multi-block previous-block offset and add byte-level HFile writer tests#19083yihua wants to merge 13 commits into
Conversation
…ve HFile writer The native hudi-io HFile writer stored each block-index first key as a bare [2-byte len][row] with no HBase KeyValue suffix. An HBase-based HFile reader parsed it as a full KeyValue and read the family-length byte one past the end, throwing ArrayIndexOutOfBoundsException in KeyValue.getFamilyLength during a point or prefix lookup of a data-block boundary key. Full scans worked; only the block-index binary search crashed. Write each block-index first key as a full HBase KeyValue key ([2-byte rowLen][row][cfLen=0][ts=LATEST][type=Put]), matching the data-block entries. The native reader is unaffected: it compares only the row via the 2-byte length prefix, so it reads both old bare-key and new full-key files. Only files written after this change are affected. Tests (hudi-io): - TestHFileReadCompatibility: an HBase reader point-looks-up every key, including block-boundary keys, in a native-written multi-block file; native-vs-HBase writer cells are byte-identical under the HBase reader; and a native file reads identically through the native and HBase readers. The point-lookup and byte-comparison cases run under both NONE and GZIP compression. - TestHFileWriter: a golden byte comparison of the data block and root block-index block locks the on-disk format; the single index entry grows by the 10-byte KeyValue suffix.
Both the data block and the root index block now serialize the HBase KeyValue key via HFileUtils.writeKeyValueKey / keyValueKeyLength, so the column-family length (0), timestamp (latest), and key type (Put) are single-sourced and cannot drift between the two blocks (a point lookup compares index keys against data keys, so a mismatch would break it). Behavior-preserving: the on-disk bytes are unchanged and the byte-exact format-lock golden still passes.
Validate the exact serialized bytes of each data-block record and each root-index entry field by field (key and value lengths, row, column-family length, timestamp, key type, and MVCC), pinning the HBase KeyValue framing at the block level.
…lock base class Move keyValueKeyLength and writeKey (renamed from writeKeyValueKey) out of HFileUtils and into the shared HFileBlock base class, next to the existing getVariableLengthEncodedBytes helper, so the data block and the root index block share them without a util class. The timestamp, key type, and suffix length constants are now private to HFileBlock. Restore the original data block comment and drop the redundant inline comment in the root index block; the rationale now lives in the keyValueKeyLength javadoc.
- Refer to the field as the KeyValue key (file-format-specific, not HBase specific) in HFileBlock javadocs and comments. - writeKey now takes the row length explicitly rather than the backing-array length, since a Key's array can be larger than its content. - Document the KeyValue key metadata suffix (column-family length, timestamp, key type) in hfile_format.md for the data block and the data index entry.
writeKey now takes the row offset as well as the length and writes row[offset, offset + length), so a key that is a view into a larger buffer (non-zero offset) serializes correctly. The root index caller passes the key's offset; the data block passes 0. No behavior change for current callers (write-path keys start at offset 0), but it removes the offset-0 assumption.
Describe what the block-index point-lookup and midKey tests validate (the HFile key encoding) rather than referencing before/after behavior.
…fied in the read-compat test Switch the assertion calls to individually static-imported methods (matching the other hudi-io tests) and, since this file is in the org.apache.hudi.io.hfile package, reference the native HFileContext unqualified while fully-qualifying the HBase HFileContext.
…format-lock test Write the fixed records with the HBase HFile writer (NONE compression, NULL checksum, latest timestamp, Put type) and assert its data block and root block-index bytes equal the same golden, confirming the native and HBase writers produce byte-identical blocks.
…i-block HFiles The native HFile writer set each data block's previous-block-offset header to the block's own offset instead of the previous block's: flushCurrentDataBlock passed currentOffset (the next block's start) instead of lastDataBlockOffset when starting the next block. The first block is -1, so single-block golden tests never caught it. The native reader is forward-only and never reads this field, so it is latent for Hudi, but the HBase reader uses it in HFileScanner.seekBefore() to step back to the prior block: on a native-written multi-block file it re-reads the same block and throws IllegalStateException at every block boundary. Adds a regression test that reads such a file with the HBase reader and asserts seekBefore(key(i)) returns key(i-1) for every record.
…ock class Move the Hadoop WritableUtils VInt encoder to HFileIndexBlock.getVarIntBytes (used by the root and meta index key lengths) and the Protobuf varint encoder to HFileFileInfoBlock.getProtobufVarIntBytes (used by the file info and trailer length prefixes), instead of the generic IOUtils / HFileBlock. The two encodings are byte-identical for 0..127 and diverge at >= 128, so the ambiguously named getVariableLengthEncodedBytes is replaced and each method's javadoc cross-references the other. The format spec now names the index keyLength encoding and distinguishes it from the protobuf-delimited trailer/file-info length prefixes.
…er tests Validate the raw on-disk bytes of every HFile section (data, meta, root data index, meta index, file info, trailer) on a multi-block file; pin single-block data/root-index bytes to a golden the HBase writer must reproduce across key lengths (single-byte through three-byte index keyLength, up to 2000-char keys); assert the index keyLength is Hadoop WritableUtils VInt and never protobuf varint; check the data block previous-block-offset chain; and unit-test the VInt encoder.
yihua
changed the title
yihua
changed the title
yihua
changed the title
github-actions
Bot
added
size:XL
There was a problem hiding this comment.
Reviewed the 3-commit delta only (the base commits were reviewed on #19071.
Verified the fix end to end: traced lastDataBlockOffset through flush/close (first block -1, each block pointing at the prior one, header offset 16), confirmed the native reader never reads prev-offset, and confirmed hudi-io main has no hadoop dependency (so the hand-rolled VInt encoder is justified rather than delegating to WritableUtils).
| @@ -150,42 +150,6 @@ public static boolean isNegativeVarLong(byte value) { | |||
| return value < -120 || (value >= -112 && value < 0); | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
writeVarInt shipped as public API in release-1.2.0 (IOUtils is public in the published hudi-io artifact) and the replacement HFileIndexBlock.getVarIntBytes is package-private, so this is a binary-incompatible removal with no deprecation cycle. Suggest keeping a deprecated public delegate here for one release, or calling the removal out explicitly in the PR/release notes if hudi-io internals are considered exempt from API guarantees.
| assertTrue(hbaseReader.getTrailer().getDataIndexCount() > 1, | ||
| "precondition: the file must have multiple data blocks for seekBefore to cross boundaries"); | ||
| HFileScanner scanner = hbaseReader.getScanner(true, true); | ||
| for (int i = 1; i < MULTI_BLOCK_RECORDS; i++) { |
There was a problem hiding this comment.
The loop starts at i=1, so seekBefore(key(0)) -- which must return false via the first block's -1 prev-offset sentinel -- is never exercised through the HBase reader. One assertFalse(scanner.seekBefore(...)) on key(0) before the loop covers the sentinel path end to end.
| // 4. Create a new data block. | ||
| currentDataBlock = HFileDataBlock.createDataBlockToWrite(context, currentOffset); | ||
| // 4. Create a new data block whose previous-block offset points at the block just flushed. | ||
| currentDataBlock = HFileDataBlock.createDataBlockToWrite(context, lastDataBlockOffset); |
There was a problem hiding this comment.
The fix is correct, but the offset is still threaded through a constructor arg at two call sites -- the exact shape that produced the bug. HFileBlock already stores previousBlockOffsetForWrite and the writer already stamps other blocks via setStartOffsetInBuffForWrite; stamping the prev-offset with a setter at flush time from one field (and dropping the constructor param) would make a wrong-variable regression unrepresentable rather than merely tested.
| currentDataBlock.getFirstKey(), lastDataBlockOffset, blockBuffer.limit()); | ||
| // 4. Create a new data block. | ||
| currentDataBlock = HFileDataBlock.createDataBlockToWrite(context, currentOffset); | ||
| // 4. Create a new data block whose previous-block offset points at the block just flushed. |
There was a problem hiding this comment.
Worth a one-line note (here or in hfile_format.md) that only DATA blocks chain prev-offsets: root/meta index, file info, and meta blocks still write -1, while HBase's writer chains every block type. Benign today -- HBase's reader only consults prev-offset for data blocks in seekBefore -- but the intentional divergence is currently undocumented.
| * @return the Protobuf varint encoding. | ||
| * @throws IOException upon error. | ||
| */ | ||
| static byte[] getProtobufVarIntBytes(int value) throws IOException { |
There was a problem hiding this comment.
This helper frames two unrelated sections -- file info AND the trailer (HFileWriterImpl statically imports it, and HFileTrailer owns the read counterpart parseDelimitedFrom) -- so HFileFileInfoBlock is an odd owner. HFileUtils (already home to readMajorVersion) fits better, and gives a consistent convention: hierarchy-scoped encoders on the block hierarchy (getVarIntBytes on HFileIndexBlock is right), cross-section framing in HFileUtils. The same convention would resolve the writeKey/keyValueKeyLength placement comment from the #19071 review.
| kv.getValueOffset(), | ||
| kv.getValueOffset() + kv.getValueLength()) | ||
| ); | ||
| assertEquals(keys[i], reader.getKeyValue().get().getKey().getContentInString()); |
There was a problem hiding this comment.
The deleted testLongKeys also asserted value bytes on this file shape (multi-block, multi-byte-vint index keys); this loop only checks key content. Asserting the value per record restores that coverage in one line.
| for (int i = 0; i < numRecords; i++) { | ||
| String key = longPrefix + String.format("%04d", i); | ||
| writer.append(key, String.format("value%04d", i).getBytes()); | ||
| void writerBlockBytesAreStableFormatLock() throws Exception { |
There was a problem hiding this comment.
Three structure nits: (1) four single-block scenarios plus the multi-block validation run in one @test, so the first failing golden masks the rest -- a @ParameterizedTest over {keys, values, goldens} isolates failures; (2) the direct calls pass dead null, null golden args that only the wrapper ever fills -- moving the golden assert up into the wrapper removes the null plumbing; (3) the helper javadoc cites a 'per-test assertion limit' that doesn't exist in JUnit or this repo's checkstyle -- reword to the real reason (readability).
| return s.getBytes(StandardCharsets.UTF_8); | ||
| } | ||
|
|
||
| private static long readLongBE(byte[] b, int off) { |
There was a problem hiding this comment.
New helpers in this file re-implement existing utilities: readLongBE == IOUtils.readLong (public, already imported here); keyContentString == new Key(bytes).getContentInString() (same package); bytes/utf8 == StringUtils.getUTF8Bytes/fromUTF8Bytes; makeKey hand-rolls a fill loop next to the existing rep(). Same theme as the #19071 comment on indexOf/readIntBE/hex -- worth one sweep.
|
|
||
| @Slf4j | ||
| class TestHFileWriter { | ||
| private static final Logger LOG = LoggerFactory.getLogger(TestHFileWriter.class); |
| size. | ||
| - **Key Length**: [variable-length encoded](https://en.wikipedia.org/wiki/Variable-length_quantity) number representing | ||
| the length of the "Key" part. | ||
| - **Key Length**: the length of the "Key" part, encoded as a Hadoop `WritableUtils` variable-length integer (VInt). A |
There was a problem hiding this comment.
nit: the byte-identical-for-0..127 / diverge-at->=128 rule now appears four times (here, both encoder javadocs, the test javadoc). Suggest keeping this doc as the canonical statement and reducing the javadocs to one-line pointers so the copies can't drift.
3 participants
Describe the issue this Pull Request addresses
Stacked on #19071 (please review that first). Three follow-ups on top of it that bring the native HFile writer in line with the internal mirror:
HFileWriterImplwrote each data block's previous-block-offset header as the block's own offset instead of the previous block's (passedcurrentOffsetrather thanlastDataBlockOffset). The first block is-1, so single-block golden tests never caught it. The native reader is forward-only and ignores the field, but the HBase reader'sHFileScanner.seekBefore()reads it and throwsIllegalStateExceptionat every block boundary on a native-written multi-block file.HFileIndexBlock.getVarIntBytesfor the HadoopWritableUtilsVInt index keyLength,HFileFileInfoBlock.getProtobufVarIntBytesfor the protobuf-delimited trailer / file-info length prefixes), replacing the ambiguously namedgetVariableLengthEncodedBytes.Summary and Changelog
HFileWriterImpl: passlastDataBlockOffset(notcurrentOffset) when starting the next data block.HFileIndexBlock/HFileFileInfoBlock/HFileBlock/IOUtils/HFileRootIndexBlock/HFileMetaIndexBlock: move the two varint encoders onto their block classes; removeHFileBlock.getVariableLengthEncodedBytesandIOUtils.writeVarInt.TestHFileWriter: byte-level layout validation of every section (data, meta, root data index, meta index, file info, trailer); golden + HBase byte-parity for single-block files across single-byte, two-byte and three-byte index keyLengths (116, 500, 2000-char first keys); the data-block previous-block-offset chain check; and VInt encoder unit tests.TestHFileReadCompatibility:hbaseReaderSeekBeforeWorksOnNativeMultiBlockFile, which reads a native-written multi-block file with the HBase reader and assertsseekBefore(key(i))returnskey(i - 1).hfile_format.md: name the index keyLength encoding (HadoopWritableUtilsVInt) and distinguish it from the protobuf-delimited trailer / file-info length prefixes.Impact
Storage-format / cross-reader compatibility: an HBase reader can now
seekBefore(reverse positioning) over native-written multi-block HFiles. No public API change; no change to single-block files or to forward reads / point lookups.Risk Level
low
The prev-offset fix only changes an 8-byte header that the native reader ignores. The encoder move is a pure refactor with no change to on-disk bytes, verified by the golden + HBase byte-parity tests. The new regression tests were confirmed to fail on the pre-fix writer, and the full
hudi-iotest suite passes (111 tests).Documentation Update
The in-repo
hfile_format.mdspec is updated in this PR. No website change.Contributor's checklist