LuciferYang opened a new pull request, #13002:
URL: https://github.com/apache/gluten/pull/13002
## What changes are proposed in this pull request?
Spark deleted the `PromotePrecision` Catalyst expression in SPARK-39316,
which landed in 3.4. Gluten kept four byte-identical passthrough stubs under
`shims/spark{34,35,40,41}` so one codebase could serve 3.3 and 3.4+. #12902
removed Spark 3.3, so the class is now Gluten's own and nothing in the
repository constructs it: the only `withNewChildren` on it lives inside
`rescaleCastForOneSide`, which is reachable only from a match on
`PromotePrecision` itself. That makes the three rewrites
`DecimalArithmeticUtil` grew around it identity functions and the two
`ExpressionConverter` arms that matched it dead code.
Removed: the four stub classes; `rescaleLiteral`, `rescaleCastForDecimal`,
`removeCastForDecimal` and the five helpers only they called;
`SparkShims.widerDecimalType` with its four overrides, whose only caller was
`checkIsWiderType` inside that rescale path; the `PROMOTE_PRECISION` name and
its `Sig`; and the `PromotePrecision(Cast)` arm in `ExpressionConverter`.
`genRescaleDecimalTransformer` no longer rescales anything, so it is renamed
and takes the child types directly.
The `CheckOverflow(b: BinaryArithmetic, ...)` arm goes for a separate
reason. On v3.4.4, v3.5.5, v4.0.2 and v4.1.1, plain `CheckOverflow` has three
construction sites, all in `SerializerBuildHelper`, where an encoder converts a
JVM `BigDecimal` to `Decimal` through a `StaticInvoke`. No rule wraps it around
an arithmetic node: decimal overflow is handled by `BinaryArithmetic` itself
via `resultDecimalType` and `checkDecimalOverflow`, and table insert and `Sum`
use the `CheckOverflowInTableInsert` and `CheckOverflowInSum` variants. The
`case c: CheckOverflow` arm further down still catches the encoder path, so
nothing is left uncovered.
Both of those arms sit behind `!transformCheckOverflow`, which only
clickhouse sets. The three `DecimalArithmeticUtil` rewrites are on the velox
and bolt path instead.
`getResultType` stays, and so does its `case other => throw new
GlutenNotSupportException`. Once the rewrites are gone it computes exactly
`b.dataType` for the four operators it handles, which invites replacing the
call with `b.dataType` outright, and that would be a behaviour change: Spark's
`Remainder.resultDecimalType` and `Pmod.resultDecimalType` do not throw, so
`decimal % decimal` and `pmod` would start being offloaded. Dropping Remainder
and Pmod from `isDecimalArithmetic` has the same effect from the other
direction, since they would then fall through to the generic arm and be
offloaded as a plain `modulus` or `pmod` without the `_deny_precision_loss`
name. A new `DecimalArithmeticUtilSuite` pins that rejection so the next reader
who notices the redundancy has a test to trip over.
## How was this patch tested?
`clean test-compile` on Spark 3.4, 3.5, 4.0 and 4.1 with `-Pbackends-velox
-Pspark-ut -Piceberg -Pdelta`, Scala 2.13 throughout. `spotless:apply` produced
no changes.
`-Pbackends-clickhouse` compiles on 3.5 only, so that is where it was
checked. On the other three profiles it does not build at all on main:
`CHRuleApi.scala` and `src-delta/main` reference Delta 3.3 APIs, and
`delta.binary.version=33` is set by the `spark-3.5` profile alone.
`-Pbackends-bolt` is out for a similar reason, that bolt does not compile on
main until #12999 lands.
On Spark 3.5: `GlutenDecimalPrecisionSuite`, `GlutenDecimalExpressionSuite`
and `GlutenArithmeticExpressionSuite` in `gluten-ut/spark35`,
`DecimalArithmeticUtilSuite` in `gluten-substrait`, plus
`MathFunctionsValidateSuite` and `MiscOperatorSuite` in `backends-velox`.
`GlutenDecimalPrecisionSuite` is the closest thing to a regression net here: it
runs Spark's own analyzer over nested decimal arithmetic, integral operands and
38-digit saturation, then asserts the transformed expression's type matches. It
only asserts the outermost node's type, though, which is why the added
`GLUTEN-7082` case compares end to end against vanilla rather than checking a
plan shape.
Three cases in `backends-velox` failed, and all three fail the same way on
main: `decimal to double preserves precision after decimal division`,
`GLUTEN-12356: high-precision decimal to double matches vanilla Spark` and
`cast null type to complex type`. The local `libvelox.dylib` predates #12605
and #12950, and it contains no `decimalToFloatHighPrecisionCastEnabled` symbol
at all, so the first two cannot pass against it. Everything else was green: 143
of 146 in `backends-velox`, 42 in `gluten-ut/spark35`, and the new unit case.
The clickhouse half of the diff, the two arms behind
`!transformCheckOverflow`, is compile-only: there is no clickhouse native
library on this machine. A `Run Gluten Clickhouse CI` pass would be worth
having before merge.
## Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude claude-opus-5
Related issue: #12991
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
