[ajitpratap0]

Summary

Constructs that failed to parse in the post-merge audit of #866–#868. Each now parses, renders through SQL() and the formatter, is reachable from Children(), is released by the pools, and round-trips.

• Snowflake PIVOT (… FOR c IN (ANY ORDER BY c) DEFAULT ON NULL (0))
• ClickHouse FROM t [AS x] FINAL SAMPLE 0.1 [OFFSET 1/2], on the first table and on joined tables. SAMPLE accepts decimal numbers and fractions only.
• SQL Server INSERT TOP (n) [PERCENT] [INTO] t …
• PostgreSQL identity columns combined with other constraints in any order. DEFAULT, identity and a generation expression on the same column are now rejected, as PostgreSQL does.
• MySQL/MariaDB ALTER TABLE partition actions: PARTITION BY, ADD (including IF NOT EXISTS, PARTITIONS n, MariaDB HISTORY/CURRENT), DROP/TRUNCATE (names, ALL, IF EXISTS), REORGANIZE, COALESCE, EXCHANGE … WITH TABLE [WITH|WITHOUT VALIDATION], ANALYZE/CHECK/OPTIMIZE/REBUILD/REPAIR, DISCARD/IMPORT TABLESPACE, REMOVE/UPGRADE PARTITIONING. Forms the structured grammar doesn't cover fall back to the as-written parse, as on main.
• PostgreSQL ROWS FROM (f(), g()) [WITH ORDINALITY] AS r(…), ARRAY[…][i], slices, and GROUPING SETS ((a), ROLLUP(b), CUBE(a,b), (), GROUPING SETS (…))
• A minus sign stays attached in type and identity options: INCREMENT -1, ClickHouse Enum8('b' = -2).

Test Plan

• gofmt, go vet, golangci-lint run (0 issues), task test:race, task test:cbinding; TestRealWorldCorpus green with new 90_review_gaps.sql statements for 6 dialects
• Round-trip, reachability and release tests for every construct
• ALTER TABLE regression check: 3,897 dialect×statement cases from testdata and test files, compared against main. Nothing main parsed is now rejected, apart from intentionally invalid identity combinations.
• Independent review: about 330 variants round-tripped, every byte prefix of 233 cases parsed without panic, and dialect gating checked (final, sample, rows still work as aliases)

Breaking Changes

• New AST fields only.
• A column with both DEFAULT and an identity or generation clause is now rejected.
• SAMPLE with a non-decimal size (e.g. hex) is rejected.
• SQL() now writes SAMPLE before ClickHouse ARRAY JOIN, where ClickHouse expects it.

🤖 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 28, 2026 4:35am UTC

[github-actions]

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

---

Review of PR #873

I read the core parser and AST changes. I did not run the test suite. The PR description lists race, cbinding, lint and corpus runs as green.

Overall: The change is well structured. The new parse paths are dialect-gated, and recursion in nested GROUPING SETS is bounded with p.depth. Hand-built SAMPLE ASTs are validated before rendering, so the renderer never writes a bare SAMPLE. The new AST nodes are covered by pool and release tests.

Notes

1. DEFAULT/identity exclusivity is applied to all dialects (pkg/sql/parser/ddl_columns.go, parseColumnDef)

   • The check lives in the shared parseColumnDef, so it applies to every dialect. It matches PostgreSQL, and as far as I know Oracle and MySQL as well, since all three reject DEFAULT together with an identity or generation clause.
   • It is still a breaking change for anyone whose existing SQL parsed before. The PR lists it under Breaking Changes, which is good. The CHANGELOG should say it too.
   • One loose end is dialect-specific syntax that sets the type string differently. For example, SQL Server IDENTITY(1,1) is not in isGeneratedColumnConstraint, so IDENTITY(1,1) DEFAULT … is still accepted. SQL Server rejects that too. This is a gap in the new rule, not a regression.

