[ajitpratap0]

Summary

v1.17.0's correctness passes made transpiler.Transpile ~2.6x slower than v1.16.0 (recorded in the v1.17.0 CHANGELOG). This restores most of it with byte-identical behaviour.

• pkg/internal/reflplan (new, internal): per-reflect.Type traversal plans cached in a sync.Map. A least fixpoint marks which types can transitively hold an interface; plans list only those exported fields, in declaration order. Strings, numbers, models.Location, quote metadata etc. are never visited.
• Walkers use plans: ast.RetargetOperators, rules.PipesOrToKeywordOr, the ||→CONCAT rewrite and retargetStringLiterals. No reflect.Type.Field calls in the hot path.
• Pooled retarget scratch: seen-map and frame stack from a sync.Pool, dropped when over 4096 entries; frames are zeroed as they pop so pooled scratch never references a released AST.
• Source prefilters in Transpile (each documented with its proof): no | byte → the two || rules are left out; no ', ", $ and no non-ASCII byte → string-literal re-rendering is skipped; no ", `, [ and no non-ASCII byte → RestyleDataTypes is skipped. SetNameQuoteStyle always runs.
• Rule lists per dialect pair built once (sync.Once, bounded to the known dialects; unknown names fall back). RulesFor still returns a fresh slice.

Design came from a workflow of three independent approaches (cached plans, prefilters/fusion, typed walker) judged on measured prototypes; implemented with Codex, reviewed by go-reviewer (no blocking issues; its medium/low items are fixed here).

Benchmarks (Apple M4, -count=8, benchstat vs fda9a5ef, all p=0.000)

Benchmark	Before	After
Transpile (MySQL CREATE TABLE → PostgreSQL)	5.18 µs, 3135 B, 52 allocs	2.68 µs (−48%), 2762 B, 43 allocs
WideExpression/OR	306 µs, 335 KB	97 µs (−68%), 57 KB
WideExpression/AND	310 µs	100 µs (−68%)
WideExpression/||	193 µs	87 µs (−55%)
SetOpLimit	10.6 µs	4.6 µs (−57%)
SelectOps/mysql2pg, pg2mysql (new)	~48 / ~54 µs (measured in review)	26 / 32 µs

Test Plan

• Differential: Transpile output and error text identical to fda9a5ef on 955,476 cases (corpus statements × dialect pairs + edge cases)
• task check (fmt, vet, golangci-lint, full race suite) and task test:cbinding
• New tests: reflplan unit tests (cycles, mutual recursion, maps, zero-length arrays, embedded and unexported fields, 32-goroutine concurrent Of); AST coverage guard comparing plans with an independent reachability search; plan-driven vs naive walk over testdata/realworld; prefilter on/off equivalence across all 81 dialect pairs plus string-opener pins; rule-cache immutability; parallel Transpile under -race; scratch reset / popped-frame invariants; 490k chain followed by a small transpile
• Existing: TestTranspileDeepChains, >512 nesting error, TestRealWorldCorpus, pool hygiene

Breaking Changes

