From 7cc6179e8c6edce908dda3ce47ade804db7467ce Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 31 Jul 2026 13:32:12 +0000 Subject: [PATCH 01/14] Support all decimal widths in DecimalByteParts via lower parts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `DecimalByteParts` reserved a `lower_parts` field but never populated it: the encoding only ever held a single signed most significant part, so decimals wider than 64 bits after narrowing were left uncompressed as raw `i128`/`i256` buffers, and `deserialize` asserted `lower_part_count == 0`. The encoding now stores the reserved lower parts. A value is a signed MSP plus `k` non-nullable `u64` parts ordered most significant first, which is the value's two's complement bit pattern cut on 64-bit boundaries: msp * 2^(64k) + Σ lower[i] * 2^(64 * (k - 1 - i)) `i128` splits into an `i64` MSP and one lower part, `i256` into an `i64` MSP and three. `split_decimal` / `assemble_decimal` in the new `limbs` module are the single definition of that layout, used by the encoding's canonicalization and by the compressor. Encoding changes: - `lower_parts` becomes a variadic slot tail, so parts are ordinary children: written and read by serde, with the child count checked against `lower_part_count` rather than asserted to be zero. - Canonicalization and `scalar_at` reassemble the parts, widening to `i128` or `i256` depending on the MSP width and part count. - `filter`, `take`, `slice` and the parent filter push-down apply to every part; `mask` and nullability `cast` touch only the MSP, which carries validity; `is_constant` requires every part to be constant, except for an all-null array whose lower parts hold undefined bits. - The `compare` push-down against a constant now bails when lower parts are present — the MSP alone no longer determines the ordering — and falls back to the canonical comparison. - The CUDA executor bails for arrays with lower parts instead of decoding the MSP as the whole value. Compressor changes: - `DecimalScheme` splits post-narrowing `i128`/`i256` arrays and cascades into each part instead of returning the decimal uncompressed. Tests: - Split/assemble round trips over both limb boundaries and both signs, at `i128::MIN/MAX` and `i256::MIN/MAX`. - Consistency, filter, cast and binary-numeric conformance suites over arrays with one and three lower parts, nullable and non-nullable. - Serde round trips for 0, 1 and 3 lower parts, asserting the part count survives, plus `deserialize` rejecting child-count and bound violations. - Construction rejects signed, nullable, mis-sized and too-many lower parts. - Compressor tests pinning one lower part for `i128`, three for `i256`, and the canonical storage width of the result. - Compression ratio: 16k wide values with 24 bits of noise compress 5.3x (`i128`) and 10.7x (`i256`) at the array level, and 7.4x through a Vortex file end to end, where before splitting they were stored raw. - A wide-decimal column added to the compat fixture so future readers must decode today's lower parts. Signed-off-by: "Joe Isaacs" --- .../src/decimal_byte_parts/compute/cast.rs | 19 +- .../src/decimal_byte_parts/compute/compare.rs | 47 ++ .../src/decimal_byte_parts/compute/filter.rs | 40 +- .../decimal_byte_parts/compute/is_constant.rs | 27 +- .../src/decimal_byte_parts/compute/mask.rs | 5 +- .../src/decimal_byte_parts/compute/mod.rs | 33 + .../src/decimal_byte_parts/compute/take.rs | 9 +- .../src/decimal_byte_parts/limbs.rs | 379 +++++++++++ .../src/decimal_byte_parts/mod.rs | 622 +++++++++++++++--- .../src/decimal_byte_parts/rules.rs | 16 +- .../src/decimal_byte_parts/slice.rs | 9 +- .../src/decimal_byte_parts/testing.rs | 51 ++ .../schemes/{decimal.rs => decimal/mod.rs} | 40 +- vortex-btrblocks/src/schemes/decimal/tests.rs | 189 ++++++ .../kernel/encodings/decimal_byte_parts.rs | 7 + vortex-file/src/tests.rs | 82 +++ .../synthetic/encodings/decimal_byte_parts.rs | 41 ++ 17 files changed, 1509 insertions(+), 107 deletions(-) create mode 100644 encodings/decimal-byte-parts/src/decimal_byte_parts/limbs.rs create mode 100644 encodings/decimal-byte-parts/src/decimal_byte_parts/testing.rs rename vortex-btrblocks/src/schemes/{decimal.rs => decimal/mod.rs} (68%) create mode 100644 vortex-btrblocks/src/schemes/decimal/tests.rs diff --git a/encodings/decimal-byte-parts/src/decimal_byte_parts/compute/cast.rs b/encodings/decimal-byte-parts/src/decimal_byte_parts/compute/cast.rs index 5ae1bf0101e..e4abd7baa7b 100644 --- a/encodings/decimal-byte-parts/src/decimal_byte_parts/compute/cast.rs +++ b/encodings/decimal-byte-parts/src/decimal_byte_parts/compute/cast.rs @@ -30,7 +30,12 @@ impl CastReduce for DecimalByteParts { .cast(array.msp().dtype().with_nullability(*target_nullability))?; Ok(Some( - DecimalByteParts::try_new(new_msp, *target_decimal)?.into_array(), + DecimalByteParts::try_new_with_lower_parts( + new_msp, + array.lower_parts().to_vec(), + *target_decimal, + )? + .into_array(), )) } } @@ -49,10 +54,14 @@ mod tests { use vortex_array::dtype::DType; use vortex_array::dtype::DecimalDType; use vortex_array::dtype::Nullability; + use vortex_array::validity::Validity; use vortex_buffer::buffer; use crate::DecimalByteParts; use crate::DecimalBytePartsArray; + use crate::decimal_byte_parts::testing::i128_parts; + use crate::decimal_byte_parts::testing::i256_of; + use crate::decimal_byte_parts::testing::i256_parts; #[test] fn test_cast_decimal_byte_parts_nullability() { @@ -117,6 +126,14 @@ mod tests { buffer![-100i32, -200, 300, -400, 500].into_array(), DecimalDType::new(10, 2), ).unwrap())] + #[case::one_lower_part(i128_parts( + vec![1i128 << 70, -(1i128 << 70), 5, (1i128 << 64) - 1, 0], + Validity::NonNullable, + ))] + #[case::three_lower_parts(i256_parts( + vec![i256_of(1, 0), i256_of(-1, 5), i256_of(0, u128::MAX)], + Validity::NonNullable, + ))] fn test_cast_decimal_byte_parts_conformance(#[case] array: DecimalBytePartsArray) { test_cast_conformance( &array.into_array(), diff --git a/encodings/decimal-byte-parts/src/decimal_byte_parts/compute/compare.rs b/encodings/decimal-byte-parts/src/decimal_byte_parts/compute/compare.rs index 3044bd6e605..fe4d69801a3 100644 --- a/encodings/decimal-byte-parts/src/decimal_byte_parts/compute/compare.rs +++ b/encodings/decimal-byte-parts/src/decimal_byte_parts/compute/compare.rs @@ -39,6 +39,12 @@ impl CompareKernel for DecimalByteParts { return Ok(None); }; + // The MSP alone only determines the ordering when it holds the whole value. With + // lower parts present, fall back to comparing the canonical decimal. + if !lhs.lower_parts().is_empty() { + return Ok(None); + } + let nullability = lhs.dtype().nullability() | rhs.dtype().nullability(); let scalar_type = lhs.msp().dtype().with_nullability(nullability); @@ -158,10 +164,12 @@ mod tests { use vortex_array::scalar_fn::fns::operators::Operator; use vortex_array::validity::Validity; use vortex_buffer::buffer; + use vortex_error::VortexExpect; use vortex_error::VortexResult; use vortex_session::VortexSession; use crate::DecimalByteParts; + use crate::decimal_byte_parts::testing::i128_parts; static SESSION: LazyLock = LazyLock::new(|| { let session = vortex_array::array_session(); @@ -220,6 +228,45 @@ mod tests { Ok(()) } + #[test] + fn compare_decimal_const_with_lower_parts() -> VortexResult<()> { + // The MSP-only pushdown is invalid once lower parts carry part of the value, so this + // must fall back to the canonical comparison rather than compare MSPs. + let values = vec![1i128 << 70, (1i128 << 70) + 1, 5, -(1i128 << 70)]; + let lhs = i128_parts(values.clone(), Validity::NonNullable).into_array(); + let decimal_dtype = *lhs + .dtype() + .as_decimal_opt() + .vortex_expect("decimal byte parts array"); + + let pivot = (1i128 << 70) + 1; + let rhs = ConstantArray::new( + Scalar::decimal( + DecimalValue::I128(pivot), + decimal_dtype, + Nullability::NonNullable, + ), + lhs.len(), + ) + .into_array(); + + let mut ctx = SESSION.create_execution_ctx(); + for (operator, predicate) in [ + (Operator::Eq, (|v, p| v == p) as fn(i128, i128) -> bool), + (Operator::NotEq, |v, p| v != p), + (Operator::Lt, |v, p| v < p), + (Operator::Lte, |v, p| v <= p), + (Operator::Gt, |v, p| v > p), + (Operator::Gte, |v, p| v >= p), + ] { + let res = lhs.clone().binary(rhs.clone(), operator)?; + let expected = + BoolArray::from_iter(values.iter().map(|v| predicate(*v, pivot))).into_array(); + assert_arrays_eq!(res, expected, &mut ctx); + } + Ok(()) + } + #[test] fn compare_decimal_const_unconvertible_comparison() { let decimal_dtype = DecimalDType::new(40, 2); diff --git a/encodings/decimal-byte-parts/src/decimal_byte_parts/compute/filter.rs b/encodings/decimal-byte-parts/src/decimal_byte_parts/compute/filter.rs index a47a6ed846b..e6aea7dd9c8 100644 --- a/encodings/decimal-byte-parts/src/decimal_byte_parts/compute/filter.rs +++ b/encodings/decimal-byte-parts/src/decimal_byte_parts/compute/filter.rs @@ -13,8 +13,15 @@ use crate::DecimalByteParts; use crate::decimal_byte_parts::DecimalBytePartsArraySlotsExt; impl FilterReduce for DecimalByteParts { fn filter(array: ArrayView<'_, Self>, mask: &Mask) -> VortexResult> { - DecimalByteParts::try_new( + let lower_parts = array + .lower_parts() + .iter() + .map(|part| part.filter(mask.clone())) + .collect::>>()?; + + DecimalByteParts::try_new_with_lower_parts( array.msp().filter(mask.clone())?, + lower_parts, *array .dtype() .as_decimal_opt() @@ -32,9 +39,13 @@ mod test { use vortex_array::arrays::PrimitiveArray; use vortex_array::compute::conformance::filter::test_filter_conformance; use vortex_array::dtype::DecimalDType; + use vortex_array::validity::Validity; use vortex_buffer::buffer; use crate::DecimalByteParts; + use crate::decimal_byte_parts::testing::i128_parts; + use crate::decimal_byte_parts::testing::i256_of; + use crate::decimal_byte_parts::testing::i256_parts; #[test] fn test_filter_decimal_byte_parts() { @@ -59,4 +70,31 @@ mod test { &mut array_session().create_execution_ctx(), ); } + + #[test] + fn test_filter_decimal_byte_parts_with_lower_parts() { + let array = i128_parts( + vec![1i128 << 70, -(1i128 << 70), 5, (1i128 << 64) - 1, 0], + Validity::NonNullable, + ); + test_filter_conformance( + &array.into_array(), + &mut array_session().create_execution_ctx(), + ); + + let array = i256_parts( + vec![ + i256_of(1, 0), + i256_of(-1, 5), + i256_of(0, u128::MAX), + i256_of(1 << 64, 7), + i256_of(0, 0), + ], + Validity::from_iter([true, false, true, true, false]), + ); + test_filter_conformance( + &array.into_array(), + &mut array_session().create_execution_ctx(), + ); + } } diff --git a/encodings/decimal-byte-parts/src/decimal_byte_parts/compute/is_constant.rs b/encodings/decimal-byte-parts/src/decimal_byte_parts/compute/is_constant.rs index 065bc5e0051..3fe59111f6e 100644 --- a/encodings/decimal-byte-parts/src/decimal_byte_parts/compute/is_constant.rs +++ b/encodings/decimal-byte-parts/src/decimal_byte_parts/compute/is_constant.rs @@ -2,6 +2,7 @@ // SPDX-FileCopyrightText: Copyright the Vortex contributors use vortex_array::ArrayRef; +use vortex_array::ArrayView; use vortex_array::ExecutionCtx; use vortex_array::aggregate_fn::AggregateFnRef; use vortex_array::aggregate_fn::fns::is_constant::IsConstant; @@ -15,7 +16,9 @@ use crate::decimal_byte_parts::DecimalBytePartsArraySlotsExt; /// DecimalByteParts-specific is_constant kernel. /// -/// Delegates to checking if the MSP (most significant part) is constant. +/// Delegates to checking that every part is constant: the MSP (most significant part) plus +/// each lower part. An all-null array is constant regardless of the bits its lower parts +/// hold in null slots. #[derive(Debug)] pub(crate) struct DecimalBytePartsIsConstantKernel; @@ -34,7 +37,27 @@ impl DynAggregateKernel for DecimalBytePartsIsConstantKernel { return Ok(None); }; - let result = is_constant(array.msp(), ctx)?; + let result = is_constant_parts(array, ctx)?; Ok(Some(IsConstant::make_partial(batch, result, ctx)?)) } } + +fn is_constant_parts( + array: ArrayView<'_, DecimalByteParts>, + ctx: &mut ExecutionCtx, +) -> VortexResult { + if !is_constant(array.msp(), ctx)? { + return Ok(false); + } + // Null slots hold undefined bits in the lower parts, so they cannot make a constant + // (all-null) array non-constant. + if array.array().all_invalid(ctx)? { + return Ok(true); + } + for part in array.lower_parts().iter() { + if !is_constant(part, ctx)? { + return Ok(false); + } + } + Ok(true) +} diff --git a/encodings/decimal-byte-parts/src/decimal_byte_parts/compute/mask.rs b/encodings/decimal-byte-parts/src/decimal_byte_parts/compute/mask.rs index 2abf347bc7a..662af162e74 100644 --- a/encodings/decimal-byte-parts/src/decimal_byte_parts/compute/mask.rs +++ b/encodings/decimal-byte-parts/src/decimal_byte_parts/compute/mask.rs @@ -16,14 +16,17 @@ use crate::decimal_byte_parts::DecimalBytePartsArraySlotsExt; impl MaskReduce for DecimalByteParts { fn mask(array: ArrayView<'_, Self>, mask: &ArrayRef) -> VortexResult> { + // Validity lives in the MSP, so only that part needs masking: the lower parts hold + // undefined bits in null slots, which is exactly what a masked-out row is. let masked_msp = MaskExpr.try_new_array( array.msp().len(), EmptyOptions, [array.msp().clone(), mask.clone()], )?; Ok(Some( - DecimalByteParts::try_new( + DecimalByteParts::try_new_with_lower_parts( masked_msp, + array.lower_parts().to_vec(), *array .dtype() .as_decimal_opt() diff --git a/encodings/decimal-byte-parts/src/decimal_byte_parts/compute/mod.rs b/encodings/decimal-byte-parts/src/decimal_byte_parts/compute/mod.rs index 6c2d0dabb31..844468545cf 100644 --- a/encodings/decimal-byte-parts/src/decimal_byte_parts/compute/mod.rs +++ b/encodings/decimal-byte-parts/src/decimal_byte_parts/compute/mod.rs @@ -19,10 +19,36 @@ mod tests { use vortex_array::compute::conformance::binary_numeric::test_binary_numeric_array; use vortex_array::compute::conformance::consistency::test_array_consistency; use vortex_array::dtype::DecimalDType; + use vortex_array::dtype::i256; + use vortex_array::validity::Validity; use vortex_buffer::buffer; use crate::DecimalByteParts; use crate::DecimalBytePartsArray; + use crate::decimal_byte_parts::testing::i128_parts; + use crate::decimal_byte_parts::testing::i256_of; + use crate::decimal_byte_parts::testing::i256_parts; + + /// Values needing more than 64 bits, so the encoding carries lower parts. + fn wide_i128() -> Vec { + vec![ + 1 << 70, + -(1 << 70), + (1 << 64) - 1, + 0, + 99_999_999_999_999_999_999_999_999_999_999_999_999, + ] + } + + fn wide_i256() -> Vec { + vec![ + i256_of(1, 0), + i256_of(-1, 0), + i256_of(0, u128::MAX), + i256_of(1 << 64, 7), + i256_of(0, 0), + ] + } #[rstest] // Basic decimal byte parts arrays @@ -70,6 +96,11 @@ mod tests { PrimitiveArray::from_iter((0..2000i64).map(|i| i * 1000000)).into_array(), DecimalDType::new(19, 6) ).unwrap())] + // Wide decimals carrying lower parts + #[case::decimal_i128_one_lower_part(i128_parts(wide_i128(), Validity::NonNullable))] + #[case::decimal_i128_nullable(i128_parts(wide_i128(), Validity::from_iter([true, false, true, true, false])))] + #[case::decimal_i256_three_lower_parts(i256_parts(wide_i256(), Validity::NonNullable))] + #[case::decimal_i256_nullable(i256_parts(wide_i256(), Validity::from_iter([false, true, true, false, true])))] fn test_decimal_byte_parts_consistency(#[case] array: DecimalBytePartsArray) { let ctx = &mut array_session().create_execution_ctx(); @@ -89,6 +120,8 @@ mod tests { buffer![-100i32, -200, 300, -400, 500].into_array(), DecimalDType::new(10, 2) ).unwrap())] + #[case::decimal_i128_one_lower_part(i128_parts(wide_i128(), Validity::NonNullable))] + #[case::decimal_i256_three_lower_parts(i256_parts(wide_i256(), Validity::NonNullable))] fn test_decimal_byte_parts_binary_numeric(#[case] array: DecimalBytePartsArray) { let ctx = &mut array_session().create_execution_ctx(); test_binary_numeric_array(&array.into_array(), ctx); diff --git a/encodings/decimal-byte-parts/src/decimal_byte_parts/compute/take.rs b/encodings/decimal-byte-parts/src/decimal_byte_parts/compute/take.rs index 7a18f7bf91b..1bfe3a7f2b6 100644 --- a/encodings/decimal-byte-parts/src/decimal_byte_parts/compute/take.rs +++ b/encodings/decimal-byte-parts/src/decimal_byte_parts/compute/take.rs @@ -18,8 +18,15 @@ impl TakeExecute for DecimalByteParts { indices: &ArrayRef, _ctx: &mut ExecutionCtx, ) -> VortexResult> { - DecimalByteParts::try_new( + let lower_parts = array + .lower_parts() + .iter() + .map(|part| part.take(indices.clone())) + .collect::>>()?; + + DecimalByteParts::try_new_with_lower_parts( array.msp().take(indices.clone())?, + lower_parts, *array .dtype() .as_decimal_opt() diff --git a/encodings/decimal-byte-parts/src/decimal_byte_parts/limbs.rs b/encodings/decimal-byte-parts/src/decimal_byte_parts/limbs.rs new file mode 100644 index 00000000000..4cf9c875019 --- /dev/null +++ b/encodings/decimal-byte-parts/src/decimal_byte_parts/limbs.rs @@ -0,0 +1,379 @@ +// SPDX-License-Identifier: Apache-2.0 +// SPDX-FileCopyrightText: Copyright the Vortex contributors + +//! Splitting decimal values into 64-bit parts, and reassembling them. +//! +//! A `DecimalByteParts` array stores each value as a signed most significant part (MSP) +//! followed by `k` unsigned 64-bit lower parts ordered most significant first. The encoded +//! value is +//! +//! ```text +//! msp * 2^(64k) + Σ_{i, +} + +/// The decimal storage type that reassembling the given parts produces. +/// +/// # Errors +/// +/// Returns an error if `msp_ptype` is not a signed integer, or if there are more than +/// [`MAX_LOWER_PARTS`] lower parts. +pub fn assembled_values_type( + msp_ptype: PType, + lower_part_count: usize, +) -> VortexResult { + if lower_part_count > MAX_LOWER_PARTS { + vortex_bail!("at most {MAX_LOWER_PARTS} lower parts are supported, got {lower_part_count}"); + } + if lower_part_count == 0 { + return DecimalType::try_from(msp_ptype); + } + let bits = msp_ptype.bit_width() + LOWER_PART_BITS * lower_part_count; + Ok(if bits <= 128 { + DecimalType::I128 + } else { + DecimalType::I256 + }) +} + +/// Split a canonical decimal array into a signed most significant part and unsigned 64-bit +/// lower parts. +/// +/// Values narrower than 128 bits are already a single signed part, so they are returned +/// with no lower parts. `i128` values split into an `i64` MSP and one lower part, `i256` +/// values into an `i64` MSP and three lower parts. +/// +/// # Errors +/// +/// Returns an error if the array's validity cannot be derived. +pub fn split_decimal(decimal: &DecimalArray) -> VortexResult { + let validity = decimal.validity()?; + Ok(match decimal.values_type() { + DecimalType::I8 => DecimalParts::flat(decimal.buffer::(), validity), + DecimalType::I16 => DecimalParts::flat(decimal.buffer::(), validity), + DecimalType::I32 => DecimalParts::flat(decimal.buffer::(), validity), + DecimalType::I64 => DecimalParts::flat(decimal.buffer::(), validity), + DecimalType::I128 => { + let (msp, lower) = split_i128(&decimal.buffer::()); + DecimalParts::new(msp, [lower], validity) + } + DecimalType::I256 => { + let (msp, lower) = split_i256(&decimal.buffer::()); + DecimalParts::new(msp, lower, validity) + } + }) +} + +/// Reassemble decimal byte parts into a canonical decimal array. +/// +/// The parts must already be canonical primitive arrays: a signed MSP, and `u64` lower +/// parts ordered most significant first. +/// +/// # Errors +/// +/// Returns an error if the parts do not describe a valid decimal, or if the MSP's validity +/// cannot be derived. +pub fn assemble_decimal( + msp: &PrimitiveArray, + lower_parts: &[PrimitiveArray], + decimal_dtype: DecimalDType, +) -> VortexResult { + let validity = msp.validity()?; + if lower_parts.is_empty() { + return Ok(match_each_signed_integer_ptype!(msp.ptype(), |P| { + // SAFETY: the buffer is typed by the array's own ptype, the decimal dtype is the + // array's, and the validity is taken from the same array. + unsafe { DecimalArray::new_unchecked(msp.to_buffer::

(), decimal_dtype, validity) } + })); + } + + let lower: Vec<&[u64]> = lower_parts + .iter() + .map(|part| part.as_slice::()) + .collect(); + Ok( + match assembled_values_type(msp.ptype(), lower_parts.len())? { + DecimalType::I256 => { + DecimalArray::new(assemble_i256(msp, &lower), decimal_dtype, validity) + } + _ => DecimalArray::new(assemble_i128(msp, &lower), decimal_dtype, validity), + }, + ) +} + +/// Combine a single row's parts into an `i128`. +#[inline] +pub(crate) fn combine_i128(msp: i64, lower: impl IntoIterator) -> i128 { + lower.into_iter().fold(i128::from(msp), |acc, part| { + (acc << LOWER_PART_BITS) | i128::from(part) + }) +} + +/// Combine a single row's parts into an `i256`. +/// +/// The lower parts fill the least significant 64-bit words, the MSP the word above them, +/// and the remaining high words are the MSP's sign extension. +#[inline] +pub(crate) fn combine_i256(msp: i64, lower: impl ExactSizeIterator) -> i256 { + let count = lower.len(); + let mut words = [if msp < 0 { u64::MAX } else { 0 }; 4]; + for (i, part) in lower.enumerate() { + words[count - 1 - i] = part; + } + words[count] = msp.cast_unsigned(); + + i256::from_parts( + u128::from(words[0]) | (u128::from(words[1]) << LOWER_PART_BITS), + (u128::from(words[2]) | (u128::from(words[3]) << LOWER_PART_BITS)).cast_signed(), + ) +} + +impl DecimalParts { + /// Parts for a decimal already stored in a single signed integer. + fn flat(values: Buffer, validity: Validity) -> Self { + Self { + msp: PrimitiveArray::new(values, validity).into_array(), + lower_parts: Vec::new(), + } + } + + fn new( + msp: Buffer, + lower_parts: impl IntoIterator>, + validity: Validity, + ) -> Self { + Self { + msp: PrimitiveArray::new(msp, validity).into_array(), + lower_parts: lower_parts + .into_iter() + .map(|part| PrimitiveArray::new(part, Validity::NonNullable).into_array()) + .collect(), + } + } +} + +#[expect( + clippy::cast_possible_truncation, + clippy::cast_sign_loss, + reason = "splitting a wide integer into 64-bit windows truncates by construction" +)] +fn split_i128(values: &Buffer) -> (Buffer, Buffer) { + let mut msp = BufferMut::::with_capacity(values.len()); + let mut lower = BufferMut::::with_capacity(values.len()); + for value in values.iter() { + msp.push((value >> LOWER_PART_BITS) as i64); + lower.push(*value as u64); + } + (msp.freeze(), lower.freeze()) +} + +#[expect( + clippy::cast_possible_truncation, + clippy::cast_sign_loss, + reason = "splitting a wide integer into 64-bit windows truncates by construction" +)] +fn split_i256(values: &Buffer) -> (Buffer, [Buffer; MAX_LOWER_PARTS]) { + let mut msp = BufferMut::::with_capacity(values.len()); + // Ordered most significant first: bits 191..128, 127..64, 63..0. + let mut lower = std::array::from_fn::<_, MAX_LOWER_PARTS, _>(|_| { + BufferMut::::with_capacity(values.len()) + }); + for value in values.iter() { + let (low, high) = value.to_parts(); + msp.push((high >> LOWER_PART_BITS) as i64); + lower[0].push(high as u64); + lower[1].push((low >> LOWER_PART_BITS) as u64); + lower[2].push(low as u64); + } + (msp.freeze(), lower.map(BufferMut::freeze)) +} + +#[expect( + clippy::useless_conversion, + reason = "the widening to i64 is a no-op only for the i64 arm of the ptype match" +)] +fn assemble_i128(msp: &PrimitiveArray, lower: &[&[u64]]) -> Buffer { + let len = msp.len(); + let mut out = BufferMut::::with_capacity(len); + match_each_signed_integer_ptype!(msp.ptype(), |P| { + let values = msp.as_slice::

(); + for (row, value) in values.iter().enumerate() { + out.push(combine_i128( + i64::from(*value), + lower.iter().map(|part| part[row]), + )); + } + }); + out.freeze() +} + +#[expect( + clippy::useless_conversion, + reason = "the widening to i64 is a no-op only for the i64 arm of the ptype match" +)] +fn assemble_i256(msp: &PrimitiveArray, lower: &[&[u64]]) -> Buffer { + let len = msp.len(); + let mut out = BufferMut::::with_capacity(len); + match_each_signed_integer_ptype!(msp.ptype(), |P| { + let values = msp.as_slice::

(); + for (row, value) in values.iter().enumerate() { + out.push(combine_i256( + i64::from(*value), + lower.iter().map(|part| part[row]), + )); + } + }); + out.freeze() +} + +#[cfg(test)] +mod tests { + use rstest::rstest; + use vortex_array::VortexSessionExecute; + use vortex_array::array_session; + use vortex_array::arrays::DecimalArray; + use vortex_array::dtype::DecimalDType; + use vortex_array::dtype::i256; + use vortex_array::validity::Validity; + use vortex_buffer::Buffer; + use vortex_buffer::buffer; + use vortex_error::VortexResult; + + use super::*; + + fn round_trip(decimal: DecimalArray) -> VortexResult { + let mut ctx = array_session().create_execution_ctx(); + let parts = split_decimal(&decimal)?; + let msp = parts.msp.execute::(&mut ctx)?; + let lower = parts + .lower_parts + .into_iter() + .map(|part| part.execute::(&mut ctx)) + .collect::>>()?; + assemble_decimal(&msp, &lower, decimal.decimal_dtype()) + } + + #[rstest] + #[case::zero(0)] + #[case::one(1)] + #[case::minus_one(-1)] + #[case::limb_boundary(1i128 << 64)] + #[case::just_below_limb_boundary((1i128 << 64) - 1)] + #[case::negative_limb_boundary(-(1i128 << 64))] + #[case::max(i128::MAX)] + #[case::min(i128::MIN)] + fn test_split_assemble_i128(#[case] value: i128) -> VortexResult<()> { + let decimal = DecimalArray::new( + Buffer::from(vec![value]), + DecimalDType::new(38, 2), + Validity::NonNullable, + ); + let round_tripped = round_trip(decimal)?; + assert_eq!(round_tripped.buffer::().as_slice(), &[value]); + Ok(()) + } + + #[rstest] + #[case::zero(i256::ZERO)] + #[case::one(i256::ONE)] + #[case::minus_one(i256::ZERO - i256::ONE)] + #[case::max(i256::MAX)] + #[case::min(i256::MIN)] + #[case::word_1(i256::from_parts(1u128 << 64, 0))] + #[case::word_2(i256::from_parts(0, 1))] + #[case::word_3(i256::from_parts(0, 1i128 << 64))] + #[case::mixed(i256::from_parts(u128::MAX, -3))] + fn test_split_assemble_i256(#[case] value: i256) -> VortexResult<()> { + let decimal = DecimalArray::new( + Buffer::from(vec![value]), + DecimalDType::new(76, 2), + Validity::NonNullable, + ); + let round_tripped = round_trip(decimal)?; + assert_eq!(round_tripped.buffer::().as_slice(), &[value]); + Ok(()) + } + + #[test] + fn test_split_narrow_decimal_has_no_lower_parts() -> VortexResult<()> { + let decimal = DecimalArray::new( + buffer![1i32, 2, 3], + DecimalDType::new(9, 2), + Validity::NonNullable, + ); + let parts = split_decimal(&decimal)?; + assert!(parts.lower_parts.is_empty()); + assert_eq!(parts.msp.dtype().as_ptype(), PType::I32); + Ok(()) + } + + #[test] + fn test_split_i256_part_count_and_types() -> VortexResult<()> { + let decimal = DecimalArray::new( + Buffer::from(vec![i256::from_i128(i128::MAX), i256::MIN]), + DecimalDType::new(76, 0), + Validity::NonNullable, + ); + let parts = split_decimal(&decimal)?; + assert_eq!(parts.lower_parts.len(), MAX_LOWER_PARTS); + assert_eq!(parts.msp.dtype().as_ptype(), PType::I64); + for part in &parts.lower_parts { + assert_eq!(part.dtype(), &LOWER_PART_DTYPE); + } + Ok(()) + } + + #[test] + fn test_assembled_values_type() -> VortexResult<()> { + assert_eq!(assembled_values_type(PType::I32, 0)?, DecimalType::I32); + assert_eq!(assembled_values_type(PType::I64, 1)?, DecimalType::I128); + assert_eq!(assembled_values_type(PType::I8, 1)?, DecimalType::I128); + assert_eq!(assembled_values_type(PType::I8, 2)?, DecimalType::I256); + assert_eq!(assembled_values_type(PType::I64, 3)?, DecimalType::I256); + assert!(assembled_values_type(PType::I64, 4).is_err()); + Ok(()) + } +} diff --git a/encodings/decimal-byte-parts/src/decimal_byte_parts/mod.rs b/encodings/decimal-byte-parts/src/decimal_byte_parts/mod.rs index d5b0024f5b7..5b3b1fd4f6f 100644 --- a/encodings/decimal-byte-parts/src/decimal_byte_parts/mod.rs +++ b/encodings/decimal-byte-parts/src/decimal_byte_parts/mod.rs @@ -9,31 +9,38 @@ use vortex_array::Array; use vortex_array::ArrayParts; use vortex_array::ArrayView; pub(crate) mod compute; +mod limbs; mod rules; mod slice; +#[cfg(test)] +pub(crate) mod testing; +pub use limbs::DecimalParts; +pub use limbs::LOWER_PART_DTYPE; +pub use limbs::MAX_LOWER_PARTS; +pub use limbs::assembled_values_type; +pub use limbs::split_decimal; use prost::Message as _; use vortex_array::ArrayEq; use vortex_array::ArrayHash; use vortex_array::ArrayId; use vortex_array::ArrayRef; +use vortex_array::ArraySlots; use vortex_array::EqMode; use vortex_array::ExecutionCtx; use vortex_array::ExecutionResult; use vortex_array::IntoArray; use vortex_array::array_slots; -use vortex_array::arrays::DecimalArray; use vortex_array::arrays::PrimitiveArray; use vortex_array::buffer::BufferHandle; use vortex_array::dtype::DType; use vortex_array::dtype::DecimalDType; +use vortex_array::dtype::DecimalType; use vortex_array::dtype::PType; -use vortex_array::match_each_signed_integer_ptype; use vortex_array::scalar::DecimalValue; use vortex_array::scalar::Scalar; use vortex_array::scalar::ScalarValue; use vortex_array::serde::ArrayChildren; -use vortex_array::smallvec::smallvec; use vortex_array::vtable::OperationsVTable; use vortex_array::vtable::VTable; use vortex_array::vtable::ValidityChild; @@ -42,10 +49,14 @@ use vortex_error::VortexExpect; use vortex_error::VortexResult; use vortex_error::vortex_bail; use vortex_error::vortex_ensure; +use vortex_error::vortex_err; use vortex_error::vortex_panic; use vortex_session::VortexSession; use vortex_session::registry::CachedId; +use crate::decimal_byte_parts::limbs::assemble_decimal; +use crate::decimal_byte_parts::limbs::combine_i128; +use crate::decimal_byte_parts::limbs::combine_i256; use crate::decimal_byte_parts::rules::PARENT_RULES; /// A [`DecimalByteParts`]-encoded Vortex array. @@ -69,6 +80,23 @@ pub struct DecimalBytesPartsMetadata { lower_part_count: u32, } +impl DecimalBytesPartsMetadata { + /// The number of lower parts encoded in this array. + /// + /// # Errors + /// + /// Returns an error if the count exceeds [`MAX_LOWER_PARTS`]. + fn lower_parts(&self) -> VortexResult { + let count = usize::try_from(self.lower_part_count) + .map_err(|_| vortex_err!("lower part count {} out of range", self.lower_part_count))?; + vortex_ensure!( + count <= MAX_LOWER_PARTS, + "at most {MAX_LOWER_PARTS} lower parts are supported, got {count}" + ); + Ok(count) + } +} + impl VTable for DecimalByteParts { type TypedArrayData = DecimalBytePartsData; @@ -90,8 +118,14 @@ impl VTable for DecimalByteParts { let Some(decimal_dtype) = dtype.as_decimal_opt() else { vortex_bail!("expected decimal dtype, got {}", dtype) }; - let msp = DecimalBytePartsSlotsView::from_slots(slots).msp; - DecimalBytePartsData::validate(msp, *decimal_dtype, dtype, len) + let slots = DecimalBytePartsSlotsView::from_slots(slots); + DecimalBytePartsData::validate( + slots.msp, + slots.lower_parts.iter(), + *decimal_dtype, + dtype, + len, + ) } fn nbuffers(_array: ArrayView<'_, Self>) -> usize { @@ -118,10 +152,12 @@ impl VTable for DecimalByteParts { array: ArrayView<'_, Self>, _session: &VortexSession, ) -> VortexResult>> { + let lower_part_count = u32::try_from(array.lower_parts().len()) + .map_err(|_| vortex_err!("lower part count exceeds u32"))?; Ok(Some( DecimalBytesPartsMetadata { zeroth_child_ptype: PType::try_from(array.msp().dtype())? as i32, - lower_part_count: 0, + lower_part_count, } .encode_to_vec(), )) @@ -137,26 +173,41 @@ impl VTable for DecimalByteParts { _session: &VortexSession, ) -> VortexResult> { let metadata = DecimalBytesPartsMetadata::decode(metadata)?; - let Some(decimal_dtype) = dtype.as_decimal_opt() else { - vortex_bail!("decoding decimal but given non decimal dtype {}", dtype) - }; + vortex_ensure!( + dtype.as_decimal_opt().is_some(), + "decoding decimal but given non decimal dtype {dtype}" + ); let encoded_dtype = DType::Primitive(metadata.zeroth_child_ptype(), dtype.nullability()); - let msp = children.get(0, &encoded_dtype, len)?; - - assert_eq!( - metadata.lower_part_count, 0, - "lower_part_count > 0 not currently supported" + let lower_part_count = metadata.lower_parts()?; + vortex_ensure!( + children.len() == DecimalBytePartsSlots::FIXED_COUNT + lower_part_count, + "expected {} children, got {}", + DecimalBytePartsSlots::FIXED_COUNT + lower_part_count, + children.len() ); - let slots = smallvec![Some(msp.clone())]; - let data = DecimalBytePartsData::try_new(msp.dtype(), msp.len(), *decimal_dtype)?; - Ok(ArrayParts::new(self.clone(), dtype.clone(), len, data).with_slots(slots)) + let msp = children.get(DecimalBytePartsSlots::MSP, &encoded_dtype, len)?; + + let mut slots = ArraySlots::with_capacity(children.len()); + slots.push(Some(msp)); + for idx in 0..lower_part_count { + slots.push(Some(children.get( + DecimalBytePartsSlots::LOWER_PARTS_OFFSET + idx, + &LOWER_PART_DTYPE, + len, + )?)); + } + + Ok( + ArrayParts::new(self.clone(), dtype.clone(), len, DecimalBytePartsData) + .with_slots(slots), + ) } fn slot_name(_array: ArrayView<'_, Self>, idx: usize) -> String { - DecimalBytePartsSlots::NAMES[idx].to_string() + DecimalBytePartsSlots::slot_name(idx) } fn reduce_parent( @@ -177,20 +228,21 @@ pub struct DecimalBytePartsSlots { /// The most significant parts of the decimal values. #[slot(0)] pub msp: ArrayRef, + /// The remaining 64-bit windows of the decimal values, most significant first. + #[slot(1..)] + pub lower_parts: Vec, } /// This array encodes decimals as between 1-4 columns of primitive typed children. -/// The most significant part (msp) sorting the most significant decimal bits. +/// The most significant part (msp) storing the most significant decimal bits. /// This array must be signed and is nullable iff the decimal is nullable. +/// Every lower part is a non-nullable `u64` holding a raw 64-bit window of the value. /// -/// e.g. for a decimal i128 \[ 127..64 | 64..0 \] msp = 127..64 and lower_part\[0\] = 64..0 +/// e.g. for a decimal i128 \[ 127..64 | 63..0 \] msp = 127..64 and lower_part\[0\] = 63..0 +/// +/// All parts live in slots, so the array carries no additional data. #[derive(Clone, Debug)] -pub struct DecimalBytePartsData { - // NOTE: the lower_parts is currently unused, we reserve this field so that it is properly - // read/written during serde, but provide no constructor to initialize this to anything - // other than the empty Vec. - _lower_parts: Vec, -} +pub struct DecimalBytePartsData; impl Display for DecimalBytePartsData { fn fmt(&self, _f: &mut Formatter<'_>) -> std::fmt::Result { @@ -198,13 +250,25 @@ impl Display for DecimalBytePartsData { } } +/// The parts of a [`DecimalBytePartsArray`]. pub struct DecimalBytePartsDataParts { + /// The most significant part, carrying the array's validity. pub msp: ArrayRef, + /// The remaining 64-bit windows, most significant first. + pub lower_parts: Vec, } impl DecimalBytePartsData { - pub fn validate( + /// Validate the parts of a [`DecimalBytePartsArray`]. + /// + /// # Errors + /// + /// Returns an error if the MSP is not a signed integer array of length `len`, if `dtype` + /// does not match the MSP's nullability, or if any lower part is not a non-nullable + /// `u64` array of length `len`. + pub fn validate<'a>( msp: &ArrayRef, + lower_parts: impl ExactSizeIterator, decimal_dtype: DecimalDType, dtype: &DType, len: usize, @@ -219,24 +283,27 @@ impl DecimalBytePartsData { "expected dtype {expected_dtype}, got {dtype}" ); vortex_ensure!(msp.len() == len, "expected len {len}, got {}", msp.len()); - Ok(()) - } - pub(crate) fn try_new( - msp_dtype: &DType, - msp_len: usize, - decimal_dtype: DecimalDType, - ) -> VortexResult { - let expected_dtype = DType::Decimal(decimal_dtype, msp_dtype.nullability()); + let lower_part_count = lower_parts.len(); vortex_ensure!( - msp_dtype.is_signed_int(), - "decimal bytes parts, first part must be a signed array" + lower_part_count <= MAX_LOWER_PARTS, + "at most {MAX_LOWER_PARTS} lower parts are supported, got {lower_part_count}" ); - let _ = msp_len; - drop(expected_dtype); - Ok(Self { - _lower_parts: Vec::new(), - }) + for (idx, part) in lower_parts.enumerate() { + vortex_ensure!( + part.dtype() == &LOWER_PART_DTYPE, + "lower part {idx} must have dtype {LOWER_PART_DTYPE}, got {}", + part.dtype() + ); + vortex_ensure!( + part.len() == len, + "lower part {idx} has len {}, expected {len}", + part.len() + ); + } + // Rejects part combinations that cannot be reassembled into a decimal value. + assembled_values_type(msp.dtype().as_ptype(), lower_part_count)?; + Ok(()) } } @@ -245,47 +312,65 @@ pub struct DecimalByteParts; impl DecimalByteParts { /// Construct a new [`DecimalBytePartsArray`] from an MSP array and decimal dtype. + /// + /// # Errors + /// + /// Returns an error if the MSP is not a signed integer array. pub fn try_new( msp: ArrayRef, decimal_dtype: DecimalDType, + ) -> VortexResult { + Self::try_new_with_lower_parts(msp, Vec::new(), decimal_dtype) + } + + /// Construct a new [`DecimalBytePartsArray`] from an MSP array, its lower parts, and a + /// decimal dtype. + /// + /// Lower parts are ordered most significant first and must each be a non-nullable `u64` + /// array of the same length as the MSP. See [`split_decimal`] for producing them from a + /// canonical decimal array. + /// + /// # Errors + /// + /// Returns an error if the parts do not describe a valid decimal, see + /// [`DecimalBytePartsData::validate`]. + pub fn try_new_with_lower_parts( + msp: ArrayRef, + lower_parts: Vec, + decimal_dtype: DecimalDType, ) -> VortexResult { let len = msp.len(); let dtype = DType::Decimal(decimal_dtype, msp.dtype().nullability()); - let slots = smallvec![Some(msp.clone())]; - let data = DecimalBytePartsData::try_new(msp.dtype(), msp.len(), decimal_dtype)?; - Ok(unsafe { - Array::from_parts_unchecked( - ArrayParts::new(DecimalByteParts, dtype, len, data).with_slots(slots), - ) - }) + let slots = DecimalBytePartsSlots { msp, lower_parts }.into_slots(); + Array::try_from_parts( + ArrayParts::new(DecimalByteParts, dtype, len, DecimalBytePartsData).with_slots(slots), + ) } } +/// The decimal storage type this array canonicalizes to. +fn values_type(array: ArrayView<'_, DecimalByteParts>) -> VortexResult { + assembled_values_type(array.msp().dtype().as_ptype(), array.lower_parts().len()) +} + /// Converts a DecimalBytePartsArray to its canonical DecimalArray representation. fn to_canonical_decimal( array: &DecimalBytePartsArray, ctx: &mut ExecutionCtx, ) -> VortexResult { - // TODO(joe): support parts len != 1 - let prim = array.msp().clone().execute::(ctx)?; - // Depending on the decimal type and the min/max of the primitive array we can choose - // the correct buffer size - - Ok(match_each_signed_integer_ptype!(prim.ptype(), |P| { - // SAFETY: The primitive array's buffer is already validated with correct type. - // The decimal dtype matches the array's dtype, and validity is preserved. - unsafe { - DecimalArray::new_unchecked( - prim.to_buffer::

