[ajitpratap0]

Summary

Part 2 of 3 splitting #858. It stacks on #866 (corpus and ratchet) and is followed by the small breaking-change PR. This PR holds all the dialect, render and quoting fixes plus the perf work. Render/quoting and parser fixes share about 40 parser files and 47 mixed files, so they ship together; the commits are kept so they can be reviewed one by one.

Result: all 2,788 corpus statements parse, format, re-parse to the same output, and keep every word. known_failures.txt goes from 1,327 entries to 0.

• Parser fixes per dialect:
  • EXPLAIN is now ExplainStatement.
  • DML: multi-table UPDATE/DELETE, OUTPUT, RETURNING … INTO, MERGE.
  • DDL: CREATE/ALTER/DROP objects and CTAS.
  • SELECT clauses, operators and table factors.
  • SQL Server scripts without semicolons, GO batches (ast.BatchSeparator), BEGIN…END bodies, PL/SQL blocks.
  • SplitStatements agrees with the parser.
• Render and quoting fixes:
  • The formatter no longer drops clauses.
  • WITH clauses before set operations stay in front.
  • Optimizer hints stay with their SELECT.
  • PostgreSQL @ stays separate from its operand.
  • Identifier quotes are preserved, and string prefixes and escapes are rendered correctly.
• Transpiler:
  • Strings and identifiers are retargeted to the target dialect.
  • Dialect-specific operators are rewritten, or refused where the target has no equivalent: ^ as power or XOR, PostgreSQL @ |/ ||/, money literals, Snowflake {…}.
  • A GO script sent to another dialect returns an error.
• AST layout: new dialect fields sit behind optional pointers, so plain statements don't carry them. Read them with DialectExtras() / DialectModifiers() and write with EnsureExtras() / EnsureModifiers().
• Performance vs main (6 interleaved rounds, M4, 22 benchmarks):
  • time −6.1% (geomean)
  • memory −34.7% B/op (geomean)
  • allocs equal or fewer
  • high-level gosqlx.Parse* 15–19% faster
  • core parser within about 5% (FullPipeline/delete +5.2%, update +3.3%, the rest ≤ +3%)
• Not in this PR: the last commit takes out the rule that rejects a GO line inside a SQL Server statement. The next PR puts it back as a separate breaking change.

Test Plan

• task check (fmt, vet, golangci-lint, race tests): exit 0
• task test:cbinding: exit 0
• TestRealWorldCorpus: 0 known failures
• Rendered SQL()/FormatAST output is byte-identical to the pre-perf baseline over all 4,597 testdata statements. The AST dump is identical apart from moved field paths.
• Independent reviews of the render, boundaries, Codex and perf work; all review findings fixed or listed below

Breaking Changes

These change behaviour for input that main accepts:

• EXPLAIN <stmt> returns *ast.ExplainStatement. Main returned a *ast.DescribeStatement that lost the statement: EXPLAIN SELECT 1 formatted as DESCRIBE "SELECT".
• gosqlx.Lint / LintWithDialect return an error wrapping ErrTokenize/ErrSyntax for invalid SQL. Main returned no error. linter.FileResult gains TokenizeError, ParseError and SyntaxError().
• Raw NUL / Ctrl-Z inside a string literal is an error where the dialect can't write it back. Main dropped or rewrote the byte when formatting.
• PIVOT/UNPIVOT IN lists require commas between entries and an alias after AS.
• CLI: format --no-uppercase now lower-cases keywords; main upper-cased them.

Not breaking for any input that works today, because main couldn't parse these inputs at all:

• BatchSeparator nodes in AST.Statements
• statement counts that exclude GO
• transpile errors for GO scripts and for operators with no equivalent in the target

The new AST fields are additions.

Known LOW items, left as is:

• Comment placement around GO without ;.
• GO 2 /* c */ is handled differently by the splitter and the parser.
• MariaDB DELETE history FROM history.
• JSON operators vs ^ precedence.
• Quoted type names are written unquoted.
• The dropped-word check compares word sets, not counts.
• FormatAST with a different target dialect doesn't apply the ^ precedence logic.

