perf(sparse): build the fixed-size-list fill tile once, not once per gap

`append_fill` handed the fill scalar to `Constant::append_to_builder` per gap,
which materialized the scalar's elements into an array every time. Hoist that
out of the loop the way the list path already does: the fill's elements become
an array once, up front, and every gap tiles that same array through
`FixedSizeListBuilder::append_array_as_repeated_list`.

A fixed-size list holds its elements back to back, so it cannot point a run of
rows at one shared range the way a list view can - but it need not copy the
tile per row either. The run goes in as a `ChunkedArray` of `n` clones of the
tile, which costs `n` reference bumps and no element data at all. The child
keeps that whole run as a single chunk: unpacking it would spill a chunk per
row into the child's chunk list, which every later append and the final
`finish` walk, and that list is what makes the difference - unpacking measured
36.0 us against 26.0 us on `(512, 7, 4)`.

Elements that are all the same scalar do better still, collapsing to a single
constant chunk however many rows they cover.

`canonicalize_sparse_fixed_size_list` medians, against develop:

  (512, 7, 4)      21.1 us -> 26.0 us   (was 79.0 us)
  (1024, 17, 8)    29.1 us -> 22.7 us   (was 139.5 us)
  (8192, 1024, 4) 150.1 us -> 34.5 us   (was 375.9 us)

Signed-off-by: Robert Kruszewski <robert@spiraldb.com>

Author: robert3005

encodings/sparse/src/canonical.rs

@@ -368,6 +368,12 @@ fn execute_sparse_fixed_size_list_inner<I: IntegerPType>(
         nullability,
         array_len,
     );
+    // The fill's elements become an array once, up front, so that a gap does not rebuild them.
+    // They are tiled per row rather than shared - a fixed-size list holds its elements back to
+    // back - unless they are all the same scalar, in which case the tile stays constant-encoded
+    // and the tiling costs nothing.
+    let fill_elements = fixed_size_list_fill_tile(fill_value.as_list(), list_size);
+
     // One mask for the whole patch array rather than a validity lookup per patch.
     let patch_validity = values
         .validity()
@@ -384,7 +390,12 @@ fn execute_sparse_fixed_size_list_inner<I: IntegerPType>(
         let sparse_idx = sparse_idx
             .to_usize()
             .vortex_expect("patch index must fit in usize");
-        append_fill(&mut builder, fill_value, sparse_idx - next_index, ctx);
+        append_fixed_size_list_fill(
+            &mut builder,
+            fill_elements.as_ref(),
+            sparse_idx - next_index,
+            ctx,
+        );
 
         // Take each patch's elements rather than slicing the patch array itself: slicing a
         // `FixedSizeList` slices its elements and its validity, and every one of those slices pays
@@ -404,38 +415,56 @@ fn execute_sparse_fixed_size_list_inner<I: IntegerPType>(
     }
 
     // Fill remaining positions after last patch.
-    append_fill(&mut builder, fill_value, array_len - next_index, ctx);
+    append_fixed_size_list_fill(
+        &mut builder,
+        fill_elements.as_ref(),
+        array_len - next_index,
+        ctx,
+    );
 
     builder.finish_into_fixed_size_list()
 }
 
-/// Appends the run of `count` fill values that covers the gap before the next patch.
+/// Materializes the elements a fixed-size-list fill value covers each of its rows with, or `None`
+/// if the fill is null.
 ///