(), - *array - .dtype() - .as_decimal_opt() - .vortex_expect("must be a decimal dtype"), - prim.validity()?, - ) - } - .into_array() - })) + let msp = array.msp().clone().execute::(ctx)?; + let lower_parts = array + .lower_parts() + .iter() + .map(|part| part.clone().execute::(ctx)) + .collect::>>()?; + + let decimal_dtype = *array + .dtype() + .as_decimal_opt() + .vortex_expect("must be a decimal dtype"); + + Ok(assemble_decimal(&msp, &lower_parts, decimal_dtype)?.into_array()) } impl OperationsVTable for DecimalByteParts { @@ -294,17 +379,31 @@ impl OperationsVTable for DecimalByteParts { index: usize, ctx: &mut ExecutionCtx, ) -> VortexResult { - // TODO(joe): support parts len != 1 let scalar = array.msp().execute_scalar(index, ctx)?; // Note. values in msp, can only be signed integers upto size i64. let primitive_scalar = scalar.as_primitive(); - // TODO(joe): extend this to support multiple parts. - let value = primitive_scalar.as_::().vortex_expect("non-null"); - Scalar::try_new( - array.dtype().clone(), - Some(ScalarValue::Decimal(DecimalValue::I64(value))), - ) + let msp = primitive_scalar.as_::().vortex_expect("non-null"); + + let lower_parts = array + .lower_parts() + .iter() + .map(|part| { + Ok(part + .execute_scalar(index, ctx)? + .as_primitive() + .as_::() + .vortex_expect("lower parts are non-nullable")) + }) + .collect::>>()?; + + let value = match values_type(array)? { + _ if lower_parts.is_empty() => DecimalValue::I64(msp), + DecimalType::I256 => DecimalValue::I256(combine_i256(msp, lower_parts.into_iter())), + _ => DecimalValue::I128(combine_i128(msp, lower_parts)), + }; + + Scalar::try_new(array.dtype().clone(), Some(ScalarValue::Decimal(value))) } } @@ -317,21 +416,39 @@ impl ValidityChild for DecimalByteParts { #[cfg(test)] mod tests { + use rstest::rstest; + use vortex_array::ArrayContext; + use vortex_array::ArrayRef; use vortex_array::IntoArray; use vortex_array::VortexSessionExecute; use vortex_array::array_session; use vortex_array::arrays::BoolArray; + use vortex_array::arrays::DecimalArray; use vortex_array::arrays::PrimitiveArray; + use vortex_array::assert_arrays_eq; use vortex_array::dtype::DType; use vortex_array::dtype::DecimalDType; use vortex_array::dtype::Nullability; + use vortex_array::dtype::PType; + use vortex_array::dtype::i256; use vortex_array::scalar::DecimalValue; use vortex_array::scalar::Scalar; use vortex_array::scalar::ScalarValue; + use vortex_array::serde::SerializeOptions; + use vortex_array::serde::SerializedArray; + use vortex_array::session::ArraySessionExt; use vortex_array::validity::Validity; + use vortex_buffer::ByteBufferMut; use vortex_buffer::buffer; + use vortex_error::VortexResult; + use vortex_session::registry::ReadContext; + use super::*; use crate::DecimalByteParts; + use crate::decimal_byte_parts::testing::encode; + use crate::decimal_byte_parts::testing::i128_parts; + use crate::decimal_byte_parts::testing::i256_of; + use crate::decimal_byte_parts::testing::i256_parts; #[test] fn test_scalar_at_decimal_parts() { @@ -371,4 +488,357 @@ mod tests { .unwrap() ); } + + /// The largest unscaled value a `Decimal(38, _)` can hold: `10^38 - 1`. + const MAX_PRECISION_38: i128 = 99_999_999_999_999_999_999_999_999_999_999_999_999; + + /// The largest unscaled value a `Decimal(76, _)` can hold: `10^76 - 1`. + fn max_precision_76() -> i256 { + i256::from_i128(10).wrapping_pow(76) - i256::ONE + } + + /// Values that exercise every 64-bit window of an `i128`, both signs, and the boundaries + /// where a lower part carries into the MSP. + fn wide_i128_values() -> Vec { + vec![ + 0, + 1, + -1, + (1 << 64) - 1, + 1 << 64, + -(1 << 64), + -((1 << 64) + 1), + MAX_PRECISION_38, + -MAX_PRECISION_38, + 1 << 100, + ] + } + + /// Values that exercise every 64-bit window of an `i256`. + fn wide_i256_values() -> Vec { + vec![ + i256::ZERO, + i256::ONE, + i256::ZERO - i256::ONE, + i256_of(0, u128::MAX), + i256_of(1, 0), + i256_of(-1, 0), + i256_of(-1, u128::MAX - 1), + i256_of(1 << 64, 12345), + max_precision_76(), + i256::ZERO - max_precision_76(), + ] + } + + #[rstest] + #[case::i128_non_nullable(i128_parts(wide_i128_values(), Validity::NonNullable))] + #[case::i256_non_nullable(i256_parts(wide_i256_values(), Validity::NonNullable))] + fn test_canonical_decimal_round_trips( + #[case] array: DecimalBytePartsArray, + ) -> VortexResult<()> { + let mut ctx = array_session().create_execution_ctx(); + let canonical = array + .clone() + .into_array() + .execute::(&mut ctx)?; + assert_arrays_eq!(array, canonical, &mut ctx); + Ok(()) + } + + #[test] + fn test_lower_part_layout_i128() -> VortexResult<()> { + let array = i128_parts(vec![(3i128 << 64) | 7], Validity::NonNullable); + assert_eq!(array.lower_parts().len(), 1); + assert_eq!(array.msp().dtype().as_ptype(), PType::I64); + assert_eq!(array.lower_parts()[0].dtype(), &LOWER_PART_DTYPE); + + let mut ctx = array_session().create_execution_ctx(); + let msp = array.msp().clone().execute::(&mut ctx)?; + let lower = array.lower_parts()[0] + .clone() + .execute::(&mut ctx)?; + assert_eq!(msp.as_slice::(), &[3]); + assert_eq!(lower.as_slice::(), &[7]); + Ok(()) + } + + #[test] + fn test_lower_part_layout_i256() -> VortexResult<()> { + let array = i256_parts( + vec![i256_of((5i128 << 64) | 6, (7u128 << 64) | 8)], + Validity::NonNullable, + ); + assert_eq!(array.lower_parts().len(), MAX_LOWER_PARTS); + + let mut ctx = array_session().create_execution_ctx(); + let msp = array.msp().clone().execute::(&mut ctx)?; + assert_eq!(msp.as_slice::(), &[5]); + for (part, expected) in array.lower_parts().iter().zip([6u64, 7, 8]) { + let part = part.clone().execute::(&mut ctx)?; + assert_eq!(part.as_slice::(), &[expected]); + } + Ok(()) + } + + #[rstest] + #[case::i128(i128_parts(wide_i128_values(), Validity::AllValid))] + #[case::i256(i256_parts(wide_i256_values(), Validity::AllValid))] + fn test_scalar_at_matches_canonical(#[case] array: DecimalBytePartsArray) -> VortexResult<()> { + let mut ctx = array_session().create_execution_ctx(); + let canonical = array + .clone() + .into_array() + .execute::(&mut ctx)? + .into_array(); + let array = array.into_array(); + for idx in 0..array.len() { + assert_eq!( + array.execute_scalar(idx, &mut ctx)?, + canonical.execute_scalar(idx, &mut ctx)?, + "scalar mismatch at index {idx}" + ); + } + Ok(()) + } + + #[test] + fn test_scalar_at_null_with_lower_parts() -> VortexResult<()> { + let array = i128_parts( + vec![1i128 << 100, 2, 3], + Validity::Array(BoolArray::from_iter([false, true, true]).into_array()), + ) + .into_array(); + let mut ctx = array_session().create_execution_ctx(); + assert_eq!( + array.execute_scalar(0, &mut ctx)?, + Scalar::null(array.dtype().clone()) + ); + assert_eq!( + array.execute_scalar(1, &mut ctx)?, + Scalar::decimal( + DecimalValue::I128(2), + DecimalDType::new(38, 2), + Nullability::Nullable + ) + ); + Ok(()) + } + + #[rstest] + #[case::no_lower_parts( + encode(&DecimalArray::new(buffer![1i32, 2, 3], DecimalDType::new(9, 2), Validity::NonNullable)) + .vortex_expect("valid decimal byte parts") + )] + #[case::one_lower_part(i128_parts(wide_i128_values(), Validity::NonNullable))] + #[case::three_lower_parts(i256_parts(wide_i256_values(), Validity::NonNullable))] + #[case::nullable_three_lower_parts(i256_parts(wide_i256_values(), Validity::AllValid))] + fn test_serde_round_trip(#[case] array: DecimalBytePartsArray) -> VortexResult<()> { + let session = array_session(); + session.arrays().register(DecimalByteParts); + + let array = array.into_array(); + let dtype = array.dtype().clone(); + let len = array.len(); + let lower_part_count = array + .as_opt::() + .vortex_expect("byte parts array") + .lower_parts() + .len(); + + let array_ctx = ArrayContext::empty(); + let serialized = array.serialize(&array_ctx, &session, &SerializeOptions::default())?; + let mut concat = ByteBufferMut::empty(); + for buf in serialized { + concat.extend_from_slice(buf.as_ref()); + } + let parts = SerializedArray::try_from(concat.freeze())?; + let decoded = parts.decode(&dtype, len, &ReadContext::new(array_ctx.to_ids()), &session)?; + + assert_eq!( + decoded + .as_opt::() + .vortex_expect("byte parts array") + .lower_parts() + .len(), + lower_part_count, + "lower parts must survive serde" + ); + + let mut ctx = session.create_execution_ctx(); + assert_arrays_eq!(array, decoded, &mut ctx); + Ok(()) + } + + fn msp() -> ArrayRef { + buffer![1i64, 2, 3].into_array() + } + + fn lower_part() -> ArrayRef { + buffer![1u64, 2, 3].into_array() + } + + #[test] + fn test_rejects_signed_lower_part() { + assert!( + DecimalByteParts::try_new_with_lower_parts( + msp(), + vec![buffer![1i64, 2, 3].into_array()], + DecimalDType::new(38, 2), + ) + .is_err() + ); + } + + #[test] + fn test_rejects_nullable_lower_part() { + let nullable = PrimitiveArray::new(buffer![1u64, 2, 3], Validity::AllValid).into_array(); + assert!( + DecimalByteParts::try_new_with_lower_parts( + msp(), + vec![nullable], + DecimalDType::new(38, 2), + ) + .is_err() + ); + } + + #[test] + fn test_rejects_mismatched_lower_part_length() { + assert!( + DecimalByteParts::try_new_with_lower_parts( + msp(), + vec![buffer![1u64, 2].into_array()], + DecimalDType::new(38, 2), + ) + .is_err() + ); + } + + #[test] + fn test_rejects_too_many_lower_parts() { + assert!( + DecimalByteParts::try_new_with_lower_parts( + msp(), + vec![lower_part(), lower_part(), lower_part(), lower_part()], + DecimalDType::new(76, 2), + ) + .is_err() + ); + } + + fn deserialize_with( + lower_part_count: u32, + children: Vec, + ) -> VortexResult> { + let metadata = DecimalBytesPartsMetadata { + zeroth_child_ptype: PType::I64 as i32, + lower_part_count, + }; + DecimalByteParts.deserialize( + &DType::Decimal(DecimalDType::new(38, 2), Nullability::NonNullable), + 3, + &metadata.encode_to_vec(), + &[], + &children, + &array_session(), + ) + } + + #[test] + fn test_deserialize_reads_lower_parts() -> VortexResult<()> { + let parts = deserialize_with(1, vec![msp(), lower_part()])?; + let array = Array::try_from_parts(parts)?; + assert_eq!(array.lower_parts().len(), 1); + + let mut ctx = array_session().create_execution_ctx(); + let canonical = array.into_array().execute::(&mut ctx)?; + assert_eq!( + canonical.buffer::().as_slice(), + &[(1i128 << 64) | 1, (2i128 << 64) | 2, (3i128 << 64) | 3] + ); + Ok(()) + } + + #[test] + fn test_deserialize_rejects_child_count_mismatch() { + // Metadata claiming a lower part that was not serialized. + assert!(deserialize_with(1, vec![msp()]).is_err()); + // Metadata claiming fewer lower parts than there are children. + assert!(deserialize_with(0, vec![msp(), lower_part()]).is_err()); + // Metadata claiming more lower parts than the encoding supports. + assert!( + deserialize_with( + 4, + vec![ + msp(), + lower_part(), + lower_part(), + lower_part(), + lower_part() + ] + ) + .is_err() + ); + } + + #[test] + fn test_wide_decimal_buffer_types() -> VortexResult<()> { + let mut ctx = array_session().create_execution_ctx(); + + let i128_array = i128_parts(vec![1i128 << 100], Validity::NonNullable); + let canonical = i128_array.into_array().execute::(&mut ctx)?; + assert_eq!(canonical.values_type(), DecimalType::I128); + + let i256_array = i256_parts(vec![i256_of(1 << 100, 0)], Validity::NonNullable); + let canonical = i256_array.into_array().execute::(&mut ctx)?; + assert_eq!(canonical.values_type(), DecimalType::I256); + + // A narrow MSP with a single lower part still fits 128 bits. + let array = DecimalByteParts::try_new_with_lower_parts( + buffer![1i8, -1, 0].into_array(), + vec![buffer![7u64, 7, 7].into_array()], + DecimalDType::new(38, 2), + )?; + let canonical = array.into_array().execute::(&mut ctx)?; + assert_eq!(canonical.values_type(), DecimalType::I128); + assert_eq!( + canonical.buffer::().as_slice(), + &[(1i128 << 64) | 7, (-1i128 << 64) | 7, 7] + ); + + // Two lower parts under a narrow MSP overflow 128 bits, so the value widens. + let array = DecimalByteParts::try_new_with_lower_parts( + buffer![1i8].into_array(), + vec![buffer![0u64].into_array(), buffer![9u64].into_array()], + DecimalDType::new(76, 2), + )?; + let canonical = array.into_array().execute::(&mut ctx)?; + assert_eq!(canonical.values_type(), DecimalType::I256); + assert_eq!(canonical.buffer::().as_slice(), &[i256_of(1, 9)]); + Ok(()) + } + + #[test] + fn test_unused_buffer_of_values_is_ignored_for_null_rows() -> VortexResult<()> { + // Null rows may hold arbitrary bits in the lower parts; they must stay null. + let array = DecimalByteParts::try_new_with_lower_parts( + PrimitiveArray::new( + buffer![0i64, 0, 0], + Validity::Array(BoolArray::from_iter([false, false, true]).into_array()), + ) + .into_array(), + vec![buffer![7u64, 9, 11].into_array()], + DecimalDType::new(38, 2), + )? + .into_array(); + + let mut ctx = array_session().create_execution_ctx(); + assert_eq!( + array.execute_scalar(0, &mut ctx)?, + Scalar::null(array.dtype().clone()) + ); + let canonical = array.clone().execute::(&mut ctx)?; + assert_arrays_eq!(array, canonical.into_array(), &mut ctx); + Ok(()) + } } diff --git a/encodings/decimal-byte-parts/src/decimal_byte_parts/rules.rs b/encodings/decimal-byte-parts/src/decimal_byte_parts/rules.rs index d4052a4bed8..46cd3d794a8 100644 --- a/encodings/decimal-byte-parts/src/decimal_byte_parts/rules.rs +++ b/encodings/decimal-byte-parts/src/decimal_byte_parts/rules.rs @@ -37,15 +37,17 @@ impl ArrayParentReduceRule for DecimalBytePartsFilterPushDownR parent: ArrayView<'_, Filter>, _child_idx: usize, ) -> VortexResult> { - // TODO(ngates): we should benchmark whether to push-down filters with "lower parts". - // For now, we only push down if there are no lower parts. - if !child._lower_parts.is_empty() { - return Ok(None); - } - + // TODO(ngates): we should benchmark whether to push-down filters with "lower parts", + // which filters each part separately rather than the canonical wide buffer once. let new_msp = child.msp().filter(parent.filter_mask().clone())?; - let new_child = DecimalByteParts::try_new( + let new_lower_parts = child + .lower_parts() + .iter() + .map(|part| part.filter(parent.filter_mask().clone())) + .collect::>>()?; + let new_child = DecimalByteParts::try_new_with_lower_parts( new_msp, + new_lower_parts, *child .dtype() .as_decimal_opt() diff --git a/encodings/decimal-byte-parts/src/decimal_byte_parts/slice.rs b/encodings/decimal-byte-parts/src/decimal_byte_parts/slice.rs index 14807421c73..c5b6c549160 100644 --- a/encodings/decimal-byte-parts/src/decimal_byte_parts/slice.rs +++ b/encodings/decimal-byte-parts/src/decimal_byte_parts/slice.rs @@ -15,9 +15,16 @@ use crate::decimal_byte_parts::DecimalBytePartsArraySlotsExt; impl SliceReduce for DecimalByteParts { fn slice(array: ArrayView<'_, Self>, range: Range) -> VortexResult> { + let lower_parts = array + .lower_parts() + .iter() + .map(|part| part.slice(range.clone())) + .collect::>>()?; + Ok(Some( - DecimalByteParts::try_new( + DecimalByteParts::try_new_with_lower_parts( array.msp().slice(range)?, + lower_parts, *array .dtype() .as_decimal_opt() diff --git a/encodings/decimal-byte-parts/src/decimal_byte_parts/testing.rs b/encodings/decimal-byte-parts/src/decimal_byte_parts/testing.rs new file mode 100644 index 00000000000..bdee6df36a8 --- /dev/null +++ b/encodings/decimal-byte-parts/src/decimal_byte_parts/testing.rs @@ -0,0 +1,51 @@ +// SPDX-License-Identifier: Apache-2.0 +// SPDX-FileCopyrightText: Copyright the Vortex contributors + +//! Test-only helpers for building byte-parts arrays. + +use vortex_array::arrays::DecimalArray; +use vortex_array::dtype::DecimalDType; +use vortex_array::dtype::i256; +use vortex_array::validity::Validity; +use vortex_buffer::Buffer; +use vortex_error::VortexExpect; +use vortex_error::VortexResult; + +use crate::DecimalByteParts; +use crate::DecimalBytePartsArray; +use crate::decimal_byte_parts::limbs::split_decimal; + +/// Encode a canonical decimal array as byte parts, splitting wide values into lower parts. +pub(crate) fn encode(decimal: &DecimalArray) -> VortexResult { + let parts = split_decimal(decimal)?; + DecimalByteParts::try_new_with_lower_parts( + parts.msp, + parts.lower_parts, + decimal.decimal_dtype(), + ) +} + +/// An `i128`-backed decimal array, encoded as byte parts with one lower part. +pub(crate) fn i128_parts(values: Vec, validity: Validity) -> DecimalBytePartsArray { + encode(&DecimalArray::new( + Buffer::from(values), + DecimalDType::new(38, 2), + validity, + )) + .vortex_expect("valid decimal byte parts") +} + +/// An `i256`-backed decimal array, encoded as byte parts with three lower parts. +pub(crate) fn i256_parts(values: Vec, validity: Validity) -> DecimalBytePartsArray { + encode(&DecimalArray::new( + Buffer::from(values), + DecimalDType::new(76, 2), + validity, + )) + .vortex_expect("valid decimal byte parts") +} + +/// Build an `i256` from a signed high `i128` and unsigned low `u128`. +pub(crate) fn i256_of(high: i128, low: u128) -> i256 { + i256::from_parts(low, high) +} diff --git a/vortex-btrblocks/src/schemes/decimal.rs b/vortex-btrblocks/src/schemes/decimal/mod.rs similarity index 68% rename from vortex-btrblocks/src/schemes/decimal.rs rename to vortex-btrblocks/src/schemes/decimal/mod.rs index 1dff2171f60..47dc3050cd5 100644 --- a/vortex-btrblocks/src/schemes/decimal.rs +++ b/vortex-btrblocks/src/schemes/decimal/mod.rs @@ -10,12 +10,12 @@ use vortex_array::ExecutionCtx; use vortex_array::IntoArray; use vortex_array::VTable; use vortex_array::arrays::DecimalArray; -use vortex_array::arrays::PrimitiveArray; use vortex_array::arrays::decimal::narrowed_decimal; -use vortex_array::dtype::DecimalType; use vortex_compressor::scheme::CompressionEstimate; use vortex_compressor::scheme::EstimateVerdict; use vortex_decimal_byte_parts::DecimalByteParts; +use vortex_decimal_byte_parts::MAX_LOWER_PARTS; +use vortex_decimal_byte_parts::split_decimal; use vortex_error::VortexResult; use crate::ArrayAndStats; @@ -28,6 +28,10 @@ use crate::SchemeExt; /// /// Narrows the decimal to the smallest integer type, compresses the underlying primitive, and wraps /// the result in a `DecimalBytePartsArray`. +/// +/// Values that stay wider than 64 bits after narrowing are split into a signed most +/// significant part and 64-bit lower parts — one for `i128`, three for `i256` — each of +/// which is compressed independently. #[derive(Debug, Copy, Clone, PartialEq, Eq)] pub struct DecimalScheme; @@ -44,9 +48,9 @@ impl Scheme for DecimalScheme { vec![DecimalByteParts.id()] } - /// Children: primitive=0. + /// Children: msp=0, lower parts=1..=3. fn num_children(&self) -> usize { - 1 + 1 + MAX_LOWER_PARTS } fn expected_compression_ratio( @@ -66,22 +70,24 @@ impl Scheme for DecimalScheme { compress_ctx: CompressorContext, exec_ctx: &mut ExecutionCtx, ) -> VortexResult { - // TODO(joe): add support splitting i128/256 buffers into chunks of primitive values - // for compression. 2 for i128 and 4 for i256. let decimal = data.array().clone().execute::(exec_ctx)?; let decimal = narrowed_decimal(decimal); - let validity = decimal.validity()?; - let prim = match decimal.values_type() { - DecimalType::I8 => PrimitiveArray::new(decimal.buffer::(), validity), - DecimalType::I16 => PrimitiveArray::new(decimal.buffer::(), validity), - DecimalType::I32 => PrimitiveArray::new(decimal.buffer::(), validity), - DecimalType::I64 => PrimitiveArray::new(decimal.buffer::(), validity), - _ => return Ok(decimal.into_array()), - }; + let parts = split_decimal(&decimal)?; - let compressed = - compressor.compress_child(&prim.into_array(), &compress_ctx, self.id(), 0, exec_ctx)?; + let msp = compressor.compress_child(&parts.msp, &compress_ctx, self.id(), 0, exec_ctx)?; + let lower_parts = parts + .lower_parts + .iter() + .enumerate() + .map(|(idx, part)| { + compressor.compress_child(part, &compress_ctx, self.id(), idx + 1, exec_ctx) + }) + .collect::>>()?; - DecimalByteParts::try_new(compressed, decimal.decimal_dtype()).map(|d| d.into_array()) + DecimalByteParts::try_new_with_lower_parts(msp, lower_parts, decimal.decimal_dtype()) + .map(|d| d.into_array()) } } + +#[cfg(test)] +mod tests; diff --git a/vortex-btrblocks/src/schemes/decimal/tests.rs b/vortex-btrblocks/src/schemes/decimal/tests.rs new file mode 100644 index 00000000000..596c2900c07 --- /dev/null +++ b/vortex-btrblocks/src/schemes/decimal/tests.rs @@ -0,0 +1,189 @@ +// SPDX-License-Identifier: Apache-2.0 +// SPDX-FileCopyrightText: Copyright the Vortex contributors + +use std::iter; +use std::sync::LazyLock; + +use rstest::rstest; +use vortex_array::ArrayRef; +use vortex_array::IntoArray; +use vortex_array::VortexSessionExecute; +use vortex_array::arrays::DecimalArray; +use vortex_array::assert_arrays_eq; +use vortex_array::dtype::DecimalDType; +use vortex_array::dtype::DecimalType; +use vortex_array::dtype::i256; +use vortex_array::validity::Validity; +use vortex_buffer::Buffer; +use vortex_decimal_byte_parts::DecimalByteParts; +use vortex_decimal_byte_parts::DecimalBytePartsArraySlotsExt; +use vortex_decimal_byte_parts::MAX_LOWER_PARTS; +use vortex_error::VortexExpect; +use vortex_error::VortexResult; +use vortex_session::VortexSession; + +use crate::BtrBlocksCompressor; + +static SESSION: LazyLock = LazyLock::new(vortex_array::array_session); + +/// Number of values per array: above the 1024-value sampling threshold, so scheme selection +/// runs on sampled estimates as it does for real file chunks. +const N: usize = 16_384; + +fn ten_pow(exp: u32) -> i256 { + i256::from_i128(10).wrapping_pow(exp) +} + +/// Deterministic 24-bit noise, so the low part of each value is neither constant nor a +/// sequence — the realistic shape for a wide decimal column with a large fixed magnitude. +fn noise(seed: u64) -> impl Iterator { + let mut state = seed; + iter::repeat_with(move || { + state = state + .wrapping_mul(6_364_136_223_846_793_005) + .wrapping_add(1_442_695_040_888_963_407); + i128::from(state >> 40) + }) +} + +/// `i128`-backed values that need more than 64 bits, so the encoding must carry one lower +/// part. +fn wide_i128_array(validity: Validity) -> DecimalArray { + let base = 10i128.pow(25); + let values: Buffer = noise(7).take(N).map(|delta| base + delta).collect(); + DecimalArray::new(values, DecimalDType::new(38, 2), validity) +} + +/// `i256`-backed values that need more than 128 bits, so the encoding must carry three +/// lower parts. +fn wide_i256_array(validity: Validity) -> DecimalArray { + let base = ten_pow(40); + let values: Buffer = noise(11) + .take(N) + .map(|delta| base + i256::from_i128(delta)) + .collect(); + DecimalArray::new(values, DecimalDType::new(76, 2), validity) +} + +fn compress(array: &ArrayRef) -> VortexResult { + BtrBlocksCompressor::default().compress(array, &mut SESSION.create_execution_ctx()) +} + +fn byte_parts(array: &ArrayRef) -> &ArrayRef { + assert!( + array.is::(), + "expected DecimalByteParts, got {}", + array.encoding_id() + ); + array +} + +fn lower_part_count(array: &ArrayRef) -> usize { + byte_parts(array) + .as_opt::() + .vortex_expect("byte parts array") + .lower_parts() + .len() +} + +#[rstest] +#[case::non_nullable(Validity::NonNullable)] +#[case::all_valid(Validity::AllValid)] +#[case::nullable(Validity::from_iter((0..N).map(|i| i % 3 != 0)))] +fn test_i128_decimal_splits_into_one_lower_part(#[case] validity: Validity) -> VortexResult<()> { + let array = wide_i128_array(validity).into_array(); + let compressed = compress(&array)?; + + assert_eq!(lower_part_count(&compressed), 1); + assert_eq!(compressed.dtype(), array.dtype()); + assert_arrays_eq!(array, compressed, &mut SESSION.create_execution_ctx()); + Ok(()) +} + +#[rstest] +#[case::non_nullable(Validity::NonNullable)] +#[case::all_valid(Validity::AllValid)] +#[case::nullable(Validity::from_iter((0..N).map(|i| i % 5 != 0)))] +fn test_i256_decimal_splits_into_three_lower_parts(#[case] validity: Validity) -> VortexResult<()> { + let array = wide_i256_array(validity).into_array(); + let compressed = compress(&array)?; + + assert_eq!(lower_part_count(&compressed), MAX_LOWER_PARTS); + assert_eq!(compressed.dtype(), array.dtype()); + assert_arrays_eq!(array, compressed, &mut SESSION.create_execution_ctx()); + Ok(()) +} + +#[test] +fn test_i256_decimal_round_trips_extreme_values() -> VortexResult<()> { + // Every 64-bit window exercised, including the sign boundary of the most significant + // part. Bounded by the precision so the values are legal `Decimal(76, 0)` scalars. + let max = ten_pow(76) - i256::ONE; + let values: Buffer = (0..N) + .map(|i| match i % 8 { + 0 => i256::ZERO, + 1 => i256::ONE, + 2 => i256::ZERO - i256::ONE, + 3 => i256::from_parts(u128::MAX, 0), + 4 => i256::from_parts(0, 1), + 5 => i256::from_parts(0, -1), + 6 => max, + _ => i256::ZERO - max, + }) + .collect(); + let array = + DecimalArray::new(values, DecimalDType::new(76, 0), Validity::NonNullable).into_array(); + + let compressed = compress(&array)?; + assert_arrays_eq!(array, compressed, &mut SESSION.create_execution_ctx()); + Ok(()) +} + +#[test] +fn test_narrow_decimal_has_no_lower_parts() -> VortexResult<()> { + // Values that fit 64 bits are narrowed rather than split, even when the declared + // precision needs an i256. + let values: Buffer = (0..N as i128).map(|i| i256::from_i128(i * 3)).collect(); + let array = + DecimalArray::new(values, DecimalDType::new(76, 2), Validity::NonNullable).into_array(); + + let compressed = compress(&array)?; + assert_eq!(lower_part_count(&compressed), 0); + assert_arrays_eq!(array, compressed, &mut SESSION.create_execution_ctx()); + Ok(()) +} + +#[test] +fn test_canonical_of_compressed_wide_decimal_keeps_storage_width() -> VortexResult<()> { + let mut ctx = SESSION.create_execution_ctx(); + + let array = wide_i128_array(Validity::NonNullable).into_array(); + let canonical = compress(&array)?.execute::(&mut ctx)?; + assert_eq!(canonical.values_type(), DecimalType::I128); + + let array = wide_i256_array(Validity::NonNullable).into_array(); + let canonical = compress(&array)?.execute::(&mut ctx)?; + assert_eq!(canonical.values_type(), DecimalType::I256); + Ok(()) +} + +/// Splitting exists to make wide decimals compressible: the parts that do not vary collapse +/// to constants and the varying part bit-packs. Without splitting these arrays are stored as +/// raw 16- and 32-byte values. +#[rstest] +#[case::i128(wide_i128_array(Validity::NonNullable), 16)] +#[case::i256(wide_i256_array(Validity::NonNullable), 32)] +fn test_wide_decimals_compress( + #[case] array: DecimalArray, + #[case] uncompressed_bytes_per_value: usize, +) -> VortexResult<()> { + let array = array.into_array(); + let uncompressed = u64::try_from(uncompressed_bytes_per_value * N)?; + let compressed = compress(&array)?.nbytes(); + + assert!( + compressed * 4 < uncompressed, + "expected at least 4x compression, got {uncompressed} -> {compressed} bytes" + ); + Ok(()) +} diff --git a/vortex-cuda/src/kernel/encodings/decimal_byte_parts.rs b/vortex-cuda/src/kernel/encodings/decimal_byte_parts.rs index 3475f26a175..a54df06fb4c 100644 --- a/vortex-cuda/src/kernel/encodings/decimal_byte_parts.rs +++ b/vortex-cuda/src/kernel/encodings/decimal_byte_parts.rs @@ -39,6 +39,13 @@ impl CudaExecute for DecimalBytePartsExecutor { .dtype() .as_decimal_opt() .vortex_expect("DecimalBytePartsArray dtype must be decimal"); + + // Reassembling lower parts into wide decimals is not implemented on the GPU; the MSP + // alone is not the value. + if !array.lower_parts().is_empty() { + vortex_bail!("DecimalBytePartsArray with lower parts is not supported on GPU") + } + let msp = array.msp().clone(); let PrimitiveDataParts { buffer, diff --git a/vortex-file/src/tests.rs b/vortex-file/src/tests.rs index 50fe702b96d..3da8fe1f016 100644 --- a/vortex-file/src/tests.rs +++ b/vortex-file/src/tests.rs @@ -36,6 +36,7 @@ use vortex_array::dtype::Nullability; use vortex_array::dtype::PType; use vortex_array::dtype::PType::I32; use vortex_array::dtype::StructFields; +use vortex_array::dtype::i256; use vortex_array::expr::and; use vortex_array::expr::cast; use vortex_array::expr::col; @@ -226,6 +227,87 @@ async fn test_round_trip_many_types() { assert_eq!(read.len(), 3); } +/// End-to-end check that decimals wider than 64 bits survive a write/read round trip and are +/// actually compressed: the compressor splits them into a most significant part plus 64-bit +/// lower parts, each of which compresses on its own. +#[tokio::test] +#[cfg_attr(miri, ignore)] +async fn test_wide_decimal_round_trip_compresses() -> VortexResult<()> { + const N: usize = 16_384; + /// Bytes each value would occupy uncompressed: `i128` plus `i256` storage. + const UNCOMPRESSED_BYTES_PER_ROW: usize = 16 + 32; + + /// Deterministic 24-bit noise, so the low bits of each value are neither constant nor a + /// sequence. + fn noise(seed: u64) -> impl Iterator { + let mut state = seed; + iter::repeat_with(move || { + state = state + .wrapping_mul(6_364_136_223_846_793_005) + .wrapping_add(1_442_695_040_888_963_407); + i128::from(state >> 40) + }) + } + + // Values that need more than 64 bits, so `i128` storage cannot be narrowed away. + let decimal_38 = DecimalArray::new( + noise(7) + .take(N) + .map(|delta| 10i128.pow(25) + delta) + .collect::>(), + DecimalDType::new(38, 2), + Validity::NonNullable, + ) + .into_array(); + + // Values that need more than 128 bits, so `i256` storage cannot be narrowed away. + let base = i256::from_i128(10).wrapping_pow(40); + let decimal_76 = DecimalArray::new( + noise(11) + .take(N) + .map(|delta| base + i256::from_i128(delta)) + .collect::>(), + DecimalDType::new(76, 4), + Validity::from_iter((0..N).map(|i| i % 9 != 0)), + ) + .into_array(); + + let st = StructArray::from_fields(&[ + ("decimal_38", decimal_38), + ("decimal_76_nullable", decimal_76), + ])? + .into_array(); + let dtype = st.dtype().clone(); + + let mut buf = ByteBufferMut::empty(); + SESSION + .write_options() + .write(&mut buf, st.clone().to_array_stream()) + .await?; + let written = buf.len(); + + let chunks: Vec<_> = SESSION + .open_options() + .open_buffer(buf)? + .scan()? + .into_array_stream()? + .try_collect() + .await?; + let read = ChunkedArray::try_new(chunks, dtype)?.into_array(); + + let mut ctx = SESSION.create_execution_ctx(); + assert_eq!(read.len(), N); + assert_arrays_eq!(st, read, &mut ctx); + + let uncompressed = N * UNCOMPRESSED_BYTES_PER_ROW; + assert!( + written * 4 < uncompressed, + "expected at least 4x compression, wrote {written} bytes for {uncompressed} bytes of \ + decimal values" + ); + Ok(()) +} + #[tokio::test] #[cfg_attr(miri, ignore)] async fn test_read_simple_with_spawn() { diff --git a/vortex-test/compat-gen/src/fixtures/arrays/synthetic/encodings/decimal_byte_parts.rs b/vortex-test/compat-gen/src/fixtures/arrays/synthetic/encodings/decimal_byte_parts.rs index 7e9ff6b7809..5571a8901a6 100644 --- a/vortex-test/compat-gen/src/fixtures/arrays/synthetic/encodings/decimal_byte_parts.rs +++ b/vortex-test/compat-gen/src/fixtures/arrays/synthetic/encodings/decimal_byte_parts.rs @@ -5,18 +5,33 @@ use vortex::array::ArrayId; use vortex::array::ArrayRef; use vortex::array::ArrayVTable; use vortex::array::IntoArray; +use vortex::array::arrays::DecimalArray; use vortex::array::arrays::PrimitiveArray; use vortex::array::arrays::StructArray; use vortex::array::dtype::DecimalDType; use vortex::array::dtype::FieldNames; +use vortex::array::dtype::i256; use vortex::array::validity::Validity; +use vortex::buffer::Buffer; use vortex::encodings::decimal_byte_parts::DecimalByteParts; +use vortex::encodings::decimal_byte_parts::DecimalBytePartsArray; +use vortex::encodings::decimal_byte_parts::split_decimal; use vortex::error::VortexResult; use vortex_array::ExecutionCtx; use super::N; use crate::fixtures::FlatLayoutFixture; +/// Encode a canonical decimal as byte parts, splitting wide values into lower parts. +fn encode_byte_parts(decimal: &DecimalArray) -> VortexResult { + let parts = split_decimal(decimal)?; + DecimalByteParts::try_new_with_lower_parts( + parts.msp, + parts.lower_parts, + decimal.decimal_dtype(), + ) +} + pub struct DecimalBytePartsFixture; impl FlatLayoutFixture for DecimalBytePartsFixture { @@ -80,6 +95,28 @@ impl FlatLayoutFixture for DecimalBytePartsFixture { let near_limit_arr = DecimalByteParts::try_new(near_limit_values.into_array(), near_limit_dtype)?; + // Wide decimals, split into an MSP plus 64-bit lower parts. + let wide_128_dtype = DecimalDType::new(38, 2); + let wide_128 = DecimalArray::new( + (0..N as i128) + .map(|i| 10i128.pow(25) + i * 7) + .collect::>(), + wide_128_dtype, + Validity::NonNullable, + ); + let wide_128_arr = encode_byte_parts(&wide_128)?; + + let wide_256_dtype = DecimalDType::new(76, 2); + let base = i256::from_i128(10).wrapping_pow(40); + let wide_256 = DecimalArray::new( + (0..N as i128) + .map(|i| base + i256::from_i128(i * 7)) + .collect::>(), + wide_256_dtype, + Validity::from_iter((0..N).map(|i| i % 7 != 0)), + ); + let wide_256_arr = encode_byte_parts(&wide_256)?; + let arr = StructArray::try_new( FieldNames::from([ "dec_10_2", @@ -90,6 +127,8 @@ impl FlatLayoutFixture for DecimalBytePartsFixture { "dec_crossing", "dec_trailing_zero", "dec_near_limit", + "dec_wide_128", + "dec_wide_256_nullable", ]), vec![ decimal_arr.into_array(), @@ -100,6 +139,8 @@ impl FlatLayoutFixture for DecimalBytePartsFixture { crossing_arr.into_array(), trailing_zero_arr.into_array(), near_limit_arr.into_array(), + wide_128_arr.into_array(), + wide_256_arr.into_array(), ], N, Validity::NonNullable, From bf7c50f8b6c8d34872df393b606d57123ea7b923 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 31 Jul 2026 15:29:45 +0000 Subject: [PATCH 02/14] Specialize decimal part assembly on the part count MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reassembling byte parts filled a stack array of 64-bit words per row at indices derived from a runtime part count, so every word placement was a dynamic index with a bounds check and nothing about the loop was known to the compiler. `benches/decimal_assemble.rs` benchmarks the candidate shapes over 65,536 rows, each spelled out in the bench so the comparison can be re-run from any revision: | shape | i128 (1 part) | i256 (3 parts) | | --------------------------- | ------------- | -------------- | | row, runtime part count | 114.4 µs | 307.5 µs | | row, constant part count | 91.7 µs | 178.2 µs | | column, lane writes | - | 401.3 µs | | column, lane writes blocked | - | 289.3 µs | | column, whole-value shifts | 88.0 µs | 2.03 ms | Row-at-a-time is not what costs — the runtime part count is. Columnar is worse for `i256`: the output word for a given part is strided by 32 bytes, so each pass scatters, and expressing the pass as whole-value shifts pays 256-bit arithmetic per row. Only for `i128`, at 16 bytes per row, does a two-pass column shape match the specialized row loop, and there both are memory bound. So the assembly loops now take the part count as a const parameter, with `assemble_decimal` dispatching 1/2/3 parts into monomorphized bodies, and the `i128` path — where a signed MSP can only ever share 128 bits with one lower part — is specialized outright. Parts are sliced to the MSP's length up front so the per-row bounds checks fall away. Through the public API, on the same 65,536 rows: | benchmark | before | after | speedup | | -------------------------------- | -------- | -------- | ------- | | `i128_assemble_shipped` | 114.7 µs | 93.1 µs | 1.23x | | `i256_assemble_shipped` | 358.4 µs | 201.3 µs | 1.78x | | `canonicalize_byte_parts` 1 part | 118.6 µs | 92.0 µs | 1.29x | | `canonicalize_byte_parts` 3 part | 361.0 µs | 201.8 µs | 1.79x | `assemble_decimal` is now public, matching `split_decimal`, so the benchmark can call the shipped path directly. Signed-off-by: "Joe Isaacs" --- Cargo.lock | 1 + encodings/decimal-byte-parts/Cargo.toml | 5 + .../benches/decimal_assemble.rs | 346 ++++++++++++++++++ .../src/decimal_byte_parts/limbs.rs | 80 ++-- .../src/decimal_byte_parts/mod.rs | 2 +- 5 files changed, 407 insertions(+), 27 deletions(-) create mode 100644 encodings/decimal-byte-parts/benches/decimal_assemble.rs diff --git a/Cargo.lock b/Cargo.lock index 51188781c9a..9480c430f3f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -9686,6 +9686,7 @@ dependencies = [ name = "vortex-decimal-byte-parts" version = "0.1.0" dependencies = [ + "codspeed-divan-compat", "num-traits", "prost 0.14.4", "rstest", diff --git a/encodings/decimal-byte-parts/Cargo.toml b/encodings/decimal-byte-parts/Cargo.toml index 4934ec4fa27..04610a83763 100644 --- a/encodings/decimal-byte-parts/Cargo.toml +++ b/encodings/decimal-byte-parts/Cargo.toml @@ -26,5 +26,10 @@ vortex-mask = { workspace = true } vortex-session = { workspace = true } [dev-dependencies] +divan = { workspace = true } rstest = { workspace = true } vortex-array = { path = "../../vortex-array", features = ["_test-harness"] } + +[[bench]] +name = "decimal_assemble" +harness = false diff --git a/encodings/decimal-byte-parts/benches/decimal_assemble.rs b/encodings/decimal-byte-parts/benches/decimal_assemble.rs new file mode 100644 index 00000000000..29fd887a888 --- /dev/null +++ b/encodings/decimal-byte-parts/benches/decimal_assemble.rs @@ -0,0 +1,346 @@ +// SPDX-License-Identifier: Apache-2.0 +// SPDX-FileCopyrightText: Copyright the Vortex contributors + +//! Reassembling `DecimalByteParts` into canonical `i128`/`i256` values. +//! +//! Canonicalizing a wide decimal walks a most significant part plus one (`i128`) or three +//! (`i256`) unsigned 64-bit lower parts, and has to produce one wide value per row. The +//! obvious shapes are: +//! +//! - **row**: one pass, gathering the row's word from each part and combining. Sub-variants +//! differ only in whether the part count is known to the compiler. +//! - **column**: one pass per part over the whole output, either accumulating into the wide +//! values or writing 64-bit lanes directly. +//! +//! Every candidate is spelled out here rather than called through the crate, so the same +//! comparison can be run from any revision. `assemble_*_shipped` calls the public API and +//! pins whichever shape the crate currently uses. +//! +//! At 65,536 rows the row shape wins, and what costs is a part count the compiler cannot +//! see, not the row-at-a-time access: specializing it is 1.25x (`i128`) and 1.8x (`i256`). +//! Columnar loses for `i256` because each lane store is strided by 32 bytes — 2.2x slower +//! than the specialized row loop, still 1.6x when cache blocked, and 11x when expressed as +//! whole-value shifts. For `i128` the two-pass column shape ties the specialized row loop: +//! at 16 bytes per row both are memory bound. + +#![allow(clippy::unwrap_used, clippy::cast_possible_truncation)] + +use divan::Bencher; +use divan::black_box; +use vortex_array::IntoArray; +use vortex_array::arrays::PrimitiveArray; +use vortex_array::dtype::DecimalDType; +use vortex_array::dtype::i256; +use vortex_array::validity::Validity; +use vortex_buffer::Alignment; +use vortex_buffer::Buffer; +use vortex_buffer::BufferMut; +use vortex_decimal_byte_parts::assemble_decimal; + +fn main() { + divan::main(); +} + +/// Rows per benchmark: a typical scan chunk, and large enough that the output does not fit +/// in L2, so the extra passes of a columnar shape are paid at their real cost. +const LEN: usize = 65_536; + +/// Rows per block in the cache-blocked columnar variant: the block's output (32 KiB of +/// `i256`) stays in L1 across all four lane passes. +const BLOCK: usize = 1024; + +const WORD_BITS: usize = 64; + +/// Deterministic pseudo-random words, so no part is constant or a sequence. +fn words(seed: u64, len: usize) -> Buffer { + let mut state = seed; + (0..len) + .map(|_| { + state = state + .wrapping_mul(6_364_136_223_846_793_005) + .wrapping_add(1_442_695_040_888_963_407); + state + }) + .collect() +} + +fn msp(seed: u64, len: usize) -> Buffer { + words(seed, len) + .iter() + .map(|w| (w >> 40).cast_signed()) + .collect() +} + +// --------------------------------------------------------------------------------------- +// i128: one lower part +// --------------------------------------------------------------------------------------- + +/// The row shape with the part count only known at runtime: a fold over an iterator of the +/// row's words. +fn i128_row_dynamic(msp: &[i64], lower: &[&[u64]]) -> Buffer { + let mut out = BufferMut::::with_capacity(msp.len()); + for (row, m) in msp.iter().enumerate() { + out.push(lower.iter().fold(i128::from(*m), |acc, part| { + (acc << WORD_BITS) | i128::from(part[row]) + })); + } + out.freeze() +} + +/// The row shape specialized to exactly one lower part. +fn i128_row_const(msp: &[i64], lower: &[u64]) -> Buffer { + let mut out = BufferMut::::with_capacity(msp.len()); + for (m, l) in msp.iter().zip(lower) { + out.push((i128::from(*m) << WORD_BITS) | i128::from(*l)); + } + out.freeze() +} + +/// The column shape: one pass writes the most significant half of every value, a second +/// pass ORs in the lower part. +fn i128_column(msp: &[i64], lower: &[u64]) -> Buffer { + let mut out = BufferMut::::zeroed(msp.len()); + for (o, m) in out.as_mut_slice().iter_mut().zip(msp) { + *o = i128::from(*m) << WORD_BITS; + } + for (o, l) in out.as_mut_slice().iter_mut().zip(lower) { + *o |= i128::from(*l); + } + out.freeze() +} + +// --------------------------------------------------------------------------------------- +// i256: three lower parts +// --------------------------------------------------------------------------------------- + +/// The row shape with a runtime part count: per row, a stack array of words is filled at +/// dynamic indices and then packed. +fn i256_row_dynamic(msp: &[i64], lower: &[&[u64]]) -> Buffer { + let count = lower.len(); + let mut out = BufferMut::::with_capacity(msp.len()); + for (row, m) in msp.iter().enumerate() { + let mut w = [if *m < 0 { u64::MAX } else { 0 }; 4]; + for (i, part) in lower.iter().enumerate() { + w[count - 1 - i] = part[row]; + } + w[count] = m.cast_unsigned(); + out.push(i256::from_parts( + u128::from(w[0]) | (u128::from(w[1]) << WORD_BITS), + (u128::from(w[2]) | (u128::from(w[3]) << WORD_BITS)).cast_signed(), + )); + } + out.freeze() +} + +/// The row shape specialized to exactly three lower parts: every word index is a constant. +fn i256_row_const(msp: &[i64], lower: [&[u64]; 3]) -> Buffer { + let mut out = BufferMut::::with_capacity(msp.len()); + for row in 0..msp.len() { + out.push(i256::from_parts( + u128::from(lower[2][row]) | (u128::from(lower[1][row]) << WORD_BITS), + (u128::from(lower[0][row]) | (u128::from(msp[row].cast_unsigned()) << WORD_BITS)) + .cast_signed(), + )); + } + out.freeze() +} + +/// The column shape as wide arithmetic: one pass per part, each shifting the whole output +/// left and ORing the part in. Four read-modify-write passes over 32 bytes per row. +fn i256_column_accumulate(msp: &[i64], lower: [&[u64]; 3]) -> Buffer { + let mut out = BufferMut::::zeroed(msp.len()); + for (o, m) in out.as_mut_slice().iter_mut().zip(msp) { + *o = i256::from_i128(i128::from(*m)); + } + for part in lower { + for (o, w) in out.as_mut_slice().iter_mut().zip(part) { + *o = (*o << WORD_BITS) | i256::from_parts(u128::from(*w), 0); + } + } + out.freeze() +} + +/// The column shape as lane writes: build the output as 64-bit words and write one lane per +/// pass. Avoids re-reading the output, but every store is strided by 32 bytes. +fn i256_column_lanes(msp: &[i64], lower: [&[u64]; 3]) -> Buffer { + let len = msp.len(); + let mut w = BufferMut::::zeroed_aligned(len * 4, Alignment::of::()); + let lanes = w.as_mut_slice(); + for (i, m) in msp.iter().enumerate() { + lanes[i * 4 + 3] = m.cast_unsigned(); + } + for (lane, part) in lower.iter().enumerate() { + for (i, word) in part.iter().enumerate() { + lanes[i * 4 + 2 - lane] = *word; + } + } + // Word order within an `i256` is ascending significance on a little-endian host. + assert!(cfg!(target_endian = "little")); + Buffer::::from_byte_buffer_aligned(w.freeze().into_byte_buffer(), Alignment::of::()) +} + +/// The column shape, cache blocked: the lane passes run over one block of rows at a time so +/// the block's output stays resident between passes. +fn i256_column_lanes_blocked(msp: &[i64], lower: [&[u64]; 3]) -> Buffer { + let len = msp.len(); + let mut w = BufferMut::::zeroed_aligned(len * 4, Alignment::of::()); + let lanes = w.as_mut_slice(); + for start in (0..len).step_by(BLOCK) { + let end = (start + BLOCK).min(len); + for i in start..end { + lanes[i * 4 + 3] = msp[i].cast_unsigned(); + } + for (lane, part) in lower.iter().enumerate() { + for i in start..end { + lanes[i * 4 + 2 - lane] = part[i]; + } + } + } + assert!(cfg!(target_endian = "little")); + Buffer::::from_byte_buffer_aligned(w.freeze().into_byte_buffer(), Alignment::of::()) +} + +// --------------------------------------------------------------------------------------- +// Benchmarks +// --------------------------------------------------------------------------------------- + +struct Parts { + msp: Buffer, + lower: Vec>, +} + +impl Parts { + fn new(lower_parts: usize) -> Self { + Self { + msp: msp(1, LEN), + lower: (0..lower_parts).map(|i| words(7 + i as u64, LEN)).collect(), + } + } + + fn lower_slices(&self) -> Vec<&[u64]> { + self.lower.iter().map(|part| part.as_slice()).collect() + } + + fn arrays(&self) -> (PrimitiveArray, Vec) { + ( + PrimitiveArray::new(self.msp.clone(), Validity::NonNullable), + self.lower + .iter() + .map(|part| PrimitiveArray::new(part.clone(), Validity::NonNullable)) + .collect(), + ) + } +} + +#[divan::bench] +fn i128_row_dynamic_parts(bencher: Bencher) { + let parts = Parts::new(1); + let lower = parts.lower_slices(); + bencher.bench(|| i128_row_dynamic(black_box(parts.msp.as_slice()), black_box(&lower))); +} + +#[divan::bench] +fn i128_row_const_parts(bencher: Bencher) { + let parts = Parts::new(1); + let lower = parts.lower_slices(); + bencher.bench(|| i128_row_const(black_box(parts.msp.as_slice()), black_box(lower[0]))); +} + +#[divan::bench] +fn i128_column_parts(bencher: Bencher) { + let parts = Parts::new(1); + let lower = parts.lower_slices(); + bencher.bench(|| i128_column(black_box(parts.msp.as_slice()), black_box(lower[0]))); +} + +#[divan::bench] +fn i256_row_dynamic_parts(bencher: Bencher) { + let parts = Parts::new(3); + let lower = parts.lower_slices(); + bencher.bench(|| i256_row_dynamic(black_box(parts.msp.as_slice()), black_box(&lower))); +} + +#[divan::bench] +fn i256_row_const_parts(bencher: Bencher) { + let parts = Parts::new(3); + let lower = parts.lower_slices(); + let lower = [lower[0], lower[1], lower[2]]; + bencher.bench(|| i256_row_const(black_box(parts.msp.as_slice()), black_box(lower))); +} + +#[divan::bench] +fn i256_column_accumulate_parts(bencher: Bencher) { + let parts = Parts::new(3); + let lower = parts.lower_slices(); + let lower = [lower[0], lower[1], lower[2]]; + bencher.bench(|| i256_column_accumulate(black_box(parts.msp.as_slice()), black_box(lower))); +} + +#[divan::bench] +fn i256_column_lanes_parts(bencher: Bencher) { + let parts = Parts::new(3); + let lower = parts.lower_slices(); + let lower = [lower[0], lower[1], lower[2]]; + bencher.bench(|| i256_column_lanes(black_box(parts.msp.as_slice()), black_box(lower))); +} + +#[divan::bench] +fn i256_column_lanes_blocked_parts(bencher: Bencher) { + let parts = Parts::new(3); + let lower = parts.lower_slices(); + let lower = [lower[0], lower[1], lower[2]]; + bencher.bench(|| i256_column_lanes_blocked(black_box(parts.msp.as_slice()), black_box(lower))); +} + +/// The shape the crate actually ships, including the buffer allocation and the ptype +/// dispatch, for one lower part. +#[divan::bench] +fn i128_assemble_shipped(bencher: Bencher) { + let parts = Parts::new(1); + let (msp, lower) = parts.arrays(); + let dtype = DecimalDType::new(38, 2); + bencher.bench(|| assemble_decimal(black_box(&msp), black_box(&lower), dtype).unwrap()); +} + +/// The shape the crate actually ships, for three lower parts. +#[divan::bench] +fn i256_assemble_shipped(bencher: Bencher) { + let parts = Parts::new(3); + let (msp, lower) = parts.arrays(); + let dtype = DecimalDType::new(76, 2); + bencher.bench(|| assemble_decimal(black_box(&msp), black_box(&lower), dtype).unwrap()); +} + +/// Canonicalizing through the public array API, so the child execution and validity handling +/// around the assembly loop are included. +#[divan::bench(args = [1, 3])] +fn canonicalize_byte_parts(bencher: Bencher, lower_parts: usize) { + use vortex_array::VortexSessionExecute; + use vortex_array::array_session; + use vortex_array::arrays::DecimalArray; + use vortex_decimal_byte_parts::DecimalByteParts; + + let parts = Parts::new(lower_parts); + let (msp, lower) = parts.arrays(); + let dtype = if lower_parts == 1 { + DecimalDType::new(38, 2) + } else { + DecimalDType::new(76, 2) + }; + let array = DecimalByteParts::try_new_with_lower_parts( + msp.into_array(), + lower.into_iter().map(IntoArray::into_array).collect(), + dtype, + ) + .unwrap() + .into_array(); + + let session = array_session(); + bencher + .with_inputs(|| session.create_execution_ctx()) + .bench_refs(|ctx| { + black_box(array.clone()) + .execute::(ctx) + .unwrap() + }); +} diff --git a/encodings/decimal-byte-parts/src/decimal_byte_parts/limbs.rs b/encodings/decimal-byte-parts/src/decimal_byte_parts/limbs.rs index 4cf9c875019..c5d20933aee 100644 --- a/encodings/decimal-byte-parts/src/decimal_byte_parts/limbs.rs +++ b/encodings/decimal-byte-parts/src/decimal_byte_parts/limbs.rs @@ -32,6 +32,7 @@ use vortex_buffer::Buffer; use vortex_buffer::BufferMut; use vortex_error::VortexResult; use vortex_error::vortex_bail; +use vortex_error::vortex_ensure; /// The maximum number of lower parts an encoded decimal can carry. /// @@ -130,18 +131,40 @@ pub fn assemble_decimal( })); } + // Slice every part to the MSP's length up front: the assembly loops then index slices the + // compiler knows are long enough, so the per-row bounds checks fall away. + let len = msp.len(); let lower: Vec<&[u64]> = lower_parts .iter() - .map(|part| part.as_slice::()) - .collect(); - Ok( - match assembled_values_type(msp.ptype(), lower_parts.len())? { - DecimalType::I256 => { - DecimalArray::new(assemble_i256(msp, &lower), decimal_dtype, validity) - } - _ => DecimalArray::new(assemble_i128(msp, &lower), decimal_dtype, validity), + .map(|part| { + let part = part.as_slice::(); + vortex_ensure!( + part.len() >= len, + "lower part has len {}, expected at least {len}", + part.len() + ); + Ok(&part[..len]) + }) + .collect::>()?; + + // The part count is dispatched to a constant so every 64-bit word lands at a compile-time + // index. Leaving it dynamic costs 1.8x on the `i256` path — see `benches/decimal_assemble.rs`. + let values = match assembled_values_type(msp.ptype(), lower.len())? { + DecimalType::I256 => match lower.as_slice() { + [first] => assemble_i256(msp, [first]), + [first, second] => assemble_i256(msp, [first, second]), + [first, second, third] => assemble_i256(msp, [first, second, third]), + _ => vortex_bail!("unsupported lower part count {}", lower.len()), }, - ) + _ => { + return Ok(DecimalArray::new( + assemble_i128(msp, lower[0]), + decimal_dtype, + validity, + )); + } + }; + Ok(DecimalArray::new(values, decimal_dtype, validity)) } /// Combine a single row's parts into an `i128`. @@ -231,38 +254,43 @@ fn split_i256(values: &Buffer) -> (Buffer, [Buffer; MAX_LOWER_PA (msp.freeze(), lower.map(BufferMut::freeze)) } +/// Only one lower part can share 128 bits with a signed MSP, so this shape is fixed. #[expect( clippy::useless_conversion, reason = "the widening to i64 is a no-op only for the i64 arm of the ptype match" )] -fn assemble_i128(msp: &PrimitiveArray, lower: &[&[u64]]) -> Buffer { - let len = msp.len(); - let mut out = BufferMut::::with_capacity(len); +fn assemble_i128(msp: &PrimitiveArray, lower: &[u64]) -> Buffer { + let mut out = BufferMut::::with_capacity(msp.len()); match_each_signed_integer_ptype!(msp.ptype(), |P| { - let values = msp.as_slice::

(); - for (row, value) in values.iter().enumerate() { - out.push(combine_i128( - i64::from(*value), - lower.iter().map(|part| part[row]), - )); + for (value, part) in msp.as_slice::

().iter().zip(lower) { + out.push((i128::from(i64::from(*value)) << LOWER_PART_BITS) | i128::from(*part)); } }); out.freeze() } +/// The lower parts fill the least significant 64-bit words, the MSP the word above them, and +/// the remaining high words are the MSP's sign extension. +/// +/// `K` is a constant so the word indices are compile-time constants and the placement loop +/// unrolls; the same loop with a runtime part count is 1.8x slower. #[expect( clippy::useless_conversion, reason = "the widening to i64 is a no-op only for the i64 arm of the ptype match" )] -fn assemble_i256(msp: &PrimitiveArray, lower: &[&[u64]]) -> Buffer { - let len = msp.len(); - let mut out = BufferMut::::with_capacity(len); +fn assemble_i256(msp: &PrimitiveArray, lower: [&[u64]; K]) -> Buffer { + let mut out = BufferMut::::with_capacity(msp.len()); match_each_signed_integer_ptype!(msp.ptype(), |P| { - let values = msp.as_slice::

