[SPARK-28560][SQL][followup] resolve the remaining comments for PR#25295 #26157
Conversation
|
@maryannxue @cloud-fan Please help me review. Thanks. |
|
ok to test |
| @@ -229,6 +229,8 @@ class AdaptiveQueryExecSuite | |||
| assert(smj.size == 3) | |||
| val bhj = findTopLevelBroadcastHashJoin(adaptivePlan) | |||
| assert(bhj.size == 2) | |||
| checkNumLocalShuffleReaders(adaptivePlan, 1) | |||
There was a problem hiding this comment.
there are 2 broadcast joins, but we only have 1 local shuffle reader. Why is it?
There was a problem hiding this comment.
checkNumLocalShuffleReaders method should call collect(plan) not call plan.collect. I have updated and check all the local shuffle reader number in existing test methods. Thanks.
|
Test build #112256 has finished for PR 26157 at commit
|
|
Test build #112264 has finished for PR 26157 at commit
|
|
thanks, merging to master! |
|
Looks like this PR introduces flaky test for "AdaptiveQueryExecSuite.multiple joins". The test hadn't been failing but started to fail around 3 days ago, and the failed assertion is added here. @JkSelf could you check this? Thanks in advance. |
|
Another observation: the test fails consistently when the test is run alone. The test passes when the test suite is run, but still be flaky. |
|
@HeartSaVioR Thanks for your great findings. I will research it later. Thanks. |
What changes were proposed in this pull request?
A followup of #25295.
OptimizeLocalShuffleReader.Why are the changes needed?
make code robust
Does this PR introduce any user-facing change?
No
How was this patch tested?
existing tests