feat(sid): wire ExactAgg routing — capability_for + sid metadata - #200
Merged
Merged
Conversation
PR 6 follow-up. PR 6 (folded into #196) landed `Capability::ExactAgg( AggregationType)` as a dormant variant — wired into `is_satisfied_by` but not produced by `capability_for`, so the analyzer never emitted candidates targeting it. This PR makes the wiring live. ## Two changes 1. **Data plane** (`storage_engines/sketch_db/index/mod.rs`): `ingest_precompute_for_agg_config` now registers `SketchInstanceMetadata.capability = Some(Capability::ExactAgg( agg_cfg.aggregation_type))` for fresh ExactAgg-backed sids. Pre-PR this field was unconditionally `None`, so capability-matching couldn't see warm-tier ExactAgg state. The legacy precompute query path (`SketchStore::query_precomputes_by_agg`) keeps working in parallel — no behaviour change for that read path. 2. **Control plane** (`sketch_algebra/capability.rs::capability_for`): Four intents flipped from `None` → `Some(Capability::ExactAgg(...))`: - `AggIntent::Sum` → `ExactAgg(Sum)` - `AggIntent::Rate { .. }` → `ExactAgg(Increase)` (rate = increase/time) - `AggIntent::Increase { .. }` → `ExactAgg(Increase)` - `AggIntent::Count { Exact }` → `ExactAgg(Sum)` (count_over_time = sum-of-1s) These were previously routed to the archive engine because warm-tier sketches don't cover exact aggregations. With ExactAgg state addressable at the analyzer, the warm-tier path becomes the primary route; archive remains the fallback on capability-miss. ## What stays on archive - `AggIntent::Avg` — Avg = Sum/Count, which requires joining two separate ExactAgg policies at query time. The L4 binder doesn't emit that pattern yet; routing Avg to ExactAgg today would surface capability-misses (no single sid satisfies it). Tracked as a future follow-up. - `AggIntent::Quantile { Exact }`, `Cardinality { Exact }`, `TopK { Exact }`, `Frequency { Exact }` — exact-accuracy variants of approximate-by-default intents. There's no exact-precompute shape for these in the AggregationType vocabulary; they need HashAgg / SortAgg / SortMerge, none of which run at the warm tier. - Archive-only intents (Absent / Present / Delta / Deriv / PredictLinear / HoltWinters / Idelta / Irate / Resets / Changes). PromQL-specific operations with no aggregation semantics worth precomputing at the warm tier. ## Test plan - [x] `cargo check --workspace` clean - [x] `cargo test --workspace --lib --bins` — green (the pre-existing `avg_finds_sum_and_count` HashMap-iteration flake is unchanged; retries succeed) - [x] Updated existing assertion `capability_for_sum_returns_none`, `..count_exact_returns_none`, `..rate_increase_return_none`, and the explicit dormancy test to lock in the flipped behavior. ## What's still owed - L4 binder rules that emit `PhysicalExpr::ExactAgg` nodes — the variant exists on the L4 algebra side, but no rule currently produces it. Until those rules land, the analyzer's flip is observable only through `WarmTierAnalysis::candidates` (the candidate is generated with the right `required_capability`) — the optimizer's plan output doesn't change yet. - Retire `PrecomputedOutput.aggregation_id` (PR 5 left this field for legacy compat; with all production reads on `policy_fp`, it's a one-line deletion). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
4 tasks
zzylol
added a commit
that referenced
this pull request
May 14, 2026
…ncrease/Count (#201) 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>
3 tasks
zzylol
added a commit
that referenced
this pull request
May 14, 2026
…everse index (#203) Closes the O(1) "which sids belong to this policy?" lookup gap that the merged-sid-identity chain left open. PR 6 follow-up #200 populated `SketchInstanceMetadata.capability` with `Some(Capability::ExactAgg(agg_type))`; this PR completes the direct-index story by adding the fingerprint back-reference and a matching `policy_fp → {sids}` index on `SketchStore`. ## What - New field `SketchInstanceMetadata.policy_fp: PolicyFingerprint`. Populated from `output.policy_fp` at registration time in `SketchStore::ingest_precompute_for_agg_config`. Other construction sites (OTel sketch ingest, tests, benches) populate `PolicyFingerprint::UNSET` — they don't carry a source `AggregationConfig` and the reverse index intentionally skips them. - New field `SketchStore.policy_to_sids: RwLock<HashMap<PolicyFingerprint, BTreeSet<u64>>>`. Maintained automatically by `register` / `remove_instance` / `remove_instances_for_agg_config`. UNSET entries are not recorded. - New public methods: - `SketchStore::sids_for_policy(fp) -> Vec<u64>` — O(1) lookup, sorted return order (BTreeSet under the hood). - `SketchStore::policy_count() -> usize` — telemetry. ## Why this matters Before this PR the query path resolved `Candidate → [sids]` by walking `SketchStore::instances_matching(metric, gbk)` and re-deriving the policy fingerprint per sid. With the index landed, the analyzer can go straight from `Candidate { metric, group_by, capability, window } → policy_fp` (via `PolicyRegistry::find_matching`, future PR) and `policy_fp → [sids]` (this PR) in two O(1) hops. ## What this PR explicitly does NOT change - OTel sketch ingest (`drivers/ingest/otel.rs`) still registers with `policy_fp: UNSET`. Sketches arrive with their shape (kind + config) embedded in the OTLP DP, not a policy reference. Deriving the fp at ingest by content-matching against `PolicyRegistry` is a natural follow-up but out of scope here. - `PolicyRegistry::find_matching` (analyzer-side candidate → policy resolution) doesn't exist yet. The plumbing landed (registry + reverse index); the matching function is the missing piece for the full O(1) query lookup. ## Test plan - [x] 6 new unit tests on the reverse index: - `sids_for_policy_returns_empty_for_unset_or_missing` - `register_indexes_one_sid_under_its_policy` - `register_groups_multiple_sids_under_one_policy` - `register_separates_distinct_policies` - `unset_policy_sids_are_not_in_reverse_index` - `remove_instance_drops_reverse_index_entry` - [x] `cargo check --workspace` clean - [x] `cargo test --workspace --lib --bins` green (one pre-existing `avg_finds_sum_and_count` HashMap-iteration flake unchanged; passes on retry) 🤖 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. PR 6 (folded into #196) landed
Capability::ExactAgg(AggregationType)as a dormant variant — wired intois_satisfied_bybut not produced bycapability_for. This PR makes the wiring live.Two changes
Data plane:
SketchInstanceMetadata.capabilityis nowSome(Capability::ExactAgg(agg_type))on ExactAgg-backed sids (wasNone). Capability-matching can now see warm-tier ExactAgg state.Control plane: four intents flipped from
None→Some(Capability::ExactAgg(...)):AggIntent::Sum→ExactAgg(Sum)AggIntent::Rate { .. }→ExactAgg(Increase)AggIntent::Increase { .. }→ExactAgg(Increase)AggIntent::Count { Exact }→ExactAgg(Sum)(count_over_time = sum-of-1s)Stays on archive
Avg— needs cross-policy Sum + Count join; L4 binder doesn't emit that pattern yet.Quantile { Exact }etc.) — no exact-precompute equivalent inAggregationType.Test plan
cargo check --workspacecleancargo test --workspace --lib --binsgreenWhat's still owed
PhysicalExpr::ExactAggnodes — variant exists, rules don't. Until they land, the analyzer's flip is observable inWarmTierAnalysis::candidatesonly; the optimizer's plan output doesn't change.PrecomputedOutput.aggregation_id(one-line deletion follow-up).🤖 Generated with Claude Code