-
Notifications
You must be signed in to change notification settings - Fork 28.5k
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.
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
[SPARK-32741][SQL][FOLLOWUP] Run plan integrity check only for effective plan changes #29928
Conversation
I will check the running time of Jenkins. cc: @dongjoon-hyun |
Kubernetes integration test starting |
Thank you for this investigation, @maropu . |
@maropu . Could you run |
Retest this please |
Thanks for the update, @dongjoon-hyun |
Kubernetes integration test status success |
Kubernetes integration test starting |
Kubernetes integration test status failure |
Test build #129327 has finished for PR 29928 at commit
|
Test build #129329 has finished for PR 29928 at commit
|
okay, it seems the running time got recovered: https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/129327/ FYI: latest running test: |
retest this please |
Oh, is it recovered? It's great, @maropu . |
Thank you so much, @maropu . |
Yea, right, I think so. The latest test run seems much faster than the previous ones: spark/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/Optimizer.scala Line 49 in f7ba952
|
Test build #129331 has finished for PR 29928 at commit
|
(I will re-run the test a couple of times before merging this..) |
Sure, go ahead~ |
Kubernetes integration test starting |
Kubernetes integration test status failure |
Test build #129341 has finished for PR 29928 at commit
|
I checked |
Thanks, @dongjoon-hyun ! Merged to master. |
Test build #129340 has finished for PR 29928 at commit
|
Thank you so much again. |
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.