From e7afa28f9939f7651786f9424bc52262808d4e32 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 27 Jun 2026 19:22:44 +0000 Subject: [PATCH 1/3] refactor: give patches correct dtype by construction `Patches::cast_values` was a crutch that papered over patch values not matching the parent array's dtype, and ALPRD/Sparse each carried a construction-time normalization step (`canonicalize_patches`, `normalize_patches_dtype`) to fix patches up after the fact. Remove all three: patch-producing operations now yield the correct dtype directly, and constructors assert rather than normalize. - `Patches::mask` widens the surviving values to nullable. Masking an array always yields a nullable result, so the masked patches now carry the correct dtype and callers (ALP mask) no longer re-cast them. - ALP take/mask: `Patches::take`/`mask` already produce values matching the taken/masked encoded array, so build via `ALP::try_new`, which asserts the patch dtype instead of forcing it. - bitpacking take: `PrimitiveArray::patch` (via `Validity::patch`) already asserts matching nullability, so no cast is needed. - ALPRD: drop `canonicalize_patches`. Exceptions are always the non-nullable left-parts dtype; `validate_parts` now requires exactly that, `take` casts the taken exceptions back to it (folded into the take call), and the other construction paths already satisfy it. Neither `ALPRD::try_new` nor `RDEncoder::encode` needs an `ExecutionCtx` anymore (the RD split does no execution); their callers that only plumbed a ctx to pass it down are cleaned up too. - Sparse: drop `normalize_patches_dtype`. Both `Sparse::try_new` and the patches-based paths now assert that the values/patches already match the fill dtype instead of casting; callers construct matching values. Signed-off-by: Robert Kruszewski Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01UDa4BYuhQyRHWCUGTanVzE Signed-off-by: Robert Kruszewski --- encodings/alp/benches/alp_compress.rs | 6 +- encodings/alp/src/alp/compute/mask.rs | 12 ++-- encodings/alp/src/alp/compute/take.rs | 15 ++--- encodings/alp/src/alp_rd/array.rs | 57 ++++--------------- encodings/alp/src/alp_rd/compute/cast.rs | 14 ++--- encodings/alp/src/alp_rd/compute/filter.rs | 11 +--- encodings/alp/src/alp_rd/compute/mask.rs | 17 +----- encodings/alp/src/alp_rd/compute/mod.rs | 22 +++---- encodings/alp/src/alp_rd/compute/take.rs | 35 +++++------- encodings/alp/src/alp_rd/mod.rs | 11 +--- encodings/alp/src/alp_rd/ops.rs | 6 +- .../fastlanes/src/bitpacking/compute/take.rs | 6 +- encodings/sparse/src/canonical.rs | 9 ++- encodings/sparse/src/compute/cast.rs | 7 ++- encodings/sparse/src/lib.rs | 44 +++++++------- vortex-array/src/patches.rs | 55 ++++++++++-------- vortex-btrblocks/src/schemes/float/alprd.rs | 3 +- .../arrays/synthetic/encodings/alprd.rs | 22 +++---- vortex/benches/single_encoding_throughput.rs | 8 +-- 19 files changed, 148 insertions(+), 212 deletions(-) diff --git a/encodings/alp/benches/alp_compress.rs b/encodings/alp/benches/alp_compress.rs index 02cc30e44a6..6e769ffc22e 100644 --- a/encodings/alp/benches/alp_compress.rs +++ b/encodings/alp/benches/alp_compress.rs @@ -146,8 +146,8 @@ fn compress_rd(bencher: Bencher, args: (usize, f64) let encoder = RDEncoder::new(primitive.as_slice::()); bencher - .with_inputs(|| (&primitive, &encoder, SESSION.create_execution_ctx())) - .bench_refs(|(primitive, encoder, ctx)| encoder.encode(primitive.as_view(), ctx)) + .with_inputs(|| (&primitive, &encoder)) + .bench_refs(|(primitive, encoder)| encoder.encode(primitive.as_view())) } #[divan::bench(types = [f32, f64], args = RD_BENCH_ARGS)] @@ -155,7 +155,7 @@ fn decompress_rd(bencher: Bencher, args: (usize, f6 let (n, fraction_patch) = args; let primitive = make_rd_array::(n, fraction_patch); let encoder = RDEncoder::new(primitive.as_slice::()); - let encoded = encoder.encode(primitive.as_view(), &mut SESSION.create_execution_ctx()); + let encoded = encoder.encode(primitive.as_view()); bencher .with_inputs(|| (&encoded, SESSION.create_execution_ctx())) diff --git a/encodings/alp/src/alp/compute/mask.rs b/encodings/alp/src/alp/compute/mask.rs index b117ac216d0..08e4c26f360 100644 --- a/encodings/alp/src/alp/compute/mask.rs +++ b/encodings/alp/src/alp/compute/mask.rs @@ -36,18 +36,16 @@ impl MaskKernel for ALP { ) -> VortexResult> { let vortex_mask = Validity::Array(mask.not()?).execute_mask(array.len(), ctx)?; let masked_encoded = array.encoded().clone().mask(mask.clone())?; - let masked_dtype = array - .dtype() - .with_nullability(masked_encoded.dtype().nullability()); + // Masking always yields a nullable array, and `Patches::mask` widens the surviving patch + // values to nullable to match, so the patches already have the correct dtype. `try_new` + // asserts this. let masked_patches = array .patches() .map(|p| p.mask(&vortex_mask, ctx)) .transpose()? - .flatten() - .map(|patches| patches.cast_values(&masked_dtype)) - .transpose()?; + .flatten(); Ok(Some( - ALP::new(masked_encoded, array.exponents(), masked_patches).into_array(), + ALP::try_new(masked_encoded, array.exponents(), masked_patches)?.into_array(), )) } } diff --git a/encodings/alp/src/alp/compute/take.rs b/encodings/alp/src/alp/compute/take.rs index 9b193a3cfbe..51a48cf2689 100644 --- a/encodings/alp/src/alp/compute/take.rs +++ b/encodings/alp/src/alp/compute/take.rs @@ -19,21 +19,16 @@ impl TakeExecute for ALP { ctx: &mut ExecutionCtx, ) -> VortexResult> { let taken_encoded = array.encoded().take(indices.clone())?; + // `Patches::take` carries the take indices' nullability into the patch values, so the + // taken patches already match `taken_encoded`'s nullability. Construct via `try_new` to + // assert that invariant instead of casting the values to force it. let taken_patches = array .patches() .map(|p| p.take(indices, ctx)) .transpose()? - .flatten() - .map(|patches| { - patches.cast_values( - &array - .dtype() - .with_nullability(taken_encoded.dtype().nullability()), - ) - }) - .transpose()?; + .flatten(); Ok(Some( - ALP::new(taken_encoded, array.exponents(), taken_patches).into_array(), + ALP::try_new(taken_encoded, array.exponents(), taken_patches)?.into_array(), )) } } diff --git a/encodings/alp/src/alp_rd/array.rs b/encodings/alp/src/alp_rd/array.rs index 9dac5c292bd..605b991d4c4 100644 --- a/encodings/alp/src/alp_rd/array.rs +++ b/encodings/alp/src/alp_rd/array.rs @@ -17,14 +17,11 @@ use vortex_array::ArrayParts; use vortex_array::ArrayRef; use vortex_array::ArraySlots; use vortex_array::ArrayView; -use vortex_array::Canonical; use vortex_array::EqMode; use vortex_array::ExecutionCtx; use vortex_array::ExecutionResult; use vortex_array::IntoArray; -use vortex_array::LEGACY_SESSION; use vortex_array::TypedArrayRef; -use vortex_array::VortexSessionExecute; use vortex_array::arrays::Primitive; use vortex_array::arrays::PrimitiveArray; use vortex_array::buffer::BufferHandle; @@ -201,6 +198,8 @@ impl VTable for ALPRD { }; let right_parts = children.get(1, &right_parts_dtype, len)?; + // Patch values are read as the non-nullable left-parts dtype, which is exactly the dtype + // the array invariant requires, so no normalization is needed here. let left_parts_patches = metadata .patches .map(|p| { @@ -217,13 +216,6 @@ impl VTable for ALPRD { ) }) .transpose()?; - // NOTE: `VTable::deserialize` has a fixed trait signature without `ExecutionCtx`, so we - // cannot plumb a ctx in here. We construct a legacy ctx locally at this trait boundary. - let left_parts_patches = ALPRDData::canonicalize_patches( - &left_parts, - left_parts_patches, - &mut LEGACY_SESSION.create_execution_ctx(), - )?; let slots = ALPRDData::make_slots(&left_parts, &right_parts, left_parts_patches.as_ref()); let data = ALPRDData::new( left_parts_dictionary, @@ -376,11 +368,8 @@ impl ALPRD { right_parts: ArrayRef, right_bit_width: u8, left_parts_patches: Option, - ctx: &mut ExecutionCtx, ) -> VortexResult { let len = left_parts.len(); - let left_parts_patches = - ALPRDData::canonicalize_patches(&left_parts, left_parts_patches, ctx)?; let slots = ALPRDData::make_slots(&left_parts, &right_parts, left_parts_patches.as_ref()); let data = ALPRDData::new(left_parts_dictionary, right_bit_width, left_parts_patches); Array::try_from_parts(ArrayParts::new(ALPRD, dtype, len, data).with_slots(slots)) @@ -408,28 +397,6 @@ impl ALPRD { } impl ALPRDData { - fn canonicalize_patches( - left_parts: &ArrayRef, - left_parts_patches: Option, - ctx: &mut ExecutionCtx, - ) -> VortexResult> { - left_parts_patches - .map(|patches| { - if !patches.values().all_valid(ctx)? { - vortex_bail!("patches must be all valid: {}", patches.values()); - } - // TODO(ngates): assert the DType, don't cast it. - // TODO(joe): assert the DType, don't cast it in the next PR. - let mut patches = patches.cast_values(&left_parts.dtype().as_nonnullable())?; - // Force execution of the lazy cast so patch values are materialized - // before serialization. - let canonical = patches.values().clone().execute::(ctx)?; - *patches.values_mut() = canonical.into_array(); - Ok(patches) - }) - .transpose() - } - /// Build a new `ALPRDArray` from components. pub fn new( left_parts_dictionary: Buffer, @@ -556,18 +523,16 @@ fn validate_parts( "patches array_len {} != outer len {len}", patches.array_len(), ); + // Left-parts exceptions are always all-valid and are stored as the non-nullable left-parts + // dtype. Requiring that exact dtype (rather than ignoring nullability) means each + // construction path must produce correct patches, removing the need to normalize them. + // Non-nullable also implies all-valid, so no separate validity check is required. + let expected = left_parts.dtype().as_nonnullable(); vortex_ensure!( - patches.dtype().eq_ignore_nullability(left_parts.dtype()), - "patches dtype {} does not match left_parts dtype {}", + patches.dtype() == &expected, + "patches dtype {} must be the non-nullable left_parts dtype {}", patches.dtype(), - left_parts.dtype(), - ); - vortex_ensure!( - patches - .values() - .all_valid(&mut LEGACY_SESSION.create_execution_ctx())?, - "patches must be all valid: {}", - patches.values() + expected, ); } @@ -672,7 +637,7 @@ mod test { // Pick a seed that we know will trigger lots of patches. let encoder: alp_rd::RDEncoder = alp_rd::RDEncoder::new(&[seed.powi(-2)]); - let rd_array = encoder.encode(real_array.as_view(), &mut ctx); + let rd_array = encoder.encode(real_array.as_view()); let decoded = rd_array .as_array() diff --git a/encodings/alp/src/alp_rd/compute/cast.rs b/encodings/alp/src/alp_rd/compute/cast.rs index 717b97b760e..68ed39dd390 100644 --- a/encodings/alp/src/alp_rd/compute/cast.rs +++ b/encodings/alp/src/alp_rd/compute/cast.rs @@ -67,7 +67,7 @@ mod tests { let values = vec![1.0f32, 1.1, 1.2, 1.3, 1.4]; let arr = PrimitiveArray::from_iter(values.clone()); let encoder = RDEncoder::new(&values); - let alprd = encoder.encode(arr.as_view(), &mut ctx); + let alprd = encoder.encode(arr.as_view()); let casted = alprd .into_array() @@ -92,7 +92,7 @@ mod tests { PrimitiveArray::from_option_iter([Some(10.0f64), None, Some(10.1), Some(10.2), None]); let values = vec![10.0f64, 10.1, 10.2]; let encoder = RDEncoder::new(&values); - let alprd = encoder.encode(arr.as_view(), &mut ctx); + let alprd = encoder.encode(arr.as_view()); // Cast to NonNullable should fail since we have nulls. The failure surfaces during // execution since the reduce path defers when the validity stat is not cached. @@ -122,31 +122,31 @@ mod tests { let values = vec![1.23f32, 4.56, 7.89, 10.11, 12.13]; let arr = PrimitiveArray::from_iter(values.clone()); let encoder = RDEncoder::new(&values); - encoder.encode(arr.as_view(), &mut array_session().create_execution_ctx()) + encoder.encode(arr.as_view()) })] #[case::f64({ let values = vec![100.1f64, 200.2, 300.3, 400.4, 500.5]; let arr = PrimitiveArray::from_iter(values.clone()); let encoder = RDEncoder::new(&values); - encoder.encode(arr.as_view(), &mut array_session().create_execution_ctx()) + encoder.encode(arr.as_view()) })] #[case::single({ let values = vec![42.42f64]; let arr = PrimitiveArray::from_iter(values.clone()); let encoder = RDEncoder::new(&values); - encoder.encode(arr.as_view(), &mut array_session().create_execution_ctx()) + encoder.encode(arr.as_view()) })] #[case::negative({ let values = vec![0.0f32, -1.5, 2.5, -3.5, 4.5]; let arr = PrimitiveArray::from_iter(values.clone()); let encoder = RDEncoder::new(&values); - encoder.encode(arr.as_view(), &mut array_session().create_execution_ctx()) + encoder.encode(arr.as_view()) })] #[case::nullable({ let arr = PrimitiveArray::from_option_iter([Some(1.1f32), None, Some(2.2), Some(3.3), None]); let values = vec![1.1f32, 2.2, 3.3]; let encoder = RDEncoder::new(&values); - encoder.encode(arr.as_view(), &mut array_session().create_execution_ctx()) + encoder.encode(arr.as_view()) })] fn test_cast_alprd_conformance(#[case] alprd: crate::alp_rd::ALPRDArray) { test_cast_conformance(&alprd.into_array()); diff --git a/encodings/alp/src/alp_rd/compute/filter.rs b/encodings/alp/src/alp_rd/compute/filter.rs index 38d3fa96156..7aa8c247f2a 100644 --- a/encodings/alp/src/alp_rd/compute/filter.rs +++ b/encodings/alp/src/alp_rd/compute/filter.rs @@ -32,7 +32,6 @@ impl FilterKernel for ALPRD { array.right_parts().filter(mask.clone())?, array.right_bit_width(), left_parts_exceptions, - ctx, )? .into_array(), )) @@ -71,7 +70,7 @@ mod test { fn test_filter(#[case] a: T, #[case] b: T, #[case] outlier: T) { let mut ctx = SESSION.create_execution_ctx(); let array = PrimitiveArray::new(buffer![a, b, outlier], Validity::NonNullable); - let encoded = RDEncoder::new(&[a, b]).encode(array.as_view(), &mut ctx); + let encoded = RDEncoder::new(&[a, b]).encode(array.as_view()); // Make sure that we're testing the exception pathway. assert!(encoded.left_parts_patches().is_some()); @@ -87,13 +86,9 @@ mod test { #[case(0.1f32, 0.2f32, 3e25f32)] #[case(0.1f64, 0.2f64, 3e100f64)] fn test_filter_simple(#[case] a: T, #[case] b: T, #[case] outlier: T) { - let mut ctx = SESSION.create_execution_ctx(); test_filter_conformance( &RDEncoder::new(&[a, b]) - .encode( - PrimitiveArray::from_iter([a, b, outlier, b, outlier]).as_view(), - &mut ctx, - ) + .encode(PrimitiveArray::from_iter([a, b, outlier, b, outlier]).as_view()) .into_array(), ); } @@ -102,13 +97,11 @@ mod test { #[case(0.1f32, 3e25f32)] #[case(0.5f64, 1e100f64)] fn test_filter_with_nulls(#[case] a: T, #[case] outlier: T) { - let mut ctx = SESSION.create_execution_ctx(); test_filter_conformance( &RDEncoder::new(&[a]) .encode( PrimitiveArray::from_option_iter([Some(a), None, Some(outlier), Some(a), None]) .as_view(), - &mut ctx, ) .into_array(), ); diff --git a/encodings/alp/src/alp_rd/compute/mask.rs b/encodings/alp/src/alp_rd/compute/mask.rs index cac98683adb..a80b5d306a1 100644 --- a/encodings/alp/src/alp_rd/compute/mask.rs +++ b/encodings/alp/src/alp_rd/compute/mask.rs @@ -4,8 +4,6 @@ use vortex_array::ArrayRef; use vortex_array::ArrayView; use vortex_array::IntoArray; -use vortex_array::LEGACY_SESSION; -use vortex_array::VortexSessionExecute; use vortex_array::arrays::scalar_fn::ScalarFnFactoryExt; use vortex_array::scalar_fn::EmptyOptions; use vortex_array::scalar_fn::fns::mask::Mask as MaskExpr; @@ -22,8 +20,8 @@ impl MaskReduce for ALPRD { EmptyOptions, [array.left_parts().clone(), mask.clone()], )?; - // NOTE: `MaskReduce::mask` has a fixed trait signature without `ExecutionCtx`, so we - // construct a legacy ctx locally at this trait boundary. + // The original exceptions are already the non-nullable left-parts dtype, which the + // (now nullable) masked array still requires, so they can be reused as-is. Ok(Some( ALPRD::try_new( array.dtype().as_nullable(), @@ -32,7 +30,6 @@ impl MaskReduce for ALPRD { array.right_parts().clone(), array.right_bit_width(), array.left_parts_patches(), - &mut LEGACY_SESSION.create_execution_ctx(), )? .into_array(), )) @@ -43,8 +40,6 @@ impl MaskReduce for ALPRD { mod tests { use rstest::rstest; use vortex_array::IntoArray; - use vortex_array::VortexSessionExecute; - use vortex_array::array_session; use vortex_array::arrays::PrimitiveArray; use vortex_array::compute::conformance::mask::test_mask_conformance; @@ -55,13 +50,9 @@ mod tests { #[case(0.1f32, 0.2f32, 3e25f32)] #[case(0.1f64, 0.2f64, 3e100f64)] fn test_mask_simple(#[case] a: T, #[case] b: T, #[case] outlier: T) { - let mut ctx = array_session().create_execution_ctx(); test_mask_conformance( &RDEncoder::new(&[a, b]) - .encode( - PrimitiveArray::from_iter([a, b, outlier, b, outlier]).as_view(), - &mut ctx, - ) + .encode(PrimitiveArray::from_iter([a, b, outlier, b, outlier]).as_view()) .into_array(), ); } @@ -70,13 +61,11 @@ mod tests { #[case(0.1f32, 3e25f32)] #[case(0.5f64, 1e100f64)] fn test_mask_with_nulls(#[case] a: T, #[case] outlier: T) { - let mut ctx = array_session().create_execution_ctx(); test_mask_conformance( &RDEncoder::new(&[a]) .encode( PrimitiveArray::from_option_iter([Some(a), None, Some(outlier), Some(a), None]) .as_view(), - &mut ctx, ) .into_array(), ); diff --git a/encodings/alp/src/alp_rd/compute/mod.rs b/encodings/alp/src/alp_rd/compute/mod.rs index f74b23fc2c2..696e622b873 100644 --- a/encodings/alp/src/alp_rd/compute/mod.rs +++ b/encodings/alp/src/alp_rd/compute/mod.rs @@ -34,47 +34,47 @@ mod tests { let values = vec![1.0f32, 1.1, 1.2, 1.3, 1.4]; let arr = PrimitiveArray::from_iter(values.clone()); let encoder = RDEncoder::new(&values); - encoder.encode(arr.as_view(), &mut SESSION.create_execution_ctx()) + encoder.encode(arr.as_view()) })] #[case::f64_array({ let values = vec![100.0f64, 100.01, 100.02, 100.03, 100.04]; let arr = PrimitiveArray::from_iter(values.clone()); let encoder = RDEncoder::new(&values); - encoder.encode(arr.as_view(), &mut SESSION.create_execution_ctx()) + encoder.encode(arr.as_view()) })] // Nullable arrays #[case::nullable_f32({ let values = vec![1.0f32, 1.2, 1.3]; let arr = PrimitiveArray::from_option_iter([Some(1.0f32), None, Some(1.2), Some(1.3), None]); let encoder = RDEncoder::new(&values); - encoder.encode(arr.as_view(), &mut SESSION.create_execution_ctx()) + encoder.encode(arr.as_view()) })] #[case::nullable_f64({ let values = vec![10.0f64, 10.2, 10.3]; let arr = PrimitiveArray::from_option_iter([Some(10.0f64), None, Some(10.2), Some(10.3), None]); let encoder = RDEncoder::new(&values); - encoder.encode(arr.as_view(), &mut SESSION.create_execution_ctx()) + encoder.encode(arr.as_view()) })] // Edge cases #[case::single_element({ let values = vec![42.42f64]; let arr = PrimitiveArray::from_iter(values.clone()); let encoder = RDEncoder::new(&values); - encoder.encode(arr.as_view(), &mut SESSION.create_execution_ctx()) + encoder.encode(arr.as_view()) })] // Arrays with small deltas (good for RD encoding) #[case::small_deltas({ let values = vec![1000.0f32, 1000.001, 1000.002, 1000.003, 1000.004]; let arr = PrimitiveArray::from_iter(values.clone()); let encoder = RDEncoder::new(&values); - encoder.encode(arr.as_view(), &mut SESSION.create_execution_ctx()) + encoder.encode(arr.as_view()) })] // Large arrays #[case::large_f32({ let values: Vec = (0..1000).map(|i| 100.0 + i as f32 * 0.01).collect(); let arr = PrimitiveArray::from_iter(values.clone()); let encoder = RDEncoder::new(&values); - encoder.encode(arr.as_view(), &mut SESSION.create_execution_ctx()) + encoder.encode(arr.as_view()) })] fn test_alp_rd_consistency(#[case] array: ALPRDArray) { let ctx = &mut SESSION.create_execution_ctx(); @@ -86,25 +86,25 @@ mod tests { let values = vec![1.0f32, 1.1, 1.2, 1.3, 1.4]; let arr = PrimitiveArray::from_iter(values.clone()); let encoder = RDEncoder::new(&values); - encoder.encode(arr.as_view(), &mut SESSION.create_execution_ctx()) + encoder.encode(arr.as_view()) })] #[case::f64_basic({ let values = vec![100.0f64, 100.01, 100.02, 100.03, 100.04]; let arr = PrimitiveArray::from_iter(values.clone()); let encoder = RDEncoder::new(&values); - encoder.encode(arr.as_view(), &mut SESSION.create_execution_ctx()) + encoder.encode(arr.as_view()) })] #[case::f32_large({ let values: Vec = (0..100).map(|i| 50.0 + i as f32 * 0.1).collect(); let arr = PrimitiveArray::from_iter(values.clone()); let encoder = RDEncoder::new(&values); - encoder.encode(arr.as_view(), &mut SESSION.create_execution_ctx()) + encoder.encode(arr.as_view()) })] #[case::f64_large({ let values: Vec = (0..100).map(|i| 1000.0 + i as f64 * 0.01).collect(); let arr = PrimitiveArray::from_iter(values.clone()); let encoder = RDEncoder::new(&values); - encoder.encode(arr.as_view(), &mut SESSION.create_execution_ctx()) + encoder.encode(arr.as_view()) })] fn test_alp_rd_binary_numeric(#[case] array: ALPRDArray) { test_binary_numeric_array(&array.into_array(), &mut SESSION.create_execution_ctx()); diff --git a/encodings/alp/src/alp_rd/compute/take.rs b/encodings/alp/src/alp_rd/compute/take.rs index dee2454ea05..8d12bfbb316 100644 --- a/encodings/alp/src/alp_rd/compute/take.rs +++ b/encodings/alp/src/alp_rd/compute/take.rs @@ -20,19 +20,21 @@ impl TakeExecute for ALPRD { ctx: &mut ExecutionCtx, ) -> VortexResult> { let taken_left_parts = array.left_parts().take(indices.clone())?; + // With nullable take indices, `Patches::take` widens the (still all-valid) exception + // values to nullable, but ALPRD stores exceptions as the non-nullable left-parts dtype. + // Narrow them back so the patches satisfy the invariant `validate_parts` enforces. This is + // a no-op when the take indices are non-nullable and the values already match. + let exceptions_dtype = taken_left_parts.dtype().as_nonnullable(); let left_parts_exceptions = array .left_parts_patches() - .map(|patches| patches.take(indices, ctx)) - .transpose()? - .flatten() - .map(|p| { - let values_dtype = p - .values() - .dtype() - .with_nullability(taken_left_parts.dtype().nullability()); - p.cast_values(&values_dtype) + .map(|patches| { + patches + .take(indices, ctx)? + .map(|taken| taken.map_values(|values| values.cast(exceptions_dtype.clone()))) + .transpose() }) - .transpose()?; + .transpose()? + .flatten(); let right_parts = array .right_parts() .take(indices.clone())? @@ -48,7 +50,6 @@ impl TakeExecute for ALPRD { right_parts, array.right_bit_width(), left_parts_exceptions, - ctx, )? .into_array(), )) @@ -87,7 +88,7 @@ mod test { let mut ctx = SESSION.create_execution_ctx(); let array = PrimitiveArray::from_iter([a, b, outlier]); - let encoded = RDEncoder::new(&[a, b]).encode(array.as_view(), &mut ctx); + let encoded = RDEncoder::new(&[a, b]).encode(array.as_view()); assert!(encoded.left_parts_patches().is_some()); assert!( @@ -113,7 +114,7 @@ mod test { fn take_with_nulls(#[case] a: T, #[case] b: T, #[case] outlier: T) { let mut ctx = SESSION.create_execution_ctx(); let array = PrimitiveArray::from_iter([a, b, outlier]); - let encoded = RDEncoder::new(&[a, b]).encode(array.as_view(), &mut ctx); + let encoded = RDEncoder::new(&[a, b]).encode(array.as_view()); assert!(encoded.left_parts_patches().is_some()); assert!( @@ -141,13 +142,9 @@ mod test { #[case(0.1f32, 0.2f32, 3e25f32)] #[case(0.1f64, 0.2f64, 3e100f64)] fn test_take_conformance_alprd(#[case] a: T, #[case] b: T, #[case] outlier: T) { - let mut ctx = SESSION.create_execution_ctx(); test_take_conformance( &RDEncoder::new(&[a, b]) - .encode( - PrimitiveArray::from_iter([a, b, outlier, b, outlier]).as_view(), - &mut ctx, - ) + .encode(PrimitiveArray::from_iter([a, b, outlier, b, outlier]).as_view()) .into_array(), ); } @@ -156,13 +153,11 @@ mod test { #[case(0.1f32, 3e25f32)] #[case(0.5f64, 1e100f64)] fn test_take_with_nulls_conformance(#[case] a: T, #[case] outlier: T) { - let mut ctx = SESSION.create_execution_ctx(); test_take_conformance( &RDEncoder::new(&[a]) .encode( PrimitiveArray::from_option_iter([Some(a), None, Some(outlier), Some(a), None]) .as_view(), - &mut ctx, ) .into_array(), ); diff --git a/encodings/alp/src/alp_rd/mod.rs b/encodings/alp/src/alp_rd/mod.rs index a9ac7ca82fe..07afe6b8adf 100644 --- a/encodings/alp/src/alp_rd/mod.rs +++ b/encodings/alp/src/alp_rd/mod.rs @@ -187,15 +187,11 @@ impl RDEncoder { /// /// Each value will be split into a left and right component, which are compressed individually. // TODO(joe): make fallible - pub fn encode(&self, array: ArrayView<'_, Primitive>, ctx: &mut ExecutionCtx) -> ALPRDArray { - match_each_alp_float_ptype!(array.ptype(), |P| { self.encode_generic::

