You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@hudi.apache.org by "vinishjail97 (via GitHub)" <gi...@apache.org> on 2026/08/27 10:39:39 UTC

[PR] [MINOR] Guard mixed-class ordering values in event-time record merge [hudi]

vinishjail97 opened a new pull request, #19769:
URL: https://github.com/apache/hudi/pull/19769

   ### Change Logs
   
   `OrderingValues#create` coerces a null ordering-field value to `HoodieRecord.DEFAULT_ORDERING_VALUE`, which is declared `int`. A record whose ordering column is `null` therefore holds `Integer(0)` while its counterpart in the same file group holds the column's real value, e.g. a `Long`. `BufferedRecordMergerFactory.shouldKeepNewerRecord` compared the two with a raw `Comparable#compareTo`, throwing `ClassCastException: java.lang.Integer cannot be cast to java.lang.Long`, which the write path wraps as `HoodieUpsertException`.
   
   The comment above that comparison already notes the hazard, but the guard it annotates applies only to DELETE records, so a non-delete record holding the sentinel was unguarded.
   
   This applies the guard `deltaMergeDeleteRecord` already uses in the same file (also used by `HoodieMergedLogRecordScanner` and `PartialUpdateAvroPayload`): skip the comparison when the incoming ordering value is the default sentinel or the two classes differ, falling back to natural order.
   
   For same-class, non-default ordering values the result is unchanged: `!(old > new)` is `new >= old`. Only the inputs that previously threw behave differently.
   
   ### Impact
   
   No API, config, or default change. Tables whose ordering field is nullable no longer fail merges.
   
   ### Risk level
   
   low
   
   Added two cases to `TestBufferedRecordMergerFactory` covering both mixed-class directions, `deltaMerge` and `finalMerge`, and same-class event-time ordering. They fail with `ClassCastException` against the unfixed method and pass with the fix (`Tests run: 3, Failures: 0, Errors: 0` in `hudi-common`).
   
   ### Documentation Update
   
   None.
   
   ### Contributor's checklist
   
   - [x] Read through [contributor's guide](https://hudi.apache.org/contribute/how-to-contribute)
   - [x] Change Logs and Impact were stated clearly
   - [x] Adequate tests were added if applicable
   - [ ] CI passed
   


-- 
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: commits-unsubscribe@hudi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [MINOR] Guard mixed-class ordering values in event-time record merge [hudi]

Posted by "danny0405 (via GitHub)" <gi...@apache.org>.
danny0405 commented on code in PR #19769:
URL: https://github.com/apache/hudi/pull/19769#discussion_r3871495666


##########
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:
   should we even allows null ordering values for non-deletes records, for records from `DELETE` statement, it is clear since it is indeed natural order or commit time order, but for regular records, we can not decide the ordering with a null ordering value which is ambiguous.
   
   I see some code paths keep the null values(Flink) while some code paths reset nulls as 0(spark), should we just abandon the reset?



-- 
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: commits-unsubscribe@hudi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [MINOR] Guard mixed-class ordering values in event-time record merge [hudi]

Posted by "danny0405 (via GitHub)" <gi...@apache.org>.
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:
   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: commits-unsubscribe@hudi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [MINOR] Guard mixed-class ordering values in event-time record merge [hudi]

Posted by "hudi-bot (via GitHub)" <gi...@apache.org>.
hudi-bot commented on PR #19769:
URL: https://github.com/apache/hudi/pull/19769#issuecomment-5437977950

   <!--
   Meta data
   {
     "version" : 1,
     "metaDataEntries" : [ {
       "hash" : "ee668ce08dfd31ae954bf94fef05d58c9f0b1b99",
       "status" : "PENDING",
       "url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16625",
       "triggerID" : "ee668ce08dfd31ae954bf94fef05d58c9f0b1b99",
       "triggerType" : "PUSH"
     } ]
   }-->
   ## CI report:
   
   * ee668ce08dfd31ae954bf94fef05d58c9f0b1b99 Azure: [PENDING](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16625) 
   
   <details>
   <summary>Bot commands</summary>
     @hudi-bot supports the following commands:
   
    - `@hudi-bot run azure` re-run the last Azure build
   </details>


-- 
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: commits-unsubscribe@hudi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [MINOR] Guard mixed-class ordering values in event-time record merge [hudi]

Posted by "danny0405 (via GitHub)" <gi...@apache.org>.
danny0405 commented on code in PR #19769:
URL: https://github.com/apache/hudi/pull/19769#discussion_r3871440038


##########
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();

Review Comment:
   this is unnecessary, line 510 already did the check.



-- 
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: commits-unsubscribe@hudi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [MINOR] Guard mixed-class ordering values in event-time record merge [hudi]

Posted by "hudi-bot (via GitHub)" <gi...@apache.org>.
hudi-bot commented on PR #19769:
URL: https://github.com/apache/hudi/pull/19769#issuecomment-5437915984

   <!--
   Meta data
   {
     "version" : 1,
     "metaDataEntries" : [ {
       "hash" : "ee668ce08dfd31ae954bf94fef05d58c9f0b1b99",
       "status" : "UNKNOWN",
       "url" : "TBD",
       "triggerID" : "ee668ce08dfd31ae954bf94fef05d58c9f0b1b99",
       "triggerType" : "PUSH"
     } ]
   }-->
   ## CI report:
   
   * ee668ce08dfd31ae954bf94fef05d58c9f0b1b99 UNKNOWN
   
   <details>
   <summary>Bot commands</summary>
     @hudi-bot supports the following commands:
   
    - `@hudi-bot run azure` re-run the last Azure build
   </details>


-- 
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: commits-unsubscribe@hudi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [MINOR] Guard mixed-class ordering values in event-time record merge [hudi]

Posted by "danny0405 (via GitHub)" <gi...@apache.org>.
danny0405 commented on code in PR #19769:
URL: https://github.com/apache/hudi/pull/19769#discussion_r3871495666


##########
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:
   This is not the case, currently we have a special handling for SQL `DELETE` statements to hardcode the ordeirng value as 0, and that is the only special case.
   
   For table that does not declare any ordering values, all the ordering values are assigned as 0 to they got the same class.
   
   In the other cases, the ordering value is just null if the field value itself is null.



-- 
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: commits-unsubscribe@hudi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [MINOR] Guard mixed-class ordering values in event-time record merge [hudi]

Posted by "vinishjail97 (via GitHub)" <gi...@apache.org>.
vinishjail97 closed pull request #19769: [MINOR] Guard mixed-class ordering values in event-time record merge
URL: https://github.com/apache/hudi/pull/19769


-- 
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: commits-unsubscribe@hudi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [MINOR] Guard mixed-class ordering values in event-time record merge [hudi]

Posted by "danny0405 (via GitHub)" <gi...@apache.org>.
danny0405 commented on code in PR #19769:
URL: https://github.com/apache/hudi/pull/19769#discussion_r3871495666


##########
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:
   should we even allows null ordering values for non-deletes records with event time merging, for records from `DELETE` statement, it is clear since it is indeed natural order or commit time order, but for regular records, we can not decide the ordering with a null ordering value which is ambiguous.
   
   I see some code paths keep the null values(Flink) while some code paths reset nulls as 0(spark), should we just abandon the null reset and let it fail fast?



-- 
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: commits-unsubscribe@hudi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [MINOR] Guard mixed-class ordering values in event-time record merge [hudi]

Posted by "hudi-agent (via GitHub)" <gi...@apache.org>.
hudi-agent commented on code in PR #19769:
URL: https://github.com/apache/hudi/pull/19769#discussion_r3870971599


##########
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
+    // not match the counterpart's real value and comparing the two directly would throw
+    // ClassCastException (e.g. Long.compareTo(Integer)).
+    boolean chooseOlder = !OrderingValues.isDefault(newOrderingVal)
+        && OrderingValues.isSameClass(oldOrderingVal, newOrderingVal)
+        && oldOrderingVal.compareTo(newOrderingVal) > 0;
+    return !chooseOlder;

Review Comment:
   πŸ€– nit: `return !chooseOlder` reads as a double negative inside a method already named `shouldKeepNewerRecord` β€” could you rename the variable to `keepOlder` and return `!keepOlder`, or flip it to `keepNewer` and return that directly? Either way reduces the mental flip.
   
   <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: commits-unsubscribe@hudi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [MINOR] Guard mixed-class ordering values in event-time record merge [hudi]

Posted by "hudi-bot (via GitHub)" <gi...@apache.org>.
hudi-bot commented on PR #19769:
URL: https://github.com/apache/hudi/pull/19769#issuecomment-5439634722

   <!--
   Meta data
   {
     "version" : 1,
     "metaDataEntries" : [ {
       "hash" : "ee668ce08dfd31ae954bf94fef05d58c9f0b1b99",
       "status" : "SUCCESS",
       "url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16625",
       "triggerID" : "ee668ce08dfd31ae954bf94fef05d58c9f0b1b99",
       "triggerType" : "PUSH"
     } ]
   }-->
   ## CI report:
   
   * ee668ce08dfd31ae954bf94fef05d58c9f0b1b99 Azure: [SUCCESS](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16625) 
   
   <details>
   <summary>Bot commands</summary>
     @hudi-bot supports the following commands:
   
    - `@hudi-bot run azure` re-run the last Azure build
   </details>


-- 
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: commits-unsubscribe@hudi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [MINOR] Guard mixed-class ordering values in event-time record merge [hudi]

Posted by "danny0405 (via GitHub)" <gi...@apache.org>.
danny0405 commented on code in PR #19769:
URL: https://github.com/apache/hudi/pull/19769#discussion_r3871495666


##########
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:
   should we even allows null ordering values for non-deletes records with event time merging, for records from `DELETE` statement, it is clear since it is indeed natural order or commit time order, but for regular records, we can not decide the ordering with a null ordering value which is ambiguous.
   
   I see some code paths keep the null values(Flink) while some code paths reset nulls as 0(spark), should we just abandon the reset?



-- 
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: commits-unsubscribe@hudi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [MINOR] Guard mixed-class ordering values in event-time record merge [hudi]

Posted by "danny0405 (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@hudi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [MINOR] Guard mixed-class ordering values in event-time record merge [hudi]

Posted by "danny0405 (via GitHub)" <gi...@apache.org>.
danny0405 commented on code in PR #19769:
URL: https://github.com/apache/hudi/pull/19769#discussion_r3871440038


##########
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();

Review Comment:
   this is unnecessary, line 510 already did the check.



-- 
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: commits-unsubscribe@hudi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [MINOR] Guard mixed-class ordering values in event-time record merge [hudi]

Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #19769:
URL: https://github.com/apache/hudi/pull/19769#issuecomment-5438998564

   ## [Codecov](https://app.codecov.io/gh/apache/hudi/pull/19769?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
   :x: Patch coverage is `83.33333%` with `1 line` in your changes missing coverage. Please review.
   :white_check_mark: Project coverage is 77.79%. Comparing base ([`d43fa7f`](https://app.codecov.io/gh/apache/hudi/commit/d43fa7fb63199a11bc0f88707b70694b5021b7a2?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`ee668ce`](https://app.codecov.io/gh/apache/hudi/commit/ee668ce08dfd31ae954bf94fef05d58c9f0b1b99?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
   :warning: Report is 2 commits behind head on master.
   
   | [Files with missing lines](https://app.codecov.io/gh/apache/hudi/pull/19769?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
   |---|---|---|
   | [...common/table/read/BufferedRecordMergerFactory.java](https://app.codecov.io/gh/apache/hudi/pull/19769?src=pr&el=tree&filepath=hudi-common%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fhudi%2Fcommon%2Ftable%2Fread%2FBufferedRecordMergerFactory.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-aHVkaS1jb21tb24vc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvY29tbW9uL3RhYmxlL3JlYWQvQnVmZmVyZWRSZWNvcmRNZXJnZXJGYWN0b3J5LmphdmE=) | 83.33% | [0 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/hudi/pull/19769?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
   
   <details><summary>Additional details and impacted files</summary>
   
   
   
   ```diff
   @@            Coverage Diff            @@
   ##             master   #19769   +/-   ##
   =========================================
     Coverage     77.79%   77.79%           
   - Complexity    33404    33406    +2     
   =========================================
     Files          2539     2539           
     Lines        140982   140987    +5     
     Branches      17018    17021    +3     
   =========================================
   + Hits         109672   109678    +6     
   + Misses        23659    23658    -1     
     Partials       7651     7651           
   ```
   
   | [Components](https://app.codecov.io/gh/apache/hudi/pull/19769/components?src=pr&el=components&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Ξ” | |
   |---|---|---|
   | [hudi-common](https://app.codecov.io/gh/apache/hudi/pull/19769/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `83.50% <83.33%> (+<0.01%)` | :arrow_up: |
   | [hudi-client](https://app.codecov.io/gh/apache/hudi/pull/19769/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `83.08% <ΓΈ> (ΓΈ)` | |
   | [hudi-flink](https://app.codecov.io/gh/apache/hudi/pull/19769/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `85.62% <ΓΈ> (ΓΈ)` | |
   | [hudi-spark-datasource](https://app.codecov.io/gh/apache/hudi/pull/19769/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `72.38% <ΓΈ> (ΓΈ)` | |
   | [hudi-utilities](https://app.codecov.io/gh/apache/hudi/pull/19769/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `72.11% <ΓΈ> (ΓΈ)` | |
   | [hudi-cli](https://app.codecov.io/gh/apache/hudi/pull/19769/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `15.06% <ΓΈ> (ΓΈ)` | |
   | [hudi-hadoop](https://app.codecov.io/gh/apache/hudi/pull/19769/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `69.23% <ΓΈ> (ΓΈ)` | |
   | [hudi-sync](https://app.codecov.io/gh/apache/hudi/pull/19769/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `75.54% <ΓΈ> (ΓΈ)` | |
   | [hudi-io](https://app.codecov.io/gh/apache/hudi/pull/19769/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `79.85% <ΓΈ> (ΓΈ)` | |
   | [hudi-timeline-service](https://app.codecov.io/gh/apache/hudi/pull/19769/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `83.44% <ΓΈ> (ΓΈ)` | |
   | [hudi-cloud](https://app.codecov.io/gh/apache/hudi/pull/19769/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `64.27% <ΓΈ> (ΓΈ)` | |
   | [hudi-kafka-connect](https://app.codecov.io/gh/apache/hudi/pull/19769/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `53.20% <ΓΈ> (ΓΈ)` | |
   
   | [Flag](https://app.codecov.io/gh/apache/hudi/pull/19769/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Ξ” | |
   |---|---|---|
   | [hadoop-mr-java-client](https://app.codecov.io/gh/apache/hudi/pull/19769/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `43.68% <83.33%> (-0.07%)` | :arrow_down: |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   | [Files with missing lines](https://app.codecov.io/gh/apache/hudi/pull/19769?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Ξ” | |
   |---|---|---|
   | [...common/table/read/BufferedRecordMergerFactory.java](https://app.codecov.io/gh/apache/hudi/pull/19769?src=pr&el=tree&filepath=hudi-common%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fhudi%2Fcommon%2Ftable%2Fread%2FBufferedRecordMergerFactory.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-aHVkaS1jb21tb24vc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvY29tbW9uL3RhYmxlL3JlYWQvQnVmZmVyZWRSZWNvcmRNZXJnZXJGYWN0b3J5LmphdmE=) | `89.67% <83.33%> (-0.33%)` | :arrow_down: |
   
   ... and [1 file with indirect coverage changes](https://app.codecov.io/gh/apache/hudi/pull/19769/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
   </details>
   <details><summary> :rocket: New features to boost your workflow: </summary>
   
   - :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
   - :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
   </details>


-- 
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: commits-unsubscribe@hudi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org