compiler: expand stars in the query text on the core path (#4567)

* compiler: expand stars in the query text on the core path

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.

* compiler: fix what the star dedupe claims, and prove it is needed

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.

* compiler: set newParser for the engines main added, and cover hidden columns

Rebasing onto main brought two things this change has to answer for.

SQL Server and DuckDB were added to initCore assigning c.parser
directly, while this branch had taken to deriving c.parser from
c.newParser at the end of the same switch. Neither side conflicted, so
the rebase produced a switch that leaves newParser nil for those two
engines and then calls it — every SQL Server and DuckDB query set
panicked. They set newParser like the rest now.

An fts5 table offers columns it does not declare, and a star must not
expand to them. The analyzer already skips a hidden column, and the
merge carried that into the star's column list along with the result
set, which is what it should do. Nothing covered it: the fts5 case
selects named columns, and it only runs under the opt-in core context.
star_expansion_core selects a star from an fts5 table on sqlite, where
it runs in the default suite, and expands to the declared column alone
the way the legacy path does.

The goldens are regenerated for codegen emitting any over interface{}.

---------

Co-authored-by: Claude <noreply@anthropic.com>

Author: kyleconroy

internal/compiler/engine.go

@@ -40,6 +40,13 @@ type Compiler struct {
 	coreAnalysis bool
 	coreDialect  core.Option
 
+	// newParser builds a parser for the configured engine, and is set for
+	// every engine either path supports. The core path analyzes statements
+	// concurrently, and a parser holds enough state that two goroutines cannot
+	// share one, so a goroutine that has to parse something of its own — the
+	// query text it just rewrote — builds its own rather than taking c.parser.
+	newParser func() Parser
+
 	schema []string
 }
 
@@ -79,7 +86,7 @@ func NewCompiler(conf config.SQL, combo config.CombinedSettings, parserOpts opts
 
 	switch conf.Engine {
 	case config.EngineSQLite:
-		c.parser = sqlite.NewParser()
+		c.newParser = func() Parser { return sqlite.NewParser() }
 		c.catalog = sqlite.NewCatalog()
 		c.selector = newSQLiteSelector()
 
@@ -93,11 +100,11 @@ func NewCompiler(conf config.SQL, combo config.CombinedSettings, parserOpts opts
 			}
 		}
 	case config.EngineMySQL:
-		c.parser = dolphin.NewParser()
+		c.newParser = func() Parser { return dolphin.NewParser() }
 		c.catalog = dolphin.NewCatalog()
 		c.selector = newDefaultSelector()
 	case config.EnginePostgreSQL:
-		c.parser = postgresql.NewParser()
+		c.newParser = func() Parser { return postgresql.NewParser() }
 		c.catalog = postgresql.NewCatalog()
 		c.selector = newDefaultSelector()
 
@@ -113,6 +120,7 @@ func NewCompiler(conf config.SQL, combo config.CombinedSettings, parserOpts opts
 	default:
 		return nil, fmt.Errorf("unknown engine: %s", conf.Engine)
 	}
+	c.parser = c.newParser()
 	return c, nil
 }
 
@@ -122,36 +130,37 @@ func (c *Compiler) initCore() error {
 	var dialect core.Option
 	switch c.conf.Engine {
 	case config.EngineSQLite:
-		c.parser = sqlite.NewParser()
+		c.newParser = func() Parser { return sqlite.NewParser() }
 		c.selector = newSQLiteSelector()
 		dialect = sqlite.Dialect()
 	case config.EngineMySQL:
-		c.parser = dolphin.NewParser()
+		c.newParser = func() Parser { return dolphin.NewParser() }
 		c.selector = newDefaultSelector()
 		dialect = dolphin.Dialect()
 	case config.EnginePostgreSQL:
-		c.parser = postgresql.NewParser()
+		c.newParser = func() Parser { return postgresql.NewParser() }
 		c.selector = newDefaultSelector()
 		dialect = postgresql.Dialect()
 	case config.EngineClickHouse:
-		c.parser = clickhouse.NewParser()
+		c.newParser = func() Parser { return clickhouse.NewParser() }
 		c.selector = newDefaultSelector()
 		dialect = clickhouse.Dialect()
 	case config.EngineGoogleSQL:
-		c.parser = googlesql.NewParser()
+		c.newParser = func() Parser { return googlesql.NewParser() }
 		c.selector = newDefaultSelector()
 		dialect = googlesql.Dialect()
 	case config.EngineMSSQL:
-		c.parser = mssql.NewParser()
+		c.newParser = func() Parser { return mssql.NewParser() }
 		c.selector = newDefaultSelector()
 		dialect = mssql.Dialect()
 	case config.EngineDuckDB:
-		c.parser = duckdb.NewParser()
+		c.newParser = func() Parser { return duckdb.NewParser() }
 		c.selector = newDefaultSelector()
 		dialect = duckdb.Dialect()
 	default:
 		return fmt.Errorf("unknown engine: %s", c.conf.Engine)
 	}
+	c.parser = c.newParser()
 	c.coreDialect = dialect
 	return nil
 }

internal/compiler/expand.go

@@ -78,6 +78,38 @@ func (c *Compiler) quote(x string) string {
 	}
 }
 
