Copilot commented on code in PR #12507:
URL: https://github.com/apache/gluten/pull/12507#discussion_r3577621613
##########
gluten-ut/spark41/src/test/scala/org/apache/spark/sql/execution/GlutenWholeStageCodegenSuite.scala:
##########
@@ -16,6 +16,748 @@
*/
package org.apache.spark.sql.execution
-import org.apache.spark.sql.GlutenSQLTestsTrait
+import org.apache.gluten.config.GlutenConfig
+import org.apache.gluten.execution._
-class GlutenWholeStageCodegenSuite extends WholeStageCodegenSuite with
GlutenSQLTestsTrait {}
+import org.apache.spark.SparkConf
+import org.apache.spark.sql.{Dataset, GlutenSQLTestsTrait, Row, SaveMode}
+import org.apache.spark.sql.functions._
+import org.apache.spark.sql.internal.SQLConf
+import org.apache.spark.sql.types.{IntegerType, StringType, StructType}
+
+import scala.reflect.ClassTag
+
+class GlutenWholeStageCodegenSuite extends WholeStageCodegenSuite with
GlutenSQLTestsTrait {
Review Comment:
This suite extends `WholeStageCodegenSuite` and then defines
`testGluten(...)` cases using the same test names as Spark’s original tests
(per the comment at lines 33–35). In ScalaTest, duplicate test names in a
single suite typically cause a `DuplicateTestNameException` during suite
construction, regardless of later filtering/excluding. To avoid that, either
(mandatory): (1) stop extending `WholeStageCodegenSuite` and instead mix in
only the shared fixtures you need, or (2) prevent the parent tests from being
registered (e.g., override the test registration method to ignore/skip only the
specific parent test names before they register), or (3) rename the Gluten
reimplemented tests (e.g., prefix with `Gluten:`) and exclude only the parent
tests by their original names.
##########
gluten-ut/spark41/src/test/scala/org/apache/spark/sql/execution/GlutenWholeStageCodegenSuite.scala:
##########
@@ -16,6 +16,748 @@
*/
package org.apache.spark.sql.execution
-import org.apache.spark.sql.GlutenSQLTestsTrait
+import org.apache.gluten.config.GlutenConfig
+import org.apache.gluten.execution._
-class GlutenWholeStageCodegenSuite extends WholeStageCodegenSuite with
GlutenSQLTestsTrait {}
+import org.apache.spark.SparkConf
+import org.apache.spark.sql.{Dataset, GlutenSQLTestsTrait, Row, SaveMode}
+import org.apache.spark.sql.functions._
+import org.apache.spark.sql.internal.SQLConf
+import org.apache.spark.sql.types.{IntegerType, StringType, StructType}
+
+import scala.reflect.ClassTag
+
+class GlutenWholeStageCodegenSuite extends WholeStageCodegenSuite with
GlutenSQLTestsTrait {
+ import testImplicits._
+
+ // Spark's tests inspect WholeStageCodegenExec and row-based operators.
Gluten replaces them
+ // with WholeStageTransformer and native operator transformers, so the
excluded parent tests
+ // are repeated below with Gluten-aware plan assertions while preserving
their result checks.
+ // Disable the forced shuffled hash join rewrite so explicit join hints
retain their semantics.
+ override def sparkConf: SparkConf = {
+ super.sparkConf
+ .set(GlutenConfig.COLUMNAR_FORCE_SHUFFLED_HASH_JOIN_ENABLED.key, "false")
+ }
+
+ private def assertWholeStageCount[T <: SparkPlan: ClassTag](
+ df: Dataset[_],
+ expectedCount: Int): Unit = {
+ val targetClass = implicitly[ClassTag[T]].runtimeClass
+ val plan = df.queryExecution.executedPlan
+ val stages = plan.collect {
+ case stage: WholeStageTransformer if
stage.child.exists(targetClass.isInstance) => stage
+ }
+ assert(
+ stages.size === expectedCount,
+ s"Expected $expectedCount WholeStageTransformer stage(s) containing " +
+ s"${targetClass.getSimpleName}, but found
${stages.size}:\n${plan.treeString}"
+ )
+ }
+
+ private def assertWholeStageContains[T <: SparkPlan: ClassTag](df:
Dataset[_]): Unit = {
+ val targetClass = implicitly[ClassTag[T]].runtimeClass
+ val plan = df.queryExecution.executedPlan
+ assert(
+ plan.exists {
+ case stage: WholeStageTransformer =>
stage.child.exists(targetClass.isInstance)
+ case _ => false
+ },
+ s"Expected a WholeStageTransformer containing
${targetClass.getSimpleName}:\n" +
+ plan.treeString
+ )
+ }
+
+ private def assertWholeStage(df: Dataset[_]): Unit = {
+
assert(df.queryExecution.executedPlan.exists(_.isInstanceOf[WholeStageTransformer]))
+ }
+
+ private def assertShuffledJoinStageCount(
+ df: Dataset[_],
+ hint: String,
+ expectedCount: Int): Unit = {
+ if (hint == "SHUFFLE_HASH") {
+ assertWholeStageCount[ShuffledHashJoinExecTransformer](df, expectedCount)
+ } else {
+ assertWholeStageCount[SortMergeJoinExecTransformer](df, expectedCount)
+ }
+ }
+
+ testGluten("range/filter should be combined") {
Review Comment:
This suite extends `WholeStageCodegenSuite` and then defines
`testGluten(...)` cases using the same test names as Spark’s original tests
(per the comment at lines 33–35). In ScalaTest, duplicate test names in a
single suite typically cause a `DuplicateTestNameException` during suite
construction, regardless of later filtering/excluding. To avoid that, either
(mandatory): (1) stop extending `WholeStageCodegenSuite` and instead mix in
only the shared fixtures you need, or (2) prevent the parent tests from being
registered (e.g., override the test registration method to ignore/skip only the
specific parent test names before they register), or (3) rename the Gluten
reimplemented tests (e.g., prefix with `Gluten:`) and exclude only the parent
tests by their original names.
##########
gluten-ut/spark40/src/test/scala/org/apache/spark/sql/execution/GlutenWholeStageCodegenSuite.scala:
##########
@@ -16,6 +16,748 @@
*/
package org.apache.spark.sql.execution
-import org.apache.spark.sql.GlutenSQLTestsTrait
+import org.apache.gluten.config.GlutenConfig
+import org.apache.gluten.execution._
-class GlutenWholeStageCodegenSuite extends WholeStageCodegenSuite with
GlutenSQLTestsTrait {}
+import org.apache.spark.SparkConf
+import org.apache.spark.sql.{Dataset, GlutenSQLTestsTrait, Row, SaveMode}
+import org.apache.spark.sql.functions._
+import org.apache.spark.sql.internal.SQLConf
+import org.apache.spark.sql.types.{IntegerType, StringType, StructType}
+
+import scala.reflect.ClassTag
+
+class GlutenWholeStageCodegenSuite extends WholeStageCodegenSuite with
GlutenSQLTestsTrait {
Review Comment:
Same issue as Spark 4.1: the suite extends `WholeStageCodegenSuite` and
re-declares many tests using identical names. This is very likely to fail suite
initialization due to duplicate test name registration. Recommended fix is the
same as above: either don’t extend `WholeStageCodegenSuite`, or block parent
test registration, or rename the Gluten tests so names don’t collide.
##########
gluten-ut/spark40/src/test/scala/org/apache/spark/sql/execution/GlutenWholeStageCodegenSuite.scala:
##########
@@ -16,6 +16,748 @@
*/
package org.apache.spark.sql.execution
-import org.apache.spark.sql.GlutenSQLTestsTrait
+import org.apache.gluten.config.GlutenConfig
+import org.apache.gluten.execution._
-class GlutenWholeStageCodegenSuite extends WholeStageCodegenSuite with
GlutenSQLTestsTrait {}
+import org.apache.spark.SparkConf
+import org.apache.spark.sql.{Dataset, GlutenSQLTestsTrait, Row, SaveMode}
+import org.apache.spark.sql.functions._
+import org.apache.spark.sql.internal.SQLConf
+import org.apache.spark.sql.types.{IntegerType, StringType, StructType}
+
+import scala.reflect.ClassTag
+
+class GlutenWholeStageCodegenSuite extends WholeStageCodegenSuite with
GlutenSQLTestsTrait {
+ import testImplicits._
+
+ // Spark's tests inspect WholeStageCodegenExec and row-based operators.
Gluten replaces them
+ // with WholeStageTransformer and native operator transformers, so the
excluded parent tests
+ // are repeated below with Gluten-aware plan assertions while preserving
their result checks.
+ // Disable the forced shuffled hash join rewrite so explicit join hints
retain their semantics.
+ override def sparkConf: SparkConf = {
+ super.sparkConf
+ .set(GlutenConfig.COLUMNAR_FORCE_SHUFFLED_HASH_JOIN_ENABLED.key, "false")
+ }
+
+ private def assertWholeStageCount[T <: SparkPlan: ClassTag](
+ df: Dataset[_],
+ expectedCount: Int): Unit = {
+ val targetClass = implicitly[ClassTag[T]].runtimeClass
+ val plan = df.queryExecution.executedPlan
+ val stages = plan.collect {
+ case stage: WholeStageTransformer if
stage.child.exists(targetClass.isInstance) => stage
+ }
+ assert(
+ stages.size === expectedCount,
+ s"Expected $expectedCount WholeStageTransformer stage(s) containing " +
+ s"${targetClass.getSimpleName}, but found
${stages.size}:\n${plan.treeString}"
+ )
+ }
+
+ private def assertWholeStageContains[T <: SparkPlan: ClassTag](df:
Dataset[_]): Unit = {
+ val targetClass = implicitly[ClassTag[T]].runtimeClass
+ val plan = df.queryExecution.executedPlan
+ assert(
+ plan.exists {
+ case stage: WholeStageTransformer =>
stage.child.exists(targetClass.isInstance)
+ case _ => false
+ },
+ s"Expected a WholeStageTransformer containing
${targetClass.getSimpleName}:\n" +
+ plan.treeString
+ )
+ }
+
+ private def assertWholeStage(df: Dataset[_]): Unit = {
+
assert(df.queryExecution.executedPlan.exists(_.isInstanceOf[WholeStageTransformer]))
+ }
+
+ private def assertShuffledJoinStageCount(
+ df: Dataset[_],
+ hint: String,
+ expectedCount: Int): Unit = {
+ if (hint == "SHUFFLE_HASH") {
+ assertWholeStageCount[ShuffledHashJoinExecTransformer](df, expectedCount)
+ } else {
+ assertWholeStageCount[SortMergeJoinExecTransformer](df, expectedCount)
+ }
+ }
+
+ testGluten("range/filter should be combined") {
Review Comment:
Same issue as Spark 4.1: the suite extends `WholeStageCodegenSuite` and
re-declares many tests using identical names. This is very likely to fail suite
initialization due to duplicate test name registration. Recommended fix is the
same as above: either don’t extend `WholeStageCodegenSuite`, or block parent
test registration, or rename the Gluten tests so names don’t collide.
##########
gluten-ut/spark41/src/test/scala/org/apache/gluten/utils/velox/VeloxTestSettings.scala:
##########
@@ -738,8 +738,36 @@ class VeloxTestSettings extends BackendTestSettings {
enableSuite[GlutenUnsafeFixedWidthAggregationMapSuite]
enableSuite[GlutenUnsafeKVExternalSorterSuite]
enableSuite[GlutenUnsafeRowSerializerSuite]
- // TODO: 4.x enableSuite[GlutenWholeStageCodegenSparkSubmitSuite] // 1
failure
- // TODO: 4.x enableSuite[GlutenWholeStageCodegenSuite] // 24 failures
+ // WholeStageCodegenSparkSubmitSuite is not enabled: the SparkSubmit test
launches Spark's
+ // main class without the Gluten plugin.
+ enableSuite[GlutenWholeStageCodegenSuite]
+ // Rewrite with Gluten-aware native whole-stage plan assertions.
+ .exclude("range/filter should be combined")
+ .exclude("HashAggregate should be included in WholeStageCodegen")
+ .exclude("SortAggregate should be included in WholeStageCodegen")
Review Comment:
The comments say the failing parent tests were rewritten with Gluten-aware
assertions, but the settings exclude those test names. If the rewritten tests
keep the same names (as in `GlutenWholeStageCodegenSuite`), these excludes will
also exclude the rewritten tests, so the rewrite won’t actually run. After
resolving the duplicate-name problem, either (mandatory) remove these excludes
so the rewritten tests execute, or (alternative) rename the rewritten tests and
keep excludes only for the original parent test names.
##########
gluten-ut/spark41/src/test/scala/org/apache/spark/sql/execution/GlutenWholeStageCodegenSuite.scala:
##########
@@ -16,6 +16,748 @@
*/
package org.apache.spark.sql.execution
-import org.apache.spark.sql.GlutenSQLTestsTrait
+import org.apache.gluten.config.GlutenConfig
+import org.apache.gluten.execution._
-class GlutenWholeStageCodegenSuite extends WholeStageCodegenSuite with
GlutenSQLTestsTrait {}
+import org.apache.spark.SparkConf
+import org.apache.spark.sql.{Dataset, GlutenSQLTestsTrait, Row, SaveMode}
+import org.apache.spark.sql.functions._
+import org.apache.spark.sql.internal.SQLConf
+import org.apache.spark.sql.types.{IntegerType, StringType, StructType}
+
+import scala.reflect.ClassTag
+
+class GlutenWholeStageCodegenSuite extends WholeStageCodegenSuite with
GlutenSQLTestsTrait {
+ import testImplicits._
+
+ // Spark's tests inspect WholeStageCodegenExec and row-based operators.
Gluten replaces them
+ // with WholeStageTransformer and native operator transformers, so the
excluded parent tests
+ // are repeated below with Gluten-aware plan assertions while preserving
their result checks.
+ // Disable the forced shuffled hash join rewrite so explicit join hints
retain their semantics.
+ override def sparkConf: SparkConf = {
+ super.sparkConf
+ .set(GlutenConfig.COLUMNAR_FORCE_SHUFFLED_HASH_JOIN_ENABLED.key, "false")
+ }
+
+ private def assertWholeStageCount[T <: SparkPlan: ClassTag](
+ df: Dataset[_],
+ expectedCount: Int): Unit = {
+ val targetClass = implicitly[ClassTag[T]].runtimeClass
+ val plan = df.queryExecution.executedPlan
+ val stages = plan.collect {
+ case stage: WholeStageTransformer if
stage.child.exists(targetClass.isInstance) => stage
+ }
+ assert(
+ stages.size === expectedCount,
+ s"Expected $expectedCount WholeStageTransformer stage(s) containing " +
+ s"${targetClass.getSimpleName}, but found
${stages.size}:\n${plan.treeString}"
+ )
+ }
+
+ private def assertWholeStageContains[T <: SparkPlan: ClassTag](df:
Dataset[_]): Unit = {
+ val targetClass = implicitly[ClassTag[T]].runtimeClass
+ val plan = df.queryExecution.executedPlan
+ assert(
+ plan.exists {
+ case stage: WholeStageTransformer =>
stage.child.exists(targetClass.isInstance)
+ case _ => false
+ },
+ s"Expected a WholeStageTransformer containing
${targetClass.getSimpleName}:\n" +
+ plan.treeString
+ )
+ }
+
+ private def assertWholeStage(df: Dataset[_]): Unit = {
+
assert(df.queryExecution.executedPlan.exists(_.isInstanceOf[WholeStageTransformer]))
+ }
+
+ private def assertShuffledJoinStageCount(
+ df: Dataset[_],
+ hint: String,
+ expectedCount: Int): Unit = {
+ if (hint == "SHUFFLE_HASH") {
+ assertWholeStageCount[ShuffledHashJoinExecTransformer](df, expectedCount)
+ } else {
+ assertWholeStageCount[SortMergeJoinExecTransformer](df, expectedCount)
+ }
+ }
+
+ testGluten("range/filter should be combined") {
+ val df = spark.range(10).filter("id = 1").selectExpr("id + 1")
+ assertWholeStage(df)
+ checkAnswer(df, Row(2))
+ }
+
+ testGluten("HashAggregate should be included in WholeStageCodegen") {
Review Comment:
This suite extends `WholeStageCodegenSuite` and then defines
`testGluten(...)` cases using the same test names as Spark’s original tests
(per the comment at lines 33–35). In ScalaTest, duplicate test names in a
single suite typically cause a `DuplicateTestNameException` during suite
construction, regardless of later filtering/excluding. To avoid that, either
(mandatory): (1) stop extending `WholeStageCodegenSuite` and instead mix in
only the shared fixtures you need, or (2) prevent the parent tests from being
registered (e.g., override the test registration method to ignore/skip only the
specific parent test names before they register), or (3) rename the Gluten
reimplemented tests (e.g., prefix with `Gluten:`) and exclude only the parent
tests by their original names.
##########
gluten-ut/spark40/src/test/scala/org/apache/spark/sql/execution/GlutenWholeStageCodegenSuite.scala:
##########
@@ -16,6 +16,748 @@
*/
package org.apache.spark.sql.execution
-import org.apache.spark.sql.GlutenSQLTestsTrait
+import org.apache.gluten.config.GlutenConfig
+import org.apache.gluten.execution._
-class GlutenWholeStageCodegenSuite extends WholeStageCodegenSuite with
GlutenSQLTestsTrait {}
+import org.apache.spark.SparkConf
+import org.apache.spark.sql.{Dataset, GlutenSQLTestsTrait, Row, SaveMode}
+import org.apache.spark.sql.functions._
+import org.apache.spark.sql.internal.SQLConf
+import org.apache.spark.sql.types.{IntegerType, StringType, StructType}
+
+import scala.reflect.ClassTag
+
+class GlutenWholeStageCodegenSuite extends WholeStageCodegenSuite with
GlutenSQLTestsTrait {
+ import testImplicits._
+
+ // Spark's tests inspect WholeStageCodegenExec and row-based operators.
Gluten replaces them
+ // with WholeStageTransformer and native operator transformers, so the
excluded parent tests
+ // are repeated below with Gluten-aware plan assertions while preserving
their result checks.
+ // Disable the forced shuffled hash join rewrite so explicit join hints
retain their semantics.
+ override def sparkConf: SparkConf = {
+ super.sparkConf
+ .set(GlutenConfig.COLUMNAR_FORCE_SHUFFLED_HASH_JOIN_ENABLED.key, "false")
+ }
+
+ private def assertWholeStageCount[T <: SparkPlan: ClassTag](
+ df: Dataset[_],
+ expectedCount: Int): Unit = {
+ val targetClass = implicitly[ClassTag[T]].runtimeClass
+ val plan = df.queryExecution.executedPlan
+ val stages = plan.collect {
+ case stage: WholeStageTransformer if
stage.child.exists(targetClass.isInstance) => stage
+ }
+ assert(
+ stages.size === expectedCount,
+ s"Expected $expectedCount WholeStageTransformer stage(s) containing " +
+ s"${targetClass.getSimpleName}, but found
${stages.size}:\n${plan.treeString}"
+ )
+ }
+
+ private def assertWholeStageContains[T <: SparkPlan: ClassTag](df:
Dataset[_]): Unit = {
+ val targetClass = implicitly[ClassTag[T]].runtimeClass
+ val plan = df.queryExecution.executedPlan
+ assert(
+ plan.exists {
+ case stage: WholeStageTransformer =>
stage.child.exists(targetClass.isInstance)
+ case _ => false
+ },
+ s"Expected a WholeStageTransformer containing
${targetClass.getSimpleName}:\n" +
+ plan.treeString
+ )
+ }
+
+ private def assertWholeStage(df: Dataset[_]): Unit = {
+
assert(df.queryExecution.executedPlan.exists(_.isInstanceOf[WholeStageTransformer]))
+ }
+
+ private def assertShuffledJoinStageCount(
+ df: Dataset[_],
+ hint: String,
+ expectedCount: Int): Unit = {
+ if (hint == "SHUFFLE_HASH") {
+ assertWholeStageCount[ShuffledHashJoinExecTransformer](df, expectedCount)
+ } else {
+ assertWholeStageCount[SortMergeJoinExecTransformer](df, expectedCount)
+ }
+ }
+
+ testGluten("range/filter should be combined") {
+ val df = spark.range(10).filter("id = 1").selectExpr("id + 1")
+ assertWholeStage(df)
+ checkAnswer(df, Row(2))
+ }
+
+ testGluten("HashAggregate should be included in WholeStageCodegen") {
Review Comment:
Same issue as Spark 4.1: the suite extends `WholeStageCodegenSuite` and
re-declares many tests using identical names. This is very likely to fail suite
initialization due to duplicate test name registration. Recommended fix is the
same as above: either don’t extend `WholeStageCodegenSuite`, or block parent
test registration, or rename the Gluten tests so names don’t collide.
##########
gluten-ut/spark40/src/test/scala/org/apache/gluten/utils/velox/VeloxTestSettings.scala:
##########
@@ -759,8 +759,36 @@ class VeloxTestSettings extends BackendTestSettings {
enableSuite[GlutenUnsafeFixedWidthAggregationMapSuite]
enableSuite[GlutenUnsafeKVExternalSorterSuite]
enableSuite[GlutenUnsafeRowSerializerSuite]
- // TODO: 4.x enableSuite[GlutenWholeStageCodegenSparkSubmitSuite] // 1
failure
- // TODO: 4.x enableSuite[GlutenWholeStageCodegenSuite] // 24 failures
+ // WholeStageCodegenSparkSubmitSuite is not enabled: the SparkSubmit test
launches Spark's
+ // main class without the Gluten plugin.
+ enableSuite[GlutenWholeStageCodegenSuite]
+ // Rewrite with Gluten-aware native whole-stage plan assertions.
+ .exclude("range/filter should be combined")
+ .exclude("HashAggregate should be included in WholeStageCodegen")
+ .exclude("SortAggregate should be included in WholeStageCodegen")
Review Comment:
Same as Spark 4.1 settings: excluding the rewritten test names prevents the
updated Gluten-aware tests from running if they share names with the originals.
Once the duplicate-name issue is addressed, update this exclude list
accordingly (remove excludes or rename rewritten tests so only the parent
originals are filtered).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
