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..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"), } @@ -368,14 +369,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/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-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/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 eff2e5c8688..1d64ba56843 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. + /// 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 /// @@ -242,11 +252,54 @@ impl DecimalData { decimal_dtype: DecimalDType, ) -> VortexResult { Self::validate(&values, values_type)?; + let maximum = DecimalType::smallest_decimal_value_type(&decimal_dtype); + vortex_ensure!( + values_type <= maximum, + InvalidArgument: "decimal storage {values_type} exceeds precision storage {maximum} for {decimal_dtype}", + ); - // 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) }) } + /// 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. @@ -257,6 +310,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 +335,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 +345,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 +384,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 +466,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, @@ -442,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, @@ -607,3 +688,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 df868396dad..dcf87b7d9df 100644 --- a/vortex-array/src/arrays/decimal/compute/cast.rs +++ b/vortex-array/src/arrays/decimal/compute/cast.rs @@ -448,14 +448,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, @@ -477,6 +476,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 @@ -583,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, ) }); @@ -598,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)?, @@ -727,7 +739,7 @@ mod tests { } let huge = DecimalArray::new( buffer![i256::ONE], - DecimalDType::new(1, -128), + DecimalDType::new(39, -128), Validity::NonNullable, ); assert!( @@ -1046,7 +1058,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, @@ -1110,6 +1122,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/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/compute/rules.rs b/vortex-array/src/arrays/decimal/compute/rules.rs index a6775aba54a..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; @@ -112,6 +113,23 @@ 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.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. #[derive(Debug, PartialEq, Eq, Hash)] struct TestDeviceBuffer(ByteBuffer); @@ -173,7 +191,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..b5d4a4d5301 --- /dev/null +++ b/vortex-array/src/arrays/decimal/tests.rs @@ -0,0 +1,152 @@ +// 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_rejects_storage_wider_than_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 result = DecimalArray::try_new_handle( + BufferHandle::new_host(bytes), + storage, + dtype, + Validity::NonNullable, + ); + 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), + &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_narrowed( + 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_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)] +#[test] +#[should_panic(expected = "decimal storage i16 exceeds")] +fn unchecked_construction_asserts_storage_bound() { + // Deliberately violate the width contract to exercise its debug assertion. + drop(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, + 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 a7f321d6507..518bba9a1f5 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) { @@ -127,9 +131,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, @@ -174,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(), @@ -229,7 +245,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; @@ -238,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, ); @@ -264,6 +283,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..133e41770fa 100644 --- a/vortex-array/src/arrays/decimal/vtable/operations.rs +++ b/vortex-array/src/arrays/decimal/vtable/operations.rs @@ -47,8 +47,8 @@ mod tests { #[test] fn test_slice() { let array = DecimalArray::new( - buffer![100i128, 200i128, 300i128, 4000i128], - DecimalDType::new(3, 2), + buffer![100i16, 200i16, 300i16, 4000i16], + 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), + buffer![100i16, 200i16, 300i16, 4000i16], + DecimalDType::new(4, 2), Validity::from_iter([false, true, false, true]), ) .into_array(); @@ -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 3813c8612b3..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!( @@ -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(); @@ -507,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!( @@ -787,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(()) @@ -827,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(()) @@ -840,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); @@ -850,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/convert.rs b/vortex-arrow/src/convert.rs index dc6ff87c612..651bacdb0c7 100644 --- a/vortex-arrow/src/convert.rs +++ b/vortex-arrow/src/convert.rs @@ -201,7 +201,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, @@ -209,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 { @@ -221,7 +223,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, @@ -229,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 { @@ -241,7 +245,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, @@ -249,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 { @@ -261,7 +267,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, @@ -273,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 { 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::() diff --git a/vortex-cuda/src/arrow/canonical.rs b/vortex-cuda/src/arrow/canonical.rs index fb06f4c48ab..262f1be4740 100644 --- a/vortex-cuda/src/arrow/canonical.rs +++ b/vortex-cuda/src/arrow/canonical.rs @@ -2254,35 +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(()) - } - - #[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, + )?)) } } 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] diff --git a/vortex-row/src/tests.rs b/vortex-row/src/tests.rs index c289fef65a6..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]), ) @@ -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();