+// starOldFunc measures how much of the query text a star reference occupies,
+// so an edit replaces the reference and nothing else. Each part is measured
+// both bare and quoted: an embed was rewritten to "table.*" in the query text,
+// preserving the way the user quoted the table, so it is measured the same way
+// as a star reference the user wrote.
+func (c *Compiler) starOldFunc(parts []string) func(string) int {
+	old := make([]string, 0, len(parts))
+	for _, p := range parts {
+		if p == "*" {
+			old = append(old, p)
+		} else {
+			old = append(old, c.quoteIdent(p))
+		}
+	}
+	return func(s string) int {
+		length := 0
+		for i, o := range old {
+			if hasSeparator := i > 0; hasSeparator {
+				length++
+			}
+			if strings.HasPrefix(s[length:], o) {
+				length += len(o)
+			} else if quoted := c.quote(o); strings.HasPrefix(s[length:], quoted) {
+				length += len(quoted)
+			} else {
+				length += len(o)
+			}
+		}
+		return length
+	}
+}
+
 func (c *Compiler) expandStmt(qc *QueryCatalog, raw *ast.RawStmt, node ast.Node) ([]source.Edit, error) {
 	tables, err := c.sourceTables(qc, node)
 	if err != nil {
@@ -168,37 +200,9 @@ func (c *Compiler) expandStmt(qc *QueryCatalog, raw *ast.RawStmt, node ast.Node)
 				cols = append(cols, cname)
 			}
 		}
-		var old []string
-		for _, p := range parts {
-			if p == "*" {
-				old = append(old, p)
-			} else {
-				old = append(old, c.quoteIdent(p))
-			}
-		}
-
-		// An embed was rewritten to "table.*" in the query text, so it is
-		// measured the same way as a star reference the user wrote.
-		oldFunc := func(s string) int {
-			length := 0
-			for i, o := range old {
-				if hasSeparator := i > 0; hasSeparator {
-					length++
-				}
-				if strings.HasPrefix(s[length:], o) {
-					length += len(o)
-				} else if quoted := c.quote(o); strings.HasPrefix(s[length:], quoted) {
-					length += len(quoted)
-				} else {
-					length += len(o)
-				}
-			}
-			return length
-		}
-
 		edits = append(edits, source.Edit{
 			Location: res.Location - raw.StmtLocation,
-			OldFunc:  oldFunc,
+			OldFunc:  c.starOldFunc(parts),
 			New:      strings.Join(cols, ", "),
 		})
 	}

internal/compiler/expand_core.go

@@ -0,0 +1,76 @@
+package compiler
+
+import (
+	"strings"
+
+	"github.com/sqlc-dev/sqlc/internal/core"
+	"github.com/sqlc-dev/sqlc/internal/source"
+	"github.com/sqlc-dev/sqlc/internal/sql/ast"
+)
+
+// expandCore rewrites the stars in a query's text with the columns the core
+// analyzer resolved them to. The analyzer has already walked the statement and
+// its subqueries, so there is nothing left to look up here: what remains is
+// deciding how each name is written, which is the engine's business and not
+// the core's.
+func (c *Compiler) expandCore(raw *ast.RawStmt, stars []core.StarExpansion) []source.Edit {
+	if len(stars) == 0 {
+		return nil
+	}
+	edits := make([]source.Edit, 0, len(stars))
+	seen := make(map[int]bool, len(stars))
+	for _, star := range stars {
+		// The analyzer types an expression again when a later clause refers to
+		// the output name it was given, so "SELECT (SELECT * FROM t) AS x ...
+		// GROUP BY x" reports the star in the subquery once per pass. The
+		// passes agree on what the star covers, and editing the same reference
+		// twice would overlap, so only the first is kept.
+		if seen[star.Location] {
+			continue
+		}
+		seen[star.Location] = true
+
+		// Everything before the star qualifies it: "foo.*" is scoped to foo,
+		// while a bare "*" covers every relation in the FROM clause.
+		scope := strings.Join(star.Fields[:len(star.Fields)-1], ".")
+
+		// An unqualified star that covers more than one relation may name the
+		// same column twice, so those are written with their relation.
+		counts := map[string]int{}
+		if scope == "" {
+			for _, col := range star.Columns {
+				counts[col.Name]++
+			}
+		}
+
+		cols := make([]string, 0, len(star.Columns))
+		for _, col := range star.Columns {
+			cname := col.Name
+			if star.Alias != "" {
+				cname = star.Alias
+			}
+			cname = c.quoteIdent(cname)
+			if scope != "" {
+				cname = c.quoteIdent(scope) + "." + cname
+			}
+			if counts[cname] > 1 {
+				cname = c.quoteIdent(col.Relation) + "." + cname
+			}
+
+			// This is important for SQLite in particular which needs to wrap
+			// jsonb column values with `json(colname)` so they're in a publicly
+			// usable format (i.e. not jsonb).
+			cols = append(cols, c.selector.ColumnExpr(cname, &Column{
+				Name:     col.Name,
+				DataType: col.DataType,
+			}))
+		}
+
+		edits = append(edits, source.Edit{
+			Location: star.Location - raw.StmtLocation,
+			OldFunc:  c.starOldFunc(star.Fields),
+			New:      strings.Join(cols, ", "),
+		})
+	}
+	return edits
+}

