-
Couldn't load subscription status.
- Fork 579
Throw an error in case duplicate CTE names are found #3719
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
base: main
Are you sure you want to change the base?
Conversation
RS2007
commented
Oct 13, 2025
- Fixes bug Parser should reject CTEs with duplicate names #3674
- With this fix:
| } | ||
|
|
||
| let mut ctes_as_subqueries = vec![]; | ||
| let mut ctes_as_subqueries: Vec<JoinedTable> = vec![]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally there would be a test for this in one of the .test suites.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup looks like there is one here: https://chromium.googlesource.com/external/github.com/sqlite/sqlite/%2B/refs/heads/new-dbconfig-options/test/with1.test
do_catchsql_test 3.2 {
CREATE TABLE t2(x INTEGER);
WITH tmp(a) AS (SELECT * FROM t1),
tmp(a) AS (SELECT * FROM t1)
SELECT * FROM tmp;
} {1 {duplicate WITH table name: tmp}}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But that's SQLite, I meant, ideally it would be in one of the test suites that Turso runs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it, added a test to the select.test file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good! The only think I see is that this fix is slightly more permissive than SQLite, because it allows things like this:
with t as (select 1), t as (select 1) select 1;whereas SQLite does not. Personally I think this is fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@PThorpe92 I don't have merge rights, but this PR looks good to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, but as @LeMikaelF mentioned, can you add a regression test for the exact query this fixes? similar to this:
Line 1065 in 00e6ed8
| # https://github.com/tursodatabase/turso/issues/3667 regression test |
can be in the same file
testing/select.test
Sure, the actual chromium repos seem to have this in a |