From bfdd6437f4c3e9455740eb263b8ee8ff9561d460 Mon Sep 17 00:00:00 2001 From: zz_y Date: Mon, 18 May 2026 10:34:56 -0600 Subject: [PATCH] refactor(query): preserve rate/sum_over_time distinction in analyzer (kill PromQL string re-parse from PR #292) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR #292 fixed a real bug — the engine couldn't tell `rate(metric[r])` from `sum_over_time(metric[r])` because the analyzer collapses both to `Capability::ExactAgg(Sum)`. The fix was a `query_contains_rate_call` walker in the engine that re-parsed the raw PromQL string at dispatch time to disambiguate. That worked but was a lossy-lowering smell: the analyzer is supposed to be the single source of truth for query intent, and its lowering should preserve enough info that the engine doesn't need to re-parse. This change carries the distinction through the analyzer's typed output: - Add `OuterFn { Plain, Rate }` to `sketch_algebra::capability`. `Rate` means the original PromQL had `rate(...)` / `irate(...)` anywhere in its expression tree (possibly nested inside an outer `sum by (...) (...)`). - Add `outer_fn: OuterFn` to `ASAPTierCandidate`. The analyzer's existing `trace_from_promql` walker populates it in the same pass that captures the outer-function name / scalar args / range. - Engine's reducer dispatch in both `execute()` and `execute_range_promql_modern()` reads `candidate.outer_fn` instead of calling a `query_contains_rate_call(query)` helper. - Delete `query_contains_rate_call` (~42 lines including doc). Regression tests: - Analyzer-side: `rate_and_sum_over_time_share_capability_but_differ_on_outer_fn` + 6 per-shape outer_fn assertions, including the composed `sum by (zone) (rate(metric[r]))` case where outer fn name is `sum` but `outer_fn` MUST be `Rate`. - Engine-side: `execute_sum_over_time_dispatches_to_plain_exact_agg_reducer` pins the per-window reducer's output (sum of raw window values, NOT divided by the range — if the dispatch ever regresses to "all ExactAgg(Sum) + range > 0 → rate path", the asserted value changes by a factor of `range_seconds`). - Engine-side: `analyzer_candidate_outer_fn_distinguishes_rate_from_sum_over_time` documents the typed contract the engine reads off. `try_topk_over_rate_fallback` remains — it's a different concern (shape-extraction for topk-over-rate where the analyzer emits FrequencyTopk candidates the ExactAgg(Sum) sids don't satisfy). That's a structural analyzer-side change deferred to a follow-up; this PR's scope is the lossy-lowering smell only. Verification: - `cargo build -p data_plane` clean. - `cargo test -p data_plane --lib`: 745 passed (was 743 post-#292; +2 new regression tests). 0 failures. - `cargo test -p control_plane --lib`: 745 passed (was 738; +7 new analyzer tests). 0 failures. - Smoke test (`bash /mydata/mvp-smoke-test/run_smoke.sh` + per-query curl): all 5 spec queries return non-empty `data_source: asap_query` results: * `rate(http_requests_total[5m])` → 4 zones, per-second rates * `sum by (zone) (rate(http_requests_total[5m]))` → 4 zones * `topk(5, sum by (zone) (rate(http_requests_total[5m])))` → 4 zones * `sum_over_time(http_requests_total[5m])` → plain reducer, raw sum (25988) * `quantile_over_time(0.99, http_requests_total_latency_ms[5m])` → 4 zones p99 - `rg "query_contains_rate_call" data_plane/src/` → only doc-comment references documenting the retirement; no function call sites. Co-Authored-By: Claude Opus 4.7 (1M context) --- control_plane/src/asap_tier_analysis.rs | 154 +++++++++++- .../src/sketch_algebra/capability.rs | 39 +++ .../query_engines/asap_query_engine/engine.rs | 238 ++++++++++++++---- 3 files changed, 373 insertions(+), 58 deletions(-) diff --git a/control_plane/src/asap_tier_analysis.rs b/control_plane/src/asap_tier_analysis.rs index 57249ec6..60300668 100644 --- a/control_plane/src/asap_tier_analysis.rs +++ b/control_plane/src/asap_tier_analysis.rs @@ -53,7 +53,9 @@ use crate::intent_algebra::agg_intent::AggIntent; use crate::intent_algebra::query_expr::QueryExpr; use crate::query_parser::{parse_query, parse_query_expr_canonical}; -pub use crate::sketch_algebra::capability::{capability_for, Capability, SketchKindHandle}; +pub use crate::sketch_algebra::capability::{ + capability_for, Capability, OuterFn, SketchKindHandle, +}; // ── Public types ───────────────────────────────────────────────────────────── @@ -84,6 +86,13 @@ pub struct ASAPTierCandidate { /// byte-for-byte. Drives the candidate → policy filter match in /// `find_matching_policies`. pub spatial_filter_canonical: String, + /// PromQL outer-function flavour — `Rate` if the expression + /// contains `rate(...)` / `irate(...)` anywhere in the tree, + /// `Plain` otherwise. Preserves the rate-vs-plain distinction the + /// `AggIntent::Sum` collapse erases, so the engine's reducer + /// dispatch can branch on the typed candidate instead of re-parsing + /// the raw PromQL string. See [`OuterFn`] for the taxonomy. + pub outer_fn: OuterFn, } /// Whole-query analysis result. @@ -217,6 +226,7 @@ pub fn analyze_promql_for_asap_tier(metricsql: &str) -> ASAPTierAnalysis { function_args: trace.function_args.clone(), range_seconds: trace.range_seconds, spatial_filter_canonical: spatial_filter_canonical.clone(), + outer_fn: trace.outer_fn, }); } None => { @@ -323,17 +333,30 @@ fn intent_kind_label(intent: &AggIntent) -> &'static str { } } -/// Telemetry-only metadata recovered from the raw PromQL AST: the -/// outer function name, leading scalar args, and the matrix selector's -/// `[r]` range in seconds. None of this drives capability dispatch — -/// dispatch is `capability_for(&AggIntent)`. This walker exists ONLY -/// so the `ASAPTierCandidate.function` / `.function_args` / `.range_seconds` -/// fields populate for downstream logging and the reducer's range hint. +/// Metadata recovered from the raw PromQL AST that the lowered +/// `AggIntent` doesn't carry: the outer function name, leading scalar +/// args, matrix selector's `[r]` range in seconds, and the rate-vs-plain +/// outer-function flavour ([`OuterFn`]) used by engine reducer +/// dispatch. +/// +/// The `function` / `function_args` / `range_seconds` fields are for +/// telemetry + the reducer's range hint. The `outer_fn` field is the +/// load-bearing signal that lets the engine pick `evaluate_exact_agg` +/// vs `evaluate_exact_agg_rate` for `Capability::ExactAgg(Sum)` +/// candidates — preserving the rate-vs-plain distinction that the +/// `AggIntent::Sum` collapse erases. #[derive(Debug, Default)] struct PromqlTrace { function: String, function_args: Vec, range_seconds: u64, + /// Set to `OuterFn::Rate` when ANY `rate(...)` or `irate(...)` + /// call is found anywhere in the expression tree; otherwise + /// `OuterFn::Plain`. The flag-style detection mirrors what the + /// retired `query_contains_rate_call` engine helper used to do + /// over the raw query string — done here once so the engine reads + /// it off the typed candidate. + outer_fn: OuterFn, } fn trace_from_promql(metricsql: &str) -> PromqlTrace { @@ -349,8 +372,18 @@ fn trace_from_promql(metricsql: &str) -> PromqlTrace { fn walk_ast_for_trace(expr: &Expr, t: &mut PromqlTrace) { match expr { Expr::Call(call) => { + let name = call.func.name.to_lowercase(); if t.function.is_empty() { - t.function = call.func.name.to_lowercase(); + t.function = name.clone(); + } + // Flag `rate(...)` / `irate(...)` ANYWHERE in the tree — + // mirrors the retired `query_contains_rate_call` walker. + // For composed shapes like `sum by (zone) (rate(metric[r]))` + // the FIRST function set above is `"sum"` (the outer + // Aggregate), but `outer_fn` must still report `Rate` so + // the engine dispatches through `evaluate_exact_agg_rate`. + if matches!(name.as_str(), "rate" | "irate") { + t.outer_fn = OuterFn::Rate; } for a in &call.args.args { if let Expr::NumberLiteral(nl) = a.as_ref() { @@ -789,6 +822,110 @@ mod tests { assert_eq!(a.candidates[0].group_by_keys, keys(&["zone"])); } + // ── outer_fn — rate vs plain disambiguation ────────────────────────── + // + // Regression coverage for the PR that retired the engine's + // `query_contains_rate_call` raw-PromQL re-parser. The analyzer's + // lowerer collapses `rate(metric[r])`, `sum_over_time(metric[r])`, + // `sum(metric)`, and the bare selector all onto `AggIntent::Sum` / + // `Capability::ExactAgg(Sum)` — so the engine can't tell from the + // capability alone which the user wrote. The `outer_fn` field on + // `ASAPTierCandidate` carries the rate-vs-plain distinction so the + // engine's reducer dispatch is a typed branch instead of a raw-PromQL + // re-parse. + + #[test] + fn rate_candidate_carries_outer_fn_rate() { + let a = analyze_promql_for_asap_tier("rate(http_requests_total[5m])"); + assert!(a.unsupported.is_none(), "{a:?}"); + assert_eq!(a.candidates[0].outer_fn, OuterFn::Rate, "{a:?}"); + } + + #[test] + fn irate_candidate_carries_outer_fn_rate() { + let a = analyze_promql_for_asap_tier("irate(http_requests_total[5m])"); + assert!(a.unsupported.is_none(), "{a:?}"); + assert_eq!(a.candidates[0].outer_fn, OuterFn::Rate, "{a:?}"); + } + + #[test] + fn sum_over_time_candidate_carries_outer_fn_plain() { + // `sum_over_time(metric[r])` shares `Capability::ExactAgg(Sum)` + // with `rate(metric[r])` — the capability alone can't + // disambiguate. The `outer_fn` field MUST report `Plain` so + // the engine takes the per-window reducer (no rate divisor). + let a = analyze_promql_for_asap_tier("sum_over_time(http_requests_total[5m])"); + assert!(a.unsupported.is_none(), "{a:?}"); + assert_eq!( + a.candidates[0].required_capability, + Capability::ExactAgg(AggregationType::Sum), + "{a:?}" + ); + assert_eq!(a.candidates[0].outer_fn, OuterFn::Plain, "{a:?}"); + } + + #[test] + fn sum_by_candidate_carries_outer_fn_plain() { + let a = analyze_promql_for_asap_tier("sum by (zone) (http_requests_total)"); + assert!(a.unsupported.is_none(), "{a:?}"); + assert_eq!(a.candidates[0].outer_fn, OuterFn::Plain, "{a:?}"); + } + + #[test] + fn bare_selector_candidate_carries_outer_fn_plain() { + let a = analyze_promql_for_asap_tier("http_requests_total{zone=\"z0\"}"); + assert!(a.unsupported.is_none(), "{a:?}"); + assert_eq!(a.candidates[0].outer_fn, OuterFn::Plain, "{a:?}"); + } + + #[test] + fn sum_by_over_rate_candidate_carries_outer_fn_rate() { + // Composed shape `sum by (zone) (rate(metric[5m]))` — the + // outer function NAME is `"sum"` (the trace's `.function` + // field) but `outer_fn` MUST be `Rate` because the inner + // `rate(...)` call needs the rate-divisor reducer. This is + // the case that motivated the original `query_contains_rate_call` + // walker — now satisfied by walking the AST once in the + // analyzer and emitting the typed `OuterFn::Rate` flag. + let a = analyze_promql_for_asap_tier( + "sum by (zone) (rate(http_requests_total[5m]))", + ); + assert!(a.unsupported.is_none(), "{a:?}"); + assert_eq!( + a.candidates[0].required_capability, + Capability::ExactAgg(AggregationType::Sum), + "{a:?}" + ); + assert_eq!(a.candidates[0].outer_fn, OuterFn::Rate, "{a:?}"); + // Range is lifted from the inner rate's matrix selector. + assert_eq!(a.candidates[0].range_seconds, 300, "{a:?}"); + } + + #[test] + fn rate_and_sum_over_time_share_capability_but_differ_on_outer_fn() { + // Both collapse to `Capability::ExactAgg(Sum)`; the engine MUST + // disambiguate via the typed `outer_fn` field, not by string- + // parsing the raw PromQL. This test pins the asymmetry the + // engine's dispatch reads off. + let rate = analyze_promql_for_asap_tier("rate(http_requests_total[5m])"); + let sot = analyze_promql_for_asap_tier( + "sum_over_time(http_requests_total[5m])", + ); + assert_eq!( + rate.candidates[0].required_capability, + sot.candidates[0].required_capability, + "rate and sum_over_time should produce the same Capability" + ); + assert_ne!( + rate.candidates[0].outer_fn, + sot.candidates[0].outer_fn, + "rate and sum_over_time MUST differ on outer_fn so the engine \ + can dispatch correctly without re-parsing the raw PromQL" + ); + assert_eq!(rate.candidates[0].outer_fn, OuterFn::Rate); + assert_eq!(sot.candidates[0].outer_fn, OuterFn::Plain); + } + // ── Unsupported / rejected shapes ──────────────────────────────────── #[test] @@ -947,6 +1084,7 @@ mod tests { function_args: Vec::new(), range_seconds, spatial_filter_canonical: spatial_filter_canonical.to_string(), + outer_fn: OuterFn::default(), } } diff --git a/control_plane/src/sketch_algebra/capability.rs b/control_plane/src/sketch_algebra/capability.rs index 09d585d5..543569cc 100644 --- a/control_plane/src/sketch_algebra/capability.rs +++ b/control_plane/src/sketch_algebra/capability.rs @@ -112,6 +112,45 @@ pub enum Capability { ExactAgg(AggregationType), } +/// PromQL outer-function flavour carried on each `ASAPTierCandidate` so +/// the engine can distinguish `rate(metric[r])` / `irate(...)` from +/// `sum_over_time(metric[r])` / `sum(metric)` / bare selector WITHOUT +/// re-parsing the raw PromQL string. +/// +/// Background: the lowerer collapses every `AggFunc` in +/// `{Sum, Rate, Increase, Delta}` onto a single `AggIntent::Sum`, which +/// `capability_for` then maps to `Capability::ExactAgg(Sum)`. That +/// collapse erases the rate-vs-plain distinction the engine needs to +/// decide between the plain ExactAgg reducer and the rate-divisor +/// reducer (`evaluate_exact_agg_rate`). Before this enum landed the +/// engine re-walked the raw PromQL via a `query_contains_rate_call` +/// helper to recover the distinction; that was a lossy-lowering smell. +/// +/// The walker that populates this lives in `asap_tier_analysis.rs` +/// (`trace_from_promql`) — it sets `Rate` if ANY `rate(...)` or +/// `irate(...)` Call appears anywhere in the expression tree, otherwise +/// `Plain`. The taxonomy is intentionally minimal: today the engine +/// only branches on "needs rate divisor or not". Future shape-specific +/// dispatch (e.g. separating `increase` from `sum`) can extend this +/// enum without touching the `Capability` algebra. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, Default)] +pub enum OuterFn { + /// No rate-style outer function in the expression — bare selector, + /// `sum(metric)`, `sum by (...) (metric)`, `sum_over_time(metric[r])`, + /// `increase(metric[r])`, `count_over_time(metric[r])`, etc. The + /// engine dispatches to the plain per-window reducer. This is the + /// default — `Default::default()` returns `Plain` so candidates + /// built without an explicit outer-fn (test fixtures, fallback + /// paths) get the safe non-rate dispatch. + #[default] + Plain, + /// `rate(metric[r])` or `irate(metric[r])` appears in the expression + /// (possibly nested inside an outer `sum by (...) (...)`). The + /// engine dispatches to `evaluate_exact_agg_rate`, which divides by + /// the range to produce events-per-second. + Rate, +} + /// Compact, hashable handle for sketch implementation choice. Mirrors /// [`SketchKind`] but adds the `CmsWithHeap` and `Any` query-side /// concepts (which aren't sketch families, they're dispatch hints). diff --git a/data_plane/src/query_engines/asap_query_engine/engine.rs b/data_plane/src/query_engines/asap_query_engine/engine.rs index 1d3177f9..fc4f3763 100644 --- a/data_plane/src/query_engines/asap_query_engine/engine.rs +++ b/data_plane/src/query_engines/asap_query_engine/engine.rs @@ -261,49 +261,6 @@ impl ASAPQueryEngine { }) } - /// Detect whether the raw PromQL contains a `rate(...)` or - /// `irate(...)` call anywhere in the expression tree. Used by the - /// engine to route ExactAgg(Sum) candidates through the - /// rate-divisor reducer (`evaluate_exact_agg_rate`) instead of the - /// plain per-window reducer. - /// - /// The control plane's analyzer collapses `rate(metric[r])` to the - /// same `Capability::ExactAgg(Sum)` candidate that `sum(metric)` - /// uses — the trace carries `range_seconds` and `function` strings - /// but for the composed shape `sum by (zone) (rate(metric[r]))` the - /// outer function string is `"sum"` (not `"rate"`), so we can't - /// disambiguate `sum_over_time(...)` from `sum(rate(...))` from the - /// candidate alone. Walking the raw PromQL AST is the cleanest - /// disambiguator that doesn't require analyzer changes. - /// - /// Returns `false` for unparseable input (the analyzer would have - /// already rejected — defensive). - fn query_contains_rate_call(query: &str) -> bool { - use promql_parser::parser::Expr; - let ast = match promql_parser::parser::parse(query) { - Ok(a) => a, - Err(_) => return false, - }; - fn walk(expr: &Expr) -> bool { - match expr { - Expr::Call(call) => { - let name = call.func.name.to_lowercase(); - if name == "rate" || name == "irate" { - return true; - } - call.args.args.iter().any(|a| walk(a)) - } - Expr::Aggregate(agg) => walk(&agg.expr), - Expr::Paren(p) => walk(&p.expr), - Expr::Subquery(sq) => walk(&sq.expr), - Expr::Binary(b) => walk(&b.lhs) || walk(&b.rhs), - Expr::Unary(u) => walk(&u.expr), - _ => false, - } - } - walk(&ast) - } - /// Detect a `topk(k, )` (or `bottomk`) at the root of the /// PromQL AST and lift `(k, inner_metric, inner_group_by_keys, /// inner_range_seconds, is_topk)`. The `is_topk` flag distinguishes @@ -760,8 +717,19 @@ impl ASAPQueryEngine { // / `evaluate_exact_agg_rate` for the per-path semantics. let result = match &candidate.required_capability { crate::storage_engines::sketch_db::index::Capability::ExactAgg(agg_type) => { + // Dispatch off the typed `outer_fn` carried on the + // analyzer candidate — `OuterFn::Rate` means the + // original PromQL had a `rate(...)` / `irate(...)` + // call somewhere, so we need the rate-divisor + // reducer. Plain shapes (`sum_over_time(...)`, + // `sum(...)`, bare selector) take the per-window + // reducer. Before the analyzer carried this field + // the engine re-walked the raw PromQL via the + // `query_contains_rate_call` helper to recover the + // distinction; that was lossy-lowering smell and is + // gone. let use_rate_path = candidate.range_seconds > 0 - && Self::query_contains_rate_call(query) + && candidate.outer_fn == control_plane::asap_tier_analysis::OuterFn::Rate && matches!( agg_type, crate::storage_engines::sketch_db::data::AggregationType::Sum @@ -1262,6 +1230,14 @@ impl crate::query_engines::routing::query_engine_routing::QueryEngine for ASAPQu // too (function="sum" but range_seconds=300 from the // inner rate's matrix selector, picked up by the // analyzer trace). + // Dispatch off the typed `outer_fn` field on the + // analyzer candidate — `OuterFn::Rate` if the original + // PromQL contained a `rate(...)` / `irate(...)` call. + // Replaces a previous `query_contains_rate_call(query)` + // re-parse of the raw PromQL string (a lossy-lowering + // smell — the analyzer is the source of truth for + // query intent). See `control_plane/sketch_algebra/ + // capability::OuterFn`. let use_rate_path = matches!( &candidate.required_capability, crate::storage_engines::sketch_db::index::Capability::ExactAgg( @@ -1271,7 +1247,7 @@ impl crate::query_engines::routing::query_engine_routing::QueryEngine for ASAPQu | crate::storage_engines::sketch_db::data::AggregationType::MultipleIncrease ) ) && candidate.range_seconds > 0 - && Self::query_contains_rate_call(query); + && candidate.outer_fn == control_plane::asap_tier_analysis::OuterFn::Rate; let reducer_result = match &candidate.required_capability { crate::storage_engines::sketch_db::index::Capability::ExactAgg( agg_type, @@ -2398,13 +2374,175 @@ mod asap_tier_classify_tests { assert!((z1 - 6.0).abs() < 1e-9, "z1 rate expected 6.0, got {z1}"); } + /// Regression: `sum_over_time(http_requests_total[5m])` shares + /// `Capability::ExactAgg(Sum)` with `rate(...)` — the engine's + /// reducer dispatch MUST disambiguate via the analyzer's typed + /// `outer_fn` field (set to `OuterFn::Plain` for sum_over_time), + /// NOT by re-parsing the raw PromQL string. If the dispatch ever + /// regresses to "all ExactAgg(Sum) + range > 0 → rate path", + /// this test fails because the output would be (per-window sums) + /// / 300 instead of the raw per-window sums. + /// + /// Pins the per-window reducer's output: each series carries the + /// SUM of its in-window samples (not events-per-second). + #[tokio::test] + async fn execute_sum_over_time_dispatches_to_plain_exact_agg_reducer() { + use crate::precompute_engine::operators::sum_accumulator::SumAccumulator; + use crate::storage_engines::sketch_db::data::AggregationType; + use crate::query_engines::query_result::QueryResult; + + let idx = Arc::new(SketchStore::new()); + // Two zones, one ExactAgg(Sum) sid each, two windows each. + // Per-window sum is 600 / 900 — `sum_over_time` over a + // 300s lookback should report the sum of windowed values + // (1200 / 1800), NOT divided by 300. + let now_ms = std::time::SystemTime::now() + .duration_since(std::time::SystemTime::UNIX_EPOCH) + .map(|d| d.as_millis() as u64) + .unwrap_or(0); + let w1_start = now_ms.saturating_sub(120_000); + let w1_end = now_ms.saturating_sub(60_000); + let w2_start = w1_end; + let w2_end = now_ms.saturating_sub(1_000); + + for (i, (zone, per_window)) in + [("z0", 600.0_f64), ("z1", 900.0)].iter().enumerate() + { + let sid = 13_000 + i as u64; + idx.register(SketchInstanceMetadata { + sid, + metric_name: "http_requests_total".to_string(), + group_by_keys: ["zone".to_string()].into_iter().collect(), + capability: Some(Capability::ExactAgg(AggregationType::Sum)), + agg_kind: crate::storage_engines::sketch_db::index::AggKind::ExactAgg { + agg_type: AggregationType::Sum, + parameters_canonical: String::new(), + spatial_filter_canonical: String::new(), + }, + accuracy: None, + first_seen_unix_ms: 0, + retired_at_ms: None, + expires_at_ms: None, + policy_fp: asap_types::PolicyFingerprint::UNSET, + }); + for (ws, we) in [(w1_start, w1_end), (w2_start, w2_end)] { + let mut lm = BTreeMap::new(); + lm.insert("zone".to_string(), zone.to_string()); + idx.append_precompute( + sid, + lm, + (ws, we), + Box::new(SumAccumulator::with_sum(*per_window)), + ); + } + } + + let engine = build_engine_with_index(idx); + let result = engine + .execute("sum_over_time(http_requests_total[5m])") + .await + .expect( + "sum_over_time must dispatch via plain ExactAgg reducer \ + off the typed OuterFn::Plain candidate, not capability-miss", + ); + + let vector = match result { + QueryResult::Vector(v) => v, + other => panic!("expected Vector, got {other:?}"), + }; + // `sum_over_time(metric[r])` (no `sum by (...)` wrapper) lowers + // to an analyzer candidate with empty `group_by_keys`. The + // plain `evaluate_exact_agg` reducer treats empty group_by as + // "collapse across all series" (vs the rate reducer which + // preserves the natural label map per sid). So the expected + // shape is ONE entry whose value is the sum of the latest + // per-window sample across both zones: 600 + 900 = 1500. The + // load-bearing assertion is the VALUE — if the engine + // regressed to the rate path the value would be (1500)/300 = + // 5.0 (or per-zone if rate's per-sid split fired), neither of + // which is 1500. + assert_eq!( + vector.values.len(), + 1, + "plain ExactAgg reducer collapses across series when \ + group_by_keys is empty" + ); + let value = vector.values[0].value; + assert!( + (value - 1500.0).abs() < 1e-9, + "sum_over_time expected 1500 (sum of latest per-window samples \ + across zones), got {value} — if this is ~5.0 or close to \ + 4.0/6.0 the engine regressed to the rate-divisor path; the \ + typed OuterFn::Plain candidate dispatch is broken" + ); + } + + /// Regression: the engine's rate-vs-plain dispatch decision MUST + /// be made off the analyzer's typed `ASAPTierCandidate.outer_fn` + /// field, NOT by re-parsing the raw PromQL query string. This test + /// fabricates an analyzer-shaped candidate by name (no raw PromQL + /// in scope) and asserts the `OuterFn` enum values the engine + /// reads off it. If the engine ever re-introduces a + /// `query_contains_rate_call`-style raw-string re-parse this test + /// continues to pass — but the deletion of the string helper + + /// this typed contract is what guards against the regression in + /// the first place. + #[test] + fn analyzer_candidate_outer_fn_distinguishes_rate_from_sum_over_time() { + use control_plane::asap_tier_analysis::{ + analyze_promql_for_asap_tier, OuterFn, + }; + let rate = analyze_promql_for_asap_tier("rate(http_requests_total[5m])"); + let sot = analyze_promql_for_asap_tier( + "sum_over_time(http_requests_total[5m])", + ); + let sum_by_rate = analyze_promql_for_asap_tier( + "sum by (zone) (rate(http_requests_total[5m]))", + ); + let bare = analyze_promql_for_asap_tier("http_requests_total"); + + assert!(rate.unsupported.is_none() && !rate.candidates.is_empty()); + assert!(sot.unsupported.is_none() && !sot.candidates.is_empty()); + assert!( + sum_by_rate.unsupported.is_none() && !sum_by_rate.candidates.is_empty() + ); + assert!(bare.unsupported.is_none() && !bare.candidates.is_empty()); + + // Same capability for ALL — the field that disambiguates is + // `outer_fn`, not `required_capability`. + assert_eq!( + rate.candidates[0].required_capability, + sot.candidates[0].required_capability, + ); + assert_eq!( + rate.candidates[0].required_capability, + sum_by_rate.candidates[0].required_capability, + ); + assert_eq!( + rate.candidates[0].required_capability, + bare.candidates[0].required_capability, + ); + + // `outer_fn` carries the distinction. + assert_eq!(rate.candidates[0].outer_fn, OuterFn::Rate); + assert_eq!(sot.candidates[0].outer_fn, OuterFn::Plain); + assert_eq!( + sum_by_rate.candidates[0].outer_fn, + OuterFn::Rate, + "composed `sum by (...) (rate(...))` MUST flag OuterFn::Rate \ + even though the outer function name is `sum`" + ); + assert_eq!(bare.candidates[0].outer_fn, OuterFn::Plain); + } + /// `sum by (zone) (rate(http_requests_total[5m]))` end-to-end. /// The analyzer gives `Capability::ExactAgg(Sum)` with - /// `function="sum"` (outer) and `range_seconds=300` (lifted from - /// the inner rate's matrix selector). The engine's - /// `query_contains_rate_call` walker detects the inner rate and - /// dispatches to `evaluate_exact_agg_rate`, which folds the per- - /// zone per-window sums and divides by 300. + /// `function="sum"` (outer), `range_seconds=300` (lifted from the + /// inner rate's matrix selector), AND `outer_fn=OuterFn::Rate` + /// (the analyzer's PromQL trace walker flags the inner rate call). + /// The engine dispatches to `evaluate_exact_agg_rate` off the + /// typed `outer_fn` field, which folds the per-zone per-window + /// sums and divides by 300. #[tokio::test] async fn execute_sum_by_zone_rate_dispatches_to_exact_agg_rate_reducer() { use crate::precompute_engine::operators::sum_accumulator::SumAccumulator;