diff --git a/vortex-array/src/arrays/decimal/array.rs b/vortex-array/src/arrays/decimal/array.rs index 61aaf898515..a07dc1ffbf7 100644 --- a/vortex-array/src/arrays/decimal/array.rs +++ b/vortex-array/src/arrays/decimal/array.rs @@ -59,7 +59,7 @@ pub struct DecimalSlots { /// /// Decimals are stored as scaled integers in a supported scalar value type. /// -/// The precisions supported for each scalar type are: +/// The canonical widths for each precision are: /// - **i8**: precision 1-2 digits /// - **i16**: precision 3-4 digits /// - **i32**: precision 5-9 digits @@ -67,12 +67,12 @@ pub struct DecimalSlots { /// - **i128**: precision 19-38 digits /// - **i256**: precision 39-76 digits /// -/// These are just the maximal ranges for each scalar type, but it is perfectly legal to store -/// values with precision that does not match this exactly. For example, a valid DecimalArray with -/// precision=39 may store its values in an `i8` if all of the actual values fit into it. -/// -/// Similarly, a `DecimalArray` can be built that stores a set of precision=2 values in a -/// `Buffer`. +/// Storage width is still independent of precision during the canonical decimal migration. +/// Existing `vortex.decimal` nodes and DecimalByteParts assembly can produce noncanonical storage. +/// For example, an array with precision 39 can currently use an `i8` buffer when all values fit, +/// while an array with precision 2 can use a `Buffer`. [`DecimalDataParts`] preserves the +/// actual width for conversion into canonical decimal leaves, optionally under +/// [`Widen`](crate::arrays::Widen). /// /// ## Precision and Scale /// @@ -86,7 +86,7 @@ pub struct DecimalSlots { /// /// ## Valid Scalar Types /// -/// The underlying storage uses these native types based on precision: +/// Canonical decimal leaves select one of these native types from precision: /// - `DecimalType::I8`, `I16`, `I32`, `I64`, `I128`, `I256` /// - Type selection is automatic based on the required precision /// @@ -111,6 +111,7 @@ pub struct DecimalSlots { pub struct DecimalData { pub(super) decimal_dtype: DecimalDType, pub(super) values: BufferHandle, + /// Actual storage type, including noncanonical widths still accepted during the migration. pub(super) values_type: DecimalType, } @@ -124,10 +125,21 @@ impl Display for DecimalData { } } +/// Decimal storage and validity, as returned by [`DecimalArray::into_data_parts`]. pub struct DecimalDataParts { + /// The precision and scale of the values. pub decimal_dtype: DecimalDType, + /// Unscaled integers of type `values_type` in host or device memory. pub values: BufferHandle, + /// Actual storage type, retained to interpret input before canonical conversion. + /// + /// Legacy `vortex.decimal` nodes and DecimalByteParts assembly can use a different width from + /// [`DecimalDType::canonical_values_type`]. Comparing the widths identifies noncanonical + /// storage, not the age of the data: current [`WidenPlugin`] writes use that wire form too. + /// + /// [`WidenPlugin`]: crate::arrays::widen::WidenPlugin pub values_type: DecimalType, + /// The validity of the values, which also gives the nullability of the array. pub validity: Validity, } @@ -235,7 +247,8 @@ impl DecimalData { /// /// # Errors /// - /// See [`DecimalArray::new_unchecked`] for invariants that are checked. + /// Returns an error if the buffer length is not a multiple of the width of `values_type`, or + /// if the buffer is not aligned to that width. pub fn try_new_handle( values: BufferHandle, values_type: DecimalType, @@ -243,7 +256,8 @@ impl DecimalData { ) -> VortexResult { Self::validate(&values, values_type)?; - // SAFETY: validate ensures all invariants are met. + // SAFETY: `validate` checks the buffer layout. Values outside the declared precision can + // produce incorrect results, but do not affect memory safety. Ok(unsafe { Self::new_unchecked_handle(values, values_type, decimal_dtype) }) } @@ -295,10 +309,10 @@ impl DecimalData { } } - /// Validates the components that would be used to create a [`DecimalArray`] from a byte buffer. + /// Validates the layout of a decimal values buffer. /// - /// This function checks all the invariants required by [`DecimalArray::new_unchecked`]. - fn validate(buffer: &BufferHandle, values_type: DecimalType) -> VortexResult<()> { + /// Checks byte length and alignment for `values_type`. Does not read values or check validity. + pub(crate) fn validate(buffer: &BufferHandle, values_type: DecimalType) -> VortexResult<()> { let byte_width = values_type.byte_width(); vortex_ensure!( buffer.len().is_multiple_of(byte_width), diff --git a/vortex-array/src/arrays/decimal/vtable/mod.rs b/vortex-array/src/arrays/decimal/vtable/mod.rs index 99d7314bbfa..64691a6cb20 100644 --- a/vortex-array/src/arrays/decimal/vtable/mod.rs +++ b/vortex-array/src/arrays/decimal/vtable/mod.rs @@ -50,9 +50,13 @@ pub(crate) fn initialize(session: &VortexSession) { kernel::initialize(session); } -// The type of the values can be determined by looking at the type info...right? +/// Metadata for the existing `vortex.decimal` wire representation. #[derive(prost::Message)] pub struct DecimalMetadata { + /// Stored [`DecimalType`] discriminant, retained for wire compatibility. + /// + /// The legacy format records the actual width separately from the logical dtype. Readers + /// need this field even when the in-memory representation uses canonical decimal leaves. #[prost(int32, tag = "1")] pub(super) values_type: i32, } diff --git a/vortex-array/src/arrays/widen/compute/aggregate.rs b/vortex-array/src/arrays/widen/compute/aggregate.rs index 731505109c7..6552df57d0f 100644 --- a/vortex-array/src/arrays/widen/compute/aggregate.rs +++ b/vortex-array/src/arrays/widen/compute/aggregate.rs @@ -86,6 +86,9 @@ fn aggregate_decimal( return Ok(Some(partial)); } + // Kernel registrations apply to the whole encoding. Keep this decimal allowlist so adding an + // integer aggregate cannot enable it here without checking its partials against canonical decimal + // execution. Every other decimal aggregate uses the default path. if !(aggregate_fn.is::() || aggregate_fn.is::() || aggregate_fn.is::() diff --git a/vortex-array/src/arrays/widen/compute/fill_null.rs b/vortex-array/src/arrays/widen/compute/fill_null.rs index d0b36961911..14cc605e8b3 100644 --- a/vortex-array/src/arrays/widen/compute/fill_null.rs +++ b/vortex-array/src/arrays/widen/compute/fill_null.rs @@ -27,7 +27,7 @@ impl FillNullReduce for Widen { let values = array.values(); let dtype = match values.dtype() { DType::Primitive(..) => { - let Some(ptype) = ptype_holding_values_and_scalar(array, fill_value)? else { + let Some(ptype) = ptype_holding_values_and_scalar(array, fill_value) else { return Ok(None); }; diff --git a/vortex-array/src/arrays/widen/compute/mod.rs b/vortex-array/src/arrays/widen/compute/mod.rs index b930fdc90d1..fef7485c7a2 100644 --- a/vortex-array/src/arrays/widen/compute/mod.rs +++ b/vortex-array/src/arrays/widen/compute/mod.rs @@ -31,6 +31,7 @@ mod slice; mod take; mod zip; +use vortex_error::VortexExpect; use vortex_error::VortexResult; use crate::ArrayRef; @@ -40,7 +41,6 @@ use crate::arrays::ConstantArray; use crate::arrays::Widen; use crate::arrays::WidenArray; use crate::arrays::widen::WidenArraySlotsExt; -use crate::arrays::widen::encode::integer_value; use crate::arrays::widen::encode::smallest_integer_ptype; use crate::builtins::ArrayBuiltins; use crate::dtype::DType; @@ -69,29 +69,30 @@ pub(super) fn wrap_values(values: ArrayRef, dtype: DType) -> VortexResult, scalar: &Scalar, -) -> VortexResult> { +) -> Option { let (DType::Primitive(values_ptype, _), DType::Primitive(logical_ptype, _)) = (array.values().dtype(), array.dtype()) else { - return Ok(None); + return None; }; if !scalar.dtype().eq_ignore_nullability(array.dtype()) { - return Ok(None); + return None; } if scalar.is_null() { - return Ok(Some(*values_ptype)); + return Some(*values_ptype); } - let value = integer_value(scalar)?; - let Some(scalar_ptype) = smallest_integer_ptype(value..=value, logical_ptype.is_signed_int()) - else { - return Ok(None); - }; + let value = scalar + .as_primitive() + .pvalue() + .and_then(PValue::as_i128) + .vortex_expect("the non-null scalar has the Widen's integer dtype"); + let scalar_ptype = smallest_integer_ptype(value..=value, logical_ptype.is_signed_int())?; if scalar_ptype.byte_width() > values_ptype.byte_width() { - Ok(Some(scalar_ptype)) + Some(scalar_ptype) } else { - Ok(Some(*values_ptype)) + Some(*values_ptype) } } diff --git a/vortex-array/src/arrays/widen/compute/numeric.rs b/vortex-array/src/arrays/widen/compute/numeric.rs index 54f51c5d214..84046fbc855 100644 --- a/vortex-array/src/arrays/widen/compute/numeric.rs +++ b/vortex-array/src/arrays/widen/compute/numeric.rs @@ -25,7 +25,7 @@ use crate::arrays::scalar_fn::ScalarFnArrayView; use crate::arrays::widen::WidenArraySlotsExt; use crate::arrays::widen::compute::cast_keeping_nullability; use crate::arrays::widen::compute::wrap_values; -use crate::arrays::widen::encode::integer_value; +use crate::arrays::widen::encode::integer_extrema; use crate::arrays::widen::encode::smallest_integer_ptype; use crate::dtype::DType; use crate::dtype::PType; @@ -162,9 +162,7 @@ fn valid_value_interval( return Ok(None); }; - Ok(Some( - integer_value(&bounds.min)?..=integer_value(&bounds.max)?, - )) + Ok(Some(integer_extrema(bounds))) } /// Returns the smallest interval that holds `lhs operator rhs` for every value of `lhs` and every diff --git a/vortex-array/src/arrays/widen/compute/zip.rs b/vortex-array/src/arrays/widen/compute/zip.rs index 69f3f1282c5..ab5e6e692bb 100644 --- a/vortex-array/src/arrays/widen/compute/zip.rs +++ b/vortex-array/src/arrays/widen/compute/zip.rs @@ -30,7 +30,7 @@ impl ZipKernel for Widen { }; (other.values().clone(), ptype) } else if let Some(constant) = if_false.as_constant() { - let Some(ptype) = ptype_holding_values_and_scalar(array, constant)? else { + let Some(ptype) = ptype_holding_values_and_scalar(array, constant) else { return Ok(None); }; (if_false.clone(), ptype) diff --git a/vortex-array/src/arrays/widen/encode.rs b/vortex-array/src/arrays/widen/encode.rs deleted file mode 100644 index 141fccde6d2..00000000000 --- a/vortex-array/src/arrays/widen/encode.rs +++ /dev/null @@ -1,206 +0,0 @@ -// SPDX-License-Identifier: Apache-2.0 -// SPDX-FileCopyrightText: Copyright the Vortex contributors - -//! Selection of narrower `Widen` children from valid values. -//! -//! Null payloads do not constrain the child dtype. The result keeps the logical dtype and uses -//! narrower storage only when that storage can hold every valid value. - -use std::ops::RangeInclusive; - -use vortex_error::VortexResult; -use vortex_error::vortex_bail; -use vortex_error::vortex_ensure; - -use crate::ArrayRef; -use crate::ExecutionCtx; -use crate::IntoArray; -use crate::aggregate_fn::NumericalAggregateOpts; -use crate::aggregate_fn::fns::min_max::min_max; -use crate::arrays::DecimalArray; -use crate::arrays::PrimitiveArray; -use crate::arrays::WidenArray; -use crate::arrays::decimal::DecimalArrayExt; -use crate::arrays::decimal::copy_to_canonical_width; -use crate::arrays::decimal::valid_range; -use crate::arrays::widen::compute::wrap_values; -use crate::builtins::ArrayBuiltins; -use crate::dtype::DType; -use crate::dtype::DecimalDType; -use crate::dtype::NativeDecimalType; -use crate::dtype::PType; -use crate::dtype::i256; -use crate::match_each_decimal_value_type; -use crate::scalar::DecimalValue; -use crate::scalar::Scalar; - -impl WidenArray { - /// Stores `array` as a `Widen` over the smallest integer type with the same signedness that - /// holds its valid values, or returns `array` unchanged when no narrower type holds them. - /// - /// Null rows do not count, so their payloads can be outside the stored type, and an empty or - /// all-null array gets a 1-byte type. Fails when `array` is not an integer array. - pub fn encode(array: PrimitiveArray, ctx: &mut ExecutionCtx) -> VortexResult { - let dtype = array.dtype().clone(); - wrap_values(narrow_values(array, ctx)?.into_array(), dtype) - } - - /// Stores a decimal array at the smallest precision that its valid values and scale allow. - /// - /// The result keeps the input dtype and nullability. Empty and all-null arrays use the - /// precision required by the scale, or 1 for a non-positive scale. Returns the input unchanged - /// when the selected precision has the canonical width of the input dtype. - /// - /// # Errors - /// - /// Returns an error if a valid value does not fit the input precision. - pub fn encode_decimal(array: DecimalArray, ctx: &mut ExecutionCtx) -> VortexResult { - let logical_decimal_dtype = array.decimal_dtype(); - let validity = array.validity()?; - let validity_mask = validity.execute_mask(array.len(), ctx)?; - let valid_value_range = match_each_decimal_value_type!(array.values_type(), |T| { - valid_range(array.buffer::().as_slice(), &validity_mask).map(|(min, max)| { - DecimalValue::from(min).as_i256()..=DecimalValue::from(max).as_i256() - }) - }); - - let child_decimal_dtype = smallest_decimal_dtype(valid_value_range, logical_decimal_dtype)?; - if child_decimal_dtype.canonical_values_type() - == logical_decimal_dtype.canonical_values_type() - { - return Ok(array.into_array()); - } - - let child_buffer_handle = match_each_decimal_value_type!(array.values_type(), |T| { - copy_to_canonical_width( - array.buffer::().as_slice(), - &validity_mask, - child_decimal_dtype, - )? - }); - let logical_dtype = array.dtype().clone(); - - let child = DecimalArray::new_handle( - child_buffer_handle, - child_decimal_dtype.canonical_values_type(), - child_decimal_dtype, - validity, - ); - - Ok(WidenArray::try_new(child.into_array(), logical_dtype)?.into_array()) - } - - /// Returns `array` cast to the type that [`Self::encode`] chooses for its values, without a - /// `Widen` around them. - /// - /// The result keeps the signedness and the validity of `array`, but its dtype can be narrower. - /// A float array returns unchanged, where [`Self::encode`] fails. This is for an internal child - /// whose parent chooses its dtype, such as list offsets or dictionary codes. An array whose - /// dtype is fixed, such as a column, needs [`Self::encode`] instead. - #[deprecated( - note = "Changes the dtype. Each caller must decide whether it needs a Widen (#10733)" - )] - pub fn encode_values( - array: PrimitiveArray, - ctx: &mut ExecutionCtx, - ) -> VortexResult { - if !array.ptype().is_int() { - return Ok(array); - } - - narrow_values(array, ctx) - } -} - -/// Returns `array` cast to the stored type that [`WidenArray::encode`] chooses, or `array` -/// unchanged when no narrower type holds its valid values. -/// -/// [`WidenArray::encode`] and the deprecated [`WidenArray::encode_values`] share this function, -/// so that `encode` needs no `#[expect(deprecated)]`. Fails when `array` is not an integer array. -fn narrow_values(array: PrimitiveArray, ctx: &mut ExecutionCtx) -> VortexResult { - let ptype = array.ptype(); - vortex_ensure!(ptype.is_int(), "Widen requires integer values, got {ptype}"); - - // No integer type is narrower than one byte, so the bounds cannot change the result. - if ptype.byte_width() == 1 { - return Ok(array); - } - - // An array without valid values fits any type, and the bounds `0..=0` select the 1-byte type. - let bounds = match min_max(array.as_ref(), ctx, NumericalAggregateOpts::default())? { - Some(bounds) => integer_value(&bounds.min)?..=integer_value(&bounds.max)?, - None => 0..=0, - }; - let Some(stored_ptype) = smallest_integer_ptype(bounds, ptype.is_signed_int()) - .filter(|stored_ptype| stored_ptype.byte_width() < ptype.byte_width()) - else { - return Ok(array); - }; - - let stored_dtype = DType::Primitive(stored_ptype, array.dtype().nullability()); - array - .into_array() - .cast(stored_dtype)? - .execute::(ctx) -} - -/// Returns the smallest integer type with the given signedness that holds every value in `bounds`, -/// or `None` when no integer type of at most 64 bits holds them. -pub(super) fn smallest_integer_ptype(bounds: RangeInclusive, signed: bool) -> Option { - let (min, max) = bounds.into_inner(); - if signed { - let min_ptype = PType::min_signed_ptype_for_value(i64::try_from(min).ok()?); - let max_ptype = PType::min_signed_ptype_for_value(i64::try_from(max).ok()?); - Some(min_ptype.max_signed_ptype(max_ptype)) - } else { - let min_ptype = PType::min_unsigned_ptype_for_value(u64::try_from(min).ok()?); - let max_ptype = PType::min_unsigned_ptype_for_value(u64::try_from(max).ok()?); - Some(min_ptype.max_unsigned_ptype(max_ptype)) - } -} - -/// Returns the value of a non-null integer scalar of at most 64 bits. -pub(super) fn integer_value(scalar: &Scalar) -> VortexResult { - if scalar.dtype().is_signed_int() { - i64::try_from(scalar).map(i128::from) - } else { - u64::try_from(scalar).map(i128::from) - } -} - -/// Returns the smallest decimal dtype that holds an unscaled valid-value range at the parent scale. -/// -/// The precision is at least 1 and at least a positive scale. An absent range represents an empty -/// or all-null array and needs only that minimum precision. The result never exceeds the parent -/// precision. Callers **must** exclude null payloads from the range, for example with -/// [`valid_range`]. -/// -/// # Errors -/// -/// Returns an error if the minimum exceeds the maximum or an endpoint exceeds the parent precision. -pub(crate) fn smallest_decimal_dtype( - range: Option>, - parent: DecimalDType, -) -> VortexResult { - let minimum_precision = parent.scale().max(1) as u8; - let Some(range) = range else { - // Empty and all-null arrays have no valid values to constrain precision. The scale still - // requires at least that many digits when positive, or precision 1 when non-positive. - return Ok(DecimalDType::new(minimum_precision, parent.scale())); - }; - - let (min, max) = range.into_inner(); - vortex_ensure!( - min <= max, - "Decimal range requires ordered extrema, got {min} to {max}" - ); - - for precision in minimum_precision..=parent.precision() { - let index = usize::from(precision); - if i256::MIN_BY_PRECISION[index] <= min && max <= i256::MAX_BY_PRECISION[index] { - return Ok(DecimalDType::new(precision, parent.scale())); - } - } - - vortex_bail!("Decimal range must fit {parent}, got {min} to {max}") -} diff --git a/vortex-array/src/arrays/widen/encode/decimal.rs b/vortex-array/src/arrays/widen/encode/decimal.rs new file mode 100644 index 00000000000..dc7ed5e0db6 --- /dev/null +++ b/vortex-array/src/arrays/widen/encode/decimal.rs @@ -0,0 +1,338 @@ +// SPDX-License-Identifier: Apache-2.0 +// SPDX-FileCopyrightText: Copyright the Vortex contributors + +//! Decimal encoding and conversion of legacy storage into canonical decimal leaves. +//! +//! The legacy `vortex.decimal` format stores integer width independently of logical precision. +//! DecimalByteParts assembly can also produce such buffers, but we now require canonical +//! `vortex.decimal` leaves to have a width determined and fixed by their precision. +//! +//! In this new world, we can retain narrow storage as a [`WidenArray`] over a canonical child. +//! Current [`WidenPlugin`] writes still use the legacy format, so this conversion can also apply +//! to newly written data (which will happen until `Widen` is added to the `core` edition). +//! +//! We unify child selection in both encoding and legacy conversion because it has non-trivial +//! logic: width alone does not determine a valid child dtype. For example, unscaled 127 fits `i8` +//! but needs precision 3, whose canonical width is `i16`. +//! +//! [`WidenPlugin`]: crate::arrays::widen::WidenPlugin + +use std::ops::RangeInclusive; + +use vortex_buffer::Buffer; +use vortex_error::VortexExpect; +use vortex_error::VortexResult; +use vortex_error::vortex_bail; +use vortex_error::vortex_ensure; +use vortex_error::vortex_ensure_eq; +use vortex_mask::Mask; + +use crate::ArrayRef; +use crate::ExecutionCtx; +use crate::IntoArray; +use crate::arrays::DecimalArray; +use crate::arrays::WidenArray; +use crate::arrays::decimal::DecimalArrayExt; +use crate::arrays::decimal::DecimalData; +use crate::arrays::decimal::DecimalDataParts; +use crate::arrays::decimal::copy_to_canonical_width; +use crate::arrays::decimal::valid_range; +use crate::buffer::BufferHandle; +use crate::dtype::DType; +use crate::dtype::DecimalDType; +use crate::dtype::NativeDecimalType; +use crate::dtype::i256; +use crate::expr::stats::Precision; +use crate::expr::stats::Stat; +use crate::match_each_decimal_value_type; +use crate::scalar::DecimalValue; +use crate::scalar::ScalarValue; +use crate::validity::Validity; + +impl WidenArray { + /// Stores a decimal array at the smallest precision that its valid values and scale allow. + /// + /// We keep the logical dtype and nullability by wrapping the narrower child in a + /// [`WidenArray`]. Reducing precision within the same storage width saves no space, so in that + /// case we return the input unchanged. + /// + /// Empty and all-null arrays have no values to constrain precision, but we still need to keep + /// the scale. They use precision 1 or the scale, whichever is greater. If a child is selected, + /// we cache any exact extrema as [`Stat::Min`] and [`Stat::Max`] for later aggregates. + /// + /// # Errors + /// + /// Returns an error if a valid value does not fit the input precision. + pub fn encode_decimal(array: DecimalArray, ctx: &mut ExecutionCtx) -> VortexResult { + let logical_decimal_dtype = array.decimal_dtype(); + let validity = array.validity()?; + let validity_mask = validity.execute_mask(array.len(), ctx)?; + let child = match_each_decimal_value_type!(array.values_type(), |T| { + let values_buffer = array.buffer::(); + let valid_value_range = + ValidRange::resolve(Precision::Absent, values_buffer.as_slice(), &validity_mask); + narrowest_decimal_child( + &values_buffer, + &validity, + &validity_mask, + valid_value_range, + logical_decimal_dtype, + )? + }); + + let Some(child) = child else { + return Ok(array.into_array()); + }; + + Ok(WidenArray::try_new(child.into_array(), array.dtype().clone())?.into_array()) + } + + /// Converts stored decimals into a canonical leaf or a [`WidenArray`] over one. + /// + /// Legacy decimal readers and DecimalByteParts assembly can give us a buffer whose width does + /// not match the logical precision. We need a canonical leaf without changing the logical + /// dtype or widening narrow storage unnecessarily. + /// + /// If the width is already canonical, we trust the declared precision and reuse the buffer + /// without reading its values. This also lets device buffers stay on the device. Otherwise, + /// child selection follows [`Self::encode_decimal`], keeping the logical dtype and validity. + /// We reuse buffers at the selected width and convert other widths without rescaling. + /// A narrowing copy writes zeros in null slots. A reused buffer keeps its null slots. + /// + /// Scanning and conversion use CPU kernels, so noncanonical buffers are brought to the host. + /// Their validity **must** already be executable on the host because these kernels need a mask + /// to exclude null payloads. + /// + /// Readers and decoders may already know the range. For noncanonical storage, `known_range` + /// lets us reuse it: + /// + /// - [`Precision::Absent`] scans valid values to find their extrema. + /// - [`Precision::Exact`] **must** give the actual minimum and maximum valid unscaled values. + /// - [`Precision::Inexact`] **must** contain every valid unscaled value. + /// + /// We cache the range on the child as [`Stat::Min`] and [`Stat::Max`], keeping its exactness + /// because enclosing bounds are not necessarily actual extrema. Scanning empty or all-null + /// input yields no extrema to cache. Canonical-width storage ignores `known_range`. + /// Supplied ranges are trusted, even when a narrowing copy checks the selected precision. + /// Incorrect ranges or declared precision can cause incorrect results in later operations. + /// + /// # Errors + /// + /// Rejects malformed buffer layout or validity length. For noncanonical storage, also rejects + /// failed host copies, reversed ranges, and range bounds outside the logical precision. A + /// narrowing copy rejects valid values outside the selected precision. + pub fn from_stored_decimal( + parts: DecimalDataParts, + known_range: Precision>, + ctx: &mut ExecutionCtx, + ) -> VortexResult { + let DecimalDataParts { + decimal_dtype, + values, + values_type, + validity, + } = parts; + + // Validate before typed buffer access or mask execution, which can panic on malformed data. + DecimalData::validate(&values, values_type)?; + let len = values.len() / values_type.byte_width(); + if let Some(validity_len) = validity.maybe_len() { + vortex_ensure_eq!( + validity_len, + len, + "Decimal validity must have one entry per value" + ); + } + + if values_type == decimal_dtype.canonical_values_type() { + let array = DecimalArray::try_new_handle(values, values_type, decimal_dtype, validity)?; + + return Ok(array.into_array()); + } + + let values = values.try_into_host_sync()?; + let mask = validity.execute_mask(len, ctx)?; + match_each_decimal_value_type!(values_type, |T| { + let values = Buffer::::from_byte_buffer(values); + let range = ValidRange::resolve(known_range, values.as_slice(), &mask); + widen_or_canonical(values, validity, &mask, range, decimal_dtype) + }) + } +} + +/// Chooses child precision from a valid-value range and the parent scale. +/// +/// We keep the parent scale so values do not need rescaling. The child precision must therefore +/// be at least 1 and at least the scale. With empty or all-null input, these are the only +/// constraints. Pass `None` for that case. Otherwise, null payloads **must** be excluded from the +/// range. +/// +/// # Errors +/// +/// Returns an error if the minimum exceeds the maximum or an endpoint exceeds the parent precision. +pub(crate) fn smallest_decimal_dtype( + range: Option>, + parent: DecimalDType, +) -> VortexResult { + let minimum_precision = parent.scale().max(1) as u8; + let Some(range) = range else { + return Ok(DecimalDType::new(minimum_precision, parent.scale())); + }; + + let (min, max) = range.into_inner(); + vortex_ensure!( + min <= max, + "Decimal range requires ordered extrema, got {min} to {max}" + ); + + for precision in minimum_precision..=parent.precision() { + let index = usize::from(precision); + if i256::MIN_BY_PRECISION[index] <= min && max <= i256::MAX_BY_PRECISION[index] { + return Ok(DecimalDType::new(precision, parent.scale())); + } + } + + vortex_bail!("Decimal range must fit {parent}, got {min} to {max}") +} + +/// Value bounds for choosing child precision and caching statistics. +/// +/// Supplied bounds need not be actual extrema, so we keep their exactness when caching them. +/// `Empty` means a scan found no valid values, rather than that statistics are unavailable. +enum ValidRange { + /// No value is valid. + Empty, + /// The smallest and the largest valid value. + Exact(RangeInclusive), + /// Bounds that hold every valid value. They need not be valid values themselves. + Bounds(RangeInclusive), +} + +impl ValidRange { + /// Uses known bounds when available, otherwise scans valid values. + /// + /// Readers and decoders may already have bounds from their input statistics. Reusing them + /// avoids scanning the buffer again. + fn resolve( + known_range: Precision>, + values: &[F], + mask: &Mask, + ) -> Self { + match known_range { + Precision::Exact(extrema) => return Self::Exact(extrema), + Precision::Inexact(bounds) => return Self::Bounds(bounds), + Precision::Absent => {} + } + + let Some((min, max)) = valid_range(values, mask) else { + return Self::Empty; + }; + let min = min.to_i256().vortex_expect("every decimal value fits i256"); + let max = max.to_i256().vortex_expect("every decimal value fits i256"); + + Self::Exact(min..=max) + } + + /// Returns the bounds that hold every valid value, or `None` when no value is valid. + fn bounds(&self) -> Option> { + match self { + Self::Empty => None, + Self::Exact(bounds) | Self::Bounds(bounds) => Some(bounds.clone()), + } + } + + /// Caches bounds on the child so later aggregates can reuse them without another scan. + /// + /// The bounds **must** fit the child's selected precision. + fn cache_statistics(self, child: &DecimalArray) { + let extrema = match self { + Self::Empty => return, + Self::Exact(bounds) => Precision::Exact(bounds), + Self::Bounds(bounds) => Precision::Inexact(bounds), + }; + let child_dtype = child.decimal_dtype(); + let min = extrema + .clone() + .map(|bounds| decimal_stat(*bounds.start(), child_dtype)); + let max = extrema.map(|bounds| decimal_stat(*bounds.end(), child_dtype)); + child.statistics().set(Stat::Min, min); + child.statistics().set(Stat::Max, max); + } +} + +/// Wraps a narrower canonical child, or builds a leaf at the logical width. +/// +/// The result must keep the logical dtype. Returning a narrow child directly would change it. +/// +/// `mask` **must** be the mask of `validity`. +fn widen_or_canonical( + values: Buffer, + validity: Validity, + mask: &Mask, + range: ValidRange, + decimal_dtype: DecimalDType, +) -> VortexResult { + if let Some(child) = narrowest_decimal_child(&values, &validity, mask, range, decimal_dtype)? { + let dtype = DType::Decimal(decimal_dtype, validity.nullability()); + return Ok(WidenArray::try_new(child.into_array(), dtype)?.into_array()); + } + + let array = build_canonical_decimal(&values, &validity, mask, decimal_dtype)?; + + Ok(array.into_array()) +} + +/// Builds the narrowest canonical child, or returns `None` if no narrower width exists. +/// +/// Reducing precision without changing width saves no space, so we return `None` in that case. +/// +/// Every valid value **must** lie in `range`, and `mask` **must** be the mask of `validity`. +fn narrowest_decimal_child( + values: &Buffer, + validity: &Validity, + mask: &Mask, + range: ValidRange, + decimal_dtype: DecimalDType, +) -> VortexResult> { + let child_dtype = smallest_decimal_dtype(range.bounds(), decimal_dtype)?; + let child_type = child_dtype.canonical_values_type(); + if child_type == decimal_dtype.canonical_values_type() { + return Ok(None); + } + + let child = build_canonical_decimal(values, validity, mask, child_dtype)?; + range.cache_statistics(&child); + + Ok(Some(child)) +} + +/// Reuses storage at the dtype's canonical width, copying only when conversion is needed. +/// +/// Every valid value **must** fit `decimal_dtype`, and `mask` **must** describe `validity`. +fn build_canonical_decimal( + values: &Buffer, + validity: &Validity, + mask: &Mask, + decimal_dtype: DecimalDType, +) -> VortexResult { + let values_type = decimal_dtype.canonical_values_type(); + let buffer = if F::DECIMAL_TYPE == values_type { + // The copy helper allocates even at the same width, so retain the existing buffer here. + BufferHandle::new_host(values.clone().into_byte_buffer()) + } else { + copy_to_canonical_width(values.as_slice(), mask, decimal_dtype)? + }; + + DecimalArray::try_new_handle(buffer, values_type, decimal_dtype, validity.clone()) +} + +/// Represents a cached bound using the child dtype. +/// +/// These statistics belong to the child, so their values need its canonical width rather than +/// the logical parent's width. `value` **must** fit the child's precision. +fn decimal_stat(value: i256, decimal_dtype: DecimalDType) -> ScalarValue { + let value = DecimalValue::try_from_i256(value, decimal_dtype) + .vortex_expect("the selected child precision contains its range bounds"); + + ScalarValue::Decimal(value) +} diff --git a/vortex-array/src/arrays/widen/encode/integer.rs b/vortex-array/src/arrays/widen/encode/integer.rs new file mode 100644 index 00000000000..f9758562f1b --- /dev/null +++ b/vortex-array/src/arrays/widen/encode/integer.rs @@ -0,0 +1,125 @@ +// SPDX-License-Identifier: Apache-2.0 +// SPDX-FileCopyrightText: Copyright the Vortex contributors + +//! Integer narrowing that preserves signedness and excludes null payloads. +//! +//! Encoding and the deprecated dtype-changing helper share this implementation so they select the +//! same storage type for each valid-value range. + +use std::ops::RangeInclusive; + +use vortex_error::VortexExpect; +use vortex_error::VortexResult; +use vortex_error::vortex_ensure; + +use crate::ArrayRef; +use crate::ExecutionCtx; +use crate::IntoArray; +use crate::aggregate_fn::NumericalAggregateOpts; +use crate::aggregate_fn::fns::min_max::MinMaxResult; +use crate::aggregate_fn::fns::min_max::min_max; +use crate::arrays::PrimitiveArray; +use crate::arrays::WidenArray; +use crate::arrays::widen::compute::wrap_values; +use crate::builtins::ArrayBuiltins; +use crate::dtype::DType; +use crate::dtype::PType; +use crate::scalar::PValue; + +impl WidenArray { + /// Stores `array` as a `Widen` over the smallest integer type with the same signedness that + /// holds its valid values, or returns `array` unchanged when no narrower type holds them. + /// + /// Null rows do not count, so their payloads can be outside the stored type, and an empty or + /// all-null array gets a 1-byte type. Fails when `array` is not an integer array. + pub fn encode(array: PrimitiveArray, ctx: &mut ExecutionCtx) -> VortexResult { + let dtype = array.dtype().clone(); + wrap_values(narrow_values(array, ctx)?.into_array(), dtype) + } + + /// Returns `array` cast to the type that [`Self::encode`] chooses for its values, without a + /// `Widen` around them. + /// + /// The result keeps the signedness and the validity of `array`, but its dtype can be narrower. + /// A float array returns unchanged, where [`Self::encode`] fails. This is for an internal child + /// whose parent chooses its dtype, such as list offsets or dictionary codes. An array whose + /// dtype is fixed, such as a column, needs [`Self::encode`] instead. + #[deprecated( + note = "Changes the dtype. Each caller must decide whether it needs a Widen (#10733)" + )] + pub fn encode_values( + array: PrimitiveArray, + ctx: &mut ExecutionCtx, + ) -> VortexResult { + if !array.ptype().is_int() { + return Ok(array); + } + + narrow_values(array, ctx) + } +} + +/// Returns `array` cast to the stored type that [`WidenArray::encode`] chooses, or `array` +/// unchanged when no narrower type holds its valid values. +/// +/// [`WidenArray::encode`] and the deprecated [`WidenArray::encode_values`] share this function, +/// so that `encode` needs no `#[expect(deprecated)]`. Fails when `array` is not an integer array. +fn narrow_values(array: PrimitiveArray, ctx: &mut ExecutionCtx) -> VortexResult { + let ptype = array.ptype(); + vortex_ensure!(ptype.is_int(), "Widen requires integer values, got {ptype}"); + + // No integer type is narrower than one byte, so the bounds cannot change the result. + if ptype.byte_width() == 1 { + return Ok(array); + } + + // An array without valid values fits any type, and the bounds `0..=0` select the 1-byte type. + let bounds = match min_max(array.as_ref(), ctx, NumericalAggregateOpts::default())? { + Some(bounds) => integer_extrema(bounds), + None => 0..=0, + }; + let Some(stored_ptype) = smallest_integer_ptype(bounds, ptype.is_signed_int()) + .filter(|stored_ptype| stored_ptype.byte_width() < ptype.byte_width()) + else { + return Ok(array); + }; + + let stored_dtype = DType::Primitive(stored_ptype, array.dtype().nullability()); + array + .into_array() + .cast(stored_dtype)? + .execute::(ctx) +} + +/// Widens integer extrema to `i128`, which holds every integer [`PValue`] exactly. +/// +/// Both extrema **must** be non-null integers, as returned by [`min_max`] on an integer array. +pub(in crate::arrays::widen) fn integer_extrema(bounds: MinMaxResult) -> RangeInclusive { + let [min, max] = [bounds.min, bounds.max].map(|bound| { + bound + .as_primitive() + .pvalue() + .and_then(PValue::as_i128) + .vortex_expect("min_max of an integer array returns non-null integer extrema") + }); + + min..=max +} + +/// Returns the smallest integer type with the given signedness that holds every value in `bounds`, +/// or `None` when no integer type of at most 64 bits holds them. +pub(in crate::arrays::widen) fn smallest_integer_ptype( + bounds: RangeInclusive, + signed: bool, +) -> Option { + let (min, max) = bounds.into_inner(); + if signed { + let min_ptype = PType::min_signed_ptype_for_value(i64::try_from(min).ok()?); + let max_ptype = PType::min_signed_ptype_for_value(i64::try_from(max).ok()?); + Some(min_ptype.max_signed_ptype(max_ptype)) + } else { + let min_ptype = PType::min_unsigned_ptype_for_value(u64::try_from(min).ok()?); + let max_ptype = PType::min_unsigned_ptype_for_value(u64::try_from(max).ok()?); + Some(min_ptype.max_unsigned_ptype(max_ptype)) + } +} diff --git a/vortex-array/src/arrays/widen/encode/mod.rs b/vortex-array/src/arrays/widen/encode/mod.rs new file mode 100644 index 00000000000..da2219d8044 --- /dev/null +++ b/vortex-array/src/arrays/widen/encode/mod.rs @@ -0,0 +1,15 @@ +// SPDX-License-Identifier: Apache-2.0 +// SPDX-FileCopyrightText: Copyright the Vortex contributors + +//! Selection of narrower `Widen` children from valid values. +//! +//! Integer narrowing and decimal conversion use separate implementations. Decimal encoding and +//! stored-value reads share one child-selection path so they cannot disagree about its precision. + +mod decimal; +#[cfg(test)] +pub(crate) use decimal::smallest_decimal_dtype; + +mod integer; +pub(super) use integer::integer_extrema; +pub(super) use integer::smallest_integer_ptype; diff --git a/vortex-array/src/arrays/widen/mod.rs b/vortex-array/src/arrays/widen/mod.rs index f387802df0b..72ba52b208a 100644 --- a/vortex-array/src/arrays/widen/mod.rs +++ b/vortex-array/src/arrays/widen/mod.rs @@ -12,8 +12,9 @@ //! //! A decimal child has the logical scale and a lower precision, and **must** be valid for its dtype. //! Comparisons, bounds checks and fills use the child dtype to bound its valid values. -//! [`WidenArray::encode_decimal`] picks the smallest precision that holds the valid values and the -//! scale. Decimal execution copies the child values into the logical canonical width. +//! [`WidenArray::encode_decimal`] and [`WidenArray::from_stored_decimal`] choose child precision +//! from valid values or supplied bounds, at the logical scale. Decimal execution copies the child +//! values into the logical canonical width. //! //! Narrowing is a physical choice, so it **must not** change a result. Execution casts the values //! to the logical dtype for integers, and an operation runs on the values only where it gives the diff --git a/vortex-array/src/arrays/widen/tests/decimal_stored.rs b/vortex-array/src/arrays/widen/tests/decimal_stored.rs new file mode 100644 index 00000000000..efc37b5b600 --- /dev/null +++ b/vortex-array/src/arrays/widen/tests/decimal_stored.rs @@ -0,0 +1,373 @@ +// SPDX-License-Identifier: Apache-2.0 +// SPDX-FileCopyrightText: Copyright the Vortex contributors + +//! Stored decimal conversion, buffer reuse, and range statistics. +//! +//! Compares converted values with canonical arrays and checks the selected child representation. + +use std::ops::RangeInclusive; + +use rstest::rstest; +use vortex_buffer::Buffer; +use vortex_buffer::ByteBuffer; +use vortex_error::VortexExpect; +use vortex_error::VortexResult; + +use super::SESSION; +use crate::ArrayRef; +use crate::IntoArray; +use crate::VortexSessionExecute; +use crate::arrays::BoolArray; +use crate::arrays::Decimal; +use crate::arrays::DecimalArray; +use crate::arrays::Widen; +use crate::arrays::WidenArray; +use crate::arrays::decimal::DecimalArrayExt; +use crate::arrays::decimal::DecimalDataParts; +use crate::arrays::widen::WidenArraySlotsExt; +use crate::assert_arrays_eq; +use crate::buffer::BufferHandle; +use crate::dtype::BigCast; +use crate::dtype::DecimalDType; +use crate::dtype::DecimalType; +use crate::dtype::i256; +use crate::expr::stats::Precision; +use crate::expr::stats::Stat; +use crate::expr::stats::StatsProvider; +use crate::match_each_decimal_value_type; +use crate::scalar::DecimalValue; +use crate::scalar::Scalar; +use crate::validity::Validity; + +/// Returns `values` as a buffer of `values_type`, with zero in each null slot. +fn stored_buffer(values: &[Option], values_type: DecimalType) -> ByteBuffer { + match_each_decimal_value_type!(values_type, |T| { + values + .iter() + .map(|value| { + ::from(value.unwrap_or_default()) + .vortex_expect("fixture values fit the stored width") + }) + .collect::>() + .into_byte_buffer() + }) +} + +/// Uses non-nullable validity when every value is present. +fn validity_of(values: &[Option]) -> Validity { + if values.iter().all(Option::is_some) { + return Validity::NonNullable; + } + + Validity::from_iter(values.iter().map(Option::is_some)) +} + +/// Returns the parts of decimal `values` stored as `values_type`. +fn stored_parts( + values: &[Option], + values_type: DecimalType, + decimal_dtype: DecimalDType, +) -> DecimalDataParts { + DecimalDataParts { + decimal_dtype, + values: BufferHandle::new_host(stored_buffer(values, values_type)), + values_type, + validity: validity_of(values), + } +} + +/// Returns `values` as the canonical array of `decimal_dtype`. +fn canonical_reference(values: &[Option], decimal_dtype: DecimalDType) -> DecimalArray { + let values_type = decimal_dtype.canonical_values_type(); + DecimalArray::new_handle( + BufferHandle::new_host(stored_buffer(values, values_type)), + values_type, + decimal_dtype, + validity_of(values), + ) +} + +/// Returns the decimal storage, through a [`Widen`] if present. +fn decimal_leaf(array: &ArrayRef) -> DecimalArray { + let leaf = match array.as_opt::() { + Some(widen) => widen.values().clone(), + None => array.clone(), + }; + + leaf.downcast::() +} + +fn i256_range(min: i128, max: i128) -> RangeInclusive { + i256::from_i128(min)..=i256::from_i128(max) +} + +/// Returns two `i32` zeros, with the alignment of `i32`. +fn two_i32_values() -> BufferHandle { + BufferHandle::new_host(Buffer::::zeroed(2).into_byte_buffer()) +} + +/// Returns a validity array of `valid`. `Validity::from_iter` would turn an all-true input into +/// `Validity::AllValid`, which has no length to check. +fn validity_array(valid: &[bool]) -> Validity { + Validity::Array(BoolArray::from_iter(valid.iter().copied()).into_array()) +} + +#[rstest] +#[case::canonical( + vec![Some(-999_999_999), None, Some(999_999_999)], + DecimalType::I32, DecimalDType::new(9, 2), None, true, +)] +#[case::stored_as_child( + vec![Some(-9_999), Some(9_999)], + DecimalType::I16, DecimalDType::new(18, 2), Some(4), true, +)] +#[case::narrowing_copy( + vec![Some(-99), None, Some(99)], + DecimalType::I32, DecimalDType::new(18, 2), Some(2), false, +)] +#[case::widening_copy( + vec![Some(32_767)], + DecimalType::I16, DecimalDType::new(10, 2), Some(5), false, +)] +#[case::no_narrower_child( + vec![Some(1_000_000_000)], + DecimalType::I32, DecimalDType::new(18, 2), None, false, +)] +#[case::scale_bound( + vec![Some(1)], + DecimalType::I8, DecimalDType::new(18, 10), None, false, +)] +#[case::negative_boundary( + vec![Some(-128)], + DecimalType::I8, DecimalDType::new(10, 0), Some(3), false, +)] +#[case::i128_narrowing_copy( + vec![Some(-99), Some(99)], + DecimalType::I128, DecimalDType::new(18, 2), Some(2), false, +)] +#[case::i128_canonical_copy( + vec![Some(100_000_000_000_000_000)], + DecimalType::I128, DecimalDType::new(18, 2), None, false, +)] +#[case::i128_stored_as_child( + vec![Some(10i128.pow(37))], + DecimalType::I128, DecimalDType::new(40, 2), Some(38), true, +)] +#[case::i128_max_widens_to_i256( + vec![Some(i128::MAX)], + DecimalType::I128, DecimalDType::new(40, 0), None, false, +)] +#[case::i256_narrowing_copy( + vec![Some(-99), None, Some(99)], + DecimalType::I256, DecimalDType::new(18, 2), Some(2), false, +)] +#[case::i256_canonical_copy( + vec![Some(10i128.pow(37))], + DecimalType::I256, DecimalDType::new(38, 0), None, false, +)] +#[case::all_null( + vec![None, None], + DecimalType::I32, DecimalDType::new(18, 2), Some(2), false, +)] +#[case::empty(vec![], DecimalType::I32, DecimalDType::new(18, 2), Some(2), false)] +fn test_from_stored_decimal( + #[case] values: Vec>, + #[case] values_type: DecimalType, + #[case] decimal_dtype: DecimalDType, + #[case] child_precision: Option, + #[case] must_reuse_buffer: bool, +) -> VortexResult<()> { + let mut ctx = SESSION.create_execution_ctx(); + let parts = stored_parts(&values, values_type, decimal_dtype); + let input_ptr = parts.values.as_host().as_ptr(); + + let result = WidenArray::from_stored_decimal(parts, Precision::Absent, &mut ctx)?; + + assert_eq!(result.is::(), child_precision.is_some()); + let leaf = decimal_leaf(&result); + let leaf_dtype = leaf.decimal_dtype(); + let expected_leaf_dtype = match child_precision { + Some(precision) => DecimalDType::new(precision, decimal_dtype.scale()), + None => decimal_dtype, + }; + assert_eq!(leaf_dtype, expected_leaf_dtype); + assert_eq!(leaf.values_type(), leaf_dtype.canonical_values_type()); + if must_reuse_buffer { + assert_eq!(leaf.buffer_handle().as_host().as_ptr(), input_ptr); + } + assert_arrays_eq!( + result, + canonical_reference(&values, decimal_dtype), + &mut ctx + ); + + Ok(()) +} + +/// Canonical storage is reused without checking values against the declared precision. +#[test] +fn test_from_stored_decimal_trusts_canonical_width() -> VortexResult<()> { + let mut ctx = SESSION.create_execution_ctx(); + let parts = stored_parts(&[Some(100)], DecimalType::I8, DecimalDType::new(2, 0)); + let input_ptr = parts.values.as_host().as_ptr(); + + let result = WidenArray::from_stored_decimal(parts, Precision::Absent, &mut ctx)?; + + assert!(result.is::()); + let decimal = result.as_::(); + assert_eq!(decimal.buffer_handle().as_host().as_ptr(), input_ptr); + assert_eq!(decimal.buffer::().as_slice(), [100]); + + Ok(()) +} + +#[rstest] +#[case::value_outside_precision( + vec![Some(1), Some(100)], Precision::Absent, "Decimal range must fit", +)] +#[case::reversed_range( + vec![Some(1), Some(2)], Precision::Inexact(i256_range(2, 1)), "ordered extrema", +)] +#[case::range_outside_precision( + vec![Some(1), Some(2)], Precision::Inexact(i256_range(1, 100)), "Decimal range must fit", +)] +fn test_from_stored_decimal_rejects_invalid_range( + #[case] values: Vec>, + #[case] range: Precision>, + #[case] expected: &str, +) -> VortexResult<()> { + let mut ctx = SESSION.create_execution_ctx(); + let parts = stored_parts(&values, DecimalType::I16, DecimalDType::new(2, 0)); + + let error = WidenArray::from_stored_decimal(parts, range, &mut ctx) + .expect_err("an invalid value range must fail"); + assert!(error.to_string().contains(expected), "{error}"); + + Ok(()) +} + +#[test] +fn test_from_stored_decimal_ignores_null_slots() -> VortexResult<()> { + let mut ctx = SESSION.create_execution_ctx(); + let decimal_dtype = DecimalDType::new(9, 0); + let validity = Validity::from_iter([false, true, false]); + let stored = Buffer::copy_from([i128::MIN, 5, i128::MAX]); + let parts = DecimalDataParts { + decimal_dtype, + values: BufferHandle::new_host(stored.into_byte_buffer()), + values_type: DecimalType::I128, + validity, + }; + + let result = WidenArray::from_stored_decimal(parts, Precision::Absent, &mut ctx)?; + + let child = decimal_leaf(&result); + assert_eq!(child.decimal_dtype(), DecimalDType::new(1, 0)); + assert_eq!(child.buffer::().as_slice(), [0, 5, 0]); + assert_arrays_eq!( + result, + canonical_reference(&[None, Some(5), None], decimal_dtype), + &mut ctx + ); + + Ok(()) +} + +/// Supplied ranges retain their exactness. An absent range records scanned extrema. +#[rstest] +#[case::scanned( + vec![Some(-5), None, Some(7)], DecimalType::I128, Precision::Absent, + 2, Precision::Exact((-5, 7)), +)] +#[case::exact( + vec![Some(-5), None, Some(7)], DecimalType::I32, Precision::Exact(i256_range(-5, 7)), + 2, Precision::Exact((-5, 7)), +)] +#[case::inexact( + vec![Some(1), Some(2)], DecimalType::I32, Precision::Inexact(i256_range(-9_999, 9_999)), 4, + Precision::Inexact((-9_999, 9_999)), +)] +fn test_from_stored_decimal_records_extrema( + #[case] values: Vec>, + #[case] values_type: DecimalType, + #[case] known_range: Precision>, + #[case] child_precision: u8, + #[case] expected: Precision<(i16, i16)>, +) -> VortexResult<()> { + let mut ctx = SESSION.create_execution_ctx(); + let decimal_dtype = DecimalDType::new(18, 2); + let parts = stored_parts(&values, values_type, decimal_dtype); + + let result = WidenArray::from_stored_decimal(parts, known_range, &mut ctx)?; + + let child_dtype = DecimalDType::new(child_precision, 2); + let child = decimal_leaf(&result); + assert_eq!(child.decimal_dtype(), child_dtype); + let statistics = child.statistics(); + let extremum = |value: i16| { + let value = DecimalValue::try_from_i256(i256::from_i128(value.into()), child_dtype) + .vortex_expect("expected extrema fit the child precision"); + Scalar::decimal(value, child_dtype, validity_of(&values).nullability()) + }; + assert_eq!( + statistics.get(Stat::Min), + expected.map(|(min, _)| extremum(min)) + ); + assert_eq!( + statistics.get(Stat::Max), + expected.map(|(_, max)| extremum(max)) + ); + assert_arrays_eq!( + result, + canonical_reference(&values, decimal_dtype), + &mut ctx + ); + + Ok(()) +} + +#[rstest] +#[case::length_not_a_multiple_of_width( + BufferHandle::new_host(ByteBuffer::copy_from([0u8; 5])), + Validity::NonNullable, + DecimalDType::new(9, 0), + "not divisible", +)] +#[case::misaligned( + BufferHandle::new_host(ByteBuffer::copy_from([0u8; 9]).slice_unaligned(1..9)), + Validity::NonNullable, + DecimalDType::new(9, 0), + "alignment", +)] +#[case::validity_too_short( + two_i32_values(), + validity_array(&[true]), + DecimalDType::new(9, 0), + "one entry per value", +)] +#[case::validity_too_long( + two_i32_values(), + validity_array(&[true, false, true]), + DecimalDType::new(18, 0), + "one entry per value", +)] +fn test_from_stored_decimal_rejects_malformed_buffer( + #[case] values: BufferHandle, + #[case] validity: Validity, + #[case] decimal_dtype: DecimalDType, + #[case] expected: &str, +) -> VortexResult<()> { + let mut ctx = SESSION.create_execution_ctx(); + let parts = DecimalDataParts { + decimal_dtype, + values, + values_type: DecimalType::I32, + validity, + }; + + let error = WidenArray::from_stored_decimal(parts, Precision::Absent, &mut ctx) + .expect_err("malformed parts must fail"); + assert!(error.to_string().contains(expected), "{error}"); + + Ok(()) +} diff --git a/vortex-array/src/arrays/widen/tests/mod.rs b/vortex-array/src/arrays/widen/tests/mod.rs index 337c8bc58eb..57c8b91508d 100644 --- a/vortex-array/src/arrays/widen/tests/mod.rs +++ b/vortex-array/src/arrays/widen/tests/mod.rs @@ -13,6 +13,7 @@ mod decimal; mod decimal_aggregates; mod decimal_compare; mod decimal_fallback; +mod decimal_stored; mod encoding; mod numeric; mod serde; diff --git a/vortex-array/src/arrays/widen/vtable/plugin.rs b/vortex-array/src/arrays/widen/vtable/plugin.rs index bc2a92e42a4..5aa707c7c05 100644 --- a/vortex-array/src/arrays/widen/vtable/plugin.rs +++ b/vortex-array/src/arrays/widen/vtable/plugin.rs @@ -5,6 +5,9 @@ //! //! The node stores the child's buffer width, while its containing format supplies the logical dtype //! of the `Widen`. No `Widen` node or child decimal dtype is written. +//! +//! This uses the legacy decimal wire format because [`Widen`] has no wire representation of its +//! own. Noncanonical stored widths can therefore come from current writers as well as old files. use vortex_error::VortexResult; use vortex_error::vortex_bail;