Repository navigation
feat(plan): estimate a pre-aggregation's groups, and judge the placement - #219
Merged
Merged
Conversation
Item G2a of ADR 0032, 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, 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 that. Still no rewrite. 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 the lesson #215 recorded, where an unknown domain divides and so had to stop reading as the largest. It admits exactly one placement of the 22 -- Q13's, as worth 1500000 to 149999, which is the 10:1 the survey measured by hand -- and refuses every other, including the one worth examining below. Three of the ADR's six conditions are not decidable in a logical plan and the header says which. "Not a superkey" has no metadata behind it, since Parquet carries no keys; its effect falls out of the reduction test instead. Condition 5's second half needs a build's key uniqueness, a runtime fact, and the obvious plan-time proxy is wrong: Q13's join takes 150,000 rows to 1,500,000 and emits 1,500,000, multiplying nothing, while the rewrite is still worth 0.494x there, because what shrinks is the build and the final grouping's input rather than the join's output. I had implemented that proxy and it refused Q13. Condition 6 is a property of the physical plan, attached long after this runs. A LEFT JOIN carries no row estimate, since 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, because changing what EstimateRows answers for a join would move ChooseBuildSide, which is ADR 0026's decision. The ADR is corrected on Q10. Its 0.945x is at a placement two joins below the aggregate, which this decision's single exchange does not reach; the placement it does reach measures 1.245x slower at SF 100, 3.155 s to 3.926 s with peak 5,164 to 6,819 MiB, answers identical. The gate refuses it, so the gate is right and the ADR's claim was wrong. On its own evidence this decision is worth one query of the 22. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @Hor911's task in 17s —— View job Claude review: no problems found I read the full diff against the AGENTS.md Code Review Rules: the
|
Coverage
Coverage gate: PASS (floors: Clang source-based coverage of the hermetic tests over |
The P2 of the review: DistinctValues walked the whole subtree per call, and JoinRows called it once per key per side, so a two-key join walked four overlapping subtrees where two would do. Plan-time only and the plans are small, so this is tidiness rather than a measurable win. EachDistinctValues answers for several columns in one walk and DistinctValues is now its product. The split is also the more honest primitive: a caller wanting the largest of several domains cannot take a maximum out of a product, which is exactly what JoinRows needs and why it was asking column by column in the first place. Verdicts over the 22 are byte-identical before and after. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
Item G2a of ADR 0032, 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:
EstimateRowsreturns a
GroupAggregate's input rows as an upper bound (join_order.cc), which is exactly what a reductiontest cannot use. So
DistinctValuesestimates 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.
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
breaking-changelabelVerification
No performance change: nothing is rewritten. One CLI golden gains the verdict, a one-line diff reviewed before
updating.
Checklist
pixi run checkpasses locally — not ticked: stops at the pre-existing shellcheck findings. Bothhalves run separately and pass.
src/<module>/tests/, or why none are needed)docs/, an ADR), or not needed.github/CODEOWNERS) were agreed with a maintainerThe open item:
docs/adr/0032-aggregate-placement.md, which this both extends with G2a and corrects on aclaim 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:
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:
test instead, for a measured reason rather than an asserted one.
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.
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
EstimateRowsanswers for a join wouldmove
ChooseBuildSide, which is ADR 0026's decision and not this item's. Without this the gate could not judgeQ13 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
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.