Repository navigation
fix(sqlite): Extract columns from CREATE TABLE ... AS statement - #4684
Open
manuelschoene wants to merge 2 commits into
Open
manuelschoene wants to merge 2 commits into
manuelschoene wants to merge 2 commits into
Conversation
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.
Fixes #4683
In the SQLite engine,
CREATE TABLE … AS SELECTadded a table to the catalog with no columns. Any query that read from that table failed withcolumn "…" does not exist. This happened on both the default and the core analyzer.Change
convertCreateTableStmt(internal/engine/sqlite/convert.go) always built anast.CreateTableStmtfromn.Columns, and that list is empty for a CTAS. The parser does return the query inmeyer.CreateTableStmt.Select, but the converter only used it to mark the statementIncomplete.Now, when
Selectis set, the converter returns anast.CreateTableAsStmtwith the converted query and anIntoClausenaming the table, as the DuckDB converter already does. Both catalogs already support this node:createTableAsin the legacy catalog andapplyViewin the core schema. They take the columns from the query's output. Then.Select != nilcheck inIncompletecould no longer be reached, so I removed it.Tests
New end-to-end case
internal/endtoend/testdata/create_table_as/sqlite/. It coversCREATE TABLE … AS SELECT *andCREATE TABLE IF NOT EXISTS … AS SELECTwith a column alias and aWHERE, and reads from both tables. The generated models areCopyAll{ID, Val, Score}andCopySome{ID, Label}.Verified on a clean checkout of this branch:
gofmt -lon the changed Go files reports nothing.go vet ./internal/engine/sqlite/... ./internal/endtoend/...reports nothing.go build ./...succeeds.go test ./internal/endtoend -run 'TestReplay/base|TestFormat|TestJsonSchema'passes, including the new case.go test ./...passes, except for tests that need a live PostgreSQL or MySQL server (TestValidSchema,internal/x/expander). No database was running locally, so these failed with connection timeouts. They do not touch the SQLite engine.The opt-in
corecontext (SQLC_TEST_CORE=1) generates aListCopyAllRowrow struct for the new case instead of theCopyAllmodel. The SQLite conversion itself works there. The core catalog dump lists only kindrrelations, so views and CTAS tables get no model, andcreate_table_as/postgresqland thecreate_viewcases fail the same way. #4614 fixes this ("Views absent from models", which also coversCREATE TABLE AS). Once that lands, this case should pass in thecorecontext too.Note
The columns keep the nullability of the
SELECT, soval TEXT NOT NULLbecomesVal string. SQLite does not copy constraints into a CTAS table, so strictly speaking the column is nullable there. This PR keeps the behavior sqlc already has for PostgreSQL. Changing it would affect both engines and could be a separate change.