[GitHub] spark pull request #22711: [SPARK-25714][SQL][followup] improve the comment ...
Github user asfgit closed the pull request at: https://github.com/apache/spark/pull/22711 --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #22711: [SPARK-25714][SQL][followup] improve the comment ...
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
[GitHub] spark pull request #22711: [SPARK-25714][SQL][followup] improve the comment ...
Github user cloud-fan commented on a diff in the pull request:
https://github.com/apache/spark/pull/22711#discussion_r224951223
--- 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.
case a And (b Or c) if !a.nullable && Not(a).semanticEquals(b) =>
And(a, c)
+ // This can break if a is null and b is false, so a can't be
nullable.
case a And (b Or c) if !a.nullable && Not(a).semanticEquals(c) =>
And(a, b)
- case (a Or b) And c if !a.nullable && a.semanticEquals(Not(c)) =>
And(b, c)
- case (a Or b) And c if !b.nullable && b.semanticEquals(Not(c)) =>
And(a, c)
+ // This can break if c is null and b is false, so c can't be
nullable.
+ case (a Or b) And c if !c.nullable && a.semanticEquals(Not(c)) =>
And(b, c)
--- End diff --
`b.nullable` and `c.nullable` are same, because `a.semanticEquals(Not(c))`.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #22711: [SPARK-25714][SQL][followup] improve the comment ...
Github user cloud-fan commented on a diff in the pull request:
https://github.com/apache/spark/pull/22711#discussion_r224951215
--- 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 --
explain which input can break it, so it's easier to understand.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #22711: [SPARK-25714][SQL][followup] improve the comment ...
Github user cloud-fan commented on a diff in the pull request:
https://github.com/apache/spark/pull/22711#discussion_r224951190
--- 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 |
--- End diff --
AND and OR is commutative, so we can reduce the entries in this table
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
