Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@ where

// SAFETY: We initialize all `len` values below via `decode` and the patch loop.
unsafe {
uninit_range.append_mask(array.validity()?.execute_mask(len, ctx)?);
uninit_range.append_mask(&array.validity()?.execute_mask(len, ctx)?);
}

// SAFETY: `decode` writes a value to every slot in this range.
Expand Down
2 changes: 1 addition & 1 deletion encodings/fastlanes/src/for/array/for_decompress.rs
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ pub(crate) fn fused_decompress<
let mut uninit_range = builder.uninit_range(bp.len());
unsafe {
// Append a dense null Mask.
uninit_range.append_mask(bp.validity()?.execute_mask(bp.as_ref().len(), ctx)?);
uninit_range.append_mask(&bp.validity()?.execute_mask(bp.as_ref().len(), ctx)?);
}

// SAFETY: `decode_into` will initialize all values in this range.
Expand Down
5 changes: 2 additions & 3 deletions vortex-array/src/builders/bool.rs
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,7 @@ impl ArrayBuilder for BoolBuilder {

self.inner.append_buffer(&bool_array.to_bit_buffer());
self.nulls.append_validity_mask(
bool_array
&bool_array
.as_ref()
.validity()
.vortex_expect("validity_mask")
Expand All @@ -139,8 +139,7 @@ impl ArrayBuilder for BoolBuilder {
}

unsafe fn set_validity_unchecked(&mut self, validity: Mask) {
self.nulls = LazyBitBufferBuilder::new(validity.len());
self.nulls.append_validity_mask(validity);
self.nulls = LazyBitBufferBuilder::from_validity_mask(validity);
}

fn finish(&mut self) -> ArrayRef {
Expand Down
5 changes: 2 additions & 3 deletions vortex-array/src/builders/decimal.rs
Original file line number Diff line number Diff line change
Expand Up @@ -207,7 +207,7 @@ impl ArrayBuilder for DecimalBuilder {
});

self.nulls.append_validity_mask(
decimal_array
&decimal_array
.as_ref()
.validity()
.vortex_expect("validity_mask")
Expand All @@ -225,8 +225,7 @@ impl ArrayBuilder for DecimalBuilder {
}

unsafe fn set_validity_unchecked(&mut self, validity: Mask) {
self.nulls = LazyBitBufferBuilder::new(validity.len());
self.nulls.append_validity_mask(validity);
self.nulls = LazyBitBufferBuilder::from_validity_mask(validity);
}

fn finish(&mut self) -> ArrayRef {
Expand Down
5 changes: 2 additions & 3 deletions vortex-array/src/builders/fixed_size_list.rs
Original file line number Diff line number Diff line change
Expand Up @@ -246,7 +246,7 @@ impl ArrayBuilder for FixedSizeListBuilder {

self.elements_builder.extend_from_array(fsl.elements());
self.nulls.append_validity_mask(
array
&array
.validity()
.vortex_expect("validity_mask in extend_from_array_unchecked")
.execute_mask(array.len(), &mut LEGACY_SESSION.create_execution_ctx())
Expand All @@ -261,8 +261,7 @@ impl ArrayBuilder for FixedSizeListBuilder {
}

unsafe fn set_validity_unchecked(&mut self, validity: Mask) {
self.nulls = LazyBitBufferBuilder::new(validity.len());
self.nulls.append_validity_mask(validity);
self.nulls = LazyBitBufferBuilder::from_validity_mask(validity);
}

fn finish(&mut self) -> ArrayRef {
Expand Down
45 changes: 42 additions & 3 deletions vortex-array/src/builders/lazy_null_builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,42 @@ impl LazyBitBufferBuilder {
}
}

/// Creates a builder pre-populated from a validity mask, taking ownership of the mask's buffer
/// instead of copying it where possible.
///
/// This is the counterpart to [`append_validity_mask`](Self::append_validity_mask) for callers
/// that want to *replace* the builder's contents with the mask rather than extend them: because
/// we own the mask, we can move its buffer in instead of copying it.
pub fn from_validity_mask(validity_mask: Mask) -> Self {
match validity_mask {
// An unmaterialized builder already represents `len` non-null values, so an all-valid
// mask stays lazy.
Mask::AllTrue(len) => Self {
inner: None,
len,
capacity: len,
},
Mask::AllFalse(len) => Self::from_buffer(BitBufferMut::new_unset(len)),
// Take ownership of the underlying buffer; `into_bit_buffer` and `try_into_mut` only
// copy when the buffer is shared, otherwise this is a move.
values @ Mask::Values(_) => Self::from_buffer(
values
.into_bit_buffer()
.try_into_mut()
.unwrap_or_else(|buffer| BitBufferMut::copy_from(&buffer)),
),
}
}

/// Creates a builder backed by an already-materialized buffer.
fn from_buffer(inner: BitBufferMut) -> Self {
Self {
inner: Some(inner),
len: 0,
capacity: 0,
}
}

/// Appends `n` non-null values to the builder.
#[inline]
pub fn append_n_non_nulls(&mut self, n: usize) {
Expand Down Expand Up @@ -82,10 +118,13 @@ impl LazyBitBufferBuilder {
}

/// Appends values from a validity mask.
pub fn append_validity_mask(&mut self, validity_mask: Mask) {
///
/// Takes the mask by reference: the `Mask::Values` case copies the underlying buffer into the
/// running null buffer, so there is nothing to gain from owning the mask.
pub fn append_validity_mask(&mut self, validity_mask: &Mask) {
match validity_mask {
Mask::AllTrue(len) => self.append_n_non_nulls(len),
Mask::AllFalse(len) => self.append_n_nulls(len),
Mask::AllTrue(len) => self.append_n_non_nulls(*len),
Mask::AllFalse(len) => self.append_n_nulls(*len),
Mask::Values(is_valid) => self.append_buffer(is_valid.bit_buffer()),
}
}
Expand Down
5 changes: 2 additions & 3 deletions vortex-array/src/builders/list.rs
Original file line number Diff line number Diff line change
Expand Up @@ -226,7 +226,7 @@ impl<O: IntegerPType> ArrayBuilder for ListBuilder<O> {

// Append validity information.
self.nulls.append_validity_mask(
array
&array
.validity()
.vortex_expect("validity_mask in extend_from_array_unchecked")
.execute_mask(array.len(), &mut LEGACY_SESSION.create_execution_ctx())
Expand Down Expand Up @@ -300,8 +300,7 @@ impl<O: IntegerPType> ArrayBuilder for ListBuilder<O> {
}

unsafe fn set_validity_unchecked(&mut self, validity: Mask) {
self.nulls = LazyBitBufferBuilder::new(validity.len());
self.nulls.append_validity_mask(validity);
self.nulls = LazyBitBufferBuilder::from_validity_mask(validity);
}

fn finish(&mut self) -> ArrayRef {
Expand Down
5 changes: 2 additions & 3 deletions vortex-array/src/builders/listview.rs
Original file line number Diff line number Diff line change
Expand Up @@ -308,7 +308,7 @@ impl<O: IntegerPType, S: IntegerPType> ArrayBuilder for ListViewBuilder<O, S> {
debug_assert!(listview.is_zero_copy_to_list());

self.nulls.append_validity_mask(
array
&array
.validity()
.vortex_expect("validity_mask in extend_from_array_unchecked")
.execute_mask(array.len(), &mut LEGACY_SESSION.create_execution_ctx())
Expand Down Expand Up @@ -364,8 +364,7 @@ impl<O: IntegerPType, S: IntegerPType> ArrayBuilder for ListViewBuilder<O, S> {
}

unsafe fn set_validity_unchecked(&mut self, validity: Mask) {
self.nulls = LazyBitBufferBuilder::new(validity.len());
self.nulls.append_validity_mask(validity);
self.nulls = LazyBitBufferBuilder::from_validity_mask(validity);
}

fn finish(&mut self) -> ArrayRef {
Expand Down
17 changes: 8 additions & 9 deletions vortex-array/src/builders/primitive.rs
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@ impl<T: NativePType> PrimitiveBuilder<T> {
}

/// Extends the primitive array with an iterator.
pub fn extend_with_iterator(&mut self, iter: impl IntoIterator<Item = T>, mask: Mask) {
pub fn extend_with_iterator(&mut self, iter: impl IntoIterator<Item = T>, mask: &Mask) {
self.values.extend(iter);
self.nulls.append_validity_mask(mask);
}
Expand Down Expand Up @@ -190,7 +190,7 @@ impl<T: NativePType> ArrayBuilder for PrimitiveBuilder<T> {

self.values.extend_from_slice(array.as_slice::<T>());
self.nulls.append_validity_mask(
array
&array
.as_ref()
.validity()
.vortex_expect("validity_mask")
Expand All @@ -208,8 +208,7 @@ impl<T: NativePType> ArrayBuilder for PrimitiveBuilder<T> {
}

unsafe fn set_validity_unchecked(&mut self, validity: Mask) {
self.nulls = LazyBitBufferBuilder::new(validity.len());
self.nulls.append_validity_mask(validity);
self.nulls = LazyBitBufferBuilder::from_validity_mask(validity);
}

fn finish(&mut self) -> ArrayRef {
Expand Down Expand Up @@ -271,7 +270,7 @@ impl<T> UninitRange<'_, T> {
/// - The caller must ensure that they safely initialize `mask.len()` primitive values via
/// [`UninitRange::copy_from_slice`].
/// - The caller must also ensure that they only call this method once.
pub unsafe fn append_mask(&mut self, mask: Mask) {
pub unsafe fn append_mask(&mut self, mask: &Mask) {
assert_eq!(
mask.len(),
self.len,
Expand Down Expand Up @@ -426,7 +425,7 @@ mod tests {

// SAFETY: We're about to initialize the values.
unsafe {
range.append_mask(mask);
range.append_mask(&mask);
}

// Initialize the values.
Expand Down Expand Up @@ -476,7 +475,7 @@ mod tests {

// SAFETY: This is expected to panic due to length mismatch.
unsafe {
range.append_mask(wrong_mask);
range.append_mask(&wrong_mask);
}
}

Expand Down Expand Up @@ -522,7 +521,7 @@ mod tests {
let initial_mask = Mask::from_iter([false, false, false]);
// SAFETY: We're about to initialize the values.
unsafe {
range.append_mask(initial_mask);
range.append_mask(&initial_mask);
}

// Now we can use set_bit to modify individual bits with relative indexing.
Expand Down Expand Up @@ -623,7 +622,7 @@ mod tests {
let mask = Mask::from_iter([true, true, false]);
// SAFETY: We're about to initialize the matching number of values.
unsafe {
range.append_mask(mask);
range.append_mask(&mask);
}

// Initialize all values.
Expand Down
5 changes: 2 additions & 3 deletions vortex-array/src/builders/struct_.rs
Original file line number Diff line number Diff line change
Expand Up @@ -180,7 +180,7 @@ impl ArrayBuilder for StructBuilder {
}

self.nulls.append_validity_mask(
array
&array
.validity()
.vortex_expect("validity_mask")
.execute_mask(array.len(), &mut LEGACY_SESSION.create_execution_ctx())
Expand All @@ -196,8 +196,7 @@ impl ArrayBuilder for StructBuilder {
}

unsafe fn set_validity_unchecked(&mut self, validity: Mask) {
self.nulls = LazyBitBufferBuilder::new(validity.len());
self.nulls.append_validity_mask(validity);
self.nulls = LazyBitBufferBuilder::from_validity_mask(validity);
}

fn finish(&mut self) -> ArrayRef {
Expand Down
61 changes: 61 additions & 0 deletions vortex-array/src/builders/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ use std::sync::Arc;

use rstest::rstest;
use vortex_error::VortexExpect;
use vortex_error::VortexResult;
use vortex_mask::Mask;

use crate::LEGACY_SESSION;
use crate::VortexSessionExecute;
Expand Down Expand Up @@ -820,3 +822,62 @@ fn test_append_scalar_repeated_same_instance() {
);
}
}

/// Test that `set_validity` correctly overrides a builder's validity across all mask variants.
///
/// `set_validity` moves the mask's buffer into the builder rather than copying it, so the
/// `sliced_offset` case is important: slicing a `Mask::Values` at a non-byte-aligned boundary
/// yields a buffer with a non-zero bit offset, which the move path must preserve.
#[rstest]
#[case::all_true(Mask::new_true(8), vec![true; 8])]
#[case::all_false(Mask::new_false(8), vec![false; 8])]
#[case::values(
Mask::from_iter([true, false, true, true, false, false, true, false]),
vec![true, false, true, true, false, false, true, false]
)]
#[case::sliced_offset(
Mask::from_iter([
false, false, false, // dropped by the slice
true, false, true, true, false, false, true, false, // kept: indices 3..11
true, true, true, true, true, // dropped by the slice
])
.slice(3..11),
vec![true, false, true, true, false, false, true, false]
)]
fn test_set_validity_overrides_validity(
#[case] mask: Mask,
#[case] expected: Vec<bool>,
) -> VortexResult<()> {
let dtype = DType::Primitive(PType::I32, Nullability::Nullable);
let mut builder = builder_with_capacity(&dtype, mask.len());
builder.append_zeros(mask.len());

builder.set_validity(mask);

let validity = builder.finish().validity()?;
for (i, &valid) in expected.iter().enumerate() {
assert_eq!(
validity.is_valid(i)?,
valid,
"validity mismatch at index {i}"
);
}
Ok(())
}

/// Test that `set_validity` is a no-op on a non-nullable builder.
#[test]
fn test_set_validity_noop_when_non_nullable() -> VortexResult<()> {
let dtype = DType::Primitive(PType::I32, Nullability::NonNullable);
let mut builder = builder_with_capacity(&dtype, 4);
builder.append_zeros(4);

// Providing an all-false mask must not make the non-nullable array invalid.
builder.set_validity(Mask::new_false(4));

let validity = builder.finish().validity()?;
for i in 0..4 {
assert!(validity.is_valid(i)?, "index {i} should remain valid");
}
Ok(())
}
Loading
Loading