[SPARK-29906][SQL][FOLLOWUP] Update the final plan in UI for AQE #26968
Conversation
|
Test build #115624 has finished for PR 26968 at commit
|
|
LGTM. Thanks |
dongjoon-hyun
changed the title
dongjoon-hyun
changed the title
dongjoon-hyun
left a comment
dongjoon-hyun
left a comment
There was a problem hiding this comment.
+1, LGTM. Merged to master.
|
@cloud-fan Is there any UT to catch such mistake that it won't happen again ? |
|
unfortunately we don't have a good test framework for UI. It would be great if we have one. |
|
|
||
| // Run the final plan when there's no more unfinished stages. | ||
| currentPhysicalPlan = applyPhysicalRules(result.newPlan, queryStageOptimizerRules) | ||
| executionId.foreach(onUpdatePlan) |
There was a problem hiding this comment.
Why is the plan sent out before isFinalPlan = true ? Isn't it the final plan ?
There was a problem hiding this comment.
It doesn't matter. onUpdatePlan only look at currentPhysicalPlan, so we must call onUpdatePlan after setting currentPhysicalPlan.
There was a problem hiding this comment.
Does it matter for what's displayed on UI or anyone interested in sql update event ?
There was a problem hiding this comment.
what do you mean? Even if you move isFinalPlan = true before this line, nothing gets changed.
There was a problem hiding this comment.
The root nodes of Physical Plan in physicalPlanDescriptions are different: AdaptiveSparkPlan(isFinalPlan=true) vs AdaptiveSparkPlan(isFinalPlan=false). So are root nodes of sparkPlan
There was a problem hiding this comment.
ah good catch! somehow I misread the code and thought it doesn't matter.
For UI it doesn't matter as we will strip the AdaptiveSparkPlanExec, but if there is a spark listener catching this event, it matters.
Can you send a PR to fix it? Thanks!
5 participants
What changes were proposed in this pull request?
a followup of #26576, which mistakenly removes the UI update of the final plan.
Why are the changes needed?
fix mistake.
Does this PR introduce any user-facing change?
no
How was this patch tested?
existing tests