[andygrove]

Which issue does this PR close?

Part of #5199 (item 4: DecimalPrecision.promote is a second full traversal of every expression tree).

Rationale for this change

QueryPlanSerde.exprToProto calls DecimalPrecision.promote on every expression it converts, and
promote is a TreeNode.transformUp. transformUp walks the whole tree and calls mapChildren at
every node, which allocates through mapProductIterator regardless of whether any case matches. The
result is a full extra traversal of every expression tree before the serde walks it again, paid on
the driver for every query and again for every query stage under AQE.

The rule only ever rewrites arithmetic that produces a decimal, so on the overwhelming majority of
expressions the traversal produces the identical tree back.

Measured with a throwaway probe over decimal-free expression trees of varying size (Spark 4.1 /
JDK 17, 20k iterations per measurement, best of 5 after warm-up):

tree nodes	promote before	promote after	share of exprToProto before	after
22	0.95 us	0.33 us	4.6%	1.3%
67	2.88 us	0.97 us	7.1%	2.6%
247	11.36 us	3.53 us	8.4%	2.8%
967	45.39 us	18.91 us	8.7%	3.9%

Trees that do contain decimal arithmetic pay the guard traversal on top of the rewrite, but that
traversal short-circuits at the first match and does not allocate.

What changes are included in this PR?

Adds a containsDecimalArithmetic guard and only runs the transformUp when it holds.

The guard is deliberately a superset of the rule: it matches Add / Subtract / Multiply /
Divide / Remainder whose dataType is a DecimalType, ignoring the operands that the rule's
own patterns check. It is sound to decide this up front because the rewrite only wraps nodes in
CheckOverflow, and CheckOverflow's data type is its child's, so no node's data type changes
part way through the transformUp and a tree without decimal arithmetic cannot grow any.

Behavior is unchanged: transformUp already returns the input tree by identity when no case
matches, so the guard removes work rather than changing the result.

How are these changes tested?

Existing coverage: CometExpressionSuite (140 tests) and the Spark 4.1
CometDecimalArithmeticViewSuite regression tests for #4124 and #5075 all pass.

Added DecimalPrecisionSuite, which pins the rule's behavior directly and is version-agnostic
(the existing unit coverage was Spark 4.1 only). It asserts that trees without decimal arithmetic
come back by identity, that each of the five arithmetic operators is wrapped in CheckOverflow
with the operator's own dataType as the target, that a decimal operator buried under nodes the
rule does not rewrite is still promoted, and that a mixed tree has only its decimal arithmetic
rewritten. These pass both with and without this change, which is the point: they document that
the guard is not observable.

The probe used for the numbers above was throwaway and is not included.

[0lai0]

Thanks @andygrove looks good to me

[0lai0]

Nit: Could you reword this comment slightly?
transformUp does call mapChildren at every node, but specialized mapChildren (UnaryLike / BinaryLike / etc.) skip the copy via fastEquals when nothing changed , so it doesn't really "rebuild every node".
The cost is the traversal + allocations (children.map / mapProductIterator).
The PR body's wording already captures this well; matching that here would help future profilers.

[andygrove]

Reworded in 4e0502e — you're right, "rebuilds every node" overstates it and points a future profiler at the wrong thing.

// `transformUp` walks the whole tree and calls `mapChildren` at every node, which allocates
// through `children.map` / `mapProductIterator` even when no case matches (the specialized
// `mapChildren` overrides do skip the node copy via `fastEquals`, so the cost is the traversal
// and its allocations, not a rebuild). The serde calls this once per expression it converts,
// so skip it when there is nothing to rewrite.

That now says the same thing as the PR body, and naming fastEquals explicitly should stop the next reader concluding there is a copy to eliminate.

[sunchao]

Summary

• Prior state and problem: DecimalPrecision.promote ran transformUp for every serialized expression, including trees without decimal arithmetic.
• Design approach: Add a short-circuiting exists guard before invoking the existing rewrite.
• Correctness / compatibility analysis: Compared Spark sources and relevant tests for 3.4.3, 3.5.9, 4.0.4, 4.1.3, and 4.2.0. The guard covers all five arithmetic-wrapping cases and preserves their result types and captured evaluation modes. No introduced P1/P2 issues found within this review.
• Key design decisions: Keeping the rewrite intact limits complexity. Decimal-containing trees incur an additional search that stops at the first qualifying operator. The reported benchmark gains were not independently reproduced.
• Implementation sketch: Adds the guard, three structural regression tests, and suite registration in both Linux and macOS workflows.
• Behavioral changes worth calling out: Decimal-free trees return directly. Qualifying trees retain the existing decimal-promotion path. No observable result regression was identified.
• Suggested improvements: None at the P1/P2 threshold.

Reviewed the entire four-file diff from 7f1e00189b1ed86f1cb5acd872d97fce694482b1 to 4e0502ef3fdd1dbf9bb2f5515ef6cd01aba2b695. Confirmed the PR is not a draft. Read the supplied reviews, comments, and threads. No substantiated existing P1/P2 blocker remains.

Routed skills: review-comet-pr and applicable audit-comet-expression guidance.

Exact-head CI: 4 successful checks, 1 cancelled, and 13 skipped. Preflight exceeded its 15-minute limit, leaving Linux/macOS builds, Spark SQL tests, Iceberg tests, and benchmarks skipped. Successful checks were CodeQL, Actions analysis, Delta build-gate verification, and labeling.

Validation: python dev/ci/check-suites.py and the base-relative git diff --check passed. Scala/native suites were not run locally: build artifacts and Spark dependencies are absent, and Maven Central access is blocked. Runtime compatibility and performance were therefore not independently validated.

[andygrove]

Thanks @0lai0 and @sunchao