[SPARK-50838][SQL]Performs additional checks inside recursive CTEs to throw an error if forbidden case is encountered #49518

milanisvet · 2025-01-15T22:16:30Z

What changes were proposed in this pull request?

Performs additional checks inside recursive CTEs to throw an error if forbidden case is encountered:

Recursive term can contain one recursive reference only.
Recursive reference can't be used in some kinds of joins and aggregations.
Recursive references are not allowed in subqueries

In addition, the name of recursive function inside CTERelationDef is rewritten to hasRecursiveCTERelationRef and adds hasItsOwnUnionLoopRef function as it is also needed to check if cteDef is recursive after substitution.

A small bug in CTESubstitution is fixed which now enables substitution of self-references within subqueries as well (but not its resolution, as they are not allowed).

Why are the changes needed?

Support for the recursive CTE.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

will be tested in #49571

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

No.

…ing of recursive CTEs

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

dtenedor

Thanks again for working on this feature, it will be very useful!

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

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

…es inlining of recursive CTEs" This reverts commit 23f65d5.

…lationRef and hasItsOwnUnionLoopRef

…n 1 self ref

milanisvet · 2025-01-20T17:02:36Z

The whole checkRecursion logic is now rewritten and placed in ResolveWithCTE as discussed offline.
One note here: I am still not sure if we should keep datatype check since it throws an error already before coming to this part of the code in case data types of anchor and recursive part are different. Also, still not sure which equality check between data types should be used

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

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

…tes checks now to be applied to unionLoop instead of cteDef node

…er recursive cteDef

milanisvet · 2025-01-21T15:37:01Z

As discussed offline, checkIfSelfReferenceIsPlacedCorrectly and checkDataTypesAnchorAndRecursiveTerm definitions left in resolveWithCTE singleton, but invoked in checkAnalysis to be invoked only once during analysis and not multiple times as if it would have been if we had invoked them in resolveWithCTE as well.

checkNumberOfSelfReferences is moved earlier to CTESubstitution stage following the "fail early" approach

dtenedor

the approach generally LGTM :) we would need thorough test coverage.

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

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

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

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

cloud-fan · 2025-01-23T08:34:49Z

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

