fix: use per-expression eval mode for decimal promotion

[peterxcli]

Which issue does this PR close?

Closes #5075.

Rationale for this change

DecimalPrecision.promote currently derives nullOnOverflow from the live session ANSI setting. Spark expressions capture their own evaluation mode, and one expression tree can contain mixed modes, such as a TRY operation nested inside an ANSI expression. Each generated CheckOverflow wrapper must therefore follow the evaluation mode of the arithmetic expression it guards.

What changes are included in this PR?

• Remove the session-derived nullOnOverflow argument from DecimalPrecision.promote.
• Derive nullOnOverflow from each decimal arithmetic node's evaluation mode.
• Add a Spark 4.1 regression test that verifies nested ANSI and TRY expressions serialize with independent overflow behavior.

How are these changes tested?

• make core
• ./mvnw test -Pspark-4.1 -Dtest=none -Dsuites=org.apache.spark.sql.comet.CometDecimalArithmeticViewSuite -Dscalastyle.skip=true (1 suite, 2 tests)
• ./mvnw scalastyle:check -Pspark-4.1
• ./mvnw spotless:apply -Pspark-4.1

[andygrove]

Thanks for picking this up. The change is correct and I traced through it to confirm the reasoning.

CometCheckOverflow.convert serializes failOnError = !expr.nullOnOverflow (math.scala:248), so nullOnOverflow = evalMode != EvalMode.ANSI becomes failOnError = evalMode == EvalMode.ANSI. That lines up with what CometDivide already does at arithmetic.scala:284 and with what WideDecimalBinaryExpr gets through eval_mode. EvalMode has three values and both LEGACY and TRY should return null on decimal overflow, so folding them together is right.

I also confirmed the bug stays latent, matching the analysis in #5075. For Plus / Minus / Multiply the native planner only builds WideDecimalBinaryExpr when operand precision can actually reach 38 (planner.rs:912-919), and there planner.rs:523-527 elides the CheckOverflow because the declared type always equals expr.dataType on both sides. Below that threshold the result precision cannot overflow. Divide has CometDivide's inner wrapper firing first, and Remainder is precision-exact. So there is no end-to-end repro available today and a proto-level unit test is the right shape of test here.

A few things I would like to see addressed before merge.

1. The new regression test does not run on Spark 4.2.

spark/pom.xml:538-539 adds only src/test/${shims.majorVerSrc} and src/test/${shims.minorVerSrc} as test sources. The spark-4.2 profile sets shims.minorVerSrc to spark-4.2 (pom.xml:747), and there is no spark/src/test/spark-4.2 directory, so everything under spark/src/test/spark-4.1/ is invisible to the 4.2 build. The whole motivation for this fix is the per-expression eval mode introduced in 4.1 and carried forward, so it would be good for the test to cover the newest supported Spark too.

Would you be up for adding src/test/${shims.minorPlusVerSrc} to the add-test-source list and moving CometDecimalArithmeticViewSuite to spark/src/test/spark-4.1+/? That picks up 4.2 and anything after it. The existing issue #4124 test gets the same benefit.

2. The TRY assertion in the new test is currently vacuous.

The test runs under SQLConf.ANSI_ENABLED -> "false". On the old code that produced nullOnOverflow = true for every wrapper, so assert(!tryAddProto.getCheckOverflow.getFailOnError) would have passed before the fix too. Only the outer assert(ansiOverflow.getFailOnError) actually catches the bug.

Could the test run the same expression tree under both ANSI_ENABLED -> "true" and "false" and assert the same two flags each time? That is really the property being fixed, that the session setting no longer participates at all, and it makes both assertions load-bearing in one direction or the other.

3. Only Add is covered, but the change touches five operators.

Would it be worth looping the new test over Add, Subtract, Multiply, Divide and Remainder? Divide is the interesting one, since CometDivide.convert emits its own CheckOverflow and the proto shape is a nested pair rather than a single wrapper. A regression that changed only the div case would slip past the test as written.

4. Please document why the flag comes from the expression.

The scaladoc on DecimalPrecision goes into real detail on why the target type must come from expr.dataType rather than SQLConf. #5075 makes the point that the old code reads as if the session flag were the right source. A sentence in that same comment saying nullOnOverflow follows each node's captured evalMode, because a TRY operation can be nested inside an ANSI expression, would stop the same thing being reintroduced later.

5. The Divide wrapper is now provably redundant, and I would like a tracking issue for it.

After this change promote wraps a decimal Divide in CheckOverflow(div, div.dataType, div.evalMode != ANSI), which serializes to failOnError = evalMode == ANSI with datatype div.dataType. CometDivide.convert (arithmetic.scala:281-288) then emits an inner CheckOverflow with exactly the same failOnError and the same datatype around the same node. The native dedup at planner.rs:523 only recognizes a WideDecimalBinaryExpr child, and the fusion at planner.rs:530 only recognizes a Cast child, so neither collapses a nested CheckOverflow. Every decimal division therefore runs an identical precision check twice per batch.

Before this PR the two flags could in principle disagree, so the duplication was arguably defensive. Now it is not. Cleaning it up is separate work, so could you open an issue and link it here rather than leaving it unrecorded?

[peterxcli]

@andygrove Thanks for the review!

1. The new regression test does not run on Spark 4.2.

• moved CometDecimalArithmeticViewSuite to spark/src/test/spark-4.1+/
• added src/test/${shims.minorPlusVerSrc} into pom.xml

2. The TRY assertion in the new test is currently vacuous.

added scala test

3. Only Add is covered, but the change touches five operators.

added Subtract, Multiply, Divide and Remainder scala tests.

4. Please document why the flag comes from the expression.

added comment.

5. The Divide wrapper is now provably redundant, and I would like a tracking issue for it.

filed #5190