Skip to content

perf: use Arrow cast for decimal rescale check - #5440

Open
peterxcli wants to merge 2 commits into
apache:mainfrom
peterxcli:refactor/decimal-rescale-arrow-cast
Open

perf: use Arrow cast for decimal rescale check#5440
peterxcli wants to merge 2 commits into
apache:mainfrom
peterxcli:refactor/decimal-rescale-arrow-cast

Conversation

@peterxcli

@peterxcli peterxcli commented Aug 23, 2026

Copy link
Copy Markdown
Member

Which issue does this PR close?

Closes #5094.

Rationale for this change

DecimalRescaleCheckOverflow duplicates 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?

  • Delegate Decimal128 rescaling and precision checks to ColumnarValue::cast_to with Arrow CastOptions.
  • Remove the hand-written rescale, precision-bound, overflow-sentinel, and null-masking logic.
  • Add coverage for scale-down rounding that crosses the output precision boundary in legacy and ANSI modes.
  • Update the expression audit documentation for the Arrow-backed implementation.

How are these changes tested?

  • cargo test -p datafusion-comet-spark-expr decimal_rescale_check --lib (15 passed)
  • cargo fmt --all -- --check
  • cargo clippy -p datafusion-comet-spark-expr --all-targets -- -D warnings
  • git diff --check
  • cargo bench -p datafusion-comet-spark-expr --bench decimal_rescale -- --quick

The 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:

Shape Before After Speedup
Scale up, no overflow 241.92 us 39.708 us 6.09x
Scale up, no overflow, nulls 480.80 us 45.135 us 10.65x
Scale down, no overflow 999.03 us 42.780 us 23.35x
Sparse overflow 374.10 us 36.719 us 10.19x
Dense overflow 364.63 us 38.451 us 9.48x
ANSI scale up, no overflow 155.48 us 69.466 us 2.24x

@peterxcli peterxcli changed the title refactor: use Arrow cast for decimal rescale check perf: use Arrow cast for decimal rescale check Aug 23, 2026

@sunchao sunchao left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:318 references absent CometConf.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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

decimal_rescale_check: replace fused rescale and precision check with arrow decimal cast

2 participants