danny0405 commented on code in PR #19502:
URL: https://github.com/apache/hudi/pull/19502#discussion_r3710867567
##########
hudi-client/hudi-spark-client/src/main/java/org/apache/hudi/table/action/commit/BaseSparkCommitActionExecutor.java:
##########
@@ -327,12 +327,11 @@ protected HoodieData<WriteStatus>
mapPartitionsAsRDD(HoodieData<HoodieRecord<T>>
if (table.requireSortedRecords()) {
// Partition and sort within each partition as a single step. This is
faster than partitioning first and then
// applying a sort.
- // 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.
+ Comparator<String> recordKeyComparator =
HoodieRecordUtils.getRecordKeyComparator(table.getBaseFileFormat());
Comparator<Tuple2<HoodieKey, Option<HoodieRecordLocation>>> comparator =
(Comparator<Tuple2<HoodieKey, Option<HoodieRecordLocation>>> & Serializable)
(t1, t2) -> {
HoodieKey key1 = t1._1;
HoodieKey key2 = t2._1;
- return StringUtils.compareUtf8Bytes(key1.getRecordKey(),
key2.getRecordKey());
Review Comment:
For Hudi’s new LSM layout, use **unsigned lexicographic UTF-8 byte order**
as the table-level record-key ordering contract.
This is the stronger choice than Java `String.compareTo()` because:
- LSM runs are persisted as Parquet strings, and Parquet defines `STRING`
ordering as unsigned byte-wise ordering of UTF-8. [[Parquet logical
types](https://parquet.apache.org/docs/file-format/types/logicaltypes/)](https://parquet.apache.org/docs/file-format/types/logicaltypes/)
- It is portable across Spark, Flink, Java, and future non-JVM
implementations.
- It matches the comparator Hudi already introduced in
[StringUtils.java](/Users/chenyuzhao/workspace/hudi-dev/hudi-io/src/main/java/org/apache/hudi/common/util/StringUtils.java:125).
- The Spark sorted-write path already uses UTF-8 ordering in
[BaseSparkCommitActionExecutor.java](/Users/chenyuzhao/workspace/hudi-dev/hudi-client/hudi-spark-client/src/main/java/org/apache/hudi/table/action/commit/BaseSparkCommitActionExecutor.java:327).
- `requireSortedRecords()` now covers both HFile and LSM tables in
[HoodieTable.java](/Users/chenyuzhao/workspace/hudi-dev/hudi-client/hudi-client-common/src/main/java/org/apache/hudi/table/HoodieTable.java:1055).
The current LSM implementation is inconsistent. These two paths still use
Java UTF-16 ordering:
-
[LsmFileGroupReaderBasedMergeHandle.java](/Users/chenyuzhao/workspace/hudi-dev/hudi-client/hudi-client-common/src/main/java/org/apache/hudi/io/LsmFileGroupReaderBasedMergeHandle.java:62)
-
[LsmFileGroupRecordIterator.java](/Users/chenyuzhao/workspace/hudi-dev/hudi-common/src/main/java/org/apache/hudi/common/table/read/lsm/LsmFileGroupRecordIterator.java:473)
They should use:
```java
Comparator.comparing(
HoodieRecord::getRecordKey,
StringUtils.UTF8_LEXICOGRAPHIC_COMPARATOR)
```
and:
```java
int keyCompare = StringUtils.compareUtf8Bytes(
left.current.getRecordKey(),
right.current.getRecordKey());
```
I would define the invariant as:
> Every LSM run is strictly ordered by the unsigned UTF-8 representation of
`_hoodie_record_key`; all writers, range partitioners, merge readers,
compaction paths, and key-range metadata must use that same ordering.
Also audit the Spark bulk-insert partitioners: several still use
`String.compareTo()`, including the bucket-index path. For composite routing,
compare `(partition path/file group, record key)` as a tuple, applying UTF-8
comparison to each string component—don’t sort a concatenated string.
Add an end-to-end test using keys whose orders differ:
```text
U+E000
U+20000
```
Expected UTF-8/Parquet order:
```text
U+E000 < U+20000
```
That test should cover writing separate L0 runs, LSM k-way merge, compaction
into L1, and reading the compacted result.
--
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]
