Skip to content

Conversation

@wangyum
Copy link
Member

@wangyum wangyum commented Jan 6, 2021

What changes were proposed in this pull request?

This pr address #30865 (review) to fix simplify conditional in predicate should consider deterministic.

Why are the changes needed?

Fix bug.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Unit test.

CaseWhen(Seq((UnresolvedAttribute("i") > Rand(0), FalseLiteral)))
val expectedCond = And(UnresolvedAttribute("i") > Rand(0), FalseLiteral)
// nondeterministic expressions are only allowed in Project, Filter, Aggregate or Window,
testFilter(originalCond, expectedCond = FalseLiteral)
Copy link
Member Author

Choose a reason for hiding this comment

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

expectedCond = FalseLiteral cause by

case FalseLiteral And _ => FalseLiteral
case _ And FalseLiteral => FalseLiteral
case TrueLiteral Or _ => TrueLiteral
case _ Or TrueLiteral => TrueLiteral

Or(Not(cond), trueValue)
case CaseWhen(Seq((_, FalseLiteral)), Some(FalseLiteral) | None) =>
FalseLiteral
case CaseWhen(Seq((cond, FalseLiteral)), Some(elseValue)) =>
Copy link
Member Author

Choose a reason for hiding this comment

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

This case has handled by

case CaseWhen(Seq((cond, trueValue)),
Some(FalseLiteral) | Some(Literal(null, BooleanType)) | None) =>
And(cond, trueValue)

Comment on lines -71 to -72
case CaseWhen(Seq((cond, TrueLiteral)), Some(FalseLiteral) | None) =>
cond
Copy link
Member Author

Choose a reason for hiding this comment

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

This case has handled by

case CaseWhen(Seq((cond, trueValue)),
Some(FalseLiteral) | Some(Literal(null, BooleanType)) | None) =>
And(cond, trueValue)

And(cond, trueValue)
case CaseWhen(Seq((cond, trueValue)), Some(TrueLiteral)) =>
Or(Not(cond), trueValue)
case CaseWhen(Seq((_, FalseLiteral)), Some(FalseLiteral) | None) =>
Copy link
Contributor

Choose a reason for hiding this comment

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

can we update the classdoc of this rule?

@SparkQA
Copy link

SparkQA commented Jan 6, 2021

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/38328/

@SparkQA
Copy link

SparkQA commented Jan 6, 2021

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/38328/

@SparkQA
Copy link

SparkQA commented Jan 6, 2021

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/38337/

@SparkQA
Copy link

SparkQA commented Jan 6, 2021

Test build #133749 has finished for PR 31067 at commit fbd837d.

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

@SparkQA
Copy link

SparkQA commented Jan 6, 2021

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/38337/

@SparkQA
Copy link

SparkQA commented Jan 6, 2021

Test build #133740 has finished for PR 31067 at commit 3ceffeb.

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

@github-actions github-actions bot added the SQL label Jan 6, 2021
Copy link
Member

@HyukjinKwon HyukjinKwon left a comment

Choose a reason for hiding this comment

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

LGTM thanks for handling this

@HyukjinKwon
Copy link
Member

Merged to master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants