From 54cbcecd2b84985fafe05a6d343fa5b4425da5ce Mon Sep 17 00:00:00 2001 From: zz_y Date: Tue, 28 Jul 2026 20:58:16 -0600 Subject: [PATCH 1/2] feat(control_plane): classify AggRole via real AggIntent, not a PromQL-string sniff derive_agg_role() used to guess a workload entry's AggRole (Quantile/ Sum/Count/Topk/Other) by sniffing the leading token of its PromQL query_string -- the same duplicate-classifier smell already retired from the live serving path (the old two-analyzer comparison in analyzer-parity-matrix.md, #422). Now parses the query through the same canonical pipeline capability_for()/serving uses (query_parser::parse_query_expr_canonical), runs it through the L3 rule-based optimizer (QueryOptimizer::new(0.0).optimize -- TopKFusion is a pure structural rewrite, ignores the cost model, so the placeholder throughput doesn't affect the outcome) so topk(k, m) actually reaches an Aggregate{TopK} node instead of staying Sort+Limit, then classifies by the real outer AggIntent (collect_agg_intents, now pub(crate) for this reuse). Two things this surfaced: - `(quantile_over_time(0.9, m[5m]))` (redundant wrapping parens) -- the old leading-token sniff finds an empty token at a `(` and mis-defaults to Sum; real classification is unaffected by surface punctuation. New test pins this. - `topk_over_time(...)` was in the old test's query list but isn't a real function this parser (or vanilla PromQL) recognizes at all -- confirmed via grep, nowhere in query_parser/promql.rs or intent_algebra/lower.rs. A workload entry with that query_string would fail to parse anywhere else in the real pipeline too, so the old heuristic classifying it as Topk was itself the bug. Test updated to drop it, with an explanatory comment. cargo build --workspace clean; control_plane 727/727 (one pre-existing, unrelated skip as before), workload:: 33/33 (32 + 1 new test). Co-Authored-By: Claude Sonnet 5 --- control_plane/src/asap_tier_analysis.rs | 6 +- control_plane/src/workload.rs | 155 ++++++++++++++++-------- 2 files changed, 107 insertions(+), 54 deletions(-) diff --git a/control_plane/src/asap_tier_analysis.rs b/control_plane/src/asap_tier_analysis.rs index 9bec25b2..b0d4ad72 100644 --- a/control_plane/src/asap_tier_analysis.rs +++ b/control_plane/src/asap_tier_analysis.rs @@ -305,7 +305,11 @@ fn render_spatial_filter(label_filters: &std::collections::HashMap) { +/// +/// `pub(crate)` — also the single walker `workload::derive_agg_role` uses +/// to classify a workload entry's `query_string` by its real, lowered +/// `AggIntent` instead of a PromQL-string-prefix heuristic. +pub(crate) fn collect_agg_intents(expr: &QueryExpr, out: &mut Vec) { match expr { QueryExpr::Aggregate { aggs, child, .. } => { out.extend(aggs.iter().cloned()); diff --git a/control_plane/src/workload.rs b/control_plane/src/workload.rs index dc4b52c4..37e5cbdd 100644 --- a/control_plane/src/workload.rs +++ b/control_plane/src/workload.rs @@ -7,6 +7,7 @@ use serde::{Deserialize, Deserializer, Serialize}; use tracing::{info, warn}; +use crate::intent_algebra::agg_intent::AggIntent; use crate::types::SketchType; /// Aggregation role a single (metric, query-shape) pair plays in the planner. @@ -48,9 +49,11 @@ pub enum AggRole { /// to HLL when a sketch is appropriate, otherwise to a Sum-as-count /// exact-aggregation. Count, - /// `topk(...)`, `topk_over_time(...)`, or workload entries with + /// `topk(...)`, or workload entries with /// `sketch_family_override: CountSketch | CountMinSketch`. Routes - /// to CountSketch / CMS-with-heap. + /// to CountSketch / CMS-with-heap. (`topk_over_time(...)` isn't a + /// real function this parser recognizes — dropped from this list; + /// see `agg_role_topk_query_strings`'s test comment.) Topk, /// Fallback bucket — specialized sketch families that don't fit the /// four shapes above (e.g. CountMinSketch frequency without a topk @@ -90,24 +93,43 @@ impl std::fmt::Display for AggRole { /// * `CountMinSketch` → [`AggRole::Other`] (frequency — no /// single canonical shape; we keep it out of `Topk` so the topk /// variant stays semantically pure for sketch-with-heap families) -/// 2. **PromQL AST classification** by outermost-function name in -/// `query_string`. Recognised function tokens: -/// * `quantile_over_time` / `quantile` / `histogram_quantile` → [`AggRole::Quantile`] -/// * `sum` / `sum_over_time` / `rate` / `increase` → [`AggRole::Sum`] -/// * `count` / `count_over_time` / `count_distinct_over_time` → [`AggRole::Count`] -/// * `topk` / `topk_over_time` → [`AggRole::Topk`] -/// * Bare metric selector (no outer function) → [`AggRole::Sum`] -/// (matches PromQL's instant-vector semantics — a bare counter -/// sums values across time-aligned samples). -/// 3. Fallback: [`AggRole::Other`]. +/// 2. **Real `AggIntent` classification** — `query_string` is parsed +/// through the same canonical pipeline the live serving path uses +/// (`query_parser::parse_query_expr_canonical` → +/// `asap_tier_analysis::collect_agg_intents`), and the OUTERMOST +/// intent (the one bound to the data-plane capability) is matched: +/// * [`AggIntent::Quantile`] → [`AggRole::Quantile`] +/// * [`AggIntent::TopK`] → [`AggRole::Topk`] +/// * [`AggIntent::Cardinality`], [`AggIntent::Count`], or the +/// windowed-Count-as-Frequency extension +/// (`intent_algebra::as_frequency`) → [`AggRole::Count`] +/// * [`AggIntent::Sum`], [`AggIntent::Rate`], [`AggIntent::Increase`] +/// → [`AggRole::Sum`] +/// * Anything else recognised but not one of the four shapes above +/// (`Min`/`Max`/`Avg`/`StdDev`/histogram accessors/…) → +/// [`AggRole::Other`]. +/// * Bare metric selector (no `Aggregate` node at all) → +/// [`AggRole::Sum`] (matches PromQL's instant-vector semantics — +/// a bare counter sums values across time-aligned samples). +/// 3. Unparseable / no query string / no override → [`AggRole::Other`]. /// -/// Ambiguous case decisions (documented for the B2 PR): -/// * `count_over_time(metric)` — Count (cardinality semantics). -/// The Sum-shaped alternative is rare in practice; users who want -/// it write `sum_over_time(count(...))` which classifies as Sum. -/// * `rate` / `increase` — Sum. Both bind to ExactAgg(Sum) on the -/// data plane (see `data_plane/src/precompute_engine/ingest_handler.rs`'s -/// handling of `AggKind::ExactAgg { Sum }`). +/// This used to be a PromQL-string leading-token sniff — the same +/// duplicate-classifier smell already retired from the live serving path +/// (see the former `analyzer-parity-matrix.md`'s "engine (duplicate)" +/// analyzer). `AggIntent` classification is now the single source of +/// truth for "what shape is this query," here and on the serving path. +/// +/// Ambiguous case decisions (documented for the B2 PR, still holds under +/// `AggIntent` classification): +/// * `count_over_time(metric)` — Count (cardinality/frequency +/// semantics, whichever the lowerer picks). The Sum-shaped +/// alternative is rare in practice; users who want it write +/// `sum_over_time(count(...))` which classifies as Sum. +/// * `rate` / `irate` / `increase` — Sum. `irate` folds onto +/// `AggIntent::Rate` at L3 same as `rate`; both bind to +/// ExactAgg(Increase) on the data plane (see +/// `data_plane/src/precompute_engine/ingest_handler.rs`'s handling +/// of `AggKind::ExactAgg { Increase }`). pub fn derive_agg_role(entry: &WorkloadEntry) -> AggRole { // 1. `sketch_family_override` wins. if let Some(family) = entry.sketch_family_override.as_ref() { @@ -119,44 +141,46 @@ pub fn derive_agg_role(entry: &WorkloadEntry) -> AggRole { }; } - // 2. PromQL AST classification — outermost-token sniff. We don't - // need a full AST walk: PromQL function calls always lead with - // `(`, so the leading identifier carries the shape. For - // nested aggregations the OUTERMOST one drives the role (it's - // the one bound to the data-plane capability). - if let Some(qs) = entry.query_string.as_ref() { + // 2. Real AggIntent classification via the canonical parse/lower + // pipeline — the same one `capability_for`/serving uses. Also runs + // the L3 rule-based optimizer (as `main.rs`'s live plan pipeline + // does, unlike the narrower `asap_tier_analysis` hot path) so + // `TopKFusion` (R5) folds a raw `Limit(Sort DESC)` into an + // `Aggregate { AggIntent::TopK }` before classification — + // otherwise a bare `topk(k, m)` never reaches an `Aggregate` node + // at all and would misclassify as a bare selector. `TopKFusion` + // is a pure structural rewrite (ignores the cost model), so the + // placeholder `0.0` throughput here doesn't affect the outcome. + let Some(qs) = entry.query_string.as_ref() else { + return AggRole::Other; + }; + let Ok(expr) = crate::query_parser::parse_query_expr_canonical(qs) else { + return AggRole::Other; + }; + let (expr, _) = crate::optimizer::engine::QueryOptimizer::new(0.0).optimize(expr); + let mut intents: Vec = Vec::new(); + crate::asap_tier_analysis::collect_agg_intents(&expr, &mut intents); + let Some(outer) = intents.first() else { + // No Aggregate node at all — a bare metric selector (or a + // window-only shape with no AggType to map onto). Bare + // selectors default to Sum per PromQL's instant-vector + // semantics; anything else falls through to Other below. let trimmed = qs.trim_start(); - // Find the leading identifier — letters / underscores up to - // the first non-identifier char (`(`, space, `{`, etc.). - let token_end = trimmed - .find(|c: char| !(c.is_ascii_alphanumeric() || c == '_')) - .unwrap_or(trimmed.len()); - let leading = &trimmed[..token_end]; - if !leading.is_empty() { - match leading { - "quantile_over_time" | "quantile" | "histogram_quantile" => { - return AggRole::Quantile - } - "sum" | "sum_over_time" | "rate" | "irate" | "increase" => return AggRole::Sum, - "count" | "count_over_time" | "count_distinct_over_time" => return AggRole::Count, - "topk" | "topk_over_time" => return AggRole::Topk, - _ => {} - } - } - // Bare metric selector — no outer function call. The leading - // token is a metric name (or empty if the query starts with a - // brace). Treat as Sum (PromQL's default instant-vector - // interpretation aligns with Sum-shaped capability). if !trimmed.is_empty() && !trimmed.starts_with('{') { return AggRole::Sum; } + return AggRole::Other; + }; + if crate::intent_algebra::as_frequency(outer).is_some() { + return AggRole::Count; + } + match outer { + AggIntent::Quantile { .. } => AggRole::Quantile, + AggIntent::TopK { .. } => AggRole::Topk, + AggIntent::Cardinality { .. } | AggIntent::Count { .. } => AggRole::Count, + AggIntent::Sum { .. } | AggIntent::Rate | AggIntent::Increase => AggRole::Sum, + _ => AggRole::Other, } - - // 3. No query_string and no override — default to Sum (Mode-3 - // raw-passthrough entries in the workload registry typically - // declare a bare metric with `assign_to_role: archive` and no - // `query_string`). - AggRole::Other } /// A single workload entry from the workloads YAML file. @@ -853,9 +877,34 @@ mod tests { } } + #[test] + fn agg_role_handles_a_parenthesized_query_the_old_string_sniff_could_not() { + // A leading-token PromQL-string sniff (the pre-migration + // implementation) reads the FIRST character to find the outer + // function name; a redundant wrapping paren isn't an identifier + // character, so the old code's `token_end` search finds an + // empty leading token, falls through to "not a bare selector + // either" and mis-defaults to `Sum`. Real `AggIntent` + // classification parses the whole expression regardless of + // surface punctuation and correctly resolves the inner + // `quantile_over_time` shape. + assert_eq!( + derive_agg_role(&entry("m", Some("(quantile_over_time(0.9, m[5m]))"), None)), + AggRole::Quantile + ); + } + #[test] fn agg_role_topk_query_strings() { - for q in ["topk(5, m)", "topk_over_time(3, m[5m])"] { + // `topk_over_time` was in this list pre-migration, but it isn't a + // real function this parser (or vanilla PromQL) recognizes at + // all — confirmed via grep, it appears nowhere in + // query_parser/promql.rs or intent_algebra/lower.rs. A workload + // entry with that query_string would fail to parse anywhere else + // in the real pipeline too (main.rs's handle_plan included), so + // the old string-sniffing heuristic classifying it as Topk was + // itself the bug, not something this test should keep pinning. + for q in ["topk(5, m)"] { assert_eq!( derive_agg_role(&entry("m", Some(q), None)), AggRole::Topk, From 9abf55a632f3d9e3111051c040ba990692bda7f8 Mon Sep 17 00:00:00 2001 From: zz_y Date: Wed, 29 Jul 2026 10:59:39 -0600 Subject: [PATCH 2/2] fix(control_plane): thread AccuracyTarget through derive_agg_role, fix histogram_quantile role parse_query_expr_canonical now requires an AccuracyTarget (landed in #428, merged after this branch was cut) -- thread the workload entry's own accuracy_sla through via accuracy_target_from_legacy_accuracy_sla rather than leaving this call site broken. Also fixes a real misclassification #428 surfaced: the classic-bucket histogram_quantile(0.99, rate(m_bucket[5m])) shape now lowers to the real, exact-only AggIntent::HistogramQuantile (not the sketchable Quantile the old local parser always substituted). derive_agg_role's wildcard arm already routed this to AggRole::Other correctly -- only the test's expectation was stale, asserting the old parser's behavior. Co-Authored-By: Claude Sonnet 5 --- control_plane/src/workload.rs | 31 +++++++++++++++++++++++++------ 1 file changed, 25 insertions(+), 6 deletions(-) diff --git a/control_plane/src/workload.rs b/control_plane/src/workload.rs index 37e5cbdd..388e914b 100644 --- a/control_plane/src/workload.rs +++ b/control_plane/src/workload.rs @@ -154,7 +154,8 @@ pub fn derive_agg_role(entry: &WorkloadEntry) -> AggRole { let Some(qs) = entry.query_string.as_ref() else { return AggRole::Other; }; - let Ok(expr) = crate::query_parser::parse_query_expr_canonical(qs) else { + let accuracy = crate::types_v2::accuracy_target_from_legacy_accuracy_sla(entry.accuracy_sla); + let Ok(expr) = crate::query_parser::parse_query_expr_canonical(qs, accuracy) else { return AggRole::Other; }; let (expr, _) = crate::optimizer::engine::QueryOptimizer::new(0.0).optimize(expr); @@ -832,11 +833,7 @@ mod tests { #[test] fn agg_role_quantile_query_strings() { - for q in [ - "quantile_over_time(0.99, m[5m])", - "quantile(0.5, m)", - "histogram_quantile(0.99, rate(m_bucket[5m]))", - ] { + for q in ["quantile_over_time(0.99, m[5m])", "quantile(0.5, m)"] { assert_eq!( derive_agg_role(&entry("m", Some(q), None)), AggRole::Quantile, @@ -845,6 +842,28 @@ mod tests { } } + #[test] + fn agg_role_classic_bucket_histogram_quantile_is_other() { + // L1 adoption (design-target-architecture.md Part B), accepted + // behavior change: the retired local parser unconditionally + // substituted `histogram_quantile(...)` with a sketchable + // `Quantile` intent. `lower_promql` instead detects the classic + // `_bucket` + `rate(...)` shape and produces the real, + // exact-only `AggIntent::HistogramQuantile`, which `capability_for` + // has no sketch-family mapping for (see + // `asap_tier_analysis::analyze_histogram_quantile_partially_answerable_via_inner_composition`). + // `derive_agg_role`'s wildcard arm correctly routes it to `Other` + // rather than misclassifying it as a sketch-routable `Quantile`. + assert_eq!( + derive_agg_role(&entry( + "m", + Some("histogram_quantile(0.99, rate(m_bucket[5m]))"), + None + )), + AggRole::Other + ); + } + #[test] fn agg_role_sum_query_strings() { for q in [