From ec2b1b5ac88d3f17c6ea7be90e79cdca2721a24a Mon Sep 17 00:00:00 2001 From: Joe Isaacs Date: Mon, 5 Oct 2026 18:13:04 +0100 Subject: [PATCH 01/11] Bound decimal storage by precision Signed-off-by: Joe Isaacs --- .../src/arrays/constant/vtable/canonical.rs | 17 +-- vortex-array/src/arrays/decimal/array.rs | 57 +++++++++- .../src/arrays/decimal/compute/cast.rs | 24 +++- .../src/arrays/decimal/compute/fill_null.rs | 28 ++++- .../src/arrays/decimal/compute/rules.rs | 15 ++- vortex-array/src/arrays/decimal/tests.rs | 107 ++++++++++++++++++ vortex-array/src/arrays/decimal/vtable/mod.rs | 55 ++++++++- .../src/arrays/decimal/vtable/operations.rs | 6 +- vortex-arrow/src/convert.rs | 16 ++- 9 files changed, 293 insertions(+), 32 deletions(-) create mode 100644 vortex-array/src/arrays/decimal/tests.rs diff --git a/vortex-array/src/arrays/constant/vtable/canonical.rs b/vortex-array/src/arrays/constant/vtable/canonical.rs index e1f9ad02f6c..490a18c2f8c 100644 --- a/vortex-array/src/arrays/constant/vtable/canonical.rs +++ b/vortex-array/src/arrays/constant/vtable/canonical.rs @@ -38,11 +38,9 @@ use crate::builders::builder_with_capacity_in; use crate::dtype::DType; use crate::dtype::DecimalType; 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; @@ -104,15 +102,12 @@ pub(crate) fn constant_canonicalize( return Ok(Canonical::Decimal(all_null)); }; - let decimal_array = match_each_decimal_value!(value, |value| { - // SAFETY: Constant decimal values with correct type and validity. - unsafe { - DecimalArray::new_unchecked( - Buffer::full(value, array.len()), - *decimal_type, - validity, - ) - } + let storage = value.decimal_type().min(size); + let decimal_array = match_each_decimal_value_type!(storage, |D| { + let value = value + .cast::() + .vortex_expect("decimal scalar fits its precision"); + DecimalArray::new(Buffer::full(value, array.len()), *decimal_type, validity) }); Canonical::Decimal(decimal_array) } diff --git a/vortex-array/src/arrays/decimal/array.rs b/vortex-array/src/arrays/decimal/array.rs index f59ca7774a9..61cf939a314 100644 --- a/vortex-array/src/arrays/decimal/array.rs +++ b/vortex-array/src/arrays/decimal/array.rs @@ -5,6 +5,7 @@ use std::fmt::Display; use std::fmt::Formatter; use itertools::Itertools; +use num_traits::AsPrimitive; use smallvec::smallvec; use vortex_buffer::Alignment; use vortex_buffer::BitBufferMut; @@ -71,8 +72,12 @@ pub struct DecimalSlots { /// values with precision that does not match this exactly. For example, a valid DecimalArray with /// precision=39 may store its values in an `i8` if all of the actual values fit into it. /// -/// Similarly, a `DecimalArray` can be built that stores a set of precision=2 values in a -/// `Buffer`. +/// The physical value type must be no wider than +/// [`DecimalType::smallest_decimal_value_type`] for the array's precision. Thus precision=2 +/// requires `i8` storage, while precision=39 permits any supported storage type. Compute kernels +/// can widen storage to the type selected by the precision without handling larger inputs. +/// Checked constructors narrow oversized host buffers to this bound; unchecked constructors +/// require it already to hold. Device buffers must already satisfy the bound. /// /// ## Precision and Scale /// @@ -187,6 +192,9 @@ impl DecimalData { /// Creates a new [`DecimalArray`] using a host-native buffer. /// + /// Oversized storage is narrowed to the type required by the precision. Non-null values + /// must fit the declared precision; their range is not checked. + /// /// # Panics /// /// Panics if the provided components do not satisfy the invariants documented in @@ -231,7 +239,9 @@ impl DecimalData { /// Constructs a new `DecimalArray` with validation from a [`BufferHandle`]. /// - /// This pathway allows building new decimal arrays that may come from host or device memory. + /// Oversized host buffers are narrowed to the type required by the precision. Device buffers + /// must already satisfy this bound. Non-null values must fit the declared precision; their + /// range is not checked. /// /// # Errors /// @@ -242,8 +252,30 @@ impl DecimalData { decimal_dtype: DecimalDType, ) -> VortexResult { Self::validate(&values, values_type)?; + let maximum = DecimalType::smallest_decimal_value_type(&decimal_dtype); + let (values, values_type) = if values_type > maximum { + vortex_ensure!( + values.is_on_host(), + InvalidArgument: "decimal device storage {values_type} exceeds precision storage {maximum}", + ); + let narrowed = match_each_decimal_value_type!(values_type, |In| { + let buffer = Buffer::::from_byte_buffer(values.unwrap_host()); + match_each_decimal_value_type!(maximum, |Out| { + // Non-null values must already fit the declared precision. Null payloads + // are unspecified and may be truncated along with the buffer. + buffer + .iter() + .map(|value| value.as_()) + .collect::>() + .into_byte_buffer() + }) + }); + (BufferHandle::new_host(narrowed), maximum) + } else { + (values, values_type) + }; - // SAFETY: validate ensures all invariants are met. + // SAFETY: buffer layout is validated and storage is bounded by the precision. Ok(unsafe { Self::new_unchecked_handle(values, values_type, decimal_dtype) }) } @@ -257,6 +289,8 @@ impl DecimalData { /// /// The caller must ensure all of the following invariants are satisfied: /// + /// - `T::DECIMAL_TYPE` must be no wider than + /// [`DecimalType::smallest_decimal_value_type`] for `decimal_dtype`. /// - All non-null values in `buffer` must be representable within the specified precision. /// - For example, with precision=5 and scale=2, all values must be in range [-999.99, 999.99]. /// - If `validity` is [`Validity::Array`], its length must exactly equal `buffer.len()`. @@ -280,6 +314,8 @@ impl DecimalData { /// /// The caller must ensure all of the following invariants are satisfied: /// + /// - `values_type` must be no wider than + /// [`DecimalType::smallest_decimal_value_type`] for `decimal_dtype`. /// - All non-null values in `values` must be representable within the specified precision. /// - For example, with precision=5 and scale=2, all values must be in range [-999.99, 999.99]. /// - If `validity` is [`Validity::Array`], its length must exactly equal `buffer.len()`. @@ -288,6 +324,10 @@ impl DecimalData { values_type: DecimalType, decimal_dtype: DecimalDType, ) -> Self { + debug_assert!( + values_type <= DecimalType::smallest_decimal_value_type(&decimal_dtype), + "decimal storage {values_type} exceeds the storage required by {decimal_dtype}", + ); Self { decimal_dtype, values, @@ -323,6 +363,8 @@ impl DecimalData { /// # Safety /// /// The caller must ensure: + /// - `values_type` is no wider than + /// [`DecimalType::smallest_decimal_value_type`] for `decimal_dtype` /// - The `byte_buffer` contains valid data for the specified `values_type` /// - The buffer length is compatible with the `values_type` (i.e., divisible by the type size) /// - All non-null values are representable within the specified precision @@ -403,6 +445,9 @@ impl Array { impl Array { /// Creates a new [`DecimalArray`] using a host-native buffer. + /// + /// Oversized storage is narrowed to the type required by the precision. Non-null values + /// must fit the declared precision; their range is not checked. pub fn new( buffer: Buffer, decimal_dtype: DecimalDType, @@ -617,3 +662,7 @@ where DecimalData::new(buffer.freeze(), decimal_dtype) } + +#[cfg(test)] +#[path = "tests.rs"] +mod tests; diff --git a/vortex-array/src/arrays/decimal/compute/cast.rs b/vortex-array/src/arrays/decimal/compute/cast.rs index dd643bdb5cf..b7e94fbbf66 100644 --- a/vortex-array/src/arrays/decimal/compute/cast.rs +++ b/vortex-array/src/arrays/decimal/compute/cast.rs @@ -428,14 +428,13 @@ where /// Upcast a DecimalArray to a wider physical representation (e.g., i32 -> i64) while keeping /// the same precision and scale. /// -/// This is useful when you need to widen the underlying storage type to accommodate operations -/// that might overflow the current representation, or to match the physical type expected by -/// downstream consumers. +/// The requested storage must remain no wider than the type required by the precision. +/// Kernels needing a wider working type should widen the buffer without constructing an array. /// /// # Errors /// -/// Returns an error if `to_values_type` is narrower than the array's current values type. -/// Only upcasting (widening) is supported. +/// Returns an error if `to_values_type` is narrower than the current storage or wider than +/// [`DecimalType::smallest_decimal_value_type`] for the array's precision. pub fn upcast_decimal_values( array: ArrayView<'_, Decimal>, to_values_type: DecimalType, @@ -457,6 +456,9 @@ pub fn upcast_decimal_values( } let decimal_dtype = array.decimal_dtype(); + if to_values_type > DecimalType::smallest_decimal_value_type(&decimal_dtype) { + vortex_bail!("Cannot widen decimal storage to {to_values_type} for {decimal_dtype}"); + } let validity = array.validity()?; // Use match_each_decimal_value_type to dispatch based on source and target types @@ -1026,7 +1028,7 @@ mod tests { #[test] fn upcast_decimal_values_i64_to_i128() { - let decimal_dtype = DecimalDType::new(18, 4); + let decimal_dtype = DecimalDType::new(19, 4); let array = DecimalArray::new( buffer![10000i64, 20000, 30000], decimal_dtype, @@ -1090,6 +1092,16 @@ mod tests { assert_eq!(buffer[2], 300); } + #[test] + fn upcast_decimal_values_exceeding_precision_fails() { + let array = DecimalArray::new( + buffer![100i64], + DecimalDType::new(18, 2), + Validity::NonNullable, + ); + assert!(upcast_decimal_values(array.as_view(), DecimalType::I128).is_err()); + } + #[test] fn upcast_decimal_values_downcast_fails() { let decimal_dtype = DecimalDType::new(18, 4); diff --git a/vortex-array/src/arrays/decimal/compute/fill_null.rs b/vortex-array/src/arrays/decimal/compute/fill_null.rs index c4de15de5cc..c4777e1c4f5 100644 --- a/vortex-array/src/arrays/decimal/compute/fill_null.rs +++ b/vortex-array/src/arrays/decimal/compute/fill_null.rs @@ -1,7 +1,6 @@ // SPDX-License-Identifier: Apache-2.0 // SPDX-FileCopyrightText: Copyright the Vortex contributors -use std::cmp::max; use std::ops::Not; use vortex_buffer::BitBuffer; @@ -16,6 +15,7 @@ use crate::array::ArrayView; use crate::arrays::BoolArray; use crate::arrays::Decimal; use crate::arrays::DecimalArray; +use crate::dtype::DecimalType; use crate::dtype::NativeDecimalType; use crate::match_each_decimal_value_type; use crate::scalar::DecimalValue; @@ -61,7 +61,7 @@ fn fill_invalid_positions( match decimal_value.cast::() { Some(fill_val) => fill_buffer::(array, is_invalid, fill_val, result_validity), None => { - let target = max(array.values_type(), decimal_value.decimal_type()); + let target = DecimalType::smallest_decimal_value_type(&array.decimal_dtype()); let upcasted = upcast_decimal_values(array, target)?; match_each_decimal_value_type!(upcasted.values_type(), |U| { let upcasted = upcasted.as_view(); @@ -87,6 +87,7 @@ fn fill_buffer( #[cfg(test)] mod tests { use vortex_buffer::buffer; + use vortex_error::VortexResult; use crate::IntoArray; use crate::VortexSessionExecute; @@ -95,11 +96,34 @@ mod tests { use crate::assert_arrays_eq; use crate::builtins::ArrayBuiltins; use crate::dtype::DecimalDType; + use crate::dtype::DecimalType; use crate::dtype::Nullability; use crate::scalar::DecimalValue; use crate::scalar::Scalar; use crate::validity::Validity; + #[test] + fn fill_null_widens_only_to_precision_bound() -> VortexResult<()> { + let mut ctx = array_session().create_execution_ctx(); + let dtype = DecimalDType::new(4, 0); + let array = DecimalArray::from_option_iter([Some(1i8), None], dtype); + let filled = array + .into_array() + .fill_null(Scalar::decimal( + DecimalValue::I128(300), + dtype, + Nullability::NonNullable, + ))? + .execute::(&mut ctx)?; + assert_eq!(filled.values_type(), DecimalType::I16); + assert_arrays_eq!( + filled, + DecimalArray::new(buffer![1i16, 300], dtype, Validity::NonNullable), + &mut ctx + ); + Ok(()) + } + #[test] fn fill_null_leading_none() { let mut ctx = array_session().create_execution_ctx(); diff --git a/vortex-array/src/arrays/decimal/compute/rules.rs b/vortex-array/src/arrays/decimal/compute/rules.rs index a6775aba54a..7979e008605 100644 --- a/vortex-array/src/arrays/decimal/compute/rules.rs +++ b/vortex-array/src/arrays/decimal/compute/rules.rs @@ -112,6 +112,19 @@ mod tests { use crate::match_each_decimal_value_type; use crate::validity::Validity; + #[test] + fn oversized_device_storage_is_rejected() { + let bytes = Buffer::from_iter([1i64]).into_byte_buffer(); + let handle = BufferHandle::new_device(Arc::new(TestDeviceBuffer(bytes))); + let result = DecimalArray::try_new_handle( + handle, + DecimalType::I64, + DecimalDType::new(2, 0), + Validity::NonNullable, + ); + assert!(result.is_err()); + } + // Host-backed storage exposes device slicing without allowing implicit host copies. #[derive(Debug, PartialEq, Eq, Hash)] struct TestDeviceBuffer(ByteBuffer); @@ -173,7 +186,7 @@ mod tests { let array = DecimalArray::try_new_handle( handle, values_type, - DecimalDType::new(3, 1), + DecimalDType::new(76, 1), validity.clone(), )? .into_array(); diff --git a/vortex-array/src/arrays/decimal/tests.rs b/vortex-array/src/arrays/decimal/tests.rs new file mode 100644 index 00000000000..b011c55d816 --- /dev/null +++ b/vortex-array/src/arrays/decimal/tests.rs @@ -0,0 +1,107 @@ +// SPDX-License-Identifier: Apache-2.0 +// SPDX-FileCopyrightText: Copyright the Vortex contributors + +use num_traits::AsPrimitive; +use rstest::rstest; +use vortex_buffer::Buffer; +use vortex_buffer::buffer; +use vortex_error::VortexResult; + +use crate::ArrayParts; +use crate::VortexSessionExecute; +use crate::array_session; +use crate::arrays::Decimal; +use crate::arrays::DecimalArray; +use crate::arrays::decimal::DecimalData; +use crate::assert_arrays_eq; +use crate::buffer::BufferHandle; +use crate::dtype::DType; +use crate::dtype::DecimalDType; +use crate::dtype::DecimalType; +use crate::dtype::Nullability; +use crate::match_each_decimal_value_type; +use crate::validity::Validity; + +#[rstest] +fn construction_bounds_storage_by_precision( + #[values(1, 2, 3, 4, 5, 9, 10, 18, 19, 38, 39, 76)] precision: u8, + #[values( + DecimalType::I8, DecimalType::I16, DecimalType::I32, + DecimalType::I64, DecimalType::I128, DecimalType::I256 + )] + storage: DecimalType, +) -> VortexResult<()> { + let dtype = DecimalDType::new(precision, 0); + let maximum = DecimalType::smallest_decimal_value_type(&dtype); + let bytes = match_each_decimal_value_type!(storage, |D| { + [-9i8, 9] + .into_iter() + .map(|v| v.as_()) + .collect::>() + .into_byte_buffer() + }); + let ptr = bytes.as_ptr(); + let array = DecimalArray::try_new_handle( + BufferHandle::new_host(bytes), + storage, + dtype, + Validity::NonNullable, + )?; + assert_eq!(array.values_type(), storage.min(maximum)); + if storage <= maximum { + assert_eq!(array.buffer_handle().as_host().as_ptr(), ptr); + } + assert_arrays_eq!( + array, + DecimalArray::new(buffer![-9i8, 9], dtype, Validity::NonNullable), + &mut array_session().create_execution_ctx() + ); + Ok(()) +} + +#[test] +fn narrowing_preserves_validity_and_ignores_null_payloads() -> VortexResult<()> { + let dtype = DecimalDType::new(2, 0); + let array = DecimalArray::try_new( + buffer![99i128, i128::MIN, -99], + dtype, + Validity::from_iter([true, false, true]), + )?; + assert_eq!(array.values_type(), DecimalType::I8); + assert_arrays_eq!( + array, + DecimalArray::from_option_iter([Some(99i8), None, Some(-99)], dtype), + &mut array_session().create_execution_ctx() + ); + Ok(()) +} + +#[test] +fn empty_oversized_storage_is_narrowed() { + let data = DecimalData::new(Buffer::::empty(), DecimalDType::new(2, 0)); + assert!(data.is_empty()); + assert_eq!(data.values_type(), DecimalType::I8); +} + +#[cfg(debug_assertions)] +#[test] +#[should_panic(expected = "decimal storage i16 exceeds")] +fn unchecked_construction_asserts_storage_bound() { + // Deliberately violate the width contract to exercise its debug assertion. + let _ = unsafe { DecimalData::new_unchecked(buffer![1i16], DecimalDType::new(2, 0)) }; +} + +#[test] +fn parts_cannot_override_the_precision_bound() { + let data = DecimalData::new(buffer![1i64], DecimalDType::new(18, 0)); + let result = DecimalArray::try_from_parts( + ArrayParts::new( + Decimal, + DType::Decimal(DecimalDType::new(2, 0), Nullability::NonNullable), + 1, + data, + ) + .with_slots(DecimalData::make_slots(&Validity::NonNullable, 1)), + ); + assert!(result.is_err()); +} diff --git a/vortex-array/src/arrays/decimal/vtable/mod.rs b/vortex-array/src/arrays/decimal/vtable/mod.rs index 612ed4b1ca7..ce66d11ab02 100644 --- a/vortex-array/src/arrays/decimal/vtable/mod.rs +++ b/vortex-array/src/arrays/decimal/vtable/mod.rs @@ -43,6 +43,10 @@ use crate::arrays::decimal::compute::rules::RULES; use crate::hash::ArrayEq; use crate::hash::ArrayHash; /// A [`Decimal`]-encoded Vortex array. +/// +/// The physical storage type is no wider than [`DecimalType::smallest_decimal_value_type`] +/// for the precision; smaller storage is allowed when the values fit. See [`DecimalData`] +/// for the full storage contract. pub type DecimalArray = Array; pub(crate) fn initialize(session: &VortexSession) { @@ -124,9 +128,18 @@ impl VTable for Decimal { len: usize, slots: &[Option], ) -> VortexResult<()> { - let DType::Decimal(_, nullability) = dtype else { + let DType::Decimal(decimal_dtype, nullability) = dtype else { vortex_bail!("Expected decimal dtype, got {dtype:?}"); }; + vortex_ensure_eq!( + data.decimal_dtype, + *decimal_dtype, + InvalidArgument: "DecimalArray data dtype does not match outer dtype", + ); + vortex_ensure!( + data.values_type <= DecimalType::smallest_decimal_value_type(decimal_dtype), + InvalidArgument: "DecimalArray storage exceeds the type required by its precision", + ); vortex_ensure_eq!( data.len(), len, @@ -220,7 +233,10 @@ mod tests { use crate::arrays::Decimal; use crate::arrays::DecimalArray; use crate::assert_arrays_eq; + use crate::dtype::DType; use crate::dtype::DecimalDType; + use crate::dtype::DecimalType; + use crate::dtype::Nullability; use crate::serde::SerializeOptions; use crate::serde::SerializedArray; use crate::validity::Validity; @@ -255,6 +271,43 @@ mod tests { assert!(decoded.is::()); } + #[test] + fn deserialize_legacy_oversized_storage() { + let session = array_session(); + let array = DecimalArray::new( + buffer![100i64, -200, 300], + DecimalDType::new(10, 2), + Validity::NonNullable, + ); + let array_ctx = ArrayContext::empty(); + let buffers = array + .into_array() + .serialize(&array_ctx, &session, &SerializeOptions::default()) + .unwrap(); + let mut bytes = ByteBufferMut::empty(); + for buffer in buffers { + bytes.extend_from_slice(buffer.as_ref()); + } + // Older writers allowed i64 storage with precision 3. Decode the same wire storage + // with that logical dtype and ensure it is normalized before reaching kernels. + let dtype = DecimalDType::new(3, 2); + let parts = SerializedArray::try_from(bytes.freeze()).unwrap(); + let decoded = parts + .decode( + &DType::Decimal(dtype, Nullability::NonNullable), + 3, + &ReadContext::new(array_ctx.to_ids()), + &session, + ) + .unwrap(); + assert_eq!(decoded.as_::().values_type(), DecimalType::I16); + assert_arrays_eq!( + decoded, + DecimalArray::new(buffer![100i16, -200, 300], dtype, Validity::NonNullable), + &mut session.create_execution_ctx() + ); + } + #[test] fn test_nullable_decimal_serde_roundtrip() { let session = array_session(); diff --git a/vortex-array/src/arrays/decimal/vtable/operations.rs b/vortex-array/src/arrays/decimal/vtable/operations.rs index aecf6638bd2..8bdcf8255bb 100644 --- a/vortex-array/src/arrays/decimal/vtable/operations.rs +++ b/vortex-array/src/arrays/decimal/vtable/operations.rs @@ -48,7 +48,7 @@ mod tests { fn test_slice() { let array = DecimalArray::new( buffer![100i128, 200i128, 300i128, 4000i128], - DecimalDType::new(3, 2), + DecimalDType::new(4, 2), Validity::NonNullable, ) .into_array(); @@ -57,14 +57,14 @@ mod tests { assert_eq!(sliced.len(), 2); let decimal = sliced.as_::(); - assert_eq!(decimal.buffer::(), buffer![200i128, 300i128]); + assert_eq!(decimal.buffer::(), buffer![200i16, 300i16]); } #[test] fn test_slice_nullable() { let array = DecimalArray::new( buffer![100i128, 200i128, 300i128, 4000i128], - DecimalDType::new(3, 2), + DecimalDType::new(4, 2), Validity::from_iter([false, true, false, true]), ) .into_array(); diff --git a/vortex-arrow/src/convert.rs b/vortex-arrow/src/convert.rs index c19bfc8ba1f..26302897690 100644 --- a/vortex-arrow/src/convert.rs +++ b/vortex-arrow/src/convert.rs @@ -200,7 +200,9 @@ impl_from_arrow_primitive!(Float16Type); impl_from_arrow_primitive!(Float32Type); impl_from_arrow_primitive!(Float64Type); -/// Zero-copy conversion of an Arrow `Decimal32` array into a Vortex decimal array. +/// Converts an Arrow `Decimal32` array into a Vortex decimal array. +/// +/// Zero-copy unless the Arrow storage is wider than the type required by its precision. pub fn from_arrow_decimal32( array: &ArrowPrimitiveArray, nullable: bool, @@ -220,7 +222,9 @@ impl FromArrowArray<&ArrowPrimitiveArray> for ArrayRef { } } -/// Zero-copy conversion of an Arrow `Decimal64` array into a Vortex decimal array. +/// Converts an Arrow `Decimal64` array into a Vortex decimal array. +/// +/// Zero-copy unless the Arrow storage is wider than the type required by its precision. pub fn from_arrow_decimal64( array: &ArrowPrimitiveArray, nullable: bool, @@ -240,7 +244,9 @@ impl FromArrowArray<&ArrowPrimitiveArray> for ArrayRef { } } -/// Zero-copy conversion of an Arrow `Decimal128` array into a Vortex decimal array. +/// Converts an Arrow `Decimal128` array into a Vortex decimal array. +/// +/// Zero-copy unless the Arrow storage is wider than the type required by its precision. pub fn from_arrow_decimal128( array: &ArrowPrimitiveArray, nullable: bool, @@ -260,7 +266,9 @@ impl FromArrowArray<&ArrowPrimitiveArray> for ArrayRef { } } -/// Zero-copy conversion of an Arrow `Decimal256` array into a Vortex decimal array. +/// Converts an Arrow `Decimal256` array into a Vortex decimal array. +/// +/// Zero-copy unless the Arrow storage is wider than the type required by its precision. pub fn from_arrow_decimal256( array: &ArrowPrimitiveArray, nullable: bool, From 51de229ac03ac8095bffb9ae2875725214568419 Mon Sep 17 00:00:00 2001 From: Joe Isaacs Date: Tue, 6 Oct 2026 17:01:41 +0100 Subject: [PATCH 02/11] Fix decimal storage regression test lint and formatting Signed-off-by: Joe Isaacs --- vortex-array/src/arrays/decimal/tests.rs | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/vortex-array/src/arrays/decimal/tests.rs b/vortex-array/src/arrays/decimal/tests.rs index b011c55d816..57e693b8615 100644 --- a/vortex-array/src/arrays/decimal/tests.rs +++ b/vortex-array/src/arrays/decimal/tests.rs @@ -26,8 +26,12 @@ use crate::validity::Validity; fn construction_bounds_storage_by_precision( #[values(1, 2, 3, 4, 5, 9, 10, 18, 19, 38, 39, 76)] precision: u8, #[values( - DecimalType::I8, DecimalType::I16, DecimalType::I32, - DecimalType::I64, DecimalType::I128, DecimalType::I256 + DecimalType::I8, + DecimalType::I16, + DecimalType::I32, + DecimalType::I64, + DecimalType::I128, + DecimalType::I256 )] storage: DecimalType, ) -> VortexResult<()> { @@ -88,7 +92,7 @@ fn empty_oversized_storage_is_narrowed() { #[should_panic(expected = "decimal storage i16 exceeds")] fn unchecked_construction_asserts_storage_bound() { // Deliberately violate the width contract to exercise its debug assertion. - let _ = unsafe { DecimalData::new_unchecked(buffer![1i16], DecimalDType::new(2, 0)) }; + drop(unsafe { DecimalData::new_unchecked(buffer![1i16], DecimalDType::new(2, 0)) }); } #[test] From 46b957a5cf6e1bc3f2544457682ae2a40cf812a8 Mon Sep 17 00:00:00 2001 From: Joe Isaacs Date: Fri, 9 Oct 2026 13:23:35 +0000 Subject: [PATCH 03/11] Merge develop and adapt decimal parts test to ArrayParts::new slots develop now initialises ArrayParts with their slots directly, so the precision-bound regression test passes the slots to the constructor instead of calling the removed with_slots helper. Signed-off-by: Joe Isaacs Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_015zQPnFWRi7CWXHpgBE8CiF --- vortex-array/src/arrays/decimal/tests.rs | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/vortex-array/src/arrays/decimal/tests.rs b/vortex-array/src/arrays/decimal/tests.rs index 57e693b8615..4c355d66f5b 100644 --- a/vortex-array/src/arrays/decimal/tests.rs +++ b/vortex-array/src/arrays/decimal/tests.rs @@ -98,14 +98,12 @@ fn unchecked_construction_asserts_storage_bound() { #[test] fn parts_cannot_override_the_precision_bound() { let data = DecimalData::new(buffer![1i64], DecimalDType::new(18, 0)); - let result = DecimalArray::try_from_parts( - ArrayParts::new( - Decimal, - DType::Decimal(DecimalDType::new(2, 0), Nullability::NonNullable), - 1, - data, - ) - .with_slots(DecimalData::make_slots(&Validity::NonNullable, 1)), - ); + let result = DecimalArray::try_from_parts(ArrayParts::new( + Decimal, + DType::Decimal(DecimalDType::new(2, 0), Nullability::NonNullable), + 1, + data, + DecimalData::make_slots(&Validity::NonNullable, 1), + )); assert!(result.is_err()); } From 1ee71320c66a5a5af1754fc4ccf2a5042773063f Mon Sep 17 00:00:00 2001 From: Joe Isaacs Date: Fri, 9 Oct 2026 13:43:38 +0000 Subject: [PATCH 04/11] Adapt out-of-precision decimal tests to the storage bound Storage is now bounded by precision, so tests that stored values wider than the precision-derived width can no longer be constructed. Move the numeric overflow tests to inputs that overflow within bounded storage, expect i8 storage for a decimal(2, 0) byte-parts split, and drop the row key-width test whose narrowing path is unreachable from a valid array. Signed-off-by: Joe Isaacs Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_015zQPnFWRi7CWXHpgBE8CiF --- .../src/decimal_byte_parts/assemble.rs | 8 ++--- .../src/scalar_fn/fns/binary/numeric/tests.rs | 32 +++++++++++-------- vortex-row/src/tests.rs | 19 ----------- 3 files changed, 22 insertions(+), 37 deletions(-) diff --git a/encodings/decimal-byte-parts/src/decimal_byte_parts/assemble.rs b/encodings/decimal-byte-parts/src/decimal_byte_parts/assemble.rs index 87389120bcf..8792d3ff820 100644 --- a/encodings/decimal-byte-parts/src/decimal_byte_parts/assemble.rs +++ b/encodings/decimal-byte-parts/src/decimal_byte_parts/assemble.rs @@ -368,14 +368,14 @@ mod tests { validity: Validity, ) -> VortexResult<()> { let mut ctx = array_session().create_execution_ctx(); - let decimal = DecimalArray::new(buffer![1i32, 2, 3], DecimalDType::new(2, 0), validity); + let decimal = DecimalArray::new(buffer![1i8, 2, 3], DecimalDType::new(2, 0), validity); let parts = split_decimal(&decimal, &mut ctx)?; assert!(parts.lower_parts.is_empty()); - assert_eq!(parts.msp.dtype().as_ptype(), PType::I32); + assert_eq!(parts.msp.dtype().as_ptype(), PType::I8); let msp = parts.msp.execute::(&mut ctx)?; assert_eq!( - msp.as_slice::().as_ptr(), - decimal.buffer::().as_ptr() + msp.as_slice::().as_ptr(), + decimal.buffer::().as_ptr() ); assert_arrays_eq!(decimal.clone(), round_trip(decimal)?, &mut ctx); Ok(()) diff --git a/vortex-array/src/scalar_fn/fns/binary/numeric/tests.rs b/vortex-array/src/scalar_fn/fns/binary/numeric/tests.rs index 3813c8612b3..5ec6dc9922f 100644 --- a/vortex-array/src/scalar_fn/fns/binary/numeric/tests.rs +++ b/vortex-array/src/scalar_fn/fns/binary/numeric/tests.rs @@ -452,10 +452,12 @@ fn test_decimal_max_precision_overflow_on_valid_lane_errors() { #[test] fn test_decimal_value_outside_working_width_errors() { - let dtype = DecimalDType::new(2, 0); - let value = i256::from_i128(1_000_000); - let lhs = DecimalArray::new(buffer![value], dtype, Validity::NonNullable).into_array(); - let rhs = DecimalArray::new(buffer![value], dtype, Validity::NonNullable).into_array(); + // Storage is bounded by precision, so an out-of-precision value can only overflow the + // working width when the result precision shares the input storage width: decimal(3, 0) and + // its decimal(4, 0) Add result are both i16, and 32_000 + 32_000 overflows it. + let dtype = DecimalDType::new(3, 0); + let lhs = DecimalArray::new(buffer![32_000i16], dtype, Validity::NonNullable).into_array(); + let rhs = DecimalArray::new(buffer![32_000i16], dtype, Validity::NonNullable).into_array(); assert!(decimal_binary(lhs, rhs, Operator::Add).is_err()); } @@ -482,22 +484,24 @@ fn test_decimal_div_negative_result_scale() -> VortexResult<()> { #[test] fn test_decimal_mul_value_outside_precision_errors() { // `DecimalArray::new` does not validate stored values against the declared precision, so Mul - // cannot assume its inputs are in-precision: 500 * 500 is 250_000, well past the 99_999 that - // the decimal(5, 0) result can represent. - let dtype = DecimalDType::new(2, 0); - let value = i256::from_i128(500); - let lhs = DecimalArray::new(buffer![value], dtype, Validity::NonNullable).into_array(); - let rhs = DecimalArray::new(buffer![value], dtype, Validity::NonNullable).into_array(); + // cannot assume its inputs are in-precision: 32_000 fits the i16 storage of decimal(3, 0), + // and 32_000 * 32_000 is 1_024_000_000, well past the 9_999_999 that the decimal(7, 0) result + // can represent while still fitting its i32 working width. + let dtype = DecimalDType::new(3, 0); + let lhs = DecimalArray::new(buffer![32_000i16], dtype, Validity::NonNullable).into_array(); + let rhs = DecimalArray::new(buffer![32_000i16], dtype, Validity::NonNullable).into_array(); assert!(decimal_binary(lhs, rhs, Operator::Mul).is_err()); } #[test] fn test_decimal_mul_value_outside_working_width_errors() { - // 50_000 * 50_000 overflows the i32 working width chosen for a decimal(5, 0) result, which - // an unchecked multiply would wrap in release and panic on in debug. - let dtype = DecimalDType::new(2, 0); - let value = i256::from_i128(50_000); + // Below i256 the working width always holds the product of two in-storage values, so only an + // i256 input can overflow it: 2^228 squared exceeds the i256 working width chosen for the + // decimal(76, 0) result, which an unchecked multiply would wrap in release and panic on in + // debug. + let dtype = DecimalDType::new(39, 0); + let value = i256::from_parts(0, 1 << 100); let lhs = DecimalArray::new(buffer![value], dtype, Validity::NonNullable).into_array(); let rhs = DecimalArray::new(buffer![value], dtype, Validity::NonNullable).into_array(); diff --git a/vortex-row/src/tests.rs b/vortex-row/src/tests.rs index c289fef65a6..1fff3e04044 100644 --- a/vortex-row/src/tests.rs +++ b/vortex-row/src/tests.rs @@ -779,25 +779,6 @@ fn decimal_null_slot_garbage_does_not_error() -> VortexResult<()> { Ok(()) } -/// A valid value too large for the dtype-derived key width must fail loudly instead of -/// silently encoding a corrupt key. -#[test] -fn decimal_value_not_fitting_key_width_errors() { - let mut ctx = array_session().create_execution_ctx(); - let dtype = DecimalDType::new(7, 5); - let field = RowSortField::new(false, true); - - let chunk = DecimalArray::new(buffer![10_000_000_000_000i64], dtype, Validity::NonNullable) - .into_array(); - - let err = convert_columns(&[chunk], &[field], &mut ctx) - .expect_err("a valid value wider than the key width must be rejected"); - assert!( - err.to_string().contains("does not fit"), - "expected a does-not-fit error, got: {err}" - ); -} - #[test] fn decimal256_keys_preserve_order_and_physical_width_independence() -> VortexResult<()> { let mut ctx = array_session().create_execution_ctx(); From 01299fcaf73b2ebd4d4c687346c9ef6870023e79 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 9 Oct 2026 14:13:23 +0000 Subject: [PATCH 05/11] Reject oversized decimal storage instead of narrowing it The checked constructors now fail when storage is wider than the type required by the precision. Importers that receive wider storage, the Arrow decimal conversions and legacy serialized arrays, narrow explicitly through DecimalData::narrow_to_precision before constructing the array. Signed-off-by: Joe Isaacs Signed-off-by: Claude Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_015zQPnFWRi7CWXHpgBE8CiF --- vortex-array/src/arrays/decimal/array.rs | 82 +++++++++++++------ .../src/arrays/decimal/compute/rules.rs | 7 +- vortex-array/src/arrays/decimal/tests.rs | 65 ++++++++++++--- vortex-array/src/arrays/decimal/vtable/mod.rs | 5 +- vortex-arrow/src/convert.rs | 8 +- 5 files changed, 127 insertions(+), 40 deletions(-) diff --git a/vortex-array/src/arrays/decimal/array.rs b/vortex-array/src/arrays/decimal/array.rs index 34fdf299170..1d64ba56843 100644 --- a/vortex-array/src/arrays/decimal/array.rs +++ b/vortex-array/src/arrays/decimal/array.rs @@ -239,8 +239,8 @@ impl DecimalData { /// Constructs a new `DecimalArray` with validation from a [`BufferHandle`]. /// - /// Oversized host buffers are narrowed to the type required by the precision. Device buffers - /// must already satisfy this bound. Non-null values must fit the declared precision; their + /// Storage must be no wider than the type required by the precision; wider storage is + /// rejected rather than narrowed. Non-null values must fit the declared precision; their /// range is not checked. /// /// # Errors @@ -253,32 +253,53 @@ impl DecimalData { ) -> VortexResult { Self::validate(&values, values_type)?; let maximum = DecimalType::smallest_decimal_value_type(&decimal_dtype); - let (values, values_type) = if values_type > maximum { - vortex_ensure!( - values.is_on_host(), - InvalidArgument: "decimal device storage {values_type} exceeds precision storage {maximum}", - ); - let narrowed = match_each_decimal_value_type!(values_type, |In| { - let buffer = Buffer::::from_byte_buffer(values.unwrap_host()); - match_each_decimal_value_type!(maximum, |Out| { - // Non-null values must already fit the declared precision. Null payloads - // are unspecified and may be truncated along with the buffer. - buffer - .iter() - .map(|value| value.as_()) - .collect::>() - .into_byte_buffer() - }) - }); - (BufferHandle::new_host(narrowed), maximum) - } else { - (values, values_type) - }; + vortex_ensure!( + values_type <= maximum, + InvalidArgument: "decimal storage {values_type} exceeds precision storage {maximum} for {decimal_dtype}", + ); // SAFETY: buffer layout is validated and storage is bounded by the precision. Ok(unsafe { Self::new_unchecked_handle(values, values_type, decimal_dtype) }) } + /// Narrows host storage wider than the type required by `decimal_dtype` down to that type. + /// + /// Storage already within the bound is returned unchanged, without copying. Non-null values + /// must already fit the declared precision; null payloads are unspecified and may be + /// truncated along with the buffer. Importers of external storage layouts use this before + /// constructing an array, since the constructors reject oversized storage. + /// + /// # Errors + /// + /// Returns an error if the buffer layout is invalid for `values_type`, or if oversized + /// storage lives on a device, where it cannot be narrowed. + pub fn narrow_to_precision( + values: BufferHandle, + values_type: DecimalType, + decimal_dtype: DecimalDType, + ) -> VortexResult<(BufferHandle, DecimalType)> { + Self::validate(&values, values_type)?; + let maximum = DecimalType::smallest_decimal_value_type(&decimal_dtype); + if values_type <= maximum { + return Ok((values, values_type)); + } + vortex_ensure!( + values.is_on_host(), + InvalidArgument: "decimal device storage {values_type} exceeds precision storage {maximum}", + ); + let narrowed = match_each_decimal_value_type!(values_type, |In| { + let buffer = Buffer::::from_byte_buffer(values.unwrap_host()); + match_each_decimal_value_type!(maximum, |Out| { + buffer + .iter() + .map(|value| value.as_()) + .collect::>() + .into_byte_buffer() + }) + }); + Ok((BufferHandle::new_host(narrowed), maximum)) + } + /// Creates a new [`DecimalArray`] without validation from these components: /// /// * `buffer` is a typed buffer containing the decimal values. @@ -487,6 +508,21 @@ impl Array { Array::try_from_parts(ArrayParts::new(Decimal, dtype, len, data, slots)) } + /// Creates a new [`DecimalArray`] from a host-native buffer, narrowing storage wider than + /// the type required by the precision. + /// + /// See [`DecimalData::narrow_to_precision`] for the narrowing contract. + pub fn try_new_narrowed( + buffer: Buffer, + decimal_dtype: DecimalDType, + validity: Validity, + ) -> VortexResult { + let values = BufferHandle::new_host(buffer.into_byte_buffer()); + let (values, values_type) = + DecimalData::narrow_to_precision(values, T::DECIMAL_TYPE, decimal_dtype)?; + Self::try_new_handle(values, values_type, decimal_dtype, validity) + } + /// Creates a new [`DecimalArray`] from an iterator of values. #[expect( clippy::same_name_method, diff --git a/vortex-array/src/arrays/decimal/compute/rules.rs b/vortex-array/src/arrays/decimal/compute/rules.rs index 7979e008605..85cd31449b8 100644 --- a/vortex-array/src/arrays/decimal/compute/rules.rs +++ b/vortex-array/src/arrays/decimal/compute/rules.rs @@ -104,6 +104,7 @@ mod tests { use crate::array_session; use crate::arrays::Decimal; use crate::arrays::DecimalArray; + use crate::arrays::decimal::DecimalData; use crate::assert_arrays_eq; use crate::buffer::BufferHandle; use crate::buffer::DeviceBuffer; @@ -117,12 +118,16 @@ mod tests { let bytes = Buffer::from_iter([1i64]).into_byte_buffer(); let handle = BufferHandle::new_device(Arc::new(TestDeviceBuffer(bytes))); let result = DecimalArray::try_new_handle( - handle, + handle.clone(), DecimalType::I64, DecimalDType::new(2, 0), Validity::NonNullable, ); assert!(result.is_err()); + // Device storage cannot be narrowed on the host either. + let narrowed = + DecimalData::narrow_to_precision(handle, DecimalType::I64, DecimalDType::new(2, 0)); + assert!(narrowed.is_err()); } // Host-backed storage exposes device slicing without allowing implicit host copies. diff --git a/vortex-array/src/arrays/decimal/tests.rs b/vortex-array/src/arrays/decimal/tests.rs index 4c355d66f5b..b5d4a4d5301 100644 --- a/vortex-array/src/arrays/decimal/tests.rs +++ b/vortex-array/src/arrays/decimal/tests.rs @@ -23,7 +23,7 @@ use crate::match_each_decimal_value_type; use crate::validity::Validity; #[rstest] -fn construction_bounds_storage_by_precision( +fn construction_rejects_storage_wider_than_precision( #[values(1, 2, 3, 4, 5, 9, 10, 18, 19, 38, 39, 76)] precision: u8, #[values( DecimalType::I8, @@ -45,16 +45,55 @@ fn construction_bounds_storage_by_precision( .into_byte_buffer() }); let ptr = bytes.as_ptr(); - let array = DecimalArray::try_new_handle( + let result = DecimalArray::try_new_handle( BufferHandle::new_host(bytes), storage, dtype, Validity::NonNullable, - )?; - assert_eq!(array.values_type(), storage.min(maximum)); - if storage <= maximum { - assert_eq!(array.buffer_handle().as_host().as_ptr(), ptr); + ); + if storage > maximum { + assert!(result.is_err()); + return Ok(()); } + let array = result?; + assert_eq!(array.values_type(), storage); + assert_eq!(array.buffer_handle().as_host().as_ptr(), ptr); + assert_arrays_eq!( + array, + DecimalArray::new(buffer![-9i8, 9], dtype, Validity::NonNullable), + &mut array_session().create_execution_ctx() + ); + Ok(()) +} + +#[rstest] +fn narrowing_bounds_storage_by_precision( + #[values(1, 2, 3, 4, 5, 9, 10, 18, 19, 38, 39, 76)] precision: u8, + #[values( + DecimalType::I8, + DecimalType::I16, + DecimalType::I32, + DecimalType::I64, + DecimalType::I128, + DecimalType::I256 + )] + storage: DecimalType, +) -> VortexResult<()> { + let dtype = DecimalDType::new(precision, 0); + let maximum = DecimalType::smallest_decimal_value_type(&dtype); + let array = match_each_decimal_value_type!(storage, |D| { + let buffer = [-9i8, 9] + .into_iter() + .map(|v| v.as_()) + .collect::>(); + let ptr = buffer.as_ptr(); + let array = DecimalArray::try_new_narrowed(buffer, dtype, Validity::NonNullable)?; + if storage <= maximum { + assert_eq!(array.buffer::().as_ptr(), ptr); + } + array + }); + assert_eq!(array.values_type(), storage.min(maximum)); assert_arrays_eq!( array, DecimalArray::new(buffer![-9i8, 9], dtype, Validity::NonNullable), @@ -66,7 +105,7 @@ fn construction_bounds_storage_by_precision( #[test] fn narrowing_preserves_validity_and_ignores_null_payloads() -> VortexResult<()> { let dtype = DecimalDType::new(2, 0); - let array = DecimalArray::try_new( + let array = DecimalArray::try_new_narrowed( buffer![99i128, i128::MIN, -99], dtype, Validity::from_iter([true, false, true]), @@ -81,10 +120,14 @@ fn narrowing_preserves_validity_and_ignores_null_payloads() -> VortexResult<()> } #[test] -fn empty_oversized_storage_is_narrowed() { - let data = DecimalData::new(Buffer::::empty(), DecimalDType::new(2, 0)); - assert!(data.is_empty()); - assert_eq!(data.values_type(), DecimalType::I8); +fn empty_oversized_storage_is_rejected_unless_narrowed() -> VortexResult<()> { + let dtype = DecimalDType::new(2, 0); + assert!(DecimalData::try_new(Buffer::::empty(), dtype).is_err()); + let array = + DecimalArray::try_new_narrowed(Buffer::::empty(), dtype, Validity::NonNullable)?; + assert!(array.is_empty()); + assert_eq!(array.values_type(), DecimalType::I8); + Ok(()) } #[cfg(debug_assertions)] diff --git a/vortex-array/src/arrays/decimal/vtable/mod.rs b/vortex-array/src/arrays/decimal/vtable/mod.rs index a168e9ecc93..ddf9d788cfe 100644 --- a/vortex-array/src/arrays/decimal/vtable/mod.rs +++ b/vortex-array/src/arrays/decimal/vtable/mod.rs @@ -187,7 +187,10 @@ impl VTable for Decimal { "DecimalArray buffer not aligned for values type {:?}", D::DECIMAL_TYPE ); - DecimalData::try_new_handle(values, metadata.values_type(), *decimal_dtype) + // Older writers allowed storage wider than the precision requires. + let (values, values_type) = + DecimalData::narrow_to_precision(values, metadata.values_type(), *decimal_dtype)?; + DecimalData::try_new_handle(values, values_type, *decimal_dtype) })?; Ok(ArrayParts::new( self.clone(), diff --git a/vortex-arrow/src/convert.rs b/vortex-arrow/src/convert.rs index ffbff439bf1..651bacdb0c7 100644 --- a/vortex-arrow/src/convert.rs +++ b/vortex-arrow/src/convert.rs @@ -211,7 +211,7 @@ pub fn from_arrow_decimal32( let decimal_type = DecimalDType::new(array.precision(), array.scale()); let buffer = Buffer::from_arrow_scalar_buffer(array.values().clone()); let validity = nulls(array.nulls(), nullable)?; - Ok(DecimalArray::new(buffer, decimal_type, validity).into_array()) + Ok(DecimalArray::try_new_narrowed(buffer, decimal_type, validity)?.into_array()) } impl FromArrowArray<&ArrowPrimitiveArray> for ArrayRef { @@ -233,7 +233,7 @@ pub fn from_arrow_decimal64( let decimal_type = DecimalDType::new(array.precision(), array.scale()); let buffer = Buffer::from_arrow_scalar_buffer(array.values().clone()); let validity = nulls(array.nulls(), nullable)?; - Ok(DecimalArray::new(buffer, decimal_type, validity).into_array()) + Ok(DecimalArray::try_new_narrowed(buffer, decimal_type, validity)?.into_array()) } impl FromArrowArray<&ArrowPrimitiveArray> for ArrayRef { @@ -255,7 +255,7 @@ pub fn from_arrow_decimal128( let decimal_type = DecimalDType::new(array.precision(), array.scale()); let buffer = Buffer::from_arrow_scalar_buffer(array.values().clone()); let validity = nulls(array.nulls(), nullable)?; - Ok(DecimalArray::new(buffer, decimal_type, validity).into_array()) + Ok(DecimalArray::try_new_narrowed(buffer, decimal_type, validity)?.into_array()) } impl FromArrowArray<&ArrowPrimitiveArray> for ArrayRef { @@ -281,7 +281,7 @@ pub fn from_arrow_decimal256( // of either type. let buffer = unsafe { std::mem::transmute::, Buffer>(buffer) }; let validity = nulls(array.nulls(), nullable)?; - Ok(DecimalArray::new(buffer, decimal_type, validity).into_array()) + Ok(DecimalArray::try_new_narrowed(buffer, decimal_type, validity)?.into_array()) } impl FromArrowArray<&ArrowPrimitiveArray> for ArrayRef { From 93389358baa2a9278f910210c19716397d8422d3 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 9 Oct 2026 14:28:16 +0000 Subject: [PATCH 06/11] Adapt tests to decimal storage rejection Tests that built decimals with storage wider than their precision allows now use the bounded width, or a precision that admits the width they exercise. Signed-off-by: Joe Isaacs Signed-off-by: Claude Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_015zQPnFWRi7CWXHpgBE8CiF --- .../src/aggregate_fn/fns/min_max/mod.rs | 2 +- .../src/aggregate_fn/fns/sum/decimal.rs | 10 ++-- .../fns/uncompressed_size_in_bytes/mod.rs | 2 +- .../src/arrays/decimal/compute/cast.rs | 20 ++++++-- .../src/arrays/decimal/compute/mod.rs | 12 ++--- vortex-array/src/arrays/decimal/vtable/mod.rs | 2 +- .../src/arrays/decimal/vtable/operations.rs | 6 +-- vortex-array/src/arrays/validation_tests.rs | 2 +- vortex-array/src/builders/decimal.rs | 4 +- vortex-array/src/scalar_fn/fns/between/mod.rs | 2 +- .../src/scalar_fn/fns/binary/compare/tests.rs | 2 +- .../src/scalar_fn/fns/binary/numeric/tests.rs | 21 ++++---- vortex-arrow/src/executor/decimal.rs | 48 +++++++++++++------ 13 files changed, 81 insertions(+), 52 deletions(-) diff --git a/vortex-array/src/aggregate_fn/fns/min_max/mod.rs b/vortex-array/src/aggregate_fn/fns/min_max/mod.rs index 46ca2acb3b8..e39446854b1 100644 --- a/vortex-array/src/aggregate_fn/fns/min_max/mod.rs +++ b/vortex-array/src/aggregate_fn/fns/min_max/mod.rs @@ -934,7 +934,7 @@ mod tests { #[test] fn test_decimal() -> VortexResult<()> { let decimal = DecimalArray::new( - buffer![100i32, 2000i32, 200i32], + buffer![100i16, 2000i16, 200i16], DecimalDType::new(4, 2), Validity::from_iter([true, false, true]), ); diff --git a/vortex-array/src/aggregate_fn/fns/sum/decimal.rs b/vortex-array/src/aggregate_fn/fns/sum/decimal.rs index e30bd1f1dfc..eebcb3d2161 100644 --- a/vortex-array/src/aggregate_fn/fns/sum/decimal.rs +++ b/vortex-array/src/aggregate_fn/fns/sum/decimal.rs @@ -147,7 +147,7 @@ mod tests { #[test] fn sum_decimal_basic() -> VortexResult<()> { let decimal = DecimalArray::new( - buffer![100i32, 200i32, 300i32], + buffer![100i16, 200i16, 300i16], DecimalDType::new(4, 2), Validity::AllValid, ); @@ -169,7 +169,7 @@ mod tests { #[test] fn sum_decimal_with_nulls() -> VortexResult<()> { let decimal = DecimalArray::new( - buffer![100i32, 200i32, 300i32, 400i32], + buffer![100i16, 200i16, 300i16, 400i16], DecimalDType::new(4, 2), Validity::from_iter([true, false, true, true]), ); @@ -191,7 +191,7 @@ mod tests { #[test] fn sum_decimal_negative_values() -> VortexResult<()> { let decimal = DecimalArray::new( - buffer![100i32, -200i32, 300i32, -50i32], + buffer![100i16, -200i16, 300i16, -50i16], DecimalDType::new(4, 2), Validity::AllValid, ); @@ -283,7 +283,7 @@ mod tests { #[test] fn sum_decimal_single_value() -> VortexResult<()> { let decimal = - DecimalArray::new(buffer![42i32], DecimalDType::new(3, 1), Validity::AllValid); + DecimalArray::new(buffer![42i16], DecimalDType::new(3, 1), Validity::AllValid); let result = sum( &decimal.into_array(), @@ -302,7 +302,7 @@ mod tests { #[test] fn sum_decimal_all_nulls_except_one() -> VortexResult<()> { let decimal = DecimalArray::new( - buffer![100i32, 200i32, 300i32, 400i32], + buffer![100i16, 200i16, 300i16, 400i16], DecimalDType::new(4, 2), Validity::from_iter([false, false, true, false]), ); 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 bb6b46567e4..9225e7730e9 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 @@ -515,7 +515,7 @@ mod tests { #[test] fn decimal_matches_materialized_size() -> VortexResult<()> { let array = DecimalArray::new( - buffer![12345i64, -123i64, 0i64], + buffer![12345i32, -123i32, 0i32], DecimalDType::new(5, 2), Validity::NonNullable, ) diff --git a/vortex-array/src/arrays/decimal/compute/cast.rs b/vortex-array/src/arrays/decimal/compute/cast.rs index bdabd69c48b..dcf87b7d9df 100644 --- a/vortex-array/src/arrays/decimal/compute/cast.rs +++ b/vortex-array/src/arrays/decimal/compute/cast.rs @@ -585,10 +585,20 @@ mod tests { target: PType, ) -> VortexResult<()> { let mut ctx = array_session().create_execution_ctx(); - let decimal_dtype = DecimalDType::new(3, 1); + // The widest precision each storage type may back keeps every case within the storage + // bound while exercising that width. + let precision = match storage { + DecimalType::I8 => 2, + DecimalType::I16 => 4, + DecimalType::I32 => 9, + DecimalType::I64 => 18, + DecimalType::I128 => 38, + DecimalType::I256 => 76, + }; + let decimal_dtype = DecimalDType::new(precision, 1); let array = match_each_decimal_value_type!(storage, |F| { DecimalArray::from_option_iter( - [Some(19i8), None, Some(123)].map(|value| value.and_then(::from)), + [Some(19i8), None, Some(99)].map(|value| value.and_then(::from)), decimal_dtype, ) }); @@ -600,10 +610,10 @@ mod tests { match_each_integer_ptype!(target, |T| { assert_arrays_eq!( casted, - PrimitiveArray::from_option_iter([Some(1 as T), None, Some(12 as T)]), + PrimitiveArray::from_option_iter([Some(1 as T), None, Some(9 as T)]), &mut ctx ); - for (value, expected) in [(19i8, 1 as T), (123, 12 as T)] { + for (value, expected) in [(19i8, 1 as T), (99, 9 as T)] { let scalar = Scalar::decimal(value.into(), decimal_dtype, Nullability::Nullable); assert_eq!( scalar.cast(&dtype)?, @@ -729,7 +739,7 @@ mod tests { } let huge = DecimalArray::new( buffer![i256::ONE], - DecimalDType::new(1, -128), + DecimalDType::new(39, -128), Validity::NonNullable, ); assert!( diff --git a/vortex-array/src/arrays/decimal/compute/mod.rs b/vortex-array/src/arrays/decimal/compute/mod.rs index a6e1cfb2848..981f499ab28 100644 --- a/vortex-array/src/arrays/decimal/compute/mod.rs +++ b/vortex-array/src/arrays/decimal/compute/mod.rs @@ -37,12 +37,12 @@ mod tests { ))] // Additional test cases #[case::decimal_small_precision(DecimalArray::new( - buffer![10i128, 20i128, 30i128], + buffer![10i32, 20i32, 30i32], DecimalDType::new(5, 1), Validity::NonNullable, ))] #[case::decimal_single(DecimalArray::new( - buffer![42i128], + buffer![42i64], DecimalDType::new(10, 0), Validity::NonNullable, ))] @@ -52,7 +52,7 @@ mod tests { Validity::NonNullable, ))] #[case::decimal_negative(DecimalArray::new( - buffer![-100i128, -200i128, 300i128, -400i128], + buffer![-100i64, -200i64, 300i64, -400i64], DecimalDType::new(10, 2), Validity::NonNullable, ))] @@ -75,7 +75,7 @@ mod tests { Validity::from_iter([true, false, true, true, false]), ))] #[case::decimal_small_precision(DecimalArray::new( - buffer![10i128, 20i128, 30i128], + buffer![10i32, 20i32, 30i32], DecimalDType::new(5, 1), Validity::NonNullable, ))] @@ -93,7 +93,7 @@ mod tests { Validity::NonNullable, ))] #[case::decimal_single(DecimalArray::new( - buffer![42i128], + buffer![42i64], DecimalDType::new(10, 0), Validity::NonNullable, ))] @@ -103,7 +103,7 @@ mod tests { Validity::NonNullable, ))] #[case::decimal_negative(DecimalArray::new( - buffer![-100i128, -200i128, 300i128, -400i128], + buffer![-100i64, -200i64, 300i64, -400i64], DecimalDType::new(10, 2), Validity::NonNullable, ))] diff --git a/vortex-array/src/arrays/decimal/vtable/mod.rs b/vortex-array/src/arrays/decimal/vtable/mod.rs index ddf9d788cfe..518bba9a1f5 100644 --- a/vortex-array/src/arrays/decimal/vtable/mod.rs +++ b/vortex-array/src/arrays/decimal/vtable/mod.rs @@ -257,7 +257,7 @@ mod tests { fn test_array_serde() { let session = array_session(); let array = DecimalArray::new( - buffer![100i128, 200i128, 300i128, 400i128, 500i128], + buffer![100i64, 200i64, 300i64, 400i64, 500i64], DecimalDType::new(10, 2), Validity::NonNullable, ); diff --git a/vortex-array/src/arrays/decimal/vtable/operations.rs b/vortex-array/src/arrays/decimal/vtable/operations.rs index 8bdcf8255bb..133e41770fa 100644 --- a/vortex-array/src/arrays/decimal/vtable/operations.rs +++ b/vortex-array/src/arrays/decimal/vtable/operations.rs @@ -47,7 +47,7 @@ mod tests { #[test] fn test_slice() { let array = DecimalArray::new( - buffer![100i128, 200i128, 300i128, 4000i128], + buffer![100i16, 200i16, 300i16, 4000i16], DecimalDType::new(4, 2), Validity::NonNullable, ) @@ -63,7 +63,7 @@ mod tests { #[test] fn test_slice_nullable() { let array = DecimalArray::new( - buffer![100i128, 200i128, 300i128, 4000i128], + buffer![100i16, 200i16, 300i16, 4000i16], DecimalDType::new(4, 2), Validity::from_iter([false, true, false, true]), ) @@ -76,7 +76,7 @@ mod tests { #[test] fn test_scalar_at() { let array = DecimalArray::new( - buffer![100i128], + buffer![100i16], DecimalDType::new(3, 2), Validity::NonNullable, ); diff --git a/vortex-array/src/arrays/validation_tests.rs b/vortex-array/src/arrays/validation_tests.rs index 48906f5036a..392f4fc3eb9 100644 --- a/vortex-array/src/arrays/validation_tests.rs +++ b/vortex-array/src/arrays/validation_tests.rs @@ -55,7 +55,7 @@ mod tests { #[test] fn test_decimal_array_validation_success() { // Valid case: buffer and validity have matching lengths. - let buffer = Buffer::from_iter([100i128, 200, 300]); + let buffer = Buffer::from_iter([100i64, 200, 300]); let decimal_dtype = crate::dtype::DecimalDType::new(10, 2); let result = DecimalArray::try_new(buffer, decimal_dtype, Validity::NonNullable); assert!(result.is_ok()); diff --git a/vortex-array/src/builders/decimal.rs b/vortex-array/src/builders/decimal.rs index fbedf2cee65..dd594ac5eb0 100644 --- a/vortex-array/src/builders/decimal.rs +++ b/vortex-array/src/builders/decimal.rs @@ -341,7 +341,7 @@ mod tests { let values = 42i8; let mut i8s = DecimalBuilder::new_in::( - DecimalDType::new(2, 1), + DecimalDType::new(20, 1), false.into(), BufferAllocatorRef::static_ref(), ); @@ -351,7 +351,7 @@ mod tests { let i8s = i8s.finish(); let mut i128s = DecimalBuilder::new_in::( - DecimalDType::new(2, 1), + DecimalDType::new(20, 1), false.into(), BufferAllocatorRef::static_ref(), ); diff --git a/vortex-array/src/scalar_fn/fns/between/mod.rs b/vortex-array/src/scalar_fn/fns/between/mod.rs index adc269b40fc..748d34da856 100644 --- a/vortex-array/src/scalar_fn/fns/between/mod.rs +++ b/vortex-array/src/scalar_fn/fns/between/mod.rs @@ -621,7 +621,7 @@ mod tests { #[test] fn test_between_decimal() { let ctx = &mut SESSION.create_execution_ctx(); - let values = buffer![100i128, 200i128, 300i128, 400i128]; + let values = buffer![100i16, 200i16, 300i16, 400i16]; let decimal_type = DecimalDType::new(3, 2); let array = DecimalArray::new(values, decimal_type, Validity::NonNullable).into_array(); diff --git a/vortex-array/src/scalar_fn/fns/binary/compare/tests.rs b/vortex-array/src/scalar_fn/fns/binary/compare/tests.rs index 4688d323c90..08939de9439 100644 --- a/vortex-array/src/scalar_fn/fns/binary/compare/tests.rs +++ b/vortex-array/src/scalar_fn/fns/binary/compare/tests.rs @@ -555,7 +555,7 @@ fn decimal_compare_mixed_storage_widths() { let mut ctx = array_session().create_execution_ctx(); let dtype = DecimalDType::new(10, 2); let lhs = DecimalArray::from_iter::([100, 250, 300], dtype).into_array(); - let rhs = DecimalArray::from_iter::([200, 250, 100], dtype).into_array(); + let rhs = DecimalArray::from_iter::([200, 250, 100], dtype).into_array(); let result = execute_compare_test(lhs, rhs, Operator::Lte); assert_arrays_eq!(result, BoolArray::from_iter([true, true, false]), &mut ctx); diff --git a/vortex-array/src/scalar_fn/fns/binary/numeric/tests.rs b/vortex-array/src/scalar_fn/fns/binary/numeric/tests.rs index 5ec6dc9922f..ee606a725fd 100644 --- a/vortex-array/src/scalar_fn/fns/binary/numeric/tests.rs +++ b/vortex-array/src/scalar_fn/fns/binary/numeric/tests.rs @@ -406,7 +406,7 @@ fn test_decimal_mixed_storage_widths() -> VortexResult<()> { let mut ctx = array_session().create_execution_ctx(); let dtype = DecimalDType::new(10, 2); let lhs = DecimalArray::from_iter::([100, 250], dtype).into_array(); - let rhs = DecimalArray::from_iter::([200, 250], dtype).into_array(); + let rhs = DecimalArray::from_iter::([200, 250], dtype).into_array(); let result = decimal_binary(lhs, rhs, Operator::Add)?; assert_arrays_eq!( @@ -511,15 +511,17 @@ fn test_decimal_mul_value_outside_working_width_errors() { #[test] fn test_decimal_value_outside_working_width_on_null_lane_ignored() -> VortexResult<()> { let mut ctx = array_session().create_execution_ctx(); - let dtype = DecimalDType::new(2, 0); + // The null lane holds a payload whose sum would overflow the i16 working width shared by + // decimal(3, 0) and its decimal(4, 0) result; invalid lanes never error. + let dtype = DecimalDType::new(3, 0); let result_dtype = result_decimal_dtype(dtype, NumericOperator::Add)?; let lhs = DecimalArray::new( - buffer![i256::from_i128(1_000_000), i256::from_i128(1)], + buffer![i16::MAX, 1], dtype, Validity::from_iter([false, true]), ) .into_array(); - let rhs = decimal_constant(i256::from_i128(1), dtype, 2); + let rhs = decimal_constant(1i16, dtype, 2); let result = decimal_binary(lhs, rhs, Operator::Add)?; assert_arrays_eq!( @@ -791,7 +793,7 @@ fn test_decimal_null_constant_yields_all_null( assert!(matches!(&result, Columnar::Constant(_))); assert_arrays_eq!( result.into_array(), - DecimalArray::from_option_iter::([None, None], result_decimal_dtype(dtype, op)?,), + DecimalArray::from_option_iter::([None, None], result_decimal_dtype(dtype, op)?,), &mut ctx ); Ok(()) @@ -831,7 +833,7 @@ fn test_decimal_empty(#[case] op: NumericOperator) -> VortexResult<()> { let result = decimal_binary(empty.clone(), empty, op.into())?; assert_arrays_eq!( result, - DecimalArray::from_iter::([], result_decimal_dtype(dtype, op)?,), + DecimalArray::from_iter::([], result_decimal_dtype(dtype, op)?,), &mut ctx ); Ok(()) @@ -844,7 +846,7 @@ fn test_decimal_empty(#[case] op: NumericOperator) -> VortexResult<()> { #[case::div(NumericOperator::Div, 3_000_000)] fn test_decimal_constant_constant_folds( #[case] op: NumericOperator, - #[case] expected: i128, + #[case] expected: i64, ) -> VortexResult<()> { let mut ctx = array_session().create_execution_ctx(); let dtype = DecimalDType::new(10, 2); @@ -854,10 +856,7 @@ fn test_decimal_constant_constant_folds( let result = decimal_binary(lhs, rhs, op.into())?; assert_arrays_eq!( result, - DecimalArray::from_iter::( - [i256::from_i128(expected); 3], - result_decimal_dtype(dtype, op)?, - ), + DecimalArray::from_iter::([expected; 3], result_decimal_dtype(dtype, op)?), &mut ctx ); Ok(()) diff --git a/vortex-arrow/src/executor/decimal.rs b/vortex-arrow/src/executor/decimal.rs index 41ffa93630f..7f0fa1549c0 100644 --- a/vortex-arrow/src/executor/decimal.rs +++ b/vortex-arrow/src/executor/decimal.rs @@ -234,6 +234,7 @@ mod tests { use vortex_array::builders::ArrayBuilder; use vortex_array::builders::DecimalBuilder; use vortex_array::dtype::DecimalDType; + use vortex_array::dtype::DecimalType; use vortex_array::dtype::NativeDecimalType; use vortex_array::validity::Validity; use vortex_buffer::buffer; @@ -242,6 +243,19 @@ mod tests { use crate::ArrowArrayExecutor; use crate::executor::decimal::DecimalArray; + /// The widest precision each storage type may back, so every case stays within the storage + /// bound while exercising that width. + fn precision_for() -> u8 { + match T::DECIMAL_TYPE { + DecimalType::I8 => 2, + DecimalType::I16 => 4, + DecimalType::I32 => 9, + DecimalType::I64 => 18, + DecimalType::I128 => 38, + DecimalType::I256 => 76, + } + } + #[test] fn decimal_to_arrow() -> VortexResult<()> { let mut ctx = array_session().create_execution_ctx(); @@ -269,13 +283,12 @@ mod tests { #[case(0i32)] #[case(0i64)] #[case(0i128)] - #[case(vortex_array::dtype::i256::ZERO)] fn test_to_arrow_decimal128( #[case] _decimal_type: T, ) -> VortexResult<()> { let mut ctx = array_session().create_execution_ctx(); let mut decimal = DecimalBuilder::new_in::( - DecimalDType::new(2, 1), + DecimalDType::new(precision_for::(), 1), false.into(), vortex_buffer::BufferAllocatorRef::static_ref(), ); @@ -284,7 +297,10 @@ mod tests { decimal.append_value(12); let decimal = decimal.finish(); - let arrow_array = decimal.execute_arrow(Some(&DataType::Decimal128(2, 1)), &mut ctx)?; + let arrow_array = decimal.execute_arrow( + Some(&DataType::Decimal128(precision_for::(), 1)), + &mut ctx, + )?; let arrow_decimal = arrow_array .as_any() .downcast_ref::() @@ -299,15 +315,12 @@ mod tests { #[case(0i8)] #[case(0i16)] #[case(0i32)] - #[case(0i64)] - #[case(0i128)] - #[case(vortex_array::dtype::i256::ZERO)] fn test_to_arrow_decimal32(#[case] _decimal_type: T) -> VortexResult<()> { use arrow_array::Decimal32Array; let mut ctx = array_session().create_execution_ctx(); let mut decimal = DecimalBuilder::new_in::( - DecimalDType::new(2, 1), + DecimalDType::new(precision_for::(), 1), false.into(), vortex_buffer::BufferAllocatorRef::static_ref(), ); @@ -316,7 +329,10 @@ mod tests { decimal.append_value(12); let decimal = decimal.finish(); - let arrow_array = decimal.execute_arrow(Some(&DataType::Decimal32(2, 1)), &mut ctx)?; + let arrow_array = decimal.execute_arrow( + Some(&DataType::Decimal32(precision_for::(), 1)), + &mut ctx, + )?; let arrow_decimal = arrow_array .as_any() .downcast_ref::() @@ -332,14 +348,12 @@ mod tests { #[case(0i16)] #[case(0i32)] #[case(0i64)] - #[case(0i128)] - #[case(vortex_array::dtype::i256::ZERO)] fn test_to_arrow_decimal64(#[case] _decimal_type: T) -> VortexResult<()> { use arrow_array::Decimal64Array; let mut ctx = array_session().create_execution_ctx(); let mut decimal = DecimalBuilder::new_in::( - DecimalDType::new(2, 1), + DecimalDType::new(precision_for::(), 1), false.into(), vortex_buffer::BufferAllocatorRef::static_ref(), ); @@ -348,7 +362,10 @@ mod tests { decimal.append_value(12); let decimal = decimal.finish(); - let arrow_array = decimal.execute_arrow(Some(&DataType::Decimal64(2, 1)), &mut ctx)?; + let arrow_array = decimal.execute_arrow( + Some(&DataType::Decimal64(precision_for::(), 1)), + &mut ctx, + )?; let arrow_decimal = arrow_array .as_any() .downcast_ref::() @@ -371,7 +388,7 @@ mod tests { ) -> VortexResult<()> { let mut ctx = array_session().create_execution_ctx(); let mut decimal = DecimalBuilder::new_in::( - DecimalDType::new(2, 1), + DecimalDType::new(precision_for::(), 1), false.into(), vortex_buffer::BufferAllocatorRef::static_ref(), ); @@ -380,7 +397,10 @@ mod tests { decimal.append_value(12); let decimal = decimal.finish(); - let arrow_array = decimal.execute_arrow(Some(&DataType::Decimal256(2, 1)), &mut ctx)?; + let arrow_array = decimal.execute_arrow( + Some(&DataType::Decimal256(precision_for::(), 1)), + &mut ctx, + )?; let arrow_decimal = arrow_array .as_any() .downcast_ref::() From 051d5fd2b6d265c7cf898ffed52726d3dafe8e57 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 9 Oct 2026 14:36:06 +0000 Subject: [PATCH 07/11] Narrow legacy byte-parts decimals and validate GPU storage Decimal byte parts written before storage was bounded by precision may be wider than it allows; host assembly narrows them explicitly while the GPU kernel, which cannot narrow device buffers, now validates instead of constructing unchecked. Signed-off-by: Joe Isaacs Signed-off-by: Claude Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_015zQPnFWRi7CWXHpgBE8CiF --- .../src/decimal_byte_parts/assemble.rs | 15 ++++++++------- vortex-cuda/src/arrow/canonical.rs | 14 -------------- .../src/kernel/encodings/decimal_byte_parts.rs | 13 ++++++++----- 3 files changed, 16 insertions(+), 26 deletions(-) diff --git a/encodings/decimal-byte-parts/src/decimal_byte_parts/assemble.rs b/encodings/decimal-byte-parts/src/decimal_byte_parts/assemble.rs index 8792d3ff820..812cf82c7e5 100644 --- a/encodings/decimal-byte-parts/src/decimal_byte_parts/assemble.rs +++ b/encodings/decimal-byte-parts/src/decimal_byte_parts/assemble.rs @@ -87,8 +87,9 @@ fn assemble_narrow_decimal( ) -> VortexResult { // TODO(mk): Broadcast a constant MSP directly instead of materializing its buffer. let msp = msp.clone().execute::(exec_ctx)?; + // Parts written before storage was bounded by precision may be wider than it allows. Ok(match_each_signed_integer_ptype!(msp.ptype(), |P| { - DecimalArray::new(msp.to_buffer::

(), decimal_dtype, validity).into_array() + DecimalArray::try_new_narrowed(msp.to_buffer::

(), decimal_dtype, validity)?.into_array() })) } @@ -118,16 +119,16 @@ fn assemble_wide_decimal_from_arrays( Ok(match_each_signed_integer_ptype!(msp.ptype(), |Msp| { let msp = msp.as_slice::(); match lower.as_slice() { - [first] => DecimalArray::new( + [first] => DecimalArray::try_new_narrowed( assemble_wide_decimal::( msp, first.as_slice::().iter().map(|&word| [word]), ), decimal_dtype, validity, - ) + )? .into_array(), - [first, second] => DecimalArray::new( + [first, second] => DecimalArray::try_new_narrowed( assemble_wide_decimal::( msp, first @@ -138,9 +139,9 @@ fn assemble_wide_decimal_from_arrays( ), decimal_dtype, validity, - ) + )? .into_array(), - [first, second, third] => DecimalArray::new( + [first, second, third] => DecimalArray::try_new_narrowed( assemble_wide_decimal::( msp, first @@ -152,7 +153,7 @@ fn assemble_wide_decimal_from_arrays( ), decimal_dtype, validity, - ) + )? .into_array(), _ => vortex_bail!("expected between one and {MAX_LOWER_PARTS} lower parts"), } diff --git a/vortex-cuda/src/arrow/canonical.rs b/vortex-cuda/src/arrow/canonical.rs index fb06f4c48ab..efae985761d 100644 --- a/vortex-cuda/src/arrow/canonical.rs +++ b/vortex-cuda/src/arrow/canonical.rs @@ -2269,20 +2269,6 @@ mod tests { Ok(()) } - #[crate::test] - async fn test_export_decimal_narrowing_from_arrow_import() -> VortexResult<()> { - let mut ctx = CudaSession::create_execution_ctx(&crate::cuda_session()) - .vortex_expect("failed to create execution context"); - let array = DecimalArray::from_iter([0i128, 1, -2], DecimalDType::new(10, 2)).into_array(); - - let err = array - .export_device_array_with_schema(&mut ctx) - .await - .unwrap_err(); - assert!(err.to_string().contains("narrowing would require")); - Ok(()) - } - #[rstest] #[case::i64( DecimalArray::from_iter([i64::MIN, -1i64, 1, i64::MAX], DecimalDType::new(39, 0)).into_array(), diff --git a/vortex-cuda/src/kernel/encodings/decimal_byte_parts.rs b/vortex-cuda/src/kernel/encodings/decimal_byte_parts.rs index a54df06fb4c..86a2065ef73 100644 --- a/vortex-cuda/src/kernel/encodings/decimal_byte_parts.rs +++ b/vortex-cuda/src/kernel/encodings/decimal_byte_parts.rs @@ -58,11 +58,14 @@ impl CudaExecute for DecimalBytePartsExecutor { .into_primitive() .into_data_parts(); - // SAFETY: The primitive array's buffer is already validated with correct type. - // The decimal dtype matches the array's dtype, and validity is preserved. - Ok(Canonical::Decimal(unsafe { - DecimalArray::new_unchecked_handle(buffer, ptype.try_into()?, decimal_dtype, validity) - })) + // Device storage cannot be narrowed, so parts wider than the precision allows are + // rejected rather than assembled. + Ok(Canonical::Decimal(DecimalArray::try_new_handle( + buffer, + ptype.try_into()?, + decimal_dtype, + validity, + )?)) } } From a7b2e70b00bec62a8c4bfac3f8b4f49da650a9f7 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 9 Oct 2026 14:49:40 +0000 Subject: [PATCH 08/11] Adapt remaining tests to decimal storage rejection Byte-parts, sparse, row and fuzz tests that built decimals with storage wider than their precision allows now use the bounded width, or a precision that admits the width they exercise. Signed-off-by: Joe Isaacs Signed-off-by: Claude Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_015zQPnFWRi7CWXHpgBE8CiF --- .../src/decimal_byte_parts/compute/between.rs | 2 +- .../src/decimal_byte_parts/plugin/tests.rs | 12 ++++----- encodings/sparse/src/canonical.rs | 4 +-- fuzz/src/array/mask.rs | 8 +++--- fuzz/src/array/sum/tests.rs | 27 ++++++++++++++----- vortex-row/src/tests.rs | 6 ++--- 6 files changed, 36 insertions(+), 23 deletions(-) diff --git a/encodings/decimal-byte-parts/src/decimal_byte_parts/compute/between.rs b/encodings/decimal-byte-parts/src/decimal_byte_parts/compute/between.rs index 0bcdd7b3417..e6bfba14325 100644 --- a/encodings/decimal-byte-parts/src/decimal_byte_parts/compute/between.rs +++ b/encodings/decimal-byte-parts/src/decimal_byte_parts/compute/between.rs @@ -172,7 +172,7 @@ mod tests { )? .into_array(); let canonical = DecimalArray::new( - Buffer::from_iter(values.iter().map(|v| *v as i128)), + Buffer::from_iter(values.iter().map(|v| *v as i64)), decimal_dtype, validity, ) diff --git a/encodings/decimal-byte-parts/src/decimal_byte_parts/plugin/tests.rs b/encodings/decimal-byte-parts/src/decimal_byte_parts/plugin/tests.rs index e76476ac39e..0e3a8f656e8 100644 --- a/encodings/decimal-byte-parts/src/decimal_byte_parts/plugin/tests.rs +++ b/encodings/decimal-byte-parts/src/decimal_byte_parts/plugin/tests.rs @@ -39,18 +39,18 @@ use crate::decimal_byte_parts::MAX_LOWER_PARTS; #[case::one_lower_part(DecimalByteParts::try_new_with_lower_parts( msp(), vec![lower_part()], DecimalDType::new(38, 2), ))] -#[case::wider_i64_storage(DecimalByteParts::encode( - &DecimalArray::new(buffer![-99i64, 0, 99], DecimalDType::new(2, 0), Validity::NonNullable), +#[case::i64_storage(DecimalByteParts::encode( + &DecimalArray::new(buffer![-99i64, 0, 99], DecimalDType::new(18, 0), Validity::NonNullable), &mut array_session().create_execution_ctx(), ))] -#[case::wider_i128_storage(DecimalByteParts::encode( - &DecimalArray::new(buffer![-99i128, 0, 99], DecimalDType::new(2, 0), Validity::NonNullable), +#[case::i128_storage(DecimalByteParts::encode( + &DecimalArray::new(buffer![-99i128, 0, 99], DecimalDType::new(38, 0), Validity::NonNullable), &mut array_session().create_execution_ctx(), ))] -#[case::wider_i256_storage(DecimalByteParts::encode( +#[case::i256_storage(DecimalByteParts::encode( &DecimalArray::new( buffer![i256::from_i128(-99), i256::ZERO, i256::from_i128(99)], - DecimalDType::new(2, 0), Validity::NonNullable, + DecimalDType::new(76, 0), Validity::NonNullable, ), &mut array_session().create_execution_ctx(), ))] diff --git a/encodings/sparse/src/canonical.rs b/encodings/sparse/src/canonical.rs index 6232e7176be..73032f8b9a2 100644 --- a/encodings/sparse/src/canonical.rs +++ b/encodings/sparse/src/canonical.rs @@ -1135,7 +1135,7 @@ mod test { let indices = buffer![0u32, 1u32, 7u32, 8u32].into_array(); let decimal_dtype = DecimalDType::new(3, 2); let patch_values = DecimalArray::new( - buffer![100i128, 200i128, 300i128, 4000i128], + buffer![100i16, 200i16, 300i16, 4000i16], decimal_dtype, Validity::from_iter([true, true, true, false]), ) @@ -1148,7 +1148,7 @@ mod test { .arrow() .execute_arrow( DecimalArray::new( - buffer![100i128, 200, 123, 123, 123, 123, 123, 300, 4000, 123], + buffer![100i16, 200, 123, 123, 123, 123, 123, 300, 4000, 123], decimal_dtype, // NB: patch indices: [0, 1, 7, 8]; patch validity: [Valid, Valid, Valid, Invalid]; ergo 0, 1, 7 are valid. Validity::from_mask(Mask::from_excluded_indices(10, vec![8]), Nullable), diff --git a/fuzz/src/array/mask.rs b/fuzz/src/array/mask.rs index 002804806ce..58b3d367766 100644 --- a/fuzz/src/array/mask.rs +++ b/fuzz/src/array/mask.rs @@ -253,16 +253,14 @@ mod tests { fn test_mask_decimal_array() { let mut ctx = array_session().create_execution_ctx(); let dtype = DecimalDType::new(10, 2); - let array = DecimalArray::from_option_iter( - [Some(1i128), Some(2), Some(3), Some(4), Some(5)], - dtype, - ); + let array = + DecimalArray::from_option_iter([Some(1i64), Some(2), Some(3), Some(4), Some(5)], dtype); let mask = Mask::from_iter([true, true, false, true, true]); let result = mask_canonical_array(canonical(array, &mut ctx), &mask, &mut ctx).unwrap(); let expected = - DecimalArray::from_option_iter([Some(1i128), Some(2), None, Some(4), Some(5)], dtype); + DecimalArray::from_option_iter([Some(1i64), Some(2), None, Some(4), Some(5)], dtype); assert_arrays_eq!(result, expected, &mut ctx); } diff --git a/fuzz/src/array/sum/tests.rs b/fuzz/src/array/sum/tests.rs index bd3c3377f39..af0695326d5 100644 --- a/fuzz/src/array/sum/tests.rs +++ b/fuzz/src/array/sum/tests.rs @@ -141,22 +141,37 @@ fn test_sum_signed_unambiguous( #[case(DecimalType::I128)] #[case(DecimalType::I256)] fn test_sum_decimal_storage(#[case] values_type: DecimalType) -> VortexResult<()> { + // Storage is bounded by precision, so each width is exercised at the widest precision it + // may back. + let precision = match values_type { + DecimalType::I8 => 2, + DecimalType::I16 => 4, + DecimalType::I32 => 9, + DecimalType::I64 => 18, + DecimalType::I128 => 38, + DecimalType::I256 => 76, + }; let array = match_each_decimal_value_type!(values_type, |D| { let value = DecimalValue::I8(99) .cast::() .ok_or_else(|| vortex_err!("99 fits in every decimal storage type"))?; DecimalArray::new( buffer![value, value, -value], - DecimalDType::new(2, 0), + DecimalDType::new(precision, 0), Validity::NonNullable, ) .into_array() }); - let expected = Scalar::decimal( - DecimalValue::I64(99), - DecimalDType::new(12, 0), - Nullability::Nullable, - ); + let sum_dtype = DecimalDType::new(u8::min(76, precision + 10), 0); + let expected_value = + match_each_decimal_value_type!(DecimalType::smallest_decimal_value_type(&sum_dtype), |R| { + DecimalValue::from( + DecimalValue::I8(99) + .cast::() + .ok_or_else(|| vortex_err!("99 fits in every decimal storage type"))?, + ) + }); + let expected = Scalar::decimal(expected_value, sum_dtype, Nullability::Nullable); let mut ctx = SESSION.create_execution_ctx(); assert_eq!( sum_canonical_array(&array, &mut ctx)?, diff --git a/vortex-row/src/tests.rs b/vortex-row/src/tests.rs index 1fff3e04044..a885e067ce4 100644 --- a/vortex-row/src/tests.rs +++ b/vortex-row/src/tests.rs @@ -758,8 +758,8 @@ fn decimal_keys_comparable_across_chunk_widths(#[case] descending: bool) -> Vort Ok(()) } -/// A null slot's backing value is unspecified and might not fit the dtype-derived key width; -/// encoding must ignore it rather than report a spurious overflow. +/// A null slot's backing value is unspecified and may lie outside the declared precision; +/// encoding must ignore it rather than let it affect the keys. #[test] fn decimal_null_slot_garbage_does_not_error() -> VortexResult<()> { let mut ctx = array_session().create_execution_ctx(); @@ -767,7 +767,7 @@ fn decimal_null_slot_garbage_does_not_error() -> VortexResult<()> { let field = RowSortField::new(false, true); let chunk = DecimalArray::new( - buffer![484i64, 10_000_000_000_000, 91], + buffer![484i32, i32::MIN, 91], dtype, Validity::from_iter([true, false, true]), ) From 7b6da81e82193a5395e46fd06c30bec01341eccf Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 9 Oct 2026 14:51:34 +0000 Subject: [PATCH 09/11] Adapt CUDA decimal tests to the storage bound The GPU filter test stores its i256 case at a precision that admits i256, and the Arrow export test for narrowing i256 storage below its precision is removed since such storage can no longer be constructed. Signed-off-by: Joe Isaacs Signed-off-by: Claude Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_015zQPnFWRi7CWXHpgBE8CiF --- vortex-cuda/src/arrow/canonical.rs | 15 --------------- vortex-cuda/src/kernel/filter/decimal.rs | 2 +- 2 files changed, 1 insertion(+), 16 deletions(-) diff --git a/vortex-cuda/src/arrow/canonical.rs b/vortex-cuda/src/arrow/canonical.rs index efae985761d..262f1be4740 100644 --- a/vortex-cuda/src/arrow/canonical.rs +++ b/vortex-cuda/src/arrow/canonical.rs @@ -2254,21 +2254,6 @@ mod tests { .await } - #[crate::test] - async fn test_export_decimal_narrowing_errors() -> VortexResult<()> { - let mut ctx = CudaSession::create_execution_ctx(&crate::cuda_session()) - .vortex_expect("failed to create execution context"); - let array = DecimalArray::from_iter([i256::from_parts(0, 1)], DecimalDType::new(38, 0)) - .into_array(); - - let err = array - .export_device_array_with_schema(&mut ctx) - .await - .unwrap_err(); - assert!(err.to_string().contains("narrowing would require")); - Ok(()) - } - #[rstest] #[case::i64( DecimalArray::from_iter([i64::MIN, -1i64, 1, i64::MAX], DecimalDType::new(39, 0)).into_array(), diff --git a/vortex-cuda/src/kernel/filter/decimal.rs b/vortex-cuda/src/kernel/filter/decimal.rs index dc10b908461..8ef267afbbf 100644 --- a/vortex-cuda/src/kernel/filter/decimal.rs +++ b/vortex-cuda/src/kernel/filter/decimal.rs @@ -81,7 +81,7 @@ mod tests { Mask::from_iter([false, true, false, true, false]) )] #[case::i256_values( - DecimalArray::from_iter([i256::from_i128(1), i256::from_i128(2), i256::from_i128(3), i256::from_i128(4), i256::from_i128(5)], DecimalDType::new(19, 5)), + DecimalArray::from_iter([i256::from_i128(1), i256::from_i128(2), i256::from_i128(3), i256::from_i128(4), i256::from_i128(5)], DecimalDType::new(39, 5)), Mask::from_iter([false, true, false, true, false]) )] #[crate::test] From 24eb1e8ea17cb58778767675acaa62b5bd5dc3c0 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 27 Oct 2026 02:13:59 +0000 Subject: [PATCH 10/11] Bound develop's new decimal tests by precision Tests added on develop built decimal storage wider than their precision admits, which the checked constructors now reject. Give each the storage its precision requires, derive expected arrays in the result's own storage, and cap needle widths at the precision bound. Signed-off-by: Joe Isaacs Signed-off-by: Claude Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_015zQPnFWRi7CWXHpgBE8CiF --- .../fns/all_non_distinct/tests.rs | 19 +++++++++--- .../fns/uncompressed_size_in_bytes/mod.rs | 2 +- .../src/arrays/decimal/compute/fill_null.rs | 5 +-- vortex-array/src/arrays/decimal/utils.rs | 3 +- vortex-array/src/arrays/decimal/vtable/mod.rs | 2 +- .../src/arrays/decimal/vtable/operations.rs | 2 +- vortex-array/src/arrays/validation_tests.rs | 2 +- vortex-array/src/builders/decimal.rs | 2 +- .../src/scalar_fn/fns/binary/numeric/tests.rs | 20 +++++++----- .../fns/list_contains/prepared/tests.rs | 31 ++++++++++++------- vortex-arrow/src/convert.rs | 21 +++---------- vortex-arrow/src/executor/decimal.rs | 11 ++++--- 12 files changed, 66 insertions(+), 54 deletions(-) diff --git a/vortex-array/src/aggregate_fn/fns/all_non_distinct/tests.rs b/vortex-array/src/aggregate_fn/fns/all_non_distinct/tests.rs index b7bcc76259b..d7f48b85ba3 100644 --- a/vortex-array/src/aggregate_fn/fns/all_non_distinct/tests.rs +++ b/vortex-array/src/aggregate_fn/fns/all_non_distinct/tests.rs @@ -29,6 +29,7 @@ use crate::dtype::DType; use crate::dtype::DecimalDType; use crate::dtype::DecimalType; use crate::dtype::FieldNames; +use crate::dtype::NativeDecimalType; use crate::dtype::Nullability; use crate::dtype::PType; use crate::match_each_decimal_value_type; @@ -299,8 +300,8 @@ fn decimal_different_value_types() -> VortexResult<()> { } /// Decimal values `[1, 2, last]` stored as `values_type`. -fn decimal_with_storage(values_type: DecimalType, last: i8) -> ArrayRef { - let dtype = DecimalDType::new(2, 0); +fn decimal_with_storage(values_type: DecimalType, precision: u8, last: i8) -> ArrayRef { + let dtype = DecimalDType::new(precision, 0); match_each_decimal_value_type!(values_type, |T| { let values = [1i8, 2, last].map(|value| { ::from(value).vortex_expect("small values fit every decimal type") @@ -331,10 +332,18 @@ fn decimal_every_storage_pair( )] rhs: DecimalType, ) -> VortexResult<()> { - assert_matches_baseline(&decimal_with_storage(lhs, 3), &decimal_with_storage(rhs, 3))?; + // Both sides share a dtype whose precision admits the wider storage; the narrower side + // stays narrow because storage may be any width up to the precision bound. + let precision = match_each_decimal_value_type!(lhs.max(rhs), |T| { + ::MAX_PRECISION + }); assert_matches_baseline( - &decimal_with_storage(lhs, 3), - &decimal_with_storage(rhs, -3), + &decimal_with_storage(lhs, precision, 3), + &decimal_with_storage(rhs, precision, 3), + )?; + assert_matches_baseline( + &decimal_with_storage(lhs, precision, 3), + &decimal_with_storage(rhs, precision, -3), ) } 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 1aa15cabdd8..041ad530d52 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 @@ -780,7 +780,7 @@ mod tests { PrimitiveArray::from_option_iter([Some(1i32), None, Some(3)]).into_array(), BoolArray::from_iter([Some(true), None, Some(false)]).into_array(), DecimalArray::new( - buffer![12345i64, -123i64, 0i64], + buffer![12345i32, -123i32, 0i32], DecimalDType::new(5, 2), Validity::NonNullable, ) diff --git a/vortex-array/src/arrays/decimal/compute/fill_null.rs b/vortex-array/src/arrays/decimal/compute/fill_null.rs index 261f0404193..32a5b1352a7 100644 --- a/vortex-array/src/arrays/decimal/compute/fill_null.rs +++ b/vortex-array/src/arrays/decimal/compute/fill_null.rs @@ -191,10 +191,11 @@ mod tests { ); } - /// fill_null with a value that overflows the array's storage type should upcast the array. + /// fill_null with a value that overflows the array's storage type should upcast the array, + /// but never beyond the storage its precision requires. #[rstest] #[case(DecimalValue::I16(200), DecimalType::I16)] - #[case(DecimalValue::I128(200), DecimalType::I128)] + #[case(DecimalValue::I128(200), DecimalType::I16)] fn fill_null_overflow_upcasts(#[case] fill: DecimalValue, #[case] expected_type: DecimalType) { let mut ctx = SESSION.create_execution_ctx(); let decimal_dtype = DecimalDType::new(3, 0); diff --git a/vortex-array/src/arrays/decimal/utils.rs b/vortex-array/src/arrays/decimal/utils.rs index 30b271c2759..fc024c7d993 100644 --- a/vortex-array/src/arrays/decimal/utils.rs +++ b/vortex-array/src/arrays/decimal/utils.rs @@ -172,6 +172,7 @@ mod tests { use crate::dtype::BigCast; use crate::dtype::DecimalDType; use crate::dtype::DecimalType; + use crate::dtype::NativeDecimalType; use crate::match_each_decimal_value_type; use crate::validity::Validity; @@ -198,8 +199,8 @@ mod tests { target: DecimalType, ) -> VortexResult<()> { let values = [1i8, -2, 3]; - let dtype = DecimalDType::new(2, 0); let array = match_each_decimal_value_type!(storage, |S| { + let dtype = DecimalDType::new(::MAX_PRECISION, 0); let values = values.map(|value| { ::from(value).vortex_expect("small values fit every decimal type") }); diff --git a/vortex-array/src/arrays/decimal/vtable/mod.rs b/vortex-array/src/arrays/decimal/vtable/mod.rs index 38065b8644f..04130c21041 100644 --- a/vortex-array/src/arrays/decimal/vtable/mod.rs +++ b/vortex-array/src/arrays/decimal/vtable/mod.rs @@ -308,7 +308,7 @@ mod tests { assert!(decoded.is::()); assert_arrays_eq!( decoded, - DecimalArray::from_iter([100i128, 200, 300, 400, 500], DecimalDType::new(10, 2)), + DecimalArray::from_iter([100i64, 200, 300, 400, 500], DecimalDType::new(10, 2)), &mut SESSION.create_execution_ctx() ); } diff --git a/vortex-array/src/arrays/decimal/vtable/operations.rs b/vortex-array/src/arrays/decimal/vtable/operations.rs index ef1dc98fcd2..620e7393bb8 100644 --- a/vortex-array/src/arrays/decimal/vtable/operations.rs +++ b/vortex-array/src/arrays/decimal/vtable/operations.rs @@ -78,7 +78,7 @@ mod tests { assert_eq!(sliced.len(), 2); assert_arrays_eq!( sliced, - DecimalArray::from_option_iter([Some(200i128), None], DecimalDType::new(3, 2)), + DecimalArray::from_option_iter([Some(200i16), None], DecimalDType::new(4, 2)), &mut SESSION.create_execution_ctx() ); } diff --git a/vortex-array/src/arrays/validation_tests.rs b/vortex-array/src/arrays/validation_tests.rs index b9b50a26de9..fbd77805dbd 100644 --- a/vortex-array/src/arrays/validation_tests.rs +++ b/vortex-array/src/arrays/validation_tests.rs @@ -60,7 +60,7 @@ mod tests { #[test] fn test_decimal_array_validation_failure_length_mismatch() { // Invalid case: validity length doesn't match buffer length. - let buffer = Buffer::from_iter([100i128, 200, 300]); + let buffer = Buffer::from_iter([100i64, 200, 300]); let validity = Validity::from_iter([true, false]); // Length 2, buffer is length 3. let decimal_dtype = crate::dtype::DecimalDType::new(10, 2); let result = DecimalArray::try_new(buffer, decimal_dtype, validity); diff --git a/vortex-array/src/builders/decimal.rs b/vortex-array/src/builders/decimal.rs index fde937a2364..3568c9a4916 100644 --- a/vortex-array/src/builders/decimal.rs +++ b/vortex-array/src/builders/decimal.rs @@ -381,7 +381,7 @@ mod tests { for value in 0i8..42 { let expected = Scalar::decimal( DecimalValue::I8(value), - DecimalDType::new(2, 1), + DecimalDType::new(20, 1), Nullability::NonNullable, ); assert_eq!( diff --git a/vortex-array/src/scalar_fn/fns/binary/numeric/tests.rs b/vortex-array/src/scalar_fn/fns/binary/numeric/tests.rs index 4da9c311051..d36cc816ea2 100644 --- a/vortex-array/src/scalar_fn/fns/binary/numeric/tests.rs +++ b/vortex-array/src/scalar_fn/fns/binary/numeric/tests.rs @@ -6,6 +6,7 @@ use std::sync::LazyLock; use rstest::rstest; use vortex_buffer::Buffer; use vortex_buffer::buffer; +use vortex_error::VortexExpect; use vortex_error::VortexResult; use vortex_session::VortexSession; @@ -20,6 +21,7 @@ use crate::arrays::DecimalArray; use crate::arrays::PrimitiveArray; use crate::assert_arrays_eq; use crate::builtins::ArrayBuiltins; +use crate::dtype::BigCast; use crate::dtype::DType; use crate::dtype::DecimalDType; use crate::dtype::DecimalType; @@ -28,6 +30,7 @@ use crate::dtype::NativePType; use crate::dtype::Nullability; use crate::dtype::PType; use crate::dtype::i256; +use crate::match_each_decimal_value_type; use crate::scalar::DecimalValue; use crate::scalar::NumericOperator; use crate::scalar::Scalar; @@ -685,14 +688,15 @@ fn test_decimal_result_uses_widened_logical_storage_width( assert_eq!(result.values_type(), expected_type); assert_eq!(result.decimal_dtype(), expected_dtype); - assert_arrays_eq!( - result, - DecimalArray::from_iter::( - [i256::from_i128(20), i256::from_i128(40)], - expected_dtype - ), - &mut SESSION.create_execution_ctx() - ); + let expected = match_each_decimal_value_type!(expected_type, |T| { + DecimalArray::from_iter::( + [20i8, 40].map(|value| { + ::from(value).vortex_expect("small values fit every decimal type") + }), + expected_dtype, + ) + }); + assert_arrays_eq!(result, expected, &mut SESSION.create_execution_ctx()); Ok(()) } diff --git a/vortex-array/src/scalar_fn/fns/list_contains/prepared/tests.rs b/vortex-array/src/scalar_fn/fns/list_contains/prepared/tests.rs index e67c1717998..772a90e8f08 100644 --- a/vortex-array/src/scalar_fn/fns/list_contains/prepared/tests.rs +++ b/vortex-array/src/scalar_fn/fns/list_contains/prepared/tests.rs @@ -198,6 +198,8 @@ fn test_decimal_bitmap_across_storage_widths( .into(); elements.push(Scalar::null(dtype.clone())); let list = Scalar::list(dtype, elements, Nullability::NonNullable); + // Storage may be any width up to the one the precision requires. + let needle_width = needle_width.min(DecimalType::smallest_decimal_value_type(&decimal)); let needles = match_each_decimal_value_type!(needle_width, |T| { DecimalArray::from_option_iter::( [Some(-2i8), Some(0), Some(1), None] @@ -238,18 +240,23 @@ fn test_decimal_wide_values( Nullability::NonNullable, ); // The last non-null value shares the low 64 bits of a member but must not match it. - let needles = DecimalArray::from_option_iter::( - [ - Some(base), - Some(second), - Some(base - i256::ONE), - Some(i256::ZERO), - Some(base + i256::from_i128(1i128 << 64)), - None, - ], - decimal, - ) - .into_array(); + let needle_values = [ + Some(base), + Some(second), + Some(base - i256::ONE), + Some(i256::ZERO), + Some(base + i256::from_i128(1i128 << 64)), + None, + ]; + let needles = + match_each_decimal_value_type!(DecimalType::smallest_decimal_value_type(&decimal), |T| { + DecimalArray::from_option_iter::( + needle_values + .map(|value| value.map(|value| DecimalValue::from(value).cast::().unwrap())), + decimal, + ) + .into_array() + }); let set = prepare(&list)?; assert_arrays_eq!( set.contains(&needles, &ListContainsOptions::default(), &mut ctx)?, diff --git a/vortex-arrow/src/convert.rs b/vortex-arrow/src/convert.rs index a6c8ff1df25..fb9e9b4633c 100644 --- a/vortex-arrow/src/convert.rs +++ b/vortex-arrow/src/convert.rs @@ -840,7 +840,6 @@ mod tests { use vortex_array::dtype::DecimalDType; use vortex_array::dtype::Nullability; use vortex_array::dtype::PType; - use vortex_array::dtype::i256; use vortex_array::extension::datetime::TimeUnit; use vortex_array::extension::datetime::Timestamp; use vortex_array::scalar::Scalar; @@ -1057,14 +1056,14 @@ mod tests { assert_arrays_eq!( vortex_array, DecimalArray::from_option_iter( - [Some(12345i128), None, Some(67890i128), Some(11111i128)], + [Some(12345i64), None, Some(67890i64), Some(11111i64)], DecimalDType::new(10, 2) ), &mut SESSION.create_execution_ctx() ); assert_arrays_eq!( vortex_array_non_null, - DecimalArray::from_iter([12345i128, 67890i128, 11111i128], DecimalDType::new(10, 2)), + DecimalArray::from_iter([12345i64, 67890i64, 11111i64], DecimalDType::new(10, 2)), &mut SESSION.create_execution_ctx() ); } @@ -1107,26 +1106,14 @@ mod tests { assert_arrays_eq!( vortex_array, DecimalArray::from_option_iter( - [ - Some(i256::from_i128(12345)), - None, - Some(i256::from_i128(67890)), - Some(i256::from_i128(11111)) - ], + [Some(12345i128), None, Some(67890i128), Some(11111i128)], DecimalDType::new(38, 10) ), &mut SESSION.create_execution_ctx() ); assert_arrays_eq!( vortex_array_non_null, - DecimalArray::from_iter( - [ - i256::from_i128(12345), - i256::from_i128(67890), - i256::from_i128(11111) - ], - DecimalDType::new(38, 10) - ), + DecimalArray::from_iter([12345i128, 67890i128, 11111i128], DecimalDType::new(38, 10)), &mut SESSION.create_execution_ctx() ); } diff --git a/vortex-arrow/src/executor/decimal.rs b/vortex-arrow/src/executor/decimal.rs index 05fc3c6ecd5..e5ca6071c16 100644 --- a/vortex-arrow/src/executor/decimal.rs +++ b/vortex-arrow/src/executor/decimal.rs @@ -318,7 +318,8 @@ mod tests { assert_eq!(arrow_decimal.value(2), 12); assert_eq!( arrow_decimal, - &Decimal128Array::from(vec![10, 11, 12]).with_precision_and_scale(2, 1)? + &Decimal128Array::from(vec![10, 11, 12]) + .with_precision_and_scale(precision_for::(), 1)? ); Ok(()) } @@ -354,7 +355,8 @@ mod tests { assert_eq!(arrow_decimal.value(2), 12); assert_eq!( arrow_decimal, - &Decimal32Array::from(vec![10, 11, 12]).with_precision_and_scale(2, 1)? + &Decimal32Array::from(vec![10, 11, 12]) + .with_precision_and_scale(precision_for::(), 1)? ); Ok(()) } @@ -391,7 +393,8 @@ mod tests { assert_eq!(arrow_decimal.value(2), 12); assert_eq!( arrow_decimal, - &Decimal64Array::from(vec![10, 11, 12]).with_precision_and_scale(2, 1)? + &Decimal64Array::from(vec![10, 11, 12]) + .with_precision_and_scale(precision_for::(), 1)? ); Ok(()) } @@ -435,7 +438,7 @@ mod tests { i256::from_i128(11), i256::from_i128(12) ]) - .with_precision_and_scale(2, 1)? + .with_precision_and_scale(precision_for::(), 1)? ); Ok(()) } From 24488cc156cc074031be92e62d8870a36a54b591 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 27 Oct 2026 02:26:14 +0000 Subject: [PATCH 11/11] Write wide legacy decimal fixtures at the canonical width The checked decimal constructors reject storage wider than the precision requires, so the compat fixtures that specified such a width can no longer build their written form. Keep the fixtures so their published files stay covered by `check`, which reads the wide form back through legacy narrowing, and write those columns at the canonical width instead. Signed-off-by: Joe Isaacs Signed-off-by: Claude Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_015zQPnFWRi7CWXHpgBE8CiF --- .../arrays/synthetic/arrays/decimal_legacy.rs | 41 +++++++++++++++---- .../src/fixtures/arrays/synthetic/mod.rs | 14 +++++-- 2 files changed, 44 insertions(+), 11 deletions(-) diff --git a/vortex-test/compat-gen/src/fixtures/arrays/synthetic/arrays/decimal_legacy.rs b/vortex-test/compat-gen/src/fixtures/arrays/synthetic/arrays/decimal_legacy.rs index 15ea9ac12f9..3c11148340b 100644 --- a/vortex-test/compat-gen/src/fixtures/arrays/synthetic/arrays/decimal_legacy.rs +++ b/vortex-test/compat-gen/src/fixtures/arrays/synthetic/arrays/decimal_legacy.rs @@ -9,10 +9,10 @@ //! values in null slots. [`FlatLayoutFixture::build`] returns the same logical values at the //! canonical width, so `check` compares each old file with a canonical array. //! -//! The overrides depend on [`DecimalArray::try_new`] accepting a stored width that differs from -//! the canonical width. Once it rejects such a width, `build_written` returns that error, and -//! `generate` and the round-trip test fail. The overrides must then be deleted. The published files -//! keep the old forms. +//! [`DecimalArray::try_new`] accepts a stored width up to the canonical width, so the narrow forms +//! are still written as stored. It rejects a wider width, so a column specified wider than the +//! canonical width is written at the canonical width instead; the published files keep the wide +//! form, which `check` reads back through legacy narrowing and compares with the canonical array. use vortex_array::ArrayId; use vortex_array::ArrayRef; @@ -52,7 +52,13 @@ pub struct LegacyDecimalColumn { impl LegacyDecimalColumn { /// Builds the column at its stored width with the raw slot values. + /// + /// A width above the canonical width can no longer be constructed, so such a column is built + /// at the canonical width like [`Self::expected`]. fn written(&self) -> VortexResult { + if self.written_type() != self.stored_type { + return self.expected(); + } decimal_array( self.decimal_dtype, self.stored_type, @@ -61,6 +67,14 @@ impl LegacyDecimalColumn { ) } + /// The width at which the column is written: its stored width, capped at the canonical width. + pub fn written_type(&self) -> DecimalType { + self.stored_type + .min(DecimalType::smallest_decimal_value_type( + &self.decimal_dtype, + )) + } + /// Builds the column at the canonical width. /// /// A column that is already stored at the canonical width keeps its raw slot values, so its @@ -72,6 +86,17 @@ impl LegacyDecimalColumn { return self.written(); } + decimal_array( + self.decimal_dtype, + canonical_type, + self.canonical_slots(), + self.validity(), + ) + } + + /// The raw slot values at the canonical width: the valid slots as specified, and zero in + /// every null slot. + pub fn canonical_slots(&self) -> [i128; ROWS] { let mut slots = self.slots; if let Some(validity) = self.validity { for (slot, valid) in slots.iter_mut().zip(validity) { @@ -80,8 +105,7 @@ impl LegacyDecimalColumn { } } } - - decimal_array(self.decimal_dtype, canonical_type, slots, self.validity()) + slots } fn validity(&self) -> Validity { @@ -350,8 +374,9 @@ fn high_scale() -> LegacyDecimalFixture { } } -/// Columns stored wider than the canonical width, as an import of an Arrow decimal type can store -/// them. +/// Columns stored wider than the canonical width, as an import of an Arrow decimal type could +/// store them. Such a width can no longer be constructed, so `generate` now writes these columns +/// at the canonical width; the published files keep the wide form. fn wide_storage() -> LegacyDecimalFixture { let max_p18 = 10i128.pow(18) - 1; let max_p38 = 10i128.pow(38) - 1; diff --git a/vortex-test/compat-gen/src/fixtures/arrays/synthetic/mod.rs b/vortex-test/compat-gen/src/fixtures/arrays/synthetic/mod.rs index e3b10450113..bbea01a5f00 100644 --- a/vortex-test/compat-gen/src/fixtures/arrays/synthetic/mod.rs +++ b/vortex-test/compat-gen/src/fixtures/arrays/synthetic/mod.rs @@ -129,10 +129,18 @@ mod tests { {canonical_type}", ); + // A width above the canonical width can no longer be constructed, so such a + // column is written at the canonical width with zeroed null slots. + let written_type = column.written_type(); + let written_slots = if written_type == column.stored_type { + column.slots + } else { + column.canonical_slots() + }; let written_field = written.unmasked_field_by_name(column.name)?; assert_eq!( written_field.as_::().values_type(), - column.stored_type, + written_type, "{context}", ); @@ -142,12 +150,12 @@ mod tests { .execute::(&mut ctx)?; assert_eq!( read_field.values_type(), - column.stored_type, + written_type, "{context} after a file round trip", ); assert_eq!( raw_slots(&read_field), - column.slots, + written_slots, "{context} after a file round trip", ); }