(); - for (row, value) in values.iter().enumerate() { - out.push(combine_i256( - i64::from(*value), - lower.iter().map(|part| part[row]), + for (row, value) in msp.as_slice::

().iter().enumerate() { + let value = i64::from(*value); + let mut words = [if value < 0 { u64::MAX } else { 0 }; 4]; + for (i, part) in lower.iter().enumerate() { + words[K - 1 - i] = part[row]; + } + words[K] = value.cast_unsigned(); + out.push(i256::from_parts( + u128::from(words[0]) | (u128::from(words[1]) << LOWER_PART_BITS), + (u128::from(words[2]) | (u128::from(words[3]) << LOWER_PART_BITS)).cast_signed(), )); } }); diff --git a/encodings/decimal-byte-parts/src/decimal_byte_parts/mod.rs b/encodings/decimal-byte-parts/src/decimal_byte_parts/mod.rs index 5b3b1fd4f6f..141e55cf7d9 100644 --- a/encodings/decimal-byte-parts/src/decimal_byte_parts/mod.rs +++ b/encodings/decimal-byte-parts/src/decimal_byte_parts/mod.rs @@ -18,6 +18,7 @@ pub(crate) mod testing; pub use limbs::DecimalParts; pub use limbs::LOWER_PART_DTYPE; pub use limbs::MAX_LOWER_PARTS; +pub use limbs::assemble_decimal; pub use limbs::assembled_values_type; pub use limbs::split_decimal; use prost::Message as _; @@ -54,7 +55,6 @@ use vortex_error::vortex_panic; use vortex_session::VortexSession; use vortex_session::registry::CachedId; -use crate::decimal_byte_parts::limbs::assemble_decimal; use crate::decimal_byte_parts::limbs::combine_i128; use crate::decimal_byte_parts::limbs::combine_i256; use crate::decimal_byte_parts::rules::PARENT_RULES; From 25a02ab92e23cb2d8adcc70334e90b971b5c7307 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 31 Jul 2026 22:52:56 +0000 Subject: [PATCH 03/14] Fix take with nullable indices, and reject parts wider than their precision MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two defects in the lower-parts support, both found by review of the preceding commits. `take` with a nullable indices array failed outright on any array carrying lower parts. Taking builds a `Dict`, and `Array::try_new` unions the codes' nullability into the values' dtype, so a non-nullable `u64` lower part came back as `u64?` — which `validate` rejects, because lower parts must be non-nullable with validity held by the MSP alone. The error propagated out of the kernel instead of falling back, so the whole scan failed with "lower part 0 must have dtype u64, got u64?". Arrays without lower parts were unaffected, so this arrived with the lower-parts work. The kernel now returns `Ok(None)` for nullable indices when lower parts are present, deferring to the canonical path, the same way `compare` already declines the MSP-only pushdown. Separately, nothing cross-checked the width the parts assemble into against the declared precision. `validate` bounded the part count and checked each part's dtype, and `assemble_decimal` dispatched purely on `(msp ptype, part count)`, so a file declaring `Decimal(38, 2)` with two lower parts deserialized happily, canonicalized to `i256` values of 39 digits, and then panicked in `Scalar::decimal`'s `vortex_expect` on scalar access. `validate` now requires the assembled type to be no wider than the precision needs, which rejects the crafted array at deserialization. The redundant `MAX_LOWER_PARTS` check goes away with it: `assembled_values_type` already performs it with the same message. The four one-line rejection tests become one `rstest` with the new over-wide case as a fifth, and `take` gains an `rstest` covering nullable indices against one and three lower parts, checked against the canonical take rather than just for absence of an error. Both new cases fail without their fix. Signed-off-by: "Joe Isaacs" --- .../src/decimal_byte_parts/compute/take.rs | 57 +++++++++++ .../src/decimal_byte_parts/mod.rs | 94 +++++++++---------- 2 files changed, 100 insertions(+), 51 deletions(-) diff --git a/encodings/decimal-byte-parts/src/decimal_byte_parts/compute/take.rs b/encodings/decimal-byte-parts/src/decimal_byte_parts/compute/take.rs index 1bfe3a7f2b6..4877e79eaf1 100644 --- a/encodings/decimal-byte-parts/src/decimal_byte_parts/compute/take.rs +++ b/encodings/decimal-byte-parts/src/decimal_byte_parts/compute/take.rs @@ -18,6 +18,13 @@ impl TakeExecute for DecimalByteParts { indices: &ArrayRef, _ctx: &mut ExecutionCtx, ) -> VortexResult> { + // Taking with nullable indices makes every taken part nullable, but lower parts must + // stay non-nullable `u64` — validity belongs to the MSP alone. Fall back to the + // canonical path rather than rebuilding parts we would have to strip nullability from. + if indices.dtype().is_nullable() && !array.lower_parts().is_empty() { + return Ok(None); + } + let lower_parts = array .lower_parts() .iter() @@ -35,3 +42,53 @@ impl TakeExecute for DecimalByteParts { .map(|a| Some(a.into_array())) } } + +#[cfg(test)] +mod tests { + use rstest::rstest; + use vortex_array::IntoArray; + use vortex_array::VortexSessionExecute; + use vortex_array::array_session; + use vortex_array::arrays::DecimalArray; + use vortex_array::arrays::PrimitiveArray; + use vortex_array::assert_arrays_eq; + use vortex_array::dtype::DecimalDType; + use vortex_array::validity::Validity; + use vortex_buffer::Buffer; + use vortex_error::VortexResult; + + use crate::decimal_byte_parts::testing::encode; + use crate::decimal_byte_parts::testing::i256_of; + + /// Taking with nullable indices must still round-trip the wide values, including the + /// null row, on arrays that carry lower parts. + #[rstest] + #[case::one_lower_part(DecimalArray::new( + Buffer::from(vec![1i128 << 70, 2, 3]), + DecimalDType::new(38, 2), + Validity::NonNullable, + ))] + #[case::three_lower_parts(DecimalArray::new( + Buffer::from(vec![i256_of(1, 1 << 70), i256_of(0, 2), i256_of(0, 3)]), + DecimalDType::new(76, 2), + Validity::NonNullable, + ))] + fn take_with_nullable_indices(#[case] decimal: DecimalArray) -> VortexResult<()> { + let session = array_session(); + crate::initialize(&session); + let mut ctx = session.create_execution_ctx(); + + let indices = PrimitiveArray::from_option_iter([Some(0u64), None, Some(2u64)]).into_array(); + let expected = decimal + .clone() + .into_array() + .take(indices.clone())? + .execute::(&mut ctx)?; + + let taken = encode(&decimal)?.into_array().take(indices)?; + let actual = taken.execute::(&mut ctx)?; + + assert_arrays_eq!(expected, actual, &mut ctx); + Ok(()) + } +} diff --git a/encodings/decimal-byte-parts/src/decimal_byte_parts/mod.rs b/encodings/decimal-byte-parts/src/decimal_byte_parts/mod.rs index 141e55cf7d9..1df4a02b8f2 100644 --- a/encodings/decimal-byte-parts/src/decimal_byte_parts/mod.rs +++ b/encodings/decimal-byte-parts/src/decimal_byte_parts/mod.rs @@ -285,10 +285,6 @@ impl DecimalBytePartsData { vortex_ensure!(msp.len() == len, "expected len {len}, got {}", msp.len()); let lower_part_count = lower_parts.len(); - vortex_ensure!( - lower_part_count <= MAX_LOWER_PARTS, - "at most {MAX_LOWER_PARTS} lower parts are supported, got {lower_part_count}" - ); for (idx, part) in lower_parts.enumerate() { vortex_ensure!( part.dtype() == &LOWER_PART_DTYPE, @@ -301,8 +297,20 @@ impl DecimalBytePartsData { part.len() ); } - // Rejects part combinations that cannot be reassembled into a decimal value. - assembled_values_type(msp.dtype().as_ptype(), lower_part_count)?; + // Rejects part combinations that cannot be reassembled into a decimal value. This also + // bounds the lower part count. + let values_type = assembled_values_type(msp.dtype().as_ptype(), lower_part_count)?; + + // The parts must not assemble into a wider value than the declared precision holds. + // Without this, a crafted array carrying more parts than its precision needs + // canonicalizes to out-of-precision values that then panic in the scalar path. + let widest = DecimalType::smallest_decimal_value_type(&decimal_dtype); + vortex_ensure!( + values_type <= widest, + "parts assemble into {values_type:?}, wider than the {widest:?} required by \ + decimal precision {}", + decimal_dtype.precision() + ); Ok(()) } } @@ -677,52 +685,26 @@ mod tests { buffer![1u64, 2, 3].into_array() } - #[test] - fn test_rejects_signed_lower_part() { - assert!( - DecimalByteParts::try_new_with_lower_parts( - msp(), - vec![buffer![1i64, 2, 3].into_array()], - DecimalDType::new(38, 2), - ) - .is_err() - ); - } - - #[test] - fn test_rejects_nullable_lower_part() { - let nullable = PrimitiveArray::new(buffer![1u64, 2, 3], Validity::AllValid).into_array(); - assert!( - DecimalByteParts::try_new_with_lower_parts( - msp(), - vec![nullable], - DecimalDType::new(38, 2), - ) - .is_err() - ); - } - - #[test] - fn test_rejects_mismatched_lower_part_length() { - assert!( - DecimalByteParts::try_new_with_lower_parts( - msp(), - vec![buffer![1u64, 2].into_array()], - DecimalDType::new(38, 2), - ) - .is_err() - ); - } - - #[test] - fn test_rejects_too_many_lower_parts() { + #[rstest] + #[case::signed_lower_part(vec![buffer![1i64, 2, 3].into_array()], DecimalDType::new(38, 2))] + #[case::nullable_lower_part( + vec![PrimitiveArray::new(buffer![1u64, 2, 3], Validity::AllValid).into_array()], + DecimalDType::new(38, 2) + )] + #[case::mismatched_length(vec![buffer![1u64, 2].into_array()], DecimalDType::new(38, 2))] + #[case::too_many_parts( + vec![lower_part(), lower_part(), lower_part(), lower_part()], + DecimalDType::new(76, 2) + )] + // Parts assembling into an i256 under a precision that only needs i128 would canonicalize + // to values outside the declared precision. + #[case::wider_than_precision(vec![lower_part(), lower_part()], DecimalDType::new(38, 2))] + fn test_rejects_invalid_parts( + #[case] lower_parts: Vec, + #[case] decimal_dtype: DecimalDType, + ) { assert!( - DecimalByteParts::try_new_with_lower_parts( - msp(), - vec![lower_part(), lower_part(), lower_part(), lower_part()], - DecimalDType::new(76, 2), - ) - .is_err() + DecimalByteParts::try_new_with_lower_parts(msp(), lower_parts, decimal_dtype).is_err() ); } @@ -759,6 +741,16 @@ mod tests { Ok(()) } + /// A crafted file may declare more lower parts than its precision needs. Assembling those + /// parts would produce values outside the declared precision, so it must be rejected at + /// deserialization rather than panicking later in the scalar path. + #[test] + fn test_deserialize_rejects_parts_wider_than_precision() { + let result = deserialize_with(2, vec![msp(), lower_part(), lower_part()]) + .and_then(Array::try_from_parts); + assert!(result.is_err(), "expected rejection, got {result:?}"); + } + #[test] fn test_deserialize_rejects_child_count_mismatch() { // Metadata claiming a lower part that was not serialized. From 86b5033a58fd29f8b19e44073ed6deb00cfef656 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 31 Jul 2026 23:09:49 +0000 Subject: [PATCH 04/14] Store i128 assembly into a pre-sized buffer, and dedupe part-wise kernels MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Re-running `benches/decimal_assemble.rs` after the review corrected a claim the previous commit made. Specializing the part count is worth 1.85x on `i256`, as reported, but on `i128` it is worth only ~1.04x — the 1.25x figure did not reproduce. What actually costs on `i128` is the write: pushing into a reserved buffer instead of storing into a pre-sized one is the whole difference at 16 bytes per row. A new `i128_row_write` variant isolates it, holding the loop shape fixed and changing only the output buffer. Over 65,536 rows, `fastest` of three runs each: | shape | i128 | i256 | | ---------------------------- | ------- | ------- | | row, runtime part count | 143 µs | 351 µs | | row, const part count, push | 138 µs | 190 µs | | row, const part count, write | 83 µs | 196 µs | | column, lane writes | 103 µs | 438 µs | So the columnar shape was never the interesting axis: it beats the *pushing* row loop on `i128` but still loses to the single-pass write, and the second pass buys nothing once the push is gone. On `i256` the write shape ties the push shape, because 32 bytes of stores per row dominate either way, so only `assemble_i128` changes. Through the array API, one lower part goes 138 µs -> 83 µs (1.6x); three parts is unchanged at ~209 µs. The rest is cleanup from the same review. Seven kernels open-coded "map every part, rebuild the array", and two of them had already been fixed in this branch for dropping the lower parts on the floor. `map_parts`, `with_msp` and `decimal_dtype` replace all seven, so a part-wise op cannot silently lose a part, and the argument for why an MSP-only rebuild is sound lives in one doc comment instead of being restated or omitted per site. Dead code: `DecimalBytePartsDataParts` had exactly one reference in the repository — its own definition — and this branch had been growing it a field and doc comments. The `[first]` arm of the `i256` dispatch is unreachable, since one lower part under a <=64-bit MSP always lands in an `i128`; a comment now says so where the arm was. Visibility: `assemble_decimal`, `assembled_values_type` and `LOWER_PART_DTYPE` had no callers outside the crate and are now crate-private. `assemble_decimal` was public only so the benchmark could call it, but `canonicalize_byte_parts` already measures the same assembly through the array API, so the two `*_assemble_shipped` benches go with it. As public API it could also panic rather than error on an unsigned MSP, since signedness is only checked on the zero-parts path. The metadata accessor `lower_parts()` returned a count while the generated slots accessor of the same name returns the arrays, both in scope in the same module; it is now `lower_part_count()`. The btrblocks scheme spelled the child layout as `1 + MAX_LOWER_PARTS` and `idx + 1` where the encoding crate has named slot constants; it now uses them. Three hand-rolled LCGs become `StdRng::seed_from_u64`, matching the rest of the repo. Four one-line rejection tests became one `rstest` in the previous commit; the two removed columnar bench variants are recorded in the module doc with their numbers rather than kept as dead code. Signed-off-by: "Joe Isaacs" --- Cargo.lock | 2 + encodings/decimal-byte-parts/Cargo.toml | 1 + .../benches/decimal_assemble.rs | 152 ++++++++---------- .../src/decimal_byte_parts/compute/cast.rs | 10 +- .../src/decimal_byte_parts/compute/filter.rs | 20 +-- .../src/decimal_byte_parts/compute/mask.rs | 15 +- .../src/decimal_byte_parts/compute/take.rs | 18 +-- .../src/decimal_byte_parts/limbs.rs | 25 ++- .../src/decimal_byte_parts/mod.rs | 78 ++++++--- .../src/decimal_byte_parts/rules.rs | 21 +-- .../src/decimal_byte_parts/slice.rs | 21 +-- vortex-btrblocks/src/schemes/decimal/mod.rs | 19 ++- vortex-btrblocks/src/schemes/decimal/tests.rs | 12 +- vortex-file/Cargo.toml | 1 + vortex-file/src/tests.rs | 12 +- 15 files changed, 182 insertions(+), 225 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 9480c430f3f..7353d7a6a72 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -9689,6 +9689,7 @@ dependencies = [ "codspeed-divan-compat", "num-traits", "prost 0.14.4", + "rand 0.10.2", "rstest", "vortex-array", "vortex-buffer", @@ -9814,6 +9815,7 @@ dependencies = [ "oneshot", "parking_lot", "pin-project-lite", + "rand 0.10.2", "rstest", "tokio", "tracing", diff --git a/encodings/decimal-byte-parts/Cargo.toml b/encodings/decimal-byte-parts/Cargo.toml index 04610a83763..ad594db5725 100644 --- a/encodings/decimal-byte-parts/Cargo.toml +++ b/encodings/decimal-byte-parts/Cargo.toml @@ -27,6 +27,7 @@ vortex-session = { workspace = true } [dev-dependencies] divan = { workspace = true } +rand = { workspace = true } rstest = { workspace = true } vortex-array = { path = "../../vortex-array", features = ["_test-harness"] } diff --git a/encodings/decimal-byte-parts/benches/decimal_assemble.rs b/encodings/decimal-byte-parts/benches/decimal_assemble.rs index 29fd887a888..e5e97a81b66 100644 --- a/encodings/decimal-byte-parts/benches/decimal_assemble.rs +++ b/encodings/decimal-byte-parts/benches/decimal_assemble.rs @@ -8,25 +8,42 @@ //! obvious shapes are: //! //! - **row**: one pass, gathering the row's word from each part and combining. Sub-variants -//! differ only in whether the part count is known to the compiler. -//! - **column**: one pass per part over the whole output, either accumulating into the wide -//! values or writing 64-bit lanes directly. +//! differ in whether the part count is known to the compiler (`_const` vs `_dynamic`) and +//! in whether the output is pushed into a reserved buffer or stored into a pre-sized one +//! (`_write`). +//! - **column**: one pass per part over the whole output, writing 64-bit lanes directly. //! //! Every candidate is spelled out here rather than called through the crate, so the same -//! comparison can be run from any revision. `assemble_*_shipped` calls the public API and -//! pins whichever shape the crate currently uses. +//! comparison can be run from any revision. `canonicalize_byte_parts` goes through the array +//! API instead, and so tracks whichever shape the crate currently ships. //! -//! At 65,536 rows the row shape wins, and what costs is a part count the compiler cannot -//! see, not the row-at-a-time access: specializing it is 1.25x (`i128`) and 1.8x (`i256`). -//! Columnar loses for `i256` because each lane store is strided by 32 bytes — 2.2x slower -//! than the specialized row loop, still 1.6x when cache blocked, and 11x when expressed as -//! whole-value shifts. For `i128` the two-pass column shape ties the specialized row loop: -//! at 16 bytes per row both are memory bound. +//! At 65,536 rows the row shape wins, but for two different reasons per width, and neither +//! is the row-at-a-time access itself: +//! +//! - `i256` is dominated by the part count being invisible to the compiler. Specializing it +//! is 1.85x. How the output is written barely matters (`_write` ties `_const`), because at +//! 32 bytes per row the stores dominate either way. +//! - `i128` is dominated by the write. Specializing the part count is worth only ~1.04x, +//! while storing into a pre-sized buffer instead of pushing is 1.6x — the bounds-checked +//! `push` is the whole cost at 16 bytes per row. +//! +//! Columnar always loses. For `i256` each lane store is strided by 32 bytes, 2.3x slower than +//! the specialized row loop. For `i128` the two-pass column shape beats the *pushing* row +//! loop but still loses to the single-pass `_write` row loop, so the two passes buy nothing +//! once the push is gone. +//! +//! Two further `i256` columnar variants were measured and then removed rather than left here +//! to rot: cache blocking the lane passes over 1024-row blocks recovered part of the strided +//! stores but was still 1.6x slower than the row loop, and expressing the passes as +//! whole-value `i256` shifts was 11x slower. #![allow(clippy::unwrap_used, clippy::cast_possible_truncation)] use divan::Bencher; use divan::black_box; +use rand::RngExt; +use rand::SeedableRng; +use rand::rngs::StdRng; use vortex_array::IntoArray; use vortex_array::arrays::PrimitiveArray; use vortex_array::dtype::DecimalDType; @@ -35,7 +52,6 @@ use vortex_array::validity::Validity; use vortex_buffer::Alignment; use vortex_buffer::Buffer; use vortex_buffer::BufferMut; -use vortex_decimal_byte_parts::assemble_decimal; fn main() { divan::main(); @@ -45,23 +61,12 @@ fn main() { /// in L2, so the extra passes of a columnar shape are paid at their real cost. const LEN: usize = 65_536; -/// Rows per block in the cache-blocked columnar variant: the block's output (32 KiB of -/// `i256`) stays in L1 across all four lane passes. -const BLOCK: usize = 1024; - const WORD_BITS: usize = 64; /// Deterministic pseudo-random words, so no part is constant or a sequence. fn words(seed: u64, len: usize) -> Buffer { - let mut state = seed; - (0..len) - .map(|_| { - state = state - .wrapping_mul(6_364_136_223_846_793_005) - .wrapping_add(1_442_695_040_888_963_407); - state - }) - .collect() + let mut rng = StdRng::seed_from_u64(seed); + (0..len).map(|_| rng.random()).collect() } fn msp(seed: u64, len: usize) -> Buffer { @@ -109,6 +114,16 @@ fn i128_column(msp: &[i64], lower: &[u64]) -> Buffer { out.freeze() } +/// The row shape writing into a pre-sized buffer rather than pushing into a reserved one, +/// to separate "one pass vs two" from "bounds-checked push vs direct store". +fn i128_row_write(msp: &[i64], lower: &[u64]) -> Buffer { + let mut out = BufferMut::::zeroed(msp.len()); + for ((o, m), l) in out.as_mut_slice().iter_mut().zip(msp).zip(lower) { + *o = (i128::from(*m) << WORD_BITS) | i128::from(*l); + } + out.freeze() +} + // --------------------------------------------------------------------------------------- // i256: three lower parts // --------------------------------------------------------------------------------------- @@ -145,21 +160,6 @@ fn i256_row_const(msp: &[i64], lower: [&[u64]; 3]) -> Buffer { out.freeze() } -/// The column shape as wide arithmetic: one pass per part, each shifting the whole output -/// left and ORing the part in. Four read-modify-write passes over 32 bytes per row. -fn i256_column_accumulate(msp: &[i64], lower: [&[u64]; 3]) -> Buffer { - let mut out = BufferMut::::zeroed(msp.len()); - for (o, m) in out.as_mut_slice().iter_mut().zip(msp) { - *o = i256::from_i128(i128::from(*m)); - } - for part in lower { - for (o, w) in out.as_mut_slice().iter_mut().zip(part) { - *o = (*o << WORD_BITS) | i256::from_parts(u128::from(*w), 0); - } - } - out.freeze() -} - /// The column shape as lane writes: build the output as 64-bit words and write one lane per /// pass. Avoids re-reading the output, but every store is strided by 32 bytes. fn i256_column_lanes(msp: &[i64], lower: [&[u64]; 3]) -> Buffer { @@ -179,25 +179,21 @@ fn i256_column_lanes(msp: &[i64], lower: [&[u64]; 3]) -> Buffer { Buffer::::from_byte_buffer_aligned(w.freeze().into_byte_buffer(), Alignment::of::()) } -/// The column shape, cache blocked: the lane passes run over one block of rows at a time so -/// the block's output stays resident between passes. -fn i256_column_lanes_blocked(msp: &[i64], lower: [&[u64]; 3]) -> Buffer { - let len = msp.len(); - let mut w = BufferMut::::zeroed_aligned(len * 4, Alignment::of::()); - let lanes = w.as_mut_slice(); - for start in (0..len).step_by(BLOCK) { - let end = (start + BLOCK).min(len); - for i in start..end { - lanes[i * 4 + 3] = msp[i].cast_unsigned(); - } - for (lane, part) in lower.iter().enumerate() { - for i in start..end { - lanes[i * 4 + 2 - lane] = part[i]; - } +/// The specialized row shape for `i256`, writing into a pre-sized buffer. +fn i256_row_write(msp: &[i64], lower: [&[u64]; K]) -> Buffer { + let mut out = BufferMut::::zeroed(msp.len()); + for (row, (o, m)) in out.as_mut_slice().iter_mut().zip(msp).enumerate() { + let mut words = [if *m < 0 { u64::MAX } else { 0 }; 4]; + for (i, part) in lower.iter().enumerate() { + words[K - 1 - i] = part[row]; } + words[K] = m.cast_unsigned(); + *o = i256::from_parts( + u128::from(words[0]) | (u128::from(words[1]) << WORD_BITS), + (u128::from(words[2]) | (u128::from(words[3]) << WORD_BITS)).cast_signed(), + ); } - assert!(cfg!(target_endian = "little")); - Buffer::::from_byte_buffer_aligned(w.freeze().into_byte_buffer(), Alignment::of::()) + out.freeze() } // --------------------------------------------------------------------------------------- @@ -254,26 +250,25 @@ fn i128_column_parts(bencher: Bencher) { } #[divan::bench] -fn i256_row_dynamic_parts(bencher: Bencher) { - let parts = Parts::new(3); +fn i128_row_write_parts(bencher: Bencher) { + let parts = Parts::new(1); let lower = parts.lower_slices(); - bencher.bench(|| i256_row_dynamic(black_box(parts.msp.as_slice()), black_box(&lower))); + bencher.bench(|| i128_row_write(black_box(parts.msp.as_slice()), black_box(lower[0]))); } #[divan::bench] -fn i256_row_const_parts(bencher: Bencher) { +fn i256_row_dynamic_parts(bencher: Bencher) { let parts = Parts::new(3); let lower = parts.lower_slices(); - let lower = [lower[0], lower[1], lower[2]]; - bencher.bench(|| i256_row_const(black_box(parts.msp.as_slice()), black_box(lower))); + bencher.bench(|| i256_row_dynamic(black_box(parts.msp.as_slice()), black_box(&lower))); } #[divan::bench] -fn i256_column_accumulate_parts(bencher: Bencher) { +fn i256_row_const_parts(bencher: Bencher) { let parts = Parts::new(3); let lower = parts.lower_slices(); let lower = [lower[0], lower[1], lower[2]]; - bencher.bench(|| i256_column_accumulate(black_box(parts.msp.as_slice()), black_box(lower))); + bencher.bench(|| i256_row_const(black_box(parts.msp.as_slice()), black_box(lower))); } #[divan::bench] @@ -284,35 +279,16 @@ fn i256_column_lanes_parts(bencher: Bencher) { bencher.bench(|| i256_column_lanes(black_box(parts.msp.as_slice()), black_box(lower))); } +/// Canonicalizing through the public array API, so the child execution and validity handling +/// around the assembly loop are included. #[divan::bench] -fn i256_column_lanes_blocked_parts(bencher: Bencher) { +fn i256_row_write_parts(bencher: Bencher) { let parts = Parts::new(3); let lower = parts.lower_slices(); let lower = [lower[0], lower[1], lower[2]]; - bencher.bench(|| i256_column_lanes_blocked(black_box(parts.msp.as_slice()), black_box(lower))); + bencher.bench(|| i256_row_write(black_box(parts.msp.as_slice()), black_box(lower))); } -/// The shape the crate actually ships, including the buffer allocation and the ptype -/// dispatch, for one lower part. -#[divan::bench] -fn i128_assemble_shipped(bencher: Bencher) { - let parts = Parts::new(1); - let (msp, lower) = parts.arrays(); - let dtype = DecimalDType::new(38, 2); - bencher.bench(|| assemble_decimal(black_box(&msp), black_box(&lower), dtype).unwrap()); -} - -/// The shape the crate actually ships, for three lower parts. -#[divan::bench] -fn i256_assemble_shipped(bencher: Bencher) { - let parts = Parts::new(3); - let (msp, lower) = parts.arrays(); - let dtype = DecimalDType::new(76, 2); - bencher.bench(|| assemble_decimal(black_box(&msp), black_box(&lower), dtype).unwrap()); -} - -/// Canonicalizing through the public array API, so the child execution and validity handling -/// around the assembly loop are included. #[divan::bench(args = [1, 3])] fn canonicalize_byte_parts(bencher: Bencher, lower_parts: usize) { use vortex_array::VortexSessionExecute; diff --git a/encodings/decimal-byte-parts/src/decimal_byte_parts/compute/cast.rs b/encodings/decimal-byte-parts/src/decimal_byte_parts/compute/cast.rs index e4abd7baa7b..7b949fcd695 100644 --- a/encodings/decimal-byte-parts/src/decimal_byte_parts/compute/cast.rs +++ b/encodings/decimal-byte-parts/src/decimal_byte_parts/compute/cast.rs @@ -11,6 +11,7 @@ use vortex_error::VortexResult; use crate::DecimalByteParts; use crate::decimal_byte_parts::DecimalBytePartsArraySlotsExt; +use crate::decimal_byte_parts::with_msp; impl CastReduce for DecimalByteParts { fn cast(array: ArrayView<'_, Self>, dtype: &DType) -> VortexResult> { @@ -29,14 +30,7 @@ impl CastReduce for DecimalByteParts { .msp() .cast(array.msp().dtype().with_nullability(*target_nullability))?; - Ok(Some( - DecimalByteParts::try_new_with_lower_parts( - new_msp, - array.lower_parts().to_vec(), - *target_decimal, - )? - .into_array(), - )) + with_msp(array, new_msp, *target_decimal).map(|a| Some(a.into_array())) } } diff --git a/encodings/decimal-byte-parts/src/decimal_byte_parts/compute/filter.rs b/encodings/decimal-byte-parts/src/decimal_byte_parts/compute/filter.rs index e6aea7dd9c8..e4fb03a5ca0 100644 --- a/encodings/decimal-byte-parts/src/decimal_byte_parts/compute/filter.rs +++ b/encodings/decimal-byte-parts/src/decimal_byte_parts/compute/filter.rs @@ -5,29 +5,15 @@ use vortex_array::ArrayRef; use vortex_array::ArrayView; use vortex_array::IntoArray; use vortex_array::arrays::filter::FilterReduce; -use vortex_error::VortexExpect; use vortex_error::VortexResult; use vortex_mask::Mask; use crate::DecimalByteParts; -use crate::decimal_byte_parts::DecimalBytePartsArraySlotsExt; +use crate::decimal_byte_parts::map_parts; + impl FilterReduce for DecimalByteParts { fn filter(array: ArrayView<'_, Self>, mask: &Mask) -> VortexResult> { - let lower_parts = array - .lower_parts() - .iter() - .map(|part| part.filter(mask.clone())) - .collect::>>()?; - - DecimalByteParts::try_new_with_lower_parts( - array.msp().filter(mask.clone())?, - lower_parts, - *array - .dtype() - .as_decimal_opt() - .vortex_expect("must be a decimal dtype"), - ) - .map(|d| Some(d.into_array())) + map_parts(array, |part| part.filter(mask.clone())).map(|d| Some(d.into_array())) } } diff --git a/encodings/decimal-byte-parts/src/decimal_byte_parts/compute/mask.rs b/encodings/decimal-byte-parts/src/decimal_byte_parts/compute/mask.rs index 662af162e74..665b0f26ae2 100644 --- a/encodings/decimal-byte-parts/src/decimal_byte_parts/compute/mask.rs +++ b/encodings/decimal-byte-parts/src/decimal_byte_parts/compute/mask.rs @@ -8,11 +8,12 @@ use vortex_array::arrays::scalar_fn::ScalarFnFactoryExt; use vortex_array::scalar_fn::EmptyOptions; use vortex_array::scalar_fn::fns::mask::Mask as MaskExpr; use vortex_array::scalar_fn::fns::mask::MaskReduce; -use vortex_error::VortexExpect; use vortex_error::VortexResult; use crate::DecimalByteParts; use crate::decimal_byte_parts::DecimalBytePartsArraySlotsExt; +use crate::decimal_byte_parts::decimal_dtype; +use crate::decimal_byte_parts::with_msp; impl MaskReduce for DecimalByteParts { fn mask(array: ArrayView<'_, Self>, mask: &ArrayRef) -> VortexResult> { @@ -23,16 +24,6 @@ impl MaskReduce for DecimalByteParts { EmptyOptions, [array.msp().clone(), mask.clone()], )?; - Ok(Some( - DecimalByteParts::try_new_with_lower_parts( - masked_msp, - array.lower_parts().to_vec(), - *array - .dtype() - .as_decimal_opt() - .vortex_expect("must be a decimal dtype"), - )? - .into_array(), - )) + with_msp(array, masked_msp, decimal_dtype(array)).map(|a| Some(a.into_array())) } } diff --git a/encodings/decimal-byte-parts/src/decimal_byte_parts/compute/take.rs b/encodings/decimal-byte-parts/src/decimal_byte_parts/compute/take.rs index 4877e79eaf1..5665813fbe4 100644 --- a/encodings/decimal-byte-parts/src/decimal_byte_parts/compute/take.rs +++ b/encodings/decimal-byte-parts/src/decimal_byte_parts/compute/take.rs @@ -6,11 +6,11 @@ use vortex_array::ArrayView; use vortex_array::ExecutionCtx; use vortex_array::IntoArray; use vortex_array::arrays::dict::TakeExecute; -use vortex_error::VortexExpect; use vortex_error::VortexResult; use crate::DecimalByteParts; use crate::decimal_byte_parts::DecimalBytePartsArraySlotsExt; +use crate::decimal_byte_parts::map_parts; impl TakeExecute for DecimalByteParts { fn take( @@ -25,21 +25,7 @@ impl TakeExecute for DecimalByteParts { return Ok(None); } - let lower_parts = array - .lower_parts() - .iter() - .map(|part| part.take(indices.clone())) - .collect::>>()?; - - DecimalByteParts::try_new_with_lower_parts( - array.msp().take(indices.clone())?, - lower_parts, - *array - .dtype() - .as_decimal_opt() - .vortex_expect("must be a decimal dtype"), - ) - .map(|a| Some(a.into_array())) + map_parts(array, |part| part.take(indices.clone())).map(|a| Some(a.into_array())) } } diff --git a/encodings/decimal-byte-parts/src/decimal_byte_parts/limbs.rs b/encodings/decimal-byte-parts/src/decimal_byte_parts/limbs.rs index c5d20933aee..adaa7555c9a 100644 --- a/encodings/decimal-byte-parts/src/decimal_byte_parts/limbs.rs +++ b/encodings/decimal-byte-parts/src/decimal_byte_parts/limbs.rs @@ -46,7 +46,7 @@ const LOWER_PART_BITS: usize = 64; /// The dtype every lower part must have: a non-nullable `u64`. /// /// Validity is carried by the most significant part alone. -pub const LOWER_PART_DTYPE: DType = DType::Primitive(PType::U64, Nullability::NonNullable); +pub(crate) const LOWER_PART_DTYPE: DType = DType::Primitive(PType::U64, Nullability::NonNullable); /// A decimal array decomposed into byte parts. pub struct DecimalParts { @@ -62,7 +62,7 @@ pub struct DecimalParts { /// /// Returns an error if `msp_ptype` is not a signed integer, or if there are more than /// [`MAX_LOWER_PARTS`] lower parts. -pub fn assembled_values_type( +pub(crate) fn assembled_values_type( msp_ptype: PType, lower_part_count: usize, ) -> VortexResult { @@ -117,7 +117,7 @@ pub fn split_decimal(decimal: &DecimalArray) -> VortexResult { /// /// Returns an error if the parts do not describe a valid decimal, or if the MSP's validity /// cannot be derived. -pub fn assemble_decimal( +pub(crate) fn assemble_decimal( msp: &PrimitiveArray, lower_parts: &[PrimitiveArray], decimal_dtype: DecimalDType, @@ -150,8 +150,9 @@ pub fn assemble_decimal( // The part count is dispatched to a constant so every 64-bit word lands at a compile-time // index. Leaving it dynamic costs 1.8x on the `i256` path — see `benches/decimal_assemble.rs`. let values = match assembled_values_type(msp.ptype(), lower.len())? { + // A single lower part can never widen to an `i256`: the MSP is at most 64 bits, so + // 64 + 64 fits an `i128` and takes the branch below. DecimalType::I256 => match lower.as_slice() { - [first] => assemble_i256(msp, [first]), [first, second] => assemble_i256(msp, [first, second]), [first, second, third] => assemble_i256(msp, [first, second, third]), _ => vortex_bail!("unsupported lower part count {}", lower.len()), @@ -260,10 +261,20 @@ fn split_i256(values: &Buffer) -> (Buffer, [Buffer; MAX_LOWER_PA reason = "the widening to i64 is a no-op only for the i64 arm of the ptype match" )] fn assemble_i128(msp: &PrimitiveArray, lower: &[u64]) -> Buffer { - let mut out = BufferMut::::with_capacity(msp.len()); + // Store into a pre-sized buffer rather than pushing into a reserved one: at 16 bytes per + // row the bounds-checked `push` dominates, and dropping it is 1.6x — see + // `i128_row_write` against `i128_row_const` in `benches/decimal_assemble.rs`. The same + // shape does not pay off for `i256`, where zeroing 32 bytes per row costs more than the + // push it saves. + let mut out = BufferMut::::zeroed(msp.len()); match_each_signed_integer_ptype!(msp.ptype(), |P| { - for (value, part) in msp.as_slice::

().iter().zip(lower) { - out.push((i128::from(i64::from(*value)) << LOWER_PART_BITS) | i128::from(*part)); + for ((slot, value), part) in out + .as_mut_slice() + .iter_mut() + .zip(msp.as_slice::

()) + .zip(lower) + { + *slot = (i128::from(i64::from(*value)) << LOWER_PART_BITS) | i128::from(*part); } }); out.freeze() diff --git a/encodings/decimal-byte-parts/src/decimal_byte_parts/mod.rs b/encodings/decimal-byte-parts/src/decimal_byte_parts/mod.rs index 1df4a02b8f2..69c51d14447 100644 --- a/encodings/decimal-byte-parts/src/decimal_byte_parts/mod.rs +++ b/encodings/decimal-byte-parts/src/decimal_byte_parts/mod.rs @@ -16,10 +16,7 @@ mod slice; pub(crate) mod testing; pub use limbs::DecimalParts; -pub use limbs::LOWER_PART_DTYPE; pub use limbs::MAX_LOWER_PARTS; -pub use limbs::assemble_decimal; -pub use limbs::assembled_values_type; pub use limbs::split_decimal; use prost::Message as _; use vortex_array::ArrayEq; @@ -55,6 +52,9 @@ use vortex_error::vortex_panic; use vortex_session::VortexSession; use vortex_session::registry::CachedId; +use crate::decimal_byte_parts::limbs::LOWER_PART_DTYPE; +use crate::decimal_byte_parts::limbs::assemble_decimal; +use crate::decimal_byte_parts::limbs::assembled_values_type; use crate::decimal_byte_parts::limbs::combine_i128; use crate::decimal_byte_parts::limbs::combine_i256; use crate::decimal_byte_parts::rules::PARENT_RULES; @@ -86,7 +86,7 @@ impl DecimalBytesPartsMetadata { /// # Errors /// /// Returns an error if the count exceeds [`MAX_LOWER_PARTS`]. - fn lower_parts(&self) -> VortexResult { + fn lower_part_count(&self) -> VortexResult { let count = usize::try_from(self.lower_part_count) .map_err(|_| vortex_err!("lower part count {} out of range", self.lower_part_count))?; vortex_ensure!( @@ -180,7 +180,7 @@ impl VTable for DecimalByteParts { let encoded_dtype = DType::Primitive(metadata.zeroth_child_ptype(), dtype.nullability()); - let lower_part_count = metadata.lower_parts()?; + let lower_part_count = metadata.lower_part_count()?; vortex_ensure!( children.len() == DecimalBytePartsSlots::FIXED_COUNT + lower_part_count, "expected {} children, got {}", @@ -250,14 +250,6 @@ impl Display for DecimalBytePartsData { } } -/// The parts of a [`DecimalBytePartsArray`]. -pub struct DecimalBytePartsDataParts { - /// The most significant part, carrying the array's validity. - pub msp: ArrayRef, - /// The remaining 64-bit windows, most significant first. - pub lower_parts: Vec, -} - impl DecimalBytePartsData { /// Validate the parts of a [`DecimalBytePartsArray`]. /// @@ -361,6 +353,48 @@ fn values_type(array: ArrayView<'_, DecimalByteParts>) -> VortexResult) -> DecimalDType { + *array + .dtype() + .as_decimal_opt() + .vortex_expect("must be a decimal dtype") +} + +/// Rebuild the array by applying `f` to the MSP and to every lower part, in slot order. +/// +/// Part-wise operations must touch every part. Going through this rather than calling +/// [`DecimalByteParts::try_new_with_lower_parts`] directly makes dropping a lower part — +/// which silently corrupts wide values — unrepresentable. +pub(crate) fn map_parts( + array: ArrayView<'_, DecimalByteParts>, + mut f: impl FnMut(&ArrayRef) -> VortexResult, +) -> VortexResult { + let msp = f(array.msp())?; + let lower_parts = array + .lower_parts() + .iter() + .map(&mut f) + .collect::>>()?; + DecimalByteParts::try_new_with_lower_parts(msp, lower_parts, decimal_dtype(array)) +} + +/// Rebuild the array with a replacement MSP, keeping its lower parts untouched. +/// +/// Only valid for operations that cannot change a row's magnitude bits — a nullability cast +/// or a mask — since the lower parts keep whatever bits they held. That is sound because +/// validity lives in the MSP alone, so lower-part bits in a null row are already undefined. +pub(crate) fn with_msp( + array: ArrayView<'_, DecimalByteParts>, + msp: ArrayRef, + decimal_dtype: DecimalDType, +) -> VortexResult { + DecimalByteParts::try_new_with_lower_parts(msp, array.lower_parts().to_vec(), decimal_dtype) +} + /// Converts a DecimalBytePartsArray to its canonical DecimalArray representation. fn to_canonical_decimal( array: &DecimalBytePartsArray, @@ -373,12 +407,7 @@ fn to_canonical_decimal( .map(|part| part.clone().execute::(ctx)) .collect::>>()?; - let decimal_dtype = *array - .dtype() - .as_decimal_opt() - .vortex_expect("must be a decimal dtype"); - - Ok(assemble_decimal(&msp, &lower_parts, decimal_dtype)?.into_array()) + Ok(assemble_decimal(&msp, &lower_parts, decimal_dtype(array.as_view()))?.into_array()) } impl OperationsVTable for DecimalByteParts { @@ -405,10 +434,13 @@ impl OperationsVTable for DecimalByteParts { }) .collect::>>()?; - let value = match values_type(array)? { - _ if lower_parts.is_empty() => DecimalValue::I64(msp), - DecimalType::I256 => DecimalValue::I256(combine_i256(msp, lower_parts.into_iter())), - _ => DecimalValue::I128(combine_i128(msp, lower_parts)), + let value = if lower_parts.is_empty() { + DecimalValue::I64(msp) + } else { + match values_type(array)? { + DecimalType::I256 => DecimalValue::I256(combine_i256(msp, lower_parts.into_iter())), + _ => DecimalValue::I128(combine_i128(msp, lower_parts)), + } }; Scalar::try_new(array.dtype().clone(), Some(ScalarValue::Decimal(value))) diff --git a/encodings/decimal-byte-parts/src/decimal_byte_parts/rules.rs b/encodings/decimal-byte-parts/src/decimal_byte_parts/rules.rs index 46cd3d794a8..572e031bacb 100644 --- a/encodings/decimal-byte-parts/src/decimal_byte_parts/rules.rs +++ b/encodings/decimal-byte-parts/src/decimal_byte_parts/rules.rs @@ -11,11 +11,10 @@ use vortex_array::optimizer::rules::ArrayParentReduceRule; use vortex_array::optimizer::rules::ParentRuleSet; use vortex_array::scalar_fn::fns::cast::CastReduceAdaptor; use vortex_array::scalar_fn::fns::mask::MaskReduceAdaptor; -use vortex_error::VortexExpect; use vortex_error::VortexResult; use crate::DecimalByteParts; -use crate::decimal_byte_parts::DecimalBytePartsArraySlotsExt; +use crate::decimal_byte_parts::map_parts; pub(super) const PARENT_RULES: ParentRuleSet = ParentRuleSet::new(&[ ParentRuleSet::lift(&DecimalBytePartsFilterPushDownRule), @@ -39,21 +38,7 @@ impl ArrayParentReduceRule for DecimalBytePartsFilterPushDownR ) -> VortexResult> { // TODO(ngates): we should benchmark whether to push-down filters with "lower parts", // which filters each part separately rather than the canonical wide buffer once. - let new_msp = child.msp().filter(parent.filter_mask().clone())?; - let new_lower_parts = child - .lower_parts() - .iter() - .map(|part| part.filter(parent.filter_mask().clone())) - .collect::>>()?; - let new_child = DecimalByteParts::try_new_with_lower_parts( - new_msp, - new_lower_parts, - *child - .dtype() - .as_decimal_opt() - .vortex_expect("must be a decimal dtype"), - )? - .into_array(); - Ok(Some(new_child)) + map_parts(child, |part| part.filter(parent.filter_mask().clone())) + .map(|c| Some(c.into_array())) } } diff --git a/encodings/decimal-byte-parts/src/decimal_byte_parts/slice.rs b/encodings/decimal-byte-parts/src/decimal_byte_parts/slice.rs index c5b6c549160..e31f717d389 100644 --- a/encodings/decimal-byte-parts/src/decimal_byte_parts/slice.rs +++ b/encodings/decimal-byte-parts/src/decimal_byte_parts/slice.rs @@ -7,30 +7,13 @@ use vortex_array::ArrayRef; use vortex_array::ArrayView; use vortex_array::IntoArray; use vortex_array::arrays::slice::SliceReduce; -use vortex_error::VortexExpect; use vortex_error::VortexResult; use crate::DecimalByteParts; -use crate::decimal_byte_parts::DecimalBytePartsArraySlotsExt; +use crate::decimal_byte_parts::map_parts; impl SliceReduce for DecimalByteParts { fn slice(array: ArrayView<'_, Self>, range: Range) -> VortexResult> { - let lower_parts = array - .lower_parts() - .iter() - .map(|part| part.slice(range.clone())) - .collect::>>()?; - - Ok(Some( - DecimalByteParts::try_new_with_lower_parts( - array.msp().slice(range)?, - lower_parts, - *array - .dtype() - .as_decimal_opt() - .vortex_expect("must be a decimal dtype"), - )? - .into_array(), - )) + map_parts(array, |part| part.slice(range.clone())).map(|d| Some(d.into_array())) } } diff --git a/vortex-btrblocks/src/schemes/decimal/mod.rs b/vortex-btrblocks/src/schemes/decimal/mod.rs index 47dc3050cd5..cbe32fc7b45 100644 --- a/vortex-btrblocks/src/schemes/decimal/mod.rs +++ b/vortex-btrblocks/src/schemes/decimal/mod.rs @@ -14,6 +14,7 @@ use vortex_array::arrays::decimal::narrowed_decimal; use vortex_compressor::scheme::CompressionEstimate; use vortex_compressor::scheme::EstimateVerdict; use vortex_decimal_byte_parts::DecimalByteParts; +use vortex_decimal_byte_parts::DecimalBytePartsSlots; use vortex_decimal_byte_parts::MAX_LOWER_PARTS; use vortex_decimal_byte_parts::split_decimal; use vortex_error::VortexResult; @@ -50,7 +51,7 @@ impl Scheme for DecimalScheme { /// Children: msp=0, lower parts=1..=3. fn num_children(&self) -> usize { - 1 + MAX_LOWER_PARTS + DecimalBytePartsSlots::FIXED_COUNT + MAX_LOWER_PARTS } fn expected_compression_ratio( @@ -74,13 +75,25 @@ impl Scheme for DecimalScheme { let decimal = narrowed_decimal(decimal); let parts = split_decimal(&decimal)?; - let msp = compressor.compress_child(&parts.msp, &compress_ctx, self.id(), 0, exec_ctx)?; + let msp = compressor.compress_child( + &parts.msp, + &compress_ctx, + self.id(), + DecimalBytePartsSlots::MSP, + exec_ctx, + )?; let lower_parts = parts .lower_parts .iter() .enumerate() .map(|(idx, part)| { - compressor.compress_child(part, &compress_ctx, self.id(), idx + 1, exec_ctx) + compressor.compress_child( + part, + &compress_ctx, + self.id(), + DecimalBytePartsSlots::LOWER_PARTS_OFFSET + idx, + exec_ctx, + ) }) .collect::>>()?; diff --git a/vortex-btrblocks/src/schemes/decimal/tests.rs b/vortex-btrblocks/src/schemes/decimal/tests.rs index 596c2900c07..d1a29ba970e 100644 --- a/vortex-btrblocks/src/schemes/decimal/tests.rs +++ b/vortex-btrblocks/src/schemes/decimal/tests.rs @@ -4,6 +4,9 @@ use std::iter; use std::sync::LazyLock; +use rand::RngExt; +use rand::SeedableRng as _; +use rand::rngs::StdRng; use rstest::rstest; use vortex_array::ArrayRef; use vortex_array::IntoArray; @@ -37,13 +40,8 @@ fn ten_pow(exp: u32) -> i256 { /// Deterministic 24-bit noise, so the low part of each value is neither constant nor a /// sequence — the realistic shape for a wide decimal column with a large fixed magnitude. fn noise(seed: u64) -> impl Iterator { - let mut state = seed; - iter::repeat_with(move || { - state = state - .wrapping_mul(6_364_136_223_846_793_005) - .wrapping_add(1_442_695_040_888_963_407); - i128::from(state >> 40) - }) + let mut rng = StdRng::seed_from_u64(seed); + iter::repeat_with(move || i128::from(rng.random::() >> 8)) } /// `i128`-backed values that need more than 64 bits, so the encoding must carry one lower diff --git a/vortex-file/Cargo.toml b/vortex-file/Cargo.toml index 347bd0fc69e..55d59433645 100644 --- a/vortex-file/Cargo.toml +++ b/vortex-file/Cargo.toml @@ -60,6 +60,7 @@ vortex-zigzag = { workspace = true } vortex-zstd = { workspace = true, optional = true } [dev-dependencies] +rand = { workspace = true } rstest = { workspace = true } tokio = { workspace = true, features = ["full"] } vortex-array = { workspace = true, features = ["_test-harness"] } diff --git a/vortex-file/src/tests.rs b/vortex-file/src/tests.rs index 3da8fe1f016..e80bdedca14 100644 --- a/vortex-file/src/tests.rs +++ b/vortex-file/src/tests.rs @@ -11,6 +11,9 @@ use flatbuffers::FlatBufferBuilder; use futures::StreamExt; use futures::TryStreamExt; use futures::pin_mut; +use rand::RngExt; +use rand::SeedableRng as _; +use rand::rngs::StdRng; use rstest::rstest; use vortex_array::ArrayRef; use vortex_array::IntoArray; @@ -240,13 +243,8 @@ async fn test_wide_decimal_round_trip_compresses() -> VortexResult<()> { /// Deterministic 24-bit noise, so the low bits of each value are neither constant nor a /// sequence. fn noise(seed: u64) -> impl Iterator { - let mut state = seed; - iter::repeat_with(move || { - state = state - .wrapping_mul(6_364_136_223_846_793_005) - .wrapping_add(1_442_695_040_888_963_407); - i128::from(state >> 40) - }) + let mut rng = StdRng::seed_from_u64(seed); + iter::repeat_with(move || i128::from(rng.random::() >> 8)) } // Values that need more than 64 bits, so `i128` storage cannot be narrowed away. From 41f8284d29170f7b1ed389f597d9107181778312 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 3 Aug 2026 09:39:14 +0000 Subject: [PATCH 05/14] Reduce take instead of executing it, and drop a duplicated filter rule MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Auditing each compute function against the reduce/execute contract — `*Reduce` operates "purely on array metadata and structure without needing to read or execute on the underlying buffers", `*Kernel`/ `*Execute` may read buffers and take an `ExecutionCtx` — turned up two kernels on the wrong side of it. `take` was implemented as `TakeExecute` and registered as an execute parent kernel, but its body ignores the context entirely: `ArrayRef::take` wraps each part in a `Dict` and optimizes, which is a lazy rewrite, and the only other work is the `validate` call rebuilding the array. It is now `TakeReduce`, registered through `TakeReduceAdaptor` alongside the other parent reduce rules, so the push-down happens during optimization rather than being deferred to execution. `TakeReduceAdaptor` also applies the empty-indices and empty-array preconditions and propagates take statistics, neither of which the execute path was doing. The nullable indices guard keeps its meaning: `Ok(None)` now means "cannot do this without buffers", which is exactly the fallback it was asking for. `DecimalBytePartsFilterPushDownRule` was byte-for-byte what `FilterReduceAdaptor(DecimalByteParts)` already does via `FilterReduce`, and was listed first so it shadowed the adaptor — which meant filtering also skipped the adaptor's empty-mask preconditions. Removed; the adaptor that was already registered covers it. The other kernels are on the correct side and stay put. `filter`, `slice`, `cast` and `mask` build lazy wrappers only. `compare` needs `all_valid` to decide whether an uncoercible constant can be answered without null checks, and `is_constant` reads its children, so both legitimately take a context. `take_pushes_down_without_executing` pins the new behavior: it asserts that `take` on a wide array reduces to the encoding rather than being left as a `vortex.dict`, and fails with "got vortex.dict" if the rule is unregistered. Signed-off-by: "Joe Isaacs" --- .../src/decimal_byte_parts/compute/kernel.rs | 8 ---- .../src/decimal_byte_parts/compute/take.rs | 39 +++++++++++++++---- .../src/decimal_byte_parts/rules.rs | 29 +------------- 3 files changed, 33 insertions(+), 43 deletions(-) diff --git a/encodings/decimal-byte-parts/src/decimal_byte_parts/compute/kernel.rs b/encodings/decimal-byte-parts/src/decimal_byte_parts/compute/kernel.rs index 5e8d28e3526..cb71ba7880c 100644 --- a/encodings/decimal-byte-parts/src/decimal_byte_parts/compute/kernel.rs +++ b/encodings/decimal-byte-parts/src/decimal_byte_parts/compute/kernel.rs @@ -1,9 +1,6 @@ // SPDX-License-Identifier: Apache-2.0 // SPDX-FileCopyrightText: Copyright the Vortex contributors -use vortex_array::ArrayVTable; -use vortex_array::arrays::Dict; -use vortex_array::arrays::dict::TakeExecuteAdaptor; use vortex_array::optimizer::kernels::ArrayKernelsExt; use vortex_array::scalar_fn::ScalarFnVTable; use vortex_array::scalar_fn::fns::binary::Binary; @@ -19,9 +16,4 @@ pub(crate) fn initialize(session: &VortexSession) { DecimalByteParts, CompareExecuteAdaptor(DecimalByteParts), ); - kernels.register_execute_parent_kernel( - Dict.id(), - DecimalByteParts, - TakeExecuteAdaptor(DecimalByteParts), - ); } diff --git a/encodings/decimal-byte-parts/src/decimal_byte_parts/compute/take.rs b/encodings/decimal-byte-parts/src/decimal_byte_parts/compute/take.rs index 5665813fbe4..578834635b8 100644 --- a/encodings/decimal-byte-parts/src/decimal_byte_parts/compute/take.rs +++ b/encodings/decimal-byte-parts/src/decimal_byte_parts/compute/take.rs @@ -3,21 +3,18 @@ use vortex_array::ArrayRef; use vortex_array::ArrayView; -use vortex_array::ExecutionCtx; use vortex_array::IntoArray; -use vortex_array::arrays::dict::TakeExecute; +use vortex_array::arrays::dict::TakeReduce; use vortex_error::VortexResult; use crate::DecimalByteParts; use crate::decimal_byte_parts::DecimalBytePartsArraySlotsExt; use crate::decimal_byte_parts::map_parts; -impl TakeExecute for DecimalByteParts { - fn take( - array: ArrayView<'_, Self>, - indices: &ArrayRef, - _ctx: &mut ExecutionCtx, - ) -> VortexResult> { +impl TakeReduce for DecimalByteParts { + /// Taking wraps each part in a `Dict` without reading any buffer, so it reduces rather + /// than executes. + fn take(array: ArrayView<'_, Self>, indices: &ArrayRef) -> VortexResult> { // Taking with nullable indices makes every taken part nullable, but lower parts must // stay non-nullable `u64` — validity belongs to the MSP alone. Fall back to the // canonical path rather than rebuilding parts we would have to strip nullability from. @@ -41,11 +38,37 @@ mod tests { use vortex_array::dtype::DecimalDType; use vortex_array::validity::Validity; use vortex_buffer::Buffer; + use vortex_buffer::buffer; use vortex_error::VortexResult; + use crate::DecimalByteParts; use crate::decimal_byte_parts::testing::encode; use crate::decimal_byte_parts::testing::i256_of; + /// Taking pushes down into the parts during optimization, with no execution context in + /// play: `ArrayRef::take` wraps the array in a `Dict` and optimizes, and the reduce rule + /// must rewrite that into a `DecimalByteParts` of taken parts. + #[test] + fn take_pushes_down_without_executing() -> VortexResult<()> { + let session = array_session(); + crate::initialize(&session); + + let decimal = DecimalArray::new( + Buffer::from(vec![1i128 << 70, 2, 3]), + DecimalDType::new(38, 2), + Validity::NonNullable, + ); + let indices = buffer![0u64, 2].into_array(); + let taken = encode(&decimal)?.into_array().take(indices)?; + + assert!( + taken.is::(), + "expected the take to reduce into the encoding, got {}", + taken.encoding_id() + ); + Ok(()) + } + /// Taking with nullable indices must still round-trip the wide values, including the /// null row, on arrays that carry lower parts. #[rstest] diff --git a/encodings/decimal-byte-parts/src/decimal_byte_parts/rules.rs b/encodings/decimal-byte-parts/src/decimal_byte_parts/rules.rs index 572e031bacb..28503d5d8af 100644 --- a/encodings/decimal-byte-parts/src/decimal_byte_parts/rules.rs +++ b/encodings/decimal-byte-parts/src/decimal_byte_parts/rules.rs @@ -1,44 +1,19 @@ // SPDX-License-Identifier: Apache-2.0 // SPDX-FileCopyrightText: Copyright the Vortex contributors -use vortex_array::ArrayRef; -use vortex_array::ArrayView; -use vortex_array::IntoArray; -use vortex_array::arrays::Filter; +use vortex_array::arrays::dict::TakeReduceAdaptor; use vortex_array::arrays::filter::FilterReduceAdaptor; use vortex_array::arrays::slice::SliceReduceAdaptor; -use vortex_array::optimizer::rules::ArrayParentReduceRule; use vortex_array::optimizer::rules::ParentRuleSet; use vortex_array::scalar_fn::fns::cast::CastReduceAdaptor; use vortex_array::scalar_fn::fns::mask::MaskReduceAdaptor; -use vortex_error::VortexResult; use crate::DecimalByteParts; -use crate::decimal_byte_parts::map_parts; pub(super) const PARENT_RULES: ParentRuleSet = ParentRuleSet::new(&[ - ParentRuleSet::lift(&DecimalBytePartsFilterPushDownRule), ParentRuleSet::lift(&CastReduceAdaptor(DecimalByteParts)), ParentRuleSet::lift(&FilterReduceAdaptor(DecimalByteParts)), ParentRuleSet::lift(&MaskReduceAdaptor(DecimalByteParts)), ParentRuleSet::lift(&SliceReduceAdaptor(DecimalByteParts)), + ParentRuleSet::lift(&TakeReduceAdaptor(DecimalByteParts)), ]); - -#[derive(Debug)] -struct DecimalBytePartsFilterPushDownRule; - -impl ArrayParentReduceRule for DecimalBytePartsFilterPushDownRule { - type Parent = Filter; - - fn reduce_parent( - &self, - child: ArrayView<'_, DecimalByteParts>, - parent: ArrayView<'_, Filter>, - _child_idx: usize, - ) -> VortexResult> { - // TODO(ngates): we should benchmark whether to push-down filters with "lower parts", - // which filters each part separately rather than the canonical wide buffer once. - map_parts(child, |part| part.filter(parent.filter_mask().clone())) - .map(|c| Some(c.into_array())) - } -} From f67a889b897ac4baf1f2c4ca57e8d65092b735ea Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 3 Aug 2026 09:50:10 +0000 Subject: [PATCH 06/14] Move wide decimal compat coverage into its own fixture MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The wide `DecimalByteParts` columns were added to the existing `decimal_byte_parts.vortex` fixture, which breaks the compat contract. `DESIGN.md` states it directly under "Fixture evolution": a fixture's `build()` is immutable once published, because `check` compares files written by older releases against what `build()` produces today. Adding a column changes the schema the generator emits, so the check fails against every previously published version — exactly the regression the fixture exists to catch, reported against unrelated releases. `decimal_byte_parts.vortex` is restored to its published definition, and the wide cases move to a new `decimal_byte_parts_wide.vortex` with a comment recording why the split exists rather than leaving the next person to rediscover the rule. The new fixture gains a negative `i128` column so sign extension above the MSP is exercised on read back, alongside the one-lower-part and nullable three-lower-part cases. Verified with `generate` followed by `check --mode exact`: 36 fixtures pass, and `decimal_byte_parts.rs` is byte-identical to its pre-branch state. Signed-off-by: "Joe Isaacs" --- .../synthetic/encodings/decimal_byte_parts.rs | 41 ------- .../encodings/decimal_byte_parts_wide.rs | 113 ++++++++++++++++++ .../arrays/synthetic/encodings/mod.rs | 2 + 3 files changed, 115 insertions(+), 41 deletions(-) create mode 100644 vortex-test/compat-gen/src/fixtures/arrays/synthetic/encodings/decimal_byte_parts_wide.rs diff --git a/vortex-test/compat-gen/src/fixtures/arrays/synthetic/encodings/decimal_byte_parts.rs b/vortex-test/compat-gen/src/fixtures/arrays/synthetic/encodings/decimal_byte_parts.rs index 5571a8901a6..7e9ff6b7809 100644 --- a/vortex-test/compat-gen/src/fixtures/arrays/synthetic/encodings/decimal_byte_parts.rs +++ b/vortex-test/compat-gen/src/fixtures/arrays/synthetic/encodings/decimal_byte_parts.rs @@ -5,33 +5,18 @@ use vortex::array::ArrayId; use vortex::array::ArrayRef; use vortex::array::ArrayVTable; use vortex::array::IntoArray; -use vortex::array::arrays::DecimalArray; use vortex::array::arrays::PrimitiveArray; use vortex::array::arrays::StructArray; use vortex::array::dtype::DecimalDType; use vortex::array::dtype::FieldNames; -use vortex::array::dtype::i256; use vortex::array::validity::Validity; -use vortex::buffer::Buffer; use vortex::encodings::decimal_byte_parts::DecimalByteParts; -use vortex::encodings::decimal_byte_parts::DecimalBytePartsArray; -use vortex::encodings::decimal_byte_parts::split_decimal; use vortex::error::VortexResult; use vortex_array::ExecutionCtx; use super::N; use crate::fixtures::FlatLayoutFixture; -/// Encode a canonical decimal as byte parts, splitting wide values into lower parts. -fn encode_byte_parts(decimal: &DecimalArray) -> VortexResult { - let parts = split_decimal(decimal)?; - DecimalByteParts::try_new_with_lower_parts( - parts.msp, - parts.lower_parts, - decimal.decimal_dtype(), - ) -} - pub struct DecimalBytePartsFixture; impl FlatLayoutFixture for DecimalBytePartsFixture { @@ -95,28 +80,6 @@ impl FlatLayoutFixture for DecimalBytePartsFixture { let near_limit_arr = DecimalByteParts::try_new(near_limit_values.into_array(), near_limit_dtype)?; - // Wide decimals, split into an MSP plus 64-bit lower parts. - let wide_128_dtype = DecimalDType::new(38, 2); - let wide_128 = DecimalArray::new( - (0..N as i128) - .map(|i| 10i128.pow(25) + i * 7) - .collect::>(), - wide_128_dtype, - Validity::NonNullable, - ); - let wide_128_arr = encode_byte_parts(&wide_128)?; - - let wide_256_dtype = DecimalDType::new(76, 2); - let base = i256::from_i128(10).wrapping_pow(40); - let wide_256 = DecimalArray::new( - (0..N as i128) - .map(|i| base + i256::from_i128(i * 7)) - .collect::>(), - wide_256_dtype, - Validity::from_iter((0..N).map(|i| i % 7 != 0)), - ); - let wide_256_arr = encode_byte_parts(&wide_256)?; - let arr = StructArray::try_new( FieldNames::from([ "dec_10_2", @@ -127,8 +90,6 @@ impl FlatLayoutFixture for DecimalBytePartsFixture { "dec_crossing", "dec_trailing_zero", "dec_near_limit", - "dec_wide_128", - "dec_wide_256_nullable", ]), vec![ decimal_arr.into_array(), @@ -139,8 +100,6 @@ impl FlatLayoutFixture for DecimalBytePartsFixture { crossing_arr.into_array(), trailing_zero_arr.into_array(), near_limit_arr.into_array(), - wide_128_arr.into_array(), - wide_256_arr.into_array(), ], N, Validity::NonNullable, diff --git a/vortex-test/compat-gen/src/fixtures/arrays/synthetic/encodings/decimal_byte_parts_wide.rs b/vortex-test/compat-gen/src/fixtures/arrays/synthetic/encodings/decimal_byte_parts_wide.rs new file mode 100644 index 00000000000..2f0b9222e61 --- /dev/null +++ b/vortex-test/compat-gen/src/fixtures/arrays/synthetic/encodings/decimal_byte_parts_wide.rs @@ -0,0 +1,113 @@ +// SPDX-License-Identifier: Apache-2.0 +// SPDX-FileCopyrightText: Copyright the Vortex contributors + +//! Wide `DecimalByteParts` fixtures: values that need lower parts. +//! +//! These live in their own fixture file rather than as extra columns on +//! `decimal_byte_parts.vortex` because a fixture's `build()` is immutable once published. +//! `check` compares files written by older releases against what `build()` produces today, +//! so changing an existing fixture's schema fails the check against every previously +//! published version — see "Fixture evolution" in `DESIGN.md`, which requires a new fixture +//! file with a new name for a new type, encoding, or structural pattern. +//! +//! So `decimal_byte_parts.vortex` keeps testing exactly what it always did, decimals whose +//! values fit a single signed part, and the MSP-plus-lower-parts layout added alongside it +//! is covered here instead. + +use vortex::array::ArrayId; +use vortex::array::ArrayRef; +use vortex::array::ArrayVTable; +use vortex::array::IntoArray; +use vortex::array::arrays::DecimalArray; +use vortex::array::arrays::StructArray; +use vortex::array::dtype::DecimalDType; +use vortex::array::dtype::FieldNames; +use vortex::array::dtype::i256; +use vortex::array::validity::Validity; +use vortex::buffer::Buffer; +use vortex::encodings::decimal_byte_parts::DecimalByteParts; +use vortex::encodings::decimal_byte_parts::DecimalBytePartsArray; +use vortex::encodings::decimal_byte_parts::split_decimal; +use vortex::error::VortexResult; +use vortex_array::ExecutionCtx; + +use super::N; +use crate::fixtures::FlatLayoutFixture; + +/// Encode a canonical decimal as byte parts, splitting wide values into lower parts. +fn encode_byte_parts(decimal: &DecimalArray) -> VortexResult { + let parts = split_decimal(decimal)?; + DecimalByteParts::try_new_with_lower_parts( + parts.msp, + parts.lower_parts, + decimal.decimal_dtype(), + ) +} + +pub struct DecimalBytePartsWideFixture; + +impl FlatLayoutFixture for DecimalBytePartsWideFixture { + fn name(&self) -> &str { + "decimal_byte_parts_wide.vortex" + } + + fn description(&self) -> &str { + "Wide decimal arrays split into a most significant part plus 64-bit lower parts" + } + + fn expected_encodings(&self) -> Vec { + vec![DecimalByteParts.id()] + } + + fn build(&self, _ctx: &mut ExecutionCtx) -> VortexResult { + // An `i128` magnitude above 2^64, so the encoding must carry one lower part. + let wide_128_dtype = DecimalDType::new(38, 2); + let wide_128 = DecimalArray::new( + (0..N as i128) + .map(|i| 10i128.pow(25) + i * 7) + .collect::>(), + wide_128_dtype, + Validity::NonNullable, + ); + let wide_128_arr = encode_byte_parts(&wide_128)?; + + // Negative values, so the sign extension above the MSP is exercised on read back. + let wide_128_negative = DecimalArray::new( + (0..N as i128) + .map(|i| -(10i128.pow(25)) - i * 7) + .collect::>(), + wide_128_dtype, + Validity::NonNullable, + ); + let wide_128_negative_arr = encode_byte_parts(&wide_128_negative)?; + + // An `i256` magnitude beyond 128 bits, so all three lower parts are populated, with + // nulls to pin that validity is carried by the MSP alone. + let wide_256_dtype = DecimalDType::new(76, 2); + let base = i256::from_i128(10).wrapping_pow(40); + let wide_256 = DecimalArray::new( + (0..N as i128) + .map(|i| base + i256::from_i128(i * 7)) + .collect::>(), + wide_256_dtype, + Validity::from_iter((0..N).map(|i| i % 7 != 0)), + ); + let wide_256_arr = encode_byte_parts(&wide_256)?; + + let arr = StructArray::try_new( + FieldNames::from([ + "dec_wide_128", + "dec_wide_128_negative", + "dec_wide_256_nullable", + ]), + vec![ + wide_128_arr.into_array(), + wide_128_negative_arr.into_array(), + wide_256_arr.into_array(), + ], + N, + Validity::NonNullable, + )?; + Ok(arr.into_array()) + } +} diff --git a/vortex-test/compat-gen/src/fixtures/arrays/synthetic/encodings/mod.rs b/vortex-test/compat-gen/src/fixtures/arrays/synthetic/encodings/mod.rs index 830b50450da..0027570e95c 100644 --- a/vortex-test/compat-gen/src/fixtures/arrays/synthetic/encodings/mod.rs +++ b/vortex-test/compat-gen/src/fixtures/arrays/synthetic/encodings/mod.rs @@ -12,6 +12,7 @@ mod bytebool; mod constant; mod datetimeparts; mod decimal_byte_parts; +mod decimal_byte_parts_wide; mod delta; mod dict; mod for_; @@ -38,6 +39,7 @@ pub fn fixtures() -> Vec> { Box::new(bytebool::ByteBoolFixture), Box::new(datetimeparts::DateTimePartsFixture), Box::new(decimal_byte_parts::DecimalBytePartsFixture), + Box::new(decimal_byte_parts_wide::DecimalBytePartsWideFixture), // Re-enable this once delta is stable // Box::new(delta::DeltaFixture), Box::new(dict::DictFixture), From b2f8d2b6682af8bab3bc1fb5f1bbeeb939292a72 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 3 Aug 2026 10:04:21 +0000 Subject: [PATCH 07/14] Benchmark hand-written 64-bit words against the u128 packing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `i256::from_parts` takes a `u128` and an `i128`, so each row of the assembly loop ends in `u128::from(w0) | (u128::from(w1) << 64)`. The reasonable suspicion is that this is worse than storing four `u64`s by hand, since 128-bit integers have a reputation for lowering badly. `i256_row_words` is that hand-written version: it builds a `u64` lane buffer and reinterprets it as `i256` at the end, so no 128-bit value is ever written. Over 65,536 rows it ties the shipped shape across four runs (`fastest` 224-228 µs against 227-236 µs), which is inside the noise on this host. Disassembly explains the tie and is the more durable evidence. Neither shape emits a single `shld`/`shrd`, and both compile to four plain 64-bit stores per row at offsets 0x0/0x8/0x10/0x18. The `i128` loop is the same: `(i128::from(msp) << 64) | i128::from(part)` becomes two 64-bit stores with no shift at all. A shift by a constant multiple of 64 followed by an or is pure data movement and LLVM recognizes it as such; the 128-bit codegen actually worth avoiding is division and remainder, which call into compiler-rt, and shifts by a runtime amount. Neither appears in this code. So no change to the assembly loops. The variant and the reasoning stay in the benchmark, because "avoid the u128" is a rewrite someone will propose again and this is the answer. Signed-off-by: "Joe Isaacs" --- .../benches/decimal_assemble.rs | 47 +++++++++++++++++++ 1 file changed, 47 insertions(+) diff --git a/encodings/decimal-byte-parts/benches/decimal_assemble.rs b/encodings/decimal-byte-parts/benches/decimal_assemble.rs index e5e97a81b66..d0198cfc589 100644 --- a/encodings/decimal-byte-parts/benches/decimal_assemble.rs +++ b/encodings/decimal-byte-parts/benches/decimal_assemble.rs @@ -36,6 +36,20 @@ //! to rot: cache blocking the lane passes over 1024-row blocks recovered part of the strided //! stores but was still 1.6x slower than the row loop, and expressing the passes as //! whole-value `i256` shifts was 11x slower. +//! +//! # Hand-written 64-bit words vs `u128` packing +//! +//! `i256::from_parts` takes a `u128` and an `i128`, so the assembly loops end each row with +//! `u128::from(w0) | (u128::from(w1) << 64)`. The reflex is that this must be worse than +//! storing four `u64`s by hand, because 128-bit integers lower badly. `i256_row_words` is +//! that hand-written version, and it ties `i256_row_const` across runs. +//! +//! Disassembling the release build says why: neither shape emits a single `shld`/`shrd`, and +//! both compile to four plain 64-bit stores per row at offsets 0x0/0x8/0x10/0x18. The `i128` +//! loop is the same — `(i128::from(msp) << 64) | i128::from(part)` becomes two 64-bit stores. +//! A shift by a constant multiple of 64 followed by an or is pure data movement, and LLVM +//! recognizes it. The 128-bit codegen worth avoiding is division/remainder, which call into +//! compiler-rt, and shifts by a runtime amount; neither appears here. #![allow(clippy::unwrap_used, clippy::cast_possible_truncation)] @@ -63,6 +77,9 @@ const LEN: usize = 65_536; const WORD_BITS: usize = 64; +/// 64-bit words in the widest decimal value (`i256`). +const MAX_VALUE_WORDS: usize = 4; + /// Deterministic pseudo-random words, so no part is constant or a sequence. fn words(seed: u64, len: usize) -> Buffer { let mut rng = StdRng::seed_from_u64(seed); @@ -179,6 +196,28 @@ fn i256_column_lanes(msp: &[i64], lower: [&[u64]; 3]) -> Buffer { Buffer::::from_byte_buffer_aligned(w.freeze().into_byte_buffer(), Alignment::of::()) } +/// The specialized row shape emitting raw 64-bit words: the output is built as a `u64` lane +/// buffer and reinterpreted as `i256` at the end, so no 128-bit shift or or is ever written. +/// +/// This exists to answer "shouldn't we avoid `u128` entirely, since LLVM handles 128-bit +/// types badly?" — it does not, for this pattern. See the module docs. +fn i256_row_words(msp: &[i64], lower: [&[u64]; K]) -> Buffer { + let len = msp.len(); + let mut w = BufferMut::::zeroed_aligned(len * MAX_VALUE_WORDS, Alignment::of::()); + let lanes = w.as_mut_slice(); + for (row, m) in msp.iter().enumerate() { + let mut words = [if *m < 0 { u64::MAX } else { 0 }; MAX_VALUE_WORDS]; + for (i, part) in lower.iter().enumerate() { + words[K - 1 - i] = part[row]; + } + words[K] = m.cast_unsigned(); + lanes[row * MAX_VALUE_WORDS..(row + 1) * MAX_VALUE_WORDS].copy_from_slice(&words); + } + // Word order within an `i256` is ascending significance on a little-endian host. + assert!(cfg!(target_endian = "little")); + Buffer::::from_byte_buffer_aligned(w.freeze().into_byte_buffer(), Alignment::of::()) +} + /// The specialized row shape for `i256`, writing into a pre-sized buffer. fn i256_row_write(msp: &[i64], lower: [&[u64]; K]) -> Buffer { let mut out = BufferMut::::zeroed(msp.len()); @@ -289,6 +328,14 @@ fn i256_row_write_parts(bencher: Bencher) { bencher.bench(|| i256_row_write(black_box(parts.msp.as_slice()), black_box(lower))); } +#[divan::bench] +fn i256_row_words_parts(bencher: Bencher) { + let parts = Parts::new(3); + let lower = parts.lower_slices(); + let lower = [lower[0], lower[1], lower[2]]; + bencher.bench(|| i256_row_words(black_box(parts.msp.as_slice()), black_box(lower))); +} + #[divan::bench(args = [1, 3])] fn canonicalize_byte_parts(bencher: Bencher, lower_parts: usize) { use vortex_array::VortexSessionExecute; From 8dc569fb2aaffe687b53872f5c27db69bd9f4271 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 3 Aug 2026 10:09:57 +0000 Subject: [PATCH 08/14] Keep only the shipped-path benchmark for decimal assembly MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The hand-written shape variants have served their purpose: the design questions they were written to answer are settled, and the answers are recorded in the module docs. Keeping them means maintaining a second copy of the assembly loop that no test covers and that silently stops representing the shipped code the moment that loop changes. `canonicalize_byte_parts` stays. It goes through the array API rather than duplicating the loop, so it tracks whatever shape the crate ships and works as a regression guard. The module docs keep the measured conclusions — const part count is 1.85x on `i256`, the pre-sized write is 1.6x on `i128`, columnar loses on both, and hand-written 64-bit words tie the `u128` packing because neither emits a shift — with a note that the variants are recoverable from history if a future change needs to re-run the comparison rather than trust the numbers. 346 lines to 105. Signed-off-by: "Joe Isaacs" --- .../benches/decimal_assemble.rs | 339 ++---------------- 1 file changed, 38 insertions(+), 301 deletions(-) diff --git a/encodings/decimal-byte-parts/benches/decimal_assemble.rs b/encodings/decimal-byte-parts/benches/decimal_assemble.rs index d0198cfc589..193149a7c68 100644 --- a/encodings/decimal-byte-parts/benches/decimal_assemble.rs +++ b/encodings/decimal-byte-parts/benches/decimal_assemble.rs @@ -1,55 +1,39 @@ // SPDX-License-Identifier: Apache-2.0 // SPDX-FileCopyrightText: Copyright the Vortex contributors -//! Reassembling `DecimalByteParts` into canonical `i128`/`i256` values. +//! Canonicalizing `DecimalByteParts` into `i128`/`i256` values. //! -//! Canonicalizing a wide decimal walks a most significant part plus one (`i128`) or three -//! (`i256`) unsigned 64-bit lower parts, and has to produce one wide value per row. The -//! obvious shapes are: +//! Reassembly walks a most significant part plus one (`i128`) or three (`i256`) unsigned +//! 64-bit lower parts and produces one wide value per row. This benchmark measures the +//! shipped path through the array API, so it tracks whatever shape the crate currently uses +//! and cannot drift away from it. //! -//! - **row**: one pass, gathering the row's word from each part and combining. Sub-variants -//! differ in whether the part count is known to the compiler (`_const` vs `_dynamic`) and -//! in whether the output is pushed into a reserved buffer or stored into a pre-sized one -//! (`_write`). -//! - **column**: one pass per part over the whole output, writing 64-bit lanes directly. +//! Alternative shapes were compared while choosing that path and then removed, since keeping +//! hand-written copies of the assembly loop here means maintaining the same loop twice. At +//! 65,536 rows, `fastest` of three runs: //! -//! Every candidate is spelled out here rather than called through the crate, so the same -//! comparison can be run from any revision. `canonicalize_byte_parts` goes through the array -//! API instead, and so tracks whichever shape the crate currently ships. +//! - **`i256` is dominated by the part count being visible to the compiler.** Specializing it +//! to a constant is 1.85x (190 µs against 351 µs). How the output is written barely matters +//! at 32 bytes per row. +//! - **`i128` is dominated by the write.** Specializing the part count is worth only ~1.04x, +//! while storing into a pre-sized buffer instead of pushing into a reserved one is 1.6x +//! (83 µs against 138 µs) — the bounds-checked `push` is the whole cost at 16 bytes per row. +//! - **Columnar always loses.** For `i256` each lane store is strided by 32 bytes, 2.3x slower +//! than the row loop (438 µs); cache blocking the passes recovered part of that and was +//! still 1.6x slower; expressing them as whole-value `i256` shifts was 11x slower. For +//! `i128` the two-pass column shape (103 µs) beats the *pushing* row loop but still loses to +//! the single-pass write, so the second pass buys nothing once the push is gone. +//! - **Hand-written 64-bit words do not beat the `u128` packing.** `i256::from_parts` takes a +//! `u128` and an `i128`, so each row ends in `u128::from(w0) | (u128::from(w1) << 64)`. +//! Writing four `u64` lanes by hand instead ties it. Disassembly says why: neither emits a +//! single `shld`/`shrd`, and both compile to four plain 64-bit stores per row at offsets +//! 0x0/0x8/0x10/0x18. The `i128` loop is the same — `(i128::from(msp) << 64) | i128::from(p)` +//! becomes two 64-bit stores. A shift by a constant multiple of 64 followed by an or is pure +//! data movement and LLVM recognizes it; the 128-bit codegen worth avoiding is division and +//! remainder, which call into compiler-rt, and shifts by a runtime amount. Neither is here. //! -//! At 65,536 rows the row shape wins, but for two different reasons per width, and neither -//! is the row-at-a-time access itself: -//! -//! - `i256` is dominated by the part count being invisible to the compiler. Specializing it -//! is 1.85x. How the output is written barely matters (`_write` ties `_const`), because at -//! 32 bytes per row the stores dominate either way. -//! - `i128` is dominated by the write. Specializing the part count is worth only ~1.04x, -//! while storing into a pre-sized buffer instead of pushing is 1.6x — the bounds-checked -//! `push` is the whole cost at 16 bytes per row. -//! -//! Columnar always loses. For `i256` each lane store is strided by 32 bytes, 2.3x slower than -//! the specialized row loop. For `i128` the two-pass column shape beats the *pushing* row -//! loop but still loses to the single-pass `_write` row loop, so the two passes buy nothing -//! once the push is gone. -//! -//! Two further `i256` columnar variants were measured and then removed rather than left here -//! to rot: cache blocking the lane passes over 1024-row blocks recovered part of the strided -//! stores but was still 1.6x slower than the row loop, and expressing the passes as -//! whole-value `i256` shifts was 11x slower. -//! -//! # Hand-written 64-bit words vs `u128` packing -//! -//! `i256::from_parts` takes a `u128` and an `i128`, so the assembly loops end each row with -//! `u128::from(w0) | (u128::from(w1) << 64)`. The reflex is that this must be worse than -//! storing four `u64`s by hand, because 128-bit integers lower badly. `i256_row_words` is -//! that hand-written version, and it ties `i256_row_const` across runs. -//! -//! Disassembling the release build says why: neither shape emits a single `shld`/`shrd`, and -//! both compile to four plain 64-bit stores per row at offsets 0x0/0x8/0x10/0x18. The `i128` -//! loop is the same — `(i128::from(msp) << 64) | i128::from(part)` becomes two 64-bit stores. -//! A shift by a constant multiple of 64 followed by an or is pure data movement, and LLVM -//! recognizes it. The 128-bit codegen worth avoiding is division/remainder, which call into -//! compiler-rt, and shifts by a runtime amount; neither appears here. +//! The removed variants are recoverable from git history if a future change needs to re-run +//! the comparison rather than trust these numbers. #![allow(clippy::unwrap_used, clippy::cast_possible_truncation)] @@ -59,27 +43,23 @@ use rand::RngExt; use rand::SeedableRng; use rand::rngs::StdRng; use vortex_array::IntoArray; +use vortex_array::VortexSessionExecute; +use vortex_array::array_session; +use vortex_array::arrays::DecimalArray; use vortex_array::arrays::PrimitiveArray; use vortex_array::dtype::DecimalDType; -use vortex_array::dtype::i256; use vortex_array::validity::Validity; -use vortex_buffer::Alignment; use vortex_buffer::Buffer; -use vortex_buffer::BufferMut; +use vortex_decimal_byte_parts::DecimalByteParts; fn main() { divan::main(); } /// Rows per benchmark: a typical scan chunk, and large enough that the output does not fit -/// in L2, so the extra passes of a columnar shape are paid at their real cost. +/// in L2. const LEN: usize = 65_536; -const WORD_BITS: usize = 64; - -/// 64-bit words in the widest decimal value (`i256`). -const MAX_VALUE_WORDS: usize = 4; - /// Deterministic pseudo-random words, so no part is constant or a sequence. fn words(seed: u64, len: usize) -> Buffer { let mut rng = StdRng::seed_from_u64(seed); @@ -93,258 +73,15 @@ fn msp(seed: u64, len: usize) -> Buffer { .collect() } -// --------------------------------------------------------------------------------------- -// i128: one lower part -// --------------------------------------------------------------------------------------- - -/// The row shape with the part count only known at runtime: a fold over an iterator of the -/// row's words. -fn i128_row_dynamic(msp: &[i64], lower: &[&[u64]]) -> Buffer { - let mut out = BufferMut::::with_capacity(msp.len()); - for (row, m) in msp.iter().enumerate() { - out.push(lower.iter().fold(i128::from(*m), |acc, part| { - (acc << WORD_BITS) | i128::from(part[row]) - })); - } - out.freeze() -} - -/// The row shape specialized to exactly one lower part. -fn i128_row_const(msp: &[i64], lower: &[u64]) -> Buffer { - let mut out = BufferMut::::with_capacity(msp.len()); - for (m, l) in msp.iter().zip(lower) { - out.push((i128::from(*m) << WORD_BITS) | i128::from(*l)); - } - out.freeze() -} - -/// The column shape: one pass writes the most significant half of every value, a second -/// pass ORs in the lower part. -fn i128_column(msp: &[i64], lower: &[u64]) -> Buffer { - let mut out = BufferMut::::zeroed(msp.len()); - for (o, m) in out.as_mut_slice().iter_mut().zip(msp) { - *o = i128::from(*m) << WORD_BITS; - } - for (o, l) in out.as_mut_slice().iter_mut().zip(lower) { - *o |= i128::from(*l); - } - out.freeze() -} - -/// The row shape writing into a pre-sized buffer rather than pushing into a reserved one, -/// to separate "one pass vs two" from "bounds-checked push vs direct store". -fn i128_row_write(msp: &[i64], lower: &[u64]) -> Buffer { - let mut out = BufferMut::::zeroed(msp.len()); - for ((o, m), l) in out.as_mut_slice().iter_mut().zip(msp).zip(lower) { - *o = (i128::from(*m) << WORD_BITS) | i128::from(*l); - } - out.freeze() -} - -// --------------------------------------------------------------------------------------- -// i256: three lower parts -// --------------------------------------------------------------------------------------- - -/// The row shape with a runtime part count: per row, a stack array of words is filled at -/// dynamic indices and then packed. -fn i256_row_dynamic(msp: &[i64], lower: &[&[u64]]) -> Buffer { - let count = lower.len(); - let mut out = BufferMut::::with_capacity(msp.len()); - for (row, m) in msp.iter().enumerate() { - let mut w = [if *m < 0 { u64::MAX } else { 0 }; 4]; - for (i, part) in lower.iter().enumerate() { - w[count - 1 - i] = part[row]; - } - w[count] = m.cast_unsigned(); - out.push(i256::from_parts( - u128::from(w[0]) | (u128::from(w[1]) << WORD_BITS), - (u128::from(w[2]) | (u128::from(w[3]) << WORD_BITS)).cast_signed(), - )); - } - out.freeze() -} - -/// The row shape specialized to exactly three lower parts: every word index is a constant. -fn i256_row_const(msp: &[i64], lower: [&[u64]; 3]) -> Buffer { - let mut out = BufferMut::::with_capacity(msp.len()); - for row in 0..msp.len() { - out.push(i256::from_parts( - u128::from(lower[2][row]) | (u128::from(lower[1][row]) << WORD_BITS), - (u128::from(lower[0][row]) | (u128::from(msp[row].cast_unsigned()) << WORD_BITS)) - .cast_signed(), - )); - } - out.freeze() -} - -/// The column shape as lane writes: build the output as 64-bit words and write one lane per -/// pass. Avoids re-reading the output, but every store is strided by 32 bytes. -fn i256_column_lanes(msp: &[i64], lower: [&[u64]; 3]) -> Buffer { - let len = msp.len(); - let mut w = BufferMut::::zeroed_aligned(len * 4, Alignment::of::()); - let lanes = w.as_mut_slice(); - for (i, m) in msp.iter().enumerate() { - lanes[i * 4 + 3] = m.cast_unsigned(); - } - for (lane, part) in lower.iter().enumerate() { - for (i, word) in part.iter().enumerate() { - lanes[i * 4 + 2 - lane] = *word; - } - } - // Word order within an `i256` is ascending significance on a little-endian host. - assert!(cfg!(target_endian = "little")); - Buffer::::from_byte_buffer_aligned(w.freeze().into_byte_buffer(), Alignment::of::()) -} - -/// The specialized row shape emitting raw 64-bit words: the output is built as a `u64` lane -/// buffer and reinterpreted as `i256` at the end, so no 128-bit shift or or is ever written. -/// -/// This exists to answer "shouldn't we avoid `u128` entirely, since LLVM handles 128-bit -/// types badly?" — it does not, for this pattern. See the module docs. -fn i256_row_words(msp: &[i64], lower: [&[u64]; K]) -> Buffer { - let len = msp.len(); - let mut w = BufferMut::::zeroed_aligned(len * MAX_VALUE_WORDS, Alignment::of::()); - let lanes = w.as_mut_slice(); - for (row, m) in msp.iter().enumerate() { - let mut words = [if *m < 0 { u64::MAX } else { 0 }; MAX_VALUE_WORDS]; - for (i, part) in lower.iter().enumerate() { - words[K - 1 - i] = part[row]; - } - words[K] = m.cast_unsigned(); - lanes[row * MAX_VALUE_WORDS..(row + 1) * MAX_VALUE_WORDS].copy_from_slice(&words); - } - // Word order within an `i256` is ascending significance on a little-endian host. - assert!(cfg!(target_endian = "little")); - Buffer::::from_byte_buffer_aligned(w.freeze().into_byte_buffer(), Alignment::of::()) -} - -/// The specialized row shape for `i256`, writing into a pre-sized buffer. -fn i256_row_write(msp: &[i64], lower: [&[u64]; K]) -> Buffer { - let mut out = BufferMut::::zeroed(msp.len()); - for (row, (o, m)) in out.as_mut_slice().iter_mut().zip(msp).enumerate() { - let mut words = [if *m < 0 { u64::MAX } else { 0 }; 4]; - for (i, part) in lower.iter().enumerate() { - words[K - 1 - i] = part[row]; - } - words[K] = m.cast_unsigned(); - *o = i256::from_parts( - u128::from(words[0]) | (u128::from(words[1]) << WORD_BITS), - (u128::from(words[2]) | (u128::from(words[3]) << WORD_BITS)).cast_signed(), - ); - } - out.freeze() -} - -// --------------------------------------------------------------------------------------- -// Benchmarks -// --------------------------------------------------------------------------------------- - -struct Parts { - msp: Buffer, - lower: Vec>, -} - -impl Parts { - fn new(lower_parts: usize) -> Self { - Self { - msp: msp(1, LEN), - lower: (0..lower_parts).map(|i| words(7 + i as u64, LEN)).collect(), - } - } - - fn lower_slices(&self) -> Vec<&[u64]> { - self.lower.iter().map(|part| part.as_slice()).collect() - } - - fn arrays(&self) -> (PrimitiveArray, Vec) { - ( - PrimitiveArray::new(self.msp.clone(), Validity::NonNullable), - self.lower - .iter() - .map(|part| PrimitiveArray::new(part.clone(), Validity::NonNullable)) - .collect(), - ) - } -} - -#[divan::bench] -fn i128_row_dynamic_parts(bencher: Bencher) { - let parts = Parts::new(1); - let lower = parts.lower_slices(); - bencher.bench(|| i128_row_dynamic(black_box(parts.msp.as_slice()), black_box(&lower))); -} - -#[divan::bench] -fn i128_row_const_parts(bencher: Bencher) { - let parts = Parts::new(1); - let lower = parts.lower_slices(); - bencher.bench(|| i128_row_const(black_box(parts.msp.as_slice()), black_box(lower[0]))); -} - -#[divan::bench] -fn i128_column_parts(bencher: Bencher) { - let parts = Parts::new(1); - let lower = parts.lower_slices(); - bencher.bench(|| i128_column(black_box(parts.msp.as_slice()), black_box(lower[0]))); -} - -#[divan::bench] -fn i128_row_write_parts(bencher: Bencher) { - let parts = Parts::new(1); - let lower = parts.lower_slices(); - bencher.bench(|| i128_row_write(black_box(parts.msp.as_slice()), black_box(lower[0]))); -} - -#[divan::bench] -fn i256_row_dynamic_parts(bencher: Bencher) { - let parts = Parts::new(3); - let lower = parts.lower_slices(); - bencher.bench(|| i256_row_dynamic(black_box(parts.msp.as_slice()), black_box(&lower))); -} - -#[divan::bench] -fn i256_row_const_parts(bencher: Bencher) { - let parts = Parts::new(3); - let lower = parts.lower_slices(); - let lower = [lower[0], lower[1], lower[2]]; - bencher.bench(|| i256_row_const(black_box(parts.msp.as_slice()), black_box(lower))); -} - -#[divan::bench] -fn i256_column_lanes_parts(bencher: Bencher) { - let parts = Parts::new(3); - let lower = parts.lower_slices(); - let lower = [lower[0], lower[1], lower[2]]; - bencher.bench(|| i256_column_lanes(black_box(parts.msp.as_slice()), black_box(lower))); -} - /// Canonicalizing through the public array API, so the child execution and validity handling /// around the assembly loop are included. -#[divan::bench] -fn i256_row_write_parts(bencher: Bencher) { - let parts = Parts::new(3); - let lower = parts.lower_slices(); - let lower = [lower[0], lower[1], lower[2]]; - bencher.bench(|| i256_row_write(black_box(parts.msp.as_slice()), black_box(lower))); -} - -#[divan::bench] -fn i256_row_words_parts(bencher: Bencher) { - let parts = Parts::new(3); - let lower = parts.lower_slices(); - let lower = [lower[0], lower[1], lower[2]]; - bencher.bench(|| i256_row_words(black_box(parts.msp.as_slice()), black_box(lower))); -} - #[divan::bench(args = [1, 3])] fn canonicalize_byte_parts(bencher: Bencher, lower_parts: usize) { - use vortex_array::VortexSessionExecute; - use vortex_array::array_session; - use vortex_array::arrays::DecimalArray; - use vortex_decimal_byte_parts::DecimalByteParts; + let msp = PrimitiveArray::new(msp(1, LEN), Validity::NonNullable); + let lower = (0..lower_parts) + .map(|i| PrimitiveArray::new(words(7 + i as u64, LEN), Validity::NonNullable)) + .collect::>(); - let parts = Parts::new(lower_parts); - let (msp, lower) = parts.arrays(); let dtype = if lower_parts == 1 { DecimalDType::new(38, 2) } else { From a14efab5063978bbd5ecfbedf902e344edb236a3 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 3 Aug 2026 10:44:51 +0000 Subject: [PATCH 09/14] Express i256 split and assembly through one 64-bit word view MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An `i256` is exactly `{_0: u64, _1: u64, _2: u64, _3: i64}`: three unsigned words beneath a single signed one. That is the same shape this encoding stores — unsigned lower parts under a signed most significant part — and it is why splitting and reassembling are pure reinterpretation rather than arithmetic. No carry crosses a word boundary, so each word compresses independently and goes back verbatim. The code did not say so. Three sites open-coded the same word math with `to_parts`/`from_parts` and shifts: `split_i256` unpacking, and `combine_i256` and `assemble_i256` packing, the latter two character for character identical. A reader had to re-derive the layout at each one, and `split_i256` carried a `cast_possible_truncation`/`cast_sign_loss` expect that hid where the truncation was meant to happen. `i256_to_words` and `i256_from_words` now name the reinterpretation, and `sign_extended_words` names the other half of the invariant: the words above the most significant part are its sign. `split_i256` reads as the inverse of `assemble_i256` at `K == MAX_LOWER_PARTS`, and says so. Codegen is unchanged. `assemble_i256` still compiles to four plain 64-bit stores per row at offsets 0x0/0x8/0x10/0x18 with no `shld`/`shrd`, and the only shifts in the function are index scaling and a single `sar $0x3f` — the branchless sign broadcast, which is the ideal lowering of the fill. Signed-off-by: "Joe Isaacs" --- .../src/decimal_byte_parts/limbs.rs | 89 +++++++++++++------ 1 file changed, 64 insertions(+), 25 deletions(-) diff --git a/encodings/decimal-byte-parts/src/decimal_byte_parts/limbs.rs b/encodings/decimal-byte-parts/src/decimal_byte_parts/limbs.rs index adaa7555c9a..a21d62d046d 100644 --- a/encodings/decimal-byte-parts/src/decimal_byte_parts/limbs.rs +++ b/encodings/decimal-byte-parts/src/decimal_byte_parts/limbs.rs @@ -176,6 +176,54 @@ pub(crate) fn combine_i128(msp: i64, lower: impl IntoIterator) -> i1 }) } +/// 64-bit words in an `i256`. +const VALUE_WORDS: usize = 4; + +/// The 64-bit words of an `i256`, ascending significance. +/// +/// An `i256` is exactly `{_0: u64, _1: u64, _2: u64, _3: i64}` — three unsigned words beneath +/// a single signed one — which is the same shape this encoding stores. That is why splitting +/// and reassembling are pure reinterpretation rather than arithmetic: no carry ever crosses a +/// word boundary, so each word can be compressed independently and put back verbatim. +/// +/// The sign lives in the most significant word alone. When the most significant part is +/// narrower than 64 bits, or sits below word 3, the words above it are its sign extension. +type ValueWords = [u64; VALUE_WORDS]; + +/// Reinterpret an `i256` as its 64-bit words. +#[inline] +const fn i256_to_words(value: i256) -> ValueWords { + let (low, high) = value.to_parts(); + #[expect( + clippy::cast_possible_truncation, + reason = "each cast takes the low 64 bits of a word pair by construction" + )] + [ + low as u64, + (low >> LOWER_PART_BITS) as u64, + high as u64, + (high >> LOWER_PART_BITS) as u64, + ] +} + +/// Reinterpret 64-bit words as an `i256`, with the most significant word carrying the sign. +#[inline] +const fn i256_from_words(words: ValueWords) -> i256 { + i256::from_parts( + (words[0] as u128) | ((words[1] as u128) << LOWER_PART_BITS), + ((words[2] as u128) | ((words[3] as u128) << LOWER_PART_BITS)) as i128, + ) +} + +/// The words of a value whose most significant part sits at `msp_word`, with every word above +/// it filled with the MSP's sign. +#[inline] +fn sign_extended_words(msp: i64, msp_word: usize) -> ValueWords { + let mut words = [if msp < 0 { u64::MAX } else { 0 }; VALUE_WORDS]; + words[msp_word] = msp.cast_unsigned(); + words +} + /// Combine a single row's parts into an `i256`. /// /// The lower parts fill the least significant 64-bit words, the MSP the word above them, @@ -183,16 +231,11 @@ pub(crate) fn combine_i128(msp: i64, lower: impl IntoIterator) -> i1 #[inline] pub(crate) fn combine_i256(msp: i64, lower: impl ExactSizeIterator) -> i256 { let count = lower.len(); - let mut words = [if msp < 0 { u64::MAX } else { 0 }; 4]; + let mut words = sign_extended_words(msp, count); for (i, part) in lower.enumerate() { words[count - 1 - i] = part; } - words[count] = msp.cast_unsigned(); - - i256::from_parts( - u128::from(words[0]) | (u128::from(words[1]) << LOWER_PART_BITS), - (u128::from(words[2]) | (u128::from(words[3]) << LOWER_PART_BITS)).cast_signed(), - ) + i256_from_words(words) } impl DecimalParts { @@ -234,23 +277,22 @@ fn split_i128(values: &Buffer) -> (Buffer, Buffer) { (msp.freeze(), lower.freeze()) } -#[expect( - clippy::cast_possible_truncation, - clippy::cast_sign_loss, - reason = "splitting a wide integer into 64-bit windows truncates by construction" -)] +/// The inverse of [`assemble_i256`] at `K == MAX_LOWER_PARTS`: word 3 becomes the signed MSP, +/// and words 2, 1, 0 become the lower parts, most significant first. fn split_i256(values: &Buffer) -> (Buffer, [Buffer; MAX_LOWER_PARTS]) { let mut msp = BufferMut::::with_capacity(values.len()); - // Ordered most significant first: bits 191..128, 127..64, 63..0. let mut lower = std::array::from_fn::<_, MAX_LOWER_PARTS, _>(|_| { BufferMut::::with_capacity(values.len()) }); for value in values.iter() { - let (low, high) = value.to_parts(); - msp.push((high >> LOWER_PART_BITS) as i64); - lower[0].push(high as u64); - lower[1].push((low >> LOWER_PART_BITS) as u64); - lower[2].push(low as u64); + let words = i256_to_words(*value); + msp.push(words[MAX_LOWER_PARTS].cast_signed()); + for (part, word) in lower + .iter_mut() + .zip(words.iter().take(MAX_LOWER_PARTS).rev()) + { + part.push(*word); + } } (msp.freeze(), lower.map(BufferMut::freeze)) } @@ -293,16 +335,13 @@ fn assemble_i256(msp: &PrimitiveArray, lower: [&[u64]; K]) -> Bu let mut out = BufferMut::::with_capacity(msp.len()); match_each_signed_integer_ptype!(msp.ptype(), |P| { for (row, value) in msp.as_slice::

().iter().enumerate() { - let value = i64::from(*value); - let mut words = [if value < 0 { u64::MAX } else { 0 }; 4]; + // The MSP occupies word `K`, the lower parts the `K` words beneath it most + // significant first, and anything above word `K` is the MSP's sign. + let mut words = sign_extended_words(i64::from(*value), K); for (i, part) in lower.iter().enumerate() { words[K - 1 - i] = part[row]; } - words[K] = value.cast_unsigned(); - out.push(i256::from_parts( - u128::from(words[0]) | (u128::from(words[1]) << LOWER_PART_BITS), - (u128::from(words[2]) | (u128::from(words[3]) << LOWER_PART_BITS)).cast_signed(), - )); + out.push(i256_from_words(words)); } }); out.freeze() From 29e3d856463b3a44a64b3439e93053fa81021d6b Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 3 Aug 2026 14:01:11 +0000 Subject: [PATCH 10/14] Gate writing lower parts behind unstable_encodings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A reader that predates lower parts expects this encoding to have exactly one child, so a file containing a multi-child `DecimalByteParts` is one it cannot open. Introducing lower parts is now gated behind `unstable_encodings` at both places that can introduce them. `DecimalByteParts::try_new_with_lower_parts` rejects a non-empty lower-parts list without the feature, and names the feature in the error. `try_new` builds a single child and is unaffected. In the compressor, the decimal scheme leaves values too wide for one signed part as the canonical decimal instead of splitting them, and reports `num_children` as 1 — restoring exactly the pre-lower-parts behaviour, which was to return the narrowed array uncompressed. The gate is on *introducing* lower parts, not on having them. Rebuilding an array whose parts already exist goes through a new crate-private `rebuild_with_lower_parts`, which every compute kernel uses via `map_parts`/`with_msp`, and `deserialize` is untouched. Gating those too would mean a build without the feature could not read a file written by a build with it — strictly worse than not being able to write one. An earlier revision of this change did gate them, and `compute_over_existing_lower_parts_is_not_gated` fails without the split: reverting `map_parts` to the public constructor breaks filter, take, slice and the consistency suite on every wide array. That test also drove the gate's shape. Letting the crate's own unit tests through the gate via `cfg!(test)` would have hidden exactly that bug, since unit tests would no longer run the configuration they ship. The gate is therefore purely `cfg!(feature = ...)`, and the test helpers that build wide arrays call `rebuild_with_lower_parts` explicitly, so a default `cargo test` still covers the multi-part paths while running the same gate production does. `tests/lower_parts_gate.rs` covers the gate itself. Tests that assert lower parts are *produced* — the btrblocks split and compression-ratio tests, and the vortex-file round trip — are gated on the feature, since without it the compressor deliberately declines. The benchmark declares `required-features` for the same reason. The wide compat fixture is gated too: it is a written file, so generating it by default would emit precisely what the gate exists to prevent. A default `generate` produces 35 fixtures, and 36 with the feature; `check --mode exact` passes in both. Signed-off-by: "Joe Isaacs" --- encodings/decimal-byte-parts/Cargo.toml | 7 ++ .../src/decimal_byte_parts/mod.rs | 74 +++++++++++++++---- .../src/decimal_byte_parts/testing.rs | 12 +-- .../tests/lower_parts_gate.rs | 62 ++++++++++++++++ vortex-btrblocks/Cargo.toml | 6 +- vortex-btrblocks/src/schemes/decimal/mod.rs | 23 ++++-- vortex-btrblocks/src/schemes/decimal/tests.rs | 21 ++++++ vortex-file/src/tests.rs | 8 ++ vortex-test/compat-gen/Cargo.toml | 5 ++ .../arrays/synthetic/encodings/mod.rs | 12 ++- 10 files changed, 201 insertions(+), 29 deletions(-) create mode 100644 encodings/decimal-byte-parts/tests/lower_parts_gate.rs diff --git a/encodings/decimal-byte-parts/Cargo.toml b/encodings/decimal-byte-parts/Cargo.toml index ad594db5725..e87c3f8a609 100644 --- a/encodings/decimal-byte-parts/Cargo.toml +++ b/encodings/decimal-byte-parts/Cargo.toml @@ -16,6 +16,11 @@ version = { workspace = true } [lints] workspace = true +[features] +# Lower parts make this encoding write more than one child, which readers that predate them +# cannot open. Gated until enough readers understand it. +unstable_encodings = [] + [dependencies] num-traits = { workspace = true } prost = { workspace = true } @@ -34,3 +39,5 @@ vortex-array = { path = "../../vortex-array", features = ["_test-harness"] } [[bench]] name = "decimal_assemble" harness = false +# Builds multi-part arrays, which the write gate only permits with this feature. +required-features = ["unstable_encodings"] diff --git a/encodings/decimal-byte-parts/src/decimal_byte_parts/mod.rs b/encodings/decimal-byte-parts/src/decimal_byte_parts/mod.rs index 69c51d14447..7ef4f346cd1 100644 --- a/encodings/decimal-byte-parts/src/decimal_byte_parts/mod.rs +++ b/encodings/decimal-byte-parts/src/decimal_byte_parts/mod.rs @@ -339,12 +339,21 @@ impl DecimalByteParts { lower_parts: Vec, decimal_dtype: DecimalDType, ) -> VortexResult { - let len = msp.len(); - let dtype = DType::Decimal(decimal_dtype, msp.dtype().nullability()); - let slots = DecimalBytePartsSlots { msp, lower_parts }.into_slots(); - Array::try_from_parts( - ArrayParts::new(DecimalByteParts, dtype, len, DecimalBytePartsData).with_slots(slots), - ) + // A reader that predates lower parts expects this encoding to have exactly one child, + // so a file containing a multi-child array is one it cannot open. Introducing lower + // parts is gated behind `unstable_encodings` until enough readers understand them. + // + // This gate is on *introducing* lower parts only. Reading them back, and rebuilding an + // array that already has them, go through `rebuild_with_lower_parts` and stay ungated — + // otherwise a build without the feature could not read a file written by one with it. + vortex_ensure!( + lower_parts.is_empty() || cfg!(feature = "unstable_encodings"), + "DecimalByteParts with lower parts requires the `unstable_encodings` feature: \ + readers that predate lower parts understand only a single child, and would fail \ + to open a file containing this array" + ); + + rebuild_with_lower_parts(msp, lower_parts, decimal_dtype) } } @@ -353,6 +362,25 @@ fn values_type(array: ArrayView<'_, DecimalByteParts>) -> VortexResult, + decimal_dtype: DecimalDType, +) -> VortexResult { + let len = msp.len(); + let dtype = DType::Decimal(decimal_dtype, msp.dtype().nullability()); + let slots = DecimalBytePartsSlots { msp, lower_parts }.into_slots(); + Array::try_from_parts( + ArrayParts::new(DecimalByteParts, dtype, len, DecimalBytePartsData).with_slots(slots), + ) +} + /// The decimal dtype this array carries. /// /// Guaranteed to be a decimal by construction: [`DecimalBytePartsData::validate`] rejects @@ -379,7 +407,7 @@ pub(crate) fn map_parts( .iter() .map(&mut f) .collect::>>()?; - DecimalByteParts::try_new_with_lower_parts(msp, lower_parts, decimal_dtype(array)) + rebuild_with_lower_parts(msp, lower_parts, decimal_dtype(array)) } /// Rebuild the array with a replacement MSP, keeping its lower parts untouched. @@ -392,7 +420,7 @@ pub(crate) fn with_msp( msp: ArrayRef, decimal_dtype: DecimalDType, ) -> VortexResult { - DecimalByteParts::try_new_with_lower_parts(msp, array.lower_parts().to_vec(), decimal_dtype) + rebuild_with_lower_parts(msp, array.lower_parts().to_vec(), decimal_dtype) } /// Converts a DecimalBytePartsArray to its canonical DecimalArray representation. @@ -735,9 +763,7 @@ mod tests { #[case] lower_parts: Vec, #[case] decimal_dtype: DecimalDType, ) { - assert!( - DecimalByteParts::try_new_with_lower_parts(msp(), lower_parts, decimal_dtype).is_err() - ); + assert!(rebuild_with_lower_parts(msp(), lower_parts, decimal_dtype).is_err()); } fn deserialize_with( @@ -773,6 +799,26 @@ mod tests { Ok(()) } + /// Reading back an array that already carries lower parts, and computing over it, must + /// work regardless of the `unstable_encodings` write gate. The gate stops a writer + /// introducing lower parts; if it also blocked the rebuild that every compute kernel does, + /// a build without the feature could not read a file written by a build with it. + #[test] + fn compute_over_existing_lower_parts_is_not_gated() -> VortexResult<()> { + let session = array_session(); + crate::initialize(&session); + let mut ctx = session.create_execution_ctx(); + + // Stands in for an array materialized from a file: the parts already exist. + let array = deserialize_with(1, vec![msp(), lower_part()]) + .and_then(Array::try_from_parts)? + .into_array(); + + let sliced = array.slice(0..2)?; + assert_eq!(sliced.execute::(&mut ctx)?.len(), 2); + Ok(()) + } + /// A crafted file may declare more lower parts than its precision needs. Assembling those /// parts would produce values outside the declared precision, so it must be rejected at /// deserialization rather than panicking later in the scalar path. @@ -818,7 +864,7 @@ mod tests { assert_eq!(canonical.values_type(), DecimalType::I256); // A narrow MSP with a single lower part still fits 128 bits. - let array = DecimalByteParts::try_new_with_lower_parts( + let array = rebuild_with_lower_parts( buffer![1i8, -1, 0].into_array(), vec![buffer![7u64, 7, 7].into_array()], DecimalDType::new(38, 2), @@ -831,7 +877,7 @@ mod tests { ); // Two lower parts under a narrow MSP overflow 128 bits, so the value widens. - let array = DecimalByteParts::try_new_with_lower_parts( + let array = rebuild_with_lower_parts( buffer![1i8].into_array(), vec![buffer![0u64].into_array(), buffer![9u64].into_array()], DecimalDType::new(76, 2), @@ -845,7 +891,7 @@ mod tests { #[test] fn test_unused_buffer_of_values_is_ignored_for_null_rows() -> VortexResult<()> { // Null rows may hold arbitrary bits in the lower parts; they must stay null. - let array = DecimalByteParts::try_new_with_lower_parts( + let array = rebuild_with_lower_parts( PrimitiveArray::new( buffer![0i64, 0, 0], Validity::Array(BoolArray::from_iter([false, false, true]).into_array()), diff --git a/encodings/decimal-byte-parts/src/decimal_byte_parts/testing.rs b/encodings/decimal-byte-parts/src/decimal_byte_parts/testing.rs index bdee6df36a8..234e66b0557 100644 --- a/encodings/decimal-byte-parts/src/decimal_byte_parts/testing.rs +++ b/encodings/decimal-byte-parts/src/decimal_byte_parts/testing.rs @@ -11,18 +11,18 @@ use vortex_buffer::Buffer; use vortex_error::VortexExpect; use vortex_error::VortexResult; -use crate::DecimalByteParts; use crate::DecimalBytePartsArray; use crate::decimal_byte_parts::limbs::split_decimal; +use crate::decimal_byte_parts::rebuild_with_lower_parts; /// Encode a canonical decimal array as byte parts, splitting wide values into lower parts. +/// +/// Goes through [`rebuild_with_lower_parts`] rather than the public constructor so these +/// helpers exercise the multi-part paths under a default `cargo test`, independent of the +/// `unstable_encodings` write gate. The gate itself is covered by `tests/lower_parts_gate.rs`. pub(crate) fn encode(decimal: &DecimalArray) -> VortexResult { let parts = split_decimal(decimal)?; - DecimalByteParts::try_new_with_lower_parts( - parts.msp, - parts.lower_parts, - decimal.decimal_dtype(), - ) + rebuild_with_lower_parts(parts.msp, parts.lower_parts, decimal.decimal_dtype()) } /// An `i128`-backed decimal array, encoded as byte parts with one lower part. diff --git a/encodings/decimal-byte-parts/tests/lower_parts_gate.rs b/encodings/decimal-byte-parts/tests/lower_parts_gate.rs new file mode 100644 index 00000000000..9c63632a322 --- /dev/null +++ b/encodings/decimal-byte-parts/tests/lower_parts_gate.rs @@ -0,0 +1,62 @@ +// SPDX-License-Identifier: Apache-2.0 +// SPDX-FileCopyrightText: Copyright the Vortex contributors + +//! The lower-parts write gate, checked from outside the crate. +//! +//! The gate lets the crate's own unit tests through so the multi-part paths stay covered by a +//! default `cargo test`. That bypass keys off `cfg!(test)`, which is false for the library +//! when it is compiled as a dependency of this integration test — so this is the only place +//! the gate's real behaviour can be observed. + +#![expect(clippy::tests_outside_test_module)] + +use vortex_array::ArrayRef; +use vortex_array::IntoArray; +use vortex_array::dtype::DecimalDType; +use vortex_buffer::buffer; +use vortex_decimal_byte_parts::DecimalByteParts; + +fn msp() -> ArrayRef { + buffer![1i64, 2, 3].into_array() +} + +fn lower_part() -> ArrayRef { + buffer![1u64, 2, 3].into_array() +} + +/// A single-child array is the stable shape and is always constructible. +#[test] +fn single_child_is_always_allowed() { + assert!(DecimalByteParts::try_new(msp(), DecimalDType::new(19, 2)).is_ok()); + assert!( + DecimalByteParts::try_new_with_lower_parts(msp(), vec![], DecimalDType::new(19, 2)).is_ok() + ); +} + +#[cfg(not(feature = "unstable_encodings"))] +#[test] +fn lower_parts_rejected_without_the_feature() { + let result = DecimalByteParts::try_new_with_lower_parts( + msp(), + vec![lower_part()], + DecimalDType::new(38, 2), + ); + let err = result.expect_err("expected the write gate to reject"); + assert!( + err.to_string().contains("unstable_encodings"), + "error should name the feature, got: {err}" + ); +} + +#[cfg(feature = "unstable_encodings")] +#[test] +fn lower_parts_allowed_with_the_feature() { + assert!( + DecimalByteParts::try_new_with_lower_parts( + msp(), + vec![lower_part()], + DecimalDType::new(38, 2), + ) + .is_ok() + ); +} diff --git a/vortex-btrblocks/Cargo.toml b/vortex-btrblocks/Cargo.toml index 255d2fbd075..ee0939d6bd2 100644 --- a/vortex-btrblocks/Cargo.toml +++ b/vortex-btrblocks/Cargo.toml @@ -48,7 +48,11 @@ vortex-session = { workspace = true } [features] # This feature enabled unstable encodings for which we don't guarantee stability. -unstable_encodings = ["dep:vortex-onpair", "vortex-zstd?/unstable_encodings"] +unstable_encodings = [ + "dep:vortex-onpair", + "vortex-zstd?/unstable_encodings", + "vortex-decimal-byte-parts/unstable_encodings", +] pco = ["dep:pco", "dep:vortex-pco"] zstd = ["dep:vortex-zstd"] diff --git a/vortex-btrblocks/src/schemes/decimal/mod.rs b/vortex-btrblocks/src/schemes/decimal/mod.rs index cbe32fc7b45..2e6135f1f6f 100644 --- a/vortex-btrblocks/src/schemes/decimal/mod.rs +++ b/vortex-btrblocks/src/schemes/decimal/mod.rs @@ -30,9 +30,11 @@ use crate::SchemeExt; /// Narrows the decimal to the smallest integer type, compresses the underlying primitive, and wraps /// the result in a `DecimalBytePartsArray`. /// -/// Values that stay wider than 64 bits after narrowing are split into a signed most -/// significant part and 64-bit lower parts — one for `i128`, three for `i256` — each of -/// which is compressed independently. +/// With `unstable_encodings`, values that stay wider than 64 bits after narrowing are split +/// into a signed most significant part and 64-bit lower parts — one for `i128`, three for +/// `i256` — each compressed independently. That writes more than one child, which readers +/// predating lower parts cannot open, so without the feature such values are left +/// uncompressed instead. #[derive(Debug, Copy, Clone, PartialEq, Eq)] pub struct DecimalScheme; @@ -49,9 +51,13 @@ impl Scheme for DecimalScheme { vec![DecimalByteParts.id()] } - /// Children: msp=0, lower parts=1..=3. + /// Children: msp=0, and with `unstable_encodings`, lower parts=1..=3. fn num_children(&self) -> usize { - DecimalBytePartsSlots::FIXED_COUNT + MAX_LOWER_PARTS + if cfg!(feature = "unstable_encodings") { + DecimalBytePartsSlots::FIXED_COUNT + MAX_LOWER_PARTS + } else { + DecimalBytePartsSlots::FIXED_COUNT + } } fn expected_compression_ratio( @@ -75,6 +81,13 @@ impl Scheme for DecimalScheme { let decimal = narrowed_decimal(decimal); let parts = split_decimal(&decimal)?; + // Splitting a value too wide for one signed part writes more than one child, which a + // reader predating lower parts cannot open. Until that is stable, leave those values + // as the canonical decimal rather than emitting a file such a reader would reject. + if !parts.lower_parts.is_empty() && !cfg!(feature = "unstable_encodings") { + return Ok(decimal.into_array()); + } + let msp = compressor.compress_child( &parts.msp, &compress_ctx, diff --git a/vortex-btrblocks/src/schemes/decimal/tests.rs b/vortex-btrblocks/src/schemes/decimal/tests.rs index d1a29ba970e..d79a30e3a34 100644 --- a/vortex-btrblocks/src/schemes/decimal/tests.rs +++ b/vortex-btrblocks/src/schemes/decimal/tests.rs @@ -1,12 +1,17 @@ // SPDX-License-Identifier: Apache-2.0 // SPDX-FileCopyrightText: Copyright the Vortex contributors +#[cfg(feature = "unstable_encodings")] use std::iter; use std::sync::LazyLock; +#[cfg(feature = "unstable_encodings")] use rand::RngExt; +#[cfg(feature = "unstable_encodings")] use rand::SeedableRng as _; +#[cfg(feature = "unstable_encodings")] use rand::rngs::StdRng; +#[cfg(feature = "unstable_encodings")] use rstest::rstest; use vortex_array::ArrayRef; use vortex_array::IntoArray; @@ -14,12 +19,14 @@ use vortex_array::VortexSessionExecute; use vortex_array::arrays::DecimalArray; use vortex_array::assert_arrays_eq; use vortex_array::dtype::DecimalDType; +#[cfg(feature = "unstable_encodings")] use vortex_array::dtype::DecimalType; use vortex_array::dtype::i256; use vortex_array::validity::Validity; use vortex_buffer::Buffer; use vortex_decimal_byte_parts::DecimalByteParts; use vortex_decimal_byte_parts::DecimalBytePartsArraySlotsExt; +#[cfg(feature = "unstable_encodings")] use vortex_decimal_byte_parts::MAX_LOWER_PARTS; use vortex_error::VortexExpect; use vortex_error::VortexResult; @@ -33,10 +40,12 @@ static SESSION: LazyLock = LazyLock::new(vortex_array::array_sess /// runs on sampled estimates as it does for real file chunks. const N: usize = 16_384; +#[cfg(feature = "unstable_encodings")] fn ten_pow(exp: u32) -> i256 { i256::from_i128(10).wrapping_pow(exp) } +#[cfg(feature = "unstable_encodings")] /// Deterministic 24-bit noise, so the low part of each value is neither constant nor a /// sequence — the realistic shape for a wide decimal column with a large fixed magnitude. fn noise(seed: u64) -> impl Iterator { @@ -44,6 +53,7 @@ fn noise(seed: u64) -> impl Iterator { iter::repeat_with(move || i128::from(rng.random::() >> 8)) } +#[cfg(feature = "unstable_encodings")] /// `i128`-backed values that need more than 64 bits, so the encoding must carry one lower /// part. fn wide_i128_array(validity: Validity) -> DecimalArray { @@ -52,6 +62,7 @@ fn wide_i128_array(validity: Validity) -> DecimalArray { DecimalArray::new(values, DecimalDType::new(38, 2), validity) } +#[cfg(feature = "unstable_encodings")] /// `i256`-backed values that need more than 128 bits, so the encoding must carry three /// lower parts. fn wide_i256_array(validity: Validity) -> DecimalArray { @@ -84,6 +95,8 @@ fn lower_part_count(array: &ArrayRef) -> usize { .len() } +// Requires the lower-parts write gate: without it the compressor leaves wide values canonical. +#[cfg(feature = "unstable_encodings")] #[rstest] #[case::non_nullable(Validity::NonNullable)] #[case::all_valid(Validity::AllValid)] @@ -98,6 +111,8 @@ fn test_i128_decimal_splits_into_one_lower_part(#[case] validity: Validity) -> V Ok(()) } +// Requires the lower-parts write gate: without it the compressor leaves wide values canonical. +#[cfg(feature = "unstable_encodings")] #[rstest] #[case::non_nullable(Validity::NonNullable)] #[case::all_valid(Validity::AllValid)] @@ -112,6 +127,8 @@ fn test_i256_decimal_splits_into_three_lower_parts(#[case] validity: Validity) - Ok(()) } +// Requires the lower-parts write gate: without it the compressor leaves wide values canonical. +#[cfg(feature = "unstable_encodings")] #[test] fn test_i256_decimal_round_trips_extreme_values() -> VortexResult<()> { // Every 64-bit window exercised, including the sign boundary of the most significant @@ -151,6 +168,8 @@ fn test_narrow_decimal_has_no_lower_parts() -> VortexResult<()> { Ok(()) } +// Requires the lower-parts write gate: without it the compressor leaves wide values canonical. +#[cfg(feature = "unstable_encodings")] #[test] fn test_canonical_of_compressed_wide_decimal_keeps_storage_width() -> VortexResult<()> { let mut ctx = SESSION.create_execution_ctx(); @@ -168,6 +187,8 @@ fn test_canonical_of_compressed_wide_decimal_keeps_storage_width() -> VortexResu /// Splitting exists to make wide decimals compressible: the parts that do not vary collapse /// to constants and the varying part bit-packs. Without splitting these arrays are stored as /// raw 16- and 32-byte values. +// Requires the lower-parts write gate: without it the compressor leaves wide values canonical. +#[cfg(feature = "unstable_encodings")] #[rstest] #[case::i128(wide_i128_array(Validity::NonNullable), 16)] #[case::i256(wide_i256_array(Validity::NonNullable), 32)] diff --git a/vortex-file/src/tests.rs b/vortex-file/src/tests.rs index e80bdedca14..6a0a84f1e38 100644 --- a/vortex-file/src/tests.rs +++ b/vortex-file/src/tests.rs @@ -11,8 +11,11 @@ use flatbuffers::FlatBufferBuilder; use futures::StreamExt; use futures::TryStreamExt; use futures::pin_mut; +#[cfg(feature = "unstable_encodings")] use rand::RngExt; +#[cfg(feature = "unstable_encodings")] use rand::SeedableRng as _; +#[cfg(feature = "unstable_encodings")] use rand::rngs::StdRng; use rstest::rstest; use vortex_array::ArrayRef; @@ -39,6 +42,7 @@ use vortex_array::dtype::Nullability; use vortex_array::dtype::PType; use vortex_array::dtype::PType::I32; use vortex_array::dtype::StructFields; +#[cfg(feature = "unstable_encodings")] use vortex_array::dtype::i256; use vortex_array::expr::and; use vortex_array::expr::cast; @@ -233,6 +237,10 @@ async fn test_round_trip_many_types() { /// End-to-end check that decimals wider than 64 bits survive a write/read round trip and are /// actually compressed: the compressor splits them into a most significant part plus 64-bit /// lower parts, each of which compresses on its own. +/// +/// Requires `unstable_encodings`: the split writes more than one child, so without the +/// feature the compressor deliberately leaves these values canonical and uncompressed. +#[cfg(feature = "unstable_encodings")] #[tokio::test] #[cfg_attr(miri, ignore)] async fn test_wide_decimal_round_trip_compresses() -> VortexResult<()> { diff --git a/vortex-test/compat-gen/Cargo.toml b/vortex-test/compat-gen/Cargo.toml index 4a62aca3671..2a5fe657d9b 100644 --- a/vortex-test/compat-gen/Cargo.toml +++ b/vortex-test/compat-gen/Cargo.toml @@ -20,6 +20,11 @@ name = "vortex-compat" path = "src/main.rs" test = false +[features] +# Fixtures for encodings whose on-disk shape is not yet stable. Kept out of the default +# fixture set so a default build never publishes a file older readers cannot open. +unstable_encodings = ["vortex/unstable_encodings"] + [dependencies] # Vortex crates vortex = { workspace = true, features = ["files", "tokio", "zstd"] } diff --git a/vortex-test/compat-gen/src/fixtures/arrays/synthetic/encodings/mod.rs b/vortex-test/compat-gen/src/fixtures/arrays/synthetic/encodings/mod.rs index 0027570e95c..4e46472a23d 100644 --- a/vortex-test/compat-gen/src/fixtures/arrays/synthetic/encodings/mod.rs +++ b/vortex-test/compat-gen/src/fixtures/arrays/synthetic/encodings/mod.rs @@ -12,6 +12,7 @@ mod bytebool; mod constant; mod datetimeparts; mod decimal_byte_parts; +#[cfg(feature = "unstable_encodings")] mod decimal_byte_parts_wide; mod delta; mod dict; @@ -32,14 +33,14 @@ pub(crate) const N: usize = 1024; /// All per-encoding fixtures. pub fn fixtures() -> Vec> { - vec![ + #[allow(unused_mut)] + let mut fixtures: Vec> = vec![ Box::new(alp::AlpFixture), Box::new(alprd::AlprdFixture), Box::new(bitpacked::BitPackedFixture), Box::new(bytebool::ByteBoolFixture), Box::new(datetimeparts::DateTimePartsFixture), Box::new(decimal_byte_parts::DecimalBytePartsFixture), - Box::new(decimal_byte_parts_wide::DecimalBytePartsWideFixture), // Re-enable this once delta is stable // Box::new(delta::DeltaFixture), Box::new(dict::DictFixture), @@ -55,5 +56,10 @@ pub fn fixtures() -> Vec> { Box::new(zstd::ZstdFixture), Box::new(zigzag::ZigZagFixture), Box::new(constant::ConstantFixture), - ] + ]; + #[cfg(feature = "unstable_encodings")] + fixtures.push(Box::new( + decimal_byte_parts_wide::DecimalBytePartsWideFixture, + )); + fixtures } From 8c5cd415a113fb361c1b52dde3dfc35764d47a2e Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 3 Aug 2026 15:46:34 +0000 Subject: [PATCH 11/14] Refuse to serialize lower parts without unstable_encodings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Gating construction and the compressor was not enough. An array read from a file can be handed straight back to a writer without passing through either: `deserialize` is deliberately ungated so a build without the feature can still read such files, and the write allow-list checks only the encoding id, not how many children it carries — `ALLOWED_ENCODINGS` inserts `DecimalByteParts.id()` unconditionally. A build that could never have constructed a multi-child array could therefore still emit one. This was demonstrable, not theoretical: `test_serde_round_trip` with three lower parts passed on default features before this change. `VTable::serialize` now refuses an array carrying lower parts unless the feature is on. That is the last point before bytes reach a file, so it covers the pass-through path as well as anything else that reaches the writer. Reading stays untouched, and so does compute over an array that already has lower parts. `serializing_read_lower_parts_is_gated` pins it, going through `deserialize` to obtain the array exactly as opening a file would, and asserting the write is refused with an error naming the feature. The three wide `test_serde_round_trip` cases move to a feature-gated variant, since without the feature serializing them is now the refusal being tested. Note this makes a stable build unable to rewrite a wide array it just read, so copying or compacting such a file fails loudly rather than producing something old readers cannot open. That is the intended trade-off while the format is unstable, but it is a behaviour change for read-modify-write on files written with the feature. Signed-off-by: "Joe Isaacs" --- .../src/decimal_byte_parts/mod.rs | 61 +++++++++++++++++-- 1 file changed, 57 insertions(+), 4 deletions(-) diff --git a/encodings/decimal-byte-parts/src/decimal_byte_parts/mod.rs b/encodings/decimal-byte-parts/src/decimal_byte_parts/mod.rs index 7ef4f346cd1..3613dcec652 100644 --- a/encodings/decimal-byte-parts/src/decimal_byte_parts/mod.rs +++ b/encodings/decimal-byte-parts/src/decimal_byte_parts/mod.rs @@ -152,6 +152,18 @@ impl VTable for DecimalByteParts { array: ArrayView<'_, Self>, _session: &VortexSession, ) -> VortexResult>> { + // The last gate before bytes reach a file. Constructing lower parts is already gated, + // but an array read from a file can be handed straight back to a writer without going + // through a constructor or the compressor, and the write allow-list only checks the + // encoding id, not how many children it carries. Without this a build lacking the + // feature could still emit a multi-child array it never could have built. + vortex_ensure!( + array.lower_parts().is_empty() || cfg!(feature = "unstable_encodings"), + "serializing DecimalByteParts with lower parts requires the `unstable_encodings` \ + feature: readers that predate lower parts understand only a single child, and \ + would fail to open a file containing this array" + ); + let lower_part_count = u32::try_from(array.lower_parts().len()) .map_err(|_| vortex_err!("lower part count exceeds u32"))?; Ok(Some( @@ -692,15 +704,29 @@ mod tests { Ok(()) } + /// Serializing lower parts is gated: an array carrying them can only be written to bytes + /// with `unstable_encodings`, so these cases only exist when the feature is on. + #[cfg(feature = "unstable_encodings")] + #[rstest] + #[case::one_lower_part(i128_parts(wide_i128_values(), Validity::NonNullable))] + #[case::three_lower_parts(i256_parts(wide_i256_values(), Validity::NonNullable))] + #[case::nullable_three_lower_parts(i256_parts(wide_i256_values(), Validity::AllValid))] + fn test_serde_round_trip_with_lower_parts( + #[case] array: DecimalBytePartsArray, + ) -> VortexResult<()> { + test_serde_round_trip(array) + } + #[rstest] #[case::no_lower_parts( encode(&DecimalArray::new(buffer![1i32, 2, 3], DecimalDType::new(9, 2), Validity::NonNullable)) .vortex_expect("valid decimal byte parts") )] - #[case::one_lower_part(i128_parts(wide_i128_values(), Validity::NonNullable))] - #[case::three_lower_parts(i256_parts(wide_i256_values(), Validity::NonNullable))] - #[case::nullable_three_lower_parts(i256_parts(wide_i256_values(), Validity::AllValid))] - fn test_serde_round_trip(#[case] array: DecimalBytePartsArray) -> VortexResult<()> { + fn test_serde_round_trip_flat(#[case] array: DecimalBytePartsArray) -> VortexResult<()> { + test_serde_round_trip(array) + } + + fn test_serde_round_trip(array: DecimalBytePartsArray) -> VortexResult<()> { let session = array_session(); session.arrays().register(DecimalByteParts); @@ -799,6 +825,33 @@ mod tests { Ok(()) } + /// An array read from a file can be handed straight back to a writer, bypassing both the + /// constructor and the compressor. Without the feature that write must be refused, or a + /// build that could never have built this array could still emit one. + #[cfg(not(feature = "unstable_encodings"))] + #[test] + fn serializing_read_lower_parts_is_gated() -> VortexResult<()> { + let session = array_session(); + session.arrays().register(DecimalByteParts); + + let array = deserialize_with(1, vec![msp(), lower_part()]) + .and_then(Array::try_from_parts)? + .into_array(); + + let err = array + .serialize( + &ArrayContext::empty(), + &session, + &SerializeOptions::default(), + ) + .expect_err("expected the write gate to refuse"); + assert!( + err.to_string().contains("unstable_encodings"), + "error should name the feature, got: {err}" + ); + Ok(()) + } + /// Reading back an array that already carries lower parts, and computing over it, must /// work regardless of the `unstable_encodings` write gate. The gate stops a writer /// introducing lower parts; if it also blocked the rebuild that every compute kernel does, From becfb9574995d6355a5ad6a2e9d24888829b56bb Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 4 Aug 2026 09:28:40 +0000 Subject: [PATCH 12/14] Add property tests for split/assemble and the compute kernels MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Uses `hegeltest`, the Hypothesis-based property testing crate that `spiraldb/fastlanes` already depends on, so the two repositories share a generator vocabulary and shrinking behaviour. Every property has the same shape: the encoding must be indistinguishable from doing the same thing to the canonical `DecimalArray`. Seven of them cover split/assemble round tripping, a serialize/decode round trip through the file path, per-row `scalar_at` against bulk canonicalization, and filter, slice and take. The properties were checked against deliberate mutations rather than assumed to be load bearing. Six real mutations, all caught: reversing the lower-part order in assembly, placing the MSP one word too low, swapping the `i256` word-pair packing, dropping the lower part on the `i128` path, reversing the order in `split_i256`, and dropping the sign fill. A seventh — logical instead of arithmetic shift in `split_i128` — is an equivalent mutant, since truncating to `i64` makes the shift kind unobservable, and is correctly not flagged. Dropping the sign fill initially survived, which is why `msp_below_the_top_word_sign_extends` exists. `split_decimal` always emits three lower parts for an `i256`, and at three parts every word is written, so the fill is dead on that path — the round-trip properties structurally cannot reach it. Only a directly constructed array with a most significant part below the top word does. That property computes its expectation independently of the assembly loop: with two lower parts the MSP occupies bits 191..128, exactly the low half of an `i256`'s signed `i128` half, so `i128::from` performs the sign extension the encoding is supposed to. The test target declares `required-features = ["unstable_encodings"]`, since building multi-part arrays is what the write gate restricts. Running the properties writes an example database to `.hegel/`, which is generated state rather than source — the same role `.hypothesis/` plays for Python, and ignored alongside it. Signed-off-by: "Joe Isaacs" --- .gitignore | 2 + Cargo.lock | 71 +++++ Cargo.toml | 1 + encodings/decimal-byte-parts/Cargo.toml | 6 + encodings/decimal-byte-parts/tests/props.rs | 335 ++++++++++++++++++++ 5 files changed, 415 insertions(+) create mode 100644 encodings/decimal-byte-parts/tests/props.rs diff --git a/.gitignore b/.gitignore index f9613807332..6db14ce5f6a 100644 --- a/.gitignore +++ b/.gitignore @@ -52,6 +52,8 @@ coverage.xml *.cover *.py,cover .hypothesis/ +# hegeltest's example database, the Rust equivalent of .hypothesis/ +.hegel/ .pytest_cache/ cover/ diff --git a/Cargo.lock b/Cargo.lock index 7353d7a6a72..8448def4c20 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2001,6 +2001,25 @@ dependencies = [ "parking_lot_core", ] +[[package]] +name = "dashu-base" +version = "0.4.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "993b95dc1b248e3f5747dcb017a41d6e75853a2e5ee4504f7d537c5b8dffdae4" + +[[package]] +name = "dashu-int" +version = "0.4.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "49c05a0d5cb0b39fcc87c46432fdac24b90dce239857c7f6b798be4ffc3c42c6" +dependencies = [ + "cfg-if", + "dashu-base", + "num-modular", + "rustversion", + "static_assertions", +] + [[package]] name = "datafusion" version = "54.1.0" @@ -3765,6 +3784,51 @@ version = "0.5.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "2304e00983f87ffb38b55b444b5e3b60a884b5d30c0fca7d82fe33449bbe55ea" +[[package]] +name = "hegeltest" +version = "0.28.7" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "100bcd6ef825f5b6a60e2f55c05bb626ebf254dd8a09d16e006c4bb7883e7f1c" +dependencies = [ + "crc32fast", + "dashu-int", + "hegeltest-c", + "hegeltest-macros", + "miniz_oxide 0.8.9", + "parking_lot", + "paste", + "rand 0.10.2", + "rustc-hash", + "tempfile", +] + +[[package]] +name = "hegeltest-c" +version = "0.30.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "7a672fd53360ca4122c1a145a85e8fef835508d7b40eb9de43498978e796c54b" +dependencies = [ + "dashu-int", + "hashbrown 0.17.1", + "libm", + "miniz_oxide 0.8.9", + "parking_lot", + "rand 0.10.2", + "rustc-hash", + "tempfile", +] + +[[package]] +name = "hegeltest-macros" +version = "0.28.7" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "ba792d78fa3740a7c1627085c34618b998b8aa0f63625721235234f525aad1aa" +dependencies = [ + "proc-macro2", + "quote", + "syn 2.0.119", +] + [[package]] name = "hermit-abi" version = "0.5.2" @@ -5824,6 +5888,12 @@ dependencies = [ "num-traits", ] +[[package]] +name = "num-modular" +version = "0.6.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "fc41a1374056e9672221567958a66c16be12d0e2c1b408761e14d901c237d5e0" + [[package]] name = "num-rational" version = "0.4.2" @@ -9687,6 +9757,7 @@ name = "vortex-decimal-byte-parts" version = "0.1.0" dependencies = [ "codspeed-divan-compat", + "hegeltest", "num-traits", "prost 0.14.4", "rand 0.10.2", diff --git a/Cargo.toml b/Cargo.toml index a9d8bfefb85..cafaef7357e 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -214,6 +214,7 @@ pyo3-bytes = "0.7" pyo3-log = "0.13.0" pyo3-object_store = "0.11.0" quote = "1.0.44" +hegeltest = "0.28.7" rand = "0.10.1" rand_distr = "0.6" ratatui = { version = "0.30", default-features = false } diff --git a/encodings/decimal-byte-parts/Cargo.toml b/encodings/decimal-byte-parts/Cargo.toml index e87c3f8a609..dab29a5002f 100644 --- a/encodings/decimal-byte-parts/Cargo.toml +++ b/encodings/decimal-byte-parts/Cargo.toml @@ -32,10 +32,16 @@ vortex-session = { workspace = true } [dev-dependencies] divan = { workspace = true } +hegeltest = { workspace = true } rand = { workspace = true } rstest = { workspace = true } vortex-array = { path = "../../vortex-array", features = ["_test-harness"] } +[[test]] +name = "props" +# Property tests cover the multi-part paths, which the write gate only permits here. +required-features = ["unstable_encodings"] + [[bench]] name = "decimal_assemble" harness = false diff --git a/encodings/decimal-byte-parts/tests/props.rs b/encodings/decimal-byte-parts/tests/props.rs new file mode 100644 index 00000000000..0d7eb3710bd --- /dev/null +++ b/encodings/decimal-byte-parts/tests/props.rs @@ -0,0 +1,335 @@ +// SPDX-License-Identifier: Apache-2.0 +// SPDX-FileCopyrightText: Copyright the Vortex contributors + +//! Property tests for splitting decimals into byte parts and putting them back together. +//! +//! Every property here is the same shape: whatever the encoding does must be indistinguishable +//! from doing it to the canonical `DecimalArray`. Round tripping covers the split/assemble +//! pair directly; the compute properties cover it indirectly, since each one canonicalizes an +//! encoded array at the end. +//! +//! The generators deliberately reach the cases hand-written tests tend to miss: values that +//! straddle a 64-bit word boundary, negative values whose sign extension fills the words above +//! the most significant part, and null rows whose lower parts hold arbitrary bits. + +#![expect(clippy::tests_outside_test_module)] + +use hegel::TestCase; +use hegel::generators as gs; +use vortex_array::ArrayContext; +use vortex_array::ArrayRef; +use vortex_array::ExecutionCtx; +use vortex_array::IntoArray; +use vortex_array::VortexSessionExecute; +use vortex_array::array_session; +use vortex_array::arrays::DecimalArray; +use vortex_array::arrays::PrimitiveArray; +use vortex_array::assert_arrays_eq; +use vortex_array::dtype::DecimalDType; +use vortex_array::dtype::i256; +use vortex_array::serde::SerializeOptions; +use vortex_array::serde::SerializedArray; +use vortex_array::validity::Validity; +use vortex_buffer::Buffer; +use vortex_buffer::ByteBufferMut; +use vortex_decimal_byte_parts::DecimalByteParts; +use vortex_decimal_byte_parts::DecimalBytePartsArray; +use vortex_decimal_byte_parts::split_decimal; +use vortex_error::VortexExpect; +use vortex_mask::Mask; +use vortex_session::registry::ReadContext; + +/// Largest magnitude a `Decimal(38, _)` can hold: 38 nines. +const MAX_I128: i128 = 10i128.pow(38) - 1; + +/// Bound on the high `i128` half of an `i256` draw. `10^37 * 2^128` is about `3.4e75`, so any +/// value built from it stays inside the 76 digits a `Decimal(76, _)` can hold. +const MAX_I256_HIGH: i128 = 10i128.pow(37); + +/// Rows per generated array. Small enough to shrink usefully, large enough that a chunked or +/// vectorized path is not trivially degenerate. +const MAX_LEN: usize = 48; + +fn ctx() -> ExecutionCtx { + let session = array_session(); + vortex_decimal_byte_parts::initialize(&session); + session.create_execution_ctx() +} + +/// Encode a canonical decimal as byte parts, splitting wide values into lower parts. +fn encode(decimal: &DecimalArray) -> DecimalBytePartsArray { + let parts = split_decimal(decimal).vortex_expect("split"); + DecimalByteParts::try_new_with_lower_parts( + parts.msp, + parts.lower_parts, + decimal.decimal_dtype(), + ) + .vortex_expect("valid byte parts") +} + +/// A validity mask of exactly `len` entries, so null rows exercise lower parts holding bits +/// that must never be read. +fn draw_validity(tc: &TestCase, len: usize) -> Validity { + let valid: Vec = tc.draw(gs::vecs(gs::booleans()).min_size(len).max_size(len)); + Validity::from_iter(valid) +} + +/// An `i128`-backed decimal. The bounds keep values inside `Decimal(38, 2)` while still +/// reaching both sides of the 64-bit word boundary the encoding splits on. +fn draw_i128_decimal(tc: &TestCase) -> DecimalArray { + let values: Vec = tc.draw( + gs::vecs( + gs::integers::() + .min_value(-MAX_I128) + .max_value(MAX_I128), + ) + .min_size(1) + .max_size(MAX_LEN), + ); + let validity = draw_validity(tc, values.len()); + DecimalArray::new(Buffer::from(values), DecimalDType::new(38, 2), validity) +} + +/// An `i256`-backed decimal, built from a signed high half and an unsigned low half so the +/// draw covers sign extension above the most significant part. +fn draw_i256_decimal(tc: &TestCase) -> DecimalArray { + let halves: Vec<(i128, u128)> = tc.draw( + gs::vecs(gs::tuples2( + gs::integers::() + .min_value(-MAX_I256_HIGH) + .max_value(MAX_I256_HIGH), + gs::integers::(), + )) + .min_size(1) + .max_size(MAX_LEN), + ); + let values: Vec = halves + .into_iter() + .map(|(high, low)| i256::from_parts(low, high)) + .collect(); + let validity = draw_validity(tc, values.len()); + DecimalArray::new(Buffer::from(values), DecimalDType::new(76, 2), validity) +} + +fn draw_decimal(tc: &TestCase) -> DecimalArray { + if tc.draw(gs::booleans()) { + draw_i128_decimal(tc) + } else { + draw_i256_decimal(tc) + } +} + +/// Canonicalize an encoded array back to a `DecimalArray`. +fn canonicalize(array: ArrayRef, ctx: &mut ExecutionCtx) -> DecimalArray { + array.execute::(ctx).vortex_expect("execute") +} + +// --------------------------------------------------------------------------------------- +// Split / assemble +// --------------------------------------------------------------------------------------- + +/// Splitting a decimal into byte parts and reassembling it must reproduce it exactly, +/// including null rows and the sign extension above a narrow most significant part. +#[hegel::test] +fn split_then_assemble_is_identity(tc: TestCase) { + let decimal = draw_decimal(&tc); + let mut ctx = ctx(); + + let round_tripped = canonicalize(encode(&decimal).into_array(), &mut ctx); + + assert_eq!(round_tripped.values_type(), decimal.values_type()); + assert_arrays_eq!(decimal, round_tripped, &mut ctx); +} + +/// Serializing an encoded array and reading it back must preserve it. This is the path a file +/// takes, so it covers the metadata carrying the lower part count as well as the buffers. +#[hegel::test] +fn serde_round_trip_preserves_values(tc: TestCase) { + let decimal = draw_decimal(&tc); + let session = array_session(); + vortex_decimal_byte_parts::initialize(&session); + let mut ctx = session.create_execution_ctx(); + + let encoded = encode(&decimal).into_array(); + let dtype = encoded.dtype().clone(); + let len = encoded.len(); + + let array_ctx = ArrayContext::empty(); + let serialized = encoded + .serialize(&array_ctx, &session, &SerializeOptions::default()) + .vortex_expect("serialize"); + let mut concat = ByteBufferMut::empty(); + for buf in serialized { + concat.extend_from_slice(buf.as_ref()); + } + let parts = SerializedArray::try_from(concat.freeze()).vortex_expect("serialized array"); + let decoded = parts + .decode(&dtype, len, &ReadContext::new(array_ctx.to_ids()), &session) + .vortex_expect("decode"); + + assert_arrays_eq!(decimal, canonicalize(decoded, &mut ctx), &mut ctx); +} + +/// Reading one row at a time must agree with canonicalizing the whole array. These are +/// separate implementations — `combine_*` per row against the bulk assembly loops — so they +/// can disagree without any test noticing. +#[hegel::test] +fn scalar_at_agrees_with_canonical(tc: TestCase) { + let decimal = draw_decimal(&tc); + let mut ctx = ctx(); + + let encoded = encode(&decimal).into_array(); + let canonical = decimal.into_array(); + + for index in 0..encoded.len() { + let from_parts = encoded + .execute_scalar(index, &mut ctx) + .vortex_expect("scalar from byte parts"); + let from_canonical = canonical + .execute_scalar(index, &mut ctx) + .vortex_expect("scalar from canonical"); + assert_eq!(from_parts, from_canonical, "row {index}"); + } +} + +// --------------------------------------------------------------------------------------- +// Compute +// --------------------------------------------------------------------------------------- + +/// Filtering the encoded array must match filtering the canonical one. The encoding pushes the +/// filter into every part, so dropping or misaligning one shows up here. +#[hegel::test] +fn filter_matches_canonical(tc: TestCase) { + let decimal = draw_decimal(&tc); + let len = decimal.len(); + let keep: Vec = tc.draw(gs::vecs(gs::booleans()).min_size(len).max_size(len)); + let mut ctx = ctx(); + + let mask = Mask::from_iter(keep); + let expected = canonicalize( + decimal + .clone() + .into_array() + .filter(mask.clone()) + .vortex_expect("filter canonical"), + &mut ctx, + ); + let actual = canonicalize( + encode(&decimal) + .into_array() + .filter(mask) + .vortex_expect("filter byte parts"), + &mut ctx, + ); + + assert_arrays_eq!(expected, actual, &mut ctx); +} + +/// Slicing must match, including slices that start partway through the array — the offsets of +/// every part have to move together. +#[hegel::test] +fn slice_matches_canonical(tc: TestCase) { + let decimal = draw_decimal(&tc); + let len = decimal.len(); + let a = tc.draw(gs::integers::().min_value(0).max_value(len)); + let b = tc.draw(gs::integers::().min_value(0).max_value(len)); + let (start, stop) = if a <= b { (a, b) } else { (b, a) }; + tc.assume(start < stop); + let mut ctx = ctx(); + + let expected = canonicalize( + decimal + .clone() + .into_array() + .slice(start..stop) + .vortex_expect("slice canonical"), + &mut ctx, + ); + let actual = canonicalize( + encode(&decimal) + .into_array() + .slice(start..stop) + .vortex_expect("slice byte parts"), + &mut ctx, + ); + + assert_arrays_eq!(expected, actual, &mut ctx); +} + +/// Taking arbitrary indices must match, including repeats and out-of-order indices. +#[hegel::test] +fn take_matches_canonical(tc: TestCase) { + let decimal = draw_decimal(&tc); + let len = decimal.len(); + let indices: Vec = tc.draw( + gs::vecs( + gs::integers::() + .min_value(0) + .max_value((len - 1) as u64), + ) + .min_size(1) + .max_size(MAX_LEN), + ); + let mut ctx = ctx(); + + let indices = PrimitiveArray::new(Buffer::from(indices), Validity::NonNullable).into_array(); + let expected = canonicalize( + decimal + .clone() + .into_array() + .take(indices.clone()) + .vortex_expect("take canonical"), + &mut ctx, + ); + let actual = canonicalize( + encode(&decimal) + .into_array() + .take(indices) + .vortex_expect("take byte parts"), + &mut ctx, + ); + + assert_arrays_eq!(expected, actual, &mut ctx); +} + +/// A most significant part sitting below the top word must sign-extend into the words above +/// it. `split_decimal` never produces this shape — it always fills all three lower parts, so +/// every word is written and the sign fill is dead — which means the round-trip properties +/// above cannot see it. Only a directly constructed array reaches it. +/// +/// The expectation is computed independently of the assembly loop: with two lower parts the +/// MSP occupies bits 191..128, which is exactly the low half of an `i256`'s signed `i128` +/// half, so widening it with `i128::from` performs the sign extension the encoding must. +#[hegel::test] +fn msp_below_the_top_word_sign_extends(tc: TestCase) { + let msp: Vec = tc.draw( + gs::vecs(gs::integers::()) + .min_size(1) + .max_size(MAX_LEN), + ); + let len = msp.len(); + let high: Vec = tc.draw(gs::vecs(gs::integers::()).min_size(len).max_size(len)); + let low: Vec = tc.draw(gs::vecs(gs::integers::()).min_size(len).max_size(len)); + let mut ctx = ctx(); + + let array = DecimalByteParts::try_new_with_lower_parts( + PrimitiveArray::new(Buffer::from(msp.clone()), Validity::NonNullable).into_array(), + vec![ + PrimitiveArray::new(Buffer::from(high.clone()), Validity::NonNullable).into_array(), + PrimitiveArray::new(Buffer::from(low.clone()), Validity::NonNullable).into_array(), + ], + DecimalDType::new(76, 2), + ) + .vortex_expect("two lower parts under an i64 msp"); + + let canonical = canonicalize(array.into_array(), &mut ctx); + let actual = canonical.buffer::(); + + for row in 0..len { + let expected = i256::from_parts( + u128::from(low[row]) | (u128::from(high[row]) << 64), + i128::from(msp[row]), + ); + assert_eq!(actual[row], expected, "row {row}, msp {}", msp[row]); + } +} From b114238b7b1f519cdf8d1ea8019844099e603017 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 4 Aug 2026 10:43:54 +0000 Subject: [PATCH 13/14] Reduce the properties to encode/decode round trips, both directions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two properties remain, one starting from each side. Generating a decoded decimal and checking encode-then-decode reproduces it covers what `split_decimal` emits. Generating an encoded array directly and checking decode-then-encode preserves the values it decodes to reaches layouts `split_decimal` never produces — it only ever emits 0, 1 or 3 lower parts under an `i64` most significant part, so drawing the part count is the only way to reach the two-part shape. The second compares decoded values rather than the arrays, because re-encoding normalizes the part count: splitting an `i256` always yields three lower parts whatever the original carried. The removed properties are recorded as a TODO rather than dropped silently, since some of them caught mutations these two do not. Verified rather than assumed: reversing the lower-part order and placing the MSP one word too low are still caught, but dropping the sign fill in `sign_extended_words` now survives both. A round trip compares decode against decode, so a decode-side sign-extension bug is invisible to it — catching that needs an oracle computed independently of the assembly loop, which is what the removed property had. The TODO says so explicitly. Also pins the one-limb invariant against the last way of reaching it. `ArrayParts` is public and `DecimalBytePartsData` is a public unit struct, so slots can be assembled by hand and passed to `Array::try_from_parts`, bypassing the gated constructor. That path stays open deliberately — it is the shape a file read produces, and closing it would stop a build without the feature reading a file written by one with it — but `hand_assembled_lower_parts_cannot_be_serialized` pins that such an array can never be turned back into bytes. So without `unstable_encodings`: `try_new` builds one limb, `try_new_with_lower_parts` refuses more, the compressor declines to split, and anything holding more than one limb — however it was obtained — cannot be serialized. Signed-off-by: "Joe Isaacs" --- .../tests/lower_parts_gate.rs | 57 ++++ encodings/decimal-byte-parts/tests/props.rs | 258 +++++------------- 2 files changed, 118 insertions(+), 197 deletions(-) diff --git a/encodings/decimal-byte-parts/tests/lower_parts_gate.rs b/encodings/decimal-byte-parts/tests/lower_parts_gate.rs index 9c63632a322..fb6f80f1a03 100644 --- a/encodings/decimal-byte-parts/tests/lower_parts_gate.rs +++ b/encodings/decimal-byte-parts/tests/lower_parts_gate.rs @@ -60,3 +60,60 @@ fn lower_parts_allowed_with_the_feature() { .is_ok() ); } + +/// The gate must hold for *every* way of getting an array with more than one limb, not just +/// the public constructor. `ArrayParts` is public and `DecimalBytePartsData` is a public unit +/// struct, so a caller can assemble slots by hand and go straight to `Array::try_from_parts`, +/// bypassing `try_new_with_lower_parts` entirely. +/// +/// That back door is left open on purpose — it is the same path `deserialize` uses, and +/// closing it would stop a build without the feature reading a file written by one with it. +/// What must hold is that such an array can never be turned back into bytes. +#[cfg(not(feature = "unstable_encodings"))] +#[test] +fn hand_assembled_lower_parts_cannot_be_serialized() { + use vortex_array::Array; + use vortex_array::ArrayContext; + use vortex_array::ArrayParts; + use vortex_array::ArraySlots; + use vortex_array::dtype::DType; + use vortex_array::dtype::Nullability; + use vortex_array::serde::SerializeOptions; + use vortex_array::session::ArraySessionExt; + use vortex_decimal_byte_parts::DecimalBytePartsData; + use vortex_error::VortexExpect; + + let session = vortex_array::array_session(); + session.arrays().register(DecimalByteParts); + + let mut slots = ArraySlots::with_capacity(2); + slots.push(Some(msp())); + slots.push(Some(lower_part())); + + // Assembling the array by hand succeeds: this is the shape a file read produces. + let array = Array::try_from_parts( + ArrayParts::new( + DecimalByteParts, + DType::Decimal(DecimalDType::new(38, 2), Nullability::NonNullable), + 3, + DecimalBytePartsData, + ) + .with_slots(slots), + ) + .vortex_expect("hand assembly is not gated") + .into_array(); + assert_eq!(array.nchildren(), 2, "expected two limbs"); + + // Writing it out does not. + let err = array + .serialize( + &ArrayContext::empty(), + &session, + &SerializeOptions::default(), + ) + .expect_err("expected the write gate to refuse"); + assert!( + err.to_string().contains("unstable_encodings"), + "error should name the feature, got: {err}" + ); +} diff --git a/encodings/decimal-byte-parts/tests/props.rs b/encodings/decimal-byte-parts/tests/props.rs index 0d7eb3710bd..583dc17e103 100644 --- a/encodings/decimal-byte-parts/tests/props.rs +++ b/encodings/decimal-byte-parts/tests/props.rs @@ -16,7 +16,6 @@ use hegel::TestCase; use hegel::generators as gs; -use vortex_array::ArrayContext; use vortex_array::ArrayRef; use vortex_array::ExecutionCtx; use vortex_array::IntoArray; @@ -27,17 +26,12 @@ use vortex_array::arrays::PrimitiveArray; use vortex_array::assert_arrays_eq; use vortex_array::dtype::DecimalDType; use vortex_array::dtype::i256; -use vortex_array::serde::SerializeOptions; -use vortex_array::serde::SerializedArray; use vortex_array::validity::Validity; use vortex_buffer::Buffer; -use vortex_buffer::ByteBufferMut; use vortex_decimal_byte_parts::DecimalByteParts; use vortex_decimal_byte_parts::DecimalBytePartsArray; use vortex_decimal_byte_parts::split_decimal; use vortex_error::VortexExpect; -use vortex_mask::Mask; -use vortex_session::registry::ReadContext; /// Largest magnitude a `Decimal(38, _)` can hold: 38 nines. const MAX_I128: i128 = 10i128.pow(38) - 1; @@ -124,212 +118,82 @@ fn canonicalize(array: ArrayRef, ctx: &mut ExecutionCtx) -> DecimalArray { array.execute::(ctx).vortex_expect("execute") } -// --------------------------------------------------------------------------------------- -// Split / assemble -// --------------------------------------------------------------------------------------- - -/// Splitting a decimal into byte parts and reassembling it must reproduce it exactly, -/// including null rows and the sign extension above a narrow most significant part. -#[hegel::test] -fn split_then_assemble_is_identity(tc: TestCase) { - let decimal = draw_decimal(&tc); - let mut ctx = ctx(); - - let round_tripped = canonicalize(encode(&decimal).into_array(), &mut ctx); - - assert_eq!(round_tripped.values_type(), decimal.values_type()); - assert_arrays_eq!(decimal, round_tripped, &mut ctx); -} - -/// Serializing an encoded array and reading it back must preserve it. This is the path a file -/// takes, so it covers the metadata carrying the lower part count as well as the buffers. -#[hegel::test] -fn serde_round_trip_preserves_values(tc: TestCase) { - let decimal = draw_decimal(&tc); - let session = array_session(); - vortex_decimal_byte_parts::initialize(&session); - let mut ctx = session.create_execution_ctx(); - - let encoded = encode(&decimal).into_array(); - let dtype = encoded.dtype().clone(); - let len = encoded.len(); - - let array_ctx = ArrayContext::empty(); - let serialized = encoded - .serialize(&array_ctx, &session, &SerializeOptions::default()) - .vortex_expect("serialize"); - let mut concat = ByteBufferMut::empty(); - for buf in serialized { - concat.extend_from_slice(buf.as_ref()); - } - let parts = SerializedArray::try_from(concat.freeze()).vortex_expect("serialized array"); - let decoded = parts - .decode(&dtype, len, &ReadContext::new(array_ctx.to_ids()), &session) - .vortex_expect("decode"); - - assert_arrays_eq!(decimal, canonicalize(decoded, &mut ctx), &mut ctx); -} - -/// Reading one row at a time must agree with canonicalizing the whole array. These are -/// separate implementations — `combine_*` per row against the bulk assembly loops — so they -/// can disagree without any test noticing. -#[hegel::test] -fn scalar_at_agrees_with_canonical(tc: TestCase) { - let decimal = draw_decimal(&tc); - let mut ctx = ctx(); - - let encoded = encode(&decimal).into_array(); - let canonical = decimal.into_array(); - - for index in 0..encoded.len() { - let from_parts = encoded - .execute_scalar(index, &mut ctx) - .vortex_expect("scalar from byte parts"); - let from_canonical = canonical - .execute_scalar(index, &mut ctx) - .vortex_expect("scalar from canonical"); - assert_eq!(from_parts, from_canonical, "row {index}"); - } -} - -// --------------------------------------------------------------------------------------- -// Compute -// --------------------------------------------------------------------------------------- - -/// Filtering the encoded array must match filtering the canonical one. The encoding pushes the -/// filter into every part, so dropping or misaligning one shows up here. -#[hegel::test] -fn filter_matches_canonical(tc: TestCase) { - let decimal = draw_decimal(&tc); - let len = decimal.len(); - let keep: Vec = tc.draw(gs::vecs(gs::booleans()).min_size(len).max_size(len)); - let mut ctx = ctx(); - - let mask = Mask::from_iter(keep); - let expected = canonicalize( - decimal - .clone() - .into_array() - .filter(mask.clone()) - .vortex_expect("filter canonical"), - &mut ctx, - ); - let actual = canonicalize( - encode(&decimal) - .into_array() - .filter(mask) - .vortex_expect("filter byte parts"), - &mut ctx, +/// A byte-parts array built directly from drawn parts, rather than by splitting a decimal. +/// +/// `split_decimal` only ever emits 0, 1 or 3 lower parts under an `i64` most significant +/// part, so drawing the part count here is the only way to reach the two-part shape and the +/// sign extension that sits above a most significant part below the top word. +fn draw_encoded(tc: &TestCase) -> (DecimalBytePartsArray, usize) { + let lower_part_count = tc.draw(gs::integers::().min_value(0).max_value(3)); + let msp: Vec = tc.draw( + gs::vecs(gs::integers::()) + .min_size(1) + .max_size(MAX_LEN), ); + let len = msp.len(); - assert_arrays_eq!(expected, actual, &mut ctx); -} - -/// Slicing must match, including slices that start partway through the array — the offsets of -/// every part have to move together. -#[hegel::test] -fn slice_matches_canonical(tc: TestCase) { - let decimal = draw_decimal(&tc); - let len = decimal.len(); - let a = tc.draw(gs::integers::().min_value(0).max_value(len)); - let b = tc.draw(gs::integers::().min_value(0).max_value(len)); - let (start, stop) = if a <= b { (a, b) } else { (b, a) }; - tc.assume(start < stop); - let mut ctx = ctx(); - - let expected = canonicalize( - decimal - .clone() - .into_array() - .slice(start..stop) - .vortex_expect("slice canonical"), - &mut ctx, - ); - let actual = canonicalize( - encode(&decimal) - .into_array() - .slice(start..stop) - .vortex_expect("slice byte parts"), - &mut ctx, - ); + let lower: Vec = (0..lower_part_count) + .map(|_| { + let part: Vec = + tc.draw(gs::vecs(gs::integers::()).min_size(len).max_size(len)); + PrimitiveArray::new(Buffer::from(part), Validity::NonNullable).into_array() + }) + .collect(); - assert_arrays_eq!(expected, actual, &mut ctx); + // The declared precision must be wide enough for what the parts assemble into. + let precision = match lower_part_count { + 0 => 18, + 1 => 38, + _ => 76, + }; + let msp = PrimitiveArray::new(Buffer::from(msp), draw_validity(tc, len)).into_array(); + let array = + DecimalByteParts::try_new_with_lower_parts(msp, lower, DecimalDType::new(precision, 2)) + .vortex_expect("valid byte parts"); + (array, len) } -/// Taking arbitrary indices must match, including repeats and out-of-order indices. +/// Encoding a decimal and decoding it again must reproduce it exactly, including null rows +/// and the storage width. #[hegel::test] -fn take_matches_canonical(tc: TestCase) { +fn decoded_survives_encode_then_decode(tc: TestCase) { let decimal = draw_decimal(&tc); - let len = decimal.len(); - let indices: Vec = tc.draw( - gs::vecs( - gs::integers::() - .min_value(0) - .max_value((len - 1) as u64), - ) - .min_size(1) - .max_size(MAX_LEN), - ); let mut ctx = ctx(); - let indices = PrimitiveArray::new(Buffer::from(indices), Validity::NonNullable).into_array(); - let expected = canonicalize( - decimal - .clone() - .into_array() - .take(indices.clone()) - .vortex_expect("take canonical"), - &mut ctx, - ); - let actual = canonicalize( - encode(&decimal) - .into_array() - .take(indices) - .vortex_expect("take byte parts"), - &mut ctx, - ); + let round_tripped = canonicalize(encode(&decimal).into_array(), &mut ctx); - assert_arrays_eq!(expected, actual, &mut ctx); + assert_eq!(round_tripped.values_type(), decimal.values_type()); + assert_arrays_eq!(decimal, round_tripped, &mut ctx); } -/// A most significant part sitting below the top word must sign-extend into the words above -/// it. `split_decimal` never produces this shape — it always fills all three lower parts, so -/// every word is written and the sign fill is dead — which means the round-trip properties -/// above cannot see it. Only a directly constructed array reaches it. +/// Decoding an encoded array and encoding it again must not change the values it decodes to. /// -/// The expectation is computed independently of the assembly loop: with two lower parts the -/// MSP occupies bits 191..128, which is exactly the low half of an `i256`'s signed `i128` -/// half, so widening it with `i128::from` performs the sign extension the encoding must. +/// Starting from the encoded side reaches part counts `split_decimal` never produces, so this +/// covers layouts the property above cannot generate. It compares decoded values rather than +/// the arrays themselves because re-encoding normalizes the part count: splitting an `i256` +/// always yields three lower parts, whatever the original array carried. #[hegel::test] -fn msp_below_the_top_word_sign_extends(tc: TestCase) { - let msp: Vec = tc.draw( - gs::vecs(gs::integers::()) - .min_size(1) - .max_size(MAX_LEN), - ); - let len = msp.len(); - let high: Vec = tc.draw(gs::vecs(gs::integers::()).min_size(len).max_size(len)); - let low: Vec = tc.draw(gs::vecs(gs::integers::()).min_size(len).max_size(len)); +fn encoded_survives_decode_then_encode(tc: TestCase) { + let (array, _len) = draw_encoded(&tc); let mut ctx = ctx(); - let array = DecimalByteParts::try_new_with_lower_parts( - PrimitiveArray::new(Buffer::from(msp.clone()), Validity::NonNullable).into_array(), - vec![ - PrimitiveArray::new(Buffer::from(high.clone()), Validity::NonNullable).into_array(), - PrimitiveArray::new(Buffer::from(low.clone()), Validity::NonNullable).into_array(), - ], - DecimalDType::new(76, 2), - ) - .vortex_expect("two lower parts under an i64 msp"); - - let canonical = canonicalize(array.into_array(), &mut ctx); - let actual = canonical.buffer::(); + let decoded = canonicalize(array.into_array(), &mut ctx); + let re_decoded = canonicalize(encode(&decoded).into_array(), &mut ctx); - for row in 0..len { - let expected = i256::from_parts( - u128::from(low[row]) | (u128::from(high[row]) << 64), - i128::from(msp[row]), - ); - assert_eq!(actual[row], expected, "row {row}, msp {}", msp[row]); - } + assert_arrays_eq!(decoded, re_decoded, &mut ctx); } + +// TODO(joe): restore the coverage removed alongside these two round trips. Each of the +// following was a property here and caught mutations that the round trips do not: +// +// - `scalar_at` against bulk canonicalization. `combine_i128`/`combine_i256` are a second +// implementation of the assembly loops and can drift from them silently. +// - filter, slice and take against the same operation on the canonical array. These caught +// part-order and word-placement mutations, though the round trips catch those too. +// - a serialize/decode round trip, which is the only property that exercised the metadata +// carrying the lower part count. +// - sign extension above a most significant part below the top word, checked against an +// expectation computed independently of the assembly loop. This is the one real gap: a +// round trip compares decode against decode, so a decode-side sign-extension bug is +// invisible to it. Dropping the sign fill in `sign_extended_words` is caught by neither +// property here. From 1975c0cc7aa325146b7bea4013f32944825c51b1 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 4 Aug 2026 12:42:44 +0000 Subject: [PATCH 14/14] Limit the decimal compressor to narrow values, gate only serialization MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three changes that move the lower-parts restriction to where it belongs. The compressor now only handles decimals that fit a single signed part. Anything still wider than 64 bits after narrowing is left as the canonical decimal, unconditionally — this is a property of the width, not of a feature, so `num_children` is back to 1 and the lower-part compression loop is gone. That restores the pre-lower-parts behaviour permanently rather than behind a flag. `serialize` keeps refusing an array carrying lower parts without `unstable_encodings`, and is now the only gate. It is the right place for it: the write allow-list checks the encoding id rather than the child count, so this is the single point every path to a file passes through, whether the array came from a constructor, a compute kernel, hand-assembled slots, or a previous read. The constructor limb check is removed. Building lower parts in memory is allowed again, which is what reading a file needs anyway, so `rebuild_with_lower_parts` — which existed only to bypass that check — disappears with it; `map_parts`, `with_msp` and the test helpers go back to the public constructor. Everything else `validate` enforces is untouched: signedness, dtypes, lengths, the part-count bound, and the width-against- precision check all still apply. Because the compressor no longer splits, the btrblocks tests asserting one and three lower parts are replaced by one asserting wide values are left canonical, and the compression-ratio test is dropped — it measured the win from splitting, which no longer happens. The vortex-file round trip loses its ratio assertion for the same reason but is now ungated, so wide decimals are covered by a default `cargo test` instead of only under the feature. The property tests and the benchmark no longer need the feature either, since neither serializes. `unstable_encodings` now reaches the encoding through `vortex` and `vortex-file` rather than `vortex-btrblocks`, which no longer has an opinion about lower parts. Verified end to end: a default `generate` produces 35 fixtures, and 36 with the feature, `check --mode exact` passing in both. Signed-off-by: "Joe Isaacs" --- encodings/decimal-byte-parts/Cargo.toml | 11 +-- .../src/decimal_byte_parts/mod.rs | 56 +++++--------- .../src/decimal_byte_parts/testing.rs | 12 +-- .../tests/lower_parts_gate.rs | 29 ++----- vortex-btrblocks/Cargo.toml | 1 - vortex-btrblocks/src/schemes/decimal/mod.rs | 43 +++-------- vortex-btrblocks/src/schemes/decimal/tests.rs | 76 ++++--------------- vortex-file/Cargo.toml | 1 + vortex-file/src/tests.rs | 25 ++---- vortex/Cargo.toml | 1 + 10 files changed, 63 insertions(+), 192 deletions(-) diff --git a/encodings/decimal-byte-parts/Cargo.toml b/encodings/decimal-byte-parts/Cargo.toml index dab29a5002f..9ec8c7cf72e 100644 --- a/encodings/decimal-byte-parts/Cargo.toml +++ b/encodings/decimal-byte-parts/Cargo.toml @@ -17,8 +17,8 @@ version = { workspace = true } workspace = true [features] -# Lower parts make this encoding write more than one child, which readers that predate them -# cannot open. Gated until enough readers understand it. +# Serializing more than one child, which readers that predate lower parts cannot open. +# Building and reading them is always allowed; only writing is gated. unstable_encodings = [] [dependencies] @@ -37,13 +37,6 @@ rand = { workspace = true } rstest = { workspace = true } vortex-array = { path = "../../vortex-array", features = ["_test-harness"] } -[[test]] -name = "props" -# Property tests cover the multi-part paths, which the write gate only permits here. -required-features = ["unstable_encodings"] - [[bench]] name = "decimal_assemble" harness = false -# Builds multi-part arrays, which the write gate only permits with this feature. -required-features = ["unstable_encodings"] diff --git a/encodings/decimal-byte-parts/src/decimal_byte_parts/mod.rs b/encodings/decimal-byte-parts/src/decimal_byte_parts/mod.rs index 3613dcec652..873f8142bd5 100644 --- a/encodings/decimal-byte-parts/src/decimal_byte_parts/mod.rs +++ b/encodings/decimal-byte-parts/src/decimal_byte_parts/mod.rs @@ -152,11 +152,10 @@ impl VTable for DecimalByteParts { array: ArrayView<'_, Self>, _session: &VortexSession, ) -> VortexResult>> { - // The last gate before bytes reach a file. Constructing lower parts is already gated, - // but an array read from a file can be handed straight back to a writer without going - // through a constructor or the compressor, and the write allow-list only checks the - // encoding id, not how many children it carries. Without this a build lacking the - // feature could still emit a multi-child array it never could have built. + // The only gate on lower parts reaching a file. Building them in memory is allowed — + // reading a file requires it — and the write allow-list checks only the encoding id, + // not how many children it carries, so this is the single point where a multi-child + // array can be refused before it becomes bytes. vortex_ensure!( array.lower_parts().is_empty() || cfg!(feature = "unstable_encodings"), "serializing DecimalByteParts with lower parts requires the `unstable_encodings` \ @@ -358,14 +357,12 @@ impl DecimalByteParts { // This gate is on *introducing* lower parts only. Reading them back, and rebuilding an // array that already has them, go through `rebuild_with_lower_parts` and stay ungated — // otherwise a build without the feature could not read a file written by one with it. - vortex_ensure!( - lower_parts.is_empty() || cfg!(feature = "unstable_encodings"), - "DecimalByteParts with lower parts requires the `unstable_encodings` feature: \ - readers that predate lower parts understand only a single child, and would fail \ - to open a file containing this array" - ); - - rebuild_with_lower_parts(msp, lower_parts, decimal_dtype) + let len = msp.len(); + let dtype = DType::Decimal(decimal_dtype, msp.dtype().nullability()); + let slots = DecimalBytePartsSlots { msp, lower_parts }.into_slots(); + Array::try_from_parts( + ArrayParts::new(DecimalByteParts, dtype, len, DecimalBytePartsData).with_slots(slots), + ) } } @@ -374,25 +371,6 @@ fn values_type(array: ArrayView<'_, DecimalByteParts>) -> VortexResult, - decimal_dtype: DecimalDType, -) -> VortexResult { - let len = msp.len(); - let dtype = DType::Decimal(decimal_dtype, msp.dtype().nullability()); - let slots = DecimalBytePartsSlots { msp, lower_parts }.into_slots(); - Array::try_from_parts( - ArrayParts::new(DecimalByteParts, dtype, len, DecimalBytePartsData).with_slots(slots), - ) -} - /// The decimal dtype this array carries. /// /// Guaranteed to be a decimal by construction: [`DecimalBytePartsData::validate`] rejects @@ -419,7 +397,7 @@ pub(crate) fn map_parts( .iter() .map(&mut f) .collect::>>()?; - rebuild_with_lower_parts(msp, lower_parts, decimal_dtype(array)) + DecimalByteParts::try_new_with_lower_parts(msp, lower_parts, decimal_dtype(array)) } /// Rebuild the array with a replacement MSP, keeping its lower parts untouched. @@ -432,7 +410,7 @@ pub(crate) fn with_msp( msp: ArrayRef, decimal_dtype: DecimalDType, ) -> VortexResult { - rebuild_with_lower_parts(msp, array.lower_parts().to_vec(), decimal_dtype) + DecimalByteParts::try_new_with_lower_parts(msp, array.lower_parts().to_vec(), decimal_dtype) } /// Converts a DecimalBytePartsArray to its canonical DecimalArray representation. @@ -789,7 +767,9 @@ mod tests { #[case] lower_parts: Vec, #[case] decimal_dtype: DecimalDType, ) { - assert!(rebuild_with_lower_parts(msp(), lower_parts, decimal_dtype).is_err()); + assert!( + DecimalByteParts::try_new_with_lower_parts(msp(), lower_parts, decimal_dtype).is_err() + ); } fn deserialize_with( @@ -917,7 +897,7 @@ mod tests { assert_eq!(canonical.values_type(), DecimalType::I256); // A narrow MSP with a single lower part still fits 128 bits. - let array = rebuild_with_lower_parts( + let array = DecimalByteParts::try_new_with_lower_parts( buffer![1i8, -1, 0].into_array(), vec![buffer![7u64, 7, 7].into_array()], DecimalDType::new(38, 2), @@ -930,7 +910,7 @@ mod tests { ); // Two lower parts under a narrow MSP overflow 128 bits, so the value widens. - let array = rebuild_with_lower_parts( + let array = DecimalByteParts::try_new_with_lower_parts( buffer![1i8].into_array(), vec![buffer![0u64].into_array(), buffer![9u64].into_array()], DecimalDType::new(76, 2), @@ -944,7 +924,7 @@ mod tests { #[test] fn test_unused_buffer_of_values_is_ignored_for_null_rows() -> VortexResult<()> { // Null rows may hold arbitrary bits in the lower parts; they must stay null. - let array = rebuild_with_lower_parts( + let array = DecimalByteParts::try_new_with_lower_parts( PrimitiveArray::new( buffer![0i64, 0, 0], Validity::Array(BoolArray::from_iter([false, false, true]).into_array()), diff --git a/encodings/decimal-byte-parts/src/decimal_byte_parts/testing.rs b/encodings/decimal-byte-parts/src/decimal_byte_parts/testing.rs index 234e66b0557..bdee6df36a8 100644 --- a/encodings/decimal-byte-parts/src/decimal_byte_parts/testing.rs +++ b/encodings/decimal-byte-parts/src/decimal_byte_parts/testing.rs @@ -11,18 +11,18 @@ use vortex_buffer::Buffer; use vortex_error::VortexExpect; use vortex_error::VortexResult; +use crate::DecimalByteParts; use crate::DecimalBytePartsArray; use crate::decimal_byte_parts::limbs::split_decimal; -use crate::decimal_byte_parts::rebuild_with_lower_parts; /// Encode a canonical decimal array as byte parts, splitting wide values into lower parts. -/// -/// Goes through [`rebuild_with_lower_parts`] rather than the public constructor so these -/// helpers exercise the multi-part paths under a default `cargo test`, independent of the -/// `unstable_encodings` write gate. The gate itself is covered by `tests/lower_parts_gate.rs`. pub(crate) fn encode(decimal: &DecimalArray) -> VortexResult { let parts = split_decimal(decimal)?; - rebuild_with_lower_parts(parts.msp, parts.lower_parts, decimal.decimal_dtype()) + DecimalByteParts::try_new_with_lower_parts( + parts.msp, + parts.lower_parts, + decimal.decimal_dtype(), + ) } /// An `i128`-backed decimal array, encoded as byte parts with one lower part. diff --git a/encodings/decimal-byte-parts/tests/lower_parts_gate.rs b/encodings/decimal-byte-parts/tests/lower_parts_gate.rs index fb6f80f1a03..dff96ebfbbe 100644 --- a/encodings/decimal-byte-parts/tests/lower_parts_gate.rs +++ b/encodings/decimal-byte-parts/tests/lower_parts_gate.rs @@ -1,12 +1,11 @@ // SPDX-License-Identifier: Apache-2.0 // SPDX-FileCopyrightText: Copyright the Vortex contributors -//! The lower-parts write gate, checked from outside the crate. +//! The lower-parts write gate. //! -//! The gate lets the crate's own unit tests through so the multi-part paths stay covered by a -//! default `cargo test`. That bypass keys off `cfg!(test)`, which is false for the library -//! when it is compiled as a dependency of this integration test — so this is the only place -//! the gate's real behaviour can be observed. +//! Lower parts can be built and computed over freely; what `unstable_encodings` gates is +//! turning them back into bytes. These tests pin both halves of that: construction always +//! works, and serialization refuses however the array was obtained. #![expect(clippy::tests_outside_test_module)] @@ -33,24 +32,10 @@ fn single_child_is_always_allowed() { ); } -#[cfg(not(feature = "unstable_encodings"))] -#[test] -fn lower_parts_rejected_without_the_feature() { - let result = DecimalByteParts::try_new_with_lower_parts( - msp(), - vec![lower_part()], - DecimalDType::new(38, 2), - ); - let err = result.expect_err("expected the write gate to reject"); - assert!( - err.to_string().contains("unstable_encodings"), - "error should name the feature, got: {err}" - ); -} - -#[cfg(feature = "unstable_encodings")] +/// Building lower parts in memory is always allowed — reading a file requires it. The gate is +/// on serialization alone. #[test] -fn lower_parts_allowed_with_the_feature() { +fn lower_parts_can_always_be_constructed() { assert!( DecimalByteParts::try_new_with_lower_parts( msp(), diff --git a/vortex-btrblocks/Cargo.toml b/vortex-btrblocks/Cargo.toml index ee0939d6bd2..dffe275fb0e 100644 --- a/vortex-btrblocks/Cargo.toml +++ b/vortex-btrblocks/Cargo.toml @@ -51,7 +51,6 @@ vortex-session = { workspace = true } unstable_encodings = [ "dep:vortex-onpair", "vortex-zstd?/unstable_encodings", - "vortex-decimal-byte-parts/unstable_encodings", ] pco = ["dep:pco", "dep:vortex-pco"] zstd = ["dep:vortex-zstd"] diff --git a/vortex-btrblocks/src/schemes/decimal/mod.rs b/vortex-btrblocks/src/schemes/decimal/mod.rs index 2e6135f1f6f..1cf5f633cdb 100644 --- a/vortex-btrblocks/src/schemes/decimal/mod.rs +++ b/vortex-btrblocks/src/schemes/decimal/mod.rs @@ -15,7 +15,6 @@ use vortex_compressor::scheme::CompressionEstimate; use vortex_compressor::scheme::EstimateVerdict; use vortex_decimal_byte_parts::DecimalByteParts; use vortex_decimal_byte_parts::DecimalBytePartsSlots; -use vortex_decimal_byte_parts::MAX_LOWER_PARTS; use vortex_decimal_byte_parts::split_decimal; use vortex_error::VortexResult; @@ -30,11 +29,9 @@ use crate::SchemeExt; /// Narrows the decimal to the smallest integer type, compresses the underlying primitive, and wraps /// the result in a `DecimalBytePartsArray`. /// -/// With `unstable_encodings`, values that stay wider than 64 bits after narrowing are split -/// into a signed most significant part and 64-bit lower parts — one for `i128`, three for -/// `i256` — each compressed independently. That writes more than one child, which readers -/// predating lower parts cannot open, so without the feature such values are left -/// uncompressed instead. +/// Only decimals that fit a single signed part are compressed. Anything still wider than 64 +/// bits after narrowing would need lower parts, which cannot be serialized, so those are left +/// as the canonical decimal. #[derive(Debug, Copy, Clone, PartialEq, Eq)] pub struct DecimalScheme; @@ -51,13 +48,9 @@ impl Scheme for DecimalScheme { vec![DecimalByteParts.id()] } - /// Children: msp=0, and with `unstable_encodings`, lower parts=1..=3. + /// Children: msp=0. This scheme never emits lower parts. fn num_children(&self) -> usize { - if cfg!(feature = "unstable_encodings") { - DecimalBytePartsSlots::FIXED_COUNT + MAX_LOWER_PARTS - } else { - DecimalBytePartsSlots::FIXED_COUNT - } + DecimalBytePartsSlots::FIXED_COUNT } fn expected_compression_ratio( @@ -81,10 +74,10 @@ impl Scheme for DecimalScheme { let decimal = narrowed_decimal(decimal); let parts = split_decimal(&decimal)?; - // Splitting a value too wide for one signed part writes more than one child, which a - // reader predating lower parts cannot open. Until that is stable, leave those values - // as the canonical decimal rather than emitting a file such a reader would reject. - if !parts.lower_parts.is_empty() && !cfg!(feature = "unstable_encodings") { + // A value too wide for one signed part splits into lower parts, and an array carrying + // those cannot be serialized. Leave it as the canonical decimal rather than build + // something the writer will refuse. + if !parts.lower_parts.is_empty() { return Ok(decimal.into_array()); } @@ -95,23 +88,7 @@ impl Scheme for DecimalScheme { DecimalBytePartsSlots::MSP, exec_ctx, )?; - let lower_parts = parts - .lower_parts - .iter() - .enumerate() - .map(|(idx, part)| { - compressor.compress_child( - part, - &compress_ctx, - self.id(), - DecimalBytePartsSlots::LOWER_PARTS_OFFSET + idx, - exec_ctx, - ) - }) - .collect::>>()?; - - DecimalByteParts::try_new_with_lower_parts(msp, lower_parts, decimal.decimal_dtype()) - .map(|d| d.into_array()) + DecimalByteParts::try_new(msp, decimal.decimal_dtype()).map(|d| d.into_array()) } } diff --git a/vortex-btrblocks/src/schemes/decimal/tests.rs b/vortex-btrblocks/src/schemes/decimal/tests.rs index d79a30e3a34..e85ee9dd228 100644 --- a/vortex-btrblocks/src/schemes/decimal/tests.rs +++ b/vortex-btrblocks/src/schemes/decimal/tests.rs @@ -1,17 +1,12 @@ // SPDX-License-Identifier: Apache-2.0 // SPDX-FileCopyrightText: Copyright the Vortex contributors -#[cfg(feature = "unstable_encodings")] use std::iter; use std::sync::LazyLock; -#[cfg(feature = "unstable_encodings")] use rand::RngExt; -#[cfg(feature = "unstable_encodings")] use rand::SeedableRng as _; -#[cfg(feature = "unstable_encodings")] use rand::rngs::StdRng; -#[cfg(feature = "unstable_encodings")] use rstest::rstest; use vortex_array::ArrayRef; use vortex_array::IntoArray; @@ -19,15 +14,12 @@ use vortex_array::VortexSessionExecute; use vortex_array::arrays::DecimalArray; use vortex_array::assert_arrays_eq; use vortex_array::dtype::DecimalDType; -#[cfg(feature = "unstable_encodings")] use vortex_array::dtype::DecimalType; use vortex_array::dtype::i256; use vortex_array::validity::Validity; use vortex_buffer::Buffer; use vortex_decimal_byte_parts::DecimalByteParts; use vortex_decimal_byte_parts::DecimalBytePartsArraySlotsExt; -#[cfg(feature = "unstable_encodings")] -use vortex_decimal_byte_parts::MAX_LOWER_PARTS; use vortex_error::VortexExpect; use vortex_error::VortexResult; use vortex_session::VortexSession; @@ -40,12 +32,10 @@ static SESSION: LazyLock = LazyLock::new(vortex_array::array_sess /// runs on sampled estimates as it does for real file chunks. const N: usize = 16_384; -#[cfg(feature = "unstable_encodings")] fn ten_pow(exp: u32) -> i256 { i256::from_i128(10).wrapping_pow(exp) } -#[cfg(feature = "unstable_encodings")] /// Deterministic 24-bit noise, so the low part of each value is neither constant nor a /// sequence — the realistic shape for a wide decimal column with a large fixed magnitude. fn noise(seed: u64) -> impl Iterator { @@ -53,7 +43,6 @@ fn noise(seed: u64) -> impl Iterator { iter::repeat_with(move || i128::from(rng.random::() >> 8)) } -#[cfg(feature = "unstable_encodings")] /// `i128`-backed values that need more than 64 bits, so the encoding must carry one lower /// part. fn wide_i128_array(validity: Validity) -> DecimalArray { @@ -62,7 +51,6 @@ fn wide_i128_array(validity: Validity) -> DecimalArray { DecimalArray::new(values, DecimalDType::new(38, 2), validity) } -#[cfg(feature = "unstable_encodings")] /// `i256`-backed values that need more than 128 bits, so the encoding must carry three /// lower parts. fn wide_i256_array(validity: Validity) -> DecimalArray { @@ -95,40 +83,27 @@ fn lower_part_count(array: &ArrayRef) -> usize { .len() } -// Requires the lower-parts write gate: without it the compressor leaves wide values canonical. -#[cfg(feature = "unstable_encodings")] +/// Values too wide for a single signed part are left as the canonical decimal. Splitting them +/// would need lower parts, which cannot be serialized, so the scheme declines rather than +/// building an array the writer would refuse. #[rstest] -#[case::non_nullable(Validity::NonNullable)] -#[case::all_valid(Validity::AllValid)] -#[case::nullable(Validity::from_iter((0..N).map(|i| i % 3 != 0)))] -fn test_i128_decimal_splits_into_one_lower_part(#[case] validity: Validity) -> VortexResult<()> { - let array = wide_i128_array(validity).into_array(); +#[case::i128(wide_i128_array(Validity::NonNullable).into_array())] +#[case::i128_nullable(wide_i128_array(Validity::from_iter((0..N).map(|i| i % 3 != 0))).into_array())] +#[case::i256(wide_i256_array(Validity::NonNullable).into_array())] +#[case::i256_nullable(wide_i256_array(Validity::from_iter((0..N).map(|i| i % 5 != 0))).into_array())] +fn test_wide_decimals_are_left_canonical(#[case] array: ArrayRef) -> VortexResult<()> { let compressed = compress(&array)?; - assert_eq!(lower_part_count(&compressed), 1); - assert_eq!(compressed.dtype(), array.dtype()); - assert_arrays_eq!(array, compressed, &mut SESSION.create_execution_ctx()); - Ok(()) -} - -// Requires the lower-parts write gate: without it the compressor leaves wide values canonical. -#[cfg(feature = "unstable_encodings")] -#[rstest] -#[case::non_nullable(Validity::NonNullable)] -#[case::all_valid(Validity::AllValid)] -#[case::nullable(Validity::from_iter((0..N).map(|i| i % 5 != 0)))] -fn test_i256_decimal_splits_into_three_lower_parts(#[case] validity: Validity) -> VortexResult<()> { - let array = wide_i256_array(validity).into_array(); - let compressed = compress(&array)?; - - assert_eq!(lower_part_count(&compressed), MAX_LOWER_PARTS); + assert!( + compressed.as_opt::().is_none(), + "expected the wide decimal to be left canonical, got {}", + compressed.encoding_id() + ); assert_eq!(compressed.dtype(), array.dtype()); assert_arrays_eq!(array, compressed, &mut SESSION.create_execution_ctx()); Ok(()) } -// Requires the lower-parts write gate: without it the compressor leaves wide values canonical. -#[cfg(feature = "unstable_encodings")] #[test] fn test_i256_decimal_round_trips_extreme_values() -> VortexResult<()> { // Every 64-bit window exercised, including the sign boundary of the most significant @@ -168,8 +143,6 @@ fn test_narrow_decimal_has_no_lower_parts() -> VortexResult<()> { Ok(()) } -// Requires the lower-parts write gate: without it the compressor leaves wide values canonical. -#[cfg(feature = "unstable_encodings")] #[test] fn test_canonical_of_compressed_wide_decimal_keeps_storage_width() -> VortexResult<()> { let mut ctx = SESSION.create_execution_ctx(); @@ -183,26 +156,3 @@ fn test_canonical_of_compressed_wide_decimal_keeps_storage_width() -> VortexResu assert_eq!(canonical.values_type(), DecimalType::I256); Ok(()) } - -/// Splitting exists to make wide decimals compressible: the parts that do not vary collapse -/// to constants and the varying part bit-packs. Without splitting these arrays are stored as -/// raw 16- and 32-byte values. -// Requires the lower-parts write gate: without it the compressor leaves wide values canonical. -#[cfg(feature = "unstable_encodings")] -#[rstest] -#[case::i128(wide_i128_array(Validity::NonNullable), 16)] -#[case::i256(wide_i256_array(Validity::NonNullable), 32)] -fn test_wide_decimals_compress( - #[case] array: DecimalArray, - #[case] uncompressed_bytes_per_value: usize, -) -> VortexResult<()> { - let array = array.into_array(); - let uncompressed = u64::try_from(uncompressed_bytes_per_value * N)?; - let compressed = compress(&array)?.nbytes(); - - assert!( - compressed * 4 < uncompressed, - "expected at least 4x compression, got {uncompressed} -> {compressed} bytes" - ); - Ok(()) -} diff --git a/vortex-file/Cargo.toml b/vortex-file/Cargo.toml index 55d59433645..89032bd2461 100644 --- a/vortex-file/Cargo.toml +++ b/vortex-file/Cargo.toml @@ -85,4 +85,5 @@ unstable_encodings = [ "dep:vortex-tensor", "vortex-zstd?/unstable_encodings", "vortex-btrblocks/unstable_encodings", + "vortex-decimal-byte-parts/unstable_encodings", ] diff --git a/vortex-file/src/tests.rs b/vortex-file/src/tests.rs index 6a0a84f1e38..98de950b9b2 100644 --- a/vortex-file/src/tests.rs +++ b/vortex-file/src/tests.rs @@ -11,11 +11,8 @@ use flatbuffers::FlatBufferBuilder; use futures::StreamExt; use futures::TryStreamExt; use futures::pin_mut; -#[cfg(feature = "unstable_encodings")] use rand::RngExt; -#[cfg(feature = "unstable_encodings")] use rand::SeedableRng as _; -#[cfg(feature = "unstable_encodings")] use rand::rngs::StdRng; use rstest::rstest; use vortex_array::ArrayRef; @@ -42,7 +39,6 @@ use vortex_array::dtype::Nullability; use vortex_array::dtype::PType; use vortex_array::dtype::PType::I32; use vortex_array::dtype::StructFields; -#[cfg(feature = "unstable_encodings")] use vortex_array::dtype::i256; use vortex_array::expr::and; use vortex_array::expr::cast; @@ -234,19 +230,15 @@ async fn test_round_trip_many_types() { assert_eq!(read.len(), 3); } -/// End-to-end check that decimals wider than 64 bits survive a write/read round trip and are -/// actually compressed: the compressor splits them into a most significant part plus 64-bit -/// lower parts, each of which compresses on its own. +/// End-to-end check that decimals wider than 64 bits survive a write/read round trip. /// -/// Requires `unstable_encodings`: the split writes more than one child, so without the -/// feature the compressor deliberately leaves these values canonical and uncompressed. -#[cfg(feature = "unstable_encodings")] +/// The compressor declines to split these — that would need lower parts, which cannot be +/// serialized — so they are written as canonical decimals. This pins that the wide path still +/// round trips through a file rather than being compressed. #[tokio::test] #[cfg_attr(miri, ignore)] -async fn test_wide_decimal_round_trip_compresses() -> VortexResult<()> { +async fn test_wide_decimal_round_trips_through_a_file() -> VortexResult<()> { const N: usize = 16_384; - /// Bytes each value would occupy uncompressed: `i128` plus `i256` storage. - const UNCOMPRESSED_BYTES_PER_ROW: usize = 16 + 32; /// Deterministic 24-bit noise, so the low bits of each value are neither constant nor a /// sequence. @@ -290,7 +282,6 @@ async fn test_wide_decimal_round_trip_compresses() -> VortexResult<()> { .write_options() .write(&mut buf, st.clone().to_array_stream()) .await?; - let written = buf.len(); let chunks: Vec<_> = SESSION .open_options() @@ -305,12 +296,6 @@ async fn test_wide_decimal_round_trip_compresses() -> VortexResult<()> { assert_eq!(read.len(), N); assert_arrays_eq!(st, read, &mut ctx); - let uncompressed = N * UNCOMPRESSED_BYTES_PER_ROW; - assert!( - written * 4 < uncompressed, - "expected at least 4x compression, wrote {written} bytes for {uncompressed} bytes of \ - decimal values" - ); Ok(()) } diff --git a/vortex/Cargo.toml b/vortex/Cargo.toml index 3cc3e40d65b..550728dd2ba 100644 --- a/vortex/Cargo.toml +++ b/vortex/Cargo.toml @@ -86,6 +86,7 @@ serde = ["vortex-array/serde", "vortex-buffer/serde", "vortex-mask/serde"] unstable_encodings = [ "dep:vortex-tensor", "vortex-btrblocks/unstable_encodings", + "vortex-decimal-byte-parts/unstable_encodings", "vortex-file?/unstable_encodings", "vortex-zstd?/unstable_encodings", ]