[ajitpratap0]

Summary

Follow-ups from the pre-merge review of #866–#868:

• format --no-uppercase (KeywordCase: KeywordLower) now lower-cases every keyword. Before, AND, IN, IS NULL, DESC, CASE, BETWEEN, data types and others stayed upper case. Identifiers, quoted names and literals are untouched. Rendering is linear in expression depth: 32,000 chained terms take 0.09 s. Raw JSON_TABLE/XMLTABLE clause text keeps its source case, as documented in the flag help and the CHANGELOG.
• CRLF input: comment text no longer carries \r into \n output.
• GO lines: in SQL Server scripts without ;, comments before or after a GO line stay on that side of it.
• MySQL STRAIGHT_JOIN … ON a = b, t3 no longer renders a space before the comma.
• transform.AddTableAlias renames qualifiers in SELECT, scope-aware like the UPDATE/DELETE path: FROM users WHERE users.id = 1 becomes FROM users u WHERE u.id = 1.
• SetLimit / SetOffset / RemoveLimit / RemoveOffset handle SQL Server TOP and FETCH FIRST/NEXT. They no longer emit LIMIT alongside TOP or FETCH. PERCENT and WITH TIES are rejected with an error rather than guessed at.

Test Plan

• gofmt, go vet, golangci-lint run (0 issues), task test:race, task test:cbinding
• Default (upper-case) output is byte-identical over the formatter corpus snapshot: 251 entries
• New tests: complete keyword case, a ratio-based linearity test for lower-case rendering, comment placement around GO and with CRLF, STRAIGHT_JOIN spacing, SELECT alias qualifiers, TOP/FETCH pagination
• Manual CLI checks of the cases above

Breaking Changes

• format --no-uppercase output changes: it is now fully lower case.
• SetLimit on a TOP or FETCH query now edits that clause instead of adding LIMIT. With PERCENT or WITH TIES it returns an error.

🤖 Generated with Claude Code

https://claude.ai/code/session_01DnbRnTFErhoMJdAjVEeCg2

[vercel]

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

Project	Deployment	Actions	Updated
gosqlx-website	Ready	Preview	Oct 27, 2026 7:26pm UTC

[github-actions]

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

---

Review of PR #870

I read the pkg/transform and pkg/formatter/comments.go diffs in full. I did not read the new lower-case renderer (render_case_helpers.go, render_expression_case.go, render.go) or the tests, and I did not run the test suite. Treat the findings below as partial.

Looks good

• checkPaginationModifiers runs before any mutation, so a rejected PERCENT or WITH TIES leaves the AST untouched.
• RemoveLimit keeps a legacy Fetch.OffsetValue by moving it to sel.Offset before it clears Fetch. That avoids silently dropping the offset.
• normalizeCommentText is applied in both JoinWithComments and joinHoisted. It trims the \r on -- comments before converting any remaining CR or CRLF.
• Reusing dmlAliasTargets for SELECT alias rewriting keeps SELECT consistent with UPDATE/DELETE.

Points to consider

1. Undocumented behavior change in AddTableAlias. Previously a SELECT that named the same table twice (a self join) got the alias on every match, silently. The new code goes through refs.check(), and the doc comment now says same-scope duplicates are an error. That is the better behavior, but the "Breaking Changes" section and the CHANGELOG should list it.
2. pkg/transform/limit.go, SetLimit on TOP. It replaces Top.Count with an INTEGER literal. If the original count was a parenthesized expression or a variable (TOP (@n)), it is overwritten. That seems right for "set the limit", but a test pinning it would document the choice.
3. SetOffset. It returns cannot combine OFFSET with TOP, but RemoveOffset has no matching check. That is harmless, since removing an offset from a TOP query is a no-op. A short comment would help.
4. normalizeCommentText. Converting a lone \r to \n inside block comments rewrites comment content. This is rare (old Mac line endings) and is the sensible choice, but it deserves a line in the CHANGELOG.
5. SetLimit on FETCH. It clears FetchExpr and sets FetchValue. If the FETCH has no ORDER BY, SQL Server rejects it, but that was already true of the input. Nothing to change.

Tests

• The coverage listed in the PR description is thorough: keyword case, a ratio-based linearity test, CRLF, GO placement, STRAIGHT_JOIN and TOP/FETCH.
• I suggest adding two cases:
  • a SetOffset / RemoveOffset round-trip on a FETCH ... OFFSET query;
  • the self-join error for SELECT AddTableAlias.
• The ratio-based timing test could be flaky under -race on CI. Make sure it either skips under the race detector (as the performance baselines do) or uses a generous ratio.

I found no security concerns. The changes are pure AST and text transforms.

[github-actions]

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

---

Review of PR #870

I read pkg/transform/limit.go and the first part of pkg/formatter/expression_scaling_test.go. I did not read the new lower-case renderer (render_case_helpers.go, render_expression_case.go, render.go) or tables_alias_select.go this time, and I did not run the tests. Treat this as a partial review. It adds to the earlier one.

Looks good

• checkPaginationModifiers runs before any mutation, so a rejected PERCENT or WITH TIES leaves the AST untouched.
• RemoveLimit moves a legacy Fetch.OffsetValue into sel.Offset before clearing Fetch, so the offset isn't dropped.
• SetOffset and RemoveOffset clear Fetch.OffsetValue, so the old offset can't be rendered next to the new one.
• The linearity test builds the ASTs outside the timed section. It also asserts the exact output before timing, so it can't pass on wrong output.

