Repository navigation
feat(cli): add --dialect flag to parse, format, analyze, lint - #526
Conversation
The validate command already accepted a --dialect flag, but parse, format, analyze, and lint were hard-wired to the default (PostgreSQL) dialect and rejected dialect-specific SQL such as MySQL's `LIMIT offset, count`. Add a --dialect flag to each of the four commands, validate the value up front (unknown names are rejected), and thread it into the tokenizer and parser at the point each command tokenizes/parses. Shared helpers live in cmd_utils.go: validateDialectName, plus tokenizerForDialect and parserForDialect. For an explicit dialect a fresh, non-pooled tokenizer is created rather than reconfiguring a pooled one, because tokenizer.Reset does not clear dialect/keyword state and a pooled dialect-configured tokenizer would leak that state to later callers. The linter package gains an exported SetDialect method so lint can tokenize/parse dialect-specific syntax, and lint --security now parses with the selected dialect via gosqlx.ParseWithDialect. The default remains PostgreSQL, so behavior without --dialect is unchanged. Fixes: ajitpratap0#523
|
@mem is attempting to deploy a commit to the coolajitpratapsingh0-9932's projects Team on Vercel. A member of the Team first needs to authorize it. |
There was a problem hiding this comment.
Coverage across all 4 commands: parse, format, analyze, and lint all get a --dialect flag, and all four route it the same way, through validateDialectName first (fails fast before touching the input), then through either tokenizerForDialect (parse/analyze) or a parser built with parser.WithDialect (format via FormatterOptionsFromConfig, lint via linter.SetDialect). Consistent pattern, easy to follow.
Pool safety, this is the part I checked closest: tokenizerForDialect in cmd_utils.go only returns a pooled tokenizer for the empty/default dialect. Any explicit dialect gets a fresh tokenizer.NewWithDialect instance with a no-op release function. That matters because the tokenizer pool's Reset() doesn't clear dialect state (see PR #525), so putting a dialect-configured tokenizer back in the pool would leak MySQL/SQLite keyword state to whatever caller grabs it next. pkg/linter/linter.go's acquireTokenizerAndParser does the exact same thing. Good, this is the correct fix and it's applied consistently in both places that needed it.
Validation: validateDialectName in cmd_utils.go checks against keywords.IsValidDialect and returns a clear error listing all valid dialects if not. Ran it manually:
$ echo "SELECT 1" | gosqlx parse --dialect bogus
Error: unknown SQL dialect "bogus"; valid dialects: postgresql, mysql, mariadb, sqlserver, oracle, sqlite, snowflake, bigquery, redshift
Exits 1, error is clear, usage text prints alongside it. Good UX.
Backward compat: omitting --dialect keeps default at empty string, which every code path treats as "use PostgreSQL." Confirmed this with the MySQL-only LIMIT 10, 20 syntax:
$ echo "SELECT a, b FROM t LIMIT 10, 20" | gosqlx parse
Error: parsing failed: ... expected statement, got COMMA
$ echo "SELECT a, b FROM t LIMIT 10, 20" | gosqlx parse --dialect mysql
AST Structure: ... Token Count: 11
Fails under default, works under mysql. Exactly what you'd want, no accidental behavior change for existing callers.
Tests: dialect_flag_test.go and pkg/linter/dialect_test.go use that same MySQL LIMIT offset, count probe query across parse, format, analyze, and lint, each proving default-dialect failure and mysql-dialect success. That's a real behavioral test, not just checking the flag gets threaded through, it actually confirms dialect-specific parsing kicks in. TestValidateDialectName covers valid/invalid/empty cases. Good coverage overall.
What I didn't see tested: no test hits the actual CLI command layer (cobra RunE functions) with --dialect end to end, coverage is at the helper/option level (CLIParserOptions, CLIFormatterOptions, etc). Not blocking since the plumbing between the cobra flag and those options structs is trivial and I traced it by hand, but worth knowing if a future refactor moves that plumbing around without a CLI-level regression test to catch it.
Ran locally: go build ./..., go vet ./..., full go test on cmd/... and pkg/linter/... all pass. Smoke-tested the built binary manually on parse and lint with valid, invalid, and default dialect values as shown above, matches expected behavior.
This is well-scoped and the pool-leak fix is correctly reused from #525's pattern. Approving.
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
This pull request has been automatically marked as stale because it has not had activity for 60 days. It will be closed in 14 days unless there is new activity. |
ajitpratap0
left a comment
There was a problem hiding this comment.
Thanks for the contribution, and for threading --dialect through parse, format, analyze and lint. I reviewed head 6ea77ed.
Security and supply chain: nothing found
- The diff touches only
cmd/gosqlx/cmd/*,pkg/linter, andCHANGELOG.md. - There are no changes to
.github/,go.mod/go.sum, scripts or build tags. No newinit(), network, exec or file access was added. The tests are plain unit tests. pkg/sql/securityis not modified.
What I verified by building the branch and running it
- Dialect threading works for
parse,formatandanalyze, with inline SQL, stdin and file input:SELECT a, b FROM t LIMIT 10, 20fails under the default dialect with E2002.- The same query succeeds with
--dialect mysql.
- Dialect gating holds:
QUALIFYis rejected under PostgreSQL and accepted under Snowflake.- An unknown dialect is rejected up front with a clear error.
lint --security --dialectparses with the selected dialect, andLinter.SetDialectis wired into the tokenize and parse step.- Pooling is handled correctly:
- The default path still uses the pooled tokenizer, returned with
defer. - A dialect-configured tokenizer is never returned to the pool, which is right because
Reset()does not clear keyword or dialect state. - The extra
p.Release()in the tokenize-error branch ofLintStringis correct.
- The default path still uses the pooled tokenizer, returned with
go vetis clean.go test -racepasses forpkg/linter/...,cmd/gosqlx/...,pkg/compatibilityandpkg/gosqlx.- Default behaviour for existing callers is unchanged. The new
Dialectoption fields andLinter.SetDialectare additive.
Required changes: none
Optional suggestions
- In
pkg/linter/linter.go, theSetDialectdoc mentions the MySQL/SQLite?placeholder. In this tree?fails to parse in every dialect, includingvalidate --dialect mysql, so please drop that example. TheLIMIT offset, countexample is accurate. validateDialectNameduplicates the inline check and error string invalidate.go. Consider havingvalidate.gocall the helper, or building the valid-dialect list fromkeywords.AllDialects(). The help text lists 7 dialects while the error message lists 9.- Consider adding a test at the cobra command level covering the unknown-dialect rejection and the flag reaching the options structs for each command, plus one for
lint --security --dialect. I checked these by hand and they work, but the current tests would not catch a wiring regression. - For non-default dialects a fresh tokenizer is built per
LintStringcall. That is fine for a CLI. It would be worth a doc note thatSetDialectis not safe to call concurrently with linting.
Maintainer note, out of scope for this PR
- The CLI's existing SQL formatter silently drops clauses it cannot print. Examples with the PR binary:
format --dialect sqlserver 'SELECT TOP 5 [a] FROM [t]'losesTOP 5.format --dialect snowflakelosesQUALIFY.format --dialect mysqllosesON DUPLICATE KEY UPDATE.
- With the default dialect,
ON CONFLICTandFOR UPDATEare dropped too, anda::intbecomesCAST. - This predates the PR and is not caused by it. The new flag makes more of it reachable, especially with
format -i, so it deserves a separate issue.
Overall this looks good to merge once you've considered the suggestions above.
Automated review standing in for the claude-review CI check, which cannot run on pull requests from forks.
# Conflicts: # CHANGELOG.md
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
) The second merge commit in PR #526 (b8a1924, "Merge remote-tracking branch 'origin/main' into HEAD") resolved a conflict by truncating CHANGELOG.md to zero bytes, dropping all 1,825 lines of release history. Nothing since has touched the file, so main has shipped with an empty changelog since 2026-10-10. Restore the file to its last good state (acc3d23, which already carries the #525 "Fixed" entry) and re-add the "Added" entry for the --dialect CLI flag that PR #526's own feature commit (4d75b95) contributed but the merge lost. Entries for the perf PRs merged since (#568-#578, #581) are deliberately left for the consolidated changelog/baselines follow-up tracked in #566. Claude-Session: https://claude.ai/code/session_01UZSk1292jPc8ipDboNxeve Co-authored-by: Ajit Pratap Singh <ajitpratapsingh@Ajits-Mac-mini-2655.local> Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
…resh performance baselines (#566) CHANGELOG [Unreleased] now covers every PR merged since v1.14.0, with a Behavior changes section for the observable changes in #597, #595, #584, #585, #588, #581, #582/#570, #517/#518 and #511, plus Added, Changed, Fixed, Performance, Security and Internal entries. The two existing entries (#526 and the tokenizer-pool dialect fix) are kept verbatim. performance_baselines.json is re-measured on an Apple M4 with BenchmarkPerformanceBaseline -count=10 -benchmem (median). Baselines are set at about 2.5x the M4 median rather than 1.5x: the CI Benchmark job on ubuntu-latest measures these queries about 1.5-3.2x slower than M4 (last 9 Tests runs on main, 0715bb9..61ca6c8), so 1.5x with the unchanged tolerances would fail CI's slowest runs. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UZSk1292jPc8ipDboNxeve
…xes; refresh performance baselines (#566) (#598) * docs: consolidate CHANGELOG for the perf audit and batch-2 fixes; refresh performance baselines (#566) CHANGELOG [Unreleased] now covers every PR merged since v1.14.0, with a Behavior changes section for the observable changes in #597, #595, #584, #585, #588, #581, #582/#570, #517/#518 and #511, plus Added, Changed, Fixed, Performance, Security and Internal entries. The two existing entries (#526 and the tokenizer-pool dialect fix) are kept verbatim. performance_baselines.json is re-measured on an Apple M4 with BenchmarkPerformanceBaseline -count=10 -benchmem (median). Baselines are set at about 2.5x the M4 median rather than 1.5x: the CI Benchmark job on ubuntu-latest measures these queries about 1.5-3.2x slower than M4 (last 9 Tests runs on main, 0715bb9..61ca6c8), so 1.5x with the unchanged tolerances would fail CI's slowest runs. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UZSk1292jPc8ipDboNxeve * docs(changelog): add batch-3 follow-up fixes (#599-#620) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UZSk1292jPc8ipDboNxeve * docs(changelog): record #632 render-injection hardening and #624 quote guards Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UZSk1292jPc8ipDboNxeve * docs(changelog): correct batch-3 entries after review; record measured costs - #628: SQLite still rejects count(*) [cnt]; only SQL Server keeps it. - #631: bare FORMAT / FORMAT <name> rules apply after SETTINGS. - Performance: record the measured costs of the batch-3 correctness fixes (Identifier and TableReference growth, quoting in SQL(), lineStarts reallocation, sorted Extract results). - #632: note the lıke, WITH CHECK OPTION and MINUS rendering changes. - Fix the [Unreleased] compare link (v1.14.0...HEAD) and add [1.14.0]. - Keep each section a single tight list. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UZSk1292jPc8ipDboNxeve * docs(changelog): describe the release as source-compatible with runtime behaviour changes Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UZSk1292jPc8ipDboNxeve --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
The validate command already accepted a --dialect flag, but parse, format, analyze, and lint were hard-wired to the default (PostgreSQL) dialect and rejected dialect-specific SQL such as MySQL's
LIMIT offset, count.Add a --dialect flag to each of the four commands, validate the value up front (unknown names are rejected), and thread it into the tokenizer and parser at the point each command tokenizes/parses.
Shared helpers live in cmd_utils.go: validateDialectName, plus tokenizerForDialect and parserForDialect. For an explicit dialect a fresh, non-pooled tokenizer is created rather than reconfiguring a pooled one, because tokenizer.Reset does not clear dialect/keyword state and a pooled dialect-configured tokenizer would leak that state to later callers.
The linter package gains an exported SetDialect method so lint can tokenize/parse dialect-specific syntax, and lint --security now parses with the selected dialect via gosqlx.ParseWithDialect.
The default remains PostgreSQL, so behavior without --dialect is unchanged.
Fixes: #523
Description
Brief description of what this PR does.
Type of Change
Changes Made
Testing
go test ./...)go test -race ./...)go test -bench=. ./...)Test Coverage
Performance Impact
Benchmark Results (if applicable)
Checklist
Related Issues
Closes #(issue number)
Screenshots (if applicable)
Add any relevant screenshots here.
Additional Notes
Any additional information that reviewers should know.