From 842b737d154ec2cb2c347a963a304ae768eaead9 Mon Sep 17 00:00:00 2001 From: Matt Katz Date: Tue, 8 Sep 2026 16:29:46 -0400 Subject: [PATCH 01/17] Register versioned decimal byte-part serialization Use one ArrayPlugin for the frozen single-part format and the new wide format. Preserve frozen files with wider physical storage and add wire contract tests plus an opt-in compatibility fixture. Signed-off-by: Matt Katz --- .../src/decimal_byte_parts/array.rs | 397 +++++++++++++++++- encodings/decimal-byte-parts/src/lib.rs | 4 +- .../decimal-byte-parts/tests/format_v2.rs | 257 ++++++++++++ vortex-test/compat-gen/Cargo.toml | 5 + .../encodings/decimal_byte_parts_v2.rs | 116 +++++ .../arrays/synthetic/encodings/mod.rs | 10 +- 6 files changed, 778 insertions(+), 11 deletions(-) create mode 100644 encodings/decimal-byte-parts/tests/format_v2.rs create mode 100644 vortex-test/compat-gen/src/fixtures/arrays/synthetic/encodings/decimal_byte_parts_v2.rs diff --git a/encodings/decimal-byte-parts/src/decimal_byte_parts/array.rs b/encodings/decimal-byte-parts/src/decimal_byte_parts/array.rs index 8c10c0f5088..d2fe86292fa 100644 --- a/encodings/decimal-byte-parts/src/decimal_byte_parts/array.rs +++ b/encodings/decimal-byte-parts/src/decimal_byte_parts/array.rs @@ -9,16 +9,20 @@ use std::hash::Hasher; use prost::Message as _; use vortex_array::Array; +use vortex_array::ArrayDeserialization; use vortex_array::ArrayEq; use vortex_array::ArrayHash; use vortex_array::ArrayId; use vortex_array::ArrayParts; +use vortex_array::ArrayPlugin; use vortex_array::ArrayRef; +use vortex_array::ArraySerialization; use vortex_array::ArraySlots; use vortex_array::ArrayView; use vortex_array::EqMode; use vortex_array::ExecutionCtx; use vortex_array::ExecutionResult; +use vortex_array::IntoArray; use vortex_array::TypedArrayRef; use vortex_array::array_slots; use vortex_array::buffer::BufferHandle; @@ -233,6 +237,9 @@ impl DecimalByteParts { lower_parts: Vec, decimal_dtype: DecimalDType, ) -> VortexResult { + // Building lower parts in memory is never gated — reading a file requires it. What is + // gated is the serialized form: an array carrying lower parts serializes under the + // `vortex.decimal_byte_parts_v2` format ID, which only editions that contain it may write. let len = msp.len(); let dtype = DType::Decimal(decimal_dtype, msp.dtype().nullability()); let slots = DecimalBytePartsSlots { msp, lower_parts }.into_slots(); @@ -336,7 +343,7 @@ impl VTable for DecimalByteParts { ) -> VortexResult>> { vortex_ensure!( array.lower_parts().is_empty(), - "serializing DecimalByteParts with lower parts is not supported" + "serializing DecimalByteParts with lower parts requires DecimalBytePartsPlugin" ); Ok(Some( DecimalBytesPartsMetadata::from_array(array)?.encode_to_vec(), @@ -432,6 +439,97 @@ pub(crate) trait DecimalBytePartsArrayExt: DecimalBytePartsArraySlotsExt { impl> DecimalBytePartsArrayExt for T {} +/// The `vortex.decimal_byte_parts_v2` serialized format ID: byte parts carrying lower parts. +/// +/// This is a serialized format, not a second in-memory encoding. `vortex.decimal_byte_parts` +/// froze promising a single child, so an array with lower parts serializes under this ID +/// instead, and both IDs deserialize back into the same [`DecimalBytePartsArray`]. A reader +/// that predates lower parts fails on this ID with an unknown-encoding error rather than +/// misreading the children. +pub fn decimal_byte_parts_v2_id() -> ArrayId { + static ID: CachedId = CachedId::new("vortex.decimal_byte_parts_v2"); + *ID +} + +/// The [`ArrayPlugin`] for [`DecimalByteParts`], owning both of its serialized formats. +/// +/// An array without lower parts serializes under the frozen `vortex.decimal_byte_parts` ID, +/// byte-identical to files written before lower parts existed. An array carrying lower parts +/// serializes under [`decimal_byte_parts_v2_id`]. Reading holds each ID to its own contract: +/// the frozen ID carries no lower parts and the v2 ID carries at least one, so recognizing the +/// newer format never widens what the frozen one may mean. +/// +/// Register this plugin, or call [`crate::initialize`], to enable both formats. Registering +/// [`DecimalByteParts`] directly only supports the frozen format. +#[derive(Clone, Debug)] +pub struct DecimalBytePartsPlugin; + +impl ArrayPlugin for DecimalBytePartsPlugin { + fn id(&self) -> ArrayId { + VTable::id(&DecimalByteParts) + } + + fn serialized_ids(&self) -> Vec { + vec![VTable::id(&DecimalByteParts), decimal_byte_parts_v2_id()] + } + + fn serialize( + &self, + array: &ArrayRef, + _session: &VortexSession, + ) -> VortexResult> { + let view = array.as_opt::().ok_or_else(|| { + vortex_err!( + "DecimalByteParts plugin cannot serialize {}", + array.encoding_id() + ) + })?; + let serialized_id = if view.lower_parts().is_empty() { + VTable::id(&DecimalByteParts) + } else { + decimal_byte_parts_v2_id() + }; + Ok(Some(ArraySerialization::from_array( + serialized_id, + array, + DecimalBytesPartsMetadata::from_array(view)?.encode_to_vec(), + ))) + } + + fn deserialize( + &self, + parts: ArrayDeserialization<'_>, + _session: &VortexSession, + ) -> VortexResult { + let metadata = DecimalBytesPartsMetadata::decode(parts.metadata)?; + let lower_part_count = metadata.lower_part_count()?; + if parts.serialized_id == decimal_byte_parts_v2_id() { + vortex_ensure!( + lower_part_count > 0, + "{} must carry at least one lower part", + parts.serialized_id + ); + } else { + vortex_ensure!( + parts.serialized_id == VTable::id(&DecimalByteParts), + "DecimalByteParts plugin does not recognize serialized ID {}", + parts.serialized_id + ); + vortex_ensure!( + lower_part_count == 0, + "{} must not carry lower parts, got {lower_part_count}", + parts.serialized_id + ); + } + Ok(Array::try_from_parts(metadata.into_array_parts( + parts.dtype, + parts.len, + parts.children, + )?)? + .into_array()) + } +} + impl OperationsVTable for DecimalByteParts { fn scalar_at( array: ArrayView<'_, DecimalByteParts>, @@ -486,15 +584,22 @@ impl ValidityChild for DecimalByteParts { #[cfg(test)] mod tests { + use prost::Message as _; use rstest::rstest; use vortex_array::Array; + use vortex_array::ArrayContext; + use vortex_array::ArrayDeserialization; + use vortex_array::ArrayId; use vortex_array::ArrayParts; + use vortex_array::ArrayPlugin; use vortex_array::ArrayRef; + use vortex_array::ArrayVTable; 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::Primitive; use vortex_array::arrays::PrimitiveArray; use vortex_array::assert_arrays_eq; use vortex_array::builtins::ArrayBuiltins; @@ -507,17 +612,27 @@ mod tests { 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_array::vtable::VTable; + use vortex_buffer::ByteBufferMut; use vortex_buffer::buffer; + use vortex_error::VortexExpect; use vortex_error::VortexResult; + use vortex_session::registry::ReadContext; use super::DecimalByteParts; use super::DecimalBytePartsArray; use super::DecimalBytePartsArraySlotsExt; use super::DecimalBytePartsData; + use super::DecimalBytePartsPlugin; + use super::DecimalBytesPartsMetadata; + use super::decimal_byte_parts_v2_id; use crate::decimal_byte_parts::LOWER_PART_DTYPE; use crate::decimal_byte_parts::MAX_LOWER_PARTS; + 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; @@ -806,6 +921,127 @@ mod tests { Ok(()) } + #[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") + )] + fn test_serde_round_trip_flat(#[case] array: DecimalBytePartsArray) -> VortexResult<()> { + test_serde_round_trip(array) + } + + #[rstest] + fn test_deserialize_frozen_with_wider_storage( + #[values(Validity::NonNullable, Validity::from_iter([true, false, true]))] + validity: Validity, + ) -> VortexResult<()> { + let session = array_session(); + crate::initialize(&session); + let mut ctx = session.create_execution_ctx(); + let decimal_dtype = DecimalDType::new(2, 0); + let expected = DecimalArray::new(buffer![1i8, 2, 3], decimal_dtype, validity.clone()); + let children = vec![PrimitiveArray::new(buffer![1i64, 2, 3], validity).into_array()]; + + // Metadata emitted by the frozen serializer for a single i64 child. + let decoded = DecimalBytePartsPlugin.deserialize( + ArrayDeserialization::new( + VTable::id(&DecimalByteParts), + expected.dtype(), + expected.len(), + &[8, 7], + &[], + &children, + ), + &session, + )?; + assert_arrays_eq!(expected, decoded, &mut ctx); + test_serde_round_trip(decoded.as_::().into_owned()) + } + + #[rstest] + #[case::i64(DecimalArray::new( + buffer![-99i64, 0, 99], DecimalDType::new(2, 0), Validity::NonNullable, + ))] + #[case::i128(DecimalArray::new( + buffer![-99i128, 0, 99], DecimalDType::new(2, 0), Validity::NonNullable, + ))] + #[case::i256(DecimalArray::new( + buffer![i256::from_i128(-99), i256::ZERO, i256::from_i128(99)], + DecimalDType::new(2, 0), Validity::NonNullable, + ))] + fn test_serde_round_trip_wider_storage(#[case] decimal: DecimalArray) -> VortexResult<()> { + let mut ctx = array_session().create_execution_ctx(); + let encoded = encode(&decimal)?; + assert_arrays_eq!(decimal, encoded, &mut ctx); + assert_eq!( + encoded.execute_scalar(0, &mut ctx)?, + decimal.execute_scalar(0, &mut ctx)?, + ); + test_serde_round_trip(encoded) + } + + fn test_serde_round_trip(array: DecimalBytePartsArray) -> VortexResult<()> { + let session = array_session(); + // Both serialized formats must be registered: an array with lower parts comes back + // under the v2 format id. + crate::initialize(&session); + + 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 expected_id = if lower_part_count == 0 { + VTable::id(&DecimalByteParts) + } else { + decimal_byte_parts_v2_id() + }; + assert_eq!( + session + .array_serialize(&array)? + .vortex_expect("byte parts arrays are serializable") + .serialized_id, + expected_id + ); + + 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() } @@ -850,6 +1086,158 @@ mod tests { assert!(Array::try_from_parts(parts).is_err()); } + fn deserialize_with( + lower_part_count: u32, + children: Vec, + ) -> VortexResult { + let serialized_id = if lower_part_count == 0 { + VTable::id(&DecimalByteParts) + } else { + decimal_byte_parts_v2_id() + }; + plugin_deserialize_with(serialized_id, lower_part_count, children) + .map(|array| array.as_::().into_owned()) + } + + #[test] + fn test_deserialize_reads_lower_parts() -> VortexResult<()> { + let array = deserialize_with(1, vec![msp(), lower_part()])?; + 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(()) + } + + /// An array read from a file can be handed straight back to a writer, bypassing both the + /// constructor and the compressor. Its serialized id must still be the v2 format, so a + /// writer whose permitted encodings predate the v2 format refuses it. + #[test] + fn read_lower_parts_serialize_under_the_wide_format() -> VortexResult<()> { + let session = array_session(); + crate::initialize(&session); + + let array = deserialize_with(1, vec![msp(), lower_part()])?.into_array(); + + let serialization = session + .array_serialize(&array)? + .vortex_expect("byte parts arrays are serializable"); + assert_eq!(serialization.serialized_id, decimal_byte_parts_v2_id()); + + let restricted = ArrayContext::empty() + .with_allowed_ids([VTable::id(&DecimalByteParts)].into_iter().collect()); + let err = array + .serialize(&restricted, &session, &SerializeOptions::default()) + .expect_err("expected the permitted-encoding check to refuse the v2 format"); + assert!( + err.to_string().contains("not permitted"), + "error should name the permitted-encoding check, got: {err}" + ); + Ok(()) + } + + /// Reading back an array that already carries lower parts, and computing over it, must + /// always work: the v2 format only restricts which writers may emit it. If reading or + /// the rebuild that every compute kernel does were blocked, a session whose editions + /// predate the v2 format could not read a file written by one that includes 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()])?.into_array(); + + let sliced = array.slice(0..2)?; + assert_eq!(sliced.execute::(&mut ctx)?.len(), 2); + Ok(()) + } + + #[rstest] + fn test_deserialize_redundant_lower_parts( + #[values(2, 3)] lower_part_count: u32, + ) -> VortexResult<()> { + let mut children = vec![buffer![0i64; 3].into_array()]; + children.extend((1..lower_part_count).map(|_| buffer![0u64; 3].into_array())); + children.push(lower_part()); + let array = deserialize_with(lower_part_count, children)?; + let expected = DecimalArray::new( + buffer![1i128, 2, 3], + DecimalDType::new(38, 2), + Validity::NonNullable, + ); + let mut ctx = array_session().create_execution_ctx(); + assert_arrays_eq!(expected, array, &mut ctx); + test_serde_round_trip(array) + } + + #[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() + ); + } + + fn plugin_deserialize_with( + serialized_id: ArrayId, + lower_part_count: u32, + children: Vec, + ) -> VortexResult { + let metadata = DecimalBytesPartsMetadata { + zeroth_child_ptype: PType::I64 as i32, + lower_part_count, + } + .encode_to_vec(); + let dtype = DType::Decimal(DecimalDType::new(38, 2), Nullability::NonNullable); + DecimalBytePartsPlugin.deserialize( + ArrayDeserialization::new(serialized_id, &dtype, 3, &metadata, &[], &children), + &array_session(), + ) + } + + /// Each serialized ID keeps its own contract: the frozen ID never carries lower parts, and + /// the v2 ID is never written without them. + #[rstest] + #[case::frozen_without_lower_parts(VTable::id(&DecimalByteParts), 0, vec![msp()], true)] + #[case::frozen_with_lower_parts( + VTable::id(&DecimalByteParts), + 1, + vec![msp(), lower_part()], + false + )] + #[case::v2_with_lower_parts(decimal_byte_parts_v2_id(), 1, vec![msp(), lower_part()], true)] + #[case::v2_without_lower_parts(decimal_byte_parts_v2_id(), 0, vec![msp()], false)] + #[case::unknown_id(ArrayVTable::id(&Primitive), 0, vec![msp()], false)] + fn plugin_holds_each_id_to_its_contract( + #[case] serialized_id: ArrayId, + #[case] lower_part_count: u32, + #[case] children: Vec, + #[case] accepted: bool, + ) { + let result = plugin_deserialize_with(serialized_id, lower_part_count, children); + assert_eq!(result.is_ok(), accepted, "{serialized_id}: {result:?}"); + } + #[test] fn test_wide_decimal_buffer_types() -> VortexResult<()> { let mut ctx = array_session().create_execution_ctx(); @@ -910,11 +1298,4 @@ mod tests { assert_arrays_eq!(array, canonical.into_array(), &mut ctx); Ok(()) } - #[test] - fn test_frozen_serializer_rejects_lower_parts() -> VortexResult<()> { - let session = array_session(); - let array = i128_parts(vec![1i128 << 70], Validity::NonNullable); - assert!(VTable::serialize(array.as_view(), &session).is_err()); - Ok(()) - } } diff --git a/encodings/decimal-byte-parts/src/lib.rs b/encodings/decimal-byte-parts/src/lib.rs index 36a53c3a614..2557555eac8 100644 --- a/encodings/decimal-byte-parts/src/lib.rs +++ b/encodings/decimal-byte-parts/src/lib.rs @@ -22,7 +22,9 @@ use vortex_session::VortexSession; /// Initialize decimal-byte-parts encoding in the given session. pub fn initialize(session: &VortexSession) { - session.arrays().register(DecimalByteParts); + // One plugin owns both serialized formats: registering it reads either ID and writes the + // one that fits the array. Which of them a writer may emit is decided by its editions. + session.arrays().register(DecimalBytePartsPlugin); compute::kernel::initialize(session); session.aggregate_fns().register_aggregate_kernel( diff --git a/encodings/decimal-byte-parts/tests/format_v2.rs b/encodings/decimal-byte-parts/tests/format_v2.rs new file mode 100644 index 00000000000..338598df032 --- /dev/null +++ b/encodings/decimal-byte-parts/tests/format_v2.rs @@ -0,0 +1,257 @@ +// SPDX-License-Identifier: Apache-2.0 +// SPDX-FileCopyrightText: Copyright the Vortex contributors + +//! The v2 serialized format. +//! +//! Lower parts can be built and computed over freely. What changes with them is the bytes: +//! an array carrying lower parts serializes under `vortex.decimal_byte_parts_v2` rather +//! than the frozen `vortex.decimal_byte_parts` format, so a writer restricted to editions +//! without the v2 format refuses it, and a reader that predates lower parts fails with an +//! unknown-encoding error instead of misreading the children. These tests pin all of that: +//! construction always works, the serialized id tracks the parts, and the permitted-encoding +//! check applies to the serialized id. + +#![expect(clippy::tests_outside_test_module)] + +use vortex_array::ArrayContext; +use vortex_array::ArrayDeserialization; +use vortex_array::ArrayId; +use vortex_array::ArrayRef; +use vortex_array::ArrayVTable; +use vortex_array::IntoArray; +use vortex_array::VortexSessionExecute; +use vortex_array::assert_arrays_eq; +use vortex_array::dtype::DecimalDType; +use vortex_array::serde::SerializeOptions; +use vortex_array::session::ArraySessionExt; +use vortex_buffer::buffer; +use vortex_decimal_byte_parts::DecimalByteParts; +use vortex_decimal_byte_parts::decimal_byte_parts_v2_id; +use vortex_error::VortexResult; +use vortex_error::vortex_err; +use vortex_session::VortexSession; + +fn msp() -> ArrayRef { + buffer![1i64, 2, 3].into_array() +} + +fn lower_part() -> ArrayRef { + buffer![1u64, 2, 3].into_array() +} + +fn session() -> VortexSession { + let session = vortex_array::array_session(); + vortex_decimal_byte_parts::initialize(&session); + session +} + +/// The wire ID the session's plugin picks for `array`. +fn serialized_id(session: &VortexSession, array: &ArrayRef) -> VortexResult { + Ok(session + .array_serialize(array)? + .ok_or_else(|| vortex_err!("byte parts arrays are serializable"))? + .serialized_id) +} + +/// 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() + ); +} + +/// Building lower parts in memory is always allowed — reading a file requires it. What +/// changes is the serialized format, not what can be constructed. +#[test] +fn lower_parts_can_always_be_constructed() { + assert!( + DecimalByteParts::try_new_with_lower_parts( + msp(), + vec![lower_part()], + DecimalDType::new(38, 2), + ) + .is_ok() + ); +} + +/// A single-child array keeps the frozen format id, byte-compatible with every reader since +/// the format froze; lower parts move the array onto the v2 format id. +#[test] +fn serialized_id_tracks_lower_parts() -> VortexResult<()> { + let session = session(); + + let flat = DecimalByteParts::try_new(msp(), DecimalDType::new(19, 2))?.into_array(); + assert_eq!( + serialized_id(&session, &flat)?, + ArrayVTable::id(&DecimalByteParts) + ); + + let wide = DecimalByteParts::try_new_with_lower_parts( + msp(), + vec![lower_part()], + DecimalDType::new(38, 2), + )? + .into_array(); + assert_eq!(serialized_id(&session, &wide)?, decimal_byte_parts_v2_id()); + + Ok(()) +} + +/// The permitted-encoding check applies to the serialized id. A context restricted to the +/// frozen format — a writer whose enabled editions predate the v2 format — must refuse an +/// array carrying lower parts, however it was obtained. +/// +/// `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. What must hold is that the resulting array cannot become +/// bytes under the frozen id. +#[test] +fn wide_format_is_refused_where_not_permitted() -> VortexResult<()> { + use vortex_array::Array; + use vortex_array::ArrayParts; + use vortex_array::ArraySlots; + use vortex_array::dtype::DType; + use vortex_array::dtype::Nullability; + use vortex_decimal_byte_parts::DecimalBytePartsData; + + let session = session(); + + 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), + )? + .into_array(); + assert_eq!(array.nchildren(), 2, "expected two limbs"); + + // A context permitting only the frozen format refuses to write it. + let restricted = ArrayContext::empty() + .with_allowed_ids([ArrayVTable::id(&DecimalByteParts)].into_iter().collect()); + let err = array + .serialize(&restricted, &session, &SerializeOptions::default()) + .expect_err("expected the permitted-encoding check to refuse the v2 format"); + assert!( + err.to_string().contains("not permitted"), + "error should name the permitted-encoding check, got: {err}" + ); + + // Permitting the v2 format id is exactly what allows the same array through. + let permissive = ArrayContext::empty().with_allowed_ids( + [ + ArrayVTable::id(&DecimalByteParts), + decimal_byte_parts_v2_id(), + ArrayVTable::id(&vortex_array::arrays::Primitive), + ] + .into_iter() + .collect(), + ); + let serialized = array.serialize(&permissive, &session, &SerializeOptions::default())?; + assert!(!serialized.is_empty()); + assert!( + permissive.to_ids().contains(&decimal_byte_parts_v2_id()), + "the file's encoding table must carry the v2 format id" + ); + + Ok(()) +} + +#[test] +fn bare_vtable_refuses_wide_serialization() -> VortexResult<()> { + let session = vortex_array::array_session(); + session.arrays().register(DecimalByteParts); + let array = DecimalByteParts::try_new_with_lower_parts( + msp(), + vec![lower_part()], + DecimalDType::new(38, 2), + )? + .into_array(); + let restricted = ArrayContext::empty().with_allowed_ids( + [ + ArrayVTable::id(&DecimalByteParts), + ArrayVTable::id(&vortex_array::arrays::Primitive), + ] + .into_iter() + .collect(), + ); + + assert!( + array + .serialize(&restricted, &session, &SerializeOptions::default()) + .is_err(), + "bare VTable registration must not write lower parts under the frozen ID" + ); + Ok(()) +} + +#[test] +fn bare_vtable_refuses_lower_parts_on_frozen_id() -> VortexResult<()> { + let session = vortex_array::array_session(); + session.arrays().register(DecimalByteParts); + let array = DecimalByteParts::try_new_with_lower_parts( + msp(), + vec![lower_part()], + DecimalDType::new(38, 2), + )? + .into_array(); + let id = ArrayVTable::id(&DecimalByteParts); + let plugin = session + .arrays() + .registry() + .get(&id) + .ok_or_else(|| vortex_err!("missing decimal plugin"))?; + let children = array.children(); + + // i64 MSP and one lower part, mislabeled as the frozen format. + let parts = ArrayDeserialization::new( + id, + array.dtype(), + array.len(), + &[8, 7, 16, 1], + &[], + &children, + ); + assert!(plugin.deserialize(parts, &session).is_err()); + Ok(()) +} + +#[test] +fn bare_vtable_keeps_frozen_serde() -> VortexResult<()> { + let session = vortex_array::array_session(); + session.arrays().register(DecimalByteParts); + let array = DecimalByteParts::try_new(msp(), DecimalDType::new(2, 0))?.into_array(); + let serialized = session + .array_serialize(&array)? + .ok_or_else(|| vortex_err!("missing decimal serialization"))?; + assert_eq!(serialized.serialized_id, ArrayVTable::id(&DecimalByteParts)); + assert_eq!(serialized.metadata, [8, 7]); + let plugin = session + .arrays() + .registry() + .get(&serialized.serialized_id) + .ok_or_else(|| vortex_err!("missing decimal plugin"))?; + let decoded = plugin.deserialize( + ArrayDeserialization::new( + serialized.serialized_id, + array.dtype(), + array.len(), + &serialized.metadata, + &[], + &serialized.children, + ), + &session, + )?; + assert_arrays_eq!(array, decoded, &mut session.create_execution_ctx()); + Ok(()) +} 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/decimal_byte_parts_v2.rs b/vortex-test/compat-gen/src/fixtures/arrays/synthetic/encodings/decimal_byte_parts_v2.rs new file mode 100644 index 00000000000..dfdcd893860 --- /dev/null +++ b/vortex-test/compat-gen/src/fixtures/arrays/synthetic/encodings/decimal_byte_parts_v2.rs @@ -0,0 +1,116 @@ +// 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, + ctx: &mut ExecutionCtx, +) -> VortexResult { + let parts = split_decimal(decimal, ctx)?; + DecimalByteParts::try_new_with_lower_parts( + parts.msp, + parts.lower_parts, + decimal.decimal_dtype(), + ) +} + +pub struct DecimalBytePartsV2Fixture; + +impl FlatLayoutFixture for DecimalBytePartsV2Fixture { + fn name(&self) -> &str { + "decimal_byte_parts_v2.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, ctx)?; + + // 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, ctx)?; + + // 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, ctx)?; + + 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..5af7596ca7b 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,8 @@ mod bytebool; mod constant; mod datetimeparts; mod decimal_byte_parts; +#[cfg(feature = "unstable_encodings")] +mod decimal_byte_parts_v2; mod delta; mod dict; mod for_; @@ -31,7 +33,8 @@ 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), @@ -53,5 +56,8 @@ 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_v2::DecimalBytePartsV2Fixture)); + fixtures } From 15193592c5d014305752ff38466f26d255b9c1ad Mon Sep 17 00:00:00 2001 From: Matt Katz Date: Wed, 9 Sep 2026 00:43:35 -0400 Subject: [PATCH 02/17] Extract decimal byte-parts serde plugin and tests Move the plugin and serde coverage into plugin.rs while preserving metadata and frozen-format VTable serde. Share wide decimal test fixtures and exercise frozen compatibility through both registration paths. Include the v2 compatibility fixture in the default suite without enabling unstable encodings. Signed-off-by: Matt Katz --- .../src/decimal_byte_parts/array.rs | 439 +----------- .../src/decimal_byte_parts/mod.rs | 3 + .../src/decimal_byte_parts/plugin.rs | 649 ++++++++++++++++++ .../src/decimal_byte_parts/testing.rs | 41 ++ .../decimal-byte-parts/tests/format_v2.rs | 257 ------- vortex-test/compat-gen/Cargo.toml | 5 - .../arrays/synthetic/encodings/mod.rs | 10 +- 7 files changed, 703 insertions(+), 701 deletions(-) create mode 100644 encodings/decimal-byte-parts/src/decimal_byte_parts/plugin.rs delete mode 100644 encodings/decimal-byte-parts/tests/format_v2.rs diff --git a/encodings/decimal-byte-parts/src/decimal_byte_parts/array.rs b/encodings/decimal-byte-parts/src/decimal_byte_parts/array.rs index d2fe86292fa..1ec6426f33e 100644 --- a/encodings/decimal-byte-parts/src/decimal_byte_parts/array.rs +++ b/encodings/decimal-byte-parts/src/decimal_byte_parts/array.rs @@ -9,20 +9,16 @@ use std::hash::Hasher; use prost::Message as _; use vortex_array::Array; -use vortex_array::ArrayDeserialization; use vortex_array::ArrayEq; use vortex_array::ArrayHash; use vortex_array::ArrayId; use vortex_array::ArrayParts; -use vortex_array::ArrayPlugin; use vortex_array::ArrayRef; -use vortex_array::ArraySerialization; use vortex_array::ArraySlots; use vortex_array::ArrayView; use vortex_array::EqMode; use vortex_array::ExecutionCtx; use vortex_array::ExecutionResult; -use vortex_array::IntoArray; use vortex_array::TypedArrayRef; use vortex_array::array_slots; use vortex_array::buffer::BufferHandle; @@ -58,13 +54,13 @@ pub type DecimalBytePartsArray = Array; #[derive(Clone, prost::Message)] pub struct DecimalBytesPartsMetadata { #[prost(enumeration = "PType", tag = "1")] - zeroth_child_ptype: i32, + pub(super) zeroth_child_ptype: i32, #[prost(uint32, tag = "2")] - lower_part_count: u32, + pub(super) lower_part_count: u32, } impl DecimalBytesPartsMetadata { - fn from_array(array: ArrayView<'_, DecimalByteParts>) -> VortexResult { + pub(super) fn from_array(array: ArrayView<'_, DecimalByteParts>) -> VortexResult { Ok(Self { zeroth_child_ptype: PType::try_from(array.msp().dtype())? as i32, lower_part_count: u32::try_from(array.lower_parts().len()) @@ -72,7 +68,7 @@ impl DecimalBytesPartsMetadata { }) } - fn into_array_parts( + pub(super) fn into_array_parts( self, dtype: &DType, len: usize, @@ -116,7 +112,7 @@ impl DecimalBytesPartsMetadata { /// # Errors /// /// Returns an error if the count exceeds [`MAX_LOWER_PARTS`]. - fn lower_part_count(&self) -> VortexResult { + pub(super) 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!( @@ -439,97 +435,6 @@ pub(crate) trait DecimalBytePartsArrayExt: DecimalBytePartsArraySlotsExt { impl> DecimalBytePartsArrayExt for T {} -/// The `vortex.decimal_byte_parts_v2` serialized format ID: byte parts carrying lower parts. -/// -/// This is a serialized format, not a second in-memory encoding. `vortex.decimal_byte_parts` -/// froze promising a single child, so an array with lower parts serializes under this ID -/// instead, and both IDs deserialize back into the same [`DecimalBytePartsArray`]. A reader -/// that predates lower parts fails on this ID with an unknown-encoding error rather than -/// misreading the children. -pub fn decimal_byte_parts_v2_id() -> ArrayId { - static ID: CachedId = CachedId::new("vortex.decimal_byte_parts_v2"); - *ID -} - -/// The [`ArrayPlugin`] for [`DecimalByteParts`], owning both of its serialized formats. -/// -/// An array without lower parts serializes under the frozen `vortex.decimal_byte_parts` ID, -/// byte-identical to files written before lower parts existed. An array carrying lower parts -/// serializes under [`decimal_byte_parts_v2_id`]. Reading holds each ID to its own contract: -/// the frozen ID carries no lower parts and the v2 ID carries at least one, so recognizing the -/// newer format never widens what the frozen one may mean. -/// -/// Register this plugin, or call [`crate::initialize`], to enable both formats. Registering -/// [`DecimalByteParts`] directly only supports the frozen format. -#[derive(Clone, Debug)] -pub struct DecimalBytePartsPlugin; - -impl ArrayPlugin for DecimalBytePartsPlugin { - fn id(&self) -> ArrayId { - VTable::id(&DecimalByteParts) - } - - fn serialized_ids(&self) -> Vec { - vec![VTable::id(&DecimalByteParts), decimal_byte_parts_v2_id()] - } - - fn serialize( - &self, - array: &ArrayRef, - _session: &VortexSession, - ) -> VortexResult> { - let view = array.as_opt::().ok_or_else(|| { - vortex_err!( - "DecimalByteParts plugin cannot serialize {}", - array.encoding_id() - ) - })?; - let serialized_id = if view.lower_parts().is_empty() { - VTable::id(&DecimalByteParts) - } else { - decimal_byte_parts_v2_id() - }; - Ok(Some(ArraySerialization::from_array( - serialized_id, - array, - DecimalBytesPartsMetadata::from_array(view)?.encode_to_vec(), - ))) - } - - fn deserialize( - &self, - parts: ArrayDeserialization<'_>, - _session: &VortexSession, - ) -> VortexResult { - let metadata = DecimalBytesPartsMetadata::decode(parts.metadata)?; - let lower_part_count = metadata.lower_part_count()?; - if parts.serialized_id == decimal_byte_parts_v2_id() { - vortex_ensure!( - lower_part_count > 0, - "{} must carry at least one lower part", - parts.serialized_id - ); - } else { - vortex_ensure!( - parts.serialized_id == VTable::id(&DecimalByteParts), - "DecimalByteParts plugin does not recognize serialized ID {}", - parts.serialized_id - ); - vortex_ensure!( - lower_part_count == 0, - "{} must not carry lower parts, got {lower_part_count}", - parts.serialized_id - ); - } - Ok(Array::try_from_parts(metadata.into_array_parts( - parts.dtype, - parts.len, - parts.children, - )?)? - .into_array()) - } -} - impl OperationsVTable for DecimalByteParts { fn scalar_at( array: ArrayView<'_, DecimalByteParts>, @@ -584,22 +489,15 @@ impl ValidityChild for DecimalByteParts { #[cfg(test)] mod tests { - use prost::Message as _; use rstest::rstest; use vortex_array::Array; - use vortex_array::ArrayContext; - use vortex_array::ArrayDeserialization; - use vortex_array::ArrayId; use vortex_array::ArrayParts; - use vortex_array::ArrayPlugin; use vortex_array::ArrayRef; - use vortex_array::ArrayVTable; 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::Primitive; use vortex_array::arrays::PrimitiveArray; use vortex_array::assert_arrays_eq; use vortex_array::builtins::ArrayBuiltins; @@ -612,30 +510,21 @@ mod tests { 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_array::vtable::VTable; - use vortex_buffer::ByteBufferMut; use vortex_buffer::buffer; - use vortex_error::VortexExpect; use vortex_error::VortexResult; - use vortex_session::registry::ReadContext; use super::DecimalByteParts; use super::DecimalBytePartsArray; use super::DecimalBytePartsArraySlotsExt; use super::DecimalBytePartsData; - use super::DecimalBytePartsPlugin; - use super::DecimalBytesPartsMetadata; - use super::decimal_byte_parts_v2_id; use crate::decimal_byte_parts::LOWER_PART_DTYPE; use crate::decimal_byte_parts::MAX_LOWER_PARTS; - 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; + use crate::decimal_byte_parts::testing::wide_i128_values; + use crate::decimal_byte_parts::testing::wide_i256_values; #[test] fn test_scalar_at_decimal_parts() { @@ -676,47 +565,6 @@ mod tests { ); } - /// 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))] @@ -921,127 +769,6 @@ mod tests { Ok(()) } - #[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") - )] - fn test_serde_round_trip_flat(#[case] array: DecimalBytePartsArray) -> VortexResult<()> { - test_serde_round_trip(array) - } - - #[rstest] - fn test_deserialize_frozen_with_wider_storage( - #[values(Validity::NonNullable, Validity::from_iter([true, false, true]))] - validity: Validity, - ) -> VortexResult<()> { - let session = array_session(); - crate::initialize(&session); - let mut ctx = session.create_execution_ctx(); - let decimal_dtype = DecimalDType::new(2, 0); - let expected = DecimalArray::new(buffer![1i8, 2, 3], decimal_dtype, validity.clone()); - let children = vec![PrimitiveArray::new(buffer![1i64, 2, 3], validity).into_array()]; - - // Metadata emitted by the frozen serializer for a single i64 child. - let decoded = DecimalBytePartsPlugin.deserialize( - ArrayDeserialization::new( - VTable::id(&DecimalByteParts), - expected.dtype(), - expected.len(), - &[8, 7], - &[], - &children, - ), - &session, - )?; - assert_arrays_eq!(expected, decoded, &mut ctx); - test_serde_round_trip(decoded.as_::().into_owned()) - } - - #[rstest] - #[case::i64(DecimalArray::new( - buffer![-99i64, 0, 99], DecimalDType::new(2, 0), Validity::NonNullable, - ))] - #[case::i128(DecimalArray::new( - buffer![-99i128, 0, 99], DecimalDType::new(2, 0), Validity::NonNullable, - ))] - #[case::i256(DecimalArray::new( - buffer![i256::from_i128(-99), i256::ZERO, i256::from_i128(99)], - DecimalDType::new(2, 0), Validity::NonNullable, - ))] - fn test_serde_round_trip_wider_storage(#[case] decimal: DecimalArray) -> VortexResult<()> { - let mut ctx = array_session().create_execution_ctx(); - let encoded = encode(&decimal)?; - assert_arrays_eq!(decimal, encoded, &mut ctx); - assert_eq!( - encoded.execute_scalar(0, &mut ctx)?, - decimal.execute_scalar(0, &mut ctx)?, - ); - test_serde_round_trip(encoded) - } - - fn test_serde_round_trip(array: DecimalBytePartsArray) -> VortexResult<()> { - let session = array_session(); - // Both serialized formats must be registered: an array with lower parts comes back - // under the v2 format id. - crate::initialize(&session); - - 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 expected_id = if lower_part_count == 0 { - VTable::id(&DecimalByteParts) - } else { - decimal_byte_parts_v2_id() - }; - assert_eq!( - session - .array_serialize(&array)? - .vortex_expect("byte parts arrays are serializable") - .serialized_id, - expected_id - ); - - 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() } @@ -1086,158 +813,6 @@ mod tests { assert!(Array::try_from_parts(parts).is_err()); } - fn deserialize_with( - lower_part_count: u32, - children: Vec, - ) -> VortexResult { - let serialized_id = if lower_part_count == 0 { - VTable::id(&DecimalByteParts) - } else { - decimal_byte_parts_v2_id() - }; - plugin_deserialize_with(serialized_id, lower_part_count, children) - .map(|array| array.as_::().into_owned()) - } - - #[test] - fn test_deserialize_reads_lower_parts() -> VortexResult<()> { - let array = deserialize_with(1, vec![msp(), lower_part()])?; - 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(()) - } - - /// An array read from a file can be handed straight back to a writer, bypassing both the - /// constructor and the compressor. Its serialized id must still be the v2 format, so a - /// writer whose permitted encodings predate the v2 format refuses it. - #[test] - fn read_lower_parts_serialize_under_the_wide_format() -> VortexResult<()> { - let session = array_session(); - crate::initialize(&session); - - let array = deserialize_with(1, vec![msp(), lower_part()])?.into_array(); - - let serialization = session - .array_serialize(&array)? - .vortex_expect("byte parts arrays are serializable"); - assert_eq!(serialization.serialized_id, decimal_byte_parts_v2_id()); - - let restricted = ArrayContext::empty() - .with_allowed_ids([VTable::id(&DecimalByteParts)].into_iter().collect()); - let err = array - .serialize(&restricted, &session, &SerializeOptions::default()) - .expect_err("expected the permitted-encoding check to refuse the v2 format"); - assert!( - err.to_string().contains("not permitted"), - "error should name the permitted-encoding check, got: {err}" - ); - Ok(()) - } - - /// Reading back an array that already carries lower parts, and computing over it, must - /// always work: the v2 format only restricts which writers may emit it. If reading or - /// the rebuild that every compute kernel does were blocked, a session whose editions - /// predate the v2 format could not read a file written by one that includes 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()])?.into_array(); - - let sliced = array.slice(0..2)?; - assert_eq!(sliced.execute::(&mut ctx)?.len(), 2); - Ok(()) - } - - #[rstest] - fn test_deserialize_redundant_lower_parts( - #[values(2, 3)] lower_part_count: u32, - ) -> VortexResult<()> { - let mut children = vec![buffer![0i64; 3].into_array()]; - children.extend((1..lower_part_count).map(|_| buffer![0u64; 3].into_array())); - children.push(lower_part()); - let array = deserialize_with(lower_part_count, children)?; - let expected = DecimalArray::new( - buffer![1i128, 2, 3], - DecimalDType::new(38, 2), - Validity::NonNullable, - ); - let mut ctx = array_session().create_execution_ctx(); - assert_arrays_eq!(expected, array, &mut ctx); - test_serde_round_trip(array) - } - - #[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() - ); - } - - fn plugin_deserialize_with( - serialized_id: ArrayId, - lower_part_count: u32, - children: Vec, - ) -> VortexResult { - let metadata = DecimalBytesPartsMetadata { - zeroth_child_ptype: PType::I64 as i32, - lower_part_count, - } - .encode_to_vec(); - let dtype = DType::Decimal(DecimalDType::new(38, 2), Nullability::NonNullable); - DecimalBytePartsPlugin.deserialize( - ArrayDeserialization::new(serialized_id, &dtype, 3, &metadata, &[], &children), - &array_session(), - ) - } - - /// Each serialized ID keeps its own contract: the frozen ID never carries lower parts, and - /// the v2 ID is never written without them. - #[rstest] - #[case::frozen_without_lower_parts(VTable::id(&DecimalByteParts), 0, vec![msp()], true)] - #[case::frozen_with_lower_parts( - VTable::id(&DecimalByteParts), - 1, - vec![msp(), lower_part()], - false - )] - #[case::v2_with_lower_parts(decimal_byte_parts_v2_id(), 1, vec![msp(), lower_part()], true)] - #[case::v2_without_lower_parts(decimal_byte_parts_v2_id(), 0, vec![msp()], false)] - #[case::unknown_id(ArrayVTable::id(&Primitive), 0, vec![msp()], false)] - fn plugin_holds_each_id_to_its_contract( - #[case] serialized_id: ArrayId, - #[case] lower_part_count: u32, - #[case] children: Vec, - #[case] accepted: bool, - ) { - let result = plugin_deserialize_with(serialized_id, lower_part_count, children); - assert_eq!(result.is_ok(), accepted, "{serialized_id}: {result:?}"); - } - #[test] fn test_wide_decimal_buffer_types() -> VortexResult<()> { let mut ctx = array_session().create_execution_ctx(); 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 919b7bb44a3..fe661fa9ef4 100644 --- a/encodings/decimal-byte-parts/src/decimal_byte_parts/mod.rs +++ b/encodings/decimal-byte-parts/src/decimal_byte_parts/mod.rs @@ -22,6 +22,7 @@ use vortex_array::dtype::PType; mod array; mod assemble; pub(crate) mod compute; +mod plugin; #[cfg(test)] mod prop_tests; mod rules; @@ -30,6 +31,8 @@ mod split; mod testing; pub use array::*; +pub use plugin::DecimalBytePartsPlugin; +pub use plugin::decimal_byte_parts_v2_id; pub use split::DecimalParts; pub use split::dbp_encode; pub use split::split_decimal; diff --git a/encodings/decimal-byte-parts/src/decimal_byte_parts/plugin.rs b/encodings/decimal-byte-parts/src/decimal_byte_parts/plugin.rs new file mode 100644 index 00000000000..4b31fb8b53e --- /dev/null +++ b/encodings/decimal-byte-parts/src/decimal_byte_parts/plugin.rs @@ -0,0 +1,649 @@ +// SPDX-License-Identifier: Apache-2.0 +// SPDX-FileCopyrightText: Copyright the Vortex contributors + +//! Serialization of decimal byte parts under the frozen and v2 format IDs. + +use prost::Message as _; +use vortex_array::Array; +use vortex_array::ArrayDeserialization; +use vortex_array::ArrayId; +use vortex_array::ArrayPlugin; +use vortex_array::ArrayRef; +use vortex_array::ArraySerialization; +use vortex_array::IntoArray; +use vortex_array::vtable::VTable; +use vortex_error::VortexResult; +use vortex_error::vortex_ensure; +use vortex_error::vortex_err; +use vortex_session::VortexSession; +use vortex_session::registry::CachedId; + +use super::DecimalByteParts; +use super::DecimalBytePartsArraySlotsExt; +use super::DecimalBytesPartsMetadata; + +/// The `vortex.decimal_byte_parts_v2` serialized format ID: byte parts carrying lower parts. +/// +/// This is a serialized format, not a second in-memory encoding. `vortex.decimal_byte_parts` +/// froze promising a single child, so an array with lower parts serializes under this ID +/// instead, and both IDs deserialize back into the same [`crate::DecimalBytePartsArray`]. A reader +/// that predates lower parts fails on this ID with an unknown-encoding error rather than +/// misreading the children. +pub fn decimal_byte_parts_v2_id() -> ArrayId { + static ID: CachedId = CachedId::new("vortex.decimal_byte_parts_v2"); + *ID +} + +/// The [`ArrayPlugin`] for [`DecimalByteParts`], owning both of its serialized formats. +/// +/// An array without lower parts serializes under the frozen `vortex.decimal_byte_parts` ID, +/// byte-identical to files written before lower parts existed. An array carrying lower parts +/// serializes under [`decimal_byte_parts_v2_id`]. Reading holds each ID to its own contract: +/// the frozen ID carries no lower parts and the v2 ID carries at least one, so recognizing the +/// newer format never widens what the frozen one may mean. +/// +/// Register this plugin, or call [`crate::initialize`], to enable both formats. Registering +/// [`DecimalByteParts`] directly only supports the frozen format. +#[derive(Clone, Debug)] +pub struct DecimalBytePartsPlugin; + +impl ArrayPlugin for DecimalBytePartsPlugin { + fn id(&self) -> ArrayId { + VTable::id(&DecimalByteParts) + } + + fn serialized_ids(&self) -> Vec { + vec![VTable::id(&DecimalByteParts), decimal_byte_parts_v2_id()] + } + + fn serialize( + &self, + array: &ArrayRef, + _session: &VortexSession, + ) -> VortexResult> { + let view = array.as_opt::().ok_or_else(|| { + vortex_err!( + "DecimalByteParts plugin cannot serialize {}", + array.encoding_id() + ) + })?; + let serialized_id = if view.lower_parts().is_empty() { + VTable::id(&DecimalByteParts) + } else { + decimal_byte_parts_v2_id() + }; + Ok(Some(ArraySerialization::from_array( + serialized_id, + array, + DecimalBytesPartsMetadata::from_array(view)?.encode_to_vec(), + ))) + } + + fn deserialize( + &self, + parts: ArrayDeserialization<'_>, + _session: &VortexSession, + ) -> VortexResult { + let metadata = DecimalBytesPartsMetadata::decode(parts.metadata)?; + let lower_part_count = metadata.lower_part_count()?; + if parts.serialized_id == decimal_byte_parts_v2_id() { + vortex_ensure!( + lower_part_count > 0, + "{} must carry at least one lower part", + parts.serialized_id + ); + } else { + vortex_ensure!( + parts.serialized_id == VTable::id(&DecimalByteParts), + "DecimalByteParts plugin does not recognize serialized ID {}", + parts.serialized_id + ); + vortex_ensure!( + lower_part_count == 0, + "{} must not carry lower parts, got {lower_part_count}", + parts.serialized_id + ); + } + Ok(Array::try_from_parts(metadata.into_array_parts( + parts.dtype, + parts.len, + parts.children, + )?)? + .into_array()) + } +} + +#[cfg(test)] +mod tests { + use rstest::rstest; + use vortex_array::ArrayContext; + use vortex_array::ArrayParts; + use vortex_array::ArraySlots; + use vortex_array::ArrayVTable; + use vortex_array::VortexSessionExecute; + use vortex_array::array_session; + use vortex_array::arrays::DecimalArray; + use vortex_array::arrays::Primitive; + 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::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::VortexExpect; + use vortex_session::registry::ReadContext; + + use super::*; + use crate::DecimalBytePartsArray; + use crate::DecimalBytePartsData; + use crate::decimal_byte_parts::testing::encode; + use crate::decimal_byte_parts::testing::i128_parts; + use crate::decimal_byte_parts::testing::i256_parts; + use crate::decimal_byte_parts::testing::wide_i128_values; + use crate::decimal_byte_parts::testing::wide_i256_values; + + #[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") + )] + fn test_serde_round_trip_flat(#[case] array: DecimalBytePartsArray) -> VortexResult<()> { + test_serde_round_trip(array) + } + + #[rstest] + fn test_deserialize_frozen_with_wider_storage( + #[values(Validity::NonNullable, Validity::from_iter([true, false, true]))] + validity: Validity, + ) -> VortexResult<()> { + let session = array_session(); + crate::initialize(&session); + let mut ctx = session.create_execution_ctx(); + let decimal_dtype = DecimalDType::new(2, 0); + let expected = DecimalArray::new(buffer![1i8, 2, 3], decimal_dtype, validity.clone()); + let children = vec![PrimitiveArray::new(buffer![1i64, 2, 3], validity).into_array()]; + + // Metadata emitted by the frozen serializer for a single i64 child. + let decoded = DecimalBytePartsPlugin.deserialize( + ArrayDeserialization::new( + VTable::id(&DecimalByteParts), + expected.dtype(), + expected.len(), + &[8, 7], + &[], + &children, + ), + &session, + )?; + assert_arrays_eq!(expected, decoded, &mut ctx); + test_serde_round_trip(decoded.as_::().into_owned()) + } + + #[rstest] + #[case::i64(DecimalArray::new( + buffer![-99i64, 0, 99], DecimalDType::new(2, 0), Validity::NonNullable, + ))] + #[case::i128(DecimalArray::new( + buffer![-99i128, 0, 99], DecimalDType::new(2, 0), Validity::NonNullable, + ))] + #[case::i256(DecimalArray::new( + buffer![i256::from_i128(-99), i256::ZERO, i256::from_i128(99)], + DecimalDType::new(2, 0), Validity::NonNullable, + ))] + fn test_serde_round_trip_wider_storage(#[case] decimal: DecimalArray) -> VortexResult<()> { + let mut ctx = array_session().create_execution_ctx(); + let encoded = encode(&decimal)?; + assert_arrays_eq!(decimal, encoded, &mut ctx); + assert_eq!( + encoded.execute_scalar(0, &mut ctx)?, + decimal.execute_scalar(0, &mut ctx)?, + ); + test_serde_round_trip(encoded) + } + + fn test_serde_round_trip(array: DecimalBytePartsArray) -> VortexResult<()> { + let session = array_session(); + // Both serialized formats must be registered: an array with lower parts comes back + // under the v2 format id. + crate::initialize(&session); + + 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 expected_id = if lower_part_count == 0 { + VTable::id(&DecimalByteParts) + } else { + decimal_byte_parts_v2_id() + }; + assert_eq!( + session + .array_serialize(&array)? + .vortex_expect("byte parts arrays are serializable") + .serialized_id, + expected_id + ); + + 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 deserialize_with( + lower_part_count: u32, + children: Vec, + ) -> VortexResult { + let serialized_id = if lower_part_count == 0 { + VTable::id(&DecimalByteParts) + } else { + decimal_byte_parts_v2_id() + }; + plugin_deserialize_with(serialized_id, lower_part_count, children) + .map(|array| array.as_::().into_owned()) + } + + #[test] + fn test_deserialize_reads_lower_parts() -> VortexResult<()> { + let array = deserialize_with(1, vec![msp(), lower_part()])?; + 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(()) + } + + /// An array read from a file can be handed straight back to a writer, bypassing both the + /// constructor and the compressor. Its serialized id must still be the v2 format, so a + /// writer whose permitted encodings predate the v2 format refuses it. + #[test] + fn read_lower_parts_serialize_under_the_wide_format() -> VortexResult<()> { + let session = array_session(); + crate::initialize(&session); + + let array = deserialize_with(1, vec![msp(), lower_part()])?.into_array(); + + let serialization = session + .array_serialize(&array)? + .vortex_expect("byte parts arrays are serializable"); + assert_eq!(serialization.serialized_id, decimal_byte_parts_v2_id()); + + let restricted = ArrayContext::empty() + .with_allowed_ids([VTable::id(&DecimalByteParts)].into_iter().collect()); + let err = array + .serialize(&restricted, &session, &SerializeOptions::default()) + .expect_err("expected the permitted-encoding check to refuse the v2 format"); + assert!( + err.to_string().contains("not permitted"), + "error should name the permitted-encoding check, got: {err}" + ); + Ok(()) + } + + /// Reading back an array that already carries lower parts, and computing over it, must + /// always work: the v2 format only restricts which writers may emit it. If reading or + /// the rebuild that every compute kernel does were blocked, a session whose editions + /// predate the v2 format could not read a file written by one that includes 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()])?.into_array(); + + let sliced = array.slice(0..2)?; + assert_eq!(sliced.execute::(&mut ctx)?.len(), 2); + Ok(()) + } + + #[rstest] + fn test_deserialize_redundant_lower_parts( + #[values(2, 3)] lower_part_count: u32, + ) -> VortexResult<()> { + let mut children = vec![buffer![0i64; 3].into_array()]; + children.extend((1..lower_part_count).map(|_| buffer![0u64; 3].into_array())); + children.push(lower_part()); + let array = deserialize_with(lower_part_count, children)?; + let expected = DecimalArray::new( + buffer![1i128, 2, 3], + DecimalDType::new(38, 2), + Validity::NonNullable, + ); + let mut ctx = array_session().create_execution_ctx(); + assert_arrays_eq!(expected, array, &mut ctx); + test_serde_round_trip(array) + } + + #[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() + ); + } + + fn plugin_deserialize_with( + serialized_id: ArrayId, + lower_part_count: u32, + children: Vec, + ) -> VortexResult { + let metadata = DecimalBytesPartsMetadata { + zeroth_child_ptype: PType::I64 as i32, + lower_part_count, + } + .encode_to_vec(); + let dtype = DType::Decimal(DecimalDType::new(38, 2), Nullability::NonNullable); + DecimalBytePartsPlugin.deserialize( + ArrayDeserialization::new(serialized_id, &dtype, 3, &metadata, &[], &children), + &array_session(), + ) + } + + /// Each serialized ID keeps its own contract: the frozen ID never carries lower parts, and + /// the v2 ID is never written without them. + #[rstest] + #[case::frozen_without_lower_parts(VTable::id(&DecimalByteParts), 0, vec![msp()], true)] + #[case::frozen_with_lower_parts( + VTable::id(&DecimalByteParts), + 1, + vec![msp(), lower_part()], + false + )] + #[case::v2_with_lower_parts(decimal_byte_parts_v2_id(), 1, vec![msp(), lower_part()], true)] + #[case::v2_without_lower_parts(decimal_byte_parts_v2_id(), 0, vec![msp()], false)] + #[case::unknown_id(ArrayVTable::id(&Primitive), 0, vec![msp()], false)] + fn plugin_holds_each_id_to_its_contract( + #[case] serialized_id: ArrayId, + #[case] lower_part_count: u32, + #[case] children: Vec, + #[case] accepted: bool, + ) { + let result = plugin_deserialize_with(serialized_id, lower_part_count, children); + assert_eq!(result.is_ok(), accepted, "{serialized_id}: {result:?}"); + } + + fn msp() -> ArrayRef { + buffer![1i64, 2, 3].into_array() + } + + fn lower_part() -> ArrayRef { + buffer![1u64, 2, 3].into_array() + } + + fn session() -> VortexSession { + let session = array_session(); + crate::initialize(&session); + session + } + + /// The wire ID the session's plugin picks for `array`. + fn serialized_id(session: &VortexSession, array: &ArrayRef) -> VortexResult { + Ok(session + .array_serialize(array)? + .ok_or_else(|| vortex_err!("byte parts arrays are serializable"))? + .serialized_id) + } + + /// 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() + ); + } + + /// Building lower parts in memory is always allowed — reading a file requires it. What + /// changes is the serialized format, not what can be constructed. + #[test] + fn lower_parts_can_always_be_constructed() { + assert!( + DecimalByteParts::try_new_with_lower_parts( + msp(), + vec![lower_part()], + DecimalDType::new(38, 2), + ) + .is_ok() + ); + } + + /// A single-child array keeps the frozen format id, byte-compatible with every reader since + /// the format froze; lower parts move the array onto the v2 format id. + #[test] + fn serialized_id_tracks_lower_parts() -> VortexResult<()> { + let session = session(); + + let flat = DecimalByteParts::try_new(msp(), DecimalDType::new(19, 2))?.into_array(); + assert_eq!( + serialized_id(&session, &flat)?, + ArrayVTable::id(&DecimalByteParts) + ); + + let wide = DecimalByteParts::try_new_with_lower_parts( + msp(), + vec![lower_part()], + DecimalDType::new(38, 2), + )? + .into_array(); + assert_eq!(serialized_id(&session, &wide)?, decimal_byte_parts_v2_id()); + + Ok(()) + } + + /// The permitted-encoding check applies to the serialized id. A context restricted to the + /// frozen format — a writer whose enabled editions predate the v2 format — must refuse an + /// array carrying lower parts, however it was obtained. + /// + /// `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. What must hold is that the resulting array cannot become + /// bytes under the frozen id. + #[test] + fn wide_format_is_refused_where_not_permitted() -> VortexResult<()> { + let session = session(); + + 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), + )? + .into_array(); + assert_eq!(array.nchildren(), 2, "expected two limbs"); + + // A context permitting only the frozen format refuses to write it. + let restricted = ArrayContext::empty() + .with_allowed_ids([ArrayVTable::id(&DecimalByteParts)].into_iter().collect()); + let err = array + .serialize(&restricted, &session, &SerializeOptions::default()) + .expect_err("expected the permitted-encoding check to refuse the v2 format"); + assert!( + err.to_string().contains("not permitted"), + "error should name the permitted-encoding check, got: {err}" + ); + + // Permitting the v2 format id is exactly what allows the same array through. + let permissive = ArrayContext::empty().with_allowed_ids( + [ + ArrayVTable::id(&DecimalByteParts), + decimal_byte_parts_v2_id(), + ArrayVTable::id(&Primitive), + ] + .into_iter() + .collect(), + ); + let serialized = array.serialize(&permissive, &session, &SerializeOptions::default())?; + assert!(!serialized.is_empty()); + assert!( + permissive.to_ids().contains(&decimal_byte_parts_v2_id()), + "the file's encoding table must carry the v2 format id" + ); + + Ok(()) + } + + #[test] + fn bare_vtable_refuses_wide_serialization() -> VortexResult<()> { + let session = array_session(); + session.arrays().register(DecimalByteParts); + let array = DecimalByteParts::try_new_with_lower_parts( + msp(), + vec![lower_part()], + DecimalDType::new(38, 2), + )? + .into_array(); + let restricted = ArrayContext::empty().with_allowed_ids( + [ + ArrayVTable::id(&DecimalByteParts), + ArrayVTable::id(&Primitive), + ] + .into_iter() + .collect(), + ); + + assert!( + array + .serialize(&restricted, &session, &SerializeOptions::default()) + .is_err(), + "bare VTable registration must not write lower parts under the frozen ID" + ); + Ok(()) + } + + #[test] + fn bare_vtable_refuses_lower_parts_on_frozen_id() -> VortexResult<()> { + let session = array_session(); + session.arrays().register(DecimalByteParts); + let array = DecimalByteParts::try_new_with_lower_parts( + msp(), + vec![lower_part()], + DecimalDType::new(38, 2), + )? + .into_array(); + let id = ArrayVTable::id(&DecimalByteParts); + let plugin = session + .arrays() + .registry() + .get(&id) + .ok_or_else(|| vortex_err!("missing decimal plugin"))?; + let children = array.children(); + + // i64 MSP and one lower part, mislabeled as the frozen format. + let parts = ArrayDeserialization::new( + id, + array.dtype(), + array.len(), + &[8, 7, 16, 1], + &[], + &children, + ); + assert!(plugin.deserialize(parts, &session).is_err()); + Ok(()) + } + + #[rstest] + #[case::vtable(false)] + #[case::plugin(true)] + fn frozen_serde_is_compatible(#[case] use_plugin: bool) -> VortexResult<()> { + let session = array_session(); + if use_plugin { + session.arrays().register(DecimalBytePartsPlugin); + } else { + session.arrays().register(DecimalByteParts); + } + let array = DecimalByteParts::try_new(msp(), DecimalDType::new(2, 0))?.into_array(); + let serialized = session + .array_serialize(&array)? + .ok_or_else(|| vortex_err!("missing decimal serialization"))?; + assert_eq!(serialized.serialized_id, ArrayVTable::id(&DecimalByteParts)); + assert_eq!(serialized.metadata, [8, 7]); + let plugin = session + .arrays() + .registry() + .get(&serialized.serialized_id) + .ok_or_else(|| vortex_err!("missing decimal plugin"))?; + let decoded = plugin.deserialize( + ArrayDeserialization::new( + serialized.serialized_id, + array.dtype(), + array.len(), + &serialized.metadata, + &[], + &serialized.children, + ), + &session, + )?; + assert_arrays_eq!(array, decoded, &mut session.create_execution_ctx()); + Ok(()) + } +} 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 2dfe2a55b3c..537dde93746 100644 --- a/encodings/decimal-byte-parts/src/decimal_byte_parts/testing.rs +++ b/encodings/decimal-byte-parts/src/decimal_byte_parts/testing.rs @@ -45,3 +45,44 @@ pub(crate) fn i256_parts(values: Vec, validity: Validity) -> DecimalBytePa pub(super) fn i256_of(high: i128, low: u128) -> i256 { i256::from_parts(low, high) } + +/// 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. +pub(crate) 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`. +pub(crate) 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(), + ] +} diff --git a/encodings/decimal-byte-parts/tests/format_v2.rs b/encodings/decimal-byte-parts/tests/format_v2.rs deleted file mode 100644 index 338598df032..00000000000 --- a/encodings/decimal-byte-parts/tests/format_v2.rs +++ /dev/null @@ -1,257 +0,0 @@ -// SPDX-License-Identifier: Apache-2.0 -// SPDX-FileCopyrightText: Copyright the Vortex contributors - -//! The v2 serialized format. -//! -//! Lower parts can be built and computed over freely. What changes with them is the bytes: -//! an array carrying lower parts serializes under `vortex.decimal_byte_parts_v2` rather -//! than the frozen `vortex.decimal_byte_parts` format, so a writer restricted to editions -//! without the v2 format refuses it, and a reader that predates lower parts fails with an -//! unknown-encoding error instead of misreading the children. These tests pin all of that: -//! construction always works, the serialized id tracks the parts, and the permitted-encoding -//! check applies to the serialized id. - -#![expect(clippy::tests_outside_test_module)] - -use vortex_array::ArrayContext; -use vortex_array::ArrayDeserialization; -use vortex_array::ArrayId; -use vortex_array::ArrayRef; -use vortex_array::ArrayVTable; -use vortex_array::IntoArray; -use vortex_array::VortexSessionExecute; -use vortex_array::assert_arrays_eq; -use vortex_array::dtype::DecimalDType; -use vortex_array::serde::SerializeOptions; -use vortex_array::session::ArraySessionExt; -use vortex_buffer::buffer; -use vortex_decimal_byte_parts::DecimalByteParts; -use vortex_decimal_byte_parts::decimal_byte_parts_v2_id; -use vortex_error::VortexResult; -use vortex_error::vortex_err; -use vortex_session::VortexSession; - -fn msp() -> ArrayRef { - buffer![1i64, 2, 3].into_array() -} - -fn lower_part() -> ArrayRef { - buffer![1u64, 2, 3].into_array() -} - -fn session() -> VortexSession { - let session = vortex_array::array_session(); - vortex_decimal_byte_parts::initialize(&session); - session -} - -/// The wire ID the session's plugin picks for `array`. -fn serialized_id(session: &VortexSession, array: &ArrayRef) -> VortexResult { - Ok(session - .array_serialize(array)? - .ok_or_else(|| vortex_err!("byte parts arrays are serializable"))? - .serialized_id) -} - -/// 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() - ); -} - -/// Building lower parts in memory is always allowed — reading a file requires it. What -/// changes is the serialized format, not what can be constructed. -#[test] -fn lower_parts_can_always_be_constructed() { - assert!( - DecimalByteParts::try_new_with_lower_parts( - msp(), - vec![lower_part()], - DecimalDType::new(38, 2), - ) - .is_ok() - ); -} - -/// A single-child array keeps the frozen format id, byte-compatible with every reader since -/// the format froze; lower parts move the array onto the v2 format id. -#[test] -fn serialized_id_tracks_lower_parts() -> VortexResult<()> { - let session = session(); - - let flat = DecimalByteParts::try_new(msp(), DecimalDType::new(19, 2))?.into_array(); - assert_eq!( - serialized_id(&session, &flat)?, - ArrayVTable::id(&DecimalByteParts) - ); - - let wide = DecimalByteParts::try_new_with_lower_parts( - msp(), - vec![lower_part()], - DecimalDType::new(38, 2), - )? - .into_array(); - assert_eq!(serialized_id(&session, &wide)?, decimal_byte_parts_v2_id()); - - Ok(()) -} - -/// The permitted-encoding check applies to the serialized id. A context restricted to the -/// frozen format — a writer whose enabled editions predate the v2 format — must refuse an -/// array carrying lower parts, however it was obtained. -/// -/// `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. What must hold is that the resulting array cannot become -/// bytes under the frozen id. -#[test] -fn wide_format_is_refused_where_not_permitted() -> VortexResult<()> { - use vortex_array::Array; - use vortex_array::ArrayParts; - use vortex_array::ArraySlots; - use vortex_array::dtype::DType; - use vortex_array::dtype::Nullability; - use vortex_decimal_byte_parts::DecimalBytePartsData; - - let session = session(); - - 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), - )? - .into_array(); - assert_eq!(array.nchildren(), 2, "expected two limbs"); - - // A context permitting only the frozen format refuses to write it. - let restricted = ArrayContext::empty() - .with_allowed_ids([ArrayVTable::id(&DecimalByteParts)].into_iter().collect()); - let err = array - .serialize(&restricted, &session, &SerializeOptions::default()) - .expect_err("expected the permitted-encoding check to refuse the v2 format"); - assert!( - err.to_string().contains("not permitted"), - "error should name the permitted-encoding check, got: {err}" - ); - - // Permitting the v2 format id is exactly what allows the same array through. - let permissive = ArrayContext::empty().with_allowed_ids( - [ - ArrayVTable::id(&DecimalByteParts), - decimal_byte_parts_v2_id(), - ArrayVTable::id(&vortex_array::arrays::Primitive), - ] - .into_iter() - .collect(), - ); - let serialized = array.serialize(&permissive, &session, &SerializeOptions::default())?; - assert!(!serialized.is_empty()); - assert!( - permissive.to_ids().contains(&decimal_byte_parts_v2_id()), - "the file's encoding table must carry the v2 format id" - ); - - Ok(()) -} - -#[test] -fn bare_vtable_refuses_wide_serialization() -> VortexResult<()> { - let session = vortex_array::array_session(); - session.arrays().register(DecimalByteParts); - let array = DecimalByteParts::try_new_with_lower_parts( - msp(), - vec![lower_part()], - DecimalDType::new(38, 2), - )? - .into_array(); - let restricted = ArrayContext::empty().with_allowed_ids( - [ - ArrayVTable::id(&DecimalByteParts), - ArrayVTable::id(&vortex_array::arrays::Primitive), - ] - .into_iter() - .collect(), - ); - - assert!( - array - .serialize(&restricted, &session, &SerializeOptions::default()) - .is_err(), - "bare VTable registration must not write lower parts under the frozen ID" - ); - Ok(()) -} - -#[test] -fn bare_vtable_refuses_lower_parts_on_frozen_id() -> VortexResult<()> { - let session = vortex_array::array_session(); - session.arrays().register(DecimalByteParts); - let array = DecimalByteParts::try_new_with_lower_parts( - msp(), - vec![lower_part()], - DecimalDType::new(38, 2), - )? - .into_array(); - let id = ArrayVTable::id(&DecimalByteParts); - let plugin = session - .arrays() - .registry() - .get(&id) - .ok_or_else(|| vortex_err!("missing decimal plugin"))?; - let children = array.children(); - - // i64 MSP and one lower part, mislabeled as the frozen format. - let parts = ArrayDeserialization::new( - id, - array.dtype(), - array.len(), - &[8, 7, 16, 1], - &[], - &children, - ); - assert!(plugin.deserialize(parts, &session).is_err()); - Ok(()) -} - -#[test] -fn bare_vtable_keeps_frozen_serde() -> VortexResult<()> { - let session = vortex_array::array_session(); - session.arrays().register(DecimalByteParts); - let array = DecimalByteParts::try_new(msp(), DecimalDType::new(2, 0))?.into_array(); - let serialized = session - .array_serialize(&array)? - .ok_or_else(|| vortex_err!("missing decimal serialization"))?; - assert_eq!(serialized.serialized_id, ArrayVTable::id(&DecimalByteParts)); - assert_eq!(serialized.metadata, [8, 7]); - let plugin = session - .arrays() - .registry() - .get(&serialized.serialized_id) - .ok_or_else(|| vortex_err!("missing decimal plugin"))?; - let decoded = plugin.deserialize( - ArrayDeserialization::new( - serialized.serialized_id, - array.dtype(), - array.len(), - &serialized.metadata, - &[], - &serialized.children, - ), - &session, - )?; - assert_arrays_eq!(array, decoded, &mut session.create_execution_ctx()); - Ok(()) -} diff --git a/vortex-test/compat-gen/Cargo.toml b/vortex-test/compat-gen/Cargo.toml index 2a5fe657d9b..4a62aca3671 100644 --- a/vortex-test/compat-gen/Cargo.toml +++ b/vortex-test/compat-gen/Cargo.toml @@ -20,11 +20,6 @@ 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 5af7596ca7b..4d799e33e74 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,7 +12,6 @@ mod bytebool; mod constant; mod datetimeparts; mod decimal_byte_parts; -#[cfg(feature = "unstable_encodings")] mod decimal_byte_parts_v2; mod delta; mod dict; @@ -33,14 +32,14 @@ pub(crate) const N: usize = 1024; /// All per-encoding fixtures. pub fn fixtures() -> Vec> { - #[allow(unused_mut)] - let mut fixtures: Vec> = 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_v2::DecimalBytePartsV2Fixture), // Re-enable this once delta is stable // Box::new(delta::DeltaFixture), Box::new(dict::DictFixture), @@ -56,8 +55,5 @@ 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_v2::DecimalBytePartsV2Fixture)); - fixtures + ] } From e2d33f0026564f5071d597a031a2242dfe0395c3 Mon Sep 17 00:00:00 2001 From: Matt Katz Date: Wed, 9 Sep 2026 17:07:26 -0400 Subject: [PATCH 03/17] Simplify decimal byte-parts plugin tests Signed-off-by: Matt Katz --- .../src/decimal_byte_parts/plugin.rs | 414 +++--------------- 1 file changed, 58 insertions(+), 356 deletions(-) diff --git a/encodings/decimal-byte-parts/src/decimal_byte_parts/plugin.rs b/encodings/decimal-byte-parts/src/decimal_byte_parts/plugin.rs index 4b31fb8b53e..e7fb773e049 100644 --- a/encodings/decimal-byte-parts/src/decimal_byte_parts/plugin.rs +++ b/encodings/decimal-byte-parts/src/decimal_byte_parts/plugin.rs @@ -22,13 +22,11 @@ use super::DecimalByteParts; use super::DecimalBytePartsArraySlotsExt; use super::DecimalBytesPartsMetadata; -/// The `vortex.decimal_byte_parts_v2` serialized format ID: byte parts carrying lower parts. +/// The `vortex.decimal_byte_parts_v2` serialized format ID, for `DecimalBytePartsArray`s carrying +/// lower parts. /// -/// This is a serialized format, not a second in-memory encoding. `vortex.decimal_byte_parts` -/// froze promising a single child, so an array with lower parts serializes under this ID -/// instead, and both IDs deserialize back into the same [`crate::DecimalBytePartsArray`]. A reader -/// that predates lower parts fails on this ID with an unknown-encoding error rather than -/// misreading the children. +/// The `vortex.decimal_byte_parts` Id corresponds to the previous version of the `DecimalBytePartsArray`, +/// which does not support lower parts. Both IDs deserialize back into the same `DecimalBytePartsArray`. pub fn decimal_byte_parts_v2_id() -> ArrayId { static ID: CachedId = CachedId::new("vortex.decimal_byte_parts_v2"); *ID @@ -39,8 +37,7 @@ pub fn decimal_byte_parts_v2_id() -> ArrayId { /// An array without lower parts serializes under the frozen `vortex.decimal_byte_parts` ID, /// byte-identical to files written before lower parts existed. An array carrying lower parts /// serializes under [`decimal_byte_parts_v2_id`]. Reading holds each ID to its own contract: -/// the frozen ID carries no lower parts and the v2 ID carries at least one, so recognizing the -/// newer format never widens what the frozen one may mean. +/// the frozen ID carries no lower parts and the v2 ID carries at least one. /// /// Register this plugin, or call [`crate::initialize`], to enable both formats. Registering /// [`DecimalByteParts`] directly only supports the frozen format. @@ -117,14 +114,11 @@ impl ArrayPlugin for DecimalBytePartsPlugin { mod tests { use rstest::rstest; use vortex_array::ArrayContext; - use vortex_array::ArrayParts; - use vortex_array::ArraySlots; use vortex_array::ArrayVTable; use vortex_array::VortexSessionExecute; use vortex_array::array_session; use vortex_array::arrays::DecimalArray; use vortex_array::arrays::Primitive; - use vortex_array::arrays::PrimitiveArray; use vortex_array::assert_arrays_eq; use vortex_array::dtype::DType; use vortex_array::dtype::DecimalDType; @@ -142,7 +136,6 @@ mod tests { use super::*; use crate::DecimalBytePartsArray; - use crate::DecimalBytePartsData; use crate::decimal_byte_parts::testing::encode; use crate::decimal_byte_parts::testing::i128_parts; use crate::decimal_byte_parts::testing::i256_parts; @@ -150,88 +143,44 @@ mod tests { use crate::decimal_byte_parts::testing::wide_i256_values; #[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") - )] - fn test_serde_round_trip_flat(#[case] array: DecimalBytePartsArray) -> VortexResult<()> { - test_serde_round_trip(array) - } - - #[rstest] - fn test_deserialize_frozen_with_wider_storage( - #[values(Validity::NonNullable, Validity::from_iter([true, false, true]))] - validity: Validity, - ) -> VortexResult<()> { - let session = array_session(); - crate::initialize(&session); - let mut ctx = session.create_execution_ctx(); - let decimal_dtype = DecimalDType::new(2, 0); - let expected = DecimalArray::new(buffer![1i8, 2, 3], decimal_dtype, validity.clone()); - let children = vec![PrimitiveArray::new(buffer![1i64, 2, 3], validity).into_array()]; - - // Metadata emitted by the frozen serializer for a single i64 child. - let decoded = DecimalBytePartsPlugin.deserialize( - ArrayDeserialization::new( - VTable::id(&DecimalByteParts), - expected.dtype(), - expected.len(), - &[8, 7], - &[], - &children, - ), - &session, - )?; - assert_arrays_eq!(expected, decoded, &mut ctx); - test_serde_round_trip(decoded.as_::().into_owned()) - } - - #[rstest] - #[case::i64(DecimalArray::new( - buffer![-99i64, 0, 99], DecimalDType::new(2, 0), Validity::NonNullable, + #[case::no_lower_parts(DecimalByteParts::try_new( + buffer![1i32, 2, 3].into_array(), DecimalDType::new(9, 2), ))] - #[case::i128(DecimalArray::new( + #[case::one_lower_part(Ok(i128_parts(wide_i128_values(), Validity::NonNullable)))] + #[case::three_lower_parts(Ok(i256_parts(wide_i256_values(), Validity::NonNullable)))] + #[case::nullable_three_lower_parts(Ok(i256_parts( + wide_i256_values(), + Validity::from_iter([true, false, true, true, true, false, true, true, true, true]), + )))] + #[case::wider_i64_storage(encode(&DecimalArray::new( + buffer![-99i64, 0, 99], DecimalDType::new(2, 0), Validity::NonNullable, + )))] + #[case::wider_i128_storage(encode(&DecimalArray::new( buffer![-99i128, 0, 99], DecimalDType::new(2, 0), Validity::NonNullable, - ))] - #[case::i256(DecimalArray::new( + )))] + #[case::wider_i256_storage(encode(&DecimalArray::new( buffer![i256::from_i128(-99), i256::ZERO, i256::from_i128(99)], DecimalDType::new(2, 0), Validity::NonNullable, + )))] + #[case::redundant_two_lower_parts(DecimalByteParts::try_new_with_lower_parts( + buffer![0i64; 3].into_array(), + vec![buffer![0u64; 3].into_array(), lower_part()], + DecimalDType::new(38, 2), ))] - fn test_serde_round_trip_wider_storage(#[case] decimal: DecimalArray) -> VortexResult<()> { - let mut ctx = array_session().create_execution_ctx(); - let encoded = encode(&decimal)?; - assert_arrays_eq!(decimal, encoded, &mut ctx); - assert_eq!( - encoded.execute_scalar(0, &mut ctx)?, - decimal.execute_scalar(0, &mut ctx)?, - ); - test_serde_round_trip(encoded) - } - - fn test_serde_round_trip(array: DecimalBytePartsArray) -> VortexResult<()> { - let session = array_session(); - // Both serialized formats must be registered: an array with lower parts comes back - // under the v2 format id. - crate::initialize(&session); - + #[case::redundant_three_lower_parts(DecimalByteParts::try_new_with_lower_parts( + buffer![0i64; 3].into_array(), + vec![buffer![0u64; 3].into_array(), buffer![0u64; 3].into_array(), lower_part()], + DecimalDType::new(38, 2), + ))] + fn test_serde_round_trip( + #[case] array: VortexResult, + ) -> VortexResult<()> { + let session = session(); + let array = array?; + let lower_part_count = array.lower_parts().len(); 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 expected_id = if lower_part_count == 0 { VTable::id(&DecimalByteParts) @@ -270,116 +219,22 @@ mod tests { Ok(()) } - fn deserialize_with( - lower_part_count: u32, - children: Vec, - ) -> VortexResult { + #[rstest] + #[case::missing_lower_part(1, vec![msp()])] + #[case::extra_lower_part(0, vec![msp(), lower_part()])] + #[case::too_many_lower_parts( + 4, vec![msp(), lower_part(), lower_part(), lower_part(), lower_part()], + )] + fn test_deserialize_rejects_child_count_mismatch( + #[case] lower_part_count: u32, + #[case] children: Vec, + ) { let serialized_id = if lower_part_count == 0 { VTable::id(&DecimalByteParts) } else { decimal_byte_parts_v2_id() }; - plugin_deserialize_with(serialized_id, lower_part_count, children) - .map(|array| array.as_::().into_owned()) - } - - #[test] - fn test_deserialize_reads_lower_parts() -> VortexResult<()> { - let array = deserialize_with(1, vec![msp(), lower_part()])?; - 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(()) - } - - /// An array read from a file can be handed straight back to a writer, bypassing both the - /// constructor and the compressor. Its serialized id must still be the v2 format, so a - /// writer whose permitted encodings predate the v2 format refuses it. - #[test] - fn read_lower_parts_serialize_under_the_wide_format() -> VortexResult<()> { - let session = array_session(); - crate::initialize(&session); - - let array = deserialize_with(1, vec![msp(), lower_part()])?.into_array(); - - let serialization = session - .array_serialize(&array)? - .vortex_expect("byte parts arrays are serializable"); - assert_eq!(serialization.serialized_id, decimal_byte_parts_v2_id()); - - let restricted = ArrayContext::empty() - .with_allowed_ids([VTable::id(&DecimalByteParts)].into_iter().collect()); - let err = array - .serialize(&restricted, &session, &SerializeOptions::default()) - .expect_err("expected the permitted-encoding check to refuse the v2 format"); - assert!( - err.to_string().contains("not permitted"), - "error should name the permitted-encoding check, got: {err}" - ); - Ok(()) - } - - /// Reading back an array that already carries lower parts, and computing over it, must - /// always work: the v2 format only restricts which writers may emit it. If reading or - /// the rebuild that every compute kernel does were blocked, a session whose editions - /// predate the v2 format could not read a file written by one that includes 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()])?.into_array(); - - let sliced = array.slice(0..2)?; - assert_eq!(sliced.execute::(&mut ctx)?.len(), 2); - Ok(()) - } - - #[rstest] - fn test_deserialize_redundant_lower_parts( - #[values(2, 3)] lower_part_count: u32, - ) -> VortexResult<()> { - let mut children = vec![buffer![0i64; 3].into_array()]; - children.extend((1..lower_part_count).map(|_| buffer![0u64; 3].into_array())); - children.push(lower_part()); - let array = deserialize_with(lower_part_count, children)?; - let expected = DecimalArray::new( - buffer![1i128, 2, 3], - DecimalDType::new(38, 2), - Validity::NonNullable, - ); - let mut ctx = array_session().create_execution_ctx(); - assert_arrays_eq!(expected, array, &mut ctx); - test_serde_round_trip(array) - } - - #[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() - ); + assert!(plugin_deserialize_with(serialized_id, lower_part_count, children).is_err()); } fn plugin_deserialize_with( @@ -411,7 +266,6 @@ mod tests { )] #[case::v2_with_lower_parts(decimal_byte_parts_v2_id(), 1, vec![msp(), lower_part()], true)] #[case::v2_without_lower_parts(decimal_byte_parts_v2_id(), 0, vec![msp()], false)] - #[case::unknown_id(ArrayVTable::id(&Primitive), 0, vec![msp()], false)] fn plugin_holds_each_id_to_its_contract( #[case] serialized_id: ArrayId, #[case] lower_part_count: u32, @@ -436,94 +290,24 @@ mod tests { session } - /// The wire ID the session's plugin picks for `array`. - fn serialized_id(session: &VortexSession, array: &ArrayRef) -> VortexResult { - Ok(session - .array_serialize(array)? - .ok_or_else(|| vortex_err!("byte parts arrays are serializable"))? - .serialized_id) - } - - /// 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() - ); - } - - /// Building lower parts in memory is always allowed — reading a file requires it. What - /// changes is the serialized format, not what can be constructed. - #[test] - fn lower_parts_can_always_be_constructed() { - assert!( - DecimalByteParts::try_new_with_lower_parts( - msp(), - vec![lower_part()], - DecimalDType::new(38, 2), - ) - .is_ok() - ); - } - - /// A single-child array keeps the frozen format id, byte-compatible with every reader since - /// the format froze; lower parts move the array onto the v2 format id. - #[test] - fn serialized_id_tracks_lower_parts() -> VortexResult<()> { + fn serialization_requires_v2_permission() -> VortexResult<()> { let session = session(); - - let flat = DecimalByteParts::try_new(msp(), DecimalDType::new(19, 2))?.into_array(); - assert_eq!( - serialized_id(&session, &flat)?, - ArrayVTable::id(&DecimalByteParts) - ); - - let wide = DecimalByteParts::try_new_with_lower_parts( + let array = DecimalByteParts::try_new_with_lower_parts( msp(), vec![lower_part()], DecimalDType::new(38, 2), )? .into_array(); - assert_eq!(serialized_id(&session, &wide)?, decimal_byte_parts_v2_id()); - - Ok(()) - } - - /// The permitted-encoding check applies to the serialized id. A context restricted to the - /// frozen format — a writer whose enabled editions predate the v2 format — must refuse an - /// array carrying lower parts, however it was obtained. - /// - /// `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. What must hold is that the resulting array cannot become - /// bytes under the frozen id. - #[test] - fn wide_format_is_refused_where_not_permitted() -> VortexResult<()> { - let session = session(); - - 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), - )? - .into_array(); - assert_eq!(array.nchildren(), 2, "expected two limbs"); - // A context permitting only the frozen format refuses to write it. - let restricted = ArrayContext::empty() - .with_allowed_ids([ArrayVTable::id(&DecimalByteParts)].into_iter().collect()); + let restricted = ArrayContext::empty().with_allowed_ids( + [ + ArrayVTable::id(&DecimalByteParts), + ArrayVTable::id(&Primitive), + ] + .into_iter() + .collect(), + ); let err = array .serialize(&restricted, &session, &SerializeOptions::default()) .expect_err("expected the permitted-encoding check to refuse the v2 format"); @@ -542,8 +326,7 @@ mod tests { .into_iter() .collect(), ); - let serialized = array.serialize(&permissive, &session, &SerializeOptions::default())?; - assert!(!serialized.is_empty()); + array.serialize(&permissive, &session, &SerializeOptions::default())?; assert!( permissive.to_ids().contains(&decimal_byte_parts_v2_id()), "the file's encoding table must carry the v2 format id" @@ -562,88 +345,7 @@ mod tests { DecimalDType::new(38, 2), )? .into_array(); - let restricted = ArrayContext::empty().with_allowed_ids( - [ - ArrayVTable::id(&DecimalByteParts), - ArrayVTable::id(&Primitive), - ] - .into_iter() - .collect(), - ); - - assert!( - array - .serialize(&restricted, &session, &SerializeOptions::default()) - .is_err(), - "bare VTable registration must not write lower parts under the frozen ID" - ); - Ok(()) - } - - #[test] - fn bare_vtable_refuses_lower_parts_on_frozen_id() -> VortexResult<()> { - let session = array_session(); - session.arrays().register(DecimalByteParts); - let array = DecimalByteParts::try_new_with_lower_parts( - msp(), - vec![lower_part()], - DecimalDType::new(38, 2), - )? - .into_array(); - let id = ArrayVTable::id(&DecimalByteParts); - let plugin = session - .arrays() - .registry() - .get(&id) - .ok_or_else(|| vortex_err!("missing decimal plugin"))?; - let children = array.children(); - - // i64 MSP and one lower part, mislabeled as the frozen format. - let parts = ArrayDeserialization::new( - id, - array.dtype(), - array.len(), - &[8, 7, 16, 1], - &[], - &children, - ); - assert!(plugin.deserialize(parts, &session).is_err()); - Ok(()) - } - - #[rstest] - #[case::vtable(false)] - #[case::plugin(true)] - fn frozen_serde_is_compatible(#[case] use_plugin: bool) -> VortexResult<()> { - let session = array_session(); - if use_plugin { - session.arrays().register(DecimalBytePartsPlugin); - } else { - session.arrays().register(DecimalByteParts); - } - let array = DecimalByteParts::try_new(msp(), DecimalDType::new(2, 0))?.into_array(); - let serialized = session - .array_serialize(&array)? - .ok_or_else(|| vortex_err!("missing decimal serialization"))?; - assert_eq!(serialized.serialized_id, ArrayVTable::id(&DecimalByteParts)); - assert_eq!(serialized.metadata, [8, 7]); - let plugin = session - .arrays() - .registry() - .get(&serialized.serialized_id) - .ok_or_else(|| vortex_err!("missing decimal plugin"))?; - let decoded = plugin.deserialize( - ArrayDeserialization::new( - serialized.serialized_id, - array.dtype(), - array.len(), - &serialized.metadata, - &[], - &serialized.children, - ), - &session, - )?; - assert_arrays_eq!(array, decoded, &mut session.create_execution_ctx()); + assert!(session.array_serialize(&array).is_err()); Ok(()) } } From 69cf104c0f9435623d70f6e16417bbd5d3f0ad35 Mon Sep 17 00:00:00 2001 From: Matt Katz Date: Wed, 9 Sep 2026 17:13:02 -0400 Subject: [PATCH 04/17] fix comment Signed-off-by: Matt Katz --- .../synthetic/encodings/decimal_byte_parts_v2.rs | 13 +------------ 1 file changed, 1 insertion(+), 12 deletions(-) diff --git a/vortex-test/compat-gen/src/fixtures/arrays/synthetic/encodings/decimal_byte_parts_v2.rs b/vortex-test/compat-gen/src/fixtures/arrays/synthetic/encodings/decimal_byte_parts_v2.rs index dfdcd893860..166d84f5ac6 100644 --- a/vortex-test/compat-gen/src/fixtures/arrays/synthetic/encodings/decimal_byte_parts_v2.rs +++ b/vortex-test/compat-gen/src/fixtures/arrays/synthetic/encodings/decimal_byte_parts_v2.rs @@ -1,18 +1,7 @@ // 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. +//! `DecimalByteParts` fixture for wide decimal values that need lower parts. use vortex::array::ArrayId; use vortex::array::ArrayRef; From 46de82afd245f83fa4ffa14c3481a36dd3cd4515 Mon Sep 17 00:00:00 2001 From: Matt Katz Date: Tue, 15 Sep 2026 00:55:46 -0400 Subject: [PATCH 05/17] Centralize decimal byte-parts serde and preserve narrowed types Signed-off-by: Matt Katz --- .../src/decimal_byte_parts/array.rs | 107 +------- .../src/decimal_byte_parts/mod.rs | 1 + .../src/decimal_byte_parts/plugin.rs | 242 +++++++++++++++++- 3 files changed, 241 insertions(+), 109 deletions(-) diff --git a/encodings/decimal-byte-parts/src/decimal_byte_parts/array.rs b/encodings/decimal-byte-parts/src/decimal_byte_parts/array.rs index 1ec6426f33e..59df7486a0e 100644 --- a/encodings/decimal-byte-parts/src/decimal_byte_parts/array.rs +++ b/encodings/decimal-byte-parts/src/decimal_byte_parts/array.rs @@ -7,14 +7,12 @@ use std::fmt::Display; use std::fmt::Formatter; use std::hash::Hasher; -use prost::Message as _; use vortex_array::Array; use vortex_array::ArrayEq; use vortex_array::ArrayHash; use vortex_array::ArrayId; use vortex_array::ArrayParts; use vortex_array::ArrayRef; -use vortex_array::ArraySlots; use vortex_array::ArrayView; use vortex_array::EqMode; use vortex_array::ExecutionCtx; @@ -24,7 +22,6 @@ use vortex_array::array_slots; use vortex_array::buffer::BufferHandle; use vortex_array::dtype::DType; use vortex_array::dtype::DecimalDType; -use vortex_array::dtype::PType; use vortex_array::scalar::DecimalValue; use vortex_array::scalar::Scalar; use vortex_array::scalar::ScalarValue; @@ -37,12 +34,10 @@ 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 super::LOWER_PART_DTYPE; use super::MAX_LOWER_PARTS; use super::assemble::assemble_decimal; use super::assemble::assemble_wide_decimal_value; @@ -51,78 +46,6 @@ use super::rules::PARENT_RULES; /// A [`DecimalByteParts`]-encoded Vortex array. pub type DecimalBytePartsArray = Array; -#[derive(Clone, prost::Message)] -pub struct DecimalBytesPartsMetadata { - #[prost(enumeration = "PType", tag = "1")] - pub(super) zeroth_child_ptype: i32, - #[prost(uint32, tag = "2")] - pub(super) lower_part_count: u32, -} - -impl DecimalBytesPartsMetadata { - pub(super) fn from_array(array: ArrayView<'_, DecimalByteParts>) -> VortexResult { - Ok(Self { - zeroth_child_ptype: PType::try_from(array.msp().dtype())? as i32, - lower_part_count: u32::try_from(array.lower_parts().len()) - .map_err(|_| vortex_err!("lower part count exceeds u32"))?, - }) - } - - pub(super) fn into_array_parts( - self, - dtype: &DType, - len: usize, - children: &dyn ArrayChildren, - ) -> VortexResult> { - vortex_ensure!( - dtype.as_decimal_opt().is_some(), - "decoding decimal but given non decimal dtype {dtype}" - ); - - let encoded_dtype = DType::Primitive(self.zeroth_child_ptype(), dtype.nullability()); - - let lower_part_count = self.lower_part_count()?; - vortex_ensure!( - children.len() == DecimalBytePartsSlots::FIXED_COUNT + lower_part_count, - "expected {} children, got {}", - DecimalBytePartsSlots::FIXED_COUNT + lower_part_count, - children.len() - ); - - 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(DecimalByteParts, dtype.clone(), len, DecimalBytePartsData) - .with_slots(slots), - ) - } - - /// The number of lower parts encoded in this array. - /// - /// # Errors - /// - /// Returns an error if the count exceeds [`MAX_LOWER_PARTS`]. - pub(super) 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!( - count <= MAX_LOWER_PARTS, - "at most {MAX_LOWER_PARTS} lower parts are supported, got {count}" - ); - Ok(count) - } -} - /// This array encodes decimals by splitting them between 1-4 columns of primitive typed children. /// /// The most significant part (MSP) stores the most significant decimal bits. It is signed and is @@ -201,6 +124,11 @@ impl DecimalBytePartsData { } } +/// The in-memory decimal byte-parts encoding. +/// +/// Register [`super::DecimalBytePartsPlugin`] or call [`crate::initialize`] to read and write +/// either serialized format. Registering this VTable directly, or calling its serde methods, +/// returns an error when serializing or deserializing, including for the frozen v1 format. #[derive(Clone, Debug)] pub struct DecimalByteParts; @@ -334,33 +262,22 @@ impl VTable for DecimalByteParts { } fn serialize( - array: ArrayView<'_, Self>, + _array: ArrayView<'_, Self>, _session: &VortexSession, ) -> VortexResult>> { - vortex_ensure!( - array.lower_parts().is_empty(), - "serializing DecimalByteParts with lower parts requires DecimalBytePartsPlugin" - ); - Ok(Some( - DecimalBytesPartsMetadata::from_array(array)?.encode_to_vec(), - )) + vortex_bail!("DecimalByteParts serialization requires DecimalBytePartsPlugin") } fn deserialize( &self, - dtype: &DType, - len: usize, - metadata: &[u8], + _dtype: &DType, + _len: usize, + _metadata: &[u8], _buffers: &[BufferHandle], - children: &dyn ArrayChildren, + _children: &dyn ArrayChildren, _session: &VortexSession, ) -> VortexResult> { - let metadata = DecimalBytesPartsMetadata::decode(metadata)?; - vortex_ensure!( - metadata.lower_part_count()? == 0, - "vortex.decimal_byte_parts must not carry lower parts" - ); - metadata.into_array_parts(dtype, len, children) + vortex_bail!("DecimalByteParts deserialization requires DecimalBytePartsPlugin") } fn slot_name(_array: ArrayView<'_, Self>, idx: usize) -> String { 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 fe661fa9ef4..0dd2243fa29 100644 --- a/encodings/decimal-byte-parts/src/decimal_byte_parts/mod.rs +++ b/encodings/decimal-byte-parts/src/decimal_byte_parts/mod.rs @@ -32,6 +32,7 @@ mod testing; pub use array::*; pub use plugin::DecimalBytePartsPlugin; +pub use plugin::DecimalBytesPartsMetadata; pub use plugin::decimal_byte_parts_v2_id; pub use split::DecimalParts; pub use split::dbp_encode; diff --git a/encodings/decimal-byte-parts/src/decimal_byte_parts/plugin.rs b/encodings/decimal-byte-parts/src/decimal_byte_parts/plugin.rs index e7fb773e049..587d40c4448 100644 --- a/encodings/decimal-byte-parts/src/decimal_byte_parts/plugin.rs +++ b/encodings/decimal-byte-parts/src/decimal_byte_parts/plugin.rs @@ -7,10 +7,17 @@ use prost::Message as _; use vortex_array::Array; use vortex_array::ArrayDeserialization; use vortex_array::ArrayId; +use vortex_array::ArrayParts; use vortex_array::ArrayPlugin; use vortex_array::ArrayRef; use vortex_array::ArraySerialization; +use vortex_array::ArraySlots; +use vortex_array::ArrayView; use vortex_array::IntoArray; +use vortex_array::dtype::DType; +use vortex_array::dtype::Nullability; +use vortex_array::dtype::PType; +use vortex_array::serde::ArrayChildren; use vortex_array::vtable::VTable; use vortex_error::VortexResult; use vortex_error::vortex_ensure; @@ -20,7 +27,9 @@ use vortex_session::registry::CachedId; use super::DecimalByteParts; use super::DecimalBytePartsArraySlotsExt; -use super::DecimalBytesPartsMetadata; +use super::DecimalBytePartsData; +use super::DecimalBytePartsSlots; +use super::MAX_LOWER_PARTS; /// The `vortex.decimal_byte_parts_v2` serialized format ID, for `DecimalBytePartsArray`s carrying /// lower parts. @@ -39,8 +48,10 @@ pub fn decimal_byte_parts_v2_id() -> ArrayId { /// serializes under [`decimal_byte_parts_v2_id`]. Reading holds each ID to its own contract: /// the frozen ID carries no lower parts and the v2 ID carries at least one. /// -/// Register this plugin, or call [`crate::initialize`], to enable both formats. Registering -/// [`DecimalByteParts`] directly only supports the frozen format. +/// Register this plugin, or call [`crate::initialize`], to enable both formats. Direct registration +/// of [`DecimalByteParts`] no longer supports serde, including for the frozen format. Replace +/// `session.arrays().register(DecimalByteParts)` with +/// `session.arrays().register(DecimalBytePartsPlugin)`; existing v1 files remain readable. #[derive(Clone, Debug)] pub struct DecimalBytePartsPlugin; @@ -110,6 +121,98 @@ impl ArrayPlugin for DecimalBytePartsPlugin { } } +/// Metadata for the frozen and v2 decimal byte-parts formats. +#[derive(Clone, prost::Message)] +pub struct DecimalBytesPartsMetadata { + #[prost(enumeration = "PType", tag = "1")] + zeroth_child_ptype: i32, + #[prost(uint32, tag = "2")] + lower_part_count: u32, + /// Unsigned storage types of the lower parts, most significant first. + #[prost(enumeration = "PType", repeated, tag = "3")] + lower_part_ptypes: Vec, +} + +impl DecimalBytesPartsMetadata { + fn from_array(array: ArrayView<'_, DecimalByteParts>) -> VortexResult { + Ok(Self { + zeroth_child_ptype: PType::try_from(array.msp().dtype())? as i32, + lower_part_count: u32::try_from(array.lower_parts().len()) + .map_err(|_| vortex_err!("lower part count exceeds u32"))?, + lower_part_ptypes: array + .lower_parts() + .iter() + .map(|part| PType::try_from(part.dtype()).map(|ptype| ptype as i32)) + .collect::>()?, + }) + } + + fn into_array_parts( + self, + dtype: &DType, + len: usize, + children: &dyn ArrayChildren, + ) -> VortexResult> { + vortex_ensure!( + dtype.as_decimal_opt().is_some(), + "decoding decimal but given non decimal dtype {dtype}" + ); + + let encoded_dtype = DType::Primitive(self.zeroth_child_ptype(), dtype.nullability()); + + let lower_part_count = self.lower_part_count()?; + vortex_ensure!( + self.lower_part_ptypes.len() == lower_part_count, + "expected {lower_part_count} lower-part dtypes, got {}", + self.lower_part_ptypes.len() + ); + vortex_ensure!( + children.len() == DecimalBytePartsSlots::FIXED_COUNT + lower_part_count, + "expected {} children, got {}", + DecimalBytePartsSlots::FIXED_COUNT + lower_part_count, + children.len() + ); + + let msp = children.get(DecimalBytePartsSlots::MSP, &encoded_dtype, len)?; + + let mut slots = ArraySlots::with_capacity(children.len()); + slots.push(Some(msp)); + for (idx, raw_ptype) in self.lower_part_ptypes.into_iter().enumerate() { + let ptype = PType::try_from(raw_ptype) + .map_err(|_| vortex_err!("invalid PType {raw_ptype} for lower part {idx}"))?; + vortex_ensure!( + ptype.is_unsigned_int(), + "lower part {idx} must have an unsigned integer dtype, got {ptype}" + ); + slots.push(Some(children.get( + DecimalBytePartsSlots::LOWER_PARTS_OFFSET + idx, + &DType::Primitive(ptype, Nullability::NonNullable), + len, + )?)); + } + + Ok( + ArrayParts::new(DecimalByteParts, dtype.clone(), len, DecimalBytePartsData) + .with_slots(slots), + ) + } + + /// The number of lower parts encoded in this array. + /// + /// # Errors + /// + /// Returns an error if the count exceeds [`MAX_LOWER_PARTS`]. + 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!( + count <= MAX_LOWER_PARTS, + "at most {MAX_LOWER_PARTS} lower parts are supported, got {count}" + ); + Ok(count) + } +} + #[cfg(test)] mod tests { use rstest::rstest; @@ -117,8 +220,10 @@ mod tests { use vortex_array::ArrayVTable; use vortex_array::VortexSessionExecute; use vortex_array::array_session; + use vortex_array::arrays::ConstantArray; use vortex_array::arrays::DecimalArray; use vortex_array::arrays::Primitive; + use vortex_array::arrays::PrimitiveArray; use vortex_array::assert_arrays_eq; use vortex_array::dtype::DType; use vortex_array::dtype::DecimalDType; @@ -172,12 +277,42 @@ mod tests { vec![buffer![0u64; 3].into_array(), buffer![0u64; 3].into_array(), lower_part()], DecimalDType::new(38, 2), ))] + #[case::narrowed_one_lower_part(DecimalByteParts::try_new_with_lower_parts( + buffer![-1i8, 0, 1].into_array(), + vec![buffer![0u8, 128, u8::MAX].into_array()], + DecimalDType::new(38, 2), + ))] + #[case::narrowed_three_lower_parts(DecimalByteParts::try_new_with_lower_parts( + buffer![-1i16, 0, 1].into_array(), + vec![ + buffer![u8::MAX, 128, 0].into_array(), + ConstantArray::new(u16::MAX, 3).into_array(), + buffer![0u32, 1 << 31, u32::MAX].into_array(), + ], + DecimalDType::new(76, 2), + ))] + #[case::nullable_mixed_lower_parts(DecimalByteParts::try_new_with_lower_parts( + PrimitiveArray::new( + buffer![-1i8, 0, 1], Validity::from_iter([true, false, true]), + ).into_array(), + vec![ + buffer![u64::MAX, 1 << 63, 0].into_array(), + buffer![0u8, 128, u8::MAX].into_array(), + buffer![u32::MAX, 1 << 31, 0].into_array(), + ], + DecimalDType::new(76, 2), + ))] fn test_serde_round_trip( #[case] array: VortexResult, ) -> VortexResult<()> { let session = session(); let array = array?; let lower_part_count = array.lower_parts().len(); + let lower_part_dtypes: Vec<_> = array + .lower_parts() + .iter() + .map(|part| part.dtype().clone()) + .collect(); let array = array.into_array(); let dtype = array.dtype().clone(); let len = array.len(); @@ -209,9 +344,11 @@ mod tests { .as_opt::() .vortex_expect("byte parts array") .lower_parts() - .len(), - lower_part_count, - "lower parts must survive serde" + .iter() + .map(|part| part.dtype().clone()) + .collect::>(), + lower_part_dtypes, + "lower-part dtypes and order must survive serde" ); let mut ctx = session.create_execution_ctx(); @@ -237,6 +374,49 @@ mod tests { assert!(plugin_deserialize_with(serialized_id, lower_part_count, children).is_err()); } + #[rstest] + #[case::missing_type(1, vec![], "expected 1 lower-part dtypes, got 0")] + #[case::extra_type( + 1, vec![PType::U64 as i32, PType::U8 as i32], + "expected 1 lower-part dtypes, got 2", + )] + #[case::frozen_with_lower_type( + 0, vec![PType::U8 as i32], "expected 0 lower-part dtypes, got 1", + )] + #[case::signed_type(1, vec![PType::I64 as i32], "unsigned integer dtype")] + #[case::float_type(1, vec![PType::F64 as i32], "unsigned integer dtype")] + #[case::unknown_type(1, vec![i32::MAX], "invalid PType")] + fn test_deserialize_rejects_invalid_lower_part_ptypes( + #[case] lower_part_count: u32, + #[case] lower_part_ptypes: Vec, + #[case] expected_error: &str, + ) { + let metadata = DecimalBytesPartsMetadata { + zeroth_child_ptype: PType::I64 as i32, + lower_part_count, + lower_part_ptypes, + } + .encode_to_vec(); + let mut children = vec![msp()]; + children.extend((0..lower_part_count).map(|_| lower_part())); + let serialized_id = if lower_part_count == 0 { + VTable::id(&DecimalByteParts) + } else { + decimal_byte_parts_v2_id() + }; + let dtype = DType::Decimal(DecimalDType::new(38, 2), Nullability::NonNullable); + let result = DecimalBytePartsPlugin.deserialize( + ArrayDeserialization::new(serialized_id, &dtype, 3, &metadata, &[], &children), + &array_session(), + ); + assert!( + result + .as_ref() + .is_err_and(|err| err.to_string().contains(expected_error)), + "expected {expected_error}, got {result:?}" + ); + } + fn plugin_deserialize_with( serialized_id: ArrayId, lower_part_count: u32, @@ -245,6 +425,7 @@ mod tests { let metadata = DecimalBytesPartsMetadata { zeroth_child_ptype: PType::I64 as i32, lower_part_count, + lower_part_ptypes: (0..lower_part_count).map(|_| PType::U64 as i32).collect(), } .encode_to_vec(); let dtype = DType::Decimal(DecimalDType::new(38, 2), Nullability::NonNullable); @@ -290,6 +471,19 @@ mod tests { session } + #[test] + fn frozen_metadata_is_unchanged() -> VortexResult<()> { + let session = session(); + let array = DecimalByteParts::try_new(msp(), DecimalDType::new(19, 2))?.into_array(); + let serialized = session + .array_serialize(&array)? + .vortex_expect("byte parts arrays are serializable"); + assert_eq!(serialized.serialized_id, VTable::id(&DecimalByteParts)); + // Frozen metadata for an i64 MSP: field 1 = 7, with no lower-part fields emitted. + assert_eq!(serialized.metadata, [8, 7]); + Ok(()) + } + #[test] fn serialization_requires_v2_permission() -> VortexResult<()> { let session = session(); @@ -336,16 +530,36 @@ mod tests { } #[test] - fn bare_vtable_refuses_wide_serialization() -> VortexResult<()> { + fn bare_vtable_refuses_serde() -> VortexResult<()> { let session = array_session(); session.arrays().register(DecimalByteParts); - let array = DecimalByteParts::try_new_with_lower_parts( - msp(), - vec![lower_part()], - DecimalDType::new(38, 2), - )? - .into_array(); - assert!(session.array_serialize(&array).is_err()); + let msp = msp(); + let array = DecimalByteParts::try_new(msp.clone(), DecimalDType::new(19, 2))?.into_array(); + let result = session.array_serialize(&array); + assert!( + result.as_ref().is_err_and(|err| err + .to_string() + .contains("DecimalByteParts serialization requires DecimalBytePartsPlugin")), + "expected unsupported VTable serialization, got {result:?}" + ); + + let id = VTable::id(&DecimalByteParts); + let plugin = session + .arrays() + .registry() + .get(&id) + .vortex_expect("registered"); + let children = vec![msp]; + let result = plugin.deserialize( + ArrayDeserialization::new(id, array.dtype(), array.len(), &[8, 7], &[], &children), + &session, + ); + assert!( + result.as_ref().is_err_and(|err| err + .to_string() + .contains("DecimalByteParts deserialization requires DecimalBytePartsPlugin")), + "expected unsupported VTable deserialization, got {result:?}" + ); Ok(()) } } From d214133bfe2867d6c65133499dd99b557a44eefa Mon Sep 17 00:00:00 2001 From: Matt Katz Date: Tue, 15 Sep 2026 14:14:49 -0400 Subject: [PATCH 06/17] Prototype DBP serde with separate version implementations Keep one plugin dispatcher with separate v1 and v2 metadata and serde functions operating directly on the current DBP array. Preserve the current draft, including the latest metadata names and validation edits, for comparison against an ArrayRepresentation prototype. Snapshot validation: 272 tests passed and 3 failed before fail-fast cancelled the remaining 22 tests. The failures cover accepting zero lower parts in v2 and two expectations of the previous error wording. Signed-off-by: Matt Katz --- .../src/decimal_byte_parts/array.rs | 7 +- .../src/decimal_byte_parts/mod.rs | 3 +- .../src/decimal_byte_parts/plugin.rs | 565 ------------------ .../src/decimal_byte_parts/plugin/mod.rs | 97 +++ .../src/decimal_byte_parts/plugin/tests.rs | 420 +++++++++++++ .../src/decimal_byte_parts/plugin/v1.rs | 84 +++ .../src/decimal_byte_parts/plugin/v2.rs | 130 ++++ vortex-btrblocks/src/trace_tests.rs | 10 +- .../golden__compact__decimal_prices.snap | 2 +- .../golden__default__decimal_prices.snap | 2 +- .../golden__unstable__decimal_prices.snap | 2 +- 11 files changed, 744 insertions(+), 578 deletions(-) delete mode 100644 encodings/decimal-byte-parts/src/decimal_byte_parts/plugin.rs create mode 100644 encodings/decimal-byte-parts/src/decimal_byte_parts/plugin/mod.rs create mode 100644 encodings/decimal-byte-parts/src/decimal_byte_parts/plugin/tests.rs create mode 100644 encodings/decimal-byte-parts/src/decimal_byte_parts/plugin/v1.rs create mode 100644 encodings/decimal-byte-parts/src/decimal_byte_parts/plugin/v2.rs diff --git a/encodings/decimal-byte-parts/src/decimal_byte_parts/array.rs b/encodings/decimal-byte-parts/src/decimal_byte_parts/array.rs index 59df7486a0e..65ea454937d 100644 --- a/encodings/decimal-byte-parts/src/decimal_byte_parts/array.rs +++ b/encodings/decimal-byte-parts/src/decimal_byte_parts/array.rs @@ -36,11 +36,11 @@ use vortex_error::vortex_bail; use vortex_error::vortex_ensure; use vortex_error::vortex_panic; use vortex_session::VortexSession; -use vortex_session::registry::CachedId; use super::MAX_LOWER_PARTS; use super::assemble::assemble_decimal; use super::assemble::assemble_wide_decimal_value; +use super::decimal_byte_parts_v2_id; use super::rules::PARENT_RULES; /// A [`DecimalByteParts`]-encoded Vortex array. @@ -124,7 +124,7 @@ impl DecimalBytePartsData { } } -/// The in-memory decimal byte-parts encoding. +/// The current in-memory decimal byte-parts encoding, identified as v2. /// /// Register [`super::DecimalBytePartsPlugin`] or call [`crate::initialize`] to read and write /// either serialized format. Registering this VTable directly, or calling its serde methods, @@ -205,8 +205,7 @@ impl VTable for DecimalByteParts { type ValidityVTable = ValidityVTableFromChild; fn id(&self) -> ArrayId { - static ID: CachedId = CachedId::new("vortex.decimal_byte_parts"); - *ID + decimal_byte_parts_v2_id() } fn validate( 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 0dd2243fa29..b989fe2bbe0 100644 --- a/encodings/decimal-byte-parts/src/decimal_byte_parts/mod.rs +++ b/encodings/decimal-byte-parts/src/decimal_byte_parts/mod.rs @@ -32,7 +32,8 @@ mod testing; pub use array::*; pub use plugin::DecimalBytePartsPlugin; -pub use plugin::DecimalBytesPartsMetadata; +pub use plugin::DecimalBytePartsV2Metadata; +pub use plugin::decimal_byte_parts_v1_id; pub use plugin::decimal_byte_parts_v2_id; pub use split::DecimalParts; pub use split::dbp_encode; diff --git a/encodings/decimal-byte-parts/src/decimal_byte_parts/plugin.rs b/encodings/decimal-byte-parts/src/decimal_byte_parts/plugin.rs deleted file mode 100644 index 587d40c4448..00000000000 --- a/encodings/decimal-byte-parts/src/decimal_byte_parts/plugin.rs +++ /dev/null @@ -1,565 +0,0 @@ -// SPDX-License-Identifier: Apache-2.0 -// SPDX-FileCopyrightText: Copyright the Vortex contributors - -//! Serialization of decimal byte parts under the frozen and v2 format IDs. - -use prost::Message as _; -use vortex_array::Array; -use vortex_array::ArrayDeserialization; -use vortex_array::ArrayId; -use vortex_array::ArrayParts; -use vortex_array::ArrayPlugin; -use vortex_array::ArrayRef; -use vortex_array::ArraySerialization; -use vortex_array::ArraySlots; -use vortex_array::ArrayView; -use vortex_array::IntoArray; -use vortex_array::dtype::DType; -use vortex_array::dtype::Nullability; -use vortex_array::dtype::PType; -use vortex_array::serde::ArrayChildren; -use vortex_array::vtable::VTable; -use vortex_error::VortexResult; -use vortex_error::vortex_ensure; -use vortex_error::vortex_err; -use vortex_session::VortexSession; -use vortex_session::registry::CachedId; - -use super::DecimalByteParts; -use super::DecimalBytePartsArraySlotsExt; -use super::DecimalBytePartsData; -use super::DecimalBytePartsSlots; -use super::MAX_LOWER_PARTS; - -/// The `vortex.decimal_byte_parts_v2` serialized format ID, for `DecimalBytePartsArray`s carrying -/// lower parts. -/// -/// The `vortex.decimal_byte_parts` Id corresponds to the previous version of the `DecimalBytePartsArray`, -/// which does not support lower parts. Both IDs deserialize back into the same `DecimalBytePartsArray`. -pub fn decimal_byte_parts_v2_id() -> ArrayId { - static ID: CachedId = CachedId::new("vortex.decimal_byte_parts_v2"); - *ID -} - -/// The [`ArrayPlugin`] for [`DecimalByteParts`], owning both of its serialized formats. -/// -/// An array without lower parts serializes under the frozen `vortex.decimal_byte_parts` ID, -/// byte-identical to files written before lower parts existed. An array carrying lower parts -/// serializes under [`decimal_byte_parts_v2_id`]. Reading holds each ID to its own contract: -/// the frozen ID carries no lower parts and the v2 ID carries at least one. -/// -/// Register this plugin, or call [`crate::initialize`], to enable both formats. Direct registration -/// of [`DecimalByteParts`] no longer supports serde, including for the frozen format. Replace -/// `session.arrays().register(DecimalByteParts)` with -/// `session.arrays().register(DecimalBytePartsPlugin)`; existing v1 files remain readable. -#[derive(Clone, Debug)] -pub struct DecimalBytePartsPlugin; - -impl ArrayPlugin for DecimalBytePartsPlugin { - fn id(&self) -> ArrayId { - VTable::id(&DecimalByteParts) - } - - fn serialized_ids(&self) -> Vec { - vec![VTable::id(&DecimalByteParts), decimal_byte_parts_v2_id()] - } - - fn serialize( - &self, - array: &ArrayRef, - _session: &VortexSession, - ) -> VortexResult> { - let view = array.as_opt::().ok_or_else(|| { - vortex_err!( - "DecimalByteParts plugin cannot serialize {}", - array.encoding_id() - ) - })?; - let serialized_id = if view.lower_parts().is_empty() { - VTable::id(&DecimalByteParts) - } else { - decimal_byte_parts_v2_id() - }; - Ok(Some(ArraySerialization::from_array( - serialized_id, - array, - DecimalBytesPartsMetadata::from_array(view)?.encode_to_vec(), - ))) - } - - fn deserialize( - &self, - parts: ArrayDeserialization<'_>, - _session: &VortexSession, - ) -> VortexResult { - let metadata = DecimalBytesPartsMetadata::decode(parts.metadata)?; - let lower_part_count = metadata.lower_part_count()?; - if parts.serialized_id == decimal_byte_parts_v2_id() { - vortex_ensure!( - lower_part_count > 0, - "{} must carry at least one lower part", - parts.serialized_id - ); - } else { - vortex_ensure!( - parts.serialized_id == VTable::id(&DecimalByteParts), - "DecimalByteParts plugin does not recognize serialized ID {}", - parts.serialized_id - ); - vortex_ensure!( - lower_part_count == 0, - "{} must not carry lower parts, got {lower_part_count}", - parts.serialized_id - ); - } - Ok(Array::try_from_parts(metadata.into_array_parts( - parts.dtype, - parts.len, - parts.children, - )?)? - .into_array()) - } -} - -/// Metadata for the frozen and v2 decimal byte-parts formats. -#[derive(Clone, prost::Message)] -pub struct DecimalBytesPartsMetadata { - #[prost(enumeration = "PType", tag = "1")] - zeroth_child_ptype: i32, - #[prost(uint32, tag = "2")] - lower_part_count: u32, - /// Unsigned storage types of the lower parts, most significant first. - #[prost(enumeration = "PType", repeated, tag = "3")] - lower_part_ptypes: Vec, -} - -impl DecimalBytesPartsMetadata { - fn from_array(array: ArrayView<'_, DecimalByteParts>) -> VortexResult { - Ok(Self { - zeroth_child_ptype: PType::try_from(array.msp().dtype())? as i32, - lower_part_count: u32::try_from(array.lower_parts().len()) - .map_err(|_| vortex_err!("lower part count exceeds u32"))?, - lower_part_ptypes: array - .lower_parts() - .iter() - .map(|part| PType::try_from(part.dtype()).map(|ptype| ptype as i32)) - .collect::>()?, - }) - } - - fn into_array_parts( - self, - dtype: &DType, - len: usize, - children: &dyn ArrayChildren, - ) -> VortexResult> { - vortex_ensure!( - dtype.as_decimal_opt().is_some(), - "decoding decimal but given non decimal dtype {dtype}" - ); - - let encoded_dtype = DType::Primitive(self.zeroth_child_ptype(), dtype.nullability()); - - let lower_part_count = self.lower_part_count()?; - vortex_ensure!( - self.lower_part_ptypes.len() == lower_part_count, - "expected {lower_part_count} lower-part dtypes, got {}", - self.lower_part_ptypes.len() - ); - vortex_ensure!( - children.len() == DecimalBytePartsSlots::FIXED_COUNT + lower_part_count, - "expected {} children, got {}", - DecimalBytePartsSlots::FIXED_COUNT + lower_part_count, - children.len() - ); - - let msp = children.get(DecimalBytePartsSlots::MSP, &encoded_dtype, len)?; - - let mut slots = ArraySlots::with_capacity(children.len()); - slots.push(Some(msp)); - for (idx, raw_ptype) in self.lower_part_ptypes.into_iter().enumerate() { - let ptype = PType::try_from(raw_ptype) - .map_err(|_| vortex_err!("invalid PType {raw_ptype} for lower part {idx}"))?; - vortex_ensure!( - ptype.is_unsigned_int(), - "lower part {idx} must have an unsigned integer dtype, got {ptype}" - ); - slots.push(Some(children.get( - DecimalBytePartsSlots::LOWER_PARTS_OFFSET + idx, - &DType::Primitive(ptype, Nullability::NonNullable), - len, - )?)); - } - - Ok( - ArrayParts::new(DecimalByteParts, dtype.clone(), len, DecimalBytePartsData) - .with_slots(slots), - ) - } - - /// The number of lower parts encoded in this array. - /// - /// # Errors - /// - /// Returns an error if the count exceeds [`MAX_LOWER_PARTS`]. - 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!( - count <= MAX_LOWER_PARTS, - "at most {MAX_LOWER_PARTS} lower parts are supported, got {count}" - ); - Ok(count) - } -} - -#[cfg(test)] -mod tests { - use rstest::rstest; - use vortex_array::ArrayContext; - use vortex_array::ArrayVTable; - use vortex_array::VortexSessionExecute; - use vortex_array::array_session; - use vortex_array::arrays::ConstantArray; - use vortex_array::arrays::DecimalArray; - use vortex_array::arrays::Primitive; - 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::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::VortexExpect; - use vortex_session::registry::ReadContext; - - use super::*; - use crate::DecimalBytePartsArray; - use crate::decimal_byte_parts::testing::encode; - use crate::decimal_byte_parts::testing::i128_parts; - use crate::decimal_byte_parts::testing::i256_parts; - use crate::decimal_byte_parts::testing::wide_i128_values; - use crate::decimal_byte_parts::testing::wide_i256_values; - - #[rstest] - #[case::no_lower_parts(DecimalByteParts::try_new( - buffer![1i32, 2, 3].into_array(), DecimalDType::new(9, 2), - ))] - #[case::one_lower_part(Ok(i128_parts(wide_i128_values(), Validity::NonNullable)))] - #[case::three_lower_parts(Ok(i256_parts(wide_i256_values(), Validity::NonNullable)))] - #[case::nullable_three_lower_parts(Ok(i256_parts( - wide_i256_values(), - Validity::from_iter([true, false, true, true, true, false, true, true, true, true]), - )))] - #[case::wider_i64_storage(encode(&DecimalArray::new( - buffer![-99i64, 0, 99], DecimalDType::new(2, 0), Validity::NonNullable, - )))] - #[case::wider_i128_storage(encode(&DecimalArray::new( - buffer![-99i128, 0, 99], DecimalDType::new(2, 0), Validity::NonNullable, - )))] - #[case::wider_i256_storage(encode(&DecimalArray::new( - buffer![i256::from_i128(-99), i256::ZERO, i256::from_i128(99)], - DecimalDType::new(2, 0), Validity::NonNullable, - )))] - #[case::redundant_two_lower_parts(DecimalByteParts::try_new_with_lower_parts( - buffer![0i64; 3].into_array(), - vec![buffer![0u64; 3].into_array(), lower_part()], - DecimalDType::new(38, 2), - ))] - #[case::redundant_three_lower_parts(DecimalByteParts::try_new_with_lower_parts( - buffer![0i64; 3].into_array(), - vec![buffer![0u64; 3].into_array(), buffer![0u64; 3].into_array(), lower_part()], - DecimalDType::new(38, 2), - ))] - #[case::narrowed_one_lower_part(DecimalByteParts::try_new_with_lower_parts( - buffer![-1i8, 0, 1].into_array(), - vec![buffer![0u8, 128, u8::MAX].into_array()], - DecimalDType::new(38, 2), - ))] - #[case::narrowed_three_lower_parts(DecimalByteParts::try_new_with_lower_parts( - buffer![-1i16, 0, 1].into_array(), - vec![ - buffer![u8::MAX, 128, 0].into_array(), - ConstantArray::new(u16::MAX, 3).into_array(), - buffer![0u32, 1 << 31, u32::MAX].into_array(), - ], - DecimalDType::new(76, 2), - ))] - #[case::nullable_mixed_lower_parts(DecimalByteParts::try_new_with_lower_parts( - PrimitiveArray::new( - buffer![-1i8, 0, 1], Validity::from_iter([true, false, true]), - ).into_array(), - vec![ - buffer![u64::MAX, 1 << 63, 0].into_array(), - buffer![0u8, 128, u8::MAX].into_array(), - buffer![u32::MAX, 1 << 31, 0].into_array(), - ], - DecimalDType::new(76, 2), - ))] - fn test_serde_round_trip( - #[case] array: VortexResult, - ) -> VortexResult<()> { - let session = session(); - let array = array?; - let lower_part_count = array.lower_parts().len(); - let lower_part_dtypes: Vec<_> = array - .lower_parts() - .iter() - .map(|part| part.dtype().clone()) - .collect(); - let array = array.into_array(); - let dtype = array.dtype().clone(); - let len = array.len(); - - let expected_id = if lower_part_count == 0 { - VTable::id(&DecimalByteParts) - } else { - decimal_byte_parts_v2_id() - }; - assert_eq!( - session - .array_serialize(&array)? - .vortex_expect("byte parts arrays are serializable") - .serialized_id, - expected_id - ); - - 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() - .iter() - .map(|part| part.dtype().clone()) - .collect::>(), - lower_part_dtypes, - "lower-part dtypes and order must survive serde" - ); - - let mut ctx = session.create_execution_ctx(); - assert_arrays_eq!(array, decoded, &mut ctx); - Ok(()) - } - - #[rstest] - #[case::missing_lower_part(1, vec![msp()])] - #[case::extra_lower_part(0, vec![msp(), lower_part()])] - #[case::too_many_lower_parts( - 4, vec![msp(), lower_part(), lower_part(), lower_part(), lower_part()], - )] - fn test_deserialize_rejects_child_count_mismatch( - #[case] lower_part_count: u32, - #[case] children: Vec, - ) { - let serialized_id = if lower_part_count == 0 { - VTable::id(&DecimalByteParts) - } else { - decimal_byte_parts_v2_id() - }; - assert!(plugin_deserialize_with(serialized_id, lower_part_count, children).is_err()); - } - - #[rstest] - #[case::missing_type(1, vec![], "expected 1 lower-part dtypes, got 0")] - #[case::extra_type( - 1, vec![PType::U64 as i32, PType::U8 as i32], - "expected 1 lower-part dtypes, got 2", - )] - #[case::frozen_with_lower_type( - 0, vec![PType::U8 as i32], "expected 0 lower-part dtypes, got 1", - )] - #[case::signed_type(1, vec![PType::I64 as i32], "unsigned integer dtype")] - #[case::float_type(1, vec![PType::F64 as i32], "unsigned integer dtype")] - #[case::unknown_type(1, vec![i32::MAX], "invalid PType")] - fn test_deserialize_rejects_invalid_lower_part_ptypes( - #[case] lower_part_count: u32, - #[case] lower_part_ptypes: Vec, - #[case] expected_error: &str, - ) { - let metadata = DecimalBytesPartsMetadata { - zeroth_child_ptype: PType::I64 as i32, - lower_part_count, - lower_part_ptypes, - } - .encode_to_vec(); - let mut children = vec![msp()]; - children.extend((0..lower_part_count).map(|_| lower_part())); - let serialized_id = if lower_part_count == 0 { - VTable::id(&DecimalByteParts) - } else { - decimal_byte_parts_v2_id() - }; - let dtype = DType::Decimal(DecimalDType::new(38, 2), Nullability::NonNullable); - let result = DecimalBytePartsPlugin.deserialize( - ArrayDeserialization::new(serialized_id, &dtype, 3, &metadata, &[], &children), - &array_session(), - ); - assert!( - result - .as_ref() - .is_err_and(|err| err.to_string().contains(expected_error)), - "expected {expected_error}, got {result:?}" - ); - } - - fn plugin_deserialize_with( - serialized_id: ArrayId, - lower_part_count: u32, - children: Vec, - ) -> VortexResult { - let metadata = DecimalBytesPartsMetadata { - zeroth_child_ptype: PType::I64 as i32, - lower_part_count, - lower_part_ptypes: (0..lower_part_count).map(|_| PType::U64 as i32).collect(), - } - .encode_to_vec(); - let dtype = DType::Decimal(DecimalDType::new(38, 2), Nullability::NonNullable); - DecimalBytePartsPlugin.deserialize( - ArrayDeserialization::new(serialized_id, &dtype, 3, &metadata, &[], &children), - &array_session(), - ) - } - - /// Each serialized ID keeps its own contract: the frozen ID never carries lower parts, and - /// the v2 ID is never written without them. - #[rstest] - #[case::frozen_without_lower_parts(VTable::id(&DecimalByteParts), 0, vec![msp()], true)] - #[case::frozen_with_lower_parts( - VTable::id(&DecimalByteParts), - 1, - vec![msp(), lower_part()], - false - )] - #[case::v2_with_lower_parts(decimal_byte_parts_v2_id(), 1, vec![msp(), lower_part()], true)] - #[case::v2_without_lower_parts(decimal_byte_parts_v2_id(), 0, vec![msp()], false)] - fn plugin_holds_each_id_to_its_contract( - #[case] serialized_id: ArrayId, - #[case] lower_part_count: u32, - #[case] children: Vec, - #[case] accepted: bool, - ) { - let result = plugin_deserialize_with(serialized_id, lower_part_count, children); - assert_eq!(result.is_ok(), accepted, "{serialized_id}: {result:?}"); - } - - fn msp() -> ArrayRef { - buffer![1i64, 2, 3].into_array() - } - - fn lower_part() -> ArrayRef { - buffer![1u64, 2, 3].into_array() - } - - fn session() -> VortexSession { - let session = array_session(); - crate::initialize(&session); - session - } - - #[test] - fn frozen_metadata_is_unchanged() -> VortexResult<()> { - let session = session(); - let array = DecimalByteParts::try_new(msp(), DecimalDType::new(19, 2))?.into_array(); - let serialized = session - .array_serialize(&array)? - .vortex_expect("byte parts arrays are serializable"); - assert_eq!(serialized.serialized_id, VTable::id(&DecimalByteParts)); - // Frozen metadata for an i64 MSP: field 1 = 7, with no lower-part fields emitted. - assert_eq!(serialized.metadata, [8, 7]); - Ok(()) - } - - #[test] - fn serialization_requires_v2_permission() -> VortexResult<()> { - let session = session(); - let array = DecimalByteParts::try_new_with_lower_parts( - msp(), - vec![lower_part()], - DecimalDType::new(38, 2), - )? - .into_array(); - - let restricted = ArrayContext::empty().with_allowed_ids( - [ - ArrayVTable::id(&DecimalByteParts), - ArrayVTable::id(&Primitive), - ] - .into_iter() - .collect(), - ); - let err = array - .serialize(&restricted, &session, &SerializeOptions::default()) - .expect_err("expected the permitted-encoding check to refuse the v2 format"); - assert!( - err.to_string().contains("not permitted"), - "error should name the permitted-encoding check, got: {err}" - ); - - // Permitting the v2 format id is exactly what allows the same array through. - let permissive = ArrayContext::empty().with_allowed_ids( - [ - ArrayVTable::id(&DecimalByteParts), - decimal_byte_parts_v2_id(), - ArrayVTable::id(&Primitive), - ] - .into_iter() - .collect(), - ); - array.serialize(&permissive, &session, &SerializeOptions::default())?; - assert!( - permissive.to_ids().contains(&decimal_byte_parts_v2_id()), - "the file's encoding table must carry the v2 format id" - ); - - Ok(()) - } - - #[test] - fn bare_vtable_refuses_serde() -> VortexResult<()> { - let session = array_session(); - session.arrays().register(DecimalByteParts); - let msp = msp(); - let array = DecimalByteParts::try_new(msp.clone(), DecimalDType::new(19, 2))?.into_array(); - let result = session.array_serialize(&array); - assert!( - result.as_ref().is_err_and(|err| err - .to_string() - .contains("DecimalByteParts serialization requires DecimalBytePartsPlugin")), - "expected unsupported VTable serialization, got {result:?}" - ); - - let id = VTable::id(&DecimalByteParts); - let plugin = session - .arrays() - .registry() - .get(&id) - .vortex_expect("registered"); - let children = vec![msp]; - let result = plugin.deserialize( - ArrayDeserialization::new(id, array.dtype(), array.len(), &[8, 7], &[], &children), - &session, - ); - assert!( - result.as_ref().is_err_and(|err| err - .to_string() - .contains("DecimalByteParts deserialization requires DecimalBytePartsPlugin")), - "expected unsupported VTable deserialization, got {result:?}" - ); - Ok(()) - } -} diff --git a/encodings/decimal-byte-parts/src/decimal_byte_parts/plugin/mod.rs b/encodings/decimal-byte-parts/src/decimal_byte_parts/plugin/mod.rs new file mode 100644 index 00000000000..8456346785e --- /dev/null +++ b/encodings/decimal-byte-parts/src/decimal_byte_parts/plugin/mod.rs @@ -0,0 +1,97 @@ +// SPDX-License-Identifier: Apache-2.0 +// SPDX-FileCopyrightText: Copyright the Vortex contributors + +//! Select a DBP wire format and dispatch to its serde implementation. + +use vortex_array::ArrayDeserialization; +use vortex_array::ArrayId; +use vortex_array::ArrayPlugin; +use vortex_array::ArrayRef; +use vortex_array::ArraySerialization; +use vortex_array::IntoArray; +use vortex_array::VTable; +use vortex_error::VortexResult; +use vortex_error::vortex_bail; +use vortex_error::vortex_err; +use vortex_session::VortexSession; +use vortex_session::registry::CachedId; + +use super::DecimalByteParts; +use super::DecimalBytePartsArraySlotsExt; + +#[cfg(test)] +mod tests; +mod v1; +mod v2; + +pub use v2::DecimalBytePartsV2Metadata; + +/// The frozen single-child decimal byte-parts format ID. +pub fn decimal_byte_parts_v1_id() -> ArrayId { + static ID: CachedId = CachedId::new("vortex.decimal_byte_parts"); + *ID +} + +/// The current in-memory DBP identity and the serialized format for arrays with lower parts. +pub fn decimal_byte_parts_v2_id() -> ArrayId { + static ID: CachedId = CachedId::new("vortex.decimal_byte_parts_v2"); + *ID +} + +/// Serde for the current DBP array using the frozen v1 and v2 wire formats. +/// +/// Each version owns its metadata schema and serde functions. Arrays without lower parts use +/// v1; arrays with lower parts use v2. Both serializers borrow the current array, and both +/// decoders construct a current [`DecimalByteParts`] array directly. +/// +/// Register this plugin, or call [`crate::initialize`], to enable both formats. Direct registration +/// of [`DecimalByteParts`] does not support serde. +#[derive(Clone, Debug)] +pub struct DecimalBytePartsPlugin; + +impl ArrayPlugin for DecimalBytePartsPlugin { + fn id(&self) -> ArrayId { + VTable::id(&DecimalByteParts) + } + + fn serialized_ids(&self) -> Vec { + vec![decimal_byte_parts_v1_id(), decimal_byte_parts_v2_id()] + } + + fn serialize( + &self, + array: &ArrayRef, + _session: &VortexSession, + ) -> VortexResult> { + let view = array.as_opt::().ok_or_else(|| { + vortex_err!( + "DecimalByteParts plugin cannot serialize {}", + array.encoding_id() + ) + })?; + let serialized = if view.lower_parts().is_empty() { + v1::serialize(view)? + } else { + v2::serialize(view)? + }; + Ok(Some(serialized)) + } + + fn deserialize( + &self, + parts: ArrayDeserialization<'_>, + _session: &VortexSession, + ) -> VortexResult { + let array = if parts.serialized_id == decimal_byte_parts_v1_id() { + v1::deserialize(parts)? + } else if parts.serialized_id == decimal_byte_parts_v2_id() { + v2::deserialize(parts)? + } else { + vortex_bail!( + "DecimalByteParts plugin does not recognize serialized ID {}", + parts.serialized_id + ) + }; + Ok(array.into_array()) + } +} diff --git a/encodings/decimal-byte-parts/src/decimal_byte_parts/plugin/tests.rs b/encodings/decimal-byte-parts/src/decimal_byte_parts/plugin/tests.rs new file mode 100644 index 00000000000..f6c8d22b990 --- /dev/null +++ b/encodings/decimal-byte-parts/src/decimal_byte_parts/plugin/tests.rs @@ -0,0 +1,420 @@ +// SPDX-License-Identifier: Apache-2.0 +// SPDX-FileCopyrightText: Copyright the Vortex contributors + +use prost::Message as _; +use rstest::rstest; +use vortex_array::ArrayContext; +use vortex_array::ArrayDeserialization; +use vortex_array::ArrayVTable; +use vortex_array::VortexSessionExecute; +use vortex_array::array_session; +use vortex_array::arrays::ConstantArray; +use vortex_array::arrays::DecimalArray; +use vortex_array::arrays::Primitive; +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::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::VortexExpect; +use vortex_session::registry::ReadContext; + +use super::*; +use crate::DecimalBytePartsArray; +use crate::DecimalBytePartsArraySlotsExt; +use crate::decimal_byte_parts::testing::encode; +use crate::decimal_byte_parts::testing::i128_parts; +use crate::decimal_byte_parts::testing::i256_parts; +use crate::decimal_byte_parts::testing::wide_i128_values; +use crate::decimal_byte_parts::testing::wide_i256_values; + +#[rstest] +#[case::v1(0)] +#[case::v2_one_lower_part(1)] +#[case::v2_two_lower_parts(2)] +#[case::v2_three_lower_parts(3)] +fn serde_reuses_children(#[case] lower_part_count: usize) -> VortexResult<()> { + let array = DecimalByteParts::try_new_with_lower_parts( + msp(), + (0..lower_part_count).map(|_| lower_part()).collect(), + DecimalDType::new(76, 2), + )? + .into_array(); + assert_eq!(array.encoding_id(), decimal_byte_parts_v2_id()); + let session = session(); + let serialized = session + .array_serialize(&array)? + .vortex_expect("serializable"); + let original_children = array.children(); + assert_eq!(original_children.len(), serialized.children.len()); + for (original, written) in original_children.iter().zip(&serialized.children) { + assert!(ArrayRef::ptr_eq(original, written)); + } + let decoded = DecimalBytePartsPlugin.deserialize( + ArrayDeserialization::new( + serialized.serialized_id, + array.dtype(), + array.len(), + &serialized.metadata, + &[], + &serialized.children, + ), + &session, + )?; + assert_eq!(decoded.encoding_id(), decimal_byte_parts_v2_id()); + let decoded_children = decoded.children(); + assert_eq!(original_children.len(), decoded_children.len()); + for (original, decoded) in original_children.iter().zip(&decoded_children) { + assert!(ArrayRef::ptr_eq(original, decoded)); + } + Ok(()) +} + +#[rstest] +#[case::wide(msp(), lower_part())] +#[case::numerically_narrow(buffer![0i64; 3].into_array(), lower_part())] +fn v1_serializer_rejects_lower_parts( + #[case] msp: ArrayRef, + #[case] lower: ArrayRef, +) -> VortexResult<()> { + let array = + DecimalByteParts::try_new_with_lower_parts(msp, vec![lower], DecimalDType::new(38, 2))?; + assert!(v1::serialize(array.as_view()).is_err()); + Ok(()) +} + +#[test] +fn frozen_decoder_ignores_unknown_metadata_fields() -> VortexResult<()> { + let child = msp(); + let children = vec![child.clone()]; + let dtype = DType::Decimal(DecimalDType::new(19, 2), Nullability::NonNullable); + // Field 3 belongs to v2. The frozen protobuf schema ignores it, as the old decoder did; + // the recognized lower-part count is still zero and there is still exactly one child. + let metadata = [8, 7, 26, 1, 0]; + let current = v1::deserialize(ArrayDeserialization::new( + decimal_byte_parts_v1_id(), + &dtype, + 3, + &metadata, + &[], + &children, + ))?; + assert!(ArrayRef::ptr_eq(&child, current.msp())); + Ok(()) +} + +#[rstest] +#[case::no_lower_parts(DecimalByteParts::try_new( + buffer![1i32, 2, 3].into_array(), DecimalDType::new(9, 2), +))] +#[case::one_lower_part(Ok(i128_parts(wide_i128_values(), Validity::NonNullable)))] +#[case::three_lower_parts(Ok(i256_parts(wide_i256_values(), Validity::NonNullable)))] +#[case::nullable_three_lower_parts(Ok(i256_parts( + wide_i256_values(), + Validity::from_iter([true, false, true, true, true, false, true, true, true, true]), +)))] +#[case::wider_i64_storage(encode(&DecimalArray::new( + buffer![-99i64, 0, 99], DecimalDType::new(2, 0), Validity::NonNullable, +)))] +#[case::wider_i128_storage(encode(&DecimalArray::new( + buffer![-99i128, 0, 99], DecimalDType::new(2, 0), Validity::NonNullable, +)))] +#[case::wider_i256_storage(encode(&DecimalArray::new( + buffer![i256::from_i128(-99), i256::ZERO, i256::from_i128(99)], + DecimalDType::new(2, 0), Validity::NonNullable, +)))] +#[case::redundant_two_lower_parts(DecimalByteParts::try_new_with_lower_parts( + buffer![0i64; 3].into_array(), + vec![buffer![0u64; 3].into_array(), lower_part()], + DecimalDType::new(38, 2), +))] +#[case::redundant_three_lower_parts(DecimalByteParts::try_new_with_lower_parts( + buffer![0i64; 3].into_array(), + vec![buffer![0u64; 3].into_array(), buffer![0u64; 3].into_array(), lower_part()], + DecimalDType::new(38, 2), +))] +#[case::narrowed_one_lower_part(DecimalByteParts::try_new_with_lower_parts( + buffer![-1i8, 0, 1].into_array(), + vec![buffer![0u8, 128, u8::MAX].into_array()], + DecimalDType::new(38, 2), +))] +#[case::narrowed_three_lower_parts(DecimalByteParts::try_new_with_lower_parts( + buffer![-1i16, 0, 1].into_array(), + vec![ + buffer![u8::MAX, 128, 0].into_array(), + ConstantArray::new(u16::MAX, 3).into_array(), + buffer![0u32, 1 << 31, u32::MAX].into_array(), + ], + DecimalDType::new(76, 2), +))] +#[case::nullable_mixed_lower_parts(DecimalByteParts::try_new_with_lower_parts( + PrimitiveArray::new( + buffer![-1i8, 0, 1], Validity::from_iter([true, false, true]), + ).into_array(), + vec![ + buffer![u64::MAX, 1 << 63, 0].into_array(), + buffer![0u8, 128, u8::MAX].into_array(), + buffer![u32::MAX, 1 << 31, 0].into_array(), + ], + DecimalDType::new(76, 2), +))] +fn test_serde_round_trip(#[case] array: VortexResult) -> VortexResult<()> { + let session = session(); + let array = array?; + let lower_part_count = array.lower_parts().len(); + let lower_part_dtypes: Vec<_> = array + .lower_parts() + .iter() + .map(|part| part.dtype().clone()) + .collect(); + let array = array.into_array(); + let dtype = array.dtype().clone(); + let len = array.len(); + + let expected_id = if lower_part_count == 0 { + decimal_byte_parts_v1_id() + } else { + decimal_byte_parts_v2_id() + }; + assert_eq!( + session + .array_serialize(&array)? + .vortex_expect("byte parts arrays are serializable") + .serialized_id, + expected_id + ); + + 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() + .iter() + .map(|part| part.dtype().clone()) + .collect::>(), + lower_part_dtypes, + "lower-part dtypes and order must survive serde" + ); + + let mut ctx = session.create_execution_ctx(); + assert_arrays_eq!(array, decoded, &mut ctx); + Ok(()) +} + +#[rstest] +#[case::missing_lower_part(1, vec![msp()])] +#[case::extra_lower_part(0, vec![msp(), lower_part()])] +#[case::too_many_lower_parts( + 4, vec![msp(), lower_part(), lower_part(), lower_part(), lower_part()], +)] +fn test_deserialize_rejects_child_count_mismatch( + #[case] lower_part_count: u32, + #[case] children: Vec, +) { + let serialized_id = if lower_part_count == 0 { + decimal_byte_parts_v1_id() + } else { + decimal_byte_parts_v2_id() + }; + assert!(plugin_deserialize_with(serialized_id, lower_part_count, children).is_err()); +} + +#[rstest] +#[case::missing_type(1, vec![], "expected 1 lower-part dtypes, got 0")] +#[case::extra_type( + 1, vec![PType::U64 as i32, PType::U8 as i32], + "expected 1 lower-part dtypes, got 2", +)] +#[case::signed_type(1, vec![PType::I64 as i32], "unsigned integer dtype")] +#[case::float_type(1, vec![PType::F64 as i32], "unsigned integer dtype")] +#[case::unknown_type(1, vec![i32::MAX], "invalid PType")] +fn test_deserialize_rejects_invalid_lower_part_ptypes( + #[case] lower_part_count: u32, + #[case] lower_part_ptypes: Vec, + #[case] expected_error: &str, +) { + let metadata = DecimalBytePartsV2Metadata { + zeroth_child_ptype: PType::I64 as i32, + lower_part_count, + lower_part_ptypes, + } + .encode_to_vec(); + let mut children = vec![msp()]; + children.extend((0..lower_part_count).map(|_| lower_part())); + let serialized_id = if lower_part_count == 0 { + decimal_byte_parts_v1_id() + } else { + decimal_byte_parts_v2_id() + }; + let dtype = DType::Decimal(DecimalDType::new(38, 2), Nullability::NonNullable); + let result = DecimalBytePartsPlugin.deserialize( + ArrayDeserialization::new(serialized_id, &dtype, 3, &metadata, &[], &children), + &array_session(), + ); + assert!( + result + .as_ref() + .is_err_and(|err| err.to_string().contains(expected_error)), + "expected {expected_error}, got {result:?}" + ); +} + +fn plugin_deserialize_with( + serialized_id: ArrayId, + lower_part_count: u32, + children: Vec, +) -> VortexResult { + let metadata = DecimalBytePartsV2Metadata { + zeroth_child_ptype: PType::I64 as i32, + lower_part_count, + lower_part_ptypes: (0..lower_part_count).map(|_| PType::U64 as i32).collect(), + } + .encode_to_vec(); + let dtype = DType::Decimal(DecimalDType::new(38, 2), Nullability::NonNullable); + DecimalBytePartsPlugin.deserialize( + ArrayDeserialization::new(serialized_id, &dtype, 3, &metadata, &[], &children), + &array_session(), + ) +} + +/// Each serialized ID keeps its own contract: the frozen ID never carries lower parts, and +/// the v2 ID is never written without them. +#[rstest] +#[case::frozen_without_lower_parts(decimal_byte_parts_v1_id(), 0, vec![msp()], true)] +#[case::frozen_with_lower_parts( + decimal_byte_parts_v1_id(), + 1, + vec![msp(), lower_part()], + false +)] +#[case::v2_with_lower_parts(decimal_byte_parts_v2_id(), 1, vec![msp(), lower_part()], true)] +#[case::v2_without_lower_parts(decimal_byte_parts_v2_id(), 0, vec![msp()], false)] +fn plugin_holds_each_id_to_its_contract( + #[case] serialized_id: ArrayId, + #[case] lower_part_count: u32, + #[case] children: Vec, + #[case] accepted: bool, +) { + let result = plugin_deserialize_with(serialized_id, lower_part_count, children); + assert_eq!(result.is_ok(), accepted, "{serialized_id}: {result:?}"); +} + +fn msp() -> ArrayRef { + buffer![1i64, 2, 3].into_array() +} + +fn lower_part() -> ArrayRef { + buffer![1u64, 2, 3].into_array() +} + +fn session() -> VortexSession { + let session = array_session(); + crate::initialize(&session); + session +} + +#[test] +fn frozen_metadata_is_unchanged() -> VortexResult<()> { + let session = session(); + let array = DecimalByteParts::try_new(msp(), DecimalDType::new(19, 2))?.into_array(); + let serialized = session + .array_serialize(&array)? + .vortex_expect("byte parts arrays are serializable"); + assert_eq!(serialized.serialized_id, decimal_byte_parts_v1_id()); + // Frozen metadata for an i64 MSP: field 1 = 7, with no lower-part fields emitted. + assert_eq!(serialized.metadata, [8, 7]); + Ok(()) +} + +#[test] +fn serialization_requires_v2_permission() -> VortexResult<()> { + let session = session(); + let array = DecimalByteParts::try_new_with_lower_parts( + msp(), + vec![lower_part()], + DecimalDType::new(38, 2), + )? + .into_array(); + + let restricted = ArrayContext::empty().with_allowed_ids( + [decimal_byte_parts_v1_id(), ArrayVTable::id(&Primitive)] + .into_iter() + .collect(), + ); + let err = array + .serialize(&restricted, &session, &SerializeOptions::default()) + .expect_err("expected the permitted-encoding check to refuse the v2 format"); + assert!( + err.to_string().contains("not permitted"), + "error should name the permitted-encoding check, got: {err}" + ); + + // Permitting the v2 format id is exactly what allows the same array through. + let permissive = ArrayContext::empty().with_allowed_ids( + [ + decimal_byte_parts_v1_id(), + decimal_byte_parts_v2_id(), + ArrayVTable::id(&Primitive), + ] + .into_iter() + .collect(), + ); + array.serialize(&permissive, &session, &SerializeOptions::default())?; + assert!( + permissive.to_ids().contains(&decimal_byte_parts_v2_id()), + "the file's encoding table must carry the v2 format id" + ); + + Ok(()) +} + +#[test] +fn bare_vtable_refuses_serde() -> VortexResult<()> { + let session = array_session(); + session.arrays().register(DecimalByteParts); + let msp = msp(); + let array = DecimalByteParts::try_new(msp.clone(), DecimalDType::new(19, 2))?.into_array(); + let result = session.array_serialize(&array); + assert!( + result.as_ref().is_err_and(|err| err + .to_string() + .contains("DecimalByteParts serialization requires DecimalBytePartsPlugin")), + "expected unsupported VTable serialization, got {result:?}" + ); + + let id = VTable::id(&DecimalByteParts); + let plugin = session + .arrays() + .registry() + .get(&id) + .vortex_expect("registered"); + let children = vec![msp]; + let result = plugin.deserialize( + ArrayDeserialization::new(id, array.dtype(), array.len(), &[8, 7], &[], &children), + &session, + ); + assert!( + result.as_ref().is_err_and(|err| err + .to_string() + .contains("DecimalByteParts deserialization requires DecimalBytePartsPlugin")), + "expected unsupported VTable deserialization, got {result:?}" + ); + Ok(()) +} diff --git a/encodings/decimal-byte-parts/src/decimal_byte_parts/plugin/v1.rs b/encodings/decimal-byte-parts/src/decimal_byte_parts/plugin/v1.rs new file mode 100644 index 00000000000..ec816c62736 --- /dev/null +++ b/encodings/decimal-byte-parts/src/decimal_byte_parts/plugin/v1.rs @@ -0,0 +1,84 @@ +// SPDX-License-Identifier: Apache-2.0 +// SPDX-FileCopyrightText: Copyright the Vortex contributors + +//! Serde for the frozen single-child DBP wire format. + +use prost::Message as _; +use vortex_array::Array; +use vortex_array::ArrayDeserialization; +use vortex_array::ArrayParts; +use vortex_array::ArraySerialization; +use vortex_array::ArrayView; +use vortex_array::dtype::DType; +use vortex_array::dtype::PType; +use vortex_array::smallvec::smallvec; +use vortex_error::VortexResult; +use vortex_error::vortex_ensure; + +use super::decimal_byte_parts_v1_id; +use crate::DecimalByteParts; +use crate::DecimalBytePartsArray; +use crate::DecimalBytePartsArraySlotsExt; +use crate::DecimalBytePartsData; + +#[derive(Clone, prost::Message)] +struct DecimalBytePartsMetadata { + #[prost(enumeration = "PType", tag = "1")] + zeroth_child_ptype: i32, + #[prost(uint32, tag = "2")] + lower_part_count: u32, +} + +pub(super) fn serialize( + array: ArrayView<'_, DecimalByteParts>, +) -> VortexResult { + vortex_ensure!( + array.lower_parts().is_empty(), + "v1 must not carry lower parts" + ); + let msp = array.msp(); + let metadata = DecimalBytePartsMetadata { + zeroth_child_ptype: PType::try_from(msp.dtype())? as i32, + lower_part_count: 0, + } + .encode_to_vec(); + Ok(ArraySerialization::new( + decimal_byte_parts_v1_id(), + metadata, + vec![], + vec![msp.clone()], + )) +} + +pub(super) fn deserialize(parts: ArrayDeserialization<'_>) -> VortexResult { + vortex_ensure!( + parts.serialized_id == decimal_byte_parts_v1_id(), + "expected the v1 format" + ); + let metadata = DecimalBytePartsMetadata::decode(parts.metadata)?; + vortex_ensure!( + parts.dtype.as_decimal_opt().is_some(), + "expected a decimal dtype" + ); + vortex_ensure!( + metadata.lower_part_count == 0, + "v1 must not carry lower parts" + ); + vortex_ensure!(parts.children.len() == 1, "v1 must carry exactly one child"); + let ptype = PType::try_from(metadata.zeroth_child_ptype)?; + vortex_ensure!( + ptype.is_signed_int(), + "MSP must have a signed integer dtype" + ); + let encoded_dtype = DType::Primitive(ptype, parts.dtype.nullability()); + let msp = parts.children.get(0, &encoded_dtype, parts.len)?; + Array::try_from_parts( + ArrayParts::new( + DecimalByteParts, + parts.dtype.clone(), + parts.len, + DecimalBytePartsData, + ) + .with_slots(smallvec![Some(msp)]), + ) +} diff --git a/encodings/decimal-byte-parts/src/decimal_byte_parts/plugin/v2.rs b/encodings/decimal-byte-parts/src/decimal_byte_parts/plugin/v2.rs new file mode 100644 index 00000000000..759ebfe61fb --- /dev/null +++ b/encodings/decimal-byte-parts/src/decimal_byte_parts/plugin/v2.rs @@ -0,0 +1,130 @@ +// SPDX-License-Identifier: Apache-2.0 +// SPDX-FileCopyrightText: Copyright the Vortex contributors + +//! Serde for DBP values with unsigned lower parts. + +use num_traits::AsPrimitive; +use prost::Message as _; +use vortex_array::Array; +use vortex_array::ArrayDeserialization; +use vortex_array::ArrayParts; +use vortex_array::ArraySerialization; +use vortex_array::ArraySlots; +use vortex_array::ArrayView; +use vortex_array::dtype::DType; +use vortex_array::dtype::Nullability; +use vortex_array::dtype::PType; +use vortex_error::VortexResult; +use vortex_error::vortex_ensure; +use vortex_error::vortex_err; + +use super::decimal_byte_parts_v2_id; +use crate::DecimalByteParts; +use crate::DecimalBytePartsArray; +use crate::DecimalBytePartsArraySlotsExt; +use crate::DecimalBytePartsData; +use crate::decimal_byte_parts::MAX_LOWER_PARTS; + +/// Metadata for decimal byte parts with per-child storage types. +#[derive(Clone, prost::Message)] +pub struct DecimalBytePartsV2Metadata { + #[prost(enumeration = "PType", tag = "1")] + pub(super) zeroth_child_ptype: i32, + #[prost(uint32, tag = "2")] + pub(super) lower_part_count: u32, + /// Unsigned storage types of the lower parts, most significant first. + #[prost(enumeration = "PType", repeated, tag = "3")] + pub(super) lower_part_ptypes: Vec, +} + +pub(super) fn serialize( + array: ArrayView<'_, DecimalByteParts>, +) -> VortexResult { + let lower_parts = array.lower_parts(); + vortex_ensure!(!lower_parts.is_empty(), "v2 requires lower parts"); + let metadata = DecimalBytePartsV2Metadata { + zeroth_child_ptype: PType::try_from(array.msp().dtype())? as i32, + lower_part_count: u32::try_from(lower_parts.len()) + .map_err(|_| vortex_err!("lower part count exceeds u32"))?, + lower_part_ptypes: lower_parts + .iter() + .map(|part| PType::try_from(part.dtype()).map(|ptype| ptype as i32)) + .collect::>()?, + } + .encode_to_vec(); + let mut children = Vec::with_capacity(1 + lower_parts.len()); + children.push(array.msp().clone()); + children.extend(lower_parts.iter().cloned()); + Ok(ArraySerialization::new( + decimal_byte_parts_v2_id(), + metadata, + vec![], + children, + )) +} + +pub(super) fn deserialize(parts: ArrayDeserialization<'_>) -> VortexResult { + vortex_ensure!( + parts.serialized_id == decimal_byte_parts_v2_id(), + "expected the v2 format" + ); + let metadata = DecimalBytePartsV2Metadata::decode(parts.metadata)?; + vortex_ensure!( + parts.dtype.as_decimal_opt().is_some(), + "expected a decimal dtype" + ); + + let n_lower_parts: usize = metadata.lower_part_count.as_(); + + vortex_ensure!( + n_lower_parts <= MAX_LOWER_PARTS, + "expected at most {MAX_LOWER_PARTS} lower parts" + ); + + let n_lower_part_ptypes = metadata.lower_part_ptypes.len(); + vortex_ensure!( + n_lower_part_ptypes == n_lower_parts, + "got {n_lower_part_ptypes} lower part ptypes but {n_lower_parts} lower parts" + ); + + let n_children = parts.children.len(); + let n_children_expected = 1 + n_lower_parts; + vortex_ensure!( + n_children == n_children_expected, + "expected {n_children_expected} children, got {n_children}" + ); + + let msp_ptype = PType::try_from(metadata.zeroth_child_ptype)?; + vortex_ensure!( + msp_ptype.is_signed_int(), + "MSP must have a signed integer ptype" + ); + + let msp_dtype = DType::Primitive(msp_ptype, parts.dtype.nullability()); + + let mut slots = ArraySlots::with_capacity(parts.children.len()); + slots.push(Some(parts.children.get(0, &msp_dtype, parts.len)?)); + + for (idx, raw_ptype) in metadata.lower_part_ptypes.into_iter().enumerate() { + let ptype = PType::try_from(raw_ptype) + .map_err(|_| vortex_err!("invalid PType {raw_ptype} for lower part {idx}"))?; + vortex_ensure!( + ptype.is_unsigned_int(), + "lower part {idx} must have an unsigned integer dtype, got {ptype}" + ); + slots.push(Some(parts.children.get( + 1 + idx, + &DType::Primitive(ptype, Nullability::NonNullable), + parts.len, + )?)); + } + Array::try_from_parts( + ArrayParts::new( + DecimalByteParts, + parts.dtype.clone(), + parts.len, + DecimalBytePartsData, + ) + .with_slots(slots), + ) +} diff --git a/vortex-btrblocks/src/trace_tests.rs b/vortex-btrblocks/src/trace_tests.rs index f173440f26d..0cae7bea87e 100644 --- a/vortex-btrblocks/src/trace_tests.rs +++ b/vortex-btrblocks/src/trace_tests.rs @@ -253,7 +253,7 @@ fn trace_scan_compare_on_compressed_quantity() -> VortexResult<()> { optimize root=vortex.binary(bool, len=4096) session=false reduce_parent static:DictionaryScalarFnValuesPushDownRule slot=0 parent=vortex.binary(bool, len=4096) child=vortex.dict(i16, len=4096) -> vortex.dict(bool, len=4096) done output=vortex.dict(bool, len=4096) - child_execute_parent session[0]:execute_parent_fn slot=0 parent=vortex.binary(bool, len=4096) child=vortex.decimal_byte_parts(decimal(15,2), len=4096) -> vortex.dict(bool, len=4096) + child_execute_parent session[0]:execute_parent_fn slot=0 parent=vortex.binary(bool, len=4096) child=vortex.decimal_byte_parts_v2(decimal(15,2), len=4096) -> vortex.dict(bool, len=4096) iter 1 current=vortex.dict(bool, len=4096) builder_active=false ExecuteSlot slot=0 parent=vortex.dict(bool, len=4096) child=fastlanes.bitpacked(u8, len=4096) iter 2 current=fastlanes.bitpacked(u8, len=4096) stack_parent=vortex.dict(bool, len=4096) slot=0 builder_active=false @@ -418,8 +418,8 @@ fn trace_scan_filter_on_compressed_table() -> VortexResult<()> { optimize root=vortex.filter(i16, len=43) session=false reduce_parent static:FilterReduceAdaptor(Dict) slot=0 parent=vortex.filter(i16, len=43) child=vortex.dict(i16, len=4096) -> vortex.dict(i16, len=43) done output=vortex.dict(i16, len=43) - reduce_parent static:FilterReduceAdaptor(DecimalByteParts) slot=0 parent=vortex.filter(decimal(15,2), len=43) child=vortex.decimal_byte_parts(decimal(15,2), len=4096) -> vortex.decimal_byte_parts(decimal(15,2), len=43) - done output=vortex.decimal_byte_parts(decimal(15,2), len=43) + reduce_parent static:FilterReduceAdaptor(DecimalByteParts) slot=0 parent=vortex.filter(decimal(15,2), len=43) child=vortex.decimal_byte_parts_v2(decimal(15,2), len=4096) -> vortex.decimal_byte_parts_v2(decimal(15,2), len=43) + done output=vortex.decimal_byte_parts_v2(decimal(15,2), len=43) optimize root=vortex.filter(vortex.date[days](i32), len=43) session=false optimize root=vortex.filter(i32, len=43) session=false reduce_parent static:FoRFilterPushDownRule slot=0 parent=vortex.filter(i32, len=43) child=fastlanes.for(i32, len=4096) -> fastlanes.for(i32, len=43) @@ -455,8 +455,8 @@ fn trace_scan_take_on_compressed_table() -> VortexResult<()> { insta::assert_snapshot!(optimized.trace.to_string(), @" optimize root=vortex.dict({l_quantity=decimal(15,2), l_shipdate=vortex.date[days](i32), l_shipmode=utf8}, len=64) session=false optimize root=vortex.dict(decimal(15,2), len=64) session=false - reduce_parent static:TakeReduceAdaptor(DecimalByteParts) slot=1 parent=vortex.dict(decimal(15,2), len=64) child=vortex.decimal_byte_parts(decimal(15,2), len=4096) -> vortex.decimal_byte_parts(decimal(15,2), len=64) - done output=vortex.decimal_byte_parts(decimal(15,2), len=64) + reduce_parent static:TakeReduceAdaptor(DecimalByteParts) slot=1 parent=vortex.dict(decimal(15,2), len=64) child=vortex.decimal_byte_parts_v2(decimal(15,2), len=4096) -> vortex.decimal_byte_parts_v2(decimal(15,2), len=64) + done output=vortex.decimal_byte_parts_v2(decimal(15,2), len=64) optimize root=vortex.dict(vortex.date[days](i32), len=64) session=false reduce_parent static:TakeReduceAdaptor(Extension) slot=1 parent=vortex.dict(vortex.date[days](i32), len=64) child=vortex.ext(vortex.date[days](i32), len=4096) -> vortex.ext(vortex.date[days](i32), len=64) done output=vortex.ext(vortex.date[days](i32), len=64) diff --git a/vortex-btrblocks/tests/snapshots/golden__compact__decimal_prices.snap b/vortex-btrblocks/tests/snapshots/golden__compact__decimal_prices.snap index 780ef30a6b0..53a7e765582 100644 --- a/vortex-btrblocks/tests/snapshots/golden__compact__decimal_prices.snap +++ b/vortex-btrblocks/tests/snapshots/golden__compact__decimal_prices.snap @@ -3,7 +3,7 @@ source: vortex-btrblocks/tests/golden.rs expression: rendered --- input: decimal(12,2), len=16384, nbytes=131072 -root: vortex.decimal_byte_parts(decimal(12,2), len=16384) nbytes=47666 +root: vortex.decimal_byte_parts_v2(decimal(12,2), len=16384) nbytes=47666 metadata: msp: vortex.pco(i32, len=16384) nbytes=47666 metadata: ptype: i32, nrows: 16384, slice: 0..16384 diff --git a/vortex-btrblocks/tests/snapshots/golden__default__decimal_prices.snap b/vortex-btrblocks/tests/snapshots/golden__default__decimal_prices.snap index f669755e4b1..8b0568be671 100644 --- a/vortex-btrblocks/tests/snapshots/golden__default__decimal_prices.snap +++ b/vortex-btrblocks/tests/snapshots/golden__default__decimal_prices.snap @@ -3,7 +3,7 @@ source: vortex-btrblocks/tests/golden.rs expression: rendered --- input: decimal(12,2), len=16384, nbytes=131072 -root: vortex.decimal_byte_parts(decimal(12,2), len=16384) nbytes=49152 +root: vortex.decimal_byte_parts_v2(decimal(12,2), len=16384) nbytes=49152 metadata: msp: fastlanes.bitpacked(i32, len=16384) nbytes=49152 metadata: bit_width: 24, offset: 0 diff --git a/vortex-btrblocks/tests/snapshots/golden__unstable__decimal_prices.snap b/vortex-btrblocks/tests/snapshots/golden__unstable__decimal_prices.snap index f669755e4b1..8b0568be671 100644 --- a/vortex-btrblocks/tests/snapshots/golden__unstable__decimal_prices.snap +++ b/vortex-btrblocks/tests/snapshots/golden__unstable__decimal_prices.snap @@ -3,7 +3,7 @@ source: vortex-btrblocks/tests/golden.rs expression: rendered --- input: decimal(12,2), len=16384, nbytes=131072 -root: vortex.decimal_byte_parts(decimal(12,2), len=16384) nbytes=49152 +root: vortex.decimal_byte_parts_v2(decimal(12,2), len=16384) nbytes=49152 metadata: msp: fastlanes.bitpacked(i32, len=16384) nbytes=49152 metadata: bit_width: 24, offset: 0 From d434d504b3c7871e392197e45c3830542a101eda Mon Sep 17 00:00:00 2001 From: Matt Katz Date: Tue, 15 Sep 2026 16:03:08 -0400 Subject: [PATCH 07/17] Require lower parts in the v2 decoder and tidy plugin tests Signed-off-by: Matt Katz --- .../src/decimal_byte_parts/plugin/mod.rs | 1 + .../src/decimal_byte_parts/plugin/tests.rs | 190 +++++++++--------- .../src/decimal_byte_parts/plugin/v2.rs | 33 ++- 3 files changed, 112 insertions(+), 112 deletions(-) diff --git a/encodings/decimal-byte-parts/src/decimal_byte_parts/plugin/mod.rs b/encodings/decimal-byte-parts/src/decimal_byte_parts/plugin/mod.rs index 8456346785e..f393a749ece 100644 --- a/encodings/decimal-byte-parts/src/decimal_byte_parts/plugin/mod.rs +++ b/encodings/decimal-byte-parts/src/decimal_byte_parts/plugin/mod.rs @@ -21,6 +21,7 @@ use super::DecimalBytePartsArraySlotsExt; #[cfg(test)] mod tests; + mod v1; mod v2; diff --git a/encodings/decimal-byte-parts/src/decimal_byte_parts/plugin/tests.rs b/encodings/decimal-byte-parts/src/decimal_byte_parts/plugin/tests.rs index f6c8d22b990..7d5e0c74bf6 100644 --- a/encodings/decimal-byte-parts/src/decimal_byte_parts/plugin/tests.rs +++ b/encodings/decimal-byte-parts/src/decimal_byte_parts/plugin/tests.rs @@ -92,11 +92,24 @@ fn v1_serializer_rejects_lower_parts( } #[test] -fn frozen_decoder_ignores_unknown_metadata_fields() -> VortexResult<()> { +fn v1_metadata_is_unchanged() -> VortexResult<()> { + let session = session(); + let array = DecimalByteParts::try_new(msp(), DecimalDType::new(19, 2))?.into_array(); + let serialized = session + .array_serialize(&array)? + .vortex_expect("byte parts arrays are serializable"); + assert_eq!(serialized.serialized_id, decimal_byte_parts_v1_id()); + // v1 metadata for an i64 MSP: field 1 = 7, with no lower-part fields emitted. + assert_eq!(serialized.metadata, [8, 7]); + Ok(()) +} + +#[test] +fn v1_decoder_ignores_unknown_metadata_fields() -> VortexResult<()> { let child = msp(); let children = vec![child.clone()]; let dtype = DType::Decimal(DecimalDType::new(19, 2), Nullability::NonNullable); - // Field 3 belongs to v2. The frozen protobuf schema ignores it, as the old decoder did; + // Field 3 belongs to v2. The v1 protobuf schema ignores it, as the old decoder did; // the recognized lower-part count is still zero and there is still exactly one child. let metadata = [8, 7, 26, 1, 0]; let current = v1::deserialize(ArrayDeserialization::new( @@ -166,7 +179,7 @@ fn frozen_decoder_ignores_unknown_metadata_fields() -> VortexResult<()> { ], DecimalDType::new(76, 2), ))] -fn test_serde_round_trip(#[case] array: VortexResult) -> VortexResult<()> { +fn serde_round_trip(#[case] array: VortexResult) -> VortexResult<()> { let session = session(); let array = array?; let lower_part_count = array.lower_parts().len(); @@ -218,13 +231,35 @@ fn test_serde_round_trip(#[case] array: VortexResult) -> Ok(()) } +/// Each serialized ID keeps its own contract: the v1 ID never carries lower parts, and the v2 +/// ID is never written without them. +#[rstest] +#[case::v1_without_lower_parts(decimal_byte_parts_v1_id(), 0, vec![msp()], true)] +#[case::v1_with_lower_parts(decimal_byte_parts_v1_id(), 1, vec![msp(), lower_part()], false)] +#[case::v2_with_lower_parts(decimal_byte_parts_v2_id(), 1, vec![msp(), lower_part()], true)] +#[case::v2_without_lower_parts(decimal_byte_parts_v2_id(), 0, vec![msp()], false)] +fn plugin_holds_each_id_to_its_contract( + #[case] serialized_id: ArrayId, + #[case] lower_part_count: u32, + #[case] children: Vec, + #[case] accepted: bool, +) { + let result = deserialize_with( + serialized_id, + lower_part_count, + u64_ptypes(lower_part_count), + children, + ); + assert_eq!(result.is_ok(), accepted, "{serialized_id}: {result:?}"); +} + #[rstest] #[case::missing_lower_part(1, vec![msp()])] #[case::extra_lower_part(0, vec![msp(), lower_part()])] #[case::too_many_lower_parts( 4, vec![msp(), lower_part(), lower_part(), lower_part(), lower_part()], )] -fn test_deserialize_rejects_child_count_mismatch( +fn deserialize_rejects_child_count_mismatch( #[case] lower_part_count: u32, #[case] children: Vec, ) { @@ -233,40 +268,33 @@ fn test_deserialize_rejects_child_count_mismatch( } else { decimal_byte_parts_v2_id() }; - assert!(plugin_deserialize_with(serialized_id, lower_part_count, children).is_err()); + let result = deserialize_with( + serialized_id, + lower_part_count, + u64_ptypes(lower_part_count), + children, + ); + assert!(result.is_err(), "{serialized_id}: {result:?}"); } #[rstest] -#[case::missing_type(1, vec![], "expected 1 lower-part dtypes, got 0")] +#[case::missing_type(vec![], "expected 1 lower-part dtypes, got 0")] #[case::extra_type( - 1, vec![PType::U64 as i32, PType::U8 as i32], + vec![PType::U64 as i32, PType::U8 as i32], "expected 1 lower-part dtypes, got 2", )] -#[case::signed_type(1, vec![PType::I64 as i32], "unsigned integer dtype")] -#[case::float_type(1, vec![PType::F64 as i32], "unsigned integer dtype")] -#[case::unknown_type(1, vec![i32::MAX], "invalid PType")] -fn test_deserialize_rejects_invalid_lower_part_ptypes( - #[case] lower_part_count: u32, +#[case::signed_type(vec![PType::I64 as i32], "unsigned integer dtype")] +#[case::float_type(vec![PType::F64 as i32], "unsigned integer dtype")] +#[case::unknown_type(vec![i32::MAX], "invalid PType")] +fn v2_decoder_rejects_invalid_lower_part_ptypes( #[case] lower_part_ptypes: Vec, #[case] expected_error: &str, ) { - let metadata = DecimalBytePartsV2Metadata { - zeroth_child_ptype: PType::I64 as i32, - lower_part_count, + let result = deserialize_with( + decimal_byte_parts_v2_id(), + 1, lower_part_ptypes, - } - .encode_to_vec(); - let mut children = vec![msp()]; - children.extend((0..lower_part_count).map(|_| lower_part())); - let serialized_id = if lower_part_count == 0 { - decimal_byte_parts_v1_id() - } else { - decimal_byte_parts_v2_id() - }; - let dtype = DType::Decimal(DecimalDType::new(38, 2), Nullability::NonNullable); - let result = DecimalBytePartsPlugin.deserialize( - ArrayDeserialization::new(serialized_id, &dtype, 3, &metadata, &[], &children), - &array_session(), + vec![msp(), lower_part()], ); assert!( result @@ -276,73 +304,6 @@ fn test_deserialize_rejects_invalid_lower_part_ptypes( ); } -fn plugin_deserialize_with( - serialized_id: ArrayId, - lower_part_count: u32, - children: Vec, -) -> VortexResult { - let metadata = DecimalBytePartsV2Metadata { - zeroth_child_ptype: PType::I64 as i32, - lower_part_count, - lower_part_ptypes: (0..lower_part_count).map(|_| PType::U64 as i32).collect(), - } - .encode_to_vec(); - let dtype = DType::Decimal(DecimalDType::new(38, 2), Nullability::NonNullable); - DecimalBytePartsPlugin.deserialize( - ArrayDeserialization::new(serialized_id, &dtype, 3, &metadata, &[], &children), - &array_session(), - ) -} - -/// Each serialized ID keeps its own contract: the frozen ID never carries lower parts, and -/// the v2 ID is never written without them. -#[rstest] -#[case::frozen_without_lower_parts(decimal_byte_parts_v1_id(), 0, vec![msp()], true)] -#[case::frozen_with_lower_parts( - decimal_byte_parts_v1_id(), - 1, - vec![msp(), lower_part()], - false -)] -#[case::v2_with_lower_parts(decimal_byte_parts_v2_id(), 1, vec![msp(), lower_part()], true)] -#[case::v2_without_lower_parts(decimal_byte_parts_v2_id(), 0, vec![msp()], false)] -fn plugin_holds_each_id_to_its_contract( - #[case] serialized_id: ArrayId, - #[case] lower_part_count: u32, - #[case] children: Vec, - #[case] accepted: bool, -) { - let result = plugin_deserialize_with(serialized_id, lower_part_count, children); - assert_eq!(result.is_ok(), accepted, "{serialized_id}: {result:?}"); -} - -fn msp() -> ArrayRef { - buffer![1i64, 2, 3].into_array() -} - -fn lower_part() -> ArrayRef { - buffer![1u64, 2, 3].into_array() -} - -fn session() -> VortexSession { - let session = array_session(); - crate::initialize(&session); - session -} - -#[test] -fn frozen_metadata_is_unchanged() -> VortexResult<()> { - let session = session(); - let array = DecimalByteParts::try_new(msp(), DecimalDType::new(19, 2))?.into_array(); - let serialized = session - .array_serialize(&array)? - .vortex_expect("byte parts arrays are serializable"); - assert_eq!(serialized.serialized_id, decimal_byte_parts_v1_id()); - // Frozen metadata for an i64 MSP: field 1 = 7, with no lower-part fields emitted. - assert_eq!(serialized.metadata, [8, 7]); - Ok(()) -} - #[test] fn serialization_requires_v2_permission() -> VortexResult<()> { let session = session(); @@ -418,3 +379,42 @@ fn bare_vtable_refuses_serde() -> VortexResult<()> { ); Ok(()) } + +fn msp() -> ArrayRef { + buffer![1i64, 2, 3].into_array() +} + +fn lower_part() -> ArrayRef { + buffer![1u64, 2, 3].into_array() +} + +fn u64_ptypes(count: u32) -> Vec { + (0..count).map(|_| PType::U64 as i32).collect() +} + +fn session() -> VortexSession { + let session = array_session(); + crate::initialize(&session); + session +} + +/// Run the plugin decoder over hand-built metadata. The v2 struct encodes the same bytes as v1 +/// when it carries no lower parts, so it serves both IDs. +fn deserialize_with( + serialized_id: ArrayId, + lower_part_count: u32, + lower_part_ptypes: Vec, + children: Vec, +) -> VortexResult { + let metadata = DecimalBytePartsV2Metadata { + zeroth_child_ptype: PType::I64 as i32, + lower_part_count, + lower_part_ptypes, + } + .encode_to_vec(); + let dtype = DType::Decimal(DecimalDType::new(38, 2), Nullability::NonNullable); + DecimalBytePartsPlugin.deserialize( + ArrayDeserialization::new(serialized_id, &dtype, 3, &metadata, &[], &children), + &array_session(), + ) +} diff --git a/encodings/decimal-byte-parts/src/decimal_byte_parts/plugin/v2.rs b/encodings/decimal-byte-parts/src/decimal_byte_parts/plugin/v2.rs index 759ebfe61fb..85829778653 100644 --- a/encodings/decimal-byte-parts/src/decimal_byte_parts/plugin/v2.rs +++ b/encodings/decimal-byte-parts/src/decimal_byte_parts/plugin/v2.rs @@ -3,7 +3,6 @@ //! Serde for DBP values with unsigned lower parts. -use num_traits::AsPrimitive; use prost::Message as _; use vortex_array::Array; use vortex_array::ArrayDeserialization; @@ -74,37 +73,37 @@ pub(super) fn deserialize(parts: ArrayDeserialization<'_>) -> VortexResult Date: Tue, 15 Sep 2026 16:07:34 -0400 Subject: [PATCH 08/17] Infer the DBP v2 lower part count from its ptypes and name the MSP field Signed-off-by: Matt Katz --- .../src/decimal_byte_parts/plugin/tests.rs | 102 +++++++++--------- .../src/decimal_byte_parts/plugin/v2.rs | 28 ++--- 2 files changed, 60 insertions(+), 70 deletions(-) diff --git a/encodings/decimal-byte-parts/src/decimal_byte_parts/plugin/tests.rs b/encodings/decimal-byte-parts/src/decimal_byte_parts/plugin/tests.rs index 7d5e0c74bf6..2a53f817674 100644 --- a/encodings/decimal-byte-parts/src/decimal_byte_parts/plugin/tests.rs +++ b/encodings/decimal-byte-parts/src/decimal_byte_parts/plugin/tests.rs @@ -30,6 +30,7 @@ use vortex_session::registry::ReadContext; use super::*; use crate::DecimalBytePartsArray; use crate::DecimalBytePartsArraySlotsExt; +use crate::decimal_byte_parts::MAX_LOWER_PARTS; use crate::decimal_byte_parts::testing::encode; use crate::decimal_byte_parts::testing::i128_parts; use crate::decimal_byte_parts::testing::i256_parts; @@ -109,8 +110,8 @@ fn v1_decoder_ignores_unknown_metadata_fields() -> VortexResult<()> { let child = msp(); let children = vec![child.clone()]; let dtype = DType::Decimal(DecimalDType::new(19, 2), Nullability::NonNullable); - // Field 3 belongs to v2. The v1 protobuf schema ignores it, as the old decoder did; - // the recognized lower-part count is still zero and there is still exactly one child. + // An unknown trailing field. The v1 protobuf schema ignores it, as the original decoder + // did; the recognized lower-part count is still zero and there is still exactly one child. let metadata = [8, 7, 26, 1, 0]; let current = v1::deserialize(ArrayDeserialization::new( decimal_byte_parts_v1_id(), @@ -234,55 +235,50 @@ fn serde_round_trip(#[case] array: VortexResult) -> Vorte /// Each serialized ID keeps its own contract: the v1 ID never carries lower parts, and the v2 /// ID is never written without them. #[rstest] -#[case::v1_without_lower_parts(decimal_byte_parts_v1_id(), 0, vec![msp()], true)] -#[case::v1_with_lower_parts(decimal_byte_parts_v1_id(), 1, vec![msp(), lower_part()], false)] -#[case::v2_with_lower_parts(decimal_byte_parts_v2_id(), 1, vec![msp(), lower_part()], true)] -#[case::v2_without_lower_parts(decimal_byte_parts_v2_id(), 0, vec![msp()], false)] +#[case::v1_without_lower_parts(decimal_byte_parts_v1_id(), v1_metadata(0), vec![msp()], true)] +#[case::v1_with_lower_parts( + decimal_byte_parts_v1_id(), + v1_metadata(1), + vec![msp(), lower_part()], + false +)] +#[case::v2_with_lower_parts( + decimal_byte_parts_v2_id(), + v2_metadata(vec![PType::U64 as i32]), + vec![msp(), lower_part()], + true +)] +#[case::v2_without_lower_parts(decimal_byte_parts_v2_id(), v2_metadata(vec![]), vec![msp()], false)] fn plugin_holds_each_id_to_its_contract( #[case] serialized_id: ArrayId, - #[case] lower_part_count: u32, + #[case] metadata: Vec, #[case] children: Vec, #[case] accepted: bool, ) { - let result = deserialize_with( - serialized_id, - lower_part_count, - u64_ptypes(lower_part_count), - children, - ); + let result = deserialize_with(serialized_id, &metadata, children); assert_eq!(result.is_ok(), accepted, "{serialized_id}: {result:?}"); } #[rstest] -#[case::missing_lower_part(1, vec![msp()])] -#[case::extra_lower_part(0, vec![msp(), lower_part()])] -#[case::too_many_lower_parts( - 4, vec![msp(), lower_part(), lower_part(), lower_part(), lower_part()], +#[case::v1_extra_child(decimal_byte_parts_v1_id(), v1_metadata(0), vec![msp(), lower_part()])] +#[case::v2_missing_child(decimal_byte_parts_v2_id(), v2_metadata(vec![PType::U64 as i32]), vec![msp()])] +#[case::v2_extra_child( + decimal_byte_parts_v2_id(), + v2_metadata(vec![PType::U64 as i32]), + vec![msp(), lower_part(), lower_part()] )] fn deserialize_rejects_child_count_mismatch( - #[case] lower_part_count: u32, + #[case] serialized_id: ArrayId, + #[case] metadata: Vec, #[case] children: Vec, ) { - let serialized_id = if lower_part_count == 0 { - decimal_byte_parts_v1_id() - } else { - decimal_byte_parts_v2_id() - }; - let result = deserialize_with( - serialized_id, - lower_part_count, - u64_ptypes(lower_part_count), - children, - ); + let result = deserialize_with(serialized_id, &metadata, children); assert!(result.is_err(), "{serialized_id}: {result:?}"); } #[rstest] -#[case::missing_type(vec![], "expected 1 lower-part dtypes, got 0")] -#[case::extra_type( - vec![PType::U64 as i32, PType::U8 as i32], - "expected 1 lower-part dtypes, got 2", -)] +#[case::none(vec![], "lower parts, got 0")] +#[case::too_many(vec![PType::U64 as i32; MAX_LOWER_PARTS + 1], "lower parts, got 4")] #[case::signed_type(vec![PType::I64 as i32], "unsigned integer dtype")] #[case::float_type(vec![PType::F64 as i32], "unsigned integer dtype")] #[case::unknown_type(vec![i32::MAX], "invalid PType")] @@ -290,11 +286,12 @@ fn v2_decoder_rejects_invalid_lower_part_ptypes( #[case] lower_part_ptypes: Vec, #[case] expected_error: &str, ) { + let mut children = vec![msp()]; + children.extend((0..lower_part_ptypes.len()).map(|_| lower_part())); let result = deserialize_with( decimal_byte_parts_v2_id(), - 1, - lower_part_ptypes, - vec![msp(), lower_part()], + &v2_metadata(lower_part_ptypes), + children, ); assert!( result @@ -388,8 +385,22 @@ fn lower_part() -> ArrayRef { buffer![1u64, 2, 3].into_array() } -fn u64_ptypes(count: u32) -> Vec { - (0..count).map(|_| PType::U64 as i32).collect() +/// v1 metadata for an i64 MSP: field 1 = 7, then field 2 only when the count is non-zero, as +/// proto3 omits default values. +fn v1_metadata(lower_part_count: u8) -> Vec { + let mut metadata = vec![8, 7]; + if lower_part_count > 0 { + metadata.extend([16, lower_part_count]); + } + metadata +} + +fn v2_metadata(lower_part_ptypes: Vec) -> Vec { + DecimalBytePartsV2Metadata { + msp_ptype: PType::I64 as i32, + lower_part_ptypes, + } + .encode_to_vec() } fn session() -> VortexSession { @@ -398,23 +409,14 @@ fn session() -> VortexSession { session } -/// Run the plugin decoder over hand-built metadata. The v2 struct encodes the same bytes as v1 -/// when it carries no lower parts, so it serves both IDs. fn deserialize_with( serialized_id: ArrayId, - lower_part_count: u32, - lower_part_ptypes: Vec, + metadata: &[u8], children: Vec, ) -> VortexResult { - let metadata = DecimalBytePartsV2Metadata { - zeroth_child_ptype: PType::I64 as i32, - lower_part_count, - lower_part_ptypes, - } - .encode_to_vec(); let dtype = DType::Decimal(DecimalDType::new(38, 2), Nullability::NonNullable); DecimalBytePartsPlugin.deserialize( - ArrayDeserialization::new(serialized_id, &dtype, 3, &metadata, &[], &children), + ArrayDeserialization::new(serialized_id, &dtype, 3, metadata, &[], &children), &array_session(), ) } diff --git a/encodings/decimal-byte-parts/src/decimal_byte_parts/plugin/v2.rs b/encodings/decimal-byte-parts/src/decimal_byte_parts/plugin/v2.rs index 85829778653..e93fa2f2122 100644 --- a/encodings/decimal-byte-parts/src/decimal_byte_parts/plugin/v2.rs +++ b/encodings/decimal-byte-parts/src/decimal_byte_parts/plugin/v2.rs @@ -27,12 +27,12 @@ use crate::decimal_byte_parts::MAX_LOWER_PARTS; /// Metadata for decimal byte parts with per-child storage types. #[derive(Clone, prost::Message)] pub struct DecimalBytePartsV2Metadata { + /// Signed storage type of the most significant part. #[prost(enumeration = "PType", tag = "1")] - pub(super) zeroth_child_ptype: i32, - #[prost(uint32, tag = "2")] - pub(super) lower_part_count: u32, - /// Unsigned storage types of the lower parts, most significant first. - #[prost(enumeration = "PType", repeated, tag = "3")] + pub(super) msp_ptype: i32, + /// Unsigned storage types of the lower parts, most significant first. Their number is the + /// lower part count. + #[prost(enumeration = "PType", repeated, tag = "2")] pub(super) lower_part_ptypes: Vec, } @@ -42,9 +42,7 @@ pub(super) fn serialize( let lower_parts = array.lower_parts(); vortex_ensure!(!lower_parts.is_empty(), "v2 requires lower parts"); let metadata = DecimalBytePartsV2Metadata { - zeroth_child_ptype: PType::try_from(array.msp().dtype())? as i32, - lower_part_count: u32::try_from(lower_parts.len()) - .map_err(|_| vortex_err!("lower part count exceeds u32"))?, + msp_ptype: PType::try_from(array.msp().dtype())? as i32, lower_part_ptypes: lower_parts .iter() .map(|part| PType::try_from(part.dtype()).map(|ptype| ptype as i32)) @@ -73,21 +71,11 @@ pub(super) fn deserialize(parts: ArrayDeserialization<'_>) -> VortexResult) -> VortexResult Date: Tue, 15 Sep 2026 16:16:59 -0400 Subject: [PATCH 09/17] Let the DBP v2 format carry zero lower parts Signed-off-by: Matt Katz --- .../src/decimal_byte_parts/plugin/mod.rs | 7 ++--- .../src/decimal_byte_parts/plugin/tests.rs | 27 ++++++++++++++++--- .../src/decimal_byte_parts/plugin/v2.rs | 13 +++++---- 3 files changed, 33 insertions(+), 14 deletions(-) diff --git a/encodings/decimal-byte-parts/src/decimal_byte_parts/plugin/mod.rs b/encodings/decimal-byte-parts/src/decimal_byte_parts/plugin/mod.rs index f393a749ece..4cafd67421f 100644 --- a/encodings/decimal-byte-parts/src/decimal_byte_parts/plugin/mod.rs +++ b/encodings/decimal-byte-parts/src/decimal_byte_parts/plugin/mod.rs @@ -41,9 +41,10 @@ pub fn decimal_byte_parts_v2_id() -> ArrayId { /// Serde for the current DBP array using the frozen v1 and v2 wire formats. /// -/// Each version owns its metadata schema and serde functions. Arrays without lower parts use -/// v1; arrays with lower parts use v2. Both serializers borrow the current array, and both -/// decoders construct a current [`DecimalByteParts`] array directly. +/// Each version owns its metadata schema and serde functions. The plugin writes v1 whenever an +/// array has no lower parts, so such arrays stay readable by older readers, and v2 otherwise. +/// The v2 format itself accepts any lower part count up to the maximum. Both serializers borrow +/// the current array, and both decoders construct a current [`DecimalByteParts`] array directly. /// /// Register this plugin, or call [`crate::initialize`], to enable both formats. Direct registration /// of [`DecimalByteParts`] does not support serde. diff --git a/encodings/decimal-byte-parts/src/decimal_byte_parts/plugin/tests.rs b/encodings/decimal-byte-parts/src/decimal_byte_parts/plugin/tests.rs index 2a53f817674..294213928c8 100644 --- a/encodings/decimal-byte-parts/src/decimal_byte_parts/plugin/tests.rs +++ b/encodings/decimal-byte-parts/src/decimal_byte_parts/plugin/tests.rs @@ -92,6 +92,26 @@ fn v1_serializer_rejects_lower_parts( Ok(()) } +#[test] +fn v2_round_trips_without_lower_parts() -> VortexResult<()> { + let child = msp(); + let array = DecimalByteParts::try_new(child.clone(), DecimalDType::new(19, 2))?; + let serialized = v2::serialize(array.as_view())?; + assert_eq!(serialized.serialized_id, decimal_byte_parts_v2_id()); + assert_eq!(serialized.children.len(), 1); + let decoded = v2::deserialize(ArrayDeserialization::new( + serialized.serialized_id, + array.dtype(), + array.len(), + &serialized.metadata, + &[], + &serialized.children, + ))?; + assert!(ArrayRef::ptr_eq(&child, decoded.msp())); + assert!(decoded.lower_parts().is_empty()); + Ok(()) +} + #[test] fn v1_metadata_is_unchanged() -> VortexResult<()> { let session = session(); @@ -232,8 +252,8 @@ fn serde_round_trip(#[case] array: VortexResult) -> Vorte Ok(()) } -/// Each serialized ID keeps its own contract: the v1 ID never carries lower parts, and the v2 -/// ID is never written without them. +/// The v1 decoder never accepts lower parts. The v2 decoder accepts any count the array allows, +/// including none, even though the plugin only writes v2 when lower parts are present. #[rstest] #[case::v1_without_lower_parts(decimal_byte_parts_v1_id(), v1_metadata(0), vec![msp()], true)] #[case::v1_with_lower_parts( @@ -248,7 +268,7 @@ fn serde_round_trip(#[case] array: VortexResult) -> Vorte vec![msp(), lower_part()], true )] -#[case::v2_without_lower_parts(decimal_byte_parts_v2_id(), v2_metadata(vec![]), vec![msp()], false)] +#[case::v2_without_lower_parts(decimal_byte_parts_v2_id(), v2_metadata(vec![]), vec![msp()], true)] fn plugin_holds_each_id_to_its_contract( #[case] serialized_id: ArrayId, #[case] metadata: Vec, @@ -277,7 +297,6 @@ fn deserialize_rejects_child_count_mismatch( } #[rstest] -#[case::none(vec![], "lower parts, got 0")] #[case::too_many(vec![PType::U64 as i32; MAX_LOWER_PARTS + 1], "lower parts, got 4")] #[case::signed_type(vec![PType::I64 as i32], "unsigned integer dtype")] #[case::float_type(vec![PType::F64 as i32], "unsigned integer dtype")] diff --git a/encodings/decimal-byte-parts/src/decimal_byte_parts/plugin/v2.rs b/encodings/decimal-byte-parts/src/decimal_byte_parts/plugin/v2.rs index e93fa2f2122..412048b3701 100644 --- a/encodings/decimal-byte-parts/src/decimal_byte_parts/plugin/v2.rs +++ b/encodings/decimal-byte-parts/src/decimal_byte_parts/plugin/v2.rs @@ -24,14 +24,13 @@ use crate::DecimalBytePartsArraySlotsExt; use crate::DecimalBytePartsData; use crate::decimal_byte_parts::MAX_LOWER_PARTS; -/// Metadata for decimal byte parts with per-child storage types. +/// Metadata for decimal byte parts with lower parts. #[derive(Clone, prost::Message)] pub struct DecimalBytePartsV2Metadata { - /// Signed storage type of the most significant part. + /// Ptype of the most significant part. #[prost(enumeration = "PType", tag = "1")] pub(super) msp_ptype: i32, - /// Unsigned storage types of the lower parts, most significant first. Their number is the - /// lower part count. + /// Ptypes of the lower parts, ordered most significant first. #[prost(enumeration = "PType", repeated, tag = "2")] pub(super) lower_part_ptypes: Vec, } @@ -40,7 +39,7 @@ pub(super) fn serialize( array: ArrayView<'_, DecimalByteParts>, ) -> VortexResult { let lower_parts = array.lower_parts(); - vortex_ensure!(!lower_parts.is_empty(), "v2 requires lower parts"); + let metadata = DecimalBytePartsV2Metadata { msp_ptype: PType::try_from(array.msp().dtype())? as i32, lower_part_ptypes: lower_parts @@ -73,8 +72,8 @@ pub(super) fn deserialize(parts: ArrayDeserialization<'_>) -> VortexResult Date: Tue, 15 Sep 2026 16:23:35 -0400 Subject: [PATCH 10/17] Rename the DBP v2 wire ID to vortex.decimal_byte_parts.v2 Signed-off-by: Matt Katz --- docs/specs/editions.md | 4 ++-- .../decimal-byte-parts/src/decimal_byte_parts/array.rs | 2 +- .../src/decimal_byte_parts/plugin/mod.rs | 2 +- vortex-btrblocks/src/trace_tests.rs | 10 +++++----- .../snapshots/golden__compact__decimal_prices.snap | 2 +- .../snapshots/golden__default__decimal_prices.snap | 2 +- .../snapshots/golden__unstable__decimal_prices.snap | 2 +- 7 files changed, 12 insertions(+), 12 deletions(-) diff --git a/docs/specs/editions.md b/docs/specs/editions.md index ee5f494861f..1aa3cfe4e11 100644 --- a/docs/specs/editions.md +++ b/docs/specs/editions.md @@ -172,9 +172,9 @@ representation gains support for wide decimals, represented by a signed most-sig represented that way, it emits `vortex.decimal_byte_parts` with `lower_part_count = 0`, even if the current in-memory array has lower-part children. - An array that cannot be collapsed into that old form losslessly uses the new - `vortex.decimal_byte_parts_v2` component, initially staged in a draft edition. + `vortex.decimal_byte_parts.v2` component, initially staged in a draft edition. - A new reader deserializes both IDs into the same in-memory representation. An older reader reports - `vortex.decimal_byte_parts_v2` as unknown instead of trying to decode a wire format it does not support. + `vortex.decimal_byte_parts.v2` as unknown instead of trying to decode a wire format it does not support. - When targeting an edition that permits only the old ID, serializing a value that can be collapsed succeeds; an irreducibly multi-part value fails because no lossless downgrade exists. diff --git a/encodings/decimal-byte-parts/src/decimal_byte_parts/array.rs b/encodings/decimal-byte-parts/src/decimal_byte_parts/array.rs index 65ea454937d..61b6dfb9f1c 100644 --- a/encodings/decimal-byte-parts/src/decimal_byte_parts/array.rs +++ b/encodings/decimal-byte-parts/src/decimal_byte_parts/array.rs @@ -163,7 +163,7 @@ impl DecimalByteParts { ) -> VortexResult { // Building lower parts in memory is never gated — reading a file requires it. What is // gated is the serialized form: an array carrying lower parts serializes under the - // `vortex.decimal_byte_parts_v2` format ID, which only editions that contain it may write. + // `vortex.decimal_byte_parts.v2` format ID, which only editions that contain it may write. let len = msp.len(); let dtype = DType::Decimal(decimal_dtype, msp.dtype().nullability()); let slots = DecimalBytePartsSlots { msp, lower_parts }.into_slots(); diff --git a/encodings/decimal-byte-parts/src/decimal_byte_parts/plugin/mod.rs b/encodings/decimal-byte-parts/src/decimal_byte_parts/plugin/mod.rs index 4cafd67421f..3b7157e1160 100644 --- a/encodings/decimal-byte-parts/src/decimal_byte_parts/plugin/mod.rs +++ b/encodings/decimal-byte-parts/src/decimal_byte_parts/plugin/mod.rs @@ -35,7 +35,7 @@ pub fn decimal_byte_parts_v1_id() -> ArrayId { /// The current in-memory DBP identity and the serialized format for arrays with lower parts. pub fn decimal_byte_parts_v2_id() -> ArrayId { - static ID: CachedId = CachedId::new("vortex.decimal_byte_parts_v2"); + static ID: CachedId = CachedId::new("vortex.decimal_byte_parts.v2"); *ID } diff --git a/vortex-btrblocks/src/trace_tests.rs b/vortex-btrblocks/src/trace_tests.rs index 0cae7bea87e..4f7161b7bba 100644 --- a/vortex-btrblocks/src/trace_tests.rs +++ b/vortex-btrblocks/src/trace_tests.rs @@ -253,7 +253,7 @@ fn trace_scan_compare_on_compressed_quantity() -> VortexResult<()> { optimize root=vortex.binary(bool, len=4096) session=false reduce_parent static:DictionaryScalarFnValuesPushDownRule slot=0 parent=vortex.binary(bool, len=4096) child=vortex.dict(i16, len=4096) -> vortex.dict(bool, len=4096) done output=vortex.dict(bool, len=4096) - child_execute_parent session[0]:execute_parent_fn slot=0 parent=vortex.binary(bool, len=4096) child=vortex.decimal_byte_parts_v2(decimal(15,2), len=4096) -> vortex.dict(bool, len=4096) + child_execute_parent session[0]:execute_parent_fn slot=0 parent=vortex.binary(bool, len=4096) child=vortex.decimal_byte_parts.v2(decimal(15,2), len=4096) -> vortex.dict(bool, len=4096) iter 1 current=vortex.dict(bool, len=4096) builder_active=false ExecuteSlot slot=0 parent=vortex.dict(bool, len=4096) child=fastlanes.bitpacked(u8, len=4096) iter 2 current=fastlanes.bitpacked(u8, len=4096) stack_parent=vortex.dict(bool, len=4096) slot=0 builder_active=false @@ -418,8 +418,8 @@ fn trace_scan_filter_on_compressed_table() -> VortexResult<()> { optimize root=vortex.filter(i16, len=43) session=false reduce_parent static:FilterReduceAdaptor(Dict) slot=0 parent=vortex.filter(i16, len=43) child=vortex.dict(i16, len=4096) -> vortex.dict(i16, len=43) done output=vortex.dict(i16, len=43) - reduce_parent static:FilterReduceAdaptor(DecimalByteParts) slot=0 parent=vortex.filter(decimal(15,2), len=43) child=vortex.decimal_byte_parts_v2(decimal(15,2), len=4096) -> vortex.decimal_byte_parts_v2(decimal(15,2), len=43) - done output=vortex.decimal_byte_parts_v2(decimal(15,2), len=43) + reduce_parent static:FilterReduceAdaptor(DecimalByteParts) slot=0 parent=vortex.filter(decimal(15,2), len=43) child=vortex.decimal_byte_parts.v2(decimal(15,2), len=4096) -> vortex.decimal_byte_parts.v2(decimal(15,2), len=43) + done output=vortex.decimal_byte_parts.v2(decimal(15,2), len=43) optimize root=vortex.filter(vortex.date[days](i32), len=43) session=false optimize root=vortex.filter(i32, len=43) session=false reduce_parent static:FoRFilterPushDownRule slot=0 parent=vortex.filter(i32, len=43) child=fastlanes.for(i32, len=4096) -> fastlanes.for(i32, len=43) @@ -455,8 +455,8 @@ fn trace_scan_take_on_compressed_table() -> VortexResult<()> { insta::assert_snapshot!(optimized.trace.to_string(), @" optimize root=vortex.dict({l_quantity=decimal(15,2), l_shipdate=vortex.date[days](i32), l_shipmode=utf8}, len=64) session=false optimize root=vortex.dict(decimal(15,2), len=64) session=false - reduce_parent static:TakeReduceAdaptor(DecimalByteParts) slot=1 parent=vortex.dict(decimal(15,2), len=64) child=vortex.decimal_byte_parts_v2(decimal(15,2), len=4096) -> vortex.decimal_byte_parts_v2(decimal(15,2), len=64) - done output=vortex.decimal_byte_parts_v2(decimal(15,2), len=64) + reduce_parent static:TakeReduceAdaptor(DecimalByteParts) slot=1 parent=vortex.dict(decimal(15,2), len=64) child=vortex.decimal_byte_parts.v2(decimal(15,2), len=4096) -> vortex.decimal_byte_parts.v2(decimal(15,2), len=64) + done output=vortex.decimal_byte_parts.v2(decimal(15,2), len=64) optimize root=vortex.dict(vortex.date[days](i32), len=64) session=false reduce_parent static:TakeReduceAdaptor(Extension) slot=1 parent=vortex.dict(vortex.date[days](i32), len=64) child=vortex.ext(vortex.date[days](i32), len=4096) -> vortex.ext(vortex.date[days](i32), len=64) done output=vortex.ext(vortex.date[days](i32), len=64) diff --git a/vortex-btrblocks/tests/snapshots/golden__compact__decimal_prices.snap b/vortex-btrblocks/tests/snapshots/golden__compact__decimal_prices.snap index 53a7e765582..fad1b4d2b4e 100644 --- a/vortex-btrblocks/tests/snapshots/golden__compact__decimal_prices.snap +++ b/vortex-btrblocks/tests/snapshots/golden__compact__decimal_prices.snap @@ -3,7 +3,7 @@ source: vortex-btrblocks/tests/golden.rs expression: rendered --- input: decimal(12,2), len=16384, nbytes=131072 -root: vortex.decimal_byte_parts_v2(decimal(12,2), len=16384) nbytes=47666 +root: vortex.decimal_byte_parts.v2(decimal(12,2), len=16384) nbytes=47666 metadata: msp: vortex.pco(i32, len=16384) nbytes=47666 metadata: ptype: i32, nrows: 16384, slice: 0..16384 diff --git a/vortex-btrblocks/tests/snapshots/golden__default__decimal_prices.snap b/vortex-btrblocks/tests/snapshots/golden__default__decimal_prices.snap index 8b0568be671..6eb4ccfed8d 100644 --- a/vortex-btrblocks/tests/snapshots/golden__default__decimal_prices.snap +++ b/vortex-btrblocks/tests/snapshots/golden__default__decimal_prices.snap @@ -3,7 +3,7 @@ source: vortex-btrblocks/tests/golden.rs expression: rendered --- input: decimal(12,2), len=16384, nbytes=131072 -root: vortex.decimal_byte_parts_v2(decimal(12,2), len=16384) nbytes=49152 +root: vortex.decimal_byte_parts.v2(decimal(12,2), len=16384) nbytes=49152 metadata: msp: fastlanes.bitpacked(i32, len=16384) nbytes=49152 metadata: bit_width: 24, offset: 0 diff --git a/vortex-btrblocks/tests/snapshots/golden__unstable__decimal_prices.snap b/vortex-btrblocks/tests/snapshots/golden__unstable__decimal_prices.snap index 8b0568be671..6eb4ccfed8d 100644 --- a/vortex-btrblocks/tests/snapshots/golden__unstable__decimal_prices.snap +++ b/vortex-btrblocks/tests/snapshots/golden__unstable__decimal_prices.snap @@ -3,7 +3,7 @@ source: vortex-btrblocks/tests/golden.rs expression: rendered --- input: decimal(12,2), len=16384, nbytes=131072 -root: vortex.decimal_byte_parts_v2(decimal(12,2), len=16384) nbytes=49152 +root: vortex.decimal_byte_parts.v2(decimal(12,2), len=16384) nbytes=49152 metadata: msp: fastlanes.bitpacked(i32, len=16384) nbytes=49152 metadata: bit_width: 24, offset: 0 From caa90b2dec6fa95903c9f50e44634b89e40271e1 Mon Sep 17 00:00:00 2001 From: Matt Katz Date: Tue, 15 Sep 2026 16:39:29 -0400 Subject: [PATCH 11/17] Trim redundant DBP plugin tests Signed-off-by: Matt Katz --- .../src/decimal_byte_parts/plugin/tests.rs | 236 ++++++------------ 1 file changed, 73 insertions(+), 163 deletions(-) diff --git a/encodings/decimal-byte-parts/src/decimal_byte_parts/plugin/tests.rs b/encodings/decimal-byte-parts/src/decimal_byte_parts/plugin/tests.rs index 294213928c8..87240ab6fcf 100644 --- a/encodings/decimal-byte-parts/src/decimal_byte_parts/plugin/tests.rs +++ b/encodings/decimal-byte-parts/src/decimal_byte_parts/plugin/tests.rs @@ -37,114 +37,6 @@ use crate::decimal_byte_parts::testing::i256_parts; use crate::decimal_byte_parts::testing::wide_i128_values; use crate::decimal_byte_parts::testing::wide_i256_values; -#[rstest] -#[case::v1(0)] -#[case::v2_one_lower_part(1)] -#[case::v2_two_lower_parts(2)] -#[case::v2_three_lower_parts(3)] -fn serde_reuses_children(#[case] lower_part_count: usize) -> VortexResult<()> { - let array = DecimalByteParts::try_new_with_lower_parts( - msp(), - (0..lower_part_count).map(|_| lower_part()).collect(), - DecimalDType::new(76, 2), - )? - .into_array(); - assert_eq!(array.encoding_id(), decimal_byte_parts_v2_id()); - let session = session(); - let serialized = session - .array_serialize(&array)? - .vortex_expect("serializable"); - let original_children = array.children(); - assert_eq!(original_children.len(), serialized.children.len()); - for (original, written) in original_children.iter().zip(&serialized.children) { - assert!(ArrayRef::ptr_eq(original, written)); - } - let decoded = DecimalBytePartsPlugin.deserialize( - ArrayDeserialization::new( - serialized.serialized_id, - array.dtype(), - array.len(), - &serialized.metadata, - &[], - &serialized.children, - ), - &session, - )?; - assert_eq!(decoded.encoding_id(), decimal_byte_parts_v2_id()); - let decoded_children = decoded.children(); - assert_eq!(original_children.len(), decoded_children.len()); - for (original, decoded) in original_children.iter().zip(&decoded_children) { - assert!(ArrayRef::ptr_eq(original, decoded)); - } - Ok(()) -} - -#[rstest] -#[case::wide(msp(), lower_part())] -#[case::numerically_narrow(buffer![0i64; 3].into_array(), lower_part())] -fn v1_serializer_rejects_lower_parts( - #[case] msp: ArrayRef, - #[case] lower: ArrayRef, -) -> VortexResult<()> { - let array = - DecimalByteParts::try_new_with_lower_parts(msp, vec![lower], DecimalDType::new(38, 2))?; - assert!(v1::serialize(array.as_view()).is_err()); - Ok(()) -} - -#[test] -fn v2_round_trips_without_lower_parts() -> VortexResult<()> { - let child = msp(); - let array = DecimalByteParts::try_new(child.clone(), DecimalDType::new(19, 2))?; - let serialized = v2::serialize(array.as_view())?; - assert_eq!(serialized.serialized_id, decimal_byte_parts_v2_id()); - assert_eq!(serialized.children.len(), 1); - let decoded = v2::deserialize(ArrayDeserialization::new( - serialized.serialized_id, - array.dtype(), - array.len(), - &serialized.metadata, - &[], - &serialized.children, - ))?; - assert!(ArrayRef::ptr_eq(&child, decoded.msp())); - assert!(decoded.lower_parts().is_empty()); - Ok(()) -} - -#[test] -fn v1_metadata_is_unchanged() -> VortexResult<()> { - let session = session(); - let array = DecimalByteParts::try_new(msp(), DecimalDType::new(19, 2))?.into_array(); - let serialized = session - .array_serialize(&array)? - .vortex_expect("byte parts arrays are serializable"); - assert_eq!(serialized.serialized_id, decimal_byte_parts_v1_id()); - // v1 metadata for an i64 MSP: field 1 = 7, with no lower-part fields emitted. - assert_eq!(serialized.metadata, [8, 7]); - Ok(()) -} - -#[test] -fn v1_decoder_ignores_unknown_metadata_fields() -> VortexResult<()> { - let child = msp(); - let children = vec![child.clone()]; - let dtype = DType::Decimal(DecimalDType::new(19, 2), Nullability::NonNullable); - // An unknown trailing field. The v1 protobuf schema ignores it, as the original decoder - // did; the recognized lower-part count is still zero and there is still exactly one child. - let metadata = [8, 7, 26, 1, 0]; - let current = v1::deserialize(ArrayDeserialization::new( - decimal_byte_parts_v1_id(), - &dtype, - 3, - &metadata, - &[], - &children, - ))?; - assert!(ArrayRef::ptr_eq(&child, current.msp())); - Ok(()) -} - #[rstest] #[case::no_lower_parts(DecimalByteParts::try_new( buffer![1i32, 2, 3].into_array(), DecimalDType::new(9, 2), @@ -165,22 +57,12 @@ fn v1_decoder_ignores_unknown_metadata_fields() -> VortexResult<()> { buffer![i256::from_i128(-99), i256::ZERO, i256::from_i128(99)], DecimalDType::new(2, 0), Validity::NonNullable, )))] -#[case::redundant_two_lower_parts(DecimalByteParts::try_new_with_lower_parts( - buffer![0i64; 3].into_array(), - vec![buffer![0u64; 3].into_array(), lower_part()], - DecimalDType::new(38, 2), -))] -#[case::redundant_three_lower_parts(DecimalByteParts::try_new_with_lower_parts( +#[case::redundant_lower_parts(DecimalByteParts::try_new_with_lower_parts( buffer![0i64; 3].into_array(), vec![buffer![0u64; 3].into_array(), buffer![0u64; 3].into_array(), lower_part()], DecimalDType::new(38, 2), ))] -#[case::narrowed_one_lower_part(DecimalByteParts::try_new_with_lower_parts( - buffer![-1i8, 0, 1].into_array(), - vec![buffer![0u8, 128, u8::MAX].into_array()], - DecimalDType::new(38, 2), -))] -#[case::narrowed_three_lower_parts(DecimalByteParts::try_new_with_lower_parts( +#[case::narrowed_lower_parts(DecimalByteParts::try_new_with_lower_parts( buffer![-1i16, 0, 1].into_array(), vec![ buffer![u8::MAX, 128, 0].into_array(), @@ -252,66 +134,93 @@ fn serde_round_trip(#[case] array: VortexResult) -> Vorte Ok(()) } -/// The v1 decoder never accepts lower parts. The v2 decoder accepts any count the array allows, -/// including none, even though the plugin only writes v2 when lower parts are present. +#[test] +fn v1_metadata_is_unchanged() -> VortexResult<()> { + let session = session(); + let array = DecimalByteParts::try_new(msp(), DecimalDType::new(19, 2))?.into_array(); + let serialized = session + .array_serialize(&array)? + .vortex_expect("byte parts arrays are serializable"); + assert_eq!(serialized.serialized_id, decimal_byte_parts_v1_id()); + // v1 metadata for an i64 MSP: field 1 = 7, with no lower-part fields emitted. + assert_eq!(serialized.metadata, [8, 7]); + Ok(()) +} + +/// The plugin only writes v2 when lower parts are present, but the format itself does not +/// require them. +#[test] +fn v2_round_trips_without_lower_parts() -> VortexResult<()> { + let child = msp(); + let array = DecimalByteParts::try_new(child.clone(), DecimalDType::new(38, 2))?; + let serialized = v2::serialize(array.as_view())?; + assert_eq!(serialized.serialized_id, decimal_byte_parts_v2_id()); + assert_eq!(serialized.children.len(), 1); + let decoded = deserialize_with( + serialized.serialized_id, + &serialized.metadata, + serialized.children, + )?; + let decoded = decoded + .as_opt::() + .vortex_expect("byte parts array"); + assert!(ArrayRef::ptr_eq(&child, decoded.msp())); + assert!(decoded.lower_parts().is_empty()); + Ok(()) +} + +/// The v1 decoder never accepts lower parts, and the v2 decoder holds its children to its +/// metadata. #[rstest] -#[case::v1_without_lower_parts(decimal_byte_parts_v1_id(), v1_metadata(0), vec![msp()], true)] -#[case::v1_with_lower_parts( +#[case::v1_lower_part_count( decimal_byte_parts_v1_id(), v1_metadata(1), vec![msp(), lower_part()], - false + "must not carry lower parts" )] -#[case::v2_with_lower_parts( +#[case::v1_extra_child( + decimal_byte_parts_v1_id(), + v1_metadata(0), + vec![msp(), lower_part()], + "exactly one child" +)] +#[case::v2_missing_child( decimal_byte_parts_v2_id(), v2_metadata(vec![PType::U64 as i32]), - vec![msp(), lower_part()], - true + vec![msp()], + "expected 2 children, got 1" )] -#[case::v2_without_lower_parts(decimal_byte_parts_v2_id(), v2_metadata(vec![]), vec![msp()], true)] -fn plugin_holds_each_id_to_its_contract( - #[case] serialized_id: ArrayId, - #[case] metadata: Vec, - #[case] children: Vec, - #[case] accepted: bool, -) { - let result = deserialize_with(serialized_id, &metadata, children); - assert_eq!(result.is_ok(), accepted, "{serialized_id}: {result:?}"); -} - -#[rstest] -#[case::v1_extra_child(decimal_byte_parts_v1_id(), v1_metadata(0), vec![msp(), lower_part()])] -#[case::v2_missing_child(decimal_byte_parts_v2_id(), v2_metadata(vec![PType::U64 as i32]), vec![msp()])] #[case::v2_extra_child( decimal_byte_parts_v2_id(), v2_metadata(vec![PType::U64 as i32]), - vec![msp(), lower_part(), lower_part()] + vec![msp(), lower_part(), lower_part()], + "expected 2 children, got 3" +)] +#[case::v2_too_many_lower_parts( + decimal_byte_parts_v2_id(), + v2_metadata(vec![PType::U64 as i32; MAX_LOWER_PARTS + 1]), + vec![msp(), lower_part(), lower_part(), lower_part(), lower_part()], + "lower parts, got 4" )] -fn deserialize_rejects_child_count_mismatch( +#[case::v2_signed_lower_part( + decimal_byte_parts_v2_id(), + v2_metadata(vec![PType::I64 as i32]), + vec![msp(), lower_part()], + "unsigned integer dtype" +)] +#[case::v2_unknown_ptype( + decimal_byte_parts_v2_id(), + v2_metadata(vec![i32::MAX]), + vec![msp(), lower_part()], + "invalid PType" +)] +fn decoder_rejects_malformed_payloads( #[case] serialized_id: ArrayId, #[case] metadata: Vec, #[case] children: Vec, -) { - let result = deserialize_with(serialized_id, &metadata, children); - assert!(result.is_err(), "{serialized_id}: {result:?}"); -} - -#[rstest] -#[case::too_many(vec![PType::U64 as i32; MAX_LOWER_PARTS + 1], "lower parts, got 4")] -#[case::signed_type(vec![PType::I64 as i32], "unsigned integer dtype")] -#[case::float_type(vec![PType::F64 as i32], "unsigned integer dtype")] -#[case::unknown_type(vec![i32::MAX], "invalid PType")] -fn v2_decoder_rejects_invalid_lower_part_ptypes( - #[case] lower_part_ptypes: Vec, #[case] expected_error: &str, ) { - let mut children = vec![msp()]; - children.extend((0..lower_part_ptypes.len()).map(|_| lower_part())); - let result = deserialize_with( - decimal_byte_parts_v2_id(), - &v2_metadata(lower_part_ptypes), - children, - ); + let result = deserialize_with(serialized_id, &metadata, children); assert!( result .as_ref() @@ -428,6 +337,7 @@ fn session() -> VortexSession { session } +/// Decode a hand-built payload of three rows through the plugin. fn deserialize_with( serialized_id: ArrayId, metadata: &[u8], From 290f6e39b490b4ef59c024c89ce74125dfdd9acb Mon Sep 17 00:00:00 2001 From: Matt Katz Date: Tue, 15 Sep 2026 17:01:43 -0400 Subject: [PATCH 12/17] Drop the curated wide decimal test fixtures Signed-off-by: Matt Katz --- .../src/decimal_byte_parts/array.rs | 39 ------------------ .../src/decimal_byte_parts/plugin/tests.rs | 37 ++--------------- .../src/decimal_byte_parts/testing.rs | 41 ------------------- 3 files changed, 3 insertions(+), 114 deletions(-) diff --git a/encodings/decimal-byte-parts/src/decimal_byte_parts/array.rs b/encodings/decimal-byte-parts/src/decimal_byte_parts/array.rs index 61b6dfb9f1c..c646765daf7 100644 --- a/encodings/decimal-byte-parts/src/decimal_byte_parts/array.rs +++ b/encodings/decimal-byte-parts/src/decimal_byte_parts/array.rs @@ -431,7 +431,6 @@ mod tests { use vortex_error::VortexResult; use super::DecimalByteParts; - use super::DecimalBytePartsArray; use super::DecimalBytePartsArraySlotsExt; use super::DecimalBytePartsData; use crate::decimal_byte_parts::LOWER_PART_DTYPE; @@ -439,8 +438,6 @@ mod tests { use crate::decimal_byte_parts::testing::i128_parts; use crate::decimal_byte_parts::testing::i256_of; use crate::decimal_byte_parts::testing::i256_parts; - use crate::decimal_byte_parts::testing::wide_i128_values; - use crate::decimal_byte_parts::testing::wide_i256_values; #[test] fn test_scalar_at_decimal_parts() { @@ -481,21 +478,6 @@ mod tests { ); } - #[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); @@ -531,27 +513,6 @@ mod tests { 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(()) - } - #[rstest] fn test_scalar_at_matches_canonical_for_each_part_count( #[values(false, true)] narrow_msp: bool, diff --git a/encodings/decimal-byte-parts/src/decimal_byte_parts/plugin/tests.rs b/encodings/decimal-byte-parts/src/decimal_byte_parts/plugin/tests.rs index 87240ab6fcf..5ff68ddba92 100644 --- a/encodings/decimal-byte-parts/src/decimal_byte_parts/plugin/tests.rs +++ b/encodings/decimal-byte-parts/src/decimal_byte_parts/plugin/tests.rs @@ -32,21 +32,14 @@ use crate::DecimalBytePartsArray; use crate::DecimalBytePartsArraySlotsExt; use crate::decimal_byte_parts::MAX_LOWER_PARTS; use crate::decimal_byte_parts::testing::encode; -use crate::decimal_byte_parts::testing::i128_parts; -use crate::decimal_byte_parts::testing::i256_parts; -use crate::decimal_byte_parts::testing::wide_i128_values; -use crate::decimal_byte_parts::testing::wide_i256_values; #[rstest] #[case::no_lower_parts(DecimalByteParts::try_new( buffer![1i32, 2, 3].into_array(), DecimalDType::new(9, 2), ))] -#[case::one_lower_part(Ok(i128_parts(wide_i128_values(), Validity::NonNullable)))] -#[case::three_lower_parts(Ok(i256_parts(wide_i256_values(), Validity::NonNullable)))] -#[case::nullable_three_lower_parts(Ok(i256_parts( - wide_i256_values(), - Validity::from_iter([true, false, true, true, true, false, true, true, true, true]), -)))] +#[case::one_lower_part(DecimalByteParts::try_new_with_lower_parts( + msp(), vec![lower_part()], DecimalDType::new(38, 2), +))] #[case::wider_i64_storage(encode(&DecimalArray::new( buffer![-99i64, 0, 99], DecimalDType::new(2, 0), Validity::NonNullable, )))] @@ -147,30 +140,6 @@ fn v1_metadata_is_unchanged() -> VortexResult<()> { Ok(()) } -/// The plugin only writes v2 when lower parts are present, but the format itself does not -/// require them. -#[test] -fn v2_round_trips_without_lower_parts() -> VortexResult<()> { - let child = msp(); - let array = DecimalByteParts::try_new(child.clone(), DecimalDType::new(38, 2))?; - let serialized = v2::serialize(array.as_view())?; - assert_eq!(serialized.serialized_id, decimal_byte_parts_v2_id()); - assert_eq!(serialized.children.len(), 1); - let decoded = deserialize_with( - serialized.serialized_id, - &serialized.metadata, - serialized.children, - )?; - let decoded = decoded - .as_opt::() - .vortex_expect("byte parts array"); - assert!(ArrayRef::ptr_eq(&child, decoded.msp())); - assert!(decoded.lower_parts().is_empty()); - Ok(()) -} - -/// The v1 decoder never accepts lower parts, and the v2 decoder holds its children to its -/// metadata. #[rstest] #[case::v1_lower_part_count( decimal_byte_parts_v1_id(), 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 537dde93746..2dfe2a55b3c 100644 --- a/encodings/decimal-byte-parts/src/decimal_byte_parts/testing.rs +++ b/encodings/decimal-byte-parts/src/decimal_byte_parts/testing.rs @@ -45,44 +45,3 @@ pub(crate) fn i256_parts(values: Vec, validity: Validity) -> DecimalBytePa pub(super) fn i256_of(high: i128, low: u128) -> i256 { i256::from_parts(low, high) } - -/// 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. -pub(crate) 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`. -pub(crate) 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(), - ] -} From ffbfd784b73d8b84dd05fa0eaa6a015af55df63d Mon Sep 17 00:00:00 2001 From: Matt Katz Date: Tue, 15 Sep 2026 17:03:15 -0400 Subject: [PATCH 13/17] Drop the redundant serialized ID check from the v2 decoder Signed-off-by: Matt Katz --- .../decimal-byte-parts/src/decimal_byte_parts/plugin/v2.rs | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/encodings/decimal-byte-parts/src/decimal_byte_parts/plugin/v2.rs b/encodings/decimal-byte-parts/src/decimal_byte_parts/plugin/v2.rs index 412048b3701..c82db1cbce3 100644 --- a/encodings/decimal-byte-parts/src/decimal_byte_parts/plugin/v2.rs +++ b/encodings/decimal-byte-parts/src/decimal_byte_parts/plugin/v2.rs @@ -48,9 +48,11 @@ pub(super) fn serialize( .collect::>()?, } .encode_to_vec(); + let mut children = Vec::with_capacity(1 + lower_parts.len()); children.push(array.msp().clone()); children.extend(lower_parts.iter().cloned()); + Ok(ArraySerialization::new( decimal_byte_parts_v2_id(), metadata, @@ -60,10 +62,6 @@ pub(super) fn serialize( } pub(super) fn deserialize(parts: ArrayDeserialization<'_>) -> VortexResult { - vortex_ensure!( - parts.serialized_id == decimal_byte_parts_v2_id(), - "expected the v2 format" - ); let metadata = DecimalBytePartsV2Metadata::decode(parts.metadata)?; vortex_ensure!( parts.dtype.as_decimal_opt().is_some(), From ec4ee2bab4a31fc49fa3430cb11cec155dbede7f Mon Sep 17 00:00:00 2001 From: Matt Katz Date: Tue, 15 Sep 2026 17:05:19 -0400 Subject: [PATCH 14/17] Inline i256::from_parts at the decimal byte-parts test sites Signed-off-by: Matt Katz --- .../src/decimal_byte_parts/array.rs | 10 ++++++---- .../src/decimal_byte_parts/compute/filter.rs | 12 ++++++------ .../src/decimal_byte_parts/compute/mod.rs | 11 +++++------ .../src/decimal_byte_parts/compute/take.rs | 4 ++-- .../src/decimal_byte_parts/testing.rs | 5 ----- 5 files changed, 19 insertions(+), 23 deletions(-) diff --git a/encodings/decimal-byte-parts/src/decimal_byte_parts/array.rs b/encodings/decimal-byte-parts/src/decimal_byte_parts/array.rs index c646765daf7..eef4d8dca02 100644 --- a/encodings/decimal-byte-parts/src/decimal_byte_parts/array.rs +++ b/encodings/decimal-byte-parts/src/decimal_byte_parts/array.rs @@ -436,7 +436,6 @@ mod tests { use crate::decimal_byte_parts::LOWER_PART_DTYPE; use crate::decimal_byte_parts::MAX_LOWER_PARTS; 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] @@ -498,7 +497,7 @@ mod tests { #[test] fn test_lower_part_layout_i256() -> VortexResult<()> { let array = i256_parts( - vec![i256_of((5i128 << 64) | 6, (7u128 << 64) | 8)], + vec![i256::from_parts((7u128 << 64) | 8, (5i128 << 64) | 6)], Validity::NonNullable, ); assert_eq!(array.lower_parts().len(), MAX_LOWER_PARTS); @@ -698,7 +697,7 @@ mod tests { 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 i256_array = i256_parts(vec![i256::from_parts(0, 1 << 100)], Validity::NonNullable); let canonical = i256_array.into_array().execute::(&mut ctx)?; assert_eq!(canonical.values_type(), DecimalType::I256); @@ -723,7 +722,10 @@ mod tests { )?; 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)]); + assert_eq!( + canonical.buffer::().as_slice(), + &[i256::from_parts(9, 1)] + ); Ok(()) } 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 49c4021dd18..6921e5dfda4 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 @@ -27,12 +27,12 @@ mod test { use vortex_array::arrays::PrimitiveArray; use vortex_array::compute::conformance::filter::test_filter_conformance; use vortex_array::dtype::DecimalDType; + use vortex_array::dtype::i256; 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] @@ -72,11 +72,11 @@ mod test { 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), + i256::from_parts(0, 1), + i256::from_parts(5, -1), + i256::from_parts(u128::MAX, 0), + i256::from_parts(7, 1 << 64), + i256::from_parts(0, 0), ], Validity::from_iter([true, false, true, true, false]), ); 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 f9848e1b2e7..c8385c2d6d6 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 @@ -27,7 +27,6 @@ mod tests { 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. @@ -43,11 +42,11 @@ mod tests { 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), + i256::from_parts(0, 1), + i256::from_parts(0, -1), + i256::from_parts(u128::MAX, 0), + i256::from_parts(7, 1 << 64), + i256::from_parts(0, 0), ] } 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 5b07af47252..d175845d7c5 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 @@ -38,6 +38,7 @@ mod tests { use vortex_array::arrays::PrimitiveArray; use vortex_array::assert_arrays_eq; use vortex_array::dtype::DecimalDType; + use vortex_array::dtype::i256; use vortex_array::validity::Validity; use vortex_buffer::Buffer; use vortex_buffer::buffer; @@ -45,7 +46,6 @@ mod tests { 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 @@ -80,7 +80,7 @@ mod tests { Validity::NonNullable, ))] #[case::three_lower_parts(DecimalArray::new( - Buffer::from(vec![i256_of(1, 1 << 70), i256_of(0, 2), i256_of(0, 3)]), + Buffer::from(vec![i256::from_parts(1 << 70, 1), i256::from_parts(2, 0), i256::from_parts(3, 0)]), DecimalDType::new(76, 2), Validity::NonNullable, ))] 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 2dfe2a55b3c..0e5dec71696 100644 --- a/encodings/decimal-byte-parts/src/decimal_byte_parts/testing.rs +++ b/encodings/decimal-byte-parts/src/decimal_byte_parts/testing.rs @@ -40,8 +40,3 @@ pub(crate) fn i256_parts(values: Vec, validity: Validity) -> DecimalBytePa )) .vortex_expect("valid decimal byte parts") } - -/// Build an `i256` from a signed high `i128` and unsigned low `u128`. -pub(super) fn i256_of(high: i128, low: u128) -> i256 { - i256::from_parts(low, high) -} From 54f746a8216ba0e3cc5145fa00d797adf64d6ea7 Mon Sep 17 00:00:00 2001 From: Matt Katz Date: Tue, 15 Sep 2026 17:07:22 -0400 Subject: [PATCH 15/17] Call dbp_encode directly in decimal byte-parts tests Signed-off-by: Matt Katz --- .../src/decimal_byte_parts/compute/take.rs | 8 +++--- .../src/decimal_byte_parts/plugin/tests.rs | 27 +++++++++++-------- .../src/decimal_byte_parts/prop_tests.rs | 14 ++++------ .../src/decimal_byte_parts/testing.rs | 24 ++++++----------- 4 files changed, 34 insertions(+), 39 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 d175845d7c5..a69c7c80e5a 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 @@ -45,7 +45,7 @@ mod tests { use vortex_error::VortexResult; use crate::DecimalByteParts; - use crate::decimal_byte_parts::testing::encode; + use crate::dbp_encode; /// 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 @@ -61,7 +61,9 @@ mod tests { Validity::NonNullable, ); let indices = buffer![0u64, 2].into_array(); - let taken = encode(&decimal)?.into_array().take(indices)?; + let taken = dbp_encode(&decimal, &mut session.create_execution_ctx())? + .into_array() + .take(indices)?; assert!( taken.is::(), @@ -96,7 +98,7 @@ mod tests { .take(indices.clone())? .execute::(&mut ctx)?; - let taken = encode(&decimal)?.into_array().take(indices)?; + let taken = dbp_encode(&decimal, &mut ctx)?.into_array().take(indices)?; let actual = taken.execute::(&mut ctx)?; assert_arrays_eq!(expected, actual, &mut ctx); diff --git a/encodings/decimal-byte-parts/src/decimal_byte_parts/plugin/tests.rs b/encodings/decimal-byte-parts/src/decimal_byte_parts/plugin/tests.rs index 5ff68ddba92..c4ac5cd82ee 100644 --- a/encodings/decimal-byte-parts/src/decimal_byte_parts/plugin/tests.rs +++ b/encodings/decimal-byte-parts/src/decimal_byte_parts/plugin/tests.rs @@ -30,8 +30,8 @@ use vortex_session::registry::ReadContext; use super::*; use crate::DecimalBytePartsArray; use crate::DecimalBytePartsArraySlotsExt; +use crate::dbp_encode; use crate::decimal_byte_parts::MAX_LOWER_PARTS; -use crate::decimal_byte_parts::testing::encode; #[rstest] #[case::no_lower_parts(DecimalByteParts::try_new( @@ -40,16 +40,21 @@ use crate::decimal_byte_parts::testing::encode; #[case::one_lower_part(DecimalByteParts::try_new_with_lower_parts( msp(), vec![lower_part()], DecimalDType::new(38, 2), ))] -#[case::wider_i64_storage(encode(&DecimalArray::new( - buffer![-99i64, 0, 99], DecimalDType::new(2, 0), Validity::NonNullable, -)))] -#[case::wider_i128_storage(encode(&DecimalArray::new( - buffer![-99i128, 0, 99], DecimalDType::new(2, 0), Validity::NonNullable, -)))] -#[case::wider_i256_storage(encode(&DecimalArray::new( - buffer![i256::from_i128(-99), i256::ZERO, i256::from_i128(99)], - DecimalDType::new(2, 0), Validity::NonNullable, -)))] +#[case::wider_i64_storage(dbp_encode( + &DecimalArray::new(buffer![-99i64, 0, 99], DecimalDType::new(2, 0), Validity::NonNullable), + &mut array_session().create_execution_ctx(), +))] +#[case::wider_i128_storage(dbp_encode( + &DecimalArray::new(buffer![-99i128, 0, 99], DecimalDType::new(2, 0), Validity::NonNullable), + &mut array_session().create_execution_ctx(), +))] +#[case::wider_i256_storage(dbp_encode( + &DecimalArray::new( + buffer![i256::from_i128(-99), i256::ZERO, i256::from_i128(99)], + DecimalDType::new(2, 0), Validity::NonNullable, + ), + &mut array_session().create_execution_ctx(), +))] #[case::redundant_lower_parts(DecimalByteParts::try_new_with_lower_parts( buffer![0i64; 3].into_array(), vec![buffer![0u64; 3].into_array(), buffer![0u64; 3].into_array(), lower_part()], diff --git a/encodings/decimal-byte-parts/src/decimal_byte_parts/prop_tests.rs b/encodings/decimal-byte-parts/src/decimal_byte_parts/prop_tests.rs index 5630d1706a8..046088d1398 100644 --- a/encodings/decimal-byte-parts/src/decimal_byte_parts/prop_tests.rs +++ b/encodings/decimal-byte-parts/src/decimal_byte_parts/prop_tests.rs @@ -21,7 +21,7 @@ use vortex_error::VortexExpect; use super::DecimalByteParts; use super::DecimalBytePartsArray; -use super::testing::encode; +use super::dbp_encode; /// Largest magnitude a `Decimal(38, _)` can hold: 38 nines. const MAX_I128: i128 = 10i128.pow(38) - 1; @@ -139,10 +139,8 @@ fn decoded_survives_encode_then_decode(tc: TestCase) { let decimal = draw_decimal(&tc); let mut ctx = ctx(); - let round_tripped = canonicalize( - encode(&decimal).vortex_expect("encode").into_array(), - &mut ctx, - ); + let encoded = dbp_encode(&decimal, &mut ctx).vortex_expect("encode"); + let round_tripped = canonicalize(encoded.into_array(), &mut ctx); assert_eq!(round_tripped.values_type(), decimal.values_type()); assert_arrays_eq!(decimal, round_tripped, &mut ctx); @@ -160,10 +158,8 @@ fn encoded_survives_decode_then_encode(tc: TestCase) { let mut ctx = ctx(); let decoded = canonicalize(array.into_array(), &mut ctx); - let re_decoded = canonicalize( - encode(&decoded).vortex_expect("encode").into_array(), - &mut ctx, - ); + let re_encoded = dbp_encode(&decoded, &mut ctx).vortex_expect("encode"); + let re_decoded = canonicalize(re_encoded.into_array(), &mut ctx); assert_arrays_eq!(decoded, re_decoded, &mut ctx); } 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 0e5dec71696..920f856b48e 100644 --- a/encodings/decimal-byte-parts/src/decimal_byte_parts/testing.rs +++ b/encodings/decimal-byte-parts/src/decimal_byte_parts/testing.rs @@ -11,32 +11,24 @@ use vortex_array::dtype::i256; use vortex_array::validity::Validity; use vortex_buffer::Buffer; use vortex_error::VortexExpect; -use vortex_error::VortexResult; use super::DecimalBytePartsArray; use super::dbp_encode; -/// Encode a canonical decimal array as byte parts, splitting wide values into lower parts. -pub(crate) fn encode(decimal: &DecimalArray) -> VortexResult { - dbp_encode(decimal, &mut array_session().create_execution_ctx()) -} - /// 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, - )) + dbp_encode( + &DecimalArray::new(Buffer::from(values), DecimalDType::new(38, 2), validity), + &mut array_session().create_execution_ctx(), + ) .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, - )) + dbp_encode( + &DecimalArray::new(Buffer::from(values), DecimalDType::new(76, 2), validity), + &mut array_session().create_execution_ctx(), + ) .vortex_expect("valid decimal byte parts") } From b7357cad5eb0c4ee8c1bc7c023b206c3647d2df8 Mon Sep 17 00:00:00 2001 From: Matt Katz Date: Tue, 15 Sep 2026 17:13:47 -0400 Subject: [PATCH 16/17] Replace dbp_encode with DecimalByteParts::encode Signed-off-by: Matt Katz --- .../src/decimal_byte_parts/compute/take.rs | 7 ++-- .../src/decimal_byte_parts/mod.rs | 1 - .../src/decimal_byte_parts/plugin/tests.rs | 7 ++-- .../src/decimal_byte_parts/prop_tests.rs | 5 ++- .../src/decimal_byte_parts/split.rs | 33 ++++++++++--------- .../src/decimal_byte_parts/testing.rs | 6 ++-- 6 files changed, 29 insertions(+), 30 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 a69c7c80e5a..a74915e9f22 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 @@ -45,7 +45,6 @@ mod tests { use vortex_error::VortexResult; use crate::DecimalByteParts; - use crate::dbp_encode; /// 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 @@ -61,7 +60,7 @@ mod tests { Validity::NonNullable, ); let indices = buffer![0u64, 2].into_array(); - let taken = dbp_encode(&decimal, &mut session.create_execution_ctx())? + let taken = DecimalByteParts::encode(&decimal, &mut session.create_execution_ctx())? .into_array() .take(indices)?; @@ -98,7 +97,9 @@ mod tests { .take(indices.clone())? .execute::(&mut ctx)?; - let taken = dbp_encode(&decimal, &mut ctx)?.into_array().take(indices)?; + let taken = DecimalByteParts::encode(&decimal, &mut ctx)? + .into_array() + .take(indices)?; let actual = taken.execute::(&mut ctx)?; assert_arrays_eq!(expected, actual, &mut ctx); 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 b989fe2bbe0..65bb8222f7a 100644 --- a/encodings/decimal-byte-parts/src/decimal_byte_parts/mod.rs +++ b/encodings/decimal-byte-parts/src/decimal_byte_parts/mod.rs @@ -36,7 +36,6 @@ pub use plugin::DecimalBytePartsV2Metadata; pub use plugin::decimal_byte_parts_v1_id; pub use plugin::decimal_byte_parts_v2_id; pub use split::DecimalParts; -pub use split::dbp_encode; pub use split::split_decimal; #[doc(hidden)] diff --git a/encodings/decimal-byte-parts/src/decimal_byte_parts/plugin/tests.rs b/encodings/decimal-byte-parts/src/decimal_byte_parts/plugin/tests.rs index c4ac5cd82ee..92c9d1edbb7 100644 --- a/encodings/decimal-byte-parts/src/decimal_byte_parts/plugin/tests.rs +++ b/encodings/decimal-byte-parts/src/decimal_byte_parts/plugin/tests.rs @@ -30,7 +30,6 @@ use vortex_session::registry::ReadContext; use super::*; use crate::DecimalBytePartsArray; use crate::DecimalBytePartsArraySlotsExt; -use crate::dbp_encode; use crate::decimal_byte_parts::MAX_LOWER_PARTS; #[rstest] @@ -40,15 +39,15 @@ use crate::decimal_byte_parts::MAX_LOWER_PARTS; #[case::one_lower_part(DecimalByteParts::try_new_with_lower_parts( msp(), vec![lower_part()], DecimalDType::new(38, 2), ))] -#[case::wider_i64_storage(dbp_encode( +#[case::wider_i64_storage(DecimalByteParts::encode( &DecimalArray::new(buffer![-99i64, 0, 99], DecimalDType::new(2, 0), Validity::NonNullable), &mut array_session().create_execution_ctx(), ))] -#[case::wider_i128_storage(dbp_encode( +#[case::wider_i128_storage(DecimalByteParts::encode( &DecimalArray::new(buffer![-99i128, 0, 99], DecimalDType::new(2, 0), Validity::NonNullable), &mut array_session().create_execution_ctx(), ))] -#[case::wider_i256_storage(dbp_encode( +#[case::wider_i256_storage(DecimalByteParts::encode( &DecimalArray::new( buffer![i256::from_i128(-99), i256::ZERO, i256::from_i128(99)], DecimalDType::new(2, 0), Validity::NonNullable, diff --git a/encodings/decimal-byte-parts/src/decimal_byte_parts/prop_tests.rs b/encodings/decimal-byte-parts/src/decimal_byte_parts/prop_tests.rs index 046088d1398..b1625485ee7 100644 --- a/encodings/decimal-byte-parts/src/decimal_byte_parts/prop_tests.rs +++ b/encodings/decimal-byte-parts/src/decimal_byte_parts/prop_tests.rs @@ -21,7 +21,6 @@ use vortex_error::VortexExpect; use super::DecimalByteParts; use super::DecimalBytePartsArray; -use super::dbp_encode; /// Largest magnitude a `Decimal(38, _)` can hold: 38 nines. const MAX_I128: i128 = 10i128.pow(38) - 1; @@ -139,7 +138,7 @@ fn decoded_survives_encode_then_decode(tc: TestCase) { let decimal = draw_decimal(&tc); let mut ctx = ctx(); - let encoded = dbp_encode(&decimal, &mut ctx).vortex_expect("encode"); + let encoded = DecimalByteParts::encode(&decimal, &mut ctx).vortex_expect("encode"); let round_tripped = canonicalize(encoded.into_array(), &mut ctx); assert_eq!(round_tripped.values_type(), decimal.values_type()); @@ -158,7 +157,7 @@ fn encoded_survives_decode_then_encode(tc: TestCase) { let mut ctx = ctx(); let decoded = canonicalize(array.into_array(), &mut ctx); - let re_encoded = dbp_encode(&decoded, &mut ctx).vortex_expect("encode"); + let re_encoded = DecimalByteParts::encode(&decoded, &mut ctx).vortex_expect("encode"); let re_decoded = canonicalize(re_encoded.into_array(), &mut ctx); assert_arrays_eq!(decoded, re_decoded, &mut ctx); diff --git a/encodings/decimal-byte-parts/src/decimal_byte_parts/split.rs b/encodings/decimal-byte-parts/src/decimal_byte_parts/split.rs index a3aa1b4afdc..8d8818924e9 100644 --- a/encodings/decimal-byte-parts/src/decimal_byte_parts/split.rs +++ b/encodings/decimal-byte-parts/src/decimal_byte_parts/split.rs @@ -27,22 +27,23 @@ use super::LOWER_PART_BITS; use super::MAX_I128_LOWER_PARTS; use super::MAX_I256_LOWER_PARTS; -/// Create a [`DecimalBytePartsArray`] from a [`DecimalArray`] by splitting it into parts. -/// -/// # Errors -/// -/// Returns an error if the decimal cannot be split. -pub fn dbp_encode( - decimal: &DecimalArray, - exec_ctx: &mut ExecutionCtx, -) -> VortexResult { - let parts = split_decimal(decimal, exec_ctx)?; - // SAFETY: splitting produces a signed MSP and zero, one, or three non-nullable u64 lower - // parts, all with the decimal's length and in most-significant-first order. This also holds - // for the constant parts used for empty and all-null inputs. The decimal dtype is preserved. - Ok(unsafe { - DecimalByteParts::new_unchecked(parts.msp, parts.lower_parts, decimal.decimal_dtype()) - }) +impl DecimalByteParts { + /// Encode a [`DecimalArray`] as byte parts, splitting wide values into lower parts. + /// + /// # Errors + /// + /// Returns an error if the decimal cannot be split. + pub fn encode( + decimal: &DecimalArray, + exec_ctx: &mut ExecutionCtx, + ) -> VortexResult { + let parts = split_decimal(decimal, exec_ctx)?; + // SAFETY: splitting produces a signed MSP and zero, one, or three non-nullable u64 lower + // parts, all with the decimal's length and in most-significant-first order. This also + // holds for the constant parts used for empty and all-null inputs. The decimal dtype is + // preserved. + Ok(unsafe { Self::new_unchecked(parts.msp, parts.lower_parts, decimal.decimal_dtype()) }) + } } /// A decimal array decomposed into byte parts. 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 920f856b48e..af270bad407 100644 --- a/encodings/decimal-byte-parts/src/decimal_byte_parts/testing.rs +++ b/encodings/decimal-byte-parts/src/decimal_byte_parts/testing.rs @@ -12,12 +12,12 @@ use vortex_array::validity::Validity; use vortex_buffer::Buffer; use vortex_error::VortexExpect; +use super::DecimalByteParts; use super::DecimalBytePartsArray; -use super::dbp_encode; /// An `i128`-backed decimal array, encoded as byte parts with one lower part. pub(crate) fn i128_parts(values: Vec, validity: Validity) -> DecimalBytePartsArray { - dbp_encode( + DecimalByteParts::encode( &DecimalArray::new(Buffer::from(values), DecimalDType::new(38, 2), validity), &mut array_session().create_execution_ctx(), ) @@ -26,7 +26,7 @@ pub(crate) fn i128_parts(values: Vec, validity: Validity) -> DecimalBytePa /// An `i256`-backed decimal array, encoded as byte parts with three lower parts. pub(crate) fn i256_parts(values: Vec, validity: Validity) -> DecimalBytePartsArray { - dbp_encode( + DecimalByteParts::encode( &DecimalArray::new(Buffer::from(values), DecimalDType::new(76, 2), validity), &mut array_session().create_execution_ctx(), ) From 8ba61ccbaf6531392b0d40c8ee0a4fe767f6324d Mon Sep 17 00:00:00 2001 From: Matt Katz Date: Tue, 15 Sep 2026 17:28:19 -0400 Subject: [PATCH 17/17] comments fix Signed-off-by: Matt Katz --- .../src/decimal_byte_parts/array.rs | 10 +--------- .../src/decimal_byte_parts/plugin/mod.rs | 11 +++++------ 2 files changed, 6 insertions(+), 15 deletions(-) diff --git a/encodings/decimal-byte-parts/src/decimal_byte_parts/array.rs b/encodings/decimal-byte-parts/src/decimal_byte_parts/array.rs index eef4d8dca02..c98b852d716 100644 --- a/encodings/decimal-byte-parts/src/decimal_byte_parts/array.rs +++ b/encodings/decimal-byte-parts/src/decimal_byte_parts/array.rs @@ -124,11 +124,6 @@ impl DecimalBytePartsData { } } -/// The current in-memory decimal byte-parts encoding, identified as v2. -/// -/// Register [`super::DecimalBytePartsPlugin`] or call [`crate::initialize`] to read and write -/// either serialized format. Registering this VTable directly, or calling its serde methods, -/// returns an error when serializing or deserializing, including for the frozen v1 format. #[derive(Clone, Debug)] pub struct DecimalByteParts; @@ -150,7 +145,7 @@ impl DecimalByteParts { /// /// Lower parts are ordered most significant first and must each be a non-nullable unsigned integer /// array of the same length as the MSP. See [`super::split_decimal`] for producing them from a - /// canonical decimal array. + /// decimal array. /// /// # Errors /// @@ -161,9 +156,6 @@ impl DecimalByteParts { lower_parts: Vec, decimal_dtype: DecimalDType, ) -> VortexResult { - // Building lower parts in memory is never gated — reading a file requires it. What is - // gated is the serialized form: an array carrying lower parts serializes under the - // `vortex.decimal_byte_parts.v2` format ID, which only editions that contain it may write. let len = msp.len(); let dtype = DType::Decimal(decimal_dtype, msp.dtype().nullability()); let slots = DecimalBytePartsSlots { msp, lower_parts }.into_slots(); diff --git a/encodings/decimal-byte-parts/src/decimal_byte_parts/plugin/mod.rs b/encodings/decimal-byte-parts/src/decimal_byte_parts/plugin/mod.rs index 3b7157e1160..3896cd51c96 100644 --- a/encodings/decimal-byte-parts/src/decimal_byte_parts/plugin/mod.rs +++ b/encodings/decimal-byte-parts/src/decimal_byte_parts/plugin/mod.rs @@ -1,7 +1,7 @@ // SPDX-License-Identifier: Apache-2.0 // SPDX-FileCopyrightText: Copyright the Vortex contributors -//! Select a DBP wire format and dispatch to its serde implementation. +//! ArrayPlugin implementation for DBP that handles different wire formats. use vortex_array::ArrayDeserialization; use vortex_array::ArrayId; @@ -27,24 +27,23 @@ mod v2; pub use v2::DecimalBytePartsV2Metadata; -/// The frozen single-child decimal byte-parts format ID. +/// The frozen single-child DBP serialized ID. pub fn decimal_byte_parts_v1_id() -> ArrayId { static ID: CachedId = CachedId::new("vortex.decimal_byte_parts"); *ID } -/// The current in-memory DBP identity and the serialized format for arrays with lower parts. +/// The current in-memory DBP ID and serialized ID for arrays with lower parts. pub fn decimal_byte_parts_v2_id() -> ArrayId { static ID: CachedId = CachedId::new("vortex.decimal_byte_parts.v2"); *ID } -/// Serde for the current DBP array using the frozen v1 and v2 wire formats. +/// Serde for the [`DecimalByteParts`] array using the frozen v1 and v2 wire formats. /// /// Each version owns its metadata schema and serde functions. The plugin writes v1 whenever an /// array has no lower parts, so such arrays stay readable by older readers, and v2 otherwise. -/// The v2 format itself accepts any lower part count up to the maximum. Both serializers borrow -/// the current array, and both decoders construct a current [`DecimalByteParts`] array directly. +/// The v2 format itself accepts any lower part count up to the maximum. /// /// Register this plugin, or call [`crate::initialize`], to enable both formats. Direct registration /// of [`DecimalByteParts`] does not support serde.