[ajitpratap0]

Summary

#603 parser/ast: JOIN ... USING (cols) is stored as a plain condition and renders as ON cols

Fixes #603. JOIN ... USING (cols) was being rendered as ON cols by ast SQL(), pkg/formatter and gosqlx format. Multi-column USING produced ON id, k, which failed to re-parse with E2002, and the CLI printed ON LIST. The parser now stores USING columns in a new additive field, JoinClause.Using []Expression, one *ast.Identifier with Pos per column, and leaves Condition nil. All three renderers emit USING (c1, c2). Children() and Walk expose the columns, ReleaseAST releases them, the CLI analyzer treats USING as a join condition, and the schema validator checks every USING column. Independent validation found 0 failures in all 11 AllDialects entries for SQL() and formatter round-trips, re-parse and idempotence; the built CLI shows the correct output; and the race gate, go vet and golangci-lint all pass. Out of scope and deferred, for sound reasons:

• parser: parenthesized set operations not accepted as derived tables or statements; ((SELECT ...)) t gives a misleading join-group error #612 (parenthesized set operations): a separate commit in this chain.
• Quoted identifiers in USING lists losing their quotes: the general quoting problem tracked by ast/formatter: quoted identifiers that are keywords or case-sensitive lose their quotes on render (SELECT "select", CTE/view column lists, table names) #611.
• ClickHouse's parenthesis-free USING id grammar.
• The CLI analyzer flagging NATURAL JOIN: existing behaviour, unrelated.
• Extract* not collecting columns from join conditions: existing behaviour, unrelated.

#612 parser: parenthesized set operations not accepted as derived tables or statements; ((SELECT ...)) t gives a misleading join-group error

Fixes #612. A derived table in FROM, JOIN or a comma list can now be a set operation, a WITH query or a doubly parenthesized query. A whole statement or a set-operation operand can now be written as (query), for example (SELECT 1) UNION (SELECT 2) and SELECT 1 UNION (SELECT 2). The AST changes are additive: TableReference.SetSubquery and SetOperation.LeftParenthesized / RightParenthesized. SQL(), pkg/formatter and the CLI formatter keep the parentheses that change meaning, such as A EXCEPT (B UNION C). ORDER BY/LIMIT that the AST cannot represent returns E4001 instead of building a wrong tree. Independent validation:

• All issue reproductions fail on the parent commit, and on HEAD they parse and round-trip through SQL() under the default parser and all 9 dialects.
• Join groups and their E4001 errors are unchanged.
• The race suite (uncached), vet, golangci-lint and the cbinding tests pass.

Deferred, as planned:

