fix: Handle nulls in bool array min/max (#5163)

fix #5157

---------

Signed-off-by: Joe Isaacs <[email protected]>
Signed-off-by: Robert Kruszewski <[email protected]>
Co-authored-by: Joe Isaacs <[email protected]>

Author: robert3005

vortex-array/src/arrays/bool/compute/min_max.rs

@@ -13,39 +13,106 @@ use crate::register_kernel;
 
 impl MinMaxKernel for BoolVTable {
     fn min_max(&self, array: &BoolArray) -> VortexResult<Option<MinMaxResult>> {
-        let x = match array.validity_mask() {
+        let mask = array.validity_mask();
+        let true_non_null = match mask {
             Mask::AllTrue(_) => array.bit_buffer().clone(),
             Mask::AllFalse(_) => return Ok(None),
-            Mask::Values(v) => array.bit_buffer().bitand(v.bit_buffer()),
+            Mask::Values(ref v) => array.bit_buffer().bitand(v.bit_buffer()),
         };
 
         // TODO(ngates): we should be able to bail out earlier as soon as we have one true and
         //  one false value.
-        let mut slices = x.set_slices();
+        let mut true_slices = true_non_null.set_slices();
         // If there are no slices, then all values are false
         // if there is a single slice that covers the entire array, then all values are true
         // otherwise, we have a mix of true and false values
 
-        let Some(slice) = slices.next() else {
+        let Some(slice) = true_slices.next() else {
             // all false
             return Ok(Some(MinMaxResult {
-                min: Scalar::new(array.dtype().clone(), false.into()),
-                max: Scalar::new(array.dtype().clone(), false.into()),
+                min: Scalar::bool(false, array.dtype().nullability()),
+                max: Scalar::bool(false, array.dtype().nullability()),
             }));
         };
-        if slice.0 == 0 && slice.1 == x.len() {
+        if slice.0 == 0 && slice.1 == array.len() {
             // all true
             return Ok(Some(MinMaxResult {
-                min: Scalar::new(array.dtype().clone(), true.into()),
-                max: Scalar::new(array.dtype().clone(), true.into()),
+                min: Scalar::bool(true, array.dtype().nullability()),
+                max: Scalar::bool(true, array.dtype().nullability()),
             }));
         };
 
+        // If the non null true slice doesn't cover the whole array we need to check for valid false values
+        match mask {
+            // if the mask is all true or all false we don't need to look for false values
+            Mask::AllTrue(_) | Mask::AllFalse(_) => {}
+            Mask::Values(v) => {
+                let false_non_null = (!array.bit_buffer()).bitand(v.bit_buffer());
+                let mut false_slices = false_non_null.set_slices();
+
+                let Some(_) = false_slices.next() else {
+                    // In this case we don't have any false values which means we are all true and null
+                    return Ok(Some(MinMaxResult {
+                        min: Scalar::bool(true, array.dtype().nullability()),
+                        max: Scalar::bool(true, array.dtype().nullability()),
+                    }));
+                };
+            }
+        }
+
         Ok(Some(MinMaxResult {
-            min: Scalar::new(array.dtype().clone(), false.into()),
-            max: Scalar::new(array.dtype().clone(), true.into()),
+            min: Scalar::bool(false, array.dtype().nullability()),
+            max: Scalar::bool(true, array.dtype().nullability()),
         }))
     }
 }
 
 register_kernel!(MinMaxKernelAdapter(BoolVTable).lift());
+
+#[cfg(test)]
+mod tests {
+    use vortex_dtype::{DType, Nullability};
+    use vortex_scalar::Scalar;
+
+    use crate::arrays::BoolArray;
+    use crate::compute::{MinMaxResult, min_max};
+
+    #[test]
+    fn test_min_max_nulls() {
+        let dtype = DType::Bool(Nullability::Nullable);
+        assert_eq!(
+            min_max(BoolArray::from_iter(vec![Some(true), Some(true), None, None]).as_ref())
+                .unwrap(),
+            Some(MinMaxResult {
+                min: Scalar::new(dtype.clone(), true.into()),
+                max: Scalar::new(dtype.clone(), true.into()),
+            })
+        );
+
+        assert_eq!(
+            min_max(BoolArray::from_iter(vec![None, Some(true), Some(true)]).as_ref()).unwrap(),
+            Some(MinMaxResult {
+                min: Scalar::new(dtype.clone(), true.into()),
+                max: Scalar::new(dtype.clone(), true.into()),
+            })
+        );
+
+        assert_eq!(
+            min_max(BoolArray::from_iter(vec![None, Some(true), Some(true), None]).as_ref())
+                .unwrap(),
+            Some(MinMaxResult {
+                min: Scalar::new(dtype.clone(), true.into()),
+                max: Scalar::new(dtype.clone(), true.into()),
+            })
+        );
+
+        assert_eq!(
+            min_max(BoolArray::from_iter(vec![Some(false), Some(false), None, None]).as_ref())
+                .unwrap(),
+            Some(MinMaxResult {
+                min: Scalar::new(dtype.clone(), false.into()),
+                max: Scalar::new(dtype, false.into()),
+            })
+        );
+    }
+}