From 76d4e5b9f9ff047b3c895f20c2cec97cac281984 Mon Sep 17 00:00:00 2001 From: zz_y Date: Thu, 14 May 2026 08:07:44 -0600 Subject: [PATCH] =?UTF-8?q?fix(sid):=20revert=20count=5Fover=5Ftime=20?= =?UTF-8?q?=E2=86=92=20ExactAgg(Sum)=20mis-routing?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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) --- .../src/sketch_algebra/capability.rs | 53 +++++++++-------- .../sketch_algebra/rules/bind_exact_agg.rs | 59 ++++++++++--------- 2 files changed, 59 insertions(+), 53 deletions(-) diff --git a/control_plane/src/sketch_algebra/capability.rs b/control_plane/src/sketch_algebra/capability.rs index 01fcef4f..6a0536d9 100644 --- a/control_plane/src/sketch_algebra/capability.rs +++ b/control_plane/src/sketch_algebra/capability.rs @@ -304,13 +304,19 @@ pub fn capability_for(intent: &AggIntent) -> Option { // accuracy is non-Exact and we hand it to the cardinality // sketch path. // - // PR-6 follow-up: exact count = sum-of-1s, which is - // served by the `AggregationType::Sum` exact-precompute - // operator at the ASAP tier. Returning that capability - // lets the analyzer route `count_over_time` to a ASAP-tier - // ExactAgg sid instead of falling through to archive. + // Exact count routes to archive (`None`). The PR #200/#201 + // follow-up flipped this to `ExactAgg(Sum)` on the theory + // "count = sum-of-1s" — but the data plane has no count + // accumulator. `SumAccumulator` only tracks `sum: f64` and + // its `query` returns `self.sum` for BOTH `Statistic::Sum` + // and `Statistic::Count`, so a `count_over_time` query + // matched against a `Sum` policy returns the sum of the + // sample VALUES, not the count of samples. Reverted here + // until a real `SumCountAccumulator` lands (the + // temporal/spatial-split work) — archive counts correctly + // in the meantime. if is_exact(accuracy) { - Some(Capability::ExactAgg(AggregationType::Sum)) + None } else { Some(Capability::CardinalityApprox) } @@ -691,18 +697,18 @@ mod tests { } #[test] - fn capability_for_count_exact_routes_to_exact_agg_sum() { - // PR-6 follow-up: `count_over_time` lowers to - // `Count{accuracy:Exact}`; count = sum-of-1s, so the ASAP-tier - // ExactAgg path uses `AggregationType::Sum`. Pre-follow-up - // this returned `None` and the analyzer routed to archive. + fn capability_for_count_exact_routes_to_archive() { + // `count_over_time` lowers to `Count{accuracy:Exact}`. The + // PR #200/#201 follow-up briefly routed this to + // `ExactAgg(Sum)`, but the data plane has no count + // accumulator — `SumAccumulator` returns its `sum` for both + // `Statistic::Sum` and `Statistic::Count`, so the result was + // sum-of-values, not sample-count. Reverted to `None` (archive + // routing) until a real `SumCountAccumulator` lands. let intent = AggIntent::Count { accuracy: AccuracyTarget::Exact, }; - assert_eq!( - capability_for(&intent), - Some(Capability::ExactAgg(AggregationType::Sum)) - ); + assert_eq!(capability_for(&intent), None); } #[test] @@ -1026,12 +1032,10 @@ mod tests { // ── capability_for: ExactAgg dormancy ──────────────────────────────── #[test] - fn pr_6_follow_up_flipped_sum_rate_increase_count_exact() { - // PR 6 first landed `Capability::ExactAgg` dormant — variant - // wired into `is_satisfied_by` but `capability_for` still - // returned `None` for Sum / Rate / Increase / Count{Exact}. - // This test locks in the follow-up that flipped those four - // intents to route through ASAP-tier ExactAgg state. + fn exact_agg_routing_covers_sum_rate_increase_only() { + // `Capability::ExactAgg` routing covers the three intents the + // data plane has a real accumulator for: `Sum` (SumAccumulator) + // and `Rate` / `Increase` (IncreaseAccumulator). assert_eq!( capability_for(&AggIntent::Sum), Some(Capability::ExactAgg(AggregationType::Sum)) @@ -1048,14 +1052,15 @@ mod tests { }), Some(Capability::ExactAgg(AggregationType::Increase)) ); + // `Count{Exact}` (count_over_time) and `Avg` both need a real + // count accumulator that doesn't exist yet — they route to + // archive until `SumCountAccumulator` lands. assert_eq!( capability_for(&AggIntent::Count { accuracy: AccuracyTarget::Exact, }), - Some(Capability::ExactAgg(AggregationType::Sum)) + None ); - // Avg stays on archive — needs cross-policy join (Sum + Count) - // that the L4 binder doesn't yet emit. Tracked as follow-up. assert_eq!(capability_for(&AggIntent::Avg), None); } diff --git a/control_plane/src/sketch_algebra/rules/bind_exact_agg.rs b/control_plane/src/sketch_algebra/rules/bind_exact_agg.rs index 961ef0b0..3d5819cd 100644 --- a/control_plane/src/sketch_algebra/rules/bind_exact_agg.rs +++ b/control_plane/src/sketch_algebra/rules/bind_exact_agg.rs @@ -1,5 +1,5 @@ -//! `BindExactAgg` — emits `PhysicalExpr::ExactAgg` for the four exact-aggregation -//! intents that the PR 6 follow-up flipped to ASAP-tier routing. +//! `BindExactAgg` — emits `PhysicalExpr::ExactAgg` for the exact-aggregation +//! intents the ASAP tier can serve from a precompute accumulator. //! //! ## Coverage //! @@ -8,7 +8,6 @@ //! | `Sum` | `agg_type: AggregationType::Sum` | //! | `Rate { .. }` | `agg_type: AggregationType::Increase` | //! | `Increase { .. }` | `agg_type: AggregationType::Increase` | -//! | `Count { accuracy: Exact }` | `agg_type: AggregationType::Sum` | //! //! `Rate` and `Increase` share the `Increase` accumulator because rate is //! computed as `increase / window_seconds` — a scalar division on the @@ -16,16 +15,18 @@ //! division (when needed) is the L5 emitter's responsibility, not the //! L4 binder's. //! -//! `Count{accuracy:Exact}` maps to `Sum` because `count_over_time` is -//! exactly the sum of presence indicators (each sample contributes 1). -//! There's no dedicated `Count` `AggregationType` variant; `Sum` -//! covers the shape. -//! //! ## What this rule does NOT bind //! -//! - `AggIntent::Avg` — needs cross-policy join (Sum / Count), no single -//! `AggregationType` covers it. Stays on archive until the L4 binder -//! gains a join rule. +//! - `AggIntent::Count { accuracy: Exact }` — `count_over_time`. The +//! PR #200/#201 follow-up bound this to `AggregationType::Sum` on the +//! "count = sum-of-1s" theory, but the data plane has no count +//! accumulator: `SumAccumulator` only tracks `sum: f64` and returns +//! it for both `Statistic::Sum` and `Statistic::Count`, so the result +//! is the sum of sample VALUES, not the sample count. Reverted — +//! `count_over_time` routes to archive (which counts correctly) +//! until a real `SumCountAccumulator` lands. +//! - `AggIntent::Avg` — needs a `(sum, count)` accumulator, same +//! missing piece. Stays on archive until `SumCountAccumulator` lands. //! - `AggIntent::Quantile { Exact }` / `Cardinality { Exact }` / //! `TopK { Exact }` / `Frequency { Exact }` — the exact-accuracy //! variants of approximate-by-default intents. No exact-precompute @@ -111,18 +112,10 @@ impl Rule for BindExactAgg { AggregationType::Increase } } - AggIntent::Count { - accuracy: AccuracyTarget::Exact, - } => { - // count_over_time = sum-of-1s, so it lowers through the - // Sum/MultipleSum accumulator family — same as - // `AggIntent::Sum` above. - if keyed { - AggregationType::MultipleSum - } else { - AggregationType::Sum - } - } + // `AggIntent::Count{Exact}` (count_over_time) is intentionally + // NOT bound here — see the module doc. It needs a real + // count accumulator, which doesn't exist yet; binding it to + // `Sum` returns sum-of-values instead of sample-count. _ => return None, }; @@ -197,13 +190,17 @@ mod tests { } #[test] - fn binds_count_exact_to_exact_agg_sum() { - check_binds( + fn does_not_bind_count_exact() { + // `count_over_time` (Count{Exact}) is NOT bound — the data + // plane has no count accumulator. Routes to archive instead. + // See the module doc. + let expr = agg_over( AggIntent::Count { accuracy: AccuracyTarget::Exact, }, - AggregationType::Sum, + "test_metric", ); + assert!(BindExactAgg.apply(&expr, &AccuracyTarget::Exact).is_none()); } #[test] @@ -282,13 +279,17 @@ mod tests { } #[test] - fn keyed_count_exact_binds_to_multiple_sum() { - check_keyed_binds( + fn keyed_count_exact_does_not_bind() { + // `count by (...) (count_over_time(...))` — Count{Exact} is + // unbound regardless of keying; no count accumulator exists. + let expr = agg_over_with_by( AggIntent::Count { accuracy: AccuracyTarget::Exact, }, - AggregationType::MultipleSum, + "test_metric", + vec![0], ); + assert!(BindExactAgg.apply(&expr, &AccuracyTarget::Exact).is_none()); } #[test]