Skip to content

perf(array): accumulate nested builder validity without a null buffer [builders-child-stack] #8966

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
robert3005 wants to merge 2 commits into from
Closed

perf(array): accumulate nested builder validity without a null buffer [builders-child-stack] #8966

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ae28c03
perf(array): accumulate nested builder validity without a null buffer
claude Jul 25, 2026
b24a656
fixes
robert3005 Aug 5, 2026
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
5 changes: 0 additions & 5 deletions vortex-array/src/arrays/varbin/builder.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -630,10 +630,6 @@ impl<O: OffsetBuilderPType> ArrayBuilder for VarBinBuilder<O> {
self.validity.reserve(additional);
}

unsafe fn set_validity_unchecked(&mut self, validity: Mask) {
self.replace_validity(validity)
}

fn finish(&mut self) -> ArrayRef {
self.finish_into_varbin().into_array()
}
Expand Down Expand Up @@ -949,7 +945,6 @@ mod tests {
builder.append_scalar(&Scalar::utf8("hello", Nullable))?;
builder.append_null();
assert_eq!(builder.len(), 3);
builder.set_validity(validity.clone());
Ok(())
})?;
assert_eq!(result.validity()?.execute_mask(3, &mut ctx)?, validity);
Expand Down
5 changes: 0 additions & 5 deletions vortex-array/src/builders/bool.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ use std::mem;
use vortex_buffer::BitBufferMut;
use vortex_error::VortexResult;
use vortex_error::vortex_ensure;
use vortex_mask::Mask;

use crate::ArrayRef;
use crate::ExecutionCtx;
Expand Down Expand Up @@ -127,10 +126,6 @@ impl ArrayBuilder for BoolBuilder {
self.nulls.reserve_exact(additional);
}

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

fn finish(&mut self) -> ArrayRef {
self.finish_into_bool().into_array()
}
Expand Down
146 changes: 1 addition & 145 deletions vortex-array/src/builders/child.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,22 +1,17 @@
// SPDX-License-Identifier: Apache-2.0
// SPDX-FileCopyrightText: Copyright the Vortex contributors

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

use crate::ArrayRef;
use crate::ExecutionCtx;
use crate::IntoArray;
use crate::arrays::ChunkedArray;
use crate::arrays::MaskedArray;
use crate::builders::ArrayBuilder;
use crate::builders::builder_with_capacity;
use crate::dtype::DType;
use crate::dtype::Nullability;
use crate::scalar::Scalar;
use crate::validity::Validity;

/// Accumulates the child of a nested [`ArrayBuilder`] without canonicalizing appended arrays.
///
Expand Down Expand Up @@ -121,42 +116,6 @@ impl ChildBuilder {
self.pending.reserve_exact(additional)
}

/// Overrides the validity of every value appended so far.
///
/// # Safety
///
/// `validity` must have the same length as [`self.len()`](Self::len).
///
/// # Panics
///
/// Panics if a chunk that was kept in its original encoding contains nulls, since replacing
/// the validity of such a chunk would require decoding it.
pub unsafe fn set_validity_unchecked(&mut self, validity: Mask) {
if !self.dtype.is_nullable() {
return;
}

if self.chunks.is_empty() {
// Fast path: every value lives in the scalar builder, which owns its null buffer.
unsafe { self.pending.set_validity_unchecked(validity) };
return;
}

// The chunks carry their own validity, so the override has to be pushed into each of them.
self.flush_pending();
let mut offset = 0;
for chunk in &mut self.chunks {
let end = offset + chunk.len();
*chunk = MaskedArray::try_new(
chunk.clone(),
Validity::from_mask(validity.slice(offset..end), Nullability::Nullable),
)
.vortex_expect("cannot override the validity of a child chunk that contains nulls")
.into_array();
offset = end;
}
}

/// Finishes the child, combining the accumulated chunks into a [`ChunkedArray`] when there is
/// more than one of them.
pub fn finish(&mut self) -> ArrayRef {
Expand All @@ -172,9 +131,7 @@ impl ChildBuilder {
return chunks.remove(0);
}

ChunkedArray::try_new(chunks, self.dtype.clone())
.vortex_expect("every child chunk has the child dtype")
.into_array()
unsafe { ChunkedArray::new_unchecked(chunks, self.dtype.clone()) }.into_array()
}

