bug[decimal-byte-parts]: unconvertible scalar in compare (#3578)

Signed-off-by: Joe Isaacs <[email protected]>

Author: joseph-isaacs

encodings/decimal-byte-parts/Cargo.toml

@@ -26,3 +26,6 @@ vortex-dtype = { workspace = true }
 vortex-error = { workspace = true }
 vortex-mask = { workspace = true }
 vortex-scalar = { workspace = true }
+
+[dev-dependencies]
+vortex-array = { path = "../../vortex-array", features = ["test-harness"] }

encodings/decimal-byte-parts/src/decimal_byte_parts/compute/compare.rs

@@ -2,8 +2,8 @@ use num_traits::NumCast;
 use vortex_array::arrays::ConstantArray;
 use vortex_array::compute::{CompareKernel, CompareKernelAdapter, Operator, compare};
 use vortex_array::{Array, ArrayRef, register_kernel};
-use vortex_dtype::{NativePType, PType, match_each_integer_ptype};
-use vortex_error::VortexResult;
+use vortex_dtype::{NativePType, Nullability, PType, match_each_integer_ptype};
+use vortex_error::{VortexExpect, VortexResult};
 use vortex_scalar::{DecimalValue, Scalar, ScalarValue, match_each_decimal_value};
 
 use crate::DecimalBytePartsVTable;
@@ -23,25 +23,40 @@ impl CompareKernel for DecimalBytePartsVTable {
             return Ok(None);
         };
 
-        let scalar_type = lhs
-            .msp
-            .dtype()
-            .with_nullability(lhs.dtype.nullability() | rhs.dtype().nullability());
+        let nullability = lhs.dtype.nullability() | rhs.dtype().nullability();
+        let scalar_type = lhs.msp.dtype().with_nullability(nullability);
 
-        let encoded_scalar = rhs_const
+        let rhs_decimal = rhs_const
             .as_decimal()
             .decimal_value()
-            .and_then(|value| {
-                decimal_value_wrapper_to_primitive(value, lhs.msp.as_primitive_typed().ptype())
-            })
-            .map(|value| Scalar::new(scalar_type.clone(), value))
-            .unwrap_or_else(|| Scalar::null(scalar_type));
+            .vortex_expect("checked for null in entry func");
+        let Some(encoded_scalar) =
+            decimal_value_wrapper_to_primitive(rhs_decimal, lhs.msp.as_primitive_typed().ptype())
+                .map(|value| Scalar::new(scalar_type.clone(), value))
+        else {
+            // here the scalar value is bigger than the msp type.
+            // TODO(joe): fixme, when allowing lsp values.
+            return Ok(Some(
+                ConstantArray::new(unconvertible_value(operator, nullability), lhs.len())
+                    .to_array(),
+            ));
+        };
         let encoded_const = ConstantArray::new(encoded_scalar, rhs.len());
         compare(&lhs.msp, &encoded_const.to_array(), operator).map(Some)
     }
 }
 
-// clippy prefers smaller functions
+fn unconvertible_value(operator: Operator, nullability: Nullability) -> Scalar {
+    // v op unconvertible where unconvertible > v_max
+    match operator {
+        // v is never eq or gt/gte
+        Operator::Eq | Operator::Gt | Operator::Gte => Scalar::bool(false, nullability),
+        // v is always eq or gt/gte
+        Operator::NotEq | Operator::Lt | Operator::Lte => Scalar::bool(true, nullability),
+    }
+}
+
+// this value return None is the decimal scalar cannot be cast the ptype.
 fn decimal_value_wrapper_to_primitive(
     decimal_value: DecimalValue,
     ptype: PType,
@@ -99,4 +114,41 @@ mod tests {
             vec![false, false, true]
         );
     }
+
+    #[test]
+    fn compare_decimal_const_unconvertible_comparison() {
+        let decimal_dtype = DecimalDType::new(40, 2);
+        let dtype = DType::Decimal(decimal_dtype, Nullability::Nullable);
+        let lhs = DecimalBytePartsArray::try_new(
+            PrimitiveArray::new(buffer![100i32, 200i32, 400i32], Validity::AllValid).to_array(),
+            vec![],
+            decimal_dtype,
+        )
+        .unwrap()
+        .to_array();
+        // This cannot be converted to a i32.
+        let rhs = ConstantArray::new(
+            Scalar::new(dtype, DecimalValue::I128(-9999999999999965304).into()),
+            lhs.len(),
+        );
+
+        let res = compare(lhs.as_ref(), rhs.as_ref(), Operator::Eq).unwrap();
+
+        assert_eq!(
+            res.to_bool().unwrap().bool_vec().unwrap(),
+            vec![false, false, false]
+        );
+
+        let res = compare(lhs.as_ref(), rhs.as_ref(), Operator::Gt).unwrap();
+        assert_eq!(
+            res.to_bool().unwrap().bool_vec().unwrap(),
+            vec![false, false, false]
+        );
+
+        let res = compare(lhs.as_ref(), rhs.as_ref(), Operator::Lt).unwrap();
+        assert_eq!(
+            res.to_bool().unwrap().bool_vec().unwrap(),
+            vec![true, true, true]
+        );
+    }
 }