You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by GitBox <gi...@apache.org> on 2021/03/01 13:41:07 UTC
[GitHub] [spark] maropu commented on a change in pull request #31318: [SPARK-34222][SQL] Enhance boolean simplification rule
maropu commented on a change in pull request #31318:
URL: https://github.com/apache/spark/pull/31318#discussion_r584716870
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/predicates.scala
##########
@@ -157,6 +157,30 @@ trait PredicateHelper extends AliasHelper with Logging {
}
}
+ protected def mergeConjunctivePredicates(exprs: Seq[Expression]): Expression = {
+ mergePredicates(exprs, And)
+ }
+
+ protected def mergeDisjunctivePredicates(exprs: Seq[Expression]): Expression = {
Review comment:
ditto
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/predicates.scala
##########
@@ -157,6 +157,30 @@ trait PredicateHelper extends AliasHelper with Logging {
}
}
+ protected def mergeConjunctivePredicates(exprs: Seq[Expression]): Expression = {
Review comment:
This method has only been used once now, so how about inlining it now.
```
// (a && b) && a && (a && c) => a && b && c
mergeConjunctivePredicates(distinct.toSeq)
==>
// (a && b) && a && (a && c) => a && b && c
mergePredicates(distinct.toSeq, And)
```
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/predicates.scala
##########
@@ -157,6 +157,30 @@ trait PredicateHelper extends AliasHelper with Logging {
}
}
+ protected def mergeConjunctivePredicates(exprs: Seq[Expression]): Expression = {
+ mergePredicates(exprs, And)
+ }
+
+ protected def mergeDisjunctivePredicates(exprs: Seq[Expression]): Expression = {
+ mergePredicates(exprs, Or)
+ }
+
+ private def mergePredicates[Expression](
Review comment:
Could you leave some comments about what this function does?
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/predicates.scala
##########
@@ -157,6 +157,30 @@ trait PredicateHelper extends AliasHelper with Logging {
}
}
+ protected def mergeConjunctivePredicates(exprs: Seq[Expression]): Expression = {
+ mergePredicates(exprs, And)
+ }
+
+ protected def mergeDisjunctivePredicates(exprs: Seq[Expression]): Expression = {
+ mergePredicates(exprs, Or)
+ }
+
+ private def mergePredicates[Expression](
Review comment:
`mergePredicates` -> `buildBalancedPredicates`?
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/predicates.scala
##########
@@ -157,6 +157,30 @@ trait PredicateHelper extends AliasHelper with Logging {
}
}
+ protected def mergeConjunctivePredicates(exprs: Seq[Expression]): Expression = {
+ mergePredicates(exprs, And)
+ }
+
+ protected def mergeDisjunctivePredicates(exprs: Seq[Expression]): Expression = {
+ mergePredicates(exprs, Or)
+ }
+
+ private def mergePredicates[Expression](
+ exprs: Seq[Expression], op: (Expression, Expression) => Expression): Expression = {
+ exprs match {
+ case Seq(expression) => expression
+ case expressions =>
+ val grouped = expressions.grouped(2).toSeq
+ val pairwiseOpd = for (g <- grouped) yield {
Review comment:
nit: `pairwiseOpd` -> `pairwiseExprs`?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org