🤖 Generated with Claude Code

https://claude.ai/code/session_01DnbRnTFErhoMJdAjVEeCg2

[github-actions]

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

---

Review of PR #867: dialect fixes, render/quoting, transpile

Scope of this review: I read internal/batch/batch.go and internal/tokscratch/tokscratch.go in full. I did not read the rest of the diff and I did not run any tests or linters. The diff against test/realworld-corpus is 324 files and about +48.9k/−6k lines, though the PR header lists 100 changed files. The PR description claims task check, task test:cbinding and the corpus test pass. I did not verify any of that.

Findings

1. tokscratch will nil-panic if the tokenizer isn't linked in (low).

• impl is a package-level Func that is nil until pkg/sql/tokenizer's init registers it, and Bytes/String/StringContext call it without a check.
• Any package that imports tokscratch without transitively importing tokenizer will hit a nil-func panic. A defensive if impl == nil { panic("tokscratch: tokenizer not linked") } would give a clearer message.
• Likewise, t any is asserted to *tokenizer.Tokenizer in the implementation. A wrong type there should fail loudly and not silently.

2. Borrowed-slice lifetime is the main risk of the perf work (medium, design).

• The tokscratch contract is "valid until the next tokenize call, Reset, or PutTokenizer", and callers must copy what they keep.
• This is easy to break later, for example by retaining []TokenWithSpan from a gosqlx.Parse* path or an LSP/cache path. The new concurrent_retained_ast_test.go and scratch_tokens_test.go are the right safety net.
• Please make sure at least one test retains an AST or tokens across a second parse on the same pooled tokenizer, then runs under -race. Also confirm that no AST node holds a Word value that points into the scratch buffer. The doc says token strings are input substrings, so this looks safe.

3. batch.Line GO detection (looks correct, a few edge cases to confirm).

• Position checks (End.Column-Start.Column != 2, single line, previous token on an earlier line) are sound and match sqlcmd's "GO alone on its line" rule.
• Count guards strconv.Atoi overflow through the error return, so GO 99999999999999999999 is treated as an invalid count. Good.
• Edge cases worth a test, if not already covered:
  • GO after a /* ... */ comment on the same line. Comments are probably not tokens, so tokens[i-1].End.Line may mistake GO for being alone on its line.
  • GO -- comment on the same line.
  • CRLF line endings.
  • Tokens that span multiple lines, such as a multi-line string ending on the line before GO. That should work, since End.Line is used.
  • go in lowercase. EqualFold handles it.
• The PR notes that GO 2 /* c */ is handled differently by the splitter and the parser. SplitStatements and the parser disagree on that input, which defeats the "SplitStatements agrees with the parser" goal for it. It is listed as a known LOW item, so just make sure it is tracked in an issue.

4. Breaking changes are documented, but they matter for the version bump.

• gosqlx.Lint/LintWithDialect now return an error for invalid SQL, and format --no-uppercase now lower-cases keywords. Callers that relied on the old behaviour will break silently or change output.
• EXPLAIN now returns *ast.ExplainStatement, and the PIVOT/UNPIVOT IN syntax is stricter. Type switches over DescribeStatement will break.
• The description says a follow-up PR carries the GO-inside-a-statement breaking change. Since this PR already has user-visible breaks, the CHANGELOG should say clearly which release they ship in. Merging them in a minor release goes against semver expectations.

5. AST layout change (DialectExtras()/EnsureExtras()).

• Moving fields behind optional pointers saves memory, which matches the claimed −34.7% B/op. It is an API-breaking move for anyone reading the old fields directly. The PR says "the new AST fields are additions", so check that no existing exported field moved. The perf section says "AST dump identical apart from moved field paths", which suggests some did.
• Pooled nodes must clear and nil these pointers on release. Otherwise stale Extras leak between reuses. Please confirm the Release* paths reset them and that a pool-reuse test covers it.

6. Transpiler refusal behaviour is good.

