Skip to content

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

Description

@jackylee-ch

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, <wider type>)) 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), #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
  • 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 — 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 [VL] Incorrect result type of nested decimal arithmetic expressions #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.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

No labels
No labels

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions