joelrobin18 commented on code in PR #59954:
URL: https://github.com/apache/spark/pull/59954#discussion_r4451266273
##########
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:
Excluded TRY-mode roots from the `Add` and `Multiply` cases, so a `try_add`
/ `try_multiply` chain keeps its written grouping and literals; ANSI and LEGACY
roots still fold their own-mode literals and keep a TRY operand as one unit.
Updated the comment above the flatten helpers accordingly. Added an optimizer
test with top-level and nested TRY chains that mix non-literal operands with
literals of both signs (both ANSI settings), and `try_arithmetic.sql` queries
over a parquet table for the cases you listed: `try_add(try_add(a, -5), b) + 1`
returns 2147483644, `try_add(try_add(a, 2147483647), 1)` returns 2147483638,
`-5 + try_add(try_add(a, 2147483647), 1)` returns 2147483633,
`try_add(try_add(a, 5), -3)` returns NULL, and `try_multiply(try_multiply(a,
-1), b) * 3` returns -2147483648 with ANSI off and raises ARITHMETIC_OVERFLOW
with ANSI on. Addressed in b2800400.
##########
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:
Now compares only the eval mode (`c.evalMode == evalMode`) in both
`flattenAdd` and `flattenMultiply`. Added a test where an ANSI `a + 1` built
with `allowDecimalPrecisionLoss=false` under an ANSI `+ 2` with `true` folds to
`a + 3`, and the Multiply analogue folds to `a * 6`. Addressed in b2800400.
--
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]
