Address Kleene boolean review feedback

Signed-off-by: Nicholas Gates <nick@nickgates.com>

Author: gatesn

vortex-array/src/scalar_fn/fns/binary/boolean.rs

@@ -6,6 +6,7 @@ use std::iter::repeat_n;
 use arrow_array::cast::AsArray;
 use vortex_buffer::BitBuffer;
 use vortex_buffer::BufferMut;
+use vortex_buffer::read_u64_le;
 use vortex_error::VortexResult;
 use vortex_error::vortex_err;
 use vortex_mask::AllOr;
@@ -37,11 +38,14 @@ use crate::scalar_fn::fns::binary::Binary;
 use crate::scalar_fn::fns::operators::Operator;
 use crate::validity::Validity;
 
-/// Trait for encoding-specific Kleene boolean kernels that operate in encoded space.
+/// Trait for encoding-specific boolean kernels that operate in encoded space.
 ///
 /// Implementations receive the encoded array as the left operand. `rhs` may be any boolean array
 /// encoding or a constant; implementations should return `Ok(None)` when they cannot handle that
 /// operand without falling back to ordinary execution.
+///
+/// Vortex's boolean [`Operator::And`] and [`Operator::Or`] variants use Kleene semantics; there is
+/// no separate two-valued boolean operator path to dispatch here.
 pub trait BooleanKernel: VTable {
     /// Execute `lhs <operator> rhs` using Kleene boolean semantics.
     fn boolean(
@@ -268,7 +272,7 @@ pub fn boolean_buffers(
             Operator::Or => lhs_values | &rhs_values,
             other => return Err(vortex_err!("Not a boolean operator: {other}")),
         };
-        return Ok(BoolArray::try_new(values, all_validity(nullability))?.into_array());
+        return Ok(BoolArray::try_new(values, Validity::from(nullability))?.into_array());
     }
 
     let lhs_valid = lhs_validity.execute_mask(len, ctx)?;
@@ -318,14 +322,14 @@ pub fn boolean_buffer_scalar(
                 .bitand_not(&Mask::from_buffer(values));
             BoolArray::try_new(
                 BitBuffer::new_unset(len),
-                mask_to_validity(valid, nullability),
+                Validity::from_mask(valid, nullability),
             )?
         }
         (Operator::Or, None) => {
             let valid = validity.execute_mask(len, ctx)? & &Mask::from_buffer(values);
             BoolArray::try_new(
                 BitBuffer::new_set(len),
-                mask_to_validity(valid, nullability),
+                Validity::from_mask(valid, nullability),
             )?
         }
         (other, _) => return Err(vortex_err!("Not a boolean operator: {other}")),
@@ -430,16 +434,16 @@ fn fused_boolean_buffers_aligned(
     operator: Operator,
     nullability: Nullability,
 ) -> VortexResult<Option<ArrayRef>> {
-    let Some(lhs_values) = word_source_from_bit_buffer(lhs_values, len) else {
+    let Some(lhs_values) = word_source_from_bit_buffer(lhs_values) else {
         return Ok(None);
     };
-    let Some(rhs_values) = word_source_from_bit_buffer(rhs_values, len) else {
+    let Some(rhs_values) = word_source_from_bit_buffer(rhs_values) else {
         return Ok(None);
     };
-    let Some(lhs_validity) = word_source_from_mask(lhs_validity, len) else {
+    let Some(lhs_validity) = word_source_from_mask(lhs_validity) else {
         return Ok(None);
     };
-    let Some(rhs_validity) = word_source_from_mask(rhs_validity, len) else {
+    let Some(rhs_validity) = word_source_from_mask(rhs_validity) else {
         return Ok(None);
     };
 
@@ -454,32 +458,18 @@ fn fused_boolean_buffers_aligned(
     )?))
 }
 
-fn word_source_from_bit_buffer(buffer: &BitBuffer, len: usize) -> Option<WordSource<'_>> {
-    if !buffer.offset().is_multiple_of(8) {
-        return None;
-    }
-
-    let n_bytes = len.div_ceil(8);
-    let start = buffer.offset() / 8;
-    let end = start + n_bytes;
-    Some(WordSource::Bytes(&buffer.inner().as_slice()[start..end]))
+fn word_source_from_bit_buffer(buffer: &BitBuffer) -> Option<WordSource<'_>> {
+    buffer.byte_aligned_bytes().map(WordSource::Bytes)
 }
 
-fn word_source_from_mask(mask: &Mask, len: usize) -> Option<WordSource<'_>> {
+fn word_source_from_mask(mask: &Mask) -> Option<WordSource<'_>> {
     match mask.bit_buffer() {
         AllOr::All => Some(WordSource::Fill(u64::MAX)),
         AllOr::None => Some(WordSource::Fill(0)),
-        AllOr::Some(buffer) => word_source_from_bit_buffer(buffer, len),
+        AllOr::Some(buffer) => word_source_from_bit_buffer(buffer),
     }
 }
 
-fn read_u64_le(bytes: &[u8]) -> u64 {
-    debug_assert!(bytes.len() <= 8);
-    let mut buf = [0; 8];
-    buf[..bytes.len()].copy_from_slice(bytes);
-    u64::from_le_bytes(buf)
-}
-
 fn fused_boolean_word_sources(
     len: usize,
     lhs_words: WordSource<'_>,
@@ -581,7 +571,7 @@ fn finish_fused_boolean_words(
     validity.truncate(n_bytes);
     Ok(BoolArray::try_new(
         BitBuffer::new(values.freeze(), len),
-        mask_to_validity(
+        Validity::from_mask(
             Mask::from_buffer(BitBuffer::new(validity.freeze(), len)),
             nullability,
         ),
@@ -651,7 +641,7 @@ where
 
     Ok(BoolArray::try_new(
         values,
-        mask_to_validity(Mask::from_buffer(validity), nullability),
+        Validity::from_mask(Mask::from_buffer(validity), nullability),
     )?
     .into_array())
 }
@@ -677,23 +667,6 @@ fn boolean_nullability(lhs: &ArrayRef, rhs: &ArrayRef) -> Nullability {
     lhs.dtype().nullability() | rhs.dtype().nullability()
 }
 
-fn all_validity(nullability: Nullability) -> Validity {
-    match nullability {
-        Nullability::NonNullable => Validity::NonNullable,
-        Nullability::Nullable => Validity::AllValid,
-    }
-}
-
-fn mask_to_validity(mask: Mask, nullability: Nullability) -> Validity {
-    match nullability {
-        Nullability::NonNullable => {
-            debug_assert!(mask.all_true());
-            Validity::NonNullable
-        }
-        Nullability::Nullable => Validity::from(mask.into_bit_buffer()),
-    }
-}
-
 #[inline]
 fn is_boolean_operator(operator: Operator) -> bool {
     matches!(operator, Operator::And | Operator::Or)

vortex-buffer/src/bit/buf.rs

@@ -272,6 +272,23 @@ impl BitBuffer {
         &self.buffer
     }
 
+    /// Return the backing bytes for this bit buffer when its logical offset is byte-aligned.
+    ///
+    /// The returned slice contains exactly `self.len().div_ceil(8)` bytes. Bits past the logical
+    /// length in the final byte are outside the buffer's logical range and should be ignored by
+    /// callers.
+    #[inline]
+    pub fn byte_aligned_bytes(&self) -> Option<&[u8]> {
+        if !self.offset.is_multiple_of(8) {
+            return None;
+        }
+
+        let n_bytes = self.len.div_ceil(8);
+        let start = self.offset / 8;
+        let end = start + n_bytes;
+        Some(&self.buffer.as_slice()[start..end])
+    }
+
     /// Retrieve the value at the given index.
     ///
     /// Panics if the index is out of bounds.
@@ -692,6 +709,19 @@ mod tests {
         assert_eq!(sliced.offset(), 2);
     }
 
+    #[test]
+    fn test_byte_aligned_bytes() {
+        let bytes: ByteBuffer = buffer![0b1010_0101u8, 0b0000_0011];
+        let buf = BitBuffer::new(bytes.clone(), 10);
+        assert_eq!(buf.byte_aligned_bytes(), Some(bytes.as_slice()));
+
+        let byte_sliced = buf.slice(8..10);
+        assert_eq!(byte_sliced.byte_aligned_bytes(), Some(&[0b0000_0011][..]));
+
+        let bit_sliced = buf.slice(1..9);
+        assert!(bit_sliced.byte_aligned_bytes().is_none());
+    }
+
     #[test]
     fn test_from_indices_dense_crosses_words() {
         let len = 130;

vortex-buffer/src/bit/mod.rs

@@ -74,6 +74,19 @@ where
     }
 }
 
+/// Read up to 8 bytes as a little-endian `u64`, zero-padding the high bytes when fewer than 8
+/// bytes are supplied.
+///
+/// This preserves Vortex's least-significant-bit-first bitmap numbering on little- and big-endian
+/// targets. For a full 8-byte slice it lowers to a single word load.
+#[inline]
+pub fn read_u64_le(bytes: &[u8]) -> u64 {
+    debug_assert!(bytes.len() <= 8);
+    let mut buf = [0u8; 8];
+    buf[..bytes.len()].copy_from_slice(bytes);
+    u64::from_le_bytes(buf)
+}
+
 /// Splice a packed word `w` (whose bits above the highest valid bit are zero) into
 /// `words` at the given bit position.
 ///
@@ -178,6 +191,7 @@ pub unsafe fn unset_bit_unchecked(buf: *mut u8, index: usize) {
 mod tests {
     use super::collect_bool_word;
     use super::pack_bools_into_words;
+    use super::read_u64_le;
 
     #[test]
     fn collect_bool_word_packs_lsb_first() {
@@ -190,6 +204,12 @@ mod tests {
         assert_eq!(collect_bool_word(0, |_| true), 0);
     }
 
+    #[test]
+    fn read_u64_le_zero_pads_tail() {
+        assert_eq!(read_u64_le(&[0x34, 0x12]), 0x1234);
+        assert_eq!(read_u64_le(&[0xff; 8]), u64::MAX);
+    }
+
     #[test]
     #[should_panic(expected = "cannot pack 65 bits into a u64 word")]
     fn collect_bool_word_rejects_too_many_bits() {

vortex-buffer/src/bit/ops.rs

@@ -7,17 +7,7 @@ use crate::BitBuffer;
 use crate::BitBufferMut;
 use crate::BufferMut;
 use crate::ByteBufferMut;
-
-/// Read up to 8 bytes as a little-endian `u64`, zero-padding the high bytes when fewer than 8 are
-/// supplied. Using [`u64::from_le_bytes`] keeps the bit-numbering identical on little- and
-/// big-endian targets; for a full 8-byte slice it lowers to a single word load.
-#[inline]
-fn read_u64_le(bytes: &[u8]) -> u64 {
-    debug_assert!(bytes.len() <= 8);
-    let mut buf = [0u8; 8];
-    buf[..bytes.len()].copy_from_slice(bytes);
-    u64::from_le_bytes(buf)
-}
+use crate::read_u64_le;
 
 trait BitWordTarget {
     fn byte_len(&self) -> usize;