From 143735a0077804aee0b6578242da6ea90530588c Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 29 Aug 2026 06:20:44 +0000 Subject: [PATCH] Fix sqlc fmt corruption in PostgreSQL and MySQL formatters MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fix six root causes of corruption found by running sqlc fmt across the endtoend test corpus and verifying the results: PostgreSQL: a named parameter with a cast (@name::type) printed as '@ name::type' because the cast binds tighter than the @ operator, so the printer saw a prefix operator and added a space — silently dropping the parameter from generated code. Prefix sigils now glue to operands that start with an identifier. MySQL (dolphin): the formatter reprinted from the compiler's normalized AST, which loses information the compiler doesn't need but a formatter must keep. sqlc fmt now uses a format parser that preserves identifier case (table names are case-sensitive on most servers), and the converter keeps what it used to drop: - NULL, TRUE/FALSE, and decimal literals convert by datum kind, so NULL no longer prints as '', 1.0 as 0, or true as 1. This also fixes type inference: a bool literal column is now bool, not int32 (selectstatic golden regenerated). - SELECT DISTINCT keeps its DISTINCT. - The ORDER BY of a compound (UNION) statement survives. - GROUP_CONCAT(... ORDER BY ...) keeps its ordering. - Optimizer hints (/*+ ... */) are carried through to printing. - Multi-table UPDATE keeps its JOIN's ON condition and the table qualifiers in SET. - DECIMAL(p,s) precision and UNSIGNED survive in column definitions. Backstop: dolphin now implements Fingerprint (via marino's restore with identifier case preserved and redundant parens unwrapped), giving MySQL the same proof PostgreSQL has: fmt accepts a formatted statement only when it provably means what the author wrote, and otherwise leaves the statement exactly as written. That is what fixes SHOW WARNINGS, which the parser rewrites into a synthetic SELECT for analysis: it now falls back to the original text instead of printing the internal form. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01UZBvADuHMZEjoF551NccgP --- internal/cmd/fmt.go | 4 +- .../endtoend/testdata/fmt/mysql/query.sql | 31 +++ .../endtoend/testdata/fmt/mysql/stdout.txt | 19 +- .../testdata/fmt/postgresql/query.sql | 5 +- .../testdata/fmt/postgresql/stdout.txt | 4 +- .../selectstatic/mysql/go/query.sql.go | 2 +- internal/engine/dolphin/convert.go | 235 +++++++++++++----- internal/engine/dolphin/fingerprint.go | 55 ++++ internal/engine/dolphin/parse.go | 16 +- internal/engine/dolphin/utils.go | 12 +- internal/sql/ast/a_expr.go | 31 ++- internal/sql/ast/column_def.go | 4 + internal/sql/ast/res_target.go | 4 + internal/sql/ast/select_stmt.go | 10 + internal/sql/ast/update_stmt.go | 13 +- 15 files changed, 369 insertions(+), 76 deletions(-) create mode 100644 internal/engine/dolphin/fingerprint.go diff --git a/internal/cmd/fmt.go b/internal/cmd/fmt.go index 0e3616eba0..54763d9420 100644 --- a/internal/cmd/fmt.go +++ b/internal/cmd/fmt.go @@ -49,7 +49,9 @@ func newQueryFormatter(engine config.Engine) queryFormatter { case config.EngineSQLite: return sqlite.NewParser() case config.EngineMySQL: - return dolphin.NewParser() + // The format parser preserves identifier case and proves every + // statement via Fingerprint, the way oliphant does for PostgreSQL. + return dolphin.NewFormatParser() default: return nil } diff --git a/internal/endtoend/testdata/fmt/mysql/query.sql b/internal/endtoend/testdata/fmt/mysql/query.sql index 3b39777bbf..c8afff5624 100644 --- a/internal/endtoend/testdata/fmt/mysql/query.sql +++ b/internal/endtoend/testdata/fmt/mysql/query.sql @@ -39,3 +39,34 @@ insert into authors ( ) values ( ?, ? ); + +-- name: CasePreserved :many +SELECT ID, Name FROM Authors WHERE Name <> '' ORDER BY Name; + +-- name: LiteralsSurvive :one +SELECT true AS t, false AS f, 1.50 AS score, CASE WHEN name = '' THEN NULL ELSE name END AS n FROM authors LIMIT 1; + +-- name: DistinctNames :many +SELECT DISTINCT name FROM authors; + +-- name: UnionOrdered :many +SELECT name AS foo FROM authors +UNION +SELECT bio AS foo FROM authors +ORDER BY foo; + +-- name: ConcatNames :many +SELECT group_concat(DISTINCT name ORDER BY name DESC SEPARATOR ' ') FROM authors GROUP BY bio; + +-- name: Hinted :one +SELECT /*+ MAX_EXECUTION_TIME(1000) */ id FROM authors LIMIT 1; + +-- name: UpdateWithJoin :exec +UPDATE authors AS a JOIN authors AS p ON p.id = a.id +SET a.name = ? +WHERE p.bio IS NOT NULL; + +-- name: ShowWarnings :many +SHOW WARNINGS; + +CREATE TABLE scores (points decimal(10, 5), views bigint unsigned NOT NULL); diff --git a/internal/endtoend/testdata/fmt/mysql/stdout.txt b/internal/endtoend/testdata/fmt/mysql/stdout.txt index 3912c73530..a015b7ada3 100644 --- a/internal/endtoend/testdata/fmt/mysql/stdout.txt +++ b/internal/endtoend/testdata/fmt/mysql/stdout.txt @@ -43,7 +43,7 @@ -- name: CastUnsigned :one SELECT CAST(id AS UNSIGNED) FROM authors LIMIT 1; -@@ -34,8 +39,5 @@ +@@ -34,14 +39,11 @@ SELECT id FROM authors LIMIT 1; -- name: CreateAuthor :execresult @@ -54,3 +54,20 @@ -) values ( - ?, ? -); + + -- name: CasePreserved :many ++SELECT ID, Name FROM Authors WHERE Name != '' ORDER BY Name; +-SELECT ID, Name FROM Authors WHERE Name <> '' ORDER BY Name; + + -- name: LiteralsSurvive :one + SELECT true AS t, false AS f, 1.50 AS score, CASE WHEN name = '' THEN NULL ELSE name END AS n FROM authors LIMIT 1; +@@ -62,7 +64,8 @@ + SELECT /*+ MAX_EXECUTION_TIME(1000) */ id FROM authors LIMIT 1; + + -- name: UpdateWithJoin :exec ++UPDATE authors AS a ++JOIN authors AS p ON p.id = a.id +-UPDATE authors AS a JOIN authors AS p ON p.id = a.id + SET a.name = ? + WHERE p.bio IS NOT NULL; + diff --git a/internal/endtoend/testdata/fmt/postgresql/query.sql b/internal/endtoend/testdata/fmt/postgresql/query.sql index a843f76478..96e8ed6ef7 100644 --- a/internal/endtoend/testdata/fmt/postgresql/query.sql +++ b/internal/endtoend/testdata/fmt/postgresql/query.sql @@ -26,5 +26,8 @@ WHERE id > $1; -- name: SearchAuthors :many SELECT id, name, bio, created_at FROM authors WHERE name LIKE $1 AND bio IS NOT NULL AND id > $2 AND name <> $3 ORDER BY name, id LIMIT $4; +-- name: AtParamsGlued :many +SELECT name FROM authors WHERE name = @slug AND @filter::bool; + -- name: DeleteAuthor :exec -DELETE FROM authors WHERE id = @id +DELETE FROM authors WHERE id = @id \ No newline at end of file diff --git a/internal/endtoend/testdata/fmt/postgresql/stdout.txt b/internal/endtoend/testdata/fmt/postgresql/stdout.txt index 0f801d777b..e58072656d 100644 --- a/internal/endtoend/testdata/fmt/postgresql/stdout.txt +++ b/internal/endtoend/testdata/fmt/postgresql/stdout.txt @@ -40,8 +40,8 @@ FROM authors WHERE id > $1; -@@ -27,4 +27,4 @@ - SELECT id, name, bio, created_at FROM authors WHERE name LIKE $1 AND bio IS NOT NULL AND id > $2 AND name <> $3 ORDER BY name, id LIMIT $4; +@@ -30,4 +30,4 @@ + SELECT name FROM authors WHERE name = @slug AND @filter::bool; -- name: DeleteAuthor :exec -DELETE FROM authors WHERE id = @id diff --git a/internal/endtoend/testdata/selectstatic/mysql/go/query.sql.go b/internal/endtoend/testdata/selectstatic/mysql/go/query.sql.go index a533820dcf..c48efce46c 100644 --- a/internal/endtoend/testdata/selectstatic/mysql/go/query.sql.go +++ b/internal/endtoend/testdata/selectstatic/mysql/go/query.sql.go @@ -17,7 +17,7 @@ type SelectStaticRow struct { Column1 string B string Num int32 - Truefield int32 + Truefield bool Floater float64 } diff --git a/internal/engine/dolphin/convert.go b/internal/engine/dolphin/convert.go index e3b99e7cd1..c52fb7e2ef 100644 --- a/internal/engine/dolphin/convert.go +++ b/internal/engine/dolphin/convert.go @@ -6,6 +6,7 @@ import ( "strings" pcast "github.com/sqlc-dev/marino/ast" + "github.com/sqlc-dev/marino/format" "github.com/sqlc-dev/marino/mysql" "github.com/sqlc-dev/marino/opcode" "github.com/sqlc-dev/marino/types" @@ -16,6 +17,10 @@ import ( type cc struct { paramCount int + // preserveCase keeps identifiers exactly as written instead of + // lowercasing them for case-insensitive catalog matching; the format + // parser sets it (see NewFormatParser). + preserveCase bool } func todo(n pcast.Node) *ast.TODO { @@ -25,17 +30,24 @@ func todo(n pcast.Node) *ast.TODO { return &ast.TODO{} } -func identifier(id string) string { +// identifier normalizes an identifier for the compiler, which matches +// case-insensitively by lowercasing everything. The format parser preserves +// the author's case instead: reprinting `Event` as `event` renames the +// table wherever table names are case-sensitive. +func (c *cc) identifier(id string) string { + if c.preserveCase { + return id + } return strings.ToLower(id) } -func NewIdentifier(t string) *ast.String { - return &ast.String{Str: identifier(t)} +func (c *cc) NewIdentifier(t string) *ast.String { + return &ast.String{Str: c.identifier(t)} } func (c *cc) convertAlterTableStmt(n *pcast.AlterTableStmt) ast.Node { alt := &ast.AlterTableStmt{ - Table: parseTableName(n.Table), + Table: c.parseTableName(n.Table), Cmds: &ast.List{}, } for _, spec := range n.Specs { @@ -99,7 +111,7 @@ func (c *cc) convertAlterTableStmt(n *pcast.AlterTableStmt) ast.Node { oldName := spec.OldColumnName.String() newName := spec.NewColumnName.String() return &ast.RenameColumnStmt{ - Table: parseTableName(n.Table), + Table: c.parseTableName(n.Table), Col: &ast.ColumnRef{Name: oldName}, NewName: &newName, } @@ -107,8 +119,8 @@ func (c *cc) convertAlterTableStmt(n *pcast.AlterTableStmt) ast.Node { case pcast.AlterTableRenameTable: // TODO: Returning here may be incorrect if there are multiple specs return &ast.RenameTableStmt{ - Table: parseTableName(n.Table), - NewName: &parseTableName(spec.NewTable).Name, + Table: c.parseTableName(n.Table), + NewName: &c.parseTableName(spec.NewTable).Name, } default: @@ -122,11 +134,19 @@ func (c *cc) convertAlterTableStmt(n *pcast.AlterTableStmt) ast.Node { } func (c *cc) convertAssignment(n *pcast.Assignment) *ast.ResTarget { - name := identifier(n.Column.Name.String()) - return &ast.ResTarget{ + name := c.identifier(n.Column.Name.String()) + target := &ast.ResTarget{ Name: &name, Val: c.convert(n.Expr), } + // A multi-table UPDATE qualifies its SET columns (SET a.x = ..., b.x = + // ...); analysis matches on Name alone, but printing must keep the + // qualifier or both assignments collapse onto whichever table resolves. + if table := n.Column.Table.String(); table != "" { + table = c.identifier(table) + target.Relation = &table + } + return target } // TODO: These codes should be defined in the sql/lang package @@ -220,11 +240,11 @@ func (c *cc) convertBinaryOperationExpr(n *pcast.BinaryOperationExpr) ast.Node { func (c *cc) convertCreateTableStmt(n *pcast.CreateTableStmt) ast.Node { create := &ast.CreateTableStmt{ - Name: parseTableName(n.Table), + Name: c.parseTableName(n.Table), IfNotExists: n.IfNotExists, } if n.ReferTable != nil { - create.ReferTable = parseTableName(n.ReferTable) + create.ReferTable = c.parseTableName(n.ReferTable) } for _, def := range n.Cols { create.Cols = append(create.Cols, convertColumnDef(def)) @@ -284,6 +304,21 @@ func convertColumnDef(def *pcast.ColumnDef) *ast.ColumnDef { } } + // DECIMAL(p,s) / NUMERIC(p,s) and FLOAT/DOUBLE with explicit precision: + // the parser leaves flen and decimal at -1 when the author wrote none, + // so their presence is the author's, and dropping them would change the + // column's type. + switch tp { + case mysql.TypeNewDecimal, mysql.TypeFloat, mysql.TypeDouble: + if flen >= 0 { + mods := []ast.Node{&ast.Integer{Ival: int64(flen)}} + if dec := def.Tp.GetDecimal(); dec > 0 { + mods = append(mods, &ast.Integer{Ival: int64(dec)}) + } + typeName.Typmods = &ast.List{Items: mods} + } + } + columnDef := ast.ColumnDef{ Colname: def.Name.String(), TypeName: typeName, @@ -303,12 +338,12 @@ func convertColumnDef(def *pcast.ColumnDef) *ast.ColumnDef { func (c *cc) convertColumnNameExpr(n *pcast.ColumnNameExpr) *ast.ColumnRef { var items []ast.Node if schema := n.Name.Schema.String(); schema != "" { - items = append(items, NewIdentifier(schema)) + items = append(items, c.NewIdentifier(schema)) } if table := n.Name.Table.String(); table != "" { - items = append(items, NewIdentifier(table)) + items = append(items, c.NewIdentifier(table)) } - items = append(items, NewIdentifier(n.Name.Name.String())) + items = append(items, c.NewIdentifier(n.Name.Name.String())) return &ast.ColumnRef{ Fields: &ast.List{ Items: items, @@ -320,7 +355,7 @@ func (c *cc) convertColumnNameExpr(n *pcast.ColumnNameExpr) *ast.ColumnRef { func (c *cc) convertColumnNames(cols []*pcast.ColumnName) *ast.List { list := &ast.List{Items: []ast.Node{}} for i := range cols { - name := identifier(cols[i].Name.String()) + name := c.identifier(cols[i].Name.String()) list.Items = append(list.Items, &ast.ResTarget{ Name: &name, Location: cols[i].OriginTextPosition(), @@ -348,9 +383,9 @@ func (c *cc) convertDeleteStmt(n *pcast.DeleteStmt) *ast.DeleteStmt { // Each table in the delete list is a ColumnRef like "jt.*" or "pt.*" items := []ast.Node{} if table.Schema.String() != "" { - items = append(items, NewIdentifier(table.Schema.String())) + items = append(items, c.NewIdentifier(table.Schema.String())) } - items = append(items, NewIdentifier(table.Name.String())) + items = append(items, c.NewIdentifier(table.Name.String())) items = append(items, &ast.A_Star{}) targets.Items = append(targets.Items, &ast.ColumnRef{ Fields: &ast.List{Items: items}, @@ -384,7 +419,7 @@ func (c *cc) convertDeleteStmt(n *pcast.DeleteStmt) *ast.DeleteStmt { func (c *cc) convertDropTableStmt(n *pcast.DropTableStmt) ast.Node { drop := &ast.DropTableStmt{IfExists: n.IfExists} for _, name := range n.Tables { - drop.Tables = append(drop.Tables, parseTableName(name)) + drop.Tables = append(drop.Tables, c.parseTableName(name)) } return drop } @@ -393,8 +428,8 @@ func (c *cc) convertRenameTableStmt(n *pcast.RenameTableStmt) ast.Node { list := &ast.List{Items: []ast.Node{}} for _, table := range n.TableToTables { list.Items = append(list.Items, &ast.RenameTableStmt{ - Table: parseTableName(table.OldTable), - NewName: &parseTableName(table.NewTable).Name, + Table: c.parseTableName(table.OldTable), + NewName: &c.parseTableName(table.NewTable).Name, }) } return list @@ -420,14 +455,17 @@ func (c *cc) convertFieldList(n *pcast.FieldList) *ast.List { func (c *cc) convertFuncCallExpr(n *pcast.FuncCallExpr) ast.Node { schema := n.Schema.String() + // Dispatch below compares against lowercase names; the emitted name + // goes through identifier so the format parser keeps the author's case. name := strings.ToLower(n.FnName.String()) + emitted := c.identifier(n.FnName.String()) // TODO: Deprecate the usage of Funcname items := []ast.Node{} if schema != "" { - items = append(items, NewIdentifier(schema)) + items = append(items, c.NewIdentifier(schema)) } - items = append(items, NewIdentifier(name)) + items = append(items, c.NewIdentifier(emitted)) // Handle DATE_ADD/DATE_SUB specially to construct INTERVAL expressions // These functions have args: [date, interval_value, TimeUnitExpr] @@ -446,7 +484,7 @@ func (c *cc) convertFuncCallExpr(n *pcast.FuncCallExpr) ast.Node { Args: args, Func: &ast.FuncName{ Schema: schema, - Name: name, + Name: emitted, }, Funcname: &ast.List{ Items: items, @@ -470,7 +508,7 @@ func (c *cc) convertFuncCallExpr(n *pcast.FuncCallExpr) ast.Node { Args: args, Func: &ast.FuncName{ Schema: schema, - Name: name, + Name: emitted, }, Funcname: &ast.List{ Items: items, @@ -551,7 +589,7 @@ func (c *cc) convertSelectField(n *pcast.SelectField) *ast.ResTarget { } var name *string if n.AsName.O != "" { - asname := identifier(n.AsName.O) + asname := c.identifier(n.AsName.O) name = &asname } return &ast.ResTarget{ @@ -601,6 +639,12 @@ func (c *cc) convertSelectStmt(n *pcast.SelectStmt) *ast.SelectStmt { Op: op, All: all, } + if n.Distinct { + // A plain DISTINCT, pg-style: a non-empty clause whose single TODO + // item means "no ON (...) expressions". + stmt.DistinctClause = &ast.List{Items: []ast.Node{&ast.TODO{}}} + } + stmt.TableHints = restoreTableHints(n.TableHints) if n.Limit != nil { stmt.LimitCount = c.convert(n.Limit.Count) stmt.LimitOffset = c.convert(n.Limit.Offset) @@ -608,6 +652,27 @@ func (c *cc) convertSelectStmt(n *pcast.SelectStmt) *ast.SelectStmt { return stmt } +// restoreTableHints renders a statement's optimizer hints back to the text +// inside their /*+ ... */ comment. The compiler has no use for hints, but +// dropping them from a query would change how the server runs it, so the +// statement carries them through to printing as text. +func restoreTableHints(hints []*pcast.TableOptimizerHint) string { + if len(hints) == 0 { + return "" + } + var sb strings.Builder + ctx := format.NewRestoreCtx(format.DefaultRestoreFlags, &sb) + for i, h := range hints { + if i != 0 { + sb.WriteString(" ") + } + if err := h.Restore(ctx); err != nil { + return "" + } + } + return sb.String() +} + func (c *cc) convertSubqueryExpr(n *pcast.SubqueryExpr) ast.Node { // Wrap subquery in SubLink to ensure parentheses are added return &ast.SubLink{ @@ -632,7 +697,7 @@ func (c *cc) convertCommonTableExpression(n *pcast.CommonTableExpression) *ast.C columns := &ast.List{} for _, col := range n.ColNameList { - columns.Items = append(columns.Items, NewIdentifier(col.String())) + columns.Items = append(columns.Items, c.NewIdentifier(col.String())) } // CTE Query is wrapped in SubqueryExpr by TiDB parser. @@ -687,6 +752,11 @@ func (c *cc) convertUpdateStmt(n *pcast.UpdateStmt) *ast.UpdateStmt { FromClause: &ast.List{}, ReturningList: &ast.List{}, WithClause: c.convertWithClause(n.With), + // The table-reference tree as written. Relations above flattens a + // multi-table UPDATE's join to bare tables for analysis, which + // would print `UPDATE a JOIN b ON ...` without its ON condition — + // an update of the whole cross product. + TableRefs: rels, } if n.Limit != nil { stmt.LimitCount = c.convert(n.Limit.Count) @@ -695,24 +765,24 @@ func (c *cc) convertUpdateStmt(n *pcast.UpdateStmt) *ast.UpdateStmt { } func (c *cc) convertValueExpr(n *pcast.ValueExprBase) *ast.A_Const { - switch n.Type.GetType() { - case mysql.TypeBit: - case mysql.TypeDate: - case mysql.TypeDatetime: - case mysql.TypeGeometry: - case mysql.TypeJSON: - case mysql.TypeNull: - case mysql.TypeSet: - case mysql.TypeShort: - case mysql.TypeDuration: - case mysql.TypeTimestamp: - // TODO: Create an AST type for these? - - case mysql.TypeTiny, - mysql.TypeInt24, - mysql.TypeYear, - mysql.TypeLong, - mysql.TypeLonglong: + // The datum's kind, not the field type, says what the literal is: a + // NULL arrives with a string field type, a decimal's value lives in + // its decimal — not float — slot, and TRUE is an int64 whose field + // type carries the boolean flag. + switch n.Datum.Kind() { + case pcast.KindNull: + return &ast.A_Const{ + Val: &ast.Null{}, + Location: n.OriginTextPosition(), + } + + case pcast.KindInt64: + if mysql.HasIsBooleanFlag(n.Type.GetFlag()) { + return &ast.A_Const{ + Val: &ast.Boolean{Boolval: n.Datum.GetInt64() > 0}, + Location: n.OriginTextPosition(), + } + } return &ast.A_Const{ Val: &ast.Integer{ Ival: n.Datum.GetInt64(), @@ -720,9 +790,15 @@ func (c *cc) convertValueExpr(n *pcast.ValueExprBase) *ast.A_Const { Location: n.OriginTextPosition(), } - case mysql.TypeDouble, - mysql.TypeFloat, - mysql.TypeNewDecimal: + case pcast.KindUint64: + return &ast.A_Const{ + Val: &ast.Integer{ + Ival: int64(n.Datum.GetUint64()), + }, + Location: n.OriginTextPosition(), + } + + case pcast.KindFloat32, pcast.KindFloat64: return &ast.A_Const{ Val: &ast.Float{ Str: strconv.FormatFloat(n.Datum.GetFloat64(), 'f', -1, 64), @@ -730,7 +806,15 @@ func (c *cc) convertValueExpr(n *pcast.ValueExprBase) *ast.A_Const { Location: n.OriginTextPosition(), } - case mysql.TypeBlob, mysql.TypeString, mysql.TypeVarchar, mysql.TypeVarString, mysql.TypeLongBlob, mysql.TypeMediumBlob, mysql.TypeTinyBlob, mysql.TypeEnum: + case pcast.KindMysqlDecimal: + // The decimal's own rendering keeps the written scale: 1.0 stays + // 1.0. + return &ast.A_Const{ + Val: &ast.Float{ + Str: n.Datum.GetMysqlDecimal().String(), + }, + Location: n.OriginTextPosition(), + } } return &ast.A_Const{ Val: &ast.String{ @@ -743,7 +827,7 @@ func (c *cc) convertValueExpr(n *pcast.ValueExprBase) *ast.A_Const { func (c *cc) convertWildCardField(n *pcast.WildCardField) *ast.ColumnRef { items := []ast.Node{} if t := n.Table.String(); t != "" { - items = append(items, NewIdentifier(t)) + items = append(items, c.NewIdentifier(t)) } items = append(items, &ast.A_Star{}) @@ -759,14 +843,17 @@ func (c *cc) convertAdminStmt(n *pcast.AdminStmt) ast.Node { } func (c *cc) convertAggregateFuncExpr(n *pcast.AggregateFuncExpr) *ast.FuncCall { + // Comparisons below use the lowercase name; the emitted name keeps the + // author's case under the format parser. name := strings.ToLower(n.F) + emitted := c.identifier(n.F) fn := &ast.FuncCall{ Func: &ast.FuncName{ - Name: name, + Name: emitted, }, Funcname: &ast.List{ Items: []ast.Node{ - NewIdentifier(name), + c.NewIdentifier(emitted), }, }, Args: &ast.List{}, @@ -798,6 +885,11 @@ func (c *cc) convertAggregateFuncExpr(n *pcast.AggregateFuncExpr) *ast.FuncCall if n.Distinct { fn.AggDistinct = true } + // GROUP_CONCAT(x ORDER BY y): the ordering decides the string that + // comes back, so it must survive into the printed call. + if n.Order != nil { + fn.AggOrder = c.convertOrderByItems(n.Order.Items) + } // Store separator for GROUP_CONCAT (only if non-default) if name == "group_concat" && separator != "" && separator != "," { @@ -966,7 +1058,7 @@ func (c *cc) convertDropDatabaseStmt(n *pcast.DropDatabaseStmt) ast.Node { return &ast.DropSchemaStmt{ MissingOk: !n.IfExists, Schemas: []*ast.String{ - NewIdentifier(n.Name.O), + c.NewIdentifier(n.Name.O), }, } } @@ -1463,15 +1555,40 @@ func (c *cc) convertSetOprSelectList(n *pcast.SetOprSelectList) ast.Node { func (c *cc) convertSetOprStmt(n *pcast.SetOprStmt) ast.Node { if n.SelectList != nil { sn := c.convertSetOprSelectList(n.SelectList) - if ss, ok := sn.(*ast.SelectStmt); ok && n.Limit != nil { - ss.LimitOffset = c.convert(n.Limit.Offset) - ss.LimitCount = c.convert(n.Limit.Count) + if ss, ok := sn.(*ast.SelectStmt); ok { + if n.Limit != nil { + ss.LimitOffset = c.convert(n.Limit.Offset) + ss.LimitCount = c.convert(n.Limit.Count) + } + // The ORDER BY that applies to the whole compound statement + // (SELECT ... UNION SELECT ... ORDER BY x) lives on the set + // operation, not on either branch. + if n.OrderBy != nil { + ss.SortClause = c.convertOrderByItems(n.OrderBy.Items) + } } return sn } return todo(n) } +// convertOrderByItems converts ORDER BY items into the SortBy list a +// SelectStmt's SortClause carries. +func (c *cc) convertOrderByItems(items []*pcast.ByItem) *ast.List { + list := &ast.List{Items: make([]ast.Node, 0, len(items))} + for _, item := range items { + dir := ast.SortByDirDefault + if item.Desc { + dir = ast.SortByDirDesc + } + list.Items = append(list.Items, &ast.SortBy{ + Node: c.convert(item.Expr), + SortbyDir: dir, + }) + } + return list +} + func (c *cc) convertSetPwdStmt(n *pcast.SetPwdStmt) ast.Node { return todo(n) } @@ -1522,8 +1639,8 @@ func (c *cc) convertSplitRegionStmt(n *pcast.SplitRegionStmt) ast.Node { } func (c *cc) convertTableName(n *pcast.TableName) *ast.RangeVar { - schema := identifier(n.Schema.String()) - rel := identifier(n.Name.String()) + schema := c.identifier(n.Schema.String()) + rel := c.identifier(n.Name.String()) return &ast.RangeVar{ Schemaname: &schema, Relname: &rel, @@ -1585,7 +1702,7 @@ func (c *cc) convertTrimDirectionExpr(n *pcast.TrimDirectionExpr) ast.Node { func (c *cc) convertTruncateTableStmt(n *pcast.TruncateTableStmt) *ast.TruncateStmt { return &ast.TruncateStmt{ - Relations: toList(n.Table), + Relations: c.toList(n.Table), } } @@ -1641,7 +1758,7 @@ func (c *cc) convertCallStmt(n *pcast.CallStmt) ast.Node { var funcname ast.List for _, s := range []string{n.Procedure.Schema.L, n.Procedure.FnName.L} { if s != "" { - funcname.Items = append(funcname.Items, NewIdentifier(s)) + funcname.Items = append(funcname.Items, c.NewIdentifier(s)) } } var args ast.List diff --git a/internal/engine/dolphin/fingerprint.go b/internal/engine/dolphin/fingerprint.go new file mode 100644 index 0000000000..1554a94f8d --- /dev/null +++ b/internal/engine/dolphin/fingerprint.go @@ -0,0 +1,55 @@ +package dolphin + +import ( + "fmt" + "strings" + + pcast "github.com/sqlc-dev/marino/ast" + "github.com/sqlc-dev/marino/format" + "github.com/sqlc-dev/marino/parser" +) + +// deparen unwraps parenthesized expressions so a redundant pair of +// parentheses added or dropped by formatting does not change the +// fingerprint. Everything else about the tree survives untouched. +type deparen struct{} + +func (v deparen) Enter(n pcast.Node) (pcast.Node, bool) { return n, false } +func (v deparen) Leave(n pcast.Node) (pcast.Node, bool) { + if p, ok := n.(*pcast.ParenthesesExpr); ok { + return p.Expr, true + } + return n, true +} + +// Fingerprint reduces a statement to a canonical form that survives changes +// in whitespace, keyword case, quoting style and redundant parentheses — +// and nothing else. Identifier case is preserved: MySQL table names are +// case-sensitive on most servers, so a formatting pass that changes one is +// a semantic change and must not fingerprint equal. fmt uses this as its +// proof that a formatted statement still means what the author wrote; a +// statement whose fingerprint it cannot match is left exactly as written. +func (p *Parser) Fingerprint(sql string) (string, error) { + // A fresh parser: p.pingcap carries per-parse comment state that + // ParseFile is still using. + stmts, _, err := parser.New().Parse(sql, "", "") + if err != nil { + return "", normalizeErr(err) + } + flags := format.RestoreStringSingleQuotes | + format.RestoreKeyWordUppercase | + format.RestoreNameBackQuotes + parts := make([]string, 0, len(stmts)) + for _, s := range stmts { + n, ok := s.Accept(deparen{}) + if !ok { + return "", fmt.Errorf("fingerprint: rewrite failed") + } + var sb strings.Builder + if err := n.Restore(format.NewRestoreCtx(flags, &sb)); err != nil { + return "", fmt.Errorf("fingerprint: %w", err) + } + parts = append(parts, sb.String()) + } + return strings.Join(parts, "; "), nil +} diff --git a/internal/engine/dolphin/parse.go b/internal/engine/dolphin/parse.go index 5b3aa7bb76..9f1f409593 100644 --- a/internal/engine/dolphin/parse.go +++ b/internal/engine/dolphin/parse.go @@ -16,11 +16,21 @@ import ( ) func NewParser() *Parser { - return &Parser{parser.New()} + return &Parser{pingcap: parser.New()} +} + +// NewFormatParser returns the parser sqlc fmt uses. It differs from the +// compiler's parser in one way: identifiers keep the case the author wrote. +// The compiler lowercases them so catalog lookups are case-insensitive, but +// a formatter that prints `Event` as `event` renames a table on the many +// servers where table names are case-sensitive. +func NewFormatParser() *Parser { + return &Parser{pingcap: parser.New(), preserveCase: true} } type Parser struct { - pingcap *parser.Parser + pingcap *parser.Parser + preserveCase bool } var lineColumn = regexp.MustCompile(`^line (\d+) column (\d+) (.*)`) @@ -86,7 +96,7 @@ func (p *Parser) ParseFile(r io.Reader) (*ast.File, error) { // own occurrence even when two statements read the same. searchFrom := 0 for i := range stmtNodes { - converter := &cc{} + converter := &cc{preserveCase: p.preserveCase} // A statement sqlc has no node for converts to a TODO and stays in // the list: the formatter needs its extent to keep it as written, // and Parse filters it out for the compiler. diff --git a/internal/engine/dolphin/utils.go b/internal/engine/dolphin/utils.go index 238c2e37dd..fd443aa8f5 100644 --- a/internal/engine/dolphin/utils.go +++ b/internal/engine/dolphin/utils.go @@ -7,21 +7,21 @@ import ( "github.com/sqlc-dev/sqlc/internal/sql/ast" ) -func parseTableName(n *pcast.TableName) *ast.TableName { +func (c *cc) parseTableName(n *pcast.TableName) *ast.TableName { return &ast.TableName{ - Schema: identifier(n.Schema.String()), - Name: identifier(n.Name.String()), + Schema: c.identifier(n.Schema.String()), + Name: c.identifier(n.Name.String()), } } -func toList(node pcast.Node) *ast.List { +func (c *cc) toList(node pcast.Node) *ast.List { var items []ast.Node switch n := node.(type) { case *pcast.TableName: if schema := n.Schema.String(); schema != "" { - items = append(items, NewIdentifier(schema)) + items = append(items, c.NewIdentifier(schema)) } - items = append(items, NewIdentifier(n.Name.String())) + items = append(items, c.NewIdentifier(n.Name.String())) default: return nil } diff --git a/internal/sql/ast/a_expr.go b/internal/sql/ast/a_expr.go index 7501ff0eb6..1327467cf5 100644 --- a/internal/sql/ast/a_expr.go +++ b/internal/sql/ast/a_expr.go @@ -56,6 +56,33 @@ func (n *A_Expr) isNamedParam() (sigil, name string, ok bool) { return "", "", false } +// gluePrefix reports that this prefix expression must print with no space +// between the operator and its operand. `@name::type` is a sqlc named +// parameter with a cast: the cast binds tighter than the operator, so the +// parse is @ applied to a TypeCast and isNamedParam does not see it — but +// `@ name::type` would break the parameter apart for sqlc's scanner. Glue +// the sigils onto any operand that starts with an identifier, which also +// keeps a genuine prefix operator like pg's absolute-value @ meaning the +// same thing. +func (n *A_Expr) gluePrefix() bool { + if set(n.Lexpr) || n.Name == nil || len(n.Name.Items) != 1 { + return false + } + s, ok := n.Name.Items[0].(*String) + if !ok || (s.Str != "@" && s.Str != ":" && s.Str != "$") { + return false + } + operand := n.Rexpr + if tc, ok := operand.(*TypeCast); ok { + operand = tc.Arg + } + switch operand.(type) { + case *ColumnRef, *FuncCall: + return true + } + return false +} + // negated returns true when the expression's operator carries the negated // spelling of a pattern-match operator (e.g. "!~~" for NOT LIKE). func (n *A_Expr) negated() bool { @@ -142,7 +169,9 @@ func (n *A_Expr) Format(buf *TrackedBuffer, d format.Dialect) { } buf.astFormat(n.Name, d) if set(n.Rexpr) { - buf.WriteString(" ") + if !n.gluePrefix() { + buf.WriteString(" ") + } buf.astFormat(n.Rexpr, d) } } diff --git a/internal/sql/ast/column_def.go b/internal/sql/ast/column_def.go index bc6687d4cb..9bacaa8083 100644 --- a/internal/sql/ast/column_def.go +++ b/internal/sql/ast/column_def.go @@ -53,6 +53,10 @@ func (n *ColumnDef) Format(buf *TrackedBuffer, d format.Dialect) { if !n.Typeless { buf.WriteString(" ") buf.astFormat(n.TypeName, d) + // MySQL integer types: signedness is part of the type. + if n.IsUnsigned { + buf.WriteString(" unsigned") + } } // Use IsArray from ColumnDef since TypeName.ArrayBounds may not be set // (for type resolution compatibility) diff --git a/internal/sql/ast/res_target.go b/internal/sql/ast/res_target.go index 58a7e1dac5..8a58b273d2 100644 --- a/internal/sql/ast/res_target.go +++ b/internal/sql/ast/res_target.go @@ -9,6 +9,10 @@ type ResTarget struct { Indirection *List `json:"indirection,omitempty"` Val Node `json:"val,omitempty"` Location int `json:"location"` + // Relation qualifies Name in a multi-table UPDATE's SET list (MySQL: + // SET t.col = ...). Analysis matches on Name alone; printing needs the + // qualifier to keep the assignment on the table the author named. + Relation *string `json:"relation,omitempty"` } func (n *ResTarget) Pos() int { diff --git a/internal/sql/ast/select_stmt.go b/internal/sql/ast/select_stmt.go index ad9e10f9a1..e4e9eaf8a8 100644 --- a/internal/sql/ast/select_stmt.go +++ b/internal/sql/ast/select_stmt.go @@ -25,6 +25,11 @@ type SelectStmt struct { All bool `json:"all"` Larg *SelectStmt `json:"larg,omitempty"` Rarg *SelectStmt `json:"rarg,omitempty"` + // TableHints is the text inside a MySQL optimizer-hint comment + // (SELECT /*+ MAX_EXECUTION_TIME(1000) */ ...). The compiler ignores + // hints; printing keeps them because they change how the server runs + // the query. + TableHints string `json:"table_hints,omitempty"` } func (n *SelectStmt) Pos() int { @@ -100,6 +105,11 @@ func (n *SelectStmt) Format(buf *TrackedBuffer, d format.Dialect) { buf.astFormat(n.Rarg, d) } else { buf.WriteString("SELECT") + if n.TableHints != "" { + buf.WriteString(" /*+ ") + buf.WriteString(n.TableHints) + buf.WriteString(" */") + } if items(n.DistinctClause) { buf.WriteString(" DISTINCT") if !todo(n.DistinctClause) { diff --git a/internal/sql/ast/update_stmt.go b/internal/sql/ast/update_stmt.go index dc154cfd35..3c92fb929c 100644 --- a/internal/sql/ast/update_stmt.go +++ b/internal/sql/ast/update_stmt.go @@ -19,6 +19,11 @@ type UpdateStmt struct { // PostgreSQL 18 RETURNING WITH (OLD AS ..., NEW AS ...) aliases ReturningOldAlias string `json:"returning_old_alias"` ReturningNewAlias string `json:"returning_new_alias"` + // TableRefs is the statement's table-reference tree as the author wrote + // it (MySQL: UPDATE a JOIN b ON ...). Relations flattens that tree to + // bare tables for analysis; printing prefers TableRefs when it is set, + // so a join's ON condition survives formatting. + TableRefs *List `json:"table_refs,omitempty"` } func (n *UpdateStmt) Pos() int { @@ -44,7 +49,9 @@ func (n *UpdateStmt) Format(buf *TrackedBuffer, d format.Dialect) { } buf.WriteString("UPDATE ") - if items(n.Relations) { + if items(n.TableRefs) { + buf.astFormat(n.TableRefs, d) + } else if items(n.Relations) { buf.astFormat(n.Relations, d) } @@ -102,6 +109,10 @@ func (n *UpdateStmt) Format(buf *TrackedBuffer, d format.Dialect) { } switch nn := item.(type) { case *ResTarget: + if nn.Relation != nil { + buf.WriteString(d.QuoteIdent(*nn.Relation)) + buf.WriteString(".") + } if nn.Name != nil { buf.WriteString(d.QuoteIdent(*nn.Name)) }