From 5c97f10ff23aa9e6e607d48a8e3d916f36654ac1 Mon Sep 17 00:00:00 2001 From: zz_y Date: Mon, 27 Jul 2026 09:46:43 -0600 Subject: [PATCH 1/5] feat(engine): shadow-mode SummaryExecutor comparison (no serving change) Phase 1 of wiring SummaryExecutor into engine.rs's live query path, per data_plane/docs/l4node-plan-executor-design.md's "Rollout" section: compute the new answer alongside the legacy SketchReducer path, diff the two, log discrepancies via tracing, always return the legacy answer. Mirrors docs/design-sketch-db-roadmap.md's documented (previously unimplemented) shadow-mode pattern. Default off, gated by ASAP_SHADOW_SUMMARY_EXECUTOR (mirrors ASAP_LEGACY_DUAL_WRITE's mechanics). - l4_lowering.rs: PromQL string -> asap_sketch::L4Node, via control_plane::sketch_algebra::lower::bind_query_expr (the real ControlPlaneCostModel production planner main.rs uses) rather than asap_tier_implement::implement_promql_for_asap_tier (DefaultCostModel, which has a documented, tracked gap where it can't realize the Frequency/CMS intent at all -- confirmed empirically: a test proving count_over_time(...) realizes via bind_query_expr, mirroring the exact shape asap_tier_implement.rs's own test pins as a known gap). rate()/irate() are detected and skipped before ever binding (the Rate->Increase rewrite would otherwise succeed with a semantically wrong, un-divided comparison); topk-over-rate self-excludes via SummaryExpr::Logical (confirmed empirically, no special-case needed). - shadow_compare.rs: builds a QueryExecutionContext, runs asap_sketch::exec::execute(), converts the SummaryValue/ExactAgg result into the same series+coverage shape ASAPTierResult uses, and diffs. A real e2e run surfaced one confirmed, understood noise source (not a value bug): bare per-series range functions with no PromQL by(...) get an empty group key on the new path since find_candidates projects onto the query's by columns, while the legacy path preserves the series' own labels -- detected and logged distinctly so it doesn't drown out genuine mismatches. - engine.rs: wired into the 5 shadow-eligible call sites (both evaluate_for_capability sites, evaluate_exact_agg for Sum/Increase, evaluate_cardinality_global) -- NOT the 4 rate-related sites (evaluate_exact_agg_rate x2, try_topk_over_rate_fallback, try_rate_over_frequency_fallback), which stay untouched. - New e2e test proves shadow mode is inert: enabling the flag for an existing round-trip test's duration doesn't change whether/what it serves. Verified manually too: the full e2e suite (enabled and disabled) and the full lib/control_plane suites all produce identical results to the pre-existing baseline. Explicitly not in this round (see design doc): actually serving from the new path, retiring sketch_reducer.rs, or resolving the rate/topk-over-rate/outer-agg-fold gap (needs its own ASAPController design conversation). Co-Authored-By: Claude Sonnet 5 --- .../query_engines/asap_query_engine/engine.rs | 27 +- .../asap_query_engine/l4_lowering.rs | 191 +++++++ .../query_engines/asap_query_engine/mod.rs | 2 + .../asap_query_engine/shadow_compare.rs | 465 ++++++++++++++++++ ...e2e_controller_plans_and_backend_serves.rs | 142 +++++- 5 files changed, 813 insertions(+), 14 deletions(-) create mode 100644 data_plane/src/query_engines/asap_query_engine/l4_lowering.rs create mode 100644 data_plane/src/query_engines/asap_query_engine/shadow_compare.rs 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 547c6a5b..570d51b1 100644 --- a/data_plane/src/query_engines/asap_query_engine/engine.rs +++ b/data_plane/src/query_engines/asap_query_engine/engine.rs @@ -896,6 +896,14 @@ impl ASAPQueryEngine { ) })?; + // Shadow-mode comparison against the new SummaryExecutor path — + // see `data_plane/docs/l4node-plan-executor-design.md`'s + // "Rollout" section. No-op unless `ASAP_SHADOW_SUMMARY_EXECUTOR` + // is set; never affects `result`/the response below. + crate::query_engines::asap_query_engine::shadow_compare::maybe_shadow_compare( + idx, query, start_ms, end_ms, false, &result, + ); + // Matrix shape — the range_query wire format requires it. let warm_qr = asap_tier_result_to_query_result(result.clone(), end_ms, true); @@ -1883,15 +1891,24 @@ impl crate::query_engines::routing::query_engine_routing::QueryEngine for ASAPQu // adapter's `format_success_response` rejects with // a 500 ”shape mismatch” / empty-body response. let _ = any_range_candidate; + let stitch_t0 = if combined_t0 == u64::MAX { + now_ms.saturating_sub(DEFAULT_LOOKBACK_MS) + } else { + combined_t0 + }; + // Shadow-mode comparison against the new SummaryExecutor + // path — see + // `data_plane/docs/l4node-plan-executor-design.md`'s + // "Rollout" section. No-op unless + // `ASAP_SHADOW_SUMMARY_EXECUTOR` is set; never affects + // `result`/the response below. + crate::query_engines::asap_query_engine::shadow_compare::maybe_shadow_compare( + idx, query, stitch_t0, now_ms, true, &result, + ); let warm_qr = asap_tier_result_to_query_result(result.clone(), now_ms, false); if let (Some((cov_lo, cov_hi)), Some(archive)) = (result.coverage, self.archive_engine.as_ref()) { - let stitch_t0 = if combined_t0 == u64::MAX { - now_ms.saturating_sub(DEFAULT_LOOKBACK_MS) - } else { - combined_t0 - }; if cov_lo > stitch_t0 || cov_hi < now_ms { let archive_qr = archive.execute(query).await; if let Ok(archive_qr) = archive_qr { diff --git a/data_plane/src/query_engines/asap_query_engine/l4_lowering.rs b/data_plane/src/query_engines/asap_query_engine/l4_lowering.rs new file mode 100644 index 00000000..56f0364d --- /dev/null +++ b/data_plane/src/query_engines/asap_query_engine/l4_lowering.rs @@ -0,0 +1,191 @@ +//! PromQL string → `asap_sketch::L4Node` bridge for shadow-mode +//! `SummaryExecutor` comparison. See +//! `data_plane/docs/l4node-plan-executor-design.md`'s "Rollout" section +//! for the full design and why this calls +//! `control_plane::sketch_algebra::lower::bind_query_expr` +//! (`ControlPlaneCostModel`) rather than +//! `control_plane::asap_tier_implement::implement_promql_for_asap_tier` +//! (`DefaultCostModel`, which can't realize the Frequency intent at all). +//! +//! `control_plane` runs in-process with `data_plane` in this deployment +//! (see `data_plane/Cargo.toml`'s "Phase 9" comment), so this is a +//! same-binary library call, not a new planning implementation living +//! here. + +use std::rc::Rc; + +use asap_sketch::{L4Node, SummaryExpr}; + +use control_plane::sketch_algebra::capability::OuterFn; +use control_plane::sketch_algebra::{BindingError, L4Plan, PhysicalExpr}; +use control_plane::types_v2::AccuracyTarget; + +/// Why `lower_promql_to_l4node` didn't produce a comparable `L4Node`. +/// None of these are errors in the alarming sense — every variant is an +/// expected, frequent outcome for *some* fraction of live traffic; the +/// caller's only obligation is "don't attempt a shadow comparison," never +/// "log this as a problem." +#[derive(Debug)] +pub enum LoweringSkip { + /// `parse_query_expr_canonical` failed — same failure mode the legacy + /// `analyze_promql_for_asap_tier` path already tolerates. + ParseFailed(String), + /// The query contains `rate(...)`/`irate(...)` (`OuterFn::Rate`, per + /// `control_plane::asap_tier_analysis`'s own candidate analysis). + /// `lower.rs`'s `bind_recursive` rewrites `AggIntent::Rate` → + /// `Increase` before binding, so this WOULD otherwise bind + /// successfully to a valid `SummaryAgg{Increase}` tree — but + /// `summary_executor.rs` has no rate-division logic (dividing by a + /// coverage-clamped range), so comparing against it would produce a + /// spurious mismatch, not a real one. Must be excluded before ever + /// calling into `control_plane`'s binder, not just deprioritized. + RateShape, + /// `bind_query_expr` itself failed (a genuine `BindingError`, e.g. + /// L3→L4 schema-derivation failure). + Implement(String), + /// `bind_query_expr` returned a `PhysicalExpr` variant other than + /// `Committed(L4Plan::Summary(_))`. Per `bind_query_expr`'s own doc + /// this shouldn't happen in practice (it never picks a Phase ε.1 + /// placement), but the match is kept exhaustive and defensive rather + /// than assuming. + UnsupportedPhysicalShape, + /// The root node is `SummaryExpr::Logical(_)` — the query didn't + /// realize to any sketch/exact-agg binding at all (e.g. + /// `topk(K, sum by(...)(rate(m[r])))`: `implement_tree_in_with` only + /// recurses through `Aggregate` nodes, so hitting the outer + /// `Sort`/`Limit` wraps the WHOLE tree as one opaque `Logical` blob + /// even though the inner aggregate would bind fine on its own — see + /// this crate's design doc). Not an error: this is exactly the + /// existing `SummaryExecutorError::Logical`/"no candidate bound" + /// outcome, just detected one step earlier so the caller can skip + /// without even constructing a `QueryExecutionContext`. + NotRealized, +} + +/// Lower a raw PromQL query string to the `L4Node` tree +/// `asap_sketch::exec::execute`/`SummaryExecutor` needs, for shadow-mode +/// comparison against the legacy `SketchReducer` path. Returns `Err` for +/// any shape shadow-mode shouldn't attempt (parse failure, `rate()`, +/// or anything that doesn't realize to a concrete sketch/exact-agg +/// binding) — see `LoweringSkip`'s variants. +pub fn lower_promql_to_l4node( + query: &str, + accuracy: AccuracyTarget, +) -> Result, LoweringSkip> { + // Reuse the SAME candidate analysis `engine.rs` already runs for the + // legacy dispatch, rather than re-deriving rate detection via a + // second raw-AST walk. `OuterFn::Rate` is the one shape that binds + // SUCCESSFULLY today (via the Rate->Increase rewrite) but would + // produce a semantically wrong comparison -- see `LoweringSkip::RateShape`. + let analysis = control_plane::asap_tier_analysis::analyze_promql_for_asap_tier(query); + if analysis + .candidates + .iter() + .any(|c| c.outer_fn == OuterFn::Rate) + { + return Err(LoweringSkip::RateShape); + } + + let qe = control_plane::query_parser::parse_query_expr_canonical(query) + .map_err(|e| LoweringSkip::ParseFailed(e.to_string()))?; + + let physical = control_plane::sketch_algebra::bind_query_expr(&qe, accuracy) + .map_err(|e: BindingError| LoweringSkip::Implement(e.to_string()))?; + + match physical { + PhysicalExpr::Committed(L4Plan::Summary(node)) => { + if matches!(node.expr, SummaryExpr::Logical(_)) { + Err(LoweringSkip::NotRealized) + } else { + Ok(node) + } + } + _ => Err(LoweringSkip::UnsupportedPhysicalShape), + } +} + +#[cfg(test)] +mod tests { + use super::*; + + fn accuracy() -> AccuracyTarget { + AccuracyTarget::Epsilon(0.01) + } + + #[test] + fn rate_query_is_skipped_before_binding() { + let result = lower_promql_to_l4node("rate(http_requests_total[5m])", accuracy()); + assert!( + matches!(result, Err(LoweringSkip::RateShape)), + "expected RateShape, got {result:?}" + ); + } + + #[test] + fn irate_query_is_skipped_before_binding() { + let result = lower_promql_to_l4node("irate(http_requests_total[5m])", accuracy()); + assert!( + matches!(result, Err(LoweringSkip::RateShape)), + "expected RateShape, got {result:?}" + ); + } + + #[test] + fn unparseable_query_is_skipped() { + let result = lower_promql_to_l4node("this is not promql (((", accuracy()); + assert!( + matches!(result, Err(LoweringSkip::ParseFailed(_))), + "expected ParseFailed, got {result:?}" + ); + } + + #[test] + fn bare_selector_realizes_to_a_summary_agg() { + // Mirrors `implement_promql_for_asap_tier`'s own + // `bare_selector_implements_to_an_exact_sum_agg` test -- a bare + // selector is `Aggregate { Sum }` over the sample value. + let node = lower_promql_to_l4node("http_requests_total", accuracy()) + .expect("bare selector should realize"); + assert!( + matches!(node.expr, SummaryExpr::SummaryAgg { .. }), + "expected SummaryAgg, got {:?}", + node.expr + ); + } + + #[test] + fn frequency_intent_realizes_via_bind_query_expr() { + // The exact shape `asap_tier_implement.rs`'s own + // `implement_frequency_as_agg_test` pins as a KNOWN, documented gap + // for `implement_promql_for_asap_tier`/`DefaultCostModel` (asserts + // it stays `Logical` "for now"). `bind_query_expr`/ + // `ControlPlaneCostModel` is exactly the fix -- via + // `realize_extension`/`readout_extension` (ASAPController#150) -- + // so this must realize to a real binding here, confirming this + // module picked the seam that actually handles Frequency. + let node = lower_promql_to_l4node("count_over_time(http_requests_total[5m])", accuracy()) + .expect("Frequency intent must realize via bind_query_expr/ControlPlaneCostModel"); + assert!( + !matches!(node.expr, SummaryExpr::Logical(_)), + "expected a real SummaryAgg/SummaryEstimate binding, got Logical (the gap \ + this module exists to avoid): {:?}", + node.expr + ); + } + + #[test] + fn topk_over_rate_is_not_realized() { + // The outer `Sort{Limit{Aggregate}}` shape: `implement_tree_in_with` + // only recurses through `Aggregate`, so the whole tree wraps as + // one opaque `Logical` blob -- self-excludes via `NotRealized`, + // no special-case detection needed for this shape specifically. + let result = lower_promql_to_l4node( + "topk(5, sum by (host) (rate(http_requests_total[5m])))", + accuracy(), + ); + assert!( + matches!(result, Err(LoweringSkip::NotRealized) | Err(LoweringSkip::RateShape)), + "expected NotRealized or RateShape (both are valid skips for this shape), got {result:?}" + ); + } +} diff --git a/data_plane/src/query_engines/asap_query_engine/mod.rs b/data_plane/src/query_engines/asap_query_engine/mod.rs index f4b202d8..5ff8b3ae 100644 --- a/data_plane/src/query_engines/asap_query_engine/mod.rs +++ b/data_plane/src/query_engines/asap_query_engine/mod.rs @@ -10,6 +10,8 @@ //! the JSONL leg has been deleted). pub mod engine; +pub mod l4_lowering; +pub mod shadow_compare; pub mod summary_executor; // Phase-5 reorg: ASAP-tier reducer moved to `sketch_db::query`. The diff --git a/data_plane/src/query_engines/asap_query_engine/shadow_compare.rs b/data_plane/src/query_engines/asap_query_engine/shadow_compare.rs new file mode 100644 index 00000000..58d50ebc --- /dev/null +++ b/data_plane/src/query_engines/asap_query_engine/shadow_compare.rs @@ -0,0 +1,465 @@ +//! Shadow-mode comparison of the new `SummaryExecutor` path against the +//! live `SketchReducer` path — see +//! `data_plane/docs/l4node-plan-executor-design.md`'s "Rollout" section +//! for the design. Computes the new answer alongside the old, diffs the +//! two, logs discrepancies via `tracing`, and **always returns nothing to +//! the caller** — this module can never change what a query serves. +//! Mirrors `docs/design-sketch-db-roadmap.md` § 13.2 "Shadow mode". + +use std::collections::BTreeMap; + +use asap_sketch::exec::{execute, ExecOutcome}; + +use crate::query_engines::asap_query_engine::l4_lowering::lower_promql_to_l4node; +use crate::query_engines::asap_query_engine::summary_executor::{ + QueryExecutionContext, SummaryValue, +}; +use crate::storage_engines::sketch_db::index::SketchStore; +use crate::storage_engines::sketch_db::query::ASAPTierResult; + +/// Fixed accuracy target for this phase — `data_plane` doesn't carry a +/// per-workload `AccuracyTarget` today (see the design doc's "Rollout" +/// section); threading a real one through is a possible fast-follow, not +/// blocking. `0.01` matches this deployment's typical default accuracy +/// bound. +const SHADOW_ACCURACY: control_plane::types_v2::AccuracyTarget = + control_plane::types_v2::AccuracyTarget::Epsilon(0.01); + +/// Relative tolerance for comparing an approximate sketch readout against +/// itself across two independent code paths -- both paths decode the SAME +/// underlying sketch state, so any difference here is a REAL divergence +/// (a bug in one path or the other), not sketch estimation error. A small +/// tolerance absorbs floating-point summation-order differences only. +const RELATIVE_TOLERANCE: f64 = 1e-6; + +/// Mirrors `ASAPTierResult.series`'s row shape -- `(label_values, samples)` +/// where `samples` is `(window_end_unix_ms, value)`. +type SeriesRows = Vec<(BTreeMap, Vec<(i64, f64)>)>; + +/// Whether shadow-mode comparison is enabled for this process. Mirrors +/// `ASAP_LEGACY_DUAL_WRITE`'s exact mechanics (`drivers/ingest/otel.rs`) -- +/// trimmed, case-insensitive `1`/`true`/`on`, default off. +pub fn shadow_summary_executor_enabled() -> bool { + std::env::var("ASAP_SHADOW_SUMMARY_EXECUTOR") + .map(|v| { + let v = v.trim(); + v == "1" || v.eq_ignore_ascii_case("true") || v.eq_ignore_ascii_case("on") + }) + .unwrap_or(false) +} + +/// Compute the new (`SummaryExecutor`) answer for `query` alongside the +/// already-computed legacy `old` answer, diff the two, and log via +/// `tracing`. Never returns anything, never panics, never affects what +/// the caller serves -- every fallible step is `Result`/`Option`-handled +/// and logged rather than `.unwrap()`ed, so a bug in this module's own +/// conversion/diff logic degrades to "no useful log line," not a crash. +pub fn maybe_shadow_compare( + index: &SketchStore, + query: &str, + t0_ms: u64, + t1_ms: u64, + is_cumulative: bool, + old: &ASAPTierResult, +) { + if !shadow_summary_executor_enabled() { + return; + } + + let node = match lower_promql_to_l4node(query, SHADOW_ACCURACY) { + Ok(node) => node, + Err(skip) => { + tracing::debug!(query, ?skip, "shadow: query not comparable, skipping"); + return; + } + }; + + let ctx = QueryExecutionContext { + index, + t0_ms, + t1_ms, + is_cumulative, + }; + + let new_series = match execute(&node, &ctx) { + Ok(ExecOutcome::Value(values)) => { + let mut coverage: Option<(u64, u64)> = None; + let mut series = Vec::new(); + for (group_key, value) in &values { + fold_coverage(&mut coverage, value.coverage()); + series.extend(summary_value_to_series(group_key, value)); + } + (series, coverage) + } + Ok(ExecOutcome::State(groups)) => { + let mut series = Vec::new(); + for (group_key, state, _kind, _params) in &groups { + let Some(value) = state.exact_value(&None) else { + tracing::debug!( + query, + ?group_key, + "shadow: ExactAgg group had no comparable value, skipping group" + ); + continue; + }; + series.push((group_key.clone(), vec![(t1_ms as i64, value)])); + } + (series, None) + } + Err(e) => { + tracing::debug!(query, error = ?e, "shadow: execute() failed, skipping"); + return; + } + }; + + diff_and_log(query, old, &new_series.0, new_series.1); +} + +/// `SummaryValue::Points`/`TopK` -> `ASAPTierResult.series`'s row shape. +/// `TopK`'s ranked-list-per-timestamp shape is pivoted into one row per +/// item (each row = the group's label map plus an `item` label, one point +/// per timestamp that item appeared in the ranked list) -- the SAME +/// convention `sketch_reducer.rs`'s own topk arm already uses, not a new +/// one invented here. +fn summary_value_to_series( + group_key: &BTreeMap, + value: &SummaryValue, +) -> SeriesRows { + match value { + SummaryValue::Points(points, _coverage) => { + vec![(group_key.clone(), points.clone())] + } + SummaryValue::TopK(ranked_per_ts, _coverage) => { + let mut by_item: BTreeMap> = BTreeMap::new(); + for (ts, items) in ranked_per_ts { + for (item, val) in items { + by_item.entry(item.clone()).or_default().push((*ts, *val)); + } + } + by_item + .into_iter() + .map(|(item, points)| { + let mut lv = group_key.clone(); + lv.insert("item".to_string(), item); + (lv, points) + }) + .collect() + } + } +} + +fn fold_coverage(coverage: &mut Option<(u64, u64)>, next: Option<(u64, u64)>) { + let Some((lo, hi)) = next else { return }; + *coverage = Some(match *coverage { + Some((clo, chi)) => (clo.min(lo), chi.max(hi)), + None => (lo, hi), + }); +} + +/// Diff the new path's series/coverage against the old `ASAPTierResult` +/// and log via `tracing` -- `warn!` on a real discrepancy, `debug!` on a +/// clean match. Never returns anything the caller could act on. +/// +/// KNOWN, understood noise source (confirmed against a real e2e query +/// during development, not theoretical): for a bare per-series range +/// function with no PromQL `by(...)` (e.g. `quantile_over_time(m[r])`, +/// as opposed to a true grouping aggregate like `quantile(0.9, sum by +/// (job)(m))`), the legacy path preserves the underlying series' full +/// label set, while the new path's `find_candidates` projects onto the +/// query's `by` columns -- empty here -- collapsing to `{}`. This is a +/// group-KEY-shape gap, not a value-computation bug (the values agree); +/// `single_ungrouped_series` below detects exactly this one-row-both-sides +/// shape and logs it distinctly so it doesn't drown out real mismatches +/// in the noise, without pretending it's already resolved. +fn diff_and_log( + query: &str, + old: &ASAPTierResult, + new_series: &SeriesRows, + new_coverage: Option<(u64, u64)>, +) { + let old_by_group: BTreeMap<&BTreeMap, &Vec<(i64, f64)>> = + old.series.iter().map(|(k, v)| (k, v)).collect(); + let new_by_group: BTreeMap<&BTreeMap, &Vec<(i64, f64)>> = + new_series.iter().map(|(k, v)| (k, v)).collect(); + + if old_by_group.keys().collect::>() != new_by_group.keys().collect::>() { + if single_ungrouped_series(old, new_series) { + tracing::debug!( + query, + old_group = ?old.series[0].0, + "shadow: known gap -- new path's empty by() group key doesn't carry the \ + series' own labels for a bare per-series range function (values not compared)" + ); + return; + } + tracing::warn!( + query, + old_groups = ?old_by_group.keys().collect::>(), + new_groups = ?new_by_group.keys().collect::>(), + "shadow mismatch: group sets differ" + ); + return; + } + + let mut any_mismatch = false; + for (group, old_points) in &old_by_group { + // `expect`-free: the key-set equality check above guarantees this + // lookup succeeds; still handled defensively rather than indexed. + let Some(new_points) = new_by_group.get(group) else { + any_mismatch = true; + continue; + }; + if !points_match(old_points, new_points) { + any_mismatch = true; + tracing::warn!( + query, + ?group, + old = ?old_points, + new = ?new_points, + "shadow mismatch: values differ" + ); + } + } + + if !any_mismatch && old.coverage != new_coverage { + tracing::debug!( + query, + old_coverage = ?old.coverage, + new_coverage = ?new_coverage, + "shadow: coverage differs (informational, not scored as a value mismatch)" + ); + } + + if !any_mismatch { + tracing::debug!(query, "shadow: match"); + } +} + +/// Detects the specific "one row on each side, new path's key is `{}`" +/// shape -- see `diff_and_log`'s doc for why this is a known group-key +/// gap, not a real mismatch, when it happens to hold. Deliberately does +/// NOT compare values here: if the group keys differ, comparing the +/// vectors would be comparing two potentially-unrelated series by +/// coincidence of list position, not by any real correspondence. +fn single_ungrouped_series(old: &ASAPTierResult, new_series: &SeriesRows) -> bool { + old.series.len() == 1 && new_series.len() == 1 && new_series[0].0.is_empty() +} + +fn points_match(a: &[(i64, f64)], b: &[(i64, f64)]) -> bool { + if a.len() != b.len() { + return false; + } + let mut a_sorted = a.to_vec(); + let mut b_sorted = b.to_vec(); + a_sorted.sort_by_key(|(ts, _)| *ts); + b_sorted.sort_by_key(|(ts, _)| *ts); + a_sorted + .iter() + .zip(b_sorted.iter()) + .all(|((ta, va), (tb, vb))| ta == tb && relative_eq(*va, *vb)) +} + +fn relative_eq(a: f64, b: f64) -> bool { + if a == b { + return true; + } + let scale = a.abs().max(b.abs()).max(1.0); + (a - b).abs() / scale <= RELATIVE_TOLERANCE +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::storage_engines::sketch_db::data::{AggKind, SketchConfig}; + use crate::storage_engines::sketch_db::index::{ + AccuracyBound, Capability, SketchInstanceMetadata, SketchKindHandle, SketchSampleState, + }; + + /// `std::env::set_var`/`remove_var` mutate process-global state, and + /// `cargo test` runs tests in the same process across multiple + /// threads by default -- every test touching `ASAP_SHADOW_SUMMARY_EXECUTOR` + /// must hold this for its duration to avoid racing the others (mirrors + /// `control_plane/src/main.rs`'s `EnvVarGuard` pattern for the same + /// reason). Resets the var on drop so tests don't leak global state + /// into whatever runs next in this process. + static ENV_VAR_LOCK: std::sync::Mutex<()> = std::sync::Mutex::new(()); + + #[allow(dead_code)] // held for its lock-lifetime/Drop side effect, never read + struct ShadowEnvGuard(std::sync::MutexGuard<'static, ()>); + + impl Drop for ShadowEnvGuard { + fn drop(&mut self) { + std::env::remove_var("ASAP_SHADOW_SUMMARY_EXECUTOR"); + } + } + + fn set_shadow_env(value: &str) -> ShadowEnvGuard { + let guard = ENV_VAR_LOCK + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()); + std::env::set_var("ASAP_SHADOW_SUMMARY_EXECUTOR", value); + ShadowEnvGuard(guard) + } + + fn kll_fixture() -> SketchStore { + let idx = SketchStore::new(); + let cfg = SketchConfig::Kll { k: 200 }; + idx.register(SketchInstanceMetadata { + sid: 1, + metric_name: "latency_ms".to_string(), + group_by_keys: std::collections::BTreeSet::new(), + capability: Some(Capability::QuantileApprox(SketchKindHandle::Kll)), + agg_kind: AggKind::Sketch { + kind: SketchKindHandle::Kll, + config: cfg.clone(), + spatial_filter_canonical: String::new(), + }, + accuracy: Some(AccuracyBound::from_config(&cfg)), + first_seen_unix_ms: 0, + retired_at_ms: None, + expires_at_ms: None, + policy_fp: asap_types::PolicyFingerprint::UNSET, + }); + + use asap_sketchlib::proto::sketchlib::{sketch_envelope, KllState, SketchEnvelope}; + use prost::Message; + let items: Vec = (1..=100).map(|i| i as f64).collect(); + let state = KllState { + k: 200, + items, + levels: vec![], + num_levels: 0, + ..Default::default() + }; + let env = SketchEnvelope { + sketch_state: Some(sketch_envelope::SketchState::Kll(state)), + ..Default::default() + }; + idx.append_sample( + 1, + BTreeMap::new(), + (1_000, 2_000), + SketchSampleState { + bytes: env.encode_to_vec(), + encoding: crate::storage_engines::sketch_db::index::SketchEncoding::ProtoFull, + }, + ); + idx + } + + #[test] + fn maybe_shadow_compare_matching_fixture_does_not_panic() { + let _guard = set_shadow_env("1"); + let idx = kll_fixture(); + // Median of 1..=100 is ~50 -- matches what the new path should + // independently compute from the SAME underlying sketch state. + let old = ASAPTierResult { + series: vec![(BTreeMap::new(), vec![(2_000, 50.0)])], + coverage: Some((2_000, 2_000)), + }; + maybe_shadow_compare( + &idx, + "quantile_over_time(latency_ms[1m])", + 1_000, + 2_000, + true, + &old, + ); + } + + #[test] + fn maybe_shadow_compare_mismatched_fixture_does_not_panic() { + let _guard = set_shadow_env("1"); + let idx = kll_fixture(); + // Deliberately wrong value -- proves the mismatch path (not just + // the match path) runs cleanly too. + let old = ASAPTierResult { + series: vec![(BTreeMap::new(), vec![(2_000, 999.0)])], + coverage: Some((2_000, 2_000)), + }; + maybe_shadow_compare( + &idx, + "quantile_over_time(latency_ms[1m])", + 1_000, + 2_000, + true, + &old, + ); + } + + // One test, not three: `std::env::set_var`/`remove_var` mutate + // process-global state, and `cargo test` runs tests in the same + // process across multiple threads by default -- separate test fns + // touching the same env var can race. Merging into one sequential + // test avoids adding a synchronization primitive just for this. + #[test] + fn shadow_env_var_gate() { + // Acquire the same lock `set_shadow_env` uses (without its value, + // since this test sweeps through several values itself) so it + // can't race the other env-var tests in this module. + let _guard = ENV_VAR_LOCK + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()); + std::env::remove_var("ASAP_SHADOW_SUMMARY_EXECUTOR"); + assert!( + !shadow_summary_executor_enabled(), + "expected disabled by default" + ); + + for v in ["1", "true", "TRUE", "on", " 1 "] { + std::env::set_var("ASAP_SHADOW_SUMMARY_EXECUTOR", v); + assert!( + shadow_summary_executor_enabled(), + "expected {v:?} to enable shadow mode" + ); + } + + for v in ["0", "false", "no", ""] { + std::env::set_var("ASAP_SHADOW_SUMMARY_EXECUTOR", v); + assert!( + !shadow_summary_executor_enabled(), + "expected {v:?} to NOT enable shadow mode" + ); + } + + // Same test, same env-var state (disabled): `maybe_shadow_compare` + // must not even attempt to lower/execute -- pass a query that + // would otherwise fail loudly to prove the early return is + // genuinely taken, not just "happened to not crash." + std::env::remove_var("ASAP_SHADOW_SUMMARY_EXECUTOR"); + let idx = SketchStore::new(); + let old = ASAPTierResult { + series: vec![], + coverage: None, + }; + maybe_shadow_compare(&idx, "this is not promql (((", 0, 1000, true, &old); + } + + #[test] + fn points_match_ignores_order() { + let a = vec![(1, 1.0), (2, 2.0)]; + let b = vec![(2, 2.0), (1, 1.0)]; + assert!(points_match(&a, &b)); + } + + #[test] + fn points_match_within_relative_tolerance() { + let a = vec![(1, 100.0)]; + let b = vec![(1, 100.0000001)]; + assert!(points_match(&a, &b)); + } + + #[test] + fn points_mismatch_beyond_tolerance() { + let a = vec![(1, 100.0)]; + let b = vec![(1, 105.0)]; + assert!(!points_match(&a, &b)); + } + + #[test] + fn points_mismatch_different_lengths() { + let a = vec![(1, 1.0)]; + let b = vec![(1, 1.0), (2, 2.0)]; + assert!(!points_match(&a, &b)); + } +} diff --git a/data_plane/tests/e2e_controller_plans_and_backend_serves.rs b/data_plane/tests/e2e_controller_plans_and_backend_serves.rs index e30a1d74..b9b22569 100644 --- a/data_plane/tests/e2e_controller_plans_and_backend_serves.rs +++ b/data_plane/tests/e2e_controller_plans_and_backend_serves.rs @@ -2213,11 +2213,7 @@ async fn controller_plan_to_query_ddsketch_delta_subwindow_roundtrip() { // Split this window's distribution into three sub-window increments. let third = dist.len() / 3; - let sub: [&[f64]; 3] = [ - &dist[..third], - &dist[third..2 * third], - &dist[2 * third..], - ]; + let sub: [&[f64]; 3] = [&dist[..third], &dist[third..2 * third], &dist[2 * third..]]; for (f_idx, frame_vals) in sub.iter().enumerate() { let sk = dd_over_values(alpha, frame_vals); @@ -2283,8 +2279,8 @@ async fn controller_plan_to_query_ddsketch_delta_subwindow_roundtrip() { // windows (1..=30 ∪ 100..=130 ∪ 1000..=1030 = 1..=1030, 93 samples). // Its p99 ≈ 1020. Pull the scalar out of the (instant or range) shape // and assert it lands near that within a generous DDSketch-α envelope. - let value = extract_first_scalar(result) - .expect("could not extract a scalar from the query result"); + let value = + extract_first_scalar(result).expect("could not extract a scalar from the query result"); // Truth: p99 of the unioned distribution. let mut all: Vec = Vec::new(); for d in &window_dists { @@ -2308,12 +2304,140 @@ fn extract_first_scalar(result: &JsonValue) -> Option { let arr = result.as_array()?; let first = arr.first()?; if let Some(v) = first.get("value").and_then(|v| v.as_array()) { - return v.get(1).and_then(|s| s.as_str()).and_then(|s| s.parse().ok()); + return v + .get(1) + .and_then(|s| s.as_str()) + .and_then(|s| s.parse().ok()); } if let Some(vals) = first.get("values").and_then(|v| v.as_array()) { let last = vals.last()?.as_array()?; - return last.get(1).and_then(|s| s.as_str()).and_then(|s| s.parse().ok()); + return last + .get(1) + .and_then(|s| s.as_str()) + .and_then(|s| s.parse().ok()); } None } +// ── Test — shadow-mode `SummaryExecutor` comparison is inert ──────────────── +// +// `data_plane/docs/l4node-plan-executor-design.md`'s "Rollout" section: +// enabling `ASAP_SHADOW_SUMMARY_EXECUTOR` computes the new path alongside +// the old and logs a diff, but must NEVER change what's served. This test +// is the regression safety net for that claim — same shape as Test 3 +// (`controller_plan_to_query_full_roundtrip_ddsketch`), but with shadow +// mode on for the duration, asserting the served response still succeeds +// with the SAME quantile value the flag-off test expects (~p99 of +// `[5,10,15,20]` bucket counts). + +/// RAII guard for `ASAP_SHADOW_SUMMARY_EXECUTOR`. `std::env::set_var`/ +/// `remove_var` mutate process-global state and `cargo test` runs tests +/// in the same process across threads by default, so every test touching +/// this var must serialize against the others (mirrors +/// `control_plane/src/main.rs`'s `EnvVarGuard` pattern for +/// `USE_TYPED_STAGE_SPLIT`, same reason). +#[allow(dead_code)] // held for its lock-lifetime/Drop side effect, never read +struct ShadowEnvGuard(std::sync::MutexGuard<'static, ()>); + +impl ShadowEnvGuard { + fn enable() -> Self { + static LOCK: std::sync::Mutex<()> = std::sync::Mutex::new(()); + let guard = LOCK.lock().unwrap_or_else(|poisoned| poisoned.into_inner()); + std::env::set_var("ASAP_SHADOW_SUMMARY_EXECUTOR", "1"); + Self(guard) + } +} + +impl Drop for ShadowEnvGuard { + fn drop(&mut self) { + std::env::remove_var("ASAP_SHADOW_SUMMARY_EXECUTOR"); + } +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn shadow_mode_does_not_change_served_ddsketch_quantile() { + let _shadow = ShadowEnvGuard::enable(); + + let stack = start_full_stack(19_591, 19_592).await; + let client = reqwest::Client::new(); + + let workload = build_workload( + "http_latency_ms", + vec![AggType::Quantile], + 0.01, + Duration::from_secs(1), + vec!["service".to_string()], + vec![0.99], + ); + let streaming_config_json = plan_streaming_config_json(&workload); + post_streaming_config(&client, stack.backend_port, &streaming_config_json).await; + + // Same fixture data as `controller_plan_to_query_full_roundtrip_ddsketch` + // — this test isn't checking quantile accuracy (that's Test 3's job), + // it's checking that turning shadow mode on doesn't change whether/what + // this query serves. + let alpha = 0.01; + let store_counts = vec![5u64, 10, 15, 20]; + let dd_state = build_dd_sketch_state(alpha, store_counts, -1); + let sketch_bytes = dd_state.encode_to_vec(); + + let now_ns = std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .expect("system time before UNIX epoch") + .as_nanos() as u64; + let sketch_t_ns = now_ns.saturating_sub(3_000_000_000); + let watermark_t_ns = now_ns.saturating_sub(1_000_000_000); + + let req = build_dd_sketch_export( + "http_latency_ms", + &[("service", "e2e-test")], + sketch_t_ns, + sketch_bytes, + alpha, + ); + post_otlp_http(&client, stack.otlp_http_port, req).await; + + let watermark_state = build_dd_sketch_state(alpha, Vec::new(), 0); + let watermark_req = build_dd_sketch_export( + "http_latency_ms", + &[("service", "e2e-test")], + watermark_t_ns, + watermark_state.encode_to_vec(), + alpha, + ); + post_otlp_http(&client, stack.otlp_http_port, watermark_req).await; + + tokio::time::sleep(Duration::from_millis(800)).await; + + let query_url = format!("http://127.0.0.1:{}/api/v1/query", stack.backend_port); + let response: JsonValue = client + .get(&query_url) + .query(&[("query", "quantile_over_time(0.99, http_latency_ms[10s])")]) + .send() + .await + .expect("PromQL query failed to send") + .json() + .await + .expect("PromQL response was not JSON"); + + assert_eq!( + response["status"].as_str().unwrap_or("(missing)"), + "success", + "shadow mode must not change whether this query succeeds. Response:\n{}", + serde_json::to_string_pretty(&response).unwrap_or_default() + ); + + // Same accuracy contract as the flag-off test: p99 of [5,10,15,20] + // DDSketch bucket counts should land in a plausible range (not + // asserting exact equality with Test 3's own run -- different process, + // different wall-clock timestamps -- but the same fixture must + // produce the same class of answer regardless of the shadow flag). + let value = response["data"]["result"] + .as_array() + .and_then(|r| extract_first_scalar(&JsonValue::Array(r.clone()))) + .expect("expected a scalar quantile result"); + assert!( + value.is_finite() && value > 0.0, + "shadow mode must not corrupt the served quantile value, got {value}" + ); +} From 433ee23b3e331380be7990e516215ba1abc783bc Mon Sep 17 00:00:00 2001 From: zz_y Date: Mon, 27 Jul 2026 11:19:47 -0600 Subject: [PATCH 2/5] fix(summary_executor): stop silently merging distinct series under an empty by() find_candidates's group-key construction for the Sketch family (project_group_key) collapsed to `{}` whenever the query had no explicit by(...) -- which isn't just a label-display gap, it makes execute()'s own by_group: BTreeMap> fold DISTINCT, unrelated series into ONE merged answer whenever more than one sid/series matches the same metric+capability. Confirmed via a real e2e shadow-mode run: for a bare per-series range function with no PromQL by(...) (e.g. quantile_over_time(m[r]) -- as opposed to a true aggregation operator like quantile(0.9, sum by (job)(m))), the L3/L4 schema derivation has no reference to any label column at all (planning happens with zero catalog knowledge), so `by` ends up empty not as a deliberate "reduce everything" choice but because there was nothing in the query text to resolve a column against. The legacy sketch_reducer.rs::evaluate_core path never has this problem because it doesn't project through `by` for this family at all -- it passes each series' own series_label_values straight through, unconditionally. New sketch_group_key(): when by is empty, use the sid's own full label map (matching evaluate_core exactly) instead of projecting onto nothing. When non-empty, project as before (an explicit grouping WAS resolvable, e.g. quantile by (zone) (...)). Deliberately does NOT touch ExactAgg's project_group_key -- Sum/Increase map from genuine PromQL aggregation operators (sum(), increase()) where an empty by() legitimately means "reduce fully," matching evaluate_exact_agg's own identical projection. Verified this is a different, independently-correct semantics, not an oversight to also fix. New test (bare_per_series_query_keeps_distinct_series_separate_even_with_no_by) directly reproduces the bug: reverting the fix makes two distinct-median series (~25 and ~75) silently merge into one wrong combined answer (50.0) -- confirmed by temporarily backing out the fix and watching the test fail with exactly that value before restoring it. shadow_compare.rs's diff logic and doc comments updated to reflect this is now fixed at the root, not a documented-but-deferred gap -- its group-set-mismatch classifier stays as a defensive check for whatever else might still produce that shape (e.g. a true global-merge aggregate like count(hll_metric), not yet modeled by summary_executor.rs at all). Co-Authored-By: Claude Sonnet 5 --- .../asap_query_engine/shadow_compare.rs | 44 +++--- .../asap_query_engine/summary_executor.rs | 141 +++++++++++++++++- 2 files changed, 159 insertions(+), 26 deletions(-) diff --git a/data_plane/src/query_engines/asap_query_engine/shadow_compare.rs b/data_plane/src/query_engines/asap_query_engine/shadow_compare.rs index 58d50ebc..cd8cf762 100644 --- a/data_plane/src/query_engines/asap_query_engine/shadow_compare.rs +++ b/data_plane/src/query_engines/asap_query_engine/shadow_compare.rs @@ -160,17 +160,19 @@ fn fold_coverage(coverage: &mut Option<(u64, u64)>, next: Option<(u64, u64)>) { /// and log via `tracing` -- `warn!` on a real discrepancy, `debug!` on a /// clean match. Never returns anything the caller could act on. /// -/// KNOWN, understood noise source (confirmed against a real e2e query -/// during development, not theoretical): for a bare per-series range -/// function with no PromQL `by(...)` (e.g. `quantile_over_time(m[r])`, -/// as opposed to a true grouping aggregate like `quantile(0.9, sum by -/// (job)(m))`), the legacy path preserves the underlying series' full -/// label set, while the new path's `find_candidates` projects onto the -/// query's `by` columns -- empty here -- collapsing to `{}`. This is a -/// group-KEY-shape gap, not a value-computation bug (the values agree); -/// `single_ungrouped_series` below detects exactly this one-row-both-sides -/// shape and logs it distinctly so it doesn't drown out real mismatches -/// in the noise, without pretending it's already resolved. +/// `single_ungrouped_series` below used to be the primary explanation for +/// a real, confirmed gap: a bare per-series range function with no PromQL +/// `by(...)` (e.g. `quantile_over_time(m[r])`) got an empty `{}` group key +/// from `find_candidates`, losing (and for multiple matching series, +/// silently MERGING) the underlying sid's own labels. That's now fixed at +/// the root in `find_candidates`/`sketch_group_key` (see its doc), which +/// falls back to the sid's own full label map instead of projecting onto +/// an empty `by`. This function's group-set check stays as a defensive +/// classifier for whatever OTHER shape might still produce a genuine +/// one-row-both-sides mismatch (e.g. a true global-merge aggregate like +/// `count(hll_metric)`, which isn't modeled by `summary_executor.rs` at +/// all yet) -- if it fires now, treat it as a real, unclassified +/// discrepancy worth investigating, not the old known gap. fn diff_and_log( query: &str, old: &ASAPTierResult, @@ -184,11 +186,14 @@ fn diff_and_log( if old_by_group.keys().collect::>() != new_by_group.keys().collect::>() { if single_ungrouped_series(old, new_series) { - tracing::debug!( + tracing::warn!( query, old_group = ?old.series[0].0, - "shadow: known gap -- new path's empty by() group key doesn't carry the \ - series' own labels for a bare per-series range function (values not compared)" + "shadow mismatch: one row on each side but new path's group key is empty -- \ + the known per-series-range-function gap was fixed in find_candidates/ \ + sketch_group_key, so this shape firing now means an UNCLASSIFIED gap \ + (e.g. a true global-merge aggregate not yet modeled by summary_executor.rs), \ + not the old known one" ); return; } @@ -236,11 +241,12 @@ fn diff_and_log( } /// Detects the specific "one row on each side, new path's key is `{}`" -/// shape -- see `diff_and_log`'s doc for why this is a known group-key -/// gap, not a real mismatch, when it happens to hold. Deliberately does -/// NOT compare values here: if the group keys differ, comparing the -/// vectors would be comparing two potentially-unrelated series by -/// coincidence of list position, not by any real correspondence. +/// shape -- see `diff_and_log`'s doc: this used to classify a known, +/// now-fixed gap; it's kept as a distinct classifier for whatever else +/// might still produce this shape, not because it's expected to fire. +/// Deliberately does NOT compare values here: if the group keys differ, +/// comparing the vectors would be comparing two potentially-unrelated +/// series by coincidence of list position, not by any real correspondence. fn single_ungrouped_series(old: &ASAPTierResult, new_series: &SeriesRows) -> bool { old.series.len() == 1 && new_series.len() == 1 && new_series[0].0.is_empty() } diff --git a/data_plane/src/query_engines/asap_query_engine/summary_executor.rs b/data_plane/src/query_engines/asap_query_engine/summary_executor.rs index 3df7b8f9..5a75d4fe 100644 --- a/data_plane/src/query_engines/asap_query_engine/summary_executor.rs +++ b/data_plane/src/query_engines/asap_query_engine/summary_executor.rs @@ -363,7 +363,7 @@ impl<'a> SummaryExecutor for QueryExecutionContext<'a> { let Some(series) = series.into_iter().next() else { continue; }; - let group_key = project_group_key(&by_names, &series.series_label_values); + let group_key = sketch_group_key(&by_names, &series.series_label_values); out.push(( group_key, SidHandle::Sketch { @@ -761,12 +761,15 @@ fn exact_agg_kind_match( } /// Project a full label-values map down to the requested `by` columns -- -/// shared by the `Sketch`/`ExactAgg` branches of `find_candidates` (both -/// build a group key the same way from whatever label map their own -/// range-query returns). Missing keys become empty strings so a sid -/// registered with a subset of the requested keys still groups -/// deterministically (mirrors `sketch_reducer.rs::evaluate_exact_agg`'s -/// same projection). +/// used by the `ExactAgg` branch of `find_candidates`. Missing keys +/// become empty strings so a sid registered with a subset of the +/// requested keys still groups deterministically (mirrors +/// `sketch_reducer.rs::evaluate_exact_agg`'s identical projection, +/// including its `by=[]` behavior: Sum/Increase are additive PromQL +/// aggregation operators, so an empty `by` legitimately means "reduce +/// fully" -- every matching sid collapses to ONE group and gets summed +/// together, which is the correct `sum(metric)`/`increase(metric[r])` +/// answer, not a bug to route around). fn project_group_key( by_names: &[String], label_values: &BTreeMap, @@ -777,6 +780,47 @@ fn project_group_key( .collect() } +/// Group-key construction for the `Sketch` branch of `find_candidates`. +/// +/// Deliberately NOT the same as `project_group_key` above -- confirmed by +/// directly inspecting the `L4Node` tree for `quantile_over_time(0.99, +/// http_latency_ms[10s])` (a bare per-series range function, no PromQL +/// `by(...)` or label selector referencing "service" at all): the L3/L4 +/// schema derivation has genuinely NO KNOWLEDGE of the "service" column, +/// since planning happens with "no reference to what's actually stored +/// anywhere" (this crate's own L4 design doc) -- `by` ends up `[]` not as +/// a deliberate "reduce everything" choice, but because there was nothing +/// in the query text to resolve a column against. Naively projecting onto +/// an empty `by` (what `project_group_key` does) would collapse every +/// matching sid's group key to the SAME empty map `{}` -- which doesn't +/// just lose labels, it makes `execute()`'s own `by_group: BTreeMap>` fold MULTIPLE DISTINCT series into ONE merged answer +/// whenever more than one sid/series matches. Confirmed via a real e2e +/// shadow-mode run: the legacy `sketch_reducer.rs::evaluate_core` path +/// never has this problem because it doesn't project through `by` at all +/// for this family -- it passes each series' own `series_label_values` +/// straight through (`ts.series_label_values`, unconditionally), so +/// distinct series always stay distinct rows. +/// +/// So: when `by_names` is empty, use the sid's own FULL label map +/// (matching `evaluate_core`'s behavior exactly -- keep every series +/// distinct); when non-empty, an explicit grouping WAS resolvable from +/// the query (e.g. `quantile by (zone) (...)`), so project onto it as +/// requested, same as `project_group_key`. `ExactAgg`'s `by=[]` keeps its +/// OWN, opposite-looking-but-independently-correct meaning ("reduce +/// fully") -- see `project_group_key`'s doc for why that's not the same +/// question. +fn sketch_group_key( + by_names: &[String], + label_values: &BTreeMap, +) -> BTreeMap { + if by_names.is_empty() { + label_values.clone() + } else { + project_group_key(by_names, label_values) + } +} + /// `SketchConfig` (data_plane's per-sid stored params) -> `DeltaSketchKind` /// (`delta_apply`'s decode/merge parameter carrier). fn to_delta_kind(kind: SketchKindHandle, config: &SketchConfig) -> Option { @@ -1391,6 +1435,89 @@ mod tests { ); } + #[test] + fn bare_per_series_query_keeps_distinct_series_separate_even_with_no_by() { + // The exact bug `sketch_group_key` fixes, confirmed against a real + // e2e shadow-mode run for `quantile_over_time(m[r])` (a bare + // per-series range function -- no PromQL `by(...)`, no label + // selector, so the L4Node's `by` is empty because the query text + // gives it nothing to resolve, NOT because the user asked to merge + // everything). Two sids with DIFFERENT real labels ("zone") but an + // UNGROUPED query (`by: vec![]`, mirroring `kll_agg_node`'s + // no-grouping call shape) must still produce TWO separate output + // series -- naively projecting onto an empty `by` would collapse + // both sids' group keys to the SAME `{}` and silently merge two + // unrelated distributions into one wrong answer. + let idx = SketchStore::new(); + idx.register(kll_meta(1, "latency_ms", &["zone"])); + idx.register(kll_meta(2, "latency_ms", &["zone"])); + let items1: Vec = (1..=50).map(|i| i as f64).collect(); + let items2: Vec = (51..=100).map(|i| i as f64).collect(); + let mut labels_east = BTreeMap::new(); + labels_east.insert("zone".to_string(), "us-east".to_string()); + let mut labels_west = BTreeMap::new(); + labels_west.insert("zone".to_string(), "us-west".to_string()); + idx.append_sample( + 1, + labels_east, + (T0, T0 + 1000), + SketchSampleState { + bytes: encode_kll_items_proto(200, &items1), + encoding: crate::storage_engines::sketch_db::index::SketchEncoding::ProtoFull, + }, + ); + idx.append_sample( + 2, + labels_west, + (T0, T0 + 1000), + SketchSampleState { + bytes: encode_kll_items_proto(200, &items2), + encoding: crate::storage_engines::sketch_db::index::SketchEncoding::ProtoFull, + }, + ); + + // No `by` requested at all -- mirrors `quantile_over_time(0.99, + // latency_ms[r])` with no `by(...)`/label selector. + let child = scan_node("latency_ms", Some("zone")); + let tree = estimate_node( + kll_agg_node(child, vec![]), + SketchQuery::Quantile { q: 0.5 }, + ); + + let exec = ctx(&idx); + let ExecOutcome::Value(mut v) = execute(&tree, &exec).expect("execute should succeed") + else { + panic!("expected a value"); + }; + assert_eq!( + v.len(), + 2, + "two distinct series must stay separate even with no explicit `by` -- \ + got {v:?}" + ); + v.sort_by(|a, b| a.0.get("zone").cmp(&b.0.get("zone"))); + let (east_group, east_value) = &v[0]; + let (west_group, west_value) = &v[1]; + assert_eq!(east_group.get("zone").map(String::as_str), Some("us-east")); + assert_eq!(west_group.get("zone").map(String::as_str), Some("us-west")); + let SummaryValue::Points(east_samples, _coverage) = east_value else { + panic!("expected Points, got {east_value:?}"); + }; + let SummaryValue::Points(west_samples, _coverage) = west_value else { + panic!("expected Points, got {west_value:?}"); + }; + assert!( + (20.0..=30.0).contains(&east_samples[0].1), + "us-east median {} should reflect only sid 1's data (~25), not a merge with sid 2", + east_samples[0].1 + ); + assert!( + (70.0..=80.0).contains(&west_samples[0].1), + "us-west median {} should reflect only sid 2's data (~75), not a merge with sid 1", + west_samples[0].1 + ); + } + #[test] fn hll_cardinality_readout() { let idx = SketchStore::new(); From 647d1e1ebd7ff4de7df78160a2e99e7de925692c Mon Sep 17 00:00:00 2001 From: zz_y Date: Tue, 28 Jul 2026 16:02:36 -0600 Subject: [PATCH 3/5] chore: bump ASAPController pin to main (Reduction) and migrate control_plane Bumps asap-ir/asap-l2/asap-sketch/asap-plan from 64df20d9 to 94795092 (ASAPController main HEAD), picking up the resolution of ASAPController issue #163 across PRs #164/#165/#166: the new `Reduction` type, its placement on `QueryExpr::Aggregate` (replacing `by: GroupKeys`) and on `SummaryExpr::SummaryAgg` (replacing `by: Vec`), and the updated `SummaryExecutor::find_candidates` signature. This commit is the compile-forced fallout of that bump on control_plane alone -- the actual data_plane find_candidates fix is the next commit. control_plane turned out to be in scope because its `intent_algebra` module RE-EXPORTS `asap_ir::intent_algebra` (`pub use`) rather than defining parallel local types, so the field rename cascades through its own L2->L3 lowerer, optimizer, CSE, cost model and physical planner. Sites split into three kinds, handled individually rather than by blind rename: * Pure passthrough (optimizer/engine.rs's recurse arm, optimizer/cse.rs, physical/allocator.rs's reconstructions, sketch_algebra/lower.rs): mechanical `by` -> `reduction`, no semantic content. * Real decision logic (intent_algebra/lower.rs): control_plane has its OWN parallel L2->L3 converter, so it needs its own copy of the `Reduction` decision, not just a rename. Ported ASAPController's rule (`is_per_series() || windowed` under an empty, non-`without` `by`), adapted to this repo's L3 shape convention -- here a range window is `Window` WRAPPING `Aggregate`, whereas ASAPController's current shape puts `TimeRange` in `Aggregate`'s child. Same structural marker for "bare range reduction with no grouping syntax," different node layout. `LQueryExpr::TopK` is always `Reduce` (a ranking never goes per-entity, even with an empty `by`). * Positional-`by` consumers that just need the key list for cost estimation / label-name recovery: use the non-panicking `reduction.group_keys().map(|k| k.keys()).unwrap_or(&[])`. physical/planner.rs's one site is provably always in the `Reduce` branch, so it uses `.expect_reduce()`. `QueryExpr::Sample { by, .. }` is deliberately untouched -- it's a separate field that #165 does not cover (per ASAPController's own design docs, `Sample` is not a reduction). Test fixtures updated to state their intent explicitly rather than leaning on an empty `by`: windowed no-`by` shapes (mirroring `quantile_over_time(m[r])`) become `PerEntity`; unwindowed aggregation operators and TopK become `Reduce([])`. Verified: `cargo check -p control_plane --all-targets` clean; `cargo test -p control_plane --lib` 766 passed / 1 failed, where the one failure (optimizer::rules::tests:: invalid_sketch_type_override_falls_back_to_default) is PRE-EXISTING and unrelated -- confirmed by stashing this whole changeset and reproducing the identical failure on the parent commit. Refs ProjectASAP/ASAPController#163, #164, #165, #166 Co-Authored-By: Claude Sonnet 5 --- Cargo.lock | 46 +++----- control_plane/Cargo.toml | 8 +- control_plane/src/intent_algebra/lower.rs | 102 +++++++++++++----- control_plane/src/intent_algebra/mod.rs | 4 +- .../src/intent_algebra/query_expr.rs | 2 +- control_plane/src/optimizer/cost/mod.rs | 36 +++++-- control_plane/src/optimizer/cse.rs | 20 ++-- control_plane/src/optimizer/engine.rs | 20 ++-- control_plane/src/optimizer/rules/mod.rs | 7 +- control_plane/src/physical/allocator.rs | 27 +++-- .../src/physical/colored_dag/allocator.rs | 2 +- control_plane/src/physical/colored_dag/dag.rs | 4 +- .../src/physical/colored_dag/tests.rs | 6 +- control_plane/src/physical/planner.rs | 17 +-- control_plane/src/physical/window_fusion.rs | 15 ++- control_plane/src/query_parser/mod.rs | 24 +++-- control_plane/src/sketch_algebra/lower.rs | 8 +- .../src/sketch_algebra/physical_expr.rs | 2 +- control_plane/src/sketch_algebra/tests.rs | 45 ++++---- crates/asap_types/Cargo.toml | 4 +- data_plane/Cargo.toml | 6 +- 21 files changed, 245 insertions(+), 160 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 1e736da8..df55c306 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -343,17 +343,7 @@ dependencies = [ [[package]] name = "asap-ir" version = "0.1.0" -source = "git+https://github.com/ProjectASAP/ASAPController?rev=64df20d90c3ddd519c726dea45c05e1fe5225ce6#64df20d90c3ddd519c726dea45c05e1fe5225ce6" -dependencies = [ - "serde", - "serde_json", - "thiserror 2.0.18", -] - -[[package]] -name = "asap-ir" -version = "0.1.0" -source = "git+https://github.com/ProjectASAP/ASAPController?rev=fc09c3aa0b0cf0297ee415d1ed49f7cdc0cc55e4#fc09c3aa0b0cf0297ee415d1ed49f7cdc0cc55e4" +source = "git+https://github.com/ProjectASAP/ASAPController?rev=94795092c5d2d9582ddf7486ac0043ceb9f8a34c#94795092c5d2d9582ddf7486ac0043ceb9f8a34c" dependencies = [ "serde", "serde_json", @@ -363,19 +353,19 @@ dependencies = [ [[package]] name = "asap-l2" version = "0.1.0" -source = "git+https://github.com/ProjectASAP/ASAPController?rev=64df20d90c3ddd519c726dea45c05e1fe5225ce6#64df20d90c3ddd519c726dea45c05e1fe5225ce6" +source = "git+https://github.com/ProjectASAP/ASAPController?rev=94795092c5d2d9582ddf7486ac0043ceb9f8a34c#94795092c5d2d9582ddf7486ac0043ceb9f8a34c" dependencies = [ - "asap-ir 0.1.0 (git+https://github.com/ProjectASAP/ASAPController?rev=64df20d90c3ddd519c726dea45c05e1fe5225ce6)", + "asap-ir", "thiserror 2.0.18", ] [[package]] name = "asap-plan" version = "0.1.0" -source = "git+https://github.com/ProjectASAP/ASAPController?rev=64df20d90c3ddd519c726dea45c05e1fe5225ce6#64df20d90c3ddd519c726dea45c05e1fe5225ce6" +source = "git+https://github.com/ProjectASAP/ASAPController?rev=94795092c5d2d9582ddf7486ac0043ceb9f8a34c#94795092c5d2d9582ddf7486ac0043ceb9f8a34c" dependencies = [ - "asap-ir 0.1.0 (git+https://github.com/ProjectASAP/ASAPController?rev=64df20d90c3ddd519c726dea45c05e1fe5225ce6)", - "asap-sketch 0.1.0 (git+https://github.com/ProjectASAP/ASAPController?rev=64df20d90c3ddd519c726dea45c05e1fe5225ce6)", + "asap-ir", + "asap-sketch", "serde_json", "thiserror 2.0.18", ] @@ -393,17 +383,9 @@ dependencies = [ [[package]] name = "asap-sketch" version = "0.1.0" -source = "git+https://github.com/ProjectASAP/ASAPController?rev=64df20d90c3ddd519c726dea45c05e1fe5225ce6#64df20d90c3ddd519c726dea45c05e1fe5225ce6" -dependencies = [ - "asap-ir 0.1.0 (git+https://github.com/ProjectASAP/ASAPController?rev=64df20d90c3ddd519c726dea45c05e1fe5225ce6)", -] - -[[package]] -name = "asap-sketch" -version = "0.1.0" -source = "git+https://github.com/ProjectASAP/ASAPController?rev=fc09c3aa0b0cf0297ee415d1ed49f7cdc0cc55e4#fc09c3aa0b0cf0297ee415d1ed49f7cdc0cc55e4" +source = "git+https://github.com/ProjectASAP/ASAPController?rev=94795092c5d2d9582ddf7486ac0043ceb9f8a34c#94795092c5d2d9582ddf7486ac0043ceb9f8a34c" dependencies = [ - "asap-ir 0.1.0 (git+https://github.com/ProjectASAP/ASAPController?rev=fc09c3aa0b0cf0297ee415d1ed49f7cdc0cc55e4)", + "asap-ir", ] [[package]] @@ -438,8 +420,8 @@ name = "asap_types" version = "0.1.0" dependencies = [ "anyhow", - "asap-ir 0.1.0 (git+https://github.com/ProjectASAP/ASAPController?rev=fc09c3aa0b0cf0297ee415d1ed49f7cdc0cc55e4)", - "asap-sketch 0.1.0 (git+https://github.com/ProjectASAP/ASAPController?rev=fc09c3aa0b0cf0297ee415d1ed49f7cdc0cc55e4)", + "asap-ir", + "asap-sketch", "clap 4.6.1", "serde", "serde_json", @@ -809,10 +791,10 @@ name = "control_plane" version = "0.1.0" dependencies = [ "anyhow", - "asap-ir 0.1.0 (git+https://github.com/ProjectASAP/ASAPController?rev=64df20d90c3ddd519c726dea45c05e1fe5225ce6)", + "asap-ir", "asap-l2", "asap-plan", - "asap-sketch 0.1.0 (git+https://github.com/ProjectASAP/ASAPController?rev=64df20d90c3ddd519c726dea45c05e1fe5225ce6)", + "asap-sketch", "asap_types", "axum", "bytes", @@ -1007,9 +989,9 @@ dependencies = [ "anyhow", "arc-swap", "arrow", - "asap-ir 0.1.0 (git+https://github.com/ProjectASAP/ASAPController?rev=64df20d90c3ddd519c726dea45c05e1fe5225ce6)", + "asap-ir", "asap-precompute-rs", - "asap-sketch 0.1.0 (git+https://github.com/ProjectASAP/ASAPController?rev=64df20d90c3ddd519c726dea45c05e1fe5225ce6)", + "asap-sketch", "asap_otel_proto", "asap_sketchlib", "asap_types", diff --git a/control_plane/Cargo.toml b/control_plane/Cargo.toml index 764594ee..d4a17415 100644 --- a/control_plane/Cargo.toml +++ b/control_plane/Cargo.toml @@ -68,10 +68,10 @@ asap_types.workspace = true # `AggIntent::Extension` hook this repo's `Extension{"frequency"}` intent # needs (ASAPController#150). 64df20d is a strict descendant of d4c1756 # (the previous pin), so nothing this repo already consumes moves. -asap-ir = { git = "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/ProjectASAP/ASAPController", rev = "64df20d90c3ddd519c726dea45c05e1fe5225ce6" } -asap-l2 = { git = "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/ProjectASAP/ASAPController", rev = "64df20d90c3ddd519c726dea45c05e1fe5225ce6" } -asap-sketch = { git = "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/ProjectASAP/ASAPController", rev = "64df20d90c3ddd519c726dea45c05e1fe5225ce6" } -asap-plan = { git = "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/ProjectASAP/ASAPController", rev = "64df20d90c3ddd519c726dea45c05e1fe5225ce6" } +asap-ir = { git = "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/ProjectASAP/ASAPController", rev = "94795092c5d2d9582ddf7486ac0043ceb9f8a34c" } +asap-l2 = { git = "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/ProjectASAP/ASAPController", rev = "94795092c5d2d9582ddf7486ac0043ceb9f8a34c" } +asap-sketch = { git = "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/ProjectASAP/ASAPController", rev = "94795092c5d2d9582ddf7486ac0043ceb9f8a34c" } +asap-plan = { git = "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/ProjectASAP/ASAPController", rev = "94795092c5d2d9582ddf7486ac0043ceb9f8a34c" } [dev-dependencies] tokio = { version = "1", features = ["full", "test-util"] } diff --git a/control_plane/src/intent_algebra/lower.rs b/control_plane/src/intent_algebra/lower.rs index ef5f86c5..b3a5a3bf 100644 --- a/control_plane/src/intent_algebra/lower.rs +++ b/control_plane/src/intent_algebra/lower.rs @@ -121,7 +121,7 @@ use crate::intent_algebra::column_resolution::{ }; use crate::intent_algebra::query_expr::{ GroupKeys, L3Scalar, Predicate, ProjectItem as CProjectItem, QueryExpr as CQueryExpr, - QueryExprError, SortKey as CSortKey, Source, WindowKind as CWindowKind, + QueryExprError, Reduction, SortKey as CSortKey, Source, WindowKind as CWindowKind, }; use crate::intent_algebra::relational::{AggFunc, QueryExpr as LQueryExpr}; use crate::intent_algebra::schema::Schema; @@ -270,6 +270,21 @@ pub fn convert(legacy: &LQueryExpr, schema: &Schema) -> Result = match input.as_ref() { LQueryExpr::Window { duration, @@ -284,17 +299,26 @@ pub fn convert(legacy: &LQueryExpr, schema: &Schema) -> Result Result Result Result { - assert!(by.is_empty(), "no GROUP BY → empty `by`: {by:?}"); + CQueryExpr::Aggregate { + reduction, aggs, .. + } => { + assert!( + matches!(&reduction, Reduction::Reduce(k) if k.is_empty()), + "no GROUP BY, unwindowed, non-per-series intent → global reduce: {reduction:?}" + ); assert!(matches!(aggs.as_slice(), [AggIntent::Sum { col: None }])); } other => panic!("expected Aggregate, got {other:?}"), @@ -822,7 +866,9 @@ mod tests { src("trades"), ); match convert_root(&legacy).unwrap() { - CQueryExpr::Aggregate { by, .. } => assert!(by.is_empty()), + CQueryExpr::Aggregate { reduction, .. } => { + assert!(matches!(&reduction, Reduction::Reduce(k) if k.is_empty())) + } other => panic!("expected Aggregate, got {other:?}"), } } @@ -844,8 +890,8 @@ mod tests { assert_eq!(kind, CWindowKind::Tumbling); assert!(matches!( *child, - CQueryExpr::Aggregate { ref by, ref aggs, .. } - if by.is_empty() + CQueryExpr::Aggregate { ref reduction, ref aggs, .. } + if matches!(reduction, Reduction::PerEntity) && matches!(aggs.as_slice(), [AggIntent::Quantile { .. }]) )); } @@ -939,8 +985,10 @@ mod tests { input: Box::new(src("m")), }; match convert_root(&legacy).unwrap() { - CQueryExpr::Aggregate { by, aggs, .. } => { - assert!(by.is_empty()); + CQueryExpr::Aggregate { + reduction, aggs, .. + } => { + assert!(matches!(&reduction, Reduction::Reduce(k) if k.is_empty())); assert!(matches!(aggs.as_slice(), [AggIntent::TopK { k: 5, .. }])); } other => panic!("expected Aggregate, got {other:?}"), diff --git a/control_plane/src/intent_algebra/mod.rs b/control_plane/src/intent_algebra/mod.rs index 55267aff..cc08b700 100644 --- a/control_plane/src/intent_algebra/mod.rs +++ b/control_plane/src/intent_algebra/mod.rs @@ -113,8 +113,8 @@ pub use expr_ir::{ArithOp, ColumnRef, CompareOp, Expr, L2Expr, L3Expr, L3Scalar} pub use query_expr::{ aggregate_output_schema, between, conjoin, label_filter_to_predicate, AtModifier, BinaryOpKind, BindingScope, DataModel, GroupKeys, GroupSide, InfoMatcher, JoinKind, LabelFilter, Predicate, - ProjectItem, QueryExpr, QueryExprError, SampleKind, SetOpKind, SortKey, Source, TimeShift, - VectorGrouping, VectorMatch, VectorMatchKind, WindowFuncKind, WindowKind, + ProjectItem, QueryExpr, QueryExprError, Reduction, SampleKind, SetOpKind, SortKey, Source, + TimeShift, VectorGrouping, VectorMatch, VectorMatchKind, WindowFuncKind, WindowKind, }; pub use schema::{cse_reuse_is_legal, Column, ColumnId, CseError, DataType, Schema}; diff --git a/control_plane/src/intent_algebra/query_expr.rs b/control_plane/src/intent_algebra/query_expr.rs index ed787243..0ba5e459 100644 --- a/control_plane/src/intent_algebra/query_expr.rs +++ b/control_plane/src/intent_algebra/query_expr.rs @@ -61,7 +61,7 @@ use asap_ir::intent_algebra::schema::ColumnId; pub use asap_ir::intent_algebra::{ aggregate_output_schema, AtModifier, BinaryOpKind, BindingScope, DataModel, GroupKeys, GroupSide, InfoMatcher, JoinKind, Predicate, ProjectItem, QueryExpr, QueryExprError, - SampleKind, SetOpKind, SortKey, Source, TimeShift, VectorGrouping, VectorMatch, + Reduction, SampleKind, SetOpKind, SortKey, Source, TimeShift, VectorGrouping, VectorMatch, VectorMatchKind, WindowFuncKind, WindowKind, }; pub use asap_ir::intent_algebra::{ArithOp, ColumnRef, CompareOp, Expr, L3Scalar}; diff --git a/control_plane/src/optimizer/cost/mod.rs b/control_plane/src/optimizer/cost/mod.rs index 6c10eb11..628e680c 100644 --- a/control_plane/src/optimizer/cost/mod.rs +++ b/control_plane/src/optimizer/cost/mod.rs @@ -355,7 +355,7 @@ fn apply_delta_decision_with( use asap_ir::intent_algebra::{BindingName, QueryId}; #[allow(unused_imports)] -use crate::intent_algebra::{AggIntent, BindingScope, QueryExpr, QueryExprError, Schema}; +use crate::intent_algebra::{AggIntent, BindingScope, QueryExpr, QueryExprError, Reduction, Schema}; /// Bundled cost of a multi-query workload, with per-root contributions /// and the savings unlocked by shared-producer credit. Returned by @@ -503,10 +503,14 @@ fn subtree_cost_bundled( Ok(node_cost_window(&in_schema) + cs) } QueryExpr::Aggregate { - by, aggs, child, .. + reduction, + aggs, + child, + .. } => { let cs = subtree_cost_bundled(child, binding_costs, schema_scope)?; let in_schema = child.output_schema_in(schema_scope)?; + let by = reduction.group_keys().map(|k| k.keys()).unwrap_or(&[]); Ok(node_cost_aggregate(by, aggs, &in_schema) + cs) } QueryExpr::LetBinding { name, expr, child } => { @@ -589,10 +593,14 @@ fn subtree_cost_standalone( Ok(node_cost_window(&in_schema) + cs) } QueryExpr::Aggregate { - by, aggs, child, .. + reduction, + aggs, + child, + .. } => { let cs = subtree_cost_standalone(child, binding_costs, schema_scope)?; let in_schema = child.output_schema_in(schema_scope)?; + let by = reduction.group_keys().map(|k| k.keys()).unwrap_or(&[]); Ok(node_cost_aggregate(by, aggs, &in_schema) + cs) } QueryExpr::LetBinding { name, expr, child } => { @@ -783,10 +791,18 @@ mod workload_cost_tests { } } - /// Wrap `child` in `Aggregate { by: [], aggs: [Quantile{q}] }`. + /// Wrap `child` in `Aggregate { reduction: [], aggs: [Quantile{q}] }` + /// — no `by()`, so `reduction` follows `lower.rs`'s own rule: + /// `PerEntity` when `child` is itself a `Window` (matches a bare + /// `quantile_over_time(...)`), `Reduce([])` otherwise. fn quantile_root(q: f64, child: QueryExpr) -> QueryExpr { + let reduction = if matches!(child, QueryExpr::Window { .. }) { + Reduction::PerEntity + } else { + Reduction::by(vec![]) + }; QueryExpr::Aggregate { - by: vec![].into(), + reduction, aggs: vec![AggIntent::Quantile { col: None, q, @@ -798,10 +814,16 @@ mod workload_cost_tests { } } - /// Wrap `child` in `Aggregate { by: [], aggs: [Max] }`. + /// Wrap `child` in `Aggregate { reduction: [], aggs: [Max] }` — same + /// `PerEntity`-when-windowed rule as `quantile_root`. fn max_root(child: QueryExpr) -> QueryExpr { + let reduction = if matches!(child, QueryExpr::Window { .. }) { + Reduction::PerEntity + } else { + Reduction::by(vec![]) + }; QueryExpr::Aggregate { - by: vec![].into(), + reduction, aggs: vec![AggIntent::Max { col: None }], output_names: Vec::new(), having: None, diff --git a/control_plane/src/optimizer/cse.rs b/control_plane/src/optimizer/cse.rs index 1014fce9..99c88ae6 100644 --- a/control_plane/src/optimizer/cse.rs +++ b/control_plane/src/optimizer/cse.rs @@ -163,13 +163,13 @@ pub fn dedupe_subtrees(roots: Vec<(QueryId, QueryExpr)>) -> CseWorkloadPlan { for (qid, root) in roots { let new_root = match root { QueryExpr::Aggregate { - by, + reduction, aggs, output_names, having, child, } if *child == shared_expr => QueryExpr::Aggregate { - by, + reduction, aggs, output_names, having, @@ -194,7 +194,7 @@ pub fn dedupe_subtrees(roots: Vec<(QueryId, QueryExpr)>) -> CseWorkloadPlan { mod tests { use super::*; use crate::intent_algebra::agg_intent::AggIntent; - use crate::intent_algebra::query_expr::{LabelFilter, Source, WindowKind}; + use crate::intent_algebra::query_expr::{LabelFilter, Reduction, Source, WindowKind}; use crate::intent_algebra::schema::{Column, DataType, Schema}; use crate::types_v2::AccuracyTarget; use std::time::Duration; @@ -254,7 +254,7 @@ mod tests { #[test] fn dedupe_subtrees_single_root_passthrough() { let q = QueryExpr::Aggregate { - by: vec![1].into(), + reduction: Reduction::by(vec![1]), aggs: vec![AggIntent::Quantile { col: None, q: 0.99, @@ -280,7 +280,7 @@ mod tests { #[test] fn quantiles_over_different_columns_do_not_dedupe() { let mk = |col: usize| QueryExpr::Aggregate { - by: vec![1].into(), + reduction: Reduction::by(vec![1]), aggs: vec![AggIntent::Quantile { col: Some(col), q: 0.5, @@ -306,7 +306,7 @@ mod tests { #[test] fn dedupe_subtrees_basic() { let q1 = QueryExpr::Aggregate { - by: vec![1].into(), + reduction: Reduction::by(vec![1]), aggs: vec![AggIntent::Quantile { col: None, q: 0.99, @@ -317,7 +317,7 @@ mod tests { child: Box::new(windowed_scan()), }; let q2 = QueryExpr::Aggregate { - by: vec![1].into(), + reduction: Reduction::by(vec![1]), aggs: vec![AggIntent::Quantile { col: None, q: 0.95, @@ -354,7 +354,7 @@ mod tests { #[test] fn dedupe_subtrees_no_shared_subexpr() { let q1 = QueryExpr::Aggregate { - by: vec![].into(), + reduction: Reduction::PerEntity, aggs: vec![AggIntent::Sum { col: None }], output_names: Vec::new(), having: None, @@ -378,7 +378,7 @@ mod tests { ), }; let q2 = QueryExpr::Aggregate { - by: vec![].into(), + reduction: Reduction::PerEntity, aggs: vec![AggIntent::Max { col: None }], output_names: Vec::new(), having: None, @@ -417,7 +417,7 @@ mod tests { ), }; let mk = || QueryExpr::Aggregate { - by: vec![].into(), + reduction: Reduction::by(vec![]), aggs: vec![AggIntent::Sum { col: None }], output_names: Vec::new(), having: None, diff --git a/control_plane/src/optimizer/engine.rs b/control_plane/src/optimizer/engine.rs index 3ebbb9cc..6de5826a 100644 --- a/control_plane/src/optimizer/engine.rs +++ b/control_plane/src/optimizer/engine.rs @@ -33,7 +33,7 @@ use std::collections::HashMap; use asap_ir::intent_algebra::BindingName; use crate::intent_algebra::agg_intent::AggIntent; -use crate::intent_algebra::query_expr::{GroupKeys, QueryExpr, SetOpKind, Source}; +use crate::intent_algebra::query_expr::{QueryExpr, Reduction, SetOpKind, Source}; use crate::intent_algebra::relational::{agg_is_exact, agg_is_mergeable}; use crate::optimizer::cost::sketch_capability::{ default_capability_table, load_capability_overrides, SketchCapability, @@ -396,7 +396,7 @@ impl RewriteRule for MergeLifting { fn try_rewrite(&self, expr: QueryExpr, _model: &dyn CostModel) -> Option { match expr { QueryExpr::Aggregate { - ref by, + ref reduction, ref aggs, ref having, ref child, @@ -406,7 +406,7 @@ impl RewriteRule for MergeLifting { let new_children: Vec = children .iter() .map(|branch| QueryExpr::Aggregate { - by: by.clone(), + reduction: reduction.clone(), aggs: aggs.clone(), output_names: Vec::new(), having: None, @@ -440,7 +440,7 @@ impl RewriteRule for HLLDedupElim { fn try_rewrite(&self, expr: QueryExpr, _model: &dyn CostModel) -> Option { match expr { QueryExpr::Aggregate { - by, + reduction, aggs, output_names, having: None, @@ -448,7 +448,7 @@ impl RewriteRule for HLLDedupElim { } if aggs.len() == 1 && matches!(&aggs[0], AggIntent::Cardinality { .. }) => { if let QueryExpr::Distinct { child: inner, .. } = *child { return Some(QueryExpr::Aggregate { - by, + reduction, aggs, output_names, having: None, @@ -523,7 +523,7 @@ impl RewriteRule for TopKFusion { // Only fuse when all keys are DESC (top-k semantics). if !keys.is_empty() && keys.iter().all(|k| !k.ascending) { return Some(QueryExpr::Aggregate { - by: GroupKeys::none(), + reduction: Reduction::by(vec![]), aggs: vec![AggIntent::TopK { k: n, accuracy: AccuracyTarget::Epsilon(0.05), @@ -861,7 +861,7 @@ impl QueryOptimizer { ) } QueryExpr::Aggregate { - by, + reduction, aggs, output_names, having, @@ -870,7 +870,7 @@ impl QueryOptimizer { let (new_child, c) = recurse!(child); ( QueryExpr::Aggregate { - by, + reduction, aggs, output_names, having, @@ -1165,7 +1165,7 @@ impl OptimizerRule for CommonSubexprElim { #[cfg(test)] mod tests { use super::*; - use crate::intent_algebra::query_expr::Predicate; + use crate::intent_algebra::query_expr::{GroupKeys, Predicate}; use crate::intent_algebra::relational::{default_cardinality, default_quantile}; use crate::intent_algebra::{ BinaryOpKind, L3Expr, L3Scalar, Schema, SortKey, Source, WindowKind, @@ -1187,7 +1187,7 @@ mod tests { /// the legacy `SketchAgg`. fn sketch_agg(intent: AggIntent, child: QueryExpr) -> QueryExpr { QueryExpr::Aggregate { - by: GroupKeys::none(), + reduction: Reduction::by(vec![]), aggs: vec![intent], output_names: Vec::new(), having: None, diff --git a/control_plane/src/optimizer/rules/mod.rs b/control_plane/src/optimizer/rules/mod.rs index 816bd286..3d78e577 100644 --- a/control_plane/src/optimizer/rules/mod.rs +++ b/control_plane/src/optimizer/rules/mod.rs @@ -231,7 +231,12 @@ pub fn bind_workload_typed_with_item_filter( child: Box::new(scan), }; let aggregate = QueryExpr::Aggregate { - by: crate::intent_algebra::GroupKeys::none(), + // Synthetic probe only -- `boundary::implementation_for` (what + // this shape actually drives) keys off `AggIntent`/accuracy alone, + // never `Reduction`, so this value doesn't affect the family pick. + // `PerEntity` is the representative choice for a windowed shape + // with no `by` (ASAPController#163/#165). + reduction: crate::intent_algebra::Reduction::PerEntity, aggs: vec![intent], output_names: Vec::new(), having: None, diff --git a/control_plane/src/physical/allocator.rs b/control_plane/src/physical/allocator.rs index d49ee2c1..85a09236 100644 --- a/control_plane/src/physical/allocator.rs +++ b/control_plane/src/physical/allocator.rs @@ -40,9 +40,8 @@ use super::plan::{CostEstimate, ExecutionMode, NodeAnnotation, PipelineStage, PlanNode}; use crate::intent_algebra::agg_intent::AggIntent; -use crate::intent_algebra::query_expr::GroupKeys; use crate::intent_algebra::relational::agg_is_exact; -use crate::intent_algebra::QueryExpr; +use crate::intent_algebra::{QueryExpr, Reduction}; use crate::types::{SketchType, StageResourceBudgets}; // ── Resource budget tracker ─────────────────────────────────────────────────── @@ -202,7 +201,7 @@ impl SketchAllocator { // * single other intent, no HAVING → budget-driven sketch // * multi-intent or HAVING → general exact Aggregate at Db QueryExpr::Aggregate { - by, + reduction, aggs, output_names, having, @@ -214,7 +213,7 @@ impl SketchAllocator { let child = self.alloc_node(*child, budget); return PlanNode { expr: QueryExpr::Aggregate { - by, + reduction, aggs, output_names, having, @@ -239,14 +238,14 @@ impl SketchAllocator { } // Single non-TopK intent → budget-driven sketch agg. let child = self.alloc_node(*child, budget); - return self.alloc_sketch_agg(by, aggs, output_names, child, budget); + return self.alloc_sketch_agg(reduction, aggs, output_names, child, budget); } // General multi-intent / HAVING aggregate → Db (exact). let child = self.alloc_node(*child, budget); let kinds: Vec<&'static str> = aggs.iter().map(canonical_intent_kind_str).collect(); PlanNode { expr: QueryExpr::Aggregate { - by, + reduction, aggs, output_names, having, @@ -609,7 +608,7 @@ impl SketchAllocator { /// `aggs.len() == 1` and that the single intent is not `TopK`. fn alloc_sketch_agg( &self, - by: GroupKeys, + reduction: Reduction, aggs: Vec, output_names: Vec, child: PlanNode, @@ -621,7 +620,7 @@ impl SketchAllocator { if matches!(intent, AggIntent::Avg { .. }) { return PlanNode { expr: QueryExpr::Aggregate { - by, + reduction, aggs, output_names, having: None, @@ -645,7 +644,7 @@ impl SketchAllocator { if agg_is_exact(&intent) { return PlanNode { expr: QueryExpr::Aggregate { - by, + reduction, aggs, output_names, having: None, @@ -675,7 +674,7 @@ impl SketchAllocator { budget.consume_agent(mem); return PlanNode { expr: QueryExpr::Aggregate { - by, + reduction, aggs, output_names, having: None, @@ -703,7 +702,7 @@ impl SketchAllocator { budget.consume_backend(mem); return PlanNode { expr: QueryExpr::Aggregate { - by, + reduction, aggs, output_names, having: None, @@ -731,7 +730,7 @@ impl SketchAllocator { // Both Agent and Backend budgets exceeded → Precompute. PlanNode { expr: QueryExpr::Aggregate { - by, + reduction, aggs, output_names, having: None, @@ -845,7 +844,7 @@ mod tests { /// `Scan` — the canonical shape the legacy `SketchAgg` folded into. fn agg(intent: AggIntent) -> QueryExpr { QueryExpr::Aggregate { - by: vec![].into(), + reduction: Reduction::by(vec![]), aggs: vec![intent], output_names: Vec::new(), having: None, @@ -1006,7 +1005,7 @@ mod tests { #[test] fn multi_intent_aggregate_goes_to_db() { let expr = QueryExpr::Aggregate { - by: vec![].into(), + reduction: Reduction::by(vec![]), aggs: vec![AggIntent::Sum { col: None }, AggIntent::Min { col: None }], output_names: Vec::new(), having: None, diff --git a/control_plane/src/physical/colored_dag/allocator.rs b/control_plane/src/physical/colored_dag/allocator.rs index 7dcd41dc..72f9d133 100644 --- a/control_plane/src/physical/colored_dag/allocator.rs +++ b/control_plane/src/physical/colored_dag/allocator.rs @@ -399,7 +399,7 @@ mod tests { #[test] fn three_stage_quantile_dag_basic() { let q = QueryExpr::Aggregate { - by: crate::intent_algebra::GroupKeys::none(), + reduction: crate::intent_algebra::Reduction::PerEntity, aggs: vec![crate::intent_algebra::AggIntent::Quantile { col: None, q: 0.99, diff --git a/control_plane/src/physical/colored_dag/dag.rs b/control_plane/src/physical/colored_dag/dag.rs index fe08ca8b..ae268f9c 100644 --- a/control_plane/src/physical/colored_dag/dag.rs +++ b/control_plane/src/physical/colored_dag/dag.rs @@ -186,7 +186,7 @@ mod tests { fn dummy_agg() -> PhysicalExpr { let q = QueryExpr::Aggregate { - by: crate::intent_algebra::GroupKeys::none(), + reduction: crate::intent_algebra::Reduction::by(vec![]), aggs: vec![crate::intent_algebra::AggIntent::Sum { col: None }], output_names: Vec::new(), having: None, @@ -197,7 +197,7 @@ mod tests { fn dummy_estimate() -> PhysicalExpr { let q = QueryExpr::Aggregate { - by: crate::intent_algebra::GroupKeys::none(), + reduction: crate::intent_algebra::Reduction::by(vec![]), aggs: vec![crate::intent_algebra::AggIntent::Quantile { col: None, q: 0.99, diff --git a/control_plane/src/physical/colored_dag/tests.rs b/control_plane/src/physical/colored_dag/tests.rs index 642411fb..77a5e0fd 100644 --- a/control_plane/src/physical/colored_dag/tests.rs +++ b/control_plane/src/physical/colored_dag/tests.rs @@ -13,7 +13,7 @@ use std::time::Duration; use asap_sketch::{L4Node, L4Schema, SketchQuery, SummaryExpr, SummaryKind, SummaryParams}; use crate::intent_algebra::{ - BindingScope, ColumnRef, GroupKeys, LabelFilter, QueryExpr, Schema, Source, WindowKind, + BindingScope, ColumnRef, LabelFilter, QueryExpr, Reduction, Schema, Source, WindowKind, }; use crate::intent_algebra::schema::{Column, DataType}; use crate::physical::colored_dag::allocator::StageAllocator; @@ -111,7 +111,7 @@ fn sketch_agg_l4(sketch: SummaryKind, params: SummaryParams, child: Rc) sketch, params, col: ColumnRef::SampleValue, - by: vec![], + reduction: Reduction::by(vec![]), }, schema: dummy_l4_schema(), }) @@ -164,7 +164,7 @@ fn is_ref(expr: &PhysicalExpr) -> bool { /// `boundary::summary_candidates`). fn quantile_kll_dag() -> PhysicalExpr { let q = QueryExpr::Aggregate { - by: GroupKeys::none(), + reduction: Reduction::by(vec![]), aggs: vec![crate::intent_algebra::AggIntent::Quantile { col: None, q: 0.99, diff --git a/control_plane/src/physical/planner.rs b/control_plane/src/physical/planner.rs index 65fec222..bff56df6 100644 --- a/control_plane/src/physical/planner.rs +++ b/control_plane/src/physical/planner.rs @@ -290,7 +290,7 @@ fn plan_node(expr: &QueryExpr, config: &PhysicalPlannerConfig) -> PhysicalNode { // * single other intent, no HAVING → sketch build, budget-placed // * multi-intent or HAVING → exact HashAggregate at QueryEngine QueryExpr::Aggregate { - by, + reduction, aggs, having, child, @@ -334,6 +334,9 @@ fn plan_node(expr: &QueryExpr, config: &PhysicalPlannerConfig) -> PhysicalNode { } // Multi-intent / HAVING aggregate → no single sketch can serve // it; fall back to an exact hash aggregation at the query engine. + // Always a genuine reduction (never per-entity -- see + // `intent_algebra::lower`'s single-intent-only per-entity rule). + let by = reduction.expect_reduce(); let child = plan_node(child, config); let mut node = PhysicalNode { op: PhysicalOp::HashAggregate { @@ -588,7 +591,7 @@ mod tests { use crate::intent_algebra::relational::{ default_cardinality, default_frequency, default_quantile, }; - use crate::intent_algebra::{Schema, Source, WindowKind}; + use crate::intent_algebra::{Reduction, Schema, Source, WindowKind}; use crate::types_v2::AccuracyTarget; fn default_config() -> PhysicalPlannerConfig { @@ -613,7 +616,7 @@ mod tests { /// canonical fold of the legacy `SketchAgg`. fn sketch_agg(intent: AggIntent, metric: &str) -> QueryExpr { QueryExpr::Aggregate { - by: vec![].into(), + reduction: Reduction::by(vec![]), aggs: vec![intent], output_names: Vec::new(), having: None, @@ -695,7 +698,7 @@ mod tests { #[test] fn plan_topk_at_query_engine() { let expr = QueryExpr::Aggregate { - by: vec![].into(), + reduction: Reduction::by(vec![]), aggs: vec![AggIntent::TopK { k: 10, accuracy: AccuracyTarget::Epsilon(0.05), @@ -714,7 +717,7 @@ mod tests { // TopK(QueryEngine) wrapping a sketch Aggregate(Agent) → Exchange // between them. let expr = QueryExpr::Aggregate { - by: vec![].into(), + reduction: Reduction::by(vec![]), aggs: vec![AggIntent::TopK { k: 5, accuracy: AccuracyTarget::Epsilon(0.05), @@ -744,7 +747,7 @@ mod tests { // Multi-intent Aggregate → exact HashAggregate at QueryEngine // (no single sketch serves multiple intents). let expr = QueryExpr::Aggregate { - by: vec![].into(), + reduction: Reduction::by(vec![]), aggs: vec![AggIntent::Sum { col: None }, AggIntent::Min { col: None }], output_names: Vec::new(), having: None, @@ -762,7 +765,7 @@ mod tests { // Backend, and the outer `TopK` intent runs at QueryEngine. // Should span: Agent → Backend → QueryEngine let expr = QueryExpr::Aggregate { - by: vec![].into(), + reduction: Reduction::by(vec![]), aggs: vec![AggIntent::TopK { k: 10, accuracy: AccuracyTarget::Epsilon(0.05), diff --git a/control_plane/src/physical/window_fusion.rs b/control_plane/src/physical/window_fusion.rs index bf42aab6..99845e1b 100644 --- a/control_plane/src/physical/window_fusion.rs +++ b/control_plane/src/physical/window_fusion.rs @@ -46,7 +46,10 @@ pub struct FusedWindowSketch<'a> { pub window_size: Duration, /// Outer `Window`'s `slide` (`Some` only for `Sliding`). pub window_slide: Option, - /// The inner `Aggregate`'s `by` columns. + /// The inner `Aggregate`'s grouping columns -- empty both when it's a + /// genuine empty-`by` reduction and when it's per-entity (no grouping + /// concept at all, ASAPController#163/#165); this field doesn't + /// distinguish the two, since nothing downstream currently needs to. pub by: &'a [ColumnId], /// The subtree below the inner `Aggregate` — the sketch's input. pub inner_child: &'a QueryExpr, @@ -74,7 +77,7 @@ pub fn recognize_windowed_sketch(expr: &QueryExpr) -> Option Option { // The canonical IR folds legacy SketchAgg / WindowedAgg-inner // / TopK / Aggregate into one variant carrying `AggIntent`s. // `by` is positional — recover the group-by label *names* // from the Binder schema (the legacy `Aggregate.keys` were // names; multi-agg aggregates reach here with a non-empty - // `by` after the Binder resolves them). + // `by` after the Binder resolves them). A per-entity + // reduction (ASAPController#163/#165) has no `by` at all — + // same as an empty one here, no label names to recover. + let by: &[ColumnId] = reduction.group_keys().map(|k| k.keys()).unwrap_or(&[]); if let Some(s) = schema { for &id in by { if let Some(col) = s.columns.get(id) { @@ -562,7 +568,7 @@ mod doc_verify_all { // `Aggregate { by: [], .. }` stacked forms. use super::parse_query_expr_canonical; use crate::intent_algebra::query_expr::QueryExpr; - use crate::intent_algebra::AggIntent; + use crate::intent_algebra::{AggIntent, Reduction}; #[test] fn example4_promql_quantile() { @@ -571,11 +577,15 @@ mod doc_verify_all { ) .unwrap(); // Canonical fold of the legacy `WindowedAgg { Quantile }`: - // `Window { Aggregate { by: [], [Quantile] } }`. + // `Window { Aggregate { reduction: PerEntity, [Quantile] } }` — no + // explicit `by()`, and windowed with a single non-per-series intent, + // so there's no grouping concept at all (see #165's `Reduction`). match &expr { QueryExpr::Window { child, .. } => match child.as_ref() { - QueryExpr::Aggregate { by, aggs, .. } => { - assert!(by.is_empty()); + QueryExpr::Aggregate { + reduction, aggs, .. + } => { + assert!(matches!(reduction, Reduction::PerEntity)); assert!(matches!(aggs.as_slice(), [AggIntent::Quantile { .. }])); } other => panic!("expected Aggregate under Window, got {other:?}"), diff --git a/control_plane/src/sketch_algebra/lower.rs b/control_plane/src/sketch_algebra/lower.rs index ec8cb310..6067b2e3 100644 --- a/control_plane/src/sketch_algebra/lower.rs +++ b/control_plane/src/sketch_algebra/lower.rs @@ -87,7 +87,7 @@ fn bind_recursive(expr: &QueryExpr, accuracy: &AccuracyTarget) -> Result { let QueryExpr::Aggregate { - by, + reduction, aggs, output_names, having, @@ -97,7 +97,7 @@ fn bind_recursive(expr: &QueryExpr, accuracy: &AccuracyTarget) -> Result Result QueryExpr { match expr { QueryExpr::Aggregate { - by, + reduction, aggs, output_names, having, child, } => QueryExpr::Aggregate { - by: by.clone(), + reduction: reduction.clone(), aggs: aggs .iter() .map(|intent| { diff --git a/control_plane/src/sketch_algebra/physical_expr.rs b/control_plane/src/sketch_algebra/physical_expr.rs index ad0e6059..41fa8a72 100644 --- a/control_plane/src/sketch_algebra/physical_expr.rs +++ b/control_plane/src/sketch_algebra/physical_expr.rs @@ -200,7 +200,7 @@ mod tests { #[test] fn committed_wraps_an_implement_tree_result() { let q = QueryExpr::Aggregate { - by: crate::intent_algebra::GroupKeys::none(), + reduction: crate::intent_algebra::Reduction::by(vec![]), aggs: vec![crate::intent_algebra::AggIntent::Quantile { col: None, q: 0.99, diff --git a/control_plane/src/sketch_algebra/tests.rs b/control_plane/src/sketch_algebra/tests.rs index 1fd9d55f..a03490ea 100644 --- a/control_plane/src/sketch_algebra/tests.rs +++ b/control_plane/src/sketch_algebra/tests.rs @@ -10,7 +10,7 @@ use asap_sketch::{L4Node, SketchQuery, SummaryExpr, SummaryKind, SummaryParams}; use crate::intent_algebra::schema::{Column, DataType}; use crate::intent_algebra::{ - AggIntent, BindingScope, LabelFilter, QueryExpr, Schema, Source, WindowKind, + AggIntent, BindingScope, LabelFilter, QueryExpr, Reduction, Schema, Source, WindowKind, }; use crate::sketch_algebra::cost_model::ForcedFamilyCostModel; use crate::sketch_algebra::lower::bind_query_expr; @@ -64,7 +64,10 @@ fn windowed_scan() -> QueryExpr { fn agg_quantile(q: f64, accuracy: AccuracyTarget) -> QueryExpr { QueryExpr::Aggregate { - by: crate::intent_algebra::GroupKeys::none(), + // No `by()` and windowed (child is `Window`) — the shape + // `quantile_over_time(...)` lowers to: per-series, not a + // cross-series reduction (see #165's `Reduction`). + reduction: Reduction::PerEntity, aggs: vec![AggIntent::Quantile { col: None, q, @@ -219,7 +222,9 @@ fn bind_picks_ddsketch_over_kll_when_eps_explicit() { /// Build an `Aggregate{TopK{k, accuracy}}` over the windowed scan. fn agg_topk(k: usize, accuracy: AccuracyTarget) -> QueryExpr { QueryExpr::Aggregate { - by: vec![].into(), + // A ranking always reduces — empty `by` ranks the whole input, + // never per-entity (see `lower.rs`'s `LQueryExpr::TopK` handling). + reduction: Reduction::by(vec![]), aggs: vec![AggIntent::TopK { k, accuracy }], output_names: Vec::new(), having: None, @@ -355,7 +360,7 @@ fn bind_cms_topk_picks_cost_min_meeting_sla() { #[test] fn bind_hll_cardinality_basic() { let expr = QueryExpr::Aggregate { - by: vec![].into(), + reduction: Reduction::PerEntity, aggs: vec![AggIntent::Cardinality { col: None, accuracy: AccuracyTarget::Epsilon(0.01), @@ -406,7 +411,7 @@ fn sum_now_binds_to_exact_agg_after_pr_6_followup() { // node shape, keyed by `SummaryKind` (see `physical_expr.rs`'s // module docs). let expr = QueryExpr::Aggregate { - by: vec![].into(), + reduction: Reduction::PerEntity, aggs: vec![AggIntent::Sum { col: None }], output_names: Vec::new(), having: None, @@ -462,7 +467,7 @@ fn bind_exact_accuracy_disables_quantile_binding() { #[test] fn phase_b_pattern_only_temporal_quantile_binds_to_sketch() { let expr = QueryExpr::Aggregate { - by: vec![].into(), + reduction: Reduction::PerEntity, aggs: vec![AggIntent::Quantile { col: None, q: 0.99, @@ -503,7 +508,7 @@ fn phase_b_pattern_only_temporal_quantile_binds_to_sketch() { #[test] fn phase_b_pattern_only_temporal_sum_binds_to_exact_agg() { let expr = QueryExpr::Aggregate { - by: vec![].into(), + reduction: Reduction::PerEntity, aggs: vec![AggIntent::Sum { col: None }], output_names: Vec::new(), having: None, @@ -532,7 +537,7 @@ fn phase_b_pattern_only_temporal_sum_binds_to_exact_agg() { #[test] fn phase_b_pattern_only_spatial_aggregate_binds_to_multiple_sum() { let expr = QueryExpr::Aggregate { - by: vec![1].into(), // service column + reduction: Reduction::by(vec![1]), // service column aggs: vec![AggIntent::Sum { col: None }], output_names: Vec::new(), having: None, @@ -541,11 +546,13 @@ fn phase_b_pattern_only_spatial_aggregate_binds_to_multiple_sum() { let bound = bind_query_expr(&expr, AccuracyTarget::Epsilon(0.01)).unwrap(); match bound { PhysicalExpr::Committed(L4Plan::Summary(node)) => match &node.expr { - SummaryExpr::SummaryAgg { sketch, by, .. } => { + SummaryExpr::SummaryAgg { + sketch, reduction, .. + } => { assert_eq!(sketch, &SummaryKind::Sum); assert_eq!( - by, - &vec![1], + reduction.group_keys().map(|k| k.keys()), + Some(&[1][..]), "keyed sum must carry the group-by column (the MultipleSum-equivalent signal)" ); } @@ -565,7 +572,7 @@ fn phase_b_pattern_only_spatial_aggregate_binds_to_multiple_sum() { #[test] fn phase_b_pattern_temporal_and_spatial_combined_binds_to_multiple_increase() { let expr = QueryExpr::Aggregate { - by: vec![1].into(), + reduction: Reduction::by(vec![1]), aggs: vec![AggIntent::Rate], output_names: Vec::new(), having: None, @@ -574,9 +581,11 @@ fn phase_b_pattern_temporal_and_spatial_combined_binds_to_multiple_increase() { let bound = bind_query_expr(&expr, AccuracyTarget::Epsilon(0.01)).unwrap(); match bound { PhysicalExpr::Committed(L4Plan::Summary(node)) => match &node.expr { - SummaryExpr::SummaryAgg { sketch, by, .. } => { + SummaryExpr::SummaryAgg { + sketch, reduction, .. + } => { assert_eq!(sketch, &SummaryKind::Increase); - assert_eq!(by, &vec![1]); + assert_eq!(reduction.group_keys().map(|k| k.keys()), Some(&[1][..])); } other => panic!("expected SummaryAgg(Increase, by=[1]), got {other:?}"), }, @@ -603,7 +612,7 @@ fn phase_b_pattern_archive_only_routes_to_archive() { "Phase β intent must flag archive" ); let expr = QueryExpr::Aggregate { - by: vec![].into(), + reduction: Reduction::PerEntity, aggs: vec![intent.clone()], output_names: Vec::new(), having: None, @@ -787,7 +796,7 @@ fn phase_b_e2e_topk_well_formed() { fn phase_b_e2e_archive_only_e2e_binding() { let intent = AggIntent::Absent; let expr = QueryExpr::Aggregate { - by: vec![].into(), + reduction: Reduction::PerEntity, aggs: vec![intent.clone()], output_names: Vec::new(), having: None, @@ -830,7 +839,7 @@ fn phase_b_archive_only_intents_round_trip_through_binder() { ]; for intent in intents { let expr = QueryExpr::Aggregate { - by: vec![].into(), + reduction: Reduction::PerEntity, aggs: vec![intent.clone()], output_names: Vec::new(), having: None, @@ -878,7 +887,7 @@ fn frequency_extension_binds_cms() { // as a real `Cms` sketch instead of declining to `Logical`. let intent = crate::intent_algebra::frequency(AccuracyTarget::Epsilon(0.01), None); let expr = QueryExpr::Aggregate { - by: vec![].into(), + reduction: Reduction::PerEntity, aggs: vec![intent], output_names: Vec::new(), having: None, diff --git a/crates/asap_types/Cargo.toml b/crates/asap_types/Cargo.toml index 11c54ca7..8da21b4e 100644 --- a/crates/asap_types/Cargo.toml +++ b/crates/asap_types/Cargo.toml @@ -16,7 +16,7 @@ xxhash-rust = { version = "0.8", features = ["xxh64"] } # via this crate): WindowType -> asap_ir::intent_algebra::query_expr::WindowKind # unification (scratchpad/artifacts/enum-unification-plan.md). Pin matches # control_plane's -- see control_plane/Cargo.toml's comment for the rationale. -asap-ir = { git = "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/ProjectASAP/ASAPController", rev = "fc09c3aa0b0cf0297ee415d1ed49f7cdc0cc55e4" } +asap-ir = { git = "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/ProjectASAP/ASAPController", rev = "94795092c5d2d9582ddf7486ac0043ceb9f8a34c" } # Step 5 of the sketch-identity unification (see # scratchpad/artifacts/enum-unification-plan.md): `AccumulatorSpec` # (accumulator_spec.rs) converges data_plane's identity representation @@ -25,4 +25,4 @@ asap-ir = { git = "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/ProjectASAP/ASAPController", rev = "fc09c3 # control_plane's pin exactly (`control_plane/Cargo.toml`) -- two # different revs of the same git dependency in one workspace resolve # to two distinct Rust types that won't unify. -asap-sketch = { git = "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/ProjectASAP/ASAPController", rev = "fc09c3aa0b0cf0297ee415d1ed49f7cdc0cc55e4" } +asap-sketch = { git = "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/ProjectASAP/ASAPController", rev = "94795092c5d2d9582ddf7486ac0043ceb9f8a34c" } diff --git a/data_plane/Cargo.toml b/data_plane/Cargo.toml index 02ff4e35..7585b08d 100644 --- a/data_plane/Cargo.toml +++ b/data_plane/Cargo.toml @@ -29,12 +29,12 @@ control_plane = { path = "../control_plane" } # control_plane's own pin) to pick up # `SketchQuery::PointCount.value: Option`, needed for the # named-key PointCount readout. -asap-sketch = { git = "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/ProjectASAP/ASAPController", rev = "64df20d90c3ddd519c726dea45c05e1fe5225ce6" } +asap-sketch = { git = "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/ProjectASAP/ASAPController", rev = "94795092c5d2d9582ddf7486ac0043ceb9f8a34c" } # `asap_sketch::L4Node`'s own fields (`SummaryExpr::Logical(Box)`, -# `SummaryAgg { col: ColumnRef, by: Vec, .. }`) are `asap-ir` +# `SummaryAgg { col: ColumnRef, reduction: Reduction, .. }`) are `asap-ir` # types, not re-exported by `asap-sketch` -- `find_candidates` needs to # walk/match them directly. Same pin-must-match rule as `asap-sketch` above. -asap-ir = { git = "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/ProjectASAP/ASAPController", rev = "64df20d90c3ddd519c726dea45c05e1fe5225ce6" } +asap-ir = { git = "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/ProjectASAP/ASAPController", rev = "94795092c5d2d9582ddf7486ac0043ceb9f8a34c" } # Shared external (workspace) serde.workspace = true From 8b1acfae473857e69861187ba390654d296f39a8 Mon Sep 17 00:00:00 2001 From: zz_y Date: Tue, 28 Jul 2026 16:02:58 -0600 Subject: [PATCH 4/5] fix(summary_executor): resolve grouping from L3's Reduction, not empty-by Replaces the `sketch_group_key()` heuristic (from this branch's previous commit) with the real signal now available upstream: `find_candidates` takes `reduction: &Reduction` instead of `by: &[ColumnId]`. The previous fix only closed HALF the bug. An empty `by: Vec` was genuinely ambiguous -- it could mean either of two OPPOSITE things: 1. "no grouping concept was expressible at all" -- a bare per-series range function like `quantile_over_time(m[r])`, where L3/L4 planning has no reference to any label column, so `by` is empty because the query text gave it nothing to resolve, not because anyone asked to merge. Must keep distinct series distinct. 2. "a genuine cross-series reduction over zero grouping columns" -- `count(hll_metric)`-shaped. Must merge every matching candidate into ONE answer. `sketch_group_key()` could not distinguish these, because by the time it saw a bare `[]` the distinction was already gone -- it just picked behavior (1) for the whole Sketch family and left (2) broken (documented at the time as "a true global-merge aggregate, not yet modeled at all"). That's exactly the ambiguity ASAPController#165 removed at the source, by making the reduction kind explicit on the node instead of inferring it from an empty key list. So the family-specific split (`sketch_group_key` for Sketch, `project_group_key` bare for ExactAgg) collapses into ONE `resolve_group_key()` driven by `Reduction`, applied uniformly to both candidate branches: * `PerEntity` -> the sid's own FULL label map, matching legacy `sketch_reducer.rs::evaluate_core` exactly (it passes `series_label_values` through unconditionally). Case 1, behavior unchanged. * `Reduce(by)` -> project onto `by`. When `by` is empty this naturally yields the SAME `{}` key for every candidate, correctly merging them. Case 2, newly fixed. When non-empty, group as before. `sketch_group_key()` is deleted. New test `genuine_full_reduction_merges_distinct_series_unlike_per_entity` covers case 2 directly: two HLL sids with different `zone` labels and disjoint item sets under `Reduce([])` must merge to ONE group with cardinality ~10, not stay separate at ~5 each. Its sibling `bare_per_series_query_keeps_distinct_series_separate_even_with_no_by` (case 1) is retained and now states which `Reduction` it exercises, so the two tests pin the two halves against each other. shadow_compare.rs's docs updated: the `count(hll_metric)` global-merge shape they flagged as unmodeled is now handled, so the group-set check there is purely a defensive classifier for genuinely unknown shapes. Verified: `cargo check -p data_plane --all-targets` clean; `cargo test -p data_plane --lib` 948 passed / 0 failed (23/23 in summary_executor, including both ambiguity-half tests). Closes the data_plane half of ProjectASAP/ASAPController#163. Refs ProjectASAP/ASAPController#164, #165 Co-Authored-By: Claude Sonnet 5 --- .../asap_query_engine/shadow_compare.rs | 30 ++- .../asap_query_engine/summary_executor.rs | 218 +++++++++++++----- 2 files changed, 172 insertions(+), 76 deletions(-) diff --git a/data_plane/src/query_engines/asap_query_engine/shadow_compare.rs b/data_plane/src/query_engines/asap_query_engine/shadow_compare.rs index cd8cf762..30dfc9ea 100644 --- a/data_plane/src/query_engines/asap_query_engine/shadow_compare.rs +++ b/data_plane/src/query_engines/asap_query_engine/shadow_compare.rs @@ -164,15 +164,21 @@ fn fold_coverage(coverage: &mut Option<(u64, u64)>, next: Option<(u64, u64)>) { /// a real, confirmed gap: a bare per-series range function with no PromQL /// `by(...)` (e.g. `quantile_over_time(m[r])`) got an empty `{}` group key /// from `find_candidates`, losing (and for multiple matching series, -/// silently MERGING) the underlying sid's own labels. That's now fixed at -/// the root in `find_candidates`/`sketch_group_key` (see its doc), which -/// falls back to the sid's own full label map instead of projecting onto -/// an empty `by`. This function's group-set check stays as a defensive +/// silently MERGING) the underlying sid's own labels. +/// +/// Both halves of that ambiguity are now fixed at the root, in +/// `find_candidates`/`resolve_group_key` (see its doc), which reads L3/L4's +/// own `Reduction` (ASAPController#163/#164/#165) instead of inferring +/// intent from an empty `by`: `PerEntity` keeps each sid's own full label +/// map (the per-series-range-function case above), while `Reduce([])` +/// deliberately shares one group key across every candidate -- so a true +/// global-merge aggregate like `count(hll_metric)`, previously called out +/// here as "not modeled at all yet," is now handled correctly too. +/// +/// This function's group-set check therefore stays only as a defensive /// classifier for whatever OTHER shape might still produce a genuine -/// one-row-both-sides mismatch (e.g. a true global-merge aggregate like -/// `count(hll_metric)`, which isn't modeled by `summary_executor.rs` at -/// all yet) -- if it fires now, treat it as a real, unclassified -/// discrepancy worth investigating, not the old known gap. +/// one-row-both-sides mismatch -- if it fires now, treat it as a real, +/// unclassified discrepancy worth investigating, not either known gap. fn diff_and_log( query: &str, old: &ASAPTierResult, @@ -190,10 +196,10 @@ fn diff_and_log( query, old_group = ?old.series[0].0, "shadow mismatch: one row on each side but new path's group key is empty -- \ - the known per-series-range-function gap was fixed in find_candidates/ \ - sketch_group_key, so this shape firing now means an UNCLASSIFIED gap \ - (e.g. a true global-merge aggregate not yet modeled by summary_executor.rs), \ - not the old known one" + both the per-series-range-function gap AND the global-merge-aggregate gap \ + are now fixed in find_candidates/resolve_group_key (driven by L3/L4's \ + Reduction), so this shape firing now means an UNCLASSIFIED gap, \ + not either known one" ); return; } diff --git a/data_plane/src/query_engines/asap_query_engine/summary_executor.rs b/data_plane/src/query_engines/asap_query_engine/summary_executor.rs index 5a75d4fe..9f004c76 100644 --- a/data_plane/src/query_engines/asap_query_engine/summary_executor.rs +++ b/data_plane/src/query_engines/asap_query_engine/summary_executor.rs @@ -63,7 +63,7 @@ use std::collections::{BTreeMap, BTreeSet}; use std::rc::Rc; use std::sync::Arc; -use asap_ir::intent_algebra::{ColumnId, ColumnRef, QueryExpr, Source}; +use asap_ir::intent_algebra::{ColumnId, ColumnRef, QueryExpr, Reduction, Source}; use asap_sketch::exec::SummaryExecutor; use asap_sketch::{L4Node, SketchQuery, SummaryExpr, SummaryKind, SummaryParams}; @@ -303,11 +303,12 @@ impl<'a> SummaryExecutor for QueryExecutionContext<'a> { sketch: &SummaryKind, params: &SummaryParams, _col: &ColumnRef, - by: &[ColumnId], + reduction: &Reduction, child: &L4Node, ) -> Result, Self::Error> { let metric = find_metric(child).ok_or(SummaryExecutorError::NoMetricFound)?; + let by: &[ColumnId] = reduction.group_keys().map(|k| k.keys()).unwrap_or(&[]); let mut by_names: Vec = Vec::with_capacity(by.len()); for &col_id in by { let name = child @@ -363,7 +364,7 @@ impl<'a> SummaryExecutor for QueryExecutionContext<'a> { let Some(series) = series.into_iter().next() else { continue; }; - let group_key = sketch_group_key(&by_names, &series.series_label_values); + let group_key = resolve_group_key(reduction, &by_names, &series.series_label_values); out.push(( group_key, SidHandle::Sketch { @@ -383,7 +384,7 @@ impl<'a> SummaryExecutor for QueryExecutionContext<'a> { let Some((label_values, windows)) = series_list.into_iter().next() else { continue; }; - let group_key = project_group_key(&by_names, &label_values); + let group_key = resolve_group_key(reduction, &by_names, &label_values); out.push(( group_key, SidHandle::ExactAgg { @@ -780,44 +781,50 @@ fn project_group_key( .collect() } -/// Group-key construction for the `Sketch` branch of `find_candidates`. +/// Group-key construction for `find_candidates`, shared by both the +/// `Sketch` and `ExactAgg` branches -- driven directly by L3/L4's own +/// `Reduction` (ASAPController#163/#164/#165), not inferred from whether +/// `by` happens to be empty. /// -/// Deliberately NOT the same as `project_group_key` above -- confirmed by -/// directly inspecting the `L4Node` tree for `quantile_over_time(0.99, -/// http_latency_ms[10s])` (a bare per-series range function, no PromQL -/// `by(...)` or label selector referencing "service" at all): the L3/L4 -/// schema derivation has genuinely NO KNOWLEDGE of the "service" column, -/// since planning happens with "no reference to what's actually stored -/// anywhere" (this crate's own L4 design doc) -- `by` ends up `[]` not as -/// a deliberate "reduce everything" choice, but because there was nothing -/// in the query text to resolve a column against. Naively projecting onto -/// an empty `by` (what `project_group_key` does) would collapse every -/// matching sid's group key to the SAME empty map `{}` -- which doesn't -/// just lose labels, it makes `execute()`'s own `by_group: BTreeMap>` fold MULTIPLE DISTINCT series into ONE merged answer -/// whenever more than one sid/series matches. Confirmed via a real e2e -/// shadow-mode run: the legacy `sketch_reducer.rs::evaluate_core` path -/// never has this problem because it doesn't project through `by` at all -/// for this family -- it passes each series' own `series_label_values` -/// straight through (`ts.series_label_values`, unconditionally), so -/// distinct series always stay distinct rows. +/// This replaces the old family-specific split (`sketch_group_key` vs. +/// `project_group_key` used bare): before `Reduction` existed on +/// `SummaryAgg`, an empty `by: Vec` was genuinely ambiguous -- +/// it could mean either "no explicit grouping was even resolvable" (a +/// bare per-series range function like `quantile_over_time(0.99, +/// http_latency_ms[10s])`, where L3/L4 planning has no reference to any +/// label column at all) or "a real cross-series reduction with zero +/// grouping columns" (`count(hll_metric)`, `sum(...)`-shaped). Those two +/// cases need OPPOSITE group-key behavior and the old `by: &[ColumnId]` +/// signature could not tell them apart -- `sketch_group_key`'s heuristic +/// (treat empty `by` as "keep every series distinct" for the Sketch +/// family only) fixed the first case but could not fix the second, since +/// by the time `find_candidates` saw a bare `[]`, the distinction was +/// already lost. /// -/// So: when `by_names` is empty, use the sid's own FULL label map -/// (matching `evaluate_core`'s behavior exactly -- keep every series -/// distinct); when non-empty, an explicit grouping WAS resolvable from -/// the query (e.g. `quantile by (zone) (...)`), so project onto it as -/// requested, same as `project_group_key`. `ExactAgg`'s `by=[]` keeps its -/// OWN, opposite-looking-but-independently-correct meaning ("reduce -/// fully") -- see `project_group_key`'s doc for why that's not the same -/// question. -fn sketch_group_key( +/// `Reduction` restores it directly: +/// - `PerEntity`: no grouping concept at all -- use the sid's own FULL +/// label map, matching the legacy `sketch_reducer.rs::evaluate_core` +/// path's behavior exactly (it passes `series_label_values` straight +/// through, unconditionally), so distinct series always stay distinct +/// rows. Applies uniformly to both families now (previously +/// `ExactAgg`'s `project_group_key` had no equivalent, since `Sum`/ +/// `Increase`-shaped exact aggregations only ever reach an unqualified +/// PromQL aggregation operator, which is never `PerEntity`). +/// - `Reduce(by)`: a genuine reduction. Project onto `by_names` as +/// before -- when `by_names` is empty this naturally returns the SAME +/// `{}` key for every matching candidate, correctly merging them into +/// one group (the fix for the `count(hll_metric)`-style case the old +/// `by: &[ColumnId]` signature couldn't resolve). When non-empty, an +/// explicit grouping was resolvable from the query (e.g. `quantile by +/// (zone) (...)`), so project onto it as requested. +fn resolve_group_key( + reduction: &Reduction, by_names: &[String], label_values: &BTreeMap, ) -> BTreeMap { - if by_names.is_empty() { - label_values.clone() - } else { - project_group_key(by_names, label_values) + match reduction { + Reduction::PerEntity => label_values.clone(), + Reduction::Reduce(_) => project_group_key(by_names, label_values), } } @@ -970,14 +977,14 @@ mod tests { }) } - fn kll_agg_node(child: Rc, by: Vec) -> Rc { + fn kll_agg_node(child: Rc, reduction: Reduction) -> Rc { Rc::new(L4Node { expr: SummaryExpr::SummaryAgg { child, sketch: SummaryKind::Kll, params: SummaryParams::Kll { k: 200 }, col: ColumnRef::SampleValue, - by, + reduction, }, schema: L4Schema { fields: vec![], @@ -987,13 +994,17 @@ mod tests { } fn hll_agg_node(child: Rc) -> Rc { + hll_agg_node_with(child, Reduction::by(vec![])) + } + + fn hll_agg_node_with(child: Rc, reduction: Reduction) -> Rc { Rc::new(L4Node { expr: SummaryExpr::SummaryAgg { child, sketch: SummaryKind::Hll, params: SummaryParams::Hll { precision: 10 }, col: ColumnRef::SampleValue, - by: vec![], + reduction, }, schema: L4Schema { fields: vec![], @@ -1138,7 +1149,7 @@ mod tests { depth: 4, }, col: ColumnRef::SampleValue, - by: vec![], + reduction: Reduction::by(vec![]), }, schema: L4Schema { fields: vec![], @@ -1195,7 +1206,7 @@ mod tests { heap_size: 10, }, col: ColumnRef::SampleValue, - by: vec![], + reduction: Reduction::by(vec![]), }, schema: L4Schema { fields: vec![], @@ -1235,7 +1246,10 @@ mod tests { sketch: SummaryKind::Sum, params: SummaryParams::Sum, col: ColumnRef::SampleValue, - by, + // Sum is a genuine PromQL aggregation operator -- an empty + // `by` always means "reduce fully," never `PerEntity` (see + // `resolve_group_key`'s doc). + reduction: Reduction::by(by), }, schema: L4Schema { fields: vec![], @@ -1283,7 +1297,7 @@ mod tests { let child = scan_node("latency_ms", None); let tree = estimate_node( - kll_agg_node(child, vec![]), + kll_agg_node(child, Reduction::by(vec![])), SketchQuery::Quantile { q: 0.5 }, ); @@ -1342,7 +1356,7 @@ mod tests { let child = scan_node("latency_ms", None); let tree = estimate_node( - kll_agg_node(child, vec![]), + kll_agg_node(child, Reduction::by(vec![])), SketchQuery::Quantile { q: 0.5 }, ); @@ -1398,7 +1412,7 @@ mod tests { let child = scan_node("latency_ms", Some("zone")); // "zone" is field index 1 in `scan_node`'s schema (0 = value). let tree = estimate_node( - kll_agg_node(child, vec![1]), + kll_agg_node(child, Reduction::by(vec![1])), SketchQuery::Quantile { q: 0.5 }, ); @@ -1437,17 +1451,18 @@ mod tests { #[test] fn bare_per_series_query_keeps_distinct_series_separate_even_with_no_by() { - // The exact bug `sketch_group_key` fixes, confirmed against a real - // e2e shadow-mode run for `quantile_over_time(m[r])` (a bare - // per-series range function -- no PromQL `by(...)`, no label - // selector, so the L4Node's `by` is empty because the query text - // gives it nothing to resolve, NOT because the user asked to merge - // everything). Two sids with DIFFERENT real labels ("zone") but an - // UNGROUPED query (`by: vec![]`, mirroring `kll_agg_node`'s - // no-grouping call shape) must still produce TWO separate output - // series -- naively projecting onto an empty `by` would collapse - // both sids' group keys to the SAME `{}` and silently merge two - // unrelated distributions into one wrong answer. + // The `Reduction::PerEntity` half of the empty-`by` ambiguity + // (ASAPController#163/#164/#165), confirmed against a real e2e + // shadow-mode run for `quantile_over_time(m[r])` (a bare per-series + // range function -- no PromQL `by(...)`, no label selector, so + // there's no grouping concept for the query to express at all, + // NOT a request to merge everything). Two sids with DIFFERENT real + // labels ("zone") under a `PerEntity` query must still produce TWO + // separate output series -- naively projecting onto an empty `by` + // would collapse both sids' group keys to the SAME `{}` and + // silently merge two unrelated distributions into one wrong + // answer. See `genuine_full_reduction_merges_distinct_series_unlike_per_entity` + // for the opposite (`Reduce([])`) case, which MUST merge. let idx = SketchStore::new(); idx.register(kll_meta(1, "latency_ms", &["zone"])); idx.register(kll_meta(2, "latency_ms", &["zone"])); @@ -1480,7 +1495,7 @@ mod tests { // latency_ms[r])` with no `by(...)`/label selector. let child = scan_node("latency_ms", Some("zone")); let tree = estimate_node( - kll_agg_node(child, vec![]), + kll_agg_node(child, Reduction::PerEntity), SketchQuery::Quantile { q: 0.5 }, ); @@ -1552,6 +1567,81 @@ mod tests { ); } + #[test] + fn genuine_full_reduction_merges_distinct_series_unlike_per_entity() { + // The other half of the empty-`by` ambiguity `Reduction` resolves + // (ASAPController#163/#164/#165): a bare per-series range function + // like `quantile_over_time(m[r])` (Reduction::PerEntity, see + // `bare_per_series_query_keeps_distinct_series_separate_even_with_no_by` + // above) must NOT merge distinct series, but a genuine + // cross-series reduction with zero grouping columns -- + // `count(hll_metric)`-shaped, Reduction::Reduce(GroupKeys::by([])) + // -- legitimately MUST merge them into one combined answer. Before + // `find_candidates` took `&Reduction` instead of `&[ColumnId]`, + // these two cases were indistinguishable from an empty `by` alone + // (the old `sketch_group_key` heuristic could only ever pick ONE + // of the two behaviors for every empty-`by` query). + // + // Two HLL sids with DIFFERENT real "zone" labels (same shape as + // the PerEntity test above) and DISJOINT item sets: under a + // genuine `Reduce([])`, they must merge into ONE group whose + // cardinality reflects BOTH sids' items combined (~10), not two + // separate ~5-item answers. + let idx = SketchStore::new(); + idx.register(hll_meta(1, "unique_users")); + idx.register(hll_meta(2, "unique_users")); + let items1: Vec<&str> = vec!["a", "b", "c", "d", "e"]; + let items2: Vec<&str> = vec!["f", "g", "h", "i", "j"]; + let mut labels_east = BTreeMap::new(); + labels_east.insert("zone".to_string(), "us-east".to_string()); + let mut labels_west = BTreeMap::new(); + labels_west.insert("zone".to_string(), "us-west".to_string()); + idx.append_sample( + 1, + labels_east, + (T0, T0 + 1000), + SketchSampleState { + bytes: encode_hll_from_items(10, &items1), + encoding: crate::storage_engines::sketch_db::index::SketchEncoding::MsgpackFull, + }, + ); + idx.append_sample( + 2, + labels_west, + (T0, T0 + 1000), + SketchSampleState { + bytes: encode_hll_from_items(10, &items2), + encoding: crate::storage_engines::sketch_db::index::SketchEncoding::MsgpackFull, + }, + ); + + let child = scan_node("unique_users", Some("zone")); + let tree = estimate_node( + hll_agg_node_with(child, Reduction::by(vec![])), + SketchQuery::Cardinality, + ); + + let exec = ctx(&idx); + let ExecOutcome::Value(v) = execute(&tree, &exec).expect("execute should succeed") else { + panic!("expected a value"); + }; + assert_eq!( + v.len(), + 1, + "a genuine full reduction must merge both sids into one group, got {v:?}" + ); + let (_group, value) = &v[0]; + let SummaryValue::Points(samples, _coverage) = value else { + panic!("expected Points, got {value:?}"); + }; + let (_ts, card) = samples[0]; + assert!( + (8.0..=12.0).contains(&card), + "merged cardinality {card} should be ~10 (both sids' disjoint item sets combined), \ + not ~5 (one sid dropped or kept separate)" + ); + } + #[test] fn single_cms_sid_total_readout() { let idx = SketchStore::new(); @@ -1910,7 +2000,7 @@ mod tests { let idx = SketchStore::new(); let child = scan_node("nonexistent_metric", None); let tree = estimate_node( - kll_agg_node(child, vec![]), + kll_agg_node(child, Reduction::by(vec![])), SketchQuery::Quantile { q: 0.5 }, ); let exec = ctx(&idx); @@ -1942,7 +2032,7 @@ mod tests { sketch: SummaryKind::Kll, params: SummaryParams::Kll { k: 500 }, col: ColumnRef::SampleValue, - by: vec![], + reduction: Reduction::by(vec![]), }, schema: L4Schema { fields: vec![], @@ -1990,7 +2080,7 @@ mod tests { let child = scan_node("latency_ms", None); let tree = estimate_node( - kll_agg_node(child, vec![]), + kll_agg_node(child, Reduction::by(vec![])), SketchQuery::Quantile { q: 0.5 }, ); @@ -2062,7 +2152,7 @@ mod tests { let child = scan_node("latency_ms", None); let tree = estimate_node( - kll_agg_node(child, vec![]), + kll_agg_node(child, Reduction::by(vec![])), SketchQuery::Quantile { q: 0.5 }, ); @@ -2118,7 +2208,7 @@ mod tests { let child = scan_node("latency_ms", None); let tree = estimate_node( - kll_agg_node(child, vec![]), + kll_agg_node(child, Reduction::by(vec![])), SketchQuery::Quantile { q: 0.5 }, ); @@ -2336,7 +2426,7 @@ mod tests { sketch: SummaryKind::MinMax, params: SummaryParams::MinMax, col: ColumnRef::SampleValue, - by: vec![], + reduction: Reduction::by(vec![]), }, schema: L4Schema { fields: vec![], From 1625244cb7f4f71914a5a2149716f6c9b6e6d5a6 Mon Sep 17 00:00:00 2001 From: zz_y Date: Tue, 28 Jul 2026 19:03:58 -0600 Subject: [PATCH 5/5] chore: bump ASAPController pin to cc18c98 (Summary variant merge, #170) Bumps asap-ir/asap-l2/asap-sketch/asap-plan from 94795092 to cc18c987 (ASAPController main HEAD), picking up ProjectASAP/ASAPController#170: 1. `SummaryExpr`'s `sketch`/`sketch_input` fields renamed to `summary`/`summary_input` on `SummaryAgg`, `SummaryJoin`, `SummaryDelete`, `SummaryEstimate`. 2. `asap_plan::boundary::Implementation::Sketch{kind,params}` / `::ExactAccumulator{kind,params}` collapsed into one `Implementation::Summary{kind,params}`, with a new `SummaryKind::is_exact()` so callers recover which case they're in from `kind` alone instead of the variant tag. Fallout, split by kind: * Pure field rename (mechanical): `control_plane/src/emit/mod.rs`, `control_plane/src/physical/colored_dag/{allocator,emitter,tests}.rs`, `control_plane/src/sketch_algebra/{physical_expr,tests}.rs`, `data_plane/src/query_engines/asap_query_engine/summary_executor.rs` (mostly test-helper `SummaryAgg{sketch: ..}` literal construction). * Real logic re-expressed against the merged variant, same behavior: - `control_plane/src/sketch_algebra/capability.rs`'s `implementation_to_capability`: was two separate match arms on the variant tag (`Sketch{kind,..}` -> approx-family capabilities, `ExactAccumulator{kind,..}` -> `ExactAgg(AggregationType)`, with a deliberate `SummaryKind::Count => None` override). Now `Summary{kind,..} if kind.is_exact()` / `Summary{kind,..}` in that order, same per-kind logic in each arm, `Count => None` override preserved verbatim. - `control_plane/src/sketch_algebra/matcher.rs`'s `SummaryFamilyMatcher::is_satisfied_by`: was `(ExactAccumulator, ExactAccumulator) => kind equality` / `(Sketch, Sketch) => sketch_family_satisfied` / `_ => false` (the `_` arm covering a variant-tag mismatch, e.g. Sketch vs ExactAccumulator, among other cases). Now both sides guarded on `kind.is_exact()` agreeing -- `(Summary, Summary) if both.is_exact()` / `(Summary, Summary) if !both.is_exact()` / `_ => false`, so a mismatched-exactness pair still falls through to `false` exactly as a mismatched-variant pair did before. The asymmetric heap-topk-also-answers-bare-frequency exception inside `sketch_family_satisfied` itself is untouched. - `control_plane/src/sketch_algebra/cost_model.rs`'s `realize_extension`: constructs `Implementation::Summary` in place of `Implementation::Sketch` (Cms is always approximate, no exactness ambiguity at this call site). Verified: `cargo check -p control_plane --all-targets` and `cargo check -p data_plane --all-targets` both clean. `cargo test -p control_plane --lib`: 766 passed / 1 failed (optimizer::rules::tests::invalid_sketch_type_override_falls_back_to_default, the same pre-existing, unrelated failure flagged on this branch's previous commit -- unaffected by this change). `cargo test -p data_plane --lib`: 948 passed / 0 failed. Refs ProjectASAP/ASAPController#170 Co-Authored-By: Claude Sonnet 5 --- Cargo.lock | 8 +- control_plane/Cargo.toml | 8 +- control_plane/src/emit/mod.rs | 6 +- .../src/physical/colored_dag/allocator.rs | 8 +- .../src/physical/colored_dag/emitter.rs | 6 +- .../src/physical/colored_dag/tests.rs | 8 +- .../src/sketch_algebra/capability.rs | 73 +++++------ .../src/sketch_algebra/cost_model.rs | 2 +- control_plane/src/sketch_algebra/matcher.rs | 22 ++-- .../src/sketch_algebra/physical_expr.rs | 8 +- control_plane/src/sketch_algebra/tests.rs | 114 +++++++++--------- crates/asap_types/Cargo.toml | 4 +- data_plane/Cargo.toml | 4 +- .../asap_query_engine/summary_executor.rs | 20 +-- 14 files changed, 151 insertions(+), 140 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index df55c306..d4ecdd64 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -343,7 +343,7 @@ dependencies = [ [[package]] name = "asap-ir" version = "0.1.0" -source = "git+https://github.com/ProjectASAP/ASAPController?rev=94795092c5d2d9582ddf7486ac0043ceb9f8a34c#94795092c5d2d9582ddf7486ac0043ceb9f8a34c" +source = "git+https://github.com/ProjectASAP/ASAPController?rev=cc18c9872bbaf0cadf43566892c3974ec9948eba#cc18c9872bbaf0cadf43566892c3974ec9948eba" dependencies = [ "serde", "serde_json", @@ -353,7 +353,7 @@ dependencies = [ [[package]] name = "asap-l2" version = "0.1.0" -source = "git+https://github.com/ProjectASAP/ASAPController?rev=94795092c5d2d9582ddf7486ac0043ceb9f8a34c#94795092c5d2d9582ddf7486ac0043ceb9f8a34c" +source = "git+https://github.com/ProjectASAP/ASAPController?rev=cc18c9872bbaf0cadf43566892c3974ec9948eba#cc18c9872bbaf0cadf43566892c3974ec9948eba" dependencies = [ "asap-ir", "thiserror 2.0.18", @@ -362,7 +362,7 @@ dependencies = [ [[package]] name = "asap-plan" version = "0.1.0" -source = "git+https://github.com/ProjectASAP/ASAPController?rev=94795092c5d2d9582ddf7486ac0043ceb9f8a34c#94795092c5d2d9582ddf7486ac0043ceb9f8a34c" +source = "git+https://github.com/ProjectASAP/ASAPController?rev=cc18c9872bbaf0cadf43566892c3974ec9948eba#cc18c9872bbaf0cadf43566892c3974ec9948eba" dependencies = [ "asap-ir", "asap-sketch", @@ -383,7 +383,7 @@ dependencies = [ [[package]] name = "asap-sketch" version = "0.1.0" -source = "git+https://github.com/ProjectASAP/ASAPController?rev=94795092c5d2d9582ddf7486ac0043ceb9f8a34c#94795092c5d2d9582ddf7486ac0043ceb9f8a34c" +source = "git+https://github.com/ProjectASAP/ASAPController?rev=cc18c9872bbaf0cadf43566892c3974ec9948eba#cc18c9872bbaf0cadf43566892c3974ec9948eba" dependencies = [ "asap-ir", ] diff --git a/control_plane/Cargo.toml b/control_plane/Cargo.toml index d4a17415..62e9dbc9 100644 --- a/control_plane/Cargo.toml +++ b/control_plane/Cargo.toml @@ -68,10 +68,10 @@ asap_types.workspace = true # `AggIntent::Extension` hook this repo's `Extension{"frequency"}` intent # needs (ASAPController#150). 64df20d is a strict descendant of d4c1756 # (the previous pin), so nothing this repo already consumes moves. -asap-ir = { git = "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/ProjectASAP/ASAPController", rev = "94795092c5d2d9582ddf7486ac0043ceb9f8a34c" } -asap-l2 = { git = "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/ProjectASAP/ASAPController", rev = "94795092c5d2d9582ddf7486ac0043ceb9f8a34c" } -asap-sketch = { git = "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/ProjectASAP/ASAPController", rev = "94795092c5d2d9582ddf7486ac0043ceb9f8a34c" } -asap-plan = { git = "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/ProjectASAP/ASAPController", rev = "94795092c5d2d9582ddf7486ac0043ceb9f8a34c" } +asap-ir = { git = "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/ProjectASAP/ASAPController", rev = "cc18c9872bbaf0cadf43566892c3974ec9948eba" } +asap-l2 = { git = "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/ProjectASAP/ASAPController", rev = "cc18c9872bbaf0cadf43566892c3974ec9948eba" } +asap-sketch = { git = "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/ProjectASAP/ASAPController", rev = "cc18c9872bbaf0cadf43566892c3974ec9948eba" } +asap-plan = { git = "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/ProjectASAP/ASAPController", rev = "cc18c9872bbaf0cadf43566892c3974ec9948eba" } [dev-dependencies] tokio = { version = "1", features = ["full", "test-util"] } diff --git a/control_plane/src/emit/mod.rs b/control_plane/src/emit/mod.rs index 4197b97d..0b86bab9 100644 --- a/control_plane/src/emit/mod.rs +++ b/control_plane/src/emit/mod.rs @@ -328,14 +328,14 @@ fn is_exact_accumulator(kind: &SummaryKind) -> bool { fn extract_from_node(node: &Rc) -> Option { match &node.expr { - SummaryExpr::SummaryAgg { sketch, .. } if !is_exact_accumulator(sketch) => { - Some(sketch.clone()) + SummaryExpr::SummaryAgg { summary, .. } if !is_exact_accumulator(summary) => { + Some(summary.clone()) } // An exact accumulator has no sketch family beneath it (its own // child is always a plain `Logical` leaf) — same as the old // `ExactAgg` case. SummaryExpr::SummaryAgg { .. } => None, - SummaryExpr::SummaryEstimate { sketch_input, .. } => extract_from_node(sketch_input), + SummaryExpr::SummaryEstimate { summary_input, .. } => extract_from_node(summary_input), SummaryExpr::SummaryMerge { children } => children.iter().find_map(extract_from_node), // Not surfaced by any `Bind*` path yet (gated on rules that // haven't landed — see `physical_expr.rs`'s module docs). diff --git a/control_plane/src/physical/colored_dag/allocator.rs b/control_plane/src/physical/colored_dag/allocator.rs index 72f9d133..3eb6c1db 100644 --- a/control_plane/src/physical/colored_dag/allocator.rs +++ b/control_plane/src/physical/colored_dag/allocator.rs @@ -193,8 +193,8 @@ impl ThreeStageWalker { // The "SketchEstimate MUST be on the same stage as its // consumers (typically Backend)" invariant is satisfied // because consumers above SummaryEstimate are also backend. - SummaryExpr::SummaryEstimate { sketch_input, .. } => { - let (cid, child_stage) = self.visit_l4node(sketch_input)?; + SummaryExpr::SummaryEstimate { summary_input, .. } => { + let (cid, child_stage) = self.visit_l4node(summary_input)?; self.dag.edges.push((id, cid)); // If child is on edge or gateway, this is a cross-stage // edge — that's expected (the wire-format hop). @@ -232,8 +232,8 @@ impl ThreeStageWalker { self.dag.edges.push((id, rid)); StageId::Gateway } - SummaryExpr::SummaryDelete { sketch_input, .. } => { - let (cid, _) = self.visit_l4node(sketch_input)?; + SummaryExpr::SummaryDelete { summary_input, .. } => { + let (cid, _) = self.visit_l4node(summary_input)?; self.dag.edges.push((id, cid)); StageId::Gateway } diff --git a/control_plane/src/physical/colored_dag/emitter.rs b/control_plane/src/physical/colored_dag/emitter.rs index dcd71a4a..60eb02d8 100644 --- a/control_plane/src/physical/colored_dag/emitter.rs +++ b/control_plane/src/physical/colored_dag/emitter.rs @@ -92,12 +92,12 @@ fn classify(expr: &PhysicalExpr) -> NodeKind<'_> { match expr { PhysicalExpr::Committed(L4Plan::Summary(node)) => match &node.expr { SummaryExpr::Logical(qe) => NodeKind::Logical(qe), - SummaryExpr::SummaryAgg { sketch, params, .. } if is_exact_accumulator(sketch) => { + SummaryExpr::SummaryAgg { summary, params, .. } if is_exact_accumulator(summary) => { let _ = params; NodeKind::ExactAgg } - SummaryExpr::SummaryAgg { sketch, params, .. } => NodeKind::SketchAgg { - sketch_type: sketch, + SummaryExpr::SummaryAgg { summary, params, .. } => NodeKind::SketchAgg { + sketch_type: summary, params, }, SummaryExpr::SummaryEstimate { query, .. } => NodeKind::SketchEstimate { query }, diff --git a/control_plane/src/physical/colored_dag/tests.rs b/control_plane/src/physical/colored_dag/tests.rs index 77a5e0fd..a73b0c69 100644 --- a/control_plane/src/physical/colored_dag/tests.rs +++ b/control_plane/src/physical/colored_dag/tests.rs @@ -104,11 +104,11 @@ fn logical_l4(qe: QueryExpr) -> Rc { /// `PhysicalExpr::SketchAgg { sketch_type, params, child }` construction, /// for fixtures that need a specific family without going through /// `implement_tree`'s cost-model selection. -fn sketch_agg_l4(sketch: SummaryKind, params: SummaryParams, child: Rc) -> Rc { +fn sketch_agg_l4(summary: SummaryKind, params: SummaryParams, child: Rc) -> Rc { Rc::new(L4Node { expr: SummaryExpr::SummaryAgg { child, - sketch, + summary, params, col: ColumnRef::SampleValue, reduction: Reduction::by(vec![]), @@ -119,9 +119,9 @@ fn sketch_agg_l4(sketch: SummaryKind, params: SummaryParams, child: Rc) /// Hand-build a `SummaryEstimate` node — mirrors the old /// `PhysicalExpr::SketchEstimate { op, child }`. -fn estimate_l4(query: SketchQuery, sketch_input: Rc) -> Rc { +fn estimate_l4(query: SketchQuery, summary_input: Rc) -> Rc { Rc::new(L4Node { - expr: SummaryExpr::SummaryEstimate { sketch_input, query }, + expr: SummaryExpr::SummaryEstimate { summary_input, query }, schema: dummy_l4_schema(), }) } diff --git a/control_plane/src/sketch_algebra/capability.rs b/control_plane/src/sketch_algebra/capability.rs index 9fe06fd4..912e8631 100644 --- a/control_plane/src/sketch_algebra/capability.rs +++ b/control_plane/src/sketch_algebra/capability.rs @@ -614,8 +614,9 @@ pub fn capability_for(intent: &AggIntent) -> Option { /// Translate `asap-plan`'s per-intent implementation decision into this /// repo's own [`Capability`] vocabulary. /// -/// `Implementation::Sketch`/`ExactAccumulator` both carry an -/// `asap_sketch::SummaryKind` — this repo's `Capability` groups those +/// `Implementation::Summary` carries an `asap_sketch::SummaryKind` for +/// both the approximate-sketch and exact-accumulator cases (told apart +/// via `kind.is_exact()`) — this repo's `Capability` groups those /// into coarser families (`QuantileApprox`/`CardinalityApprox`/ /// `FrequencyEstimate`/`FrequencyTopk` for sketches; `ExactAgg(AggregationType)` /// for accumulators) because that's the granularity the sketch index @@ -630,31 +631,11 @@ fn implementation_to_capability(implementation: asap_plan::Implementation) -> Op match implementation { Implementation::PassThrough => None, - Implementation::Sketch { kind, .. } => match kind { - SummaryKind::Kll | SummaryKind::DDSketch => { - Some(Capability::QuantileApprox(SketchKindHandle::Any)) - } - SummaryKind::Hll | SummaryKind::Theta | SummaryKind::Kmv => { - Some(Capability::CardinalityApprox) - } - SummaryKind::Cms | SummaryKind::CountSketch => { - Some(Capability::FrequencyEstimate(SketchKindHandle::Any)) - } - SummaryKind::CmsWithHeap | SummaryKind::CountSketchWithHeap => { - Some(Capability::FrequencyTopk(SketchKindHandle::Any)) - } - SummaryKind::Sum - | SummaryKind::Count - | SummaryKind::MinMax - | SummaryKind::Increase - | SummaryKind::Rate => { - unreachable!( - "{kind:?} is an exact-accumulator SummaryKind, never returned inside \ - Implementation::Sketch by asap_plan::boundary::implementation_for" - ) - } - }, - Implementation::ExactAccumulator { kind, .. } => match kind { + // Exact accumulator half of the merged `Summary` variant + // (ASAPController#170 collapsed `Sketch`/`ExactAccumulator` into + // one `Summary { kind, params }`, recoverable via `kind.is_exact()` + // — see `asap_sketch::SummaryKind::is_exact`'s doc). + Implementation::Summary { kind, .. } if kind.is_exact() => match kind { SummaryKind::Sum => Some(Capability::ExactAgg(AggregationType::Sum)), SummaryKind::MinMax => Some(Capability::ExactAgg(AggregationType::MinMax)), SummaryKind::Increase | SummaryKind::Rate => { @@ -667,11 +648,10 @@ fn implementation_to_capability(implementation: asap_plan::Implementation) -> Op // `count_over_time` query matched against a `Sum` policy // would silently return sum-of-values, not sample-count). // `asap_plan::boundary::implementation_for` still reports - // `Count{Exact}` as an `ExactAccumulator` (it assumes a real - // count accumulator exists, which is true in ASAPController's - // own reference implementation) — deliberately overridden - // here to `None` (archive) until a real - // `SumCountAccumulator` lands. + // `Count{Exact}` as exact (it assumes a real count + // accumulator exists, which is true in ASAPController's own + // reference implementation) — deliberately overridden here to + // `None` (archive) until a real `SumCountAccumulator` lands. SummaryKind::Count => None, SummaryKind::Kll | SummaryKind::DDSketch @@ -683,8 +663,33 @@ fn implementation_to_capability(implementation: asap_plan::Implementation) -> Op | SummaryKind::CountSketch | SummaryKind::CountSketchWithHeap => { unreachable!( - "{kind:?} is a sketch-family SummaryKind, never returned inside \ - Implementation::ExactAccumulator by asap_plan::boundary::implementation_for" + "{kind:?} is a sketch-family SummaryKind, so kind.is_exact() is false — \ + never reached inside the is_exact() guard" + ) + } + }, + // Approximate-sketch half. + Implementation::Summary { kind, .. } => match kind { + SummaryKind::Kll | SummaryKind::DDSketch => { + Some(Capability::QuantileApprox(SketchKindHandle::Any)) + } + SummaryKind::Hll | SummaryKind::Theta | SummaryKind::Kmv => { + Some(Capability::CardinalityApprox) + } + SummaryKind::Cms | SummaryKind::CountSketch => { + Some(Capability::FrequencyEstimate(SketchKindHandle::Any)) + } + SummaryKind::CmsWithHeap | SummaryKind::CountSketchWithHeap => { + Some(Capability::FrequencyTopk(SketchKindHandle::Any)) + } + SummaryKind::Sum + | SummaryKind::Count + | SummaryKind::MinMax + | SummaryKind::Increase + | SummaryKind::Rate => { + unreachable!( + "{kind:?} is an exact-accumulator SummaryKind, so kind.is_exact() is \ + true — the arm above already handles it" ) } }, diff --git a/control_plane/src/sketch_algebra/cost_model.rs b/control_plane/src/sketch_algebra/cost_model.rs index 80e584ef..0bf18863 100644 --- a/control_plane/src/sketch_algebra/cost_model.rs +++ b/control_plane/src/sketch_algebra/cost_model.rs @@ -277,7 +277,7 @@ impl CostModel for ControlPlaneCostModel { return Implementation::PassThrough; }; let (width, depth) = Self::cms_width_depth(eps, delta); - Implementation::Sketch { + Implementation::Summary { kind: SummaryKind::Cms, params: SummaryParams::Cms { width: width.next_power_of_two(), diff --git a/control_plane/src/sketch_algebra/matcher.rs b/control_plane/src/sketch_algebra/matcher.rs index 9463e624..4f9d4c2e 100644 --- a/control_plane/src/sketch_algebra/matcher.rs +++ b/control_plane/src/sketch_algebra/matcher.rs @@ -54,16 +54,22 @@ impl Matcher for SummaryFamilyMatcher { /// but not the reverse (a heap-less sketch cannot enumerate top-k /// items it never tracked). fn is_satisfied_by(&self, required: &Implementation, available: &Implementation) -> bool { + // ASAPController#170 merged `Sketch`/`ExactAccumulator` into one + // `Summary { kind, params }` variant, recoverable via + // `kind.is_exact()`. The variant-tag mismatch that used to fall + // through to `_ => false` (comparing a `Sketch` against an + // `ExactAccumulator`) is now an explicit `is_exact()` mismatch + // between the two sides, still falling through the same way. match (required, available) { (Implementation::PassThrough, _) => true, ( - Implementation::ExactAccumulator { kind: required, .. }, - Implementation::ExactAccumulator { kind: have, .. }, - ) => required == have, + Implementation::Summary { kind: required, .. }, + Implementation::Summary { kind: have, .. }, + ) if required.is_exact() && have.is_exact() => required == have, ( - Implementation::Sketch { kind: required, .. }, - Implementation::Sketch { kind: have, .. }, - ) => sketch_family_satisfied(required, have), + Implementation::Summary { kind: required, .. }, + Implementation::Summary { kind: have, .. }, + ) if !required.is_exact() && !have.is_exact() => sketch_family_satisfied(required, have), _ => false, } } @@ -174,12 +180,12 @@ mod tests { fn sketch(kind: SummaryKind) -> Implementation { let params = params_for(&kind); - Implementation::Sketch { kind, params } + Implementation::Summary { kind, params } } fn accumulator(kind: SummaryKind) -> Implementation { let params = params_for(&kind); - Implementation::ExactAccumulator { kind, params } + Implementation::Summary { kind, params } } #[test] diff --git a/control_plane/src/sketch_algebra/physical_expr.rs b/control_plane/src/sketch_algebra/physical_expr.rs index 41fa8a72..23749351 100644 --- a/control_plane/src/sketch_algebra/physical_expr.rs +++ b/control_plane/src/sketch_algebra/physical_expr.rs @@ -214,13 +214,13 @@ mod tests { let e = PhysicalExpr::committed(node); match e { PhysicalExpr::Committed(L4Plan::Summary(node)) => match &node.expr { - asap_sketch::SummaryExpr::SummaryEstimate { query, sketch_input } => { + asap_sketch::SummaryExpr::SummaryEstimate { query, summary_input } => { assert!(matches!(query, asap_sketch::SketchQuery::Quantile { q } if *q == 0.99)); - match &sketch_input.expr { + match &summary_input.expr { asap_sketch::SummaryExpr::SummaryAgg { - sketch, params, child, .. + summary, params, child, .. } => { - assert_eq!(sketch, &SummaryKind::Kll); + assert_eq!(summary, &SummaryKind::Kll); assert_eq!(params, &SummaryParams::Kll { k: 200 }); assert!(matches!(child.expr, asap_sketch::SummaryExpr::Logical(_))); } diff --git a/control_plane/src/sketch_algebra/tests.rs b/control_plane/src/sketch_algebra/tests.rs index a03490ea..4b060b30 100644 --- a/control_plane/src/sketch_algebra/tests.rs +++ b/control_plane/src/sketch_algebra/tests.rs @@ -109,7 +109,7 @@ fn node_is_archive(node: &Rc) -> bool { _ => false, }, SummaryExpr::SummaryAgg { child, .. } => node_is_archive(child), - SummaryExpr::SummaryEstimate { sketch_input, .. } => node_is_archive(sketch_input), + SummaryExpr::SummaryEstimate { summary_input, .. } => node_is_archive(summary_input), SummaryExpr::SummaryMerge { children } => children.iter().any(node_is_archive), SummaryExpr::SummaryJoin { outer, inner, .. } => { node_is_archive(outer) || node_is_archive(inner) @@ -117,7 +117,7 @@ fn node_is_archive(node: &Rc) -> bool { SummaryExpr::SummarySubtract { left, right } => { node_is_archive(left) || node_is_archive(right) } - SummaryExpr::SummaryDelete { sketch_input, .. } => node_is_archive(sketch_input), + SummaryExpr::SummaryDelete { summary_input, .. } => node_is_archive(summary_input), } } @@ -138,17 +138,17 @@ fn bind_kll_quantile_basic() { match &node.expr { SummaryExpr::SummaryEstimate { query, - sketch_input, + summary_input, } => { assert!(matches!(query, SketchQuery::Quantile { q } if *q == 0.99)); - match &sketch_input.expr { + match &summary_input.expr { SummaryExpr::SummaryAgg { - sketch, + summary, params, child, .. } => { - assert_eq!(sketch, &SummaryKind::Kll); + assert_eq!(summary, &SummaryKind::Kll); assert_eq!(params, &SummaryParams::Kll { k: 200 }); assert!(matches!(child.expr, SummaryExpr::Logical(_))); } @@ -172,12 +172,12 @@ fn bind_ddsketch_quantile_basic() { match &node.expr { SummaryExpr::SummaryEstimate { query, - sketch_input, + summary_input, } => { assert!(matches!(query, SketchQuery::Quantile { q } if *q == 0.99)); - match &sketch_input.expr { - SummaryExpr::SummaryAgg { sketch, params, .. } => { - assert_eq!(sketch, &SummaryKind::DDSketch); + match &summary_input.expr { + SummaryExpr::SummaryAgg { summary, params, .. } => { + assert_eq!(summary, &SummaryKind::DDSketch); match params { SummaryParams::DDSketch { alpha } => { assert!((alpha - 0.01).abs() < 1e-12) @@ -203,10 +203,10 @@ fn bind_picks_ddsketch_over_kll_when_eps_explicit() { .expect("bind_query_expr should not error"); match bound { PhysicalExpr::Committed(L4Plan::Summary(node)) => match &node.expr { - SummaryExpr::SummaryEstimate { sketch_input, .. } => match &sketch_input.expr { - SummaryExpr::SummaryAgg { sketch, .. } => { + SummaryExpr::SummaryEstimate { summary_input, .. } => match &summary_input.expr { + SummaryExpr::SummaryAgg { summary, .. } => { assert_eq!( - sketch, + summary, &SummaryKind::DDSketch, "dispatcher should pick DDSketch over KLL on ε-driven Quantile" ); @@ -242,16 +242,16 @@ fn topk_binding_family(bound: &PhysicalExpr) -> (SummaryKind, u32, u32) { PhysicalExpr::Committed(L4Plan::Summary(node)) => match &node.expr { SummaryExpr::SummaryEstimate { query, - sketch_input, + summary_input, } => { assert!(matches!(query, SketchQuery::TopK { k } if *k == 10)); - match &sketch_input.expr { - SummaryExpr::SummaryAgg { sketch, params, .. } => match params { + match &summary_input.expr { + SummaryExpr::SummaryAgg { summary, params, .. } => match params { SummaryParams::CmsWithHeap { width, depth, .. } => { - (sketch.clone(), *width, *depth) + (summary.clone(), *width, *depth) } SummaryParams::CountSketchWithHeap { width, depth, .. } => { - (sketch.clone(), *width, *depth) + (summary.clone(), *width, *depth) } other => { panic!("expected CmsWithHeap/CountSketchWithHeap params, got {other:?}") @@ -295,7 +295,7 @@ fn bind_cms_topk_loose_recall_picks_cms_heap() { /// the old fixture used `AggIntent::TopK{accuracy: Exact}` (the intent's /// OWN accuracy) to signal "tight/exact-recall". Under /// `asap_plan::boundary::implementation_for_with`, the per-intent -/// sketch-vs-exact boundary decision checks the intent's own `accuracy` +/// summary-vs-exact boundary decision checks the intent's own `accuracy` /// field FIRST: `TopK{accuracy: Exact}` now declines to bind at all /// (`SummaryExpr::Logical`) rather than reaching the cost model's /// family-selection logic at all — see `topk_exact_accuracy_declines_to_bind` @@ -374,12 +374,12 @@ fn bind_hll_cardinality_basic() { PhysicalExpr::Committed(L4Plan::Summary(node)) => match &node.expr { SummaryExpr::SummaryEstimate { query, - sketch_input, + summary_input, } => { assert!(matches!(query, SketchQuery::Cardinality)); - match &sketch_input.expr { - SummaryExpr::SummaryAgg { sketch, params, .. } => { - assert_eq!(sketch, &SummaryKind::Hll); + match &summary_input.expr { + SummaryExpr::SummaryAgg { summary, params, .. } => { + assert_eq!(summary, &SummaryKind::Hll); match params { SummaryParams::Hll { precision } => { assert!( @@ -401,7 +401,7 @@ fn bind_hll_cardinality_basic() { #[test] fn sum_now_binds_to_exact_agg_after_pr_6_followup() { - // `AggIntent::Sum` binds to a bare `SummaryAgg` with `sketch: + // `AggIntent::Sum` binds to a bare `SummaryAgg` with `summary: // SummaryKind::Sum` and no `SummaryEstimate` wrapper (the partial // state *is* the value — see `asap_plan::bind`'s module docs). The // old locally-defined `PhysicalExpr::ExactAgg { agg_type, .. }` @@ -420,9 +420,9 @@ fn sum_now_binds_to_exact_agg_after_pr_6_followup() { let bound = bind_query_expr(&expr, AccuracyTarget::Exact).expect("no error"); match bound { PhysicalExpr::Committed(L4Plan::Summary(node)) => match &node.expr { - SummaryExpr::SummaryAgg { sketch, params, .. } => { + SummaryExpr::SummaryAgg { summary, params, .. } => { assert_eq!( - sketch, + summary, &SummaryKind::Sum, "Sum should bind to SummaryAgg(Sum)" ); @@ -438,14 +438,14 @@ fn sum_now_binds_to_exact_agg_after_pr_6_followup() { fn bind_exact_accuracy_disables_quantile_binding() { // Quantile under `AccuracyTarget::Exact` should NOT bind — the // optimizer falls back to an exact path. (Per design.md §6 line - // ~1254 — "the sketch path is selected, not mandated".) + // ~1254 — "the summary path is selected, not mandated".) let expr = agg_quantile(0.99, AccuracyTarget::Exact); let bound = bind_query_expr(&expr, AccuracyTarget::Exact).expect("no error"); match bound { PhysicalExpr::Committed(L4Plan::Summary(node)) => { assert!( matches!(&node.expr, SummaryExpr::Logical(qe) if matches!(**qe, QueryExpr::Aggregate { .. })), - "Exact accuracy should disable sketch binding and pass through as Logical, got {:?}", + "Exact accuracy should disable summary binding and pass through as Logical, got {:?}", node.expr ); } @@ -461,7 +461,7 @@ fn bind_exact_accuracy_disables_quantile_binding() { // matching binding without going back through asap-planner-rs. /// `ONLY_TEMPORAL` — `quantile_over_time(0.99, m[5m])`. -/// asap-planner-rs path: ONLY_TEMPORAL pattern 1 → KLL/DDSketch sketch. +/// asap-planner-rs path: ONLY_TEMPORAL pattern 1 → KLL/DDSketch summary. /// Control plane path: `Aggregate{Quantile{0.99}}` over `Window` → /// binds a quantile-capable family → `SummaryAgg{KLL/DDSketch}`. #[test] @@ -482,12 +482,12 @@ fn phase_b_pattern_only_temporal_quantile_binds_to_sketch() { PhysicalExpr::Committed(L4Plan::Summary(node)) => match &node.expr { SummaryExpr::SummaryEstimate { query, - sketch_input, + summary_input, } => { assert!(matches!(query, SketchQuery::Quantile { .. })); - match &sketch_input.expr { - SummaryExpr::SummaryAgg { sketch, .. } => { - assert!(matches!(sketch, SummaryKind::Kll | SummaryKind::DDSketch)); + match &summary_input.expr { + SummaryExpr::SummaryAgg { summary, .. } => { + assert!(matches!(summary, SummaryKind::Kll | SummaryKind::DDSketch)); } other => panic!("expected SummaryAgg under SummaryEstimate, got {other:?}"), } @@ -502,7 +502,7 @@ fn phase_b_pattern_only_temporal_quantile_binds_to_sketch() { /// variants that legacy `single_query.rs` accepts). /// /// Control plane path: `Aggregate{Sum}` over `Window` → binds to a bare -/// `SummaryAgg{sketch: SummaryKind::Sum}` (an exact mergeable +/// `SummaryAgg{summary: SummaryKind::Sum}` (an exact mergeable /// accumulator — see `sum_now_binds_to_exact_agg_after_pr_6_followup`'s /// doc comment for the `ExactAgg` → `SummaryAgg` unification). #[test] @@ -517,8 +517,8 @@ fn phase_b_pattern_only_temporal_sum_binds_to_exact_agg() { let bound = bind_query_expr(&expr, AccuracyTarget::Epsilon(0.01)).unwrap(); match bound { PhysicalExpr::Committed(L4Plan::Summary(node)) => match &node.expr { - SummaryExpr::SummaryAgg { sketch, .. } => { - assert_eq!(sketch, &SummaryKind::Sum); + SummaryExpr::SummaryAgg { summary, .. } => { + assert_eq!(summary, &SummaryKind::Sum); } other => panic!("expected SummaryAgg(Sum), got {other:?}"), }, @@ -547,9 +547,9 @@ fn phase_b_pattern_only_spatial_aggregate_binds_to_multiple_sum() { match bound { PhysicalExpr::Committed(L4Plan::Summary(node)) => match &node.expr { SummaryExpr::SummaryAgg { - sketch, reduction, .. + summary, reduction, .. } => { - assert_eq!(sketch, &SummaryKind::Sum); + assert_eq!(summary, &SummaryKind::Sum); assert_eq!( reduction.group_keys().map(|k| k.keys()), Some(&[1][..]), @@ -582,9 +582,9 @@ fn phase_b_pattern_temporal_and_spatial_combined_binds_to_multiple_increase() { match bound { PhysicalExpr::Committed(L4Plan::Summary(node)) => match &node.expr { SummaryExpr::SummaryAgg { - sketch, reduction, .. + summary, reduction, .. } => { - assert_eq!(sketch, &SummaryKind::Increase); + assert_eq!(summary, &SummaryKind::Increase); assert_eq!(reduction.group_keys().map(|k| k.keys()), Some(&[1][..])); } other => panic!("expected SummaryAgg(Increase, by=[1]), got {other:?}"), @@ -674,7 +674,7 @@ fn phase_b_e2e_quantile_over_time_binds_to_quantile_sketch() { let kind = crate::emit::extract_root_sketch_kind(&bound); assert!( matches!(kind, Some(SummaryKind::Kll) | Some(SummaryKind::DDSketch)), - "expected quantile sketch family, got {kind:?}" + "expected quantile summary family, got {kind:?}" ); assert!( !binding_is_archive(&bound), @@ -683,9 +683,9 @@ fn phase_b_e2e_quantile_over_time_binds_to_quantile_sketch() { } /// `sum_over_time.yaml` — the legacy planner produces an exact-sum -/// aggregation row (no sketch). Control plane path: `Aggregate{Sum}` over +/// aggregation row (no summary). Control plane path: `Aggregate{Sum}` over /// `Window` → binds to an exact accumulator (`SummaryAgg{Sum}`), which is -/// neither an approximate sketch (so `extract_root_sketch_kind`, which +/// neither an approximate summary (so `extract_root_sketch_kind`, which /// excludes exact accumulators — see its doc comment — returns `None`) /// nor archive-routed. #[test] @@ -696,7 +696,7 @@ fn phase_b_e2e_sum_over_time_falls_through_to_logical() { ); assert!( crate::emit::extract_root_sketch_kind(&bound).is_none(), - "sum_over_time should not produce an approximate sketch" + "sum_over_time should not produce an approximate summary" ); assert!( !binding_is_archive(&bound), @@ -708,7 +708,7 @@ fn phase_b_e2e_sum_over_time_falls_through_to_logical() { /// temporal aggregation; the legacy planner emits an exact-sum row keyed /// on the by-label. Control plane path: `Aggregate{Sum, by=[…]}` over /// `Window` → binds to an exact accumulator (`SummaryAgg{Sum, by=[…]}`) — -/// no approximate sketch family. The by-label is preserved on the L3 +/// no approximate summary family. The by-label is preserved on the L3 /// group-by-id list, which Phase α's routing emit reads to build the /// per-label rollup partition. #[test] @@ -717,7 +717,7 @@ fn phase_b_e2e_sum_by_preserves_grouping_label() { "sum by (instance) (sum_over_time(http_requests_total[5m]))", AccuracyTarget::Epsilon(0.01), ); - // No approximate sketch family for plain Sum. + // No approximate summary family for plain Sum. assert!(crate::emit::extract_root_sketch_kind(&bound).is_none()); // The end shape may carry `Logical(Aggregate{by, ...})` beneath a // `SummaryAgg{Sum}` wrapper, or `Logical(Window{...})` when the @@ -742,8 +742,8 @@ fn phase_b_e2e_sum_by_preserves_grouping_label() { /// `rate_increase.yaml` — the legacy planner emits a MultipleIncrease /// (counter-reset adjusted) row. Control plane path: `Aggregate{Rate}` over /// `Window` → `bind_query_expr` rewrites `Rate` to `Increase` and binds an -/// exact accumulator (`SummaryAgg{Increase}`) — no approximate sketch -/// family. Both paths produce a single non-sketch streaming row; the L5 +/// exact accumulator (`SummaryAgg{Increase}`) — no approximate summary +/// family. Both paths produce a single non-summary streaming row; the L5 /// emitter is the one that picks the actual MultipleIncrease processor. #[test] fn phase_b_e2e_rate_falls_through_to_logical() { @@ -764,7 +764,7 @@ fn phase_b_e2e_rate_falls_through_to_logical() { /// At the time of writing, the control plane's `parse_query` may flatten /// `topk` differently (no `inside_topk` propagation through TopK + /// nested aggregate). The test asserts the END-STATE: either a -/// CountSketch sketch fired, OR a Logical pass-through (which Phase γ +/// CountSketch summary fired, OR a Logical pass-through (which Phase γ /// can decide whether to refine). The contract Phase β cares about is /// that the bound expression is well-formed. #[test] @@ -807,7 +807,7 @@ fn phase_b_e2e_archive_only_e2e_binding() { binding_is_archive(&bound), "archive-only intent must surface archive flag through L4 binding" ); - // No approximate sketch fires for archive-only intents. + // No approximate summary fires for archive-only intents. assert!(crate::emit::extract_root_sketch_kind(&bound).is_none()); } @@ -870,11 +870,11 @@ fn phase_b_archive_only_intents_round_trip_through_binder() { // // `AggIntent::Extension` (this deployment's `Frequency` point-query, // built via `crate::intent_algebra::frequency(accuracy, item)`) now binds to a -// real `Cms` sketch via `ControlPlaneCostModel::realize_extension`/ +// real `Cms` summary via `ControlPlaneCostModel::realize_extension`/ // `readout_extension` (ASAPController#150) — see `frequency_extension_binds_cms` // below and `optimizer::rules::mod::tests::typed_binding_endpoint_request_freq_binds_cms`. // `AggIntent::TopK { accuracy: Exact }` still declines to bind -// (`SummaryExpr::Logical`) rather than sketch — a REAL, accepted +// (`SummaryExpr::Logical`) rather than summary — a REAL, accepted // behavior change from this migration that remains open // (`TopK{Exact}`'s `exact_realization` has no accumulator form for it — // see `lower.rs`'s module docs and `cost_model.rs`'s module docs, @@ -884,7 +884,7 @@ fn phase_b_archive_only_intents_round_trip_through_binder() { fn frequency_extension_binds_cms() { // `ControlPlaneCostModel::realize_extension`/`readout_extension` // (ASAPController#150) now realize `AggIntent::Extension{"frequency"}` - // as a real `Cms` sketch instead of declining to `Logical`. + // as a real `Cms` summary instead of declining to `Logical`. let intent = crate::intent_algebra::frequency(AccuracyTarget::Epsilon(0.01), None); let expr = QueryExpr::Aggregate { reduction: Reduction::PerEntity, @@ -897,7 +897,7 @@ fn frequency_extension_binds_cms() { match bound { PhysicalExpr::Committed(L4Plan::Summary(node)) => { let SummaryExpr::SummaryEstimate { - sketch_input, + summary_input, query, } = &node.expr else { @@ -905,14 +905,14 @@ fn frequency_extension_binds_cms() { }; assert!( matches!( - &sketch_input.expr, + &summary_input.expr, SummaryExpr::SummaryAgg { - sketch: SummaryKind::Cms, + summary: SummaryKind::Cms, .. } ), "expected a Cms SummaryAgg, got {:?}", - sketch_input.expr + summary_input.expr ); assert!( matches!( diff --git a/crates/asap_types/Cargo.toml b/crates/asap_types/Cargo.toml index 8da21b4e..e343d8a0 100644 --- a/crates/asap_types/Cargo.toml +++ b/crates/asap_types/Cargo.toml @@ -16,7 +16,7 @@ xxhash-rust = { version = "0.8", features = ["xxh64"] } # via this crate): WindowType -> asap_ir::intent_algebra::query_expr::WindowKind # unification (scratchpad/artifacts/enum-unification-plan.md). Pin matches # control_plane's -- see control_plane/Cargo.toml's comment for the rationale. -asap-ir = { git = "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/ProjectASAP/ASAPController", rev = "94795092c5d2d9582ddf7486ac0043ceb9f8a34c" } +asap-ir = { git = "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/ProjectASAP/ASAPController", rev = "cc18c9872bbaf0cadf43566892c3974ec9948eba" } # Step 5 of the sketch-identity unification (see # scratchpad/artifacts/enum-unification-plan.md): `AccumulatorSpec` # (accumulator_spec.rs) converges data_plane's identity representation @@ -25,4 +25,4 @@ asap-ir = { git = "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/ProjectASAP/ASAPController", rev = "947950 # control_plane's pin exactly (`control_plane/Cargo.toml`) -- two # different revs of the same git dependency in one workspace resolve # to two distinct Rust types that won't unify. -asap-sketch = { git = "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/ProjectASAP/ASAPController", rev = "94795092c5d2d9582ddf7486ac0043ceb9f8a34c" } +asap-sketch = { git = "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/ProjectASAP/ASAPController", rev = "cc18c9872bbaf0cadf43566892c3974ec9948eba" } diff --git a/data_plane/Cargo.toml b/data_plane/Cargo.toml index 7585b08d..87a1882d 100644 --- a/data_plane/Cargo.toml +++ b/data_plane/Cargo.toml @@ -29,12 +29,12 @@ control_plane = { path = "../control_plane" } # control_plane's own pin) to pick up # `SketchQuery::PointCount.value: Option`, needed for the # named-key PointCount readout. -asap-sketch = { git = "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/ProjectASAP/ASAPController", rev = "94795092c5d2d9582ddf7486ac0043ceb9f8a34c" } +asap-sketch = { git = "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/ProjectASAP/ASAPController", rev = "cc18c9872bbaf0cadf43566892c3974ec9948eba" } # `asap_sketch::L4Node`'s own fields (`SummaryExpr::Logical(Box)`, # `SummaryAgg { col: ColumnRef, reduction: Reduction, .. }`) are `asap-ir` # types, not re-exported by `asap-sketch` -- `find_candidates` needs to # walk/match them directly. Same pin-must-match rule as `asap-sketch` above. -asap-ir = { git = "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/ProjectASAP/ASAPController", rev = "94795092c5d2d9582ddf7486ac0043ceb9f8a34c" } +asap-ir = { git = "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/ProjectASAP/ASAPController", rev = "cc18c9872bbaf0cadf43566892c3974ec9948eba" } # Shared external (workspace) serde.workspace = true diff --git a/data_plane/src/query_engines/asap_query_engine/summary_executor.rs b/data_plane/src/query_engines/asap_query_engine/summary_executor.rs index 9f004c76..c5667fbd 100644 --- a/data_plane/src/query_engines/asap_query_engine/summary_executor.rs +++ b/data_plane/src/query_engines/asap_query_engine/summary_executor.rs @@ -891,7 +891,7 @@ fn find_metric(node: &L4Node) -> Option { match &node.expr { SummaryExpr::Logical(qe) => find_metric_in_query_expr(qe), SummaryExpr::SummaryAgg { child, .. } => find_metric(child), - SummaryExpr::SummaryEstimate { sketch_input, .. } => find_metric(sketch_input), + SummaryExpr::SummaryEstimate { summary_input, .. } => find_metric(summary_input), SummaryExpr::SummaryMerge { children } => children.first().and_then(|c| find_metric(c)), _ => None, } @@ -981,7 +981,7 @@ mod tests { Rc::new(L4Node { expr: SummaryExpr::SummaryAgg { child, - sketch: SummaryKind::Kll, + summary: SummaryKind::Kll, params: SummaryParams::Kll { k: 200 }, col: ColumnRef::SampleValue, reduction, @@ -1001,7 +1001,7 @@ mod tests { Rc::new(L4Node { expr: SummaryExpr::SummaryAgg { child, - sketch: SummaryKind::Hll, + summary: SummaryKind::Hll, params: SummaryParams::Hll { precision: 10 }, col: ColumnRef::SampleValue, reduction, @@ -1013,10 +1013,10 @@ mod tests { }) } - fn estimate_node(sketch_input: Rc, query: SketchQuery) -> Rc { + fn estimate_node(summary_input: Rc, query: SketchQuery) -> Rc { Rc::new(L4Node { expr: SummaryExpr::SummaryEstimate { - sketch_input, + summary_input, query, }, schema: L4Schema { @@ -1143,7 +1143,7 @@ mod tests { Rc::new(L4Node { expr: SummaryExpr::SummaryAgg { child, - sketch: SummaryKind::Cms, + summary: SummaryKind::Cms, params: SummaryParams::Cms { width: 256, depth: 4, @@ -1199,7 +1199,7 @@ mod tests { Rc::new(L4Node { expr: SummaryExpr::SummaryAgg { child, - sketch: SummaryKind::CmsWithHeap, + summary: SummaryKind::CmsWithHeap, params: SummaryParams::CmsWithHeap { width: 256, depth: 4, @@ -1243,7 +1243,7 @@ mod tests { Rc::new(L4Node { expr: SummaryExpr::SummaryAgg { child, - sketch: SummaryKind::Sum, + summary: SummaryKind::Sum, params: SummaryParams::Sum, col: ColumnRef::SampleValue, // Sum is a genuine PromQL aggregation operator -- an empty @@ -2029,7 +2029,7 @@ mod tests { let mismatched = Rc::new(L4Node { expr: SummaryExpr::SummaryAgg { child, - sketch: SummaryKind::Kll, + summary: SummaryKind::Kll, params: SummaryParams::Kll { k: 500 }, col: ColumnRef::SampleValue, reduction: Reduction::by(vec![]), @@ -2423,7 +2423,7 @@ mod tests { let tree = Rc::new(L4Node { expr: SummaryExpr::SummaryAgg { child, - sketch: SummaryKind::MinMax, + summary: SummaryKind::MinMax, params: SummaryParams::MinMax, col: ColumnRef::SampleValue, reduction: Reduction::by(vec![]),