test: enable ignored 4.0 tests, enable ansi mode#3454
test: enable ignored 4.0 tests, enable ansi mode#3454parthchandra wants to merge 2 commits intoapache:mainfrom
Conversation
parthchandra
changed the title
parthchandra
force-pushed
the
spark-4.0-support
branch
2 times, most recently
from
eedc50a to
e08426c
Compare
parthchandra
changed the title
| @@ -231,6 +231,11 @@ class FallbackStorageSuite extends SparkFunSuite with LocalSparkContext { | ||
| } | ||
|
|
||
| test("Upload from all decommissioned executors") { |
There was a problem hiding this comment.
should these tests just be tagged with IgnoreComet rather than adding code?
There was a problem hiding this comment.
IgnoreComet only works with tests extending SQLTestUtils (which overrides the test method). FallbackStorageSuite is a core module test and does not extendt SQLTestUtils.
To override the test method in SparkFunSuite and also in SQLTestUtils one would end up probably creating a new mixin trait for IgnoreComet tests and I didn't want to complicate things with this PR. Also, it doesn't seem like we are likely to need it too often in core modules.
dev/diffs/4.0.1.diff
Outdated
| + */ | ||
| + protected def enableCometAnsiMode: Boolean = { | ||
| + val v = System.getenv("ENABLE_COMET_ANSI_MODE") | ||
| + v != null && v.toBoolean | ||
| + if (v != null) v.toBoolean else true | ||
| + } |
There was a problem hiding this comment.
This method can be removed. We no longer have a config in Comet for this.
There was a problem hiding this comment.
(of course, can be done in a separate PR)
There was a problem hiding this comment.
Actually, nm, it looks like this is for enabling Spark ANSI mode, not Comet ANSI mode, so maybe the method name and env var name are just confusing. I will take another look at this tomorrow.
There was a problem hiding this comment.
Spark 4 Ansi mode is the default. This may be leftover from 3.5.
kazuyukitanimura
left a comment
kazuyukitanimura
left a comment
There was a problem hiding this comment.
Thank you @parthchandra
dev/diffs/4.0.1.diff
Outdated
| @@ -390,7 +396,8 @@ class KeyGroupedPartitioningSuite extends DistributionAndOrderingSuiteBase { | ||
| assert(collectAllShuffles(df.queryExecution.executedPlan).isEmpty, | ||
| "should contain no shuffle when sorting by partition values") | ||
| } else { | ||
| - assert(collectAllShuffles(df.queryExecution.executedPlan).size == 1, | ||
| + val s = collectAllShuffles(df.queryExecution.executedPlan) | ||
| + assert(s.size == 1, | ||
| "should contain one shuffle when optimization is disabled") | ||
| } | ||
| checkAnswer(df, answer) |
There was a problem hiding this comment.
Do we need this part of diff?
dev/diffs/4.0.1.diff
Outdated
| + protected def enableCometAnsiMode: Boolean = { | ||
| + val v = System.getenv("ENABLE_COMET_ANSI_MODE") | ||
| + v != null && v.toBoolean | ||
| + if (v != null) v.toBoolean else true |
There was a problem hiding this comment.
If enabling by default, is it more like
v == null || v == "1" || v.toBoolean
| case GlobalLimitExec(_, s: SortExec, _) => !s.global | ||
| case GlobalLimitExec(_, ProjectExec(_, s: SortExec), _) => !s.global | ||
| + case CometGlobalLimitExec(_, _, _, _, s: CometSortExec, _) => | ||
| + !s.originalPlan.asInstanceOf[SortExec].global |
There was a problem hiding this comment.
We do not need b, I guess we can directly return find
|
@kazuyukitanimura @andygrove thank you for looking at this PR. I found an issue with this diff. Let me change this to draft. |
parthchandra
force-pushed
the
spark-4.0-support
branch
from
513946c to
6c4ba5c
Compare
|
@andygrove @kazuyukitanimura this is ready for review again. |
3 participants
enabling tests that fail in 4.0, and with ansi mode enabled to see what still fails