test: Fix explain with exteded info comet test #436
Conversation
| @@ -1399,7 +1399,7 @@ class CometExpressionSuite extends CometTestBase with AdaptiveSparkPlanHelper { | |||
| CometConf.COMET_EXEC_ENABLED.key -> "true", | |||
| CometConf.COMET_SHUFFLE_ENFORCE_MODE_ENABLED.key -> "true", | |||
| CometConf.COMET_EXEC_ALL_OPERATOR_ENABLED.key -> "true", | |||
| "spark.sql.extendedExplainProvider" -> "org.apache.comet.ExtendedExplainInfo") { | |||
There was a problem hiding this comment.
So even we don't config it, the test still can pass?
There was a problem hiding this comment.
Currently it passes because, before Spark 4.0, supportsExtendedExplainInfo in CometTestBase.scala is always false.
There was a problem hiding this comment.
I meant previously the config name is wrong, so the config doesn't apply at all. So no matter with or without this config, this test passes?
There was a problem hiding this comment.
Correct, it passes with or without this config for Spark 3.x because the test is disabled by supportsExtendedExplainInfo that is false.
For Spark 4.0 or any Spark fork that implements the extended info, supportsExtendedExplainInfo returns true. Then it fails with the previous wrong config name.
There was a problem hiding this comment.
Hmm, but we don't run tests against Spark 4.0 currently, right?
Although it is okay for this PR as it is to fix incorrect config name, it sounds a little strange to have a test that can pass no matter what.
There was a problem hiding this comment.
Maybe we should move this test to Spark 4.0 particular test folder later.
There was a problem hiding this comment.
There is another part in the same test that is valid for all Spark versions.
Perhaps, we can consider separating them out in the future cc @parthchandra
Thanks for reviewing
There was a problem hiding this comment.
Will do that @kazuyukitanimura
| @@ -249,7 +249,7 @@ abstract class CometTestBase | |||
| var dfSpark: Dataset[Row] = null | |||
| withSQLConf( | |||
| CometConf.COMET_ENABLED.key -> "false", | |||
| "spark.sql.extendedExplainProvider" -> "") { | |||
| "spark.sql.extendedExplainProviders" -> "") { | |||
There was a problem hiding this comment.
nit: could we make this literal as a constant filed in this CometTestBase such as how we define org.apache.spark.sql.CometTestBase#shuffleManager?
It could be reused in CometExpressionSuite.scala as well.
There was a problem hiding this comment.
I agree, and do we really need to pass this param with blank value?
There was a problem hiding this comment.
Thanks,
nit: could we make this literal as a constant
I added the constant in a shim because it is defined in Spark 4.0
do we really need to pass this param with blank value?
Yes, the caller of this method (CometExpressionSuite) defines a value and need to overwrite.
| @@ -259,7 +259,7 @@ abstract class CometTestBase | |||
| dfSpark.queryExecution.explainString(ExtendedMode), | |||
| dfComet.queryExecution.explainString(ExtendedMode)) | |||
| if (supportsExtendedExplainInfo(dfSpark.queryExecution)) { | |||
| assert(diff.contains(expectedInfo)) | |||
| assert(expectedInfo.exists(s => diff.contains(s))) | |||
There was a problem hiding this comment.
Don't we need to have all expected info in the diff?
There was a problem hiding this comment.
Thanks
Updated
|
Merged. Thanks @kazuyukitanimura @andygrove @advancedxy @comphead |
|
Thank you all! |
* test: Fix explain with exteded info comet test * address review comments * address review comments (cherry picked from commit 7fe20b1)
Which issue does this PR close?
Closes #.
Rationale for this change
The test was not properly done due to a typo
What changes are included in this PR?
Fixed the typo as well as a string match bug
How are these changes tested?