Skip to content

[SPARK-56983][SQL] DecimalAggregates widened-Cast peel must preserve query semantics#56036

Open
cloud-fan wants to merge 3 commits into
apache:masterfrom
cloud-fan:SPARK-decimal-aggregates-semantics-fix
Open

[SPARK-56983][SQL] DecimalAggregates widened-Cast peel must preserve query semantics#56036
cloud-fan wants to merge 3 commits into
apache:masterfrom
cloud-fan:SPARK-decimal-aggregates-semantics-fix

Conversation

@cloud-fan
Copy link
Copy Markdown
Contributor

@cloud-fan cloud-fan commented May 21, 2026

What changes were proposed in this pull request?

Two semantic-preserving fixes to the SPARK-56627 widened-Cast peel in
DecimalAggregates:

  1. SUM arm: Replace Cast(MakeDecimal(_, p + 10, s), bounded(pPrime + 10, s))
    with MakeDecimal(_, min(pPrime + 10, 38), s). The merged form's inner
    MakeDecimal narrowed the overflow check to 10^(p+10), where the
    un-rewritten SUM(Cast(x, dec(pPrime, s))) accepted up to
    10^min(pPrime+10, 38). For pPrime + 10 > 18, Decimal.setOrNull falls
    into the BigDecimal branch and never rejects a Long, so the cleaner form
    preserves the original overflow boundary for all Long-fit sums.

  2. AVG arm: Change the guard from p <= 7
    (AVG_PEEL_MAX_INNER_PRECISION) to pPrime + 4 <= 15
    (MAX_DOUBLE_DIGITS). The merged guard switched AVG(CAST(x AS DECIMAL(pPrime, s)))
    from full Decimal arithmetic to Double-regime whenever p <= 7, including
    the pPrime > 11 band where un-rewritten was Decimal-exact. The new guard
    ensures the rewrite only fires inside the existing AVG fast-path's Double
    envelope.

Why are the changes needed?

A query rewrite should preserve the query's observable semantics. The
SPARK-56627 rule changed semantics in two ways:

  • SUM: Inner MakeDecimal(_, p + 10, s) could return null (non-ANSI) or
    throw (ANSI) for long-fit sums in `(10^(p+10), 10^min(pPrime+10, 38))`.
    Example: `SUM(CAST(x AS DECIMAL(15, 2)))` where `x: DECIMAL(5, 2)`
    rejected at `10^15` instead of `10^25` -- reachable around `~10^10`
    rows of small-precision input.
  • AVG: For `pPrime > 11, p <= 7`, the rule switched an exact-Decimal
    computation into Double-regime, visible as last-digit rounding differences
    at any input size. TPC-DS q18 (`p=7, pPrime=12`) was affected.

Does this PR introduce any user-facing change?

Yes -- restores the un-rewritten semantics for affected queries:

  • `SUM(CAST(x AS DECIMAL(pPrime, s)))` no longer rejects long-fit sums
    that the un-rewritten expression would have accepted.
  • `AVG(CAST(x AS DECIMAL(pPrime, s)))` with `pPrime + 4 > MAX_DOUBLE_DIGITS`
    is no longer peeled -- the un-rewritten Decimal-exact path is preserved.
    TPC-DS q18 AVG aggregations revert to the un-peeled form (visible in the
    regenerated `q18` plan-stability files).

How was this patch tested?

  • `DecimalAggregatesSuite`: 37/37 pass, including updated SUM arm shape
    assertions, the property-test invariants for the new MakeDecimal-only
    form, and the new AVG bound boundary tests.
  • `DataFrameAggregateSuite`: SPARK-56627 numerical-equivalence PBTs pass
    under the new AVG generator domain (`pPrime in [p+1, 11]`).
  • TPC-DS q18 plan-stability files regenerated locally.
  • `DecimalAggregatesBenchmark` AVG cases (B2, B4) updated to `pPrime = 11`
    (the new bound). Result files left stale relative to the code change --
    the `benchmark.yml` GHA workflow can regenerate them.

Was this patch authored or co-authored using generative AI tooling?

Generated-by: Claude Opus 4.7

cloud-fan added 3 commits May 21, 2026 11:38
@cloud-fan
[SPARK-56983][SQL] DecimalAggregates widened-Cast peel must preserve …
d8b93cd 
…query semantics