• Returning errors for GO scripts and for operators with no target equivalent (^, @, |/, money literals) is the safe choice over silently producing wrong SQL.
• The known gap is that FormatAST with a different target dialect doesn't apply the ^ precedence logic. That can produce semantically different SQL (power vs XOR precedence), so consider returning an error there instead of leaving it as a LOW item.

7. Dropped-word check compares word sets, not counts (known LOW). A formatter that drops one of several repeated words (for example one AND among several) would pass. Switching to a multiset comparison is cheap and would make the "keeps every word" guarantee real.

8. Process.

• 106 commits and about 49k lines in one PR is hard to review. The description's plan to keep commits reviewable is sensible, but a large share of the diff is generated or testdata (known_failures.txt alone is about −1,300 lines).
• The performance claims (time −6.1%, memory −34.7%) come from the author's M4 laptop. performance_baselines.json is the CI gate, so check CI for regressions in FullPipeline/delete (+5.2%) and update (+3.3%) under the repo's tolerance.

Summary

The code I read (batch, tokscratch) is clean, well documented and defensively written. The main risks are the borrowed-token-buffer lifetime contract, pointer-reset on pooled nodes, and the breaking-change release strategy. I can't vouch for the parser, formatter and transpiler changes, which are the bulk of the PR. A reviewer who knows those areas should go through them, with the corpus ratchet and task test:race as the backstop.

[github-actions]

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

---

Review of PR #867

Scope: I read internal/batch/batch.go and internal/tokscratch/tokscratch.go in full. I did not read the other ~98 changed files (parser, formatter, transpiler, AST). I did not run tests, linters or benchmarks. The pass/fail claims in the PR description are unverified.

Findings

1. tokscratch calls a nil impl with no guard (low). tokscratch.go:43-65

• impl stays nil until pkg/sql/tokenizer's init registers it. Bytes, String and StringContext call it directly.
• A package that imports tokscratch without linking tokenizer gets a bare nil-func panic. A check like if impl == nil { panic("tokscratch: tokenizer not linked") } would give a clearer message.
• Inside the registered func, the t any → *tokenizer.Tokenizer assertion should use the two-value form and fail with a clear message. I did not read that code, so check it.

2. Borrowed token buffer lifetime (medium, design).

• The contract says the slice is valid only until the next tokenize call, Reset or PutTokenizer. A retained slice would be silently corrupted, with no panic, and only -race or a targeted test would catch it.
• The doc says token strings are input substrings, so retained ASTs should be safe.
• Please make sure a test parses twice on the same pooled tokenizer, keeps the first AST, and compares it after the second parse, under -race. concurrent_retained_ast_test.go looks aimed at this; I did not read it.
• Check that every tokscratch caller copies the tokens before they escape. That includes the LSP and any caching path.

3. batch.Line GO detection (batch.go:58-79).

• The shape checks look sound: an unquoted identifier with EqualFold "GO", a single line, width 2, and the previous token ending on an earlier line.
• Count rejects non-digits and an Atoi overflow, so GO 99999999999999999999 is invalid.
• The "alone on its line" rule depends on comments not being tokens. Please confirm tests exist for these inputs:
  • /* c */ GO, where the previous token may be on an earlier line and the comment is invisible.
  • GO -- c.
  • CRLF line endings.
  • A multi-line string or comment ending just before GO.
  • GO as the first token (i == 0).
• Line does not require Start.Column == 1, so GO indented, or GO after a leading block comment on its own line, is accepted. That is the same as sqlcmd, but it is worth a test.
• The PR already lists GO 2 /* c */ as splitter and parser disagreeing. That cuts against the "SplitStatements agrees with the parser" claim. Please track it in an issue.

4. Breaking changes and versioning.

• Breaking changes in this PR:
  • EXPLAIN now returns *ast.ExplainStatement.
  • gosqlx.Lint / LintWithDialect now return an error for invalid SQL.
  • Raw NUL / Ctrl-Z in a string literal is now an error.
  • PIVOT/UNPIVOT IN syntax is stricter.
  • format --no-uppercase now lower-cases keywords.
• The CHANGELOG should say which release carries these. Shipping them in a minor release goes against semver expectations. At the least, call them out under a "Breaking" heading.

