Repository navigation
perf(formatter): look up lower-case keywords in a constant table [W5-07] - #936
Open
ajitpratap0 wants to merge 5 commits into
Open
ajitpratap0 wants to merge 5 commits into
ajitpratap0 wants to merge 5 commits into
Conversation
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DnbRnTFErhoMJdAjVEeCg2
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
github-actions
Bot
added
the
documentation
|
Claude finished @ajitpratap0's task in 24s —— View job Review of PR #936: lower-case keyword lookup tableVerdict: LGTM. I found no correctness issues. I read the diff but did not run the tests or benchmarks. The numbers below are from the PR description. Correctness
Performance
Minor suggestions (non-blocking)
Security / APINo security concerns and no public API change. The CHANGELOG entry is accurate and follows the existing style. |
Perf gate: PASS (shadow mode: report only, this check never fails)Base Runner CPU:
Rules: each arm runs 20 randomized code layouts ( Gated suite (GOGC=off, GOMAXPROCS=1)
A/A' control (base revision, disjoint layout seeds): 0 of 52 benchmarks moved more than 3%; 0 would have been flagged by the time rule; suite geomean +0.21%, p 0.327. Low power2 benchmark(s) have under 80% estimated power to be flagged on their own for a +5% slowdown (their spread across layouts is too wide for 20 layouts at the Benjamini-Hochberg level alpha/m = 0.000192). A slowdown below the last column usually goes unflagged there; instructions/op, allocs/op and the suite geomean still guard them.
Power is estimated with the normal approximation for the U test (Noether): power = Phi(ln(1+shift)/s * sqrt(K/2) * sqrt(3/pi) - z(1 - alpha/(2m))), with s the spread and K the layouts per arm. Instructions per operation (cmd/perfdriver)
Secondary metric: default GC, GOMAXPROCS=1 (informational)
Environmentbenchstat (primary metric, samples pooled across layouts: informational, the verdict uses the per-layout test) |
|
Claude finished @ajitpratap0's task in 24s —— View job Review of PR #936: lower-case keyword lookup tableVerdict: LGTM. I read the diff but did not run the tests or benchmarks. The numbers below come from the PR description. Correctness
Performance
Minor suggestions (non-blocking)
Security / APINo security concerns and no public API change. The CHANGELOG entry is fine. |
…e [W5-07] Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DnbRnTFErhoMJdAjVEeCg2
|
Claude finished @ajitpratap0's task in 20s —— View job Review of PR #936: lower-case keyword lookup tableVerdict: LGTM. I read the code but could not run the tests or benchmarks here. Shell commands that chain operations needed approval, so the test results come from the PR description and the CI perf-gate comment. Correctness
Performance
Minor, non-blocking
Security / APINo security concerns, no public API change, and the CHANGELOG entry is accurate. |
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
W5-07 (FMT-5).
nodeFormatter.kwcalledstrings.ToLowerfor every keyword in lower-case mode, allocating a string each time. It now useslowerKeyword: a constant table of every keyword spelling the renderers emit, falling back tostrings.ToLoweronly for user text, so output is byte-identical. The table covers DML/query keywords, the DDL writers inpkg/sql/ast(ast.DDLStyle{Keyword: f.kw},sql_ddl_objects.go,sql_alter.go), the callback helpers (CTE search/cycle, sample, ROWS FROM, table-ref pre/post alias, table hints, PIVOT/UNPIVOT) and theasthelper return sets (join, set-op, alter-action, merge, lock, fetch/offset rows). Fix round 1 closed the DDL/helper gap found in review.Test Plan
strings.ToLower;lowerKeywordequalsstrings.ToLoweron hits, misses, mixed-case, non-ASCII;kwallocates 0 for known keywords; everykw("literal")site inpkg/formatterAND everywriteKw/kwliteral site inpkg/sql/astis in the table (go/parser scan);buildLowerKeywordTablepanics on duplicates (tested); helper return sets (incl. parser join combinations) are checked against the table;TestLowerKeywordCorpusMissesrenders the dialect corpus with a fallback counter and asserts zero misses for parser-produced ASTs.task checkok,task test:cbindingok,go test -count=1 ./pkg/compatibility/...ok, staticcheck./pkg/formatterclean.task bench:ab BASE=origin/main BENCH='./pkg/formatter=^BenchmarkRender'(layout-randomized, shared M4, indicative only): Render/lower allocs/op geomean -29.0% (simple_select 6->4, select_where 12->7, join 27->20, aggregate 12->8, subquery 16->10, cte 25->17, window 7->5, complex 86->73, insert 18->15, update 15->12, delete 10->7), B/op -2.6..-7.5% per row, time geomean -10.4% (all p<0.001, -7.7..-13.1%). Compact/readable/sql rows unchanged (allocs equal, time within noise).pkg/fingerprint/fingerprint.go:150(SHA256 fingerprint, not touched by this PR).Breaking Changes
None. No API or output change.
🤖 Generated with Claude Code
https://claude.ai/code/session_01DnbRnTFErhoMJdAjVEeCg2