internal/compiler/parse_core.go

@@ -2,6 +2,7 @@ package compiler
 
 import (
 	"errors"
+	"fmt"
 	"strings"
 
 	"github.com/sqlc-dev/sqlc/internal/core"
@@ -64,6 +65,17 @@ func (c *Compiler) parseQueryCore(raw *ast.RawStmt, src string, pre *preprocess.
 		for _, p := range res.Parameters {
 			params = append(params, Parameter{Number: p.Number, Column: coreParamColumn(p, namedParams)})
 		}
+		expanded, err = source.Mutate(rawSQL, c.expandCore(raw, res.Stars))
+		if err != nil {
+			return nil, err
+		}
+	}
+
+	// If the query string was edited, make sure the syntax is valid
+	if expanded != rawSQL {
+		if _, err := c.newParser().Parse(strings.NewReader(expanded)); err != nil {
+			return nil, fmt.Errorf("edited query syntax is invalid: %w", err)
+		}
 	}
 
 	trimmed, comments, err := source.StripComments(expanded)

internal/core/analysis.go

@@ -10,9 +10,39 @@ const (
 )
 
 type PrepareResult struct {
-	Command    Command     `json:"command,omitempty"`
-	Columns    []Column    `json:"columns"`
-	Parameters []Parameter `json:"parameters"`
+	Command    Command         `json:"command,omitempty"`
+	Columns    []Column        `json:"columns"`
+	Parameters []Parameter     `json:"parameters"`
+	Stars      []StarExpansion `json:"stars,omitempty"`
+}
+
+// StarExpansion is what a star in a target list stands for. The analyzer
+// resolves the reference against the query's scope and reports the columns it
+// covers; rewriting the query text with them is the caller's to do, since only
+// it knows how the engine quotes an identifier.
+type StarExpansion struct {
+	// Location is where the target the star belongs to starts, measured the
+	// way the AST measures a node: from the beginning of the file the
+	// statement was parsed from.
+	Location int `json:"location"`
+
+	// Fields is the reference as it was written, with the star as its last
+	// element: ["*"] for a bare star and ["foo", "*"] for a qualified one.
+	Fields []string `json:"fields"`
+
+	// Alias is the output name the target was given, if any.
+	Alias string `json:"alias,omitempty"`
+
+	Columns []StarColumn `json:"columns"`
+}
+
+// StarColumn is a single column a star expanded to.
+type StarColumn struct {
+	// Relation is the name the column's relation goes by in the query, which
+	// is its alias when it was given one.
+	Relation string `json:"relation,omitempty"`
+	Name     string `json:"name"`
+	DataType string `json:"data_type,omitempty"`
 }
 
 type ColumnSource struct {

internal/core/analyzer/analyzer.go

@@ -14,6 +14,7 @@ func Prepare(cat *core.Catalog, stmt ast.Node) (core.PrepareResult, error) {
 	a := &analyzer{
 		cat:    cat,
 		params: map[int]core.Parameter{},
+		stars:  &[]core.StarExpansion{},
 	}
 	switch s := stmt.(type) {
 	case *ast.SelectStmt:
@@ -63,6 +64,18 @@ type analyzer struct {
 
 	// resolving guards against an alias that refers to itself.
 	resolving map[string]bool
+
+	// stars are the expansions every star in the statement asked for, shared
+	// with the analyzers of the queries nested in it so one statement reports
+	// all of them.
+	stars *[]core.StarExpansion
+}
+
+func (a *analyzer) recordStar(s core.StarExpansion) {
+	if a.stars == nil {
+		return
+	}
+	*a.stars = append(*a.stars, s)
 }
 
 // subquery analyzes a nested SELECT. It shares the parameter set, so a
@@ -74,6 +87,7 @@ func (a *analyzer) subquery(s *ast.SelectStmt) (*analyzer, error) {
 		params: a.params,
 		outer:  a.scope,
 		ctes:   a.ctes,
+		stars:  a.stars,
 	}
 	if err := sub.analyzeSelect(s); err != nil {
 		return nil, err
@@ -105,11 +119,15 @@ func derivedRel(alias string, cols []core.Column) scopeRel {
 }
 
 func (a *analyzer) result() core.PrepareResult {
-	return core.PrepareResult{
+	res := core.PrepareResult{
 		Command:    a.command,
 		Columns:    a.columns,
 		Parameters: orderedParams(a.params),
 	}
+	if a.stars != nil {
+		res.Stars = *a.stars
+	}
+	return res
 }
 
 func orderedParams(m map[int]core.Parameter) []core.Parameter {