Support wide decimals in DecimalBytePartsArray and kernels - #9809
Conversation
Merging this PR will regress 2 benchmarks
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ | Simulation | decompress[datetime_for_bp] |
160.3 µs | 193.9 µs | -17.32% |
| ❌ | WallTime | filtered_owned_i64_avx512[OneNullInEight] |
22.4 µs | 26.3 µs | -15.02% |
| ⚡ | Simulation | decompress[u64, (4000, 1024)] |
86.3 µs | 71.5 µs | +20.7% |
| ⚡ | Simulation | allocate_drop_arrow[0] |
456.9 ns | 402.7 ns | +13.45% |
| ⚡ | Simulation | chunked_bool_canonical_into[(1000, 10)] |
30.7 µs | 27.2 µs | +12.9% |
| ⚡ | Simulation | allocate_drop_bytes[0] |
575.7 ns | 521.6 ns | +10.39% |
| 🆕 | WallTime | dbp_assemble_neon[(I128, 1024)] |
N/A | 605 ns | N/A |
| 🆕 | WallTime | dbp_assemble_neon[(I128, 8192)] |
N/A | 3.7 µs | N/A |
| 🆕 | WallTime | dbp_assemble_neon[(I256, 1024)] |
N/A | 2.1 µs | N/A |
| 🆕 | WallTime | dbp_assemble_neon[(I256, 8192)] |
N/A | 15 µs | N/A |
| 🆕 | WallTime | dbp_assemble_neon[(I64, 1024)] |
N/A | 108 ns | N/A |
| 🆕 | WallTime | dbp_assemble_neon[(I64, 8192)] |
N/A | 111 ns | N/A |
| 🆕 | WallTime | dbp_split_all_null_neon[(I128, 1024)] |
N/A | 246 ns | N/A |
| 🆕 | WallTime | dbp_split_all_null_neon[(I128, 8192)] |
N/A | 251 ns | N/A |
| 🆕 | WallTime | dbp_split_all_null_neon[(I256, 1024)] |
N/A | 254 ns | N/A |
| 🆕 | WallTime | dbp_split_all_null_neon[(I256, 8192)] |
N/A | 257 ns | N/A |
| 🆕 | WallTime | dbp_split_all_null_neon[(I64, 1024)] |
N/A | 271 ns | N/A |
| 🆕 | WallTime | dbp_split_all_null_neon[(I64, 8192)] |
N/A | 287 ns | N/A |
| 🆕 | WallTime | dbp_split_all_valid_neon[(I128, 1024)] |
N/A | 767 ns | N/A |
| 🆕 | WallTime | dbp_split_all_valid_neon[(I128, 8192)] |
N/A | 4.2 µs | N/A |
| ... | ... | ... | ... | ... | ... |
ℹ️ Only the first 20 benchmarks are displayed. Go to the app to view all benchmarks.
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing mk/dbp-array (18b8b99) with mk/dbp-v2-feature (4edb7aa)
Footnotes
-
204 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
-
1 benchmark was run, but is now archived. If it was deleted in another branch, consider rebasing to remove it from the report. Instead if it was added back, click here to restore it. ↩
f7dfebe to
a566a28
Compare
DecimalBytePartsArray and kernels
Represent wide decimals with a signed high part and up to three unsigned low parts. Add validation, execution, kernel support, property tests, and assembly benchmarks while keeping serialization on the frozen format. Signed-off-by: Matt Katz <mhkatz97@gmail.com>
Move array helpers onto a crate-private extension trait, preserve decimal precision and scale when replacing the MSP, and group slicing with the other compute operations. Inline canonical execution and select scalar storage directly from the lower-part count. Signed-off-by: Matt Katz <mhkatz97@gmail.com>
Signed-off-by: Matt Katz <mhkatz97@gmail.com>
Signed-off-by: Matt Katz <mhkatz97@gmail.com>
Signed-off-by: Matt Katz <mhkatz97@gmail.com>
7283d4f to
a20094e
Compare
| .cast(array.msp().dtype().with_nullability(*target_nullability))?; | ||
|
|
||
| Ok(Some( | ||
| DecimalByteParts::try_new(new_msp, *target_decimal)?.into_array(), |
There was a problem hiding this comment.
I think this was redundant to begin with because we check above that target dtype is same as current modulo nullability
Signed-off-by: Matt Katz <mhkatz97@gmail.com>
Signed-off-by: Matt Katz <mhkatz97@gmail.com>
| divan::main(); | ||
| } | ||
|
|
||
| #[vortex_bench_support::cpu_features] |
There was a problem hiding this comment.
can you do this over the kernel working with [T] not vortex arrays. This is more noisy so it would be nice to only run if over small loops with allocs
|
|
||
| #[test] | ||
| fn test_cast_decimal_byte_parts_nullability() { | ||
| let mut ctx = array_session().create_execution_ctx(); | ||
| let decimal_dtype = DecimalDType::new(10, 2); | ||
| let array = | ||
| DecimalByteParts::try_new(buffer![100i32, 200, 300, 400].into_array(), decimal_dtype) | ||
| .unwrap(); | ||
|
|
||
| // Cast to nullable decimal | ||
| let casted = array | ||
| .into_array() | ||
| .cast(DType::Decimal(decimal_dtype, Nullability::Nullable)) | ||
| .unwrap(); | ||
| assert_eq!( | ||
| casted.dtype(), | ||
| &DType::Decimal(decimal_dtype, Nullability::Nullable) | ||
| ); | ||
|
|
||
| // Verify the values are preserved | ||
| let decoded = casted.execute::<DecimalArray>(&mut ctx).unwrap(); | ||
| assert_eq!(decoded.len(), 4); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_cast_decimal_byte_parts_nullable_to_non_nullable() { | ||
| let mut ctx = array_session().create_execution_ctx(); | ||
| let decimal_dtype = DecimalDType::new(10, 2); | ||
| let array = DecimalByteParts::try_new( | ||
| PrimitiveArray::from_option_iter([Some(100i32), None, Some(300)]).into_array(), | ||
| decimal_dtype, | ||
| ) | ||
| .unwrap(); | ||
|
|
||
| // Cast to non-nullable should fail due to nulls - force evaluation via execute::<Canonical> | ||
| let result = array | ||
| .into_array() | ||
| .cast(DType::Decimal(decimal_dtype, Nullability::NonNullable)) | ||
| .and_then(|a| a.execute::<Canonical>(&mut ctx).map(|c| c.into_array())); | ||
| assert!(result.is_err()); | ||
| } | ||
|
|
||
| #[rstest] | ||
| #[case::i32(DecimalByteParts::try_new( | ||
| buffer![100i32, 200, 300, 400, 500].into_array(), | ||
| DecimalDType::new(10, 2), | ||
| ).unwrap())] | ||
| #[case::i64(DecimalByteParts::try_new( | ||
| buffer![1000i64, 2000, 3000, 4000].into_array(), | ||
| DecimalDType::new(19, 4), | ||
| ).unwrap())] | ||
| #[case::nullable(DecimalByteParts::try_new( | ||
| PrimitiveArray::from_option_iter([Some(100i32), None, Some(300), Some(400), None]) | ||
| .into_array(), | ||
| DecimalDType::new(10, 2), | ||
| ).unwrap())] | ||
| #[case::single(DecimalByteParts::try_new( | ||
| buffer![42i32].into_array(), | ||
| DecimalDType::new(5, 1), | ||
| ).unwrap())] | ||
| #[case::negative(DecimalByteParts::try_new( | ||
| buffer![-100i32, -200, 300, -400, 500].into_array(), | ||
| DecimalDType::new(10, 2), | ||
| ).unwrap())] | ||
| #[case::one_lower_part(i128_parts( | ||
| vec![1i128 << 70, -(1i128 << 70), 5, (1i128 << 64) - 1, 0], | ||
| Validity::NonNullable, | ||
| ))] | ||
| #[case::three_lower_parts(i256_parts( | ||
| vec![i256_of(1, 0), i256_of(-1, 5), i256_of(0, u128::MAX)], | ||
| Validity::NonNullable, | ||
| ))] | ||
| fn test_cast_decimal_byte_parts_conformance(#[case] array: DecimalBytePartsArray) { |
There was a problem hiding this comment.
we don't need this many new tests
| // The MSP alone only determines the ordering when it holds the whole value. With | ||
| // lower parts present, fall back to comparing the canonical decimal. | ||
| if !lhs.lower_parts().is_empty() { | ||
| return Ok(None); | ||
| } |
There was a problem hiding this comment.
add a todo saying we could be smarter here
| impl TakeReduce for DecimalByteParts { | ||
| fn take(array: ArrayView<'_, Self>, indices: &ArrayRef) -> VortexResult<Option<ArrayRef>> { | ||
| // Taking with nullable indices makes every taken part nullable, but lower parts must | ||
| // stay non-nullable `u64` — validity belongs to the MSP alone. Fall back to the | ||
| // canonical path rather than rebuilding parts we would have to strip nullability from. | ||
| if indices.dtype().is_nullable() && !array.lower_parts().is_empty() { | ||
| return Ok(None); | ||
| } | ||
|
|
||
| array | ||
| .map_parts(|part| part.take(indices.clone())) | ||
| .map(|a| Some(a.into_array())) | ||
| } | ||
| } |
There was a problem hiding this comment.
add a todo saying we could impl this using fill null or smthing else
| let mut value = T::from(msp).vortex_expect("MSP fits in the output type"); | ||
| for part in lower { | ||
| value = (value << LOWER_PART_BITS) | ||
| | T::from(part).vortex_expect("lower word fits in the output type"); |
There was a problem hiding this comment.
Can we remove this failure it will likely break simd. Can this ever fail. It should be checked outside the loop once and not checked here?
There was a problem hiding this comment.
Can add infallible upcast from u64/i64 to i256 instead
DecimalBytePartsArraypreviously stored the entire unscaled decimal value in one signed integer child, limiting it to values that fit in 64 bits. It now supports wide decimals by representing each value as integer parts that can be compressed independently, while preserving the decimal's logical precision, scale, and nullability.The array has a signed most significant part (MSP) and up to three unsigned 64-bit lower parts, ordered most significant first. Splitting canonical decimal storage produces:
i8/i16/i32/i64i128i64MSP + oneu64lower parti256i64MSP + threeu64lower partsOnly the MSP carries validity. Every lower part must be a non-nullable
u64array with the same length, and splitting wide decimals zeroes the parts at null positions. All children remainArrayRefs, so their individual encodings are independent of the decimal representation.execute::<DecimalArray>reassembles aDecimalArrayfrom the MSP and lower parts children.takewith nullable indices is not yet supported by the DecimalByteParts kernel for arrays with lower parts; it falls back to canonical execution. Taking each part directly would make the lower parts nullable, violating the representation's invariant. The frozen serializer also continues to reject arrays with lower parts.