None. No public API change; pkg/internal/reflplan is internal. CHANGELOG entry will be added once the v1.17.0 release PR (#877) lands, to avoid conflicting with its CHANGELOG rewrite.

🤖 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 29, 2026 10:18am UTC

[github-actions]

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

---

Review of PR #889: cached reflection plans and source prefilters

Verdict: Approve in spirit, with a few non-blocking notes. I read the diff of the core files (reflplan.go, retarget_operators.go, transpiler.go, dialect_rules.go). I did not read every test file, and I did not run the test suite or benchmarks. The 955k-case differential test and the benchstat numbers in the PR description are unverified by me.

Done well

• reflplan: building plans with a least fixpoint is correct for cyclic and mutually recursive types. Plans are fully built before LoadOrStore publishes them. If two goroutines race on the same type, they publish equivalent plans, which is harmless.
• Rule cache: it is bounded to the nine known dialects, unknown names fall back to buildRules, and RulesFor still returns a fresh slice. The rs[:len(rs):len(rs)] guard against appending into the registry's backing array is kept.
• Prefilters: each is a superset test on source bytes, so a false positive only costs a walk. I checked the [ case: the parser produces "[]" array suffixes from a [ token (ddl_column_ext.go:246), so typeDelim covers it. The proof comments are good.
• Scratch pooling: zeroing frames as they pop and discarding oversized scratch are the right call.

Notes (non-blocking)

1. Panic path in RetargetOperators (retarget_operators.go, around lines 53-67).
   • sc.stack is only reassigned after walk returns. If walk panics, the deferred reset() sees sc.stack with length 0, so clear(sc.stack) does nothing. The backing array can still hold reflect.Values pointing into the AST, and the scratch goes back into the pool.
   • Fix: on the panic path, either don't return the scratch to the pool, or clear sc.stack[:cap(sc.stack)].
   • A panic is unlikely, but the comment promises pooled scratch never references a released AST.
2. clear(sc.stack) on early error return. That path (nesting error) clears only up to len, which is correct. Frames popped earlier were zeroed. This is fine, and a short comment saying so would help.
3. Seen-set semantics (walk doc comment).
   • Pruning interface-free pointers removes them from seen. The comment argues this can only cause extra visits. That holds only if no interface-free node aliases a rewrite-bearing node's address.
   • Struct-first-field aliasing is the classic case, where &s and &s.firstField share an address. The uintptr key is the pointer value only, not the type. Pruned pointers are never inserted, so they can't suppress a real visit. The risk is the reverse: a visited non-pruned pointer can mask a different-typed pointer to the same address.
   • That risk is pre-existing, and I think it is not made worse here. It's worth a regression test if you don't already have one.
4. Prefilter coverage depends on the parser. The "string literals only come from newStringLiteral" claim and the "openers contain ', ", $ or non-ASCII" claim are exactly the kind that rot when someone adds a new literal form, such as a new dialect-specific literal. Consider a guard test that fails when a new caller of newStringLiteral appears. Alternatively, run the on/off equivalence test over the full corpus in CI, not just the 81 dialect pairs.
5. scanSource cost. It is a single byte loop, which is cheap. It can't early-exit, but it is O(n) and negligible next to parsing.
6. Doc and housekeeping.
   • The CLAUDE.md dependency graph update (ast → … internal/reflplan) is accurate.
   • The CHANGELOG entry is deferred to after docs(release): prepare v1.17.0 #877, which makes sense. Please make sure it isn't forgotten.
   • reflplan.go's package comment is one dense paragraph. Splitting it would help readability.

Test coverage

The coverage looks strong: unit tests for cycles, maps and zero-length arrays, a concurrent Of, an AST coverage guard against an independent reachability search, prefilter on/off equivalence, and a parallel -race transpile. The only gap I see is the panic-path scratch hygiene in note 1.

Security

I found nothing concerning. The cache keys are reflect.Types and the rule cache is bounded, so callers can't grow either one.
· branch perf/transpile-reflplan

[github-actions]

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

---

Review of PR #889: cached reflection plans and source prefilters

I read the core diff again on this push (retarget_operators.go in full, plus the earlier pass over reflplan.go, transpiler.go and dialect_rules.go). I did not run the tests or benchmarks. The 955k-case differential and the benchstat numbers are the author's and I haven't checked them.

Verdict: No blocking issues. The design is sound and the invariants are documented. I have one small hygiene fix and a few suggestions.

What looks right

• reflplan: the least fixpoint handles cycles and mutual recursion. Plans are complete before they are published, so two goroutines racing on the same type just publish equivalent plans.
• Prefilters: each is a superset test on source bytes, so a false positive only costs a walk. The proof comments are good.
• Rule cache: it is bounded to the known dialects, unknown names fall back, and RulesFor still returns a fresh slice.
• Walker: using explicit heap frames and zeroing each frame as it pops keeps pooled scratch from referencing a released AST on the normal and error-return paths.

Findings

1. Pooled scratch can keep AST references if walk panics (retarget_operators.go, RetargetOperators and reset).
   • sc.stack = r.walk(...) is only assigned after walk returns. If it panics, sc.stack is still the old stack[:0].
   • The deferred reset() then runs clear(sc.stack) over length 0, which clears nothing. The backing array can still hold reflect.Values into the AST, and the scratch goes back to the pool.
   • Fix: clear the full capacity (clear(sc.stack[:cap(sc.stack)])). The cost is small because cap is limited to 4096. Alternatively, don't pool the scratch when unwinding from a panic.
   • This contradicts the stated "pooled scratch never references a released AST" guarantee. It is unlikely to occur in practice, and a one-line test would pin it.
2. Address-only seen key (walk doc comment). Keys are uintptr with no type. A visited node could mask a different-typed pointer at the same address, such as a struct and its first field. This predates the PR, and pruning doesn't make it worse because pruned pointers are never inserted. A small regression test with a first-field alias would document the behaviour. Keying on (type, ptr) would remove the problem entirely, at some cost.
3. Prefilter assumptions can go stale. The claims that string literals only come from newStringLiteral and that openers always contain ', ", $ or a non-ASCII byte will break silently when someone adds a new literal form. Options are a guard test that fails when a new caller of newStringLiteral appears, or running the on/off equivalence test over the full corpus in CI instead of only the 81 dialect pairs.
4. Housekeeping:
   • The reflplan package comment is one dense paragraph and would read better split up.
   • A short comment on the error-return path of reset would help. It clears only up to len, which is correct, since earlier frames were zeroed on pop.
   • Don't forget the CHANGELOG entry once docs(release): prepare v1.17.0 #877 lands.

Test coverage

Coverage is strong: the reflplan unit tests, the AST reachability guard, the plan versus naive walk comparison, prefilter equivalence, the parallel -race transpile and the deep-chain tests. The only gap I found is the panic-path scratch hygiene in finding 1.

Security

I found no concerns. The caches are keyed by reflect.Type or bounded to known dialects, so callers can't grow them.
· branch perf/transpile-reflplan

[github-actions]

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

---

Review of PR #889: cached reflection plans and source prefilters

I re-read retarget_operators.go at the current head (d09d316). I did not run the tests or benchmarks. The 955k-case differential and the benchstat numbers are the author's, and I haven't checked them.

Verdict: No blocking issues.

Previous finding is fixed

The panic-path scratch leak I raised earlier is resolved by d09d316. RetargetOperators now returns the scratch to the pool only after walk completes (completed = true). A panicking walk drops the scratch instead of pooling it, so no stale reflect.Values reach the pool. Skipping the pool on a panic is simpler than clearing the full capacity. The comment on reset also covers the early-error path.

Remaining suggestions (non-blocking)

1. Test for the panic path. The fix has no test of its own. A small test would stop it regressing. It could force a panic inside a walk, for example with a custom node, and check that the pool never hands back scratch holding AST references.
2. Address-only seen key. Keys are uintptr with no type, so a visited struct could mask a different-typed pointer to its first field. This predates the PR, and pruning doesn't make it worse. A first-field-alias regression test would document the behaviour.
3. Prefilter assumptions can go stale. The claims that string literals only come from newStringLiteral, and that openers contain ', ", $ or a non-ASCII byte, will break silently if someone adds a new literal form. A guard test on callers of newStringLiteral would catch that. Running the on/off equivalence test over the full corpus in CI would also work.
4. Housekeeping.
   • Split the dense reflplan package comment into a few paragraphs.
   • Add the CHANGELOG entry once docs(release): prepare v1.17.0 #877 lands.

Summary

• Design and correctness: the least-fixpoint plans, bounded rule cache and prefilter proofs all look sound.
• Tests: coverage is strong. The only gap I see is the panic path in suggestion 1.
• Security: the caches are keyed by reflect.Type or bounded to known dialects, so callers can't grow them.
  · branch perf/transpile-reflplan

[github-actions]

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

---

Review of PR #889: cached reflection plans and source prefilters

I re-read retarget_operators.go at the current head (43820a5). I did not run the tests or benchmarks. The 955k-case differential and the benchstat numbers are the author's, and I haven't checked them.

Verdict: No blocking issues.

Resolved since the last review

• Panic-path scratch leak: RetargetOperators pools the scratch only when completed is true (retarget_operators.go:61-71). A panicking walk drops it, so stale reflect.Values never reach the pool. The comment on reset covers the early-error path.
• CHANGELOG: the Unreleased entry from 43820a5 replaces the deferred note in the PR description.

Remaining suggestions (non-blocking)

1. Panic-path test. The completed guard has no test of its own. A test that forces a panic mid-walk and checks the scratch isn't re-pooled would pin it.
2. Address-only seen key. Keys are uintptr with no type, so a visited struct could mask a different-typed pointer to its first field. This predates the PR, and pruning doesn't make it worse. A first-field-alias regression test would document the behaviour.
3. Prefilter assumptions can go stale. The claims that string literals only come from newStringLiteral and that openers contain ', ", $ or a non-ASCII byte will break silently if someone adds a new literal form. A guard test on callers of newStringLiteral would catch that. Running the on/off equivalence test over the full corpus in CI would also work.
4. Docs. Split the dense reflplan package comment into a few paragraphs.

Summary

• Design and correctness: the least-fixpoint plans, bounded rule cache and prefilter proofs look sound.
• Tests: coverage is strong. The only gap I see is suggestion 1.
• Security: the caches are keyed by reflect.Type or bounded to known dialects, so callers can't grow them.
  · branch perf/transpile-reflplan