danny0405 commented on code in PR #19769:
URL: https://github.com/apache/hudi/pull/19769#discussion_r3871798864
##########
hudi-common/src/main/java/org/apache/hudi/common/table/read/BufferedRecordMergerFactory.java:
##########
@@ -512,6 +512,16 @@ private static <T> boolean
shouldKeepNewerRecord(BufferedRecord<T> oldRecord, Bu
// The orderingVal is constant 0 (int) and not guaranteed to match the
type of the old or new record's ordering value.
return true;
}
- return
newRecord.getOrderingValue().compareTo(oldRecord.getOrderingValue()) >= 0;
+ Comparable oldOrderingVal = oldRecord.getOrderingValue();
+ Comparable newOrderingVal = newRecord.getOrderingValue();
+ // Checks the ordering value does not equal to 0
+ // because we use 0 as the default value which means natural order.
+ // OrderingValues#create coerces a null ordering field value to that
default, so its class need
Review Comment:
Agree. I think the default `Integer(0)` should remain specific to records
produced by a `DELETE` statement, while ordinary records should preserve a null
ordering value instead of silently converting it to the sentinel.
Suggested changes:
- In
[`RecordContext#getOrderingValue`](https://github.com/apache/hudi/blob/master/hudi-common/src/main/java/org/apache/hudi/common/engine/RecordContext.java#L352-L411),
return `null` when the ordering-field list is empty and preserve `null` when a
configured field is null.
- Apply the same behavior to
[`HoodieSparkRecord#doGetOrderingValue`](https://github.com/apache/hudi/blob/master/hudi-client/hudi-spark-client/src/main/java/org/apache/hudi/common/model/HoodieSparkRecord.java#L360-L384),
aligning it with the Flink and Avro record paths.
- Make the Spark, Flink, and common ordering-value converters return `null`
immediately when their input is null.
- Keep the DELETE path unchanged:
[`HoodieEmptyRecord`](https://github.com/apache/hudi/blob/master/hudi-common/src/main/java/org/apache/hudi/common/model/HoodieEmptyRecord.java#L37-L50)
continues assigning `0`, and
[`BufferedRecord#isCommitTimeOrderingDelete`](https://github.com/apache/hudi/blob/master/hudi-common/src/main/java/org/apache/hudi/common/table/read/BufferedRecord.java#L65-L67)
can remain sentinel-based.
- Continue recognizing legacy `0` sentinels when reading older records.
- Add coverage for AVRO/SPARK/FLINK records, commit-time records with null
ordering values, event-time records preserving null, DELETE records retaining
`0`, and null buffer serialization.
With that model, the mixed-class state addressed here is removed at its
source:
```text
ordinary record with no ordering value -> null
DELETE statement record -> Integer(0)
legacy sentinel -> still supported on read
```
I would therefore avoid adding a general mixed-class natural-order fallback
in `shouldKeepNewerRecord`; it could silently accept an ambiguous event-time
record instead of allowing the invalid ordering value to fail when a comparison
is required.
--
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]
