[ajitpratap0]

Summary

Audit 2,788 production-style SQL statements across eight dialects and preserve their parse/format round trips with zero listed corpus failures. Retain SQL Server GO batch boundaries and repeat counts, so formatting a procedure, view or trigger followed by another batch does not absorb that batch into the object definition.

Complete the render and performance reviews: fix raw JSON_TABLE/XMLTABLE literal quoting and dialect-sensitive nested hint placement; verify scratch-buffer ownership with concurrent retained ASTs; align the audit harness with the corpus detector. Keep SQL Server transpilation batch-aware, reject GO scripts targeting other dialects, and prevent CLI validation from accepting invalid GO counts through its splitter fallback. Update the unreleased changelog and dialect compatibility results.

Test Plan

• task check: PASS, including fmt, vet, lint (0 issues) and race tests.
• task test:cbinding: PASS with CGO and race detection.
• All eight dialect harness runs: 2,788/2,788 pass; known_failures.txt unchanged.
• Before/after perf output snapshot: all 2,788 FormatAST strings byte-identical between b609a11 and 78362f5.
• Prefix truncation: 266,281 byte prefixes, zero parse/render panics, both immutable merged HEAD and final source.
• Independent source review: no remaining actionable findings.
• Integrate main 326fdd1: preserve BigQuery PIVOT/UNPIVOT alongside Oracle XML, dynamic IN lists and source aliases; keep one owner per aggregate expression. Both parent test suites and mixed-feature round trips pass under race detection.
• Re-run task check, task test:cbinding, documentation drift checks and the full 266,281-prefix sweep after base integration: all pass, zero panics. Three interleaved before/after benchmark rounds show unchanged synthetic allocation counts and effectively stable corpus allocations.
• TestPerformanceRegression: PASS; performance baseline thresholds unchanged (metadata matches main).
• Five interleaved benchmark rounds: fixed 2,725-statement corpus parse bytes -62.2% and parse+format bytes -56.6% versus 6c81d7c; median times -9.2% and -7.9%.
• Synthetic parser latency versus origin/main 5502fc3 regresses 24.4% (SimpleSelect) and 29.5% (ComplexSelect). AST growth explains the extra allocation bytes; this change does not claim a general latency improvement.
• UNION scaling guard: native and race-plus-atomic-coverage runs pass; a recursive prefix-copying negative control fails the allocation budget at 1,024 UNIONs. The full 80,000-UNION exact-output assertion is retained, with a separate size-scaled benchmark.
• Remote CI at 0db001a: 43 checks PASS, including Vercel; two package-publishing jobs skipped as expected for a PR.

Breaking Changes

• With no dialect selected, unknown backslash escapes now return an error instead of silently changing the string value.
• SQL Server AST.Statements and recovery results now include BatchSeparator nodes for GO; clients enumerating statements must account for them.
• Transpiling a GO script to a target other than SQL Server now returns an error because its batch scopes and repeat counts cannot be represented there.
• Rare SELECT dialect fields move behind optional SelectStatement.Extras. Those fields were introduced only on this unreleased branch; handle nil on reads and allocate before writes.
• Additive AST literal prefix/escape and identifier quote metadata preserves source meaning and target-dialect transpilation.

Known low-priority follow-ups remain: deep (>100-branch) T-SQL IF/ELSE-IF chains, malformed-block parser/splitter fallback differences, and possible further AST layout optimization. No database execution-equivalence claim is made from the syntax/round-trip corpus.

[github-actions]

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

---

Review of PR #858

Scope: The merge base now resolves (326fdd1), but at this size I spot-checked rather than read the whole diff. I read internal/batch/batch.go, internal/batch/batch_test.go and internal/tokscratch/tokscratch.go in full, and I looked at where batch.End is called. I did not run tests or lint, and I did not review the formatter, parser, CLI or splitter bodies. This is not a sign-off.

Resolved from earlier bot reviews

• Trailing comments: batch_test.go has a "go 3 -- repeat\nSELECT 1" case that expects a separator. So GO -- comment is covered, and the tokenizer evidently drops comments from the token stream. That earlier concern needs no action.
• Case: End uses EqualFold, so lowercase go is accepted, and the test covers it.

internal/batch/batch.go

• Count, IsText and End are tight, and the validation is sound. Quoted, string, fractional, zero and trailing-word forms are all rejected.
• Suggestion: add test cases for GO;, GO /* x */, GO 5 -- c, and GO at end of input with no newline. The existing table doesn't include them.
• Minor: End requires End.Column-Start.Column == 2. If the tokenizer ever reports byte columns for lines with leading multibyte characters, that is still fine, because GO itself is ASCII. No action needed.
• Minor: IsText uses strings.Fields. That means GO\t5 and GO 5 both match, which is the intended behavior.
• Possible gap: End only checks that the previous token's end line is less than GO's line. A multi-line block comment ending on the same line as GO (/* c */ GO) is invisible if comments are dropped, so that form would be treated as a separator. SSMS also treats it as a separator only in some versions, so I'd add a test that pins the behavior you want.

internal/tokscratch/tokscratch.go

