fix(r): schema-qualify CTE table refs to avoid circular references - #285
Closed
cpsievert wants to merge 2 commits into
Closed
fix(r): schema-qualify CTE table refs to avoid circular references#285cpsievert wants to merge 2 commits into
cpsievert wants to merge 2 commits into
Conversation
ellmer 0.5.0 moves model details from Provider into a new Model class: Provider() no longer accepts model as its second positional argument, and Chat$new() requires a separate model argument. Gate the mock chat client construction on whether ellmer::Model exists so tests pass on both old and new ellmer. Fixes #283
DuckDB 1.3.0+ and SQLite reject a CTE whose name matches a table it references (e.g., WITH t AS (SELECT ... FROM t) SELECT * FROM t), raising a 'circular reference' error. This broke 4 TblSqlSource tests that use transformed tbls (CTE mode). The fix schema-qualifies table references in the CTE body (e.g., FROM main.test_table instead of FROM test_table), which disambiguates the CTE name from the physical table. The schema name is obtained via current_schema() with a fallback to 'main' for databases that don't support it (e.g., SQLite).
Contributor
Author
|
Closing this PR — the 4 TblSqlSource test failures it addresses are local-only, caused by an outdated local DuckDB (1.3.0). CI uses DuckDB 1.5.5, which already fixed this circular CTE reference bug upstream in DuckDB 1.5.0 (PR #19116), resolving issue #6389. The schema-qualification workaround in this PR adds complexity to work around a DuckDB bug that no longer exists in the versions CI and most users run. If support for older DuckDB (< 1.5.0) becomes a requirement, this approach could be revisited. Thanks to @cpsievert for the investigation. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Four
TblSqlSourcetests fail with a "circular reference to CTE" error on DuckDB 1.3.0+ (and SQLite):This happens when a
TblSqlSourceis created from a transformedtbl_sql(e.g.,dplyr::filter(),dplyr::select()). The class wraps the transformed tbl's SQL as a CTE named after the table:DuckDB 1.3.0+ and SQLite reject this because the CTE name (
"test_table") shadows the physical table (test_table) referenced in the CTE body, creating a circular reference.Fix
Schema-qualify the table references in the CTE body so they resolve to the physical table, not the CTE:
Two helper functions are added to
TblSqlSource.R:get_current_schema(conn)— triesSELECT current_schema()(DuckDB, PostgreSQL) and falls back to"main"for databases that don't support it (SQLite).qualify_cte_table_refs(conn, cte_body)— uses a regex to prefixFROM/JOINtable references with the schema name. Skips already-qualified names and subqueryFROMclauses.Verification
All 64
test-TblSqlSource.Rtests pass (previously 4 failed). Fulldevtools::test()shows no new failures or regressions.