maropu commented on a change in pull request #32488:
URL: https://github.com/apache/spark/pull/32488#discussion_r638376223
##########
File path:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/UnwrapCastInBinaryComparison.scala
##########
@@ -121,6 +129,49 @@ object UnwrapCastInBinaryComparison extends
Rule[LogicalPlan] {
if canImplicitlyCast(fromExp, toType, literalType) =>
simplifyNumericComparison(be, fromExp, toType, value)
+ // As the analyzer makes sure that the list of In is already of the same
data type, then the
+ // rule can simply check the first literal in `in.list` can implicitly
cast to `toType` or not,
+ // and this rule doesn't convert in when `in.list` is empty.
+ case in @ In(Cast(fromExp, toType: NumericType, _), list @ Seq(firstLit,
_*))
+ if canImplicitlyCast(fromExp, toType, firstLit.dataType) &&
in.inSetConvertible =>
+ val (newValueList, exp) =
+ list.map(lit => unwrapCast(EqualTo(in.value, lit)))
+ .partition {
+ case EqualTo(_, _: Literal) => true
+ case _ => false
Review comment:
On second thoughts, this is a bit risky if unexpected patterns found.
Sorry for bothering you, but how about this instead?
```
case And(IsNull(_), Literal(null, BooleanType)) => false
case _ => throw new IllegalStateException("....")
```
##########
File path:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/UnwrapCastInBinaryComparison.scala
##########
@@ -121,6 +129,49 @@ object UnwrapCastInBinaryComparison extends
Rule[LogicalPlan] {
if canImplicitlyCast(fromExp, toType, literalType) =>
simplifyNumericComparison(be, fromExp, toType, value)
+ // As the analyzer makes sure that the list of In is already of the same
data type, then the
+ // rule can simply check the first literal in `in.list` can implicitly
cast to `toType` or not,
+ // and this rule doesn't convert in when `in.list` is empty.
+ case in @ In(Cast(fromExp, toType: NumericType, _), list @ Seq(firstLit,
_*))
+ if canImplicitlyCast(fromExp, toType, firstLit.dataType) &&
in.inSetConvertible =>
+ val (newValueList, exp) =
+ list.map(lit => unwrapCast(EqualTo(in.value, lit)))
+ .partition {
+ case EqualTo(_, _: Literal) => true
+ case _ => false
+ }
+
+ val (nonNullValueList, nullValueList) = newValueList.partition {
+ case EqualTo(_, NonNullLiteral(_, _: NumericType)) => true
+ case _ => false
+ }
+ // make sure the new return list have the same dataType.
+ val newList = {
+ if (nonNullValueList.nonEmpty) {
+ // cast the null value to the dataType of nonNullValueList
+ // when the nonNullValueList is nonEmpty.
+ nullValueList.map {
+ case EqualTo(_, lit) =>
+ Cast(lit,
nonNullValueList.head.asInstanceOf[EqualTo].left.dataType)
+ } ++ nonNullValueList.map {case EqualTo(_, lit) => lit}
+ } else {
+ // the new value list only contains null value,
+ // cast the null value to fromExp.dataType.
+ nullValueList.map {
+ case EqualTo(_, lit) =>
+ Cast(lit, fromExp.dataType)
+ }
+ }
+ }
+
+ val unwrapIn = In(fromExp, newList)
+ // since `exp` are all the same,
+ // convert to a single value `And(IsNull(_), Literal(null,
BooleanType))`.
+ exp.headOption match {
Review comment:
we can assume `assert(exp.length == 1, ...)`?
##########
File path:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/UnwrapCastInBinaryComparison.scala
##########
@@ -121,6 +129,49 @@ object UnwrapCastInBinaryComparison extends
Rule[LogicalPlan] {
if canImplicitlyCast(fromExp, toType, literalType) =>
simplifyNumericComparison(be, fromExp, toType, value)
+ // As the analyzer makes sure that the list of In is already of the same
data type, then the
+ // rule can simply check the first literal in `in.list` can implicitly
cast to `toType` or not,
+ // and this rule doesn't convert in when `in.list` is empty.
+ case in @ In(Cast(fromExp, toType: NumericType, _), list @ Seq(firstLit,
_*))
+ if canImplicitlyCast(fromExp, toType, firstLit.dataType) &&
in.inSetConvertible =>
+ val (newValueList, exp) =
+ list.map(lit => unwrapCast(EqualTo(in.value, lit)))
+ .partition {
+ case EqualTo(_, _: Literal) => true
+ case _ => false
+ }
+
+ val (nonNullValueList, nullValueList) = newValueList.partition {
+ case EqualTo(_, NonNullLiteral(_, _: NumericType)) => true
+ case _ => false
Review comment:
ditto
```
case EqualTo(_, Literal(null, _)) => false
case _ => throw new IllegalStateException("....")
```
--
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:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
