You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "karuppayya (via GitHub)" <gi...@apache.org> on 2025/07/09 22:58:10 UTC
[PR] [SPARK-52741][SQL] RemoveFiles ShuffleCleanup mode doesnt work with non-adaptive execution [spark]
karuppayya opened a new pull request, #51432:
URL: https://github.com/apache/spark/pull/51432
### 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-52741][SQL] RemoveFiles ShuffleCleanup mode doesnt work with non-adaptive execution [spark]
Posted by "karuppayya (via GitHub)" <gi...@apache.org>.
karuppayya commented on code in PR #51432:
URL: https://github.com/apache/spark/pull/51432#discussion_r2201953275
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/QueryExecutionSuite.scala:
##########
@@ -350,14 +350,19 @@ class QueryExecutionSuite extends SharedSparkSession {
}
test("SPARK-47764: Cleanup shuffle dependencies - RemoveShuffleFiles mode") {
Review 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`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-52741][SQL] RemoveFiles ShuffleCleanup mode doesnt work with non-adaptive execution [spark]
Posted by "karuppayya (via GitHub)" <gi...@apache.org>.
karuppayya commented on code in PR #51432:
URL: https://github.com/apache/spark/pull/51432#discussion_r2201953275
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/QueryExecutionSuite.scala:
##########
@@ -350,14 +350,19 @@ class QueryExecutionSuite extends SharedSparkSession {
}
test("SPARK-47764: Cleanup shuffle dependencies - RemoveShuffleFiles mode") {
Review 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 and make them them consistent . Let me know
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-52741][SQL] RemoveFiles ShuffleCleanup mode doesnt work with non-adaptive execution [spark]
Posted by "karuppayya (via GitHub)" <gi...@apache.org>.
karuppayya commented on PR #51432:
URL: https://github.com/apache/spark/pull/51432#issuecomment-3186827968
@cloud-fan can you take a look if you have cycles
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-52741][SQL] RemoveFiles ShuffleCleanup mode doesnt work with non-adaptive execution [spark]
Posted by "peter-toth (via GitHub)" <gi...@apache.org>.
peter-toth commented on code in PR #51432:
URL: https://github.com/apache/spark/pull/51432#discussion_r2202421414
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/QueryExecutionSuite.scala:
##########
@@ -350,14 +350,19 @@ class QueryExecutionSuite extends SharedSparkSession {
}
test("SPARK-47764: Cleanup shuffle dependencies - RemoveShuffleFiles mode") {
Review Comment:
No, your PR affects other `ShuffleCleanupMode`s too so the other 2 should work with both AQE enabled and disabled as well.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-52741][SQL] RemoveFiles ShuffleCleanup mode doesnt work with non-adaptive execution [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #51432:
URL: https://github.com/apache/spark/pull/51432#discussion_r2213531758
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/QueryExecutionSuite.scala:
##########
@@ -350,14 +350,19 @@ class QueryExecutionSuite extends SharedSparkSession {
}
test("SPARK-47764: Cleanup shuffle dependencies - RemoveShuffleFiles mode") {
Review Comment:
I pinged the original author and committer, @karuppayya .
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-52741][SQL] RemoveFiles ShuffleCleanup mode doesnt work with non-adaptive execution [spark]
Posted by "karuppayya (via GitHub)" <gi...@apache.org>.
karuppayya commented on code in PR #51432:
URL: https://github.com/apache/spark/pull/51432#discussion_r2211935112
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/QueryExecutionSuite.scala:
##########
@@ -350,14 +350,19 @@ class QueryExecutionSuite extends SharedSparkSession {
}
test("SPARK-47764: Cleanup shuffle dependencies - RemoveShuffleFiles mode") {
Review Comment:
I have added blocks to other tests.
@peter-toth @dongjoon-hyun Can you take look please
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-52741][SQL] RemoveFiles ShuffleCleanup mode doesnt work with non-adaptive execution [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan closed pull request #51432: [SPARK-52741][SQL] RemoveFiles ShuffleCleanup mode doesnt work with non-adaptive execution
URL: https://github.com/apache/spark/pull/51432
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-52741][SQL] RemoveFiles ShuffleCleanup mode doesnt work with non-adaptive execution [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #51432:
URL: https://github.com/apache/spark/pull/51432#discussion_r2196361874
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/QueryExecutionSuite.scala:
##########
@@ -350,14 +350,19 @@ class QueryExecutionSuite extends SharedSparkSession {
}
test("SPARK-47764: Cleanup shuffle dependencies - RemoveShuffleFiles mode") {
- val plan = spark.range(100).repartition(10).logicalPlan
- val df = Dataset.ofRows(spark, plan, RemoveShuffleFiles)
- df.collect()
-
- val blockManager = spark.sparkContext.env.blockManager
- assert(blockManager.migratableResolver.getStoredShuffles().isEmpty)
- assert(blockManager.diskBlockManager.getAllBlocks().isEmpty)
- cleanupShuffles()
+ Seq(true, false).foreach { adaptiveEnabled => {
+ withSQLConf((SQLConf.ADAPTIVE_EXECUTION_ENABLED.key, adaptiveEnabled.toString)) {
+ val plan = spark.range(100).repartition(10).logicalPlan
+ val df = Dataset.ofRows(spark, plan, RemoveShuffleFiles)
+ df.collect()
+
+ val blockManager = spark.sparkContext.env.blockManager
+ assert(blockManager.migratableResolver.getStoredShuffles().isEmpty)
+ assert(blockManager.diskBlockManager.getAllBlocks().isEmpty)
+ cleanupShuffles()
+ }
+ }
+ }
Review Comment:
indentations?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-52741][SQL] RemoveFiles ShuffleCleanup mode doesnt work with non-adaptive execution [spark]
Posted by "peter-toth (via GitHub)" <gi...@apache.org>.
peter-toth commented on code in PR #51432:
URL: https://github.com/apache/spark/pull/51432#discussion_r2200140320
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/QueryExecutionSuite.scala:
##########
@@ -350,14 +350,19 @@ class QueryExecutionSuite extends SharedSparkSession {
}
test("SPARK-47764: Cleanup shuffle dependencies - RemoveShuffleFiles mode") {
Review 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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-52741][SQL] RemoveFiles ShuffleCleanup mode doesnt work with non-adaptive execution [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on PR #51432:
URL: https://github.com/apache/spark/pull/51432#issuecomment-3190989969
thanks, merging to master/4.0!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-52741][SQL] RemoveFiles ShuffleCleanup mode doesnt work with non-adaptive execution [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #51432:
URL: https://github.com/apache/spark/pull/51432#issuecomment-3055056337
cc @peter-toth
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-52741][SQL] RemoveFiles ShuffleCleanup mode doesnt work with non-adaptive execution [spark]
Posted by "karuppayya (via GitHub)" <gi...@apache.org>.
karuppayya commented on code in PR #51432:
URL: https://github.com/apache/spark/pull/51432#discussion_r2196484361
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/QueryExecutionSuite.scala:
##########
@@ -350,14 +350,19 @@ class QueryExecutionSuite extends SharedSparkSession {
}
test("SPARK-47764: Cleanup shuffle dependencies - RemoveShuffleFiles mode") {
Review 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-52741][SQL] RemoveFiles ShuffleCleanup mode doesnt work with non-adaptive execution [spark]
Posted by "karuppayya (via GitHub)" <gi...@apache.org>.
karuppayya commented on code in PR #51432:
URL: https://github.com/apache/spark/pull/51432#discussion_r2201953275
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/QueryExecutionSuite.scala:
##########
@@ -350,14 +350,19 @@ class QueryExecutionSuite extends SharedSparkSession {
}
test("SPARK-47764: Cleanup shuffle dependencies - RemoveShuffleFiles mode") {
Review 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-52741][SQL] RemoveFiles ShuffleCleanup mode doesnt work with non-adaptive execution [spark]
Posted by "peter-toth (via GitHub)" <gi...@apache.org>.
peter-toth commented on code in PR #51432:
URL: https://github.com/apache/spark/pull/51432#discussion_r2202421414
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/QueryExecutionSuite.scala:
##########
@@ -350,14 +350,19 @@ class QueryExecutionSuite extends SharedSparkSession {
}
test("SPARK-47764: Cleanup shuffle dependencies - RemoveShuffleFiles mode") {
Review Comment:
No, your PR affects other `ShuffleCleanupMode`s 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-52741][SQL] RemoveFiles ShuffleCleanup mode doesnt work with non-adaptive execution [spark]
Posted by "karuppayya (via GitHub)" <gi...@apache.org>.
karuppayya commented on code in PR #51432:
URL: https://github.com/apache/spark/pull/51432#discussion_r2212236249
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/QueryExecutionSuite.scala:
##########
@@ -350,14 +350,19 @@ class QueryExecutionSuite extends SharedSparkSession {
}
test("SPARK-47764: Cleanup shuffle dependencies - RemoveShuffleFiles mode") {
- val plan = spark.range(100).repartition(10).logicalPlan
- val df = Dataset.ofRows(spark, plan, RemoveShuffleFiles)
- df.collect()
-
- val blockManager = spark.sparkContext.env.blockManager
- assert(blockManager.migratableResolver.getStoredShuffles().isEmpty)
- assert(blockManager.diskBlockManager.getAllBlocks().isEmpty)
- cleanupShuffles()
+ Seq(true, false).foreach { adaptiveEnabled => {
+ withSQLConf((SQLConf.ADAPTIVE_EXECUTION_ENABLED.key, adaptiveEnabled.toString)) {
+ val plan = spark.range(100).repartition(10).logicalPlan
+ val df = Dataset.ofRows(spark, plan, RemoveShuffleFiles)
+ df.collect()
+
+ val blockManager = spark.sparkContext.env.blockManager
+ assert(blockManager.migratableResolver.getStoredShuffles().isEmpty)
+ assert(blockManager.diskBlockManager.getAllBlocks().isEmpty)
+ cleanupShuffles()
+ }
+ }
+ }
Review Comment:
Fixed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org