[ajitpratap0]

Summary

Part 3 of 3 splitting #858. It stacks on #867. Once this is merged, the combined tree is exactly #858's head c8bd83b7; only the CHANGELOG wording differs.

sqlcmd ends a batch at any line holding only GO [n], wherever that line appears. On main the parser read such a line as an identifier. SELECT a,\nGO\nFROM t became the column list a, go and formatted as SELECT a, go FROM t, which sqlcmd would split somewhere else. With this PR, in SQL Server:

• A statement that runs across a GO [n] line fails with "GO batch separator inside a statement: a line holding only GO ends the batch".
• GO 0, GO 1.5 or an overflowing count alone on a line fails with "invalid GO repeat count", instead of a FROM / end-of-statement error from the statement before it.
• Recovery parsing reports the error and resumes at the GO line, so the next batch is still parsed.

The check is SQL Server only, and the 4 restored tests cover parsing, recovery and FormatWithDialect.

Test Plan

• task check: exit 0
• TestParser_GOLineInsideStatement, TestParser_RecoveryStopsAtGOLineInsideStatement, TestBatchSeparatorInvalidCountMessage, TestBatchSeparatorInsideStatementIsAnError pass
• TestRealWorldCorpus: 0 known failures

Breaking Changes

SQL Server input with a line holding only GO [n] inside a statement no longer parses. Main accepted it with go taken as a column or alias name. Other dialects are unchanged.

🤖 Generated with Claude Code

https://claude.ai/code/session_01DnbRnTFErhoMJdAjVEeCg2

[vercel]

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

1 Skipped Deployment

Project	Deployment	Actions	Updated
gosqlx-website	Ignored	Preview	Oct 26, 2026 10:33pm UTC

[github-actions]

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

---

Review of PR #868

• Read the diff and the internal/batch.Line helper it relies on

Verdict: looks good. I have a few non-blocking suggestions. I did not run the tests.

What I checked

• Detection: batchLineError has a cheap prefilter (identifier token, length 2). It then defers to batch.Line. batch.Line rejects quoted identifiers ([go], "go"), GO sharing a line with other tokens, and tokens without source positions. This keeps false positives low, and the same-line cases are covered by tests.
• Dialect gating: the check runs only under IsSQLServer(), both at the call sites and inside the helper. Other dialects are untouched, and the PostgreSQL case is tested.
• Cost: each statement is scanned once over its own token range, so the total work is linear in the input.
• Error path: when the statement parser failed, the scan includes currentPos. A GO 0 or GO 1.5 line therefore gives the "invalid GO repeat count" error instead of a misleading FROM or end-of-statement error. "alias position" (SELECT a\nGO 2\nFROM t) correctly reports a FROM error. GO ends the first batch there, so that's expected.
• Recovery: p.seek(at) followed by continue resumes at the GO token. The next iteration then parses it as a BatchSeparator, and the recovery test asserts this.

Suggestions (non-blocking)

1. Duplicated call-site logic: parser.go has the same start := p.currentPos … batchLineError block in parseTokens and parseContextTokens. A small helper such as p.parseStatementChecked() would keep the two from drifting. The // batchLineError returns err unchanged elsewhere comment also sits next to an IsSQLServer() guard that the helper already performs. One of the two checks is redundant.
2. Progress guarantee in recovery: p.seek(at) relies on at > stmtStartPos. If at equalled stmtStartPos, the loop would not advance. That should not happen, because a statement starting at a GO line is parsed as a BatchSeparator first. A short comment or an assertion would make the invariant explicit.
3. Test gaps:
   • Nested constructs spanning a GO line: BEGIN … GO … END, a CTE, and a subquery. sqlcmd splits those too, but it would be good to pin the behavior.
   • parseContextTokens: no test hits the context-aware path.
   • Recovery after a failed parse (err != nil) containing a GO line. Only the successful-parse branch is covered.
4. Changelog: the breaking change is documented under "Behavior changes". Consider also linking it to fix(dialect): complete real-world SQL audit and preserve round trips #858 and fix(dialect): parse, format and transpile the real-world corpus in all 8 dialects #867 for traceability.

No security or performance concerns.

[github-actions]

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

---

Review of PR #868

Verdict: looks good. I have a few non-blocking notes. I read the diff but did not run the tests.

What I checked

