[SPARK-53413] Shuffle cleanup for commands #52157
Conversation
|
@cloud-fan @ulysses-you Please help review when you get a chance to. |
| case exec: ShuffleExchangeLike => | ||
| exec.shuffleId | ||
| } | ||
| case command: V2CommandExec => |
There was a problem hiding this comment.
how about other commands?
There was a problem hiding this comment.
I have handled the DataWritingCommandExec,
I think it might be tricky for ExecutedCommandExec/V1Commands, since the spark plan is created internally.
Should we enumerate each SparkPlan type and handle them?
| @@ -150,7 +150,7 @@ class QueryExecution( | |||
| // with the rest of processing of the root plan being just outputting command results, | |||
| // for eagerly executed commands we mark this place as beginning of execution. | |||
| tracker.setReadyForExecution() | |||
| val qe = sparkSession.sessionState.executePlan(p, mode) | |||
| val qe = sparkSession.sessionState.executePlan(p, mode, shuffleCleanupMode) | |||
There was a problem hiding this comment.
instead of calling executePlan, can we construct QueryExecution instance directly? I feel this executePlan method is a bit useless.
There was a problem hiding this comment.
This seems to have been introduced here. It doesnt mention the rationale behind. But like you mentioned it does nt serve any real purpose, creating new QueryExecution
|
Why do we need this handling in commands? What about other flows? Or how is this different from other flows? |
| case exec: ShuffleExchangeLike => | ||
| exec.shuffleId | ||
| } | ||
| case command: V2CommandExec => |
There was a problem hiding this comment.
I have handled the DataWritingCommandExec,
I think it might be tricky for ExecutedCommandExec/V1Commands, since the spark plan is created internally.
Should we enumerate each SparkPlan type and handle them?
| @@ -3586,7 +3586,8 @@ object SQLConf { | |||
| val CLASSIC_SHUFFLE_DEPENDENCY_FILE_CLEANUP_ENABLED = | |||
| buildConf("spark.sql.classic.shuffleDependency.fileCleanup.enabled") | |||
| .doc("When enabled, shuffle files will be cleaned up at the end of classic " + | |||
| "SQL executions.") | |||
| "SQL executions. Note that this cleanup may cause stage retries and regenerate " + | |||
There was a problem hiding this comment.
This is related to this comment
karuppayya
force-pushed
the
SPARK-53413
branch
from
ef482e2 to
25406bc
Compare
All other flows were handled here |
What changes were proposed in this pull request?
Changes to cleanup shuffle generated from running commands(eg writes)
This was also brought by @cloud-fan and @ulysses-you here
Why are the changes needed?
To cleanupshuffle generated from commands
Does this PR introduce any user-facing change?
No
How was this patch tested?
Unit test added
Was this patch authored or co-authored using generative AI tooling?
No