Skip to content

Commit 048b19b

Browse files
committed
Handle any list builder kind in append_to_builder
`builder_with_capacity` produces a `ListViewBuilder` for `DType::List`, but `List::append_to_builder` downcast the builder to `ListBuilder<u64>` and bailed otherwise — so executing a `List`-encoded array into the canonical builder (the one `builder_with_capacity` returns) failed. `ListView::append_to_builder` had the mirror-image assumption, only accepting a `ListViewBuilder<u64, u64>`. Both encodings now append through a `match_each_list_builder!` macro that enumerates the possible list builder offset (and, for `ListViewBuilder`, size) integer types and tries downcasting to each, rather than assuming the offset/size types produced by `builder_with_capacity`. Either encoding works with either builder kind and any unsigned offset/size types; only a non-list builder bails. Other encodings that yield list dtypes (constant, dict, chunked, ...) already route here through the canonical-dispatch fallback. Signed-off-by: Robert <robert@spiraldb.com> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Mkrgj5SuJpaKBXK9jydtSc
1 parent d7c69bd commit 048b19b

4 files changed

Lines changed: 111 additions & 16 deletions

File tree

vortex-array/src/arrays/list/vtable/mod.rs

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,7 @@ use crate::arrays::list::compute::rules::PARENT_RULES;
3535
use crate::arrays::listview::list_view_from_list;
3636
use crate::buffer::BufferHandle;
3737
use crate::builders::ArrayBuilder;
38-
use crate::builders::ListBuilder;
38+
use crate::builders::match_each_list_builder;
3939
use crate::dtype::DType;
4040
use crate::dtype::Nullability;
4141
use crate::dtype::PType;
@@ -198,10 +198,12 @@ impl VTable for List {
198198
builder: &mut dyn ArrayBuilder,
199199
ctx: &mut ExecutionCtx,
200200
) -> VortexResult<()> {
201-
let Some(builder) = builder.as_any_mut().downcast_mut::<ListBuilder<u64>>() else {
202-
vortex_bail!("append_to_builder for List requires a ListBuilder<u64, u64>");
203-
};
204-
builder.append_list_array(&array.into_owned().into_array(), ctx)
201+
let array = array.into_owned().into_array();
202+
match_each_list_builder!(&array, builder, ctx);
203+
vortex_bail!(
204+
"append_to_builder for List requires a list builder, got a builder for {}",
205+
builder.dtype()
206+
)
205207
}
206208
}
207209

vortex-array/src/arrays/listview/vtable/mod.rs

Lines changed: 8 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ use crate::ArrayRef;
2020
use crate::EqMode;
2121
use crate::ExecutionCtx;
2222
use crate::ExecutionResult;
23+
use crate::IntoArray;
2324
use crate::array::Array;
2425
use crate::array::ArrayId;
2526
use crate::array::ArrayView;
@@ -34,7 +35,7 @@ use crate::arrays::listview::array::SLOT_NAMES;
3435
use crate::arrays::listview::compute::rules::PARENT_RULES;
3536
use crate::buffer::BufferHandle;
3637
use crate::builders::ArrayBuilder;
37-
use crate::builders::ListViewBuilder;
38+
use crate::builders::match_each_list_builder;
3839
use crate::dtype::DType;
3940
use crate::dtype::Nullability;
4041
use crate::dtype::PType;
@@ -222,14 +223,12 @@ impl VTable for ListView {
222223
builder: &mut dyn ArrayBuilder,
223224
ctx: &mut ExecutionCtx,
224225
) -> VortexResult<()> {
225-
// `builder_with_capacity` always produces a `ListViewBuilder<u64, u64>` for `DType::List`.
226-
let Some(builder) = builder
227-
.as_any_mut()
228-
.downcast_mut::<ListViewBuilder<u64, u64>>()
229-
else {
230-
vortex_bail!("append_to_builder for ListView requires a ListViewBuilder<u64, u64>");
231-
};
232-
builder.append_listview_array(&array.into_owned(), ctx)
226+
let array = array.into_owned().into_array();
227+
match_each_list_builder!(&array, builder, ctx);
228+
vortex_bail!(
229+
"append_to_builder for ListView requires a list builder, got a builder for {}",
230+
builder.dtype()
231+
)
233232
}
234233

