From 30e3260d658902cf19594fa9037f357d858b0441 Mon Sep 17 00:00:00 2001 From: Connor Tsui Date: Tue, 3 Nov 2026 14:32:27 -0500 Subject: [PATCH 1/5] Read stored decimal values into the canonical array or a Widen Storage width alone cannot determine a valid decimal child precision. For example, unscaled 127 fits i8 but requires precision 3 and an i16 canonical child. Share child selection between encoding and stored-value reads so comparisons and aggregates can trust the selected child dtype. Reuse canonical buffers without scanning, including device buffers. Resolve other widths on the host from valid-value bounds and scale, retaining the range exactness in child statistics. Validate buffer layout and validity length before typed access, and use checked constructors. Signed-off-by: Connor Tsui --- vortex-array/src/arrays/decimal/array.rs | 40 +- vortex-array/src/arrays/decimal/vtable/mod.rs | 6 +- .../src/arrays/widen/encode/decimal.rs | 286 ++++++++++++++ .../widen/{encode.rs => encode/integer.rs} | 105 +---- vortex-array/src/arrays/widen/encode/mod.rs | 15 + vortex-array/src/arrays/widen/mod.rs | 5 +- .../src/arrays/widen/tests/decimal_stored.rs | 373 ++++++++++++++++++ vortex-array/src/arrays/widen/tests/mod.rs | 1 + .../src/arrays/widen/vtable/plugin.rs | 3 + 9 files changed, 721 insertions(+), 113 deletions(-) create mode 100644 vortex-array/src/arrays/widen/encode/decimal.rs rename vortex-array/src/arrays/widen/{encode.rs => encode/integer.rs} (50%) create mode 100644 vortex-array/src/arrays/widen/encode/mod.rs create mode 100644 vortex-array/src/arrays/widen/tests/decimal_stored.rs diff --git a/vortex-array/src/arrays/decimal/array.rs b/vortex-array/src/arrays/decimal/array.rs index 61aaf898515..5a5ee560209 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. Precision is trusted, as in other checked + // decimal constructors, and violations can produce incorrect results. 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/encode/decimal.rs b/vortex-array/src/arrays/widen/encode/decimal.rs new file mode 100644 index 00000000000..3560f3bc3ce --- /dev/null +++ b/vortex-array/src/arrays/widen/encode/decimal.rs @@ -0,0 +1,286 @@ +// SPDX-License-Identifier: Apache-2.0 +// SPDX-FileCopyrightText: Copyright the Vortex contributors + +//! Decimal encoding and stored-value reads with valid child precision. +//! +//! Child precision depends on valid values and scale, not just storage width. For example, +//! unscaled 127 fits `i8` but needs precision 3, whose canonical width is `i16`. Both entry points +//! share child selection and cache its value range for later aggregates. +//! +//! [`WidenArray::from_stored_decimal`] is the conversion boundary for legacy decimal storage and +//! decoded DecimalByteParts buffers. + +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_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. + /// + /// 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. The child records + /// the exact extrema of the valid values as its [`Stat::Min`] and [`Stat::Max`]. + /// + /// # 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 = scan_valid_range(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()) + } + + /// Reads stored decimal values as a canonical array or a [`WidenArray`]. + /// + /// Keeps the dtype and validity in `parts`. Canonical-width storage is reused without reading + /// values, including device buffers. Its declared precision is trusted, and `known_range` is + /// ignored. + /// + /// Other widths use host memory and select a child as [`Self::encode_decimal`] does. Validity + /// **must** be executable on the host. The child reuses matching stored widths. + /// Otherwise the values are copied to the selected width, with null slots zeroed on narrowing. + /// If no child is narrower than the logical dtype, the result is a canonical array. + /// + /// For noncanonical storage, `known_range` controls child selection: + /// + /// - [`Precision::Absent`] scans and caches exact extrema on the selected child. + /// - [`Precision::Exact`] **must** give the actual minimum and maximum valid unscaled values. + /// - [`Precision::Inexact`] **must** contain every valid unscaled value. + /// + /// Supplied ranges retain their exactness in the child's [`Stat::Min`] and [`Stat::Max`]. + /// An incorrect range 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!( + validity_len == len, + "Decimal validity must have one entry per value ({len}), got {validity_len}" + ); + } + + 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 = match known_range { + Precision::Exact(extrema) => ValidRange::Exact(extrema), + Precision::Inexact(bounds) => ValidRange::Bounds(bounds), + Precision::Absent => scan_valid_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. +/// +/// Precision is at least 1 and at least the scale. An absent range represents empty or all-null +/// input. 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}") +} + +/// The range of the valid decimal values that selects a decimal child. +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 { + /// 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()), + } + } +} + +/// Scans valid values, keeping empty input distinct from supplied bounds. +fn scan_valid_range(values: &[F], mask: &Mask) -> ValidRange { + match valid_range(values, mask) { + Some((min, max)) => { + let min = min.to_i256().vortex_expect("every decimal value fits i256"); + let max = max.to_i256().vortex_expect("every decimal value fits i256"); + ValidRange::Exact(min..=max) + } + None => ValidRange::Empty, + } +} + +/// Wraps a narrower child in a [`WidenArray`], or copies to the logical canonical width. +/// +/// `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 buffer = copy_to_canonical_width(values.as_slice(), mask, decimal_dtype)?; + + let array = DecimalArray::try_new_handle( + buffer, + decimal_dtype.canonical_values_type(), + decimal_dtype, + validity, + )?; + + Ok(array.into_array()) +} + +/// Builds the narrowest canonical child, or returns `None` if no narrower width exists. +/// +/// Every valid value **must** lie in `range`, and `mask` **must** be the mask of `validity`. +/// +/// Reuses storage at the selected width. Other widths use [`copy_to_canonical_width`]. The child +/// caches `range` as its extrema, preserving exactness. +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); + } + + // The copy helper allocates even at the same width, so reuse matching storage here. + let buffer = if F::DECIMAL_TYPE == child_type { + BufferHandle::new_host(values.clone().into_byte_buffer()) + } else { + copy_to_canonical_width(values.as_slice(), mask, child_dtype)? + }; + + let child = DecimalArray::try_new_handle(buffer, child_type, child_dtype, validity.clone())?; + + let extrema = match range { + ValidRange::Empty => return Ok(Some(child)), + ValidRange::Exact(bounds) => Precision::Exact(bounds), + ValidRange::Bounds(bounds) => Precision::Inexact(bounds), + }; + 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); + + Ok(Some(child)) +} + +/// Builds a statistic at the child's canonical width. `value` **must** fit its 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.rs b/vortex-array/src/arrays/widen/encode/integer.rs similarity index 50% rename from vortex-array/src/arrays/widen/encode.rs rename to vortex-array/src/arrays/widen/encode/integer.rs index 141fccde6d2..0ee0e735f4d 100644 --- a/vortex-array/src/arrays/widen/encode.rs +++ b/vortex-array/src/arrays/widen/encode/integer.rs @@ -1,15 +1,14 @@ // SPDX-License-Identifier: Apache-2.0 // SPDX-FileCopyrightText: Copyright the Vortex contributors -//! Selection of narrower `Widen` children from valid values. +//! Integer narrowing that preserves signedness and excludes null payloads. //! -//! 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. +//! 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::VortexResult; -use vortex_error::vortex_bail; use vortex_error::vortex_ensure; use crate::ArrayRef; @@ -17,21 +16,12 @@ 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 { @@ -45,51 +35,6 @@ impl WidenArray { 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. /// @@ -146,7 +91,10 @@ fn narrow_values(array: PrimitiveArray, ctx: &mut ExecutionCtx) -> VortexResult< /// 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 { +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()?); @@ -160,47 +108,10 @@ pub(super) fn smallest_integer_ptype(bounds: RangeInclusive, signed: bool) } /// Returns the value of a non-null integer scalar of at most 64 bits. -pub(super) fn integer_value(scalar: &Scalar) -> VortexResult { +pub(in crate::arrays::widen) 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/mod.rs b/vortex-array/src/arrays/widen/encode/mod.rs new file mode 100644 index 00000000000..d1b890397e7 --- /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_value; +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; From e9f0de5cb3fef1ccfb095980ef84f893085c020c Mon Sep 17 00:00:00 2001 From: Connor Tsui Date: Tue, 3 Nov 2026 15:12:03 -0500 Subject: [PATCH 2/5] Use PValue::as_i128 for Widen integer bounds Remove the local signed/unsigned conversion helper. Its callers already establish non-null integer values, and PValue::as_i128 holds every integer PValue exactly. The scalar type-selection helper is now infallible. Signed-off-by: Connor Tsui --- .../src/arrays/widen/compute/fill_null.rs | 2 +- vortex-array/src/arrays/widen/compute/mod.rs | 25 ++++++++++--------- .../src/arrays/widen/compute/numeric.rs | 14 ++++++++--- vortex-array/src/arrays/widen/compute/zip.rs | 2 +- .../src/arrays/widen/encode/integer.rs | 24 ++++++++++-------- vortex-array/src/arrays/widen/encode/mod.rs | 1 - 6 files changed, 38 insertions(+), 30 deletions(-) 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..bcb282c1e75 100644 --- a/vortex-array/src/arrays/widen/compute/numeric.rs +++ b/vortex-array/src/arrays/widen/compute/numeric.rs @@ -25,12 +25,12 @@ 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::smallest_integer_ptype; use crate::dtype::DType; use crate::dtype::PType; use crate::kernel::ExecuteParentKernel; use crate::scalar::NumericOperator; +use crate::scalar::PValue; use crate::scalar::Scalar; use crate::scalar_fn::fns::binary::Binary; use crate::scalar_fn::fns::binary::execute_numeric; @@ -162,9 +162,15 @@ fn valid_value_interval( return Ok(None); }; - Ok(Some( - integer_value(&bounds.min)?..=integer_value(&bounds.max)?, - )) + 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") + }); + + Ok(Some(min..=max)) } /// 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/integer.rs b/vortex-array/src/arrays/widen/encode/integer.rs index 0ee0e735f4d..c8f3759dacc 100644 --- a/vortex-array/src/arrays/widen/encode/integer.rs +++ b/vortex-array/src/arrays/widen/encode/integer.rs @@ -8,6 +8,7 @@ use std::ops::RangeInclusive; +use vortex_error::VortexExpect; use vortex_error::VortexResult; use vortex_error::vortex_ensure; @@ -22,7 +23,7 @@ use crate::arrays::widen::compute::wrap_values; use crate::builtins::ArrayBuiltins; use crate::dtype::DType; use crate::dtype::PType; -use crate::scalar::Scalar; +use crate::scalar::PValue; impl WidenArray { /// Stores `array` as a `Widen` over the smallest integer type with the same signedness that @@ -73,7 +74,17 @@ fn narrow_values(array: PrimitiveArray, ctx: &mut ExecutionCtx) -> VortexResult< // 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)?, + Some(bounds) => { + 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 + } None => 0..=0, }; let Some(stored_ptype) = smallest_integer_ptype(bounds, ptype.is_signed_int()) @@ -106,12 +117,3 @@ pub(in crate::arrays::widen) fn smallest_integer_ptype( Some(min_ptype.max_unsigned_ptype(max_ptype)) } } - -/// Returns the value of a non-null integer scalar of at most 64 bits. -pub(in crate::arrays::widen) 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) - } -} diff --git a/vortex-array/src/arrays/widen/encode/mod.rs b/vortex-array/src/arrays/widen/encode/mod.rs index d1b890397e7..8e714613f93 100644 --- a/vortex-array/src/arrays/widen/encode/mod.rs +++ b/vortex-array/src/arrays/widen/encode/mod.rs @@ -11,5 +11,4 @@ mod decimal; pub(crate) use decimal::smallest_decimal_dtype; mod integer; -pub(super) use integer::integer_value; pub(super) use integer::smallest_integer_ptype; From fe24fc7d7a7f66beb7a23918c8b5df15b49c4184 Mon Sep 17 00:00:00 2001 From: Connor Tsui Date: Tue, 3 Nov 2026 15:31:41 -0500 Subject: [PATCH 3/5] Clarify stored decimal conversion and share canonical construction Explain why legacy decimal nodes and DecimalByteParts output need canonical conversion, including why only noncanonical widths require host access. Keep range resolution and statistic exactness together, share canonical array construction, and use vortex_ensure_eq for validity length. Signed-off-by: Connor Tsui --- .../src/arrays/widen/encode/decimal.rs | 199 +++++++++++------- 1 file changed, 125 insertions(+), 74 deletions(-) diff --git a/vortex-array/src/arrays/widen/encode/decimal.rs b/vortex-array/src/arrays/widen/encode/decimal.rs index 3560f3bc3ce..a52033d45ea 100644 --- a/vortex-array/src/arrays/widen/encode/decimal.rs +++ b/vortex-array/src/arrays/widen/encode/decimal.rs @@ -1,14 +1,21 @@ // SPDX-License-Identifier: Apache-2.0 // SPDX-FileCopyrightText: Copyright the Vortex contributors -//! Decimal encoding and stored-value reads with valid child precision. +//! Decimal encoding and conversion of legacy storage into canonical decimal leaves. //! -//! Child precision depends on valid values and scale, not just storage width. For example, -//! unscaled 127 fits `i8` but needs precision 3, whose canonical width is `i16`. Both entry points -//! share child selection and cache its value range for later aggregates. +//! 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. //! -//! [`WidenArray::from_stored_decimal`] is the conversion boundary for legacy decimal storage and -//! decoded DecimalByteParts buffers. +//! 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; @@ -17,6 +24,7 @@ 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; @@ -44,10 +52,13 @@ use crate::validity::Validity; impl WidenArray { /// 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. The child records - /// the exact extrema of the valid values as its [`Stat::Min`] and [`Stat::Max`]. + /// 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 /// @@ -58,7 +69,8 @@ impl WidenArray { 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 = scan_valid_range(values_buffer.as_slice(), &validity_mask); + let valid_value_range = + ValidRange::resolve(Precision::Absent, values_buffer.as_slice(), &validity_mask); narrowest_decimal_child( &values_buffer, &validity, @@ -75,24 +87,32 @@ impl WidenArray { Ok(WidenArray::try_new(child.into_array(), array.dtype().clone())?.into_array()) } - /// Reads stored decimal values as a canonical array or a [`WidenArray`]. + /// Converts stored decimals into a canonical leaf or a [`WidenArray`] over one. /// - /// Keeps the dtype and validity in `parts`. Canonical-width storage is reused without reading - /// values, including device buffers. Its declared precision is trusted, and `known_range` is - /// ignored. + /// 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. /// - /// Other widths use host memory and select a child as [`Self::encode_decimal`] does. Validity - /// **must** be executable on the host. The child reuses matching stored widths. - /// Otherwise the values are copied to the selected width, with null slots zeroed on narrowing. - /// If no child is narrower than the logical dtype, the result is a canonical array. + /// 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. + /// Narrowing zeroes null slots. /// - /// For noncanonical storage, `known_range` controls child selection: + /// 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. /// - /// - [`Precision::Absent`] scans and caches exact extrema on the selected child. + /// 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. /// - /// Supplied ranges retain their exactness in the child's [`Stat::Min`] and [`Stat::Max`]. + /// 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`. /// An incorrect range or declared precision can cause incorrect results in later operations. /// /// # Errors @@ -116,9 +136,10 @@ impl WidenArray { DecimalData::validate(&values, values_type)?; let len = values.len() / values_type.byte_width(); if let Some(validity_len) = validity.maybe_len() { - vortex_ensure!( - validity_len == len, - "Decimal validity must have one entry per value ({len}), got {validity_len}" + vortex_ensure_eq!( + validity_len, + len, + "Decimal validity must have one entry per value" ); } @@ -132,11 +153,7 @@ impl WidenArray { let mask = validity.execute_mask(len, ctx)?; match_each_decimal_value_type!(values_type, |T| { let values = Buffer::::from_byte_buffer(values); - let range = match known_range { - Precision::Exact(extrema) => ValidRange::Exact(extrema), - Precision::Inexact(bounds) => ValidRange::Bounds(bounds), - Precision::Absent => scan_valid_range(values.as_slice(), &mask), - }; + let range = ValidRange::resolve(known_range, values.as_slice(), &mask); widen_or_canonical(values, validity, &mask, range, decimal_dtype) }) } @@ -144,8 +161,10 @@ impl WidenArray { /// Chooses child precision from a valid-value range and the parent scale. /// -/// Precision is at least 1 and at least the scale. An absent range represents empty or all-null -/// input. Null payloads **must** be excluded from the range. +/// 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 /// @@ -175,7 +194,10 @@ pub(crate) fn smallest_decimal_dtype( vortex_bail!("Decimal range must fit {parent}, got {min} to {max}") } -/// The range of the valid decimal values that selects a decimal child. +/// 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, @@ -186,6 +208,30 @@ enum ValidRange { } 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 { @@ -193,21 +239,29 @@ impl ValidRange { Self::Exact(bounds) | Self::Bounds(bounds) => Some(bounds.clone()), } } -} -/// Scans valid values, keeping empty input distinct from supplied bounds. -fn scan_valid_range(values: &[F], mask: &Mask) -> ValidRange { - match valid_range(values, mask) { - Some((min, max)) => { - let min = min.to_i256().vortex_expect("every decimal value fits i256"); - let max = max.to_i256().vortex_expect("every decimal value fits i256"); - ValidRange::Exact(min..=max) - } - None => ValidRange::Empty, + /// 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 child in a [`WidenArray`], or copies to the logical canonical width. +/// 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( @@ -222,24 +276,16 @@ fn widen_or_canonical( return Ok(WidenArray::try_new(child.into_array(), dtype)?.into_array()); } - let buffer = copy_to_canonical_width(values.as_slice(), mask, decimal_dtype)?; - - let array = DecimalArray::try_new_handle( - buffer, - decimal_dtype.canonical_values_type(), - decimal_dtype, - validity, - )?; + 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. /// -/// Every valid value **must** lie in `range`, and `mask` **must** be the mask of `validity`. +/// Reducing precision without changing width saves no space, so we return `None` in that case. /// -/// Reuses storage at the selected width. Other widths use [`copy_to_canonical_width`]. The child -/// caches `range` as its extrema, preserving exactness. +/// Every valid value **must** lie in `range`, and `mask` **must** be the mask of `validity`. fn narrowest_decimal_child( values: &Buffer, validity: &Validity, @@ -253,31 +299,36 @@ fn narrowest_decimal_child( return Ok(None); } - // The copy helper allocates even at the same width, so reuse matching storage here. - let buffer = if F::DECIMAL_TYPE == child_type { - BufferHandle::new_host(values.clone().into_byte_buffer()) - } else { - copy_to_canonical_width(values.as_slice(), mask, child_dtype)? - }; + let child = build_canonical_decimal(values, validity, mask, child_dtype)?; + range.cache_statistics(&child); - let child = DecimalArray::try_new_handle(buffer, child_type, child_dtype, validity.clone())?; + Ok(Some(child)) +} - let extrema = match range { - ValidRange::Empty => return Ok(Some(child)), - ValidRange::Exact(bounds) => Precision::Exact(bounds), - ValidRange::Bounds(bounds) => Precision::Inexact(bounds), +/// 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)? }; - 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); - Ok(Some(child)) + DecimalArray::try_new_handle(buffer, values_type, decimal_dtype, validity.clone()) } -/// Builds a statistic at the child's canonical width. `value` **must** fit its precision. +/// 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"); From 1154a335a504db15c5e9865f4e503fadd88ca3c4 Mon Sep 17 00:00:00 2001 From: Connor Tsui Date: Tue, 3 Nov 2026 16:18:29 -0500 Subject: [PATCH 4/5] Clarify decimal conversion contracts and share integer extrema Signed-off-by: Connor Tsui --- .../src/arrays/widen/compute/aggregate.rs | 2 ++ .../src/arrays/widen/compute/numeric.rs | 12 ++------ .../src/arrays/widen/encode/decimal.rs | 2 +- .../src/arrays/widen/encode/integer.rs | 28 +++++++++++-------- vortex-array/src/arrays/widen/encode/mod.rs | 1 + 5 files changed, 23 insertions(+), 22 deletions(-) diff --git a/vortex-array/src/arrays/widen/compute/aggregate.rs b/vortex-array/src/arrays/widen/compute/aggregate.rs index 731505109c7..79d75868b92 100644 --- a/vortex-array/src/arrays/widen/compute/aggregate.rs +++ b/vortex-array/src/arrays/widen/compute/aggregate.rs @@ -86,6 +86,8 @@ fn aggregate_decimal( return Ok(Some(partial)); } + // D36 defines the decimal allowlist. Registrations cover the encoding, not the dtype, so + // every other decimal aggregate must keep the default path. if !(aggregate_fn.is::() || aggregate_fn.is::() || aggregate_fn.is::() diff --git a/vortex-array/src/arrays/widen/compute/numeric.rs b/vortex-array/src/arrays/widen/compute/numeric.rs index bcb282c1e75..84046fbc855 100644 --- a/vortex-array/src/arrays/widen/compute/numeric.rs +++ b/vortex-array/src/arrays/widen/compute/numeric.rs @@ -25,12 +25,12 @@ 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_extrema; use crate::arrays::widen::encode::smallest_integer_ptype; use crate::dtype::DType; use crate::dtype::PType; use crate::kernel::ExecuteParentKernel; use crate::scalar::NumericOperator; -use crate::scalar::PValue; use crate::scalar::Scalar; use crate::scalar_fn::fns::binary::Binary; use crate::scalar_fn::fns::binary::execute_numeric; @@ -162,15 +162,7 @@ fn valid_value_interval( return Ok(None); }; - 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") - }); - - Ok(Some(min..=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/encode/decimal.rs b/vortex-array/src/arrays/widen/encode/decimal.rs index a52033d45ea..74fa656d512 100644 --- a/vortex-array/src/arrays/widen/encode/decimal.rs +++ b/vortex-array/src/arrays/widen/encode/decimal.rs @@ -97,7 +97,7 @@ impl WidenArray { /// 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. - /// Narrowing zeroes null slots. + /// 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 diff --git a/vortex-array/src/arrays/widen/encode/integer.rs b/vortex-array/src/arrays/widen/encode/integer.rs index c8f3759dacc..f9758562f1b 100644 --- a/vortex-array/src/arrays/widen/encode/integer.rs +++ b/vortex-array/src/arrays/widen/encode/integer.rs @@ -16,6 +16,7 @@ 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; @@ -74,17 +75,7 @@ fn narrow_values(array: PrimitiveArray, ctx: &mut ExecutionCtx) -> VortexResult< // 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) => { - 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 - } + Some(bounds) => integer_extrema(bounds), None => 0..=0, }; let Some(stored_ptype) = smallest_integer_ptype(bounds, ptype.is_signed_int()) @@ -100,6 +91,21 @@ fn narrow_values(array: PrimitiveArray, ctx: &mut ExecutionCtx) -> VortexResult< .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( diff --git a/vortex-array/src/arrays/widen/encode/mod.rs b/vortex-array/src/arrays/widen/encode/mod.rs index 8e714613f93..da2219d8044 100644 --- a/vortex-array/src/arrays/widen/encode/mod.rs +++ b/vortex-array/src/arrays/widen/encode/mod.rs @@ -11,4 +11,5 @@ mod decimal; pub(crate) use decimal::smallest_decimal_dtype; mod integer; +pub(super) use integer::integer_extrema; pub(super) use integer::smallest_integer_ptype; From d2a9738ea33c571c8afeedb9954e71a13b3dedc4 Mon Sep 17 00:00:00 2001 From: Connor Tsui Date: Tue, 3 Nov 2026 16:33:49 -0500 Subject: [PATCH 5/5] Make decimal conversion comments self-contained Signed-off-by: Connor Tsui --- vortex-array/src/arrays/decimal/array.rs | 4 ++-- vortex-array/src/arrays/widen/compute/aggregate.rs | 5 +++-- vortex-array/src/arrays/widen/encode/decimal.rs | 3 ++- 3 files changed, 7 insertions(+), 5 deletions(-) diff --git a/vortex-array/src/arrays/decimal/array.rs b/vortex-array/src/arrays/decimal/array.rs index 5a5ee560209..a07dc1ffbf7 100644 --- a/vortex-array/src/arrays/decimal/array.rs +++ b/vortex-array/src/arrays/decimal/array.rs @@ -256,8 +256,8 @@ impl DecimalData { ) -> VortexResult { Self::validate(&values, values_type)?; - // SAFETY: `validate` checks the buffer layout. Precision is trusted, as in other checked - // decimal constructors, and violations can produce incorrect results. + // 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) }) } diff --git a/vortex-array/src/arrays/widen/compute/aggregate.rs b/vortex-array/src/arrays/widen/compute/aggregate.rs index 79d75868b92..6552df57d0f 100644 --- a/vortex-array/src/arrays/widen/compute/aggregate.rs +++ b/vortex-array/src/arrays/widen/compute/aggregate.rs @@ -86,8 +86,9 @@ fn aggregate_decimal( return Ok(Some(partial)); } - // D36 defines the decimal allowlist. Registrations cover the encoding, not the dtype, so - // every other decimal aggregate must keep the default path. + // 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/encode/decimal.rs b/vortex-array/src/arrays/widen/encode/decimal.rs index 74fa656d512..dc7ed5e0db6 100644 --- a/vortex-array/src/arrays/widen/encode/decimal.rs +++ b/vortex-array/src/arrays/widen/encode/decimal.rs @@ -113,7 +113,8 @@ impl WidenArray { /// 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`. - /// An incorrect range or declared precision can cause incorrect results in later operations. + /// 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 ///