nsivabalan commented on code in PR #18941:
URL: https://github.com/apache/hudi/pull/18941#discussion_r3532104318
##########
hudi-common/src/main/java/org/apache/hudi/metadata/HoodieBackedTableMetadata.java:
##########
@@ -258,7 +259,10 @@ public HoodieData<HoodieRecord<HoodieMetadataPayload>>
getRecordsByKeyPrefixes(
private static TreeSet<String>
getDistinctSortedKeysForSingleSlice(HoodieData<String> keys) {
List<String> keysList = keys.collectAsList();
- return new TreeSet<>(keysList);
+ // Order by UTF-8 bytes to match the HFile order used for point lookups.
+ TreeSet<String> sortedKeys = new
TreeSet<>(StringUtils.UTF8_LEXICOGRAPHIC_COMPARATOR);
+ sortedKeys.addAll(keysList);
Review Comment:
**🚨 BLOCKING** -- multi-slice RI lookup path likely needs the same fix.
The single-slice path is now correct via
`getDistinctSortedKeysForSingleSlice`, but the multi-file-group path below
(line ~308-327) goes through:
```java
getEngineContext().mapGroupsByKey(persistedInitialPairData, processFunction,
keySpace, true);
```
The underlying `rangeBasedRepartitionForEachKey` (in
`HoodieSparkEngineContext.java:393-410`) uses
`ConditionalRangePartitioner.CompositeKeyComparator`, whose `compare` (line
94-103) is:
```java
int cmp = o1._1().compareTo(o2._1()); // file group index (Integer)
if (cmp != 0) { return cmp; }
return o1._2().compareTo(o2._2()); // encoded record key (String) --
UTF-16 order
```
So within each file group, `sortedKeys` arrives at `processFunction` in
UTF-16 order, `keysList` preserves that order, and the downstream
`lookupRecordsItr` → `Predicates.in` → `RecordByKeyIterator.seekTo` does the
same forward-only HFile seek on UTF-16-ordered keys → same silent-miss bug for
supplementary-char keys.
Any RI table that scales past `withRecordIndexFileGroupCount.min` hits this
path in steady state, so I don't think it can wait for a follow-up.
Simplest fix: inside `processFunction`, resort `keysList` with
`StringUtils.UTF8_LEXICOGRAPHIC_COMPARATOR` before passing to
`lookupRecordsItr`. Safe because `mapRecordKeyToFileGroupIndex` only hashes,
doesn't depend on ordering. (Pushing UTF-8 ordering down into the
`CompositeKeyComparator` would be a much larger change.)
The new `testRecordIndexBootstrapWithBinaryRecordKeys` pins to 1 file group
(`withRecordIndexFileGroupCount(1, 1)`), so this path is currently uncovered.
##########
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);
Review Comment:
**⚠️ IMPORTANT** -- log-block write is fixed here, but downstream MDT
compaction / merge still sorts by `String` order and would re-hit the same
failure on the first RI compaction after bootstrap.
`HoodieTable.requireSortedRecords()` returns `true` for any HFile base
format (the MDT is HFile). Three sort sites on the MDT create/merge path are
still on `String.compareTo`:
1. `BaseCreateHandle.java:135` (the `requireSortedRecords()` branch):
```java
keyIterator = recordMap.keySet().stream().sorted().iterator();
```
This is what `HoodieCreateHandle` uses when it emits a new MDT HFile
during compaction. For supplementary-codepoint keys this will throw `"Added a
key not lexically larger than previous"` -- exactly the failure the PR fixes on
the bootstrap side.
2. `SortedKeyBasedFileGroupRecordBuffer.java:61`, `:68`, and `:105` -- the
buffer selected by `DefaultFileGroupRecordBufferLoader` /
`StreamingFileGroupRecordBufferLoader` when `withSortOutput(true)` (set in
`FileGroupReaderBasedMergeHandle.java:347` based on `requireSortedRecords()`).
Line 61 sorts log records by `String` order; line 68's
`peek().compareTo(recordKey)` compares in `String` order against a base file
already written in UTF-8 order. On supplementary-char keys the two orderings
disagree, so the log-key priority queue can advance past a matching base record
-- silently dropping a log entry from the merge output. That's a
data-correctness issue (wrong RI mapping propagates to query time), not just a
write-side crash.
3. `HoodieRecordUtils.java:232` (`sortRecordsByRecordKey`) -- generic helper
on the same pattern; worth checking callers before/after fixing.
All three are single-line swaps: `.sorted()` →
`.sorted(UTF8_LEXICOGRAPHIC_COMPARATOR)`, and `compareTo` →
`StringUtils.compareUtf8Bytes`.
Happy to leave this as a follow-up JIRA if you'd rather keep this PR
bootstrap-scoped -- but in that case the PR title/description should make clear
the fix is bootstrap-only and the next compaction re-breaks the RI, so
operators know to watch for it.
##########
hudi-spark-datasource/hudi-spark/src/test/java/org/apache/hudi/functional/TestHoodieBackedMetadata.java:
##########
@@ -1944,6 +1945,88 @@ public void testClusteringWithRecordIndex() throws
Exception {
testTableOperationsForMetaIndexImpl(writeConfig);
}
+ /**
+ * Record index bootstrap over binary / non-ASCII record keys must succeed.
The RI HFile orders
+ * keys by their raw UTF-8 bytes, so the bulk-insert partitioner must sort
by UTF-8 bytes too;
+ * sorting by {@link String#compareTo(String)} (UTF-16) fails with
+ * "Added a key not lexically larger than previous".
+ */
+ @Test
+ public void testRecordIndexBootstrapWithBinaryRecordKeys() throws Exception {
Review Comment:
**⚠️ IMPORTANT** -- great reproducer; the interleaved U+E000 / U+20000
prefixes are exactly the pathological shape (UTF-16 order reverses UTF-8
order). Two coverage gaps worth closing so this test suite would catch the two
blockers I flagged inline:
1. **Multi-slice lookup coverage.** This test pins the RI to 1 file group
(`withRecordIndexFileGroupCount(1, 1)`), so it never exercises the
`mapGroupsByKey` path in `HoodieBackedTableMetadata`. A near-clone with
`(min=4, max=4)` would cover the multi-slice concern.
2. **Post-compaction coverage.** Add another upsert batch after the
bootstrap batch, force an MDT compaction (e.g.
`hoodie.metadata.compact.max.delta.commits` set low), and re-run the same
"every key resolves" assertion. This would catch the `BaseCreateHandle` /
`SortedKeyBasedFileGroupRecordBuffer` sort sites.
Both should reuse `generateRecordsWithBinaryKeys` -- nothing new to build.
Also consider a direct `TestStringUtils` unit test for the comparator:
- ASCII pair: assert result equals `String.compareTo`
- supplementary pair: assert order flips vs `String.compareTo`
- empty string / prefix pair / identical strings
Pins down the comparator independent of the RI plumbing.
##########
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:
**💬 SUGGESTION** -- style consistency. `HoodieInlineLogAppendHandle` uses
the named-comparator form:
```java
Comparator.comparing(HoodieRecord::getRecordKey,
StringUtils.UTF8_LEXICOGRAPHIC_COMPARATOR)
```
Matching that here makes it obvious the two sites do the same comparison:
```java
records.sort(Comparator.comparing(r -> r.getKey().getRecordKey(),
StringUtils.UTF8_LEXICOGRAPHIC_COMPARATOR));
```
Purely stylistic; take it or leave it.
##########
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:
Two small notes on this method:
**⚠️ IMPORTANT -- null handling.** `getUTF8Bytes(null)` will NPE. Record
keys are non-null by convention, but since `UTF8_LEXICOGRAPHIC_COMPARATOR` is a
public constant in a general utility class, some future caller will hand it
null. Either document "non-null args" on the method Javadoc, or match
`String.compareTo`'s null semantics (both throw NPE, so documenting is
sufficient).
**💅 NIT -- Javadoc direction.** The method Javadoc currently redirects to
the constant, but the method is called at named sites
(`JavaHoodieMetadataBulkInsertPartitioner`,
`SparkHoodieMetadataBulkInsertPartitioner`). A reader stopping on the method
wants the "why HFile, why not `String.compareTo`" rationale here. Consider
flipping: put the detailed HBase-`CellComparatorImpl` explanation on the
method, and have the constant say "Serializable wrapper around
`compareUtf8Bytes`; see that method for rationale."
--
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]
