[GitHub] [spark] peter-toth commented on a diff in pull request #36146: [SPARK-38404][SQL] Improve CTE resolution when a nested CTE references an outer CTE

[GitBox <gi...@apache.org>]

peter-toth commented on code in PR #36146:
URL: https://github.com/apache/spark/pull/36146#discussion_r854249361


##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/CTESubstitution.scala:
##########
@@ -221,31 +227,18 @@ object CTESubstitution extends Rule[LogicalPlan] {
         //   WITH t3 AS (SELECT * FROM t1)
         // )
         // t3 should resolve the t1 to `SELECT 2` instead of `SELECT 1`.
-        traverseAndSubstituteCTE(relation, isCommand, cteDefs)._1
-      }
-
-      if (cteDefs.length > lastCTEDefCount) {
-        // We have added more CTE relations to the `cteDefs` from the inner CTE, and these relations
-        // should also be substituted with `resolvedCTERelations` as inner CTE relation can refer to
-        // outer CTE relation. For example:
-        // WITH t1 AS (SELECT 1)
-        // t2 AS (
-        //   WITH t3 AS (SELECT * FROM t1)
-        // )
-        for (i <- lastCTEDefCount until cteDefs.length) {
-          val substituted =
-            substituteCTE(cteDefs(i).child, isLegacy || isCommand, resolvedCTERelations.toSeq)
-          cteDefs(i) = cteDefs(i).copy(child = substituted)
-        }
+        traverseAndSubstituteCTE(relation, isCommand, resolvedCTERelations, cteDefs)._1
       }
-
       // CTE definition can reference a previous one
-      val substituted =
-        substituteCTE(innerCTEResolved, isLegacy || isCommand, resolvedCTERelations.toSeq)
+      val substituted = substituteCTE(innerCTEResolved, isLegacy || isCommand, resolvedCTERelations)
       val cteRelation = CTERelationDef(substituted)
-      resolvedCTERelations += (name -> cteRelation)
+      if (!(isLegacy || isCommand)) {
+        cteDefs += cteRelation
+      }
+      // Prepending new CTEs makes sure that those have higher priority over outer ones.

Review Comment:
   I don't think we currently allow duplicate names at a given level:
   ```
   WITH
   ^^^
     t1 AS (SELECT 1),
     t1 AS (SELECT 1 + (SELECT * FROM t1))
   SELECT * FROM t1
       
   
   org.apache.spark.sql.catalyst.parser.ParseException: 
   CTE definition can't have duplicate names: 't1'.(line 2, pos 0)
   ```
   But if we allowed this construct I would expect `2` as result. 



-- 
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-unsubscribe@spark.apache.org

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


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