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]