Remove PartialOrd implementation for ScalarValue (#7742)

## Summary

This might help with #7699,
but it might not. I made the changes though so here we go.

Removes the `PartialOrd` implementation for `ScalarValue`, ensuring that
we are only able to compare `Scalar`s which carry a `DType`.

## API Changes

Removes the `PartialOrd` impl.

## Testing

Existing tests should suffice since this just moves some logic around.

Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>

Author: connortsui20

vortex-array/public-api.lock

@@ -14368,10 +14368,6 @@ impl core::cmp::PartialEq for vortex_array::scalar::ScalarValue
 
 pub fn vortex_array::scalar::ScalarValue::eq(&self, &vortex_array::scalar::ScalarValue) -> bool
 
-impl core::cmp::PartialOrd for vortex_array::scalar::ScalarValue
-
-pub fn vortex_array::scalar::ScalarValue::partial_cmp(&self, &Self) -> core::option::Option<core::cmp::Ordering>
-
 impl core::convert::From<&[u8]> for vortex_array::scalar::ScalarValue
 
 pub fn vortex_array::scalar::ScalarValue::from(&[u8]) -> Self

vortex-array/src/scalar/scalar_impl.rs

@@ -14,6 +14,7 @@ use vortex_error::vortex_panic;
 use crate::dtype::DType;
 use crate::dtype::NativeDType;
 use crate::dtype::PType;
+use crate::dtype::StructFields;
 use crate::scalar::Scalar;
 use crate::scalar::ScalarValue;
 
@@ -263,6 +264,16 @@ impl Scalar {
     }
 }
 
+/// We implement `Hash` manually to be consistent with `PartialEq`. Since we ignore nullability in
+/// equality comparisons, we must also ignore it when hashing to maintain the invariant that equal
+/// values have equal hashes.
+impl Hash for Scalar {
+    fn hash<H: Hasher>(&self, state: &mut H) {
+        self.dtype.as_nonnullable().hash(state);
+        self.value.hash(state);
+    }
+}
+
 /// We implement `PartialEq` manually because we want to ignore nullability when comparing scalars.
 /// Two scalars with the same value but different nullability should be considered equal.
 ///
@@ -288,7 +299,14 @@ impl PartialOrd for Scalar {
     /// - Non-null values are compared according to their natural ordering
     ///
     /// # Examples
-    /// ```ignore
+    ///
+    /// ```
+    /// use std::cmp::Ordering;
+    /// use vortex_array::dtype::DType;
+    /// use vortex_array::dtype::Nullability;
+    /// use vortex_array::dtype::PType;
+    /// use vortex_array::scalar::Scalar;
+    ///
     /// // Same types compare successfully
     /// let a = Scalar::primitive(10i32, Nullability::NonNullable);
     /// let b = Scalar::primitive(20i32, Nullability::NonNullable);
@@ -308,16 +326,101 @@ impl PartialOrd for Scalar {
         if !self.dtype().eq_ignore_nullability(other.dtype()) {
             return None;
         }
-        self.value().partial_cmp(&other.value())
+
+        partial_cmp_scalar_values(self.dtype(), self.value(), other.value())
     }
 }
 
