hudi-agent commented on code in PR #19637:
URL: https://github.com/apache/hudi/pull/19637#discussion_r3786481912
##########
hudi-common/src/main/java/org/apache/hudi/common/util/OrderingValues.java:
##########
@@ -117,4 +120,10 @@ public static List<Comparable> getValues(ArrayComparable
orderingValue) {
public static boolean isCommitTimeOrderingValue(Comparable orderingValue) {
return orderingValue == null || OrderingValues.isDefault(orderingValue);
}
+
+ // Replaces a null field value (e.g. a nullable ordering column) with the
default ordering value (0)
Review Comment:
🤖 nit: `handleNullValue` is a bit vague — "handle" could mean anything.
Could you rename it to something that states the outcome, like `nullToDefault`
or `coerceNullToDefault`?
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
hudi-common/src/main/java/org/apache/hudi/common/util/OrderingValues.java:
##########
@@ -117,4 +120,10 @@ public static List<Comparable> getValues(ArrayComparable
orderingValue) {
public static boolean isCommitTimeOrderingValue(Comparable orderingValue) {
return orderingValue == null || OrderingValues.isDefault(orderingValue);
}
+
+ // Replaces a null field value (e.g. a nullable ordering column) with the
default ordering value (0)
+ // so that downstream compareTo calls never throw NullPointerException.
+ private static Comparable handleNullValue(Comparable value) {
+ return value != null ? value : DEFAULT_VALUE;
+ }
Review Comment:
🤖 Coercing null to DEFAULT_VALUE (Integer 0) fixes the all-null case, but
does it actually resolve the mixed case? In
`BufferedRecordMergerFactory.shouldKeepNewerRecord` the non-delete path
(`newRecord.getOrderingValue().compareTo(oldRecord.getOrderingValue())`) has no
`isSameClass` guard — only the delete paths do. So a record whose nullable
ordering field is null (→ Integer 0) merging against one with a real `Long`
value would throw ClassCastException (Integer vs Long) instead of the previous
NPE. The test's note says this is guarded via `isSameClass`, but I don't see
that guard on this comparison. @yihua could you confirm whether the mixed
null/non-null event-time-ordering case is actually covered?
<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]
