test: enable ignored 4.0 tests, enable ansi mode#3454
test: enable ignored 4.0 tests, enable ansi mode#3454parthchandra wants to merge 4 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
|
@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. |
dev/diffs/4.0.1.diff
Outdated
| - val aqePlanRoot = findNodeInSparkPlanInfo(inMemoryScanNode.get, | ||
| - _.nodeName.contains("ResultQueryStage")) | ||
| - aqePlanRoot.get.children.head.nodeName == "AQEShuffleRead" | ||
| + aqeNode.get.children.head.nodeName == "AQEShuffleRead" || | ||
| + (aqeNode.get.children.head.nodeName.contains("WholeStageCodegen") && | ||
| + aqeNode.get.children.head.children.head.nodeName == "ColumnarToRow" && | ||
| + aqeNode.get.children.head.children.head.children.head.nodeName == "InputAdapter" && | ||
| + aqeNode.get.children.head.children.head.children.head.children.head.nodeName == | ||
| + "AQEShuffleRead") | ||
| + // Spark 4.0 wraps results in ResultQueryStage. The coalescing indicator is AQEShuffleRead | ||
| + // as the direct child of InputAdapter. | ||
| + // AdaptiveSparkPlan -> ResultQueryStage -> WholestageCodegen -> | ||
| + // CometColumnarToRow -> InputAdapter -> AQEShuffleRead (if coalesced) | ||
| + val resultStage = aqeNode.get.children.head // ResultQueryStage | ||
| + val wsc = resultStage.children.head // WholeStageCodegen | ||
| + val c2r = wsc.children.head // ColumnarToRow or CometColumnarToRow | ||
| + val inputAdapter = c2r.children.head // InputAdapter | ||
| + inputAdapter.children.head.nodeName == "AQEShuffleRead" |
There was a problem hiding this comment.
This new test is not equivalent because some of the nodeName tests are skipped.
Should the change be something like
...
(aqeNode.get.children.head.children.head.nodeName == "ColumnarToRow" ||
aqeNode.get.children.head.children.head.nodeName == "CometColumnarToRow") &&
...
There was a problem hiding this comment.
Added the check for the node names. Apart from the CometColumnarToRow, the test was failing because there is an additional ResultQuerryStage. in Spark 4.0
| test("Tags set from session are prefixed with session UUID") { | ||
| + // This test relies on job scheduling order which is timing-dependent and becomes unreliable | ||
| + // when Comet is enabled due to changes in async execution behaviour. | ||
| + assume(!classic.SparkSession.isCometEnabled, | ||
| + "Skipped when Comet is enabled: test results are timing-dependent") |
There was a problem hiding this comment.
Not a blocker but we use
IgnoreComet
For skipping tsets
There was a problem hiding this comment.
IgnoreComet is only available for SQL tests. This is a core test. (See similar comment above)
3 participants
enabling tests that fail in 4.0, and with ansi mode enabled to see what still fails