[SPARK-50739][SQL] Recursive CTE. Analyzer changes to unravel and resolve the recursion components. #49351

nemanjapetr-db · 2025-01-03T17:30:19Z

What changes were proposed in this pull request?

Instruction for reviewers https://docs.google.com/document/d/1qcEJxqoXcr5cSt6HgIQjWQSqhfkSaVYkoDHsg5oxXp4/edit
Introduction of UnionLoop and UnionLoopRef logical plan classes. Changes in ResolveWithCTE.scala to have the analyzer grok recursive anchors. Specifically we substitute CTERelationRef with UnionLoopRef, and Union with UnionLoop. We untangle the dead loop in resolving where recursive CTE reference is referring to an unresolved CTE definition, which itself cannot be resolved as one of its descendants is an unresolved CTE reference.

Why are the changes needed?

Support for the recursive CTE.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Proposed changes in this PR are no-op. Tested
./build/sbt "test:testOnly org.apache.spark.sql.SQLQueryTestSuite"
./build/sbt "test:testOnly *PlanParserSuite"

Was this patch authored or co-authored using generative AI tooling?

No

…ocs.google.com/document/d/1qcEJxqoXcr5cSt6HgIQjWQSqhfkSaVYkoDHsg5oxXp4/edit . Changes in ResolveWithCTE.scala to have the analyzer grok recursive anchors. Introduction of UnionLoop and UnionLoopRef logical plan classes.

cloud-fan · 2025-01-06T08:49:58Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveWithCTE.scala