• parenthesized set operations inside expression subqueries
• WITH ... (SELECT ...) and CREATE VIEW v AS (SELECT ...)
• INSERT INTO t (SELECT ...) (parser: INSERT gaps: RETURNING after INSERT ... SELECT, DEFAULT VALUES, MySQL INSERT ... SET, INSERT INTO t (SELECT ...) #610)
• nested query-level ORDER BY/LIMIT, which needs a Query wrapper node
• ClickHouse SETTINGS after a parenthesized statement
• releasing SetOperation operands from ReleaseAST, CTE bodies and expression subqueries
• INTERSECT binding tighter than UNION/EXCEPT
• readable-format semicolon placement (formatter: readable style emits ; inside CTE/IN/EXISTS subqueries and set operations, and ;; after INSERT ... SELECT / CREATE VIEW #602)

Also known: BenchmarkParserSimpleSelect is +4.8% and +16 B/op because TableReference grew from 128 to 136 bytes; reordering fields could recover this in a follow-up.

Commits (2), one implementation commit per issue plus test-only follow-ups:

• 9120fce fix(parser,ast,formatter): accept parenthesized set operations and WITH queries as derived tables and statements (parser: parenthesized set operations not accepted as derived tables or statements; ((SELECT ...)) t gives a misleading join-group error #612)
• 1d36dd2 fix(parser,ast,formatter): keep JOIN ... USING (cols) instead of rendering ON cols (parser/ast: JOIN ... USING (cols) is stored as a plain condition and renders as ON cols #603)

28 files changed, 2378 insertions(+), 151 deletions(-)

Test Plan

• Each issue's reproduction cases were run against main before the fix (Haiku scout), and a failing regression test was written before the code change.
• Independent review (Opus, or Sonnet on small chains) with a bounded fix loop.
• Independent Opus validator ran the repository race gate (CGO_ENABLED=1 go test -race -timeout 300s $(go list ./... | grep -v /cbinding)), go vet, golangci-lint, re-ran the reproduction cases on HEAD, and confirmed the new tests fail on the parent commit.
• Perf items: benchstat -count=6 before/after on the same machine.

#603 evidence (validator)

Not applicable: this is a bug fix with no perf items, and joins without USING keep their Children() fast paths. Before and after evidence:

On the parent (1d36dd2^ = 61ca6c8, extracted with git archive into scratchpad/b3/603/parent):
- `SELECT * FROM a JOIN b USING (id)` gives SQL() and compact `SELECT * FROM a INNER JOIN b ON id`.
- `SELECT * FROM a LEFT JOIN b USING (id, k)` gives SQL() and compact `... LEFT JOIN b ON id, k`. Re-parsing that fails with E2002 at line 1, column 34 (expected statement, got COMMA).
- The parent CLI `format --compact` prints `SELECT * FROM a LEFT JOIN b ON LIST`.

On HEAD, a scratch harness (scratchpad/b3/603/val/harness, using a replace directive to the worktree) ran 6 SQL cases across all 11 entries of keywords.AllDialects(): generic, postgresql, mysql, mariadb, sqlserver, oracle, sqlite, snowflake, bigquery, redshift, clickhouse. The cases were single USING, multi USING, ON, NATURAL, CROSS, and a mixed chain `JOIN b USING (id) JOIN c ON ... RIGHT JOIN d USING (x, y, z)`. Result: 0 failures. Every case gives:
- SQL() as `... USING (id)` / `... USING (id, k)`
- a successful re-parse, idempotent SQL(), and identical Using names after re-parse
- formatter compact and readable output that re-parses, with no `ON id` and no `LIST`

FormatString gives `SELECT *\nFROM a\nLEFT JOIN b USING (id, k);`. The USING identifiers have Pos set (id@1:31; id@1:36, k@1:40), and Condition is nil.

The CLI built from HEAD prints:
- format: `SELECT\n  *\nFROM a\nINNER JOIN b USING (id)` and `...LEFT JOIN b USING (id, k)`
- --compact: `SELECT * FROM a INNER JOIN b USING (id)` and `SELECT * FROM a LEFT JOIN b USING (id, k)`
- validate: `Valid SQL` for both
- analyze: no JOIN_WITHOUT_CONDITION or cartesian finding for either. As a control, CROSS JOIN is still flagged.

Validating the old broken output `SELECT * FROM a LEFT JOIN b ON id, k` still fails with E2002, as expected.

Regression tests against the parent: the new parser, ast, formatter and cmd test fil

#603 race gate: PASS. Command run: CGO_ENABLED=1 go test -race -timeout 300s $(go list ./... | grep -v /cbinding) in the worktree at HEAD 1d36dd2. Exit 0, 44 packages ok, no FAIL lines and no flakes, so nothing was re-run. Log: /private/tmp/claude-501/-Users-ajitpratapsingh-dev-GoSQLX/b99cd3cf-cb92-4294-a0db-d6a7c9a86253/scratchpad/b3/603/val/race.txt. TestChildrenCoverage_VisitorContract and the new concurrent TestJoinUsing_PoolRelease_Issue603 are both in this run.
#612 evidence (validator)

This is a bug fix, so the evidence is the reproductions and round-trips. I also re-ran the optional benchmark.

Reproductions: I built a separate harness module in the scratchpad (scratchpad/b3/612/val/harness) that points at the worktree, and a copy that points at a git archive of the parent 1d36dd2 (scratchpad/b3/612/parent). For each input it parses under the default parser and all 9 dialects, then runs SQL(), parses again, and also renders with formatter.FormatAST in compact and readable style and parses that.
- All 6 issue shapes fail on the parent with the errors the scout recorded, and parse on HEAD in all 10 dialect configurations with SQL() round-trip stable:
  - `SELECT * FROM (SELECT 1 UNION SELECT 2) t`
  - `... JOIN (SELECT 1 UNION SELECT 2) t ON ...`
  - `((SELECT 1) UNION (SELECT 2)) t`
  - `((SELECT 1)) t`, which renders as `(SELECT 1) t`
  - `(WITH c AS ...) t`
  - `(SELECT 1) UNION (SELECT 2)`
  - `(SELECT 1)`, which renders as `SELECT 1`
  - `(SELECT 1) ORDER BY 1`, which renders as `SELECT 1 ORDER BY 1`
  - `SELECT 1 UNION (SELECT 2)`
- The scout's "works today" controls give the same output as on the parent.
- The CLI binary built from HEAD validates `SELECT * FROM (SELECT 1 UNION SELECT 2) t`. `gosqlx format` prints `(SELECT 1) UNION (SELECT 2)` with its parentheses.
- Meaning-changing parentheses are kept: `A EXCEPT (B UNION C)` and `A UNION (B ORDER BY a LIMIT 3)` render with them. `A UNION (B) ORDER BY a` renders as `A UNION B ORDER BY a`.
- These now return E4001: `(q ORDER BY a LIMIT 1) ORDER BY a`, `SELECT 1 UNION (SELECT 2 UNION SELECT 3) ORDER BY 1`, and a right operand with WITH followed by ORDER BY.
- Unchanged from the parent, including errors: join groups `((SELECT a FROM x) t JOIN u ...)`, the E4001 for an alias after a join group, the E4001 for a comma list inside a join group, `(t)`, `((t))`, `(1)`, `(VALUES ...)`, and `SELECT a FROM t ORDER BY a LIMIT 1 UNION SELECT b FROM u`.

Regression tests on the parent: I copied the new te

#612 race gate: PASS. CGO_ENABLED=1 go test -race -timeout 300s $(go list ./... | grep -v /cbinding) exited 0 and every package passed; the first run had 29 cached packages. I then re-ran the whole suite with -count=1, so nothing was cached. It again exited 0 with no FAIL: parser 10.0s, ast 4.0s, formatter 2.0s, cmd/gosqlx/cmd 2.0s. task test:cbinding passed (pkg/cbinding ok). TestPerformanceRegression (./pkg/sql/parser, without race, -count=1) passed. No flaky tests were seen.

Breaking Changes

• parser/ast: JOIN ... USING (cols) is stored as a plain condition and renders as ON cols #603: Additive: new exported field Using []ast.Expression on ast.JoinClause (appended after Condition; existing keyed literals still compile). Observable behaviour change: for JOIN ... USING (cols) the parser now leaves Condition nil and stores the columns in Using, as plain *ast.Identifier nodes with no ListExpression wrapper. Every renderer (ast SQL(), pkg/formatter, CLI format) now emits USING (cols) instead of ON cols. Children() for USING joins returns the Using identifiers in place of the old Condition node. No exported function signatures change; parseJoinCondition is unexported. The commit body and PR state the representation change.
• parser: parenthesized set operations not accepted as derived tables or statements; ((SELECT ...)) t gives a misleading join-group error #612: Additive only.
• New exported fields: ast.TableReference.SetSubquery *ast.SetOperation, ast.SetOperation.LeftParenthesized bool and ast.SetOperation.RightParenthesized bool. No signature or field type changes; TableReference.Subquery stays *SelectStatement and still holds every derived table whose body is a single SELECT.
• New accepted inputs: set operations and WITH queries as derived tables; doubly-parenthesized derived queries; statements and set-operation operands written in parentheses. A FROM-less SELECT may also be followed by ORDER BY, LIMIT, OFFSET or FETCH.
• New E4001 errors for ORDER BY, LIMIT, OFFSET or FETCH after a parenthesized query whose tail cannot be represented.
• Renderers (ast SQL(), pkg/formatter, CLI formatter) print parentheses only where the new flags are set or Right is a *SetOperation. Output for every previously parseable input is unchanged.
• Documented in the field doc comments, pkg/sql/ast/doc.go, docs/API_REFERENCE.md and docs/SQL_COMPATIBILITY.md. CHANGELOG is left to the release workflow.

Notes

• CHANGELOG.md is intentionally not touched; the consolidated CHANGELOG PR docs: consolidate CHANGELOG for the perf audit and batch-2/batch-3 fixes; refresh performance baselines (#566) #598 gets the entries for this batch before it merges.
• Part of the batch-3 merge train (follow-ups cli: gosqlx format silently corrupts many expressions; switch the CLI to pkg/formatter #599-schema/postgres: loader misreports index columns and key flags (column order, INCLUDE, expression and partitioned indexes, read-only roles) #620); later PRs in the train are rebased onto this one after it merges.
• Base: main.

Fixes #603
Fixes #612
Follow-up to #566

🤖 Generated with Claude Code

https://claude.ai/code/session_01UZSk1292jPc8ipDboNxeve

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated
gosqlx-website	Ready	Preview	Oct 14, 2026 7:04am UTC

[github-actions]

Claude finished @ajitpratap0's task in 35s —— View job

---

Review of PR #630 (JOIN ... USING and parenthesized set operations)

I read the parser, AST, release and SQL-rendering diffs. I did not run the test suite in this job, so the CI results and your validator logs are the only evidence for behavior.

Summary: This is a well-scoped, additive change. The Using []Expression design leaves Condition nil, which avoids the old ON id, k bug at its source. I found no blocking issues. The points below are minor.

What looks good

• Additive AST: JoinClause.Using, TableReference.SetSubquery and the SetOperation.*Parenthesized flags are all appended fields, so existing keyed literals still compile.
• Pool safety: releaseSelectStatement releases Using and zeroes the whole JoinClause. Left is still deliberately not released. The new releaseSetOperationOperands walks the left spine iteratively and recurses only on a parenthesized right operand, which MaxRecursionDepth bounds.
• Recursion limits: parseParenthesizedQuery calls enterRecursion/leaveRecursion. The (( lookahead returns false once the run exceeds MaxRecursionDepth, so the join path reports E2007.
• Conservative failure mode: Cases the AST can't represent return E4001 instead of building a wrong tree.
• Tests: Coverage is broad: parser, AST, formatter, CLI, schema, pool release, and the children/visitor contract.

Suggestions (non-blocking)

1. Children() nil check (pkg/sql/ast/ast_clauses.go, childrenInPlace): if u != nil won't catch a typed-nil *Identifier stored in the interface. The parser never produces one, but a hand-built AST could. This matches how other nodes behave, so I'd leave it.

2. Tail merge changes tree shape (attachParenthesizedQueryTail): (A UNION B) ORDER BY x merges ORDER BY into operand B and drops its parentheses. That matches how A UNION B ORDER BY x is already represented, and the doc comment says so. A caveat is that the AST can't tell the two inputs apart, so the formatter output is textually different from the source. A short test pinning this (it may already exist in paren_query_test.go) would document the lossy behavior.

3. Lookahead cost (parenOpensQueryExpression): It scans to the matching ) on every (( start, and nesting is capped by MaxRecursionDepth. Pathological nested input could be roughly O(depth × length). It is bounded, so it's fine, but a fuzz or benchmark case with deeply nested (( would confirm this.

4. Extra scan in SQL rendering (SetOperation.writeSQL): The condition s.RightParenthesized || (ok && rightSetOp != nil) is correct. The rightSetOp != nil guard handles a typed-nil right operand, which is good. Extracting a small helper would make the rule "a SetOperation right operand is always parenthesized" easier to find, since the formatter and CLI formatter duplicate it.

5. Known follow-ups already noted in the PR:

   • The TableReference size growth (128 to 136 bytes, +4.8% on BenchmarkParserSimpleSelect). Reordering fields, for example moving the new pointer next to the other pointer fields, may recover it.
   • Quoted identifiers in USING lists lose their quotes (ast/formatter: quoted identifiers that are keywords or case-sensitive lose their quotes on render (SELECT "select", CTE/view column lists, table names) #611).
   • SetOperation operands are not released from statement-level ReleaseAST. This is a leak-to-GC rather than a correctness bug, because the objects are simply not returned to the pool.

6. Docs/CHANGELOG: The PR intentionally leaves CHANGELOG.md to docs: consolidate CHANGELOG for the perf audit and batch-2/batch-3 fixes; refresh performance baselines (#566) #598. Make sure the parser/ast: JOIN ... USING (cols) is stored as a plain condition and renders as ON cols #603 behavior change is listed there, because downstream code that read Condition for USING joins will now see nil.

Security

No concerns. There is no new unbounded recursion or allocation driven by input: depth is bounded, and the closeAt array is fixed-size.

Verdict

The changes look correct and well tested, and I have no blocking concerns. This is a comment, not a formal approval.

[ajitpratap0]

Merge train (batch 3): rebased onto main after #628 merged (205a6c3).

Conflicts:

• TableFunc vs SetSubquery in tableRefSQL/writeTableRef: both branches kept. Order is Subquery → TableFunc → SetSubquery → name; the cases are mutually exclusive.
• parseFromClause stop condition: !isDMLReturningKeyword() and the ORDER/LIMIT/OFFSET/FETCH check are combined.
• parseDerivedTableQuery: parser: parenthesized set operations not accepted as derived tables or statements; ((SELECT ...)) t gives a misleading join-group error #612 renamed this function, and fix(parser): INSERT ... SELECT RETURNING, DEFAULT VALUES, MySQL INSERT ... SET and parenthesized sources (#610) #623 had added a dmlReturning save/restore to the old one. The renamed function now keeps the save/restore, so derived tables (including parenthesized set operations) are still a nested scope.

Checks after the rebase:

• Full race gate passes locally.
• go vet is clean; golangci-lint reports 0 issues.

Adversarial pass (Opus): found no must-fix items. The should-fix security item ADV630-1 is fixed in this PR: checkUnionInjection now walks right-nested and parenthesized set-operation trees, and Opus validated it.

Deferred to follow-up issues: two scanner gaps that predate this PR. Derived tables and comma-joined system tables are not scanned, and the NULL-column UNION check never fires.

[github-actions]

Claude finished @ajitpratap0's task in 21s —— View job

---

Review of PR #630 (updated for the new security-scanner commit)

I read the new commit 205a6c3 in full. For the rest of the PR I relied on my earlier read and on your validator logs. I did not run the test suite in this job.

Security scanner change (pkg/sql/security/scanner.go)

This fixes a real detection gap. After #612, x UNION (SELECT … FROM pg_user UNION …) builds a right-nested SetOperation, and the old Right.(*ast.SelectStatement) assertion missed it. The new tree walk looks right.

• Each SELECT is visited once for the union checks, and a separate leaf walk handles the per-statement checks. Findings should not be duplicated.
• inUnionRight propagates through nesting, so UNION (a EXCEPT b) still checks both a and b. That is the conservative choice.
• strings.EqualFold replaces ToUpper, which is fine.
• Non-blocking: the left operand of a top-level UNION is not checked, as before. A leading (SELECT … FROM pg_user) UNION … therefore only gets the generic per-statement checks. That matches the old behavior, but it is worth a comment so nobody assumes otherwise.
• Non-blocking: checkUnionSelect only inspects *ast.SelectStatement. A WITH … SELECT right operand, if it is represented differently, would be skipped. Confirm the new TestScan_UnionInjectionInNestedSetOperations has a case for it.
• Non-blocking: the walk recurses on both operands. Depth is bounded by the parser's MaxRecursionDepth, but a hand-built AST could be arbitrarily deep.

Rest of the PR

My previous review still stands.

• The additive AST fields are fine: JoinClause.Using, TableReference.SetSubquery, and SetOperation.LeftParenthesized/RightParenthesized.
• Pool release of Using is handled.
• Cases the AST cannot represent return E4001 instead of building a wrong tree.
• Test coverage is broad: parser, AST, formatter, CLI, schema, and pool release.

Remaining suggestions, all non-blocking:

1. Add a test that pins the lossy tail merge, where (A UNION B) ORDER BY x renders the same as A UNION B ORDER BY x.
2. Fuzz or benchmark deeply nested (( input to confirm the parenOpensQueryExpression lookahead stays cheap.
3. Pull the "a SetOperation right operand is always parenthesized" rule into one helper. The AST renderer, pkg/formatter, and the CLI formatter each duplicate it.
4. Recover the TableReference size regression (128 to 136 bytes, +4.8% on BenchmarkParserSimpleSelect) by reordering fields.
5. Make sure the docs: consolidate CHANGELOG for the perf audit and batch-2/batch-3 fixes; refresh performance baselines (#566) #598 CHANGELOG entry notes that Condition is now nil for USING joins.

Verdict

No blocking issues. This is a comment, not a formal approval.