2. Sign handling in parseTypeArgsString (pkg/sql/parser/ddl_columns.go)

   • The new Minus/Plus case keeps the sign attached whenever a number follows. When a number precedes the sign, the output loses its spacing. For example, (1 - 2) becomes 1-2. That is probably harmless for type and identity options, but a test with a number before the sign would pin the behaviour.
   • If the sign is not followed by a number, the code falls through to writeRawLeaf. That is fine.

3. ROWS FROM identification (pkg/sql/parser/table_review_gaps.go)

   • isRowsFromStart correctly requires PostgreSQL plus ROWS followed by FROM. The WITH ORDINALITY check inspects both Quote and Word.QuoteStyle, which is more verbose than it needs to be. A small helper such as isUnquotedWord(tok, "ORDINALITY") would make it easier to reuse for the other WITH ORDINALITY sites.
   • parseRowsFrom builds a throwaway TableReference just to call parseTableColumnAliases. This works, but it is a bit indirect.

4. parsePivotDefault swallows nothing for non-Snowflake (pkg/sql/parser/table_review_gaps.go)

   • If a non-Snowflake PIVOT … DEFAULT ON NULL is written, the function returns nil and the later "expected )" error comes from elsewhere. This is fine, but the resulting error message will be less specific than a "DEFAULT ON NULL is Snowflake-only" message.

5. SAMPLE rendering order change (pkg/sql/ast/sql.go, pkg/formatter/render.go)

   • This is a behavioural change, because SQL() now writes SAMPLE before ARRAY JOIN. It is documented and matches ClickHouse's grammar. The golden test was updated (sql_golden_test.go), so this is fine.

6. releaseAlterStatement ownership (pkg/sql/ast/pool.go)

   • It releases partition fields from Operations when that slice is non-empty, and otherwise from Operation. If both are populated with the same op, which a parser could produce by setting Operation = Operations[0], the first branch handles it once. If they point to different ops, the singular one is leaked rather than released. Please confirm the parser never sets them differently.

7. Test coverage looks strong. It includes round-trip, reachability, release and prefix-fuzz coverage, plus a regression comparison of 3,897 ALTER TABLE cases against main. One missing case is GROUPING SETS nested close to MaxRecursionDepth. A test that asserts the RecursionDepthLimitError path would exercise the new guard.

Verdict

No blocking issues found. The items above are small improvements and a few questions. I'd merge once the CHANGELOG mentions the DEFAULT/identity rejection.

[github-actions]

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

---

Review of PR #873

I read the diff for the parser, AST pooling, ddl_columns.go and the CHANGELOG. I did not run the test suite. The PR description lists race, cbinding, lint and corpus runs as green.

Overall: The change is solid. The new parse paths are dialect-gated, and the new AST nodes are covered by round-trip, reachability and release tests. I found no blocking issues.

Notes

1. DEFAULT/identity exclusivity applies to all dialects (pkg/sql/parser/ddl_columns.go, parseColumnDef)

   • The CHANGELOG entry under "Changed" already covers it, so the earlier request is met.
   • isGeneratedColumnConstraint lists only the GENERATED … types. Dialect-specific forms such as SQL Server IDENTITY(1,1), MySQL AUTO_INCREMENT and SQLite AUTOINCREMENT are not counted, so IDENTITY(1,1) DEFAULT 0 is still accepted. This is a gap in the new rule, not a regression. A follow-up issue would be enough.
   • Duplicate GENERATED … AS IDENTITY clauses are rejected. Repeated plain DEFAULT clauses are not rejected by this switch. That is probably unchanged from main, but please confirm.

2. Sign handling in parseTypeArgsString

   • The new Minus/Plus case attaches the sign only when the next token is a number. For (1 - 2), prevWasIdent is false after the first number, so the output may lose its spacing and become 1-2. That is harmless for type and identity options. A test with a number before the sign (e.g. DECIMAL(10, -2), INCREMENT 5 -1) would pin the behaviour.

