fix: binary_numeric is now correct (+ tests!)

encodings/dict/Cargo.toml

@@ -29,6 +29,7 @@ workspace = true
 [dev-dependencies]
 criterion = { workspace = true }
 rand = { workspace = true }
+vortex-array = { workspace = true, features = ["test-harness"] }
 
 [[bench]]
 name = "dict_compress"

encodings/dict/src/compute/binary_numeric.rs

@@ -0,0 +1,29 @@
+use vortex_array::array::ConstantArray;
+use vortex_array::compute::{binary_numeric, BinaryNumericFn};
+use vortex_array::{ArrayData, IntoArrayData};
+use vortex_error::VortexResult;
+use vortex_scalar::BinaryNumericOperator;
+
+use crate::{DictArray, DictEncoding};
+
+impl BinaryNumericFn<DictArray> for DictEncoding {
+    fn binary_numeric(
+        &self,
+        array: &DictArray,
+        rhs: &ArrayData,
+        op: BinaryNumericOperator,
+    ) -> VortexResult<Option<ArrayData>> {
+        let Some(rhs_scalar) = rhs.as_constant() else {
+            return Ok(None);
+        };
+
+        let rhs_const_array = ConstantArray::new(rhs_scalar, array.values().len()).into_array();
+
+        DictArray::try_new(
+            array.codes(),
+            binary_numeric(&array.values(), &rhs_const_array, op)?,
+        )
+        .map(IntoArrayData::into_array)
+        .map(Some)
+    }
+}

encodings/dict/src/compute/mod.rs

@@ -1,13 +1,14 @@
+mod binary_numeric;
 mod compare;
 mod like;
 
 use vortex_array::compute::{
-    binary_numeric, filter, scalar_at, slice, take, BinaryNumericFn, CompareFn, ComputeVTable,
-    FilterFn, FilterMask, LikeFn, ScalarAtFn, SliceFn, TakeFn,
+    filter, scalar_at, slice, take, BinaryNumericFn, CompareFn, ComputeVTable, FilterFn,
+    FilterMask, LikeFn, ScalarAtFn, SliceFn, TakeFn,
 };
 use vortex_array::{ArrayData, IntoArrayData};
 use vortex_error::VortexResult;
-use vortex_scalar::{BinaryNumericOperator, Scalar};
+use vortex_scalar::Scalar;
 
 use crate::{DictArray, DictEncoding};
 
@@ -41,23 +42,6 @@ impl ComputeVTable for DictEncoding {
     }
 }
 
