cloud-fan commented on code in PR #54112:
URL: https://github.com/apache/spark/pull/54112#discussion_r2758306319
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/expressions.scala:
##########
@@ -506,22 +515,18 @@ object BooleanSimplification extends Rule[LogicalPlan]
with PredicateHelper {
case Not(a LessThan b) => GreaterThanOrEqual(a, b)
case Not(a LessThanOrEqual b) => GreaterThan(a, b)
- // SPARK-54881: push down the NOT operators on children, before attaching
the junction Node
- // to the main tree. This ensures idempotency in an optimal way and avoids
an extra rule
- // iteration.
+ // De Morgan's Laws can create new Not expressions that need further
simplification.
case Not(a Or b) =>
- And(Not(a), Not(b)).transformDownWithPruning(_.containsPattern(NOT),
ruleId) {
- actualExprTransformer
- }
+ And(simplifyNot(Not(a)), simplifyNot(Not(b)))
case Not(a And b) =>
- Or(Not(a), Not(b)).transformDownWithPruning(_.containsPattern(NOT),
ruleId) {
- actualExprTransformer
- }
+ Or(simplifyNot(Not(a)), simplifyNot(Not(b)))
case Not(Not(e)) => e
case Not(IsNull(e)) => IsNotNull(e)
case Not(IsNotNull(e)) => IsNull(e)
+
+ case _ => not
}
}
Review Comment:
Previously it was `O(n^2)`: `actualExprTransformer` is called in
`transformExpressionsUpWithPruning`, and itself also calls
`transformDownWithPruning`. We repeatedly transform the sub-expression tree.
I don't think recursion is a concern, as these transform APIs are all
recursive under the hood.
--
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]