+                cteDef
+              }
+            } else {
+              if (cteDef.recursionAnchor.nonEmpty) {


how can this happen? i.e. A non-recursive CTE relation contains recursionAnchor

Why do you believe it is non-recursive, it is within if (cteDEf.recursive) block?

oh sorry I misread the code. Now the question becomes: why do we empty out the recursionAnchor if the CTE def is resolved?

recursionAnchor removed, comment is now moot.

cloud-fan · 2025-01-06T09:05:09Z

...alyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala

+   * mapping between the original and reference sequences are symmetric.
+   */
+  private def rewriteConstraints(
+                                  reference: Seq[Attribute],


nit: 4 spaces indentation. Please follow the code style of the previous code: https://github.com/apache/spark/pull/49351/files#diff-0ac2c89f8cc0d00e8fe7717b01697f36f20fe8abf2def09bcfde0ad84b30e467L552

cloud-fan · 2025-01-06T09:05:25Z

...alyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala

+ * @param limit An optional limit that can be pushed down to the node to stop the loop earlier.
+ */
+case class UnionLoop(
+                      id: Long,


nit: 4 spaces indentation

cloud-fan · 2025-01-06T09:05:40Z

...alyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala

+ *                    results.
+ */
+case class UnionLoopRef(
+                         loopId: Long,


...alyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala

cloud-fan · 2025-01-06T09:14:13Z

After CTESubstitution, we will create a CTERelationDef whose child is a union query with CTERelationRef referencing itslef. IIUC our goal is to rewrite this recursive CTERelationDef's child to be a UnionLoop with UnionLoopRef. Why do we need to put this anchor field in CTERelationDef?

nemanjapetr-db

Please take another look.

nemanjapetr-db · 2025-01-06T10:23:05Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveWithCTE.scala

+                cteDef
+              }
+            } else {
+              if (cteDef.recursionAnchor.nonEmpty) {


Why do you believe it is non-recursive, it is within if (cteDEf.recursive) block?

nemanjapetr-db · 2025-01-06T10:25:31Z

...alyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala

+   * mapping between the original and reference sequences are symmetric.
+   */
+  private def rewriteConstraints(
+                                  reference: Seq[Attribute],


nemanjapetr-db · 2025-01-06T10:26:06Z

...alyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala

+ * @param limit An optional limit that can be pushed down to the node to stop the loop earlier.
+ */
+case class UnionLoop(
+                      id: Long,


nemanjapetr-db · 2025-01-06T10:30:46Z

...alyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala

+ *                    results.
+ */
+case class UnionLoopRef(
+                         loopId: Long,


...alyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala

…oop, and a recursive CTERelationRef into UnionLoopRef.

nemanjapetr-db

I will get back to recursionAnchor variable in the next round, I would like to understand the benefits of removing it since it's removal is quite hairy.

nemanjapetr-db · 2025-01-08T23:51:47Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveWithCTE.scala

+                cteDef
+              }
+            } else {
+              if (cteDef.recursionAnchor.nonEmpty) {


recursionAnchor removed, comment is now moot.

…ic of detecting and storing fully and resolved CTE Definitions. Add error cases.

…ecursion achor, instead locate it under CTE child via pattern matching when it is needed. recursiveAnchorResolved() now returns anchor iff it is resolved and can be used to populate and read from a temporary map of CTE definitions.

nemanjapetr-db · 2025-01-10T17:37:57Z

Please take another look.

CTERelationDef does not contain anchor any longer -- when needed it is fetched from its child via pattern matching. Code is greatly simplified and all previous convoluted questions on recursionAnchor are now moot.

I added several exceptions for the unsupported cases of Union under the CTE Definition.

Substitution rules for UnionLoop/Ref are added for 4 cases of Union under CTE Definition.

CTERelationDef change has broken some tests, will work on those now.

vladimirg-db · 2025-01-10T18:35:20Z

Please update the PR title to have a full sentence.

vladimirg-db · 2025-01-10T18:41:30Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveWithCTE.scala

+                // Project yet), leaving us with cases of SubqueryAlias->Union and SubqueryAlias->
+                // UnresolvedSubqueryColumnAliases->Union. The same applies to Distinct Union.
+                cteDef.failAnalysis(
+                    errorClass = "INVALID_RECURSIVE_CTE",


Shall we add a new error class to error-conditions.json and a new function to QueryCompilationErrors?

Yes let's please do this

vladimirg-db · 2025-01-10T18:43:47Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveWithCTE.scala

-        cteDefs.foreach { cteDef =>
-          if (cteDef.resolved) {
-            cteDefMap.put(cteDef.id, cteDef)
+        val newCTEDefs = cteDefs.map { cteDef =>


It feels like this map body should have two distinct cases - recursive and not recursive. We can rewrite it like this:

cteDefs.map { case cteDef if !cteDef.isRecursive => ... case cteDef => ... }

vladimirg-db · 2025-01-10T18:45:40Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveWithCTE.scala

+          val newCTEDef = if (cteDef.recursive) {
+            cteDef.child match {
+              // Substitutions to UnionLoop and UnionLoopRef.
+              case a @ SubqueryAlias(_, Union(Seq(anchor, recursion), false, false)) =>


I know that developers in Catalyst really enjoy one-letter variables in matches, but it does not feel like a good code health.

Addressed, though this code may be changed to address the other feedback.

vladimirg-db · 2025-01-10T19:02:14Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveWithCTE.scala

+          val newCTEDef = if (cteDef.recursive) {
+            cteDef.child match {
+              // Substitutions to UnionLoop and UnionLoopRef.
+              case a @ SubqueryAlias(_, Union(Seq(anchor, recursion), false, false)) =>


Maybe we can introduce an extractor object to reduce complexity here:

object ReplaceUnionWithUnionLoop { def unapply(plan: LogicalPlan): Option[UnionLoop] = plan match { case union: Union(Seq(anchor, recursion), false, false) => Some(UnionLoop(cteDef.id, anchor, transformRefs(recursion))) case distinctUnion: Distinct(Union(Seq(anchor, recursion), false, false)) => Some(UnionLoop(cteDef.id, Distinct(anchor), Except(transformRefs(recursion), UnionLoopRef(cteDef.id, cteDef.output, true), false))) case _ => None } }

+1 this code seems duplicated, we can use a helper to dedup it

I will address this one in the next round.

The change was not possible. Regardless the comment is now moot as we changed the underlying code.

vladimirg-db · 2025-01-10T19:05:48Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveWithCTE.scala

@@ -37,21 +38,150 @@ object ResolveWithCTE extends Rule[LogicalPlan] {
    }
  }

+  // Substitute CTERelationRef with UnionLoopRef.
+  private def transformRefs(plan: LogicalPlan) = {


Shall we place methods in a top-down order as they are used? More natural for reading.

Renamed to be more readable and moved below.

vladimirg-db · 2025-01-10T19:07:54Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveWithCTE.scala

@@ -37,21 +38,150 @@ object ResolveWithCTE extends Rule[LogicalPlan] {
    }
  }

+  // Substitute CTERelationRef with UnionLoopRef.
+  private def transformRefs(plan: LogicalPlan) = {


Suggested change

private def transformRefs(plan: LogicalPlan) = {

private def replaceSimpleRefsWithUnionLoopRefs(plan: LogicalPlan) = {

vladimirg-db · 2025-01-10T19:08:32Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveWithCTE.scala

@@ -37,21 +38,150 @@ object ResolveWithCTE extends Rule[LogicalPlan] {
    }
  }

+  // Substitute CTERelationRef with UnionLoopRef.


Can you please explain in the method doc why exactly are we replacing all the simple refs with union refs under a UnionLoop

+1, let's expand all the comments in this PR heavily to give a lot of context about the total algorithm to be performed, steps taken, etc.

vladimirg-db · 2025-01-10T19:11:11Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveWithCTE.scala

+            ref.copy(_resolved = true, output = cteDef.output, isStreaming = cteDef.isStreaming)
+          } else {
+            // In the non-recursive case, cteDefMap contains only resolved Definitions.
+            cteDef.failAnalysis(


It changes the non-recursive behavior - if the def is unresolved now we would throw an error. Also, completely unrelated to the problem.

In the non-recursive case:

We are pulling this def out from cteDefMap, and it is in that map precisely because it is resolved. The map insertion code above is:

if (newCTEDef.resolved) {
cteDefMap.put(newCTEDef.id, newCTEDef)
}

This is a sanity check that is enforcing the invariant.

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveWithCTE.scala

dtenedor

Thanks for working on this!!

dtenedor · 2025-01-10T20:00:35Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/QueryPlan.scala

@@ -762,4 +762,15 @@ object QueryPlan extends PredicateHelper {
      case e: AnalysisException => append(e.toString)
    }
  }
+
+  /**
+   * Generate detailed field string with different format based on type of input value


this comment and method name are a bit generic; could we expand them to mention what type of field we are referring to here, and when this might be used? can we give a brief example?

...alyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala

dtenedor · 2025-01-10T20:03:46Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveWithCTE.scala

@@ -37,21 +38,150 @@ object ResolveWithCTE extends Rule[LogicalPlan] {
    }
  }

+  // Substitute CTERelationRef with UnionLoopRef.


+1, let's expand all the comments in this PR heavily to give a lot of context about the total algorithm to be performed, steps taken, etc.

dtenedor · 2025-01-10T20:04:16Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveWithCTE.scala

+          val newCTEDef = if (cteDef.recursive) {
+            cteDef.child match {
+              // Substitutions to UnionLoop and UnionLoopRef.
+              case a @ SubqueryAlias(_, Union(Seq(anchor, recursion), false, false)) =>


+1 this code seems duplicated, we can use a helper to dedup it

dtenedor · 2025-01-10T20:04:27Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveWithCTE.scala

+                // Project yet), leaving us with cases of SubqueryAlias->Union and SubqueryAlias->
+                // UnresolvedSubqueryColumnAliases->Union. The same applies to Distinct Union.
+                cteDef.failAnalysis(
+                    errorClass = "INVALID_RECURSIVE_CTE",


Yes let's please do this

…handle errors. Simplified pattern matching. Added comments to generateFieldString. Added more comemnts to basicLogicalOperators.scala

nemanjapetr-db

I deferred a few pieces of feedback for the next round. Several changes incorporated here including

Added error handlers in error-conditions.json and QueryCompilationError
Simplified pattern matching.
Added comments to generateFieldString.
Added more comments to basicLogicalOperators.scala

...alyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala

nemanjapetr-db · 2025-01-11T22:01:49Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/QueryPlan.scala

@@ -762,4 +762,15 @@ object QueryPlan extends PredicateHelper {
      case e: AnalysisException => append(e.toString)
    }
  }
+
+  /**
+   * Generate detailed field string with different format based on type of input value


nemanjapetr-db · 2025-01-12T06:15:19Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveWithCTE.scala

+            ref.copy(_resolved = true, output = cteDef.output, isStreaming = cteDef.isStreaming)
+          } else {
+            // In the non-recursive case, cteDefMap contains only resolved Definitions.
+            cteDef.failAnalysis(


In the non-recursive case:

We are pulling this def out from cteDefMap, and it is in that map precisely because it is resolved. The map insertion code above is:

if (newCTEDef.resolved) {
cteDefMap.put(newCTEDef.id, newCTEDef)
}

This is a sanity check that is enforcing the invariant.

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveWithCTE.scala

nemanjapetr-db · 2025-01-12T21:48:31Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveWithCTE.scala

+                // Project yet), leaving us with cases of SubqueryAlias->Union and SubqueryAlias->
+                // UnresolvedSubqueryColumnAliases->Union. The same applies to Distinct Union.
+                cteDef.failAnalysis(
+                    errorClass = "INVALID_RECURSIVE_CTE",


nemanjapetr-db · 2025-01-12T22:06:58Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveWithCTE.scala

+          val newCTEDef = if (cteDef.recursive) {
+            cteDef.child match {
+              // Substitutions to UnionLoop and UnionLoopRef.
+              case a @ SubqueryAlias(_, Union(Seq(anchor, recursion), false, false)) =>


I will address this one in the next round.

nemanjapetr-db · 2025-01-12T23:41:07Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveWithCTE.scala

+          val newCTEDef = if (cteDef.recursive) {
+            cteDef.child match {
+              // Substitutions to UnionLoop and UnionLoopRef.
+              case a @ SubqueryAlias(_, Union(Seq(anchor, recursion), false, false)) =>


Addressed, though this code may be changed to address the other feedback.

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveWithCTE.scala

cloud-fan · 2025-01-13T13:21:28Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveWithCTE.scala

+  private def transformRefs(plan: LogicalPlan) = {
+    plan.transformWithPruning(_.containsPattern(CTE)) {
+      case r: CTERelationRef if r.recursive =>
+        UnionLoopRef(r.cteId, r.output, false)


I think the output should be the output of recursive anchor plan.

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveWithCTE.scala

…cursiveCteError.

cloud-fan · 2025-01-14T08:17:48Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveWithCTE.scala

+            val newCTEDef = cteDef
+            if (newCTEDef.resolved) {
+              cteDefMap.put(newCTEDef.id, newCTEDef)
+            }
+            newCTEDef


Suggested change

val newCTEDef = cteDef

if (newCTEDef.resolved) {

cteDefMap.put(newCTEDef.id, newCTEDef)

}

newCTEDef

if (cteDef.resolved) {

cteDefMap.put(cteDef.id, cteDef)

}

cteDef

...alyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveWithCTE.scala

…hes unsupported CTE UNION placements in ResolveWithCTE.

…o catalyst/plans/logical/cteOperators.scala. Added unittest for the recursive CTE analyzer.

nemanjapetr-db · 2025-01-16T23:50:14Z

Added Milan's analyzer unittest to this PR. Refactored CTE operators out of basicLogicalOperators into its own file. Added more comments.

...atalyst/src/test/scala/org/apache/spark/sql/catalyst/analysis/ResolveRecursiveCTESuite.scala

common/utils/src/main/resources/error/error-conditions.json

sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryCompilationErrors.scala

vladimirg-db · 2025-01-17T08:41:47Z

...atalyst/src/test/scala/org/apache/spark/sql/catalyst/analysis/ResolveRecursiveCTESuite.scala

+    def getBeforePlan(cteDef: CTERelationDef): LogicalPlan = {
+      val anchor = Project(Seq(Alias(Literal(1), "1")()), OneRowRelation())
+
+      val recursionPart = Project(anchor.output,


The way the tree is decomposed here (using local variables) is very hard to read. Maybe we can actually have a solid tree structure instead of. this decomposition?

As discussed offline I will do it in a follow up PR.

sql/core/src/main/scala/org/apache/spark/sql/execution/ExplainUtils.scala

…mplified ResolveRecursiveCTESuite by removing several Project's.

cloud-fan · 2025-01-17T15:20:58Z

sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryCompilationErrors.scala

+    new AnalysisException(
+      errorClass = "INVALID_RECURSIVE_CTE",
+      messageParameters = Map(
+        "error" -> error


we don't have this parameter any more.

cloud-fan · 2025-01-17T15:24:35Z

thanks, merging to master/4.0! I'll create a followup PR shortly to do some cleanup

…olve the recursion components ### What changes were proposed in this pull request? Instruction for reviewers https://docs.google.com/document/d/1qcEJxqoXcr5cSt6HgIQjWQSqhfkSaVYkoDHsg5oxXp4/edit Introduction of UnionLoop and UnionLoopRef logical plan classes. Changes in ResolveWithCTE.scala to have the analyzer grok recursive anchors. Specifically we substitute CTERelationRef with UnionLoopRef, and Union with UnionLoop. We untangle the dead loop in resolving where recursive CTE reference is referring to an unresolved CTE definition, which itself cannot be resolved as one of its descendants is an unresolved CTE reference. ### Why are the changes needed? Support for the recursive CTE. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Proposed changes in this PR are no-op. Tested ./build/sbt "test:testOnly org.apache.spark.sql.SQLQueryTestSuite" ./build/sbt "test:testOnly *PlanParserSuite" ### Was this patch authored or co-authored using generative AI tooling? No Closes #49351 from nemanjapetr-db/nemanjapetr-db/rcte3. Authored-by: Nemanja Petrovic <[email protected]> Signed-off-by: Wenchen Fan <[email protected]> (cherry picked from commit 3b114af) Signed-off-by: Wenchen Fan <[email protected]>

### What changes were proposed in this pull request? A followup of #49351 to simplify the test via dsl. ### Why are the changes needed? code cleanup ### Does this PR introduce _any_ user-facing change? no ### How was this patch tested? N/A ### Was this patch authored or co-authored using generative AI tooling? no Closes #49557 from cloud-fan/clean. Authored-by: Wenchen Fan <[email protected]> Signed-off-by: Dongjoon Hyun <[email protected]>

### What changes were proposed in this pull request? A followup of #49351 to simplify the test via dsl. ### Why are the changes needed? code cleanup ### Does this PR introduce _any_ user-facing change? no ### How was this patch tested? N/A ### Was this patch authored or co-authored using generative AI tooling? no Closes #49557 from cloud-fan/clean. Authored-by: Wenchen Fan <[email protected]> Signed-off-by: Dongjoon Hyun <[email protected]> (cherry picked from commit 66dd7dd) Signed-off-by: Dongjoon Hyun <[email protected]>

github-actions bot added the SQL label Jan 3, 2025

cloud-fan reviewed Jan 6, 2025

View reviewed changes

...alyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala Outdated Show resolved Hide resolved

nemanjapetr-db commented Jan 6, 2025

View reviewed changes

nemanjapetr-db changed the title ~~[SPARK-24497][SQL][WIP] Recursive CTE. Changes in ResolveWithCTE.scala to have the analyzer grok recursive anchors.~~ [SPARK-50739][SQL][WIP] Recursive CTE. Changes in ResolveWithCTE.scala to have the analyzer grok recursive anchors. Jan 6, 2025

nemanjapetr-db changed the title ~~[SPARK-50739][SQL][WIP] Recursive CTE. Changes in ResolveWithCTE.scala to have the analyzer grok recursive anchors.~~ [WIP][SPARK-50739][SQL] Recursive CTE. Changes in ResolveWithCTE.scala to have Jan 6, 2025

nemanjapetr-db added 2 commits January 8, 2025 16:16

Fixed alignment in basicLogicalOperators.scala

dbb715b

Add substitution rules for transforming a recursive Union into UnionL…

0043e58

…oop, and a recursive CTERelationRef into UnionLoopRef.

nemanjapetr-db commented Jan 9, 2025

View reviewed changes

nemanjapetr-db added 3 commits January 10, 2025 16:46

Insert substitution rules to UnionLoop and UnionLoopRef. Simplify log…

a61a0df

…ic of detecting and storing fully and resolved CTE Definitions. Add error cases.

Comment changes.

79adea1

vladimirg-db reviewed Jan 10, 2025

View reviewed changes

dtenedor reviewed Jan 10, 2025

View reviewed changes

nemanjapetr-db changed the title ~~[WIP][SPARK-50739][SQL] Recursive CTE. Changes in ResolveWithCTE.scala to have~~ [WIP][SPARK-50739][SQL] Recursive CTE. Analyzer changes to unravel and resolve the recursion components. Jan 11, 2025

Added changes in error-conditions.json and QueryCompilationErrors to …

be3a70a

…handle errors. Simplified pattern matching. Added comments to generateFieldString. Added more comemnts to basicLogicalOperators.scala

nemanjapetr-db commented Jan 12, 2025

View reviewed changes

cloud-fan reviewed Jan 13, 2025

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveWithCTE.scala Show resolved Hide resolved

cloud-fan reviewed Jan 13, 2025

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveWithCTE.scala Outdated Show resolved Hide resolved

Simplifcations of resolveWithCTE via case cteDef. More descriptive re…

cdb9484

…cursiveCteError.

cloud-fan reviewed Jan 14, 2025

View reviewed changes

...alyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala Outdated Show resolved Hide resolved

nemanjapetr-db added 2 commits January 15, 2025 16:42

Style change, move params to the next line.

db5ab67

Update comments in ResolveWithCTE.

d1210c6

cloud-fan reviewed Jan 16, 2025

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveWithCTE.scala Outdated Show resolved Hide resolved

cloud-fan reviewed Jan 16, 2025

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveWithCTE.scala Outdated Show resolved Hide resolved

cloud-fan reviewed Jan 16, 2025

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveWithCTE.scala Outdated Show resolved Hide resolved

cloud-fan reviewed Jan 16, 2025

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveWithCTE.scala Outdated Show resolved Hide resolved

nemanjapetr-db added 2 commits January 16, 2025 10:17

Stylistic changes plus more stricter [case other] condition that catc…

bf35d5a

…hes unsupported CTE UNION placements in ResolveWithCTE.

Refactored CTE related operators out of basicLogicalOperators and int…

c33dc80

…o catalyst/plans/logical/cteOperators.scala. Added unittest for the recursive CTE analyzer.

cloud-fan reviewed Jan 17, 2025

View reviewed changes

...atalyst/src/test/scala/org/apache/spark/sql/catalyst/analysis/ResolveRecursiveCTESuite.scala Show resolved Hide resolved

cloud-fan reviewed Jan 17, 2025

View reviewed changes

...atalyst/src/test/scala/org/apache/spark/sql/catalyst/analysis/ResolveRecursiveCTESuite.scala Outdated Show resolved Hide resolved

cloud-fan reviewed Jan 17, 2025

View reviewed changes

...atalyst/src/test/scala/org/apache/spark/sql/catalyst/analysis/ResolveRecursiveCTESuite.scala Outdated Show resolved Hide resolved

cloud-fan reviewed Jan 17, 2025

View reviewed changes

...atalyst/src/test/scala/org/apache/spark/sql/catalyst/analysis/ResolveRecursiveCTESuite.scala Outdated Show resolved Hide resolved

cloud-fan reviewed Jan 17, 2025

View reviewed changes

...atalyst/src/test/scala/org/apache/spark/sql/catalyst/analysis/ResolveRecursiveCTESuite.scala Show resolved Hide resolved

cloud-fan reviewed Jan 17, 2025

View reviewed changes

...atalyst/src/test/scala/org/apache/spark/sql/catalyst/analysis/ResolveRecursiveCTESuite.scala Outdated Show resolved Hide resolved

cloud-fan reviewed Jan 17, 2025

View reviewed changes

common/utils/src/main/resources/error/error-conditions.json Outdated Show resolved Hide resolved

cloud-fan reviewed Jan 17, 2025

View reviewed changes

common/utils/src/main/resources/error/error-conditions.json Outdated Show resolved Hide resolved

vladimirg-db reviewed Jan 17, 2025

View reviewed changes

nemanjapetr-db added 2 commits January 17, 2025 12:16

Merge branch 'master' into rcte3

4eec1ae

INVALID_RECURSIVE_CTE put in alphabetic order that fixed CI error. Si…

e3cabd5

…mplified ResolveRecursiveCTESuite by removing several Project's.

cloud-fan reviewed Jan 17, 2025

View reviewed changes

cloud-fan changed the title ~~[WIP][SPARK-50739][SQL] Recursive CTE. Analyzer changes to unravel and resolve the recursion components.~~ [SPARK-50739][SQL] Recursive CTE. Analyzer changes to unravel and resolve the recursion components. Jan 17, 2025

cloud-fan closed this in 3b114af Jan 17, 2025

cloud-fan mentioned this pull request Jan 17, 2025

[SPARK-50739][SQL][FOLLOW] Simplify ResolveRecursiveCTESuite with dsl #49557

Closed

nemanjapetr-db deleted the nemanjapetr-db/rcte3 branch January 17, 2025 16:53

	private def transformRefs(plan: LogicalPlan) = {
	private def replaceSimpleRefsWithUnionLoopRefs(plan: LogicalPlan) = {

[SPARK-50739][SQL] Recursive CTE. Analyzer changes to unravel and resolve the recursion components. #49351

[SPARK-50739][SQL] Recursive CTE. Analyzer changes to unravel and resolve the recursion components. #49351

Uh oh!

Conversation

nemanjapetr-db commented Jan 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Was this patch authored or co-authored using generative AI tooling?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nemanjapetr-db Jan 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cloud-fan commented Jan 6, 2025

Uh oh!

nemanjapetr-db left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nemanjapetr-db left a comment

Choose a reason for hiding this comment

Uh oh!

nemanjapetr-db Jan 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nemanjapetr-db commented Jan 10, 2025

Uh oh!

vladimirg-db commented Jan 10, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vladimirg-db Jan 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nemanjapetr-db commented Jan 3, 2025 •

edited

Loading

nemanjapetr-db Jan 8, 2025 •

edited

Loading

nemanjapetr-db Jan 8, 2025 •

edited

Loading

vladimirg-db Jan 10, 2025 •

edited

Loading