fix: avoid duplicate CheckOverflow evaluation for decimal division

[peterxcli]

Which issue does this PR close?

Closes #5190.

Rationale for this change

DecimalPrecision.promote and CometDivide.convert both wrapped decimal Divide expressions in equivalent CheckOverflow nodes. The native planner does not collapse that shape, so successful decimal division could perform the same precision scan twice.

Promotion can also run more than once when serializers such as ArrayContains recursively call the public exprToProto API. If decimal overflow wrapping is owned by DecimalPrecision.promote, the transformation therefore needs to be idempotent.

What changes are included in this PR?

• Make DecimalPrecision.promote collapse adjacent equivalent CheckOverflow wrappers around decimal binary arithmetic while preserving wrappers with different result types or overflow modes.
• Remove the specialized decimal CheckOverflow creation from CometDivide.convert, keeping overflow wrapping centralized in decimal promotion.
• Add a real-SQL proto regression through array_contains covering decimal Add, Subtract, Multiply, Divide, and Remainder, including LEGACY, TRY, and ANSI divide modes.
• Update the existing per-expression eval-mode regression to expect a single decimal Divide wrapper.

How are these changes tested?

• /mvnw test -Pspark-4.1 -Dtest=none -Dsuites="org.apache.spark.sql.comet.CometDecimalArithmeticViewSuite"
• ./mvnw test -Pspark-4.2 -Dtest=none -Dsuites="org.apache.spark.sql.comet.CometDecimalArithmeticViewSuite"
• ./mvnw test -Pspark-3.4 -Dtest=none -Dsuites="org.apache.comet.CometSqlFileTestSuite decimal_div"
• ./mvnw test -Pspark-4.2 -Dtest=none -Dsuites="org.apache.comet.CometSqlFileTestSuite decimal_div"
• git diff --check

[andygrove]

Thanks for picking this up. I think the direction here is better than what the issue proposed. Centralizing the wrapper in DecimalPrecision.promote also fixes the same duplication for Add, Subtract, Multiply, and Remainder, which #5190 never mentioned.

I checked the native side to make sure removing the serde wrapper is safe. Decimal Divide plans to a decimal_div ScalarFunctionExpr rather than a WideDecimalBinaryExpr, so the surviving CheckOverflow is never eliminated by the fast path in planner.rs. The i128::MAX sentinel handling is intact. I also worked through the collapse rule against transformUp and it converges at any nesting depth, since each level collapses on the way up.

A few things I would like to see addressed before this merges. I left them inline, plus two notes on the PR description below.

The rationale is that the duplicate wrapper does the precision scan twice on every successful decimal division batch. Could you add a rough before and after number to confirm the win is real? Something from tpcds-micro-benchmarks/ over a decimal-division-heavy query would be enough.

It would also be worth noting in the description that you took the opposite approach from the one proposed in #5190, and why. I think your choice is the better one, but a reader comparing the issue to the diff will be confused without that note.

CI is fully green across 3.4, 3.5, 4.0, 4.1, 4.2, TPC-DS, TPC-H, and rust-test.

[andygrove]

One thing I want to check here. The guard you removed fired on expr.dataType.isInstanceOf[DecimalType] alone. The promote rule requires both operands to match DecimalExpression as well, and getSupportLevel only looks at expr.left.dataType. So if a decimal-typed Divide with a non-decimal operand ever reaches this serde, there is no CheckOverflow anywhere and the native i128::MAX sentinel comes back as a real value.

I could not construct that shape through Spark's type coercion, so I believe it is unreachable today. Since the failure mode is silent wrong data rather than a fallback, would you be up for a defensive guard? Returning Unsupported from getSupportLevel when expr.dataType is decimal but the operands would not match promote's pattern would keep the invariant checkable next to the code that depends on it.

[peterxcli]

added a sanity check.

[andygrove]

This is a good fix, and the bottom-up collapse converges correctly at any depth.

The re-promotion it works around comes from serdes calling the public exprToProto on children that promote has already visited. There are 64 such call sites in the serde package. The aggregate ones genuinely need the promoting entry point, but the ones in arrays.scala and bitwise.scala are re-walking an already-promoted tree, and each walk drags liftCoverageTags along with it. That is separate work from this PR, but could you file a tracking issue for it and link it here? Otherwise it will not get picked up.

[peterxcli]

filed. #5248

[andygrove]

This regression does not depend on anything 4.1-specific, but sitting under spark-4.1+ means 3.4, 3.5, and 4.0 CI never runs it, and the duplication it guards against exists on all of them. Could it move to a version-agnostic suite under spark/src/test/scala/org/apache/spark/sql/comet/? A new CometDecimalPromotionSuite would work. The other two tests in this file can stay where they are since they need NumericEvalContext.

While it is moving, two additions would be worth it. ANSI is only exercised for divide, but the collapse guard compares nullOnOverflow, so ANSI is exactly the case that would catch a wrong guard for add, subtract, multiply, and remainder. And a direct assertion that promote(promote(e)) == promote(e) would state the idempotency property outright instead of leaving it to be inferred from the array_contains proto shape.

[peterxcli]

• moved to CometDecimalPromotionSuite.
• make ANSI also exercise for add, subtract, multiply, and remainder
• addded promote(promote(e)) == promote(e)

[andygrove]

This tuple has two unlabeled booleans in it, and ("divide TRY", true, ..., false) is hard to read without counting positions back to the destructuring on the next block. A small case class with named fields, or moving ansiEnabled and failOnError next to their names, would make this easier to extend when someone adds a case later.

[peterxcli]

refactored.