Copilot commented on code in PR #12506:
URL: https://github.com/apache/gluten/pull/12506#discussion_r3577651167
##########
gluten-ut/spark41/src/test/scala/org/apache/spark/sql/execution/GlutenRemoveRedundantProjectsSuite.scala:
##########
@@ -16,8 +16,192 @@
*/
package org.apache.spark.sql.execution
-import org.apache.spark.sql.GlutenSQLTestsTrait
+import org.apache.spark.sql.{GlutenSQLTestsTrait, Row}
+import org.apache.spark.sql.internal.SQLConf
+import org.apache.spark.sql.types.StructType
class GlutenRemoveRedundantProjectsSuite
extends RemoveRedundantProjectsSuite
- with GlutenSQLTestsTrait {}
+ with GlutenSQLTestsTrait {
+
+ // The original tests count Spark ProjectExec nodes, while Gluten converts
offloaded projects to
+ // ProjectExecTransformer. PullOutPreProject and PullOutPostProject may also
insert additional
+ // projects while rewriting the Spark physical plan, so project counts are
not directly
+ // comparable. Therefore, these tests only verify that query results are
identical with redundant
+ // project removal enabled and disabled.
+ private def assertProjectExec(query: String, enabled: Int, disabled: Int):
Unit = {
+ val df = sql(query)
+ // When enabling AQE, the DPP subquery filters is replaced in runtime.
+ df.collect()
+ // assertProjectExecCount(df, enabled)
+ val result = df.collect()
+ withSQLConf(SQLConf.REMOVE_REDUNDANT_PROJECTS_ENABLED.key -> "false") {
+ val df2 = sql(query)
+ df2.collect()
+ // assertProjectExecCount(df2, disabled)
+ checkAnswer(df2, result)
+ }
+ }
Review Comment:
This helper no longer guarantees that the first run is executed with
redundant-project removal enabled; it relies on whatever the ambient default
is, so the test can become a no-op comparison (disabled vs disabled) if the
default is changed. Consider explicitly setting
`SQLConf.REMOVE_REDUNDANT_PROJECTS_ENABLED` to `true` for the first run (e.g.,
wrapping both phases in `withSQLConf`). Also, `df.collect()` and
`df2.collect()` are executed twice/extra here; since `checkAnswer` will trigger
an action, it should be possible to reduce this to a single collection per
query to avoid recomputation and potential flakiness.
##########
gluten-ut/spark41/src/test/scala/org/apache/gluten/utils/velox/VeloxTestSettings.scala:
##########
@@ -713,10 +713,26 @@ class VeloxTestSettings extends BackendTestSettings {
enableSuite[GlutenLogicalPlanTagInSparkPlanSuite]
enableSuite[GlutenOptimizeMetadataOnlyQuerySuite]
enableSuite[GlutenPersistedViewTestSuite]
- // TODO: 4.x enableSuite[GlutenPlannerSuite] // 1 failure
- // TODO: 4.x enableSuite[GlutenProjectedOrderingAndPartitioningSuite] // 6
failures
+ // GlutenPlannerSuite is not enabled: it validates Spark planner
implementation details.
+ // GlutenProjectedOrderingAndPartitioningSuite is not enabled: it validates
Spark planner
+ // output ordering and partitioning metadata.
enableSuite[GlutenQueryPlanningTrackerEndToEndSuite]
- // TODO: 4.x enableSuite[GlutenRemoveRedundantProjectsSuite] // 14 failures
+ enableSuite[GlutenRemoveRedundantProjectsSuite]
+ // Rewrite as result checks because Gluten transforms and may pull out
additional projects.
+ .exclude("project with filter")
+ .exclude("project with specific column ordering")
+ .exclude("project with extra columns")
+ .exclude("project with fewer columns")
+ .exclude("aggregate without ordering requirement")
+ .exclude("aggregate with ordering requirement")
+ .exclude("join without ordering requirement")
+ .exclude("join with ordering requirement")
+ .exclude("window function")
+ .exclude("generate should require column ordering")
+ .exclude("subquery")
+ .exclude("SPARK-33697: UnionExec should require column ordering")
+ .exclude("SPARK-33697: remove redundant projects under expand")
+ .exclude("SPARK-36020: Project should not be removed when child's logical
link is different")
Review Comment:
The PR’s goal is to fix `GlutenRemoveRedundantProjectsSuite` for Spark 4.x,
but Velox test settings enable the suite and then exclude every test in it.
That effectively prevents CI from exercising the rewritten tests. If the tests
are now result-based and expected to pass, these exclusions should be removed
(or replaced with an allowlist like `.includeAllGlutenTests()` if that’s the
project convention). If they are still not ready to run, it would be clearer to
keep the suite disabled rather than enabling it with a full exclusion list.
##########
gluten-ut/spark41/src/test/scala/org/apache/spark/sql/execution/GlutenRemoveRedundantProjectsSuite.scala:
##########
@@ -16,8 +16,192 @@
*/
package org.apache.spark.sql.execution
-import org.apache.spark.sql.GlutenSQLTestsTrait
+import org.apache.spark.sql.{GlutenSQLTestsTrait, Row}
+import org.apache.spark.sql.internal.SQLConf
+import org.apache.spark.sql.types.StructType
class GlutenRemoveRedundantProjectsSuite
extends RemoveRedundantProjectsSuite
- with GlutenSQLTestsTrait {}
+ with GlutenSQLTestsTrait {
+
+ // The original tests count Spark ProjectExec nodes, while Gluten converts
offloaded projects to
+ // ProjectExecTransformer. PullOutPreProject and PullOutPostProject may also
insert additional
+ // projects while rewriting the Spark physical plan, so project counts are
not directly
+ // comparable. Therefore, these tests only verify that query results are
identical with redundant
+ // project removal enabled and disabled.
+ private def assertProjectExec(query: String, enabled: Int, disabled: Int):
Unit = {
+ val df = sql(query)
+ // When enabling AQE, the DPP subquery filters is replaced in runtime.
+ df.collect()
+ // assertProjectExecCount(df, enabled)
+ val result = df.collect()
+ withSQLConf(SQLConf.REMOVE_REDUNDANT_PROJECTS_ENABLED.key -> "false") {
+ val df2 = sql(query)
+ df2.collect()
+ // assertProjectExecCount(df2, disabled)
+ checkAnswer(df2, result)
+ }
+ }
+
+ testGluten("project with filter") {
+ val query = "select * from testView where a > 5"
+ assertProjectExec(query, 0, 1)
+ }
+
+ testGluten("project with specific column ordering") {
+ val query = "select key, a, b, c from testView"
+ assertProjectExec(query, 1, 1)
+ }
+
+ testGluten("project with extra columns") {
+ val query = "select a, b, c, key, a from testView"
+ assertProjectExec(query, 1, 1)
+ }
+
+ testGluten("project with fewer columns") {
+ val query = "select a from testView where a > 3"
+ assertProjectExec(query, 1, 1)
+ }
+
+ testGluten("aggregate without ordering requirement") {
+ val query = "select sum(a) as sum_a, key, last(b) as last_b " +
+ "from (select key, a, b from testView where a > 100) group by key"
+ assertProjectExec(query, 0, 1)
+ }
+
+ testGluten("aggregate with ordering requirement") {
+ val query = "select a, sum(b) as sum_b from testView group by a"
+ assertProjectExec(query, 1, 1)
+ }
+
+ testGluten("join without ordering requirement") {
+ val query = "select t1.key, t2.key, t1.a, t2.b from (select key, a, b, c
from testView)" +
+ " as t1 join (select key, a, b, c from testView) as t2 on t1.c > t2.c
and t1.key > 10"
+ assertProjectExec(query, 1, 3)
+ }
+
+ testGluten("join with ordering requirement") {
+ val query = "select * from (select key, a, c, b from testView) as t1 join
" +
+ "(select key, a, b, c from testView) as t2 on t1.key = t2.key where t2.a
> 50"
+ assertProjectExec(query, 2, 2)
+ }
+
+ testGluten("window function") {
+ val query = "select key, b, avg(a) over (partition by key order by a " +
+ "rows between 1 preceding and 1 following) as avg from testView"
+ assertProjectExec(query, 1, 2)
+ }
+
+ testGluten("generate should require column ordering") {
+ withTempView("testData") {
+ spark.range(0, 10, 1)
+ .selectExpr("id as key", "id * 2 as a", "id * 3 as b")
+ .createOrReplaceTempView("testData")
+
+ val data = sql("select key, a, b, count(*) from testData group by key,
a, b limit 2")
+ val df = data.selectExpr("a", "b", "key", "explode(array(key, a, b)) as
d").filter("d > 0")
+ df.collect()
+ val plan = df.queryExecution.executedPlan
+ val numProjects = collectWithSubqueries(plan) { case p: ProjectExec => p
}.length
+
+ // Create a new plan that reverse the GenerateExec output and add a new
ProjectExec between
+ // GenerateExec and its child. This is to test if the ProjectExec is
removed, the output of
+ // the query will be incorrect.
+ val newPlan = stripAQEPlan(plan).transform {
+ case g @ GenerateExec(_, requiredChildOutput, _, _, child) =>
+ g.copy(
+ requiredChildOutput = requiredChildOutput.reverse,
+ child = ProjectExec(requiredChildOutput.reverse, child))
+ }
+
+ // Re-apply remove redundant project rule.
+ val rule = RemoveRedundantProjects
+ val newExecutedPlan = rule.apply(newPlan)
+ // The manually added ProjectExec node shouldn't be removed.
+ // assert(collectWithSubqueries(newExecutedPlan) {
+ // case p: ProjectExec => p
+ // }.size == numProjects + 1)
Review Comment:
`numProjects` is currently computed but not used because the only consumer
assertion is commented out. If the assertion is intentionally disabled for
Gluten, consider removing `numProjects` (and the commented assertion block) to
avoid dead code / warnings. If you still want to preserve the intent of the
test, replace the commented assertion with a Gluten-appropriate invariant
(e.g., checking the presence/position of the manually inserted project node in
a way that matches Gluten’s transformed plan nodes).
##########
gluten-ut/spark41/src/test/scala/org/apache/spark/sql/execution/GlutenRemoveRedundantProjectsSuite.scala:
##########
@@ -16,8 +16,192 @@
*/
package org.apache.spark.sql.execution
-import org.apache.spark.sql.GlutenSQLTestsTrait
+import org.apache.spark.sql.{GlutenSQLTestsTrait, Row}
+import org.apache.spark.sql.internal.SQLConf
+import org.apache.spark.sql.types.StructType
class GlutenRemoveRedundantProjectsSuite
extends RemoveRedundantProjectsSuite
- with GlutenSQLTestsTrait {}
+ with GlutenSQLTestsTrait {
+
+ // The original tests count Spark ProjectExec nodes, while Gluten converts
offloaded projects to
+ // ProjectExecTransformer. PullOutPreProject and PullOutPostProject may also
insert additional
+ // projects while rewriting the Spark physical plan, so project counts are
not directly
+ // comparable. Therefore, these tests only verify that query results are
identical with redundant
+ // project removal enabled and disabled.
+ private def assertProjectExec(query: String, enabled: Int, disabled: Int):
Unit = {
+ val df = sql(query)
+ // When enabling AQE, the DPP subquery filters is replaced in runtime.
+ df.collect()
+ // assertProjectExecCount(df, enabled)
+ val result = df.collect()
+ withSQLConf(SQLConf.REMOVE_REDUNDANT_PROJECTS_ENABLED.key -> "false") {
+ val df2 = sql(query)
+ df2.collect()
+ // assertProjectExecCount(df2, disabled)
+ checkAnswer(df2, result)
+ }
+ }
+
+ testGluten("project with filter") {
+ val query = "select * from testView where a > 5"
+ assertProjectExec(query, 0, 1)
+ }
+
+ testGluten("project with specific column ordering") {
+ val query = "select key, a, b, c from testView"
+ assertProjectExec(query, 1, 1)
+ }
+
+ testGluten("project with extra columns") {
+ val query = "select a, b, c, key, a from testView"
+ assertProjectExec(query, 1, 1)
+ }
+
+ testGluten("project with fewer columns") {
+ val query = "select a from testView where a > 3"
+ assertProjectExec(query, 1, 1)
+ }
+
+ testGluten("aggregate without ordering requirement") {
+ val query = "select sum(a) as sum_a, key, last(b) as last_b " +
+ "from (select key, a, b from testView where a > 100) group by key"
+ assertProjectExec(query, 0, 1)
+ }
+
+ testGluten("aggregate with ordering requirement") {
+ val query = "select a, sum(b) as sum_b from testView group by a"
+ assertProjectExec(query, 1, 1)
+ }
+
+ testGluten("join without ordering requirement") {
+ val query = "select t1.key, t2.key, t1.a, t2.b from (select key, a, b, c
from testView)" +
+ " as t1 join (select key, a, b, c from testView) as t2 on t1.c > t2.c
and t1.key > 10"
+ assertProjectExec(query, 1, 3)
+ }
+
+ testGluten("join with ordering requirement") {
+ val query = "select * from (select key, a, c, b from testView) as t1 join
" +
+ "(select key, a, b, c from testView) as t2 on t1.key = t2.key where t2.a
> 50"
+ assertProjectExec(query, 2, 2)
+ }
+
+ testGluten("window function") {
+ val query = "select key, b, avg(a) over (partition by key order by a " +
+ "rows between 1 preceding and 1 following) as avg from testView"
+ assertProjectExec(query, 1, 2)
+ }
+
+ testGluten("generate should require column ordering") {
+ withTempView("testData") {
+ spark.range(0, 10, 1)
+ .selectExpr("id as key", "id * 2 as a", "id * 3 as b")
+ .createOrReplaceTempView("testData")
+
+ val data = sql("select key, a, b, count(*) from testData group by key,
a, b limit 2")
+ val df = data.selectExpr("a", "b", "key", "explode(array(key, a, b)) as
d").filter("d > 0")
+ df.collect()
+ val plan = df.queryExecution.executedPlan
+ val numProjects = collectWithSubqueries(plan) { case p: ProjectExec => p
}.length
Review Comment:
`numProjects` is currently computed but not used because the only consumer
assertion is commented out. If the assertion is intentionally disabled for
Gluten, consider removing `numProjects` (and the commented assertion block) to
avoid dead code / warnings. If you still want to preserve the intent of the
test, replace the commented assertion with a Gluten-appropriate invariant
(e.g., checking the presence/position of the manually inserted project node in
a way that matches Gluten’s transformed plan nodes).
##########
gluten-ut/spark40/src/test/scala/org/apache/gluten/utils/velox/VeloxTestSettings.scala:
##########
@@ -734,10 +734,26 @@ class VeloxTestSettings extends BackendTestSettings {
enableSuite[GlutenLogicalPlanTagInSparkPlanSuite]
enableSuite[GlutenOptimizeMetadataOnlyQuerySuite]
enableSuite[GlutenPersistedViewTestSuite]
- // TODO: 4.x enableSuite[GlutenPlannerSuite] // 1 failure
- // TODO: 4.x enableSuite[GlutenProjectedOrderingAndPartitioningSuite] // 6
failures
+ // GlutenPlannerSuite is not enabled: it validates Spark planner
implementation details.
+ // GlutenProjectedOrderingAndPartitioningSuite is not enabled: it validates
Spark planner
+ // output ordering and partitioning metadata.
enableSuite[GlutenQueryPlanningTrackerEndToEndSuite]
- // TODO: 4.x enableSuite[GlutenRemoveRedundantProjectsSuite] // 14 failures
+ enableSuite[GlutenRemoveRedundantProjectsSuite]
+ // Rewrite as result checks because Gluten transforms and may pull out
additional projects.
+ .exclude("project with filter")
+ .exclude("project with specific column ordering")
+ .exclude("project with extra columns")
+ .exclude("project with fewer columns")
+ .exclude("aggregate without ordering requirement")
+ .exclude("aggregate with ordering requirement")
+ .exclude("join without ordering requirement")
+ .exclude("join with ordering requirement")
+ .exclude("window function")
+ .exclude("generate should require column ordering")
+ .exclude("subquery")
+ .exclude("SPARK-33697: UnionExec should require column ordering")
+ .exclude("SPARK-33697: remove redundant projects under expand")
+ .exclude("SPARK-36020: Project should not be removed when child's logical
link is different")
Review Comment:
Same issue as Spark 4.1 settings: the suite is enabled but all tests are
excluded, so the intended fix isn’t validated in Velox CI runs. Consider
removing the exclusions if the rewritten tests are expected to pass, or leave
the suite disabled until it can run meaningfully.
##########
gluten-ut/spark41/src/test/scala/org/apache/spark/sql/execution/GlutenRemoveRedundantProjectsSuite.scala:
##########
@@ -16,8 +16,192 @@
*/
package org.apache.spark.sql.execution
-import org.apache.spark.sql.GlutenSQLTestsTrait
+import org.apache.spark.sql.{GlutenSQLTestsTrait, Row}
+import org.apache.spark.sql.internal.SQLConf
+import org.apache.spark.sql.types.StructType
class GlutenRemoveRedundantProjectsSuite
extends RemoveRedundantProjectsSuite
- with GlutenSQLTestsTrait {}
+ with GlutenSQLTestsTrait {
+
+ // The original tests count Spark ProjectExec nodes, while Gluten converts
offloaded projects to
+ // ProjectExecTransformer. PullOutPreProject and PullOutPostProject may also
insert additional
+ // projects while rewriting the Spark physical plan, so project counts are
not directly
+ // comparable. Therefore, these tests only verify that query results are
identical with redundant
+ // project removal enabled and disabled.
+ private def assertProjectExec(query: String, enabled: Int, disabled: Int):
Unit = {
+ val df = sql(query)
+ // When enabling AQE, the DPP subquery filters is replaced in runtime.
Review Comment:
Fix grammar in comment: change 'filters is replaced' to 'filters are
replaced' (same comment appears in the Spark 4.0 variant too).
--
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]
