ahshahid commented on code in PR #54112:
URL: https://github.com/apache/spark/pull/54112#discussion_r2758238160
##########
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:
Have 2 comments:
1) Have found recursive calls to be less efficient, as compared to getting
the recursive behaviour via call on an instance, where the instance itself is
changing at every step.. like it is happening in transformDown
2) Any future modification , say adding new case etc, would require analysis
whether it should go in both the places ( original transform down, and new
simplifyNot function, or only in once place... atleast this is what I feel..
though I may be wrong.
But if that identification would be straightforward and any hypothetical new
case added would be needed only in one of the functions, then this distinction
is good. Though recursive call on method will be a concern for complex
expressions....
--
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]
