fix: Fuzzer reference compare results in null when comparing to null (#2676)

Author: robert3005

Cargo.lock

@@ -22,6 +22,7 @@ arrow-buffer = { workspace = true }
 arrow-ord = { workspace = true }
 futures-util = { workspace = true }
 libfuzzer-sys = { workspace = true }
+thiserror = { workspace = true }
 tokio = { workspace = true, features = ["full"] }
 vortex-array = { workspace = true, features = ["arbitrary"] }
 vortex-btrblocks = { workspace = true }

fuzz/Cargo.toml

@@ -1,4 +1,5 @@
 #![no_main]
+#![allow(clippy::unwrap_used)]
 
 use libfuzzer_sys::{Corpus, fuzz_target};
 use vortex_array::aliases::hash_set::HashSet;
@@ -12,7 +13,8 @@ use vortex_array::compute::{
 use vortex_array::vtable::EncodingVTable;
 use vortex_array::{Array, ArrayRef};
 use vortex_btrblocks::BtrBlocksCompressor;
-use vortex_error::VortexUnwrap;
+use vortex_error::{VortexUnwrap, vortex_panic};
+use vortex_fuzz::error::{VortexFuzzError, VortexFuzzResult};
 use vortex_fuzz::{Action, FuzzArrayAction, sort_canonical_array};
 use vortex_scalar::Scalar;
 
@@ -25,18 +27,18 @@ fuzz_target!(|fuzz_action: FuzzArrayAction| -> Corpus {
                 current_array = BtrBlocksCompressor
                     .compress(current_array.to_canonical().vortex_unwrap().as_ref())
                     .vortex_unwrap();
-                assert_array_eq(&expected.array(), &current_array, i);
+                assert_array_eq(&expected.array(), &current_array, i).unwrap();
             }
             Action::Slice(range) => {
                 current_array = slice(&current_array, range.start, range.end).vortex_unwrap();
-                assert_array_eq(&expected.array(), &current_array, i);
+                assert_array_eq(&expected.array(), &current_array, i).unwrap();
             }
             Action::Take(indices) => {
                 if indices.is_empty() {
                     return Corpus::Reject;
                 }
                 current_array = take(&current_array, &indices).vortex_unwrap();
-                assert_array_eq(&expected.array(), &current_array, i);
+                assert_array_eq(&expected.array(), &current_array, i).unwrap();
             }
             Action::SearchSorted(s, side) => {
                 // TODO(robert): Ideally we'd preserve the encoding perfectly but this is close enough
@@ -53,20 +55,26 @@ fuzz_target!(|fuzz_action: FuzzArrayAction| -> Corpus {
                 {
                     sorted = BtrBlocksCompressor.compress(&sorted).vortex_unwrap();
                 }
-                assert_search_sorted(sorted, s, side, expected.search(), i)
+                assert_search_sorted(sorted, s, side, expected.search(), i).unwrap()
             }
             Action::Filter(mask) => {
                 current_array = filter(&current_array, &mask).vortex_unwrap();
-                assert_array_eq(&expected.array(), &current_array, i);
+                assert_array_eq(&expected.array(), &current_array, i).unwrap();
             }
             Action::Compare(v, op) => {
-                current_array = compare(
+                let compare_result = compare(
                     &current_array,
-                    &ConstantArray::new(v, current_array.len()).into_array(),
+                    &ConstantArray::new(v.clone(), current_array.len()).into_array(),
                     op,
                 )
                 .vortex_unwrap();
-                assert_array_eq(&expected.array(), &current_array, i);
+                if let Err(e) = assert_array_eq(&expected.array(), &compare_result, i) {
+                    vortex_panic!(
+                        "Failed to compare {}with {op} {v}\nError: {e}",
+                        current_array.tree_display()
+                    )
+                }
+                current_array = compare_result;
             }
         }
     }
@@ -79,38 +87,47 @@ fn assert_search_sorted(
     side: SearchSortedSide,
     expected: SearchResult,
     step: usize,
-) {
+) -> VortexFuzzResult<()> {
     let search_result = search_sorted(&array, s.clone(), side).vortex_unwrap();
-    assert_eq!(
-        expected,
-        search_result,
-        "Expected to find {s}({}) at {expected} in {} from {side} but instead found it at {search_result} in step {step}",
-        s.dtype(),
-        array.tree_display()
-    );
+    if search_result != expected {
+        Err(VortexFuzzError::SearchSortedError(
+            s,
+            expected,
+            array.to_array(),
+            side,
+            search_result,
+            step,
+        ))
+    } else {
+        Ok(())
+    }
 }
 
 // TODO(ngates): this is horrific... we should have an array_equals compute function?
-fn assert_array_eq(lhs: &dyn Array, rhs: &dyn Array, step: usize) {
-    assert_eq!(
-        lhs.len(),
-        rhs.len(),
-        "LHS len {} != RHS len {}, lhs is {} rhs is {} in step {step}",
-        lhs.len(),
-        rhs.len(),
-        lhs.tree_display(),
-        rhs.tree_display()
-    );
+fn assert_array_eq(lhs: &ArrayRef, rhs: &ArrayRef, step: usize) -> VortexFuzzResult<()> {
+    if lhs.len() != rhs.len() {
+        return Err(VortexFuzzError::LengthMismatch(
+            lhs.len(),
+            rhs.len(),
+            lhs.to_array(),
+            rhs.to_array(),
+            step,
+        ));
+    }
     for idx in 0..lhs.len() {
         let l = scalar_at(lhs, idx).vortex_unwrap();
         let r = scalar_at(rhs, idx).vortex_unwrap();
 
-        assert_eq!(
-            l,
-            r,
-            "{l} != {r} at index {idx}, lhs is {} rhs is {} in step {step}",
-            lhs.tree_display(),
-            rhs.tree_display()
-        );
+        if l != r {
+            return Err(VortexFuzzError::ArrayNotEqual(
+                l,
+                r,
+                idx,
+                lhs.to_array(),
+                rhs.to_array(),
+                step,
+            ));
+        }
     }
+    Ok(())
 }

fuzz/fuzz_targets/array_ops.rs

@@ -1,19 +1,34 @@
+use std::fmt::Debug;
+use std::ops::Deref;
+
+use arrow_buffer::BooleanBuffer;
 use vortex_array::accessor::ArrayAccessor;
 use vortex_array::arrays::BoolArray;
 use vortex_array::compute::{Operator, scalar_at, scalar_cmp};
+use vortex_array::validity::Validity;
 use vortex_array::{Array, ArrayRef, ToCanonical};
-use vortex_dtype::{DType, match_each_native_ptype};
-use vortex_error::VortexResult;
+use vortex_dtype::{DType, NativePType, match_each_native_ptype};
+use vortex_error::{VortexExpect, VortexResult};
 use vortex_scalar::Scalar;
 
 pub fn compare_canonical_array(
     array: &dyn Array,
     value: &Scalar,
     operator: Operator,
 ) -> VortexResult<ArrayRef> {
+    if value.is_null() {
+        return Ok(
+            BoolArray::new(BooleanBuffer::new_unset(array.len()), Validity::AllInvalid)
+                .into_array(),
+        );
+    }
+
     match array.dtype() {
         DType::Bool(_) => {
-            let bool = value.as_bool().value();
+            let bool = value
+                .as_bool()
+                .value()
+                .vortex_expect("nulls handled before");
             Ok(compare_to(
                 array
                     .to_bool()?
@@ -29,8 +44,8 @@ pub fn compare_canonical_array(
             let primitive = value.as_primitive();
             let primitive_array = array.to_primitive()?;
             match_each_native_ptype!(p, |$P| {
-                let pval = primitive.typed_value::<$P>();
-                Ok(compare_to(
+                let pval = primitive.typed_value::<$P>().vortex_expect("nulls handled before");
+                Ok(compare_native_ptype(
                     primitive_array
                         .as_slice::<$P>()
                         .iter()
@@ -43,20 +58,26 @@ pub fn compare_canonical_array(
             })
         }
         DType::Utf8(_) => array.to_varbinview()?.with_iterator(|iter| {
-            let utf8_value = value.as_utf8().value();
+            let utf8_value = value
+                .as_utf8()
+                .value()
+                .vortex_expect("nulls handled before");
             compare_to(
                 iter.map(|v| v.map(|b| unsafe { str::from_utf8_unchecked(b) })),
-                utf8_value.as_deref(),
+                utf8_value.deref(),
                 operator,
             )
         }),
         DType::Binary(_) => array.to_varbinview()?.with_iterator(|iter| {
-            let binary_value = value.as_binary().value();
+            let binary_value = value
+                .as_binary()
+                .value()
+                .vortex_expect("nulls handled before");
             compare_to(
                 // Don't understand the lifetime problem here but identity map makes it go away
                 #[allow(clippy::map_identity)]
                 iter.map(|v| v),
-                binary_value.as_deref(),
+                binary_value.deref(),
                 operator,
             )
         }),
@@ -75,18 +96,38 @@ pub fn compare_canonical_array(
     }
 }
 
-fn compare_to<T: PartialOrd + PartialEq>(
+fn compare_to<T: PartialOrd + PartialEq + Debug>(
+    values: impl Iterator<Item = Option<T>>,
+    cmp_value: T,
+    operator: Operator,
+) -> ArrayRef {
+    BoolArray::from_iter(values.map(|val| {
+        val.map(|v| match operator {
+            Operator::Eq => v == cmp_value,
+            Operator::NotEq => v != cmp_value,
+            Operator::Gt => v > cmp_value,
+            Operator::Gte => v >= cmp_value,
+            Operator::Lt => v < cmp_value,
+            Operator::Lte => v <= cmp_value,
+        })
+    }))
+    .into_array()
+}
+
+fn compare_native_ptype<T: NativePType>(
     values: impl Iterator<Item = Option<T>>,
-    value: Option<T>,
+    cmp_value: T,
     operator: Operator,
 ) -> ArrayRef {
-    BoolArray::from_iter(values.map(|v| match operator {
-        Operator::Eq => v == value,
-        Operator::NotEq => v != value,
-        Operator::Gt => v > value,
-        Operator::Gte => v >= value,
-        Operator::Lt => v < value,
-        Operator::Lte => v <= value,
+    BoolArray::from_iter(values.map(|val| {
+        val.map(|v| match operator {
+            Operator::Eq => v.is_eq(cmp_value),
+            Operator::NotEq => !v.is_eq(cmp_value),
+            Operator::Gt => v.is_gt(cmp_value),
+            Operator::Gte => v.is_ge(cmp_value),
+            Operator::Lt => v.is_lt(cmp_value),
+            Operator::Lte => v.is_le(cmp_value),
+        })
     }))
     .into_array()
 }

fuzz/src/compare.rs

@@ -0,0 +1,45 @@
+use std::fmt;
+use std::fmt::{Debug, Display, Formatter};
+
+use vortex_array::ArrayRef;
+use vortex_array::compute::{SearchResult, SearchSortedSide};
+use vortex_error::VortexError;
+use vortex_scalar::Scalar;
+
+fn tree_display(arr: &ArrayRef) -> impl Display {
+    arr.tree_display()
+}
+
+#[derive(thiserror::Error)]
+pub enum VortexFuzzError {
+    #[error("Expected to find {0} at {1} in {tree} from {3} but instead found it at {4} in step {5}", tree = tree_display(.2))]
+    SearchSortedError(
+        Scalar,
+        SearchResult,
+        ArrayRef,
+        SearchSortedSide,
+        SearchResult,
+        usize,
+    ),
+
+    #[error("{0} != {1} at index {2}, lhs is {lhs_tree} rhs is {rhs_tree} in step {5}",lhs_tree = tree_display(.3), rhs_tree = tree_display(.4))]
+    ArrayNotEqual(Scalar, Scalar, usize, ArrayRef, ArrayRef, usize),
+
+    #[error("LHS len {0} != RHS len {1}, lhs is {lhs_tree} rhs is {rhs_tree} in step {4}", lhs_tree = tree_display(.2), rhs_tree = tree_display(.3))]
+    LengthMismatch(usize, usize, ArrayRef, ArrayRef, usize),
+
+    #[error(transparent)]
+    VortexError(
+        #[from]
+        #[backtrace]
+        VortexError,
+    ),
+}
+
+impl Debug for VortexFuzzError {
+    fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result {
+        Display::fmt(self, f)
+    }
+}
+
+pub type VortexFuzzResult<T> = Result<T, VortexFuzzError>;

fuzz/src/error.rs

@@ -1,4 +1,7 @@
+#![feature(error_generic_member_access)]
+
 mod compare;
+pub mod error;
 mod filter;
 mod search_sorted;
 mod slice;