5. AST layout (DialectExtras() / EnsureExtras()).

• The description says the new fields are additions, and the perf section says the AST dump is identical "apart from moved field paths". Those two statements disagree on whether existing exported fields moved. If any did, that is an API break for direct field readers and needs a CHANGELOG entry.
• Pooled nodes must nil the extras/modifiers pointers in their Release* paths. Otherwise stale dialect data leaks between reuses. Please confirm a pool-reuse test covers this: parse a dialect-specific statement, release it, then parse a plain one and assert the extras are nil.

6. Transpiler.

• Refusing to translate GO scripts and operators with no target equivalent is the right call.
• FormatAST to a different target dialect not applying the ^ precedence logic can silently change semantics. Power and XOR bind differently, so I'd return an error there rather than leave it as a LOW item.

7. Dropped-word check compares word sets, not counts. A formatter that drops one of several repeated tokens, such as one AND, would pass. A multiset comparison is cheap and would make the "keeps every word" guarantee real.

8. Process and performance.

• The diff is very large (the header says 100 files, but the diff against the base is ~324 files). The commit-by-commit review approach is reasonable.
• The benchmarks come from one M4 machine. Check CI against performance_baselines.json for FullPipeline/delete (+5.2%) and update (+3.3%).

Summary

The two small files I read are clean, documented and defensive. The main risks are the borrowed-buffer lifetime, pointer reset on pooled nodes, the "moved field paths" ambiguity, and how the breaking changes ship. The parser, formatter and transpiler changes are the bulk of the PR and still need a reviewer who knows those areas, with the corpus ratchet and task test:race as the backstop.

[github-actions]

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

---

Review of PR #867

Scope: I read internal/batch/batch.go and internal/tokscratch/tokscratch.go in full. I did not read the rest of the diff (parser, formatter, transpiler, AST, CLI). I ran no tests, linters or benchmarks, so the pass/fail claims in the description are unverified. The earlier automated reviews covered the same two files, so most points below repeat theirs, with a few corrections.

Findings

1. tokscratch nil impl (low). internal/tokscratch/tokscratch.go:43-65

• impl is nil until pkg/sql/tokenizer's init calls Register. Bytes, String and StringContext call it with no check.
• A package that imports tokscratch without linking tokenizer gets a bare nil-func panic. A one-line guard with a clear message would help.
• I did not read the registered func, so I can't say whether its t any assertion uses the two-value form. Check that it does.

2. Borrowed token buffer lifetime (medium).

• The slice and its Word values are valid only until the next tokenize call, Reset or PutTokenizer. A retained slice would be corrupted silently.
• The package doc says callers must copy and never return the slice to a user. Please confirm that every caller outside the parser's preprocess step honours this, especially LSP and any caching path.
• pkg/gosqlx/concurrent_retained_ast_test.go looks like the right test. I did not read it. It should parse twice on the same pooled tokenizer, keep the first AST, compare it after the second parse, and run under -race.

3. batch.Line (batch.go:58-79).

• The shape checks are sound: unquoted identifier equal to "GO" (case-insensitive), single line, width 2, previous token ends on an earlier line.
• Count rejects non-digits and overflow, so GO 99999999999999999999 is invalid.
• The "alone on its line" rule relies on comments not being tokens. If they aren't, /* c */ GO is accepted. The same goes for an indented GO. That matches sqlcmd, but it should have tests. internal/batch/batch_test.go is only 48 lines, so check that it covers:
  • GO after a comment on the same line
  • GO -- c
  • CRLF line endings
  • GO as the first token
  • GO 0 and GO 1.5
• The line-75 check rejects GO followed by a token on the same line. GO 2 /* c */ is already listed as a known gap, and I'd track it in an issue. It cuts against the claim that SplitStatements agrees with the parser.

4. Breaking changes and versioning.

• Breaking changes in this PR:
  • EXPLAIN now returns *ast.ExplainStatement.
  • Lint and LintWithDialect now return an error for invalid SQL.
  • Raw NUL and Ctrl-Z in string literals are now errors.
  • PIVOT/UNPIVOT IN syntax is stricter.
  • format --no-uppercase now lower-cases keywords.
