[SPARK-25860][SPARK-26107] [FOLLOW-UP] Rule ReplaceNullWithFalseInPredicate #23139

gatorsmile · 2018-11-26T03:18:16Z

What changes were proposed in this pull request?

Based on #22857 and #23079, this PR did a few updates

Limit the data types of NULL to Boolean.
Limit the input data type of replaceNullWithFalse to Boolean; throw an exception in the testing mode.
Create a new file for the rule ReplaceNullWithFalseInPredicate
Update the description of this rule.

How was this patch tested?

Added a test case

gatorsmile · 2018-11-26T04:00:39Z

cc @dbtsai @aokolnychyi @rednaxelafx @cloud-fan

cloud-fan · 2018-11-26T04:26:23Z

...src/main/scala/org/apache/spark/sql/catalyst/optimizer/ReplaceNullWithFalseInPredicate.scala

+   * `Literal(null, BooleanType)`.
+   */
+  private def replaceNullWithFalse(e: Expression): Expression = {
+    if (e.dataType != BooleanType) {


do we need this? And, Or, If all return boolean, and we already requires boolean type for literal case.

How about the LambdaFunction? My major concern is the future changes might forget to add it?

We don't handle LambdaFunction inside this method, it's caller side.

This method is to deal with optimizable boolean expressions, and return the original expression if it's not: https://github.com/apache/spark/pull/23139/files#diff-0bb4fc0a3c867b855f84dd1db8867139R103

See this line https://github.com/apache/spark/pull/23139/files/e41681096867cbc6d2556da83ce733092d6df841#diff-a1acb054bc8888376603ef510e6d0ee0

My major concern is we should not completely rely on the caller to ensure the data type is Boolean. In the future, the new code changes might not completely follow our current assumption.

Had an offline discussion with @rednaxelafx . We can issue an exception instead of silently bypass it.

SparkQA · 2018-11-26T05:13:34Z

Test build #99255 has finished for PR 23139 at commit 6b6997d.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2018-11-26T08:05:02Z

Test build #99262 has finished for PR 23139 at commit e416810.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

gatorsmile · 2018-11-26T08:28:27Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/expressions.scala

-      And(replaceNullWithFalse(left), replaceNullWithFalse(right))
-    case Or(left, right) =>
-      Or(replaceNullWithFalse(left), replaceNullWithFalse(right))
-    case Literal(null, _) => FalseLiteral


How about this line? What happened if the input data type of e is not Boolean?

gatorsmile · 2018-11-26T08:30:40Z

retest this please

SparkQA · 2018-11-26T12:04:36Z

Test build #99270 has finished for PR 23139 at commit e416810.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

dbtsai · 2018-11-26T19:26:23Z

Although we are trying to make sure in the caller side to only call replaceNullWithFalse when the expression is boolean type, I agree that for safety, we should check it and throw exception for future development.

dbtsai · 2018-11-26T19:29:23Z

...src/main/scala/org/apache/spark/sql/catalyst/optimizer/ReplaceNullWithFalseInPredicate.scala

+      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)


Test for this? Why not also throw exception in runtime since this should never be hit?

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.

Added a test case.

Sounds fair.

SparkQA · 2018-11-26T22:23:45Z

Test build #99283 has finished for PR 23139 at commit 3501420.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

dbtsai · 2018-11-26T23:20:27Z

LGTM.

aokolnychyi · 2018-11-26T23:23:25Z

...src/main/scala/org/apache/spark/sql/catalyst/optimizer/ReplaceNullWithFalseInPredicate.scala

+
+/**
+ * A rule that replaces `Literal(null, BooleanType)` with `FalseLiteral`, if possible, in the search
+ * condition of the WHERE/HAVING/ON(JOIN) clauses, which contain an implicit Boolean operator


I think the scope of this rule is a bit bigger. For example, some higher-order functions, conditions of all If and CaseWhen expressions. Would it make sense to replace "in the search condition of the WHERE/HAVING/ON(JOIN) clauses" with "in predicates"?

The extra scope is covered by "Moreover, ..."

aokolnychyi · 2018-11-26T23:25:22Z

...src/main/scala/org/apache/spark/sql/catalyst/optimizer/ReplaceNullWithFalseInPredicate.scala

+/**
+ * A rule that replaces `Literal(null, BooleanType)` with `FalseLiteral`, if possible, in the search
+ * condition of the WHERE/HAVING/ON(JOIN) clauses, which contain an implicit Boolean operator
+ * "(search condition) = TRUE". The replacement is only valid when `Literal(null, BooleanType)` is


I am not sure I understand "which contain an implicit Boolean operator "(search condition) = TRUE"". Could you, please, elaborate a bit?

This is based on the ANSI SQL. All these clauses have the implicit Boolean operator "(search condition) = TRUE". That is why NULL and FALSE do not satisfy the condition in these clauses

SparkQA · 2018-11-27T02:27:54Z

Test build #99294 has finished for PR 23139 at commit 8b0401c.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

dbtsai · 2018-11-27T04:51:50Z

Thanks. Merged into master.

…icate ## What changes were proposed in this pull request? Based on apache#22857 and apache#23079, this PR did a few updates - Limit the data types of NULL to Boolean. - Limit the input data type of replaceNullWithFalse to Boolean; throw an exception in the testing mode. - Create a new file for the rule ReplaceNullWithFalseInPredicate - Update the description of this rule. ## How was this patch tested? Added a test case Closes apache#23139 from gatorsmile/followupSpark-25860. Authored-by: gatorsmile <[email protected]> Signed-off-by: DB Tsai <[email protected]>

gatorsmile added 2 commits November 25, 2018 19:00

fix

d2c5e66

fix

6b6997d

cloud-fan reviewed Nov 26, 2018

View reviewed changes

fix the test case

e416810

gatorsmile commented Nov 26, 2018

View reviewed changes

issue an exception

3501420

dbtsai reviewed Nov 26, 2018

View reviewed changes

added a test case

8b0401c

aokolnychyi reviewed Nov 26, 2018

View reviewed changes

cloud-fan approved these changes Nov 27, 2018

View reviewed changes

asfgit closed this in 85383d2 Nov 27, 2018

dbtsai mentioned this pull request Dec 20, 2018

[SPARK-26390][SQL] ColumnPruning rule should only do column pruning #23343

Closed

[SPARK-25860][SPARK-26107] [FOLLOW-UP] Rule ReplaceNullWithFalseInPredicate #23139

[SPARK-25860][SPARK-26107] [FOLLOW-UP] Rule ReplaceNullWithFalseInPredicate #23139

Uh oh!

Conversation

gatorsmile commented Nov 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

gatorsmile commented Nov 26, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cloud-fan Nov 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Nov 26, 2018

Uh oh!

SparkQA commented Nov 26, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gatorsmile commented Nov 26, 2018

Uh oh!

SparkQA commented Nov 26, 2018

Uh oh!

dbtsai commented Nov 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Nov 26, 2018

Uh oh!

dbtsai commented Nov 26, 2018

Uh oh!

aokolnychyi Nov 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Nov 27, 2018

Uh oh!

dbtsai commented Nov 27, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

gatorsmile commented Nov 26, 2018 •

edited

Loading

cloud-fan Nov 26, 2018 •

edited

Loading

dbtsai commented Nov 26, 2018 •

edited

Loading

aokolnychyi Nov 26, 2018 •

edited

Loading