cloud-fan commented on code in PR #59954:
URL: https://github.com/apache/spark/pull/59954#discussion_r4444272165
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/expressions.scala:
##########
@@ -273,19 +273,23 @@ object ConstantPropagation extends Rule[LogicalPlan] {
* Reorder associative integral-type operators and fold all constants into one.
*/
object ReorderAssociativeOperator extends Rule[LogicalPlan] {
+ // Flatten only operators evaluated in the same mode as the root: rebuilding
a TRY-mode Add or
+ // Multiply (try_add, try_multiply) in the enclosing mode would change its
overflow behavior.
private def flattenAdd(
expression: Expression,
- groupSet: ExpressionSet): Seq[Expression] = expression match {
- case expr @ Add(l, r, _) if !groupSet.contains(expr) =>
- flattenAdd(l, groupSet) ++ flattenAdd(r, groupSet)
+ groupSet: ExpressionSet,
+ evalContext: NumericEvalContext): Seq[Expression] = expression match {
+ case expr @ Add(l, r, c) if c == evalContext && !groupSet.contains(expr) =>
Review Comment:
**Non-blocking (P2):** `c == evalContext` compares the whole
`NumericEvalContext`, which holds `allowDecimalPrecisionLoss` as well as
`evalMode`. That flag only affects decimal result types (`resultDecimalType`),
and this rule only rewrites integral chains, so rebuilding such an operator in
the root's context is exact. Master folded chains whose operators share the
eval mode but differ only in that flag; this PR keeps them apart. Example:
create a view under `spark.sql.decimalOperations.allowPrecisionLoss=false` that
defines `x = a + 2` (views capture modified configs). Then run `SELECT x + 1
FROM v` in a session with the default setting. The optimized plan now keeps `(a
+ 2) + 1` instead of `a + 3`. Multiply, SQL function bodies, and Datasets built
across a change to that config behave the same way. Results are unchanged, but
each such boundary adds one integral operation per row.
Comparing only the mode (`c.evalMode == evalContext.evalMode`, here and in
`flattenMultiply`) keeps the cross-mode fix and restores this folding. It also
makes the code match the comment above, which says "same mode".
**Verification:**
- **Regression:** An ANSI-mode Add of (a + 1) built with
allowDecimalPrecisionLoss=false, nested under an ANSI-mode + 2 built with
allowDecimalPrecisionLoss=true, is optimized to a + 3; the Multiply analogue (a
* 2) * 3 is optimized to a * 6.
- **Behavior:** The TRY, ANSI, and LEGACY boundary cases for Add and
Multiply under both ANSI settings still keep the different-mode operand and
fold only literals of the root's mode.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/expressions.scala:
##########
@@ -273,19 +273,23 @@ object ConstantPropagation extends Rule[LogicalPlan] {
* Reorder associative integral-type operators and fold all constants into one.
*/
object ReorderAssociativeOperator extends Rule[LogicalPlan] {
+ // Flatten only operators evaluated in the same mode as the root: rebuilding
a TRY-mode Add or
+ // Multiply (try_add, try_multiply) in the enclosing mode would change its
overflow behavior.
private def flattenAdd(
expression: Expression,
- groupSet: ExpressionSet): Seq[Expression] = expression match {
- case expr @ Add(l, r, _) if !groupSet.contains(expr) =>
- flattenAdd(l, groupSet) ++ flattenAdd(r, groupSet)
+ groupSet: ExpressionSet,
+ evalContext: NumericEvalContext): Seq[Expression] = expression match {
+ case expr @ Add(l, r, c) if c == evalContext && !groupSet.contains(expr) =>
Review Comment:
**Blocking (P1):** This guard keeps a TRY-mode chain as a single operand of
an ANSI or LEGACY root. But `transformExpressionsDown` then visits that operand
as its own root (`case a @ Add(_, _, f)` with `f = TRY`). There, `flattenAdd`
recurses through the TRY descendants, and the rule rebuilds the chain with the
non-literal operands first and the folded literal last. TRY arithmetic is not
associative: each step returns NULL only when its own operation overflows, so
regrouping changes which rows are NULL.
Example: a table row with `a = 2147483647, b = 1`, and `SELECT
try_add(try_add(a, -5), b) + 1 FROM t`. As written, the steps give 2147483642,
then 2147483643, then 2147483644. This PR rewrites the TRY chain to
`try_add(try_add(a, b), -5)`. `a + b` overflows, so the query returns NULL. On
master, the outer `+` absorbed the chain and rebuilt it in its own mode. With
`spark.sql.ansi.enabled=false` that wrapped back to the correct 2147483644;
with ANSI on it raised ARITHMETIC_OVERFLOW. Either way, this PR turns those
rows into a silent NULL. Multiply has the same problem: with ANSI off,
`try_multiply(try_multiply(a, -1), b) * 3` with `a = 1073741824, b = 2` returns
-2147483648 on master and NULL with this PR. Folding literals is affected too.
Under ANSI, `-5 + try_add(try_add(a, 2147483647), 1)` with `a = -10` should
return 2147483633, but the TRY chain now folds `try_add(2147483647, 1)` into
NULL for every row.
Top-level TRY chains such as `try_add(try_add(a, -5), b)` were already
regrouped this way on master, so their results also stay wrong. ANSI and LEGACY
chains are not affected. The new test has no TRY chain that mixes a non-literal
operand with literals of different signs, so it does not catch this.
**Recommended change:** In ReorderAssociativeOperator, exclude TRY-mode
roots from the Add and Multiply cases, so the rule leaves a chain of try_add or
try_multiply exactly as written. ANSI and LEGACY roots keep folding their
own-mode operands and keep a TRY operand as one unit. Update the comment above
the flatten helpers to say that TRY arithmetic is never regrouped, because a
TRY step returns NULL on its own overflow and regrouping changes which step
overflows. Extend the associative-reordering optimizer tests, and add SQL query
coverage for try arithmetic results, with nested and top-level TRY chains that
mix non-literal operands and literals of both signs.
**Why this works:** A TRY chain's result depends on which intermediate step
overflows. With TRY roots excluded, the rule never changes the grouping or
literal folding of TRY operations. The PR's mode guard already keeps TRY
operands out of ANSI and LEGACY chains, so no path left in the rule regroups
TRY arithmetic.
**Scope:** Stop ReorderAssociativeOperator from regrouping TRY-mode
Add/Multiply chains, and cover nested and top-level TRY chains in the optimizer
and SQL result tests.
**Compatibility:** Folding and regrouping of ANSI and LEGACY chains, the
zero and one simplifications, and the grouping-expression guard for Aggregate
behave as on the base branch.
**Risks:** Chains made only of TRY operations, such as try_add(try_add(a,
1), 2), are no longer folded into try_add(a, 3), so they evaluate one more TRY
operation per row and their optimized plans change. This is the cost of correct
results and can be rolled back by removing the TRY exclusion.
**Constraints:** ANSI and LEGACY roots must keep folding their own-mode
literals around a kept TRY operand, as the PR's new optimizer test expects (for
example, a TRY add plus 1 plus 2 still becomes the TRY add plus 3).
**Exclusions:**
- Regrouping inside ANSI-only chains, which can move or suppress an overflow
error, stays as it is on the base branch, where the existing SPARK-49915 and
SPARK-50380 optimizer tests assert it; changing it is a separate decision
outside this fix.
**Success:** The rule leaves every TRY-mode Add or Multiply chain with its
written grouping and literals. In queries that combine nested try_add or
try_multiply with regular arithmetic, each TRY operation returns NULL exactly
when its own written step overflows, under both ANSI settings. ANSI and LEGACY
chains still fold their own-mode literals and keep different-mode operands as
single units.
**Verification:**
- **Regression:** Under both ANSI settings, try_add(try_add(a, -5), b) + 1
over a table row with a = 2147483647 and b = 1 returns 2147483644, and the
optimized plan keeps the TRY chain in its written order.
- **Regression:** A top-level try_add(try_add(a, 2147483647), 1) over a
table row with a = -10 returns 2147483638 rather than NULL, and
try_add(try_add(a, 5), -3) with a = 2147483643 returns NULL.
- **Regression:** In a session with ANSI disabled,
try_multiply(try_multiply(a, -1), b) * 3 over a table row with a = 1073741824
and b = 2 returns -2147483648.
- **Behavior:** For Add and Multiply under both ANSI settings, ANSI and
LEGACY roots still fold their own-mode literals around a kept TRY operand and
keep each operand in its own mode.
- **Compatibility:** Existing try arithmetic SQL query results and the
existing associative-reordering plan expectations for ANSI and LEGACY chains,
including the zero and one simplifications and the Aggregate grouping guard,
are unchanged.
--
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]
