Vivek1106-04 opened a new pull request, #59437:
URL: https://github.com/apache/spark/pull/59437
### What changes were proposed in this pull request?
`Add.canonicalized` and `Multiply.canonicalized` flatten adjacent operators
and reorder all operands (`buildCanonicalizedPlan` -> `orderCommutative` ->
`gatherCommutative`), with no exception for `FLOAT`/`DOUBLE`.
This PR changes canonicalization for floating-point `Add`/`Multiply` to
reorder only the two direct children, without flattening the operator tree:
- Adds `CommutativeExpression.orderBinaryCommutative`, which canonicalizes
and sorts the two children of a binary commutative operator.
- `Add` and `Multiply` use it when the (resolved) result type is `FloatType`
or `DoubleType`. All other types keep the existing flattening path, including
the `MultiCommutativeOp` memory optimization and the SPARK-40903 decimal
fallback.
So `a + b` and `b + a` stay semantically equal, while `(a + b) + c` and `a +
(b + c)` no longer are.
### Why are the changes needed?
Floating-point `+` and `*` are commutative but not associative, so `(a + b)
+ c` and `a + (b + c)` canonicalized to the same expression while evaluating to
different values. Every consumer of semantic equality may then substitute one
for the other, and two of them return wrong results today.
With `a = 1.0E16`, `b = -1.0E16`, `c = 1.0` (`(a + b) + c = 1.0`, `a + (b +
c) = 0.0`):
1. Subexpression elimination:
```sql
SELECT (a + b) + c AS x, a + (b + c) AS y FROM t
-- before: x=1.0 y=1.0 (y is wrong); after: x=1.0 y=0.0
```
2. Cached-plan reuse (`CacheManager` / `sameResult`):
```scala
val d1 = spark.sql("select (a+b)+c as v from t"); d1.cache();
d1.collect() // [1.0]
spark.sql("select a+(b+c) as v from t").collect()
// before: [1.0], served from d1's cache entry; after: [0.0]
```
Integer, decimal, boolean and bitwise operators are unaffected:
re-associating those does not change the returned value.
### Does this PR introduce _any_ user-facing change?
Yes, a correctness fix. Queries containing floating-point `+`/`*`
expressions that differ only in association no longer share
subexpression-elimination results or cached plans, so they return the correct
values instead of the value of the other expression. Expressions that differ
only in operand order of a single operator (`a + b` vs `b + a`) are still
deduplicated.
### How was this patch tested?
New tests:
- `CanonicalizeSuite`: for `FloatType`/`DoubleType`, `Add`/`Multiply`, and
`MULTI_COMMUTATIVE_OP_OPT_THRESHOLD` of 1 and 100, commutative forms are
semantically equal and re-associated forms are not.
- `SQLQuerySuite`: end-to-end subexpression elimination for `+` (precision
loss) and `*` (overflow to Infinity), across CSE on/off and whole-stage codegen
on/off.
- `CachedTableSuite`: a re-associated query does not hit the cache entry of
the original and returns the correct value.
All three fail without the fix and pass with it. Also ran
`CanonicalizeSuite`, `SubexpressionEliminationSuite`,
`ReorderAssociativeOperatorSuite`, `ArithmeticExpressionSuite`, and all
`*PlanStability*Suite` (322 tests, no plan changes).
### Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Claude Opus 5.5)
--
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]
