Copilot commented on code in PR #12506:
URL: https://github.com/apache/gluten/pull/12506#discussion_r3576963866
##########
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:
`assertProjectExec` currently triggers multiple actions (two `collect()` on
the enabled DF, plus a `collect()` and `checkAnswer` on the disabled DF), so
each test executes the same query up to 4 times. This makes the suite
significantly slower and the extra `collect()` calls no longer serve a purpose
now that plan-node counting is commented out. Also, the helper claims to
compare enabled vs disabled behavior but only explicitly sets the disabled
side; setting the enabled side explicitly makes the test intent clearer and
avoids relying on defaults.
##########
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 now unused because the corresponding assertion is commented
out. Keeping both the unused value and the commented assertion block adds noise
and can confuse future readers about what this test is actually validating.
##########
gluten-ut/spark40/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 now unused because the corresponding assertion is commented
out. Keeping both the unused value and the commented assertion block adds noise
and can confuse future readers about what this test is actually validating.
##########
gluten-ut/spark40/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:
`assertProjectExec` currently triggers multiple actions (two `collect()` on
the enabled DF, plus a `collect()` and `checkAnswer` on the disabled DF), so
each test executes the same query up to 4 times. This makes the suite
significantly slower and the extra `collect()` calls no longer serve a purpose
now that plan-node counting is commented out. Also, the helper claims to
compare enabled vs disabled behavior but only explicitly sets the disabled
side; setting the enabled side explicitly makes the test intent clearer and
avoids relying on defaults.
##########
gluten-ut/spark41/src/test/scala/org/apache/gluten/utils/velox/VeloxTestSettings.scala:
##########
@@ -713,10 +713,25 @@ 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]
+ .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:
This suite is being enabled but the config enumerates exclusions for each
upstream Spark test case name. Since this class now defines Gluten-prefixed
replacements via `testGluten`, it’s more robust to include only Gluten tests
(by prefix) rather than maintaining a long exclusion list that must be kept in
sync with upstream Spark’s suite as it evolves.
##########
gluten-ut/spark40/src/test/scala/org/apache/gluten/utils/velox/VeloxTestSettings.scala:
##########
@@ -734,10 +734,25 @@ 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]
+ .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:
This suite is being enabled but the config enumerates exclusions for each
upstream Spark test case name. Since this class now defines Gluten-prefixed
replacements via `testGluten`, it’s more robust to include only Gluten tests
(by prefix) rather than maintaining a long exclusion list that must be kept in
sync with upstream Spark’s suite as it evolves.
--
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]
