sunchao commented on a change in pull request #32488:
URL: https://github.com/apache/spark/pull/32488#discussion_r629559679
##########
File path:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/UnwrapCastInBinaryComparison.scala
##########
@@ -21,7 +21,7 @@ import org.apache.spark.sql.catalyst.expressions._
import org.apache.spark.sql.catalyst.expressions.Literal.FalseLiteral
import org.apache.spark.sql.catalyst.plans.logical.LogicalPlan
import org.apache.spark.sql.catalyst.rules.Rule
-import org.apache.spark.sql.catalyst.trees.TreePattern.BINARY_COMPARISON
+import org.apache.spark.sql.catalyst.trees.TreePattern.{BINARY_COMPARISON, IN}
Review comment:
can we update the scala doc for the newly added `IN` case?
##########
File path:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/UnwrapCastInBinaryComparison.scala
##########
@@ -89,10 +89,11 @@ import org.apache.spark.sql.types._
*/
object UnwrapCastInBinaryComparison extends Rule[LogicalPlan] {
override def apply(plan: LogicalPlan): LogicalPlan =
plan.transformWithPruning(
- _.containsPattern(BINARY_COMPARISON), ruleId) {
+ _.containsAnyPattern(BINARY_COMPARISON, IN), ruleId) {
Review comment:
what about `INSET`? should we handle that too?
##########
File path:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/UnwrapCastInBinaryComparison.scala
##########
@@ -121,6 +122,17 @@ object UnwrapCastInBinaryComparison extends
Rule[LogicalPlan] {
if canImplicitlyCast(fromExp, toType, literalType) =>
simplifyNumericComparison(be, fromExp, toType, value)
+ case in @ In(Cast(fromExp, toType: NumericType, _), list)
+ if list.forall(v =>
+ canImplicitlyCast(fromExp, toType, v.dataType) &&
v.isInstanceOf[Literal]
Review comment:
nit: we can check `v.isInstanceOf[Literal]` first which is cheaper.
##########
File path:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/UnwrapCastInBinaryComparison.scala
##########
@@ -121,6 +122,17 @@ object UnwrapCastInBinaryComparison extends
Rule[LogicalPlan] {
if canImplicitlyCast(fromExp, toType, literalType) =>
simplifyNumericComparison(be, fromExp, toType, value)
+ case in @ In(Cast(fromExp, toType: NumericType, _), list)
+ if list.forall(v =>
+ canImplicitlyCast(fromExp, toType, v.dataType) &&
v.isInstanceOf[Literal]
+ ) =>
+ val newValueList =
+ list.map(lit => unwrapCast(EqualTo(in.value, lit)))
+ .collect {
+ case EqualTo(_, lit: Literal) => lit
Review comment:
`unwrapCast` could convert a `EqualTo` to a different expression. For
instance:
```
x = 2147483648 => if(isnull(x), null, false)
```
if `x` is of integer type. I think we should handle those here too?
--
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
