Skip to content

fix(control-plane): preserve query semantics through physical planning #449

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
zzylol merged 1 commit into from
Sep 2, 2026
Merged

fix(control-plane): preserve query semantics through physical planning #449

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 4 additions & 11 deletions control_plane/src/emit/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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:?}"
);
Expand Down
81 changes: 13 additions & 68 deletions control_plane/src/optimizer/rules/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -466,57 +439,29 @@ 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)
.unwrap_or_else(|| panic!("bind declined for override {ov:?}"));
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]
Expand Down
6 changes: 6 additions & 0 deletions data_plane/src/query_engines/asap_query_engine/engine.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(_))
)
}

Expand Down
4 changes: 0 additions & 4 deletions data_plane/tests/e2e_controller_plans_and_backend_serves.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();

Expand Down
16 changes: 16 additions & 0 deletions docs/developer_docs/control-plane/physical-compiler.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down