• Make sure the CHANGELOG has a clear "Breaking" section naming the target release. Shipping these in a minor release goes against semver expectations.

5. AST layout.

• The description says the new fields are additions. The perf section says the AST dump is identical "apart from moved field paths". If any existing exported field moved behind DialectExtras() or DialectModifiers(), that is an API break and needs a CHANGELOG entry.
• Every Release* path for pooled nodes must nil the extras and modifiers pointers. I'd want a pool-reuse test: parse a dialect-specific statement, release it, parse a plain one, and assert the extras are nil.

6. Transpiler. Refusing GO scripts and operators with no target equivalent is the safe choice. FormatAST to a different dialect not applying the ^ precedence logic can change semantics silently, because power and XOR bind differently. I'd return an error there and not leave it as a LOW item.

7. Dropped-word check. It compares word sets, not counts. A formatter that drops one of several repeated words, such as one AND, would pass. A multiset comparison is cheap and would make the "keeps every word" guarantee real.

8. Process and performance.

• The PR header says 100 files, but the diff against the base is much larger. The commit-by-commit review plan is reasonable.
• The benchmarks come from one M4 laptop. Check CI against performance_baselines.json for FullPipeline/delete (+5.2%) and update (+3.3%).

Summary

The two small files I read are clean and documented. The main risks are the borrowed-buffer lifetime contract, pointer reset on pooled nodes, the "moved field paths" ambiguity, and how the breaking changes ship. The parser, formatter and transpiler changes are the bulk of the PR and still need a reviewer who knows those areas. The corpus ratchet and task test:race are the backstop.

[ajitpratap0]

Pre-merge review and fixes

Four independent reviews ran over the whole stack before merging: parser/tokenizer, AST/rendering, formatter/transform/APIs, and security/DoS. Every HIGH finding, and every MEDIUM that main doesn't have, is fixed on this branch:

• Formatter comment layer (c6834dac)
  • Optimizer-hint placement was quadratic: 140 KB of hints took 4.3 s and 2.6 GB. It now takes 13 ms.
  • Comment lookup was O(comments × statements). It is now a binary search.
  • Comments are now matched to the parser's statement start positions (new AST.StatementStarts). A procedure or trigger body, or a T-SQL statement without ;, can no longer attach a comment to the wrong statement. If the positions disagree, the comments go to the top as before, never next to the wrong statement.
• Splitter (81eee98c)
  • An unterminated Oracle q'… made splitting quadratic: 240 KB took 1.7 s, now 10 ms.
  • MySQL DELIMITER tokens are capped at 32 bytes.
• Parser (19b36aad)
  • Outside SQL Server, a utility statement (SET, USE, GRANT, COPY, …) without ; swallowed the next line's statement, hiding it from walkers. That is now an error.
  • Transaction statements are checked against a per-dialect grammar.
  • <=> is accepted only in MySQL, MariaDB and PostgreSQL. == is accepted only in SQLite and ClickHouse.
  • An unbalanced T-SQL BEGIN/END, and a bare IF or WHILE, are errors.
• AST (32d76349)
  • MERGE keeps the source's column alias list.
  • Children() now reaches CREATE TABLE and materialized-view ENGINE clauses, CLONE … AT, partition bounds, SELECT TOP and ARRAY JOIN. A reflection test over the corpus enforces this.
  • AST.SQL() no longer drops sequence statements.
  • DDL release helpers honour the release mode.
• Small items: parse excludes GO lines from its statement count. CHANGELOG notes cover the fingerprint change for quoted identifiers, --no-uppercase being partial, and the stricter utility-statement and transaction rules.

Verification: the security review fuzzed about 813k mutated inputs and found no panics. Every scanner payload that main caught (93 payloads × 9 dialects) is still caught. Parser benchmarks are flat. task check and task test:cbinding pass.

Known limitations, documented in the CHANGELOG:

