-
Notifications
You must be signed in to change notification settings - Fork 28.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[SPARK-25402][SQL][BACKPORT-2.2] Null handling in BooleanSimplification #22403
Conversation
This PR is to fix the null handling in BooleanSimplification. In the rule BooleanSimplification, there are two cases that do not properly handle null values. The optimization is not right if either side is null. This PR is to fix them. Added test cases Closes apache#22390 from gatorsmile/fixBooleanSimplification. Authored-by: gatorsmile <gatorsmile@gmail.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com> (cherry picked from commit 79cc597) Signed-off-by: Wenchen Fan <wenchen@databricks.com>
Test build #95999 has finished for PR 22403 at commit
|
@@ -37,6 +38,7 @@ class BooleanSimplificationSuite extends PlanTest with PredicateHelper { | |||
Batch("Constant Folding", FixedPoint(50), | |||
NullPropagation(conf), | |||
ConstantFolding, | |||
SimplifyConditionals, | |||
BooleanSimplification, | |||
PruneFilters(conf)) :: Nil | |||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At line 46, SPARK-17851 changed like the following. We need that change. After updating that, it passes the test without any problems.
- val testRelation = LocalRelation('a.int, 'b.int, 'c.int, 'd.string)
+ val testRelation = LocalRelation('a.int, 'b.int, 'c.int, 'd.string,
'e.boolean, 'f.boolean, 'g.boolean, 'h.boolean)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. Thanks!
Test build #96033 has finished for PR 22403 at commit
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1, LGTM.
Merged to branch-2.2. |
## What changes were proposed in this pull request? This PR is to fix the null handling in BooleanSimplification. In the rule BooleanSimplification, there are two cases that do not properly handle null values. The optimization is not right if either side is null. This PR is to fix them. ## How was this patch tested? Added test cases Closes #22403 from gatorsmile/backportSpark25402. Authored-by: gatorsmile <gatorsmile@gmail.com> Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
## What changes were proposed in this pull request? This PR is to fix the null handling in BooleanSimplification. In the rule BooleanSimplification, there are two cases that do not properly handle null values. The optimization is not right if either side is null. This PR is to fix them. ## How was this patch tested? Added test cases Closes apache#22403 from gatorsmile/backportSpark25402. Authored-by: gatorsmile <gatorsmile@gmail.com> Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
## What changes were proposed in this pull request? This PR is to fix the null handling in BooleanSimplification. In the rule BooleanSimplification, there are two cases that do not properly handle null values. The optimization is not right if either side is null. This PR is to fix them. ## How was this patch tested? Added test cases Closes apache#22403 from gatorsmile/backportSpark25402. Authored-by: gatorsmile <gatorsmile@gmail.com> Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
What changes were proposed in this pull request?
This PR is to fix the null handling in BooleanSimplification. In the rule BooleanSimplification, there are two cases that do not properly handle null values. The optimization is not right if either side is null. This PR is to fix them.
How was this patch tested?
Added test cases