Github user gatorsmile commented on a diff in the pull request:
https://github.com/apache/spark/pull/22711#discussion_r224951558
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/expressions.scala
---
@@ -276,31 +276,37 @@ object BooleanSimplification extends
Rule[LogicalPlan] with PredicateHelper {
case a And b if a.semanticEquals(b) => a
case a Or b if a.semanticEquals(b) => a
- // The following optimization is applicable only when the operands
are not nullable,
+ // The following optimizations are applicable only when the operands
are not nullable,
// since the three-value logic of AND and OR are different in NULL
handling.
// See the chart:
// +---------+---------+---------+---------+
- // | p | q | p OR q | p AND q |
+ // | operand | operand | OR | AND |
// +---------+---------+---------+---------+
// | TRUE | TRUE | TRUE | TRUE |
// | TRUE | FALSE | TRUE | FALSE |
- // | TRUE | UNKNOWN | TRUE | UNKNOWN |
- // | FALSE | TRUE | TRUE | FALSE |
// | FALSE | FALSE | FALSE | FALSE |
- // | FALSE | UNKNOWN | UNKNOWN | FALSE |
// | UNKNOWN | TRUE | TRUE | UNKNOWN |
// | UNKNOWN | FALSE | UNKNOWN | FALSE |
// | UNKNOWN | UNKNOWN | UNKNOWN | UNKNOWN |
// +---------+---------+---------+---------+
+
+ // This can break if a is null and c is false, so a can't be
nullable.
--- End diff --
The current code will not break. Thus, the comment is confusing to the
future reader. To make it clear, we can just give the actual value.
> (NULL And (NULL Or FALSE)) = NULL, but (NULL And FALSE) = FALSE. Thus, a
can't be nullable.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
