[SPARK-28583][SQL] Subqueries should not call onUpdatePlan in Adaptive Query Execution
#25316
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Test build #108479 has finished for PR 25316 at commit
|
cloud-fan
reviewed
Aug 1, 2019
| // Apply the same instance of this rule to sub-queries so that sub-queries all share the | ||
| // same `stageCache` for Exchange reuse. | ||
| val adaptivePlan = this.apply(queryExec.sparkPlan) | ||
| val adaptivePlan = this.applyInternal(queryExec.sparkPlan, queryExec) |
Contributor
There was a problem hiding this comment.
When we reach here, it means we are creating AdaptiveSparkPlanExec for a subquery. Shall we simply set a boolean flag here (e.g. adaptivePlan.copy(isSubquery = true)) instead of passing around the QueryExecution?
Contributor
There was a problem hiding this comment.
nvm, this is more flexible, in case some places create QueryExecution without execution id and execute.
hvanhovell
reviewed
Aug 1, 2019
| session.sparkContext.getLocalProperty(SQLExecution.EXECUTION_ID_KEY)).flatMap { idStr => | ||
| val id = idStr.toLong | ||
| val qe = SQLExecution.getQueryExecution(id) | ||
| if (qe.eq(queryExecution)) Some(id) else None |
Contributor
There was a problem hiding this comment.
Can you add some doc on why this is needed. It is kinda annoying when you have to check the git blame to figure out why code is there.
|
Test build #108685 has finished for PR 25316 at commit
|
Contributor
|
LGTM if https://github.com/apache/spark/pull/25316/files#r309864918 is addressed |
Contributor
|
Merging to master |
|
Test build #108778 has finished for PR 25316 at commit
|
gatorsmile
pushed a commit
that referenced
this pull request
Jan 22, 2020
### What changes were proposed in this pull request? After [PR#25316](#25316) fixed the dead lock issue in [PR#25308](#25308), the subquery metrics can not be shown in UI as following screenshot.  This PR fix the subquery UI shown issue by adding `SparkListenerSQLAdaptiveSQLMetricUpdates` event to update the suquery sql metric. After with this PR, the suquery UI can show correctly as following screenshot:  ### Why are the changes needed? Showing the subquery metric in UI when enable AQE ### Does this PR introduce any user-facing change? No ### How was this patch tested? Existing UT Closes #27260 from JkSelf/fixSubqueryUI. Authored-by: jiake <[email protected]> Signed-off-by: Xiao Li <[email protected]>
j-baker
pushed a commit
to palantir/spark
that referenced
this pull request
Jan 25, 2020
…ive Query Execution ## What changes were proposed in this pull request? Subqueries do not have their own execution id, thus when calling `AdaptiveSparkPlanExec.onUpdatePlan`, it will actually get the `QueryExecution` instance of the main query, which is wasteful and problematic. It could cause issues like stack overflow or dead locks in some circumstances. This PR fixes this issue by making `AdaptiveSparkPlanExec` compare the `QueryExecution` object retrieved by current execution ID against the `QueryExecution` object from which this plan is created, and only update the UI when the two instances are the same. ## How was this patch tested? Manual tests on TPC-DS queries. Closes apache#25316 from maryannxue/aqe-updateplan-fix. Authored-by: maryannxue <[email protected]> Signed-off-by: herman <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
Subqueries do not have their own execution id, thus when calling
AdaptiveSparkPlanExec.onUpdatePlan, it will actually get theQueryExecutioninstance of the main query, which is wasteful and problematic. It could cause issues like stack overflow or dead locks in some circumstances.This PR fixes this issue by making
AdaptiveSparkPlanExeccompare theQueryExecutionobject retrieved by current execution ID against theQueryExecutionobject from which this plan is created, and only update the UI when the two instances are the same.How was this patch tested?
Manual tests on TPC-DS queries.