-/// We implement `Hash` manually to be consistent with `PartialEq`. Since we ignore nullability
-/// in equality comparisons, we must also ignore it when hashing to maintain the invariant that
-/// equal values have equal hashes.
-impl Hash for Scalar {
-    fn hash<H: Hasher>(&self, state: &mut H) {
-        self.dtype.as_nonnullable().hash(state);
-        self.value.hash(state);
+/// Compare two optional scalar values using `dtype` for nested tuple interpretation.
+fn partial_cmp_scalar_values(
+    dtype: &DType,
+    lhs: Option<&ScalarValue>,
+    rhs: Option<&ScalarValue>,
+) -> Option<Ordering> {
+    match (lhs, rhs) {
+        (None, None) => Some(Ordering::Equal),
+        (None, Some(_)) => Some(Ordering::Less),
+        (Some(_), None) => Some(Ordering::Greater),
+        (Some(lhs), Some(rhs)) => partial_cmp_non_null_scalar_values(dtype, lhs, rhs),
+    }
+}
+
+/// Compare two non-null scalar values, consulting `dtype` only for tuple-backed values.
+fn partial_cmp_non_null_scalar_values(
+    dtype: &DType,
+    lhs: &ScalarValue,
+    rhs: &ScalarValue,
+) -> Option<Ordering> {
+    // `Scalar::validate` guarantees that a scalar's value matches its dtype. Most of the scalar
+    // value variants have only 1 method of comparison, regardless of the dtype.
+    match (lhs, rhs) {
+        (ScalarValue::Bool(lhs), ScalarValue::Bool(rhs)) => lhs.partial_cmp(rhs),
+        (ScalarValue::Primitive(lhs), ScalarValue::Primitive(rhs)) => lhs.partial_cmp(rhs),
+        (ScalarValue::Decimal(lhs), ScalarValue::Decimal(rhs)) => lhs.partial_cmp(rhs),
+        (ScalarValue::Utf8(lhs), ScalarValue::Utf8(rhs)) => lhs.partial_cmp(rhs),
+        (ScalarValue::Binary(lhs), ScalarValue::Binary(rhs)) => lhs.partial_cmp(rhs),
+        // `Tuple` is the exception here. Since it backs lists, fixed-size lists, and structs, we
+        // need the dtype to know whether children share one element dtype or use per-field dtypes.
+        (ScalarValue::Tuple(lhs), ScalarValue::Tuple(rhs)) => {
+            partial_cmp_tuple_values(dtype, lhs, rhs)
+        }
+        // Variant values can have a different dtype in each row, so it doesn't make sense to
+        // compare them.
+        (ScalarValue::Variant(_), ScalarValue::Variant(_)) => None,
+        _ => None,
+    }
+}
+
+/// Compare tuple values according to the list, fixed-size list, or struct dtype layout.
+fn partial_cmp_tuple_values(
+    dtype: &DType,
+    lhs: &[Option<ScalarValue>],
+    rhs: &[Option<ScalarValue>],
+) -> Option<Ordering> {
+    match dtype {
+        DType::List(element_dtype, _) | DType::FixedSizeList(element_dtype, ..) => {
+            partial_cmp_list_values(element_dtype, lhs, rhs)
+        }
+        DType::Struct(fields, _) => partial_cmp_struct_values(fields, lhs, rhs),
+        DType::Extension(ext_dtype) => {
+            partial_cmp_tuple_values(ext_dtype.storage_dtype(), lhs, rhs)
+        }
+        _ => None,
+    }
+}
+
+/// Compare list tuple values using the shared element dtype for each element.
+fn partial_cmp_list_values(
+    element_dtype: &DType,
+    lhs: &[Option<ScalarValue>],
+    rhs: &[Option<ScalarValue>],
+) -> Option<Ordering> {
+    for (lhs, rhs) in lhs.iter().zip(rhs.iter()) {
+        match partial_cmp_scalar_values(element_dtype, lhs.as_ref(), rhs.as_ref())? {
+            Ordering::Equal => continue,
+            ordering => return Some(ordering),
+        }
+    }
+
+    Some(lhs.len().cmp(&rhs.len()))
+}
+
+/// Compare struct tuple values using each field's dtype in field order.
+fn partial_cmp_struct_values(
+    fields: &StructFields,
+    lhs: &[Option<ScalarValue>],
+    rhs: &[Option<ScalarValue>],
+) -> Option<Ordering> {
+    if lhs.len() != fields.nfields() || rhs.len() != fields.nfields() {
+        return None;
     }
+
+    for ((field_dtype, lhs), rhs) in fields.fields().zip(lhs.iter()).zip(rhs.iter()) {
+        match partial_cmp_scalar_values(&field_dtype, lhs.as_ref(), rhs.as_ref())? {
+            Ordering::Equal => continue,
+            ordering => return Some(ordering),
+        }
+    }
+
+    Some(Ordering::Equal)
 }

vortex-array/src/scalar/scalar_value.rs

@@ -3,7 +3,6 @@
 
 //! Core [`ScalarValue`] type definition.
 
-use std::cmp::Ordering;
 use std::fmt::Display;
 use std::fmt::Formatter;
 
@@ -111,22 +110,6 @@ impl ScalarValue {
     }
 }
 
-impl PartialOrd for ScalarValue {
-    fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
-        match (self, other) {
-            (ScalarValue::Bool(a), ScalarValue::Bool(b)) => a.partial_cmp(b),
-            (ScalarValue::Primitive(a), ScalarValue::Primitive(b)) => a.partial_cmp(b),
-            (ScalarValue::Decimal(a), ScalarValue::Decimal(b)) => a.partial_cmp(b),
-            (ScalarValue::Utf8(a), ScalarValue::Utf8(b)) => a.partial_cmp(b),
-            (ScalarValue::Binary(a), ScalarValue::Binary(b)) => a.partial_cmp(b),
-            (ScalarValue::Tuple(a), ScalarValue::Tuple(b)) => a.partial_cmp(b),
-            (ScalarValue::Variant(a), ScalarValue::Variant(b)) => a.partial_cmp(b),
-            // (ScalarValue::Extension(a), ScalarValue::Extension(b)) => a.partial_cmp(b),
-            _ => None,
-        }
-    }
-}
-
 impl Display for ScalarValue {
     fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
         match self {