Github user gatorsmile commented on a diff in the pull request:
https://github.com/apache/spark/pull/23139#discussion_r236450239
--- 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 --
The tests might not catch all the cases if the test coverage is not
complete. Such an exception should not block the query execution. Thus, we just
throw an exception in our testing mode instead of the production mode.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
