perf: use Arrow cast for decimal rescale check - #5440
Conversation
sunchao
left a comment
There was a problem hiding this comment.
Summary
First review of a2f10a54ca137504a5320d1bf05647c26fdee4ca against bba47448ecddca5d6bf62e2ca843b64703cf734d. After five independent scopes covering semantics, compatibility/edge cases, routing/resources, tests/builds, and discussion deduplication, I found no verified, introduced [P1] or [P2] issue. Approving the implementation with the validation limitations below; the full JVM CI matrix is not green.
Prior state and problem
DecimalRescaleCheckOverflow duplicated decimal scaling, HALF_UP rounding, precision bounds, and overflow handling already implemented by Arrow. Its legacy array path wrote an i128::MAX sentinel, scanned for that sentinel, and conditionally performed another null-masking pass. Maintaining the custom array/scalar arithmetic alongside Arrow added both code and work on this path.
Design approach
The evaluator retains its Decimal128 type guard and delegates conversion to ColumnarValue::cast_to with the requested output precision/scale. safe: !fail_on_error maps legacy overflow to null and ANSI overflow to an error. This reuses the existing Arrow 58.4.0/DataFusion 54.1.0 dependencies without changing planner eligibility or adding dependency requirements.
Correctness / compatibility analysis
For valid input coefficients in the previously supported scale-delta range, the checked scale-up and signed HALF_UP scale-down paths agree with the removed implementation. Arrow's strict downscale fast-path condition still validates cases where rounding gains a digit. Scalar/array parity, nulls, non-byte-aligned slices, target metadata, child errors, and shared-buffer lifetimes were also checked. Metadata-dependent fast paths assume coefficients satisfy their declared source precision; synthetic violations were reproduced but no supported producer into this fusion was established.
Validation evidence:
- A local harness importing the unchanged expression passed all 15 tests; all 146 resolved registry dependency versions/checksums match this PR's lockfile. Separate probes, rerun during reconciliation, covered 167,504 metadata shapes / 31,498,960 input rows, 11,532 scalar/array comparisons, sliced/null arrays, and ownership/error cleanup. These are focused expression probes, not end-to-end Spark execution.
- The Rust CI job ran 900 tests successfully, with 4 skipped, including all 15 affected tests. Workspace Clippy and bench compilation passed; Linux/macOS native builds passed. The CI merge commit's tree exactly matches the reviewed head. Local formatting and whitespace checks passed.
- All 35 failing main-CI jobs stop at the same preexisting test-compilation error:
CometAggregateSuite.scala:318references absentCometConf.COMET_EXEC_SHUFFLE_ENABLED. The relevant test/config/shim blobs are identical at base and head. These failures provide no JVM runtime parity evidence. The separate Delta build gate also failed, but suppresses the underlying Maven diagnostic; its cause is unproven. Benchmark timing estimates were not independently rerun.
Key design decisions
The fusion remains limited to CheckOverflow(Cast(Decimal128)) where the cast target matches the check's target; ordinary arithmetic overflow wrappers remain in place. Evaluation now derives source precision/scale from the evaluated value, consistent with the child's type contract. Identity/widening conversions may share immutable Arrow buffers, while rescaling allocates output values; the ownership probes found no premature release or retained input reference on the tested error paths.
Implementation sketch
The patch removes the custom precision-bound helper, rescale helper, separate array/scalar branches, and sentinel/null-masking machinery. The new regression exercises 99.4, 99.5, and -99.5 converted to precision 2, scale 0: legacy returns 99, null, null, while ANSI reports overflow. Existing tests remain registered and executed, and the audit documentation is updated to describe the Arrow-backed implementation rather than the removed masking optimization.
Behavioral changes worth calling out
Legacy null-versus-ANSI-error behavior is preserved for the reviewed valid-input domain, but low-level Arrow error variants/messages can change. The old fused evaluator already used generic Arrow errors rather than ordinary CheckOverflow's structured Spark error/query-context path; this patch does not add that missing context. Large valid downward scale changes can now round to zero instead of the old blanket scale-delta error. Extreme negative-scale release behavior was not exercised end-to-end, and the reported quick benchmark speedups are not independently certified here.
Suggested improvements
No in-scope [P1] or [P2] code correction is requested. The remaining validation work is to repair the existing Scala test-compilation blocker separately, restore the JVM/Spark matrix, and obtain the Delta gate's underlying diagnostic before treating those checks as successful integration evidence. Any stronger performance claim should use repeated benchmark measurements; this approval does not certify the quick-mode point estimates.
Which issue does this PR close?
Closes #5094.
Rationale for this change
DecimalRescaleCheckOverflowduplicates Arrow's Decimal128 cast kernel, including rescaling, HALF_UP rounding, precision validation, and legacy overflow nulling. Reusing Arrow's single-pass implementation removes the duplicate arithmetic and sentinel/null-masking path while preserving legacy and ANSI behavior.What changes are included in this PR?
ColumnarValue::cast_towith ArrowCastOptions.How are these changes tested?
cargo test -p datafusion-comet-spark-expr decimal_rescale_check --lib(15 passed)cargo fmt --all -- --checkcargo clippy -p datafusion-comet-spark-expr --all-targets -- -D warningsgit diff --checkcargo bench -p datafusion-comet-spark-expr --bench decimal_rescale -- --quickThe quick Criterion run was used as a regression screen. Point estimates improved in all measured shapes, although the quick-mode samples were not statistically significant: