From c7c368162b7e2268cd06f523e3a62e283d50ad7b Mon Sep 17 00:00:00 2001 From: Robert Date: Tue, 15 Sep 2026 12:29:08 +0000 Subject: [PATCH 1/2] perf(array): apply the sparse decode techniques to constant canonicalization `ConstantArray::append_to_builder` already shares one copy of a constant run's values the way `SparseArray` shares its fill value, but `constant_canonicalize` still materialized per row. Bring it in line: - A constant fixed-size list now builds one row's elements and tiles that single copy over the run, as `fixed_size_list_fill_tile` does for a sparse fill. Elements that are all the same scalar - which a null list's placeholders always are - stay one `ConstantArray` covering the whole run, so the common cases stop being `list_size * len` scalar appends. - A constant string or binary run adopts the scalar's own buffer instead of copying its bytes, and only when the value is too long to inline. A value of exactly `MAX_INLINED_SIZE` bytes lives in its view, so the buffer pushed for it was never referenced; `uncompressed_size_in_bytes` carried the same off-by-one and is corrected to match. - A constant list's offsets and sizes take the narrowest width that can describe the list, via `match_smallest_list_offset_type!`, rather than always costing eight bytes a row once decoded. Signed-off-by: Robert Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01KwhzKebXYiuUjPxBmDdTF9 --- .../fns/uncompressed_size_in_bytes/mod.rs | 4 +- .../src/arrays/constant/vtable/canonical.rs | 244 ++++++++++++++---- 2 files changed, 198 insertions(+), 50 deletions(-) diff --git a/vortex-array/src/aggregate_fn/fns/uncompressed_size_in_bytes/mod.rs b/vortex-array/src/aggregate_fn/fns/uncompressed_size_in_bytes/mod.rs index a23bcfdc2c9..cbeffdc3d6c 100644 --- a/vortex-array/src/aggregate_fn/fns/uncompressed_size_in_bytes/mod.rs +++ b/vortex-array/src/aggregate_fn/fns/uncompressed_size_in_bytes/mod.rs @@ -259,8 +259,10 @@ pub(crate) fn constant_uncompressed_size_in_bytes( fn constant_varbinview_value_size(len: usize, scalar_len: Option) -> VortexResult { let views_size = checked_len_mul(len, size_of::(), "binary view")?; + // A value short enough to inline lives entirely in its view, so only a longer one adds a data + // buffer - matching what `constant_canonicalize` builds. let data_size = match scalar_len { - Some(scalar_len) if scalar_len >= BinaryView::MAX_INLINED_SIZE => u64::try_from(scalar_len) + Some(scalar_len) if scalar_len > BinaryView::MAX_INLINED_SIZE => u64::try_from(scalar_len) .map_err(|e| vortex_err!("Failed to convert data buffer length to u64: {e}"))?, _ => 0, }; diff --git a/vortex-array/src/arrays/constant/vtable/canonical.rs b/vortex-array/src/arrays/constant/vtable/canonical.rs index e6a656afa51..257f0dd63f4 100644 --- a/vortex-array/src/arrays/constant/vtable/canonical.rs +++ b/vortex-array/src/arrays/constant/vtable/canonical.rs @@ -3,18 +3,23 @@ use std::sync::Arc; +use itertools::Itertools; use vortex_buffer::BitBuffer; use vortex_buffer::Buffer; use vortex_buffer::BufferAllocatorRef; +use vortex_buffer::BufferString; +use vortex_buffer::ByteBuffer; use vortex_buffer::buffer; use vortex_error::VortexExpect; use vortex_error::VortexResult; +use crate::ArrayRef; use crate::Canonical; use crate::ExecutionCtx; use crate::IntoArray; use crate::array::ArrayView; use crate::arrays::BoolArray; +use crate::arrays::ChunkedArray; use crate::arrays::Constant; use crate::arrays::ConstantArray; use crate::arrays::DecimalArray; @@ -36,6 +41,7 @@ use crate::dtype::Nullability; use crate::match_each_decimal_value; use crate::match_each_decimal_value_type; use crate::match_each_native_ptype; +use crate::match_smallest_list_offset_type; use crate::scalar::DecimalValue; use crate::scalar::Scalar; use crate::validity::Validity; @@ -111,19 +117,21 @@ pub(crate) fn constant_canonicalize( Canonical::Decimal(decimal_array) } DType::Utf8(_) => { - let value = scalar.as_utf8().value(); - let const_value = value.as_ref().map(|v| v.as_bytes()); + let value = scalar + .as_utf8() + .value() + .cloned() + .map(BufferString::into_inner); Canonical::VarBinView(constant_canonical_byte_view( - const_value, + value, array.dtype(), array.len(), )) } DType::Binary(_) => { let value = scalar.as_binary().value().cloned(); - let const_value = value.as_ref().map(|v| v.as_slice()); Canonical::VarBinView(constant_canonical_byte_view( - const_value, + value, array.dtype(), array.len(), )) @@ -206,8 +214,12 @@ pub(crate) fn constant_canonicalize( }) } +/// Builds the canonical view array for a constant string or binary run. +/// +/// The value is stored once: inlined into the repeated view when it is short enough, and otherwise +/// adopted as the array's single data buffer, so nothing is copied however long the run. fn constant_canonical_byte_view( - scalar_bytes: Option<&[u8]>, + scalar_bytes: Option, dtype: &DType, len: usize, ) -> VarBinViewArray { @@ -227,11 +239,13 @@ fn constant_canonical_byte_view( } Some(scalar_bytes) => { // Create a view to hold the scalar bytes. - // If the scalar cannot be inlined, allocate a single buffer large enough to hold it. - let view = BinaryView::make_view(scalar_bytes, 0, 0); + let view = BinaryView::make_view(scalar_bytes.as_slice(), 0, 0); + // A value short enough to inline lives entirely in its view; only a longer one needs + // its bytes as a data buffer, and then the scalar's own buffer is adopted rather than + // copied. let mut buffers = Vec::new(); - if scalar_bytes.len() >= BinaryView::MAX_INLINED_SIZE { - buffers.push(Buffer::copy_from(scalar_bytes)); + if scalar_bytes.len() > BinaryView::MAX_INLINED_SIZE { + buffers.push(scalar_bytes); } // Clone our constant view `len` times. @@ -294,9 +308,15 @@ fn constant_canonical_list_array( Validity::NonNullable }; - // Somewhat arbitrarily choose `u64` as the type for offsets and sizes. - let offsets = ConstantArray::new::(0, len).into_array(); - let sizes = ConstantArray::new::(list.len() as u64, len).into_array(); + // Every row has the same offset and size, so the narrowest width that can describe the list is + // enough - and is what a consumer that decodes them pays for. + let (offsets, sizes) = match_smallest_list_offset_type!(list.len(), |O| { + let size = O::try_from(list.len()).vortex_expect("list length fits the chosen offset type"); + ( + ConstantArray::new::(O::default(), len).into_array(), + ConstantArray::new::(size, len).into_array(), + ) + }); debug_assert!(!offsets.dtype().is_nullable()); debug_assert!(!sizes.dtype().is_nullable()); @@ -307,6 +327,12 @@ fn constant_canonical_list_array( unsafe { ListViewArray::new_unchecked(elements, offsets, sizes, validity) } } +/// Creates a [`FixedSizeListArray`] whose every row holds the same list. +/// +/// A fixed-size list holds its elements back to back, so a run of `len` identical rows is the +/// list's elements tiled `len` times - there is no layout that lets the rows share one range of +/// elements the way a list view's can. Building that tiling still costs nothing per row: see +/// [`tile_fixed_size_list_elements`]. fn constant_canonical_fixed_size_list_array( values: Option>, element_dtype: &DType, @@ -315,40 +341,67 @@ fn constant_canonical_fixed_size_list_array( len: usize, allocator: &BufferAllocatorRef, ) -> FixedSizeListArray { - match values { - None => { - // Even though the scalar is null, we still have to allocate the correct amount of space - // for the given `DType`. - let elements_len = list_size as usize * len; - let mut element_builder = - builder_with_capacity_in(element_dtype, elements_len, allocator); - element_builder.append_defaults(elements_len); - let elements = element_builder.finish(); - - // SAFETY: The elements array has a length that is a multiple of `list_size`, and the - // validity is `AllInvalid` so we don't care about the length. - unsafe { - FixedSizeListArray::new_unchecked(elements, list_size, Validity::AllInvalid, len) - } - } - Some(values) => { - let mut elements_builder = - builder_with_capacity_in(element_dtype, len * values.len(), allocator); + let elements_len = list_size as usize * len; + + let (elements, validity) = match values { + // A null list has no elements of its own, only the placeholders the layout requires. They + // are all the element dtype's default value, so one constant array covers the whole run. + None => ( + ConstantArray::new(Scalar::default_value(element_dtype), elements_len).into_array(), + Validity::AllInvalid, + ), + Some(values) => ( + tile_fixed_size_list_elements(&values, element_dtype, len, elements_len, allocator), + Validity::from(list_nullability), + ), + }; - for _ in 0..len { - for v in &values { - elements_builder - .append_scalar(v) - .vortex_expect("must be a same dtype"); - } + // SAFETY: `elements` holds exactly `list_size * len` values, and the validity is one of + // `AllInvalid`, `AllValid` or `NonNullable`, none of which carry a length of their own. + unsafe { FixedSizeListArray::new_unchecked(elements, list_size, validity, len) } +} + +/// Tiles one row's `values` across a run of `len` rows, storing them once. +/// +/// Elements that are all the same scalar stay a [`ConstantArray`], so the whole run's elements are +/// a single array however long it is. Otherwise the row materializes once and the run becomes +/// `len` chunks pointing at that one copy, rather than `len` copies of it. +fn tile_fixed_size_list_elements( + values: &[Scalar], + element_dtype: &DType, + len: usize, + elements_len: usize, + allocator: &BufferAllocatorRef, +) -> ArrayRef { + // An empty run, or a degenerate `list_size == 0`, has no elements to tile at all. + if elements_len == 0 { + return Canonical::empty(element_dtype).into_array(); + } + + match values.iter().all_equal_value() { + Ok(uniform) => ConstantArray::new(uniform.clone(), elements_len).into_array(), + Err(_) => { + let mut elements_builder = + builder_with_capacity_in(element_dtype, values.len(), allocator); + for value in values { + elements_builder + .append_scalar(value) + .vortex_expect("fixed-size-list element scalar was invalid"); } + let tile = elements_builder.finish(); - let elements = elements_builder.finish(); - let validity = Validity::from(list_nullability); + if len == 1 { + return tile; + } - // SAFETY: The elements array has a length that is a multiple of `list_size`, and the - // validity is either `NonNullable` or `AllValid` so we don't care about the length. - unsafe { FixedSizeListArray::new_unchecked(elements, list_size, validity, len) } + // SAFETY: every chunk is `tile` itself, so they share its dtype and none is empty. + unsafe { + ChunkedArray::new_unchecked( + std::iter::repeat_n(tile, len).collect::>(), + element_dtype.clone(), + ) + } + .into_array() } } } @@ -360,6 +413,7 @@ mod tests { use enum_iterator::all; use itertools::Itertools; + use rstest::rstest; use vortex_error::VortexExpect; use vortex_error::VortexResult; use vortex_session::VortexSession; @@ -367,6 +421,8 @@ mod tests { use crate::Canonical; use crate::IntoArray; use crate::VortexSessionExecute; + use crate::arrays::Chunked; + use crate::arrays::Constant; use crate::arrays::ConstantArray; use crate::arrays::FixedSizeListArray; use crate::arrays::ListViewArray; @@ -375,6 +431,7 @@ mod tests { use crate::arrays::StructArray; use crate::arrays::VarBinArray; use crate::arrays::VarBinViewArray; + use crate::arrays::chunked::ChunkedArrayExt; use crate::arrays::fixed_size_list::FixedSizeListArrayExt; use crate::arrays::fixed_size_list::FixedSizeListArraySlotsExt; use crate::arrays::listview::ListViewArraySlotsExt; @@ -486,7 +543,7 @@ mod tests { .offsets() .clone() .execute::(&mut ctx)?, - PrimitiveArray::from_iter([0u64, 2]), + PrimitiveArray::from_iter([0u32, 2]), &mut ctx ); assert_arrays_eq!( @@ -494,7 +551,7 @@ mod tests { .sizes() .clone() .execute::(&mut ctx)?, - PrimitiveArray::from_iter([2u64, 2]), + PrimitiveArray::from_iter([2u32, 2]), &mut ctx ); Ok(()) @@ -522,7 +579,7 @@ mod tests { .clone() .execute::(&mut ctx) .unwrap(), - PrimitiveArray::from_iter([0u64, 0]), + PrimitiveArray::from_iter([0u32, 0]), &mut ctx ); assert_arrays_eq!( @@ -531,7 +588,7 @@ mod tests { .clone() .execute::(&mut ctx) .unwrap(), - PrimitiveArray::from_iter([0u64, 0]), + PrimitiveArray::from_iter([0u32, 0]), &mut ctx ); } @@ -557,7 +614,7 @@ mod tests { .clone() .execute::(&mut ctx) .unwrap(), - PrimitiveArray::from_iter([0u64, 0]), + PrimitiveArray::from_iter([0u32, 0]), &mut ctx ); assert_arrays_eq!( @@ -566,7 +623,7 @@ mod tests { .clone() .execute::(&mut ctx) .unwrap(), - PrimitiveArray::from_iter([0u64, 0]), + PrimitiveArray::from_iter([0u32, 0]), &mut ctx ); } @@ -889,4 +946,93 @@ mod tests { ); } } + + /// A constant fixed-size list whose elements are all the same scalar stores those elements as + /// one constant array, so the run costs nothing per row. + #[test] + fn test_canonicalize_fixed_size_list_uniform_elements_stay_constant() { + let mut ctx = SESSION.create_execution_ctx(); + let fsl_scalar = Scalar::fixed_size_list( + Arc::new(DType::Primitive(PType::I32, Nullability::NonNullable)), + vec![Scalar::primitive(7i32, Nullability::NonNullable); 3], + Nullability::NonNullable, + ); + + let canonical = ConstantArray::new(fsl_scalar, 10_000) + .into_array() + .execute::(&mut ctx) + .unwrap(); + + assert_eq!(canonical.len(), 10_000); + assert_eq!(canonical.elements().len(), 30_000); + assert!( + canonical.elements().as_opt::().is_some(), + "uniform elements should stay constant-encoded rather than materialize per row", + ); + for index in [0, 5_000, 9_999] { + assert_arrays_eq!( + canonical.fixed_size_list_elements_at(index).unwrap(), + PrimitiveArray::from_iter([7i32, 7, 7]), + &mut ctx + ); + } + } + + /// A constant fixed-size list whose elements differ materializes one copy of the row and tiles + /// it, so the run holds a chunk per row rather than a copy of the elements per row. + #[test] + fn test_canonicalize_fixed_size_list_tiles_one_copy_of_mixed_elements() { + let mut ctx = SESSION.create_execution_ctx(); + const LEN: usize = 1_000; + let fsl_scalar = Scalar::fixed_size_list( + Arc::new(DType::Primitive(PType::I32, Nullability::NonNullable)), + vec![ + Scalar::primitive(1i32, Nullability::NonNullable), + Scalar::primitive(2i32, Nullability::NonNullable), + ], + Nullability::NonNullable, + ); + + let canonical = ConstantArray::new(fsl_scalar, LEN) + .into_array() + .execute::(&mut ctx) + .unwrap(); + + assert_eq!(canonical.elements().len(), 2 * LEN); + assert_eq!( + canonical.elements().as_::().nchunks(), + LEN, + "every row should reference the same tile rather than hold its own copy", + ); + for index in [0, 1, LEN - 1] { + assert_arrays_eq!( + canonical.fixed_size_list_elements_at(index).unwrap(), + PrimitiveArray::from_iter([1i32, 2]), + &mut ctx + ); + } + } + + /// A value short enough to inline lives in its view; only a longer one costs a data buffer, + /// and then the scalar's own bytes are adopted rather than copied. + #[rstest] + #[case::inlined("exactly12chr", 0)] + #[case::referenced("thirteen chrs", 1)] + fn test_canonicalize_string_stores_the_value_once( + #[case] value: &str, + #[case] data_buffers: usize, + ) { + let mut ctx = SESSION.create_execution_ctx(); + let canonical = ConstantArray::new(value.to_string(), 100) + .into_array() + .execute::(&mut ctx) + .unwrap(); + + assert_eq!(canonical.data_buffers().len(), data_buffers); + assert_arrays_eq!( + canonical, + VarBinViewArray::from_iter_str(std::iter::repeat_n(value, 100)), + &mut ctx + ); + } } From 99c8ec360f9757e25fcbff3ea78182a4f5591c28 Mon Sep 17 00:00:00 2001 From: Robert Date: Tue, 15 Sep 2026 12:38:50 +0000 Subject: [PATCH 2/2] perf(sparse): fill the decimal buffer in one go, and trim comments `execute_sparse_decimal` pushed the fill value through `DecimalBuilder` a value at a time. Build the buffer directly instead, the way `constant_canonicalize` already does for a constant decimal run. Also tightens the comments added in the previous commit. Signed-off-by: Robert Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01KwhzKebXYiuUjPxBmDdTF9 --- encodings/sparse/src/canonical.rs | 35 ++++++++-------- .../fns/uncompressed_size_in_bytes/mod.rs | 3 +- .../src/arrays/constant/vtable/canonical.rs | 40 ++++++------------- 3 files changed, 32 insertions(+), 46 deletions(-) diff --git a/encodings/sparse/src/canonical.rs b/encodings/sparse/src/canonical.rs index d2317dbae2c..caace3303f2 100644 --- a/encodings/sparse/src/canonical.rs +++ b/encodings/sparse/src/canonical.rs @@ -12,6 +12,7 @@ use vortex_array::Canonical; use vortex_array::ExecutionCtx; use vortex_array::IntoArray; use vortex_array::arrays::BoolArray; +use vortex_array::arrays::DecimalArray; use vortex_array::arrays::FixedSizeList; use vortex_array::arrays::FixedSizeListArray; use vortex_array::arrays::ListView; @@ -32,7 +33,6 @@ use vortex_array::arrays::struct_::StructArrayExt; use vortex_array::arrays::varbinview::build_views::BinaryView; use vortex_array::buffer::BufferHandle; use vortex_array::builders::ArrayBuilder; -use vortex_array::builders::DecimalBuilder; use vortex_array::builders::FixedSizeListBuilder; use vortex_array::builders::ListViewBuilder; use vortex_array::builders::VarBinBuilder; @@ -785,22 +785,23 @@ fn execute_sparse_decimal( len: usize, ctx: &mut ExecutionCtx, ) -> VortexResult { - let mut builder = - DecimalBuilder::with_capacity_in::(len, decimal_dtype, nullability, ctx.allocator()); - match fill_value.decimal_value() { - Some(fill_value) => { - let fill_value = fill_value - .cast::() - .vortex_expect("unexpected value type"); - for _ in 0..len { - builder.append_value(fill_value) - } - } - None => { - builder.append_nulls(len); - } - } - let filled_array = builder.finish_into_decimal(); + // Fill the buffer in one go rather than a value at a time, as the other fills do. + let (values, validity) = match fill_value.decimal_value() { + Some(fill_value) => ( + Buffer::full( + fill_value + .cast::() + .vortex_expect("unexpected value type"), + len, + ), + Validity::from(nullability), + ), + None => (Buffer::::zeroed(len), Validity::AllInvalid), + }; + + // SAFETY: the buffer holds `len` values of the dtype's value type, and the validity carries no + // length of its own. + let filled_array = unsafe { DecimalArray::new_unchecked(values, decimal_dtype, validity) }; Ok(filled_array.patch(patches, ctx)?.into_array()) } diff --git a/vortex-array/src/aggregate_fn/fns/uncompressed_size_in_bytes/mod.rs b/vortex-array/src/aggregate_fn/fns/uncompressed_size_in_bytes/mod.rs index cbeffdc3d6c..4c1f5038458 100644 --- a/vortex-array/src/aggregate_fn/fns/uncompressed_size_in_bytes/mod.rs +++ b/vortex-array/src/aggregate_fn/fns/uncompressed_size_in_bytes/mod.rs @@ -259,8 +259,7 @@ pub(crate) fn constant_uncompressed_size_in_bytes( fn constant_varbinview_value_size(len: usize, scalar_len: Option) -> VortexResult { let views_size = checked_len_mul(len, size_of::(), "binary view")?; - // A value short enough to inline lives entirely in its view, so only a longer one adds a data - // buffer - matching what `constant_canonicalize` builds. + // Only a value too long to inline adds a data buffer, matching `constant_canonicalize`. let data_size = match scalar_len { Some(scalar_len) if scalar_len > BinaryView::MAX_INLINED_SIZE => u64::try_from(scalar_len) .map_err(|e| vortex_err!("Failed to convert data buffer length to u64: {e}"))?, diff --git a/vortex-array/src/arrays/constant/vtable/canonical.rs b/vortex-array/src/arrays/constant/vtable/canonical.rs index 257f0dd63f4..e1f9ad02f6c 100644 --- a/vortex-array/src/arrays/constant/vtable/canonical.rs +++ b/vortex-array/src/arrays/constant/vtable/canonical.rs @@ -215,9 +215,6 @@ pub(crate) fn constant_canonicalize( } /// Builds the canonical view array for a constant string or binary run. -/// -/// The value is stored once: inlined into the repeated view when it is short enough, and otherwise -/// adopted as the array's single data buffer, so nothing is copied however long the run. fn constant_canonical_byte_view( scalar_bytes: Option, dtype: &DType, @@ -240,9 +237,8 @@ fn constant_canonical_byte_view( Some(scalar_bytes) => { // Create a view to hold the scalar bytes. let view = BinaryView::make_view(scalar_bytes.as_slice(), 0, 0); - // A value short enough to inline lives entirely in its view; only a longer one needs - // its bytes as a data buffer, and then the scalar's own buffer is adopted rather than - // copied. + // A value short enough to inline lives entirely in its view, so only a longer one + // needs a data buffer. Adopt the scalar's own rather than copying it. let mut buffers = Vec::new(); if scalar_bytes.len() > BinaryView::MAX_INLINED_SIZE { buffers.push(scalar_bytes); @@ -308,8 +304,7 @@ fn constant_canonical_list_array( Validity::NonNullable }; - // Every row has the same offset and size, so the narrowest width that can describe the list is - // enough - and is what a consumer that decodes them pays for. + // Every row has the same offset and size, so use the narrowest width that fits the list. let (offsets, sizes) = match_smallest_list_offset_type!(list.len(), |O| { let size = O::try_from(list.len()).vortex_expect("list length fits the chosen offset type"); ( @@ -328,11 +323,6 @@ fn constant_canonical_list_array( } /// Creates a [`FixedSizeListArray`] whose every row holds the same list. -/// -/// A fixed-size list holds its elements back to back, so a run of `len` identical rows is the -/// list's elements tiled `len` times - there is no layout that lets the rows share one range of -/// elements the way a list view's can. Building that tiling still costs nothing per row: see -/// [`tile_fixed_size_list_elements`]. fn constant_canonical_fixed_size_list_array( values: Option>, element_dtype: &DType, @@ -344,8 +334,7 @@ fn constant_canonical_fixed_size_list_array( let elements_len = list_size as usize * len; let (elements, validity) = match values { - // A null list has no elements of its own, only the placeholders the layout requires. They - // are all the element dtype's default value, so one constant array covers the whole run. + // A null list's elements are all placeholders, so one constant array covers the run. None => ( ConstantArray::new(Scalar::default_value(element_dtype), elements_len).into_array(), Validity::AllInvalid, @@ -356,16 +345,16 @@ fn constant_canonical_fixed_size_list_array( ), }; - // SAFETY: `elements` holds exactly `list_size * len` values, and the validity is one of - // `AllInvalid`, `AllValid` or `NonNullable`, none of which carry a length of their own. + // SAFETY: `elements` holds exactly `list_size * len` values, and the validity carries no + // length of its own. unsafe { FixedSizeListArray::new_unchecked(elements, list_size, validity, len) } } /// Tiles one row's `values` across a run of `len` rows, storing them once. /// -/// Elements that are all the same scalar stay a [`ConstantArray`], so the whole run's elements are -/// a single array however long it is. Otherwise the row materializes once and the run becomes -/// `len` chunks pointing at that one copy, rather than `len` copies of it. +/// A fixed-size list holds its elements back to back, so the rows cannot share one range the way a +/// list view's can. Uniform elements stay a [`ConstantArray`] covering the whole run; otherwise the +/// row materializes once and the run chunks that single copy. fn tile_fixed_size_list_elements( values: &[Scalar], element_dtype: &DType, @@ -373,7 +362,7 @@ fn tile_fixed_size_list_elements( elements_len: usize, allocator: &BufferAllocatorRef, ) -> ArrayRef { - // An empty run, or a degenerate `list_size == 0`, has no elements to tile at all. + // An empty run, or a degenerate `list_size == 0`. if elements_len == 0 { return Canonical::empty(element_dtype).into_array(); } @@ -947,8 +936,7 @@ mod tests { } } - /// A constant fixed-size list whose elements are all the same scalar stores those elements as - /// one constant array, so the run costs nothing per row. + /// Uniform elements should stay one constant array, not materialize per row. #[test] fn test_canonicalize_fixed_size_list_uniform_elements_stay_constant() { let mut ctx = SESSION.create_execution_ctx(); @@ -978,8 +966,7 @@ mod tests { } } - /// A constant fixed-size list whose elements differ materializes one copy of the row and tiles - /// it, so the run holds a chunk per row rather than a copy of the elements per row. + /// Mixed elements should materialize once, with every row chunking that one copy. #[test] fn test_canonicalize_fixed_size_list_tiles_one_copy_of_mixed_elements() { let mut ctx = SESSION.create_execution_ctx(); @@ -1013,8 +1000,7 @@ mod tests { } } - /// A value short enough to inline lives in its view; only a longer one costs a data buffer, - /// and then the scalar's own bytes are adopted rather than copied. + /// Only a value too long to inline should cost a data buffer. #[rstest] #[case::inlined("exactly12chr", 0)] #[case::referenced("thirteen chrs", 1)]