• Detection: batchLineError (pkg/sql/parser/statement_boundary.go) prefilters on identifier tokens of length 2. It then defers to batch.Line, which keeps false positives low. The scan covers each statement's own token range once, so total work is linear.
• Dialect gating: the check runs only for SQL Server. Other dialects keep their old behavior.
• Error path: when the statement parser failed, the scan includes currentPos. GO 0 and GO 1.5 therefore get "invalid GO repeat count" in place of a misleading FROM error.
• Recovery: p.seek(at) and continue resume at the GO token, which is then parsed as a BatchSeparator.

Suggestions

1. Recovery misses the error branch (recovery.go): the GO check runs only when err == nil. If a statement fails and the failure is caused by a GO line (for example SELECT a,\nGO\nFROM t where the parse stops later), recovery reports the generic parse error. It then uses the usual sync logic, which may skip past the GO line and swallow the next batch. parseTokens handles this case, so recovery and the strict path now differ. Consider calling batchLineError(stmtStartPos, err) in both branches and seeking to the GO token when it returns one.
2. Dropped statement in recovery: when the GO error fires on a successfully parsed stmt, the code does continue without releasing or appending it. This is probably fine, since the AST nodes are garbage collected. If pooled nodes are expected to be released elsewhere, check that this path doesn't leak them.
3. Progress guarantee: p.seek(at) requires at > stmtStartPos, or the loop would not advance. That holds because a statement beginning at a GO line parses as a BatchSeparator first. A short comment stating the invariant would help.
4. Duplicated call sites: the startPos … batchLineError block appears twice, in parseTokens and parseContextTokens. The IsSQLServer() guard is also redundant, since the helper already checks it, and the inline comment next to it is confusing. A small helper would stop the two copies drifting apart.
5. Test gaps:
   • There is no test of parseContextTokens.
   • There is no test of recovery after a failed parse that contains a GO line (see item 1).
   • There is no test of nested constructs such as BEGIN … GO … END, CTEs and subqueries.
6. Changelog: consider linking fix(dialect): complete real-world SQL audit and preserve round trips #858 and fix(dialect): parse, format and transpile the real-world corpus in all 8 dialects #867 for traceability.

No security concerns. I don't expect a performance impact: it adds a linear scan, and only for SQL Server.

[github-actions]

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

---

Review of PR #868

I read the diff but did not run the tests.

Verdict: the design is sound. I have a few non-blocking suggestions, and one of them is worth fixing before merge.

What I checked

• Detection (statement_boundary.go): batchLineError prefilters on identifier tokens of length 2, then defers to batch.Line. Each statement's token range is scanned once, so the cost is linear.
• Dialect gating: the check is SQL Server only, so other dialects are unchanged.
• Error path: when the statement parser failed, the scan includes currentPos. GO 0 and GO 1.5 therefore get "invalid GO repeat count" in place of a misleading FROM error.

Suggestions

1. Recovery ignores the error branch (recovery.go:210). The GO check runs only when err == nil. parseTokens calls batchLineError(startPos, err) for failed parses too. Take SELECT a,\nGO 0\nFROM t. Recovery reports the generic parse error and uses its usual sync logic, which can skip past the GO line and swallow the next batch. Strict mode and recovery therefore disagree. Call batchLineError(stmtStartPos, err) in both branches, and when it returns an index, report the error and p.seek(at).
2. Progress invariant in recovery: p.seek(at) followed by continue only advances if at > stmtStartPos. That holds because a statement starting at a GO line parses as a BatchSeparator first. Add a one-line comment saying so, or guard with at > stmtStartPos, so a future change can't cause an infinite loop.
3. Duplicated call sites (parser.go, in both parseTokens and parseContextTokens):
   • The startPos … batchLineError block is copied twice. A small helper such as p.parseStatementChecked() would stop the copies drifting apart.
   • The if p.IsSQLServer() guard is redundant, because the helper already checks the dialect. The _, err = discard also hides the index. Drop the guard and the confusing inline comment.
4. Dropped statement in recovery: when the GO error fires after a successful parse, stmt is discarded without ast.ReleaseStatement or similar. This is a leak only if statements are pooled in this path. Check how the existing error branch handles it and match it.
5. Test gaps:
   • No test covers parseContextTokens.
   • No test covers recovery after a failed parse that contains a GO line (see item 1).
   • No test covers nested constructs (BEGIN … GO … END, a CTE, or a subquery). sqlcmd splits those too, so a test would pin the behavior.
6. Changelog: consider linking fix(dialect): complete real-world SQL audit and preserve round trips #858 and fix(dialect): parse, format and transpile the real-world corpus in all 8 dialects #867.

No security concerns. The performance cost is a linear scan on SQL Server only.