compiler: expand stars in the query text on the core path - #4567
Open
kyleconroy wants to merge 2 commits into
Open
compiler: expand stars in the query text on the core path#4567kyleconroy wants to merge 2 commits into
kyleconroy wants to merge 2 commits into
Conversation
Copy link
Copy Markdown
Collaborator
Sorry, something went wrong.
The core analyzer already resolved a star to the columns it covers, but only for the result set — the query it handed to codegen still said "SELECT *", so the generated SQL asked the database for whatever the table happened to hold at run time rather than the columns sqlc scanned into. Every case in the corpus that selects a star generated different code through the core than through the legacy path. The analyzer now reports each star along with the columns it stands for, sharing one list with the analyzers of the queries nested in it so a statement reports the stars in its subqueries and CTEs too. The compiler turns those into edits on the query text, which is where the engine's quoting rules and SQLite's jsonb wrapping live and where the legacy path already does the same rewrite — the two now produce byte-identical SQL across the corpus. Rewriting means reparsing to check the edit produced valid SQL, and the core path analyzes statements concurrently. A parser holds too much state for two goroutines to share one, so the compiler keeps the constructor and a goroutine builds its own. 181 of the 527 cases failing under the core context now pass.
Review of the previous commit turned up three things. The dedupe in expandCore was justified by a CTE analyzed twice, which does not happen — a CTE is analyzed once and cached. Instrumenting it to panic on a duplicate showed the corpus never hits it at all. The real source is an expression typed a second time when a later clause refers to the output name it was given: "SELECT (SELECT * FROM baz LIMIT 1) AS x FROM foo GROUP BY x" reports the star in the subquery once for GROUP BY and once for the target list, and dropping the dedupe turns that query into an overlapping edit. The comment now says so and the query is part of the star_expansion_core case on every engine, where it generates what the legacy path generates. expandCore returned an error it never produced, so it returns only edits. newParser was set on the core path alone, leaving a nil func field on a Compiler built the other way. Nothing calls it there today, but a field that is only sometimes valid is one to get wrong later, so the legacy path sets it too and takes its own parser from it.
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
2 participants
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.
The core analyzer already resolved a star to the columns it covers, but only for the result set. The query it handed to codegen still said SELECT *, so the generated SQL asked the database for whatever the table happened to hold at run time rather than the columns sqlc scanned into. Every case in the corpus that selects a star generated different code through the core than through the legacy path.
What changed
The analyzer reports each star along with the columns it stands for (core.StarExpansion), sharing one list with the analyzers of the queries nested in it — the same way the parameter set is shared — so a statement reports the stars in its subqueries and CTEs too.
The compiler turns those into edits on the query text. That is where the engine's quoting rules and SQLite's jsonb wrapping live, and where the legacy path already does the same rewrite, so the two now produce byte-identical SQL. The measurement of how much text a star reference occupies was pulled out of expandStmt into starOldFunc and is shared by both paths.
I looked at internal/x/expander and did not use it. It rewrites the AST and formats the result, which normalizes whitespace, drops comments and appends a ;. TestReplay expects every context to generate identical code, and the committed goldens come from surgical edits that preserve the query as written — SELECT t.a, t.b FROM foo "t" keeps the quoted alias in the FROM. Its column names also come from a live database, which the core needs none of.
Rewriting the query means reparsing it to check the edit produced valid SQL, the way the legacy path does. The core path analyzes statements concurrently, and a parser holds too much state for two goroutines to share one, so the compiler keeps the constructor and a goroutine that has to parse something of its own builds its own parser. Sharing c.parser here panicked the MySQL parser intermittently.
Testing
star_expansion_core is a new case per engine, pinned to the experiment through its exec.json, so the rewrite is covered without the SQLC_TEST_CORE opt-in. It covers a bare star, repeated and qualified stars, a quoted alias, a reserved-word column, a join whose relations share a column name, a subquery, a CTE and RETURNING *. I checked its generated SQL against the legacy path on all three engines — identical byte for byte.
The three experiment_coreanalyzer goldens had captured the unexpanded stars and are regenerated.
Under the core context, 527 failing cases drop to 346 — 181 fixed, no regressions — and none of the ones still failing has a SQL-text difference left. The remaining failures are column type resolution and a CTE metadata divergence, both unrelated. The full suite passes with --tags=examples against both databases, and the corpus is clean under -race.
Generated by Claude Code