fix[mask]: intersect_by_rank and bit to arrow (#5545)

Signed-off-by: Joe Isaacs <[email protected]>

Author: joseph-isaacs

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

@@ -247,8 +247,8 @@ impl FromIterator<Option<bool>> for BoolArray {
 
 impl IntoArray for BitBuffer {
     fn into_array(self) -> ArrayRef {
-        let len = self.len();
-        BoolArray::try_new(self.into_inner(), 0, len, Validity::NonNullable)
+        let (offset, len, buffer) = self.into_inner();
+        BoolArray::try_new(buffer, offset, len, Validity::NonNullable)
             .vortex_expect("known correct")
             .into_array()
     }

vortex-buffer/src/bit/arrow.rs

@@ -22,9 +22,7 @@ impl From<BooleanBuffer> for BitBuffer {
 
 impl From<BitBuffer> for BooleanBuffer {
     fn from(value: BitBuffer) -> Self {
-        let offset = value.offset();
-        let len = value.len();
-        let buffer = value.into_inner();
+        let (offset, len, buffer) = value.into_inner();
 
         BooleanBuffer::new(buffer.into_arrow_buffer(), offset, len)
     }

vortex-buffer/src/bit/buf.rs

@@ -4,6 +4,7 @@
 use std::ops::BitAnd;
 use std::ops::BitOr;
 use std::ops::BitXor;
+use std::ops::Bound;
 use std::ops::Not;
 use std::ops::RangeBounds;
 
@@ -195,14 +196,14 @@ impl BitBuffer {
     /// Panics if the slice would extend beyond the end of the buffer.
     pub fn slice(&self, range: impl RangeBounds<usize>) -> Self {
         let start = match range.start_bound() {
-            std::ops::Bound::Included(&s) => s,
-            std::ops::Bound::Excluded(&s) => s + 1,
-            std::ops::Bound::Unbounded => 0,
+            Bound::Included(&s) => s,
+            Bound::Excluded(&s) => s + 1,
+            Bound::Unbounded => 0,
         };
         let end = match range.end_bound() {
-            std::ops::Bound::Included(&e) => e + 1,
-            std::ops::Bound::Excluded(&e) => e,
-            std::ops::Bound::Unbounded => self.len,
+            Bound::Included(&e) => e + 1,
+            Bound::Excluded(&e) => e,
+            Bound::Unbounded => self.len,
         };
 
         assert!(start <= end);
@@ -215,9 +216,13 @@ impl BitBuffer {
 
     /// Slice any full bytes from the buffer, leaving the offset < 8.
     pub fn shrink_offset(self) -> Self {
+        let word_start = self.offset / 8;
+        let word_end = (self.offset + self.len).div_ceil(8);
+
+        let buffer = self.buffer.slice(word_start..word_end);
+
         let bit_offset = self.offset % 8;
         let len = self.len;
-        let buffer = self.into_inner();
         BitBuffer::new_with_offset(buffer, len, bit_offset)
     }
 
@@ -273,13 +278,9 @@ impl BitBuffer {
 // Conversions
 
 impl BitBuffer {
-    /// Consumes this `BoolBuffer` and returns the backing `Buffer<u8>` with any offset
-    /// and length information applied.
-    pub fn into_inner(self) -> ByteBuffer {
-        let word_start = self.offset / 8;
-        let word_end = (self.offset + self.len).div_ceil(8);
-
-        self.buffer.slice(word_start..word_end)
+    /// Returns the offset, len and underlying buffer.
+    pub fn into_inner(self) -> (usize, usize, ByteBuffer) {
+        (self.offset, self.len, self.buffer)
     }
 
     /// Attempt to convert this `BitBuffer` into a mutable version.
@@ -295,11 +296,9 @@ impl BitBuffer {
     /// If the caller doesn't hold only reference to the underlying buffer, a copy is created.
     /// The second value of the tuple is a bit_offset of the first value in the first byte
     pub fn into_mut(self) -> BitBufferMut {
-        let offset = self.offset;
-        let len = self.len;
+        let (offset, len, inner) = self.into_inner();
         // TODO(robert): if we are copying here we could strip offset bits
-        let inner = self.into_inner().into_mut();
-        BitBufferMut::from_buffer(inner, offset, len)
+        BitBufferMut::from_buffer(inner.into_mut(), offset, len)
     }
 }
 

vortex-mask/src/intersect_by_rank.rs

@@ -32,8 +32,8 @@ impl Mask {
         match (self.indices(), mask.indices()) {
             (AllOr::All, _) => mask.clone(),
             (_, AllOr::All) => self.clone(),
-            (AllOr::None, _) => Self::new_false(0),
-            (_, AllOr::None) => Self::new_false(self.len()),
+            (AllOr::None, _) | (_, AllOr::None) => Self::new_false(self.len()),
+
             (AllOr::Some(self_indices), AllOr::Some(mask_indices)) => {
                 Self::from_indices(
                     self.len(),
@@ -95,6 +95,13 @@ mod test {
         assert_eq!(this.intersect_by_rank(&mask), Mask::from_indices(5, vec![]));
     }
 
+    #[test]
+    fn mask_intersect_by_rank_all_false() {
+        let this = Mask::AllFalse(10);
+        let mask = Mask::AllFalse(0);
+        assert_eq!(this.intersect_by_rank(&mask), Mask::AllFalse(10));
+    }
+
     #[rstest]
     #[case::all_true_with_all_true(
         Mask::new_true(5),

vortex-vector/src/vector_ops.rs

@@ -5,6 +5,7 @@
 //! [`VectorMut`], respectively.
 
 use std::fmt::Debug;
+use std::ops::Bound;
 use std::ops::RangeBounds;
 
 use vortex_mask::Mask;
@@ -178,8 +179,6 @@ pub trait VectorMutOps: private::Sealed + Into<VectorMut> + Sized {
 
 /// Converts a range bounds into a length, given the total length of the vector.
 pub(crate) fn range_bounds_to_len(bounds: impl RangeBounds<usize> + Debug, len: usize) -> usize {
-    use std::ops::Bound;
-
     let start = match bounds.start_bound() {
         Bound::Included(&s) => s,
         Bound::Excluded(&s) => s + 1,