Skip to content

[SPARK-32741][SQL][FOLLOWUP] Run plan integrity check only for effective plan changes - #29928

Closed
maropu wants to merge 1 commit into
apache:masterfrom
maropu:PR29585-FOLLOWUP
Closed

[SPARK-32741][SQL][FOLLOWUP] Run plan integrity check only for effective plan changes#29928
maropu wants to merge 1 commit into
apache:masterfrom
maropu:PR29585-FOLLOWUP

Conversation

@maropu

@maropumaropu commented Oct 2, 2020•
edited
Loading

Copy link
Copy Markdown
Member

What changes were proposed in this pull request?

(This is a followup PR of #29585) The PR modified RuleExecutor#isPlanIntegral code for checking if a plan has globally-unique attribute IDs, but this check made Jenkins maven test jobs much longer (See the Dongjoon comment and thanks, @dongjoon-hyun !). To recover running time for the Jenkins tests, this PR intends to update the code to run plan integrity check only for effective plans.

Why are the changes needed?

To recover running time for Jenkins tests.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Existing tests.

@maropu

maropu commented Oct 2, 2020

Copy link
Copy Markdown
MemberAuthor

I will check the running time of Jenkins. cc: @dongjoon-hyun

@SparkQA

SparkQA commented Oct 2, 2020

Copy link
Copy Markdown

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

@dongjoon-hyun

dongjoon-hyun commented Oct 2, 2020

Copy link
Copy Markdown
Member

Thank you for this investigation, @maropu .
Also, cc @cloud-fan .

@dongjoon-hyun

dongjoon-hyun commented Oct 2, 2020•
edited
Loading

Copy link
Copy Markdown
Member

@maropu . Could you run Maven run? SBT run is parallelized by default.

@dongjoon-hyundongjoon-hyun changed the title [WIP][SPARK-32741][SQL][FOLLOWUP] Run plan integrity check only for effective plans[WIP][SPARK-32741][SQL][FOLLOWUP][test-maven] Run plan integrity check only for effective plansOct 2, 2020
@dongjoon-hyun

dongjoon-hyun commented Oct 2, 2020

Copy link
Copy Markdown
Member

Retest this please

@maropu

maropu commented Oct 2, 2020

Copy link
Copy Markdown
MemberAuthor

Thanks for the update, @dongjoon-hyun

@SparkQA

SparkQA commented Oct 2, 2020

Copy link
Copy Markdown

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

@SparkQA

SparkQA commented Oct 2, 2020

Copy link
Copy Markdown

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

@SparkQA

SparkQA commented Oct 2, 2020

Copy link
Copy Markdown

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

@SparkQA

SparkQA commented Oct 2, 2020

Copy link
Copy Markdown

Test build #129327 has finished for PR 29928 at commit 6582f90.

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

@SparkQA

SparkQA commented Oct 2, 2020

Copy link
Copy Markdown

Test build #129329 has finished for PR 29928 at commit 6582f90.

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

@maropu

maropu commented Oct 2, 2020

Copy link
Copy Markdown
MemberAuthor

okay, it seems the running time got recovered: https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/129327/

FYI: latest running test:
https://amplab.cs.berkeley.edu/jenkins/view/Spark%20QA%20Test%20(Dashboard)/job/spark-master-test-maven-hadoop-2.7-hive-2.3/
#848 => Took 8 hr 1 min (PR #29585 merged)
#847 => Took 8 hr 1 min (PR #29585 merged)
#846 => Took 6 hr 45 min
#845 => Took 6 hr 41 min
#844 => Took 6 hr 43 min

@maropu

maropu commented Oct 2, 2020

Copy link
Copy Markdown
MemberAuthor

retest this please

@maropumaropu changed the title [WIP][SPARK-32741][SQL][FOLLOWUP][test-maven] Run plan integrity check only for effective plans[SPARK-32741][SQL][FOLLOWUP][test-maven] Run plan integrity check only for effective plansOct 2, 2020
@maropumaropu changed the title [SPARK-32741][SQL][FOLLOWUP][test-maven] Run plan integrity check only for effective plans[SPARK-32741][SQL][FOLLOWUP][test-maven] Run plan integrity check only for effective plan changesOct 2, 2020
@dongjoon-hyun

dongjoon-hyun commented Oct 2, 2020

Copy link
Copy Markdown
Member

Oh, is it recovered? It's great, @maropu .

@dongjoon-hyundongjoon-hyun changed the title [SPARK-32741][SQL][FOLLOWUP][test-maven] Run plan integrity check only for effective plan changes[SPARK-32741][SQL][FOLLOWUP] Run plan integrity check only for effective plan changesOct 2, 2020
@dongjoon-hyun

dongjoon-hyun commented Oct 2, 2020

Copy link
Copy Markdown
Member

Thank you so much, @maropu .

@maropu

maropu commented Oct 2, 2020

Copy link
Copy Markdown
MemberAuthor

This PR is still an improvement follow-up. Did I understand correctly, @maropu ?

Yea, right, I think so. The latest test run seems much faster than the previous ones: Took 5 hr 6 min v.s. 6 hr 41-45 minhttps://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/129329/
I'm not 100% sure that this is true now, but probably, that's because this update skips the other unencessary integrity checks, too:

spark/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/Optimizer.scala

Line 49 in f7ba952

plan.find(PlanHelper.specialExpressionsInUnsupportedOperator(_).nonEmpty).isEmpty &&

@SparkQA

SparkQA commented Oct 2, 2020

Copy link
Copy Markdown

Test build #129331 has finished for PR 29928 at commit 6582f90.

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

@maropu

maropu commented Oct 2, 2020

Copy link
Copy Markdown
MemberAuthor

(I will re-run the test a couple of times before merging this..)

@dongjoon-hyun

dongjoon-hyun commented Oct 2, 2020

Copy link
Copy Markdown
Member

Sure, go ahead~

@SparkQA

SparkQA commented Oct 2, 2020

Copy link
Copy Markdown

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

@SparkQA

SparkQA commented Oct 2, 2020

Copy link
Copy Markdown

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

@SparkQA

SparkQA commented Oct 2, 2020

Copy link
Copy Markdown

Test build #129341 has finished for PR 29928 at commit 6582f90.

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

@maropu

maropu commented Oct 2, 2020

Copy link
Copy Markdown
MemberAuthor

I checked Took 5 hr 36 min in https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/129341/testReport/
This PR looks fine, so I'll merge this.

@maropumaropu closed this in 82721ceOct 2, 2020
@maropu

maropu commented Oct 2, 2020

Copy link
Copy Markdown
MemberAuthor

Thanks, @dongjoon-hyun ! Merged to master.

@SparkQA

SparkQA commented Oct 2, 2020

Copy link
Copy Markdown

Test build #129340 has finished for PR 29928 at commit 6582f90.

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

@dongjoon-hyun

dongjoon-hyun commented Oct 2, 2020•
edited
Loading

Copy link
Copy Markdown
Member

Thank you so much again.

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

Reviewers

@dongjoon-hyundongjoon-hyundongjoon-hyun approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@maropu@SparkQA@dongjoon-hyun