comphead commented on code in PR #6607:
URL: https://github.com/apache/datafusion-comet/pull/6607#discussion_r4197642414
##########
spark/src/test/scala/org/apache/spark/sql/comet/execution/shuffle/CometCelebornShufflePlanningSuite.scala:
##########
@@ -265,6 +265,21 @@ class CometCelebornShufflePlanningSuite extends
CometTestBase {
}
}
+ test("the shuffle input conversion leaves a Spark child to Spark's shuffle")
{
+ // The conversion only takes over from the JVM columnar shuffle, which
Celeborn does not
+ // have, and Celeborn's native shuffle needs a native child.
+ withSQLConf(
+ CometConf.COMET_SHUFFLE_MODE.key -> "native",
Review Comment:
This test sets `spark.comet.shuffle.mode` to `native`. In that mode
`columnarShuffleFailureReasons` already returns "Comet columnar shuffle not
enabled", so `shuffleSupported` is `None` and `convertsInputForNativeShuffle`
is false with or without Celeborn. Reading the code, the test would still pass
if the Celeborn check in `shuffleSupported` were removed. The suite default is
`auto`, and there the Celeborn check is the only thing that keeps a Spark child
on Spark's shuffle.
Could this test run in `auto` so that it covers the exclusion? I expect it
to pass there but have not run it.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
