diff --git a/docs/specs/row-encoding.md b/docs/specs/row-encoding.md index e555735ac26..cf93f2f3fcd 100644 --- a/docs/specs/row-encoding.md +++ b/docs/specs/row-encoding.md @@ -275,7 +275,7 @@ null_sentinel || zero(width(T)) ## Decimal Decimals are encoded as their scaled signed integer storage value. The selected storage -width is the smallest decimal value type for the decimal precision: +width is the canonical width of the decimal precision, `DecimalDType::canonical_values_type()`: | Precision | Storage | | --- | --- | diff --git a/encodings/sparse/src/canonical.rs b/encodings/sparse/src/canonical.rs index 796fda56ef5..e3995afd106 100644 --- a/encodings/sparse/src/canonical.rs +++ b/encodings/sparse/src/canonical.rs @@ -40,7 +40,6 @@ use vortex_array::builders::VarBinViewBuilder; use vortex_array::builders::builder_with_capacity_in; use vortex_array::dtype::DType; use vortex_array::dtype::DecimalDType; -use vortex_array::dtype::DecimalType; use vortex_array::dtype::IntegerPType; use vortex_array::dtype::NativeDecimalType; use vortex_array::dtype::NativePType; @@ -314,8 +313,7 @@ pub(super) fn execute_sparse(parts: SparseParts, ctx: &mut ExecutionCtx) -> Vort }) } DType::Decimal(decimal_dtype, nullability) => { - let canonical_decimal_value_type = - DecimalType::smallest_decimal_value_type(decimal_dtype); + let canonical_decimal_value_type = decimal_dtype.canonical_values_type(); let fill_decimal = fill_value.as_decimal(); match_each_decimal_value_type!(canonical_decimal_value_type, |D| { execute_sparse_decimal::( diff --git a/fuzz/src/array/sum/mod.rs b/fuzz/src/array/sum/mod.rs index 98b3f69ce2c..657f6e86223 100644 --- a/fuzz/src/array/sum/mod.rs +++ b/fuzz/src/array/sum/mod.rs @@ -11,7 +11,6 @@ use vortex_array::arrays::bool::BoolArrayExt; use vortex_array::dtype::BigCast; use vortex_array::dtype::DType; use vortex_array::dtype::DecimalDType; -use vortex_array::dtype::DecimalType; use vortex_array::dtype::MAX_PRECISION; use vortex_array::dtype::Nullability::Nullable; use vortex_array::dtype::i256; @@ -51,7 +50,7 @@ pub fn sum_canonical_array( let limit = i256::from_i128(10) .checked_pow(output_dtype.precision().into()) .vortex_expect("10^76 fits in i256"); - let values_type = DecimalType::smallest_decimal_value_type(&output_dtype); + let values_type = output_dtype.canonical_values_type(); match_each_decimal_value_type!(values_type, |I| { let limit = ::from(limit) .vortex_expect("precision limit fits native accumulator"); diff --git a/vortex-array/src/aggregate_fn/fns/sum/decimal.rs b/vortex-array/src/aggregate_fn/fns/sum/decimal.rs index 799d83b7219..83bb2e0d65e 100644 --- a/vortex-array/src/aggregate_fn/fns/sum/decimal.rs +++ b/vortex-array/src/aggregate_fn/fns/sum/decimal.rs @@ -17,7 +17,6 @@ use crate::ExecutionCtx; use crate::arrays::DecimalArray; use crate::dtype::DType; use crate::dtype::DecimalDType; -use crate::dtype::DecimalType; use crate::dtype::NativeDecimalType; use crate::match_each_decimal_value_type; use crate::scalar::DecimalValue; @@ -46,7 +45,7 @@ pub(crate) fn accumulate_decimal( .as_decimal_opt() .vortex_expect("decimal sum result dtype"); - let values_type = DecimalType::smallest_decimal_value_type(dtype); + let values_type = dtype.canonical_values_type(); match_each_decimal_value_type!(d.values_type(), |T| { match_each_decimal_value_type!(values_type, |I| { let initial: I = value diff --git a/vortex-array/src/aggregate_fn/fns/uncompressed_size_in_bytes/decimal.rs b/vortex-array/src/aggregate_fn/fns/uncompressed_size_in_bytes/decimal.rs index c04fa55f0ab..8fb1c8a94fe 100644 --- a/vortex-array/src/aggregate_fn/fns/uncompressed_size_in_bytes/decimal.rs +++ b/vortex-array/src/aggregate_fn/fns/uncompressed_size_in_bytes/decimal.rs @@ -8,7 +8,6 @@ use super::validity_uncompressed_size_in_bytes; use crate::ExecutionCtx; use crate::arrays::DecimalArray; use crate::arrays::decimal::DecimalArrayExt; -use crate::dtype::DecimalType; pub(super) fn decimal_uncompressed_size_in_bytes( array: &DecimalArray, @@ -17,10 +16,8 @@ pub(super) fn decimal_uncompressed_size_in_bytes( let value_size = u64::try_from(array.len()) .map_err(|e| vortex_err!("Failed to convert decimal array length to u64: {e}"))? .checked_mul( - u64::try_from( - DecimalType::smallest_decimal_value_type(&array.decimal_dtype()).byte_width(), - ) - .map_err(|e| vortex_err!("Failed to convert decimal byte width to u64: {e}"))?, + u64::try_from(array.decimal_dtype().canonical_values_type().byte_width()) + .map_err(|e| vortex_err!("Failed to convert decimal byte width to u64: {e}"))?, ) .ok_or_else(|| vortex_err!("uncompressed size in bytes overflowed u64"))?; let validity_size = validity_uncompressed_size_in_bytes( diff --git a/vortex-array/src/aggregate_fn/fns/uncompressed_size_in_bytes/list_view.rs b/vortex-array/src/aggregate_fn/fns/uncompressed_size_in_bytes/list_view.rs index 8abf0586396..1ebf6373c5f 100644 --- a/vortex-array/src/aggregate_fn/fns/uncompressed_size_in_bytes/list_view.rs +++ b/vortex-array/src/aggregate_fn/fns/uncompressed_size_in_bytes/list_view.rs @@ -17,7 +17,6 @@ use crate::arrays::listview::ListViewArraySlotsExt; use crate::arrays::listview::ListViewRebuildMode; use crate::arrays::primitive::PrimitiveArrayExt; use crate::dtype::DType; -use crate::dtype::DecimalType; use crate::dtype::IntegerPType; use crate::match_each_integer_ptype; @@ -100,7 +99,7 @@ fn referenced_elements_size( } DType::Decimal(decimal, _) => checked_len_mul( referenced.count, - DecimalType::smallest_decimal_value_type(decimal).byte_width(), + decimal.canonical_values_type().byte_width(), "decimal", )?, _ => { 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..8e4b75aee70 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 @@ -53,7 +53,6 @@ use crate::arrays::ListView; use crate::arrays::map::MapArraySlotsExt; use crate::arrays::varbinview::BinaryView; use crate::dtype::DType; -use crate::dtype::DecimalType; use crate::dtype::Nullability::NonNullable; use crate::dtype::PType; use crate::expr::stats::Precision; @@ -362,7 +361,7 @@ pub(crate) fn constant_uncompressed_size_in_bytes( } DType::Decimal(decimal_type, _) => checked_len_mul( array.len(), - DecimalType::smallest_decimal_value_type(decimal_type).byte_width(), + decimal_type.canonical_values_type().byte_width(), "decimal", )?, DType::Utf8(_) => constant_varbinview_value_size( @@ -525,8 +524,8 @@ mod tests { /// The size the array occupies once rebuilt through the canonical builders, which is the /// layout [`UncompressedSizeInBytes`] is defined against: the builders normalize physical - /// widths that the input array is free to choose differently, picking the smallest decimal - /// value type for a precision and `u64` list-view offsets and sizes. + /// widths that the input array is free to choose differently, picking the canonical decimal + /// width of a precision and `u64` list-view offsets and sizes. /// /// Builders no longer canonicalize their children, so the finished array is only canonical at /// the top level - recursively canonicalize it before measuring. diff --git a/vortex-array/src/array/haystack/mod.rs b/vortex-array/src/array/haystack/mod.rs index 828d6f6f03d..5e9065b7113 100644 --- a/vortex-array/src/array/haystack/mod.rs +++ b/vortex-array/src/array/haystack/mod.rs @@ -31,7 +31,6 @@ use crate::arrays::decimal::converted_buffer; use crate::arrays::primitive::PrimitiveArrayExt; use crate::arrays::varbinview::BinaryView; use crate::dtype::DType; -use crate::dtype::DecimalType; use crate::dtype::NativeDecimalType; use crate::dtype::NativePType; use crate::dtype::PType; @@ -116,7 +115,7 @@ fn new_state(elements: ArrayRef, ctx: &mut ExecutionCtx) -> VortexResult { - let values_type = DecimalType::smallest_decimal_value_type(decimal); + let values_type = decimal.canonical_values_type(); let elements = elements.execute::(ctx)?; let all_valid = Mask::new_true(elements.len()); match_each_decimal_value_type!(values_type, |T| { diff --git a/vortex-array/src/arrays/arbitrary.rs b/vortex-array/src/arrays/arbitrary.rs index 810157eaf41..f5035fad8ca 100644 --- a/vortex-array/src/arrays/arbitrary.rs +++ b/vortex-array/src/arrays/arbitrary.rs @@ -143,7 +143,7 @@ fn random_array_chunk( }, d @ DType::Decimal(decimal, n) => { let elem_len = chunk_len.unwrap_or(u.int_in_range(0..=20)?); - match_each_decimal_value_type!(DecimalType::smallest_decimal_value_type(decimal), |D| { + match_each_decimal_value_type!(decimal.canonical_values_type(), |D| { let mut builder = DecimalBuilder::new_in::( *decimal, *n, diff --git a/vortex-array/src/arrays/constant/vtable/canonical.rs b/vortex-array/src/arrays/constant/vtable/canonical.rs index bbea85c4077..0d82cc96058 100644 --- a/vortex-array/src/arrays/constant/vtable/canonical.rs +++ b/vortex-array/src/arrays/constant/vtable/canonical.rs @@ -40,7 +40,6 @@ use crate::builders::ArrayBuilder; use crate::builders::VarBinViewBuilder; 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; @@ -94,7 +93,7 @@ pub(crate) fn constant_canonicalize( }) } DType::Decimal(decimal_type, ..) => { - let size = DecimalType::smallest_decimal_value_type(decimal_type); + let size = decimal_type.canonical_values_type(); let decimal = scalar.as_decimal(); let Some(value) = decimal.decimal_value() else { let all_null = match_each_decimal_value_type!(size, |D| { @@ -353,7 +352,7 @@ fn flat_elements( .into_array() }), DType::Decimal(decimal, nullability) => { - let values_type = DecimalType::smallest_decimal_value_type(decimal); + let values_type = decimal.canonical_values_type(); match_each_decimal_value_type!(values_type, |T| { let mut buffer = BufferMut::::with_capacity_in(elements.len(), allocator.clone()); diff --git a/vortex-array/src/arrays/decimal/compute/cast.rs b/vortex-array/src/arrays/decimal/compute/cast.rs index 29b38ce9406..c8f561af0b7 100644 --- a/vortex-array/src/arrays/decimal/compute/cast.rs +++ b/vortex-array/src/arrays/decimal/compute/cast.rs @@ -152,7 +152,7 @@ impl CastKernel for Decimal { ConstantArray::new(Scalar::null(dtype.clone()), array.len()).into_array(), )); } - let target_values_type = DecimalType::smallest_decimal_value_type(to_decimal_dtype); + let target_values_type = to_decimal_dtype.canonical_values_type(); match_each_decimal_value_type!(array.values_type(), |F| { match_each_decimal_value_type!(target_values_type, |T| { diff --git a/vortex-array/src/arrays/decimal/utils.rs b/vortex-array/src/arrays/decimal/utils.rs index 30b271c2759..f1fb8716e5b 100644 --- a/vortex-array/src/arrays/decimal/utils.rs +++ b/vortex-array/src/arrays/decimal/utils.rs @@ -4,13 +4,20 @@ use itertools::Itertools; use itertools::MinMaxResult; use vortex_buffer::Buffer; +use vortex_buffer::BufferMut; +use vortex_compute::lane_kernels::IndexedSourceExt; use vortex_error::VortexExpect; use vortex_error::VortexResult; use vortex_error::vortex_bail; +use vortex_error::vortex_ensure_eq; +use vortex_mask::AllOr; use vortex_mask::Mask; use crate::arrays::DecimalArray; use crate::arrays::decimal::DecimalArrayExt; +use crate::buffer::BufferHandle; +use crate::dtype::BigCast; +use crate::dtype::DecimalDType; use crate::dtype::DecimalType; use crate::dtype::NativeDecimalType; use crate::dtype::i256; @@ -104,10 +111,151 @@ pub fn converted_buffer( }) } +/// Copies unscaled decimal values into a new buffer at the canonical width of `target`. +/// +/// The copy holds `values.len()` values of [`DecimalDType::canonical_values_type`]. It never +/// rescales, so the values keep the scale of the source. +/// +/// - A narrowing copy, where the canonical width is narrower than `F`, checks every valid value +/// against the precision of `target` with [`check_precision`]. A value that fits the width but +/// not the precision fails too. Null slots of the copy hold zeros. +/// - A widening or same-width copy checks nothing and keeps the null slots. The caller **must** +/// know that every valid value fits `target`, for example because `target` has at least the +/// precision of the source dtype. Otherwise the copy holds values outside the precision, which +/// is an incorrect result but not undefined behavior. A caller that owns a buffer that already +/// has the canonical width can keep it and call [`check_precision`] instead. +/// +/// # Errors +/// +/// Returns an error if `validity` and `values` have different lengths, or if a narrowing copy +/// finds a valid value outside the precision of `target`, as [`check_precision`] describes. +pub fn copy_to_canonical_width( + values: &[F], + validity: &Mask, + target: DecimalDType, +) -> VortexResult { + vortex_ensure_eq!(validity.len(), values.len()); + + let canonical_type = target.canonical_values_type(); + if F::DECIMAL_TYPE == canonical_type { + return Ok(BufferHandle::new_host( + Buffer::::copy_from(values).into_byte_buffer(), + )); + } + + let copy = match_each_decimal_value_type!(canonical_type, |W| { + if const { (F::DECIMAL_TYPE as u8) <= (W::DECIMAL_TYPE as u8) } { + widen_values::(values).into_byte_buffer() + } else { + check_precision(values, validity, target)?; + narrow_checked_values::(values, validity).into_byte_buffer() + } + }); + + Ok(BufferHandle::new_host(copy)) +} + +/// Widens every value to `W`, which **must** be at least as wide as `F`. Null slots keep their +/// values. +fn widen_values(values: &[F]) -> Buffer { + values + .iter() + .map(|&value| { + ::from(value) + .vortex_expect("W is at least as wide as F, so every value of F fits W") + }) + .collect() +} + +/// Narrows values that [`check_precision`] accepted to `W`, and writes zeros into the null slots. +fn narrow_checked_values( + values: &[F], + validity: &Mask, +) -> Buffer { + let len = values.len(); + let valid_bits = match validity.bit_buffer() { + AllOr::All => None, + AllOr::None => return Buffer::zeroed(len), + AllOr::Some(valid_bits) => Some(valid_bits), + }; + + // NB: a null slot can hold a value that does not fit `W`, so the map must not fail on it. + // `map_masked_into` then overwrites every null slot with zero. + let narrow = |value: F| ::from(value).unwrap_or_default(); + + let mut buffer = BufferMut::::with_capacity(len); + let out = &mut buffer.spare_capacity_mut()[..len]; + match valid_bits { + None => values.map_into(out, narrow), + Some(valid_bits) => values.map_masked_into(valid_bits, out, narrow), + } + + // SAFETY: `map_into` and `map_masked_into` write every lane of `out`. + unsafe { buffer.set_len(len) }; + + buffer.freeze() +} + +/// Checks that every valid value fits the precision of `decimal_dtype`. +/// +/// Null slots are not checked, so they can hold any value. The check takes a materialized +/// [`Mask`], so it needs no execution context. +/// +/// # Errors +/// +/// Returns an error if `validity` and `values` have different lengths, or if a valid value is +/// outside the precision of `decimal_dtype`. The error names the first such row, its value and +/// `decimal_dtype`. +pub fn check_precision( + values: &[T], + validity: &Mask, + decimal_dtype: DecimalDType, +) -> VortexResult<()> { + vortex_ensure_eq!(validity.len(), values.len()); + + // The bound tables stop at `T::MAX_PRECISION`. Every value of `T` has at most + // `T::MAX_PRECISION + 1` digits, so a larger precision holds every value of `T`. + let precision = decimal_dtype.precision(); + if precision > T::MAX_PRECISION { + return Ok(()); + } + let min = T::MIN_BY_PRECISION[usize::from(precision)]; + let max = T::MAX_BY_PRECISION[usize::from(precision)]; + let is_outside = |value: T| (value < min) | (value > max); + + let valid_bits = match validity.bit_buffer() { + AllOr::All => None, + AllOr::None => return Ok(()), + AllOr::Some(valid_bits) => Some(valid_bits), + }; + + // The fold is branchless so that it vectorizes. It also reads the null slots, so a hit only + // means that the rescan below must look for a valid row outside the precision. + let any_outside = values + .iter() + .fold(false, |acc, &value| acc | is_outside(value)); + if !any_outside { + return Ok(()); + } + + let first_outside = match valid_bits { + None => values.iter().position(|&value| is_outside(value)), + Some(valid_bits) => valid_bits + .set_indices() + .find(|&row| is_outside(values[row])), + }; + if let Some(row) = first_outside { + vortex_bail!( + "decimal value at row {row} must be within the precision of {decimal_dtype}, got {}", + values[row] + ); + } + + Ok(()) +} + macro_rules! try_downcast { ($array:expr, from: $src:ty, to: $($dst:ty),*) => {{ - use crate::dtype::BigCast; - // Collect the min/max of the values let minmax = $array.buffer::<$src>().iter().copied().minmax(); match minmax { @@ -167,14 +315,53 @@ mod tests { use vortex_error::VortexResult; use vortex_mask::Mask; + use super::check_precision; use super::converted_buffer; + use super::copy_to_canonical_width; use crate::arrays::DecimalArray; use crate::dtype::BigCast; use crate::dtype::DecimalDType; use crate::dtype::DecimalType; + use crate::dtype::NativeDecimalType; + use crate::dtype::ToI256; + use crate::dtype::i256; use crate::match_each_decimal_value_type; use crate::validity::Validity; + /// Runs [`copy_to_canonical_width`], checks that the copy has the canonical width of `target`, + /// and reads the copy back as `i256`. + fn copy_as_i256( + values: &[F], + validity: &Mask, + target: DecimalDType, + ) -> VortexResult> { + let copy = copy_to_canonical_width(values, validity, target)?; + let width = target.canonical_values_type(); + assert_eq!(copy.len(), values.len() * width.byte_width()); + + Ok(match_each_decimal_value_type!(width, |W| { + Buffer::::from_byte_buffer(copy.unwrap_host()) + .iter() + .map(|value| value.to_i256().vortex_expect("decimal values fit i256")) + .collect() + })) + } + + /// Returns `10^precision`, the smallest magnitude outside `precision`. + fn ten_to_the(precision: u8) -> i256 { + i256::from_i128(10) + .checked_pow(precision.into()) + .vortex_expect("10^76 fits i256") + } + + /// Converts each test value to the native type `T`. + fn to_native(values: &[i256]) -> Vec { + values + .iter() + .map(|&value| ::from(value).vortex_expect("test value fits the type")) + .collect() + } + /// Each pair selects a different widening or narrowing conversion. #[rstest] fn converted_buffer_every_storage_pair( @@ -215,4 +402,145 @@ mod tests { }); Ok(()) } + + #[rstest] + #[case::i8_to_i16(DecimalType::I8, 3)] + #[case::i16_to_i32(DecimalType::I16, 5)] + #[case::i32_to_i64(DecimalType::I32, 10)] + #[case::i64_to_i128(DecimalType::I64, 19)] + #[case::i128_to_i256(DecimalType::I128, 39)] + #[case::i8_to_i256(DecimalType::I8, 76)] + #[case::i64_same_width(DecimalType::I64, 18)] + fn test_copy_to_canonical_width_widens( + #[case] source: DecimalType, + #[case] target_precision: u8, + ) -> VortexResult<()> { + let target = DecimalDType::new(target_precision, 2); + match_each_decimal_value_type!(source, |F| { + // The extremes of the largest precision that `F` holds, and zero. + let limit = ten_to_the(F::MAX_PRECISION) - i256::ONE; + let expected = [-limit, i256::ZERO, limit]; + let values = to_native::(&expected); + assert_eq!(copy_as_i256(&values, &Mask::new_true(3), target)?, expected); + }); + Ok(()) + } + + /// Each case puts `10^p` inside the target width, so only the precision check can reject it. + #[rstest] + #[case::i16_to_i8(DecimalType::I16, 2)] + #[case::i32_to_i16(DecimalType::I32, 4)] + #[case::i64_to_i32(DecimalType::I64, 9)] + #[case::i128_to_i64(DecimalType::I128, 18)] + #[case::i256_to_i128(DecimalType::I256, 38)] + #[case::i256_to_i8(DecimalType::I256, 1)] + fn test_copy_to_canonical_width_narrowing_boundaries( + #[case] source: DecimalType, + #[case] target_precision: u8, + ) -> VortexResult<()> { + let target = DecimalDType::new(target_precision, 0); + let limit = ten_to_the(target_precision); + let inside = [-(limit - i256::ONE), limit - i256::ONE]; + let outside = [-limit, limit]; + + match_each_decimal_value_type!(source, |F| { + let values = to_native::(&inside); + assert_eq!(copy_as_i256(&values, &Mask::new_true(2), target)?, inside); + + for value in to_native::(&outside) { + assert!(copy_to_canonical_width(&[value], &Mask::new_true(1), target).is_err()); + } + }); + Ok(()) + } + + #[test] + fn test_copy_to_canonical_width_zeros_null_slots() -> VortexResult<()> { + // Row 1 holds a null value outside the `i32` width, and row 3 a null value inside the + // width but outside the precision. + let values = [12_345i128, 10i128.pow(30), -99_999, 100_000]; + let validity = Mask::from_iter([true, false, true, false]); + + let copy = copy_as_i256(&values, &validity, DecimalDType::new(5, 2))?; + assert_eq!(copy, [12_345, 0, -99_999, 0].map(i256::from_i128)); + Ok(()) + } + + #[test] + fn test_copy_to_canonical_width_error_names_row() { + // Row 1 is a null slot outside the precision, so row 2 is the first failing valid row. + let values = [1i64, 1_000_000, 1_000, 5_000]; + let validity = Mask::from_iter([true, false, true, true]); + + let error = copy_to_canonical_width(&values, &validity, DecimalDType::new(3, 0)) + .expect_err("row 2 is outside the precision") + .to_string(); + assert!(error.contains("row 2"), "{error}"); + assert!(error.contains("got 1000"), "{error}"); + assert!(error.contains("decimal(3,0)"), "{error}"); + } + + #[test] + fn test_copy_to_canonical_width_all_null() -> VortexResult<()> { + let values = [10i128.pow(30), i128::MIN, 7]; + let copy = copy_as_i256(&values, &Mask::new_false(3), DecimalDType::new(5, 2))?; + assert_eq!(copy, [i256::ZERO; 3]); + Ok(()) + } + + /// Precision 4 narrows the `i32` values, and precision 18 widens them. + #[rstest] + fn test_copy_to_canonical_width_length_mismatch(#[values(4, 18)] target_precision: u8) { + let target = DecimalDType::new(target_precision, 0); + let result = copy_to_canonical_width(&[1i32, 2, 3], &Mask::new_true(2), target); + assert!(result.is_err()); + } + + /// Each case puts `10^p` inside the width of the values, so only the precision rejects it. + #[rstest] + #[case::i8(DecimalType::I8, 2)] + #[case::i16(DecimalType::I16, 4)] + #[case::i32(DecimalType::I32, 9)] + #[case::i64(DecimalType::I64, 18)] + #[case::i128(DecimalType::I128, 38)] + #[case::i256(DecimalType::I256, 76)] + #[case::i128_below_max_precision(DecimalType::I128, 5)] + fn test_check_precision_boundaries( + #[case] values_type: DecimalType, + #[case] precision: u8, + ) -> VortexResult<()> { + let decimal_dtype = DecimalDType::new(precision, 0); + let limit = ten_to_the(precision); + let inside = [-(limit - i256::ONE), limit - i256::ONE]; + let outside = [-limit, limit]; + + match_each_decimal_value_type!(values_type, |T| { + check_precision(&to_native::(&inside), &Mask::new_true(2), decimal_dtype)?; + + for value in to_native::(&outside) { + assert!(check_precision(&[value], &Mask::new_true(1), decimal_dtype).is_err()); + } + }); + Ok(()) + } + + /// Every value of a type has at most one digit more than its `MAX_PRECISION`. + #[test] + fn test_check_precision_above_max_precision_of_values_type() -> VortexResult<()> { + let all_valid = Mask::new_true(2); + check_precision(&[i8::MIN, i8::MAX], &all_valid, DecimalDType::new(3, 0))?; + check_precision(&[i64::MIN, i64::MAX], &all_valid, DecimalDType::new(19, 0))?; + Ok(()) + } + + #[test] + fn test_check_precision_ignores_null_slots() -> VortexResult<()> { + let values = [5i32, 1_000_000_000, -7]; + let validity = Mask::from_iter([true, false, true]); + let decimal_dtype = DecimalDType::new(4, 0); + + check_precision(&values, &validity, decimal_dtype)?; + check_precision(&values, &Mask::new_false(3), decimal_dtype)?; + Ok(()) + } } diff --git a/vortex-array/src/arrays/primitive/compute/cast.rs b/vortex-array/src/arrays/primitive/compute/cast.rs index eb9a461c8b0..af2ad86cf2f 100644 --- a/vortex-array/src/arrays/primitive/compute/cast.rs +++ b/vortex-array/src/arrays/primitive/compute/cast.rs @@ -143,7 +143,7 @@ fn cast_to_decimal( let validity = source_validity .clone() .cast_nullability(nullability, array.len(), ctx)?; - let values_type = DecimalType::smallest_decimal_value_type(&decimal_dtype); + let values_type = decimal_dtype.canonical_values_type(); if decimal_dtype.scale() == 0 && signed_primitive_decimal_type(array.ptype()) == Some(values_type) @@ -1058,10 +1058,7 @@ mod test { .cast(DType::Decimal(decimal_dtype, Nullability::NonNullable))? .execute::(&mut ctx)?; - assert_eq!( - casted.values_type(), - DecimalType::smallest_decimal_value_type(&decimal_dtype) - ); + assert_eq!(casted.values_type(), decimal_dtype.canonical_values_type()); match_each_decimal_value_type!(casted.values_type(), |T| { let expected = [10i8, 3].map(::from); let actual = casted diff --git a/vortex-array/src/builders/mod.rs b/vortex-array/src/builders/mod.rs index 480c3f6a701..e87c78d800b 100644 --- a/vortex-array/src/builders/mod.rs +++ b/vortex-array/src/builders/mod.rs @@ -412,17 +412,14 @@ pub fn builder_with_capacity_in( }) } DType::Decimal(decimal_type, n) => { - match_each_decimal_value_type!( - DecimalType::smallest_decimal_value_type(decimal_type), - |D| { - Box::new(DecimalBuilder::with_capacity_in::( - capacity, - *decimal_type, - *n, - allocator, - )) - } - ) + match_each_decimal_value_type!(decimal_type.canonical_values_type(), |D| { + Box::new(DecimalBuilder::with_capacity_in::( + capacity, + *decimal_type, + *n, + allocator, + )) + }) } DType::Utf8(n) => Box::new(VarBinViewBuilder::with_capacity_in( DType::Utf8(*n), diff --git a/vortex-array/src/canonical.rs b/vortex-array/src/canonical.rs index d154bbc3954..e4160d76c02 100644 --- a/vortex-array/src/canonical.rs +++ b/vortex-array/src/canonical.rs @@ -184,18 +184,15 @@ impl Canonical { }) } DType::Decimal(decimal_type, n) => { - match_each_decimal_value_type!( - DecimalType::smallest_decimal_value_type(decimal_type), - |D| { - Canonical::Decimal(unsafe { - DecimalArray::new_unchecked::( - Buffer::empty(), - *decimal_type, - Validity::from(n), - ) - }) - } - ) + match_each_decimal_value_type!(decimal_type.canonical_values_type(), |D| { + Canonical::Decimal(unsafe { + DecimalArray::new_unchecked::( + Buffer::empty(), + *decimal_type, + Validity::from(n), + ) + }) + }) } DType::Utf8(n) => Canonical::VarBinView(unsafe { VarBinViewArray::new_unchecked( diff --git a/vortex-array/src/dtype/decimal/mod.rs b/vortex-array/src/dtype/decimal/mod.rs index 10396eab24d..0d442d6bd8a 100644 --- a/vortex-array/src/dtype/decimal/mod.rs +++ b/vortex-array/src/dtype/decimal/mod.rs @@ -100,6 +100,22 @@ impl DecimalDType { self.scale } + /// The width of the values buffer of a canonical decimal array with this precision. + /// + /// This is the narrowest [`DecimalType`] that holds every unscaled value of the precision, so + /// each decimal dtype has exactly one canonical values type. The precision is always between 1 + /// and [`MAX_PRECISION`], so the last arm covers precision 39 to 76. + pub const fn canonical_values_type(&self) -> DecimalType { + match self.precision.get() { + 1..=2 => DecimalType::I8, + 3..=4 => DecimalType::I16, + 5..=9 => DecimalType::I32, + 10..=18 => DecimalType::I64, + 19..=38 => DecimalType::I128, + _ => DecimalType::I256, + } + } + /// Return the max number of bits required to fit a decimal with `precision` in. pub fn required_bit_width(&self) -> usize { (self.precision.get() as f32 * 10.0f32.log(2.0)) @@ -137,6 +153,8 @@ impl TryFrom for DecimalDType { #[cfg(test)] mod tests { + use rstest::rstest; + use super::*; use crate::dtype::DType; use crate::dtype::Nullability; @@ -280,6 +298,28 @@ mod tests { assert_eq!(bits, 253); } + #[rstest] + #[case::p1(1, DecimalType::I8)] + #[case::p2(2, DecimalType::I8)] + #[case::p3(3, DecimalType::I16)] + #[case::p4(4, DecimalType::I16)] + #[case::p5(5, DecimalType::I32)] + #[case::p9(9, DecimalType::I32)] + #[case::p10(10, DecimalType::I64)] + #[case::p18(18, DecimalType::I64)] + #[case::p19(19, DecimalType::I128)] + #[case::p38(38, DecimalType::I128)] + #[case::p39(39, DecimalType::I256)] + #[case::p76(76, DecimalType::I256)] + fn test_values_type_boundaries( + #[case] precision: u8, + #[case] expected: DecimalType, + ) -> VortexResult<()> { + let decimal = DecimalDType::try_new(precision, 0)?; + assert_eq!(decimal.canonical_values_type(), expected); + Ok(()) + } + #[test] fn test_try_from_dtype() { let decimal = DecimalDType::try_new(10, 2).unwrap(); diff --git a/vortex-array/src/dtype/decimal/types.rs b/vortex-array/src/dtype/decimal/types.rs index f83a3918a86..aa99bacd6b2 100644 --- a/vortex-array/src/dtype/decimal/types.rs +++ b/vortex-array/src/dtype/decimal/types.rs @@ -42,20 +42,6 @@ pub enum DecimalType { } impl DecimalType { - /// Maps a `DecimalDType` (precision) into the smallest `DecimalType` that can represent it. - pub fn smallest_decimal_value_type(decimal_dtype: &DecimalDType) -> DecimalType { - match decimal_dtype.precision() { - 1..=2 => DecimalType::I8, - 3..=4 => DecimalType::I16, - 5..=9 => DecimalType::I32, - 10..=18 => DecimalType::I64, - 19..=38 => DecimalType::I128, - 39..=76 => DecimalType::I256, - 0 => unreachable!("precision must be greater than 0"), - p => unreachable!("precision larger than 76 is invalid found precision {p}"), - } - } - /// Returns the size in bytes of the underlying native type for this decimal type. pub fn byte_width(&self) -> usize { match self { @@ -70,7 +56,7 @@ impl DecimalType { /// True if `Self` can represent every value of the type `DecimalDType`. pub fn is_compatible_decimal_value_type(self, dtype: DecimalDType) -> bool { - self >= Self::smallest_decimal_value_type(&dtype) + self >= dtype.canonical_values_type() } } diff --git a/vortex-array/src/dtype/dtype_impl.rs b/vortex-array/src/dtype/dtype_impl.rs index 27d083edff8..3af9c558dc3 100644 --- a/vortex-array/src/dtype/dtype_impl.rs +++ b/vortex-array/src/dtype/dtype_impl.rs @@ -21,7 +21,6 @@ use crate::dtype::PType; use crate::dtype::StructFields; use crate::dtype::UnionVariants; use crate::dtype::decimal::DecimalDType; -use crate::dtype::decimal::DecimalType; use crate::dtype::extension::ExtDTypeRef; use crate::dtype::nullability::Nullability; @@ -316,9 +315,7 @@ impl DType { Null => Some(0), Bool(_) => Some(1), Primitive(ptype, _) => Some(ptype.byte_width()), - Decimal(decimal, _) => { - Some(DecimalType::smallest_decimal_value_type(decimal).byte_width()) - } + Decimal(decimal, _) => Some(decimal.canonical_values_type().byte_width()), Utf8(_) | Binary(_) | List(..) | Map(..) => None, FixedSizeList(elem_dtype, list_size, _) => { elem_dtype.element_size().map(|s| s * *list_size as usize) diff --git a/vortex-array/src/scalar/arbitrary.rs b/vortex-array/src/scalar/arbitrary.rs index 844ef6f6397..8cb2ccdbb8f 100644 --- a/vortex-array/src/scalar/arbitrary.rs +++ b/vortex-array/src/scalar/arbitrary.rs @@ -160,14 +160,11 @@ fn random_pvalue(u: &mut Unstructured, ptype: &PType) -> Result { /// Returns an error if the underlying arbitrary generation fails. pub fn random_decimal(u: &mut Unstructured, decimal_type: &DecimalDType) -> Result { let precision = decimal_type.precision(); - let value = match_each_decimal_value_type!( - DecimalType::smallest_decimal_value_type(decimal_type), - |D| { - DecimalValue::from(u.int_in_range( - D::MIN_BY_PRECISION[precision as usize]..=D::MAX_BY_PRECISION[precision as usize], - )?) - } - ); + let value = match_each_decimal_value_type!(decimal_type.canonical_values_type(), |D| { + DecimalValue::from(u.int_in_range( + D::MIN_BY_PRECISION[precision as usize]..=D::MAX_BY_PRECISION[precision as usize], + )?) + }); Ok(ScalarValue::Decimal(value)) } diff --git a/vortex-array/src/scalar/typed_view/decimal/arithmetic.rs b/vortex-array/src/scalar/typed_view/decimal/arithmetic.rs index ec7b57f538c..aa6217818ec 100644 --- a/vortex-array/src/scalar/typed_view/decimal/arithmetic.rs +++ b/vortex-array/src/scalar/typed_view/decimal/arithmetic.rs @@ -106,7 +106,7 @@ pub(crate) fn checked_decimal_numeric( op: NumericOperator, ) -> Option { let work = decimal_numeric_work_dtype(input, result, op); - match_each_decimal_value_type!(DecimalType::smallest_decimal_value_type(&work), |W| { + match_each_decimal_value_type!(work.canonical_values_type(), |W| { let value = checked_at_width::(lhs.cast::()?, rhs.cast::()?, result.scale(), op)?; DecimalValue::from(value).normalize(result) }) diff --git a/vortex-array/src/scalar/typed_view/decimal/dvalue.rs b/vortex-array/src/scalar/typed_view/decimal/dvalue.rs index 5fe5a353e7c..7525bd4aaad 100644 --- a/vortex-array/src/scalar/typed_view/decimal/dvalue.rs +++ b/vortex-array/src/scalar/typed_view/decimal/dvalue.rs @@ -183,9 +183,7 @@ impl DecimalValue { /// Returns the 0 value given the [`DecimalType`]. pub fn zero(decimal_type: &DecimalDType) -> Self { - let smallest_type = DecimalType::smallest_decimal_value_type(decimal_type); - - match smallest_type { + match decimal_type.canonical_values_type() { DecimalType::I8 => DecimalValue::I8(0), DecimalType::I16 => DecimalValue::I16(0), DecimalType::I32 => DecimalValue::I32(0), @@ -206,7 +204,7 @@ impl DecimalValue { /// Checks the dtype precision and returns the value in the dtype's native storage width. pub(crate) fn normalize(&self, decimal_type: DecimalDType) -> Option { - let value_type = DecimalType::smallest_decimal_value_type(&decimal_type); + let value_type = decimal_type.canonical_values_type(); match_each_decimal_value_type!(value_type, |T| { let value = self.cast::()?; // `T` is the narrowest type covering `precision`, so both tables index in bounds. diff --git a/vortex-array/src/scalar_fn/fns/binary/numeric/decimal.rs b/vortex-array/src/scalar_fn/fns/binary/numeric/decimal.rs index 8742ea66a93..0573cc303a7 100644 --- a/vortex-array/src/scalar_fn/fns/binary/numeric/decimal.rs +++ b/vortex-array/src/scalar_fn/fns/binary/numeric/decimal.rs @@ -49,7 +49,6 @@ use crate::arrays::decimal::widened_buffer; use crate::dtype::BigCast; use crate::dtype::DType; use crate::dtype::DecimalDType; -use crate::dtype::DecimalType; use crate::dtype::NativeDecimalType; use crate::match_each_decimal_value_type; use crate::scalar::DecimalValue; @@ -95,7 +94,7 @@ pub(super) fn execute_numeric_decimal( let valid_rows = validity.execute_mask(len, ctx)?; let work_dtype = decimal_numeric_work_dtype(*decimal_dtype, result_decimal_dtype, op); - match_each_decimal_value_type!(DecimalType::smallest_decimal_value_type(&work_dtype), |W| { + match_each_decimal_value_type!(work_dtype.canonical_values_type(), |W| { let constants = DecimalOpConstants::::new(result_decimal_dtype, op)?; macro_rules! execute_typed { ($Op:ty) => { @@ -410,7 +409,7 @@ fn decimal_array_narrowed( validity: Validity, allocator: &BufferAllocatorRef, ) -> ArrayRef { - let target = DecimalType::smallest_decimal_value_type(&decimal_dtype); + let target = decimal_dtype.canonical_values_type(); if target == W::DECIMAL_TYPE { return DecimalArray::new(values, decimal_dtype, validity).into_array(); } diff --git a/vortex-arrow/src/convert.rs b/vortex-arrow/src/convert.rs index 0dbdc822a2d..2d119b90a5f 100644 --- a/vortex-arrow/src/convert.rs +++ b/vortex-arrow/src/convert.rs @@ -82,12 +82,16 @@ use vortex_array::arrays::StructArray; use vortex_array::arrays::TemporalArray; use vortex_array::arrays::VarBinArray; use vortex_array::arrays::VarBinViewArray; +use vortex_array::arrays::decimal::check_precision; +use vortex_array::arrays::decimal::copy_to_canonical_width; use vortex_array::arrays::struct_::StructArrayExt; +use vortex_array::buffer::BufferHandle; use vortex_array::dtype::DType; use vortex_array::dtype::DecimalDType; use vortex_array::dtype::FieldNames; use vortex_array::dtype::IntegerPType; use vortex_array::dtype::MapDType; +use vortex_array::dtype::NativeDecimalType; use vortex_array::dtype::NativePType; use vortex_array::dtype::Nullability; use vortex_array::dtype::PType; @@ -104,6 +108,7 @@ use vortex_error::vortex_ensure; use vortex_error::vortex_ensure_eq; use vortex_error::vortex_err; use vortex_error::vortex_panic; +use vortex_mask::Mask; use crate::dtype::from_arrow_time_unit; @@ -176,52 +181,125 @@ where Ok(PrimitiveArray::new(buffer, validity).into_array()) } -/// Zero-copy conversion of an Arrow `Decimal32` array into a Vortex decimal array. +/// Conversion of an Arrow `Decimal32` array into a Vortex decimal array, zero-copy when the Arrow +/// width is the [canonical width](DecimalDType::canonical_values_type), otherwise a checked copy. pub fn from_arrow_decimal32( array: &ArrowPrimitiveArray, nullable: bool, ) -> VortexResult { - 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()) + let values = Buffer::from_arrow_scalar_buffer(array.values().clone()); + from_arrow_decimal( + values, + array.nulls(), + array.precision(), + array.scale(), + nullable, + ) } -/// Zero-copy conversion of an Arrow `Decimal64` array into a Vortex decimal array. +/// Conversion of an Arrow `Decimal64` array into a Vortex decimal array, zero-copy when the Arrow +/// width is the [canonical width](DecimalDType::canonical_values_type), otherwise a checked copy. pub fn from_arrow_decimal64( array: &ArrowPrimitiveArray, nullable: bool, ) -> VortexResult { - 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()) + let values = Buffer::from_arrow_scalar_buffer(array.values().clone()); + from_arrow_decimal( + values, + array.nulls(), + array.precision(), + array.scale(), + nullable, + ) } -/// Zero-copy conversion of an Arrow `Decimal128` array into a Vortex decimal array. +/// Conversion of an Arrow `Decimal128` array into a Vortex decimal array, zero-copy when the Arrow +/// width is the [canonical width](DecimalDType::canonical_values_type), otherwise a checked copy. pub fn from_arrow_decimal128( array: &ArrowPrimitiveArray, nullable: bool, ) -> VortexResult { - 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()) + let values = Buffer::from_arrow_scalar_buffer(array.values().clone()); + from_arrow_decimal( + values, + array.nulls(), + array.precision(), + array.scale(), + nullable, + ) } -/// Zero-copy conversion of an Arrow `Decimal256` array into a Vortex decimal array. +/// Conversion of an Arrow `Decimal256` array into a Vortex decimal array, zero-copy when the Arrow +/// width is the [canonical width](DecimalDType::canonical_values_type), otherwise a checked copy. pub fn from_arrow_decimal256( array: &ArrowPrimitiveArray, nullable: bool, ) -> VortexResult { - let decimal_type = DecimalDType::new(array.precision(), array.scale()); - let buffer = Buffer::from_arrow_scalar_buffer(array.values().clone()); + let values = Buffer::from_arrow_scalar_buffer(array.values().clone()); // SAFETY: Our i256 implementation has the same bit-pattern representation of the // arrow_buffer::i256 type. It is safe to treat values held inside the buffer as values // 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()) + let values = unsafe { std::mem::transmute::, Buffer>(values) }; + from_arrow_decimal( + values, + array.nulls(), + array.precision(), + array.scale(), + nullable, + ) +} + +/// Converts the values of an Arrow decimal array into a Vortex decimal array at the canonical width +/// of its precision. +/// +/// arrow-rs does not check that the values of an array fit its precision, so this function checks +/// every valid value. When `T` is the canonical width, the array keeps the Arrow buffer and +/// [`check_precision`] checks it. Otherwise [`copy_to_canonical_width`] copies the values, which +/// writes zeros into the null slots when it narrows. Null slots are never checked. +/// +/// # Errors +/// +/// Returns an error if the precision or scale is invalid, if `nullable` is false and the array has +/// nulls, or if a valid value is outside the precision. +fn from_arrow_decimal( + values: Buffer, + arrow_nulls: Option<&NullBuffer>, + precision: u8, + scale: i8, + nullable: bool, +) -> VortexResult { + let decimal_dtype = DecimalDType::try_new(precision, scale)?; + let validity = nulls(arrow_nulls, nullable)?; + let mask = arrow_nulls.map_or_else( + || Mask::new_true(values.len()), + |arrow_nulls| Mask::from_buffer(BitBuffer::from(arrow_nulls.inner().clone())), + ); + + let values_type = decimal_dtype.canonical_values_type(); + let checked_values = if T::DECIMAL_TYPE == values_type { + check_precision(values.as_slice(), &mask, decimal_dtype) + .map(|()| BufferHandle::new_host(values.into_byte_buffer())) + } else { + copy_to_canonical_width(values.as_slice(), &mask, decimal_dtype) + }; + let checked_values = checked_values.map_err(|err| { + err.with_context(format!( + "Arrow decimal import requires every valid value to be within the precision of \ + {decimal_dtype}; call arrow-rs PrimitiveArray::null_if_overflow_precision before \ + the import to turn such values into nulls" + )) + })?; + + // SAFETY: Every valid value is within the precision of `decimal_dtype`. `check_precision` + // checked the zero-copy values, and `copy_to_canonical_width` checks a narrowing copy. A + // widening copy needs no check: the canonical width is wider than `T` only when the precision + // is above `T::MAX_PRECISION`, and every value of `T` has at most `T::MAX_PRECISION + 1` + // digits. Arrow keeps its null buffer at the length of the values, so the validity matches. + let array = unsafe { + DecimalArray::new_unchecked_handle(checked_values, values_type, decimal_dtype, validity) + }; + + Ok(array.into_array()) } /// Conversion of an Arrow temporal array (timestamp/date/time) into a Vortex temporal @@ -755,6 +833,8 @@ mod tests { use arrow_array::BooleanArray; use arrow_array::Date32Array; use arrow_array::Date64Array; + use arrow_array::Decimal128Array; + use arrow_array::Decimal256Array; use arrow_array::FixedSizeListArray as ArrowFixedSizeListArray; use arrow_array::Float32Array; use arrow_array::Float64Array; @@ -796,6 +876,7 @@ mod tests { use arrow_array::types::Float16Type; use arrow_buffer::BooleanBuffer; use arrow_buffer::Buffer as ArrowBuffer; + use arrow_buffer::NullBuffer; use arrow_buffer::OffsetBuffer; use arrow_buffer::ScalarBuffer; use arrow_schema::DataType; @@ -830,6 +911,7 @@ mod tests { use vortex_array::assert_arrays_eq; use vortex_array::dtype::DType; use vortex_array::dtype::DecimalDType; + use vortex_array::dtype::DecimalType; use vortex_array::dtype::Nullability; use vortex_array::dtype::PType; use vortex_array::dtype::i256; @@ -1035,10 +1117,11 @@ mod tests { let vortex_array_non_null = from_arrow_dyn(&decimal_array_non_null, false).unwrap(); assert_eq!(vortex_array_non_null.len(), 3); - // Verify metadata - should be DecimalArray with correct precision and scale + // Verify metadata: a DecimalArray with the precision, scale and canonical width let decimal_vortex_array = vortex_array.as_::(); assert_eq!(decimal_vortex_array.decimal_dtype().precision(), 10); assert_eq!(decimal_vortex_array.decimal_dtype().scale(), 2); + assert_eq!(decimal_vortex_array.values_type(), DecimalType::I64); let decimal_vortex_array_non_null = vortex_array_non_null.as_::(); assert_eq!( @@ -1046,17 +1129,21 @@ mod tests { 10 ); assert_eq!(decimal_vortex_array_non_null.decimal_dtype().scale(), 2); + assert_eq!( + decimal_vortex_array_non_null.values_type(), + DecimalType::I64 + ); 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() ); } @@ -1085,10 +1172,11 @@ mod tests { let vortex_array_non_null = from_arrow_dyn(&decimal_array_non_null, false).unwrap(); assert_eq!(vortex_array_non_null.len(), 3); - // Verify metadata - should be DecimalArray with correct precision and scale + // Verify metadata: a DecimalArray with the precision, scale and canonical width let decimal_vortex_array = vortex_array.as_::(); assert_eq!(decimal_vortex_array.decimal_dtype().precision(), 38); assert_eq!(decimal_vortex_array.decimal_dtype().scale(), 10); + assert_eq!(decimal_vortex_array.values_type(), DecimalType::I128); let decimal_vortex_array_non_null = vortex_array_non_null.as_::(); assert_eq!( @@ -1096,31 +1184,145 @@ mod tests { 38 ); assert_eq!(decimal_vortex_array_non_null.decimal_dtype().scale(), 10); + assert_eq!( + decimal_vortex_array_non_null.values_type(), + DecimalType::I128 + ); 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() + ); + } + + #[rstest] + #[case::decimal32_p4(DataType::Decimal32(4, 2), DecimalType::I16, false)] + #[case::decimal64_p9(DataType::Decimal64(9, 2), DecimalType::I32, false)] + #[case::decimal128_p5(DataType::Decimal128(5, 2), DecimalType::I32, false)] + #[case::decimal128_p18(DataType::Decimal128(18, 2), DecimalType::I64, false)] + #[case::decimal128_p38(DataType::Decimal128(38, 2), DecimalType::I128, true)] + #[case::decimal256_p38(DataType::Decimal256(38, 2), DecimalType::I128, false)] + #[case::decimal256_p40(DataType::Decimal256(40, 2), DecimalType::I256, true)] + fn test_decimal_import_canonical_width( + #[case] data_type: DataType, + #[case] values_type: DecimalType, + #[case] zero_copy: bool, + ) -> VortexResult<()> { + let (precision, scale) = match data_type { + DataType::Decimal32(precision, scale) + | DataType::Decimal64(precision, scale) + | DataType::Decimal128(precision, scale) + | DataType::Decimal256(precision, scale) => (precision, scale), + _ => unreachable!("every case is an Arrow decimal type, got {data_type}"), + }; + + // The largest and smallest values of the precision, around a null and a zero. + let max = i256::from_i128(10).wrapping_pow(u32::from(precision)) - i256::ONE; + let values = [Some(-max), None, Some(i256::ZERO), Some(max)]; + let arrow_values = values.map(|value| value.map(arrow_buffer::i256::from)); + let bounds = + Decimal256Array::from_iter(arrow_values).with_precision_and_scale(precision, scale)?; + let arrow = arrow_cast::cast(&bounds, &data_type)?; + + let imported = from_arrow_dyn(arrow.as_ref(), true)?; + let decimal = imported.as_::(); + assert_eq!(decimal.values_type(), values_type); + if zero_copy { + assert_eq!( + decimal.buffer_handle().as_host().as_ptr(), + arrow.to_data().buffers()[0].as_ptr() + ); + } + + assert_arrays_eq!( + imported, + DecimalArray::from_option_iter(values, DecimalDType::new(precision, scale)), + &mut SESSION.create_execution_ctx() + ); + Ok(()) + } + + /// The null slot holds 10^30, which is outside both precisions and does not fit `i32`. + #[rstest] + #[case::narrowing(5, false)] + #[case::equal_width(20, true)] + fn test_decimal_import_ignores_null_slots( + #[case] precision: u8, + #[case] zero_copy: bool, + ) -> VortexResult<()> { + let arrow = Decimal128Array::new( + ScalarBuffer::from(vec![12_345, 10i128.pow(30), -12_345]), + Some(NullBuffer::from(vec![true, false, true])), + ) + .with_precision_and_scale(precision, 2)?; + + let imported = from_arrow_dyn(&arrow, true)?; + let decimal = imported.as_::(); + if zero_copy { + assert_eq!( + decimal.buffer_handle().as_host().as_ptr(), + arrow.to_data().buffers()[0].as_ptr() + ); + } else { + // A narrowing copy writes zeros into the null slots. + assert_eq!(decimal.values_type(), DecimalType::I32); + assert_eq!(decimal.buffer::().as_slice(), &[12_345, 0, -12_345]); + } + + assert_arrays_eq!( + imported, + DecimalArray::from_option_iter( + [Some(12_345i128), None, Some(-12_345)], + DecimalDType::new(precision, 2) ), &mut SESSION.create_execution_ctx() ); + Ok(()) + } + + #[test] + fn test_decimal_import_rejects_value_outside_precision() -> VortexResult<()> { + let arrow = + Decimal128Array::from(vec![12_345, 1_000_000]).with_precision_and_scale(5, 2)?; + + let err = from_arrow_dyn(&arrow, true) + .expect_err("a valid value outside the precision must fail the import"); + let message = err.to_string(); + assert!(message.contains("row 1"), "{message}"); + assert!(message.contains("null_if_overflow_precision"), "{message}"); + Ok(()) + } + + #[test] + fn test_decimal_import_zero_copy_rejects_value_outside_precision() -> VortexResult<()> { + let arrow = + Decimal128Array::from(vec![12_345, 10i128.pow(20)]).with_precision_and_scale(20, 2)?; + + let err = from_arrow_dyn(&arrow, true) + .expect_err("a valid value outside the precision must fail the import"); + let message = err.to_string(); + assert!(message.contains("row 1"), "{message}"); + assert!(message.contains("null_if_overflow_precision"), "{message}"); + Ok(()) + } + + #[rstest] + #[case::precision_zero(DataType::Decimal128(0, 0))] + #[case::precision_above_max(DataType::Decimal128(77, 0))] + #[case::scale_above_precision(DataType::Decimal128(5, 6))] + fn test_decimal_import_rejects_invalid_precision(#[case] data_type: DataType) { + // `with_data_type` does not validate the precision or scale, as an FFI import would not. + let arrow = Decimal128Array::from(vec![1]).with_data_type(data_type); + + assert!(from_arrow_dyn(&arrow, true).is_err()); } // Test temporal array conversions @@ -1540,7 +1742,7 @@ mod tests { Arc::new(Int32Array::from(vec![Some(1), None, Some(3)])), Arc::new(StringArray::from(vec!["a", "b", "c"])), ], - Some(arrow_buffer::NullBuffer::new(BooleanBuffer::from(vec![ + Some(NullBuffer::new(BooleanBuffer::from(vec![ true, false, true, ]))), ); @@ -1779,8 +1981,7 @@ mod tests { ]); // Create nulls buffer - second list is null - let null_buffer = - arrow_buffer::NullBuffer::new(BooleanBuffer::from(vec![true, false, true])); + let null_buffer = NullBuffer::new(BooleanBuffer::from(vec![true, false, true])); let arrow_array_nullable = ArrowFixedSizeListArray::try_new( field, @@ -1889,8 +2090,7 @@ mod tests { assert_eq!(sizes_array.ptype(), PType::I32); // Test nullable ListView - let null_buffer = - arrow_buffer::NullBuffer::new(BooleanBuffer::from(vec![true, false, true, true])); + let null_buffer = NullBuffer::new(BooleanBuffer::from(vec![true, false, true, true])); let arrow_array_nullable = GenericListViewArray::try_new( Arc::clone(&field), diff --git a/vortex-cuda/src/kernel/arrays/constant.rs b/vortex-cuda/src/kernel/arrays/constant.rs index 86f13a77e2b..81d5f97cf3d 100644 --- a/vortex-cuda/src/kernel/arrays/constant.rs +++ b/vortex-cuda/src/kernel/arrays/constant.rs @@ -18,7 +18,6 @@ use vortex::array::match_each_native_simd_ptype; use vortex::array::validity::Validity; use vortex::dtype::DType; use vortex::dtype::DecimalDType; -use vortex::dtype::DecimalType; use vortex::dtype::NativeDecimalType; use vortex::dtype::NativePType; use vortex::error::VortexResult; @@ -70,7 +69,7 @@ impl CudaExecute for ConstantNumericExecutor { DType::Decimal(decimal_dtype, nullability) => { let decimal_dtype = *decimal_dtype; let validity: Validity = nullability.into(); - let values_type = DecimalType::smallest_decimal_value_type(&decimal_dtype); + let values_type = decimal_dtype.canonical_values_type(); match_each_decimal_value_type!(values_type, |D| { materialize_constant_decimal::(array, decimal_dtype, validity, ctx).await }) diff --git a/vortex-duckdb/src/convert/vector.rs b/vortex-duckdb/src/convert/vector.rs index d5fc6338153..19e75bec24b 100644 --- a/vortex-duckdb/src/convert/vector.rs +++ b/vortex-duckdb/src/convert/vector.rs @@ -12,6 +12,9 @@ use vortex::array::arrays::ListViewArray; use vortex::array::arrays::PrimitiveArray; use vortex::array::arrays::StructArray; use vortex::array::arrays::TemporalArray; +use vortex::array::arrays::decimal::check_precision; +use vortex::array::arrays::decimal::copy_to_canonical_width; +use vortex::array::buffer::BufferHandle; use vortex::array::builders::ArrayBuilder; use vortex::array::builders::VarBinViewBuilder; use vortex::array::dtype::extension::ExtDType; @@ -25,6 +28,7 @@ use vortex::dtype::DType; use vortex::dtype::DecimalDType; use vortex::dtype::DecimalType; use vortex::dtype::FieldNames; +use vortex::dtype::NativeDecimalType; use vortex::dtype::NativePType; use vortex::dtype::Nullability; use vortex::encodings::uuid::Uuid; @@ -91,6 +95,24 @@ fn vector_i128_values(vector: &VectorRef, len: usize) -> impl TrustedLen( + values: &[T], + validity: &Mask, + decimal_dtype: DecimalDType, +) -> VortexResult { + if T::DECIMAL_TYPE == decimal_dtype.canonical_values_type() { + check_precision(values, validity, decimal_dtype)?; + } + + copy_to_canonical_width(values, validity, decimal_dtype) +} + fn vector_mapped P>( vector: &VectorRef, len: usize, @@ -305,36 +327,46 @@ pub fn flat_vector_to_vortex(vector: &VectorRef, len: usize) -> VortexResult { let data = vector.as_slice_with_len::(len); - if DecimalType::smallest_decimal_value_type(&decimal_dtype) == DecimalType::I8 { - // Lossy conversion is defined behavior in Rust so this is fine - let buf = - Buffer::::from_trusted_len_iter(data.iter().map(|&v| v.as_())); - DecimalArray::try_new(buf, decimal_dtype, validity) - } else { - DecimalArray::try_new(Buffer::copy_from(data), decimal_dtype, validity) - } + copy_decimal_values(data, &mask, decimal_dtype)? } DecimalType::I32 => { let data = vector.as_slice_with_len::(len); - DecimalArray::try_new(Buffer::copy_from(data), decimal_dtype, validity) + copy_decimal_values(data, &mask, decimal_dtype)? } DecimalType::I64 => { let data = vector.as_slice_with_len::(len); - DecimalArray::try_new(Buffer::copy_from(data), decimal_dtype, validity) + copy_decimal_values(data, &mask, decimal_dtype)? } DecimalType::I128 => { + // DuckDB columns can be misaligned for `i128`, so `vector_i128_values` reads + // them into a new buffer, which already has the canonical width of precision + // 19 to 38. let data = Buffer::from_iter(vector_i128_values(vector, len)); - DecimalArray::try_new(data, decimal_dtype, validity) + check_precision(data.as_slice(), &mask, decimal_dtype)?; + BufferHandle::new_host(data.into_byte_buffer()) } _ => vortex_bail!("Unsupported decimal precision: {precision}"), - } - .map(|a| a.into_array()) + }; + let validity = Validity::from_mask(mask, Nullability::Nullable); + + // SAFETY: `copy_decimal_values` and the `I128` arm checked that every valid value is + // within the precision of `decimal_dtype`. The values have the canonical width, and + // they hold `len` values, as the validity does. + let array = unsafe { + DecimalArray::new_unchecked_handle( + values, + decimal_dtype.canonical_values_type(), + decimal_dtype, + validity, + ) + }; + + Ok(array.into_array()) } DUCKDB_TYPE::DUCKDB_TYPE_UUID => { let mut bytes = BufferMut::::with_capacity(len * 16); @@ -440,8 +472,10 @@ mod tests { use std::sync::LazyLock; use geo_types::point; + use rstest::rstest; use vortex::array::VortexSessionExecute; use vortex::array::arrays::BoolArray; + use vortex::array::arrays::Decimal; use vortex::array::arrays::Extension; use vortex::array::arrays::VarBinViewArray; use vortex::array::arrays::fixed_size_list::FixedSizeListArrayExt; @@ -449,6 +483,7 @@ mod tests { use vortex::array::arrays::listview::ListViewArraySlotsExt; use vortex::array::arrays::struct_::StructArrayExt; use vortex::array::assert_arrays_eq; + use vortex::dtype::BigCast; use vortex::error::VortexExpect; use vortex::mask::Mask; use vortex::session::VortexSession; @@ -464,6 +499,39 @@ mod tests { static SESSION: LazyLock = LazyLock::new(VortexSession::empty); + /// Builds a DuckDB decimal vector of `precision` and scale 0 that holds `slots`, and marks + /// `null_rows` as null. Each slot is written at the DuckDB storage width of `precision`. + fn decimal_vector(precision: u8, slots: &[i128], null_rows: &[usize]) -> VortexResult { + let len = slots.len(); + let logical_type = LogicalType::decimal_type(precision, 0)?; + let mut vector = Vector::with_capacity(&logical_type, len); + + match precision_to_duckdb_storage_size(&DecimalDType::try_new(precision, 0)?)? { + DecimalType::I16 => write_decimal_slots::(&mut vector, slots), + DecimalType::I32 => write_decimal_slots::(&mut vector, slots), + DecimalType::I64 => write_decimal_slots::(&mut vector, slots), + DecimalType::I128 => write_decimal_slots::(&mut vector, slots), + storage => unreachable!("DuckDB stores decimals in 16 to 128 bits, got {storage}"), + } + + // SAFETY: Vector was created with this length. + let validity_slice = unsafe { vector.ensure_validity_bitslice(len) }; + for &row in null_rows { + validity_slice.set(row, false); + } + + Ok(vector) + } + + fn write_decimal_slots(vector: &mut Vector, slots: &[i128]) { + // SAFETY: Vector was created with a capacity of `slots.len()`. + let storage = unsafe { vector.as_slice_mut::(slots.len()) }; + for (storage, &slot) in storage.iter_mut().zip(slots) { + *storage = ::from(slot) + .vortex_expect("test slot must fit the DuckDB storage width"); + } + } + #[test] fn test_integer_vector_conversion() { let mut ctx = SESSION.create_execution_ctx(); @@ -1121,4 +1189,75 @@ mod tests { Ok(()) } + + #[rstest] + #[case::p1(1, DecimalType::I8)] + #[case::p2(2, DecimalType::I8)] + #[case::p3(3, DecimalType::I16)] + #[case::p4(4, DecimalType::I16)] + #[case::p9(9, DecimalType::I32)] + #[case::p18(18, DecimalType::I64)] + #[case::p38(38, DecimalType::I128)] + fn test_decimal_vector_conversion( + #[case] precision: u8, + #[case] values_type: DecimalType, + ) -> VortexResult<()> { + let mut ctx = SESSION.create_execution_ctx(); + + // The largest and smallest values of the precision, around a null and a zero. + let max = 10i128.pow(precision.into()) - 1; + let vector = decimal_vector(precision, &[-max, 0, 0, max], &[1])?; + + let result = flat_vector_to_vortex(&vector, 4)?; + assert_eq!(result.as_::().values_type(), values_type); + + let expected = DecimalArray::from_option_iter( + [Some(-max), None, Some(0), Some(max)], + DecimalDType::new(precision, 0), + ); + assert_arrays_eq!(result, expected, &mut ctx); + Ok(()) + } + + #[test] + fn test_decimal_vector_null_slot_garbage() -> VortexResult<()> { + // 300 fits neither precision 2 nor `i8`, but the import must not check a null slot. + let vector = decimal_vector(2, &[42, 300, -42], &[1])?; + + let result = flat_vector_to_vortex(&vector, 3)?; + let decimal = result.as_::(); + assert_eq!(decimal.values_type(), DecimalType::I8); + assert_eq!(decimal.buffer::().as_slice(), &[42, 0, -42]); + Ok(()) + } + + #[test] + fn test_decimal_vector_value_outside_precision() -> VortexResult<()> { + // Precision 2 narrows the DuckDB `i16` storage into `i8`. + let vector = decimal_vector(2, &[42, 150], &[])?; + + let err = flat_vector_to_vortex(&vector, 2) + .expect_err("a valid value outside the precision must fail the import"); + let message = err.to_string(); + assert!(message.contains("row 1"), "{message}"); + Ok(()) + } + + /// Each precision keeps the DuckDB storage width, and `outside` fits that width but not the + /// precision. + #[rstest] + #[case::p4_i16(4, 10_000)] + #[case::p20_i128(20, 10i128.pow(20))] + fn test_decimal_vector_equal_width_outside_precision( + #[case] precision: u8, + #[case] outside: i128, + ) -> VortexResult<()> { + let vector = decimal_vector(precision, &[42, outside], &[])?; + + let err = flat_vector_to_vortex(&vector, 2) + .expect_err("a valid value outside the precision must fail the import"); + let message = err.to_string(); + assert!(message.contains("row 1"), "{message}"); + Ok(()) + } } diff --git a/vortex-row/src/codec.rs b/vortex-row/src/codec.rs index 9b621200d48..15570b24b09 100644 --- a/vortex-row/src/codec.rs +++ b/vortex-row/src/codec.rs @@ -582,7 +582,7 @@ fn add_size_map( /// so keys from differently compressed chunks stay memcmp-comparable. Both the size plan /// ([`row_width_for_dtype`]) and the encoder derive their width from here. fn decimal_key_type(dt: &DecimalDType) -> DecimalType { - DecimalType::smallest_decimal_value_type(dt) + dt.canonical_values_type() } fn add_size_varbinview( 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..9a0491dbb56 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 @@ -67,7 +67,7 @@ impl LegacyDecimalColumn { /// expected form is its written form. A column that changes width gets zeros in its null /// slots, because a raw null-slot value need not fit the canonical width. fn expected(&self) -> VortexResult { - let canonical_type = DecimalType::smallest_decimal_value_type(&self.decimal_dtype); + let canonical_type = self.decimal_dtype.canonical_values_type(); if canonical_type == self.stored_type { return self.written(); } 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..5396729b4cf 100644 --- a/vortex-test/compat-gen/src/fixtures/arrays/synthetic/mod.rs +++ b/vortex-test/compat-gen/src/fixtures/arrays/synthetic/mod.rs @@ -28,7 +28,6 @@ mod tests { use vortex_array::arrays::struct_::StructArrayExt; use vortex_array::assert_arrays_eq; use vortex_array::dtype::BigCast; - use vortex_array::dtype::DecimalType; use vortex_array::match_each_decimal_value_type; use vortex_error::VortexExpect; use vortex_error::VortexResult; @@ -83,8 +82,7 @@ mod tests { let Some(decimal) = node.as_opt::() else { continue; }; - let canonical_type = - DecimalType::smallest_decimal_value_type(&decimal.decimal_dtype()); + let canonical_type = decimal.decimal_dtype().canonical_values_type(); assert_eq!( decimal.values_type(), canonical_type, @@ -120,8 +118,7 @@ mod tests { column.name.contains(&spec_name), "{context}: the name must contain {spec_name}", ); - let canonical_type = - DecimalType::smallest_decimal_value_type(&column.decimal_dtype); + let canonical_type = column.decimal_dtype.canonical_values_type(); assert_eq!( column.stored_type == canonical_type, column.name.starts_with("canonical_"),