peter-toth commented on code in PR #40093:
URL: https://github.com/apache/spark/pull/40093#discussion_r1121764260
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/expressions.scala:
##########
@@ -200,14 +200,20 @@ object ConstantPropagation extends Rule[LogicalPlan] {
private def replaceConstants(condition: Expression, equalityPredicates:
EqualityPredicates)
: Expression = {
- val constantsMap = AttributeMap(equalityPredicates.map(_._1))
- val predicates = equalityPredicates.map(_._2).toSet
- def replaceConstants0(expression: Expression) = expression transform {
+ val allConstantsMap = AttributeMap(equalityPredicates.map(_._1))
+ val allPredicates = equalityPredicates.map(_._2).toSet
+ def replaceConstants0(
+ expression: Expression, constantsMap: AttributeMap[Literal]) =
expression transform {
case a: AttributeReference => constantsMap.getOrElse(a, a)
}
condition transform {
- case e @ EqualTo(_, _) if !predicates.contains(e) => replaceConstants0(e)
- case e @ EqualNullSafe(_, _) if !predicates.contains(e) =>
replaceConstants0(e)
+ case b: BinaryComparison =>
Review Comment:
1. I'm not sure I get the issue. If a complex expression is equal to a
constant why we can't use that constant where the complex expression appears? I
don't think it matters if some parts of the complex expression can also be
replaced to another constat. The replace happens using `transformDown` so
larger complex expressions are replaced to constants first. E.g. `a = 1 AND (a
+ b) = 1 AND (a + b) > 2` -> `a = 1 AND (1 + b) = 1 AND 1 > 2` -> `false` makes
sense to me.
3. Yes we can. The reason why I didn't do those kind of shortcuts in
`ConstantPropagation` in my PR is because there are other, similar cases where
it isn't that easy to do a shortcut (or simplification has been handled in
other rules so why doing it here as well) . E.g. `a = 1 AND a > 2` -> `a = 1
AND 1 > 2` -> `false` where the 2nd step (`1 > 2` -> `false`) could also be
handled in `ConstantPropagation` but those foldable expressions are already
handled in `ConstantFolding` and then in `BooleanSimplification` when contant
propagation has been done.
--
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: reviews-unsubscr...@spark.apache.org
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
