mihailotim-db commented on code in PR #50606:
URL: https://github.com/apache/spark/pull/50606#discussion_r2048461271


##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/analysis/GroupByOrdinalsRepeatedAnalysisSuite.scala:
##########
@@ -17,63 +17,42 @@
 
 package org.apache.spark.sql.catalyst.analysis
 
-import org.apache.spark.sql.catalyst.analysis.TestRelations.{testRelation, 
testRelation2}
+import org.apache.spark.sql.catalyst.analysis.TestRelations.testRelation
 import org.apache.spark.sql.catalyst.dsl.expressions._
 import org.apache.spark.sql.catalyst.dsl.plans._
 import org.apache.spark.sql.catalyst.expressions.{GenericInternalRow, Literal}
 import org.apache.spark.sql.catalyst.plans.logical.LocalRelation
-import org.apache.spark.sql.internal.SQLConf
 
-class SubstituteUnresolvedOrdinalsSuite extends AnalysisTest {
-  private lazy val a = testRelation2.output(0)
-  private lazy val b = testRelation2.output(1)
+class GroupByOrdinalsRepeatedAnalysisSuite extends AnalysisTest {
 
   test("unresolved ordinal should not be unresolved") {
     // Expression OrderByOrdinal is unresolved.
     assert(!UnresolvedOrdinal(0).resolved)
   }
 
-  test("order by ordinal") {
-    // Tests order by ordinal, apply single rule.
-    val plan = testRelation2.orderBy(Literal(1).asc, Literal(2).asc)
+  test("SPARK-45920: group by ordinal repeated analysis") {
+    val plan = testRelation.groupBy(Literal(1))(Literal(100).as("a")).analyze
     comparePlans(
-      SubstituteUnresolvedOrdinals.apply(plan),
-      testRelation2.orderBy(UnresolvedOrdinal(1).asc, 
UnresolvedOrdinal(2).asc))
-
-    // Tests order by ordinal, do full analysis
-    checkAnalysis(plan, testRelation2.orderBy(a.asc, b.asc))
+      plan,
+      testRelation.groupBy(Literal(1))(Literal(100).as("a")).analyze
+    )
 
-    // order by ordinal can be turned off by config
-    withSQLConf(SQLConf.ORDER_BY_ORDINAL.key -> "false") {

Review Comment:
   We are removing `SubstituteUnresolvedOrdinals` object and we also have 
golden file tests for these cases so I think it would be redundant to rewrite 
them again



-- 
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: reviews-unsubscr...@spark.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

Reply via email to