(array, ctx) }) + pub fn encode(&self, array: ArrayView<'_, Primitive>) -> ALPRDArray { + match_each_alp_float_ptype!(array.ptype(), |P| { self.encode_generic::

(array) }) } - fn encode_generic( - &self, - array: ArrayView<'_, Primitive>, - ctx: &mut ExecutionCtx, - ) -> ALPRDArray + fn encode_generic(&self, array: ArrayView<'_, Primitive>) -> ALPRDArray where T: ALPRDFloat + NativePType, T::UINT: NativePType, @@ -294,7 +290,6 @@ impl RDEncoder { packed_right, self.right_bit_width, exceptions, - ctx, ) .vortex_expect("ALPRDArray construction in encode") } diff --git a/encodings/alp/src/alp_rd/ops.rs b/encodings/alp/src/alp_rd/ops.rs index be8d8f88948..0fc0dc40fa1 100644 --- a/encodings/alp/src/alp_rd/ops.rs +++ b/encodings/alp/src/alp_rd/ops.rs @@ -90,7 +90,7 @@ mod test { fn test_slice(#[case] a: T, #[case] b: T, #[case] outlier: T) { let mut ctx = SESSION.create_execution_ctx(); let array = PrimitiveArray::from_iter([a, b, outlier]); - let encoded = RDEncoder::new(&[a, b]).encode(array.as_view(), &mut ctx); + let encoded = RDEncoder::new(&[a, b]).encode(array.as_view()); assert!(encoded.left_parts_patches().is_some()); assert_arrays_eq!(encoded, array, &mut ctx); @@ -106,7 +106,7 @@ mod test { ) { let mut ctx = SESSION.create_execution_ctx(); let array = PrimitiveArray::from_iter([a, b, outlier]); - let encoded = RDEncoder::new(&[a, b]).encode(array.as_view(), &mut ctx); + let encoded = RDEncoder::new(&[a, b]).encode(array.as_view()); assert!(encoded.left_parts_patches().is_some()); assert_arrays_eq!(encoded, array, &mut ctx); } @@ -118,7 +118,7 @@ mod test { let b = 0.2f64; let outlier = 3e100f64; let array = PrimitiveArray::from_option_iter([Some(a), Some(b), Some(outlier)]); - let encoded = RDEncoder::new(&[a, b]).encode(array.as_view(), &mut ctx); + let encoded = RDEncoder::new(&[a, b]).encode(array.as_view()); assert!(encoded.left_parts_patches().is_some()); assert_arrays_eq!( encoded, diff --git a/encodings/fastlanes/src/bitpacking/compute/take.rs b/encodings/fastlanes/src/bitpacking/compute/take.rs index 7ffe051ca40..a37ebbdff4f 100644 --- a/encodings/fastlanes/src/bitpacking/compute/take.rs +++ b/encodings/fastlanes/src/bitpacking/compute/take.rs @@ -150,8 +150,10 @@ fn take_primitive( if let Some(patches) = array.patches() && let Some(patches) = patches.take(&indices.clone().into_array(), ctx)? { - let cast_patches = patches.cast_values(unpatched_taken.dtype())?; - return unpatched_taken.patch(&cast_patches, ctx); + // `take` carries the take indices' nullability into the patch values, matching + // `unpatched_taken`'s validity. `patch` (via `Validity::patch`) asserts the two agree, + // so no cast is required to align them. + return unpatched_taken.patch(&patches, ctx); } Ok(unpatched_taken) diff --git a/encodings/sparse/src/canonical.rs b/encodings/sparse/src/canonical.rs index 1829d4fffd1..afca2b29a18 100644 --- a/encodings/sparse/src/canonical.rs +++ b/encodings/sparse/src/canonical.rs @@ -1385,7 +1385,8 @@ mod test { #[test] fn test_sparse_fixed_size_list_non_null_fill() -> VortexResult<()> { let elements = buffer![1i32, 2, 3, 4, 5, 6].into_array(); - let fsl = FixedSizeListArray::try_new(elements, 2, Validity::AllValid, 3)?.into_array(); + // Non-nullable values to match the non-nullable fill below. + let fsl = FixedSizeListArray::try_new(elements, 2, Validity::NonNullable, 3)?.into_array(); let indices = buffer![0u8, 2u8, 4u8].into_array(); let fill_value = Scalar::fixed_size_list( @@ -1462,7 +1463,8 @@ mod test { // Create patch values: only 3 distinct lists out of 100 total positions. let elements = buffer![10i32, 11, 20, 21, 30, 31].into_array(); - let fsl = FixedSizeListArray::try_new(elements, 2, Validity::AllValid, 3)?.into_array(); + // Non-nullable values to match the non-nullable fill below. + let fsl = FixedSizeListArray::try_new(elements, 2, Validity::NonNullable, 3)?.into_array(); // Patches at positions 5, 50, and 95 out of 100. let indices = buffer![5u32, 50, 95].into_array(); @@ -1520,7 +1522,8 @@ mod test { fn test_sparse_fixed_size_list_single_element() -> VortexResult<()> { // Test with a single element FSL array. let elements = buffer![42i32, 43].into_array(); - let fsl = FixedSizeListArray::try_new(elements, 2, Validity::AllValid, 1)?.into_array(); + // Non-nullable values to match the non-nullable fill below. + let fsl = FixedSizeListArray::try_new(elements, 2, Validity::NonNullable, 1)?.into_array(); let indices = buffer![0u32].into_array(); let fill_value = Scalar::fixed_size_list( diff --git a/encodings/sparse/src/compute/cast.rs b/encodings/sparse/src/compute/cast.rs index 39c21805426..21af0bc212a 100644 --- a/encodings/sparse/src/compute/cast.rs +++ b/encodings/sparse/src/compute/cast.rs @@ -144,7 +144,9 @@ mod tests { // zero fill, and keeps the result in the Sparse encoding. let sparse = Sparse::try_new( buffer![0u64, 1, 2, 3, 4].into_array(), - buffer![10u64, 20, 30, 40, 50].into_array(), + // Nullable values to match the null (nullable) fill. + PrimitiveArray::from_option_iter([Some(10u64), Some(20), Some(30), Some(40), Some(50)]) + .into_array(), 5, Scalar::null_native::(), )?; @@ -171,7 +173,8 @@ mod tests { // non-nullable, which must not panic. let sparse = Sparse::try_new( buffer![1u64, 3].into_array(), - buffer![10u64, 20].into_array(), + // Nullable values to match the null (nullable) fill. + PrimitiveArray::from_option_iter([Some(10u64), Some(20)]).into_array(), 5, Scalar::null_native::(), )?; diff --git a/encodings/sparse/src/lib.rs b/encodings/sparse/src/lib.rs index f94555434aa..93c0bbe0363 100644 --- a/encodings/sparse/src/lib.rs +++ b/encodings/sparse/src/lib.rs @@ -385,6 +385,12 @@ impl Sparse { fill_value: Scalar, ) -> VortexResult { let dtype = fill_value.dtype().clone(); + vortex_ensure!( + values.dtype() == &dtype, + "sparse values dtype {} must match fill value dtype {}", + values.dtype(), + dtype, + ); let patches = Patches::new(len, 0, indices, values, None)?; let slots = SparseData::make_slots(&patches); let data = SparseData::from_patches(&patches, fill_value)?; @@ -424,25 +430,6 @@ impl Sparse { } impl SparseData { - fn normalize_patches_dtype(patches: Patches, fill_value: &Scalar) -> VortexResult { - let fill_dtype = fill_value.dtype(); - let values_dtype = patches.values().dtype(); - - vortex_ensure!( - values_dtype.eq_ignore_nullability(fill_dtype), - "fill value, {:?}, should be instance of values dtype, {} but was {}.", - fill_value, - values_dtype, - fill_dtype, - ); - - if values_dtype == fill_dtype { - Ok(patches) - } else { - patches.cast_values(fill_dtype) - } - } - pub fn validate( patches: &Patches, fill_value: &Scalar, @@ -482,16 +469,23 @@ impl SparseData { .vortex_expect("SparseArray patch slots must be present") } - /// Build a new SparseData from an existing set of patches, normalizing dtypes. + /// Build a new SparseData from an existing set of patches. pub fn try_new_from_patches(patches: Patches, fill_value: Scalar) -> VortexResult { - let patches = Self::normalize_patches_dtype(patches, &fill_value)?; - Ok(Self::from_patches_unchecked(&patches, fill_value)) + Self::from_patches(&patches, fill_value) } - /// Extract metadata from patches to create SparseData, with dtype normalization. + /// Extract metadata from patches to create SparseData. + /// + /// Patch values must already match the fill dtype; callers are expected to construct patches + /// with the correct dtype rather than relying on this to normalize them. fn from_patches(patches: &Patches, fill_value: Scalar) -> VortexResult { - let patches = Self::normalize_patches_dtype(patches.clone(), &fill_value)?; - Ok(Self::from_patches_unchecked(&patches, fill_value)) + vortex_ensure!( + patches.values().dtype() == fill_value.dtype(), + "patch values dtype {} must match fill dtype {}", + patches.values().dtype(), + fill_value.dtype(), + ); + Ok(Self::from_patches_unchecked(patches, fill_value)) } /// Extract metadata from patches to create SparseData, without validation. diff --git a/vortex-array/src/patches.rs b/vortex-array/src/patches.rs index a9952a7e99b..2df53a8e7f9 100644 --- a/vortex-array/src/patches.rs +++ b/vortex-array/src/patches.rs @@ -440,20 +440,6 @@ impl Patches { )) } - pub fn cast_values(self, values_dtype: &DType) -> VortexResult { - // SAFETY: casting does not affect the relationship between the indices and values - unsafe { - Ok(Self::new_unchecked( - self.array_len, - self.offset, - self.indices, - self.values.cast(values_dtype.clone())?, - self.chunk_offsets, - self.offset_within_chunk, - )) - } - } - /// Get the patched value at a given index if it exists. pub fn get_patched(&self, index: usize) -> VortexResult> { self.search_index(index)? @@ -693,6 +679,10 @@ impl Patches { /// Mask the patches, REMOVING the patches where the mask is true. /// Unlike filter, this preserves the patch indices. /// Unlike mask on a single array, this does not set masked values to null. + /// + /// Masking an array always yields a nullable result (see [`crate::scalar_fn::fns::mask`]), + /// so the surviving patch values are widened to nullable to match the masked parent. Callers + /// therefore receive patches with the correct dtype and do not need to re-cast them. // TODO(joe): make this lazy and remove the ctx. pub fn mask(&self, mask: &Mask, ctx: &mut ExecutionCtx) -> VortexResult> { if mask.len() != self.array_len { @@ -705,7 +695,7 @@ impl Patches { let filter_mask = match mask.bit_buffer() { AllOr::All => return Ok(None), - AllOr::None => return Ok(Some(self.clone())), + AllOr::None => return self.clone().into_nullable_values().map(Some), AllOr::Some(masked) => { let patch_indices = self.indices().clone().execute::(ctx)?; match_each_unsigned_integer_ptype!(patch_indices.ptype(), |P| { @@ -727,7 +717,7 @@ impl Patches { let filtered_indices = self.indices.filter(filter_mask.clone())?; let filtered_values = self.values.filter(filter_mask)?; - Ok(Some(Self { + Self { array_len: self.array_len, offset: self.offset, indices: filtered_indices, @@ -735,7 +725,21 @@ impl Patches { // TODO(0ax1): Chunk offsets are invalid after a filter is applied. chunk_offsets: None, offset_within_chunk: self.offset_within_chunk, - })) + } + .into_nullable_values() + .map(Some) + } + + /// Widen the patch values to their nullable dtype, leaving indices and offsets untouched. + /// + /// The values stay logically unchanged (all currently-valid entries remain valid); only the + /// dtype's nullability flag is set. Used by [`Self::mask`], whose result must be nullable. + fn into_nullable_values(self) -> VortexResult { + if self.values.dtype().is_nullable() { + return Ok(self); + } + let nullable = self.values.dtype().as_nullable(); + self.map_values(|values| values.cast(nullable)) } /// Slice the patches by a range of the patched array. @@ -1519,10 +1523,11 @@ mod test { let mask = Mask::new_false(10); let masked = patches.mask(&mask, &mut ctx).unwrap().unwrap(); - // No patch values should be masked + // Masking widens the surviving (still valid) values to nullable. + assert!(masked.values().dtype().is_nullable()); assert_arrays_eq!( masked.values(), - PrimitiveArray::from_iter([100i32, 200, 300]), + PrimitiveArray::from_option_iter([Some(100i32), Some(200), Some(300)]), &mut ctx ); assert!(masked.values().is_valid(0, &mut ctx).unwrap()); @@ -1559,7 +1564,7 @@ mod test { assert_eq!(masked.values().len(), 1); assert_arrays_eq!( masked.values(), - PrimitiveArray::from_iter([200i32]), + PrimitiveArray::from_option_iter([Some(200i32)]), &mut ctx ); @@ -1598,7 +1603,7 @@ mod test { ); assert_arrays_eq!( masked.values(), - PrimitiveArray::from_iter([200i32, 300]), + PrimitiveArray::from_option_iter([Some(200i32), Some(300)]), &mut ctx ); } @@ -1905,7 +1910,7 @@ mod test { ); assert_arrays_eq!( masked.values(), - PrimitiveArray::from_iter([200i32]), + PrimitiveArray::from_option_iter([Some(200i32)]), &mut ctx ); } @@ -1957,7 +1962,7 @@ mod test { ); assert_arrays_eq!( masked.values(), - PrimitiveArray::from_iter([100i32, 200, 300]), + PrimitiveArray::from_option_iter([Some(100i32), Some(200), Some(300)]), &mut ctx ); } @@ -2016,7 +2021,7 @@ mod test { ); assert_arrays_eq!( masked.values(), - PrimitiveArray::from_iter([100i32, 400]), + PrimitiveArray::from_option_iter([Some(100i32), Some(400)]), &mut ctx ); } @@ -2048,7 +2053,7 @@ mod test { ); assert_arrays_eq!( masked.values(), - PrimitiveArray::from_iter([100i32, 300]), + PrimitiveArray::from_option_iter([Some(100i32), Some(300)]), &mut ctx ); } diff --git a/vortex-btrblocks/src/schemes/float/alprd.rs b/vortex-btrblocks/src/schemes/float/alprd.rs index f97ae077882..969afabad49 100644 --- a/vortex-btrblocks/src/schemes/float/alprd.rs +++ b/vortex-btrblocks/src/schemes/float/alprd.rs @@ -66,7 +66,7 @@ impl Scheme for ALPRDScheme { ptype => vortex_panic!("cannot ALPRD compress ptype {ptype}"), }; - let alp_rd = encoder.encode(primitive_array, exec_ctx); + let alp_rd = encoder.encode(primitive_array); let dtype = alp_rd.dtype().clone(); let right_bit_width = alp_rd.right_bit_width(); let mut parts = ALPRDArrayOwnedExt::into_data_parts(alp_rd); @@ -82,7 +82,6 @@ impl Scheme for ALPRDScheme { parts.right_parts, right_bit_width, parts.left_parts_patches, - exec_ctx, )? .into_array()) } diff --git a/vortex-test/compat-gen/src/fixtures/arrays/synthetic/encodings/alprd.rs b/vortex-test/compat-gen/src/fixtures/arrays/synthetic/encodings/alprd.rs index a073dfc6693..1ebfc3d8f21 100644 --- a/vortex-test/compat-gen/src/fixtures/arrays/synthetic/encodings/alprd.rs +++ b/vortex-test/compat-gen/src/fixtures/arrays/synthetic/encodings/alprd.rs @@ -46,7 +46,7 @@ impl FlatLayoutFixture for AlprdFixture { vec![ALPRD.id()] } - fn build(&self, ctx: &mut ExecutionCtx) -> VortexResult { + fn build(&self, _ctx: &mut ExecutionCtx) -> VortexResult { let sensor: PrimitiveArray = (0..N) .map(|i| { let noise = ((i * 7 + 13) % 100) as f64 / 1000.0; @@ -151,31 +151,31 @@ impl FlatLayoutFixture for AlprdFixture { "nullable_specials", ]), vec![ - sensor_enc.encode(sensor.as_view(), ctx).into_array(), - drift_enc.encode(drift.as_view(), ctx).into_array(), + sensor_enc.encode(sensor.as_view()).into_array(), + drift_enc.encode(drift.as_view()).into_array(), constant_enc - .encode(constant_series.as_view(), ctx) + .encode(constant_series.as_view()) .into_array(), decreasing_enc - .encode(decreasing.as_view(), ctx) + .encode(decreasing.as_view()) .into_array(), oscillating_enc - .encode(oscillating.as_view(), ctx) + .encode(oscillating.as_view()) .into_array(), periodic_resets_enc - .encode(periodic_resets.as_view(), ctx) + .encode(periodic_resets.as_view()) .into_array(), nullable_enc - .encode(sensor_nullable.as_view(), ctx) + .encode(sensor_nullable.as_view()) .into_array(), special_enc - .encode(special_values.as_view(), ctx) + .encode(special_values.as_view()) .into_array(), boundary_enc - .encode(boundary_specials.as_view(), ctx) + .encode(boundary_specials.as_view()) .into_array(), nullable_special_enc - .encode(nullable_specials.as_view(), ctx) + .encode(nullable_specials.as_view()) .into_array(), ], N, diff --git a/vortex/benches/single_encoding_throughput.rs b/vortex/benches/single_encoding_throughput.rs index a20777af583..b75f832a7a1 100644 --- a/vortex/benches/single_encoding_throughput.rs +++ b/vortex/benches/single_encoding_throughput.rs @@ -299,10 +299,10 @@ fn bench_alp_rd_compress_f64(bencher: Bencher) { let (_, _, float_array) = setup_primitive_arrays(); with_byte_counter(bencher, NUM_VALUES * 8) - .with_inputs(|| (&float_array, SESSION.create_execution_ctx())) - .bench_refs(|(a, ctx)| { + .with_inputs(|| &float_array) + .bench_refs(|a| { let encoder = RDEncoder::new(a.as_slice::()); - encoder.encode(a.as_view(), ctx) + encoder.encode(a.as_view()) }); } @@ -310,7 +310,7 @@ fn bench_alp_rd_compress_f64(bencher: Bencher) { fn bench_alp_rd_decompress_f64(bencher: Bencher) { let (_, _, float_array) = setup_primitive_arrays(); let encoder = RDEncoder::new(float_array.as_slice::()); - let compressed = encoder.encode(float_array.as_view(), &mut SESSION.create_execution_ctx()); + let compressed = encoder.encode(float_array.as_view()); with_byte_counter(bencher, NUM_VALUES * 8) .with_inputs(|| (&compressed, SESSION.create_execution_ctx())) From 4c04d49507594a415abae22e759c8e845dd650f6 Mon Sep 17 00:00:00 2001 From: Robert Kruszewski Date: Tue, 30 Jun 2026 00:43:46 +0100 Subject: [PATCH 2/3] format Signed-off-by: Robert Kruszewski --- .../arrays/synthetic/encodings/alprd.rs | 20 +++++-------------- 1 file changed, 5 insertions(+), 15 deletions(-) diff --git a/vortex-test/compat-gen/src/fixtures/arrays/synthetic/encodings/alprd.rs b/vortex-test/compat-gen/src/fixtures/arrays/synthetic/encodings/alprd.rs index 1ebfc3d8f21..ef41fbf8a58 100644 --- a/vortex-test/compat-gen/src/fixtures/arrays/synthetic/encodings/alprd.rs +++ b/vortex-test/compat-gen/src/fixtures/arrays/synthetic/encodings/alprd.rs @@ -153,24 +153,14 @@ impl FlatLayoutFixture for AlprdFixture { vec![ sensor_enc.encode(sensor.as_view()).into_array(), drift_enc.encode(drift.as_view()).into_array(), - constant_enc - .encode(constant_series.as_view()) - .into_array(), - decreasing_enc - .encode(decreasing.as_view()) - .into_array(), - oscillating_enc - .encode(oscillating.as_view()) - .into_array(), + constant_enc.encode(constant_series.as_view()).into_array(), + decreasing_enc.encode(decreasing.as_view()).into_array(), + oscillating_enc.encode(oscillating.as_view()).into_array(), periodic_resets_enc .encode(periodic_resets.as_view()) .into_array(), - nullable_enc - .encode(sensor_nullable.as_view()) - .into_array(), - special_enc - .encode(special_values.as_view()) - .into_array(), + nullable_enc.encode(sensor_nullable.as_view()).into_array(), + special_enc.encode(special_values.as_view()).into_array(), boundary_enc .encode(boundary_specials.as_view()) .into_array(), From 95975297edc6e6082c44fe83e13cc548d1b0cd01 Mon Sep 17 00:00:00 2001 From: Robert Kruszewski Date: Tue, 30 Jun 2026 00:47:17 +0100 Subject: [PATCH 3/3] comments Signed-off-by: Robert Kruszewski --- encodings/alp/src/alp/compute/mask.rs | 3 --- encodings/alp/src/alp/compute/take.rs | 3 --- encodings/alp/src/alp_rd/array.rs | 2 -- encodings/alp/src/alp_rd/compute/mask.rs | 2 -- encodings/fastlanes/src/bitpacking/compute/take.rs | 3 --- 5 files changed, 13 deletions(-) diff --git a/encodings/alp/src/alp/compute/mask.rs b/encodings/alp/src/alp/compute/mask.rs index 08e4c26f360..2a528987143 100644 --- a/encodings/alp/src/alp/compute/mask.rs +++ b/encodings/alp/src/alp/compute/mask.rs @@ -36,9 +36,6 @@ impl MaskKernel for ALP { ) -> VortexResult> { let vortex_mask = Validity::Array(mask.not()?).execute_mask(array.len(), ctx)?; let masked_encoded = array.encoded().clone().mask(mask.clone())?; - // Masking always yields a nullable array, and `Patches::mask` widens the surviving patch - // values to nullable to match, so the patches already have the correct dtype. `try_new` - // asserts this. let masked_patches = array .patches() .map(|p| p.mask(&vortex_mask, ctx)) diff --git a/encodings/alp/src/alp/compute/take.rs b/encodings/alp/src/alp/compute/take.rs index 51a48cf2689..2e071eaa2aa 100644 --- a/encodings/alp/src/alp/compute/take.rs +++ b/encodings/alp/src/alp/compute/take.rs @@ -19,9 +19,6 @@ impl TakeExecute for ALP { ctx: &mut ExecutionCtx, ) -> VortexResult> { let taken_encoded = array.encoded().take(indices.clone())?; - // `Patches::take` carries the take indices' nullability into the patch values, so the - // taken patches already match `taken_encoded`'s nullability. Construct via `try_new` to - // assert that invariant instead of casting the values to force it. let taken_patches = array .patches() .map(|p| p.take(indices, ctx)) diff --git a/encodings/alp/src/alp_rd/array.rs b/encodings/alp/src/alp_rd/array.rs index 605b991d4c4..28ca30f909c 100644 --- a/encodings/alp/src/alp_rd/array.rs +++ b/encodings/alp/src/alp_rd/array.rs @@ -198,8 +198,6 @@ impl VTable for ALPRD { }; let right_parts = children.get(1, &right_parts_dtype, len)?; - // Patch values are read as the non-nullable left-parts dtype, which is exactly the dtype - // the array invariant requires, so no normalization is needed here. let left_parts_patches = metadata .patches .map(|p| { diff --git a/encodings/alp/src/alp_rd/compute/mask.rs b/encodings/alp/src/alp_rd/compute/mask.rs index a80b5d306a1..2773387c738 100644 --- a/encodings/alp/src/alp_rd/compute/mask.rs +++ b/encodings/alp/src/alp_rd/compute/mask.rs @@ -20,8 +20,6 @@ impl MaskReduce for ALPRD { EmptyOptions, [array.left_parts().clone(), mask.clone()], )?; - // The original exceptions are already the non-nullable left-parts dtype, which the - // (now nullable) masked array still requires, so they can be reused as-is. Ok(Some( ALPRD::try_new( array.dtype().as_nullable(), diff --git a/encodings/fastlanes/src/bitpacking/compute/take.rs b/encodings/fastlanes/src/bitpacking/compute/take.rs index a37ebbdff4f..34aa5a0c3f5 100644 --- a/encodings/fastlanes/src/bitpacking/compute/take.rs +++ b/encodings/fastlanes/src/bitpacking/compute/take.rs @@ -150,9 +150,6 @@ fn take_primitive( if let Some(patches) = array.patches() && let Some(patches) = patches.take(&indices.clone().into_array(), ctx)? { - // `take` carries the take indices' nullability into the patch values, matching - // `unpatched_taken`'s validity. `patch` (via `Validity::patch`) asserts the two agree, - // so no cast is required to align them. return unpatched_taken.patch(&patches, ctx); }