QuakeWang commented on code in PR #588:
URL: https://github.com/apache/paimon-rust/pull/588#discussion_r3631879779
##########
crates/paimon/src/table/aggregator/numeric.rs:
##########
@@ -438,12 +438,18 @@ fn agg_minmax(
($acc:expr, $ty:ty) => {{
let v = downcast::<$ty>(array, field_name)?.value(row_idx);
// Match Java `Float.compare` / `Double.compare`, which order NaN
- // greater than any other value (including +Infinity). Using
- // `total_cmp` makes that ordering explicit and deterministic.
+ // greater than any other value (including +Infinity) and compare
+ // all NaN representations as equal. For non-NaN values,
+ // `total_cmp` preserves Java's ordering of -0.0 before +0.0.
*$acc = Some(match *$acc {
None => v,
Some(prev) => {
- let cmp = v.total_cmp(&prev);
+ let cmp = match (v.is_nan(), prev.is_nan()) {
+ (true, true) => std::cmp::Ordering::Equal,
Review Comment:
After different NaN representations compare as equal here, both `min` and
`max` retain the existing accumulator, but this only matches Java's
`FieldMaxAgg`. Java's `FieldMinAgg` returns the new `inputField` when values
compare as equal, so for consecutive NaNs with different bit patterns, Java
`min` retains the latter while this implementation retains the former.
--
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]
