Decimal Add/Sub kernels#8724
Conversation
Merging this PR will degrade performance by 20.42%
Warning Please fix the performance issues or acknowledge them on CodSpeed. Performance Changes
Tip Investigate this regression by commenting Comparing 
Footnotes
|
mhk197
added
changelog/feature
|
Verdict: Likely regression (high confidence) How to read Verdict and Engines
datafusion / vortex-file-compressed (1.262x ❌, 0↑ 2↓)
datafusion / parquet (0.989x ➖, 0↑ 0↓)
duckdb / vortex-file-compressed (1.102x ❌, 0↑ 1↓)
duckdb / parquet (1.019x ➖, 0↑ 0↓)
No file size changes detected. |
Mechanical move mirroring the compare split: dtype-generic lane machinery stays in numeric/mod.rs (bounds relaxed to T: Default so wider decimal types can reuse it), primitive-specific code moves to numeric/primitive.rs, tests to numeric/tests.rs. Signed-off-by: Matt Katz <mhkatz97@gmail.com>
Add and Sub over decimal arrays sharing a decimal dtype, applied to the unscaled stored integers (exact at a shared scale) in a working width wide enough that in-precision inputs cannot spuriously overflow. Precision violations error only on valid lanes, matching primitive semantics. Mul and Div need rescaling and are gated off until follow-up PRs. widened_buffer moves to arrays/decimal so compare and numeric share it. Signed-off-by: Matt Katz <mhkatz97@gmail.com>
The conformance harness now accepts decimal arrays, checking Add/Sub results element-wise against DecimalScalar::checked_binary_numeric for representative constants. Wire it into the decimal, chunked, and decimal-byte-parts compute tests, and add decimal Add cases to the binary_ops benchmark. Signed-off-by: Matt Katz <mhkatz97@gmail.com>
Signed-off-by: Matt Katz <mhkatz97@gmail.com>
Signed-off-by: Matt Katz <mhkatz97@gmail.com>
6ed525c to
df4f7b6
Compare
Polar Signals Profiling ResultsLatest Run
Previous Runs (1)
Powered by Polar Signals Cloud |
## Rationale for this change - Closes: #8807 Three benchmarks stayed flaky after #8742, flipping between the same two values on PRs that can't affect them. Same root causes and fixes as #8742: | Benchmark | Seen flaky on | Why | Fix | | --- | --- | --- | --- | | `true_count_vortex_buffer[128]` | ±11.17% on 9 unrelated PRs (#8805, #8811, #8812, #8820, #8843, #8803, …) | a 128-bit popcount measures harness overhead and code layout, not the count | drop the 128 size | | runend `compress[(100000, 4)]` | ±11.9% on #8805, #8750, #8856 | allocates in the timed region; glibc malloc differs across runner images | mimalloc as global allocator | | `cast_decimal` `copy_*[65536]` | identical flags on #8838 and #8724 | same glibc-malloc cause (512 KB alloc per iteration) | mimalloc as global allocator | Left alone: `compact_sliced[(4096, 90)]` (single sighting) and the CUDA walltime benches (hosted-runner walltime noise, a runner config issue). The allocator swap shifts every benchmark in the two touched binaries once — see the comment below. Needs a one-time CodSpeed acknowledgment, like #8742. ## What changes are included in this PR? One commit per benchmark; bench files only. Ran `cargo check` + `clippy` on the three bench targets, smoke-ran the binaries, `cargo +nightly fmt`. --------- Signed-off-by: Claude <noreply@anthropic.com> Co-authored-by: Claude <noreply@anthropic.com>
| #[inline(always)] | ||
| fn apply<W>(lhs: W, rhs: W, plan: &DecimalValueBounds<W>) -> Option<W> | ||
| where | ||
| W: NativeDecimalType + CheckedAdd + CheckedSub, |
There was a problem hiding this comment.
Maybe we should add CheckedAdd + CheckedSub to the trait bounds of NativeDecimalType now?
| pub(super) fn checked_valid_lanes<T, F>( | ||
| len: usize, | ||
| valid_bits: &BitBuffer, | ||
| mut checked_at: F, | ||
| ) -> CheckedValues<T> | ||
| where | ||
| T: Default, | ||
| F: FnMut(usize) -> Option<T>, | ||
| { | ||
| let mut values = BufferMut::<T>::zeroed(len); | ||
| let mut failed = false; | ||
| { | ||
| let values = values.as_mut_slice(); | ||
| for_each_valid_idx(len, valid_bits, |idx| { | ||
| let Some(value) = checked_at(idx) else { | ||
| failed = true; | ||
| return false; | ||
| }; | ||
| values[idx] = value; | ||
| true | ||
| }); | ||
| } | ||
|
|
||
| CheckedValues { | ||
| values: values.freeze(), | ||
| failed, | ||
| } | ||
| } |
There was a problem hiding this comment.
This (
) might help here. I spent a little bit of time trying to ensure that LLVM will SIMD this even with failuresThere was a problem hiding this comment.
If what you have here is faster we would unify these.
|
This is just what my codex review said: Details• The arithmetic implementation works for ordinary valid arrays, but it breaks legitimate DataFusion filter pushdown at decimal storage-width boundaries and can silently accept out-of-precision input lanes. Both are observable correctness failures. Full review comments:
Maybe this is an existing issue? |
connortsui20
left a comment
connortsui20
left a comment
There was a problem hiding this comment.
seems fine to me, I dont think that codex review is real
4 participants
Adds shared infrastructure for decimal arithmetic and native Add/Sub kernels.
Operand alignment
The execution kernel requires both operands to have the same precision and scale (nullability may differ). Addition and subtraction can then operate directly and exactly on the unscaled integers because both values use the same power-of-ten factor.
Vortex's optional coercion pass can align operands with different decimal dtypes before execution. Query-engine integrations may instead insert their own planner casts. If neither layer aligns the operands, the kernel rejects them rather than performing an implicit per-kernel rescale.
Result precision
Adding or subtracting two
p-digit integers can require one additional carry digit. This PR follows the Arrow/Hive result-type rule for already aligned operands, capped at Vortex's maximum decimal precision:The scale remains unchanged because the operation is performed on integers at the same scale. For example:
Widening the logical precision can also widen the native storage used for the result. The kernel executes at the smallest native width capable of representing the result dtype, including the important
decimal(18, s) -> decimal(19, s)(i64 -> i128) anddecimal(38, s) -> decimal(39, s)(i128 -> i256) transitions. Inputs may already use a wider physical representation; they are converted into the selected working width before arithmetic.At precision 76 the result dtype cannot widen to precision 77, so it stays at precision 76 and the kernel explicitly checks the mathematical result against the precision-76 bound.
Nulls, constants, and errors
Implementation
numeric/{mod,primitive,decimal,tests}.rs, mirroring the compare implementation.execute::<DecimalArray>, while extracting constant and null operands directly.widened_bufferfromarrays/decimalbetween comparison and numeric execution.Comparative performance
Measured over arrays with ~60k elements, using same result width in Vortex and Arrow.
i32, non-nulli64, non-nulli128, non-nulli256, nullable