Fix NULL semantics in ScalarValue::partial_cmp (fixes #19579)

Author: Brijesh-Thakkar

datafusion/common/src/error.rs

@@ -1243,7 +1243,6 @@ mod test {
     // To pass the test the environment variable RUST_BACKTRACE should be set to 1 to enforce backtrace
     #[cfg(feature = "backtrace")]
     #[test]
-    #[expect(clippy::unnecessary_literal_unwrap)]
     fn test_enabled_backtrace() {
         match std::env::var("RUST_BACKTRACE") {
             Ok(val) if val == "1" => {}

datafusion/common/src/scalar/mod.rs

@@ -569,12 +569,14 @@ impl PartialOrd for ScalarValue {
     fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
         use ScalarValue::*;
 
+        // NULL values are incomparable (SQL tri-state logic)
         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
+        #[expect(unreachable_patterns)]
         match (self, other) {
             (Decimal32(v1, p1, s1), Decimal32(v2, p2, s2)) => {
                 if p1.eq(p2) && s1.eq(s2) {
@@ -727,10 +729,9 @@ impl PartialOrd for ScalarValue {
                 if k1 == k2 { v1.partial_cmp(v2) } else { None }
             }
             (Dictionary(_, _), _) => None,
-            // Nulls are handled by the early return above
-            (Null, _) | (_, Null) => unreachable!(
-                "Nulls are already handled before entering ScalarValue::partial_cmp match"
-            ),
+            // These patterns are unreachable due to the early return above,
+            // but needed for exhaustiveness checking
+            (Null, _) | (_, Null) => unreachable!(),
         }
     }
 }
@@ -4023,15 +4024,15 @@ impl ScalarValue {
         arr1 == &right
     }
 
-   /// Compare two `ScalarValue`s.
+    /// Compare two `ScalarValue`s.
     ///
     /// Returns an error if:
     /// * the values are of incompatible types, or
     /// * either value is NULL.
     ///
     /// This differs from `partial_cmp`, which returns `None` for NULL inputs
     /// instead of an error.
-        pub fn try_cmp(&self, other: &Self) -> Result<Ordering> {
+    pub fn try_cmp(&self, other: &Self) -> Result<Ordering> {
         self.partial_cmp(other).ok_or_else(|| {
             _internal_datafusion_err!("Uncomparable values: {self:?}, {other:?}")
         })
@@ -9361,34 +9362,16 @@ mod tests {
     fn scalar_partial_ordering_nulls() {
         use ScalarValue::*;
 
-        assert_eq!(
-            Int32(Some(3)).partial_cmp(&Int32(None)),
-            None
-        );
+        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(Some(3))), None);
 
-        assert_eq!(
-            Int32(None).partial_cmp(&Int32(None)),
-            None
-        );
+        assert_eq!(Int32(None).partial_cmp(&Int32(None)), None);
 
-        assert_eq!(
-            Null.partial_cmp(&Int32(Some(3))),
-            None
-        );
+        assert_eq!(Null.partial_cmp(&Int32(Some(3))), None);
 
-        assert_eq!(
-            Int32(Some(3)).partial_cmp(&Null),
-            None
-        );
+        assert_eq!(Int32(Some(3)).partial_cmp(&Null), None);
 
-        assert_eq!(
-            Null.partial_cmp(&Null),
-            None
-        );
+        assert_eq!(Null.partial_cmp(&Null), None);
     }
 }