Repository navigation
perf(parser): size joins slice from a JOIN-count hint [W4-03] - #929
Merged
Merged
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
documentation
|
Claude finished @ajitpratap0's task in 16s —— View job Review of PR #929: size joins slice from a JOIN-count hintI read the diff and did not run the tests or benchmarks. The change is small and safe: the hint only affects slice capacity, so it can't change correctness. What looks good
Points to consider
Verdict: Fine to merge once the perf gate verdict is in. I'd like a note, or a quick check, on the multi-statement over-allocation in point 1. |
Perf gate: PASS (shadow mode: report only, this check never fails)Base
Rules: time regresses at >= +3% with p < 0.01 (Mann-Whitney U), or a suite geomean >= +1.5% (per-round geomeans, same test); allocs/op must not increase; B/op must not rise more than 2%; instructions/op must not rise more than 1.5%. An A/A control with more than 10% of benchmarks moving over 3% is noisy: re-run once, then inconclusive (neutral). Gated suite (GOGC=off, GOMAXPROCS=1)
A/A control: 0 of 50 benchmarks moved more than 3%. Instructions per operation (cmd/perfdriver)
Secondary metric: default GC, GOMAXPROCS=1 (informational)
Environmentbenchstat (primary metric) |
…ss [W4-03] The preprocess-loop counter cost about 1.3% instructions on join-free queries (CI perf gate). Scan a 256-token lookahead only when the first join is appended, so join-free statements pay nothing. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DnbRnTFErhoMJdAjVEeCg2
|
Claude finished @ajitpratap0's task in 34s —— View job Review of PR #929: size joins slice from a JOIN-count hintI read the current head ( What the code does now
Good
Points to consider
Verdict: The code is fine. Before merging, I'd refresh the PR description and perf-gate result and decide on point 1. A semicolon stop is a one-line change. |
…daries [W4-03] The 256-token lookahead counted JOINs of neighbouring statements, UNION branches and nested subqueries, so short single-join statements in scripts allocated cap 8 (416 B per JoinClause), doubling B/op. Count only depth-0 JOINs and stop at ';', UNION/EXCEPT/INTERSECT or the closing ')'. Adds capacity assertions for scripts, set-op branches and subquery depth, and a many-short-join-statements benchmark. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DnbRnTFErhoMJdAjVEeCg2
|
Claude finished @ajitpratap0's task in 22s —— View job Review of PR #929: size joins slice from a JOIN lookaheadI read the diff against Summary: The code is fine to merge. The earlier review points are addressed in this head. What changed
Earlier concerns
Remaining points (minor)
Verdict: The change is simple and safe, since it only affects capacity. The memory gain is real (about -20% B/op on join queries), and time is neutral within noise. Approve-worthy from my side. |
…W4-03] Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DnbRnTFErhoMJdAjVEeCg2
|
Claude finished @ajitpratap0's task in 35s —— View job Review of PR #929: size joins slice from a JOIN lookaheadI read the diff against Verdict: The change is safe to merge. It only affects slice capacity, so it can't change the AST. What it does
Good
Minor points (none blocking)
|
…[W4-03] Add TestJoinsFullSliceRegrownFromHint, which fails if the grow condition reverts to cap(joins) == 0 (cap 8 from repeated doubling instead of a hint-sized grow). Correct the joinsCap comment: it also runs when a full slice is handed in. Merges origin/main. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DnbRnTFErhoMJdAjVEeCg2
|
Claude finished @ajitpratap0's task in 22s —— View job Review of PR #929: size joins slice from a JOIN lookaheadI read the diff against Verdict: Safe to merge. The hint only changes slice capacity, so the AST can't change. What I checked
Minor points (none blocking)
Tests: The new |
|
Claude finished @ajitpratap0's task in 14s —— View job Review of PR #929: size joins slice from a JOIN lookaheadI read the diff against Verdict: Safe to merge. The hint only changes slice capacity, so the AST can't change. What I checked
Minor points (none blocking)
|
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
W4-03 (PLAN, C0 rank 4).
parseJoinClausesusedmake([]ast.JoinClause, 0, 2). It now allocates with capacity1 + (depth-0 JOIN tokens that follow), capped at 8, scanning at most 256 tokens and stopping at a depth-0;, UNION/EXCEPT/INTERSECT or the)closing the enclosing level, viaParser.joinsCap(). The scan runs only when the first join of a FROM clause is appended, so join-free statements pay nothing. No API or AST change; only slice capacity differs.Deviation from the PLAN text ("count in the preprocess loop"): the first version did count in
preprocessTokensInto. The CI gate showed +1.2% to +1.7% instructions/op on every e2e case (including join-free ones) and +3% to +5% time on several join-free benchmarks, so the counter was moved to a lazy lookahead (commit b0eeb75). Review found that the first lazy version (256-token window only) scanned across statement and UNION boundaries, so scripts of short single-join statements allocated cap 8 (416 B/JoinClause) and B/op rose about +101% to +124% on such inputs (invisible to the gated suite, whose ParseRW medium statements are long). Fixed by making the scan depth-aware: it counts only depth-0 JOINs and stops at;, UNION/EXCEPT/INTERSECT or the closing), which also ignores JOINs inside nested subqueries.Test Plan
task check,task test:cbinding,go test -count=1 ./pkg/compatibility/...: pass. staticcheck on./pkg/sql/parser: clean.parseTokens, B/op deterministic; base / old head / this head): 50 x single-join statements 93,957 / 210,344 / 69,929 B/op (-26% vs base); mixed 1-join/2-join script 99,963 / 221,448 / 87,948 (-12%); 20 xJOIN ... UNION ALL39,254 / 78,755 / 29,646 (-24%).ParseE2E/join2,138 andcomplex5,405 B/op are unchanged from the previous head. NewBenchmarkParseManyShortJoinStatementskeeps this visible.pkg/sql/parser/join_hint_test.go: capacity and join content/order for 1,2,3,5,8,9,12 joins; window and cap behaviour injoinsCap; multi-statement script with a join-free statement in the middle; capacity assertions (cap 1 for each single-join statement in a script, for each UNION/EXCEPT/INTERSECT branch, for a subquery followed by outer joins, outer cap ignoring nested-subquery joins). RevertingjoinsCapto the previous version fails 7 of these subtests.task bench:ab(shared M4, indicative):ParseE2E/joinB/op 2.305Ki to 1.836Ki (-20.34%, p=0.000),complex6.398Ki to 5.023Ki (-21.49%), complex allocs 50 to 49; time unchanged within noise (geomean +0.59%, p=0.80).ParseE2E/joinB/op -20.34%, time -0.60% (p=0.028);complexB/op -21.49%, allocs 50 to 49, time -1.23%;ParseFresh/joinB/op -19.67%;ParseRW/*/mediumB/op -3.2% to -7.9% with fewer allocs on 6 of 7 dialects;Normalize/join-3.32% andTransformParse/join-4.11% time (both flagged faster); suite geomean -0.47% (p=0.029). allocs/op never rise and B/op never rises on any gated benchmark; instructions/op stays within +0.78% (worste2e/join), under the 1.5% limit. Largest time movements are on join-free cases and sit above their A/A control (ParseE2E/aggregate+2.25%,select_where+1.09%,Normalize/simple_select+1.34%), all under the 3% threshold and with allocs/B/op and instructions/op unchanged. The later commits on this branch change only a test and a doc comment (plus a merge of main), not parser code.(a JOIN b) JOIN c) and one join followed by a comma table (a JOIN b, c). Both are outside the gated suite.ParseE2E/join. Reported, not chased.TestJoinsCapSubqueryDepthnow places the nested subquery after the first join (WHERE, and a derived table in a later join) and asserts exact outer caps 1 and 2; removing the depth-0 guard injoinsCapfails it (outer cap 3 and 4).parseJoinClausesnow grows a full slice by the hint (slices.Grow) instead of only sizing an empty one. NewTestJoinsCapParenGroupThenOuterandTestJoinsCommaAfterJoin.TestJoinsFullSliceRegrownFromHintpins the full-slice regrow: with the condition reverted tocap(joins) == 0a parenthesized group followed by four outer joins ends at cap 8 (repeated doubling) and the test fails.testing.AllocsPerRun, pre-tokenized):SELECT * FROM (a JOIN b ON a.i=b.i) JOIN c ON c.i=a.i13 to 14 allocs; the same with a further JOIN is unchanged at 18;SELECT * FROM a JOIN b ON a.i=b.i, c10 to 11. The lookahead stops at the group's)and cannot see a trailing comma table, so the cap-1 slice must grow once. Accepted; also noted in the CHANGELOG.MINUSis tokenized as an identifier and is not a scan boundary; a MINUS chain can over-count later branches' JOINs, bounded by cap 8 and the 256-token window (capacity hint only). Noted in thejoinsCapdoc comment.Breaking Changes
None.
🤖 Generated with Claude Code
https://claude.ai/code/session_01DnbRnTFErhoMJdAjVEeCg2