From ee9ddf4b1796b02c483b336bae0f889d5db7404f Mon Sep 17 00:00:00 2001 From: Matt Katz Date: Tue, 8 Sep 2026 16:29:46 -0400 Subject: [PATCH 1/4] 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/mod.rs | 390 +++++++++++++++++- 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, 770 insertions(+), 12 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/mod.rs b/encodings/decimal-byte-parts/src/decimal_byte_parts/mod.rs index b9babd52aaa..f24ad9548d6 100644 --- a/encodings/decimal-byte-parts/src/decimal_byte_parts/mod.rs +++ b/encodings/decimal-byte-parts/src/decimal_byte_parts/mod.rs @@ -6,7 +6,9 @@ use std::fmt::Formatter; use std::hash::Hasher; use vortex_array::Array; +use vortex_array::ArrayDeserialization; use vortex_array::ArrayParts; +use vortex_array::ArraySerialization; use vortex_array::ArrayView; pub(crate) mod compute; mod limbs; @@ -26,11 +28,13 @@ use prost::Message as _; use vortex_array::ArrayEq; use vortex_array::ArrayHash; use vortex_array::ArrayId; +use vortex_array::ArrayPlugin; use vortex_array::ArrayRef; use vortex_array::ArraySlots; use vortex_array::EqMode; use vortex_array::ExecutionCtx; use vortex_array::ExecutionResult; +use vortex_array::IntoArray; use vortex_array::TypedArrayRef; use vortex_array::array_slots; use vortex_array::arrays::PrimitiveArray; @@ -177,7 +181,9 @@ impl DecimalByteParts { lower_parts: Vec, decimal_dtype: DecimalDType, ) -> VortexResult { - // Lower parts are supported in memory; the frozen serializer still rejects them. + // 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(); @@ -256,7 +262,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(), @@ -426,6 +432,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>, @@ -480,12 +577,15 @@ impl ValidityChild for DecimalByteParts { #[cfg(test)] mod tests { use rstest::rstest; + use vortex_array::ArrayContext; 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::dtype::DType; @@ -497,12 +597,18 @@ 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_buffer::ByteBufferMut; use vortex_buffer::buffer; use vortex_error::VortexResult; + use vortex_session::registry::ReadContext; use super::*; use crate::DecimalByteParts; + use crate::decimal_byte_parts::testing::encode; use crate::decimal_byte_parts::testing::i128_parts; use crate::decimal_byte_parts::testing::i256_of; use crate::decimal_byte_parts::testing::i256_parts; @@ -713,6 +819,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() } @@ -757,6 +984,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(); @@ -817,11 +1196,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 387b7980327915d6953ce61a6ae5c0c8fab5a1f9 Mon Sep 17 00:00:00 2001 From: Matt Katz Date: Wed, 9 Sep 2026 00:43:35 -0400 Subject: [PATCH 2/4] 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/mod.rs | 425 +----------- .../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 +- 6 files changed, 699 insertions(+), 688 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/mod.rs b/encodings/decimal-byte-parts/src/decimal_byte_parts/mod.rs index f24ad9548d6..a49c9c87f17 100644 --- a/encodings/decimal-byte-parts/src/decimal_byte_parts/mod.rs +++ b/encodings/decimal-byte-parts/src/decimal_byte_parts/mod.rs @@ -5,13 +5,13 @@ use std::fmt::Display; use std::fmt::Formatter; use std::hash::Hasher; +use prost::Message as _; use vortex_array::Array; -use vortex_array::ArrayDeserialization; use vortex_array::ArrayParts; -use vortex_array::ArraySerialization; use vortex_array::ArrayView; pub(crate) mod compute; mod limbs; +mod plugin; mod rules; #[cfg(test)] pub(crate) mod testing; @@ -24,17 +24,16 @@ pub mod _benchmarking { pub use super::limbs::assemble_decimal; } -use prost::Message as _; +pub use plugin::DecimalBytePartsPlugin; +pub use plugin::decimal_byte_parts_v2_id; use vortex_array::ArrayEq; use vortex_array::ArrayHash; use vortex_array::ArrayId; -use vortex_array::ArrayPlugin; use vortex_array::ArrayRef; use vortex_array::ArraySlots; use vortex_array::EqMode; use vortex_array::ExecutionCtx; use vortex_array::ExecutionResult; -use vortex_array::IntoArray; use vortex_array::TypedArrayRef; use vortex_array::array_slots; use vortex_array::arrays::PrimitiveArray; @@ -432,97 +431,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>, @@ -577,15 +485,12 @@ impl ValidityChild for DecimalByteParts { #[cfg(test)] mod tests { use rstest::rstest; - use vortex_array::ArrayContext; 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::dtype::DType; @@ -597,21 +502,17 @@ 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_buffer::ByteBufferMut; use vortex_buffer::buffer; use vortex_error::VortexResult; - use vortex_session::registry::ReadContext; use super::*; use crate::DecimalByteParts; - use crate::decimal_byte_parts::testing::encode; use crate::decimal_byte_parts::testing::i128_parts; use crate::decimal_byte_parts::testing::i256_of; use crate::decimal_byte_parts::testing::i256_parts; + 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() { @@ -652,47 +553,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))] @@ -819,127 +679,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() } @@ -984,158 +723,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/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 d2ce68f3700..950c963f7a2 100644 --- a/encodings/decimal-byte-parts/src/decimal_byte_parts/testing.rs +++ b/encodings/decimal-byte-parts/src/decimal_byte_parts/testing.rs @@ -51,3 +51,44 @@ pub(crate) fn i256_parts(values: Vec, validity: Validity) -> DecimalBytePa pub(crate) 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 b8dbf6a38aa32a729cb0abeb72c940200eb5a74d Mon Sep 17 00:00:00 2001 From: Matt Katz Date: Wed, 9 Sep 2026 17:07:26 -0400 Subject: [PATCH 3/4] 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 72f23963ada9d2cbf6650ee9a988e950e14ed8e1 Mon Sep 17 00:00:00 2001 From: Matt Katz Date: Wed, 9 Sep 2026 17:13:02 -0400 Subject: [PATCH 4/4] 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;