Fix fuzzer incorrect mask application (#6877)

#6515

- Apply validity & mask in fuzzer target generation.
  Before, validity & !mask was generated.
- Add tracing logging to fuzzer
- Add debug output for fuzzer mask, info output for mistmatches,
  initial array, and fuzz actions

Signed-off-by: Mikhail Kot <mikhail@spiraldb.com>

Author: myrrc

Cargo.lock

@@ -50,6 +50,9 @@ vortex-file = { workspace = true, optional = true }
 tokio = { workspace = true, features = ["rt", "macros"], optional = true }
 vortex-cuda = { workspace = true, optional = true }
 
+tracing = { workspace = true }
+tracing-subscriber = { workspace = true }
+
 [lints]
 workspace = true
 

fuzz/Cargo.toml

@@ -10,7 +10,11 @@ use vortex_error::vortex_panic;
 use vortex_fuzz::FuzzArrayAction;
 use vortex_fuzz::run_fuzz_action;
 
-fuzz_target!(|fuzz_action: FuzzArrayAction| -> Corpus {
+fuzz_target!(
+    init: {
+        tracing_subscriber::fmt::init();
+    },
+    |fuzz_action: FuzzArrayAction| -> Corpus {
     match run_fuzz_action(fuzz_action) {
         Ok(true) => Corpus::Keep,
         Ok(false) => Corpus::Reject,

fuzz/fuzz_targets/array_ops.rs

@@ -23,28 +23,31 @@ use vortex_error::VortexResult;
 use vortex_mask::AllOr;
 use vortex_mask::Mask;
 
-/// Set to false any entries for which the mask is true.
-///
+/// Apply a logical AND of a validity and a mask.
+/// This needs to be coherent with applications of Mask.
 /// The result is always nullable. The result has the same length as self.
 #[inline]
 pub fn mask_validity(validity: &Validity, mask: &Mask) -> Validity {
-    match mask.bit_buffer() {
-        AllOr::All => Validity::AllInvalid,
-        AllOr::None => validity.clone().into_nullable(),
-        AllOr::Some(make_invalid) => match validity {
+    let out = match mask.bit_buffer() {
+        AllOr::All => validity.clone().into_nullable(),
+        AllOr::None => Validity::AllInvalid,
+        AllOr::Some(make_valid) => match validity {
+            Validity::AllInvalid => Validity::AllInvalid,
             Validity::NonNullable | Validity::AllValid => {
-                Validity::from_bit_buffer(!make_invalid, Nullability::Nullable)
+                Validity::from_bit_buffer(make_valid.clone(), Nullability::Nullable)
             }
-            Validity::AllInvalid => Validity::AllInvalid,
             Validity::Array(is_valid) => {
                 let is_valid = is_valid.to_bool();
                 Validity::from_bit_buffer(
-                    is_valid.to_bit_buffer() & !make_invalid,
+                    is_valid.to_bit_buffer() & make_valid,
                     Nullability::Nullable,
                 )
             }
         },
-    }
+    };
+
+    tracing::debug!(validity = ?validity, mask = ?mask, out = ?out, "generated fuzzer mask");
+    out
 }
 
 /// Apply mask on the canonical form of the array to get a consistent baseline.
@@ -173,7 +176,7 @@ mod tests {
     #[test]
     fn test_mask_bool_array() {
         let array = BoolArray::from_iter([true, false, true, false, true]);
-        let mask = Mask::from_iter([true, false, false, true, false]);
+        let mask = Mask::from_iter([false, true, true, false, true]);
 
         let result = mask_canonical_array(array.to_canonical().unwrap(), &mask).unwrap();
 
@@ -184,7 +187,7 @@ mod tests {
     #[test]
     fn test_mask_primitive_array() {
         let array = PrimitiveArray::from_iter([1i32, 2, 3, 4, 5]);
-        let mask = Mask::from_iter([false, true, false, true, false]);
+        let mask = Mask::from_iter([true, false, true, false, true]);
 
         let result = mask_canonical_array(array.to_canonical().unwrap(), &mask).unwrap();
 
@@ -195,7 +198,7 @@ mod tests {
     #[test]
     fn test_mask_primitive_array_with_nulls() {
         let array = PrimitiveArray::from_option_iter([Some(1i32), None, Some(3), Some(4), None]);
-        let mask = Mask::from_iter([true, false, false, true, false]);
+        let mask = Mask::from_iter([false, true, true, false, true]);
 
         let result = mask_canonical_array(array.to_canonical().unwrap(), &mask).unwrap();
 
@@ -210,7 +213,7 @@ mod tests {
             [Some(1i128), Some(2), Some(3), Some(4), Some(5)],
             dtype,
         );
-        let mask = Mask::from_iter([false, false, true, false, false]);
+        let mask = Mask::from_iter([true, true, false, true, true]);
 
         let result = mask_canonical_array(array.to_canonical().unwrap(), &mask).unwrap();
 
@@ -222,7 +225,7 @@ mod tests {
     #[test]
     fn test_mask_varbinview_array() {
         let array = VarBinViewArray::from_iter_str(["one", "two", "three", "four", "five"]);
-        let mask = Mask::from_iter([true, false, true, false, true]);
+        let mask = Mask::from_iter([false, true, false, true, false]);
 
         let result = mask_canonical_array(array.to_canonical().unwrap(), &mask).unwrap();
 
@@ -241,7 +244,7 @@ mod tests {
                 .with_zero_copy_to_list(true)
         };
 
-        let mask = Mask::from_iter([false, true, false]);
+        let mask = Mask::from_iter([true, false, true]);
 
         let result = mask_canonical_array(array.to_canonical().unwrap(), &mask).unwrap();
 
@@ -257,7 +260,7 @@ mod tests {
         let array =
             FixedSizeListArray::try_new(elements, 2, Nullability::NonNullable.into(), 3).unwrap();
 
-        let mask = Mask::from_iter([true, false, true]);
+        let mask = Mask::from_iter([false, true, false]);
 
         let result = mask_canonical_array(array.to_canonical().unwrap(), &mask).unwrap();
 
@@ -281,7 +284,7 @@ mod tests {
         )
         .unwrap();
 
-        let mask = Mask::from_iter([false, true, false]);
+        let mask = Mask::from_iter([true, false, true]);
 
         let result = mask_canonical_array(array.to_canonical().unwrap(), &mask).unwrap();
 
@@ -292,9 +295,9 @@ mod tests {
     }
 
     #[test]
-    fn test_mask_all_true() {
+    fn test_mask_all_false() {
         let array = PrimitiveArray::from_iter([1i32, 2, 3, 4, 5]);
-        let mask = Mask::AllTrue(5);
+        let mask = Mask::AllFalse(5);
 
         let result = mask_canonical_array(array.to_canonical().unwrap(), &mask).unwrap();
 
@@ -303,9 +306,9 @@ mod tests {
     }
 
     #[test]
-    fn test_mask_all_false() {
+    fn test_mask_all_true() {
         let array = PrimitiveArray::from_iter([1i32, 2, 3, 4, 5]);
-        let mask = Mask::AllFalse(5);
+        let mask = Mask::AllTrue(5);
 
         let result = mask_canonical_array(array.to_canonical().unwrap(), &mask).unwrap();
 
@@ -317,10 +320,9 @@ mod tests {
     #[test]
     fn test_mask_empty_array() {
         let array = PrimitiveArray::from_iter(Vec::<i32>::new());
-        let mask = Mask::AllFalse(0);
-
-        let result = mask_canonical_array(array.to_canonical().unwrap(), &mask).unwrap();
-
-        assert_eq!(result.len(), 0);
+        for mask in [Mask::AllFalse(0), Mask::AllTrue(0)] {
+            let result = mask_canonical_array(array.to_canonical().unwrap(), &mask).unwrap();
+            assert_eq!(result.len(), 0);
+        }
     }
 }

fuzz/src/array/mask.rs

@@ -37,6 +37,7 @@ use strum::EnumIter;
 use strum::IntoEnumIterator;
 pub(crate) use sum::*;
 pub(crate) use take::*;
+use tracing::info;
 use vortex_array::ArrayRef;
 use vortex_array::DynArray;
 use vortex_array::IntoArray;
@@ -560,7 +561,14 @@ pub fn run_fuzz_action(fuzz_action: FuzzArrayAction) -> crate::error::VortexFuzz
     let FuzzArrayAction { array, actions } = fuzz_action;
     let mut current_array = array.to_array();
 
+    info!(
+        "Initial array:\nTree:\n{}Values:\n{:#}",
+        current_array.display_tree(),
+        current_array.display_values()
+    );
+
     for (i, (action, expected)) in actions.into_iter().enumerate() {
+        info!(id = i, action = ?action);
         match action {
             Action::Compress(strategy) => {
                 let canonical = current_array

fuzz/src/array/mod.rs

@@ -97,12 +97,29 @@ impl Display for VortexFuzzError {
                     "MinMax mismatch: expected {lhs:?} got {rhs:?} in step {step}\nBacktrace:\n{backtrace}"
                 )
             }
-            VortexFuzzError::ArrayNotEqual(expected, actual, idx, lhs, rhs, step, backtrace) => {
+            VortexFuzzError::ArrayNotEqual(
+                expected_scalar,
+                actual_scalar,
+                idx,
+                expected_array,
+                current_array,
+                step,
+                backtrace,
+            ) => {
+                let expected_tree = expected_array.display_tree();
+                let current_tree = current_array.display_tree();
+                let expected_values = expected_array.display_values();
+                let current_values = current_array.display_values();
                 write!(
                     f,
-                    "{expected} != {actual} at index {idx}, lhs is {} rhs is {} in step {step}\nBacktrace:\n{backtrace}",
-                    lhs.display_tree(),
-                    rhs.display_tree(),
+                    "Mismatch at step {step} at index {idx}\n\
+                    Expected scalar:\n{expected_scalar}\n\
+                    Actual scalar:\n{actual_scalar}\n\
+                    Expected tree:\n{expected_tree}\n\
+                    Current tree:\n{current_tree}\
+                    Expected values:\n{expected_values:#}\n\
+                    Current values:\n{current_values:#}\
+                    \n{backtrace}"
                 )
             }
             VortexFuzzError::DTypeMismatch(lhs, rhs, step, backtrace) => {

fuzz/src/error.rs

@@ -476,8 +476,9 @@ impl dyn DynArray + '_ {
             DisplayOptions::CommaSeparatedScalars {
                 omit_comma_after_space,
             } => {
-                write!(f, "[")?;
+                write!(f, "{}", if f.alternate() { "[\n" } else { "[" })?;
                 let sep = if *omit_comma_after_space { "," } else { ", " };
+                let sep = if f.alternate() { ",\n" } else { sep };
                 write!(
                     f,
                     "{}",
@@ -487,7 +488,7 @@ impl dyn DynArray + '_ {
                             .map_or_else(|e| format!("<error: {e}>"), |s| s.to_string()))
                         .format(sep)
                 )?;
-                write!(f, "]")
+                write!(f, "{}", if f.alternate() { "\n]" } else { "]" })
             }
             DisplayOptions::TreeDisplay {
                 buffers,

vortex-array/src/display/mod.rs

@@ -36,7 +36,7 @@ use crate::scalar_fn::fns::binary::Binary;
 use crate::scalar_fn::fns::operators::Operator;
 
 /// Validity information for an array
-#[derive(Clone, Debug)]
+#[derive(Clone)]
 pub enum Validity {
     /// Items *can't* be null
     NonNullable,
@@ -50,6 +50,17 @@ pub enum Validity {
     Array(ArrayRef),
 }
 
+impl Debug for Validity {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        match self {
+            Self::NonNullable => write!(f, "NonNullable"),
+            Self::AllValid => write!(f, "AllValid"),
+            Self::AllInvalid => write!(f, "AllInvalid"),
+            Self::Array(arr) => write!(f, "SomeValid({})", arr.as_ref().display_values()),
+        }
+    }
+}
+
 impl Validity {
     /// Make a step towards canonicalising validity if necessary
     pub fn execute(self, ctx: &mut ExecutionCtx) -> VortexResult<Validity> {

vortex-array/src/validity.rs

@@ -336,6 +336,10 @@ impl core::fmt::Debug for vortex_buffer::BitBuffer
 
 pub fn vortex_buffer::BitBuffer::fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result
 
+impl core::fmt::Display for vortex_buffer::BitBuffer
+
+pub fn vortex_buffer::BitBuffer::fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result
+
 impl core::iter::traits::collect::FromIterator<bool> for vortex_buffer::BitBuffer
 
 pub fn vortex_buffer::BitBuffer::from_iter<T: core::iter::traits::collect::IntoIterator<Item = bool>>(iter: T) -> Self

vortex-buffer/public-api.lock

@@ -1,6 +1,9 @@
 // SPDX-License-Identifier: Apache-2.0
 // SPDX-FileCopyrightText: Copyright the Vortex contributors
 
+use std::fmt::Display;
+use std::fmt::Formatter;
+use std::fmt::Result as FmtResult;
 use std::ops::BitAnd;
 use std::ops::BitOr;
 use std::ops::BitXor;
@@ -35,6 +38,18 @@ pub struct BitBuffer {
     len: usize,
 }
 
+const LIMIT_LEN: usize = 16;
+impl Display for BitBuffer {
+    fn fmt(&self, f: &mut Formatter<'_>) -> FmtResult {
+        let limit = f.precision().unwrap_or(LIMIT_LEN);
+        let buf: Vec<bool> = self.into_iter().take(limit).collect();
+        f.debug_struct("BitBuffer")
+            .field("len", &self.len)
+            .field("buffer", &buf)
+            .finish()
+    }
+}
+
 impl PartialEq for BitBuffer {
     fn eq(&self, other: &Self) -> bool {
         if self.len != other.len {