+      // Also, if recursion is allowed, we should check that there is no self-reference within
+      // subqueries inside the CTE definition.
+      if (allowRecursion) {
+        checkForSelfReferenceInSubquery(name, relation)


Actually we should do the check in CheckAnalysis because refs can be lazily resolved due to PlanWithUnresolvedIdentifier

The whole offline discussion we had on this is explained in the comment below.

…ursive refs inside subqueries as well. Remove check for subquery in CTESubstitution.

milanisvet · 2025-01-23T14:11:04Z

As discussed offline about subqueries:

The bug in CTESubstitution that self-references within subqueries are not substituted is fixed now.
We agreed that we don't want to resolve self-references within subqueries at all, therefore, we can not perform the check about this in checkAnalysis, but rather in ResolveWithCTE.
More thorough explanation: check in checkAnalysis would require union->unionLoop substitution, so that we still have cteId even if the CTE is inlined. However, only substituting union->unionLoop and not CTERelationRef->UnionLoopRef would make the current ResolveWithCTE code behave as not intended. We don't want to perform second substitution as we agreed not to resolve self-references within subqueries

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

… from/to ResolveWithCTE

milanisvet · 2025-01-23T15:37:46Z

As discussed offline about subqueries:

The bug in CTESubstitution that self-references within subqueries are not substituted is fixed now.

We agreed that we don't want to resolve self-references within subqueries at all, therefore, we can not perform the check about this in checkAnalysis, but rather in ResolveWithCTE.

More thorough explanation: check in checkAnalysis would require union->unionLoop substitution, so that we still have cteId even if the CTE is inlined. However, only substituting union->unionLoop and not CTERelationRef->UnionLoopRef would make the current ResolveWithCTE code behave as not intended. We don't want to perform second substitution as we agreed not to resolve self-references within subqueries

Check about self references in subqueries moved back to checkAnalysis and performed in different way, in order not to have to check it reapeatedly in ResolveWithCTE (fixedPoint rule, in contrast checkAnalysis performed only once)

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

cloud-fan · 2025-01-24T04:32:24Z

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

+      case unionLoop: UnionLoop =>
+        // Recursive CTEs have already substituted Union to UnionLoop at this stage.
+        // Here we perform additional checks for them.
+        checkIfSelfReferenceIsPlacedCorrectly(unionLoop)



nit: let's match CTERelationDef here and call checkForSelfReferenceInSubquery. The function should use the cte id to find recursive references in subquery expressions.

But there we come to the problem that we discussed. Union won't be substituted to UnionLoop in case we have a self-reference in subquery.
Should I just leave it in ResolveWithCTE? Because anyway, the part of ResolveWithCTE I placed it will always be executed at most once.

cloud-fan · 2025-01-24T04:39:21Z

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

+            messageParameters = Map.empty)
+        case other =>
+      }
+    unionLoop.foreach {


nit: let's not use .foreach here which causes O(n^2) time complexity. When matching certain nodes we will call unionLoopRefNotAllowedUnderCurrentNode which also traverse the tree.

how about this:

def checkIfSelfReferenceIsPlacedCorrectly(plan: LogicalPlan, allowRecursiveRef: Boolean = true) { case Join(left, right, LeftOuter, _, _) => checkIfSelfReferenceIsPlacedCorrectly(left, allowRecursiveRef) checkIfSelfReferenceIsPlacedCorrectly(right, allowRecursiveRef = false) ... case _: UnionLoopRef if !allowRecursiveRef => fail ... }

Makes completely sense. It should be fixed now. Thanks a lot

cloud-fan · 2025-01-24T13:13:05Z

thanks, merging to master/4.0!

…o throw an error if forbidden case is encountered ### What changes were proposed in this pull request? Performs additional checks inside recursive CTEs to throw an error if forbidden case is encountered: 1. Recursive term can contain one recursive reference only. 2. Recursive reference can't be used in some kinds of joins and aggregations. 3. Recursive references are not allowed in subqueries In addition, the name of `recursive` function inside `CTERelationDef` is rewritten to `hasRecursiveCTERelationRef` and adds `hasItsOwnUnionLoopRef` function as it is also needed to check if cteDef is recursive after substitution. A small bug in `CTESubstitution` is fixed which now enables substitution of self-references within subqueries as well (but not its resolution, as they are not allowed). ### Why are the changes needed? Support for the recursive CTE. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? will be tested in #49571 ### Was this patch authored or co-authored using generative AI tooling? No. Closes #49518 from milanisvet/checkRecursion. Authored-by: Milan Cupac <[email protected]> Signed-off-by: Wenchen Fan <[email protected]> (cherry picked from commit 4021d91) Signed-off-by: Wenchen Fan <[email protected]>

Add checkAnalysis and corresponding errors

7f71cdc

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

Adjust optimizer in case UnionLoop is encountered, and disables inlin…

23f65d5

…ing of recursive CTEs

milanisvet changed the title ~~Add checkAnalysis and corresponding errors~~ [WIP][SPARK-50838][SQL]Add checkRecursion to check if all the rules about recursive queries are fulfilled. Adjust optimizer with UnionLoop cases. Jan 16, 2025

cloud-fan reviewed Jan 16, 2025

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/CheckAnalysis.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/CheckAnalysis.scala Outdated Show resolved Hide resolved

Merge branch 'master' into checkRecursion

9f4a84a

dtenedor reviewed Jan 17, 2025

View reviewed changes

milanisvet added 7 commits January 20, 2025 12:42

Revert "Adjust optimizer in case UnionLoop is encountered, and disabl…

05cb795

…es inlining of recursive CTEs" This reverts commit 23f65d5.

Split recursive funciton inside CTERelationDef into hasRecursiveCTERe…

c455756

…lationRef and hasItsOwnUnionLoopRef

Add condintion that recursive cteDefs should not be inlined if necessary

f46823b

Add checkNumberOfSelfReferences funciton to check if we have more tha…

8f3cfa8

…n 1 self ref

Merge branch 'master' into checkRecursion

5d70004

Rewrite error message. Corrects some typos.

c6dbae5

Removes checkRecursion

b9ebdff

milanisvet added 4 commits January 20, 2025 16:04

Deletes debugging print inside checkNumberOfSelfReferences

0da1043

Corrects wrong error message

0a885a6

Add check if anchor and recursion term have the same data type

ec8c8c3

Add check to throw error if self reference is placed at forbidden cases

b7af544

milanisvet added 2 commits January 20, 2025 18:05

Rewrite comments

d4af85a

Rewrite comments

3f11993

cloud-fan reviewed Jan 21, 2025

View reviewed changes

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

cloud-fan reviewed Jan 21, 2025

View reviewed changes

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

milanisvet added 2 commits January 21, 2025 16:19

Move "data type" and "place" checks back to checkRecursion, and rewri…

ffa8aa2

…tes checks now to be applied to unionLoop instead of cteDef node

Move check to CTESubstitution that there are no multiple references p…

879df0a

…er recursive cteDef

dtenedor reviewed Jan 21, 2025

View reviewed changes