Github user dbtsai commented on a diff in the pull request:
https://github.com/apache/spark/pull/23139#discussion_r236394865
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/ReplaceNullWithFalseInPredicate.scala
---
@@ -79,29 +80,31 @@ object ReplaceNullWithFalseInPredicate extends
Rule[LogicalPlan] {
* an expression that is not [[CaseWhen]], [[If]], [[And]], [[Or]] or
* `Literal(null, BooleanType)`.
*/
- private def replaceNullWithFalse(e: Expression): Expression = {
- if (e.dataType != BooleanType) {
+ private def replaceNullWithFalse(e: Expression): Expression = e match {
+ case Literal(null, BooleanType) =>
+ FalseLiteral
+ case And(left, right) =>
+ And(replaceNullWithFalse(left), replaceNullWithFalse(right))
+ case Or(left, right) =>
+ Or(replaceNullWithFalse(left), replaceNullWithFalse(right))
+ case cw: CaseWhen if cw.dataType == BooleanType =>
+ val newBranches = cw.branches.map { case (cond, value) =>
+ replaceNullWithFalse(cond) -> replaceNullWithFalse(value)
+ }
+ val newElseValue = cw.elseValue.map(replaceNullWithFalse)
+ CaseWhen(newBranches, newElseValue)
+ case i @ If(pred, trueVal, falseVal) if i.dataType == BooleanType =>
+ If(replaceNullWithFalse(pred), replaceNullWithFalse(trueVal),
replaceNullWithFalse(falseVal))
+ case e if e.dataType == BooleanType =>
e
- } else {
- e match {
- case Literal(null, BooleanType) =>
- FalseLiteral
- case And(left, right) =>
- And(replaceNullWithFalse(left), replaceNullWithFalse(right))
- case Or(left, right) =>
- Or(replaceNullWithFalse(left), replaceNullWithFalse(right))
- case cw: CaseWhen =>
- val newBranches = cw.branches.map { case (cond, value) =>
- replaceNullWithFalse(cond) -> replaceNullWithFalse(value)
- }
- val newElseValue = cw.elseValue.map(replaceNullWithFalse)
- CaseWhen(newBranches, newElseValue)
- case If(pred, trueVal, falseVal) =>
- If(replaceNullWithFalse(pred),
- replaceNullWithFalse(trueVal),
- replaceNullWithFalse(falseVal))
- case _ => e
+ case e =>
+ val message = "Expected a Boolean type expression in
replaceNullWithFalse, " +
+ s"but got the type `${e.dataType.catalogString}` in `${e.sql}`."
+ if (Utils.isTesting) {
+ throw new IllegalArgumentException(message)
--- End diff --
Test for this? Why not also throw exception in runtime since this should
never be hit?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
