From d30ba9b9ecedd31afddf51b9031f1592d97d9fcc Mon Sep 17 00:00:00 2001 From: zz_y Date: Mon, 31 Aug 2026 13:44:51 -0600 Subject: [PATCH] fix(control-plane): preserve planned query semantics --- control_plane/src/emit/mod.rs | 15 +--- control_plane/src/optimizer/rules/mod.rs | 81 +++---------------- .../query_engines/asap_query_engine/engine.rs | 6 ++ ...e2e_controller_plans_and_backend_serves.rs | 4 - .../control-plane/physical-compiler.md | 16 ++++ 5 files changed, 39 insertions(+), 83 deletions(-) diff --git a/control_plane/src/emit/mod.rs b/control_plane/src/emit/mod.rs index 332039717..c2faa6bd2 100644 --- a/control_plane/src/emit/mod.rs +++ b/control_plane/src/emit/mod.rs @@ -1214,16 +1214,9 @@ mod runtime_tests { mk(AggType::Cardinality, Some(SketchType::HLL), Vec::new()), WorkloadCharacteristics::default(), ); - // TopK → CountSketch-with-heap. Not plain `Frequency, None` - // (capability-matched CMS default) any more — `Frequency`'s - // capability-matched default is `AggIntent::Extension`-shaped, - // which `asap_aware_mapping::boundary::implementation_for` maps to - // `PassThrough` unconditionally (ASAPController#150), so it no - // longer contributes a family to the union at all. Use a - // `CountSketch` override instead — it re-derives the statistic to - // `TopK` (not `Extension`-shaped), so it still binds, and still - // exercises "3 distinct capabilities on one metric → union of 3 - // distinct families". + // Frequency → CMS. This deployment's capability catalog exposes + // CountSketch only for TopK, so the incompatible override is ignored; + // importantly it does not rewrite this workload into TopK. store.set( METRIC, AggRole::Other, @@ -1245,7 +1238,7 @@ mod runtime_tests { BTreeSet::from([ SketchKind::DDSketch, SketchKind::Hll, - SketchKind::CountSketchWithHeap + SketchKind::Cms ]), "a metric queried by 3 capabilities must accumulate 3 families (UNION, not first-wins)\nmap: {map:?}" ); diff --git a/control_plane/src/optimizer/rules/mod.rs b/control_plane/src/optimizer/rules/mod.rs index 7ba4f6b76..683dcf887 100644 --- a/control_plane/src/optimizer/rules/mod.rs +++ b/control_plane/src/optimizer/rules/mod.rs @@ -93,45 +93,18 @@ pub fn bind_workload_typed_with_item_filter( // Priority: workload-spec metric-name match → AggType-driven // default. The metric-name match owns the demo contract rows; the // AggType fallback covers everything else. - let (mut statistic, mut accuracy_pref) = + let (statistic, accuracy_pref) = classify_demo_metric(&w.metric_name).unwrap_or_else(|| match w.aggregations[0] { AggType::Quantile => (StatisticClass::Quantile, AccuracyPreference::RelativeError), AggType::Cardinality => (StatisticClass::Cardinality, AccuracyPreference::default()), AggType::Frequency => (StatisticClass::Frequency, AccuracyPreference::default()), }); - // An explicit `sketch_family_override` is authoritative for the FAMILY - // — and therefore for the STATISTIC CLASS it answers. The query-derived - // statistic above only covers Quantile/Cardinality/Frequency from - // `AggType`; a `count(...)` / `topk(...)` / `count_over_time(...)` - // query can classify as the wrong class, so without this an HLL / - // CountMinSketch / CountSketch override would mismatch the derived - // statistic, be rejected by `is_valid_pair` below, and silently fall - // back to the catalog default (DDSketch) — the controller would then - // emit `family: ddsketch` for an HLL/CMS/CountSketch metric. Re-derive - // the statistic from the override whenever the derived one is - // incompatible, so the override drives both family and statistic. - if let Some(st) = w.sketch_type_override.as_ref() { - let ov = SketchKind::from(st.clone()); - if !is_valid_pair(ov.clone(), statistic) { - let (s, ap) = match ov { - SketchKind::DDSketch | SketchKind::Kll => { - (StatisticClass::Quantile, AccuracyPreference::RelativeError) - } - SketchKind::Hll => (StatisticClass::Cardinality, AccuracyPreference::default()), - SketchKind::Cms => (StatisticClass::Frequency, AccuracyPreference::default()), - SketchKind::CountSketch => (StatisticClass::TopK, AccuracyPreference::default()), - // `ov` always comes from `SketchKind::from(SketchType)` - // (`w.sketch_type_override` is the legacy 5-family enum), - // so only these 5 canonical families are ever reachable. - other => unreachable!( - "sketch_type_override resolved to an unsupported SketchKind {other:?}" - ), - }; - statistic = s; - accuracy_pref = ap; - } - } + // The query/operator owns the statistic. An override may select a + // compatible implementation (for example KLL instead of DDSketch for a + // quantile), but must never rewrite query semantics merely to make an + // incompatible family fit. Contract-row classification above handles the + // MVP's count/topk/frequency metrics before this compatibility check. // SumRateCount → no sketch (raw passthrough). Decline the typed // binding so the caller falls back to the legacy raw plan. @@ -466,30 +439,19 @@ mod tests { assert_eq!(plan.agent_config.sketch_type, SketchType::CountSketch); } - /// Regression: an explicit `sketch_family_override` must pin the family - /// (and statistic) even when the query's `AggType` classifies as a - /// different/incompatible class. Without the override re-deriving the - /// statistic, `is_valid_pair` rejected HLL/CMS/CountSketch overrides - /// against a Quantile-classified query and fell back to DDSketch, so - /// the controller emitted `family: ddsketch` for those metrics. + /// An override chooses an implementation; it cannot change a query's + /// statistic just to make an incompatible family appear valid. #[test] - fn override_pins_nonquantile_family_over_misclassified_query() { + fn incompatible_override_does_not_rewrite_query_semantics() { use crate::emit::extract_root_sketch_kind; use planner_types::post_asap::SketchKind; for (ov, expect) in [ (SketchType::DDSketch, SketchKind::DDSketch), (SketchType::KLL, SketchKind::Kll), - (SketchType::HLL, SketchKind::Hll), - // CountSketch override re-derives statistic as TopK (see the - // override arm below), and `bind_cms_topk` always binds the - // heap-bearing kind for a top-k intent — matches this - // fixture's pre-`SketchKind`-split expectation, when - // `with_heap: true` was a params flag rather than kind - // identity. - (SketchType::CountSketch, SketchKind::CountSketchWithHeap), + (SketchType::HLL, SketchKind::DDSketch), + (SketchType::CountSketch, SketchKind::DDSketch), + (SketchType::CountMinSketch, SketchKind::DDSketch), ] { - // Query classifies as Quantile (the mis-derived case observed - // live for count()/topk()/count_over_time()); the override must win. let mut w = workload(vec![AggType::Quantile]); w.sketch_type_override = Some(ov.clone()); let pe = bind_workload_typed(&w) @@ -497,26 +459,9 @@ mod tests { assert_eq!( extract_root_sketch_kind(&pe), Some(expect.clone()), - "override {ov:?} should pin family {expect:?}, not fall back to DDSketch", + "override {ov:?} must not change Quantile semantics", ); } - - // `CountMinSketch` re-derives statistic to `Frequency`, which is - // `AggIntent::Extension`-shaped (this deployment's point-frequency - // query). `ControlPlaneCostModel::realize_extension`/ - // `readout_extension` (ASAPController#150) now realize it as - // `SketchKind::Cms` — matching `capability_matching::pick_family`'s - // own `Frequency -> Cms` mapping — so this override binds like any - // other now. - let mut w = workload(vec![AggType::Quantile]); - w.sketch_type_override = Some(SketchType::CountMinSketch); - let pe = bind_workload_typed(&w) - .unwrap_or_else(|| panic!("CountMinSketch override should bind (ASAPController#150)")); - assert_eq!( - extract_root_sketch_kind(&pe), - Some(SketchKind::Cms), - "CountMinSketch override should pin Cms (Frequency's family), not decline", - ); } #[test] 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 a041f80ab..89c774cdb 100644 --- a/data_plane/src/query_engines/asap_query_engine/engine.rs +++ b/data_plane/src/query_engines/asap_query_engine/engine.rs @@ -605,9 +605,15 @@ fn effective_sketch_function( fn effective_is_cumulative( candidate: &control_plane::asap_tier_analysis::ASAPTierCandidate, ) -> bool { + use crate::storage_engines::sketch_db::index::Capability; + use control_plane::asap_tier_analysis::OuterAgg; + matches!( effective_sketch_function(candidate), "quantile_over_time" | "count_distinct_over_time" | "topk_over_time" + ) || matches!( + (&candidate.required_capability, &candidate.outer_agg), + (Capability::CardinalityApprox, OuterAgg::Count(_)) ) } 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 f222cf26c..d5b8ad1cd 100644 --- a/data_plane/tests/e2e_controller_plans_and_backend_serves.rs +++ b/data_plane/tests/e2e_controller_plans_and_backend_serves.rs @@ -2765,10 +2765,6 @@ async fn live_serve_actually_answers_ddsketch_quantile() { // over the real data instead of being merged with it. Tracked as // https://github.com/ProjectASAP/ASAPQuery-backend/issues/431. #[tokio::test(flavor = "multi_thread", worker_threads = 4)] -#[ignore = "known gap, exposed (not caused) by sketch_reducer.rs's retirement -- see the \ - module comment immediately above this test for the full root cause \ - (effective_is_cumulative misclassifies bare count(), previously masked by the \ - legacy reducer fallback). Tracked as ASAPQuery-backend#431."] async fn live_serve_hll_global_count_merges_across_sids() { let _live = LiveServeEnvGuard::enable(); diff --git a/docs/developer_docs/control-plane/physical-compiler.md b/docs/developer_docs/control-plane/physical-compiler.md index cb698f2e8..7a8a9ef4f 100644 --- a/docs/developer_docs/control-plane/physical-compiler.md +++ b/docs/developer_docs/control-plane/physical-compiler.md @@ -3,6 +3,22 @@ > Interface status: target public API. Existing migration modules must converge > on this boundary. +## Current implementation boundary + +The production path has not yet converged on the public interfaces below. It +currently consumes ASAPPlanner types pinned in `control_plane/Cargo.toml`, then +uses `physical::colored_dag::StageAllocator`, `ThreeStageEmitter`, and +`backend_plan::from_stage_config` to produce agent YAML and `BackendPlan`. +Callers must not treat the target `PlanningRequest`, `PhysicalCompiler`, or +`CompiledPlanBundle` examples below as implemented Rust APIs. + +ASAPPlanner owns logical semantics and summary selection. In particular, a +deployment override may choose only a family compatible with the selected +statistic; the physical compiler must reject or ignore an incompatible +override, never change the statistic to make the override fit. Upgrading the +Planner pin is a whole-interface migration because newer Planner revisions +change the post-ASAP family, reduction, grouping, and maintenance types. + ## 1. Code architecture The control plane has three public layers: