hudi-agent commented on code in PR #18941:
URL: https://github.com/apache/hudi/pull/18941#discussion_r3565783858
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/io/BaseCreateHandle.java:
##########
@@ -131,8 +132,10 @@ protected void doWrite(HoodieRecord record, HoodieSchema
schema, TypedProperties
public void write() {
Iterator<String> keyIterator;
if (hoodieTable.requireSortedRecords()) {
- // Sorting the keys limits the amount of extra memory required for
writing sorted records
- keyIterator = recordMap.keySet().stream().sorted().iterator();
+ // Sorting the keys limits the amount of extra memory required for
writing sorted records.
+ // requireSortedRecords() is true only for HFile base files, which order
keys by UTF-8 bytes,
+ // not String (UTF-16) order, so sort with the matching comparator.
+ keyIterator =
recordMap.keySet().stream().sorted(StringUtils.UTF8_LEXICOGRAPHIC_COMPARATOR).iterator();
Review Comment:
🤖 This fixes the sorted create/insert path, but the sibling sorted *merge*
path — `HoodieSortedMergeHandle` (`new PriorityQueue<>()` at line 50,
`newRecordKeysSorted.peek().compareTo(key)` at line 81) — is selected by the
exact same `requireSortedRecords()` gate (via `HoodieMergeHandleFactory`) yet
still orders new keys by `String`/UTF-16. For an HFILE base-file table with
non-ASCII keys, a follow-up upsert/compaction merge would still emit keys out
of HFile order and hit "Added a key not lexically larger than previous". Is
that merge path meant to be in scope here too? @nsivabalan
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
hudi-client/hudi-java-client/src/main/java/org/apache/hudi/metadata/JavaHoodieMetadataBulkInsertPartitioner.java:
##########
@@ -39,7 +40,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(Comparator.comparing(r -> r.getKey().getRecordKey(),
StringUtils.UTF8_LEXICOGRAPHIC_COMPARATOR));
Review Comment:
🤖 nit: could you keep the `record` lambda param name instead of shortening
it to `r`? The original read a bit clearer and the rename isn't needed for the
comparator change.
<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]
