diff --git a/encodings/decimal-byte-parts/src/decimal_byte_parts/array.rs b/encodings/decimal-byte-parts/src/decimal_byte_parts/array.rs index f0b03720e00..796789d5799 100644 --- a/encodings/decimal-byte-parts/src/decimal_byte_parts/array.rs +++ b/encodings/decimal-byte-parts/src/decimal_byte_parts/array.rs @@ -26,7 +26,6 @@ use vortex_array::dtype::DecimalDType; use vortex_array::require_child; use vortex_array::scalar::DecimalValue; use vortex_array::scalar::Scalar; -use vortex_array::scalar::ScalarValue; use vortex_array::serde::ArrayChildren; use vortex_array::vtable::OperationsVTable; use vortex_array::vtable::VTable; @@ -397,7 +396,7 @@ impl OperationsVTable for DecimalByteParts { ), }; - Scalar::try_new(array.dtype().clone(), Some(ScalarValue::Decimal(value))) + Scalar::try_decimal(value, array.decimal_dtype(), array.dtype().nullability()) } } diff --git a/fuzz/src/array/scalar_at.rs b/fuzz/src/array/scalar_at.rs index c978aacbc0d..28b85fcf810 100644 --- a/fuzz/src/array/scalar_at.rs +++ b/fuzz/src/array/scalar_at.rs @@ -48,11 +48,11 @@ pub fn scalar_at_canonical_array( } Canonical::Decimal(array) => { match_each_decimal_value_type!(array.values_type(), |D| { - Scalar::decimal( + Scalar::try_decimal( DecimalValue::from(array.buffer::()[index]), array.decimal_dtype(), array.dtype().nullability(), - ) + )? }) } Canonical::VarBinView(array) => varbin_scalar(array.bytes_at(index), array.dtype()), diff --git a/vortex-array/src/aggregate_fn/fns/min_max/decimal.rs b/vortex-array/src/aggregate_fn/fns/min_max/decimal.rs index 2fa1e137ffb..f7e8d4004d0 100644 --- a/vortex-array/src/aggregate_fn/fns/min_max/decimal.rs +++ b/vortex-array/src/aggregate_fn/fns/min_max/decimal.rs @@ -47,21 +47,23 @@ where let decimal_dtype = array.decimal_dtype(); - Ok( - valid_range(array.buffer::().as_slice(), &validity).map(|(min, max)| { - let min_scalar = Scalar::decimal(min.into(), decimal_dtype, NonNullable); - let max_scalar = if min == max { - min_scalar.clone() - } else { - Scalar::decimal(max.into(), decimal_dtype, NonNullable) - }; + let Some((min, max)) = valid_range(array.buffer::().as_slice(), &validity) else { + return Ok(None); + }; + + // Stored values are not guaranteed to fit the precision (for example, when read from a + // file), so return an error instead of panicking. + let min_scalar = Scalar::try_decimal(min.into(), decimal_dtype, NonNullable)?; + let max_scalar = if min == max { + min_scalar.clone() + } else { + Scalar::try_decimal(max.into(), decimal_dtype, NonNullable)? + }; - MinMaxResult { - min: min_scalar, - max: max_scalar, - } - }), - ) + Ok(Some(MinMaxResult { + min: min_scalar, + max: max_scalar, + })) } /// Returns the smallest and the largest valid value, or `None` when no value is valid. diff --git a/vortex-array/src/aggregate_fn/fns/min_max/mod.rs b/vortex-array/src/aggregate_fn/fns/min_max/mod.rs index ce3e938a8fd..de3e6c2dfbd 100644 --- a/vortex-array/src/aggregate_fn/fns/min_max/mod.rs +++ b/vortex-array/src/aggregate_fn/fns/min_max/mod.rs @@ -1060,6 +1060,54 @@ mod tests { }) } + #[rstest] + #[case::one_element(buffer![100i8])] + #[case::min_max(buffer![1i8, 100i8])] + #[case::negative_min(buffer![-100i8, 1i8])] + fn test_decimal_value_outside_precision_errors(#[case] values: Buffer) -> VortexResult<()> { + // 100 and -100 do not fit in decimal(2, 0), and `DecimalArray::new` does not check the + // values. + let decimal = DecimalArray::new(values, DecimalDType::new(2, 0), Validity::NonNullable); + let mut ctx = SESSION.create_execution_ctx(); + let err = min_max( + &decimal.into_array(), + &mut ctx, + NumericalAggregateOpts::default(), + ) + .unwrap_err(); + assert!( + err.to_string().contains("does not fit in precision"), + "unexpected error: {err}" + ); + Ok(()) + } + + #[test] + fn test_decimal_value_outside_precision_in_null_slot() -> VortexResult<()> { + // The out-of-range value sits in a null slot, so it must be ignored. + let decimal = DecimalArray::new( + buffer![5i8, 100i8], + DecimalDType::new(2, 0), + Validity::from_iter([true, false]), + ); + let mut ctx = SESSION.create_execution_ctx(); + let result = min_max( + &decimal.into_array(), + &mut ctx, + NumericalAggregateOpts::default(), + )? + .vortex_expect("should have result"); + + let expected = Scalar::decimal( + DecimalValue::I8(5), + DecimalDType::new(2, 0), + Nullability::NonNullable, + ); + assert_eq!(result.min, expected); + assert_eq!(result.max, expected); + Ok(()) + } + #[test] fn list_and_fixed_size_list_return_dtype() { let element_dtype = DType::Primitive(PType::I32, Nullability::Nullable); diff --git a/vortex-array/src/arrays/decimal/array.rs b/vortex-array/src/arrays/decimal/array.rs index 61aaf898515..91521aab93e 100644 --- a/vortex-array/src/arrays/decimal/array.rs +++ b/vortex-array/src/arrays/decimal/array.rs @@ -251,15 +251,15 @@ impl DecimalData { /// /// * `buffer` is a typed buffer containing the decimal values. /// * `decimal_dtype` specifies the decimal precision and scale. - /// * `validity` holds the null values. /// - /// # Safety + /// Values are not checked against the precision of `decimal_dtype`, either here, in + /// [`DecimalArray::try_new`], or when an array is read from a file. Code that turns stored values + /// into scalars must use a fallible path such as + /// [`Scalar::try_decimal`](crate::scalar::Scalar::try_decimal). /// - /// The caller must ensure all of the following invariants are satisfied: + /// # Safety /// - /// - All non-null values in `buffer` must be representable within the specified precision. - /// - For example, with precision=5 and scale=2, all values must be in range [-999.99, 999.99]. - /// - If `validity` is [`Validity::Array`], its length must exactly equal `buffer.len()`. + /// `buffer` is a typed [`Buffer`], so there are no further requirements on the caller. pub unsafe fn new_unchecked( buffer: Buffer, decimal_dtype: DecimalDType, @@ -276,13 +276,15 @@ impl DecimalData { /// Create a new array with decimal values backed by the given buffer handle. /// + /// As with [`DecimalArray::new_unchecked`], values are not checked against the precision of + /// `decimal_dtype`. + /// /// # Safety /// - /// The caller must ensure all of the following invariants are satisfied: + /// - The length of `values` must be a multiple of the byte width of `values_type`. + /// - `values` must be aligned to the native type of `values_type`. /// - /// - All non-null values in `values` must be representable within the specified precision. - /// - For example, with precision=5 and scale=2, all values must be in range [-999.99, 999.99]. - /// - If `validity` is [`Validity::Array`], its length must exactly equal `buffer.len()`. + /// Breaking either rule does not cause undefined behavior, but [`DecimalData::buffer`] panics. pub unsafe fn new_unchecked_handle( values: BufferHandle, values_type: DecimalType, @@ -325,7 +327,6 @@ impl DecimalData { /// The caller must ensure: /// - The `byte_buffer` contains valid data for the specified `values_type` /// - The buffer length is compatible with the `values_type` (i.e., divisible by the type size) - /// - All non-null values are representable within the specified precision /// - If `validity` is [`Validity::Array`], its length must equal the number of elements pub unsafe fn new_unchecked_from_byte_buffer( byte_buffer: ByteBuffer, diff --git a/vortex-array/src/arrays/decimal/vtable/operations.rs b/vortex-array/src/arrays/decimal/vtable/operations.rs index 09d1d31a29c..c43ff65c7a6 100644 --- a/vortex-array/src/arrays/decimal/vtable/operations.rs +++ b/vortex-array/src/arrays/decimal/vtable/operations.rs @@ -19,13 +19,14 @@ impl OperationsVTable for Decimal { index: usize, _ctx: &mut ExecutionCtx, ) -> VortexResult { - Ok(match_each_decimal_value_type!(array.values_type(), |D| { - Scalar::decimal( + // Stored values can fall outside the precision, see `DecimalData::new_unchecked`. + match_each_decimal_value_type!(array.values_type(), |D| { + Scalar::try_decimal( DecimalValue::from(array.buffer::()[index]), array.decimal_dtype(), array.dtype().nullability(), ) - })) + }) } } @@ -33,7 +34,9 @@ impl OperationsVTable for Decimal { mod tests { use std::sync::LazyLock; + use rstest::rstest; use vortex_buffer::buffer; + use vortex_error::VortexResult; use vortex_session::VortexSession; use crate::IntoArray; @@ -102,4 +105,46 @@ mod tests { ) ); } + + #[rstest] + #[case::positive(100i8)] + #[case::negative(-100i8)] + fn test_scalar_at_value_outside_precision_errors(#[case] value: i8) -> VortexResult<()> { + // `DecimalArray::new` does not check that the values fit the precision. + let array = DecimalArray::new( + buffer![value], + DecimalDType::new(2, 0), + Validity::NonNullable, + ); + + let err = array + .execute_scalar(0, &mut SESSION.create_execution_ctx()) + .unwrap_err(); + assert!( + err.to_string().contains("does not fit in precision"), + "unexpected error: {err}" + ); + Ok(()) + } + + #[test] + fn test_scalar_at_ignores_value_outside_precision_in_null_slot() -> VortexResult<()> { + let array = DecimalArray::new( + buffer![5i8, 100i8], + DecimalDType::new(2, 0), + Validity::from_iter([true, false]), + ); + let mut ctx = SESSION.create_execution_ctx(); + + assert_eq!( + array.execute_scalar(0, &mut ctx)?, + Scalar::decimal( + DecimalValue::I8(5), + DecimalDType::new(2, 0), + Nullability::Nullable + ) + ); + assert!(array.execute_scalar(1, &mut ctx)?.is_null()); + Ok(()) + } } diff --git a/vortex-array/src/scalar/constructor.rs b/vortex-array/src/scalar/constructor.rs index 81f72054696..b4528608a9c 100644 --- a/vortex-array/src/scalar/constructor.rs +++ b/vortex-array/src/scalar/constructor.rs @@ -65,16 +65,35 @@ impl Scalar { } /// Creates a new decimal scalar with the given value, precision, scale, and nullability. + /// + /// # Panics + /// + /// Panics if `value` does not fit in the precision of `decimal_type`. Use + /// [`Scalar::try_decimal`] when the value comes from data that has not been validated. pub fn decimal( value: DecimalValue, decimal_type: DecimalDType, nullability: Nullability, ) -> Self { + Self::try_decimal(value, decimal_type, nullability) + .vortex_expect("decimal value must fit the precision of `decimal_type`") + } + + /// Attempts to create a new decimal scalar with the given value, precision, scale, and + /// nullability. + /// + /// # Errors + /// + /// Returns an error if `value` does not fit in the precision of `decimal_type`. + pub fn try_decimal( + value: DecimalValue, + decimal_type: DecimalDType, + nullability: Nullability, + ) -> VortexResult { Self::try_new( DType::Decimal(decimal_type, nullability), Some(ScalarValue::Decimal(value)), ) - .vortex_expect("unable to construct a decimal `Scalar`") } /// Creates a new UTF-8 scalar from a string-like value. diff --git a/vortex-python/src/scalar/factory.rs b/vortex-python/src/scalar/factory.rs index 1ecd944fa8c..2e0df0dbb9c 100644 --- a/vortex-python/src/scalar/factory.rs +++ b/vortex-python/src/scalar/factory.rs @@ -155,11 +155,8 @@ fn scalar_helper_inner(value: &Bound<'_, PyAny>, dtype: Option<&DType>) -> PyRes "Value can't be represented as decimal", )); }; - return Ok(Scalar::decimal( - value, - *decimal_dtype, - Nullability::NonNullable, - )); + return Scalar::try_decimal(value, *decimal_dtype, Nullability::NonNullable) + .map_err(|e| PyValueError::new_err(e.to_string())); } if let Ok(integer) = value.cast::() {