Skip to content

[SPARK-25402][SQL] Null handling in BooleanSimplification #22390

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.

Sign up for GitHub

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

Jump to bottom
Closed
gatorsmile wants to merge 3 commits into from
Closed

[SPARK-25402][SQL] Null handling in BooleanSimplification #22390

gatorsmile wants to merge 3 commits into from

Conversation

@gatorsmile
Copy link
Member

@gatorsmile gatorsmile commented Sep 11, 2018

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

@gatorsmile
Copy link
Member Author

gatorsmile commented Sep 11, 2018

cc @cloud-fan @adrian-ionescu

cloud-fan
cloud-fan reviewed Sep 11, 2018
View reviewed changes
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/expressions.scala
case TrueLiteral Or _ => TrueLiteral
case _ Or TrueLiteral => TrueLiteral

case a And b if Not(a).semanticEquals(b) => FalseLiteral
Copy link
Contributor

@cloud-fan cloud-fan Sep 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how about

case a And b if Not(a).semanticEquals(b) || a.semanticEquals(Not(b)) =>
  If(IsNull(a), null, FalseLiteral)

Copy link
Member

@gengliangwang gengliangwang Sep 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

Copy link
Contributor

@adrian-ionescu adrian-ionescu Sep 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

Copy link
Member Author

@gatorsmile gatorsmile Sep 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about

      case a And b if Not(a).semanticEquals(b) || a.semanticEquals(Not(b)) =>
        if (!a.nullable && !b.nullable) {
          FalseLiteral
        } else {
          If(IsNull(a), Literal.create(null, a.dataType), FalseLiteral)
        }

Copy link
Member

@gengliangwang gengliangwang Sep 11, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it should be

      case a And b if Not(a).semanticEquals(b) || a.semanticEquals(Not(b)) =>
        if (!a.nullable && !b.nullable) {
          FalseLiteral
        } else if (a.nullable) {
          If(IsNull(a), Literal.create(null, a.dataType), FalseLiteral)
        } else {
          If(IsNull(b), Literal.create(null, b.dataType), FalseLiteral)
       }

But the current code should work. As the condition Not(a).semanticEquals(b) || a.semanticEquals(Not(b)) is satisfied, in current code it is impossible that a is not nullable and b is nullable.

gengliangwang
gengliangwang approved these changes Sep 11, 2018
View reviewed changes
Copy link
Member

@gengliangwang gengliangwang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otherwise LGTM 👍

...lyst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/BooleanSimplificationSuite.scala Outdated
testRelation.output, Seq(Row(1, 2, 3, "abc"))
)

val testNotnullableRelation = LocalRelation('a.int.notNull, 'b.int.notNull, 'c.int.notNull,
Copy link
Member

@gengliangwang gengliangwang Sep 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: testNotnullableRelation => testNotNullableRelation

...lyst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/BooleanSimplificationSuite.scala Outdated
'd.string.notNull, 'e.boolean.notNull, 'f.boolean.notNull, 'g.boolean.notNull,
'h.boolean.notNull)

val testNotnullableRelationWithData = LocalRelation.fromExternalRows(
Copy link
Member

@gengliangwang gengliangwang Sep 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

testNotnullableRelationWithData => testNotNullableRelationWithData

@SparkQA
Copy link

SparkQA commented Sep 11, 2018

Test build #95923 has finished for PR 22390 at commit cf863bb.

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

@SparkQA
Copy link

SparkQA commented Sep 11, 2018

Test build #95955 has finished for PR 22390 at commit b05052e.

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

@cloud-fan
Copy link
Contributor

cloud-fan commented Sep 12, 2018

LGTM

gengliangwang
gengliangwang approved these changes Sep 12, 2018
View reviewed changes
Copy link
Member

@gengliangwang gengliangwang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

adrian-ionescu
adrian-ionescu approved these changes Sep 12, 2018
View reviewed changes
Copy link
Contributor

@adrian-ionescu adrian-ionescu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@SparkQA
Copy link

SparkQA commented Sep 12, 2018

Test build #95976 has finished for PR 22390 at commit 61b2d55.

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

@gengliangwang
Copy link
Member

gengliangwang commented Sep 12, 2018

retest this please.

@SparkQA
Copy link

SparkQA commented Sep 12, 2018

Test build #95981 has finished for PR 22390 at commit 61b2d55.

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

asfgit pushed a commit that referenced this pull request Sep 12, 2018
@gatorsmile @cloud-fan
[SPARK-25402][SQL] Null handling in BooleanSimplification
40e4db0 
## 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 #22390 from gatorsmile/fixBooleanSimplification.

Authored-by: gatorsmile <[email protected]>
Signed-off-by: Wenchen Fan <[email protected]>
(cherry picked from commit 79cc597)
Signed-off-by: Wenchen Fan <[email protected]>
@asfgit asfgit closed this in 79cc597 Sep 12, 2018
asfgit pushed a commit that referenced this pull request Sep 12, 2018
@gatorsmile @cloud-fan
[SPARK-25402][SQL] Null handling in BooleanSimplification
db9c041 
## 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 #22390 from gatorsmile/fixBooleanSimplification.

Authored-by: gatorsmile <[email protected]>
Signed-off-by: Wenchen Fan <[email protected]>
(cherry picked from commit 79cc597)
Signed-off-by: Wenchen Fan <[email protected]>
@cloud-fan
Copy link
Contributor

cloud-fan commented Sep 12, 2018

thanks, merging to master/2.4/2.3!

@cloud-fan
Copy link
Contributor

cloud-fan commented Sep 12, 2018

can you send a new PR for 2.2? thanks

gatorsmile added a commit to gatorsmile/spark that referenced this pull request Sep 12, 2018
@gatorsmile
[SPARK-25402][SQL] Null handling in BooleanSimplification
7088371 
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 <[email protected]>
Signed-off-by: Wenchen Fan <[email protected]>
(cherry picked from commit 79cc597)
Signed-off-by: Wenchen Fan <[email protected]>
fjh100456 pushed a commit to fjh100456/spark that referenced this pull request Sep 13, 2018
@gatorsmile @fjh100456
[SPARK-25402][SQL] Null handling in BooleanSimplification
02fdbf7 
## 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#22390 from gatorsmile/fixBooleanSimplification.

Authored-by: gatorsmile <[email protected]>
Signed-off-by: Wenchen Fan <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cloud-fan cloud-fan cloud-fan left review comments

@gengliangwang gengliangwang gengliangwang approved these changes

+1 more reviewer

@adrian-ionescu adrian-ionescu adrian-ionescu approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@gatorsmile @SparkQA @cloud-fan @gengliangwang @adrian-ionescu