diff --git a/vortex-array/src/arrays/bool/compute/min_max.rs b/vortex-array/src/arrays/bool/compute/min_max.rs index 3752fed811d..7e03c278a0d 100644 --- a/vortex-array/src/arrays/bool/compute/min_max.rs +++ b/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> { - 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()), + }) + ); + } +}