feat(sid): L4 binder rule emits PhysicalExpr::ExactAgg for Sum/Rate/Increase/Count - #201
Merged
Merged
Conversation
…ncrease/Count PR 6 follow-up part 2. The PR 6 follow-up (PR #200) flipped `capability_for` to return `Some(Capability::ExactAgg(...))` for Sum / Rate / Increase / Count{Exact} so the analyzer surfaces those intents as warm-tier candidates. But the L4 binder had no rule to lower them into the algebra — they kept passing through as `PhysicalExpr::Logical`, which the L5 emitter routes to archive. This PR adds the missing binder so the optimizer's plan output finally reflects the analyzer flip. ## What 1. New variant `PhysicalExpr::ExactAgg { agg_type, child }` (`sketch_algebra/physical_expr.rs`). Counterpart to the data-plane `AggKind::ExactAgg` / `AggPayload::ExactAgg` shape. No `SketchEstimate` wrapper because ExactAgg produces the answer directly — the accumulator's value IS the result. 2. New convenience constructor `PhysicalExpr::exact_agg_over_logical`, mirroring the existing `estimate_over_agg` for sketches. 3. New binder rule `bind_exact_agg::BindExactAgg` (`sketch_algebra/rules/bind_exact_agg.rs`) with priority `2` (above `bind_archive_only`'s 1, below the sketch families at 4–6). Maps: - `AggIntent::Sum` → `ExactAgg(Sum)` - `AggIntent::Rate { window }` → `ExactAgg(Increase)` - `AggIntent::Increase { window }` → `ExactAgg(Increase)` - `AggIntent::Count{Exact}` → `ExactAgg(Sum)` (count = sum-of-1s) 4. Wired into `dispatch` in `sketch_algebra/rules/mod.rs`. 5. Exhaustive-match coverage added in three downstream walkers: - `emit::extract_root_sketch_kind` → `None` (no sketch family) - `optimizer::rules::extract_family` → `None` - `physical::colored_dag::allocator::visit` → `StageId::Edge` (same locality as `SketchAgg`; accumulator runs in the precompute pipeline at edge) - `sketch_algebra::tests::collect_sketch_kinds` → recurse into child - `sketch_algebra::tests::binding_is_archive` → `false` (warm path, not cold) 6. Updated two existing tests that asserted Sum's old behaviour: - `bind_no_match_passes_through_logical` → `sum_now_binds_to_exact_agg_after_pr_6_followup` - `phase_b_pattern_only_temporal_sum_falls_through_to_logical` → `phase_b_pattern_only_temporal_sum_binds_to_exact_agg` ## What's not in scope - `bind_exact_agg` rejects `Aggregate{by: [..]}` inputs. A keyed-group ExactAgg follow-up would emit `MultipleSum` / `MultipleIncrease` instead. The dispatch / allocator / emit walkers already handle ExactAgg structurally; only the binder logic gates on `by.is_empty()`. - `AggIntent::Avg` still routes to archive — needs cross-policy Sum+Count join, separate concern. - `AggIntent::Min` / `Max` stay on the quantile-sketch path (DDSketch/KLL answer them via quantile(0)/quantile(1)); adding a MinMax ExactAgg binding would compete with that path unprofiled. Decision deferred. ## Test plan - [x] `cargo check --workspace` clean - [x] `cargo test --workspace --lib --bins` green (one pre-existing `avg_finds_sum_and_count` HashMap-iteration-order flake is unchanged; passes on retry) - [x] 9 new unit tests on `BindExactAgg` covering each intent + negative cases (by-clause / Avg / approximate-Count / zero-window) + priority gate 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2 tasks
zzylol
added a commit
that referenced
this pull request
May 14, 2026
`count_over_time` was routed to the warm-tier ExactAgg path by PRs #200 (capability_for) and #201 (bind_exact_agg) on the theory "count = sum-of-1s". But the data plane has no count accumulator: struct SumAccumulator { sum: f64 } // no count field fn query(&self, statistic, ..) { match statistic { Statistic::Sum | Statistic::Count => Ok(self.sum), // ← both! ... } } `SumAccumulatorUpdater::update_single` does `self.sum += value` — no projection-to-1 anywhere. So a `count_over_time(m[5m])` query matched against an `m` Sum policy returned the **sum of the sample values**, not the count of samples. Silently wrong. This regression was introduced by my PRs #200/#201 — before them `capability_for(Count{Exact})` returned `None` and the query went to the archive engine, which counts correctly. ## Changes - `capability_for(AggIntent::Count { accuracy: Exact })` → `None` (was `Some(ExactAgg(Sum))`). `count_over_time` routes to archive. - `bind_exact_agg` — `Count{Exact}` arm removed; the rule no longer emits `ExactAgg(Sum)` / `ExactAgg(MultipleSum)` for count. - Tests updated: `capability_for_count_exact_routes_to_archive`, `exact_agg_routing_covers_sum_rate_increase_only`, `does_not_bind_count_exact`, `keyed_count_exact_does_not_bind`. `AggIntent::Count { accuracy: Epsilon/EpsilonDelta }` is unchanged — the approximate-count (`count by (...) (count_over_time(...))` distinct-count idiom) still routes to `CardinalityApprox`. ## Follow-up The correct fix — a real `SumCountAccumulator { sum, count }` that answers Sum / Count / Avg — lands with the temporal/spatial-split work. Until then, `count_over_time` and `avg_over_time` are archive-served (correct, just not warm-tier-accelerated). ## Test plan - [x] `cargo check --workspace` clean - [x] `cargo test --workspace --lib --bins` green 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
zzylol
added a commit
that referenced
this pull request
Jul 20, 2026
…ivergences on Count and Min/Max
Fixes the two real behavioral divergences found while auditing
capability_for against sketch_algebra::rules::dispatch (both claim to
answer the same AggIntent -> ASAP-tier-capability question but disagreed):
- Count{non-Exact}: capability_for claimed CardinalityApprox (HLL,
"distinct count" semantics); rules::dispatch's BindCmsOnCount treats
it as a bare frequency point-query (CMS). ASAPController's own
crates/plan/src/bind.rs::readout maps Count to SketchQuery::PointCount,
confirming CMS is correct -- the CardinalityApprox premise never had a
real caller anyway (distinct_over_time/COUNT(DISTINCT) always lower to
AggIntent::Cardinality, never Count). AggIntent::Count{non-Exact} is
unreachable via this repo's own PromQL frontend today regardless
(lower.rs only constructs Count{Exact} or the Extension-based
Frequency intent), so this is a consistency fix, not a live routing
change.
- Min/Max: capability_for claimed QuantileApprox (min = quantile(0),
max = quantile(1)), but no rule in sketch_algebra::rules actually
implements that -- bind_kll_quantile/bind_ddsketch_quantile only ever
match AggIntent::Quantile, never Min/Max, so the promised coverage
didn't exist and Min/Max silently fell through to archive regardless.
ASAPController's own crates/plan/src/boundary.rs treats Min/Max as an
exact mergeable accumulator (SummaryKind::MinMax), same tier as
Sum/Rate/Increase -- correct, since comparing two partial extrema
needs no approximation at all. bind_exact_agg.rs now binds
Min/Max -> AggregationType::MinMax (keyed -> MultipleMinMax), using
the data plane's already-fully-wired MinMaxAccumulator; capability_for
now returns ExactAgg(MinMax) to match.
Also fixes a stale doc-table line (Count{Exact} claimed to return
Some(ExactAgg(Sum)); the actual code has returned None since the
PR #200/#201 revert).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
zzylol
added a commit
that referenced
this pull request
Jul 21, 2026
…ivergences on Count and Min/Max
Fixes the two real behavioral divergences found while auditing
capability_for against sketch_algebra::rules::dispatch (both claim to
answer the same AggIntent -> ASAP-tier-capability question but disagreed):
- Count{non-Exact}: capability_for claimed CardinalityApprox (HLL,
"distinct count" semantics); rules::dispatch's BindCmsOnCount treats
it as a bare frequency point-query (CMS). ASAPController's own
crates/plan/src/bind.rs::readout maps Count to SketchQuery::PointCount,
confirming CMS is correct -- the CardinalityApprox premise never had a
real caller anyway (distinct_over_time/COUNT(DISTINCT) always lower to
AggIntent::Cardinality, never Count). AggIntent::Count{non-Exact} is
unreachable via this repo's own PromQL frontend today regardless
(lower.rs only constructs Count{Exact} or the Extension-based
Frequency intent), so this is a consistency fix, not a live routing
change.
- Min/Max: capability_for claimed QuantileApprox (min = quantile(0),
max = quantile(1)), but no rule in sketch_algebra::rules actually
implements that -- bind_kll_quantile/bind_ddsketch_quantile only ever
match AggIntent::Quantile, never Min/Max, so the promised coverage
didn't exist and Min/Max silently fell through to archive regardless.
ASAPController's own crates/plan/src/boundary.rs treats Min/Max as an
exact mergeable accumulator (SummaryKind::MinMax), same tier as
Sum/Rate/Increase -- correct, since comparing two partial extrema
needs no approximation at all. bind_exact_agg.rs now binds
Min/Max -> AggregationType::MinMax (keyed -> MultipleMinMax), using
the data plane's already-fully-wired MinMaxAccumulator; capability_for
now returns ExactAgg(MinMax) to match.
Also fixes a stale doc-table line (Count{Exact} claimed to return
Some(ExactAgg(Sum)); the actual code has returned None since the
PR #200/#201 revert).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
4 tasks
zzylol
added a commit
that referenced
this pull request
Jul 22, 2026
…#408) * feat(control_plane): adopt asap_sketch::L4Node for the L4 IR (Step B) Retires control_plane's own locally-defined, flat PhysicalExpr L4 algebra (Logical/SketchAgg/SketchEstimate/SketchMerge/ExactAgg) in favor of ASAPController's canonical L4 IR, asap_sketch::{SummaryExpr, L4Node} -- the same move Step 3 of the enum-unification made for SketchKind -> SummaryKind, one layer up. PhysicalExpr is now a thin L5 wrapper: `Committed(L4Plan)` for the common case, plus the two Phase eps.1 placement variants. L4Plan keeps only what asap_sketch genuinely doesn't have -- named LetBinding/Ref fan-in sharing (asap_sketch's own DAG sharing is structural, via Rc, but this crate's rule-firing walk still needs a name to thread a bound value across sibling calls). The 7 bind_kll_quantile/bind_ddsketch_quantile/bind_hll_cardinality/ bind_cms_count/bind_cms_topk/bind_exact_agg/bind_archive_only Rule structs are retired -- their selection/sizing policy (KLL's k rungs, DDSketch-over-KLL priority, TopK recall tiers, wire-cost tie-breaks) is preserved verbatim in the new ControlPlaneCostModel, plugged into asap_plan::bind::implement_tree_in_with via the CostModel trait (rank_candidates + the new size_params hook, ASAPController#146) instead of a bespoke dispatcher -- so schema derivation, col/by computation, and DAG construction are asap_plan::bind's, not a forked copy. Three node shapes get a small local pre-pass in sketch_algebra::lower before delegating, because asap_plan::boundary::implementation_for actively binds them to something this deployment's data plane can't (or deliberately shouldn't) serve: - AggIntent::Count{accuracy: Exact} would bind SummaryKind::Count, which has no data-plane accumulator (PR #200/#201 already established this is wrong -- reverted, stays on archive). - AggIntent::Rate would bind its own SummaryKind::Rate; this deployment represents Rate as an Increase accumulator (rate = increase / window, a query-time division, not a separate accumulator). - AggIntent::Extension (Frequency) and TopK{accuracy: Exact} both decline to bind at all (asap_plan's Extension/exact-TopK coverage gaps -- filed upstream as ASAPController#150 and #151); this is a real, accepted behavior change from the retired bind_cms_count/bind_cms_topk rules, not a bug -- see the updated tests in optimizer/rules/mod.rs and emit/mod.rs. Also fixes a real bug surfaced by the migration: PhysicalExpr now carries Rc<L4Node> (asap_sketch's own DAG-sharing mechanism), so holding a bound PhysicalExpr across an .await point made handle_plan's generated Future !Send, breaking axum::Handler. Scoped the Rc-bearing computation into a synchronous block that resolves down to Send-safe StageConfig output before the first await. Pins ASAPQuery-backend's ASAPController dependency to 12482fd77945ab5771a021c6112750fd9284f8ed (ASAPController PR #146, CostModel::size_params + bind::logical). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * chore: re-pin ASAPController to main (post #146/#154 merge) ASAPController#146 (CostModel::size_params) and #154 (bind::logical visibility) are both merged. Re-pin from the feat/costmodel-size-params-hook branch tip to main's current commit now that both land there. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 5 <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
1 participant
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
PR 6 follow-up part 2. PR #200 flipped
capability_forto route Sum / Rate / Increase / Count{Exact} to ExactAgg. This PR adds the L4 binder rule so the optimizer's plan output reflects the flip — without it, those intents passed through asPhysicalExpr::Logicaland the L5 emitter routed them to archive anyway.What's new
PhysicalExpr::ExactAgg { agg_type, child }variant — L4 counterpart to the data-planeAggKind::ExactAgg. NoSketchEstimatewrapper because ExactAgg produces the answer directly.bind_exact_agg::BindExactAggrule, priority 2:Sum→ExactAgg(Sum)Rate { window }/Increase { window }→ExactAgg(Increase)Count { Exact }→ExactAgg(Sum)(count_over_time = sum-of-1s)dispatch; exhaustive-match coverage added inemit::extract_root_sketch_kind,optimizer::rules::extract_family,colored_dag::allocator(StageId::Edge, same locality as SketchAgg), and the test walkers.Stays on archive
Avg— needs cross-policy Sum+Count join.Min/Max— quantile-sketch path (DDSketch/KLL via quantile(0)/quantile(1)) is unchanged.by: [..]Sum/Rate/Increase —MultipleSum/MultipleIncreaseExactAgg binding deferred to a follow-up.Test plan
cargo check --workspacecleancargo test --workspace --lib --binsgreen (one pre-existing HashMap-iteration-order flake unchanged)BindExactAgg🤖 Generated with Claude Code