Skip to content

Commit dc44163

Browse files
ajitpratap0ajitpratap0
authored
fix: architect-review sprint 2 — all 5 sprints in one PR (#518)
1 parent d5a5fff commit dc44163

31 files changed

Lines changed: 6486 additions & 3918 deletions

‎README.md‎

Lines changed: 29 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -41,8 +41,12 @@
4141
GoSQLX is a **production-ready SQL parsing SDK** for Go. It tokenizes, parses, and generates ASTs from SQL with zero-copy optimizations and intelligent object pooling - handling **1.38M+ operations per second** with sub-microsecond latency.
4242

4343
```go
44-
ast, _ := gosqlx.Parse("SELECT u.name, COUNT(*) FROM users u JOIN orders o ON u.id = o.user_id GROUP BY u.name")
45-
// → Full AST with statements, columns, joins, grouping - ready for analysis, transformation, or formatting
44+
// v1.15+ recommended entry point: ParseTree returns an opaque Tree,
45+
// so you don't need to import pkg/sql/ast just to get started.
46+
tree, _ := gosqlx.ParseTree(ctx, "SELECT u.name, COUNT(*) FROM users u JOIN orders o ON u.id = o.user_id GROUP BY u.name",
47+
gosqlx.WithDialect("postgresql"))
48+
fmt.Println("Tables:", tree.Tables())
49+
fmt.Println(tree.Format(gosqlx.WithIndent(2), gosqlx.WithUppercaseKeywords(true)))
4650
```
4751

4852
### Why GoSQLX?
@@ -69,23 +73,30 @@ import (
6973
)
7074

7175
func main() {
72-
// Parse any SQL dialect
73-
ast, _ := gosqlx.Parse("SELECT * FROM users WHERE active = true")
74-
fmt.Printf("%d statement(s)\n", len(ast.Statements))
75-
76-
// Format messy SQL
77-
clean, _ := gosqlx.Format("select id,name from users where id=1", gosqlx.DefaultFormatOptions())
78-
fmt.Println(clean)
79-
// SELECT
80-
// id,
81-
// name
82-
// FROM users
83-
// WHERE id = 1
84-
85-
// Catch errors before production
86-
if err := gosqlx.Validate("SELECT * FROM"); err != nil {
87-
fmt.Println(err) // → expected table name
76+
ctx := context.Background()
77+
78+
// ParseTree (v1.15+) is the recommended entry point. It returns an
79+
// opaque handle with built-in helpers — no need to import pkg/sql/ast.
80+
tree, err := gosqlx.ParseTree(ctx, "SELECT id, name FROM users WHERE active = true",
81+
gosqlx.WithDialect("postgresql"))
82+
if err != nil {
83+
// Sentinel errors work with errors.Is
84+
if errors.Is(err, gosqlx.ErrSyntax) {
85+
log.Fatalf("syntax error: %v", err)
86+
}
87+
log.Fatal(err)
8888
}
89+
fmt.Println("Tables:", tree.Tables())
90+
fmt.Println(tree.Format(gosqlx.WithIndent(2), gosqlx.WithUppercaseKeywords(true)))
91+
92+
// Walk the AST — typed walkers avoid the type-assertion dance:
93+
tree.WalkSelects(func(s *ast.SelectStatement) bool {
94+
fmt.Printf(" SELECT with %d columns\n", len(s.Columns))
95+
return true
96+
})
97+
98+
// The legacy Parse/Format/Validate API still works for v1.x code.
99+
// See docs/MIGRATION.md for the Tree migration guide.
89100
}
90101
```
91102

‎docs/ARCHITECT_REVIEW_2026-04-21.md‎

Lines changed: 262 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,262 @@
1+
# GoSQLX Architect Review (Round 2) — 2026-04-21
2+
3+
Second-round review after PR #517 (merged 2026-04-21). Five parallel architect agents did a fresh pass across core parsing, foundation, public API, advanced features, and cross-cutting infra. This is a **delta document**: what landed cleanly, what slipped through, what's newly surfaced.
4+
5+
Previous review: `docs/ARCHITECT_REVIEW_2026-04-16.md`.
6+
7+
---
8+
9+
## Executive Summary
10+
11+
PR #517 **delivered all 11 claimed fixes** to a solid baseline. But:
12+
13+
1. **One class-C1 defect was missed**: subquery leaks in `PutExpression` (see §1).
14+
2. **The new infrastructure is unused**: `pkg/sql/dialect.Capabilities` has zero production callers; legacy `Parse` doesn't wrap errors in the new sentinels; README still shows the old Parse pattern. You built the engine but left the signage.
15+
3. **Structural debt grew**: `ast.go` is +136 lines, `pool.go` +500 lines, `gosqlx.go` is now a 694-line candidate for the next god-file. 6 of 9 pre-v2.0 items are still open.
16+
4. **CI health is operationally fragile**: two follow-up commits (timeout bump, staticcheck fix) after #517 are symptoms of an un-sharded race suite.
17+
18+
Net assessment: **PR #517 was additive, not transformative**. The "strangler fig" pattern needs actual strangling to follow through.
19+
20+
---
21+
22+
## 1. Critical Miss: Subquery Leaks in `PutExpression`
23+
24+
**Defect**: `pkg/sql/ast/pool.go` lines 1344-1455 — every expression type that embeds a `*SelectStatement` subquery sets `e.Subquery = nil` without releasing the statement first.
25+
26+
Affected paths:
27+
- `InExpression` (line 1344-1358)
28+
- `SubqueryExpression` (line 1360-1362)
29+
- `ExistsExpression` (line 1404-1406)
30+
- `AnyExpression` (line 1408-1415)
31+
- `AllExpression` (line 1417-1424)
32+
- `ArrayConstructorExpression.Subquery` (line 1446-1455)
33+
34+
Every `WHERE id IN (SELECT ...)`, `EXISTS (SELECT ...)`, `col = ANY(SELECT ...)` leaks the full inner SelectStatement including Columns, From, Where, Joins, Windows. Same defect class C1/C2 fixed, just at expression level. `pool_leak_test.go` doesn't cover these constructs — that's why round-1 tests didn't catch it.
35+
36+
**Fix**: call `releaseStatement(e.Subquery)` before `e.Subquery = nil` in all six sites. Add test: parse `SELECT x FROM t WHERE id IN (SELECT y FROM u WHERE z = 1)` 1000 times, assert stable heap.
37+
38+
**Severity**: CRITICAL — same production perf claim at stake.
39+
40+
---
41+
42+
## 2. Fix Quality — What Landed Well
43+
44+
| Item | Status | Notes |
45+
|------|--------|-------|
46+
| C1/C2 statement-level leaks | Solid | `releaseTableReference` helper factoring is right; no double-release risk |
47+
| C3 metrics DoS | Solid | Lock-free atomic buckets; 4 allocs/op `GetStats` proven |
48+
| C5 linter `ast.Walk` migration | Solid | Consistent pattern; L029 latent bug found & fixed in flight |
49+
| C6 Children() coverage | Partial | Test has structural gap (see §3) |
50+
| H6 errors immutability | Solid | All callers audited; return-value pattern universal |
51+
| H7 structured parser errors | Solid | Zero `fmt.Errorf` remaining in parser |
52+
| H8 config loader | Solid | Schema complete; walk-up safe; `**` glob properly implemented |
53+
| H9 LSP type switch | Solid | 15 statement kinds; default fallback correct |
54+
| H10 LSP real ranges | Solid | Semicolon fallback only hits rare DDL-no-Pos case |
55+
| H11 keyword conflicts | Solid | `keywordsEquivalent` is the right semantic gate; resolutions preserve runtime |
56+
57+
Eight of eleven fixes are production-quality. Three have material issues addressed below.
58+
59+
---
60+
61+
## 3. Issues in the Fixes Themselves
62+
63+
### 3.1 `children_coverage_test.go` has a structural gap
64+
65+
`pkg/sql/ast/children_coverage_test.go:286-288` explicitly admits: "concrete-typed fields like `*CommonTableExpr` cannot accept a bare sentinel." But the AST references concrete pointer types throughout (`*WithClause`, `*TopClause`, `*MergeWhenClause`, `*OnConflict`, `*ConnectByClause`, `*SampleClause`). **The test only exercises interface-typed fields.** Plus `childrenCoverageAllowlist` (line 233) is declared but **never consulted** — it's misleading documentation.
66+
67+
**Fix**: (a) generate zero-value concrete mocks via reflection for each concrete pointer type, or (b) register a per-type fixture table. And either wire the allowlist into the test skip logic or delete it.
68+
69+
### 3.2 `releaseStatement` dispatch missing cases
70+
71+
`pkg/sql/ast/pool.go:541` — no case for statement types defined in `dml.go` (`*Select`, `*Insert`, `*Update`, `*Delete` — the legacy duplicates), nor for `*PragmaStatement` wrapper variants. If the parser ever stores these in `AST.Statements`, `ReleaseAST` silently drops without returning to pool. Low severity because they may never be pooled today, but the dispatch should mirror pool declarations.
72+
73+
### 3.3 `putExpressionImpl` allocates its work queue
74+
75+
`pool.go:1257` — `workQueue := make([]Expression, 0, 32)` runs **on every call**. In the hot path, that's one heap alloc per `PutExpression`, called transitively 10-100× per parse. Pool the work queue itself via `sync.Pool`, or use a fixed-size stack array with spillover.
76+
77+
### 3.4 `metrics.errorsMutex` is vestigial
78+
79+
`pkg/metrics/metrics.go:408` — retained "to serialize rare reset paths" but `reset()` uses atomic stores that already race-safe against concurrent increments. The mutex protects nothing. Delete.
80+
81+
### 3.5 `ErrorsByType` wire format change is undocumented breaking change
82+
83+
Round-1 fix changed map keys from `err.Error()` strings to `ErrorCode` strings. Grafana/dashboard users bound to the old keys are silently broken. Needs a CHANGELOG note and ideally a deprecation window with dual-emission.
84+
85+
---
86+
87+
## 4. "Two Models Colliding" — The Core Observation
88+
89+
Round 1 introduced new infrastructure that hasn't been adopted:
90+
91+
### 4.1 `dialect.Capabilities` has **zero production callers**
92+
93+
Grep confirms: `p.Capabilities()` / `p.DialectTyped()` / `p.IsSnowflake()` etc. used nowhere in parser production code. Meanwhile `p.dialect` string is referenced **88 times** (up from 72 at round 1) across 17 files. The old pattern is growing; the new one is read-only.
94+
95+
Parser now has three representations coexisting:
96+
1. `p.dialect string` — actual storage, 88 reads
97+
2. `keywords.DialectXxx` string constants — cast as `string(keywords.DialectOracle)` in 60+ places
98+
3. `dialect.Dialect` typed — zero production usage
99+
100+
**Recommendation**: Stop doing big-bang migrations. Cache `p.dialectTyped dialect.Dialect` in the Parser struct populated by `WithDialect`. Migrate 3-5 pure-capability sites per release (QUALIFY, PREWHERE, ARRAY JOIN, ILIKE, BRACKET_QUOTING). Add a CI grep gate: new production code may not introduce `p.dialect ==`; must use `p.Is<Dialect>()` or `p.Capabilities()`. Without this gate, v2.0 arrives with more, not fewer, string comparisons.
101+
102+
### 4.2 Sentinel errors only work on new API
103+
104+
`gosqlx.ErrSyntax`, `ErrTokenize`, `ErrTimeout`, `ErrUnsupportedDialect` work for `ParseTree` / `ParseReader`. Legacy `Parse` / `ParseWithContext` / `ParseWithDialect` still return `fmt.Errorf("tokenization failed: %w", err)` without the sentinel. So:
105+
106+
```go
107+
ast, err := gosqlx.Parse(sql)
108+
if errors.Is(err, gosqlx.ErrSyntax) { ... } // NEVER MATCHES
109+
```
110+
111+
**Fix**: 4-line retrofit per legacy function, purely additive to the error chain. Unifies the story.
112+
113+
### 4.3 README & package doc still promote legacy
114+
115+
`README.md` lines 63-90: canonical example is still the old Parse + type-assertion pattern. Zero mentions of `ParseTree`, `Tree`, `WithDialect`, `ErrSyntax`, `FormatTree`. Package `doc.go` lists legacy functions as "primary entry points" — Tree isn't mentioned at all.
116+
117+
This is the **single largest DX leak remaining**. Round-1's criticism ("new users reach for Parse and type-assert") is still architecturally true on the surface.
118+
119+
### 4.4 No compat tests for new Tree API
120+
121+
`pkg/compatibility/api_stability_test.go` covers only legacy surface (ast.Node, pools, token types). The Tree API is unprotected. If someone refactors `ParseTree` to drop `ctx` or change `Option` to a struct, compat suite won't flag it.
122+
123+
---
124+
125+
## 5. Tree API Completeness Gaps
126+
127+
Round-1 added Tree; usability audit surfaces what's missing for real workflows:
128+
129+
1. **No typed walkers** — `WalkSelects(func(*ast.SelectStatement) bool)`, `WalkExpressions(...)`. Users still write `if stmt, ok := n.(*ast.SelectStatement); ok` dance. sqlparser-rs and vitess both ship typed visitors.
130+
2. **No `Rewrite(pre, post)`** — closes parity gap with vitess.
131+
3. **No `Tree.Clone()`** for copy-on-write experiments.
132+
4. **No `Tree.Subqueries() []*Tree` / `Tree.CTEs()`** — common SQL analysis need.
133+
5. **`Release()` is a documented no-op** — aspirational for future pooling, but creates a training hazard today.
134+
6. **Tree carries full source string** — 1MB SQL doubles memory.
135+
136+
Fixing (1) and (2) takes the Tree from "viable" to "competitive."
137+
138+
---
139+
140+
## 6. `ParseReader` Pitfalls
141+
142+
### 6.1 No bounded read
143+
`pkg/gosqlx/reader.go:67` — `io.ReadAll(r)` unconditionally. A 100MB SQL dump allocates 200MB (ReadAll + string conversion). Real-world exposures: migration files, data dumps, HTTP POST bodies. Add `WithMaxBytes(n)` + `ErrTooLarge` sentinel.
144+
145+
### 6.2 `ParseReaderMultiple` splitter is not dialect-aware
146+
Correctly handles: single quotes, double-quoted identifiers, line comments, block comments.
147+
Misses:
148+
- PostgreSQL dollar-quoted strings (`$$...;...$$`) — will split on inner `;`, producing garbage
149+
- MySQL/ClickHouse backtick identifiers containing `;`
150+
- SQL Server bracketed identifiers containing `;`
151+
- PostgreSQL E-string backslash escapes (`E'\''`)
152+
- PostgreSQL nested block comments (`/* /* nested */ */`)
153+
154+
For a library advertising 8 dialects this is a shipping hole. Expose `SplitStatements(sql, dialect)` with dialect-aware handling.
155+
156+
---
157+
158+
## 7. Structural Debt — Getting Worse
159+
160+
Line counts after PR #517:
161+
162+
| File | Before | After | Δ |
163+
|------|--------|-------|---|
164+
| pkg/sql/ast/ast.go | 2327 | **2463** | +136 |
165+
| pkg/sql/ast/pool.go | ~1500 | **2030** | +500 |
166+
| pkg/sql/ast/sql.go | 1853 | 1853 | 0 |
167+
| pkg/sql/tokenizer/tokenizer.go | 1842 | 1842 | 0 |
168+
| pkg/sql/parser/parser.go | 1186 | 1195 | +9 |
169+
| pkg/gosqlx/gosqlx.go | — | **694** | new |
170+
171+
Every core file exceeds the 400-line ceiling in the project's own `coding-style.md` by 3-6×. PR #517 **added** to ast.go and pool.go rather than splitting. `gosqlx.go` is trending toward god-file status.
172+
173+
**Natural split seams**:
174+
- `ast.go` → `ast_statements.go` + `ast_expressions.go` + `ast_literals.go` + `ast_clauses.go`
175+
- `pool.go` → `pool.go` (declarations) + `pool_statement_release.go` + `pool_expression_release.go`
176+
- `sql.go` (String()/SQL() serializer) along the same node-category axis
177+
178+
Do this before v2.0 breaking changes — refactoring 4KLOC while also breaking APIs is a merge-conflict nightmare.
179+
180+
---
181+
182+
## 8. CI Health Symptoms
183+
184+
PR #517 follow-ups (e0f0992 `increase race detector timeout to 120s`, c01edeb `resolve staticcheck and race detector failures`) are patches on a bigger problem:
185+
186+
- `task test:race` runs the entire tree under `-race` with 3-5× overhead. No sharding.
187+
- `pool_leak_test.go` uses `runtime.GC()` + heap measurement — will be flaky on shared runners. Gate behind `-short` or build tag.
188+
- Task install not cached across jobs — each of 4 race/cbinding jobs reinstalls it.
189+
- `perf-regression` still `continue-on-error: true` with 60-65% tolerance — decorative.
190+
191+
Expect another timeout bump within 1-2 sprints unless split into `test:race:fast` + `test:race:integration`.
192+
193+
---
194+
195+
## 9. Pre-v2.0 Punch-List Status
196+
197+
| # | Item | Round 1 | Round 2 |
198+
|---|------|---------|---------|
199+
| 1 | God-file splits | Open | **Worse** (+136L) |
200+
| 2 | ConversionResult.PositionMapping removal | Open | Open (still `Deprecated`) |
201+
| 3 | Merge/delete pkg/sql/token | Open | Open (still imported 18× ) |
202+
| 4 | Move non-API packages to internal/ | Open | Open (no `internal/` at root) |
203+
| 5 | DialectRegistry replacing keywords switch | Open | Open |
204+
| 6 | gosqlx.Tree opaque wrapper | Open | **Done** |
205+
| 7 | Functional options | Open | **Done** |
206+
| 8 | Structured errors in parser | Open | **Done (H7)** |
207+
| 9 | Logger interface injection | Open | Open (fmt.Println in 41 files) |
208+
209+
**Progress: 3 of 9 complete. 1 worse. 5 unchanged.**
210+
211+
---
212+
213+
## 10. Recommended v1.16 Sprint Plan
214+
215+
Ordered by leverage:
216+
217+
**Sprint A — "Fix the misses" (3-4 days)**
218+
1. Subquery leak in `PutExpression` (§1) — critical
219+
2. `Children()` coverage test gap (§3.1)
220+
3. `releaseStatement` dispatch completeness (§3.2)
221+
4. Retrofit legacy error wrappers with sentinels (§4.2)
222+
5. Delete vestigial `metrics.errorsMutex` (§3.4)
223+
224+
**Sprint B — "Close the narrative" (3-4 days)**
225+
6. README rewrite leading with `ParseTree` example
226+
7. `doc.go` rewrite promoting Tree as primary entry
227+
8. `docs/MIGRATION.md` Tree migration section + deprecation timeline
228+
9. Add Tree API to `pkg/compatibility/api_stability_test.go`
229+
230+
**Sprint C — "Tree competitive" (1 week)**
231+
10. Typed walkers (`WalkSelects`, `WalkExpressions`, generics-based)
232+
11. `Tree.Rewrite(pre, post)` for transformation
233+
12. `Tree.Clone()` for COW workflows
234+
13. Dialect-aware `SplitStatements` (dollar-quoting, backticks, brackets)
235+
14. `WithMaxBytes` + `ErrTooLarge` on ParseReader
236+
237+
**Sprint D — "Begin the strangling" (1 week)**
238+
15. Cache `p.dialectTyped` field in Parser
239+
16. Migrate 5 pure-capability sites to `p.Capabilities()` (QUALIFY, PREWHERE, ARRAY JOIN, ILIKE, BRACKET_QUOTING)
240+
17. Add CI grep gate forbidding new `p.dialect ==` in production code
241+
18. Allocate `workQueue` from pool in `putExpressionImpl` (§3.3)
242+
243+
**Sprint E — "Structural debt" (1-2 weeks)**
244+
19. Split `ast.go` by node category
245+
20. Split `pool.go` by responsibility
246+
21. Shard `task test:race` into fast + integration
247+
22. `tools/tools.go` for dev-tool pinning
248+
23. Delete `examples/cmd/cmd` committed binary
249+
250+
Sprints A+B are 1 week. Add C and you have v1.16 with a credible adoption story. D+E belong in v1.17 or a dedicated structural PR.
251+
252+
---
253+
254+
## 11. Net Assessment
255+
256+
**Trend**: net-better on DX surface (Tree, options, sentinels), net-worse on structure (god files grew, 2-model coexistence).
257+
258+
**What PR #517 really was**: a correctness & API-expansion PR. It wasn't a refactor. Treating it as if it closed the architectural debt would be wrong — every structural punch-list item except three is still open, and the new code compounds some of it.
259+
260+
**The "adoption still stuck in round 1" observation** from the public API agent is the single most important line in this review. The Tree API exists but the library's outer layer (README, doc.go, compat tests) still treats legacy Parse as canonical. Until that flips, the adoption story hasn't moved.
261+
262+
**For HN launch / v1.15 release**: do Sprint A + B minimum. That's one week of work, and it turns "we added Tree" into "Tree is how you use this library."

‎pkg/gosqlx/errors.go‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -46,4 +46,8 @@ var (
4646
// ErrUnsupportedDialect indicates the dialect supplied via WithDialect is
4747
// not recognized by the underlying keywords package.
4848
ErrUnsupportedDialect = errors.New("gosqlx: unsupported dialect")
49+
// ErrTooLarge is returned when input exceeds the configured maximum byte
50+
// size (see WithMaxBytes). Callers can test errors.Is(err, ErrTooLarge) to
51+
// distinguish cap-enforcement failures from read/parse errors.
52+
ErrTooLarge = errors.New("gosqlx: input too large")
4953
)

0 commit comments

Comments
 (0)