[ajitpratap0]

Summary

Part 1 of 4 splitting #858 into topical PRs. This one adds only tests and tooling; no library code changes.

• Corpus: testdata/realworld/<dialect>/ holds 2,788 production-style statements across PostgreSQL, MySQL, MariaDB, SQL Server, Oracle, SQLite, Snowflake and ClickHouse.
• Ratchet test: TestRealWorldCorpus runs every statement through split → parse → format → reparse → idempotency → dropped-word check, against testdata/realworld/known_failures.txt. A new failure fails the test, and so does a listed failure that now passes. Regenerate the list with go test ./pkg/gosqlx -run TestRealWorldCorpus -update-known-failures.
• Baseline on main: 1,327 of the 2,788 statements fail. 1,114 don't parse, 160 lose words when formatted, 48 don't re-parse and 5 aren't idempotent. Per dialect: ClickHouse 182, MariaDB 176, MySQL 200, Oracle 211, PostgreSQL 132, Snowflake 146, SQLite 121, SQL Server 159. The later PRs in this series shrink the list to 0.
• Harness: go run ./scripts/dialect-audit -dialect <d> file.sql runs the same checks on any SQL file and prints a summary to stderr.
• Shared detector: internal/dropcheck holds the dropped-word detector used by the harness and the test.
• Benchmarks: BenchmarkRealWorldParse and BenchmarkRealWorldParseFormat measure the whole corpus.
• Formatter walker: TestFormatAST_ReadableSemicolonOnlyTerminatesStatements walks all of testdata/. It found 6 corpus statements on main whose ; placement is wrong, so it skips testdata/realworld until the fix PR removes the skip.

Test Plan

• task check (fmt, vet, golangci-lint, race tests): exit 0
• TestRealWorldCorpus passes with the regenerated list
• go run ./scripts/dialect-audit -dialect mysql testdata/realworld/mysql/*.sql → 405 statements, 205 pass
• .gitattributes already forces LF for *.sql, so the byte-exact checks also hold on Windows CI

Breaking Changes

None.

🤖 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 9:57pm UTC

[github-actions]

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

---

Review of PR #866: real-world corpus and ratchet test

This is a tests-and-tooling PR. I read realworld_corpus_test.go, dropcheck.go and scripts/dialect-audit/main.go. I did not run the tests, and I did not read the other changed files, including the 2,788 corpus statements.

The ratchet is a good design. A new failure fails the test, and so does a listed failure that now passes. Stale entries and a changed failure stage are also caught, and the panic recovery is useful. Nothing blocks the merge. These are the points I'd fix or consider:

1. Orphaned comment (realworld_corpus_test.go:49-52)
The comment says the "shared read-only stand-ins returned by DialectExtras/DialectModifiers accessors must still be empty". Nothing in the test checks that. It looks like a leftover from another change and misleads readers. Remove it, or add the check.

2. Dropped-word detector (dropcheck.go)

• ignorable contains AS, ASC, INNER and OUTER. The detector therefore can't notice the formatter dropping those words. That is fine for noise, but OUTER and INNER are ignored in every context. It's worth a short comment saying so.
• DroppedWords only checks that each input word appears somewhere in the output. A clause that was dropped but whose words occur elsewhere, such as WHERE x vs HAVING x, goes undetected. The comment says "cheap detector", so this is acceptable. Say that it can miss drops so nobody relies on it as proof.
• In Words, the '\'' case treats \ as an escape inside string literals. That is wrong for standard SQL, PostgreSQL and SQLite, where 'C:\' is a complete string. Such a string would swallow following text and could hide dropped words. A per-dialect flag, or MySQL-only handling, would be more precise.
• Double-quoted and backtick identifiers are scanned as plain text. That is intended, since the comment says quoted identifiers keep their text. Bracketed [name] identifiers (SQL Server) work the same way.
• A $ followed by a digit, as in $1, is correctly not treated as a dollar quote. A $ that is not a dollar quote falls through to i++, which is fine.
• The doc comments are partly duplicated, and CanonicalSynonyms's comment has a typo: "the synonyms of synonyms in s". Fix it.
• CanonicalSynonyms also rewrites matches inside string literals and comments. Words skips those regions, so a literal like 'ISNULL' is rewritten, then skipped, and nothing breaks. It is still wasteful and a little fragile.

3. Known-failures parser (loadKnownFailures)

• The strings.IndexByte(line, '#') == 0 check is the only comment test. Keys like file.sql#3 contain #, so that is correct, and the " #" trailing-comment handling is fine too.
• Malformed lines (len(fields) != 2) are silently skipped. A typo in an entry then makes that statement fail as "unexpected" instead of pointing at the bad line. Make it t.Errorf so a bad entry shows up.
• Duplicate keys are not detected either.

4. Test mechanics

• The statement index in a key (file.sql#N) changes whenever someone inserts or removes a statement in a corpus file. This will cause churn across the fix PRs. It's acceptable, but document it in the header of known_failures.txt.
• filepath.Glob errors and an empty file list are ignored. If a dialect directory is empty, the test passes silently.
• -update-known-failures is defined with flag.Bool in a _test.go file. That works, but it registers the flag for the whole gosqlx test binary. The name is specific, so this is fine.
• Update mode never fails on a bad dialect directory, but it does write the file, and a partial run would overwrite the list. That is unlikely.
• compact truncates with s[:240], which can cut a multi-byte UTF-8 rune in half. Use rune-aware truncation. oneLine in dialect-audit (300 bytes) has the same problem.

5. scripts/dialect-audit/main.go

• check() duplicates realworldCheck. The two copies have already diverged in details such as the error text. Consider moving the pipeline into internal/dropcheck or a small shared package. The PR already shares the detector, so this would finish the job.
• stripComments and onlyComments are two copies of the same helper. Neither handles /* */ comments, so a block-comment-only statement is counted. The corpus test and the CLI both handle comments only with --.
• The -dialect value is not validated, while the test calls keywords.IsValidDialect. An unknown dialect currently runs with whatever fallback the parser applies.
• The help text says "emit JSON lines for every failure". Passing statements are never emitted in JSON mode, even with -v. Document this.

