Fix: dont do lazy filters on write (#6194)

This should fix the CI failures right now.

More detailed discussion that I'll write incoming...

Basically we shouldn't be eagerly decompressing validity when filtering
it, but for now to make sure things aren't broken this will have to do.

Edit: This should fix the break on spiral as well @delta003, as reported
by @danking

---------

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

Author: connortsui20

vortex-array/src/arrays/filter/execute/mod.rs

@@ -31,22 +31,15 @@ mod struct_;
 mod varbinview;
 
 /// Reconstruct a [`Mask`] from an [`Arc<MaskValues>`].
-#[inline]
 fn values_to_mask(values: &Arc<MaskValues>) -> Mask {
     Mask::Values(values.clone())
 }
 
-/// A helper function that lazily filters a [`Validity`] with a selection mask.
-///
-/// If the validity is a [`Validity::Array`], then this wraps it in a `FilterArray` instead of
-/// eagerly filtering the values immediately.
+/// A helper function that lazily filters a [`Validity`] with selection mask values.
 fn filter_validity(validity: Validity, mask: &Arc<MaskValues>) -> Validity {
-    match validity {
-        v @ (Validity::NonNullable | Validity::AllValid | Validity::AllInvalid) => v,
-        Validity::Array(arr) => {
-            Validity::Array(FilterArray::new(arr.clone(), values_to_mask(mask)).into_array())
-        }
-    }
+    validity
+        .filter(&values_to_mask(mask))
+        .vortex_expect("Somehow unable to wrap filter around a validity array")
 }
 
 /// Check for some fast-path execution conditions before calling [`execute_filter`].

vortex-array/src/compute/filter.rs

@@ -76,7 +76,7 @@ pub(crate) fn warm_up_vtable() -> usize {
 /// not the case.
 pub fn filter(array: &dyn Array, mask: &Mask) -> VortexResult<ArrayRef> {
     // TODO(connor): Remove this function completely!!!
-    array.filter(mask.clone())
+    Ok(array.filter(mask.clone())?.to_canonical()?.into_array())
 }
 
 struct Filter;

vortex-array/src/patches.rs

@@ -40,6 +40,7 @@ use crate::IntoArray;
 use crate::ToCanonical;
 use crate::arrays::PrimitiveArray;
 use crate::compute::cast;
+use crate::compute::filter;
 use crate::compute::is_sorted;
 use crate::compute::take;
 use crate::search_sorted::SearchResult;
@@ -626,8 +627,8 @@ impl Patches {
         }
 
         // SAFETY: filtering indices/values with same mask maintains their 1:1 relationship
-        let filtered_indices = self.indices.filter(filter_mask.clone())?;
-        let filtered_values = self.values.filter(filter_mask)?;
+        let filtered_indices = filter(&self.indices, &filter_mask)?;
+        let filtered_values = filter(&self.values, &filter_mask)?;
 
         Ok(Some(Self {
             array_len: self.array_len,
@@ -1148,8 +1149,10 @@ fn filter_patches_with_mask<T: IntegerPType>(
     }
 
     let new_patch_indices = new_patch_indices.into_array();
-    let new_patch_values =
-        patch_values.filter(Mask::from_indices(patch_values.len(), new_mask_indices))?;
+    let new_patch_values = filter(
+        patch_values,
+        &Mask::from_indices(patch_values.len(), new_mask_indices),
+    )?;
 
     Ok(Some(Patches::new(
         true_count,

vortex-array/src/validity.rs

@@ -181,17 +181,27 @@ impl Validity {
         }
     }
 
-    /// Keep only the entries for which the mask is true.
+    /// Lazily filters a [`Validity`] with a selection mask, which keeps only the entries for which
+    /// the mask is true.
     ///
     /// The result has length equal to the number of true values in mask.
+    ///
+    /// If the validity is a [`Validity::Array`], then this lazily wraps it in a `FilterArray`
+    /// instead of eagerly filtering the values immediately.
     pub fn filter(&self, mask: &Mask) -> VortexResult<Self> {
         // NOTE(ngates): we take the mask as a reference to avoid the caller cloning unnecessarily
         //  if we happen to be NonNullable, AllValid, or AllInvalid.
         match self {
             v @ (Validity::NonNullable | Validity::AllValid | Validity::AllInvalid) => {
                 Ok(v.clone())
             }
-            Validity::Array(arr) => Ok(Validity::Array(arr.filter(mask.clone())?)),
+            Validity::Array(arr) => Ok(Validity::Array(
+                arr.filter(mask.clone())?
+                    // TODO(connor): This is wrong!!! We should not be eagerly decompressing the
+                    // validity array.
+                    .to_canonical()?
+                    .into_array(),
+            )),
         }
     }
 

vortex-btrblocks/src/float.rs

@@ -571,7 +571,7 @@ mod tests {
             .display_as(DisplayOptions::MetadataOnly)
             .to_string()
             .to_lowercase();
-        assert_eq!(display, "vortex.primitive(f32?, len=96)");
+        assert_eq!(display, "vortex.sparse(f32?, len=96)");
 
         Ok(())
     }

vortex-btrblocks/src/string.rs

@@ -502,7 +502,7 @@ mod tests {
             .display_as(DisplayOptions::MetadataOnly)
             .to_string()
             .to_lowercase();
-        assert_eq!(display, "vortex.varbinview(utf8?, len=100)");
+        assert_eq!(display, "vortex.sparse(utf8?, len=100)");
 
         Ok(())
     }