Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test: Fix explain with exteded info comet test #436

Merged
merged 3 commits into from
May 16, 2024
Merged

test: Fix explain with exteded info comet test #436

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e844a0e
test: Fix explain with exteded info comet test
kazuyukitanimura May 15, 2024
f5380e5
address review comments
kazuyukitanimura May 16, 2024
1bde6ea
address review comments
kazuyukitanimura May 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion spark/src/test/scala/org/apache/comet/CometExpressionSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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") {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So even we don't config it, the test still can pass?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently it passes because, before Spark 4.0, supportsExtendedExplainInfo in CometTestBase.scala is always false.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@viirya viirya May 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should move this test to Spark 4.0 particular test folder later.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do that @kazuyukitanimura

"spark.sql.extendedExplainProviders" -> "org.apache.comet.ExtendedExplainInfo") {
val table = "test"
withTable(table) {
sql(s"create table $table(c0 int, c1 int , c2 float) using parquet")
Expand Down
4 changes: 2 additions & 2 deletions spark/src/test/scala/org/apache/spark/sql/CometTestBase.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -249,7 +249,7 @@ abstract class CometTestBase
var dfSpark: Dataset[Row] = null
withSQLConf(
CometConf.COMET_ENABLED.key -> "false",
"spark.sql.extendedExplainProvider" -> "") {
"spark.sql.extendedExplainProviders" -> "") {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, and do we really need to pass this param with blank value?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

dfSpark = Dataset.ofRows(spark, df.logicalPlan)
expected = dfSpark.collect()
}
Expand All @@ -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)))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't we need to have all expected info in the diff?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks

Updated

}
val extendedInfo =
new ExtendedExplainInfo().generateExtendedInfo(dfComet.queryExecution.executedPlan)
Expand Down
Loading