From 4c75184bec6ec49d01e743d1e6f6c7e6b8a59357 Mon Sep 17 00:00:00 2001 From: Connor Tsui Date: Tue, 27 Oct 2026 14:02:23 -0400 Subject: [PATCH 1/5] Add DecimalDType::canonical_values_type The canonical width of a decimal values buffer depends only on the precision. Add `DecimalDType::canonical_values_type()` as the one owner of that mapping, so that later changes can ask the dtype for its width instead of a free function on `DecimalType`. Test every precision boundary between two widths. Signed-off-by: Connor Tsui --- vortex-array/src/dtype/decimal/mod.rs | 40 +++++++++++++++++++++++++++ 1 file changed, 40 insertions(+) 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(); From 51ae18f658964ff44fb54e23b5c4169cd22401d5 Mon Sep 17 00:00:00 2001 From: Connor Tsui Date: Tue, 27 Oct 2026 14:05:01 -0400 Subject: [PATCH 2/5] Replace smallest_decimal_value_type with canonical_values_type Migrate every call of `DecimalType::smallest_decimal_value_type` to `DecimalDType::canonical_values_type()` with the same dtype, and remove the old function. Arithmetic keeps its work dtype and casts keep their target dtype. `is_compatible_decimal_value_type` now compares against the canonical width. Signed-off-by: Connor Tsui --- docs/specs/row-encoding.md | 2 +- encodings/sparse/src/canonical.rs | 4 +--- fuzz/src/array/sum/mod.rs | 3 +-- .../src/aggregate_fn/fns/sum/decimal.rs | 3 +-- .../fns/uncompressed_size_in_bytes/decimal.rs | 7 ++----- .../uncompressed_size_in_bytes/list_view.rs | 3 +-- .../fns/uncompressed_size_in_bytes/mod.rs | 7 +++---- vortex-array/src/array/haystack/mod.rs | 3 +-- vortex-array/src/arrays/arbitrary.rs | 2 +- .../src/arrays/constant/vtable/canonical.rs | 5 ++--- .../src/arrays/decimal/compute/cast.rs | 2 +- .../src/arrays/primitive/compute/cast.rs | 7 ++----- vortex-array/src/builders/mod.rs | 19 +++++++---------- vortex-array/src/canonical.rs | 21 ++++++++----------- vortex-array/src/dtype/decimal/types.rs | 16 +------------- vortex-array/src/dtype/dtype_impl.rs | 5 +---- vortex-array/src/scalar/arbitrary.rs | 13 +++++------- .../scalar/typed_view/decimal/arithmetic.rs | 2 +- .../src/scalar/typed_view/decimal/dvalue.rs | 6 ++---- .../scalar_fn/fns/binary/numeric/decimal.rs | 5 ++--- vortex-cuda/src/kernel/arrays/constant.rs | 3 +-- vortex-duckdb/src/convert/vector.rs | 2 +- vortex-row/src/codec.rs | 2 +- .../arrays/synthetic/arrays/decimal_legacy.rs | 2 +- .../src/fixtures/arrays/synthetic/mod.rs | 7 ++----- 25 files changed, 52 insertions(+), 99 deletions(-) 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/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/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-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..70526ff6127 100644 --- a/vortex-duckdb/src/convert/vector.rs +++ b/vortex-duckdb/src/convert/vector.rs @@ -311,7 +311,7 @@ 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 { + if decimal_dtype.canonical_values_type() == 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_())); 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_"), From 7905e13019e01cf4c6dd484ae1a3085247327c50 Mon Sep 17 00:00:00 2001 From: Connor Tsui Date: Tue, 27 Oct 2026 14:14:21 -0400 Subject: [PATCH 3/5] Add copy_to_canonical_width and check_precision Imports and decoders need one shared way to move decimal values to the canonical width of their dtype. copy_to_canonical_width copies values into DecimalDType::canonical_values_type(). A narrowing copy checks every valid value against the target precision, not only the width, and writes zeros into null slots. A widening or same-width copy checks nothing. The copy never rescales. check_precision fails on a valid value outside the precision and ignores null slots. It runs a branchless fold first and looks for the failing row only when the fold finds a value outside the precision. Both functions are public through vortex_array::arrays::decimal. The try_downcast macro drops its local BigCast import, because the module now imports it. Signed-off-by: Connor Tsui --- vortex-array/src/arrays/decimal/utils.rs | 332 ++++++++++++++++++++++- 1 file changed, 330 insertions(+), 2 deletions(-) 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(()) + } } From 1a070854d10e368fa446c38a60f4c260fe4bae93 Mon Sep 17 00:00:00 2001 From: Connor Tsui Date: Tue, 27 Oct 2026 14:23:16 -0400 Subject: [PATCH 4/5] Import Arrow decimals at the canonical width The Arrow decimal import kept the Arrow width, so a Decimal128 array of precision 5 became an i128 Vortex array, and no import checked that the values fit the precision. One private function now serves all four from_arrow_decimal* functions. When the Arrow width is the canonical width of the precision, it keeps the Arrow buffer and runs check_precision. Otherwise it copies with copy_to_canonical_width, which writes zeros into null slots when it narrows. The dtype now comes from DecimalDType::try_new, so an invalid precision or scale from FFI returns an error instead of a panic. The error names arrow-rs PrimitiveArray::null_if_overflow_precision, which turns values outside the precision into nulls. The expected arrays of the Decimal128 and Decimal256 conversion tests now use the canonical widths (i64 for precision 10, i128 for precision 38), and the tests assert the width. Signed-off-by: Connor Tsui --- vortex-arrow/src/convert.rs | 286 ++++++++++++++++++++++++++++++------ 1 file changed, 243 insertions(+), 43 deletions(-) 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), From 9836fef3652f2da6aebe78a49663cb5343245f5d Mon Sep 17 00:00:00 2001 From: Connor Tsui Date: Tue, 27 Oct 2026 14:27:10 -0400 Subject: [PATCH 5/5] Import DuckDB decimals at the canonical width The DuckDB decimal import narrowed i16 storage into i8 for precision 1 to 2 with a lossy `as` cast, so a value outside the precision, or garbage in a null slot, wrapped silently. It also checked no value against the precision. The i16, i32 and i64 widths now copy through copy_to_canonical_width, which checks a narrowing copy and writes zeros into the null slots. The same-width copies and the i128 buffer are checked with check_precision. The array is then built with the unchecked constructor. Signed-off-by: Connor Tsui --- vortex-duckdb/src/convert/vector.rs | 169 +++++++++++++++++++++++++--- 1 file changed, 154 insertions(+), 15 deletions(-) diff --git a/vortex-duckdb/src/convert/vector.rs b/vortex-duckdb/src/convert/vector.rs index 70526ff6127..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 decimal_dtype.canonical_values_type() == 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(()) + } }