[ajitpratap0]

Summary

#602 formatter: readable style emits ; inside CTE/IN/EXISTS subqueries and set operations, and ;; after INSERT ... SELECT / CREATE VIEW

In readable style (AddSemicolon=true), the formatter now ends only the outermost statement with ';'. Before this change, CTE bodies, IN/EXISTS/scalar subqueries and set-operation operands got an inner ';' that broke re-parsing, and INSERT ... SELECT and CREATE [MATERIALIZED] VIEW ended in ';;'. The fix adds an unexported nestedOpts helper that clears AddSemicolon and uses it at the 8 nested render sites. renderSetOperation now writes one ';' after the whole chain, still in a single linear pass over the spine. Compact output is byte-identical to before across 163 corpus statements. In a corpus check over testdata, readable-style re-parse failures fell from 36 to 6 and ';;' occurrences from 13 to 0. The new regression tests (24 round-trip cases, a corpus invariant, and FormatExpression subquery cases) fail on the parent and pass on HEAD; the race suite, vet and golangci-lint are clean. Known issues left for follow-ups: SHOW/DESCRIBE/SEQUENCE/fallback renderers never emit ';' (for example "SHOW TABLES\nSELECT 1;"), NOT EXISTS renders as "NOT (EXISTS (...) NOT )", MATCH...AGAINST and ARRAY-cast rendering are broken, the table name "groups" does not re-parse, nested-subquery indentation, and parenthesization of right-nested set operations.

#600 ast/formatter: SQL() and pkg/formatter drop IGNORE NULLS, named arguments, ON DUPLICATE KEY UPDATE and INSERT OUTPUT; parser discards SELECT ... SETTINGS

Fixes #600. tree.SQL() and pkg/formatter (compact and readable) now keep five constructs they used to drop:

• FunctionCall IGNORE/RESPECT NULLS
• named arguments (f(x => 1), via the new (*ast.NamedArgument).SQL())
• MySQL ON DUPLICATE KEY UPDATE
• SQL Server INSERT ... OUTPUT
• ClickHouse SELECT ... SETTINGS

The SETTINGS clause is now parsed into the new additive field SelectStatement.Settings ([]ast.QuerySetting). For set operations it is stored on the rightmost SELECT. It is released and zeroed in the pool release path. A trailing FORMAT <name> is still accepted as parse-only, and a malformed SETTINGS list or a bare FORMAT now returns a syntax error.

Every reproduction from the issue round-trips verbatim through SQL(), FormatAST compact and FormatAST readable, and re-parses in its own dialect. The PostgreSQL ON CONFLICT ... RETURNING control case is unchanged. The new regression tests fail on the parent commit and pass on HEAD. The full -race suite, go vet and golangci-lint are clean.

Deferred (follow-ups):

• CLI gosqlx format still uses its own renderer, so it keeps dropping these clauses until cli: gosqlx format silently corrupts many expressions; switch the CLI to pkg/formatter #599 switches it to pkg/formatter.
• parse -f json does not serialize Settings.
• SETTINGS in other positions (CTE-led queries, subqueries, INSERT ... SELECT) is not handled.
• CREATE TABLE ... SETTINGS is still not stored.
• FORMAT is still not modeled on the AST.
• MergeStatement.Output is not rendered.
• Oracle in-argument LAST_VALUE(x IGNORE NULLS) is not parsed.
• Snowflake table-function arguments in FROM are still dropped; that is ast/formatter: SQL() and FormatAST drop table-function call arguments in FROM (generate_series(1,10) -> generate_series) #607.

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