235234
fn reduce_parent(

vortex-array/src/builders/list.rs

Lines changed: 50 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -173,8 +173,9 @@ impl<O: IntegerPType> ListBuilder<O> {
173173
/// [`ListViewArray`] and converting into the `ListArray` (`n + 1` offsets) layout.
174174
///
175175
/// [`ListBuilder`] is not the canonical builder for [`DType::List`] (that is
176-
/// [`ListViewBuilder`](crate::builders::ListViewBuilder)), so no encoding dispatches into it via
177-
/// `append_to_builder`; this helper exists for direct callers such as tests.
176+
/// [`ListViewBuilder`](crate::builders::ListViewBuilder)), so it is never produced by
177+
/// [`builder_with_capacity`]. List encodings still dispatch into it via `match_each_list_builder`
178+
/// when a caller supplies one directly.
178179
pub(crate) fn append_list_array(
179180
&mut self,
180181
array: &ArrayRef,
@@ -334,6 +335,7 @@ mod tests {
334335
use Nullability::Nullable;
335336
use vortex_buffer::buffer;
336337
use vortex_error::VortexExpect;
338+
use vortex_error::VortexResult;
337339

338340
use crate::IntoArray;
339341
use crate::array_session;
@@ -344,6 +346,8 @@ mod tests {
344346
use crate::arrays::listview::ListViewArrayExt;
345347
use crate::assert_arrays_eq;
346348
use crate::builders::ArrayBuilder;
349+
use crate::builders::ListViewBuilder;
350+
use crate::builders::builder_with_capacity;
347351
use crate::builders::list::ListArray;
348352
use crate::builders::list::ListBuilder;
349353
use crate::dtype::DType;
@@ -514,6 +518,50 @@ mod tests {
514518
);
515519
}
516520

521+
/// `append_to_builder` must handle any list builder kind without assuming the offset/size
522+
/// integer types produced by `builder_with_capacity`. It appends a `List`-encoded array and a
523+
/// `ListView`-encoded array into `ListViewBuilder`s and `ListBuilder`s with assorted (and
524+
/// non-`u64`) offset/size types.
525+
#[test]
526+
fn test_append_to_builder_any_list_builder() -> VortexResult<()> {
527+
let mut ctx = array_session().create_execution_ctx();
528+
529+
let list = ListArray::from_iter_opt_slow::<u64, _, _>(
530+
[Some(vec![0, 1, 2]), None, Some(vec![4, 5])],
531+
Arc::new(I32.into()),
532+
)?
533+
.into_array();
534+
let listview = list
535+
.clone()
536+
.execute::<ListViewArray>(&mut ctx)?
537+
.into_array();
538+
let elem_dtype = || Arc::new(I32.into());
539+
540+
// `builder_with_capacity` produces a `ListViewBuilder` for `DType::List`; appending the
541+
// `List`-encoded array must dispatch into it instead of bailing.
542+
let mut listview_builder = builder_with_capacity(list.dtype(), list.len());
543+
list.append_to_builder(listview_builder.as_mut(), &mut ctx)?;
544+
assert_arrays_eq!(listview_builder.finish(), list, &mut ctx);
545+
546+
// Offset/size types are enumerated, not assumed: a `ListViewBuilder` with non-`u64`
547+
// offset and size types must work for both source encodings.
548+
let mut lv_u32_u8 = ListViewBuilder::<u32, u8>::with_capacity(elem_dtype(), Nullable, 8, 4);
549+
list.append_to_builder(&mut lv_u32_u8, &mut ctx)?;
550+
assert_arrays_eq!(lv_u32_u8.finish(), list, &mut ctx);
551+
552+
let mut lv_u16_u16 =
553+
ListViewBuilder::<u16, u16>::with_capacity(elem_dtype(), Nullable, 8, 4);
554+
listview.append_to_builder(&mut lv_u16_u16, &mut ctx)?;
555+
assert_arrays_eq!(lv_u16_u16.finish(), list, &mut ctx);
556+
557+
// A `ListView`-encoded array appended into a `ListBuilder` with a non-`u64` offset type.
558+
let mut list_builder = ListBuilder::<u32>::with_capacity(elem_dtype(), Nullable, 8, 4);
559+
listview.append_to_builder(&mut list_builder, &mut ctx)?;
560+
assert_arrays_eq!(list_builder.finish(), list, &mut ctx);
561+
562+
Ok(())
563+
}
564+
517565
#[test]
518566
fn test_extend_builder() {
519567
test_extend_builder_gen::<i8>();

vortex-array/src/builders/mod.rs

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -284,3 +284,49 @@ pub fn builder_with_capacity_in(
284284
let _allocator = allocator;
285285
builder_with_capacity(dtype, capacity)
286286
}
287+
288+
/// Append a list-typed `array` into `builder` by enumerating the possible list builder offset (and,
289+
/// for [`ListViewBuilder`], size) integer types and dispatching to the first concrete type that
290+
/// matches. On a match it `return`s the append result from the enclosing function; if no list
291+
/// builder matches it falls through, leaving the caller to bail.
292+
///
293+
/// [`builder_with_capacity`] produces a `ListViewBuilder<u64, u64>` for [`DType::List`], but a
294+
/// caller may supply a builder with different offset/size types (or a [`ListBuilder`]), so the
295+
/// concrete type cannot be assumed — hence the enumeration. List offsets and sizes are always
296+
/// unsigned integers.
297+
macro_rules! match_each_list_builder {
298+
($array:expr, $builder:expr, $ctx:expr) => {{
299+
$crate::builders::match_each_list_builder!(@list $array, $builder, $ctx, [u8, u16, u32, u64]);
300+
$crate::builders::match_each_list_builder!(
301+
@view $array, $builder, $ctx, [u8, u16, u32, u64], [u8, u16, u32, u64]
302+
);
303+
}};
304+
(@list $array:expr, $builder:expr, $ctx:expr, [$($O:ty),*]) => {{
305+
$(
306+
if let Some(builder) = $builder
307+
.as_any_mut()
308+
.downcast_mut::<$crate::builders::ListBuilder<$O>>()
309+
{
310+
return builder.append_list_array($array, $ctx);
311+
}
312+
)*
313+
}};
314+
// For each offset type `O`, expand over every size type `S` (cartesian product).
315+
(@view $array:expr, $builder:expr, $ctx:expr, [$($O:ty),*], $sizes:tt) => {{
316+
$(
317+
$crate::builders::match_each_list_builder!(@view_o $array, $builder, $ctx, $O, $sizes);
318+
)*
319+
}};
320+
(@view_o $array:expr, $builder:expr, $ctx:expr, $O:ty, [$($S:ty),*]) => {{
321+
$(
322+
if let Some(builder) = $builder
323+
.as_any_mut()
324+
.downcast_mut::<$crate::builders::ListViewBuilder<$O, $S>>()
325+
{
326+
let listview = $array.clone().execute::<$crate::arrays::ListViewArray>($ctx)?;
327+
return builder.append_listview_array(&listview, $ctx);
328+
}
329+
)*
330+
}};
331+
}
332+
pub(crate) use match_each_list_builder;

0 commit comments

Comments
 (0)