apache / apache/gluten

[CORE] Remove the leftover PromotePrecision shim and its decimal rescale path

Open
#12,991 4 comments 0 reactions 1 assignee Claimed by @LuciferYang View on GitHub
Dominant language
Scala
Stars
1.6k
Forks
657
Avg merge
3d 17h
Merged PRs (30d)
86

Description

Gluten still ships a hand-written `org.apache.spark.sql.catalyst.expressions.PromotePrecision` in all four shim modules, plus the decimal-arithmetic rewrites built around it. Both are unreachable now that Spark 3.3 support is gone, and one comment in `ExpressionConverter` still says otherwise.

**Why it exists.** Up to Spark 3.3, `DecimalPrecision` wrapped decimal operands in `PromotePrecision(Cast(child, ))` and the operation in `CheckOverflow`. Velox derives a decimal result type from its *input* types, so the widened operands gave the wrong scale, and `DecimalArithmeticUtil` grew three rewrites to undo them: recover a literal's real precision/scale, narrow an integral operand's cast to the minimal decimal type, and strip decimal→decimal widening casts. When 3.4 deleted the expression ([SPARK-39316](https://issues.apache.org/jira/browse/SPARK-39316)), #3360 added a transparent stub in Spark's package so one code path could still serve 3.3 and 3.4; it was then copied to 3.5 (#4822), 4.0 (#9768) and 4.1.

**Why it is no longer needed.** Beyond the node simply never appearing, Spark 3.4+ performs all three adjustments itself — `BinaryArithmetic.resultDecimalType` carries the result type, `DecimalPrecision.decimalAndDecimal` only rewrites `BinaryComparison`, and `nondecimalAndDecimal` casts via `DecimalType.forType` / `fromLiteral`. So Gluten's three rewrites return their input unchanged. The same reasoning retires the `CheckOverflow(BinaryArithmetic, ...)` case: in 3.4–4.1 only the encoder path constructs `CheckOverflow`, while table inserts and `Sum` build the `InTableInsert` / `InSum` variants.

**To remove**

- `DecimalArithmeticUtil`: the three identity rewrites and the private helpers only they reach
- `ExpressionConverter`: the `PromotePrecision` case and pass-through alternative, the unreachable `CheckOverflow(BinaryArithmetic, ...)` case, and [this comment](https://github.com/apache/gluten/blob/5ff78c30606a3d175d486d1dea7a9b6e3f7f14a3/gluten-substrait/src/main/scala/org/apache/gluten/expression/ExpressionConverter.scala#L691-L693)
- the `promote_precision` signature and name constant — no native backend implements it
- the four shim stubs
- `SparkShims.widerDecimalType` and its overrides, whose only caller goes away above

**Two things to watch**

1. `isDecimalArithmetic` admits `Remainder`/`Pmod`, but result-type derivation [throws for them](https://github.com/apache/gluten/blob/5ff78c30606a3d175d486d1dea7a9b6e3f7f14a3/gluten-substrait/src/main/scala/org/apache/gluten/utils/DecimalArithmeticUtil.scala#L65-L66) — today that throw is the only thing keeping `decimal % decimal` off Velox. Keep an explicit rejection and add a fallback test.
2. This may close #7082, which blames exactly this machinery and suggests keeping `PromotePrecision(Cast(...))`. Worth running its repro on 3.5/4.1 first: if it passes, add it as a test and close that issue.

No behaviour change expected otherwise; the result type Gluten declares already matches Spark's. Follow-up to #12807.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.