[SPARK-52741][SQL] RemoveFiles ShuffleCleanup mode doesnt work with non-adaptive execution #51432
Conversation
sql/core/src/test/scala/org/apache/spark/sql/execution/QueryExecutionSuite.scala
Outdated
Show resolved
Hide resolved
|
cc @peter-toth |
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.
There was a problem hiding this comment.
I have added blocks to other tests.
@peter-toth @dongjoon-hyun Can you take look please
There was a problem hiding this comment.
I pinged the original author and committer, @karuppayya .
There was a problem hiding this comment.
This seems to be introduced at Apache Spark 4.0.0 by
cc @bozhang2820 and @cloud-fan from #45930
|
@cloud-fan can you take a look if you have cycles |
|
thanks, merging to master/4.0! |
…on-adaptive execution ### 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? - To cleanup shuffle files of non-adaptive query executions - Consistency in behavior between adaptive and non-adaptive shuffle cleanup based on the cleanup mode ### 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 Closes #51432 from karuppayya/SPARK-52741. Lead-authored-by: Karuppayya Rajendran <[email protected]> Co-authored-by: Karuppayya Rajendran <[email protected]> Signed-off-by: Wenchen Fan <[email protected]> (cherry picked from commit 337a67f) Signed-off-by: Wenchen Fan <[email protected]>
…on-adaptive execution ### 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? - To cleanup shuffle files of non-adaptive query executions - Consistency in behavior between adaptive and non-adaptive shuffle cleanup based on the cleanup mode ### 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 Closes apache#51432 from karuppayya/SPARK-52741. Lead-authored-by: Karuppayya Rajendran <[email protected]> Co-authored-by: Karuppayya Rajendran <[email protected]> Signed-off-by: Wenchen Fan <[email protected]> (cherry picked from commit af9910d) Signed-off-by: Wenchen Fan <[email protected]>
4 participants
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