hudi-agent commented on code in PR #18941:
URL: https://github.com/apache/hudi/pull/18941#discussion_r3484988375
##########
hudi-client/hudi-java-client/src/main/java/org/apache/hudi/metadata/JavaHoodieMetadataBulkInsertPartitioner.java:
##########
@@ -39,7 +39,7 @@ public List<HoodieRecord<T>>
repartitionRecords(List<HoodieRecord<T>> records, i
if (records.isEmpty()) {
return records;
}
- records.sort(Comparator.comparing(record ->
record.getKey().getRecordKey()));
+ records.sort((r1, r2) ->
StringUtils.compareUtf8Bytes(r1.getKey().getRecordKey(),
r2.getKey().getRecordKey()));
Review Comment:
🤖 nit: could you use `Comparator.comparing(r -> r.getKey().getRecordKey(),
StringUtils.UTF8_LEXICOGRAPHIC_COMPARATOR)` here instead of the raw lambda?
`HoodieInlineLogAppendHandle` uses the named-comparator form
(`Comparator.comparing(HoodieRecord::getRecordKey,
StringUtils.UTF8_LEXICOGRAPHIC_COMPARATOR)`), and keeping the same style across
both sort sites makes it clearer they're doing the same comparison.
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
hudi-io/src/main/java/org/apache/hudi/common/util/StringUtils.java:
##########
@@ -120,6 +122,31 @@ public static byte[] getUTF8Bytes(String str) {
return str.getBytes(StandardCharsets.UTF_8);
}
+ /**
+ * Serializable comparator ordering strings by their unsigned UTF-8 byte
representation, matching
+ * the ordering HFiles enforce (HBase's {@code CellComparatorImpl}). Unlike
+ * {@link String#compareTo(String)} (UTF-16), this stays consistent with
HFile ordering for
+ * non-ASCII / binary keys.
+ */
+ public static final Comparator<String> UTF8_LEXICOGRAPHIC_COMPARATOR =
+ (Comparator<String> & Serializable) StringUtils::compareUtf8Bytes;
+
+ /**
+ * Compares two strings by their unsigned UTF-8 byte order. See {@link
#UTF8_LEXICOGRAPHIC_COMPARATOR}.
+ */
+ public static int compareUtf8Bytes(String s1, String s2) {
+ byte[] b1 = getUTF8Bytes(s1);
Review Comment:
🤖 nit: `compareUtf8Bytes` allocates two `byte[]` arrays on every call, so
it's not cheap inside a sort comparator over large key sets. It might be worth
a one-line note in the Javadoc — something like "Note: allocates two byte
arrays per call; prefer the `UTF8_LEXICOGRAPHIC_COMPARATOR` constant when the
comparator object can be reused" — so callers in hot paths are aware.
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/io/HoodieInlineLogAppendHandle.java:
##########
@@ -321,7 +322,9 @@ protected HoodieLogBlock getDataBlock(HoodieWriteConfig
writeConfig,
case HFILE_DATA_BLOCK:
// Not supporting positions in HFile data blocks
header.remove(HeaderMetadataType.BASE_FILE_INSTANT_TIME_OF_RECORD_POSITIONS);
- records.sort(Comparator.comparing(HoodieRecord::getRecordKey));
+ // HFile orders keys by their raw UTF-8 bytes, so sort by UTF-8 bytes
rather than
+ // String (UTF-16) order to keep non-ASCII / binary keys consistent
with the writer.
+ records.sort(Comparator.comparing(HoodieRecord::getRecordKey,
StringUtils.UTF8_LEXICOGRAPHIC_COMPARATOR));
Review Comment:
🤖 The log-block write order is fixed here, but the compaction/merge that
later consumes these blocks for MDT HFiles still seems to sort by `String`
order. `SortedKeyBasedFileGroupRecordBuffer` (selected when
`requireSortedRecords()` is true, i.e. HFile/MDT) merges log keys via
`Comparator.comparing(getRecordKey)` / `.compareTo` against a UTF-8-ordered
base file, and `BaseCreateHandle` does `keySet().stream().sorted()`. For
supplementary-char keys (e.g. U+20000 vs U+E000, whose String and UTF-8 orders
reverse — exactly the keys in the new test), won't the first MDT compaction
emit records out of UTF-8 order and fail/corrupt the new base HFile? Is
covering the compaction path intended as a follow-up, or should it be part of
this fix? @nsivabalan
<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]