3. releaseAlterStatement (pkg/sql/ast/pool.go)

   • Release uses Operations when it is non-empty and otherwise falls back to Operation. If a parser path ever sets both to different ops, the singular op's partition fields would not be released. This does not leak memory, because the Go GC still frees the values. It only skips pool reuse. A short comment that the two fields are mutually exclusive, or releasing both with an identity check, would remove the ambiguity.

4. Minor cleanups (pkg/sql/parser/table_review_gaps.go)

   • The ORDINALITY check inspects both Quote and Word.QuoteStyle. A small helper such as isUnquotedWord(tok, "ORDINALITY") would reduce repetition.
   • parseRowsFrom builds a throwaway TableReference only to call parseTableColumnAliases.

5. Error message for non-Snowflake PIVOT … DEFAULT ON NULL

   • parsePivotDefault returns nil outside Snowflake, so the user sees a generic "expected )" error. This is fine, but a dialect-specific message would be friendlier.

6. SAMPLE before ARRAY JOIN

   • This is a deliberate rendering-order change. It matches ClickHouse's grammar, and the golden test and CHANGELOG are updated.

7. Tests

   • Coverage is strong: round-trip, reachability, release, prefix-fuzz, and the 3,897-case ALTER TABLE regression comparison.
   • One gap: no test drives GROUPING SETS nested past MaxRecursionDepth to assert the RecursionDepthLimitError path of the new p.depth guard.

Verdict

I would merge this. The items above are optional follow-ups.

[github-actions]

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

---

Review of PR #873

I read the diff for ddl_columns.go, pool.go and grouping.go. I did not run the tests. The PR description lists race, cbinding, lint and corpus runs as green. Two earlier reviews on this PR already covered the rest, and I agree with their verdict.

Overall: I found no blocking issues. The change is well scoped, the new paths are dialect-gated, and the tests cover round-trip, reachability and release.

Notes

1. DEFAULT/identity exclusivity (ddl_columns.go, parseColumnDef)

   • The check is correct and the error messages are clear.
   • isGeneratedColumnConstraint lists only the GENERATED … types. SQL Server IDENTITY(1,1), MySQL AUTO_INCREMENT and SQLite AUTOINCREMENT are not counted, so those can still be combined with DEFAULT. That is a gap in the rule, not a regression. A follow-up issue would cover it.
   • Whether repeated plain DEFAULT clauses are rejected is unchanged from main.

2. Sign handling in parseTypeArgsString

   • A sign attaches to the next token only when that token is a number. That works for INCREMENT -1 and Enum8('b' = -2).
   • A test with a number before the sign would pin the spacing, for example INCREMENT 5 -1 or DECIMAL(10, -2). A sign that follows a number may lose its spacing. This is unlikely to matter for type and identity options.

3. Nested GROUPING SETS (grouping.go)

   • The p.depth guard is correct and the defer restores the depth on every path.
   • No test drives nesting past MaxRecursionDepth to assert the RecursionDepthLimitError. One small test would cover the new guard.
   • A nested GROUPING SETS is stored as a single element of set. The comment says its sets are concatenated into the enclosing list. Please check that SQL() and the formatter re-render it as GROUPING SETS (…), not flattened. The round-trip tests should show this.

4. releaseAlterStatement (pool.go)

   • It releases from Operations when that slice is non-empty and otherwise from Operation. If a parser path set both to different ops, the singular op would skip pool reuse. The GC still frees it, so nothing leaks.
   • A one-line comment saying the two fields are mutually exclusive would help.

5. Minor, table_review_gaps.go

   • An isUnquotedWord(tok, "ORDINALITY") helper would shorten the ORDINALITY check.
   • parsePivotDefault returns nil outside Snowflake, so the user gets a generic "expected )" error. A dialect-specific message would be friendlier.

Verdict

I would merge this. The items above are optional follow-ups.