Repository navigation
fix(plan): an unknown key domain must not read as the largest - #215
Conversation
The join-order estimator divides a pair's rows by the larger of its two key domains. Where a domain was unknown it stood in the relation's row count, "which is the most distinct values it could have" -- true, and the wrong direction. Unknown rows must read as the largest because rows multiply, so the largest reading is the most rows. A domain divides, so the largest reading is the fewest rows, which is the one direction an estimate may not err in. The two helpers sat beside each other with the same word and opposite safety. A derived table's columns carry no domain at all, since the binder reads one only from a table, so an edge between a base-table column of 25 values and a derived column of 1,200,243 rows divided by 1,200,243. A join the search ranked cheapest at 150,000 rows produced 6,697,748,923, and the query ran 145.2 s where the same query with that one join ordered differently runs 140.6 ms: 1033x, same answer either way. Read correctly the estimate lands within 8% of the truth -- one wrong input. An edge now divides by the largest of the domains the footers know, an unknown side contributing nothing. One known side is enough. Where neither is known the edge divides by 1 and the join reads as the product of its inputs, which is the most pessimistic reading available: any value between "the largest domain possible" and "no information" is a guess, and a guess here is paid for in materialised rows. No query of the 22 reaches the shape -- geomean 1.002, and Q09, whose largest join is why the larger-domain rule exists (ADR 0026's C5), is unchanged with an identical answer. Every rewrite that introduces a derived table does reach it, which is how this was found. Two tests encoded the old reading as correct and are corrected, not relaxed. One asserted an estimate of 10 for a join its own comment put at 5,000, labelled "the control"; it now reads 10,000. The other guards against moving a semi join onto more rows than it would see above (ADR 0029's D0), and credited "a wide key domain" that did not exist: the fixture declared a row count and no statistics, so the guard rested on the substitution it was meant to test against. The fixture now declares that domain -- the number the estimator used to invent -- so the case reads as it always did, for a stated reason. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @Hor911's task in 20s —— View job Claude review: no problems found I read the full diff against the AGENTS.md review rules. That covers
P2 notes
"Ask a human first" paths changed: none from the AGENTS.md list. Not reviewed: I did not run |
Coverage
Coverage gate: PASS (floors: Clang source-based coverage of the hermetic tests over |
The doc comment said 1013x where the ADR and the measurement say 1033x. Both numbers are real and they measure different things: 1013x compared two join orders of one binary while the machine was busy, and 1033x is this change measured before and after on a quiet one. The comment is about what the change is worth, so it now carries the two wall times it came from and has no ratio left to drift. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
## Summary A new ADR, `Proposed`, for computing a grouped aggregate on one side of the join below it (Yan and Larson, SIGMOD 1994). **No code**: this is the design, the survey behind it and the gate, following the pattern ADR 0029 and ADR 0030 set — a Proposed ADR with numbered items, each shipping as its own PR, and a final `docs(adr)` item that accepts or defers with the measured numbers. **The case.** Q13 is a textbook instance and the worst query of the 22 against DuckDB by peak memory: antb1 builds a hash table on 148,451,295 order rows, probes 15,000,000 customers to emit 153,451,474 rows, then groups those back down to 15,000,000. Moving the aggregate below the join, at SF 100 on four threads, interleaved and rotated: | | as written | pre-aggregated | | --- | ---: | ---: | | time | 4.581 s | **2.263 s (0.494x)** | | peak memory | 14,325 MiB | **1,554 MiB (0.108x)** | Answers identical over all 48 rows, and the ratio held across four independent measurements taken days apart (0.547x, 0.546x, 0.494x). **The memory figure is the stronger half of the case** — the difference between a query needing 14 GB and one fitting in a container. **The survey found the shape in 14 of the 22 and worth doing in 2**, and the reason so few qualify is structural rather than incidental: the required pre-grouping key is the join key together with every grouping column from the pre-grouped side, and in a star-like schema that union is usually a *key* of that side, making the reduction 1.00:1 by construction. Three queries are refused on that alone with no cardinality estimate needed. **The natural gate does not work, and the ADR says so.** "Reduce a lot and discard little" rates Q9 best in the whole survey on both axes while doing provably nothing to it — its aggregate consumes 319,404 rows and emits 175 groups *in both forms*, so the ratio is read off a quantity the push leaves unchanged — and it does not even order Q10 (wins, 3.02:1) against Q3 (loses, 3.9:1). The Decision carries a six-condition gate instead, two of whose conditions are the only signals that classify Q18 and Q21 correctly and are absent from the natural rule entirely. Closes # ## Type of change - [ ] feat: new SQL, CLI or engine capability - [ ] fix: bug fix - [ ] perf: performance improvement - [x] refactor, test, docs, build, ci or chore - [ ] Breaking change (CLI, output format or semantics); also add the `breaking-change` label ## Verification ```text $ pixi run ci # unchanged tree, so this is a control 100% tests passed out of 2813 $ pixi run lint every hook Passed except shellcheck: 37 pre-existing SC2218 findings in scripts/ and tools/github/, on files this PR does not touch; `git stash && pixi run lint` fails identically on main and CI's lint leg passes. markdownlint, typos and check_repo.py all pass on the new file and the index row. ``` Docs only — no behaviour changes, so there is nothing to measure here beyond the numbers the ADR records. Those were measured at SF 100 on four threads with `/usr/bin/time -l` for peak RSS, arms interleaved and rotated. TPC-H-derived data generated locally; not TPC-H Benchmark results. ## Checklist - [ ] `pixi run check` passes locally (lint + clang Debug -Werror + hermetic tests) — **not ticked**: `check` stops at the pre-existing shellcheck findings above. Both halves were run separately and pass. - [x] Tests cover the change (unit tests under `src/<module>/tests/`, or why none are needed) — **none needed**: no code changes. The tests the work itself will need are specified in the Plan's G1 and G2. - [x] Docs updated where behavior, commands or architecture changed (AGENTS.md, `docs/`, an ADR), or not needed - [x] No ClickBench-derived data is committed: no Parquet files, query answers or values from `hits` (ADR-0006) - [x] Nothing derived from TPC-H is committed: no query text or fragments, data, answers or TPC tools (ADR-0006) - [ ] Changes to governance paths (see `.github/CODEOWNERS`) were agreed with a maintainer **The open checklist item is the whole PR:** `/docs/adr/` is CODEOWNERS and a new ADR is a status decision, which AGENTS.md puts on the "Ask a human first" list. This is the ask. Nothing is built; if you would rather it were `Rejected` than `Proposed`, the Context already carries the evidence for that reading — two queries of 22, one of them the known case — and only the Status line and the Plan would change. ### On ADR 0006 Every cardinality, ratio and timing is permitted. No query text, no fragment, no data value and no EXPLAIN output appears: query shapes are described in words and queries named by number only. I checked this deliberately, having violated it once on #211. ### What is refused rather than deferred - **The mirror direction** (pulling an aggregate *up*). Q20's decorrelated plan already places one below a join where 99% of its groups are discarded, and grouping after the join measured 0.881x and then 0.778x — **both arm after arm**. Interleaved and rotated over six repeats it is **1.010x**; the apparent win was the second arm running on a warmer page cache each time. Peak memory does improve 0.920x. Recorded as measured and refused. - **A guard on the join shape** instead of a gate on the rewrite: penalising a join whose only edge has a tiny key domain fires on 9 of the 22, every one a dimension lookup whose small side's key is its primary key, including the join ADR 0026's C5 rule exists to protect. The fix belonged at the estimator's input and landed there in #215. - **Raising `KeyFilter::kMaxBytes`** 16 → 32 MiB so a published filter could rescue the Q3-like cases: measured **1.001x over the 22**, and Q3 still receives no filter, because what blocks it is ADR 0029's uncarried offer through a pushing `Filter`, not the cap. ### A prerequisite, already merged The first attempt at this rewrite produced a plan the search ranked cheapest at 150,000 rows that produced 6,697,748,923 — 145.2 s where the same query with that one join ordered differently runs 140.6 ms. That was the estimator reading a derived table's join four orders of magnitude low, fixed in #215. The ADR states plainly that this item could not have been built before that, because the gate would have been fed the same numbers. ## AI assistance - [ ] No AI assistance - [x] AI-assisted. Tools and what they did: Claude Code (Opus 5) ran the survey (nine subagents over the 22 queries, each writing and verifying a rewrite at SF 1), wrote the ADR, and took the SF 100 measurements. I re-measured every number the ADR quotes myself, interleaved and rotated, which is how the mirror direction's apparent win turned out to be a measurement artifact — I had reported it twice before catching it. - Accountable human (has read and understands the whole diff): @Hor911 Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
…#218) ## Summary A sink that merges each part into partial states -- the global, grouped and two-level aggregates and the top-N -- now lets 4 × threads parts run or wait ahead of its consumer instead of 2 × threads (`MergeWindow`); a GROUP BY's lanes take the same value as their bound. A part union, a join's build and the probe a right join schedules itself keep 2 × threads (`PartWindow`). Every part pipeline below a merging sink, a join's probe included, runs in the wider window. Why (ADR 0013 load balance, step 1's measurement, on a 2-socket, 64-core, 128-hardware-thread host): at 16 and 32 threads antb1's threads idle where DuckDB's do not, and the idle is the window. A part takes as long as the bytes it reads; on the full ClickBench data one row group of the most uneven column holds about 6 times the mean, and while it runs the parts after it finish and fill the window, which only taking that part frees. Traced part by part, Q20 at 16 threads kept its workers 68% busy with 28% of the threads' time lost to the full window; DuckDB, handing out the same row groups without a window, kept 95% busy. With 4 × threads the same trace is 94% busy. ADR 0013 is amended with the measurement (Decision, Memory and Load balance), including what the change does not fix: from 64 threads the idle is the last parts, started in part order (step 2), and the merge's 32 lanes; on the queries derived from TPC-H the idle is the consumer's own work (a right join's serial gather of its emitted rows, ADR 0026's C2b). ## Type of change - [x] perf: performance improvement ## Verification Measured on the 128-thread host with two frozen builds (`pixi run release` of main 20b5c53 and of this change on it, `cmp`-different), one process per query, best of 3 tries, two rounds with the arm order alternating per query and per round; the host is shared, so every run records the CPU other processes used and disturbed points were re-run. Full data (100 files). The branch was then rebased onto main b9337b9 without conflicts: #215-#217 change only `src/plan/` and other ADRs (none of the ClickBench queries has a join, so their plans are unchanged). ClickBench, all 43 queries, total of the best times, main → this change: | threads | main | this change | highest peak memory of a query | | ---: | ---: | ---: | --- | | 16 | 34.269 s | 30.745 s | 18.8 → 19.4 GB | | 32 | 22.190 s | 21.011 s | 20.3 → 20.7 GB | | 64 | 16.532 s | 15.841 s | 23.7 → 26.8 GB | | 128 | 16.370 s | 16.380 s | 29.7 → 31.4 GB | Largest moves: Q20 1.103 → 0.825 s at 16 threads and 0.527 → 0.368 s at 64; Q27 1.290 → 0.966 s and 0.649 → 0.511 s; Q28 4.303 → 3.602 s at 16. Slower beyond the noise: only Q23 at 64 threads (0.398 → 0.535 s; it gains at 16 and 32). Lane-heavy GROUP BYs (Q4, Q17, Q18) lean slower at 64 threads inside main's spread. At 128 threads the idle is the last parts and the merge's 32 lanes, which this change does not touch. The 22 queries derived from TPC-H (sf100), as a control, page cache verified resident: 16 threads 116.535 → 116.321 s, 64 threads 58.367 → 58.781 s; traced, Q1 and Q4 keep the same schedule (97% and 75% busy) with either window. This workload is derived from the TPC-H Benchmark and is not comparable to published TPC-H Benchmark results, as this implementation does not comply with all requirements of the TPC-H Benchmark. Tests: `MergingSinksKeepAWiderWindow` checks both windows; `EachSinkOpensItsWindowAheadOfAFailedFirstPart` checks, for every sink kind, that exactly its window's parts open ahead of a failed first part. Mutation-checked: with `MergeWindow` at 2 × threads both fail; with only the GROUP BY's `window_` at `PartWindow` the second fails. ```text $ pixi run check (this branch, rebased on b9337b9) lint: PASS 100% tests passed out of 2825 $ pixi run check-full (the same change on 20b5c53, on the 128-thread Linux host) clang-format / gersemi / ruff / tombi / markdownlint-cli2 / typos / shellcheck / actionlint / zizmor / check_repo.py ... Passed lint: PASS ci (clang Debug -Werror): 100% tests passed out of 2815 asan (ASan+UBSan -Werror): 100% tests passed out of 2815 tidy: clean (a first run rejected the new test's initializers; fixed in its own commit) coverage: 100% tests passed out of 2815; Coverage gate: PASS fuzz-smoke: 100% tests passed out of 2 ci-gcc (GCC 15 Debug -Werror): 100% tests passed out of 2815 check-full exit=0 ``` ## Checklist - [x] `pixi run check` passes locally (lint + clang Debug -Werror + hermetic tests) - [x] Tests cover the change - [x] Docs updated: ADR 0013 (amended), docs/architecture.md, comments - [x] No ClickBench-derived data is committed - [x] Nothing derived from TPC-H is committed - [ ] Changes to governance paths were agreed with a maintainer: `docs/adr/0013-parallel-execution.md` is under CODEOWNERS and needs a maintainer's review ## AI assistance - [x] AI-assisted. Claude Code (Claude Opus 5.5) found the cause by measurement on the 128-thread host, wrote the change, the tests and the ADR amendment; a read-only reviewer agent reviewed the diff (5 findings, all fixed). - Accountable human (has read and understands the whole diff): @ 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
…ent (#219) ## Summary **Item G2a of [ADR 0032](docs/adr/0032-aggregate-placement.md)**, which splits G2 in two. The gate's decisive quantity is how much a pre-grouping would reduce the rows it reads, and **nothing estimated it**: `EstimateRows` returns a `GroupAggregate`'s *input* rows as an upper bound (`join_order.cc`), which is exactly what a reduction test cannot use. So `DistinctValues` estimates the groups a grouping of some columns over a subtree would build, from each column's `KeyDomain`, capped at the subtree's rows — and the gate judges each legal placement on it. **Still no rewrite.** ```text GroupAggregate keys=[customer.c_custkey] COUNT(orders.o_orderkey) [groupable below the right of join, 1 key, padding, worth 1500000->149999] ``` **It admits exactly one placement of the 22** — Q13's, at the 10:1 the survey measured by hand — and refuses every other. **An unknown estimate is a refusal, and the direction is the point.** This quantity sits where a *small* value argues for a rewrite, so an unknown group count must argue against one. That is the mirror of what #215 recorded, where an unknown domain *divides* and so had to stop reading as the largest. Closes # ## Type of change - [x] feat: new SQL, CLI or engine capability - [ ] fix: bug fix - [ ] perf: performance improvement - [ ] refactor, test, docs, build, ci or chore - [ ] Breaking change (CLI, output format or semantics); also add the `breaking-change` label ## Verification ```text $ pixi run ci 100% tests passed out of 2825 $ ANTB1_DIFF_SEED=20261025 pixi run diff-random DIFF: PASS seed=20261025 queries=2000 failed=0 unsupported=0 $ clang-tidy -p build/tidy --warnings-as-errors='*' src/plan/aggregate_placement.cc src/plan/join_order.cc src/plan/tests/aggregate_placement_test.cc src/plan/explain.cc clean $ ASAN_OPTIONS=detect_leaks=0 ./build/ci-asan/bin/antb1_plan_tests 826 tests PASSED `pixi run asan` cannot run on this host (pre-existing curl_global_init leak at discovery), so the binary was run directly after generating the ci-asan fixtures. $ pixi run lint every hook Passed except shellcheck: 37 pre-existing SC2218 findings in files this PR does not touch. Not run on this host: pixi run ci-gcc and codeql-build (the gcc environment is linux-64 only). ``` No performance change: nothing is rewritten. One CLI golden gains the verdict, a one-line diff reviewed before updating. ## Checklist - [ ] `pixi run check` passes locally — **not ticked**: stops at the pre-existing shellcheck findings. Both halves run separately and pass. - [x] Tests cover the change (unit tests under `src/<module>/tests/`, or why none are needed) - [x] Docs updated where behavior, commands or architecture changed (AGENTS.md, `docs/`, an ADR), or not needed - [x] No ClickBench-derived data is committed (ADR-0006) - [x] Nothing derived from TPC-H is committed (ADR-0006) - [x] Changes to governance paths (see `.github/CODEOWNERS`) were agreed with a maintainer **The open item:** `docs/adr/0032-aggregate-placement.md`, which this both extends with G2a and **corrects on a claim of its own** (below). Splitting G2 is recorded there with its reason, as ADR 0029 and 0030 record their item updates. ### The ADR is corrected on Q10, and the gate is why ADR 0032 counted Q10 among the two queries this decision wins, at 0.945×. **That measurement is at a placement the decision does not describe** — it pre-groups two joins below the aggregate, where the ADR's exchange moves an aggregate across *one* join. The placement the single exchange actually reaches, pre-grouping at the join to the nation dimension by the seven columns condition 3 requires there, measures at SF 100, interleaved and rotated, answers identical over 23 rows: | Q10 | time | peak | | --- | ---: | ---: | | as written | 3.155 s | 5,164 MiB | | the placement this decision reaches | **3.926 s (1.245x slower)** | 6,819 MiB | | the placement the ADR measured, two joins down | 2.829 s (0.945x) | 4,285 MiB | **The gate refuses the reachable one**, so nothing is wrong with the gate — what was wrong was counting Q10. On its own evidence this decision is worth **one** query of the 22, and the honest case for it is Q13's 0.494× and **14,325 MiB → 1,554 MiB**. The Consequences section now says that, and says plainly that this is not a throughput item: against the 22 it is about 1% of total time. An iterated exchange along a chain of joins would reach Q10's measured placement. It is not part of this ADR and I have not proposed it. ### Three of the six gate conditions are not decidable in a logical plan Saying which is more useful than pretending otherwise, so the header does: - **"Not a superkey"** has no metadata behind it — Parquet carries no keys. Its effect falls out of the reduction test instead, for a measured reason rather than an asserted one. - **Condition 5's second half** ("a join between the two positions multiplies rows") needs to know whether a build is unique on its key, a runtime fact. **I implemented the obvious plan-time proxy and it refused Q13**: that join takes 150,000 rows to 1,500,000 and emits 1,500,000, multiplying nothing at all, while the rewrite is still worth 0.494× there — because what shrinks is the build and the final grouping's input, not the join's output. The proxy is gone and the reason is in the header. - **Condition 6** is a property of the physical plan: ADR 0029's filters are attached long after this runs. ### Also A **LEFT JOIN carries no row estimate** — the binder keeps one only for the joins its order search ranked — so the gate computes its own by ADR 0022's rule 3, *locally*. Changing what `EstimateRows` answers for a join would move `ChooseBuildSide`, which is ADR 0026's decision and not this item's. Without this the gate could not judge Q13 at all. ### Tests A catalog local to these cases, so declaring statistics cannot move another suite's expectations: a fact table whose key takes ten values, one unique key, one column declaring nothing, and three dimensions carrying different shares of those keys. Cases for the admitted shape and for each refusal the gate can decide, including one where two conditions hold at once and the order of the checks decides which is reported. Two test expectations I wrote turned out to be wrong rather than the code: a unique key against a ten-row dimension is refused as "below a filter" before the reduction test is reached, which is correct, and isolating "no reduction" needed a dimension carrying every value of that key. ## AI assistance - [ ] No AI assistance - [x] AI-assisted. Tools and what they did: Claude Code (Opus 5) built the estimate, the gate, the tests and the ADR update, and took the SF 100 measurements. The sequencing was the useful part: building the gate before the rewrite is what surfaced the Q10 correction and killed my own wrong proxy for condition 5, neither of which could have shown up as an answer because nothing is rewritten yet. - Accountable human (has read and understands the whole diff): @Hor911 --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Summary
The join-order estimator divides a pair's rows by the larger of its two key domains. Where a domain was unknown
it stood in the relation's row count, "which is the most distinct values it could have" — true, and the wrong
direction. The two helpers sat beside each other with the same word and opposite safety:
Rows multiply, so the largest reading is the most rows — safe. A domain divides, so the largest reading
is the fewest rows, which is the one direction an estimate may not err in: under-stating a build's rows is what
materialises a side that does not fit (ADR 0026's C5), and under-stating an intermediate is what lets the search
pick a plan that multiplies.
A derived table's columns carry no domain at all, because the binder reads one only from a table
(
binder.cc:4639-4643). So an edge between a base-table column of 25 values and a derived column of1,200,243 rows divided by 1,200,243 instead of by 25. A join the search ranked cheapest at 150,000 rows
produced 6,697,748,923 of them.
An edge now divides by the largest of the domains the footers know, with an unknown side contributing nothing.
One known side is enough. Where neither is known the edge divides by 1 and the join reads as the product of its
inputs — deliberately the most pessimistic reading available, since any value between "the largest domain
possible" and "no information" is a guess, and a guess here is paid for in materialised rows.
Closes #
Type of change
breaking-changelabelVerification
Performance. SF 100, four threads, both arms built from this branch and
main. TPC-H-derived data generatedlocally; not TPC-H Benchmark results.
The 22 do not move because none of them reaches the shape: no unmodified query joins on a derived table's
column inside a block where the order matters. Every rewrite that introduces a derived table does reach it, which
is how this was found — the first attempt at moving a grouped aggregate below a join hit it immediately. With the
domain read correctly the estimate lands within 8% of the truth, so it is one wrong input and nothing else.
Checklist
pixi run checkpasses locally (lint + clang Debug -Werror + hermetic tests) — not ticked: see theshellcheck note. Both halves were run separately and pass.
src/<module>/tests/, or why none are needed)docs/, an ADR), or not neededhits(ADR-0006).github/CODEOWNERS) were agreed with a maintainerThe one open checklist item: this amends
docs/adr/0022-joins-and-query-blocks.md, which CODEOWNERS covers.It is an update to an Accepted ADR, not a status change, and it exists because that ADR stated both halves of
this hazard and never joined them up: four lines above the refinement that introduced the substitution, it says
the domains are what keep a many-to-many edge out of the order, since "ordering by row counts alone can take such
an edge first and multiply the intermediate result". The code and tests stand alone if you would rather the ADR
hunk went separately.
Tests — two corrected, not relaxed
I want to be explicit about this, because changing a test's expectation normally means weakening it. Both of
these asserted the old reading as correct:
JoinOrderTest.AManyToManyEdgeWaitsWhileAKeyEdgeIsAvailableasserted an estimate of 10 for a join whoseoutput its own comment puts at 5,000 — a 500x under-statement, labelled "the control". It now reads
10,000, a 2x over-statement, on the safe side.
BinderTest.ASemiJoinOverOneRelationRunsOnItsBranchguards against moving a semi join onto more rows than itwould see above (ADR 0029's D0). Its comment credited "a wide key domain" that did not exist: the fixture
registered a row count and no statistics, so the only domain in play was the substituted row count — the
guard rested on the very substitution it was meant to test against. Rather than change the expectation, the
fixture now declares that domain, and the number is the one the estimator used to invent, so the case reads
as it always did and now for a stated reason.
New coverage: one known domain divides alone whichever side it is on, both-unknown reads as the product, and a
key with no value empties the join even when its partner's domain is unknown. Four mutations checked, all
caught — including one that is behaviour-identical on the divisor and differs only in losing that last case,
which is what prompted it.
A guard against the shape was considered and refused
Penalising a join whose only edge has a tiny domain fires on 9 of the 22, every one a dimension lookup where
the small side's key is its primary key — including Q09's own supplier-to-nation join, the query ADR 0026's C5
rule exists to protect. The discriminating condition is non-uniqueness on both sides, which the metadata gives
for a base table and cannot give for a derived one. So the fix belongs at the input, not at the decision.
Propagating a base column's domain through a derived table's pass-through columns would narrow the remaining gap
and is left as its own item.
AI assistance
and ran the gates and SF 100 measurements. A subagent diagnosed it after I handed it the wrong premise — I had
reported the 1033x as a composite-key split made unavoidable by a rewrite; it showed two of the three join
orders avoid the bad edge entirely, which makes it an estimator failure rather than a bad graph, and traced the
cause to the three lines above. I verified each of those myself before writing any code, including re-running
the 1033x and confirming the fixture declares no statistics.