hudi-agent commented on code in PR #19071:
URL: https://github.com/apache/hudi/pull/19071#discussion_r3503860778
##########
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) {
+ return SIZEOF_INT16 + rowLength + KEY_METADATA_SUFFIX_LENGTH;
+ }
+
+ /**
+ * Writes the KeyValue key for a row:
+ * {@code [2-byte rowLen][row][1-byte cfLen=0][8-byte ts=LATEST][1-byte
type=Put]}. See
+ * {@link #keyValueKeyLength(int)} for why the data and index blocks share
this encoding.
+ *
+ * @param out output stream to write to.
+ * @param row buffer holding the row (key content) bytes.
+ * @param offset start of the row within {@code row}; a key may be a view
into a larger buffer.
+ * @param rowLength number of row bytes to write; passed explicitly because
a key's backing array
+ * may be larger than its content length.
+ */
+ protected static void writeKey(DataOutputStream out, byte[] row, int offset,
int rowLength)
+ throws IOException {
Review Comment:
🤖 nit: `DataOutputStream.writeShort(int)` already accepts an `int` and
writes only the low two bytes, so the `(short)` cast is redundant —
`out.writeShort(rowLength)` is a bit clearer and avoids implying the narrowing
truncation is semantically intentional here.
<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:
##########
@@ -339,6 +425,32 @@ private static void assertArrayEquals(byte[] expected,
byte[] actual) {
}
}
+ private static int indexOf(byte[] haystack, byte[] needle) {
+ outer:
+ for (int i = 0; i + needle.length <= haystack.length; i++) {
+ for (int j = 0; j < needle.length; j++) {
+ if (haystack[i + j] != needle[j]) {
+ continue outer;
+ }
+ }
+ return i;
+ }
+ return -1;
+ }
+
+ private static int readIntBE(byte[] b, int off) {
+ return ((b[off] & 0xff) << 24) | ((b[off + 1] & 0xff) << 16)
Review Comment:
🤖 nit: `readIntBE` reimplements what `IOUtils.readInt(byte[], int)` already
does in the Hudi codebase — could you replace this private helper with a direct
call to `IOUtils.readInt` to avoid the local duplicate?
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
hudi-io/src/main/java/org/apache/hudi/io/hfile/HFileRootIndexBlock.java:
##########
@@ -109,15 +108,11 @@ public ByteBuffer getUncompressedBlockDataToWrite()
throws IOException {
outputStream.writeLong(entry.getOffset());
outputStream.writeInt(entry.getSize());
- // Key length + 2 (SIZEOF_INT16 for the 2-byte row key length prefix).
- // Use Hadoop WritableUtils VarInt encoding to match HBase's HFile
format.
- byte[] keyLength = writeVarInt(
- entry.getFirstKey().getLength() + SIZEOF_INT16);
- outputStream.write(keyLength);
- // Key length.
- outputStream.writeShort((short) entry.getFirstKey().getLength());
- // Key.
- outputStream.write(entry.getFirstKey().getBytes());
+ // Use Hadoop WritableUtils VarInt encoding to match HBase's HFile
format and the reader.
+ int kvKeyLength = keyValueKeyLength(entry.getFirstKey().getLength());
Review Comment:
🤖 nit: `entry.getFirstKey()` is called once here (inline, to get
`.getLength()`) and then again on line 114 to assign `firstKey`. Could you
hoist the `Key firstKey = entry.getFirstKey()` extraction to before this line
and use `firstKey.getLength()` here instead? It reads more naturally
top-to-bottom and avoids the double call.
<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]
