Skip to content

[SPARK-33722][SQL] Handle DELETE in ReplaceNullWithFalseInPredicate #30688

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
wants to merge 1 commit into from
Closed

[SPARK-33722][SQL] Handle DELETE in ReplaceNullWithFalseInPredicate #30688

wants to merge 1 commit into from

Conversation

@aokolnychyi
Copy link
Contributor

@aokolnychyi aokolnychyi commented Dec 9, 2020

What changes were proposed in this pull request?

This PR adds DeleteFromTable to supported plans in ReplaceNullWithFalseInPredicate.

Why are the changes needed?

This change allows Spark to optimize delete conditions like we optimize filters.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

This PR extends the existing test cases to also cover DeleteFromTable.

@github-actions github-actions bot added the SQL label Dec 9, 2020
@aokolnychyi
Copy link
Contributor Author

aokolnychyi commented Dec 9, 2020

cc @dongjoon-hyun @holdenk @viirya @sunchao @dbtsai @cloud-fan @HyukjinKwon

@dbtsai
Copy link
Member

dbtsai commented Dec 9, 2020

LGTM. Pending tests.

@SparkQA
Copy link

SparkQA commented Dec 9, 2020

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

@SparkQA
Copy link

SparkQA commented Dec 9, 2020

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

dongjoon-hyun
dongjoon-hyun approved these changes Dec 9, 2020
View reviewed changes
Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

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

+1, LGTM. Thanks, @aokolnychyi and @dbtsai .
Merged to master for Apache Spark 3.2.0

viirya
viirya reviewed Dec 9, 2020
View reviewed changes
Copy link
Member

@viirya viirya 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 Dec 9, 2020

Test build #132502 has finished for PR 30688 at commit 61ca405.

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

cloud-fan
cloud-fan reviewed Dec 10, 2020
View reviewed changes
...src/main/scala/org/apache/spark/sql/catalyst/optimizer/ReplaceNullWithFalseInPredicate.scala
def apply(plan: LogicalPlan): LogicalPlan = plan transform {
case f @ Filter(cond, _) => f.copy(condition = replaceNullWithFalse(cond))
case j @ Join(_, _, _, Some(cond), _) => j.copy(condition = Some(replaceNullWithFalse(cond)))
case d @ DeleteFromTable(_, Some(cond)) => d.copy(condition = Some(replaceNullWithFalse(cond)))
Copy link
Contributor

@cloud-fan cloud-fan Dec 10, 2020

Choose a reason for hiding this comment

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

how about UPDATE and MERGE? they have conditions as well.

Copy link
Member

@dongjoon-hyun dongjoon-hyun Dec 10, 2020

Choose a reason for hiding this comment

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

It's beyond of this scope of this PR~

Copy link
Member

@HyukjinKwon HyukjinKwon Dec 10, 2020

Choose a reason for hiding this comment

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

Yeah, it's fine to fix it separately. But still how about fixing them in a separate PR @aokolnychyi?

Copy link
Member

@dongjoon-hyun dongjoon-hyun Dec 10, 2020

Choose a reason for hiding this comment

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

Sure, I guess so. There are many things on the way in this area.

Copy link
Contributor Author

@aokolnychyi aokolnychyi Dec 10, 2020

Choose a reason for hiding this comment

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

I will follow up for sure! Just wanted to keep the scope small.

Copy link
Member

@HyukjinKwon HyukjinKwon Dec 10, 2020

Choose a reason for hiding this comment

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

Thanks, @aokolnychyi.

flyrain pushed a commit to flyrain/spark that referenced this pull request Sep 21, 2021
@aokolnychyi @dongjoon-hyun
[SPARK-33722][SQL] Handle DELETE in ReplaceNullWithFalseInPredicate
e7a37e8 
### What changes were proposed in this pull request?

This PR adds `DeleteFromTable` to supported plans in `ReplaceNullWithFalseInPredicate`.

### Why are the changes needed?

This change allows Spark to optimize delete conditions like we optimize filters.

### Does this PR introduce _any_ user-facing change?

No.

### How was this patch tested?

This PR extends the existing test cases to also cover `DeleteFromTable`.

Closes apache#30688 from aokolnychyi/spark-33722.

Authored-by: Anton Okolnychyi <[email protected]>
Signed-off-by: Dongjoon Hyun <[email protected]>
(cherry picked from commit fa9ce1d)
Signed-off-by: Dongjoon Hyun <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@viirya viirya viirya left review comments

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

@HyukjinKwon HyukjinKwon HyukjinKwon left review comments

@dongjoon-hyun dongjoon-hyun dongjoon-hyun approved these changes

Assignees

No one assigned

Labels

SQL

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@aokolnychyi @dbtsai @SparkQA @viirya @cloud-fan @HyukjinKwon @dongjoon-hyun