c21 commented on a change in pull request #31318: URL: https://github.com/apache/spark/pull/31318#discussion_r580103906
########## File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/expressions.scala ########## @@ -366,22 +367,34 @@ object BooleanSimplification extends Rule[LogicalPlan] with PredicateHelper { // ((c || ...) && (d || ...)) || a || b (common :+ And(ldiff.reduce(Or), rdiff.reduce(Or))).reduce(Or) } + } else { + // No common factors from disjunctive predicates, reduce common factor from conjunction + val all = splitConjunctivePredicates(left) ++ splitConjunctivePredicates(right) + val distinct = ExpressionSet(all) + if (all.size == distinct.size) { + // No common factors, return the original predicate + and + } else { + // (((a && b) && a && (a && c))) => a && b && c + distinct.reduce(And) + } } // Common factor elimination for disjunction case or @ (left Or right) => // 1. Split left and right to get the conjunctive predicates, - // i.e. lhs = (a, b), rhs = (a, c) + // i.e. lhs = (a, b), rhs = (a, c) Review comment: nit: similarly to above, could we change to `lhs = (a && b), rhs = (a && c)`? ########## File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/expressions.scala ########## @@ -366,22 +367,34 @@ object BooleanSimplification extends Rule[LogicalPlan] with PredicateHelper { // ((c || ...) && (d || ...)) || a || b Review comment: nit: given we are here now, it seems that expression order is not right. Shall we update to ``` a || b || ((c || ...) && (d || ...)) ``` ########## File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/expressions.scala ########## @@ -366,6 +363,16 @@ object BooleanSimplification extends Rule[LogicalPlan] with PredicateHelper { // ((c || ...) && (d || ...)) || a || b (common :+ And(ldiff.reduce(Or), rdiff.reduce(Or))).reduce(Or) } + } else { + // No common factors from disjunctive predicates, reduce common factor from conjunction + val all = splitConjunctivePredicates(left) ++ splitConjunctivePredicates(right) Review comment: It seems that we are improving to handle conjunctive and disjunctive cases separately. I am wondering could we do a better job to handle the expression with mix of `&&` and `||`? e.g. ``` (a || b) && (a && b) ``` to ``` a && b ``` ########## File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/expressions.scala ########## @@ -366,22 +367,34 @@ object BooleanSimplification extends Rule[LogicalPlan] with PredicateHelper { // ((c || ...) && (d || ...)) || a || b (common :+ And(ldiff.reduce(Or), rdiff.reduce(Or))).reduce(Or) } + } else { + // No common factors from disjunctive predicates, reduce common factor from conjunction + val all = splitConjunctivePredicates(left) ++ splitConjunctivePredicates(right) + val distinct = ExpressionSet(all) + if (all.size == distinct.size) { + // No common factors, return the original predicate + and + } else { + // (((a && b) && a && (a && c))) => a && b && c Review comment: nit: could we remove unnecessary parentheses to help better read of comment? ``` (a && b) && a && (a && c) => a && b && c ``` ---------------------------------------------------------------- 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: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org