Skip to content

[SPARK-28375][SQL] Make pullupCorrelatedPredicate idempotent #25268

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
dilipbiswal wants to merge 4 commits into from
Closed

[SPARK-28375][SQL] Make pullupCorrelatedPredicate idempotent #25268

dilipbiswal wants to merge 4 commits into from

Conversation

@dilipbiswal
Copy link
Contributor

@dilipbiswal dilipbiswal commented Jul 26, 2019 •
edited
Loading

What changes were proposed in this pull request?

This PR makes the optimizer rule PullupCorrelatedPredicates idempotent.

How was this patch tested?

A new test PullupCorrelatedPredicatesSuite

@SparkQA
Copy link

SparkQA commented Jul 27, 2019

Test build #108239 has finished for PR 25268 at commit c27d840.

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

@maropu
Copy link
Member

maropu commented Jul 27, 2019

WIP?

dongjoon-hyun
dongjoon-hyun reviewed Jul 27, 2019
View reviewed changes
...src/test/scala/org/apache/spark/sql/catalyst/optimizer/PullupCorrelatedPredicatesSuite.scala Outdated
assert(optimized.resolved)
}

test("SPARK-28375 PullupCorrelatedPredicates in correlated subquery idiompotency check") {
Copy link
Member

@dongjoon-hyun dongjoon-hyun Jul 27, 2019

Choose a reason for hiding this comment

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

For me, this is an improvement PR. We don't need use JIRA id.

@dilipbiswal dilipbiswal changed the title [SPARK-28375][SQL] Make pullupCorrelatedPredicate idempotent [SPARK-28375][SQL][WIP] Make pullupCorrelatedPredicate idempotent Jul 27, 2019
@dilipbiswal
Copy link
Contributor Author

dilipbiswal commented Jul 27, 2019

@yeshengm The test results look ok. Can you please look at the changes and let me know what you think.. ?

@dilipbiswal dilipbiswal changed the title [SPARK-28375][SQL][WIP] Make pullupCorrelatedPredicate idempotent [SPARK-28375][SQL] Make pullupCorrelatedPredicate idempotent Jul 28, 2019
@SparkQA
Copy link

SparkQA commented Jul 28, 2019

Test build #108260 has finished for PR 25268 at commit e2be78c.

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

@yeshengm
Copy link
Contributor

yeshengm commented Jul 28, 2019

@dilipbiswal I added an idempotence checker recently. Could you rebase your branch on the latest master and test it again?

@SparkQA
Copy link

SparkQA commented Jul 29, 2019

Test build #108288 has finished for PR 25268 at commit 5a19309.

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

yeshengm
yeshengm reviewed Jul 29, 2019
View reviewed changes
...src/test/scala/org/apache/spark/sql/catalyst/optimizer/PullupCorrelatedPredicatesSuite.scala Outdated
comparePlans(optimized, doubleOptimized)
}

test("PullupCorrelatedPredicates exists correlated subquery idiompotency check") {
Copy link
Contributor

@yeshengm yeshengm Jul 29, 2019

Choose a reason for hiding this comment

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

nit: spell "idempotence"

yeshengm
yeshengm reviewed Jul 29, 2019
View reviewed changes
...src/test/scala/org/apache/spark/sql/catalyst/optimizer/PullupCorrelatedPredicatesSuite.scala Outdated
assert(optimized.resolved)
}

test("PullupCorrelatedPredicates in correlated subquery idiompotency check") {
Copy link
Contributor

@yeshengm yeshengm Jul 29, 2019

Choose a reason for hiding this comment

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

same here

yeshengm
yeshengm reviewed Jul 29, 2019
View reviewed changes
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/subquery.scala
}

private def rewriteSubQueries(plan: LogicalPlan, outerPlans: Seq[LogicalPlan]): LogicalPlan = {
def getJoinCondition(newCond: Seq[Expression], oldCond: Seq[Expression]): Seq[Expression] = {
Copy link
Contributor

@yeshengm yeshengm Jul 29, 2019

Choose a reason for hiding this comment

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

Is it true that this function is to workaround cases where the newCond is empty, while outer references is not empty? Maybe we should add some comment here since it might be tricky to understand...

yeshengm
yeshengm reviewed Jul 29, 2019
View reviewed changes
Copy link
Contributor

@yeshengm yeshengm left a comment

Choose a reason for hiding this comment

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

Overall LGTM, only some minor comments. Thanks! @dilipbiswal

@SparkQA
Copy link

SparkQA commented Jul 29, 2019

Test build #108295 has finished for PR 25268 at commit 3b224f0.

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

@dilipbiswal
Copy link
Contributor Author

dilipbiswal commented Jul 29, 2019

retest this please

@SparkQA
Copy link

SparkQA commented Jul 29, 2019

Test build #108308 has finished for PR 25268 at commit 3b224f0.

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

@dilipbiswal
Copy link
Contributor Author

dilipbiswal commented Jul 29, 2019

retest this please

@SparkQA
Copy link

SparkQA commented Jul 29, 2019

Test build #108339 has finished for PR 25268 at commit 3b224f0.

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

gatorsmile
gatorsmile reviewed Jul 30, 2019
View reviewed changes
Copy link
Member

@gatorsmile gatorsmile 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! Merged to master

@dilipbiswal
Copy link
Contributor Author

dilipbiswal commented Jul 31, 2019

Thanks a lot @yeshengm @gatorsmile

@dongjoon-hyun
Copy link
Member

dongjoon-hyun commented Jan 25, 2020

Hi, All.
When I saw this PR at the first, it was an improvement PR.
However, the JIRA is marked as correctness. Do you think we need to backport this?

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

Reviewers

@dongjoon-hyun dongjoon-hyun dongjoon-hyun left review comments

@gatorsmile gatorsmile gatorsmile left review comments

+1 more reviewer

@yeshengm yeshengm yeshengm left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

SQL

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@dilipbiswal @SparkQA @maropu @yeshengm @dongjoon-hyun @gatorsmile