• The registration pattern is reasonable and the ownership contract is clearly documented.
• Robustness: impl is nil unless pkg/sql/tokenizer is linked in, and t any is asserted inside the registered function. Both fail with a panic rather than an error. All callers are internal, so this is low risk. A one-line nil guard that returns an error would give a clearer failure for test binaries that import it indirectly.
• Register is unsynchronized, which is fine while it runs only from init.
• Risk to keep an eye on: the "valid until next tokenize/Reset/Put" contract is the main hazard. scratch_tokens_test.go and concurrent_retained_ast_test.go target it directly. Make sure the AST never retains Word values that point into the scratch buffer. The PR notes the preprocess step copies them.

PR-level

• Breaking changes: the five listed changes are documented. A short CHANGELOG.md migration snippet for the SelectStatement.Extras nil check (read: nil check, write: allocate first) would help consumers.
• Performance: the 24–29% synthetic parse latency regression versus main is disclosed. Please file a tracking issue for the AST layout follow-up so it isn't lost. The same goes for deep T-SQL IF/ELSE chains and the splitter/parser fallback differences.
• Size: about 50k added lines in 84 commits is hard to review. Splitting the corpus/harness, formatter and parser work would help next time.
• Testing: the described coverage is strong (2,788-statement corpus, prefix truncation, race and cbinding runs). I did not verify those results. Remote CI, including Vercel, should be green before merging.

[github-actions]

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

---

Review of PR #858 (spot-check, not a sign-off)

The merge base resolves this time (286f2c9). The diff is 408 files and about 53k added lines, so I didn't read all of it. I read internal/batch/batch.go and internal/tokscratch/tokscratch.go in full. I did not run tests or lint. I did not read the formatter, parser, CLI or splitter bodies.

internal/batch/batch.go

• Validation: Count and Line split "shape of a separator" from "valid count". That is what lets CLI validation reject GO 0 and GO 1.5 instead of falling back to a plain identifier. This is sound.
• Same-line prefix: /* c */ GO is only rejected if comments appear in the token stream. The earlier bot reviews concluded the tokenizer drops comments, so /* c */ GO would likely be accepted as a separator. sqlcmd behavior here varies, so add a test that pins the behavior you want.
• Missing test cases: I'd add tests for GO;, GO 5 -- c, GO at end of input with no newline, and GO immediately after a multi-line block comment.
• Column check: End.Column-Start.Column != 2 assumes columns count the same units for both ends. That holds because GO is ASCII. The check is fine.
• IsText: it is case-insensitive and uses strings.Fields, so GO\t5 matches. Make sure the formatter output goes through it consistently.

internal/tokscratch/tokscratch.go

• Pattern: the registration pattern avoids the import cycle, and the borrowed-buffer contract is well documented.
• Nil impl: any binary that links tokscratch without pkg/sql/tokenizer panics with a nil func call. A guard that returns an error is cheap.
• t any: a wrong type panics inside the registered function. A comma-ok assertion there would return a clear error.
• Missing context check: Bytes and String take no context, so no guard is needed. The StringContext pre-check is fine.
• Main hazard: an AST retaining Word values that alias the scratch buffer. The doc says the parser's preprocess step copies them. concurrent_retained_ast_test.go targets this directly, which is the right test. If you haven't already, run it under -race with a pool size of 1 to maximize buffer reuse.

PR-level

• Breaking changes: the five listed changes are documented. The SelectStatement.Extras change (nil-check on reads, allocate before writes) deserves a short migration snippet in CHANGELOG.md. So does the BatchSeparator nodes appearing in AST.Statements.
• Performance: the 24–29% synthetic latency regression versus main is disclosed. Open a tracking issue for the AST layout work so it isn't lost. The deep T-SQL IF/ELSE chains and the splitter/parser fallback differences should get issues too.
• Size: the PR is very hard to review as one unit. Splitting the corpus/harness, formatter and parser changes would help next time.
• Testing: the described coverage is strong (2,788-statement corpus, 266k prefix truncation, race and cbinding runs). I didn't verify these results. Confirm remote CI is green after the latest sync before merging.

[ajitpratap0]

This PR is split into a stack of three for review:

1. test(realworld): add multi-dialect real-world corpus with ratchet #866: real-world corpus, ratchet test and harness, based on main. 1,327 known failures on main.
2. fix(dialect): parse, format and transpile the real-world corpus in all 8 dialects #867: all dialect, render, quoting and transpile fixes plus the perf work, stacked on test(realworld): add multi-dialect real-world corpus with ratchet #866. Known failures drop to 0.
3. fix(parser)!: treat a GO line inside a SQL Server statement as an error #868: the one breaking parser rule (a GO line inside a SQL Server statement is an error), stacked on fix(dialect): parse, format and transpile the real-world corpus in all 8 dialects #867.

With all three merged, the code is identical to this PR's head c8bd83b7; only the CHANGELOG wording differs, and it corrects the escape note: main already rejected unknown backslash escapes. This PR can be closed once the stack is accepted.

🤖 Generated with Claude Code

[ajitpratap0]

Superseded by the stack #866 → #867 → #868, all merged into main (15ffeec, e3e01c5, 953138e). The main tree matches the reviewed stack.