6. Corpus size and CI cost
TestRealWorldCorpus parses and formats 2,788 statements twice each. This should be fast. Check the runtime under -race, because CLAUDE.md requires a 60s timeout.

7. Benchmarks
I did not read realworld_bench_test.go. Make sure the corpus loading happens before b.ResetTimer() and that benchmark results are released through the pool APIs the project requires.

8. Process

• The walker in pkg/formatter/nested_semicolon_test.go skips testdata/realworld until a later PR. Please add a TODO(#NNN) comment linking the follow-up, so the skip can't be forgotten.
• The CHANGELOG entry is present. Good.
• I can't verify the PR's claim that task check passes. Please confirm that CI is green.

Summary

Solid and well-scoped. The one clear issue is the orphaned comment in the test. The backslash-escape handling in Words, the rune-unsafe truncation and the duplicated pipeline code would be cheap to fix now. The malformed-line handling in the known-failures loader is also worth tightening.

[github-actions]

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

---

Review of PR #866 (updated for the latest push)

I re-read realworld_corpus_test.go and internal/dropcheck/dropcheck.go at the current head. I did not run the tests, and I did not read the other changed files, including the corpus SQL.

Several points from the earlier review are already fixed:

• The orphaned comment is gone.
• compact now truncates by rune.
• Malformed and duplicate known_failures.txt entries now call t.Errorf.
• The header of known_failures.txt now says that indexes shift when statements are added or removed.

Still open

1. Backslash escape in Words (dropcheck.go:98). \ is treated as an escape inside every '...' literal. In standard SQL, PostgreSQL and SQLite, 'C:\' is a complete string. The scanner then swallows the text after it, and a dropped word there goes unnoticed. This only produces false negatives, so it doesn't block the merge. Consider gating it on MySQL, MariaDB and ClickHouse, or accept it and add a comment.
2. Comment typo (dropcheck.go:43): "rewrites the synonyms of synonyms in s". It should read "rewrites the synonym spellings in s". The doc comments on DroppedWords and the package also repeat each other, and the synonyms comment wraps oddly.
3. ignorable (dropcheck.go:75) hides dropped AS, ASC, INNER and OUTER in every context. A short comment saying so would help. DroppedWords also only checks that each word appears somewhere in the output. It misses a dropped clause whose words occur elsewhere, so the docs should say it is not proof of fidelity.
4. filepath.Glob result ignored (realworld_corpus_test.go:62). If a dialect directory is empty or the glob fails, that dialect passes silently. A t.Errorf when len(files) == 0 would catch it.
5. Update mode can overwrite the list after a partial run. Under -update-known-failures, a t.Fatalf on a read error stops the run before the write, so this only matters if an entire dialect directory is missing. Low risk.
6. onlyComments handles only -- comments, not /* */. The same helper is duplicated in scripts/dialect-audit, along with check(), which duplicates realworldCheck. Moving the pipeline into a shared package would stop the two copies from diverging.
7. Walker skip in pkg/formatter/nested_semicolon_test.go. Add a TODO(#NNN) pointing to the follow-up PR that removes it.
8. -race runtime. Please confirm that TestRealWorldCorpus stays well under the 60s timeout with -race. I haven't run it.

Summary: Nothing blocks the merge. Items 4 and 7 are cheap and worth doing before merge. The rest can be follow-ups.