/// Moves whatever the scalar builder holds into `chunks`, keeping the chunks in logical order.
Expand All @@ -192,9 +149,7 @@ impl ChildBuilder {
mod tests {
use rstest::rstest;
use vortex_buffer::buffer;
use vortex_error::VortexExpect;
use vortex_error::VortexResult;
use vortex_mask::Mask;

use super::ChildBuilder;
use crate::ArrayRef;
Expand All @@ -205,7 +160,6 @@ mod tests {
use crate::arrays::ChunkedArray;
use crate::arrays::Constant;
use crate::arrays::ConstantArray;
use crate::arrays::Masked;
use crate::arrays::Primitive;
use crate::arrays::PrimitiveArray;
use crate::arrays::chunked::ChunkedArrayExt;
Expand Down Expand Up @@ -380,104 +334,6 @@ mod tests {
Ok(())
}

/// Overriding the validity once chunks exist has to push the override into each chunk, sliced
/// to that chunk's own range.
#[test]
fn test_set_validity_pushes_the_override_into_every_chunk() -> VortexResult<()> {
let mut ctx = array_session().create_execution_ctx();
let dtype = DType::Primitive(I32, Nullable);
let mut builder = ChildBuilder::with_capacity(&dtype, 0);

builder.append_array(&nullable_constant(1, CHUNK_LEN), &mut ctx)?;
builder.append_array(&nullable_constant(2, CHUNK_LEN), &mut ctx)?;

// Straddle the chunk boundary, so an override sliced wrongly cannot pass.
let invalid = [CHUNK_LEN - 1, CHUNK_LEN];
let validity = Mask::from_iter((0..2 * CHUNK_LEN).map(|i| !invalid.contains(&i)));
unsafe { builder.set_validity_unchecked(validity) };

let child = builder.finish();
let chunked = child.as_::<Chunked>();
assert_eq!(chunked.nchunks(), 2);
// The override was layered over the chunks rather than decoding them.
assert!(chunked.iter_chunks().all(|chunk| chunk.is::<Masked>()));

let expected = PrimitiveArray::from_option_iter(
(0..2 * CHUNK_LEN)
.map(|i| (!invalid.contains(&i)).then_some(if i < CHUNK_LEN { 1i32 } else { 2 })),
)
.into_array();
assert_arrays_eq!(&child, &expected, &mut ctx);

Ok(())
}

/// Values still sitting in the scalar builder are part of the override too.
#[test]
fn test_set_validity_covers_pending_scalars() -> VortexResult<()> {
let mut ctx = array_session().create_execution_ctx();
let dtype = DType::Primitive(I32, Nullable);
let mut builder = ChildBuilder::with_capacity(&dtype, 0);

builder.append_array(&nullable_constant(1, CHUNK_LEN), &mut ctx)?;
builder.append_scalar(&Scalar::primitive(2i32, Nullable))?;

let mut validity = vec![true; CHUNK_LEN + 1];
validity[CHUNK_LEN] = false;
unsafe { builder.set_validity_unchecked(Mask::from_iter(validity)) };

let child = builder.finish();
let expected = PrimitiveArray::from_option_iter(
std::iter::repeat_n(Some(1i32), CHUNK_LEN).chain([None]),
)
.into_array();
assert_arrays_eq!(&child, &expected, &mut ctx);

Ok(())
}

/// A non-nullable child cannot carry nulls, so the override is dropped and the chunks are left
/// exactly as they were appended.
#[test]
fn test_set_validity_is_a_noop_for_a_non_nullable_child() -> VortexResult<()> {
let mut ctx = array_session().create_execution_ctx();
let mut builder = ChildBuilder::with_capacity(&DType::from(I32), 0);

builder.append_array(&constant(1, CHUNK_LEN), &mut ctx)?;
builder.append_array(&constant(2, CHUNK_LEN), &mut ctx)?;
unsafe { builder.set_validity_unchecked(Mask::new_false(2 * CHUNK_LEN)) };

let child = builder.finish();
let chunked = child.as_::<Chunked>();
assert!(chunked.iter_chunks().all(|chunk| chunk.is::<Constant>()));

let expected = ChunkedArray::try_new(
vec![constant(1, CHUNK_LEN), constant(2, CHUNK_LEN)],
DType::from(I32),
)?
.into_array();
assert_arrays_eq!(&child, &expected, &mut ctx);

Ok(())
}

/// Replacing the validity of a chunk that already contains nulls would mean decoding it, which
/// is exactly what the chunk exists to avoid.
#[test]
#[should_panic(expected = "cannot override the validity of a child chunk that contains nulls")]
fn test_set_validity_rejects_a_chunk_that_contains_nulls() {
let mut ctx = array_session().create_execution_ctx();
let dtype = DType::Primitive(I32, Nullable);
let mut builder = ChildBuilder::with_capacity(&dtype, 0);

let with_nulls = ConstantArray::new(Scalar::null(dtype), CHUNK_LEN).into_array();
builder
.append_array(&with_nulls, &mut ctx)
.vortex_expect("append");

unsafe { builder.set_validity_unchecked(Mask::new_true(CHUNK_LEN)) };
}

#[test]
fn test_finish_resets_the_builder() -> VortexResult<()> {
let mut ctx = array_session().create_execution_ctx();
Expand Down
5 changes: 0 additions & 5 deletions vortex-array/src/builders/decimal.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ use vortex_error::VortexResult;
use vortex_error::vortex_ensure;
use vortex_error::vortex_err;
use vortex_error::vortex_panic;
use vortex_mask::Mask;

use crate::ArrayRef;
use crate::ExecutionCtx;
Expand Down Expand Up @@ -219,10 +218,6 @@ impl ArrayBuilder for DecimalBuilder {
self.nulls.reserve_exact(additional);
}

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

fn finish(&mut self) -> ArrayRef {
self.finish_into_decimal().into_array()
}
Expand Down
5 changes: 0 additions & 5 deletions vortex-array/src/builders/extension.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ use std::any::Any;

use vortex_error::VortexResult;
use vortex_error::vortex_ensure;
use vortex_mask::Mask;

use crate::ArrayRef;
use crate::ExecutionCtx;
Expand Down Expand Up @@ -114,10 +113,6 @@ impl ArrayBuilder for ExtensionBuilder {
self.storage.reserve_exact(capacity)
}

unsafe fn set_validity_unchecked(&mut self, validity: Mask) {
unsafe { self.storage.set_validity_unchecked(validity) };
}

fn finish(&mut self) -> ArrayRef {
self.finish_into_extension().into_array()
}
Expand Down
14 changes: 4 additions & 10 deletions vortex-array/src/builders/fixed_size_list.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ use vortex_error::VortexResult;
use vortex_error::vortex_bail;
use vortex_error::vortex_ensure;
use vortex_error::vortex_panic;
use vortex_mask::Mask;

use crate::ArrayRef;
use crate::ExecutionCtx;
Expand All @@ -19,7 +18,7 @@ use crate::arrays::fixed_size_list::FixedSizeListArraySlotsExt;
use crate::builders::ArrayBuilder;
use crate::builders::ChildBuilder;
use crate::builders::DEFAULT_BUILDER_CAPACITY;
use crate::builders::LazyBitBufferBuilder;
use crate::builders::ValidityBuilder;
use crate::canonical::Canonical;
use crate::dtype::DType;
use crate::dtype::Nullability;
Expand All @@ -39,7 +38,7 @@ pub struct FixedSizeListBuilder {
/// The null map builder of the [`FixedSizeListArray`].
///
/// We also use this type to store the length of the final output array.
nulls: LazyBitBufferBuilder,
nulls: ValidityBuilder,
}

impl FixedSizeListBuilder {
Expand All @@ -64,7 +63,7 @@ impl FixedSizeListBuilder {

let elements_builder = ChildBuilder::with_capacity(&element_dtype, elements_capacity);
let fsl_dtype = DType::FixedSizeList(element_dtype, list_size, nullability);
let nulls = LazyBitBufferBuilder::new(capacity);
let nulls = ValidityBuilder::new(capacity);

Self {
dtype: fsl_dtype,
Expand Down Expand Up @@ -116,8 +115,7 @@ impl FixedSizeListBuilder {
}

self.elements_builder.append_array(array.elements(), ctx)?;
self.nulls
.append_validity_mask(&array.validity()?.execute_mask(array.len(), ctx)?);
self.nulls.append_validity(array.validity()?, array.len());
Ok(())
}

Expand Down Expand Up @@ -261,10 +259,6 @@ impl ArrayBuilder for FixedSizeListBuilder {
self.nulls.reserve_exact(additional);
}

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

fn finish(&mut self) -> ArrayRef {
self.finish_into_fixed_size_list().into_array()
}
Expand Down
36 changes: 0 additions & 36 deletions vortex-array/src/builders/lazy_null_builder.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,42 +31,6 @@ 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
Loading
Loading