• d6049e4 fix(parser): address review for ast/formatter: SQL() and pkg/formatter drop IGNORE NULLS, named arguments, ON DUPLICATE KEY UPDATE and INSERT OUTPUT; parser discards SELECT ... SETTINGS #600
• 4c2e377 fix(ast,formatter,parser): keep IGNORE NULLS, named arguments, ON DUPLICATE KEY UPDATE, INSERT OUTPUT and ClickHouse SELECT SETTINGS (ast/formatter: SQL() and pkg/formatter drop IGNORE NULLS, named arguments, ON DUPLICATE KEY UPDATE and INSERT OUTPUT; parser discards SELECT ... SETTINGS #600)
• 1a97287 fix(formatter): terminate only the outermost statement in readable style (formatter: readable style emits ; inside CTE/IN/EXISTS subqueries and set operations, and ;; after INSERT ... SELECT / CREATE VIEW #602)

14 files changed, 957 insertions(+), 35 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.

#602 evidence (validator)

Not a perf item, so benchstat does not apply. Evidence for the bug fix:
(1) Parent check: I extracted 61ca6c8 (the parent) with git archive into scratchpad/b3/602/parent and added nested_semicolon_test.go. On the parent, 22 of the 24 TestFormat_NestedStatementsNoInnerSemicolon subtests FAIL. The 2 that pass are the controls (derived table and select-list subquery), as expected. TestFormatAST_ReadableSemicolonOnlyTerminatesStatements and all 3 TestFormatExpression_SubqueryNoSemicolon subtests also FAIL there. On HEAD every one of these tests passes.
(2) Scout cases on HEAD, through FormatString:
- WITH c AS (SELECT 1)\nSELECT *\nFROM c;
- SELECT *\nFROM t\nWHERE a IN (SELECT 1);
- SELECT *\nFROM t\nWHERE EXISTS (SELECT 1);
- SELECT 1\nUNION\nSELECT 2;
- INSERT INTO t (a)\nSELECT a\nFROM u;
- CREATE VIEW v AS\nSELECT 1;
- Control: SELECT *\nFROM (SELECT 1) s;
- Control: SELECT (SELECT 1);
All of them re-parse, a second pass produces identical output, and the compact AST is the same before and after.
(3) Corpus diff, parent vs HEAD, over 163 parseable statements (testdata plus extras): compact output is byte-identical. Readable re-parse failures drop from 36 to 6, and ';;' occurrences from 13 to 0. No readable output on HEAD contains ';)' or ';' before UNION/EXCEPT/INTERSECT. The 6 remaining failures are exactly the non-semicolon defects listed in out_of_scope, and all of them also fail on the parent: NOT EXISTS rendered as NOT (EXISTS ... NOT ) (3 statements), MATCH AGAINST, ARRAY cast, and the "groups" table name.

#602 race gate: PASS. Ran CGO_ENABLED=1 go test -race -timeout 300s $(go list ./... | grep -v /cbinding) on HEAD 1a97287: exit 0, 44 packages ok, no FAIL lines and no flakes (log at scratchpad/b3/602/race.txt). CGO_ENABLED=1 go test -race ./pkg/cbinding/ also passed (ok, 1.8s).
#600 evidence (validator)

Not a performance item, so benchstat was not run. Bug evidence, with each case's output re-parsed under the same dialect:

- All 5 scout reproductions now render verbatim through tree.SQL() and FormatAST(CompactStyle), and the readable style keeps the clause. All of these outputs re-parse:
  - `FIRST_VALUE(x) IGNORE NULLS OVER (ORDER BY y)`
  - `f(x => 1)`
  - MySQL `... ON DUPLICATE KEY UPDATE a = 2`
  - SQL Server `INSERT INTO t (a) OUTPUT INSERTED.id VALUES (1)`
  - ClickHouse `SELECT a FROM t SETTINGS max_threads = 4` (readable: `SELECT a\nFROM t\nSETTINGS max_threads = 4;`)
- The PostgreSQL ON CONFLICT ... RETURNING control case is unchanged and still verbatim.
- Extra cases that also round-trip:
  - `LAG(x) RESPECT NULLS`
  - quoted named argument `"my arg" => 1`
  - multi-row ON DUPLICATE KEY with `VALUES(a), b = b + 1`
  - OUTPUT with INSERT ... SELECT
  - UNION ALL ... SETTINGS (stored on the rightmost select)
  - UNION ALL ... ORDER BY ... LIMIT ... SETTINGS (clause order kept)
  - SETTINGS with a boolean or string value
  - `SETTINGS ...; SELECT 1`
  - `SETTINGS ... FORMAT Null` / `FORMAT JSONEachRow` (parse OK; FORMAT is dropped as before, parse-only)

Regression tests fail on the parent tree, built with `git archive 1a97287` and the new test files copied in:
- TestRender_Issue600_KeepsClauses fails with "SQL() mismatch", "compact FormatAST mismatch", "readable FormatAST lost clause IGNORE NULLS / x => 1 / ON DUPLICATE KEY UPDATE ...", and "did not round-trip".
- The AST and parser tests do not compile on the parent, because SelectStatement.Settings and QuerySetting do not exist there.

The review-fix tests fail on 4c2e377 with the exact E2002 error the report gave: "expected statement, got NULL" for `FORMAT Null`.

#600 race gate: PASS. Ran CGO_ENABLED=1 go test -race -timeout 300s $(go list ./... | grep -v /cbinding) at HEAD d6049e4: 44 packages ok, 0 FAIL, exit 0, no flakes. That run includes TestClickHouseSettingsTTLFormat, clickhouse_qa select_settings and select_settings_multi, the new pool release test, and the sql_golden_test.

Breaking Changes

• formatter: readable style emits ; inside CTE/IN/EXISTS subqueries and set operations, and ;; after INSERT ... SELECT / CREATE VIEW #602: There are no exported API or signature changes. The only new identifier is the unexported helper nestedOpts. Observable behaviour changes only when AddSemicolon is set (ReadableStyle, FormatString's default): nested statements (CTE bodies; IN, EXISTS and scalar subqueries rendered by the formatter; set-operation operands; and the query of INSERT ... SELECT / CREATE [MATERIALIZED] VIEW) no longer get their own ';'. A set operation chain ends with a single ';'. Compact style is unchanged. The commit/PR should describe this as a correctness fix: the old output did not re-parse, or contained ';;'.
• ast/formatter: SQL() and pkg/formatter drop IGNORE NULLS, named arguments, ON DUPLICATE KEY UPDATE and INSERT OUTPUT; parser discards SELECT ... SETTINGS #600: The changes are additive and non-breaking.
• New exported type ast.QuerySetting{Name string; Value Expression; Pos models.Location}. It is plain data, not a Node, and has a (*QuerySetting).SQL() string method.
• New exported field ast.SelectStatement.Settings []QuerySetting, which the parser populates only for ClickHouse.
• New method (*ast.NamedArgument).SQL() string.

Observable behaviour changes:

• tree.SQL() and pkg/formatter now emit IGNORE/RESPECT NULLS, name => value named arguments, INSERT ... OUTPUT, ON DUPLICATE KEY UPDATE and ClickHouse SELECT ... SETTINGS. Before, these were dropped.
• SelectStatement.Children() includes setting values.
• ClickHouse SELECT ... SETTINGS lists that are malformed (missing name, '=' or value, or trailing tokens other than FORMAT <name>) now return a syntax error instead of being skipped.

The commit body will document these. No exported signatures change, and the legacy ast.Setting type is untouched.

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 #602
Fixes #600
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:14am UTC

[github-actions]

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

---

PR #631 review

Verdict: looks good. I have no blocking issues. I read the diff of the non-test source files and did not run the test suite. I'm relying on the validator evidence in the PR description for the race gate.

What I checked

• formatter: readable style emits ; inside CTE/IN/EXISTS subqueries and set operations, and ;; after INSERT ... SELECT / CREATE VIEW #602 nestedOpts (pkg/formatter/render.go): it clears AddSemicolon at all the nested render sites: CTE bodies, IN, EXISTS, scalar subqueries, INSERT...SELECT, both view kinds, and set-operation operands. renderSetOperation now writes one ; after the whole chain. It still walks the spine in one linear pass, so the earlier fix(tokenizer,parser,formatter): bound input-driven recursion, comment counts and set-operation rendering #597 linear-time fix is intact. Taking opts by value means the caller's options are never mutated.
• ast/formatter: SQL() and pkg/formatter drop IGNORE NULLS, named arguments, ON DUPLICATE KEY UPDATE and INSERT OUTPUT; parser discards SELECT ... SETTINGS #600 SQL() and formatter: the clause order matches the grammar.
  • IGNORE/RESPECT NULLS comes after ) and before WITHIN GROUP, FILTER and OVER.
  • OUTPUT comes before VALUES or the query.
  • ON DUPLICATE KEY UPDATE comes before RETURNING.
  • SETTINGS comes after FOR.
• Pooling: releaseSelectStatement releases each setting value and zeroes the struct before truncating, so there is no stale reference or leak. pool_settings_release_test.go covers it.
• Parser (clickhouse_select_settings.go):
  • It replaces the old skip-to-EOF loop with real parsing, and for set operations it attaches the settings to the rightmost SELECT.
  • Accepting keyword-like setting names such as limit is pragmatic.
  • A bare FORMAT is now an error, as the PR says.

Minor observations (non-blocking)

1. FORMAT <name> is still dropped. The PR already lists this. SQL() or the formatter on SELECT ... SETTINGS x=1 FORMAT JSON silently loses the format hint. If the AST output feeds ClickHouse clients, that changes behaviour, so a doc comment on QuerySetting or Settings would help.
2. Settings on a non-rightmost operand are not handled in renderSetOperation. The parser only fills the rightmost SELECT, so this is fine today. An AST built by hand with Settings on a left operand would render ... SETTINGS ... UNION ..., which is invalid. Acceptable, since nothing produces that tree.
3. isQuerySettingName accepts any token that starts with a letter or _. That includes reserved words, and the next token must be =, so that is fine. It would also accept a token like FORMAT as a setting name, e.g. SETTINGS a=1, FORMAT=2. That is an edge case, and ClickHouse itself is ambiguous there.
4. ReadableStyle output for the known leftovers (NOT EXISTS, SHOW/DESCRIBE without ;, right-nested set-operation parentheses) is deferred. I agree these belong in follow-ups. For the right-nested case, renderSetOperation ignores the Right subtree's own precedence, so a UNION (b INTERSECT c) can change meaning when re-rendered. It is worth a tracking issue if one doesn't exist yet.
5. Error wrapping: InvalidSyntaxError(...).WithCause(err) embeds %v of the cause in the message and also attaches it as the cause. The text is duplicated if both are printed. This is cosmetic.

Tests

Coverage is good. It has 24 round-trip cases, a corpus invariant, a pool release test, parser error cases and golden updates. The reported fail-on-parent / pass-on-HEAD check is the right way to validate this. The only test I'd add is a round-trip for a set operation inside a CTE inside INSERT...SELECT, so that nestedOpts is exercised across several levels at once.

Security and performance

There are no security concerns. The parser changes are bounded loops that always advance a token. There is no new allocation on hot paths beyond the Settings slice, which only ClickHouse queries with SETTINGS populate.

CHANGELOG is intentionally deferred to #598, as the PR notes.
· branch fix/formatter-semicolons-dropped-clauses

[ajitpratap0]

Merge train (batch 3): rebased onto main after #630 merged (9c8640b).

Conflict resolution: these resolutions match the trial integration of all 12 branches, where the full race gate passed.

• renderSetOperation: combines parser: parenthesized set operations not accepted as derived tables or statements; ((SELECT ...)) t gives a misleading join-group error #612 and formatter: readable style emits ; inside CTE/IN/EXISTS subqueries and set operations, and ;; after INSERT ... SELECT / CREATE VIEW #602.
  • From parser: parenthesized set operations not accepted as derived tables or statements; ((SELECT ...)) t gives a misleading join-group error #612: parenthesized left and right operands are wrapped, and a right operand that is a *SetOperation is wrapped.
  • From formatter: readable style emits ; inside CTE/IN/EXISTS subqueries and set operations, and ;; after INSERT ... SELECT / CREATE VIEW #602: every operand is rendered with nestedOpts, and one ; goes after the whole chain.
• renderInsert and InsertStatement.writeSQL: OUTPUT is placed after the column list, before SET/VALUES/SELECT/DEFAULT VALUES, and is skipped for the MySQL SET form (parser: INSERT gaps: RETURNING after INSERT ... SELECT, DEFAULT VALUES, MySQL INSERT ... SET, INSERT INTO t (SELECT ...) #610).

Checks after the rebase: the full race gate passes locally, go vet is clean, and golangci-lint reports 0 issues.

Adversarial pass (Opus): no must-fix findings. One should-fix: ClickHouse SETTINGS … INTO OUTFILE is now rejected where it used to be silently swallowed. This is intentional and consistent with SELECT … INTO OUTFILE already being rejected on main. INTO OUTFILE support goes to a follow-up issue.

[github-actions]

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

---

PR #631 review

Verdict: looks good, no blocking issues. I read the non-test source diff (render.go, sql.go, clickhouse_select_settings.go, the pool release path). I did not run the test suite. I'm relying on the validator evidence in the PR description for the race gate.

#602 (semicolons)

• nestedOpts is applied at all nested render sites: CTE bodies, IN, EXISTS, scalar subqueries, INSERT...SELECT, both view kinds, and set-operation operands. opts is taken by value, so the caller's options are never mutated.
• renderSetOperation now writes a single ; after the chain. It still walks the spine in one linear pass, so the fix(tokenizer,parser,formatter): bound input-driven recursion, comment counts and set-operation rendering #597 linear-time behaviour is kept.
• The old code rendered the innermost left operand and non-parenthesized right operands with opts. Both now use inner, which is the right fix.

#600 (dropped clauses)

• Clause order in SQL() and the formatter matches the grammar:
  • IGNORE/RESPECT NULLS comes after ) and before WITHIN GROUP, FILTER and OVER.
  • OUTPUT comes before VALUES or the query.
  • ON DUPLICATE KEY UPDATE comes before RETURNING.
  • SETTINGS comes after FOR.
• The parser replaces the old skip-to-EOF loop with real parsing. It attaches settings to the rightmost SELECT, and a malformed list or a bare FORMAT is now an error.
• releaseSelectStatement releases and zeroes settings, and pool_settings_release_test.go covers it.

Non-blocking observations

1. FORMAT <name> is still dropped. After SQL() or the formatter, SELECT ... SETTINGS x=1 FORMAT JSON loses the format hint. A doc comment on SelectStatement.Settings saying so would help.
2. Settings on a non-rightmost set-operation operand. renderSetOperation would emit ... SETTINGS ... UNION ..., which is invalid. The parser never builds such a tree, but a hand-built AST could.
3. isQuerySettingName is permissive. It accepts any word-like token, so FORMAT is accepted as a setting name in SETTINGS a=1, FORMAT=2. Because = must follow, this is only an edge case.
4. Right-nested set operations. renderSetOperation ignores the precedence of the Right subtree, so a UNION (b INTERSECT c) may change meaning when re-rendered. It is already listed as deferred; I'd open a tracking issue if one doesn't exist.
5. Duplicated error text. InvalidSyntaxError(...%v...).WithCause(err) embeds the cause in the message and also attaches it as the cause. The text is duplicated if both are printed. This is cosmetic.
6. Possible test to add. A round-trip for a set operation inside a CTE inside INSERT...SELECT would exercise nestedOpts across several levels at once.

Security and performance

There are no security concerns. The new parser loops always advance a token. The only new allocation is the Settings slice, which only ClickHouse queries with SETTINGS populate.

CHANGELOG is deferred to #598, as the PR notes.
· branch fix/formatter-semicolons-dropped-clauses