From c60ad4c3f5c5a702c382bc4182b81eae3fe898b8 Mon Sep 17 00:00:00 2001 From: Ajit Pratap Singh Date: Mon, 12 Oct 2026 11:15:07 +0530 Subject: [PATCH 1/3] perf(ast): stop heap-copying nodes in Children() so Walk/Inspect allocate less (#557) Children() used to copy every value-typed child (TableReference, JoinClause, WindowSpec, OrderByExpression, WhenClause, UpdateExpression, ...) to the heap through the `x := x; append(children, &x)` pattern. It also built unsized slices through temporary nodifyExpressions slices. Every Walk/Inspect user paid for this: linter, security scanner, metadata extraction, fingerprint/normalize and transpiler. Visitors that mutated those nodes changed copies, not the AST. - Take addresses by index (&s.From[i], &s.Joins[i], &f.OrderBy[i], ...) in every Children(), removing all G601 copy sites outside sql.go rendering. - Presize the result and append expressions directly in SelectStatement, InsertStatement, WindowSpec, FunctionCall, CaseExpression and MergeStatement. Each now makes exactly one allocation. - Add unexported pointer-receiver childrenInPlace on *JoinClause, *WindowFrame and *MergeStatement, and an unexported childrenOf that Walk uses. Walk now visits Left/Right, Start and TargetTable/SourceTable in place without copying the receiver. The exported value-receiver Children()/TokenLiteral are unchanged in signature and output, so no method set changes. - Document the aliasing and lifetime contract on Node and Walk. Behaviour change (intended): visitor mutations of value-typed children now reach the AST, and those pointers share the AST's lifetime. Measured (benchstat, -count=6, allocs/op and B/op): Inspect/select_join_window 47 -> 26 allocs, 3216 -> 928 B Inspect/insert_multirow 16 -> 1 allocs, 2144 -> 576 B Fingerprint 182 -> 170 allocs, 20.8 -> 19.0 KiB Normalize 178 -> 166 allocs Lint/Realistic 728 -> 580 allocs, 56.8 -> 36.6 KiB ExtractMetadataLarge 587 -> 466 allocs, 49.6 -> 31.8 KiB Scan*, Transpile unchanged Tests: a visit-order golden generated from the pre-change code, a Walk-vs-Children equivalence check, pointer-identity tests for every rewritten Children(), childrenOf/Children() agreement, one-alloc budgets, visitor-mutation regression tests checked through SQL() and the formatter, and a concurrent pooled-tree Inspect test under -race. New BenchmarkInspect, BenchmarkFingerprint and BenchmarkNormalize. Fixes #557 Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01UZSk1292jPc8ipDboNxeve --- pkg/fingerprint/fingerprint_bench_test.go | 44 ++++ pkg/sql/ast/ast.go | 7 + pkg/sql/ast/ast_clauses.go | 72 +++--- pkg/sql/ast/ast_expressions.go | 40 ++-- pkg/sql/ast/ast_statements.go | 175 ++++++++++---- pkg/sql/ast/children_alias_test.go | 267 +++++++++++++++++++++ pkg/sql/ast/dml.go | 15 +- pkg/sql/ast/testdata/visit_order.golden | 198 ++++++++++++++++ pkg/sql/ast/trigger.go | 5 +- pkg/sql/ast/visitor.go | 30 ++- pkg/sql/ast/visitor_bench_test.go | 82 +++++++ pkg/sql/ast/visitor_mutation_test.go | 271 ++++++++++++++++++++++ pkg/sql/ast/visitor_order_test.go | 175 ++++++++++++++ 13 files changed, 1278 insertions(+), 103 deletions(-) create mode 100644 pkg/fingerprint/fingerprint_bench_test.go create mode 100644 pkg/sql/ast/children_alias_test.go create mode 100644 pkg/sql/ast/testdata/visit_order.golden create mode 100644 pkg/sql/ast/visitor_bench_test.go create mode 100644 pkg/sql/ast/visitor_mutation_test.go create mode 100644 pkg/sql/ast/visitor_order_test.go diff --git a/pkg/fingerprint/fingerprint_bench_test.go b/pkg/fingerprint/fingerprint_bench_test.go new file mode 100644 index 000000000..0a4fea111 --- /dev/null +++ b/pkg/fingerprint/fingerprint_bench_test.go @@ -0,0 +1,44 @@ +// Copyright 2026 GoSQLX Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package fingerprint_test + +import ( + "testing" + + "github.com/ajitpratap0/GoSQLX/pkg/fingerprint" +) + +const benchJoinSQL = "SELECT c.id, c.name, SUM(o.total) AS spend FROM customers c " + + "JOIN orders o ON o.customer_id = c.id LEFT JOIN regions g ON g.code = c.region " + + "WHERE c.active = 1 AND o.created_at > '2024-01-01' GROUP BY c.id, c.name " + + "ORDER BY spend DESC, c.name LIMIT 50" + +func BenchmarkFingerprint(b *testing.B) { + b.ReportAllocs() + for i := 0; i < b.N; i++ { + if _, err := fingerprint.Fingerprint(benchJoinSQL); err != nil { + b.Fatal(err) + } + } +} + +func BenchmarkNormalize(b *testing.B) { + b.ReportAllocs() + for i := 0; i < b.N; i++ { + if _, err := fingerprint.Normalize(benchJoinSQL); err != nil { + b.Fatal(err) + } + } +} diff --git a/pkg/sql/ast/ast.go b/pkg/sql/ast/ast.go index 43f598ec2..8a32985d6 100644 --- a/pkg/sql/ast/ast.go +++ b/pkg/sql/ast/ast.go @@ -31,6 +31,13 @@ import ( // The Node interface enables the visitor pattern for AST traversal. Use the // Walk() and Inspect() functions from visitor.go to traverse the tree. // +// Children stored by value in a parent (TableReference, JoinClause, +// OrderByExpression, WindowSpec, WhenClause, UpdateExpression, ColumnDef and +// its constraints, SetClause, ...) are returned by Children() as pointers into +// the parent's storage rather than as fresh copies. Mutations through them +// change the AST, and they share the AST's lifetime: do not retain them after +// ast.ReleaseAST, just like every other child pointer. +// // Example - Checking node type: // // switch node := astNode.(type) { diff --git a/pkg/sql/ast/ast_clauses.go b/pkg/sql/ast/ast_clauses.go index b58044e62..7aea4010e 100644 --- a/pkg/sql/ast/ast_clauses.go +++ b/pkg/sql/ast/ast_clauses.go @@ -78,12 +78,22 @@ type JoinClause struct { func (j *JoinClause) expressionNode() {} func (j JoinClause) TokenLiteral() string { return j.Type + " JOIN" } -func (j JoinClause) Children() []Node { - children := []Node{&j.Left, &j.Right} + +// Children returns Left, Right and, when present, Condition. +// +// The value receiver is kept for API compatibility. Called on a value it +// returns addresses into the receiver copy (one heap copy per call), while +// Walk and Inspect use childrenInPlace and visit the stored fields in place +// (#557). +func (j JoinClause) Children() []Node { return j.childrenInPlace() } + +// childrenInPlace is Children for a JoinClause reached through a pointer: +// Left and Right are returned as pointers into *j itself. +func (j *JoinClause) childrenInPlace() []Node { if j.Condition != nil { - children = append(children, j.Condition) + return []Node{&j.Left, &j.Right, j.Condition} } - return children + return []Node{&j.Left, &j.Right} } // TableReference represents a table reference in a FROM clause. @@ -246,11 +256,16 @@ type WindowSpec struct { func (w *WindowSpec) statementNode() {} func (w WindowSpec) TokenLiteral() string { return "WINDOW" } func (w WindowSpec) Children() []Node { - children := make([]Node, 0) - children = append(children, nodifyExpressions(w.PartitionBy)...) - for _, orderBy := range w.OrderBy { - orderBy := orderBy // G601: Create local copy to avoid memory aliasing - children = append(children, &orderBy) + n := len(w.PartitionBy) + len(w.OrderBy) + if w.FrameClause != nil { + n++ + } + children := make([]Node, 0, n) + for _, e := range w.PartitionBy { + children = append(children, e) + } + for i := range w.OrderBy { + children = append(children, &w.OrderBy[i]) } if w.FrameClause != nil { children = append(children, w.FrameClause) @@ -267,13 +282,22 @@ type WindowFrame struct { func (w *WindowFrame) statementNode() {} func (w WindowFrame) TokenLiteral() string { return w.Type } -func (w WindowFrame) Children() []Node { - // Start is a value type, always include it to support visitor traversal. - children := []Node{&w.Start} + +// Children returns Start and, when present, End. +// +// The value receiver is kept for API compatibility. Called on a value it +// returns addresses into the receiver copy (one heap copy per call), while +// Walk and Inspect use childrenInPlace and visit the stored fields in place +// (#557). +func (w WindowFrame) Children() []Node { return w.childrenInPlace() } + +// childrenInPlace is Children for a WindowFrame reached through a pointer: +// Start is a value type, always included, and returned as a pointer into *w. +func (w *WindowFrame) childrenInPlace() []Node { if w.End != nil { - children = append(children, w.End) + return []Node{&w.Start, w.End} } - return children + return []Node{&w.Start} } // WindowFrameBound represents window frame bound @@ -377,9 +401,8 @@ func (o OnConflict) TokenLiteral() string { return "ON CONFLICT" } func (o OnConflict) Children() []Node { children := nodifyExpressions(o.Target) if o.Action.DoUpdate != nil { - for _, update := range o.Action.DoUpdate { - update := update // G601: Create local copy to avoid memory aliasing - children = append(children, &update) + for i := range o.Action.DoUpdate { + children = append(children, &o.Action.DoUpdate[i]) } } if o.Action.Where != nil { @@ -404,9 +427,8 @@ func (u *UpsertClause) expressionNode() {} func (u UpsertClause) TokenLiteral() string { return "ON DUPLICATE KEY UPDATE" } func (u UpsertClause) Children() []Node { children := make([]Node, len(u.Updates)) - for i, update := range u.Updates { - update := update // G601: Create local copy to avoid memory aliasing - children[i] = &update + for i := range u.Updates { + children[i] = &u.Updates[i] } return children } @@ -422,9 +444,8 @@ func (c *ColumnDef) expressionNode() {} func (c ColumnDef) TokenLiteral() string { return c.Name } func (c ColumnDef) Children() []Node { children := make([]Node, len(c.Constraints)) - for i, constraint := range c.Constraints { - constraint := constraint // G601: Create local copy to avoid memory aliasing - children[i] = &constraint + for i := range c.Constraints { + children[i] = &c.Constraints[i] } return children } @@ -721,9 +742,8 @@ func (m MatchRecognizeClause) TokenLiteral() string { return "MATCH_RECOGNIZE" } func (m MatchRecognizeClause) Children() []Node { var nodes []Node nodes = append(nodes, nodifyExpressions(m.PartitionBy)...) - for _, ob := range m.OrderBy { - ob := ob - nodes = append(nodes, &ob) + for i := range m.OrderBy { + nodes = append(nodes, &m.OrderBy[i]) } for _, md := range m.Measures { if md.Expr != nil { diff --git a/pkg/sql/ast/ast_expressions.go b/pkg/sql/ast/ast_expressions.go index b22c0bde8..97c5d205e 100644 --- a/pkg/sql/ast/ast_expressions.go +++ b/pkg/sql/ast/ast_expressions.go @@ -153,9 +153,19 @@ type FunctionCall struct { func (f *FunctionCall) expressionNode() {} func (f FunctionCall) TokenLiteral() string { return f.Name } func (f FunctionCall) Children() []Node { - children := nodifyExpressions(f.Arguments) - if len(f.Parameters) > 0 { - children = append(children, nodifyExpressions(f.Parameters)...) + n := len(f.Arguments) + len(f.Parameters) + len(f.OrderBy) + len(f.WithinGroup) + if f.Over != nil { + n++ + } + if f.Filter != nil { + n++ + } + children := make([]Node, 0, n) + for _, e := range f.Arguments { + children = append(children, e) + } + for _, e := range f.Parameters { + children = append(children, e) } if f.Over != nil { children = append(children, f.Over) @@ -163,13 +173,11 @@ func (f FunctionCall) Children() []Node { if f.Filter != nil { children = append(children, f.Filter) } - for _, orderBy := range f.OrderBy { - orderBy := orderBy // G601: Create local copy to avoid memory aliasing - children = append(children, &orderBy) + for i := range f.OrderBy { + children = append(children, &f.OrderBy[i]) } - for _, orderBy := range f.WithinGroup { - orderBy := orderBy // G601: Create local copy to avoid memory aliasing - children = append(children, &orderBy) + for i := range f.WithinGroup { + children = append(children, &f.WithinGroup[i]) } return children } @@ -185,13 +193,19 @@ type CaseExpression struct { func (c *CaseExpression) expressionNode() {} func (c CaseExpression) TokenLiteral() string { return "CASE" } func (c CaseExpression) Children() []Node { - children := make([]Node, 0) + n := len(c.WhenClauses) + if c.Value != nil { + n++ + } + if c.ElseClause != nil { + n++ + } + children := make([]Node, 0, n) if c.Value != nil { children = append(children, c.Value) } - for _, when := range c.WhenClauses { - when := when // G601: Create local copy to avoid memory aliasing - children = append(children, &when) + for i := range c.WhenClauses { + children = append(children, &c.WhenClauses[i]) } if c.ElseClause != nil { children = append(children, c.ElseClause) diff --git a/pkg/sql/ast/ast_statements.go b/pkg/sql/ast/ast_statements.go index 7f5e893cd..1e54d0664 100644 --- a/pkg/sql/ast/ast_statements.go +++ b/pkg/sql/ast/ast_statements.go @@ -151,20 +151,59 @@ func (s *SelectStatement) statementNode() {} func (s *SelectStatement) queryExpressionNode() {} func (s SelectStatement) TokenLiteral() string { return "SELECT" } +// Children returns the statement's child nodes in source order. The slice is +// sized up front, and value-typed children (From, Joins, Windows, OrderBy) are +// returned as pointers into this statement's slices: the receiver is a copy, +// but its slice headers share the parser-owned backing arrays, so mutations +// through these pointers reach the AST and nothing is heap-copied (#557). func (s SelectStatement) Children() []Node { - children := make([]Node, 0) + n := len(s.DistinctOnColumns) + len(s.Columns) + len(s.From) + len(s.Joins) + + len(s.GroupBy) + len(s.Windows) + len(s.OrderBy) + if s.With != nil { + n++ + } + if s.Sample != nil { + n++ + } + if s.PrewhereClause != nil { + n++ + } + if s.Where != nil { + n++ + } + if s.Having != nil { + n++ + } + if s.Qualify != nil { + n++ + } + if s.Fetch != nil { + n++ + } + if s.For != nil { + n++ + } + if s.StartWith != nil { + n++ + } + if s.ConnectBy != nil { + n++ + } + children := make([]Node, 0, n) if s.With != nil { children = append(children, s.With) } - children = append(children, nodifyExpressions(s.DistinctOnColumns)...) - children = append(children, nodifyExpressions(s.Columns)...) - for _, from := range s.From { - from := from // G601: Create local copy to avoid memory aliasing - children = append(children, &from) + for _, e := range s.DistinctOnColumns { + children = append(children, e) + } + for _, e := range s.Columns { + children = append(children, e) } - for _, join := range s.Joins { - join := join // G601: Create local copy to avoid memory aliasing - children = append(children, &join) + for i := range s.From { + children = append(children, &s.From[i]) + } + for i := range s.Joins { + children = append(children, &s.Joins[i]) } if s.Sample != nil { children = append(children, s.Sample) @@ -175,20 +214,20 @@ func (s SelectStatement) Children() []Node { if s.Where != nil { children = append(children, s.Where) } - children = append(children, nodifyExpressions(s.GroupBy)...) + for _, e := range s.GroupBy { + children = append(children, e) + } if s.Having != nil { children = append(children, s.Having) } if s.Qualify != nil { children = append(children, s.Qualify) } - for _, window := range s.Windows { - window := window // G601: Create local copy to avoid memory aliasing - children = append(children, &window) + for i := range s.Windows { + children = append(children, &s.Windows[i]) } - for _, orderBy := range s.OrderBy { - orderBy := orderBy // G601: Create local copy to avoid memory aliasing - children = append(children, &orderBy) + for i := range s.OrderBy { + children = append(children, &s.OrderBy[i]) } if s.Fetch != nil { children = append(children, s.Fetch) @@ -223,20 +262,44 @@ func (i *InsertStatement) statementNode() {} func (i InsertStatement) TokenLiteral() string { return "INSERT" } func (i InsertStatement) Children() []Node { - children := make([]Node, 0) + n := len(i.Columns) + len(i.Output) + len(i.Returning) + for _, row := range i.Values { + n += len(row) + } + if i.With != nil { + n++ + } + if i.Query != nil { + n++ + } + if i.OnConflict != nil { + n++ + } + if i.OnDuplicateKey != nil { + n++ + } + children := make([]Node, 0, n) if i.With != nil { children = append(children, i.With) } - children = append(children, nodifyExpressions(i.Columns)...) - children = append(children, nodifyExpressions(i.Output)...) + for _, e := range i.Columns { + children = append(children, e) + } + for _, e := range i.Output { + children = append(children, e) + } // Flatten multi-row values for Children() for _, row := range i.Values { - children = append(children, nodifyExpressions(row)...) + for _, e := range row { + children = append(children, e) + } } if i.Query != nil { children = append(children, i.Query) } - children = append(children, nodifyExpressions(i.Returning)...) + for _, e := range i.Returning { + children = append(children, e) + } if i.OnConflict != nil { children = append(children, i.OnConflict) } @@ -288,13 +351,11 @@ func (u UpdateStatement) Children() []Node { if u.With != nil { children = append(children, u.With) } - for _, assignment := range u.Assignments { - assignment := assignment // G601: Create local copy to avoid memory aliasing - children = append(children, &assignment) + for i := range u.Assignments { + children = append(children, &u.Assignments[i]) } - for _, from := range u.From { - from := from // G601: Create local copy to avoid memory aliasing - children = append(children, &from) + for i := range u.From { + children = append(children, &u.From[i]) } if u.Where != nil { children = append(children, u.Where) @@ -329,20 +390,17 @@ func (c *CreateTableStatement) statementNode() {} func (c CreateTableStatement) TokenLiteral() string { return "CREATE TABLE" } func (c CreateTableStatement) Children() []Node { children := make([]Node, 0) - for _, col := range c.Columns { - col := col // G601: Create local copy to avoid memory aliasing - children = append(children, &col) + for i := range c.Columns { + children = append(children, &c.Columns[i]) } - for _, constraint := range c.Constraints { - constraint := constraint // G601: Create local copy to avoid memory aliasing - children = append(children, &constraint) + for i := range c.Constraints { + children = append(children, &c.Constraints[i]) } if c.PartitionBy != nil { children = append(children, c.PartitionBy) } - for _, p := range c.Partitions { - p := p // G601: Create local copy - children = append(children, &p) + for i := range c.Partitions { + children = append(children, &c.Partitions[i]) } return children } @@ -366,9 +424,8 @@ func (d DeleteStatement) Children() []Node { if d.With != nil { children = append(children, d.With) } - for _, using := range d.Using { - using := using // G601: Create local copy to avoid memory aliasing - children = append(children, &using) + for i := range d.Using { + children = append(children, &d.Using[i]) } if d.Where != nil { children = append(children, d.Where) @@ -403,9 +460,8 @@ func (a *AlterTableStatement) statementNode() {} func (a AlterTableStatement) TokenLiteral() string { return "ALTER TABLE" } func (a AlterTableStatement) Children() []Node { children := make([]Node, len(a.Actions)) - for i, action := range a.Actions { - action := action // G601: Create local copy to avoid memory aliasing - children[i] = &action + for i := range a.Actions { + children[i] = &a.Actions[i] } return children } @@ -446,9 +502,8 @@ func (c *CreateIndexStatement) statementNode() {} func (c CreateIndexStatement) TokenLiteral() string { return "CREATE INDEX" } func (c CreateIndexStatement) Children() []Node { children := make([]Node, 0) - for _, col := range c.Columns { - col := col // G601: Create local copy to avoid memory aliasing - children = append(children, &col) + for i := range c.Columns { + children = append(children, &c.Columns[i]) } if c.Where != nil { children = append(children, c.Where) @@ -474,15 +529,34 @@ type MergeStatement struct { func (m *MergeStatement) statementNode() {} func (m MergeStatement) TokenLiteral() string { return "MERGE" } -func (m MergeStatement) Children() []Node { - children := []Node{&m.TargetTable, &m.SourceTable} + +// Children returns TargetTable, SourceTable, OnCondition, the WHEN clauses and +// the OUTPUT columns, in that order. +// +// The value receiver is kept for API compatibility. Called on a value it +// returns addresses into the receiver copy (one heap copy per call), while +// Walk and Inspect use childrenInPlace and visit the stored fields in place +// (#557). +func (m MergeStatement) Children() []Node { return m.childrenInPlace() } + +// childrenInPlace is Children for a MergeStatement reached through a pointer: +// TargetTable and SourceTable are returned as pointers into *m itself. +func (m *MergeStatement) childrenInPlace() []Node { + n := 2 + len(m.WhenClauses) + len(m.Output) + if m.OnCondition != nil { + n++ + } + children := make([]Node, 0, n) + children = append(children, &m.TargetTable, &m.SourceTable) if m.OnCondition != nil { children = append(children, m.OnCondition) } for _, when := range m.WhenClauses { children = append(children, when) } - children = append(children, nodifyExpressions(m.Output)...) + for _, e := range m.Output { + children = append(children, e) + } return children } @@ -521,9 +595,8 @@ func (a *MergeAction) expressionNode() {} func (a MergeAction) TokenLiteral() string { return a.ActionType } func (a MergeAction) Children() []Node { children := make([]Node, 0) - for _, set := range a.SetClauses { - set := set // G601: Create local copy - children = append(children, &set) + for i := range a.SetClauses { + children = append(children, &a.SetClauses[i]) } for _, val := range a.Values { children = append(children, val) diff --git a/pkg/sql/ast/children_alias_test.go b/pkg/sql/ast/children_alias_test.go new file mode 100644 index 000000000..6479ae546 --- /dev/null +++ b/pkg/sql/ast/children_alias_test.go @@ -0,0 +1,267 @@ +// Copyright 2026 GoSQLX Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package ast + +import ( + "fmt" + "testing" +) + +// Tests for #557: Children() returns value-typed children as pointers into +// the parent's storage instead of heap copies, and Walk reaches the fields of +// JoinClause, WindowFrame and MergeStatement in place through childrenOf. + +func ident(name string) *Identifier { return &Identifier{Name: name} } + +func lit(v int) *LiteralValue { return &LiteralValue{Value: v, Type: "INTEGER"} } + +func obe(name string) OrderByExpression { + return OrderByExpression{Expression: ident(name), Ascending: true} +} + +// assertAliases checks that every pointer in want appears in the result of +// children, and that a second call returns the very same pointer at the same +// position (pointer identity is stable). +func assertAliases(t *testing.T, children func() []Node, want []Node) { + t.Helper() + first, second := children(), children() + if len(first) != len(second) { + t.Fatalf("two Children() calls returned %d and %d nodes", len(first), len(second)) + } + for wi, w := range want { + idx := -1 + for i, c := range first { + if c == w { + idx = i + break + } + } + if idx < 0 { + t.Fatalf("want[%d] (%T %p) is not returned by Children(); got a copy instead", wi, w, w) + } + if second[idx] != w { + t.Fatalf("want[%d] (%T): second call returned a different pointer", wi, w) + } + } +} + +func TestChildren_AliasesStoredNodes(t *testing.T) { + sel := &SelectStatement{ + Columns: []Expression{ident("a")}, + From: []TableReference{{Name: "t1"}, {Name: "t2"}}, + Joins: []JoinClause{{Type: "INNER", Right: TableReference{Name: "t3"}}}, + Windows: []WindowSpec{{Name: "w"}}, + OrderBy: []OrderByExpression{obe("a"), obe("b")}, + } + fn := &FunctionCall{ + Name: "F", + Arguments: []Expression{ident("x")}, + OrderBy: []OrderByExpression{obe("y")}, + WithinGroup: []OrderByExpression{obe("z")}, + } + ce := &CaseExpression{WhenClauses: []WhenClause{{Condition: ident("c"), Result: lit(1)}, {Condition: ident("d"), Result: lit(2)}}} + ws := &WindowSpec{PartitionBy: []Expression{ident("g")}, OrderBy: []OrderByExpression{obe("o")}} + upd := &UpdateStatement{ + Assignments: []UpdateExpression{{Column: ident("a"), Value: lit(1)}}, + From: []TableReference{{Name: "f"}}, + } + del := &DeleteStatement{Using: []TableReference{{Name: "u1"}, {Name: "u2"}}} + ct := &CreateTableStatement{ + Columns: []ColumnDef{{Name: "id"}}, + Constraints: []TableConstraint{{Type: "CHECK", Check: ident("ok")}}, + Partitions: []PartitionDefinition{{Name: "p0"}}, + } + at := &AlterTableStatement{Actions: []AlterTableAction{{Type: "ADD COLUMN"}, {Type: "DROP COLUMN"}}} + ci := &CreateIndexStatement{Columns: []IndexColumn{{Column: "a"}, {Column: "b"}}} + ma := &MergeAction{ActionType: "UPDATE", SetClauses: []SetClause{{Column: "v", Value: lit(1)}}} + oc := &OnConflict{Action: OnConflictAction{DoUpdate: []UpdateExpression{{Column: ident("b"), Value: lit(2)}}}} + up := &UpsertClause{Updates: []UpdateExpression{{Column: ident("b"), Value: lit(3)}}} + cd := &ColumnDef{Name: "c", Constraints: []ColumnConstraint{{Type: "NOT NULL"}, {Type: "UNIQUE"}}} + mr := &MatchRecognizeClause{OrderBy: []OrderByExpression{obe("ts")}} + ls := &Select{From: []TableReference{{Name: "t"}}, OrderBy: []OrderByExpression{obe("a")}} + lu := &Update{Updates: []UpdateExpression{{Column: ident("a"), Value: lit(1)}}} + te := &TriggerEvent{Type: TriggerEventUpdate, Columns: []Identifier{{Name: "a"}, {Name: "b"}}} + + cases := []struct { + name string + node Node + want []Node + }{ + {"SelectStatement", sel, []Node{&sel.From[0], &sel.From[1], &sel.Joins[0], &sel.Windows[0], &sel.OrderBy[0], &sel.OrderBy[1]}}, + {"FunctionCall", fn, []Node{&fn.OrderBy[0], &fn.WithinGroup[0]}}, + {"CaseExpression", ce, []Node{&ce.WhenClauses[0], &ce.WhenClauses[1]}}, + {"WindowSpec", ws, []Node{&ws.OrderBy[0]}}, + {"UpdateStatement", upd, []Node{&upd.Assignments[0], &upd.From[0]}}, + {"DeleteStatement", del, []Node{&del.Using[0], &del.Using[1]}}, + {"CreateTableStatement", ct, []Node{&ct.Columns[0], &ct.Constraints[0], &ct.Partitions[0]}}, + {"AlterTableStatement", at, []Node{&at.Actions[0], &at.Actions[1]}}, + {"CreateIndexStatement", ci, []Node{&ci.Columns[0], &ci.Columns[1]}}, + {"MergeAction", ma, []Node{&ma.SetClauses[0]}}, + {"OnConflict", oc, []Node{&oc.Action.DoUpdate[0]}}, + {"UpsertClause", up, []Node{&up.Updates[0]}}, + {"ColumnDef", cd, []Node{&cd.Constraints[0], &cd.Constraints[1]}}, + {"MatchRecognizeClause", mr, []Node{&mr.OrderBy[0]}}, + {"LegacySelect", ls, []Node{&ls.From[0], &ls.OrderBy[0]}}, + {"LegacyUpdate", lu, []Node{&lu.Updates[0]}}, + {"TriggerEvent", te, []Node{&te.Columns[0], &te.Columns[1]}}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + assertAliases(t, c.node.Children, c.want) + // Walk sees the same pointers. + assertAliases(t, func() []Node { return childrenOf(c.node) }, c.want) + }) + } +} + +// nodeSignature describes a child list by dynamic type and TokenLiteral. +func nodeSignature(nodes []Node) []string { + out := make([]string, len(nodes)) + for i, n := range nodes { + out[i] = fmt.Sprintf("%T %q", n, n.TokenLiteral()) + } + return out +} + +// TestChildrenOf_InPlaceFastPath covers the value-receiver nodes whose fields +// Walk reaches through childrenInPlace: childrenOf must address the stored +// struct and must agree with the exported Children() in shape. +func TestChildrenOf_InPlaceFastPath(t *testing.T) { + cond := &BinaryExpression{Left: ident("a"), Operator: "=", Right: ident("b")} + end := &WindowFrameBound{Type: "CURRENT ROW"} + when := &MergeWhenClause{Type: "MATCHED"} + output := ident("out") + + join := &JoinClause{Type: "LEFT", Left: TableReference{Name: "l"}, Right: TableReference{Name: "r"}, Condition: cond} + joinNoCond := &JoinClause{Type: "CROSS", Left: TableReference{Name: "l"}, Right: TableReference{Name: "r"}} + frame := &WindowFrame{Type: "ROWS", Start: WindowFrameBound{Type: "UNBOUNDED PRECEDING"}, End: end} + frameNoEnd := &WindowFrame{Type: "ROWS", Start: WindowFrameBound{Type: "CURRENT ROW"}} + merge := &MergeStatement{ + TargetTable: TableReference{Name: "tgt"}, + SourceTable: TableReference{Name: "src"}, + OnCondition: cond, + WhenClauses: []*MergeWhenClause{when}, + Output: []Expression{output}, + } + + cases := []struct { + name string + node Node + valueRes []Node // Children() on the stored value + inPlace []Node // exact pointers childrenOf must return + shared []int // indices of pointer-typed children both paths share + }{ + {"JoinClause", join, join.Children(), []Node{&join.Left, &join.Right, cond}, []int{2}}, + {"JoinClauseNoCondition", joinNoCond, joinNoCond.Children(), []Node{&joinNoCond.Left, &joinNoCond.Right}, nil}, + {"WindowFrame", frame, frame.Children(), []Node{&frame.Start, end}, []int{1}}, + {"WindowFrameNoEnd", frameNoEnd, frameNoEnd.Children(), []Node{&frameNoEnd.Start}, nil}, + {"MergeStatement", merge, merge.Children(), []Node{&merge.TargetTable, &merge.SourceTable, cond, when, output}, []int{2, 3, 4}}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + got := childrenOf(c.node) + if len(got) != len(c.inPlace) { + t.Fatalf("childrenOf returned %d nodes, want %d", len(got), len(c.inPlace)) + } + for i := range got { + if got[i] != c.inPlace[i] { + t.Fatalf("childrenOf[%d] = %T %p, want the stored field %p", i, got[i], got[i], c.inPlace[i]) + } + } + gs, vs := nodeSignature(got), nodeSignature(c.valueRes) + if fmt.Sprint(gs) != fmt.Sprint(vs) { + t.Fatalf("childrenOf and Children() disagree:\n in place: %v\n value: %v", gs, vs) + } + for _, i := range c.shared { + if c.valueRes[i] != got[i] { + t.Fatalf("pointer child %d differs between Children() and childrenOf", i) + } + } + }) + } +} + +var childrenSink []Node + +func TestChildren_AllocBudget(t *testing.T) { + if raceDetectorEnabled { + t.Skip("allocation counts are not meaningful under -race") + } + sel := &SelectStatement{ + Columns: []Expression{ident("a"), ident("b")}, + From: []TableReference{{Name: "t1"}, {Name: "t2"}}, + Joins: []JoinClause{ + {Type: "INNER", Right: TableReference{Name: "t3"}, Condition: ident("c1")}, + {Type: "LEFT", Right: TableReference{Name: "t4"}, Condition: ident("c2")}, + }, + Where: &BinaryExpression{Left: ident("a"), Operator: ">", Right: lit(1)}, + GroupBy: []Expression{ident("a")}, + Windows: []WindowSpec{{Name: "w", OrderBy: []OrderByExpression{obe("a")}}}, + OrderBy: []OrderByExpression{obe("a"), obe("b")}, + } + fn := &FunctionCall{ + Name: "F", + Arguments: []Expression{ident("x"), ident("y")}, + OrderBy: []OrderByExpression{obe("y")}, + WithinGroup: []OrderByExpression{obe("z")}, + } + ce := &CaseExpression{ + Value: ident("v"), + WhenClauses: []WhenClause{ + {Condition: lit(1), Result: lit(10)}, + {Condition: lit(2), Result: lit(20)}, + {Condition: lit(3), Result: lit(30)}, + }, + ElseClause: lit(0), + } + ws := &WindowSpec{ + PartitionBy: []Expression{ident("g")}, + OrderBy: []OrderByExpression{obe("o")}, + FrameClause: &WindowFrame{Type: "ROWS"}, + } + ins := &InsertStatement{ + Columns: []Expression{ident("a"), ident("b")}, + Values: [][]Expression{ + {lit(1), lit(2)}, + {lit(3), lit(4)}, + {lit(5), lit(6)}, + }, + Returning: []Expression{ident("a")}, + } + join := &JoinClause{Type: "INNER", Right: TableReference{Name: "r"}, Condition: ident("c")} + frame := &WindowFrame{Type: "ROWS", End: &WindowFrameBound{Type: "CURRENT ROW"}} + + cases := []struct { + name string + fn func() []Node + }{ + {"SelectStatement", func() []Node { return childrenOf(sel) }}, + {"FunctionCall", func() []Node { return childrenOf(fn) }}, + {"CaseExpression", func() []Node { return childrenOf(ce) }}, + {"WindowSpec", func() []Node { return childrenOf(ws) }}, + {"InsertStatement", func() []Node { return childrenOf(ins) }}, + {"JoinClauseInPlace", func() []Node { return childrenOf(join) }}, + {"WindowFrameInPlace", func() []Node { return childrenOf(frame) }}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + allocs := testing.AllocsPerRun(100, func() { childrenSink = c.fn() }) + if allocs != 1 { + t.Fatalf("got %v allocs per call, want exactly 1 (the result slice)", allocs) + } + }) + } + childrenSink = nil +} diff --git a/pkg/sql/ast/dml.go b/pkg/sql/ast/dml.go index 498d63b59..cbf1887b7 100644 --- a/pkg/sql/ast/dml.go +++ b/pkg/sql/ast/dml.go @@ -36,9 +36,8 @@ func (s Select) TokenLiteral() string { return "SELECT" } func (s Select) Children() []Node { children := make([]Node, 0) children = append(children, nodifyExpressions(s.Columns)...) - for _, from := range s.From { - from := from // G601: Create local copy to avoid memory aliasing - children = append(children, &from) + for i := range s.From { + children = append(children, &s.From[i]) } if s.Where != nil { children = append(children, s.Where) @@ -47,9 +46,8 @@ func (s Select) Children() []Node { if s.Having != nil { children = append(children, s.Having) } - for _, orderBy := range s.OrderBy { - orderBy := orderBy // G601: Create local copy to avoid memory aliasing - children = append(children, &orderBy) + for i := range s.OrderBy { + children = append(children, &s.OrderBy[i]) } return children } @@ -119,9 +117,8 @@ func (u Update) TokenLiteral() string { return "UPDATE" } func (u Update) Children() []Node { children := make([]Node, 0) children = append(children, &u.Table) - for _, update := range u.Updates { - update := update // G601: Create local copy to avoid memory aliasing - children = append(children, &update) + for i := range u.Updates { + children = append(children, &u.Updates[i]) } if u.Where != nil { children = append(children, u.Where) diff --git a/pkg/sql/ast/testdata/visit_order.golden b/pkg/sql/ast/testdata/visit_order.golden new file mode 100644 index 000000000..1d235126b --- /dev/null +++ b/pkg/sql/ast/testdata/visit_order.golden @@ -0,0 +1,198 @@ +== select_distinct_on_joins +*ast.SelectStatement "SELECT" + *ast.Identifier "a" + *ast.Identifier "a" + *ast.Identifier "b" + *ast.TableReference "t1" + *ast.JoinClause "INNER JOIN" + *ast.TableReference "t1" + *ast.TableReference "t2" + *ast.BinaryExpression "=" + *ast.Identifier "id" + *ast.Identifier "id" + *ast.JoinClause "LEFT JOIN" + *ast.TableReference "(t1_with_1_joins)" + *ast.TableReference "d" + *ast.SelectStatement "SELECT" + *ast.Identifier "id" + *ast.TableReference "t3" + *ast.BinaryExpression "=" + *ast.Identifier "id" + *ast.Identifier "id" + *ast.BinaryExpression ">" + *ast.Identifier "a" + *ast.LiteralValue "1" + *ast.Identifier "a" + *ast.Identifier "b" + *ast.BinaryExpression ">" + *ast.FunctionCall "COUNT" + *ast.Identifier "*" + *ast.LiteralValue "1" + *ast.OrderByExpression "ORDER BY" + *ast.Identifier "a" +== window_aggregate_order +*ast.SelectStatement "SELECT" + *ast.FunctionCall "SUM" + *ast.Identifier "x" + *ast.WindowSpec "WINDOW" + *ast.Identifier "g" + *ast.OrderByExpression "ORDER BY" + *ast.Identifier "y" + *ast.WindowFrame "ROWS" + *ast.WindowFrameBound "PRECEDING" + *ast.LiteralValue "1" + *ast.WindowFrameBound "CURRENT ROW" + *ast.FunctionCall "STRING_AGG" + *ast.Identifier "n" + *ast.LiteralValue "," + *ast.OrderByExpression "ORDER BY" + *ast.Identifier "n" + *ast.FunctionCall "PERCENTILE_CONT" + *ast.LiteralValue "0.5" + *ast.OrderByExpression "ORDER BY" + *ast.Identifier "v" + *ast.TableReference "t" + *ast.WindowSpec "WINDOW" + *ast.Identifier "g" + *ast.OrderByExpression "ORDER BY" + *ast.Identifier "y" +== case_expressions +*ast.SelectStatement "SELECT" + *ast.CaseExpression "CASE" + *ast.WhenClause "WHEN" + *ast.BinaryExpression "=" + *ast.Identifier "a" + *ast.LiteralValue "1" + *ast.LiteralValue "x" + *ast.WhenClause "WHEN" + *ast.BinaryExpression "=" + *ast.Identifier "a" + *ast.LiteralValue "2" + *ast.LiteralValue "y" + *ast.LiteralValue "z" + *ast.CaseExpression "CASE" + *ast.Identifier "b" + *ast.WhenClause "WHEN" + *ast.LiteralValue "1" + *ast.LiteralValue "2" + *ast.TableReference "t" +== cte_union +*ast.SetOperation "UNION" + *ast.SelectStatement "SELECT" + *ast.WithClause "WITH" + *ast.CommonTableExpr "c" + *ast.SelectStatement "SELECT" + *ast.AliasedExpression "x" + *ast.LiteralValue "1" + *ast.Identifier "x" + *ast.TableReference "c" + *ast.SelectStatement "SELECT" + *ast.LiteralValue "2" +== update_returning +*ast.UpdateStatement "UPDATE" + *ast.UpdateExpression "=" + *ast.Identifier "a" + *ast.LiteralValue "1" + *ast.UpdateExpression "=" + *ast.Identifier "b" + *ast.BinaryExpression "+" + *ast.Identifier "b" + *ast.LiteralValue "1" + *ast.BinaryExpression "=" + *ast.Identifier "id" + *ast.LiteralValue "2" + *ast.Identifier "a" +== delete_returning +*ast.DeleteStatement "DELETE" + *ast.BinaryExpression "=" + *ast.Identifier "id" + *ast.LiteralValue "1" + *ast.Identifier "id" +== insert_on_conflict +*ast.InsertStatement "INSERT" + *ast.Identifier "a" + *ast.Identifier "b" + *ast.LiteralValue "1" + *ast.LiteralValue "2" + *ast.LiteralValue "3" + *ast.LiteralValue "4" + *ast.Identifier "a" + *ast.OnConflict "ON CONFLICT" + *ast.Identifier "a" + *ast.UpdateExpression "=" + *ast.Identifier "b" + *ast.Identifier "b" + *ast.BinaryExpression "<" + *ast.Identifier "b" + *ast.LiteralValue "10" +== insert_on_duplicate_key +*ast.InsertStatement "INSERT" + *ast.Identifier "a" + *ast.Identifier "b" + *ast.LiteralValue "1" + *ast.LiteralValue "2" + *ast.UpsertClause "ON DUPLICATE KEY UPDATE" + *ast.UpdateExpression "=" + *ast.Identifier "b" + *ast.FunctionCall "VALUES" + *ast.Identifier "b" + *ast.UpdateExpression "=" + *ast.Identifier "a" + *ast.BinaryExpression "+" + *ast.Identifier "a" + *ast.LiteralValue "1" +== create_table +*ast.CreateTableStatement "CREATE TABLE" + *ast.ColumnDef "id" + *ast.ColumnConstraint "PRIMARY KEY" + *ast.ColumnDef "name" + *ast.ColumnConstraint "NOT NULL" + *ast.ColumnConstraint "DEFAULT" + *ast.LiteralValue "x" + *ast.TableConstraint "CHECK" + *ast.BinaryExpression ">" + *ast.Identifier "id" + *ast.LiteralValue "0" +== create_index +*ast.CreateIndexStatement "CREATE INDEX" + *ast.IndexColumn "a" + *ast.IndexColumn "b" + *ast.BinaryExpression ">" + *ast.Identifier "a" + *ast.LiteralValue "0" +== merge +*ast.MergeStatement "MERGE" + *ast.TableReference "tgt" + *ast.TableReference "src" + *ast.BinaryExpression "=" + *ast.Identifier "id" + *ast.Identifier "id" + *ast.MergeWhenClause "WHEN MATCHED" + *ast.MergeAction "UPDATE" + *ast.SetClause "t.v" + *ast.Identifier "v" + *ast.MergeWhenClause "WHEN NOT_MATCHED" + *ast.MergeAction "INSERT" + *ast.Identifier "id" + *ast.Identifier "v" +== match_recognize +*ast.SelectStatement "SELECT" + *ast.Identifier "*" + *ast.TableReference "t" + *ast.MatchRecognizeClause "MATCH_RECOGNIZE" + *ast.Identifier "s" + *ast.OrderByExpression "ORDER BY" + *ast.Identifier "ts" + *ast.FunctionCall "MATCH_NUMBER" + *ast.BinaryExpression ">" + *ast.Identifier "price" + *ast.FunctionCall "PREV" + *ast.Identifier "price" + *ast.BinaryExpression "<" + *ast.Identifier "price" + *ast.FunctionCall "PREV" + *ast.Identifier "price" +== alter_table +*ast.AlterStatement "ALTER" + *ast.AlterTableOperation "ALTER TABLE" + *ast.ColumnDef "c" diff --git a/pkg/sql/ast/trigger.go b/pkg/sql/ast/trigger.go index d62911117..8e15d7a6e 100644 --- a/pkg/sql/ast/trigger.go +++ b/pkg/sql/ast/trigger.go @@ -217,9 +217,8 @@ func (t TriggerEvent) Children() []Node { return nil } nodes := make([]Node, len(t.Columns)) - for i, col := range t.Columns { - col := col // G601: avoid memory aliasing - nodes[i] = &col + for i := range t.Columns { + nodes[i] = &t.Columns[i] } return nodes } diff --git a/pkg/sql/ast/visitor.go b/pkg/sql/ast/visitor.go index b5ea4d6fc..e118d078c 100644 --- a/pkg/sql/ast/visitor.go +++ b/pkg/sql/ast/visitor.go @@ -157,6 +157,17 @@ type Visitor interface { // fmt.Printf("Validation error: %v\n", err) // } // +// Aliasing and lifetime: +// +// Children stored by value in a parent (TableReference, JoinClause, +// OrderByExpression, WindowSpec, WindowFrameBound, WhenClause, +// UpdateExpression, ColumnDef and its constraints, SetClause, ...) are passed +// to the visitor as pointers into the parent's storage, not as copies, as long +// as the parent itself is reached through a pointer (always the case in a +// parsed tree). A visitor that mutates them therefore changes the AST itself. +// Like every other child pointer, they share the AST's lifetime and must not +// be retained after ast.ReleaseAST. +// // See also: Inspect(), Visitor, Inspector func Walk(v Visitor, node Node) error { if node == nil { @@ -172,7 +183,7 @@ func Walk(v Visitor, node Node) error { return nil } - for _, child := range node.Children() { + for _, child := range childrenOf(node) { if err := Walk(visitor, child); err != nil { return err } @@ -182,6 +193,23 @@ func Walk(v Visitor, node Node) error { return err } +// childrenOf returns n's children for Walk. For value-receiver nodes whose +// Children() must address their own fields (JoinClause, WindowFrame, +// MergeStatement) it uses the pointer-receiver childrenInPlace so the stored +// struct is neither heap-copied nor detached from the AST (#557). A concrete +// type switch is used because it is a cheap type-word compare. +func childrenOf(n Node) []Node { + switch t := n.(type) { + case *JoinClause: + return t.childrenInPlace() + case *WindowFrame: + return t.childrenInPlace() + case *MergeStatement: + return t.childrenInPlace() + } + return n.Children() +} + // Inspector represents a function-based AST visitor for simplified traversal. // // Inspector is a function type that can be used to traverse the AST without diff --git a/pkg/sql/ast/visitor_bench_test.go b/pkg/sql/ast/visitor_bench_test.go new file mode 100644 index 000000000..88fb9056b --- /dev/null +++ b/pkg/sql/ast/visitor_bench_test.go @@ -0,0 +1,82 @@ +// Copyright 2026 GoSQLX Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package ast_test + +import ( + "testing" + + "github.com/ajitpratap0/GoSQLX/pkg/gosqlx" + "github.com/ajitpratap0/GoSQLX/pkg/sql/ast" +) + +const inspectSelectJoinWindowSQL = `WITH recent AS ( + SELECT o.customer_id, SUM(o.total) AS spend + FROM orders o + WHERE o.created_at > '2024-01-01' + GROUP BY o.customer_id +) +SELECT c.id, c.name, r.spend, + ROW_NUMBER() OVER (PARTITION BY c.region ORDER BY r.spend DESC) AS rn, + CASE WHEN r.spend > 1000 THEN 'gold' WHEN r.spend > 100 THEN 'silver' ELSE 'bronze' END AS tier +FROM customers c +JOIN recent r ON r.customer_id = c.id +LEFT JOIN regions g ON g.code = c.region +WHERE c.active = 1 AND c.name LIKE 'A%' +ORDER BY r.spend DESC, c.name ASC +LIMIT 50` + +const inspectInsertMultirowSQL = `INSERT INTO t (a, b, c) VALUES + (1, 'a', 1.5), (2, 'b', 2.5), (3, 'c', 3.5), (4, 'd', 4.5), (5, 'e', 5.5), + (6, 'f', 6.5), (7, 'g', 7.5), (8, 'h', 8.5), (9, 'i', 9.5), (10, 'j', 10.5)` + +// BenchmarkInspect measures one full ast.Inspect traversal of an already +// parsed tree, i.e. the per-walk cost every linter rule, security check and +// fingerprint pass pays (#557). +func BenchmarkInspect(b *testing.B) { + cases := []struct { + name string + sql string + }{ + {"select_join_window", inspectSelectJoinWindowSQL}, + {"insert_multirow", inspectInsertMultirowSQL}, + } + for _, c := range cases { + b.Run(c.name, func(b *testing.B) { + tree, err := gosqlx.Parse(c.sql) + if err != nil { + b.Fatalf("parse: %v", err) + } + defer ast.ReleaseAST(tree) + visited := 0 + f := func(n ast.Node) bool { + if n != nil { + visited++ + } + return true + } + b.ReportAllocs() + b.ResetTimer() + for i := 0; i < b.N; i++ { + for _, stmt := range tree.Statements { + ast.Inspect(stmt, f) + } + } + b.StopTimer() + if visited == 0 { + b.Fatal("Inspect visited no nodes") + } + }) + } +} diff --git a/pkg/sql/ast/visitor_mutation_test.go b/pkg/sql/ast/visitor_mutation_test.go new file mode 100644 index 000000000..361ab21c2 --- /dev/null +++ b/pkg/sql/ast/visitor_mutation_test.go @@ -0,0 +1,271 @@ +// Copyright 2026 GoSQLX Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package ast_test + +import ( + "strings" + "sync" + "testing" + + "github.com/ajitpratap0/GoSQLX/pkg/formatter" + "github.com/ajitpratap0/GoSQLX/pkg/sql/ast" + "github.com/ajitpratap0/GoSQLX/pkg/sql/keywords" + "github.com/ajitpratap0/GoSQLX/pkg/sql/parser" + "github.com/ajitpratap0/GoSQLX/pkg/sql/tokenizer" +) + +// Before #557, Children() handed visitors heap copies of value-typed nodes, +// so these mutations were silently lost. They must now reach the AST. + +func mustParse(t *testing.T, sql string, dialect keywords.SQLDialect) *ast.AST { + t.Helper() + tree, err := parser.ParseWithDialect(sql, dialect) + if err != nil { + t.Fatalf("parse %q: %v", sql, err) + } + if len(tree.Statements) != 1 { + t.Fatalf("parse %q: got %d statements, want 1", sql, len(tree.Statements)) + } + return tree +} + +func mustSelect(t *testing.T, tree *ast.AST) *ast.SelectStatement { + t.Helper() + sel, ok := tree.Statements[0].(*ast.SelectStatement) + if !ok { + t.Fatalf("expected *ast.SelectStatement, got %T", tree.Statements[0]) + } + return sel +} + +func mustFunc(t *testing.T, e ast.Expression) *ast.FunctionCall { + t.Helper() + fn, ok := e.(*ast.FunctionCall) + if !ok { + t.Fatalf("expected *ast.FunctionCall, got %T", e) + } + return fn +} + +func TestWalk_VisitorMutationReachesAST(t *testing.T) { + t.Run("TableReference_and_OrderBy", func(t *testing.T) { + tree := mustParse(t, "SELECT a FROM t1 JOIN t2 ON t1.id = t2.id ORDER BY a DESC", keywords.DialectGeneric) + sel := mustSelect(t, tree) + ast.Inspect(sel, func(n ast.Node) bool { + switch x := n.(type) { + case *ast.TableReference: + if x.Name != "" && !strings.HasPrefix(x.Name, "x_") { + x.Name = "x_" + x.Name + } + case *ast.OrderByExpression: + x.Ascending = true + } + return true + }) + if got := sel.From[0].Name; got != "x_t1" { + t.Errorf("From[0].Name = %q, want x_t1", got) + } + if len(sel.Joins) != 1 { + t.Fatalf("got %d joins, want 1", len(sel.Joins)) + } + if got := sel.Joins[0].Right.Name; got != "x_t2" { + t.Errorf("Joins[0].Right.Name = %q, want x_t2", got) + } + if !sel.OrderBy[0].Ascending { + t.Errorf("OrderBy[0].Ascending was not flipped") + } + for _, out := range []string{sel.SQL(), formatter.FormatAST(tree, ast.FormatOptions{})} { + if !strings.Contains(out, "FROM x_t1") || !strings.Contains(out, "JOIN x_t2") { + t.Errorf("renamed tables missing from rendered SQL: %s", out) + } + if strings.Contains(out, "DESC") { + t.Errorf("rendered SQL still has DESC: %s", out) + } + } + }) + + t.Run("Window_Aggregate_WithinGroup_Frame", func(t *testing.T) { + tree := mustParse(t, "SELECT SUM(x) OVER (PARTITION BY g ORDER BY y DESC ROWS BETWEEN 1 PRECEDING AND CURRENT ROW), "+ + "STRING_AGG(n, ',' ORDER BY n DESC), PERCENTILE_CONT(0.5) WITHIN GROUP (ORDER BY v DESC) FROM t", keywords.DialectPostgreSQL) + sel := mustSelect(t, tree) + ast.Inspect(sel, func(n ast.Node) bool { + switch x := n.(type) { + case *ast.OrderByExpression: + x.Ascending = true + case *ast.WindowFrameBound: + if x.Type == "PRECEDING" { + x.Type = "FOLLOWING" + } + } + return true + }) + if len(sel.Columns) != 3 { + t.Fatalf("got %d columns, want 3", len(sel.Columns)) + } + sum, agg, pct := mustFunc(t, sel.Columns[0]), mustFunc(t, sel.Columns[1]), mustFunc(t, sel.Columns[2]) + if sum.Over == nil || len(sum.Over.OrderBy) != 1 || !sum.Over.OrderBy[0].Ascending { + t.Errorf("window ORDER BY was not flipped: %+v", sum.Over) + } + if sum.Over.FrameClause == nil || sum.Over.FrameClause.Start.Type != "FOLLOWING" { + t.Errorf("WindowFrame.Start mutation did not reach the AST: %+v", sum.Over.FrameClause) + } + if len(agg.OrderBy) != 1 || !agg.OrderBy[0].Ascending { + t.Errorf("aggregate ORDER BY was not flipped") + } + if len(pct.WithinGroup) != 1 || !pct.WithinGroup[0].Ascending { + t.Errorf("WITHIN GROUP ORDER BY was not flipped") + } + if out := sel.SQL(); strings.Contains(out, "DESC") || !strings.Contains(out, "FOLLOWING") { + t.Errorf("mutations not visible in SQL(): %s", out) + } + }) + + t.Run("WhenClause", func(t *testing.T) { + tree := mustParse(t, "SELECT CASE WHEN a = 1 THEN 'x' WHEN a = 2 THEN 'y' ELSE 'z' END FROM t", keywords.DialectGeneric) + sel := mustSelect(t, tree) + ast.Inspect(sel, func(n ast.Node) bool { + if w, ok := n.(*ast.WhenClause); ok { + w.Result = &ast.LiteralValue{Value: "changed", Type: "STRING"} + } + return true + }) + out := sel.SQL() + if strings.Count(out, "'changed'") != 2 || strings.Contains(out, "'x'") { + t.Errorf("WhenClause mutation not visible in SQL(): %s", out) + } + }) + + t.Run("UpdateExpression", func(t *testing.T) { + tree := mustParse(t, "UPDATE t SET a = 1", keywords.DialectGeneric) + upd, ok := tree.Statements[0].(*ast.UpdateStatement) + if !ok { + t.Fatalf("expected *ast.UpdateStatement, got %T", tree.Statements[0]) + } + repl := &ast.LiteralValue{Value: 42, Type: "INTEGER"} + ast.Inspect(upd, func(n ast.Node) bool { + if u, ok := n.(*ast.UpdateExpression); ok { + u.Value = repl + return false + } + return true + }) + if upd.Assignments[0].Value != ast.Expression(repl) { + t.Fatalf("UpdateExpression mutation did not reach the AST") + } + if out := upd.SQL(); !strings.Contains(out, "42") { + t.Errorf("UpdateExpression mutation not visible in SQL(): %s", out) + } + }) + + t.Run("MergeTargetSource", func(t *testing.T) { + tree := mustParse(t, "MERGE INTO tgt AS t USING src AS s ON t.id = s.id WHEN MATCHED THEN UPDATE SET t.v = s.v "+ + "WHEN NOT MATCHED THEN INSERT (id, v) VALUES (s.id, s.v);", keywords.DialectSQLServer) + m, ok := tree.Statements[0].(*ast.MergeStatement) + if !ok { + t.Fatalf("expected *ast.MergeStatement, got %T", tree.Statements[0]) + } + ast.Inspect(m, func(n ast.Node) bool { + if tr, ok := n.(*ast.TableReference); ok && tr.Name != "" { + tr.Name = "x_" + tr.Name + } + return true + }) + if m.TargetTable.Name != "x_tgt" || m.SourceTable.Name != "x_src" { + t.Fatalf("MERGE table rename did not reach the AST: target=%q source=%q", m.TargetTable.Name, m.SourceTable.Name) + } + if out := m.SQL(); !strings.Contains(out, "x_tgt") || !strings.Contains(out, "x_src") { + t.Errorf("MERGE rename not visible in SQL(): %s", out) + } + }) +} + +// TestInspect_ConcurrentPooledTrees walks pooled trees from several +// goroutines. Children now alias pooled storage, so this checks under -race +// that walks stay deterministic and never observe another tree's nodes. +func TestInspect_ConcurrentPooledTrees(t *testing.T) { + queries := []string{ + "SELECT a FROM t1 JOIN t2 ON t1.id = t2.id LEFT JOIN t3 ON t3.id = t2.id ORDER BY a DESC, b", + "SELECT SUM(x) OVER (PARTITION BY g ORDER BY y ROWS BETWEEN 1 PRECEDING AND CURRENT ROW) FROM t ORDER BY 1", + "SELECT CASE WHEN a = 1 THEN 'x' WHEN a = 2 THEN 'y' ELSE 'z' END FROM t JOIN u ON u.id = t.id", + } + type counts struct{ tables, orderBys int } + countTree := func(sql string) (counts, error) { + tkz := tokenizer.GetTokenizer() + defer tokenizer.PutTokenizer(tkz) + tokens, err := tkz.Tokenize([]byte(sql)) + if err != nil { + return counts{}, err + } + p := parser.GetParser() + defer parser.PutParser(p) + tree, err := p.ParseFromModelTokens(tokens) + if err != nil { + return counts{}, err + } + defer ast.ReleaseAST(tree) + var c counts + for _, stmt := range tree.Statements { + ast.Inspect(stmt, func(n ast.Node) bool { + switch n.(type) { + case *ast.TableReference: + c.tables++ + case *ast.OrderByExpression: + c.orderBys++ + } + return true + }) + } + return c, nil + } + + want := make([]counts, len(queries)) + for i, q := range queries { + c, err := countTree(q) + if err != nil { + t.Fatalf("query %d: %v", i, err) + } + if c.tables == 0 { + t.Fatalf("query %d: no table references visited", i) + } + want[i] = c + } + + const goroutines, iterations = 8, 200 + var wg sync.WaitGroup + errs := make(chan string, goroutines) + for g := 0; g < goroutines; g++ { + wg.Add(1) + go func(g int) { + defer wg.Done() + for i := 0; i < iterations; i++ { + qi := (g + i) % len(queries) + got, err := countTree(queries[qi]) + if err != nil { + errs <- err.Error() + return + } + if got != want[qi] { + errs <- "query " + queries[qi] + ": counts changed across pooled trees" + return + } + } + }(g) + } + wg.Wait() + close(errs) + for e := range errs { + t.Error(e) + } +} diff --git a/pkg/sql/ast/visitor_order_test.go b/pkg/sql/ast/visitor_order_test.go new file mode 100644 index 000000000..c9c95dc28 --- /dev/null +++ b/pkg/sql/ast/visitor_order_test.go @@ -0,0 +1,175 @@ +// Copyright 2026 GoSQLX Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package ast_test + +import ( + "flag" + "fmt" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/ajitpratap0/GoSQLX/pkg/sql/ast" + "github.com/ajitpratap0/GoSQLX/pkg/sql/keywords" + "github.com/ajitpratap0/GoSQLX/pkg/sql/parser" +) + +// updateVisitOrder regenerates testdata/visit_order.golden. The golden file +// was generated from the Children() implementation that predates #557, so it +// pins the visit order that the allocation work must preserve. +var updateVisitOrder = flag.Bool("update-visit-order", false, "rewrite testdata/visit_order.golden") + +const visitOrderGolden = "testdata/visit_order.golden" + +type visitOrderCase struct { + name string + dialect keywords.SQLDialect + sql string +} + +var visitOrderCorpus = []visitOrderCase{ + {"select_distinct_on_joins", keywords.DialectPostgreSQL, + "SELECT DISTINCT ON (a) a, b FROM t1 JOIN t2 ON t1.id = t2.id LEFT JOIN (SELECT id FROM t3) d ON d.id = t1.id WHERE a > 1 GROUP BY a, b HAVING COUNT(*) > 1 ORDER BY a DESC NULLS LAST LIMIT 5"}, + {"window_aggregate_order", keywords.DialectPostgreSQL, + "SELECT SUM(x) OVER (PARTITION BY g ORDER BY y ROWS BETWEEN 1 PRECEDING AND CURRENT ROW), STRING_AGG(n, ',' ORDER BY n DESC), PERCENTILE_CONT(0.5) WITHIN GROUP (ORDER BY v) FROM t WINDOW w AS (PARTITION BY g ORDER BY y)"}, + {"case_expressions", keywords.DialectGeneric, + "SELECT CASE WHEN a = 1 THEN 'x' WHEN a = 2 THEN 'y' ELSE 'z' END, CASE b WHEN 1 THEN 2 END FROM t"}, + {"cte_union", keywords.DialectGeneric, + "WITH c AS (SELECT 1 AS x) SELECT x FROM c UNION ALL SELECT 2"}, + {"update_returning", keywords.DialectGeneric, + "UPDATE t SET a = 1, b = b + 1 WHERE id = 2 RETURNING a"}, + {"delete_returning", keywords.DialectGeneric, + "DELETE FROM t WHERE id = 1 RETURNING id"}, + {"insert_on_conflict", keywords.DialectPostgreSQL, + "INSERT INTO t (a, b) VALUES (1, 2), (3, 4) ON CONFLICT (a) DO UPDATE SET b = EXCLUDED.b WHERE t.b < 10 RETURNING a"}, + {"insert_on_duplicate_key", keywords.DialectMySQL, + "INSERT INTO t (a, b) VALUES (1, 2) ON DUPLICATE KEY UPDATE b = VALUES(b), a = a + 1"}, + {"create_table", keywords.DialectGeneric, + "CREATE TABLE t (id INT PRIMARY KEY, name VARCHAR(10) NOT NULL DEFAULT 'x', CONSTRAINT ck CHECK (id > 0))"}, + {"create_index", keywords.DialectGeneric, + "CREATE INDEX idx ON t (a, b) WHERE a > 0"}, + {"merge", keywords.DialectSQLServer, + "MERGE INTO tgt AS t USING src AS s ON t.id = s.id WHEN MATCHED THEN UPDATE SET t.v = s.v WHEN NOT MATCHED THEN INSERT (id, v) VALUES (s.id, s.v);"}, + {"match_recognize", keywords.DialectSnowflake, + "SELECT * FROM t MATCH_RECOGNIZE (PARTITION BY s ORDER BY ts MEASURES MATCH_NUMBER() AS m ALL ROWS PER MATCH PATTERN (UP+ DOWN+) DEFINE UP AS price > PREV(price), DOWN AS price < PREV(price))"}, + {"alter_table", keywords.DialectGeneric, + "ALTER TABLE t ADD COLUMN c INT"}, +} + +func parseCorpus(t testing.TB, c visitOrderCase) *ast.AST { + t.Helper() + tree, err := parser.ParseWithDialect(c.sql, c.dialect) + if err != nil { + t.Fatalf("%s: parse %q (%s): %v", c.name, c.sql, c.dialect, err) + } + return tree +} + +// orderRecorder is a depth-tracking Visitor. Walk calls Visit(nil) once a +// node's children are done, which pops one level. +type orderRecorder struct { + b *strings.Builder + depth int +} + +func (r *orderRecorder) Visit(n ast.Node) (ast.Visitor, error) { + if n == nil { + r.depth-- + return nil, nil + } + fmt.Fprintf(r.b, "%s%T %q\n", strings.Repeat(" ", r.depth), n, n.TokenLiteral()) + r.depth++ + return r, nil +} + +func walkOrder(t testing.TB, tree *ast.AST) string { + t.Helper() + var b strings.Builder + for _, stmt := range tree.Statements { + r := &orderRecorder{b: &b} + if err := ast.Walk(r, stmt); err != nil { + t.Fatalf("walk: %v", err) + } + } + return b.String() +} + +// childrenOrder walks the tree with nothing but the exported Node.Children, +// the way third-party walkers do, using the same line format as walkOrder. +func childrenOrder(tree *ast.AST) string { + var b strings.Builder + var rec func(n ast.Node, depth int) + rec = func(n ast.Node, depth int) { + if n == nil { + return + } + fmt.Fprintf(&b, "%s%T %q\n", strings.Repeat(" ", depth), n, n.TokenLiteral()) + for _, c := range n.Children() { + rec(c, depth+1) + } + } + for _, stmt := range tree.Statements { + rec(stmt, 0) + } + return b.String() +} + +func TestWalk_VisitOrderGolden(t *testing.T) { + var b strings.Builder + for _, c := range visitOrderCorpus { + tree := parseCorpus(t, c) + fmt.Fprintf(&b, "== %s\n", c.name) + b.WriteString(walkOrder(t, tree)) + ast.ReleaseAST(tree) + } + got := b.String() + + path := filepath.FromSlash(visitOrderGolden) + if *updateVisitOrder { + if err := os.WriteFile(path, []byte(got), 0o600); err != nil { + t.Fatalf("write golden: %v", err) + } + return + } + want, err := os.ReadFile(path) + if err != nil { + t.Fatalf("read golden (regenerate with -update-visit-order): %v", err) + } + if got != string(want) { + gotLines := strings.Split(got, "\n") + wantLines := strings.Split(string(want), "\n") + for i := 0; i < len(gotLines) && i < len(wantLines); i++ { + if gotLines[i] != wantLines[i] { + t.Fatalf("visit order differs at line %d:\n got: %s\nwant: %s", i+1, gotLines[i], wantLines[i]) + } + } + t.Fatalf("visit order differs in length: got %d lines, want %d", len(gotLines), len(wantLines)) + } +} + +// TestWalk_MatchesChildrenRecursion guards against Walk's in-place fast path +// drifting from the exported Children() methods. +func TestWalk_MatchesChildrenRecursion(t *testing.T) { + for _, c := range visitOrderCorpus { + t.Run(c.name, func(t *testing.T) { + tree := parseCorpus(t, c) + defer ast.ReleaseAST(tree) + if w, r := walkOrder(t, tree), childrenOrder(tree); w != r { + t.Fatalf("Walk and Children() recursion disagree\nwalk:\n%s\nchildren:\n%s", w, r) + } + }) + } +} From 27854aef50a560aa6cf48fb74c42c0b6b83d8798 Mon Sep 17 00:00:00 2001 From: Ajit Pratap Singh Date: Tue, 13 Oct 2026 11:07:44 +0530 Subject: [PATCH 2/3] test(ast): tolerate CRLF checkouts in the visit-order golden comparison On the windows-latest CI runner git checks testdata/visit_order.golden out with CRLF line endings (core.autocrlf), so TestWalk_VisitOrderGolden failed with "got" and "want" lines that print identically. Normalize CRLF to LF when reading the golden; the test is about visit order, not line endings. Verified locally against a CRLF-converted copy of the file. Part of #557 Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01UZSk1292jPc8ipDboNxeve --- pkg/sql/ast/visitor_order_test.go | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/pkg/sql/ast/visitor_order_test.go b/pkg/sql/ast/visitor_order_test.go index c9c95dc28..1d42abe52 100644 --- a/pkg/sql/ast/visitor_order_test.go +++ b/pkg/sql/ast/visitor_order_test.go @@ -148,9 +148,12 @@ func TestWalk_VisitOrderGolden(t *testing.T) { if err != nil { t.Fatalf("read golden (regenerate with -update-visit-order): %v", err) } - if got != string(want) { + // Git on Windows may check the golden out with CRLF line endings + // (core.autocrlf); the comparison is about visit order, not line endings. + wantStr := strings.ReplaceAll(string(want), "\r\n", "\n") + if got != wantStr { gotLines := strings.Split(got, "\n") - wantLines := strings.Split(string(want), "\n") + wantLines := strings.Split(wantStr, "\n") for i := 0; i < len(gotLines) && i < len(wantLines); i++ { if gotLines[i] != wantLines[i] { t.Fatalf("visit order differs at line %d:\n got: %s\nwant: %s", i+1, gotLines[i], wantLines[i]) From aa8cbead1bcf05d6d52adbf9952f1c864d04e5e2 Mon Sep 17 00:00:00 2001 From: Ajit Pratap Singh Date: Tue, 13 Oct 2026 12:21:27 +0530 Subject: [PATCH 3/3] fix(ast): walk Insert/Delete/Update Table in place; guard child aliasing with a reflection test (#557 review follow-up) Pre-merge review found that Insert, Delete and Update still had value-receiver Children() methods returning &x.Table, a pointer into the receiver copy, so visitor mutations of Table during Walk/Inspect were silently lost. Give each a pointer-receiver childrenInPlace and route them through childrenOf, as already done for JoinClause, WindowFrame and MergeStatement. The exported value-receiver Children() is unchanged (on a value it still returns copies). TestChildrenOf_InPlaceFastPath covers the three new cases. Add TestWalk_ChildPointersAliasParentStorage, which reflectively fills every listed Node type and asserts each child pointer Walk yields lies inside the parent's storage. It reported exactly Insert/Delete/Update before this change and nothing after, and fails if a future value-receiver node returns &field outside childrenOf. Part of #557 Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01UZSk1292jPc8ipDboNxeve --- pkg/sql/ast/children_alias_test.go | 21 ++ pkg/sql/ast/dml.go | 24 ++- pkg/sql/ast/visitor.go | 8 +- pkg/sql/ast/visitor_alias_test.go | 313 +++++++++++++++++++++++++++++ 4 files changed, 362 insertions(+), 4 deletions(-) create mode 100644 pkg/sql/ast/visitor_alias_test.go diff --git a/pkg/sql/ast/children_alias_test.go b/pkg/sql/ast/children_alias_test.go index 6479ae546..1a5c46cdb 100644 --- a/pkg/sql/ast/children_alias_test.go +++ b/pkg/sql/ast/children_alias_test.go @@ -156,6 +156,23 @@ func TestChildrenOf_InPlaceFastPath(t *testing.T) { Output: []Expression{output}, } + where := &BinaryExpression{Left: ident("w"), Operator: "=", Right: ident("v")} + ret := ident("ret") + insert := &Insert{ + Table: TableReference{Name: "t"}, + Columns: []Expression{ident("c")}, + Values: [][]Expression{{ident("v1")}, {ident("v2")}}, + ReturningClause: []Expression{ret}, + } + del := &Delete{Table: TableReference{Name: "t"}, Where: where, ReturningClause: []Expression{ret}} + delNoWhere := &Delete{Table: TableReference{Name: "t"}} + upd := &Update{ + Table: TableReference{Name: "t"}, + Updates: []UpdateExpression{{Column: ident("a"), Value: ident("b")}, {Column: ident("c"), Value: ident("d")}}, + Where: where, + ReturningClause: []Expression{ret}, + } + cases := []struct { name string node Node @@ -168,6 +185,10 @@ func TestChildrenOf_InPlaceFastPath(t *testing.T) { {"WindowFrame", frame, frame.Children(), []Node{&frame.Start, end}, []int{1}}, {"WindowFrameNoEnd", frameNoEnd, frameNoEnd.Children(), []Node{&frameNoEnd.Start}, nil}, {"MergeStatement", merge, merge.Children(), []Node{&merge.TargetTable, &merge.SourceTable, cond, when, output}, []int{2, 3, 4}}, + {"Insert", insert, insert.Children(), []Node{&insert.Table, insert.Columns[0], insert.Values[0][0], insert.Values[1][0], ret}, []int{1, 2, 3, 4}}, + {"Delete", del, del.Children(), []Node{&del.Table, where, ret}, []int{1, 2}}, + {"DeleteNoWhere", delNoWhere, delNoWhere.Children(), []Node{&delNoWhere.Table}, nil}, + {"Update", upd, upd.Children(), []Node{&upd.Table, &upd.Updates[0], &upd.Updates[1], where, ret}, []int{3, 4}}, } for _, c := range cases { t.Run(c.name, func(t *testing.T) { diff --git a/pkg/sql/ast/dml.go b/pkg/sql/ast/dml.go index cbf1887b7..e75275b2d 100644 --- a/pkg/sql/ast/dml.go +++ b/pkg/sql/ast/dml.go @@ -66,7 +66,13 @@ func (i *Insert) statementNode() {} func (i Insert) TokenLiteral() string { return "INSERT" } // Children implements Node and returns all child nodes of this Insert statement. -func (i Insert) Children() []Node { +// Called on a value, Table is a pointer into a copy of the receiver; Walk and +// Inspect use childrenInPlace and visit the stored Table in place (#557). +func (i Insert) Children() []Node { return i.childrenInPlace() } + +// childrenInPlace is Children for an Insert reached through a pointer: Table +// is returned as a pointer into *i itself. +func (i *Insert) childrenInPlace() []Node { children := make([]Node, 0) children = append(children, &i.Table) children = append(children, nodifyExpressions(i.Columns)...) @@ -90,7 +96,13 @@ func (d *Delete) statementNode() {} func (d Delete) TokenLiteral() string { return "DELETE" } // Children implements Node and returns all child nodes of this Delete statement. -func (d Delete) Children() []Node { +// Called on a value, Table is a pointer into a copy of the receiver; Walk and +// Inspect use childrenInPlace and visit the stored Table in place (#557). +func (d Delete) Children() []Node { return d.childrenInPlace() } + +// childrenInPlace is Children for a Delete reached through a pointer: Table +// is returned as a pointer into *d itself. +func (d *Delete) childrenInPlace() []Node { children := make([]Node, 0) children = append(children, &d.Table) if d.Where != nil { @@ -114,7 +126,13 @@ func (u *Update) statementNode() {} func (u Update) TokenLiteral() string { return "UPDATE" } // Children implements Node and returns all child nodes of this Update statement. -func (u Update) Children() []Node { +// Called on a value, Table is a pointer into a copy of the receiver; Walk and +// Inspect use childrenInPlace and visit the stored Table in place (#557). +func (u Update) Children() []Node { return u.childrenInPlace() } + +// childrenInPlace is Children for an Update reached through a pointer: Table +// and the Updates elements are returned as pointers into *u itself. +func (u *Update) childrenInPlace() []Node { children := make([]Node, 0) children = append(children, &u.Table) for i := range u.Updates { diff --git a/pkg/sql/ast/visitor.go b/pkg/sql/ast/visitor.go index e118d078c..0cdf14dd8 100644 --- a/pkg/sql/ast/visitor.go +++ b/pkg/sql/ast/visitor.go @@ -195,7 +195,7 @@ func Walk(v Visitor, node Node) error { // childrenOf returns n's children for Walk. For value-receiver nodes whose // Children() must address their own fields (JoinClause, WindowFrame, -// MergeStatement) it uses the pointer-receiver childrenInPlace so the stored +// MergeStatement, Insert, Delete, Update) it uses the pointer-receiver childrenInPlace so the stored // struct is neither heap-copied nor detached from the AST (#557). A concrete // type switch is used because it is a cheap type-word compare. func childrenOf(n Node) []Node { @@ -206,6 +206,12 @@ func childrenOf(n Node) []Node { return t.childrenInPlace() case *MergeStatement: return t.childrenInPlace() + case *Insert: + return t.childrenInPlace() + case *Delete: + return t.childrenInPlace() + case *Update: + return t.childrenInPlace() } return n.Children() } diff --git a/pkg/sql/ast/visitor_alias_test.go b/pkg/sql/ast/visitor_alias_test.go new file mode 100644 index 000000000..60afa244c --- /dev/null +++ b/pkg/sql/ast/visitor_alias_test.go @@ -0,0 +1,313 @@ +package ast + +import ( + "fmt" + "reflect" + "sort" + "testing" +) + +// This file guards the aliasing contract documented on Walk: every child a +// visitor receives must point into the parent's own storage (a field, or the +// backing array of a slice field), never into a heap copy. A value-receiver +// Children() that returns &field points into the receiver COPY, so visitor +// mutations would be silently lost. Such nodes must expose a pointer-receiver +// childrenInPlace and be routed through childrenOf (see JoinClause, +// WindowFrame, MergeStatement, Insert, Delete, Update). +// +// If TestWalk_ChildPointersAliasParentStorage fails for a node, do NOT relax +// the test: add a childrenInPlace method for that type and a case for it in +// childrenOf (visitor.go), then extend TestChildrenOf_InPlaceFastPath. + +// aliasCheckTypes lists the Node types exercised by the aliasing test. Add new +// Node types that implement Children() here. +var aliasCheckTypes = []any{ + (*AliasedExpression)(nil), + (*AllExpression)(nil), + (*AlterColumnOperation)(nil), + (*AlterConnectorOperation)(nil), + (*AlterPolicyOperation)(nil), + (*AlterRoleOperation)(nil), + (*AlterSequenceStatement)(nil), + (*AlterStatement)(nil), + (*AlterTableAction)(nil), + (*AlterTableOperation)(nil), + (*AlterTableStatement)(nil), + (*AnyExpression)(nil), + (*ArrayConstructorExpression)(nil), + (*ArraySliceExpression)(nil), + (*ArraySubscriptExpression)(nil), + (*AST)(nil), + (*BetweenExpression)(nil), + (*BinaryExpression)(nil), + (*CaseExpression)(nil), + (*CastExpression)(nil), + (*ClusteredBy)(nil), + (*ColumnConstraint)(nil), + (*ColumnDef)(nil), + (*CommentDef)(nil), + (*CommonTableExpr)(nil), + (*ConnectByClause)(nil), + (*CreateIndexStatement)(nil), + (*CreateMaterializedViewStatement)(nil), + (*CreateSequenceStatement)(nil), + (*CreateTable)(nil), + (*CreateTableStatement)(nil), + (*CreateViewStatement)(nil), + (*CubeExpression)(nil), + (*Delete)(nil), + (*DeleteStatement)(nil), + (*DescribeStatement)(nil), + (*DropSequenceStatement)(nil), + (*DropStatement)(nil), + (*ExistsExpression)(nil), + (*ExtractExpression)(nil), + (*FetchClause)(nil), + (*ForClause)(nil), + (*ForSystemTimeClause)(nil), + (*FunctionCall)(nil), + (*FunctionDesc)(nil), + (*GroupingSetsExpression)(nil), + (*Ident)(nil), + (*Identifier)(nil), + (*IndexColumn)(nil), + (*InExpression)(nil), + (*Insert)(nil), + (*InsertStatement)(nil), + (*IntervalExpression)(nil), + (*JoinClause)(nil), + (*ListExpression)(nil), + (*LiteralValue)(nil), + (*MatchRecognizeClause)(nil), + (*MergeAction)(nil), + (*MergeStatement)(nil), + (*MergeWhenClause)(nil), + (*NamedArgument)(nil), + (*ObjectName)(nil), + (*OnConflict)(nil), + (*OrderByExpression)(nil), + (*ParenthesizedExpression)(nil), + (*PartitionBy)(nil), + (*PartitionDefinition)(nil), + (*PeriodDefinition)(nil), + (*PivotClause)(nil), + (*PositionExpression)(nil), + (*PragmaStatement)(nil), + (*Query)(nil), + (*ReferenceDefinition)(nil), + (*RefreshMaterializedViewStatement)(nil), + (*ReplaceStatement)(nil), + (*RollupExpression)(nil), + (*RowAccessPolicy)(nil), + (*SampleClause)(nil), + (*Select)(nil), + (*SelectStatement)(nil), + (*SetClause)(nil), + (*SetOperation)(nil), + (*ShowStatement)(nil), + (*StatementImpl)(nil), + (*SubqueryExpression)(nil), + (*SubstringExpression)(nil), + (*TableConstraint)(nil), + (*TableOption)(nil), + (*TableReference)(nil), + (*TimeTravelClause)(nil), + (*TopClause)(nil), + (*TriggerEvent)(nil), + (*TriggerExecBody)(nil), + (*TriggerObject)(nil), + (*TriggerPeriod)(nil), + (*TriggerReferencing)(nil), + (*TruncateStatement)(nil), + (*TupleExpression)(nil), + (*UnaryExpression)(nil), + (*UnpivotClause)(nil), + (*UnsupportedStatement)(nil), + (*Update)(nil), + (*UpdateExpression)(nil), + (*UpdateStatement)(nil), + (*UpsertClause)(nil), + (*Value)(nil), + (*Values)(nil), + (*VariantPath)(nil), + (*WhenClause)(nil), + (*WindowFrame)(nil), + (*WindowFrameBound)(nil), + (*WindowSpec)(nil), + (*WithClause)(nil), + (*WithFillClause)(nil), +} + +// aliasFillCandidates are concrete values used to populate interface-typed +// fields (Expression, Statement, Node, ...) with distinct, non-nil children. +var aliasFillCandidates = []any{&Identifier{Name: "x"}, &SelectStatement{}, &WindowSpec{}, &WithClause{}, &TableReference{}} + +// aliasFill populates exported fields reflectively so Children() has +// something to return for every field kind. +func aliasFill(v reflect.Value, depth int) { + switch v.Kind() { + case reflect.String: + v.SetString("x") + case reflect.Bool: + v.SetBool(true) + case reflect.Interface: + for _, c := range aliasFillCandidates { + cv := reflect.ValueOf(c) + if cv.Type().Implements(v.Type()) { + nv := reflect.New(cv.Type().Elem()) // fresh instance each time + if id, ok := nv.Interface().(*Identifier); ok { + id.Name = "x" + } + v.Set(nv) + return + } + } + case reflect.Ptr: + if depth > 2 || v.Type().Elem().Kind() != reflect.Struct { + return + } + nv := reflect.New(v.Type().Elem()) + aliasFill(nv.Elem(), depth+1) + v.Set(nv) + case reflect.Slice: + if depth > 3 { + return + } + s := reflect.MakeSlice(v.Type(), 2, 2) + for i := 0; i < 2; i++ { + aliasFill(s.Index(i), depth+1) + } + v.Set(s) + case reflect.Struct: + if depth > 4 { + return + } + for i := 0; i < v.NumField(); i++ { + if v.Type().Field(i).IsExported() && v.Field(i).CanSet() { + aliasFill(v.Field(i), depth+1) + } + } + } +} + +type aliasRange struct{ lo, hi uintptr } + +// aliasCollect records the storage ranges of slice backing arrays reachable +// from v and the pointers it holds. A child that is one of those pointers, or +// lies inside one of those ranges, is owned by the parent. +func aliasCollect(v reflect.Value, ranges *[]aliasRange, ptrs map[uintptr]bool) { + switch v.Kind() { + case reflect.Ptr: + if !v.IsNil() { + ptrs[v.Pointer()] = true + } + case reflect.Interface: + if !v.IsNil() { + if e := v.Elem(); e.Kind() == reflect.Ptr { + ptrs[e.Pointer()] = true + } + } + case reflect.Slice: + if v.Len() > 0 { + lo := v.Pointer() + *ranges = append(*ranges, aliasRange{lo, lo + uintptr(v.Len())*v.Type().Elem().Size()}) + for i := 0; i < v.Len(); i++ { + aliasCollect(v.Index(i), ranges, ptrs) + } + } + case reflect.Struct: + for i := 0; i < v.NumField(); i++ { + aliasCollect(v.Field(i), ranges, ptrs) + } + } +} + +// aliasLeaves is a Visitor that records the direct children of root without +// descending into them. +type aliasLeaves struct { + root Node + got []Node +} + +func (d *aliasLeaves) Visit(n Node) (Visitor, error) { + if n == nil { + return nil, nil + } + if n == d.root { + return d, nil + } + d.got = append(d.got, n) + return nil, nil +} + +// aliasDetached returns the types of kids that do not alias parent's storage. +func aliasDetached(parent reflect.Value, kids []Node) (detached []string) { + var ranges []aliasRange + ptrs := map[uintptr]bool{} + el := parent.Elem() + lo := parent.Pointer() + ranges = append(ranges, aliasRange{lo, lo + el.Type().Size()}) + aliasCollect(el, &ranges, ptrs) + for _, k := range kids { + kv := reflect.ValueOf(k) + if kv.Kind() != reflect.Ptr { + // A node stored by value in the interface is a snapshot the + // visitor cannot mutate through; that is a different (known) + // limitation, not pointer aliasing into a receiver copy. + continue + } + p := kv.Pointer() + ok := ptrs[p] + for _, r := range ranges { + if p >= r.lo && p < r.hi { + ok = true + } + } + if !ok { + detached = append(detached, fmt.Sprintf("%T", k)) + } + } + return detached +} + +// TestWalk_ChildPointersAliasParentStorage fills every listed Node type and +// asserts that each child pointer Walk yields lies inside the parent's own +// storage. Calling the exported Children() on a value is documented to return +// copies, so that path is not checked; only Walk's path is. +func TestWalk_ChildPointersAliasParentStorage(t *testing.T) { + var failures []string + for _, x := range aliasCheckTypes { + pt := reflect.TypeOf(x) + if pt.Elem().Kind() != reflect.Struct { + continue + } + pv := reflect.New(pt.Elem()) + aliasFill(pv.Elem(), 0) + node, ok := pv.Interface().(Node) + if !ok { + continue + } + d := &aliasLeaves{root: node} + func() { + defer func() { + if r := recover(); r != nil { + failures = append(failures, fmt.Sprintf("%s: panic %v", pt.Elem().Name(), r)) + } + }() + if err := Walk(d, node); err != nil { + failures = append(failures, fmt.Sprintf("%s: Walk error %v", pt.Elem().Name(), err)) + return + } + if det := aliasDetached(pv, d.got); len(det) > 0 { + failures = append(failures, fmt.Sprintf("%s: Walk yields children outside the parent's storage: %v", pt.Elem().Name(), det)) + } + }() + } + sort.Strings(failures) + for _, f := range failures { + t.Error(f) + } + if len(failures) > 0 { + t.Log("a value-receiver Children() returning &field points into the receiver copy; add a childrenInPlace method and route the type through childrenOf (visitor.go)") + } +}