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 1cdf27c15bd..417896b6bc1 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") @@ -374,14 +375,14 @@ mod tests { validity: Validity, ) -> VortexResult<()> { let mut ctx = 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!(round_trip(decimal.clone())?, 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 8ef4d81daf3..c0d8be74efb 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 f74c3bf1c55..3e9b5285ef9 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 @@ -43,18 +43,18 @@ static SESSION: LazyLock = LazyLock::new(VortexSession::empty); #[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 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 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 SESSION.create_execution_ctx(), ))] diff --git a/encodings/sparse/src/canonical.rs b/encodings/sparse/src/canonical.rs index 796fda56ef5..37d3252ca18 100644 --- a/encodings/sparse/src/canonical.rs +++ b/encodings/sparse/src/canonical.rs @@ -1125,7 +1125,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]), ) @@ -1139,7 +1139,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 c20f024448a..6256fcb67d1 100644 --- a/fuzz/src/array/mask.rs +++ b/fuzz/src/array/mask.rs @@ -250,16 +250,14 @@ mod tests { fn test_mask_decimal_array() { let mut ctx = VortexSession::empty().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 f48fb01604f..312ac1866d0 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/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/min_max/mod.rs b/vortex-array/src/aggregate_fn/fns/min_max/mod.rs index 8aa0e559e84..118766ad860 100644 --- a/vortex-array/src/aggregate_fn/fns/min_max/mod.rs +++ b/vortex-array/src/aggregate_fn/fns/min_max/mod.rs @@ -943,7 +943,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 799d83b7219..9d4a77f5c15 100644 --- a/vortex-array/src/aggregate_fn/fns/sum/decimal.rs +++ b/vortex-array/src/aggregate_fn/fns/sum/decimal.rs @@ -151,7 +151,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, ); @@ -170,7 +170,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]), ); @@ -189,7 +189,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, ); @@ -269,7 +269,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(), &mut SESSION.create_execution_ctx())?; @@ -285,7 +285,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 eca323d70a1..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 @@ -670,7 +670,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, ) @@ -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/constant/vtable/canonical.rs b/vortex-array/src/arrays/constant/vtable/canonical.rs index bbea85c4077..c1f3d1b4b61 100644 --- a/vortex-array/src/arrays/constant/vtable/canonical.rs +++ b/vortex-array/src/arrays/constant/vtable/canonical.rs @@ -42,11 +42,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::ListScalar; use crate::scalar::Scalar; use crate::scalar::ScalarValue; @@ -110,15 +108,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 61aaf898515..05367f2307b 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, + "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(), + "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, @@ -464,6 +530,21 @@ impl Array { Array::try_from_parts(parts) } + /// 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, @@ -629,3 +710,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 29b38ce9406..e795865bbd8 100644 --- a/vortex-array/src/arrays/decimal/compute/cast.rs +++ b/vortex-array/src/arrays/decimal/compute/cast.rs @@ -439,14 +439,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, @@ -468,6 +467,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 @@ -625,10 +627,20 @@ mod tests { target: PType, ) -> VortexResult<()> { let mut ctx = 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, ) }); @@ -637,10 +649,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)?, @@ -754,7 +766,7 @@ mod tests { } let huge = DecimalArray::new( buffer![i256::ONE], - DecimalDType::new(1, -128), + DecimalDType::new(39, -128), Validity::NonNullable, ); assert!( @@ -1086,7 +1098,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, @@ -1152,6 +1164,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 3755f6c6d1f..32a5b1352a7 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(); @@ -90,6 +90,7 @@ mod tests { use rstest::rstest; use vortex_buffer::buffer; + use vortex_error::VortexResult; use vortex_session::VortexSession; use crate::IntoArray; @@ -106,6 +107,28 @@ mod tests { static SESSION: LazyLock = LazyLock::new(VortexSession::empty); + #[test] + fn fill_null_widens_only_to_precision_bound() -> VortexResult<()> { + let mut ctx = 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 = SESSION.create_execution_ctx(); @@ -168,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/compute/mod.rs b/vortex-array/src/arrays/decimal/compute/mod.rs index 88a8713c2dd..460f383deee 100644 --- a/vortex-array/src/arrays/decimal/compute/mod.rs +++ b/vortex-array/src/arrays/decimal/compute/mod.rs @@ -41,12 +41,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, ))] @@ -56,7 +56,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, ))] @@ -76,7 +76,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, ))] @@ -94,7 +94,7 @@ mod tests { Validity::NonNullable, ))] #[case::decimal_single(DecimalArray::new( - buffer![42i128], + buffer![42i64], DecimalDType::new(10, 0), Validity::NonNullable, ))] @@ -104,7 +104,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 8f1d09ea92a..6f684a7a105 100644 --- a/vortex-array/src/arrays/decimal/compute/rules.rs +++ b/vortex-array/src/arrays/decimal/compute/rules.rs @@ -105,6 +105,7 @@ mod tests { use crate::VortexSessionExecute; 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; @@ -115,6 +116,23 @@ mod tests { static SESSION: LazyLock = LazyLock::new(VortexSession::empty); + #[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); @@ -176,7 +194,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..a78dccc94ba --- /dev/null +++ b/vortex-array/src/arrays/decimal/tests.rs @@ -0,0 +1,156 @@ +// SPDX-License-Identifier: Apache-2.0 +// SPDX-FileCopyrightText: Copyright the Vortex contributors + +use std::sync::LazyLock; + +use num_traits::AsPrimitive; +use rstest::rstest; +use vortex_buffer::Buffer; +use vortex_buffer::buffer; +use vortex_error::VortexResult; +use vortex_session::VortexSession; + +use crate::ArrayParts; +use crate::VortexSessionExecute; +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; + +static SESSION: LazyLock = LazyLock::new(VortexSession::empty); + +#[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 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 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 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/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 99d7314bbfa..04130c21041 100644 --- a/vortex-array/src/arrays/decimal/vtable/mod.rs +++ b/vortex-array/src/arrays/decimal/vtable/mod.rs @@ -44,6 +44,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) { @@ -139,9 +143,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, + "DecimalArray data dtype does not match outer dtype", + ); + vortex_ensure!( + data.values_type <= DecimalType::smallest_decimal_value_type(decimal_dtype), + "DecimalArray storage exceeds the type required by its precision", + ); vortex_ensure_eq!( data.len(), len, @@ -187,6 +200,9 @@ impl VTable for Decimal { "DecimalArray buffer not aligned for values type {:?}", D::DECIMAL_TYPE ); + // Older writers allowed storage wider than the precision requires. + let (values, values_type) = + DecimalData::narrow_to_precision(values, values_type, *decimal_dtype)?; DecimalData::try_new_handle(values, values_type, *decimal_dtype) })?; Ok(ArrayParts::new( @@ -266,7 +282,7 @@ mod tests { #[test] fn test_array_serde() { let array = DecimalArray::new( - buffer![100i128, 200i128, 300i128, 400i128, 500i128], + buffer![100i64, 200i64, 300i64, 400i64, 500i64], DecimalDType::new(10, 2), Validity::NonNullable, ); @@ -292,7 +308,43 @@ 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() + ); + } + + #[test] + fn deserialize_legacy_oversized_storage() { + 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() ); } diff --git a/vortex-array/src/arrays/decimal/vtable/operations.rs b/vortex-array/src/arrays/decimal/vtable/operations.rs index 09d1d31a29c..620e7393bb8 100644 --- a/vortex-array/src/arrays/decimal/vtable/operations.rs +++ b/vortex-array/src/arrays/decimal/vtable/operations.rs @@ -52,8 +52,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(); @@ -62,14 +62,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(); @@ -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() ); } @@ -86,7 +86,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 3c59d1a02c2..fbd77805dbd 100644 --- a/vortex-array/src/arrays/validation_tests.rs +++ b/vortex-array/src/arrays/validation_tests.rs @@ -48,10 +48,19 @@ mod tests { ); } + #[test] + fn test_decimal_array_validation_success() { + // Valid case: buffer and validity have matching lengths. + 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()); + } + #[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 2a664f17398..3568c9a4916 100644 --- a/vortex-array/src/builders/decimal.rs +++ b/vortex-array/src/builders/decimal.rs @@ -348,7 +348,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(), ); @@ -358,7 +358,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(), ); @@ -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/between/mod.rs b/vortex-array/src/scalar_fn/fns/between/mod.rs index 44f7dc29c3b..ca92f4b128e 100644 --- a/vortex-array/src/scalar_fn/fns/between/mod.rs +++ b/vortex-array/src/scalar_fn/fns/between/mod.rs @@ -659,7 +659,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 83d01e2d831..e065cb9d5bd 100644 --- a/vortex-array/src/scalar_fn/fns/binary/compare/tests.rs +++ b/vortex-array/src/scalar_fn/fns/binary/compare/tests.rs @@ -578,7 +578,7 @@ fn decimal_compare_mixed_storage_widths() { let mut ctx = 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 325e55665f8..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; @@ -422,7 +425,7 @@ fn test_decimal_mixed_storage_widths() -> VortexResult<()> { let mut ctx = 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!( @@ -458,10 +461,12 @@ fn test_decimal_nullable_lanes() -> VortexResult<()> { #[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()); } @@ -488,22 +493,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(); @@ -513,15 +520,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 = 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!( @@ -679,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(()) } @@ -806,7 +816,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(()) @@ -846,7 +856,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(()) @@ -859,7 +869,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 = SESSION.create_execution_ctx(); let dtype = DecimalDType::new(10, 2); @@ -869,10 +879,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-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 0dbdc822a2d..fb9e9b4633c 100644 --- a/vortex-arrow/src/convert.rs +++ b/vortex-arrow/src/convert.rs @@ -176,7 +176,9 @@ where Ok(PrimitiveArray::new(buffer, validity).into_array()) } -/// 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, @@ -184,10 +186,12 @@ 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()) } -/// 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, @@ -195,10 +199,12 @@ 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()) } -/// 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, @@ -206,10 +212,12 @@ 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()) } -/// 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, @@ -221,7 +229,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()) } /// Conversion of an Arrow temporal array (timestamp/date/time) into a Vortex temporal @@ -832,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; @@ -1049,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() ); } @@ -1099,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 d86406277bd..e5ca6071c16 100644 --- a/vortex-arrow/src/executor/decimal.rs +++ b/vortex-arrow/src/executor/decimal.rs @@ -235,6 +235,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; @@ -246,6 +247,19 @@ mod tests { static SESSION: LazyLock = LazyLock::new(VortexSession::empty); + /// 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 = SESSION.create_execution_ctx(); @@ -277,13 +291,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 = 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(), ); @@ -292,7 +305,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::() @@ -302,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(()) } @@ -311,15 +328,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 = 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(), ); @@ -328,7 +342,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::() @@ -338,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(()) } @@ -348,14 +366,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 = 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(), ); @@ -364,7 +380,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::() @@ -374,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(()) } @@ -391,7 +411,7 @@ mod tests { ) -> VortexResult<()> { let mut ctx = 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(), ); @@ -400,7 +420,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::() @@ -415,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(()) } diff --git a/vortex-cuda/src/arrow/canonical.rs b/vortex-cuda/src/arrow/canonical.rs index 4bb7714b100..2f125230ba6 100644 --- a/vortex-cuda/src/arrow/canonical.rs +++ b/vortex-cuda/src/arrow/canonical.rs @@ -2295,35 +2295,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 ee3814ca9fc..fe398ca36c7 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 c09997520b5..caff884fb91 100644 --- a/vortex-cuda/src/kernel/filter/decimal.rs +++ b/vortex-cuda/src/kernel/filter/decimal.rs @@ -86,7 +86,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 ea1ae1fec38..1a5f6809401 100644 --- a/vortex-row/src/tests.rs +++ b/vortex-row/src/tests.rs @@ -801,8 +801,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 = SESSION.create_execution_ctx(); @@ -810,7 +810,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]), ) @@ -823,25 +823,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 = 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 = SESSION.create_execution_ctx(); 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", ); }