diff --git a/asap-query-engine/src/drivers/query/servers/http.rs b/asap-query-engine/src/drivers/query/servers/http.rs index 49045f3a..2b33dcc9 100644 --- a/asap-query-engine/src/drivers/query/servers/http.rs +++ b/asap-query-engine/src/drivers/query/servers/http.rs @@ -4691,7 +4691,8 @@ struct PrecomputeJobRequest { /// POST /api/v1/precompute /// /// The controller creates PrecomputeJobs when a query's upper sub-tree -/// (e.g., TopK, HistogramQuantile) requires evaluation on merged sketches. +/// (e.g., TopK or a histogram-quantile-shaped Aggregate{Quantile(φ)}) +/// requires evaluation on merged sketches. /// This endpoint receives that job and runs it against the SimpleMapStore. async fn handle_precompute_job( State(state): State, diff --git a/controller/src/deployment_model.rs b/controller/src/deployment_model.rs index 59c86de7..26816e39 100644 --- a/controller/src/deployment_model.rs +++ b/controller/src/deployment_model.rs @@ -196,8 +196,10 @@ mod tests { let id = DeploymentModelId::asaplifecycle(); assert!(reg.contains(&id)); let m = reg.lookup(&id).expect("asaplifecycle must be registered"); - // The default rule set carries the 12 engine rules. - assert_eq!(m.rules.len(), 12, "asaplifecycle should ship the 12 engine rules"); + // The default rule set carries the 11 engine rules. (R6 + // HistogramQuantileFusion was retired in Step γ5 — see + // `optimizer::engine` module note.) + assert_eq!(m.rules.len(), 11, "asaplifecycle should ship the 11 engine rules"); // Emitter set carries the three demo emitters. assert!(m.emitters.has("opamp_edge_yaml")); assert!(m.emitters.has("streaming_config_json")); diff --git a/controller/src/intent_algebra/agg_intent.rs b/controller/src/intent_algebra/agg_intent.rs index b8bc07ab..47464233 100644 --- a/controller/src/intent_algebra/agg_intent.rs +++ b/controller/src/intent_algebra/agg_intent.rs @@ -104,10 +104,10 @@ pub enum AggIntent { // captures the intent so the routing decision is layered above intent. // // Note: `histogram_quantile(φ, …)` is NOT an L3 intent — it's a PromQL - // /MetricsQL language-level operator (a query-expression node carried - // by `legacy_expr::QueryExpr::HistogramQuantile`). The L1→L3 lowerer - // maps `histogram_quantile(q, bucket_metric)` semantically to - // `AggIntent::Quantile { q, accuracy }`; bucket-aware handling is a + // /MetricsQL language-level operator. Per Step γ5 of the legacy_expr + // migration, the PromQL parser substitutes it directly into a plain + // `Aggregate { Quantile(φ) }` (which lowers to + // `AggIntent::Quantile { q, accuracy }`); bucket-aware handling is a // physical-planner concern, not an L3 intent. // /// `absent(vector_selector)` — 1 iff the selector matched no series in @@ -176,9 +176,10 @@ impl AggIntent { /// /// Note: `histogram_quantile(...)` was previously listed as /// archive-only here but is no longer an `AggIntent` variant — - /// it's a PromQL/MetricsQL language-level operator (carried by - /// `legacy_expr::QueryExpr::HistogramQuantile`). The L1→L3 - /// lowerer maps it semantically to `AggIntent::Quantile { q, .. }`. + /// it's a PromQL/MetricsQL language-level operator. Per Step γ5, + /// the PromQL parser substitutes it into a plain + /// `Aggregate { Quantile(φ) }`, which lowers to + /// `AggIntent::Quantile { q, .. }`. pub fn archive_only(&self) -> bool { matches!( self, diff --git a/controller/src/intent_algebra/aggregate_bridge.rs b/controller/src/intent_algebra/aggregate_bridge.rs index b95fa833..1dfbd3f6 100644 --- a/controller/src/intent_algebra/aggregate_bridge.rs +++ b/controller/src/intent_algebra/aggregate_bridge.rs @@ -16,7 +16,7 @@ //! //! Many `Aggregate.input` subtrees still hold legacy types (other not- //! yet-migrated variants — `SketchAgg`, `WindowedAgg`, `TopK`, -//! `HistogramQuantile`, `PromQLSubquery`). The canonical +//! `PromQLSubquery`). The canonical //! `QueryExpr::Aggregate.child: Box` field expects a CANONICAL //! `QueryExpr` — so until Steps γ2-γ7 migrate those variants, we cannot //! build a fully-canonical `Aggregate` rooted at a legacy parser's emit diff --git a/controller/src/intent_algebra/legacy_expr.rs b/controller/src/intent_algebra/legacy_expr.rs index 3363ff65..17b29d78 100644 --- a/controller/src/intent_algebra/legacy_expr.rs +++ b/controller/src/intent_algebra/legacy_expr.rs @@ -492,12 +492,11 @@ pub enum QueryExpr { // ── PromQL-specific operators ───────────────────────────────────────── - /// `histogram_quantile(φ, )` — converts an HLL / histogram - /// sketch into a quantile estimate. - HistogramQuantile { - phi: f64, - input: Box, - }, + // Note: `histogram_quantile(φ, )` is no longer a `QueryExpr` + // variant. Per Step γ5 of the legacy_expr migration, the PromQL parser + // substitutes the call with a plain `Aggregate { Quantile(φ) }` so + // downstream code (lowerer, optimizer, physical planner) sees a single + // canonical Quantile intent. /// PromQL sub-query syntax: `[range:resolution]`. PromQLSubquery { @@ -784,7 +783,6 @@ impl QueryExpr { | QueryExpr::TopK { input, .. } | QueryExpr::Sort { input, .. } | QueryExpr::Limit { input, .. } - | QueryExpr::HistogramQuantile { input, .. } | QueryExpr::PromQLSubquery { input, .. } => input.walk(f), QueryExpr::Aggregate { input, .. } => input.walk(f), @@ -832,7 +830,6 @@ impl QueryExpr { | QueryExpr::Sort { input, .. } | QueryExpr::Limit { input, .. } | QueryExpr::Aggregate { input, .. } - | QueryExpr::HistogramQuantile { input, .. } | QueryExpr::PromQLSubquery { input, .. } => input.source_name(), QueryExpr::Merge { inputs } => inputs.first()?.source_name(), QueryExpr::Join { left, .. } @@ -1094,20 +1091,6 @@ mod tests { } } - #[test] - fn histogram_quantile_node() { - let expr = QueryExpr::HistogramQuantile { - phi: 0.95, - input: Box::new(QueryExpr::Source(SourceSpec { name: "hist".into() })), - }; - match expr { - QueryExpr::HistogramQuantile { phi, .. } => { - assert!((phi - 0.95).abs() < 1e-9); - } - _ => panic!(), - } - } - #[test] fn promql_subquery_node() { let expr = QueryExpr::PromQLSubquery { diff --git a/controller/src/intent_algebra/legacy_lower.rs b/controller/src/intent_algebra/legacy_lower.rs index 14a2001d..35235ff3 100644 --- a/controller/src/intent_algebra/legacy_lower.rs +++ b/controller/src/intent_algebra/legacy_lower.rs @@ -106,10 +106,6 @@ pub fn lower_to_sketch_algebra_with_schema( offset, input: Box::new(lower_to_sketch_algebra_with_schema(*input, parent_schema)), }, - QueryExpr::HistogramQuantile { phi, input } => QueryExpr::HistogramQuantile { - phi, - input: Box::new(lower_to_sketch_algebra_with_schema(*input, parent_schema)), - }, QueryExpr::PromQLSubquery { range, resolution, input } => QueryExpr::PromQLSubquery { range, resolution, @@ -544,22 +540,6 @@ mod tests { } } - #[test] - fn histogram_quantile_passthrough() { - let expr = QueryExpr::HistogramQuantile { - phi: 0.95, - input: Box::new(make_agg(AggFunc::Quantile(0.95), src("m"))), - }; - let lowered = lower_to_sketch_algebra(expr); - match &lowered { - QueryExpr::HistogramQuantile { phi, input } => { - assert!((phi - 0.95).abs() < 1e-9); - assert!(matches!(input.as_ref(), QueryExpr::SketchAgg { .. })); - } - other => panic!("expected HistogramQuantile, got {other:?}"), - } - } - #[test] fn rate_lowered_to_sum() { let expr = make_agg(AggFunc::Rate, src("m")); diff --git a/controller/src/language_logical_plan/plan.rs b/controller/src/language_logical_plan/plan.rs index 3063ff0b..5e71058d 100644 --- a/controller/src/language_logical_plan/plan.rs +++ b/controller/src/language_logical_plan/plan.rs @@ -19,8 +19,9 @@ //! //! The `intent_algebra` (L3) layer (Phase B, separate worktree) is //! responsible for normalising this language-specific tree into the -//! language-orthogonal `QueryExpr` shape (e.g. dropping -//! `HistogramQuantile`, `PromQLSubquery`). +//! language-orthogonal `QueryExpr` shape (e.g. dropping `PromQLSubquery`). +//! `histogram_quantile(...)` is no longer a legacy variant — Step γ5 +//! substitutes it at the parser level into a plain `Aggregate{Quantile(φ)}`. use std::collections::HashMap; use std::time::Duration; diff --git a/controller/src/optimizer/engine.rs b/controller/src/optimizer/engine.rs index 1e6fb348..1e2f9473 100644 --- a/controller/src/optimizer/engine.rs +++ b/controller/src/optimizer/engine.rs @@ -14,7 +14,7 @@ //! | R3 | `HLLDedupElim` | Eliminate `Dedup` before HLL (HLL is inherently distinct) | //! | R4 | `FilterWindowSwap` | Swap `Filter` below `Window` to reduce window input size | //! | R5 | `TopKFusion` | Absorb `Limit` / `TopK` into a `CountSketch` agg | -//! | R6 | `HistogramQuantileFusion` | Recognise `HistogramQuantile(φ, Agg(DDSketch))` and mark | +//! | R6 | _(retired)_ | `HistogramQuantileFusion` retired in Step γ5 — `histogram_quantile` is now a parser-level substitution | //! | R7 | `SubqueryDecorrelation` | Hoist correlated `ScalarSubquery` to a `LetBinding` | //! | R8 | `CommonSubexprElim` | Extract identical sub-trees into `LetBinding`s | //! | R9 | `HydraConversion` | Convert multi-key `Partition + Agg` into `Hydra` sketch | @@ -229,7 +229,7 @@ impl CostModel for DefaultCostModel { | QueryExpr::Window { .. } => &dc.agent, QueryExpr::Partition { .. } | QueryExpr::Merge { .. } | QueryExpr::Distinct { .. } => &dc.backend_collector, - QueryExpr::TopK { .. } | QueryExpr::HistogramQuantile { .. } + QueryExpr::TopK { .. } | QueryExpr::BinaryOp { .. } | QueryExpr::PromQLSubquery { .. } => &dc.backend_db, QueryExpr::Aggregate { .. } => &dc.original_db, _ => &dc.agent, @@ -492,76 +492,15 @@ impl RewriteRule for TopKFusion { } } -// ── R6: HistogramQuantileFusion ─────────────────────────────────────────────── - -/// Recognise `HistogramQuantile(φ, SketchAgg(DDSketch([φ]), …))` and -/// simplify to a single annotated node that the allocator handles as one -/// DDSketch query. -/// -/// `HistogramQuantile(φ, SketchAgg(DDSketch(qs), col, e))` -/// where `qs` contains `φ` -/// → `HistogramQuantile(φ, SketchAgg(DDSketch(qs), col, e))` [marked fused] -/// -/// In practice we just ensure the quantile is in the DDSketch's quantile -/// list so the allocator emits a single sketch with the right φ. -pub struct HistogramQuantileFusion; - -impl RewriteRule for HistogramQuantileFusion { - fn name(&self) -> &'static str { "HistogramQuantileFusion" } - - fn try_rewrite(&self, expr: QueryExpr, _model: &dyn CostModel, _parent_schema: &Schema) -> Option { - // Canonical Quantile is single-φ post Step α — multi-φ fan-out - // happens at construction time, so the "merge phi into the - // existing quantile list" branch is now a "if phis match, keep - // structure; else build a Merge of two SketchAgg siblings". For - // this PR we keep the structural marker (identity rewrite) and - // defer the Merge-aware fusion to Step γ; the original rule was - // primarily a structural marker anyway. - match expr { - QueryExpr::HistogramQuantile { phi, input } => { - match *input { - QueryExpr::SketchAgg { - op: AggIntent::Quantile { q, accuracy }, - col, - input: inner, - } => { - if (q - phi).abs() < f64::EPSILON { - // Quantile already matches φ — keep shape. - Some(QueryExpr::HistogramQuantile { - phi, - input: Box::new(QueryExpr::SketchAgg { - op: AggIntent::Quantile { q, accuracy }, - col, - input: inner, - }), - }) - } else { - // φ ≠ q: build a Merge of two single-φ - // SketchAgg siblings (F1 fan-out for the - // multi-φ case) and re-wrap. - let new_q = QueryExpr::SketchAgg { - op: AggIntent::Quantile { q: phi, accuracy: accuracy.clone() }, - col: col.clone(), - input: inner.clone(), - }; - let old_q = QueryExpr::SketchAgg { - op: AggIntent::Quantile { q, accuracy }, - col, - input: inner, - }; - Some(QueryExpr::HistogramQuantile { - phi, - input: Box::new(QueryExpr::Merge { inputs: vec![new_q, old_q] }), - }) - } - } - other => Some(QueryExpr::HistogramQuantile { phi, input: Box::new(other) }), - } - } - _ => None, - } - } -} +// ── R6 (retired): HistogramQuantileFusion ───────────────────────────────────── +// +// Per Step γ5 of the legacy_expr migration, `histogram_quantile(φ, …)` is +// substituted at the PromQL parser level into a plain +// `Aggregate{Quantile(φ)}`. The fusion rule that used to merge a +// `HistogramQuantile` wrapper with an inner DDSketch is no longer needed — +// the parser emits a single `Aggregate{Quantile(φ)}` directly, and the +// L1→L3 lowerer turns it into a single `SketchAgg{Quantile(φ, ...)}` with +// no wrapper to fuse. // ── R7: SubqueryDecorrelation ───────────────────────────────────────────────── @@ -1003,10 +942,6 @@ impl QueryOptimizer { let (new_input, c) = recurse!(input); (QueryExpr::Limit { n, offset, input: new_input }, c) } - QueryExpr::HistogramQuantile { phi, input } => { - let (new_input, c) = recurse!(input); - (QueryExpr::HistogramQuantile { phi, input: new_input }, c) - } QueryExpr::PromQLSubquery { range, resolution, input } => { let (new_input, c) = recurse!(input); (QueryExpr::PromQLSubquery { range, resolution, input: new_input }, c) @@ -1051,7 +986,9 @@ fn default_rules() -> Vec> { Box::new(WindowMerge), Box::new(PartitionElim), Box::new(TopKFusion), - Box::new(HistogramQuantileFusion), + // R6 (HistogramQuantileFusion) retired in Step γ5 — see the + // module-level note above; histogram_quantile is now a parser- + // level substitution into plain `Aggregate{Quantile(φ)}`. Box::new(MergeLifting), Box::new(SetOpFusion), Box::new(HydraConversion), @@ -1075,7 +1012,7 @@ pub fn default_rules_as_optimizer_rules() Box::new(WindowMerge), Box::new(PartitionElim), Box::new(TopKFusion), - Box::new(HistogramQuantileFusion), + // R6 (HistogramQuantileFusion) retired in Step γ5. Box::new(MergeLifting), Box::new(SetOpFusion), Box::new(HydraConversion), @@ -1121,10 +1058,6 @@ impl OptimizerRule for TopKFusion { fn name(&self) -> &'static str { ::name(self) } fn category(&self) -> RuleCategory { RuleCategory::Fusion } } -impl OptimizerRule for HistogramQuantileFusion { - fn name(&self) -> &'static str { ::name(self) } - fn category(&self) -> RuleCategory { RuleCategory::Fusion } -} impl OptimizerRule for MergeLifting { fn name(&self) -> &'static str { ::name(self) } fn category(&self) -> RuleCategory { RuleCategory::PushDown } @@ -1256,46 +1189,11 @@ mod tests { ); } - // ── R6: HistogramQuantileFusion ─────────────────────────────────────────── - - #[test] - fn r6_fans_out_to_merge_when_phi_differs() { - use crate::types_v2::AccuracyTarget; - // Step α: canonical Quantile is single-φ; R6 emits a Merge of - // two single-φ SketchAgg siblings when φ doesn't match the - // existing intent's q. Apply R6 directly so this test is - // independent of downstream rules (CommonSubexprElim, etc.). - let expr = QueryExpr::HistogramQuantile { - phi: 0.95, - input: Box::new(QueryExpr::SketchAgg { - op: AggIntent::Quantile { q: 0.5, accuracy: AccuracyTarget::Epsilon(0.01) }, - col: ColumnRef::SampleValue, - input: Box::new(src("latency")), - }), - }; - let rule = HistogramQuantileFusion; - let model = DefaultCostModel { raw_bytes_per_sec: 100_000.0, deployment: None }; - let schema = infer_schema_for_root(&expr); - let result = rule.try_rewrite(expr, &model, &schema).expect("R6 should fire"); - match &result { - QueryExpr::HistogramQuantile { input, .. } => { - match input.as_ref() { - QueryExpr::Merge { inputs } => { - let mut qs: Vec = inputs.iter().filter_map(|i| match i { - QueryExpr::SketchAgg { op: AggIntent::Quantile { q, .. }, .. } => { - Some(*q) - } - _ => None, - }).collect(); - qs.sort_by(|a, b| a.partial_cmp(b).unwrap()); - assert_eq!(qs, vec![0.5, 0.95]); - } - other => panic!("expected Merge of SketchAgg siblings, got {other:?}"), - } - } - other => panic!("unexpected {other:?}"), - } - } + // ── R6 (retired) ────────────────────────────────────────────────────────── + // HistogramQuantileFusion was removed in Step γ5 — `histogram_quantile` + // is now substituted at the parser level into a plain + // `Aggregate{Quantile(φ)}`, so the wrapper-fusion rule has no input + // shape to match. See the module-level note for details. // ── R10: WindowMerge ────────────────────────────────────────────────────── diff --git a/controller/src/optimizer/trait_def.rs b/controller/src/optimizer/trait_def.rs index 5e43e5b7..05b3f895 100644 --- a/controller/src/optimizer/trait_def.rs +++ b/controller/src/optimizer/trait_def.rs @@ -69,7 +69,7 @@ pub trait OptimizerRule: Send + Sync { mod tests { use super::*; use crate::optimizer::engine::{ - CommonSubexprElim, FilterWindowSwap, HLLDedupElim, HistogramQuantileFusion, + CommonSubexprElim, FilterWindowSwap, HLLDedupElim, HydraConversion, MergeLifting, PartitionElim, PredicatePushDown, SetOpFusion, SubqueryDecorrelation, TopKFusion, WindowMerge, }; @@ -89,7 +89,7 @@ mod tests { Box::new(WindowMerge), Box::new(PartitionElim), Box::new(TopKFusion), - Box::new(HistogramQuantileFusion), + // R6 (HistogramQuantileFusion) retired in Step γ5. Box::new(MergeLifting), Box::new(SetOpFusion), Box::new(HydraConversion), diff --git a/controller/src/physical/allocator.rs b/controller/src/physical/allocator.rs index 6818763c..7c5e5f16 100644 --- a/controller/src/physical/allocator.rs +++ b/controller/src/physical/allocator.rs @@ -23,8 +23,10 @@ //! | TopK | Precompute | always | //! | Merge | Backend | always | //! | Aggregate, Project, Sort, Limit | Db | always | -//! | HistogramQuantile | Db | always | //! | PromQLSubquery, BinaryOp | Precompute | has sketch children | +//! +//! `histogram_quantile(φ, …)` is substituted at the parser level into a plain +//! `Aggregate{Quantile(φ)}` (Step γ5) — no dedicated allocator arm. //! | LetBinding | same as body | propagated | use crate::intent_algebra::legacy_expr::QueryExpr; @@ -483,35 +485,9 @@ impl SketchAllocator { } // ── PromQL-specific ─────────────────────────────────────────── - QueryExpr::HistogramQuantile { phi, input } => { - let child = self.alloc_node(*input, budget, parent_schema); - // If the child is a sketch, elevate to Precompute; - // otherwise fall through to Db. - let stage = if child.mode == ExecutionMode::Sketch { - PipelineStage::Precompute - } else { - PipelineStage::Db - }; - let rationale = format!("histogram_quantile(φ={phi}) at {stage}"); - PlanNode { - expr: QueryExpr::HistogramQuantile { - phi, - input: Box::new(child.expr.clone()), - }, - stage, - mode: ExecutionMode::Sketch, - cost: CostEstimate { - bytes_per_sec: self.raw_bytes_per_sec * 0.02, - ..Default::default() - }, - annotation: NodeAnnotation { - sketch_type: Some(SketchType::DDSketch), - rationale, - ..Default::default() - }, - children: vec![child], - } - } + // (histogram_quantile is substituted at the parser level into a + // plain Aggregate{Quantile(φ)}; see Step γ5. The Aggregate arm + // above handles the resulting Quantile intent.) QueryExpr::PromQLSubquery { range, resolution, input } => { let child = self.alloc_node(*input, budget, parent_schema); @@ -1009,21 +985,9 @@ mod tests { assert_eq!(node.stage, PipelineStage::Db); } - // ── HistogramQuantile + DDSketch → Precompute ───────────────────────────── - - #[test] - fn histogram_quantile_over_sketch_at_precompute() { - let expr = QueryExpr::HistogramQuantile { - phi: 0.95, - input: Box::new(QueryExpr::SketchAgg { - op: default_quantile(0.95), - col: ColumnRef::SampleValue, - input: Box::new(src("hist")), - }), - }; - let node = alloc(unlimited(), expr); - assert_eq!(node.stage, PipelineStage::Precompute); - } + // (`histogram_quantile` is substituted at the parser level into a plain + // `Aggregate{Quantile(φ)}` — see Step γ5. The Aggregate → SketchAgg path + // is covered by other tests.) // ── LetBinding inherits body stage ──────────────────────────────────────── diff --git a/controller/src/physical/planner.rs b/controller/src/physical/planner.rs index 576daa0f..a7b0d6fe 100644 --- a/controller/src/physical/planner.rs +++ b/controller/src/physical/planner.rs @@ -369,7 +369,9 @@ fn plan_node( node } - // ── TopK / HistogramQuantile / BinaryOp: QueryEngine stage ── + // ── TopK / BinaryOp: QueryEngine stage ── + // (histogram_quantile is substituted at the parser level into a plain + // Aggregate{Quantile(φ)} — no dedicated arm needed; see step γ5.) QueryExpr::TopK { k, input, .. } => { let child = plan_node(input, config, parent_schema); let mut node = PhysicalNode { @@ -382,21 +384,6 @@ fn plan_node( node } - QueryExpr::HistogramQuantile { phi, input } => { - let child = plan_node(input, config, parent_schema); - let mut node = PhysicalNode { - op: PhysicalOp::SketchEval { - sketch_type: SketchType::DDSketch, - func: EvalFunc::Quantile(vec![*phi]), - }, - placement: Placement::QueryEngine, - cost: PhysicalCost::default(), - children: vec![child], - }; - insert_exchange_if_needed(&mut node); - node - } - QueryExpr::BinaryOp { op, lhs, rhs, .. } => { let left = plan_node(lhs, config, parent_schema); let right = plan_node(rhs, config, parent_schema); diff --git a/controller/src/physical/stage_split.rs b/controller/src/physical/stage_split.rs index ac3e9f07..43923b0f 100644 --- a/controller/src/physical/stage_split.rs +++ b/controller/src/physical/stage_split.rs @@ -7,7 +7,7 @@ //! |---|---| //! | Agent OTel Collector | `Source`, `Filter`, `Window`, `SketchAgg` (sketch ops) | //! | Backend OTel Collector | `Partition`, `Merge`, `Dedup`, `Aggregate { Exact(Sum\|Count\|Min\|Max) }` | -//! | ASAPQuery Precompute Engine | `TopK`, `HistogramQuantile`, `PromQLSubquery`, deferred sketch ops | +//! | ASAPQuery Precompute Engine | `TopK`, `PromQLSubquery`, deferred sketch ops | //! | DB-side query | `Aggregate { Avg }` (non-mergeable) | //! //! # ExactAgg / AggFunc deferral @@ -33,7 +33,9 @@ //! [`expr_to_promql`] converts a [`QueryExpr`] tree to a valid PromQL expression //! consumed by the ASAPQuery Precompute Engine's query engine. Unlike the old //! flat-template approach, this uses a proper recursive descent so it handles -//! `histogram_quantile`, `PromQLSubquery`, and vector `BinaryOp` nodes natively. +//! `PromQLSubquery` and vector `BinaryOp` nodes natively. (`histogram_quantile` +//! is substituted at the parser level into a plain `Aggregate{Quantile(φ)}`; +//! see Step γ5.) use std::time::Duration; @@ -62,7 +64,7 @@ pub fn split_expr_by_stage(expr: &QueryExpr, budgets: &StageResourceBudgets) -> walk(expr, &mut plan, budgets, &schema); // Build the precompute query_expr from the full tree when the precompute - // stage is active (TopK, HistogramQuantile, PromQLSubquery, or deferred ops). + // stage is active (TopK, PromQLSubquery, or deferred ops). if plan.precompute.active && plan.precompute.query_expr.is_empty() { plan.precompute.query_expr = expr_to_promql(expr); } @@ -81,9 +83,10 @@ pub fn split_expr_by_stage(expr: &QueryExpr, budgets: &StageResourceBudgets) -> /// Sketch data is already ingested from the Backend OTel Collector; the PromQL /// describes the aggregation to apply over it. /// -/// Uses recursive descent, so it handles `histogram_quantile`, -/// `PromQLSubquery`, and vector `BinaryOp` nodes that the old flat-template -/// could not represent. +/// Uses recursive descent, so it handles `PromQLSubquery` and vector +/// `BinaryOp` nodes that the old flat-template could not represent. +/// (`histogram_quantile` is substituted at the parser level into a plain +/// `Aggregate{Quantile(φ)}`; see Step γ5.) /// /// Step β: the root-level [`Schema`] is derived from the outermost /// `Source` leaf and threaded through the recursive descent. The @@ -216,11 +219,8 @@ fn walk( walk(input, plan, budgets, parent_schema); } - // HistogramQuantile — precompute engine applies it over ingested histograms. - QueryExpr::HistogramQuantile { input, .. } => { - plan.precompute.active = true; - walk(input, plan, budgets, parent_schema); - } + // (histogram_quantile is substituted at the parser level into a plain + // Aggregate{Quantile(φ)}; see step γ5. No dedicated stage-split arm.) // PromQLSubquery — precompute engine evaluates the sub-query. QueryExpr::PromQLSubquery { input, .. } => { @@ -461,12 +461,9 @@ fn promql_from_qe(expr: &QueryExpr, ctx: &mut PromQLCtx, parent_schema: &Schema) } QueryExpr::Sort { input, .. } => promql_from_qe(input, ctx, parent_schema), - // ── histogram_quantile(φ, rate(selector[w])) ───────────────────────── - QueryExpr::HistogramQuantile { phi, input } => { - let selector = promql_from_qe(input, ctx, parent_schema); - let window = window_str(ctx.window); - format!("histogram_quantile({phi}, rate({selector}{window}))") - } + // (histogram_quantile lowered to plain Aggregate{Quantile(φ)} at the + // parser level; the resulting Aggregate node round-trips to PromQL + // via the `quantile_over_time(φ, …)` shape above.) // ── PromQL subquery expr[range:step] ───────────────────────────────── QueryExpr::PromQLSubquery { range, resolution, input } => { @@ -858,22 +855,6 @@ mod tests { assert_eq!(plan.precompute.topk, Some(10)); } - #[test] - fn histogram_quantile_activates_precompute() { - let expr = QueryExpr::HistogramQuantile { - phi: 0.99, - input: Box::new(QueryExpr::Window { - duration: Duration::from_secs(300), - slide: None, - input: Box::new(source("http_request_duration_seconds_bucket")), - }), - }; - let plan = split_expr_by_stage(&expr, &no_budget()); - assert!(plan.precompute.active); - assert!(!plan.precompute.query_expr.is_empty()); - assert!(plan.precompute.query_expr.contains("histogram_quantile(0.99")); - } - #[test] fn binary_op_activates_precompute() { let lhs = source("metric_a"); @@ -969,22 +950,6 @@ mod tests { assert!(ql.starts_with("topk(10,"), "expected topk prefix: {ql}"); } - #[test] - fn promql_histogram_quantile() { - let expr = QueryExpr::HistogramQuantile { - phi: 0.95, - input: Box::new(QueryExpr::Window { - duration: Duration::from_secs(300), - slide: None, - input: Box::new(source("http_request_duration_seconds_bucket")), - }), - }; - let ql = expr_to_promql(&expr); - assert!(ql.contains("histogram_quantile(0.95"), "got: {ql}"); - assert!(ql.contains("rate("), "got: {ql}"); - assert!(ql.contains("[5m]"), "got: {ql}"); - } - #[test] fn promql_binary_op_renders_operator() { let expr = QueryExpr::BinaryOp { diff --git a/controller/src/query_parser/mod.rs b/controller/src/query_parser/mod.rs index 9d87971d..90620c09 100644 --- a/controller/src/query_parser/mod.rs +++ b/controller/src/query_parser/mod.rs @@ -221,7 +221,6 @@ impl QeCollector { QueryExpr::Project { input, .. } | QueryExpr::Sort { input, .. } | QueryExpr::Limit { input, .. } - | QueryExpr::HistogramQuantile { input, .. } | QueryExpr::PromQLSubquery { input, .. } => self.visit(input, parent_schema), QueryExpr::Join { left, right, .. } | QueryExpr::SetOp { left, right, .. } diff --git a/controller/src/query_parser/promql.rs b/controller/src/query_parser/promql.rs index c6658c0b..7c910c9e 100644 --- a/controller/src/query_parser/promql.rs +++ b/controller/src/query_parser/promql.rs @@ -15,7 +15,7 @@ //! | Expression | AggFunc | //! |---|---| //! | `quantile_over_time(φ, m[w])` | Quantile(φ) | -//! | `histogram_quantile(φ, rate(m[w]))` | (HistogramQuantile node — not an Aggregate) | +//! | `histogram_quantile(φ, rate(m[w]))` | Quantile(φ) (parser-level substitution; see step γ5) | //! | `avg_over_time(m[w])` | Avg | //! | `min_over_time(m[w])` | Min | //! | `max_over_time(m[w])` | Max | @@ -149,7 +149,7 @@ fn modifier_to_partition(modifier: &LabelModifier) -> PartitionKeys { // // | PromQL pattern | QueryExpr node | // |--------------------------|---------------------------------------| -// | `histogram_quantile(φ…)` | HistogramQuantile { phi } | +// | `histogram_quantile(φ…)` | Aggregate { Quantile(φ) } (step γ5) | // | `m[5m:1m]` subquery | PromQLSubquery { 5m, Some(1m) } | // | `a op b` binary | BinaryOp { VectorMatch } | @@ -163,8 +163,9 @@ use promql_parser::parser::{token::TokenType, BinaryExpr, VectorMatchCardinality /// Parse a PromQL expression string directly into an optimised [`QueryExpr`]. /// -/// This preserves -/// `HistogramQuantile`, `PromQLSubquery`, and `BinaryOp` nodes natively. +/// This preserves `PromQLSubquery` and `BinaryOp` nodes natively; +/// `histogram_quantile(φ, …)` is substituted into a plain +/// `Aggregate { Quantile(φ) }` per Step γ5 of the legacy_expr migration. pub fn parse_promql_expr(query: &str) -> anyhow::Result { let expr = parser::parse(query) .map_err(|e| anyhow!("PromQL parse error: {e}"))?; @@ -308,15 +309,29 @@ fn walk_aggregate_qe(agg: &AggregateExpr, ctx: WalkCtx) -> anyhow::Result anyhow::Result { let name = call.func.name; match name { - // histogram_quantile → native HistogramQuantile node (PromQL-specific). + // histogram_quantile(φ, bucket_metric) → plain Aggregate { Quantile(φ) }. + // + // Per Step γ5 of the legacy_expr migration: at the PromQL parser level + // we substitute `histogram_quantile(φ, bucket_metric)` with the same + // shape that `quantile_over_time(φ, m[w])` produces — an `Aggregate` + // carrying a single `AggFunc::Quantile(φ)`. Downstream code (the + // L1→L3 lowerer, the optimizer, the physical planner) then sees a + // plain Quantile and routes via the existing `AggIntent::Quantile` + // path. Bucket-aware physical reduction is a physical-planner + // concern, not an IR variant. The legacy `QueryExpr::HistogramQuantile` + // variant has been retired. + // + // The inner `rate(...)` is the buckets argument; we use a fresh + // `WalkCtx::default()` because an outer `topk` context would otherwise + // rewrite the Quantile(φ) into a Count-frequency aggregate, which + // would be semantically wrong for the histogram-quantile reduction. "histogram_quantile" => { let phi = extract_call_num_arg(call, 0)?; let rate_expr = call.args.args[1].as_ref(); let (source, filters, window) = extract_inner_matrix(rate_expr)?; - let inner = build_qe_aggregate(source, filters, window, + Ok(build_qe_aggregate(source, filters, window, AggFunc::Quantile(phi), - WalkCtx::default()); - Ok(QueryExpr::HistogramQuantile { phi, input: Box::new(inner) }) + WalkCtx::default())) } // All other function calls: map to AggFunc (Layer 2). "quantile_over_time" => { @@ -576,6 +591,49 @@ mod tests { assert_eq!(pq.quantiles, vec![0.95]); } + /// Step γ5 contract: at the PromQL parser level, `histogram_quantile(φ, …)` + /// is substituted into a plain `QueryExpr::Aggregate { aggs: [AggItem { + /// func: AggFunc::Quantile(φ), … }], … }` so downstream code sees a + /// single canonical Quantile intent (no `QueryExpr::HistogramQuantile` + /// wrapper). `ParsedQuery.quantiles` records the φ via the existing + /// multi-quantile machinery. + #[test] + fn histogram_quantile_lowers_to_plain_aggregate_quantile() { + use crate::intent_algebra::legacy_expr::{AggFunc, ColumnRef, QueryExpr}; + + let qe = super::parse_promql_expr( + r#"histogram_quantile(0.99, rate(http_requests_bucket{le="0.5"}[5m]))"#, + ) + .expect("parse should succeed"); + + // The top of the tree must be a plain Aggregate with a single + // Quantile(0.99) AggItem — NOT a HistogramQuantile wrapper. + match &qe { + QueryExpr::Aggregate { aggs, .. } => { + assert_eq!(aggs.len(), 1, "expected single AggItem, got {aggs:?}"); + let item = &aggs[0]; + match item.func { + AggFunc::Quantile(phi) => { + assert!((phi - 0.99).abs() < 1e-9, "expected φ=0.99, got {phi}"); + } + ref other => panic!("expected AggFunc::Quantile(0.99), got {other:?}"), + } + assert!(matches!(item.col, ColumnRef::SampleValue)); + } + other => panic!("expected Aggregate, got {other:?}"), + } + + // The flat ParsedQuery view exposes the φ via the existing + // multi-quantile machinery. + let pq = pq(r#"histogram_quantile(0.99, rate(http_requests_bucket{le="0.5"}[5m]))"#); + assert_eq!(pq.aggregations, vec![AggType::Quantile]); + assert_eq!(pq.quantiles, vec![0.99]); + assert_eq!( + pq.label_filters.get("le").map(String::as_str), + Some("0.5"), + ); + } + // ── avg_over_time ───────────────────────────────────────────────────────── #[test] diff --git a/controller/src/sketch_algebra/rules/bind_archive_only.rs b/controller/src/sketch_algebra/rules/bind_archive_only.rs index fcd622c9..8b6b2c8d 100644 --- a/controller/src/sketch_algebra/rules/bind_archive_only.rs +++ b/controller/src/sketch_algebra/rules/bind_archive_only.rs @@ -125,12 +125,10 @@ mod tests { #[test] fn binds_absent_archive_only() { // `histogram_quantile(...)` is no longer an L3 intent — it's a - // PromQL operator that the controller's PromQL parser lowers via - // `legacy_expr::QueryExpr::HistogramQuantile`. The L3 mapping - // `histogram_quantile(q, bucket_metric) → Quantile{q,...}` is a - // semantic-only documented contract; the canonical archive-only - // anchor for this test is `Absent` (which has no warm-tier sketch - // family). + // PromQL operator that the controller's PromQL parser substitutes + // (Step γ5) into a plain `Aggregate { Quantile(φ) }`. The + // canonical archive-only anchor for this test is `Absent` + // (which has no warm-tier sketch family). let expr = agg_with(AggIntent::Absent); let out = BindArchiveOnly .apply(&expr, &AccuracyTarget::Epsilon(0.01)) diff --git a/controller/src/sketch_algebra/tests.rs b/controller/src/sketch_algebra/tests.rs index 9412722e..605c0b1f 100644 --- a/controller/src/sketch_algebra/tests.rs +++ b/controller/src/sketch_algebra/tests.rs @@ -480,9 +480,9 @@ fn phase_b_pattern_temporal_and_spatial_combined() { /// for the archive tier. /// /// `histogram_quantile(...)` was previously an L3 intent here but is no -/// longer — it's a PromQL/MetricsQL language-level operator (carried by -/// `legacy_expr::QueryExpr::HistogramQuantile`), NOT a semantic intent. -/// The L1→L3 lowerer's documented contract is +/// longer — it's a PromQL/MetricsQL language-level operator that the +/// parser substitutes (Step γ5) into a plain `Aggregate { Quantile(φ) }`, +/// NOT a semantic intent. The L1→L3 lowerer's documented contract is /// `histogram_quantile(q, bucket_metric) → AggIntent::Quantile { q, .. }`; /// bucket-aware reduction is a physical-planner concern. #[test] @@ -717,10 +717,10 @@ fn phase_b_e2e_topk_well_formed() { /// /// Replaces the prior `phase_b_e2e_histogram_quantile_e2e_through_parser` /// — `histogram_quantile(...)` is now a PromQL/MetricsQL language-level -/// operator carried by `legacy_expr::QueryExpr::HistogramQuantile`, NOT -/// an L3 intent. The L1→L3 contract maps it semantically to `Quantile{q}`; -/// the archive-only routing this test exercises uses `Absent` as a -/// stable proxy (every archive-only variant follows the same code path). +/// operator that the parser substitutes (Step γ5) into a plain +/// `Aggregate { Quantile(φ) }`, NOT an L3 intent of its own. The +/// archive-only routing this test exercises uses `Absent` as a stable +/// proxy (every archive-only variant follows the same code path). #[test] fn phase_b_e2e_archive_only_e2e_binding() { let intent = AggIntent::Absent; diff --git a/controller/src/warm_tier_analysis.rs b/controller/src/warm_tier_analysis.rs index 6ea8a610..c2adef6d 100644 --- a/controller/src/warm_tier_analysis.rs +++ b/controller/src/warm_tier_analysis.rs @@ -435,13 +435,13 @@ mod tests { #[test] fn analyze_histogram_quantile_is_rejected() { // `histogram_quantile(...)` is a PromQL/MetricsQL language-level - // operator (a `legacy_expr::QueryExpr::HistogramQuantile` node), - // NOT an L3 intent. The inner argument shape requires a - // `rate(bucket[r])` which the analyzer rejects as an exact-counter - // intent, so the analyzer returns SOME unsupported reason. The - // architectural mapping `histogram_quantile(q, bucket_metric)` → - // `AggIntent::Quantile{q,...}` is documented but the bucket-aware - // physical reduction is not yet wired into the warm-tier path. + // operator, NOT an L3 intent. Per Step γ5, the PromQL parser + // substitutes it into a plain `Aggregate { Quantile(φ) }` so + // downstream sees the canonical Quantile intent. The inner argument + // shape requires a `rate(bucket[r])` which the analyzer rejects as + // an exact-counter intent, so the analyzer returns SOME unsupported + // reason; the bucket-aware physical reduction is not yet wired into + // the warm-tier path. let a = analyze_promql_for_warm_tier( "histogram_quantile(0.99, sum(rate(http_latency_bucket[5m])) by (le))", );