• A missing ; after a statement whose body can hold statements (CREATE TRIGGER/FUNCTION, PREPARE … AS) still folds the next statement in.
• A second statement on the same line as GRANT or COPY is not split off.
• Pre-existing and unchanged: the transform LIMIT rules ignore TOP/FETCH; splitter gaps with MySQL backslash escapes and parenthesized CREATE RULE bodies.

[github-actions]

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

---

Review of PR #867

Scope: This is a targeted review, not a full one. I read internal/tokscratch/tokscratch.go and checked how widely the dialect extras are used in pkg/sql/ast. I did not read the parser, formatter or transpiler diffs, and I ran no tests or benchmarks. The PR's pass/fail claims are unverified. Earlier automated reviews covered the same files, so I only repeat what still matters.

Findings

1. tokscratch calls impl without a nil check (low). internal/tokscratch/tokscratch.go:43-65

• impl stays nil until pkg/sql/tokenizer's init registers it. Bytes, String and StringContext call it directly.
• Any package that imports tokscratch without linking tokenizer gets a bare nil-func panic. A guard like if impl == nil { panic("tokscratch: tokenizer not linked") } gives a clearer message.
• Check that the registered func asserts t.(*tokenizer.Tokenizer) with the two-value form. CLAUDE.md requires that style, and I did not read that code.

2. Borrowed token buffer (medium, design).

• The returned slice is valid only until the next tokenize call, Reset or PutTokenizer. A caller that retains it gets silent corruption.
• Confirm that no caller outside the parser's preprocess step retains it, especially LSP and any cache. pkg/gosqlx/concurrent_retained_ast_test.go looks like the right safety net. It should parse twice on the same pooled tokenizer, keep the first AST, compare it after the second parse, and run under -race.

3. Pooled nodes and the new optional pointers.

• There are about 150 EnsureExtras / DialectExtras uses in pkg/sql/ast. Every Release* path must nil the extras and modifiers pointers.
• Please add or confirm a pool-reuse test. It should parse a dialect-specific statement, release it, parse a plain statement from the same pool, and assert the extras are nil.
• The PR says the new fields are additions, but the perf section mentions "moved field paths". If any existing exported field moved, that is an API break and needs a CHANGELOG entry.

4. batch.Line edge cases (internal/batch/batch.go).

• internal/batch/batch_test.go is only 48 lines. Check it covers:
  • /* c */ GO
  • GO -- c
  • CRLF line endings
  • GO as the first token
  • GO 0 and GO 1.5
  • GO after a multi-line string
• GO 2 /* c */ is handled differently by the splitter and the parser, which undercuts "SplitStatements agrees with the parser". Track it in an issue.

5. Transpiler FormatAST with ^ precedence. FormatAST with a different target dialect does not apply the ^ precedence logic. Power and XOR bind differently, so the output can silently change meaning. Returning an error there would be safer than leaving it as a LOW item.

6. Dropped-word check. It compares word sets, not counts. A formatter that drops one of several repeated words, such as one AND, would pass. A multiset comparison is cheap and makes the "keeps every word" guarantee real.

7. Breaking changes and versioning.

• Breaking changes in this PR:
  • EXPLAIN now returns *ast.ExplainStatement.
  • Lint and LintWithDialect now return an error for invalid SQL.
  • Raw NUL and Ctrl-Z in string literals are now errors.
  • PIVOT/UNPIVOT IN syntax is stricter.
  • format --no-uppercase now lower-cases keywords.
  • Utility statements without ; are now errors outside SQL Server.
• The CHANGELOG should give these a "Breaking" heading and name the target release. Shipping them in a minor release goes against semver expectations.

8. Performance. The numbers come from one M4 machine. Check CI against performance_baselines.json for FullPipeline/delete (+5.2%) and update (+3.3%).

Summary

The code I read is clean and well documented. The pre-merge fuzzing, the security review and the corpus ratchet reduce risk. Before merge, confirm the pointer-reset test (3), the buffer-lifetime test (2) and the CHANGELOG release target (7). The parser, formatter and transpiler changes still need a reviewer who knows those areas.

[github-actions]

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

---

Review of PR #867

