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.

[andygrove]

Thanks for working through all four of the earlier comments. The guard in CometDivide.getSupportLevel mirrors promote's pattern exactly, since DecimalExpression.unapply returns Some only when the dataType is decimal, so the guard covers precisely the shapes promotion would skip. The suite move is confirmed by CI, it now runs in the [exec] group on 3.4 through 4.2. The ANSI coverage across all five operators is the coverage I wanted, since the collapse guard compares nullOnOverflow. And the explicit promote(promote(e)) == promote(e) assertion states the property outright.

I also traced the removal of the serde wrapper for regressions and did not find one. No serde constructs a decimal Divide, so every decimal Divide reaching CometDivide.convert arrived through exprToProto and has been promoted. The end-to-end behavior is covered by the existing SQL file tests, decimal_div.sql has legacy overflow to null and decimal_div_ansi.sql has expect_error(NUMERIC_VALUE_OUT_OF_RANGE) for both / and div plus try_divide overflow to null. Those are the paths that would break if the i128::MAX sentinel leaked, and they pass.

#5248 is a good writeup. It correctly notes that not all 64 call sites should change, and that some serialize independent or synthesized roots.

CI is green apart from Spark SQL Tests (Spark 4.1) / spark-sql-sql_hive-2, which is HiveExternalCatalogVersionsSuite aborting on the mirror download. That is unrelated to this PR.

Two things from my earlier review are still open on the PR description.

First, the note about approach. This PR does the opposite of what #5190 proposed. It removes the serde wrapper and keeps promotion as the single owner, rather than removing the promotion wrapper. I think your choice is the better one, and it is what lets the same fix cover add, subtract, multiply, and remainder. But a reader comparing the issue to the diff will be confused without a sentence saying so.

Second, the numbers. I asked for a rough before and after over a decimal-division-heavy query from tpcds-micro-benchmarks/. If you have run one, please add it. If the difference turns out to be too small to measure reliably, that is a fine answer too, but then I would rather the description lead with the idempotency and single-ownership argument and drop the framing that the duplicate scan is a meaningful cost. Either resolution works, I just do not want the perf claim sitting there unbacked.

It would also help to link #5248 in the description, not only in the review thread, so it stays visible from the merged commit.

[andygrove]

One thing worth guarding against here. The re-promotion this test exercises only happens because CometArrayContains serializes its children through the public exprToProto. That is exactly what #5248 proposes to change. Once someone switches arrays.scala to exprToProtoInternal, the array_contains half of this test stops covering the duplicate-wrapper path and keeps passing silently.

Could you add a short comment naming that dependency and linking #5248, so whoever picks up that work knows this regression needs re-pointing? The promote(promote(e)) == promote(e) assertion below is independent and will still hold, so it is only the proto-shape half that is at risk.