[SPARK-52741][SQL] RemoveFiles ShuffleCleanup mode doesnt work with non-adaptive execution #51432
Conversation
| cleanupShuffles() | ||
| } | ||
| } | ||
| } |
|
cc @peter-toth |
| @@ -350,14 +350,19 @@ class QueryExecutionSuite extends SharedSparkSession { | |||
| } | |||
|
|
|||
| test("SPARK-47764: Cleanup shuffle dependencies - RemoveShuffleFiles mode") { | |||
There was a problem hiding this comment.
I just realized that the test has a jira number. Should i create a new test with the jira id used for this change? @dongjoon-hyun please advise
There was a problem hiding this comment.
IMO you can extend the existing tests when the new functionality can be easily covered by those. But why didn't you adjust all SPARK-47764: ... tests?
There was a problem hiding this comment.
@peter-toth I modified only SPARK-47764: Cleanup shuffle dependencies - RemoveShuffleFiles mode, since my change was specific to org.apache.spark.sql.execution.RemoveShuffleFiles. I can add negatibve assertions to other and make them consistent . Let me know
There was a problem hiding this comment.
No, your PR affects other ShuffleCleanupModes too so the other 2 should work with both AQE enabled and disabled as well. All you need to do is to wrap those tests into an AQE on/off loop.
What changes were proposed in this pull request?
Currently, shuffle cleanup only works for adaptive execution plans. Non-adaptive execution plans are not cleaned up. Thing change cleans it.
Why are the changes needed?
Does this PR introduce any user-facing change?
No
How was this patch tested?
Modified existing unit tests to cover this case
Was this patch authored or co-authored using generative AI tooling?
No