[SPARK-46741][SQL] Cache Table with CTE won't work #53333

AngersZhuuuu · 2025-12-05T03:25:12Z

What changes were proposed in this pull request?

Reopen #44767
Cache Table with CTE won't work, there are two reasons

In the current code CTE in CacheTableAsSelect will be inlined
CTERelation Ref and Def didn't handle the CTEId doCanonicalize issue
Cause the current case can't be matched.

Why are the changes needed?

Fix Bug

Does this PR introduce any user-facing change?

Yea, Cache table with CTE can work after this pr

For added cache.sql final query
EXPLAIN EXTENDED SELECT * FROM cache_nested_cte_table;

Before this pr, the plan as below, cache won't work.

After this pr

How was this patch tested?

Added UT

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

No

This reverts commit 054231f.

AngersZhuuuu · 2025-12-05T03:34:14Z

@cloud-fan Could you take a look again? in #44767 you approved but not merge this pr.
This bug really impact product job's performance

AngersZhuuuu · 2025-12-08T03:09:57Z

ping @cloud-fan

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/normalizer/WithCTENormalized.scala

sql/core/src/test/resources/sql-tests/results/cte-recursion.sql.out

sql/core/src/test/resources/sql-tests/results/cache.sql.out

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/normalizer/NormalizeCTEIds.scala

peter-toth · 2025-12-12T16:41:52Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/normalizer/NormalizeCTEIds.scala

+        ctas.copy(plan = apply(plan))
+
+      case withCTE @ WithCTE(plan, cteDefs) =>
+        val defIdToNewId = withCTE.cteDefs.map(_.id).map((_, curId.getAndIncrement())).toMap


Why we use an AtomicLong.getAndIncrement() here? Could a simple var work here?
Anyways, this is just a nit.

Hmm, as you normalize WithCTE nodes one by one, but the nodes can be nested, once you normalized one of them, can the normalized ids conflict with the not yet normalized ids of an other node?

I can't come up with an exact example where processing WithCTE nodes one by one is an issue, but I wonder if traversing the whole plan and maintaining a global replacement map would be safer and more efficient solution.

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/normalizer/NormalizeCTEIds.scala

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/normalizer/WithCTENormalized.scala

sql/core/src/test/scala/org/apache/spark/sql/SQLQuerySuite.scala

cloud-fan · 2025-12-18T07:47:42Z

thanks, merging to master/4.1!

peter-toth · 2025-12-18T07:48:03Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/normalizer/NormalizeCTEIds.scala

