nsivabalan commented on PR #18941:
URL: https://github.com/apache/hudi/pull/18941#issuecomment-4936978923
🚨 **BLOCKING — one more MDT sort site still on `String.compareTo`; same
failure mode as the ones fixed in this PR.**
`HoodieSortedMergeHandle` is on the MDT merge/compaction path when the data
table has binary / non-ASCII record keys, and it still orders keys by UTF-16:
- `HoodieSortedMergeHandle.java:50` — `newRecordKeysSorted = new
PriorityQueue<>()` uses natural (UTF-16) ordering.
- `HoodieSortedMergeHandle.java:81` —
`newRecordKeysSorted.peek().compareTo(key) <= 0` compares the (UTF-16-ordered)
new-insert queue head against a base-HFile key that's in UTF-8 byte order.
This class *is* on the MDT path:
`HoodieMergeHandleFactory.getMergeHandleClassesWrite` (L173) and
`getMergeHandleClassesCompaction` (L205) both unconditionally override to
`HoodieSortedMergeHandle` whenever `table.requireSortedRecords()` is true —
which is exactly the HFile / MDT condition. So the default
`FileGroupReaderBasedMergeHandle` is bypassed on this path, and
`HoodieSortedMergeHandle` is what actually runs for MDT merges/compactions.
For data-table record keys in binary / non-ASCII format (the exact case this
PR targets), the failure mode matches the ones already fixed here:
- New-insert queue head sorts *before* a base-file key in UTF-16 but *after*
it in UTF-8. The `while (peek.compareTo(key) <= 0)` loop drains the queue past
the matching base key → either the "Added a key not lexically larger than
previous" write failure at HFile flush, or a silent corruption where the wrong
file-group location is persisted for that record key in the RI.
Fix is two lines, same shape as `SortedKeyBasedFileGroupRecordBuffer`:
```java
// L50
private final Queue<String> newRecordKeysSorted =
new PriorityQueue<>(StringUtils.UTF8_LEXICOGRAPHIC_COMPARATOR);
// L81
while (!newRecordKeysSorted.isEmpty()
&& StringUtils.compareUtf8Bytes(newRecordKeysSorted.peek(), key) <= 0) {
```
Also worth adding a test in the same shape as
`testRecordIndexBootstrapWithBinaryRecordKeysAfterCompaction`, but exercising
the non-FileGroupReader merge handle path (or asserting that the same test with
`FileGroupReaderBasedMergeHandle` disabled still passes). Otherwise the same
class of bug re-emerges here after the first MDT merge on a table with binary
record keys.
Rest of the fixes on this PR look good; just need this one MDT sort site
covered too so the PR is complete on all paths where `requireSortedRecords()`
gates MDT writes.
--
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]