Points to consider

1. Timing test and -race.
   • TestFormatASTExpressionLinearGrowth has no skip for the race detector. The 64x bound against an expected ~16x is generous. Even so, a ratio of single timings can be flaky on a shared runner, and -race plus GC pauses add noise.
   • Taking the minimum of a few runs inside expressionFormatDuration would make it more stable. I couldn't see its body in the part I read, so it may already do this.
   • The commit message mentions "20ms samples". If that is what the helper does, this may already be handled.
2. SetLimit on TOP (limit.go).
   • It overwrites Top.Count with an INTEGER literal. A TOP (@n) or parenthesized expression is lost.
   • That's probably the intended meaning of "set the limit". A test or a doc line would pin it down.
3. SetOffset vs RemoveOffset on TOP.
   • SetOffset errors with cannot combine OFFSET with TOP. RemoveOffset is a silent no-op, which is correct.
   • A one-line comment would explain the asymmetry.
4. SetLimit on FETCH. It leaves Fetch.OffsetValue alone, so the offset is kept. That's right. A SetLimit, SetOffset, RemoveLimit round-trip test on a FETCH ... OFFSET query would lock it in.
5. Self-join behavior change (from the earlier review, not re-checked).
   • AddTableAlias on a SELECT that names the same table twice now errors instead of aliasing both.
   • Add that to the "Breaking Changes" section and the CHANGELOG.
   • Add a test for it.
6. CHANGELOG. Mention that a lone \r inside block comments is now converted to \n.

I found no security concerns. The changes are AST and text transforms only.

[github-actions]

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

---

Review of PR #870

I read the lower-case renderer (render_case_helpers.go, render_expression_case.go) in full, and I read the pkg/transform and comment changes in the earlier passes. I did not run the tests or the linter. Treat the findings as a code read only.

Looks good

• The slot approach hides operands and names from lowerSyntax, so identifiers, quoted names and literals stay untouched. Slots expand once at the root, which gives linear rendering.
• newExpressionCase picks a slot prefix that doesn't collide with identifiers in the source, so user text such as gosqlx_case_slot_0_ can't be mistaken for a slot.
• restore is iterative with an explicit stack, so deep nesting won't overflow the Go stack.
• lowerDataType leaves qualified (user-defined) types alone.
• checkPaginationModifiers runs before any mutation, so a rejected PERCENT or WITH TIES leaves the AST untouched.
• RemoveLimit moves a legacy Fetch.OffsetValue into sel.Offset before clearing Fetch.
• normalizeCommentText is applied on both the join and hoist paths.

Points to consider

1. lowerSyntax quote handling (render_case_helpers.go).
   • It treats [ as a delimiter. [ is also the array subscript in PostgreSQL (arr[1]), but operands are slotted before this runs, so it is likely safe.
   • It also skips \ escapes inside '...', whereas standard SQL doesn't escape that way. A literal like 'a\' is not an issue because literals are normally slotted. A test with a backslash-ending literal inside CASE would confirm this.
2. Hand-maintained keyword lists.
   • builtinTypeWord and builtinFunctionName are fixed lists. Any type or function missing from them keeps its upper-case spelling under --no-uppercase, for example GEOMETRY, TINYTEXT, NOW.
   • Where expressionKeywords.IsKeyword doesn't cover it, that is silent partial upper-casing. This is not a bug, but you could document it or add a test that pins the known gaps.
3. Branches that bypass slot casing.
   • VariantPath returns copy.SQL() and ClauseArgument returns copy.SQL(). The latter is documented as raw text.
   • VariantPath and ArrayConstructorExpression keep upper-case keywords unless the shared case-aware code covers them. A lower-case test for ARRAY(SELECT …) and Snowflake paths would show this.
   • AnyExpression/AllExpression call exprSQL(c.expr(...)), which goes through the non-case helper. That works because the operand is a slot.
4. CollateExpression. It uses strings.Replace(rendered, " COLLATE ", " collate ", 1). If the operand's rendering contains COLLATE it is not the first match, but operands are slotted so this holds. It does depend on SQL() spelling, so a test would guard it.
5. AddTableAlias self-join (from the earlier reviews).
   • A SELECT that names the same table twice in one scope now errors instead of aliasing both.
   • Add this to "Breaking Changes" and the CHANGELOG, and add a test.
6. SetLimit on TOP. It replaces Top.Count, so TOP (@n) becomes a literal. A test or doc line would pin this down.
7. SetOffset vs RemoveOffset on TOP. SetOffset errors and RemoveOffset is a no-op. A one-line comment would explain it.
8. FETCH ... OFFSET round trip. Add a SetLimit, SetOffset, RemoveLimit round-trip test.
9. Timing test. TestFormatASTExpressionLinearGrowth has a generous 64x bound against ~16x expected. Taking the minimum over several samples, or skipping under -race, would reduce flakiness. The commit message mentions 20ms samples, so this may already be handled.
10. CHANGELOG. Note that a lone \r inside block comments is now converted to \n.

I found no security concerns. The changes are AST and text transforms only.