-/// The run goes in as a single [`ConstantArray`] rather than one appended value per row. For a
-/// list dtype that is the difference between storing the fill value once per gap and once per row:
-/// canonicalizing a constant list array points every view at one copy of the value, and the
-/// builder keeps that layout.
-fn append_fill(
-    builder: &mut dyn ArrayBuilder,
-    fill_value: &Scalar,
+/// Elements that are all the same scalar stay a constant array, so tiling them over a gap costs
+/// nothing however many rows it covers.
+fn fixed_size_list_fill_tile(fill: ListScalar, list_size: u32) -> Option<ArrayRef> {
+    let elements = fill.elements()?;
+
+    Some(match elements.iter().all_equal_value() {
+        Ok(uniform) => ConstantArray::new(uniform.clone(), list_size as usize).into_array(),
+        Err(_) => {
+            let mut builder = builder_with_capacity(fill.element_dtype(), elements.len());
+            for element in &elements {
+                builder
+                    .append_scalar(element)
+                    .vortex_expect("fixed-size-list element scalar was invalid");
+            }
+            builder.finish()
+        }
+    })
+}
+
+/// Appends the run of `count` fill lists that covers the gap before the next patch.
+fn append_fixed_size_list_fill(
+    builder: &mut FixedSizeListBuilder,
+    fill_elements: Option<&ArrayRef>,
     count: usize,
     ctx: &mut ExecutionCtx,
 ) {
     if count == 0 {
         return;
     }
 
-    if fill_value.is_null() {
-        // A null fill has no elements to share, and the builder can record the nulls without
-        // going through an array at all.
-        builder.append_nulls(count);
-        return;
+    match fill_elements {
+        Some(fill_elements) => builder
+            .append_array_as_repeated_list(fill_elements, count, ctx)
+            .vortex_expect("Failed to append sparse fixed-size-list fill value"),
+        // A null fill has no elements of its own, only the placeholders the builder writes.
+        None => builder.append_nulls(count),
     }
-
-    ConstantArray::new(fill_value.clone(), count)
-        .into_array()
-        .append_to_builder(builder, ctx)
-        .vortex_expect("Failed to append sparse fill value");
 }
 
 fn execute_sparse_bools(

vortex-array/src/builders/child.rs

@@ -68,6 +68,12 @@ impl ChildBuilder {
     /// at a boundary only the caller can see. Callers that would rather have the values copied
     /// should append them as scalars.
     ///
+    /// An appended [`ChunkedArray`] stays one chunk rather than giving up its own. Unpacking it
+    /// would spill a chunk per row into this child for a caller expressing a repeated run as
+    /// chunks - a tiled fixed-size list, say - and the child's chunk list is what every later
+    /// append and the final [`finish`](Self::finish) walk. Nesting costs one level, not one per
+    /// appended array.
+    ///
     /// Nothing is decoded here, so `_ctx` goes unused; it stays in the signature so that the
     /// nested builders forwarding their [`ExecutionCtx`] here do not have to explain why they
     /// don't.
@@ -85,34 +91,12 @@ impl ChildBuilder {
 
         self.flush_pending();
         self.chunks_len += array.len();
+
         self.chunks.push(array.clone());
 
         Ok(())
     }
 
-    /// Appends the *values* of `array` to the child, copying them instead of keeping the array as a
-    /// chunk of its own.
-    ///
-    /// [`append_array`](Self::append_array) is the right choice almost always: it references the
-    /// array rather than decoding it. This is for the caller that would otherwise append the same
-    /// tiny array over and over - the elements of one fixed-size list, repeated for a run of rows -
-    /// where a chunk per append costs more in indirection than copying the values costs outright.
-    /// Only the caller can see that, which is why the builder does not guess at it.
-    pub fn append_array_values(
-        &mut self,
-        array: &ArrayRef,
-        ctx: &mut ExecutionCtx,
-    ) -> VortexResult<()> {
-        vortex_ensure!(
-            array.dtype() == &self.dtype,
-            "Cannot append an array of dtype {} to a child builder of dtype {}",
-            array.dtype(),
-            self.dtype,
-        );
-
-        array.append_to_builder(self.pending.as_mut(), ctx)
-    }
-
     /// Appends a single [`Scalar`] to the child.
     pub fn append_scalar(&mut self, scalar: &Scalar) -> VortexResult<()> {
         self.pending.append_scalar(scalar)

vortex-array/src/builders/fixed_size_list.rs

@@ -15,6 +15,7 @@ use vortex_mask::Mask;
 use crate::ArrayRef;
 use crate::ExecutionCtx;
 use crate::IntoArray;
+use crate::arrays::ChunkedArray;
 use crate::arrays::ConstantArray;
 use crate::arrays::FixedSizeListArray;
 use crate::arrays::fixed_size_list::FixedSizeListArraySlotsExt;
@@ -107,19 +108,69 @@ impl FixedSizeListBuilder {
         Ok(())
     }
 
-    /// Appends the same fixed-size list `value` `n` times.
+    /// Appends `array` as `n` identical non-null lists.
     ///
-    /// A fixed-size list array holds its elements back to back, so a run of `n` identical lists is
-    /// the value's elements tiled `n` times - there is no layout that lets the rows share one copy
-    /// the way a list view's can. Two cases avoid paying for the tiling anyway:
+    /// A fixed-size list array holds its elements back to back, so `n` identical lists are the
+    /// array's elements tiled `n` times - there is no layout that lets the rows share one range of
+    /// elements the way a list view's can. The tiling costs nothing to build even so: the elements
+    /// go in as a [`ChunkedArray`] of `n` clones of the same array, so the tile's values are stored
+    /// once however many rows reference them, and the child holds the whole run as one chunk. A
+    /// constant tile does better still, collapsing to a single constant chunk.
     ///
-    /// - a null value needs no elements at all, only placeholders, which
-    ///   [`append_nulls`](ArrayBuilder::append_nulls) writes in bulk;
-    /// - a value whose elements are all the same scalar tiles to a constant array, which goes in as
-    ///   a single chunk.
+    /// A caller with a run of appends to make should hand over the same `array` each time rather
+    /// than rebuild it, which is the whole reason this takes an array instead of a scalar.
+    pub fn append_array_as_repeated_list(
+        &mut self,
+        array: &ArrayRef,
+        n: usize,
+        ctx: &mut ExecutionCtx,
+    ) -> VortexResult<()> {
+        vortex_ensure!(
+            array.dtype() == self.element_dtype(),
+            "Array dtype {:?} does not match list element dtype {:?}",
+            array.dtype(),
+            self.element_dtype()
+        );
+        vortex_ensure!(
+            array.len() == self.list_size() as usize,
+            "Array length {} does not match fixed list size {}",
+            array.len(),
+            self.list_size()
+        );
+
+        if n == 0 {
+            return Ok(());
+        }
+
+        let tiled_len = n * array.len();
+        match array.as_constant() {
+            Some(element) => {
+                let tiled = ConstantArray::new(element, tiled_len).into_array();
+                self.elements_builder.append_array(&tiled, ctx)?;
+            }
+            None => {
+                // SAFETY: every chunk is `array` itself, so they share its dtype and none is empty.
+                let tiled = unsafe {
+                    ChunkedArray::new_unchecked(
+                        std::iter::repeat_n(array.clone(), n).collect::<Vec<_>>(),
+                        self.element_dtype().clone(),
+                    )
+                };
+                self.elements_builder
+                    .append_array(&tiled.into_array(), ctx)?;
+            }
+        }
+        self.nulls.append_n_non_nulls(n);
+
+        Ok(())
+    }
+
+    /// Appends the same fixed-size list `value` `n` times.
     ///
-    /// Otherwise the tile is copied in per row. That is one copy of each element, where
-    /// canonicalizing the run first would build the whole tiled array and then copy it again.
+    /// This materializes the value's elements and hands them to
+    /// [`append_array_as_repeated_list`](Self::append_array_as_repeated_list) - as a constant array
+    /// when they are all the same scalar, so that the tiling costs nothing. A caller with a run of
+    /// appends to make should materialize the elements once itself and call that directly.
     pub(crate) fn append_constant(
         &mut self,
         value: ListScalar,
@@ -139,36 +190,26 @@ impl FixedSizeListBuilder {
             return Ok(());
         };
 
+        let list_size = self.list_size() as usize;
         vortex_ensure!(
-            elements.len() == self.list_size() as usize,
+            elements.len() == list_size,
             "Scalar list length {} does not match fixed list size {}",
             elements.len(),
-            self.list_size()
+            list_size
         );
 
-        let tiled_len = n * elements.len();
-        if let Ok(uniform) = elements.iter().all_equal_value() {
-            let tile = ConstantArray::new(uniform.clone(), tiled_len).into_array();
-            self.elements_builder.append_array(&tile, ctx)?;
-            self.nulls.append_n_non_nulls(n);
-            return Ok(());
-        }
-
-        let tile = {
-            let mut tile_builder = builder_with_capacity(self.element_dtype(), elements.len());
-            for element in &elements {
-                tile_builder.append_scalar(element)?;
+        let tile = match elements.iter().all_equal_value() {
+            Ok(uniform) => ConstantArray::new(uniform.clone(), list_size).into_array(),
+            Err(_) => {
+                let mut tile_builder = builder_with_capacity(self.element_dtype(), list_size);
+                for element in &elements {
+                    tile_builder.append_scalar(element)?;
+                }
+                tile_builder.finish()
             }
-            tile_builder.finish()
         };
 
-        self.elements_builder.reserve_exact(tiled_len);
-        for _ in 0..n {
-            self.elements_builder.append_array_values(&tile, ctx)?;
-        }
-        self.nulls.append_n_non_nulls(n);
-
-        Ok(())
+        self.append_array_as_repeated_list(&tile, n, ctx)
     }
 
     /// Appends the values of a canonical [`FixedSizeListArray`] to the builder, recursing into the