feat(ir): make Aggregate's reduction kind explicit, not inferred (#165) - #166
Merged
Merged
Conversation
Implements design proposal #165, borrowed from how a semantic layer like dbt keeps "what kind of computation is this" (a measure's `agg` type) and "which dimensions to group by" (query-time `group_by`) as two independent, explicitly-declared things -- never letting the second answer the first. `QueryExpr::Aggregate` previously carried `by: GroupKeys` alone. Whether a node was a genuine cross-series reduction or a per-series/windowed pass-through with no grouping concept at all (`rate`, `*_over_time`, ...) was *inferred*, downstream, from `by.is_empty() && (intent.is_per_series() || child is TimeRange/Subquery)` -- computed independently in at least two places (`output_schema_in`, `asap-plan::bind`) and dependent on the `TimeRange`/`Subquery` child staying *structurally adjacent*, an invariant nothing in the type system protected. A future rewrite pass inserting a node between an `Aggregate` and its range-window child would silently misclassify a per-series node as a cross-series one -- a wrong merge, the same failure mode #163 was about, just relocated one layer up. - `Reduction::{Reduce(GroupKeys), PerEntity}` (asap-ir) replaces `Aggregate.by`. Decided once, at L2->L3 lowering (which knows unambiguously whether it's building a bare per-series range function or a genuine aggregation operator), using the same structural signals the old heuristic used -- just computed once, at the producer, instead of re-derived by every consumer. - `output_schema_in`/`aggregate_output_schema` read `Reduction` directly; the old heuristic and its `is_range_child`/`per_series` recomputation are gone. - `asap-plan::bind_summary_agg` reads `Reduction` directly instead of recomputing per-series-ness itself. - `canonicalize`'s heavy-hitter/ROW_NUMBER rewrites, `cse`'s pass-through rewrite, and every test constructing an `Aggregate` literal across the workspace updated to the new shape. Not a code-level bug fix by itself (today's construction sites already got this right) -- it closes the door on the *next* one, by making the type system enforce what used to be an unenforced convention. ## Test plan - [x] All classifications (Reduce vs PerEntity) verified against the existing PromQL/SQL conformance suites -- 100/100 in `promql_conformance.rs`, 44/44 in `promql_lowering.rs`, full SQL corpora -- rather than asserted from first principles alone. - [x] `cargo build --workspace` / `cargo test --workspace` -- clean, zero failures across every crate. - [x] `cargo clippy --workspace --all-targets --all-features -- -D warnings` -- clean. - [x] `cargo fmt --all --check` -- clean. Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
zzylol
added a commit
that referenced
this pull request
Jul 28, 2026
…e empty-by ambiguity Fixes #163. Reworked on top of #165/#166: rather than introducing a parallel SummaryGrouping type at L4 (this branch's original approach), SummaryAgg now carries asap-ir's Reduction directly -- the same value the L3 Aggregate node it was bound from already carried. asap-plan::bind already decides Reduce vs PerEntity once, at L3->L4 binding (#165); this just stops flattening that decision back into an ambiguous bare Vec<ColumnId> on the way into SummaryAgg. `SummaryExecutor::find_candidates`'s `by: &[ColumnId]` becomes `reduction: &Reduction`, so every implementer (including ASAPQuery-backend's `data_plane`, the consumer that hit this) must handle both cases explicitly instead of guessing. Two ambiguous cases this resolves: - A bare per-series range function with no grouping syntax at all (`quantile_over_time(0.99, m[10s])`) -- no grouping concept, must never merge across entities. - An aggregation operator explicitly invoked with no `by(...)` (`count(hll_metric)`) -- a genuine full reduction, must merge everything into one group. Both used to collapse to the same `by: []`, with nothing in the trait able to tell them apart (see the issue for a concrete two-disjoint-HLL- sids repro). ## Test plan - [x] Two `asap-plan` binding tests proving the L3 Reduction survives onto the L4 SummaryAgg node unchanged for both ambiguous cases -- the classification logic itself is already covered by #165's much larger test suite (100+ conformance tests), so these are narrowly scoped to the L3->L4 passthrough. - [x] New `asap-sketch` wiring test proving `execute()` passes `reduction` through to `find_candidates` unmodified. - [x] Existing `asap-plan`/`asap-sketch`/e2e tests and assertions updated to the new shape. - [x] `cargo build --workspace` / `cargo test --workspace` -- clean. - [x] `cargo clippy --workspace --all-targets --all-features -- -D warnings` -- clean. - [x] `cargo fmt --all --check` -- clean. Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
Merged
6 tasks
zzylol
added a commit
that referenced
this pull request
Jul 28, 2026
…e empty-by ambiguity (#164) Fixes #163. Reworked on top of #165/#166: rather than introducing a parallel SummaryGrouping type at L4 (this branch's original approach), SummaryAgg now carries asap-ir's Reduction directly -- the same value the L3 Aggregate node it was bound from already carried. asap-plan::bind already decides Reduce vs PerEntity once, at L3->L4 binding (#165); this just stops flattening that decision back into an ambiguous bare Vec<ColumnId> on the way into SummaryAgg. `SummaryExecutor::find_candidates`'s `by: &[ColumnId]` becomes `reduction: &Reduction`, so every implementer (including ASAPQuery-backend's `data_plane`, the consumer that hit this) must handle both cases explicitly instead of guessing. Two ambiguous cases this resolves: - A bare per-series range function with no grouping syntax at all (`quantile_over_time(0.99, m[10s])`) -- no grouping concept, must never merge across entities. - An aggregation operator explicitly invoked with no `by(...)` (`count(hll_metric)`) -- a genuine full reduction, must merge everything into one group. Both used to collapse to the same `by: []`, with nothing in the trait able to tell them apart (see the issue for a concrete two-disjoint-HLL- sids repro). ## Test plan - [x] Two `asap-plan` binding tests proving the L3 Reduction survives onto the L4 SummaryAgg node unchanged for both ambiguous cases -- the classification logic itself is already covered by #165's much larger test suite (100+ conformance tests), so these are narrowly scoped to the L3->L4 passthrough. - [x] New `asap-sketch` wiring test proving `execute()` passes `reduction` through to `find_candidates` unmodified. - [x] Existing `asap-plan`/`asap-sketch`/e2e tests and assertions updated to the new shape. - [x] `cargo build --workspace` / `cargo test --workspace` -- clean. - [x] `cargo clippy --workspace --all-targets --all-features -- -D warnings` -- clean. - [x] `cargo fmt --all --check` -- clean. Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
zzylol
added a commit
that referenced
this pull request
Jul 28, 2026
Rebased onto main (now past #162/#164/#165/#166) after PR #157's original 37-commit history hit real conflicts with the intervening Reduction/L4 grouping-model work -- squashed to one commit here since the PR merges as a squash anyway, so the intermediate history wasn't worth preserving through a 37-way conflict resolution. `docs/design.md` was a 1500+ line document mixing layer design, migration planning, and crate-by-crate speculation, most of it already stale relative to the shipped code. This does the same move `l4node-execution- model.md` already made for L4 across the rest of the pipeline: each layer gets its own doc, `design.md` becomes a thin index into them, and every doc is cross-checked against current code rather than copied from old drafts. - New per-layer docs: `l1-query-language.md`, `l2-logical-plan.md`, `l3-intent-algebra.md`, `l5-physical-plan.md` (l4's already existed, renamed to `l4-summary-bound-ir.md` to match the shipped `SummaryExpr`/`SummaryKind`/`L4Node` naming instead of the older "sketch" framing). `l1-query-language.md` also absorbs the old standalone `promql-lowering.md` as a section. - `design.md` cut to a slim index: one paragraph per L1-L5 linking to its doc, plus a trimmed Glossary. - Deleted `docs/migration-plan.md` and `docs/intent-algebra- reconciliation.md` -- both were migration-era planning artifacts, the latter already self-annotated as a superseded historical snapshot. - `crates/sketch/src/exec.rs`'s doc comments repointed at the renamed `l4-summary-bound-ir.md`. Known gap carried over from the original PR (not introduced by this rebase): README.md still links to `docs/migration-plan.md` and to `docs/design.md`'s old §3/§6/§8/§9/§11/§12 section numbers, none of which exist after this change -- worth a follow-up. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
zzylol
added a commit
that referenced
this pull request
Jul 28, 2026
Rebased onto main (now past #162/#164/#165/#166) after PR #157's original 37-commit history hit real conflicts with the intervening Reduction/L4 grouping-model work -- squashed to one commit here since the PR merges as a squash anyway, so the intermediate history wasn't worth preserving through a 37-way conflict resolution. `docs/design.md` was a 1500+ line document mixing layer design, migration planning, and crate-by-crate speculation, most of it already stale relative to the shipped code. This does the same move `l4node-execution- model.md` already made for L4 across the rest of the pipeline: each layer gets its own doc, `design.md` becomes a thin index into them, and every doc is cross-checked against current code rather than copied from old drafts. - New per-layer docs: `l1-query-language.md`, `l2-logical-plan.md`, `l3-intent-algebra.md`, `l5-physical-plan.md` (l4's already existed, renamed to `l4-summary-bound-ir.md` to match the shipped `SummaryExpr`/`SummaryKind`/`L4Node` naming instead of the older "sketch" framing). `l1-query-language.md` also absorbs the old standalone `promql-lowering.md` as a section. - `design.md` cut to a slim index: one paragraph per L1-L5 linking to its doc, plus a trimmed Glossary. - Deleted `docs/migration-plan.md` and `docs/intent-algebra- reconciliation.md` -- both were migration-era planning artifacts, the latter already self-annotated as a superseded historical snapshot. - `crates/sketch/src/exec.rs`'s doc comments repointed at the renamed `l4-summary-bound-ir.md`. Known gap carried over from the original PR (not introduced by this rebase): README.md still links to `docs/migration-plan.md` and to `docs/design.md`'s old §3/§6/§8/§9/§11/§12 section numbers, none of which exist after this change -- worth a follow-up. Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
3 tasks
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
Implements design proposal #165.
QueryExpr::Aggregatepreviously carriedby: GroupKeysalone. Whether a node was a genuine cross-series reduction or a per-series/windowed pass-through with no grouping concept at all (rate,*_over_time, ...) was inferred, downstream, fromby.is_empty() && (intent.is_per_series() || child is TimeRange/Subquery)— computed independently in at least two places (output_schema_in,asap-plan::bind) and dependent on theTimeRange/Subquerychild staying structurally adjacent, an invariant nothing in the type system protected. A future rewrite pass inserting a node between anAggregateand its range-window child would silently misclassify a per-series node as a cross-series one — a wrong merge, the same failure mode #163 was about, just relocated one layer up.This borrows the dbt idea discussed on #165: keep "what kind of computation is this" (a measure's
aggtype) and "which dimensions to group by" (query-timegroup_by) as two independent, explicitly-declared things — never let the second answer the first.Changes
asap-ir:Reduction::{Reduce(GroupKeys), PerEntity}replacesAggregate.by. Decided once, at L2→L3 lowering (which knows unambiguously whether it's building a bare per-series range function or a genuine aggregation operator), using the same structural signals the old heuristic used — just computed once, at the producer, instead of re-derived by every consumer.output_schema_in/aggregate_output_schemareadReductiondirectly; the old heuristic and itsis_range_child/per_seriesrecomputation are gone.asap-plan:bind_summary_aggreadsReductiondirectly instead of recomputing per-series-ness itself.asap-l2:canonicalize's heavy-hitter/ROW_NUMBERrewrites andcse's pass-through rewrite updated to the new shape.Aggregateliteral across the workspace updated — most were mechanical, but eachReducevsPerEntityclassification was checked against the actual query shape, not guessed.Not a code-level bug fix by itself — today's construction sites already got this right — it closes the door on the next one, by making the type system enforce what used to be an unenforced convention.
This lands independently of #164 (the L4-level
SummaryGroupingfix for #163); once merged, #164 can be simplified to read this field directly instead of re-deriving the same judgment a second time via its own heuristic.Test plan
ReducevsPerEntityclassification verified against the existing PromQL/SQL conformance suites — 100/100 inpromql_conformance.rs, 44/44 inpromql_lowering.rs, full SQL corpora — rather than asserted from first principles alone.cargo build --workspace/cargo test --workspace— clean, zero failures across every crate.cargo clippy --workspace --all-targets --all-features -- -D warnings— clean.cargo fmt --all --check— clean.🤖 Generated with Claude Code