From 774897e50a3d62c1cb2d88afcea7c3c04e23127d Mon Sep 17 00:00:00 2001 From: Robert Kruszewski Date: Thu, 30 Jul 2026 22:16:46 +0100 Subject: [PATCH 1/6] Dict: gather dictionary values straight into a VarBinBuilder MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Appending a string dictionary to a builder went through `take_canonical`, which first materializes the values at full logical length — allocating and re-reading a views buffer proportional to the row count. The dictionary is usually far smaller than the column, so resolving each code against it in place skips that intermediate and leaves one `memcpy` per row. The fast path indexes the codes and the dictionary's views and data buffers as host slices, so `varbin_fast_path_is_host` gates it on residency; a device-resident dictionary still takes the canonical route, where a device kernel can serve it. Signed-off-by: Robert Kruszewski Signed-off-by: Robert Kruszewski --- vortex-array/src/arrays/dict/vtable/mod.rs | 241 +++++++++++++++++++++ 1 file changed, 241 insertions(+) diff --git a/vortex-array/src/arrays/dict/vtable/mod.rs b/vortex-array/src/arrays/dict/vtable/mod.rs index d9ced9eeffa..892e74e2b94 100644 --- a/vortex-array/src/arrays/dict/vtable/mod.rs +++ b/vortex-array/src/arrays/dict/vtable/mod.rs @@ -3,13 +3,17 @@ use std::hash::Hasher; +use num_traits::AsPrimitive; use prost::Message; use smallvec::smallvec; +use vortex_buffer::BitBufferMut; use vortex_error::VortexResult; use vortex_error::vortex_bail; use vortex_error::vortex_ensure; use vortex_error::vortex_err; use vortex_error::vortex_panic; +use vortex_mask::AllOr; +use vortex_mask::Mask; use vortex_session::VortexSession; use vortex_session::registry::CachedId; @@ -34,17 +38,24 @@ use crate::array::VTable; use crate::array::with_empty_buffers; use crate::arrays::ConstantArray; use crate::arrays::Primitive; +use crate::arrays::PrimitiveArray; +use crate::arrays::VarBinViewArray; use crate::arrays::dict::DictArrayExt; use crate::arrays::dict::DictArraySlotsExt; use crate::arrays::dict::compute::rules::PARENT_RULES; use crate::arrays::dict::execute::take_canonical; use crate::buffer::BufferHandle; use crate::builders::ArrayBuilder; +use crate::builders::VarBinBuilder; use crate::dtype::DType; +use crate::dtype::IntegerPType; use crate::dtype::Nullability; +use crate::dtype::OffsetBuilderPType; use crate::dtype::PType; use crate::executor::ExecutionCtx; use crate::executor::ExecutionResult; +use crate::match_each_integer_ptype; +use crate::match_each_varbin_builder; use crate::require_child; use crate::scalar::Scalar; use crate::serde::ArrayChildren; @@ -223,6 +234,14 @@ impl VTable for Dict { && !codes.validity()?.definitely_all_null() { let codes = codes.into_owned(); + if let Canonical::VarBinView(values) = Canonical::from(values) + && varbin_fast_path_is_host(&codes, &values) + && let Some(result) = match_each_varbin_builder!(builder, |builder| { + append_dict_to_varbin(&codes, &values, builder, ctx) + }) + { + return result; + } let canonical = take_canonical(values, &codes, ctx)?.into_array(); canonical.append_to_builder(builder, ctx)?; return Ok(()); @@ -245,3 +264,225 @@ impl VTable for Dict { PARENT_RULES.evaluate(array, parent, child_idx) } } + +/// Whether every buffer that [`append_dict_to_varbin`] dereferences directly is host-resident. +/// +/// The fast path reads the codes and the dictionary's views and data buffers as plain slices, +/// which is only possible for host allocations. Device-resident dictionaries fall back to the +/// canonical take, which goes through the compute stack and can therefore be served by a device +/// kernel. This mirrors [`DictArrayExt::validate_all_values_referenced`], which likewise skips +/// host-only work when the codes are not resident on the host. +/// +/// This is a handful of enum discriminant checks over the array's buffer handles, so it stays +/// outside the per-row loop and costs nothing measurable on the host-resident path. +fn varbin_fast_path_is_host(codes: &PrimitiveArray, values: &VarBinViewArray) -> bool { + codes.buffer_handle().is_on_host() + && values.views_handle().is_on_host() + && values.data_buffers().iter().all(BufferHandle::is_on_host) +} + +/// Gathers the dictionary values straight into `builder`. +/// +/// The canonical route first takes the values to full logical length, which allocates and then +/// re-reads a views buffer proportional to the row count. The dictionary is usually far smaller +/// than the column, so resolving each code against it in place skips that intermediate entirely +/// and leaves one `memcpy` per row as the only work. +/// +/// The caller must have checked [`varbin_fast_path_is_host`]; this function indexes the backing +/// buffers as host slices and panics otherwise. +fn append_dict_to_varbin( + codes: &PrimitiveArray, + values: &VarBinViewArray, + builder: &mut VarBinBuilder, + ctx: &mut ExecutionCtx, +) -> VortexResult<()> +where + usize: AsPrimitive, +{ + let len = codes.as_ref().len(); + let codes_validity = codes.as_ref().validity()?.execute_mask(len, ctx)?; + let values_validity = values.validity()?.execute_mask(values.len(), ctx)?; + + // Resolve the dictionary's storage once so that looking up a code is an O(1) read. + let views = values.views(); + let buffers = values + .data_buffers() + .iter() + .map(|buffer| buffer.as_host().as_slice()) + .collect::>(); + + match_each_integer_ptype!(codes.ptype(), |C| { + let codes = codes.as_slice::(); + let validity = dict_validity(&codes_validity, codes, &values_validity); + let view = |row: usize| &views[AsPrimitive::::as_(codes[row])]; + + // Both passes below resolve a row through its code, so the byte total comes from the same + // walk over the valid rows that the copy will make. + let num_bytes = match validity.bit_buffer() { + AllOr::All => (0..len).map(|row| view(row).len() as usize).sum(), + AllOr::None => { + builder.push_nulls(len); + return Ok(()); + } + AllOr::Some(bits) => { + let mut total = 0; + bits.for_each_set_index(|row| total += view(row).len() as usize); + total + } + }; + + builder.append_valid_slices(num_bytes, &validity, |row| view(row).bytes(&buffers)) + }) +} + +/// Combines the codes' validity with the dictionary values' validity resolved through the codes. +/// +/// A code pointing at a null dictionary entry produces a null row, matching what +/// [`take_canonical`] derives from [`Validity::take`](crate::validity::Validity::take). +fn dict_validity( + codes_validity: &Mask, + codes: &[C], + values_validity: &Mask, +) -> Mask { + if values_validity.all_true() { + return codes_validity.clone(); + } + + let mut valid = BitBufferMut::new_unset(codes.len()); + match codes_validity.bit_buffer() { + AllOr::All => { + for (row, &code) in codes.iter().enumerate() { + valid.set_to(row, values_validity.value(code.as_())); + } + } + AllOr::None => {} + AllOr::Some(bits) => bits.for_each_set_index(|row| { + valid.set_to(row, values_validity.value(codes[row].as_())); + }), + } + Mask::from_buffer(valid.freeze()) +} + +#[cfg(test)] +mod tests { + use std::any::Any; + use std::ops::Range; + use std::sync::Arc; + + use futures::FutureExt; + use futures::future::BoxFuture; + use vortex_buffer::Alignment; + use vortex_buffer::ByteBuffer; + use vortex_buffer::buffer; + + use super::*; + use crate::VortexSessionExecute; + use crate::array_session; + use crate::arrays::VarBinViewArray; + use crate::arrays::dict::DictArray; + use crate::assert_arrays_eq; + use crate::buffer::DeviceBuffer; + use crate::dtype::Nullability::NonNullable; + use crate::dtype::Nullability::Nullable; + use crate::validity::Validity; + + const LONG: &str = "a string that is far too long to be inlined in a view"; + + /// A stand-in for a real device allocation so residency handling can be exercised without a + /// GPU. Only the host/device discriminant matters here; the payload is never read back. + #[derive(Debug, PartialEq, Eq, Hash)] + struct FakeDeviceBuffer(Vec); + + impl DeviceBuffer for FakeDeviceBuffer { + fn as_any(&self) -> &dyn Any { + self + } + + fn len(&self) -> usize { + self.0.len() + } + + fn alignment(&self) -> Alignment { + Alignment::of::() + } + + fn copy_to_host_sync(&self, alignment: Alignment) -> VortexResult { + Ok(ByteBuffer::copy_from_aligned(&self.0, alignment)) + } + + fn copy_to_host( + &self, + alignment: Alignment, + ) -> VortexResult>> { + let copied = self.copy_to_host_sync(alignment)?; + Ok(async move { Ok(copied) }.boxed()) + } + + fn slice(&self, range: Range) -> Arc { + Arc::new(Self(self.0[range].to_vec())) + } + + fn aligned(self: Arc, _alignment: Alignment) -> VortexResult> { + Ok(self) + } + } + + fn to_device(handle: &BufferHandle) -> BufferHandle { + BufferHandle::new_device(Arc::new(FakeDeviceBuffer(handle.as_host().to_vec()))) + } + + #[test] + fn append_to_builder_gathers_through_the_dictionary() -> VortexResult<()> { + let mut ctx = array_session().create_execution_ctx(); + let dict = DictArray::try_new( + PrimitiveArray::from_option_iter([Some(0u32), Some(2), None, Some(1), Some(0)]) + .into_array(), + VarBinViewArray::from_iter([Some(LONG), None, Some("short")], DType::Utf8(Nullable)) + .into_array(), + )?; + + let mut builder = VarBinBuilder::::new(DType::Utf8(Nullable)); + dict.append_to_builder(&mut builder, &mut ctx)?; + + let expected = VarBinViewArray::from_iter( + [Some(LONG), Some("short"), None, None, Some(LONG)], + DType::Utf8(Nullable), + ); + assert_arrays_eq!(builder.finish_into_varbin(), expected, &mut ctx); + Ok(()) + } + + /// The fast path indexes the codes and the dictionary's buffers as host slices, so it must be + /// skipped whenever any of them lives on a device. Regression test for the fast path panicking + /// on device-resident dictionaries instead of deferring to the canonical take. + #[test] + fn fast_path_is_skipped_for_device_resident_buffers() { + let codes = PrimitiveArray::new(buffer![0u32, 1, 0], Validity::NonNullable); + let values = VarBinViewArray::from_iter_str([LONG, "short"]); + + assert!(varbin_fast_path_is_host(&codes, &values)); + + let device_codes = PrimitiveArray::from_buffer_handle( + to_device(codes.buffer_handle()), + PType::U32, + Validity::NonNullable, + ); + assert!(!varbin_fast_path_is_host(&device_codes, &values)); + + let device_views = VarBinViewArray::new_handle( + to_device(values.views_handle()), + Arc::clone(values.data_buffers()), + DType::Utf8(NonNullable), + Validity::NonNullable, + ); + assert!(!varbin_fast_path_is_host(&codes, &device_views)); + + let device_data = VarBinViewArray::new_handle( + values.views_handle().clone(), + values.data_buffers().iter().map(to_device).collect(), + DType::Utf8(NonNullable), + Validity::NonNullable, + ); + assert!(!varbin_fast_path_is_host(&codes, &device_data)); + } +} From 5c5a35b9a4d95e0cb7b1cb9b2b399546fb5b9d6d Mon Sep 17 00:00:00 2001 From: Robert Kruszewski Date: Wed, 5 Aug 2026 00:13:32 +0100 Subject: [PATCH 2/6] fixes Signed-off-by: Robert Kruszewski --- vortex-array/src/arrays/dict/execute.rs | 111 ++++++++++----------- vortex-array/src/arrays/dict/vtable/mod.rs | 108 ++------------------ 2 files changed, 62 insertions(+), 157 deletions(-) diff --git a/vortex-array/src/arrays/dict/execute.rs b/vortex-array/src/arrays/dict/execute.rs index 4a3ccda43c2..cfc2ab89c46 100644 --- a/vortex-array/src/arrays/dict/execute.rs +++ b/vortex-array/src/arrays/dict/execute.rs @@ -3,13 +3,13 @@ //! Execution logic for DictArray - takes from values using codes (indices). -use vortex_error::VortexExpect; use vortex_error::VortexResult; +use vortex_error::{VortexExpect, vortex_bail}; +use crate::ArrayView; use crate::Canonical; use crate::CanonicalView; use crate::ExecutionCtx; -use crate::IntoArray; use crate::arrays::Bool; use crate::arrays::BoolArray; use crate::arrays::Decimal; @@ -20,6 +20,7 @@ use crate::arrays::FixedSizeList; use crate::arrays::FixedSizeListArray; use crate::arrays::ListView; use crate::arrays::ListViewArray; +use crate::arrays::Null; use crate::arrays::NullArray; use crate::arrays::Primitive; use crate::arrays::PrimitiveArray; @@ -38,35 +39,34 @@ use crate::arrays::variant::VariantArraySlotsExt; /// by looking up each code in the values array. pub(crate) fn take_canonical( values: CanonicalView, - codes: &PrimitiveArray, + codes: ArrayView<'_, Primitive>, ctx: &mut ExecutionCtx, ) -> VortexResult { - let values = Canonical::from(values); Ok(match values { - Canonical::Null(a) => Canonical::Null(take_null(&a, codes)), - Canonical::Bool(a) => Canonical::Bool(take_bool(&a, codes, ctx)?), - Canonical::Primitive(a) => Canonical::Primitive(take_primitive(&a, codes, ctx)), - Canonical::Decimal(a) => Canonical::Decimal(take_decimal(&a, codes, ctx)), - Canonical::VarBinView(a) => Canonical::VarBinView(take_varbinview(&a, codes, ctx)), - Canonical::List(a) => Canonical::List(take_listview(&a, codes, ctx)), - Canonical::Map(_) => vortex_error::vortex_bail!("Map arrays don't support take"), - Canonical::FixedSizeList(a) => { - Canonical::FixedSizeList(take_fixed_size_list(&a, codes, ctx)) + CanonicalView::Null(a) => Canonical::Null(take_null(a, codes)), + CanonicalView::Bool(a) => Canonical::Bool(take_bool(a, codes, ctx)?), + CanonicalView::Primitive(a) => Canonical::Primitive(take_primitive(a, codes, ctx)), + CanonicalView::Decimal(a) => Canonical::Decimal(take_decimal(a, codes, ctx)), + CanonicalView::VarBinView(a) => Canonical::VarBinView(take_varbinview(a, codes, ctx)), + CanonicalView::List(a) => Canonical::List(take_listview(a, codes, ctx)), + Canonical::Map(_) => vortex_bail!("Map arrays don't support take"), + CanonicalView::FixedSizeList(a) => { + Canonical::FixedSizeList(take_fixed_size_list(a, codes, ctx)) } - Canonical::Struct(a) => Canonical::Struct(take_struct(&a, codes)), - Canonical::Union(_) => { + CanonicalView::Struct(a) => Canonical::Struct(take_struct(a, codes)), + CanonicalView::Union(_) => { todo!( "TODO(connor)[Union]: implement dictionary execution after Union take supports \ nullable indices and outer null propagation" ) } - Canonical::Extension(a) => Canonical::Extension(take_extension(&a, codes, ctx)), - Canonical::Variant(a) => { - let indices = codes.clone().into_array(); + CanonicalView::Extension(a) => Canonical::Extension(take_extension(a, codes, ctx)), + CanonicalView::Variant(a) => { + let indices = codes.array().clone(); let taken_core_storage = a.core_storage().take(indices.clone())?; let taken_shredded = a .shredded() - .map(|shredded| shredded.take(indices.clone())) + .map(|shredded| shredded.take(indices)) .transpose()?; Canonical::Variant(VariantArray::try_new(taken_core_storage, taken_shredded)?) } @@ -74,32 +74,29 @@ pub(crate) fn take_canonical( } /// Take for NullArray is trivial - just create a new NullArray with the new length. -fn take_null(_array: &NullArray, codes: &PrimitiveArray) -> NullArray { +fn take_null(_array: ArrayView<'_, Null>, codes: ArrayView<'_, Primitive>) -> NullArray { NullArray::new(codes.len()) } -// TODO(joe): use dict_bool_take fn take_bool( - array: &BoolArray, - codes: &PrimitiveArray, + array: ArrayView<'_, Bool>, + codes: ArrayView<'_, Primitive>, ctx: &mut ExecutionCtx, ) -> VortexResult { - let codes_ref = codes.clone().into_array(); - let array = array.as_view(); - Ok(::take(array, &codes_ref, ctx)? + let codes_ref = codes.array(); + Ok(::take(array, codes_ref, ctx)? .vortex_expect("take bool should not return None") .as_::() .into_owned()) } fn take_primitive( - array: &PrimitiveArray, - codes: &PrimitiveArray, + array: ArrayView<'_, Primitive>, + codes: ArrayView<'_, Primitive>, ctx: &mut ExecutionCtx, ) -> PrimitiveArray { - let codes_ref = codes.clone().into_array(); - let array = array.as_view(); - ::take(array, &codes_ref, ctx) + let codes_ref = codes.array(); + ::take(array, codes_ref, ctx) .vortex_expect("take primitive array") .vortex_expect("take primitive should not return None") .as_::() @@ -107,13 +104,12 @@ fn take_primitive( } fn take_decimal( - array: &DecimalArray, - codes: &PrimitiveArray, + array: ArrayView<'_, Decimal>, + codes: ArrayView<'_, Primitive>, ctx: &mut ExecutionCtx, ) -> DecimalArray { - let codes_ref = codes.clone().into_array(); - let array = array.as_view(); - ::take(array, &codes_ref, ctx) + let codes_ref = codes.array(); + ::take(array, codes_ref, ctx) .vortex_expect("take decimal array") .vortex_expect("take decimal should not return None") .as_::() @@ -121,13 +117,12 @@ fn take_decimal( } fn take_varbinview( - array: &VarBinViewArray, - codes: &PrimitiveArray, + array: ArrayView<'_, VarBinView>, + codes: ArrayView<'_, Primitive>, ctx: &mut ExecutionCtx, ) -> VarBinViewArray { - let codes_ref = codes.clone().into_array(); - let array = array.as_view(); - ::take(array, &codes_ref, ctx) + let codes_ref = codes.array(); + ::take(array, codes_ref, ctx) .vortex_expect("take varbinview array") .vortex_expect("take varbinview should not return None") .as_::() @@ -135,13 +130,12 @@ fn take_varbinview( } fn take_listview( - array: &ListViewArray, - codes: &PrimitiveArray, + array: ArrayView<'_, ListView>, + codes: ArrayView<'_, Primitive>, ctx: &mut ExecutionCtx, ) -> ListViewArray { - let codes_ref = codes.clone().into_array(); - let array = array.as_view(); - ::take(array, &codes_ref, ctx) + let codes_ref = codes.array(); + ::take(array, codes_ref, ctx) .vortex_expect("take listview execute") .vortex_expect("ListView TakeExecute should not return None") .as_::() @@ -149,23 +143,21 @@ fn take_listview( } fn take_fixed_size_list( - array: &FixedSizeListArray, - codes: &PrimitiveArray, + array: ArrayView<'_, FixedSizeList>, + codes: ArrayView<'_, Primitive>, ctx: &mut ExecutionCtx, ) -> FixedSizeListArray { - let codes_ref = codes.clone().into_array(); - let array = array.as_view(); - ::take(array, &codes_ref, ctx) + let codes_ref = codes.array(); + ::take(array, codes_ref, ctx) .vortex_expect("take fixed size list array") .vortex_expect("take fixed size list should not return None") .as_::() .into_owned() } -fn take_struct(array: &StructArray, codes: &PrimitiveArray) -> StructArray { - let codes_ref = codes.clone().into_array(); - let array = array.as_view(); - ::take(array, &codes_ref) +fn take_struct(array: ArrayView<'_, Struct>, codes: ArrayView<'_, Primitive>) -> StructArray { + let codes_ref = codes.array(); + ::take(array, codes_ref) .vortex_expect("take struct array") .vortex_expect("take struct should not return None") .as_::() @@ -173,13 +165,12 @@ fn take_struct(array: &StructArray, codes: &PrimitiveArray) -> StructArray { } fn take_extension( - array: &ExtensionArray, - codes: &PrimitiveArray, + array: ArrayView<'_, Extension>, + codes: ArrayView<'_, Primitive>, ctx: &mut ExecutionCtx, ) -> ExtensionArray { - let codes_ref = codes.clone().into_array(); - let array = array.as_view(); - ::take(array, &codes_ref, ctx) + let codes_ref = codes.array(); + ::take(array, codes_ref, ctx) .vortex_expect("take extension storage") .vortex_expect("take extension should not return None") .as_::() diff --git a/vortex-array/src/arrays/dict/vtable/mod.rs b/vortex-array/src/arrays/dict/vtable/mod.rs index 892e74e2b94..eaed9e72cc1 100644 --- a/vortex-array/src/arrays/dict/vtable/mod.rs +++ b/vortex-array/src/arrays/dict/vtable/mod.rs @@ -6,7 +6,6 @@ use std::hash::Hasher; use num_traits::AsPrimitive; use prost::Message; use smallvec::smallvec; -use vortex_buffer::BitBufferMut; use vortex_error::VortexResult; use vortex_error::vortex_bail; use vortex_error::vortex_ensure; @@ -28,6 +27,7 @@ use crate::ArrayEq; use crate::ArrayHash; use crate::ArrayRef; use crate::Canonical; +use crate::CanonicalView; use crate::EqMode; use crate::IntoArray; use crate::array::Array; @@ -38,8 +38,7 @@ use crate::array::VTable; use crate::array::with_empty_buffers; use crate::arrays::ConstantArray; use crate::arrays::Primitive; -use crate::arrays::PrimitiveArray; -use crate::arrays::VarBinViewArray; +use crate::arrays::VarBinView; use crate::arrays::dict::DictArrayExt; use crate::arrays::dict::DictArraySlotsExt; use crate::arrays::dict::compute::rules::PARENT_RULES; @@ -48,7 +47,6 @@ use crate::buffer::BufferHandle; use crate::builders::ArrayBuilder; use crate::builders::VarBinBuilder; use crate::dtype::DType; -use crate::dtype::IntegerPType; use crate::dtype::Nullability; use crate::dtype::OffsetBuilderPType; use crate::dtype::PType; @@ -216,7 +214,7 @@ impl VTable for Dict { Ok(ExecutionResult::done(take_canonical( values.as_::(), - &codes.downcast::(), + codes.as_::(), ctx, )?)) } @@ -233,16 +231,15 @@ impl VTable for Dict { ) && !codes.validity()?.definitely_all_null() { - let codes = codes.into_owned(); - if let Canonical::VarBinView(values) = Canonical::from(values) - && varbin_fast_path_is_host(&codes, &values) + if let CanonicalView::VarBinView(values) = values && let Some(result) = match_each_varbin_builder!(builder, |builder| { - append_dict_to_varbin(&codes, &values, builder, ctx) + let validity = array.validity()?.execute_mask(array.len(), ctx)?; + append_dict_to_varbin(codes, values, validity, builder) }) { return result; } - let canonical = take_canonical(values, &codes, ctx)?.into_array(); + let canonical = take_canonical(values, codes, ctx)?.into_array(); canonical.append_to_builder(builder, ctx)?; return Ok(()); } @@ -265,22 +262,6 @@ impl VTable for Dict { } } -/// Whether every buffer that [`append_dict_to_varbin`] dereferences directly is host-resident. -/// -/// The fast path reads the codes and the dictionary's views and data buffers as plain slices, -/// which is only possible for host allocations. Device-resident dictionaries fall back to the -/// canonical take, which goes through the compute stack and can therefore be served by a device -/// kernel. This mirrors [`DictArrayExt::validate_all_values_referenced`], which likewise skips -/// host-only work when the codes are not resident on the host. -/// -/// This is a handful of enum discriminant checks over the array's buffer handles, so it stays -/// outside the per-row loop and costs nothing measurable on the host-resident path. -fn varbin_fast_path_is_host(codes: &PrimitiveArray, values: &VarBinViewArray) -> bool { - codes.buffer_handle().is_on_host() - && values.views_handle().is_on_host() - && values.data_buffers().iter().all(BufferHandle::is_on_host) -} - /// Gathers the dictionary values straight into `builder`. /// /// The canonical route first takes the values to full logical length, which allocates and then @@ -291,17 +272,15 @@ fn varbin_fast_path_is_host(codes: &PrimitiveArray, values: &VarBinViewArray) -> /// The caller must have checked [`varbin_fast_path_is_host`]; this function indexes the backing /// buffers as host slices and panics otherwise. fn append_dict_to_varbin( - codes: &PrimitiveArray, - values: &VarBinViewArray, + codes: ArrayView<'_, Primitive>, + values: ArrayView<'_, VarBinView>, + validity: Mask, builder: &mut VarBinBuilder, - ctx: &mut ExecutionCtx, ) -> VortexResult<()> where usize: AsPrimitive, { let len = codes.as_ref().len(); - let codes_validity = codes.as_ref().validity()?.execute_mask(len, ctx)?; - let values_validity = values.validity()?.execute_mask(values.len(), ctx)?; // Resolve the dictionary's storage once so that looking up a code is an O(1) read. let views = values.views(); @@ -313,7 +292,6 @@ where match_each_integer_ptype!(codes.ptype(), |C| { let codes = codes.as_slice::(); - let validity = dict_validity(&codes_validity, codes, &values_validity); let view = |row: usize| &views[AsPrimitive::::as_(codes[row])]; // Both passes below resolve a row through its code, so the byte total comes from the same @@ -335,34 +313,6 @@ where }) } -/// Combines the codes' validity with the dictionary values' validity resolved through the codes. -/// -/// A code pointing at a null dictionary entry produces a null row, matching what -/// [`take_canonical`] derives from [`Validity::take`](crate::validity::Validity::take). -fn dict_validity( - codes_validity: &Mask, - codes: &[C], - values_validity: &Mask, -) -> Mask { - if values_validity.all_true() { - return codes_validity.clone(); - } - - let mut valid = BitBufferMut::new_unset(codes.len()); - match codes_validity.bit_buffer() { - AllOr::All => { - for (row, &code) in codes.iter().enumerate() { - valid.set_to(row, values_validity.value(code.as_())); - } - } - AllOr::None => {} - AllOr::Some(bits) => bits.for_each_set_index(|row| { - valid.set_to(row, values_validity.value(codes[row].as_())); - }), - } - Mask::from_buffer(valid.freeze()) -} - #[cfg(test)] mod tests { use std::any::Any; @@ -373,18 +323,16 @@ mod tests { use futures::future::BoxFuture; use vortex_buffer::Alignment; use vortex_buffer::ByteBuffer; - use vortex_buffer::buffer; use super::*; use crate::VortexSessionExecute; use crate::array_session; + use crate::arrays::PrimitiveArray; use crate::arrays::VarBinViewArray; use crate::arrays::dict::DictArray; use crate::assert_arrays_eq; use crate::buffer::DeviceBuffer; - use crate::dtype::Nullability::NonNullable; use crate::dtype::Nullability::Nullable; - use crate::validity::Validity; const LONG: &str = "a string that is far too long to be inlined in a view"; @@ -451,38 +399,4 @@ mod tests { assert_arrays_eq!(builder.finish_into_varbin(), expected, &mut ctx); Ok(()) } - - /// The fast path indexes the codes and the dictionary's buffers as host slices, so it must be - /// skipped whenever any of them lives on a device. Regression test for the fast path panicking - /// on device-resident dictionaries instead of deferring to the canonical take. - #[test] - fn fast_path_is_skipped_for_device_resident_buffers() { - let codes = PrimitiveArray::new(buffer![0u32, 1, 0], Validity::NonNullable); - let values = VarBinViewArray::from_iter_str([LONG, "short"]); - - assert!(varbin_fast_path_is_host(&codes, &values)); - - let device_codes = PrimitiveArray::from_buffer_handle( - to_device(codes.buffer_handle()), - PType::U32, - Validity::NonNullable, - ); - assert!(!varbin_fast_path_is_host(&device_codes, &values)); - - let device_views = VarBinViewArray::new_handle( - to_device(values.views_handle()), - Arc::clone(values.data_buffers()), - DType::Utf8(NonNullable), - Validity::NonNullable, - ); - assert!(!varbin_fast_path_is_host(&codes, &device_views)); - - let device_data = VarBinViewArray::new_handle( - values.views_handle().clone(), - values.data_buffers().iter().map(to_device).collect(), - DType::Utf8(NonNullable), - Validity::NonNullable, - ); - assert!(!varbin_fast_path_is_host(&codes, &device_data)); - } } From 68d1b42e2ecd28ffa0cea47e484ea40b1870a68e Mon Sep 17 00:00:00 2001 From: Robert Kruszewski Date: Wed, 5 Aug 2026 00:14:52 +0100 Subject: [PATCH 3/6] fixes Signed-off-by: Robert Kruszewski --- vortex-array/src/arrays/dict/execute.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/vortex-array/src/arrays/dict/execute.rs b/vortex-array/src/arrays/dict/execute.rs index cfc2ab89c46..c0f415483af 100644 --- a/vortex-array/src/arrays/dict/execute.rs +++ b/vortex-array/src/arrays/dict/execute.rs @@ -49,7 +49,7 @@ pub(crate) fn take_canonical( CanonicalView::Decimal(a) => Canonical::Decimal(take_decimal(a, codes, ctx)), CanonicalView::VarBinView(a) => Canonical::VarBinView(take_varbinview(a, codes, ctx)), CanonicalView::List(a) => Canonical::List(take_listview(a, codes, ctx)), - Canonical::Map(_) => vortex_bail!("Map arrays don't support take"), + CanonicalView::Map(_) => vortex_bail!("Map arrays don't support take"), CanonicalView::FixedSizeList(a) => { Canonical::FixedSizeList(take_fixed_size_list(a, codes, ctx)) } From 1a2a0444a2d8128d7c28180c9c199c7708d3aa43 Mon Sep 17 00:00:00 2001 From: Robert Kruszewski Date: Wed, 5 Aug 2026 00:21:56 +0100 Subject: [PATCH 4/6] less Signed-off-by: Robert Kruszewski --- vortex-array/src/arrays/dict/vtable/mod.rs | 52 ---------------------- 1 file changed, 52 deletions(-) diff --git a/vortex-array/src/arrays/dict/vtable/mod.rs b/vortex-array/src/arrays/dict/vtable/mod.rs index eaed9e72cc1..197dbd8da1f 100644 --- a/vortex-array/src/arrays/dict/vtable/mod.rs +++ b/vortex-array/src/arrays/dict/vtable/mod.rs @@ -315,15 +315,6 @@ where #[cfg(test)] mod tests { - use std::any::Any; - use std::ops::Range; - use std::sync::Arc; - - use futures::FutureExt; - use futures::future::BoxFuture; - use vortex_buffer::Alignment; - use vortex_buffer::ByteBuffer; - use super::*; use crate::VortexSessionExecute; use crate::array_session; @@ -336,49 +327,6 @@ mod tests { const LONG: &str = "a string that is far too long to be inlined in a view"; - /// A stand-in for a real device allocation so residency handling can be exercised without a - /// GPU. Only the host/device discriminant matters here; the payload is never read back. - #[derive(Debug, PartialEq, Eq, Hash)] - struct FakeDeviceBuffer(Vec); - - impl DeviceBuffer for FakeDeviceBuffer { - fn as_any(&self) -> &dyn Any { - self - } - - fn len(&self) -> usize { - self.0.len() - } - - fn alignment(&self) -> Alignment { - Alignment::of::() - } - - fn copy_to_host_sync(&self, alignment: Alignment) -> VortexResult { - Ok(ByteBuffer::copy_from_aligned(&self.0, alignment)) - } - - fn copy_to_host( - &self, - alignment: Alignment, - ) -> VortexResult>> { - let copied = self.copy_to_host_sync(alignment)?; - Ok(async move { Ok(copied) }.boxed()) - } - - fn slice(&self, range: Range) -> Arc { - Arc::new(Self(self.0[range].to_vec())) - } - - fn aligned(self: Arc, _alignment: Alignment) -> VortexResult> { - Ok(self) - } - } - - fn to_device(handle: &BufferHandle) -> BufferHandle { - BufferHandle::new_device(Arc::new(FakeDeviceBuffer(handle.as_host().to_vec()))) - } - #[test] fn append_to_builder_gathers_through_the_dictionary() -> VortexResult<()> { let mut ctx = array_session().create_execution_ctx(); From 6f57c8455cd939fa77bdaadf1abd883dc5f1df53 Mon Sep 17 00:00:00 2001 From: Robert Kruszewski Date: Wed, 5 Aug 2026 15:15:19 +0100 Subject: [PATCH 5/6] remove Signed-off-by: Robert Kruszewski --- vortex-array/src/arrays/dict/vtable/mod.rs | 3 --- 1 file changed, 3 deletions(-) diff --git a/vortex-array/src/arrays/dict/vtable/mod.rs b/vortex-array/src/arrays/dict/vtable/mod.rs index 197dbd8da1f..1afc5642a43 100644 --- a/vortex-array/src/arrays/dict/vtable/mod.rs +++ b/vortex-array/src/arrays/dict/vtable/mod.rs @@ -268,9 +268,6 @@ impl VTable for Dict { /// re-reads a views buffer proportional to the row count. The dictionary is usually far smaller /// than the column, so resolving each code against it in place skips that intermediate entirely /// and leaves one `memcpy` per row as the only work. -/// -/// The caller must have checked [`varbin_fast_path_is_host`]; this function indexes the backing -/// buffers as host slices and panics otherwise. fn append_dict_to_varbin( codes: ArrayView<'_, Primitive>, values: ArrayView<'_, VarBinView>, From 759cab5b073e780b81eaf23d7b28003e2529c3a0 Mon Sep 17 00:00:00 2001 From: Robert Kruszewski Date: Wed, 5 Aug 2026 15:26:00 +0100 Subject: [PATCH 6/6] format Signed-off-by: Robert Kruszewski --- vortex-array/src/arrays/dict/execute.rs | 3 ++- vortex-array/src/arrays/dict/vtable/mod.rs | 1 - 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/vortex-array/src/arrays/dict/execute.rs b/vortex-array/src/arrays/dict/execute.rs index c0f415483af..6462ed38d16 100644 --- a/vortex-array/src/arrays/dict/execute.rs +++ b/vortex-array/src/arrays/dict/execute.rs @@ -3,8 +3,9 @@ //! Execution logic for DictArray - takes from values using codes (indices). +use vortex_error::VortexExpect; use vortex_error::VortexResult; -use vortex_error::{VortexExpect, vortex_bail}; +use vortex_error::vortex_bail; use crate::ArrayView; use crate::Canonical; diff --git a/vortex-array/src/arrays/dict/vtable/mod.rs b/vortex-array/src/arrays/dict/vtable/mod.rs index 1afc5642a43..62e324f4ba5 100644 --- a/vortex-array/src/arrays/dict/vtable/mod.rs +++ b/vortex-array/src/arrays/dict/vtable/mod.rs @@ -319,7 +319,6 @@ mod tests { use crate::arrays::VarBinViewArray; use crate::arrays::dict::DictArray; use crate::assert_arrays_eq; - use crate::buffer::DeviceBuffer; use crate::dtype::Nullability::Nullable; const LONG: &str = "a string that is far too long to be inlined in a view";