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