-impl BinaryNumericFn<DictArray> for DictEncoding {
-    fn binary_numeric(
-        &self,
-        array: &DictArray,
-        rhs: &ArrayData,
-        op: BinaryNumericOperator,
-    ) -> VortexResult<Option<ArrayData>> {
-        if !rhs.is_constant() {
-            return Ok(None);
-        }
-
-        DictArray::try_new(array.codes(), binary_numeric(&array.values(), rhs, op)?)
-            .map(IntoArrayData::into_array)
-            .map(Some)
-    }
-}
-
 impl ScalarAtFn<DictArray> for DictEncoding {
     fn scalar_at(&self, array: &DictArray, index: usize) -> VortexResult<Scalar> {
         let dict_index: usize = scalar_at(array.codes(), index)?.as_ref().try_into()?;
@@ -94,8 +78,9 @@ impl SliceFn<DictArray> for DictEncoding {
 mod test {
     use vortex_array::accessor::ArrayAccessor;
     use vortex_array::array::{ConstantArray, PrimitiveArray, VarBinViewArray};
+    use vortex_array::compute::binary_numeric::test_harness::test_binary_numeric;
     use vortex_array::compute::{compare, scalar_at, slice, Operator};
-    use vortex_array::{ArrayLen, IntoArrayData, IntoArrayVariant, ToArrayData};
+    use vortex_array::{ArrayData, ArrayLen, IntoArrayData, IntoArrayVariant, ToArrayData};
     use vortex_dtype::{DType, Nullability};
     use vortex_scalar::Scalar;
 
@@ -143,8 +128,7 @@ mod test {
         );
     }
 
-    #[test]
-    fn compare_sliced_dict() {
+    fn sliced_dict_array() -> ArrayData {
         let reference = PrimitiveArray::from_nullable_vec(vec![
             Some(42),
             Some(-9),
@@ -155,8 +139,14 @@ mod test {
         ]);
         let (codes, values) = dict_encode_primitive(&reference);
         let dict = DictArray::try_new(codes.into_array(), values.into_array()).unwrap();
-        let sliced = slice(dict, 1, 4).unwrap();
+        slice(dict, 1, 4).unwrap()
+    }
+
+    #[test]
+    fn compare_sliced_dict() {
+        let sliced = sliced_dict_array();
         let compared = compare(sliced, ConstantArray::new(42, 3), Operator::Eq).unwrap();
+
         assert_eq!(
             scalar_at(&compared, 0).unwrap(),
             Scalar::bool(false, Nullability::Nullable)
@@ -170,4 +160,10 @@ mod test {
             Scalar::bool(true, Nullability::Nullable)
         );
     }
+
+    #[test]
+    fn test_dict_binary_numeric() {
+        let array = sliced_dict_array();
+        test_binary_numeric::<i32>(array)
+    }
 }

encodings/runend/Cargo.toml

@@ -23,5 +23,8 @@ vortex-dtype = { workspace = true }
 vortex-error = { workspace = true }
 vortex-scalar = { workspace = true }
 
+[dev-dependencies]
+vortex-array = { workspace = true, features = ["test-harness"] }
+
 [lints]
 workspace = true

encodings/runend/src/compute/binary_numeric.rs

@@ -0,0 +1,31 @@
+use vortex_array::array::ConstantArray;
+use vortex_array::compute::{binary_numeric, BinaryNumericFn};
+use vortex_array::{ArrayData, ArrayLen, IntoArrayData};
+use vortex_error::VortexResult;
+use vortex_scalar::BinaryNumericOperator;
+
+use crate::{RunEndArray, RunEndEncoding};
+
+impl BinaryNumericFn<RunEndArray> for RunEndEncoding {
+    fn binary_numeric(
+        &self,
+        array: &RunEndArray,
+        rhs: &ArrayData,
+        op: BinaryNumericOperator,
+    ) -> VortexResult<Option<ArrayData>> {
+        let Some(rhs_scalar) = rhs.as_constant() else {
+            return Ok(None);
+        };
+
+        let rhs_const_array = ConstantArray::new(rhs_scalar, array.values().len()).into_array();
+
+        RunEndArray::with_offset_and_length(
+            array.ends(),
+            binary_numeric(&array.values(), &rhs_const_array, op)?,
+            array.offset(),
+            array.len(),
+        )
+        .map(IntoArrayData::into_array)
+        .map(Some)
+    }
+}

encodings/runend/src/compute/mod.rs

@@ -1,3 +1,4 @@
+mod binary_numeric;
 mod compare;
 mod fill_null;
 mod invert;
@@ -9,14 +10,14 @@ use std::ops::AddAssign;
 use num_traits::AsPrimitive;
 use vortex_array::array::{BooleanBuffer, PrimitiveArray};
 use vortex_array::compute::{
-    binary_numeric, filter, scalar_at, slice, BinaryNumericFn, CompareFn, ComputeVTable,
-    FillNullFn, FilterFn, FilterMask, InvertFn, ScalarAtFn, SliceFn, TakeFn,
+    filter, scalar_at, slice, BinaryNumericFn, CompareFn, ComputeVTable, FillNullFn, FilterFn,
+    FilterMask, InvertFn, ScalarAtFn, SliceFn, TakeFn,
 };
 use vortex_array::variants::PrimitiveArrayTrait;
 use vortex_array::{ArrayData, ArrayLen, IntoArrayData, IntoArrayVariant};
 use vortex_dtype::{match_each_unsigned_integer_ptype, NativePType};
 use vortex_error::{VortexResult, VortexUnwrap};
-use vortex_scalar::{BinaryNumericOperator, Scalar};
+use vortex_scalar::Scalar;
 
 use crate::{RunEndArray, RunEndEncoding};
 
@@ -54,28 +55,6 @@ impl ComputeVTable for RunEndEncoding {
     }
 }
 
-impl BinaryNumericFn<RunEndArray> for RunEndEncoding {
-    fn binary_numeric(
-        &self,
-        array: &RunEndArray,
-        rhs: &ArrayData,
-        op: BinaryNumericOperator,
-    ) -> VortexResult<Option<ArrayData>> {
-        if !rhs.is_constant() {
-            return Ok(None);
-        }
-
-        RunEndArray::with_offset_and_length(
-            array.ends(),
-            binary_numeric(&array.values(), rhs, op)?,
-            array.offset(),
-            array.len(),
-        )
-        .map(IntoArrayData::into_array)
-        .map(Some)
-    }
-}
-
 impl ScalarAtFn<RunEndArray> for RunEndEncoding {
     fn scalar_at(&self, array: &RunEndArray, index: usize) -> VortexResult<Scalar> {
         scalar_at(array.values(), array.find_physical_index(index)?)
@@ -163,6 +142,7 @@ fn filter_run_ends<R: NativePType + AddAssign + From<bool> + AsPrimitive<u64>>(
 #[cfg(test)]
 mod test {
     use vortex_array::array::PrimitiveArray;
+    use vortex_array::compute::binary_numeric::test_harness::test_binary_numeric;
     use vortex_array::compute::{filter, scalar_at, slice, FilterMask};
     use vortex_array::{ArrayDType, ArrayLen, IntoArrayData, IntoArrayVariant, ToArrayData};
     use vortex_dtype::{DType, Nullability, PType};
@@ -345,4 +325,10 @@ mod test {
             [1, 4, 2]
         );
     }
+
+    #[test]
+    fn test_runend_binary_numeric() {
+        let array = ree_array().into_array();
+        test_binary_numeric::<i32>(array)
+    }
 }

vortex-array/Cargo.toml

@@ -58,6 +58,7 @@ vortex-scalar = { workspace = true, features = ["flatbuffers", "serde"] }
 [features]
 arbitrary = ["dep:arbitrary", "vortex-dtype/arbitrary"]
 canonical_counter = []
+test-harness = []
 
 [target.'cfg(target_arch = "wasm32")'.dependencies]
 # Enable the JS feature of getrandom (via rand) to supprt wasm32 target
@@ -66,6 +67,7 @@ getrandom = { workspace = true, features = ["js"] }
 [dev-dependencies]
 criterion = { workspace = true }
 rstest = { workspace = true }
+vortex-array = { workspace = true, features = ["test-harness"] }
 
 [[bench]]
 name = "search_sorted"

vortex-array/src/array/chunked/compute/binary_numeric.rs

@@ -0,0 +1,28 @@
+use vortex_error::VortexResult;
+use vortex_scalar::BinaryNumericOperator;
+
+use crate::array::{ChunkedArray, ChunkedEncoding};
+use crate::compute::{binary_numeric, slice, BinaryNumericFn};
+use crate::{ArrayDType as _, ArrayData, IntoArrayData};
+
+impl BinaryNumericFn<ChunkedArray> for ChunkedEncoding {
+    fn binary_numeric(
+        &self,
+        array: &ChunkedArray,
+        rhs: &ArrayData,
+        op: BinaryNumericOperator,
+    ) -> VortexResult<Option<ArrayData>> {
+        let mut start = 0;
+
+        let mut new_chunks = Vec::with_capacity(array.nchunks());
+        for chunk in array.chunks() {
+            let end = start + chunk.len();
+            new_chunks.push(binary_numeric(&chunk, &slice(rhs, start, end)?, op)?);
+            start = end;
+        }
+
+        ChunkedArray::try_new(new_chunks, array.dtype().clone())
+            .map(IntoArrayData::into_array)
+            .map(Some)
+    }
+}

vortex-array/src/array/chunked/compute/mod.rs

@@ -9,6 +9,7 @@ use crate::compute::{
 };
 use crate::{ArrayData, IntoArrayData};
 
+mod binary_numeric;
 mod boolean;
 mod compare;
 mod fill_null;

vortex-array/src/array/chunked/mod.rs

@@ -9,12 +9,9 @@ use itertools::Itertools;
 use serde::{Deserialize, Serialize};
 use vortex_dtype::{DType, Nullability, PType};
 use vortex_error::{vortex_bail, vortex_panic, VortexExpect as _, VortexResult, VortexUnwrap};
-use vortex_scalar::BinaryNumericOperator;
 
 use crate::array::primitive::PrimitiveArray;
-use crate::compute::{
-    binary_numeric, scalar_at, search_sorted_usize, slice, BinaryNumericFn, SearchSortedSide,
-};
+use crate::compute::{scalar_at, search_sorted_usize, SearchSortedSide};
 use crate::encoding::ids;
 use crate::iter::{ArrayIterator, ArrayIteratorAdapter};
 use crate::stats::StatsSet;
@@ -234,35 +231,14 @@ impl ValidityVTable<ChunkedArray> for ChunkedEncoding {
     }
 }
 
-impl BinaryNumericFn<ChunkedArray> for ChunkedEncoding {
-    fn binary_numeric(
-        &self,
-        array: &ChunkedArray,
-        rhs: &ArrayData,
-        op: BinaryNumericOperator,
-    ) -> VortexResult<Option<ArrayData>> {
-        let mut start = 0;
-
-        let mut new_chunks = Vec::with_capacity(array.nchunks());
-        for chunk in array.chunks() {
-            let end = start + chunk.len();
-            new_chunks.push(binary_numeric(&chunk, &slice(rhs, start, end)?, op)?);
-            start = end;
-        }
-
-        ChunkedArray::try_new(new_chunks, array.dtype().clone())
-            .map(IntoArrayData::into_array)
-            .map(Some)
-    }
-}
-
 #[cfg(test)]
 mod test {
     use vortex_dtype::{DType, Nullability, PType};
     use vortex_error::VortexResult;
 
     use crate::array::chunked::ChunkedArray;
-    use crate::compute::{scalar_at, sub_scalar};
+    use crate::compute::binary_numeric::test_harness::test_binary_numeric;
+    use crate::compute::{scalar_at, sub_scalar, try_cast};
     use crate::{assert_arrays_eq, ArrayDType, IntoArrayData, IntoArrayVariant};
 
     fn chunked_array() -> ChunkedArray {
@@ -374,4 +350,13 @@ mod test {
         assert_eq!(rechunked.nchunks(), 4);
         assert_arrays_eq!(chunked, rechunked);
     }
+
+    #[test]
+    fn test_chunked_binary_numeric() {
+        let array = chunked_array().into_array();
+        // The tests test both X - 1 and 1 - X, so we need signed values
+        let signed_dtype = DType::from(PType::try_from(array.dtype()).unwrap().to_signed());
+        let array = try_cast(array, &signed_dtype).unwrap();
+        test_binary_numeric::<u64>(array)
+    }
 }

vortex-array/src/array/constant/compute/binary_numeric.rs

@@ -21,7 +21,7 @@ impl BinaryNumericFn<ConstantArray> for ConstantEncoding {
                 array
                     .scalar()
                     .as_primitive()
-                    .checked_numeric_operator(rhs.as_primitive(), op)?
+                    .checked_binary_numeric(rhs.as_primitive(), op)?
                     .ok_or_else(|| vortex_err!("numeric overflow"))?,
                 array.len(),
             )