test: Enable Comet shuffle in Spark SQL tests #210
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #210 +/- ##
============================================
- Coverage 33.41% 33.32% -0.09%
- Complexity 768 769 +1
============================================
Files 107 107
Lines 36329 37037 +708
Branches 7935 8106 +171
============================================
+ Hits 12138 12342 +204
- Misses 21643 22098 +455
- Partials 2548 2597 +49 ☔ View full report in Codecov by Sentry. |
sunchao
force-pushed
the
enable-shuffle-tests
branch
from
b58a3e8
to
7ec1500
Compare
sunchao
force-pushed
the
enable-shuffle-tests
branch
from
3662dab
to
77216a5
Compare
sunchao
force-pushed
the
enable-shuffle-tests
branch
from
8d4d8a4
to
11acec2
Compare
sunchao
force-pushed
the
enable-shuffle-tests
branch
from
11acec2
to
04c4aed
Compare
|
cc @viirya @advancedxy this PR is ready now |
advancedxy
reviewed
Mar 25, 2024
| @@ -1238,6 +1392,9 @@ index ed2e309fa07..3767d4e7ca4 100644 | |||
| + conf | |||
| + .set("spark.comet.exec.enabled", "true") | |||
| + .set("spark.comet.exec.all.enabled", "true") | |||
| + .set("spark.shuffle.manager", | |||
| + "org.apache.spark.sql.comet.execution.shuffle.CometShuffleManager") | |||
| + .set("spark.comet.exec.shuffle.enabled", "true") | |||
There was a problem hiding this comment.
what about comet columnar shuffle? I think we are prioritizing that as the first class shuffle support.
There was a problem hiding this comment.
It is still pending on the work to make it as default. Once it becomes default, all the tests will automatically switch to use it.
There was a problem hiding this comment.
I see, that makes sense.
|
Other changes lgtm. Only about whether we should enable columnar shuffle by default or not. |
advancedxy
approved these changes
Mar 25, 2024
viirya
reviewed
Mar 26, 2024
|
|
||
| - test("SPARK-35886: PromotePrecision should be subexpr replaced") { | ||
| + test("SPARK-35886: PromotePrecision should be subexpr replaced", | ||
| + IgnoreComet("TODO: fix Comet for this test")) { |
There was a problem hiding this comment.
This will be forgotten easily. Can we create ticket for that?
viirya
reviewed
Mar 26, 2024
|
|
||
| test("SPARK-38204: flatMapGroupsWithState should require StatefulOpClusteredDistribution " + | ||
| - "from children - without initial state") { | ||
| + "from children - without initial state", IgnoreComet("TODO: fix Comet for this test")) { |
viirya
reviewed
Mar 26, 2024
| test("SPARK-38204: flatMapGroupsWithState should require ClusteredDistribution " + | ||
| - "from children if the query starts from checkpoint in 3.2.x - without initial state") { | ||
| + "from children if the query starts from checkpoint in 3.2.x - without initial state", | ||
| + IgnoreComet("TODO: fix Comet for this test")) { |
viirya
reviewed
Mar 26, 2024
| test("SPARK-38204: flatMapGroupsWithState should require ClusteredDistribution " + | ||
| - "from children if the query starts from checkpoint in prior to 3.2") { | ||
| + "from children if the query starts from checkpoint in prior to 3.2", | ||
| + IgnoreComet("TODO: fix Comet for this test")) { |
viirya
reviewed
Mar 26, 2024
|
|
||
| case class Result(key: Long, count: Int) | ||
|
|
||
| +// TODO: fix Comet to enable this suite |
viirya
approved these changes
Mar 26, 2024
7 tasks
|
I created #231 to track these failed tests. |
|
Merged. Thanks. |
wangyum
pushed a commit
to wangyum/datafusion-comet
that referenced
this pull request
Mar 28, 2024
* test: Enable Comet shuffle in Spark SQL tests * disable some tests * disable another test * update * update
snmvaughan
pushed a commit
to snmvaughan/arrow-datafusion-comet
that referenced
this pull request
Apr 4, 2024
* test: Enable Comet shuffle in Spark SQL tests * disable some tests * disable another test * update * update
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Closes #195.
Rationale for this change
Currently the Spark SQL tests do not enable Comet shuffle yet, which causes it to miss quite a lot test coverage. This PR enables shuffle for them. It uses the default shuffle mechanism for now (Comet native shuffle).
I have to disable a few tests that are failing probably due to some bugs in Comet itself. Once this is merged, I'll create separate Github issues to track them.
What changes are included in this PR?
Enable Comet shuffle for Spark SQL tests.
How are these changes tested?