Scope: This is a targeted review. I read internal/tokscratch/tokscratch.go, internal/batch/batch_test.go and the registration site in pkg/sql/tokenizer/scratch.go. I did not read the parser, formatter, transpiler or AST diffs. I ran no tests, linters or benchmarks, so the pass/fail claims in the description are unverified.

Corrections to earlier automated reviews

• The t any assertion is already safe. pkg/sql/tokenizer/scratch.go:30 uses t, ok := tv.(*Tokenizer), which is the two-value form CLAUDE.md requires.
• batch_test.go is more complete than earlier reviews implied. It covers GO, go 3 -- repeat, [GO], "GO", 'GO', GO 1.5, GO 0, GO SELECT 1, and tokens without spans.

Findings

1. tokscratch has no nil guard on impl (low). internal/tokscratch/tokscratch.go:50-65

• impl is nil until pkg/sql/tokenizer's init registers it. A package that imports tokscratch without linking tokenizer gets a bare nil-func panic.
• A one-line if impl == nil { panic("tokscratch: tokenizer not linked") } would give a clearer message. This is optional, since only internal packages can import it.

2. Borrowed token buffer lifetime (medium, design).

• The returned slice is valid only until the next tokenize call, Reset or PutTokenizer. A retained slice is silently corrupted. The doc comment states the contract well.
• pkg/gosqlx/concurrent_retained_ast_test.go looks like the right safety net. Please confirm it keeps the first AST, parses again on the same pooled tokenizer, compares the first AST afterwards, and runs under -race.
• Please confirm no caller outside the parser's preprocess step retains the slice, especially the LSP and any caching path.

3. batch.Line edge-case tests (low).

• The existing tests cover the main shapes. They do not cover these cases:
  • /* c */ GO and an indented GO. These are accepted if comments are not tokens. That matches sqlcmd, but it should be pinned by a test.
  • CRLF line endings.
  • GO after a multi-line string or comment.
• GO 2 /* c */ is handled differently by the splitter and the parser. That conflicts with the "SplitStatements agrees with the parser" goal for this input, so please track it in an issue.

4. Pooled nodes and the optional pointers (medium).

• There are about 150 EnsureExtras / DialectExtras uses in pkg/sql/ast. Every Release* path must nil the extras and modifiers pointers, or stale dialect data leaks between pooled reuses.
• Please add or confirm a pool-reuse test. It should parse a dialect-specific statement, release it, parse a plain one, and assert the extras are nil.
• The description says "new fields are additions", but the perf section says "moved field paths". If any existing exported field moved, that is an API break and needs a CHANGELOG entry.

5. Transpiler FormatAST with ^ precedence (low-medium). FormatAST with a different target dialect skips the ^ precedence logic. Power and XOR bind differently, so this can silently change meaning. Returning an error would be safer than leaving it as a known LOW item.

6. Dropped-word check (low). It compares word sets, not counts. A formatter that drops one of several repeated words, such as one AND, would pass. A multiset comparison is cheap and makes the "keeps every word" guarantee real.

7. Breaking changes and release target.

• Breaking changes in this PR:
  • EXPLAIN now returns *ast.ExplainStatement.
  • Lint and LintWithDialect now return errors for invalid SQL.
  • Raw NUL and Ctrl-Z in string literals are now errors.
  • PIVOT/UNPIVOT IN syntax is stricter.
  • format --no-uppercase now lower-cases keywords.
  • Utility statements without ; are now errors outside SQL Server.
  • <=> and == are now restricted to specific dialects.
• Please make sure the CHANGELOG has an explicit "Breaking" heading that names the target release. Shipping these in a minor release goes against semver expectations.

8. Performance. The numbers come from one M4 machine. Check CI against performance_baselines.json for FullPipeline/delete (+5.2%) and update (+3.3%).

Summary

The code I read is clean and well documented. The pre-merge fuzzing, the security review and the corpus ratchet reduce risk. Before merging, confirm the pooled-node reset test (4), the buffer-lifetime test (2) and the CHANGELOG release target (7). The parser, formatter and transpiler changes still need a reviewer who knows those areas.