Skip to content

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

Closed
AngersZhuuuu wants to merge 14 commits into
apache:masterfrom
AngersZhuuuu:SPARK-46741
Closed

[SPARK-46741][SQL] Cache Table with CTE won't work#44767
AngersZhuuuu wants to merge 14 commits into
apache:masterfrom
AngersZhuuuu:SPARK-46741

Conversation

@AngersZhuuuu

Copy link
Copy Markdown
Contributor

What changes were proposed in this pull request?

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

How was this patch tested?

Added UT

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

No

@AngersZhuuuu

Copy link
Copy Markdown
ContributorAuthor

ping @cloud-fan

@AngersZhuuuuAngersZhuuuu changed the title [SPARK-46741][SQL] Cache Table with CET won't work[SPARK-46741][SQL] Cache Table with CTE won't workJan 17, 2024
@AngersZhuuuu

Copy link
Copy Markdown
ContributorAuthor

How about current? @cloud-fan

_.containsAnyPattern(CTE, PLAN_EXPRESSION)) {
case ref: CTERelationRef =>
ref.copy(cteId = defIndex(ref.cteId).toLong)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we make sure there is no nested WithCTE?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we make sure there is no nested WithCTE?

Form the CTESubtitution's code, and it's pr, there won't have nested WithCTE. #37751 cc @maryannxue

@cloud-fancloud-fanJan 19, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shall we add assert to guarantee this assumption?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cloud-fan Found nested WithCTE case, added to cte.sql and here change the method to support nested CTE, pls take a look again.

Comment threadsql/core/src/test/resources/sql-tests/inputs/cache.sql
@AngersZhuuuu

Copy link
Copy Markdown
ContributorAuthor

ping @cloud-fan@yaooqinn

@AngersZhuuuu

Copy link
Copy Markdown
ContributorAuthor

@HyukjinKwon

Copy link
Copy Markdown
Member

what was behaviour before? Would be great to show the result before/after

@AngersZhuuuu

Copy link
Copy Markdown
ContributorAuthor

what was behaviour before? Would be great to show the result before/after

For the query in cache.sql

EXPLAIN EXTENDED SELECT * FROM cache_nested_cte_table

before this pr, cached table cache_nested_cte_table won't match, will execute again, after this pr, it can match the InMemoryRelation

@github-actions

Copy link
Copy Markdown

We're closing this PR because it hasn't been updated in a while. This isn't a judgement on the merit of the PR in any way. It's just a way of keeping the PR queue manageable.
If you'd like to revive this PR, please reopen it and ask a committer to remove the Stale tag!

cloud-fan pushed a commit that referenced this pull request Dec 18, 2025
### 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 <angers.zhu@gmail.com>
Signed-off-by: Wenchen Fan <wenchen@databricks.com>
cloud-fan pushed a commit that referenced this pull request Dec 18, 2025
### 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 <angers.zhu@gmail.com>
Signed-off-by: Wenchen Fan <wenchen@databricks.com>
(cherry picked from commit 8f69679)
Signed-off-by: Wenchen Fan <wenchen@databricks.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@AngersZhuuuu@HyukjinKwon@cloud-fan