The SPARK-56627 widened-Cast peel changed observable query semantics in two
ways. Fix both so the rewrite never diverges from the un-rewritten form
beyond what the existing un-widened arms already accept.

* SUM arm: replace
    Cast(MakeDecimal(_, p + 10, s), bounded(pPrime + 10, s))
  with
    MakeDecimal(_, min(pPrime + 10, MAX_PRECISION), s).
  The merged form narrowed MakeDecimal's overflow check to 10^(p+10),
  rejecting long-fit sums in (10^(p+10), 10^min(pPrime+10, 38)) that the
  un-rewritten expression would have accepted. For target precision > 18
  Decimal.setOrNull falls into the BigDecimal branch and never rejects a
  Long, so the cleaner form preserves the original overflow boundary.

* AVG arm: change the guard from `p <= 7` to `pPrime + 4 <= 15`. The merged
  guard converted exact-Decimal AVG into Double-regime for pPrime > 11,
  introducing rounding divergences. The new guard only fires inside the
  existing AVG fast-path envelope, so cases the un-widened arm wouldn't
  optimize stay on the slow-but-exact path.

TPC-DS q18 AVG aggregations (pPrime = 12) revert to the un-peeled form
under the new bound; plan-stability files regenerated accordingly.

Co-authored-by: Isaac
@cloud-fan
address self-review: drop stale SPARK-56627 framing from new code
8f9afaf 
- DecimalAggregatesBenchmark.scala header: replace AVG_PEEL_MAX_INNER_PRECISION
  reference with the actual `pPrime + 4 <= MAX_DOUBLE_DIGITS` guard; split the
  case-design rationale per section so Section B is described accurately
  ("p+1 boundary plus pPrime upper bound", not "p+10-class wider").
- DecimalAggregatesSuite.scala: rename the (F1 guard) test and reword its
  comment block so it stands on its own without the F-prefixed shorthand
  carried over from the original SPARK-56627 PR.

Co-authored-by: Isaac
@cloud-fan
[SPARK-56983][SQL][FOLLOWUP] Self-review polish on DecimalAggregates …
32b2714 
…peel

- Couple SUM MakeDecimal target type directly to `DecimalType.bounded(pPrime
  + 10, s_scale)` instead of restating its precision-clamp formula, so the
  link to `Sum.resultType` is explicit.
- Drop the now-tautological `retryUntil(...)` from the SUM PBT `peelGen`;
  the `Gen.choose` bounds already enforce the peel-firing predicate.
- Refresh the stale `DecimalType.bounded(pPrime+4, s+4)` comment in the AVG
  numerical-equivalence PBT to match the rewrite's plain `DecimalType(...)`.

Co-authored-by: Isaac
@cloud-fan
Copy link
Copy Markdown
Contributor Author

cloud-fan commented May 21, 2026

cc @yaooqinn @peter-toth

yaooqinn
yaooqinn approved these changes May 21, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@yaooqinn yaooqinn left a comment

Choose a reason for hiding this comment

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

Both semantics claims ground out:

  • SUM arm: old Cast(MakeDecimal(_, p+10, s), bounded(pPrime+10, s)) narrows the long-fit acceptance window via Decimal.setOrNull(unscaled, p+10, s) (Decimal.scala L96, long branch). For pPrime+10 > 18 un-rewritten accepts up to 10^min(pPrime+10, 38); the new MakeDecimal(_, min(pPrime+10, 38), s) falls into the BigDecimal branch and matches that boundary.
  • AVG arm: old guard p <= 7 could fire when un-rewritten pPrime + 4 > MAX_DOUBLE_DIGITS was Decimal-exact, switching the result regime. New guard pPrime + 4 <= MAX_DOUBLE_DIGITS keeps the rewrite inside the un-rewritten Double envelope. q18 plan-stability diff confirms the four decimal(7,2)→(12,2) AVG sites correctly revert to the un-peeled form.

Pattern ordering preserved (widened arm before un-widened, both Sum and Average); MakeDecimal nullOnOverflow path unchanged.

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

Reviewers

@peter-toth peter-toth peter-toth approved these changes

@yaooqinn yaooqinn yaooqinn approved these changes

@dongjoon-hyun dongjoon-hyun dongjoon-hyun approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@cloud-fan @peter-toth @yaooqinn @dongjoon-hyun