+object NormalizeCTEIds extends Rule[LogicalPlan]{
+  override def apply(plan: LogicalPlan): LogicalPlan = {
+    val curId = new java.util.concurrent.atomic.AtomicLong()
+    plan transformDown {


Does this rule normalize ids of WithCTE nodes in subquery expressions?

ah missed it, we should use transformDownWithSubqueries here as well.

@AngersZhuuuu can you create a followup?

### What changes were proposed in this pull request? Reopen #44767 Cache Table with CTE won't work, there are two reasons 1. In the current code CTE in CacheTableAsSelect will be inlined 2. CTERelation Ref and Def didn't handle the CTEId doCanonicalize issue Cause the current case can't be matched. ### Why are the changes needed? Fix Bug ### Does this PR introduce _any_ user-facing change? Yea, Cache table with CTE can work after this pr For added `cache.sql` final query `EXPLAIN EXTENDED SELECT * FROM cache_nested_cte_table;` Before this pr, the plan as below, cache won't work. <img width="1067" height="584" alt="截屏2025-12-05 11 22 05" src="https://github.com/user-attachments/assets/045df794-38e2-47d9-848e-cfc3c7525671" /> After this pr <img width="1279" height="824" alt="截屏2025-12-05 11 32 38" src="https://github.com/user-attachments/assets/86f5ab33-67c6-44d0-b5d8-4bec51a2d5b7" /> ### How was this patch tested? Added UT ### Was this patch authored or co-authored using generative AI tooling? No Closes #53333 from AngersZhuuuu/SPARK-46741. Authored-by: Angerszhuuuu <[email protected]> Signed-off-by: Wenchen Fan <[email protected]> (cherry picked from commit 8f69679) Signed-off-by: Wenchen Fan <[email protected]>

…expression subquery ### What changes were proposed in this pull request? Follow comment #53333 (comment) ### Why are the changes needed? Support all case ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? UT ### Was this patch authored or co-authored using generative AI tooling? No Closes #53526 from AngersZhuuuu/SPARK-46741-FOLLOWUP. Lead-authored-by: Angerszhuuuu <[email protected]> Co-authored-by: Wenchen Fan <[email protected]> Signed-off-by: Wenchen Fan <[email protected]>

…expression subquery ### What changes were proposed in this pull request? Follow comment #53333 (comment) ### Why are the changes needed? Support all case ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? UT ### Was this patch authored or co-authored using generative AI tooling? No Closes #53526 from AngersZhuuuu/SPARK-46741-FOLLOWUP. Lead-authored-by: Angerszhuuuu <[email protected]> Co-authored-by: Wenchen Fan <[email protected]> Signed-off-by: Wenchen Fan <[email protected]> (cherry picked from commit d65ee81) Signed-off-by: Wenchen Fan <[email protected]>

AngersZhuuuu added 15 commits January 17, 2024 17:00

[SPARK-46741][SQL] Cache Table with CET won't work

c151b60

Update basicLogicalOperators.scala

b9711ab

Update basicLogicalOperators.scala

38c68d8

update

d587e6d

Merge branch 'master' into SPARK-46741

8b2a25d

Follow comment to add normalize rule

bb15d32

Merge branch 'master' into SPARK-46741

fa17613

Merge branch 'master' into SPARK-46741

a6e2657

Update QueryExecution.scala

c9017ef

Update cache.sql.out

4aa6aee

Merge branch 'master' into SPARK-46741

99bf379

follow comment

054231f

Update cache.sql.out

92a213d

Revert "follow comment"

123a986

This reverts commit 054231f.

Merge branch 'master' into SPARK-46741

611b1b4

github-actions bot added the SQL label Dec 5, 2025

AngersZhuuuu added 2 commits December 5, 2025 11:27

Update BaseSessionStateBuilder.scala

5448b73

update

99ee735

Update cte-recursion.sql.out

6a92b32

cloud-fan reviewed Dec 12, 2025

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/normalizer/WithCTENormalized.scala Outdated Show resolved Hide resolved

cloud-fan reviewed Dec 12, 2025

View reviewed changes

sql/core/src/test/resources/sql-tests/results/cte-recursion.sql.out Outdated Show resolved Hide resolved

cloud-fan reviewed Dec 12, 2025

View reviewed changes

sql/core/src/test/resources/sql-tests/results/cache.sql.out Outdated Show resolved Hide resolved

Update WithCTENormalized.scala

5770ff3

cloud-fan reviewed Dec 12, 2025

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/normalizer/NormalizeCTEIds.scala Outdated Show resolved Hide resolved

Update WithCTENormalized.scala

7243d1f

peter-toth reviewed Dec 12, 2025

View reviewed changes

Update WithCTENormalized.scala

40a4332

cloud-fan reviewed Dec 15, 2025

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/normalizer/NormalizeCTEIds.scala Show resolved Hide resolved

AngersZhuuuu added 4 commits December 16, 2025 11:56

Update WithCTENormalized.scala

95bcf5d

Merge branch 'master' into SPARK-46741

2b170fd

Update

1adab54

Update SQLQuerySuite.scala

f99feb1

cloud-fan reviewed Dec 17, 2025

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/normalizer/WithCTENormalized.scala Outdated Show resolved Hide resolved

cloud-fan reviewed Dec 17, 2025

View reviewed changes

sql/core/src/test/scala/org/apache/spark/sql/SQLQuerySuite.scala Outdated Show resolved Hide resolved

cloud-fan reviewed Dec 17, 2025

View reviewed changes

sql/core/src/test/scala/org/apache/spark/sql/SQLQuerySuite.scala Outdated Show resolved Hide resolved

cloud-fan reviewed Dec 17, 2025

View reviewed changes

sql/core/src/test/scala/org/apache/spark/sql/SQLQuerySuite.scala Outdated Show resolved Hide resolved

AngersZhuuuu added 2 commits December 18, 2025 09:00

follow comment

de0a94d

update

d10fd2b

cloud-fan approved these changes Dec 18, 2025

View reviewed changes

peter-toth reviewed Dec 18, 2025

View reviewed changes

cloud-fan closed this in 8f69679 Dec 18, 2025

AngersZhuuuu mentioned this pull request Dec 18, 2025

[SPARK-46741][SQL] Cache Table with CTE should work when CTE in plan expression subquery #53526

Closed

[SPARK-46741][SQL] Cache Table with CTE won't work #53333

[SPARK-46741][SQL] Cache Table with CTE won't work #53333

Uh oh!

Conversation

AngersZhuuuu commented Dec 5, 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!

AngersZhuuuu commented Dec 5, 2025

Uh oh!

AngersZhuuuu commented Dec 8, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

peter-toth Dec 12, 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!

peter-toth Dec 12, 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!

peter-toth Dec 15, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cloud-fan commented Dec 18, 2025

Uh oh!

peter-toth Dec 18, 2025

Choose a reason for hiding this comment

Uh oh!

cloud-fan Dec 18, 2025

Choose a reason for hiding this comment

Uh oh!

cloud-fan Dec 18, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

AngersZhuuuu commented Dec 5, 2025 •

edited

Loading

peter-toth Dec 12, 2025 •

edited

Loading

peter-toth Dec 12, 2025 •

edited

Loading