Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Resolve the conflict between the CTE name and the referring table name. #8302

Merged
merged 4 commits into from
Jul 20, 2023

Conversation

douenergy
Copy link
Contributor

@douenergy douenergy commented Jul 18, 2023

Fix #5673 and #4987

Provide a clear error message and guide for the case of CTE name and table name shadowing.

with
orders as (
    select * from stg_orders
    where ordered_at >= (select max(ordered_at) from orders)
),
some_more_logic as (
    select *
    from orders
)
select * from some_more_logic;

Error: Binder Error: Circular reference to CTE "orders", There are two possible solutions.

  1. use WITH RECURSIVE to use recursive CTEs.
  2. If you want to use the TABLE name "orders" the same as the CTE name, please explicitly add "SCHEMA" before table name. You can try "main.orders" (main is the duckdb default schema)

with
orders as (
    select * from stg_orders
    where ordered_at >= (select max(ordered_at) from main.orders) # add schema here 
),
some_more_logic as (
    select *
    from orders
)
select * from some_more_logic;

So user can explicitly add "SCHEMA" to the CTE tableref.

We can pass the schema_name + table_name to FindCTE, as @aarashy says in (#4987 ) "CTE names are never allowed to be qualified" (Postgres also not allow qualified CTE name).

If a tableref is qualified, we can confirm that it should not be a CTE name.

@github-actions github-actions bot marked this pull request as draft July 18, 2023 19:22
@douenergy douenergy marked this pull request as ready for review July 18, 2023 19:22
@github-actions github-actions bot marked this pull request as draft July 18, 2023 21:06
@douenergy douenergy marked this pull request as ready for review July 18, 2023 21:07
Copy link
Collaborator

@Mytherin Mytherin left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! LGTM.

One comment:

src/planner/binder/tableref/bind_basetableref.cpp Outdated Show resolved Hide resolved
@github-actions github-actions bot marked this pull request as draft July 19, 2023 12:53
@douenergy douenergy marked this pull request as ready for review July 19, 2023 12:53
@douenergy
Copy link
Contributor Author

Code coverage test pass on my fork !
https://github.com/douenergy/duckdb/actions/runs/5599354959

@Mytherin Please take a look.

@Mytherin Mytherin merged commit 6fb793c into duckdb:master Jul 20, 2023
53 checks passed
@Mytherin
Copy link
Collaborator

Thanks!

@douenergy douenergy mentioned this pull request Jul 20, 2023
18 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Referring to an object inside a CTE with the same name mistakenly thinks you're being recursive
2 participants