Copilot commented on code in PR #19587:
URL: https://github.com/apache/datafusion/pull/19587#discussion_r2655800075
##########
datafusion/common/src/scalar/mod.rs:
##########
@@ -568,10 +568,15 @@ impl PartialEq for ScalarValue {
impl PartialOrd for ScalarValue {
fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
use ScalarValue::*;
+
+ if self.is_null() || other.is_null() {
+ return None;
+ }
// This purposely doesn't have a catch-all "(_, _)" so that
// any newly added enum variant will require editing this list
// or else face a compile error
match (self, other) {
+ (Null, _) | (_, Null) => None,
Review Comment:
This match arm is unreachable. The NULL check at lines 572-574 already
returns `None` for any NULL value (including `ScalarValue::Null` and any
variant with a `None` value like `Int32(None)`). This match arm will never be
executed and should be removed.
```suggestion
```
##########
datafusion/common/src/scalar/mod.rs:
##########
@@ -9348,4 +9350,23 @@ mod tests {
]
);
}
+ #[test]
+ fn scalar_partial_ordering_nulls() {
+ use ScalarValue::*;
+
+ assert_eq!(
+ Int32(Some(3)).partial_cmp(&Int32(None)),
+ None
+ );
+
+ assert_eq!(
+ Int32(None).partial_cmp(&Int32(Some(3))),
+ None
+ );
+
+ assert_eq!(
+ Int32(None).partial_cmp(&Int32(None)),
+ None
+ );
+ }
Review Comment:
The test should also cover the explicit `ScalarValue::Null` variant to
ensure comprehensive NULL handling. Consider adding test cases like
`Null.partial_cmp(&Int32(Some(3)))` and `Null.partial_cmp(&Null)` to verify
that the explicit NULL variant is also handled correctly.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
