From a330e5ac602715fb16989dcf825706f079f959df Mon Sep 17 00:00:00 2001 From: zz_y Date: Wed, 22 Jul 2026 10:40:54 -0600 Subject: [PATCH 1/6] =?UTF-8?q?docs(data=5Fplane):=20Step=20C=20design=20d?= =?UTF-8?q?oc=20=E2=80=94=20recursive=20L4Node=20plan-executor?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Planning doc, not an implementation (same spirit as sketchindex-sid-unification-plan.md). Grounds the design in what ASAPQueryEngine::execute() actually does today: a flat Vec loop that overwrites rather than folds multiple candidates, real cross-sid merge only for ExactAgg (+ a global-only HLL special case), and two raw-AST fallbacks (topk-over-rate, rate-over-frequency) that exist because today's flat Capability vocabulary can't express those compositions. Proposes replacing the flat loop with a real recursive walk over asap_sketch::L4Node (already built control-plane-side since Step A/B, never consumed by data_plane), and flags four open questions that need a decision before/while implementing: where the tree comes from, SummaryMerge's param-equality requirement for sketch families (Capability matching is family-level only, doesn't see SummaryParams), where rank-and-slice post-processing (topk-over-non-frequency-measure) lives now that the reason it doesn't tree-ify today turns out to be narrower than it first looked, and rollout shape. Co-Authored-By: Claude Sonnet 5 --- .../docs/l4node-plan-executor-design.md | 422 ++++++++++++++++++ 1 file changed, 422 insertions(+) create mode 100644 data_plane/docs/l4node-plan-executor-design.md diff --git a/data_plane/docs/l4node-plan-executor-design.md b/data_plane/docs/l4node-plan-executor-design.md new file mode 100644 index 00000000..38f42283 --- /dev/null +++ b/data_plane/docs/l4node-plan-executor-design.md @@ -0,0 +1,422 @@ +# Step C: a recursive `L4Node` plan-executor for `ASAPQueryEngine` + +This is a planning doc, not an implementation — same spirit as +[`sketchindex-sid-unification-plan.md`](./sketchindex-sid-unification-plan.md). +It records what today's query-execution path actually does (verified by +reading the live code, not by memory of what it's supposed to do), +proposes a design, and flags the places where the design isn't settled +yet. + +**Status as of Jul 2026:** not started. Step A and Step B (see +`control_plane/docs/design.md` §3's terminology table and +`control_plane/src/sketch_algebra/physical_expr.rs`'s module docs) landed +the canonical L4 IR — `asap_sketch::{SummaryExpr, L4Node}` — and a real +L3→L4 binder (`asap_plan::bind::implement_tree_in_with`, +`control_plane::asap_tier_implement::implement_promql_for_asap_tier`) on +the **control-plane** side only. `data_plane` has never consumed either; +grepping `data_plane/src` for `asap_sketch`, `L4Node`, `SummaryExpr`, +`SummaryAgg`, `SummaryMerge`, `SummaryEstimate` returns zero hits (the +`asap_sketch*`-prefixed names it does use, e.g. `DdSketch`/`KllSketch`, +are the unrelated `asap_sketchlib` sketch-math crate). Wiring the query +path onto the tree is new integration work, not finishing something +half-connected. + +## 1. What today's engine actually does + +The live entry point is `ASAPQueryEngine::execute()` +(`data_plane/src/query_engines/asap_query_engine/engine.rs:1264-1923`). +There is no tree anywhere in this path — the older `find_compatible_aggregation`/ +`handle_query` dispatcher is fully retired (comments only, e.g. +`engine.rs:1904-1912`) and `query_precompute_for_statistic` +(`engine.rs:602-631`) is `#[cfg(test)]`-only, unreachable from production. + +``` +PromQL string + → control_plane::asap_tier_analysis::analyze_promql_for_asap_tier(query) + → query_parser::parse_query_expr_canonical // PromQL → L3 QueryExpr + → capability_for(&AggIntent) // AggIntent → Capability, per node + → ASAPTierAnalysis { candidates: Vec, unsupported } // FLAT LIST + → for candidate in &analysis.candidates { // engine.rs:1394 + sids = idx.instances_matching(metric, group_by_keys) + sids = sids.filter(|sid| candidate.required_capability.is_satisfied_by(sid.capability)) + result = SketchReducer::evaluate_exact_agg / evaluate_for_capability(...) + combined_result = Some(result) // ← overwrites every iteration + } + → asap_tier_result_to_query_result(combined_result, ...) +``` + +`ASAPTierAnalysis`/`ASAPTierCandidate` (`control_plane/src/asap_tier_analysis.rs:67-131`) +are a flat `Vec`, each candidate a leaf-shaped unit: `metric_name`, +`group_by_keys`, `required_capability: Capability`, plus +`outer_fn`/`outer_agg` flags used to encode *some* composition (e.g. "rate +over sum") without any actual nesting. + +**This loop doesn't fold multiple candidates — it overwrites.** The +comment at `engine.rs:1367-1372` says so directly: *"Multi-candidate +aggregation is deferred (single-result shapes today)... When more than +one candidate is supported, a follow-up will fold per-candidate +ASAPTierResults."* Today's queries happen to produce ≤1 real candidate, +so this has never been exercised. **This means a real tree-walk executor +is new behavior, not a refactor of an existing fold** — there's no +existing multi-node composition semantics to be faithful to. + +### 1a. The typed capability matcher is real, and already control-plane-coupled + +`data_plane` is not independent of `control_plane` today — despite the +crate boundary, `data_plane::storage_engines::sketch_db::data::Capability` +is a direct re-export: + +```rust +// data_plane/src/storage_engines/sketch_db/data/mod.rs:59 +pub use control_plane::sketch_algebra::{Capability, SketchKindHandle}; +``` + +and the matcher call is real and load-bearing (`engine.rs:1480-1487`): + +```rust +let satisfied = idx + .with_instance(*sid, |m| { + m.capability + .as_ref() + .map(|cap| required.is_satisfied_by(cap)) + .unwrap_or(false) + }) + .unwrap_or(false); +``` + +`Capability::is_satisfied_by` (`control_plane/src/sketch_algebra/capability.rs:330-379`) +is asymmetric and encodes real cross-family equivalences: a +`MultipleSum`-indexed sid satisfies a required `Sum`; a `Sum`-indexed sid +satisfies a required `Increase`/`Rate` (via `sum_satisfies_increase`, +added this session for PR #395) but never the reverse. **Whatever "sid +catalog lookup" primitive `SummaryAgg` leaves resolve into in the new +design must call this exact function, unchanged** — it would be very easy +to accidentally re-derive a subtly different, wrong matcher while +re-plumbing this into a tree-executor's leaf-resolution step. + +### 1b. `StreamingConfig` carries no tree either + +The only wire payload `data_plane` consumes from `control_plane` is +`StreamingConfig` (`crates/asap_types/src/streaming_config.rs:58-73`): + +```rust +pub struct StreamingConfig { + pub aggregation_configs: HashMap, // keyed by PolicyFingerprint + pub storage_backend: StorageBackend, + pub monitors: Vec, +} +``` + +`AggregationConfig` is one flat struct per policy (family, params, +grouping, window, spatial filter) — used at *ingest* time to decide what +to build, and secondarily as a `policy_fp → {sids}` index optimization at +query time (`find_matching_policies`, `engine.rs:716-724`, itself a +fallback because `policy_fp` often comes back `UNSET` for real wire +sketches — issue #271/#272). No readout/estimate/merge structure rides on +the wire at all. **This confirms the design question below is real, not +hypothetical**: today, `data_plane` re-derives everything about "what to +compute" independently, per query, from the PromQL text — it has never +received a plan from `control_plane`. + +### 1c. Merge exists for exactly one family today + +Two real merge paths, not unified: + +- **ExactAgg (general, keyed, works today).** `SketchReducer::evaluate_exact_agg` + (`sketch_reducer.rs:903-1084`) groups all matching sids' accumulators by + `(projected_group_by_keys, window_end)`, folds each bucket via + `AggregateCore::merge_with` (line 1024), reads out via `query_statistic`. + Comment at `sketch_reducer.rs:1009-1015`: *"multiple entries come from + multiple sids that share the projected group (e.g. several (zone=z0, + rack=*) sids collapsing to a single zone=z0 group)."* This is the one + real, general precedent for what `SummaryMerge` needs to become. +- **HLL (special-cased, global only).** `evaluate_cardinality_global` + (`sketch_reducer.rs:428-527`) merges HLL registers register-wise across + every candidate sid, but only fires when `group_by_keys.is_empty()` + (`engine.rs:1744-1752`) — the comment at `engine.rs:1734-1743` explains + why: the per-series path would otherwise double-count or never produce + one global number. + +**For every other grouped sketch case — `quantile by (zone) (...)`, +`count by (zone) (unique_metric)`, per-group `topk`/`frequency` — there is +no cross-sid merge at all today.** `evaluate_core` +(`sketch_reducer.rs:534-857`) just pushes each sid's series into the +output with no de-dup on `(series_label_values)` — two sids projecting to +the same group silently produce two output series with identical labels, +not one merged series. **This is a real, currently-shipping bug, and +per-group sketch merge for `SummaryMerge` is new work, not a +generalization of something that already generalizes.** + +**Sketch merge is conditional on exact param match, and today's +`Capability` matcher can't see that.** `asap_sketch::SummaryExpr::SummaryMerge`'s +own doc (`crates/sketch/src/expr.rs:87-90`) is explicit: *"all inputs must +agree on `(sketch, params)` and the catalog flag `mergeable` must be +true"* — two KLL sketches only merge cleanly if they were built with the +same `k` (compaction schedule); two CMS matrices only merge if `width`/`depth` +match. `control_plane::sketch_algebra::Capability` (the type +`is_satisfied_by`-matching already runs on, §1a) is **family-level only** +— `Capability::QuantileApprox(SketchKindHandle)` etc. carry a family +selector, never a concrete `SummaryParams` value +(`control_plane/src/sketch_algebra/capability.rs:42-73`). So the existing +matcher answers "does this sid's family satisfy the requirement," not +"do these two sids' params match each other" — a real gap for +`SummaryMerge`'s child-sid selection, which needs the stricter check. +Concretely: if two sids covering the same group were registered with +different `k`/`width`/`depth` (plausible after a re-plan bumps accuracy +targets, or a rolling deploy changes catalog defaults mid-flight), +`instances_matching` + `is_satisfied_by` would happily return both as +individually-satisfying candidates, but folding them via `SummaryMerge` +would be invalid. This needs an explicit param-equality filter *within* +each `SummaryMerge`'s child set (not just per-child `Capability` +satisfaction), plus a decision for what happens to a mismatched minority +(exclude and note partial coverage? `CapabilityMiss` the whole group? +no downsampling/resize path exists in any sketch library used here today, +so "resize to match" is not on the table) — see open question 2. + +### 1d. Two structural escape hatches exist today — but the reason is narrower than it first looks + +- `try_topk_over_rate_fallback` (`engine.rs:280-537`) hand-parses the raw + `promql_parser` AST for `topk(K, sum by (gbk) (rate(metric[r])))` (or + `bottomk`/`irate`), synthesizes an `ExactAgg(Sum)` candidate for the + *inner* `sum(rate(...))`, evaluates it exactly, then **sorts and slices + the result to the top/bottom K groups in-engine, after readout**. This + runs *before* the typed candidate path and bypasses it. +- `try_rate_over_frequency_fallback` (`engine.rs:557-600`) similarly + special-cases `rate(cms_metric[r])` against a `FrequencyEstimate` sid. + +**Correction from an earlier draft of this doc:** `topk(K, expr)` does +*not* categorically fail to decompose into the L4 tree — `AggIntent::TopK` +is explicitly documented as ranking "by the *aggregate output*" +(`crates/ir/src/intent_algebra/agg_intent.rs:79-87`), i.e. it's designed +to compose with an inner aggregate, not just rank raw per-key frequency. +The real story, grounded in `control_plane/src/query_parser/promql.rs:309-345`: + +```rust +let measure = if is_count_over_time(agg.expr.as_ref()) { + RankingMeasure::Frequency +} else { + RankingMeasure::NonAdditive +}; +if is_frequency_heavy_hitter(descending, measure) { + return Ok(QueryExpr::TopK { k, by, input: Box::new(inner) }); // → AggIntent::TopK +} +// otherwise: Sort { Limit { inner } } — the generic order-by-value path +``` + +`AggIntent::TopK` only fires for `topk` (never `bottomk`) ranking by +`count_over_time(...)` specifically (`RankingMeasure::Frequency`) — that +shape *does* decompose today, straight into a single bindable +`SummaryAgg{CmsWithHeap/CountSketchWithHeap}` node, no fallback needed. +Every other ranking measure — including `sum by (gbk) (rate(...))`, the +shape the fallback actually handles — is `RankingMeasure::NonAdditive` +and lowers to `QueryExpr::Sort { QueryExpr::Limit { } }` +instead. The inner `Aggregate` (the `sum(rate(...))`) would itself bind +fine on its own (Step B already proved `Aggregate{Sum, child: Aggregate{Rate,...}}` +binds to a nested `SummaryAgg{Sum, child: SummaryAgg{Increase,...}}`) — +the actual blocker is that `implement_tree_in_with` only recurses through +the `Aggregate` spine (its own "conservative fallbacks" doc: a logical +parent above a bindable aggregate subsumes the whole subtree unbound), and +`Sort`/`Limit` aren't `Aggregate` — so `Sort{Limit{Aggregate{...}}}` stops +at the outer `Sort` and the *whole thing* falls to one opaque `Logical` +node, never exposing the inner `Aggregate` to be bound at all. + +**This means the fallback isn't necessarily permanent, forced-external +special-casing** — it's the same shape of gap Step B's `lower.rs` +pre-passes already close for other cases (Count{Exact}, Rate). A +control-plane-side pre-pass recognizing `Sort{Limit{Aggregate{...}}}` +could strip the `Sort`/`Limit` wrapper, recurse `implement_tree_in_with` +into the inner `Aggregate` to get a real `SummaryAgg`/`SummaryEstimate` +node, and carry the stripped rank/limit info *alongside* the tree (not as +a new `SummaryExpr` variant — ranking-and-slicing a readout is an +execution-time/L5 concern, not an L4 "what sketch to build" concern) for +`data_plane`'s executor to apply after evaluating that subtree. Whether +that pre-pass is worth building now or the raw-AST fallback should stay +as-is a while longer is an open call — see open question 3 — but the +reasoning for *why* is now precise instead of "these are fundamentally +incompatible," which was wrong. + +### 1e. Other things any executor rewrite must reproduce exactly + +- **Keyed-CMS safe-miss gate** (`engine.rs:1549-1626`): a + `FrequencyEstimate` sid answers only bucket totals; a keyed selector + like `cms_metric{item="X"}` must `CapabilityMiss` to archive *unless* + the sid is registered in `item_label` mode and the filter resolves to + that label. Skipping this returns a *wrong answer*, not just a miss — + correctness-critical, must be preserved verbatim. +- **Coverage-aware archive stitching** (`stitch_warm_and_archive`, + `engine.rs:1136-1190`): compares `ASAPTierResult.coverage` against the + requested window and merges with an archive-engine answer by + `(labels, timestamp)` when the warm tier only partially covers it. This + is a cross-cutting concern layered *around* whatever answers the query, + not encodable in the L4 IR — it belongs at the tree root only, once, + not per node (an N-way per-node stitch is not what this needs to + become). +- **`instances_matching` + capability-check is duplicated at ~4 call + sites** in `engine.rs` today rather than being one function — worth + consolidating into the single reusable primitive `SummaryAgg` leaves + would call, rather than duplicating it a 5th time. +- `EngineRouter`/`BackendStorageRouting` + (`data_plane/src/query_engines/routing/`) sit one layer above all of + this and are orthogonal — they decide *which whole engine* (warm sketch + tier vs. archive tier) answers a query, never *which node inside* it. + The only contract the new executor must keep is `execute()`'s existing + `Err(EngineError::CapabilityMiss(...))` signaling (used at ~7 sites + today, each firing `spawn_capability_miss_notify`), so archive failover + keeps working unchanged. + +## 2. Proposed design + +### 2a. Where the tree comes from + +`data_plane` builds its own `Rc` tree at query time, mirroring +Step A's `implement_promql_for_asap_tier` rather than waiting for +`control_plane` to ship one over the wire. Rationale: `data_plane` +already independently re-parses PromQL and independently derives +`AggIntent`s today (§1); it is *already* the pattern in this codebase +that the query-time IR is re-derived locally rather than carried on +`StreamingConfig` (which is, and stays, ingest-time-only — see §1b). Two +concrete integration options, need a decision before implementation: + +1. `data_plane` takes a direct dependency on `asap_plan`/`asap_sketch` + (already a `control_plane` git dependency; would become a `data_plane` + one too) and calls `asap_plan::bind::implement_tree_in_with` itself, + passing its own `CostModel` (mirroring + `control_plane::sketch_algebra::cost_model::ControlPlaneCostModel`, or + literally reusing it if `data_plane` is willing to depend on + `control_plane` for it the way it already does for `Capability`). +2. `data_plane` calls into a new `control_plane`-side function (e.g. + exposing `implement_promql_for_asap_tier` or an equivalent) rather + than depending on `asap_plan` directly, keeping `asap_plan`/`asap_sketch` + coupling confined to `control_plane`, consistent with `data_plane`'s + existing pattern of depending on `control_plane::sketch_algebra` + types (`Capability`) rather than reaching past it to `asap_sketch` + itself. + +Leaning toward (2) — it matches the existing `Capability` re-export +pattern (§1a) and keeps `asap_plan`/`asap_sketch` as a `control_plane`-only +upstream coupling, one seam instead of two — but this needs to be decided +before writing code, not discovered mid-implementation. + +### 2b. The recursive walk + +```rust +fn execute_node(node: &L4Node, window: TimeRange, ctx: &ExecCtx) + -> Result +{ + match &node.expr { + SummaryExpr::SummaryAgg { sketch, params, col, by, .. } => { + // LEAF: one consolidated sid-catalog-lookup primitive, + // replacing the ~4 duplicated call sites in engine.rs today. + let required = capability_from_summary_kind(sketch, params); + let sids = ctx.index + .instances_matching(&ctx.metric, by) + .filter(|sid| ctx.index.with_instance(*sid, |m| + m.capability.as_ref().is_some_and(|cap| required.is_satisfied_by(cap)) + ).unwrap_or(false)); + ExecResult::SketchState { sids: sids.collect(), by: by.clone() } + } + SummaryExpr::SummaryEstimate { sketch_input, query } => { + let state = execute_node(sketch_input, window, ctx)?; // recurse + readout(state, query) // quantile / cardinality / point_count / topk + } + SummaryExpr::SummaryMerge { children } => { + let states: Vec<_> = children.iter() + .map(|c| execute_node(c, window, ctx)) + .collect::>()?; + // generalizes evaluate_exact_agg's (group, window) fold (§1c) + // to every SummaryKind, not just ExactAgg — but unlike ExactAgg + // accumulators, sketch families require exact (kind, params) + // agreement to merge at all (§1c's mergeable-params landmine); + // this must filter/partition each group's candidate sids on + // params equality *before* folding, not just Capability + // satisfaction. + merge_by_group(states) + } + SummaryExpr::Logical(qe) => { + // No summary committed for this subtree — same meaning as + // today's "no candidate bound": CapabilityMiss, let + // EngineRouter fail over to archive. + Err(EngineError::CapabilityMiss(/* ... */)) + } + SummaryExpr::SummaryJoin { .. } + | SummaryExpr::SummarySubtract { .. } + | SummaryExpr::SummaryDelete { .. } => { + // No Bind* path produces these yet (see physical_expr.rs's + // module docs) — unreachable in practice today. + unimplemented!("not yet surfaced by any binder") + } + } +} +``` + +`execute()` calls `execute_node` once at the tree root, then applies the +coverage-aware archive stitch (§1e) once on the final result — not +per-node. + +### 2c. What's genuinely new work vs. what's porting + +| Piece | Status | +|---|---| +| `SummaryAgg` leaf → sid lookup + capability check | Porting — consolidates 4 duplicated call sites, must call `is_satisfied_by` unchanged (§1a) | +| `SummaryEstimate` → readout | Porting — `evaluate_core`'s per-family decode blocks (quantile/cardinality/frequency/topk) already exist, just need re-homing under the new dispatch | +| `SummaryMerge` for `ExactAgg`-family kinds | Porting — `evaluate_exact_agg`'s fold-by-`(group,window)` already does this | +| `SummaryMerge` for sketch families (KLL/DDSketch/CMS/CountSketch by group) | **New** — doesn't exist today outside the global-only HLL special case (§1c), and needs a param-equality filter `Capability` matching alone doesn't provide (§1c) | +| Keyed-CMS safe-miss gate | Porting — must be preserved verbatim inside the `SummaryEstimate`/`FrequencyEstimate` readout path | +| Coverage-aware archive stitch | Porting — moves from per-query to tree-root-only, same logic | +| `topk(K, sum(...))` (`RankingMeasure::NonAdditive`) / `rate(cms(...))` post-processing | Not tree-shaped under today's `implement_tree_in_with` (stops at the outer `Sort`/`Limit`), but potentially *becomes* tree-shaped down to a thin rank/limit wrapper with a control-plane-side pre-pass — see the corrected §1d and open question 3 | + +## 3. Open questions (need a decision before/while implementing) + +1. **Tree-source integration (§2a)**: does `data_plane` depend on + `asap_plan`/`asap_sketch` directly, or call through a `control_plane` + seam? Leaning (2) above but unconfirmed. +2. **`SummaryMerge` semantics for sketch families**: `evaluate_exact_agg`'s + `(group, window)` bucketing is the closest precedent, but it needs a + param-equality filter on top that `Capability` alone can't provide + (§1c) — `control_plane::sketch_algebra::Capability` is family-level + only (`SketchKindHandle`, no `SummaryParams`), so `SummaryMerge`'s + child-sid selection needs a *stricter* check than + `instances_matching` + `is_satisfied_by` already gives the plain + `SummaryAgg` leaf case: exact `(SummaryKind, SummaryParams)` agreement + across every sid folded into one merge. Open sub-questions: what + happens to a sid that matches on `Capability` but not on params (drop + it and report partial coverage? `CapabilityMiss` the whole group? no + resize/downsample path exists in any sketch library used here today); + does merging change the `SummaryEstimate`'s accuracy math (a merged + multi-sid sketch can have different error bounds than a single-sid + one — is that tracked anywhere, or a new gap); and does the same + `(group, window)` bucketing generalize cleanly to every sketch family + `data_plane` needs to merge here, or do some (e.g. CMS-with-heap, where + merging matrices is well-defined but merging *heaps* isn't the same + operation) need family-specific merge logic beyond a generic fold? +3. **Where does "rank-and-slice a readout" live?** Corrected in §1d: + `topk`/`bottomk` ranking by a non-`count_over_time` measure (the shape + `try_topk_over_rate_fallback` handles) lowers to `QueryExpr::Sort { + QueryExpr::Limit { } }`, which `implement_tree_in_with` + doesn't decompose today (stops at the outer `Sort`, whole subtree + stays `Logical`) — but the inner `Aggregate` would bind fine on its + own if reached. Options: (a) leave the raw-AST fallback exactly as-is, + `data_plane`-side, unchanged; (b) add a `control_plane`-side pre-pass + (same shape as Step B's `lower.rs` pre-passes) that strips `Sort`/ + `Limit`, binds the inner `Aggregate` for real, and carries the + rank/limit as metadata *alongside* the resulting `L4Node` (not a new + `SummaryExpr` variant — this is an L5/execution-time concern, not an + L4 "what to build" concern) for `data_plane`'s executor to apply after + evaluating that subtree; (c) something upstream in `asap_plan` + (unlikely — this is deployment-specific PromQL-surface shape + recognition, the same category of thing Step B's `lower.rs` + pre-passes handle locally rather than pushing upstream). (b) is more + work than (a) but turns a permanent raw-AST special case into a real, + general "any `Sort{Limit{...}}}`-wrapped bindable aggregate" capability + — worth deciding deliberately rather than defaulting to (a) just + because it's less work right now. +4. **Rollout**: land behind a parallel path (mirroring + `USE_TYPED_STAGE_SPLIT`/`USE_TYPED_SKETCH_ALGEBRA`'s env-var-gated + parallel-path pattern from Step A/B) so the new executor can be + compared against the existing flat dispatcher before it becomes the + only path, or replace `execute()`'s loop directly once confident? + Given §1c/§1d's gaps are real, currently-shipping-behavior questions + (not just refactor risk), a parallel/comparable rollout seems safer + than a hard cutover, but this changes the shape of the work + noticeably (need to keep both paths correct simultaneously for a + while) and should be confirmed before starting. From 1db042a88370deef7b4d7452daa6480711e57c87 Mon Sep 17 00:00:00 2001 From: zz_y Date: Wed, 22 Jul 2026 11:01:08 -0600 Subject: [PATCH 2/6] docs(data_plane): trim Step C design doc Cuts the doc from ~420 to ~100 lines -- drops exhaustive file:line citations, quoted source blocks, and the meta-commentary correcting an earlier draft's wrong topk-over-rate claim. Keeps the four real gaps, the executor sketch, and the four open questions; nothing substantive lost, just the supporting evidence that mattered for getting the design right, not for reading it afterward. Co-Authored-By: Claude Sonnet 5 --- .../docs/l4node-plan-executor-design.md | 482 +++--------------- 1 file changed, 82 insertions(+), 400 deletions(-) diff --git a/data_plane/docs/l4node-plan-executor-design.md b/data_plane/docs/l4node-plan-executor-design.md index 38f42283..b7a73699 100644 --- a/data_plane/docs/l4node-plan-executor-design.md +++ b/data_plane/docs/l4node-plan-executor-design.md @@ -1,422 +1,104 @@ # Step C: a recursive `L4Node` plan-executor for `ASAPQueryEngine` -This is a planning doc, not an implementation — same spirit as +Planning doc, not an implementation — same spirit as [`sketchindex-sid-unification-plan.md`](./sketchindex-sid-unification-plan.md). -It records what today's query-execution path actually does (verified by -reading the live code, not by memory of what it's supposed to do), -proposes a design, and flags the places where the design isn't settled -yet. +Status: not started. -**Status as of Jul 2026:** not started. Step A and Step B (see -`control_plane/docs/design.md` §3's terminology table and -`control_plane/src/sketch_algebra/physical_expr.rs`'s module docs) landed -the canonical L4 IR — `asap_sketch::{SummaryExpr, L4Node}` — and a real -L3→L4 binder (`asap_plan::bind::implement_tree_in_with`, -`control_plane::asap_tier_implement::implement_promql_for_asap_tier`) on -the **control-plane** side only. `data_plane` has never consumed either; -grepping `data_plane/src` for `asap_sketch`, `L4Node`, `SummaryExpr`, -`SummaryAgg`, `SummaryMerge`, `SummaryEstimate` returns zero hits (the -`asap_sketch*`-prefixed names it does use, e.g. `DdSketch`/`KllSketch`, -are the unrelated `asap_sketchlib` sketch-math crate). Wiring the query -path onto the tree is new integration work, not finishing something -half-connected. +## Today -## 1. What today's engine actually does +`ASAPQueryEngine::execute()` (`asap_query_engine/engine.rs:1264-1923`) has no +tree. It gets a flat `Vec` from +`analyze_promql_for_asap_tier` and loops: -The live entry point is `ASAPQueryEngine::execute()` -(`data_plane/src/query_engines/asap_query_engine/engine.rs:1264-1923`). -There is no tree anywhere in this path — the older `find_compatible_aggregation`/ -`handle_query` dispatcher is fully retired (comments only, e.g. -`engine.rs:1904-1912`) and `query_precompute_for_statistic` -(`engine.rs:602-631`) is `#[cfg(test)]`-only, unreachable from production. - -``` -PromQL string - → control_plane::asap_tier_analysis::analyze_promql_for_asap_tier(query) - → query_parser::parse_query_expr_canonical // PromQL → L3 QueryExpr - → capability_for(&AggIntent) // AggIntent → Capability, per node - → ASAPTierAnalysis { candidates: Vec, unsupported } // FLAT LIST - → for candidate in &analysis.candidates { // engine.rs:1394 - sids = idx.instances_matching(metric, group_by_keys) - sids = sids.filter(|sid| candidate.required_capability.is_satisfied_by(sid.capability)) - result = SketchReducer::evaluate_exact_agg / evaluate_for_capability(...) - combined_result = Some(result) // ← overwrites every iteration - } - → asap_tier_result_to_query_result(combined_result, ...) -``` - -`ASAPTierAnalysis`/`ASAPTierCandidate` (`control_plane/src/asap_tier_analysis.rs:67-131`) -are a flat `Vec`, each candidate a leaf-shaped unit: `metric_name`, -`group_by_keys`, `required_capability: Capability`, plus -`outer_fn`/`outer_agg` flags used to encode *some* composition (e.g. "rate -over sum") without any actual nesting. - -**This loop doesn't fold multiple candidates — it overwrites.** The -comment at `engine.rs:1367-1372` says so directly: *"Multi-candidate -aggregation is deferred (single-result shapes today)... When more than -one candidate is supported, a follow-up will fold per-candidate -ASAPTierResults."* Today's queries happen to produce ≤1 real candidate, -so this has never been exercised. **This means a real tree-walk executor -is new behavior, not a refactor of an existing fold** — there's no -existing multi-node composition semantics to be faithful to. - -### 1a. The typed capability matcher is real, and already control-plane-coupled - -`data_plane` is not independent of `control_plane` today — despite the -crate boundary, `data_plane::storage_engines::sketch_db::data::Capability` -is a direct re-export: - -```rust -// data_plane/src/storage_engines/sketch_db/data/mod.rs:59 -pub use control_plane::sketch_algebra::{Capability, SketchKindHandle}; ``` - -and the matcher call is real and load-bearing (`engine.rs:1480-1487`): - -```rust -let satisfied = idx - .with_instance(*sid, |m| { - m.capability - .as_ref() - .map(|cap| required.is_satisfied_by(cap)) - .unwrap_or(false) - }) - .unwrap_or(false); -``` - -`Capability::is_satisfied_by` (`control_plane/src/sketch_algebra/capability.rs:330-379`) -is asymmetric and encodes real cross-family equivalences: a -`MultipleSum`-indexed sid satisfies a required `Sum`; a `Sum`-indexed sid -satisfies a required `Increase`/`Rate` (via `sum_satisfies_increase`, -added this session for PR #395) but never the reverse. **Whatever "sid -catalog lookup" primitive `SummaryAgg` leaves resolve into in the new -design must call this exact function, unchanged** — it would be very easy -to accidentally re-derive a subtly different, wrong matcher while -re-plumbing this into a tree-executor's leaf-resolution step. - -### 1b. `StreamingConfig` carries no tree either - -The only wire payload `data_plane` consumes from `control_plane` is -`StreamingConfig` (`crates/asap_types/src/streaming_config.rs:58-73`): - -```rust -pub struct StreamingConfig { - pub aggregation_configs: HashMap, // keyed by PolicyFingerprint - pub storage_backend: StorageBackend, - pub monitors: Vec, -} -``` - -`AggregationConfig` is one flat struct per policy (family, params, -grouping, window, spatial filter) — used at *ingest* time to decide what -to build, and secondarily as a `policy_fp → {sids}` index optimization at -query time (`find_matching_policies`, `engine.rs:716-724`, itself a -fallback because `policy_fp` often comes back `UNSET` for real wire -sketches — issue #271/#272). No readout/estimate/merge structure rides on -the wire at all. **This confirms the design question below is real, not -hypothetical**: today, `data_plane` re-derives everything about "what to -compute" independently, per query, from the PromQL text — it has never -received a plan from `control_plane`. - -### 1c. Merge exists for exactly one family today - -Two real merge paths, not unified: - -- **ExactAgg (general, keyed, works today).** `SketchReducer::evaluate_exact_agg` - (`sketch_reducer.rs:903-1084`) groups all matching sids' accumulators by - `(projected_group_by_keys, window_end)`, folds each bucket via - `AggregateCore::merge_with` (line 1024), reads out via `query_statistic`. - Comment at `sketch_reducer.rs:1009-1015`: *"multiple entries come from - multiple sids that share the projected group (e.g. several (zone=z0, - rack=*) sids collapsing to a single zone=z0 group)."* This is the one - real, general precedent for what `SummaryMerge` needs to become. -- **HLL (special-cased, global only).** `evaluate_cardinality_global` - (`sketch_reducer.rs:428-527`) merges HLL registers register-wise across - every candidate sid, but only fires when `group_by_keys.is_empty()` - (`engine.rs:1744-1752`) — the comment at `engine.rs:1734-1743` explains - why: the per-series path would otherwise double-count or never produce - one global number. - -**For every other grouped sketch case — `quantile by (zone) (...)`, -`count by (zone) (unique_metric)`, per-group `topk`/`frequency` — there is -no cross-sid merge at all today.** `evaluate_core` -(`sketch_reducer.rs:534-857`) just pushes each sid's series into the -output with no de-dup on `(series_label_values)` — two sids projecting to -the same group silently produce two output series with identical labels, -not one merged series. **This is a real, currently-shipping bug, and -per-group sketch merge for `SummaryMerge` is new work, not a -generalization of something that already generalizes.** - -**Sketch merge is conditional on exact param match, and today's -`Capability` matcher can't see that.** `asap_sketch::SummaryExpr::SummaryMerge`'s -own doc (`crates/sketch/src/expr.rs:87-90`) is explicit: *"all inputs must -agree on `(sketch, params)` and the catalog flag `mergeable` must be -true"* — two KLL sketches only merge cleanly if they were built with the -same `k` (compaction schedule); two CMS matrices only merge if `width`/`depth` -match. `control_plane::sketch_algebra::Capability` (the type -`is_satisfied_by`-matching already runs on, §1a) is **family-level only** -— `Capability::QuantileApprox(SketchKindHandle)` etc. carry a family -selector, never a concrete `SummaryParams` value -(`control_plane/src/sketch_algebra/capability.rs:42-73`). So the existing -matcher answers "does this sid's family satisfy the requirement," not -"do these two sids' params match each other" — a real gap for -`SummaryMerge`'s child-sid selection, which needs the stricter check. -Concretely: if two sids covering the same group were registered with -different `k`/`width`/`depth` (plausible after a re-plan bumps accuracy -targets, or a rolling deploy changes catalog defaults mid-flight), -`instances_matching` + `is_satisfied_by` would happily return both as -individually-satisfying candidates, but folding them via `SummaryMerge` -would be invalid. This needs an explicit param-equality filter *within* -each `SummaryMerge`'s child set (not just per-child `Capability` -satisfaction), plus a decision for what happens to a mismatched minority -(exclude and note partial coverage? `CapabilityMiss` the whole group? -no downsampling/resize path exists in any sketch library used here today, -so "resize to match" is not on the table) — see open question 2. - -### 1d. Two structural escape hatches exist today — but the reason is narrower than it first looks - -- `try_topk_over_rate_fallback` (`engine.rs:280-537`) hand-parses the raw - `promql_parser` AST for `topk(K, sum by (gbk) (rate(metric[r])))` (or - `bottomk`/`irate`), synthesizes an `ExactAgg(Sum)` candidate for the - *inner* `sum(rate(...))`, evaluates it exactly, then **sorts and slices - the result to the top/bottom K groups in-engine, after readout**. This - runs *before* the typed candidate path and bypasses it. -- `try_rate_over_frequency_fallback` (`engine.rs:557-600`) similarly - special-cases `rate(cms_metric[r])` against a `FrequencyEstimate` sid. - -**Correction from an earlier draft of this doc:** `topk(K, expr)` does -*not* categorically fail to decompose into the L4 tree — `AggIntent::TopK` -is explicitly documented as ranking "by the *aggregate output*" -(`crates/ir/src/intent_algebra/agg_intent.rs:79-87`), i.e. it's designed -to compose with an inner aggregate, not just rank raw per-key frequency. -The real story, grounded in `control_plane/src/query_parser/promql.rs:309-345`: - -```rust -let measure = if is_count_over_time(agg.expr.as_ref()) { - RankingMeasure::Frequency -} else { - RankingMeasure::NonAdditive -}; -if is_frequency_heavy_hitter(descending, measure) { - return Ok(QueryExpr::TopK { k, by, input: Box::new(inner) }); // → AggIntent::TopK +for candidate in candidates { + sids = instances_matching(metric, group_by).filter(|sid| required.is_satisfied_by(sid.capability)); + result = evaluate_exact_agg / evaluate_for_capability(sids, ...); + combined_result = Some(result); // ← overwrites, never folds } -// otherwise: Sort { Limit { inner } } — the generic order-by-value path ``` -`AggIntent::TopK` only fires for `topk` (never `bottomk`) ranking by -`count_over_time(...)` specifically (`RankingMeasure::Frequency`) — that -shape *does* decompose today, straight into a single bindable -`SummaryAgg{CmsWithHeap/CountSketchWithHeap}` node, no fallback needed. -Every other ranking measure — including `sum by (gbk) (rate(...))`, the -shape the fallback actually handles — is `RankingMeasure::NonAdditive` -and lowers to `QueryExpr::Sort { QueryExpr::Limit { } }` -instead. The inner `Aggregate` (the `sum(rate(...))`) would itself bind -fine on its own (Step B already proved `Aggregate{Sum, child: Aggregate{Rate,...}}` -binds to a nested `SummaryAgg{Sum, child: SummaryAgg{Increase,...}}`) — -the actual blocker is that `implement_tree_in_with` only recurses through -the `Aggregate` spine (its own "conservative fallbacks" doc: a logical -parent above a bindable aggregate subsumes the whole subtree unbound), and -`Sort`/`Limit` aren't `Aggregate` — so `Sort{Limit{Aggregate{...}}}` stops -at the outer `Sort` and the *whole thing* falls to one opaque `Logical` -node, never exposing the inner `Aggregate` to be bound at all. - -**This means the fallback isn't necessarily permanent, forced-external -special-casing** — it's the same shape of gap Step B's `lower.rs` -pre-passes already close for other cases (Count{Exact}, Rate). A -control-plane-side pre-pass recognizing `Sort{Limit{Aggregate{...}}}` -could strip the `Sort`/`Limit` wrapper, recurse `implement_tree_in_with` -into the inner `Aggregate` to get a real `SummaryAgg`/`SummaryEstimate` -node, and carry the stripped rank/limit info *alongside* the tree (not as -a new `SummaryExpr` variant — ranking-and-slicing a readout is an -execution-time/L5 concern, not an L4 "what sketch to build" concern) for -`data_plane`'s executor to apply after evaluating that subtree. Whether -that pre-pass is worth building now or the raw-AST fallback should stay -as-is a while longer is an open call — see open question 3 — but the -reasoning for *why* is now precise instead of "these are fundamentally -incompatible," which was wrong. - -### 1e. Other things any executor rewrite must reproduce exactly - -- **Keyed-CMS safe-miss gate** (`engine.rs:1549-1626`): a - `FrequencyEstimate` sid answers only bucket totals; a keyed selector - like `cms_metric{item="X"}` must `CapabilityMiss` to archive *unless* - the sid is registered in `item_label` mode and the filter resolves to - that label. Skipping this returns a *wrong answer*, not just a miss — - correctness-critical, must be preserved verbatim. -- **Coverage-aware archive stitching** (`stitch_warm_and_archive`, - `engine.rs:1136-1190`): compares `ASAPTierResult.coverage` against the - requested window and merges with an archive-engine answer by - `(labels, timestamp)` when the warm tier only partially covers it. This - is a cross-cutting concern layered *around* whatever answers the query, - not encodable in the L4 IR — it belongs at the tree root only, once, - not per node (an N-way per-node stitch is not what this needs to - become). -- **`instances_matching` + capability-check is duplicated at ~4 call - sites** in `engine.rs` today rather than being one function — worth - consolidating into the single reusable primitive `SummaryAgg` leaves - would call, rather than duplicating it a 5th time. -- `EngineRouter`/`BackendStorageRouting` - (`data_plane/src/query_engines/routing/`) sit one layer above all of - this and are orthogonal — they decide *which whole engine* (warm sketch - tier vs. archive tier) answers a query, never *which node inside* it. - The only contract the new executor must keep is `execute()`'s existing - `Err(EngineError::CapabilityMiss(...))` signaling (used at ~7 sites - today, each firing `spawn_capability_miss_notify`), so archive failover - keeps working unchanged. - -## 2. Proposed design - -### 2a. Where the tree comes from - -`data_plane` builds its own `Rc` tree at query time, mirroring -Step A's `implement_promql_for_asap_tier` rather than waiting for -`control_plane` to ship one over the wire. Rationale: `data_plane` -already independently re-parses PromQL and independently derives -`AggIntent`s today (§1); it is *already* the pattern in this codebase -that the query-time IR is re-derived locally rather than carried on -`StreamingConfig` (which is, and stays, ingest-time-only — see §1b). Two -concrete integration options, need a decision before implementation: - -1. `data_plane` takes a direct dependency on `asap_plan`/`asap_sketch` - (already a `control_plane` git dependency; would become a `data_plane` - one too) and calls `asap_plan::bind::implement_tree_in_with` itself, - passing its own `CostModel` (mirroring - `control_plane::sketch_algebra::cost_model::ControlPlaneCostModel`, or - literally reusing it if `data_plane` is willing to depend on - `control_plane` for it the way it already does for `Capability`). -2. `data_plane` calls into a new `control_plane`-side function (e.g. - exposing `implement_promql_for_asap_tier` or an equivalent) rather - than depending on `asap_plan` directly, keeping `asap_plan`/`asap_sketch` - coupling confined to `control_plane`, consistent with `data_plane`'s - existing pattern of depending on `control_plane::sketch_algebra` - types (`Capability`) rather than reaching past it to `asap_sketch` - itself. - -Leaning toward (2) — it matches the existing `Capability` re-export -pattern (§1a) and keeps `asap_plan`/`asap_sketch` as a `control_plane`-only -upstream coupling, one seam instead of two — but this needs to be decided -before writing code, not discovered mid-implementation. - -### 2b. The recursive walk +Four real gaps this leaves: + +1. **No fold.** The loop overwrites instead of combining — fine today because + real queries only ever produce ≤1 candidate, but there's no existing + multi-node behavior to be "faithful to" once that's no longer true. +2. **Merge only exists for `ExactAgg`.** `evaluate_exact_agg` folds sids by + `(group, window)` via `AggregateCore::merge_with` — the one real + precedent. HLL has a global-only special case. Every other grouped sketch + case (`quantile by (zone) (...)`, per-group `topk`/`frequency`) silently + emits duplicate un-merged series today when two sids project to the same + group — a live bug, not a gap in a working feature. +3. **Sketch merge needs exact param match, and nothing checks that today.** + `SummaryMerge` requires every child to agree on `(SummaryKind, + SummaryParams)`, not just family — but `Capability` (what + `is_satisfied_by` matches on) is family-level only. `AccumulatorSpec` + (`asap_types::accumulator_spec`, landed) now carries the params + `data_plane` needs for this check; nothing wires it into merge-candidate + selection yet. +4. **Two raw-AST fallbacks exist** (`try_topk_over_rate_fallback`, + `try_rate_over_frequency_fallback`) for compositions the flat `Capability` + vocabulary can't express. The first is `topk(K, sum by(gbk)(rate(m[r])))` + — PromQL classifies this as `RankingMeasure::NonAdditive` (only + `topk(K, count_over_time(...))` maps to the real `AggIntent::TopK` + sketch), which lowers to `QueryExpr::Sort{Limit{Aggregate}}`. + `implement_tree_in_with` only recurses through `Aggregate`, so this whole + shape stays one opaque `Logical` blob today even though the inner + aggregate would bind fine on its own. + +The canonical tree (`asap_sketch::{SummaryExpr, L4Node}`) already exists and +is built control-plane-side (`implement_promql_for_asap_tier`, Step A/B) — +`data_plane` has never consumed it. + +## Proposed executor ```rust -fn execute_node(node: &L4Node, window: TimeRange, ctx: &ExecCtx) - -> Result -{ +fn execute_node(node: &L4Node, window: TimeRange, ctx: &ExecCtx) -> Result { match &node.expr { - SummaryExpr::SummaryAgg { sketch, params, col, by, .. } => { - // LEAF: one consolidated sid-catalog-lookup primitive, - // replacing the ~4 duplicated call sites in engine.rs today. - let required = capability_from_summary_kind(sketch, params); - let sids = ctx.index - .instances_matching(&ctx.metric, by) - .filter(|sid| ctx.index.with_instance(*sid, |m| - m.capability.as_ref().is_some_and(|cap| required.is_satisfied_by(cap)) - ).unwrap_or(false)); - ExecResult::SketchState { sids: sids.collect(), by: by.clone() } + SummaryAgg { sketch, params, by, .. } => { + // one consolidated sid lookup, replacing ~4 duplicated call sites + let sids = ctx.index.instances_matching(&ctx.metric, by) + .filter(|s| required_capability(sketch, params).is_satisfied_by(s.capability)); + ExecResult::SketchState { sids, by: by.clone() } } - SummaryExpr::SummaryEstimate { sketch_input, query } => { - let state = execute_node(sketch_input, window, ctx)?; // recurse - readout(state, query) // quantile / cardinality / point_count / topk + SummaryEstimate { sketch_input, query } => { + readout(execute_node(sketch_input, window, ctx)?, query) } - SummaryExpr::SummaryMerge { children } => { - let states: Vec<_> = children.iter() - .map(|c| execute_node(c, window, ctx)) - .collect::>()?; - // generalizes evaluate_exact_agg's (group, window) fold (§1c) - // to every SummaryKind, not just ExactAgg — but unlike ExactAgg - // accumulators, sketch families require exact (kind, params) - // agreement to merge at all (§1c's mergeable-params landmine); - // this must filter/partition each group's candidate sids on - // params equality *before* folding, not just Capability - // satisfaction. - merge_by_group(states) - } - SummaryExpr::Logical(qe) => { - // No summary committed for this subtree — same meaning as - // today's "no candidate bound": CapabilityMiss, let - // EngineRouter fail over to archive. - Err(EngineError::CapabilityMiss(/* ... */)) - } - SummaryExpr::SummaryJoin { .. } - | SummaryExpr::SummarySubtract { .. } - | SummaryExpr::SummaryDelete { .. } => { - // No Bind* path produces these yet (see physical_expr.rs's - // module docs) — unreachable in practice today. - unimplemented!("not yet surfaced by any binder") + SummaryMerge { children } => { + let states = children.iter().map(|c| execute_node(c, window, ctx)).collect::,_>>()?; + merge_by_group(states) // must filter to exact (kind, params) match per group first } + Logical(_) => Err(EngineError::CapabilityMiss(..)), // same meaning as "no candidate bound" today + SummaryJoin | SummarySubtract | SummaryDelete => unimplemented!(), // no Bind* produces these yet } } ``` -`execute()` calls `execute_node` once at the tree root, then applies the -coverage-aware archive stitch (§1e) once on the final result — not -per-node. - -### 2c. What's genuinely new work vs. what's porting +`execute()` calls this once at the root, then applies the existing +coverage-aware archive stitch once on the result (not per-node). -| Piece | Status | +| Piece | New or porting | |---|---| -| `SummaryAgg` leaf → sid lookup + capability check | Porting — consolidates 4 duplicated call sites, must call `is_satisfied_by` unchanged (§1a) | -| `SummaryEstimate` → readout | Porting — `evaluate_core`'s per-family decode blocks (quantile/cardinality/frequency/topk) already exist, just need re-homing under the new dispatch | -| `SummaryMerge` for `ExactAgg`-family kinds | Porting — `evaluate_exact_agg`'s fold-by-`(group,window)` already does this | -| `SummaryMerge` for sketch families (KLL/DDSketch/CMS/CountSketch by group) | **New** — doesn't exist today outside the global-only HLL special case (§1c), and needs a param-equality filter `Capability` matching alone doesn't provide (§1c) | -| Keyed-CMS safe-miss gate | Porting — must be preserved verbatim inside the `SummaryEstimate`/`FrequencyEstimate` readout path | -| Coverage-aware archive stitch | Porting — moves from per-query to tree-root-only, same logic | -| `topk(K, sum(...))` (`RankingMeasure::NonAdditive`) / `rate(cms(...))` post-processing | Not tree-shaped under today's `implement_tree_in_with` (stops at the outer `Sort`/`Limit`), but potentially *becomes* tree-shaped down to a thin rank/limit wrapper with a control-plane-side pre-pass — see the corrected §1d and open question 3 | - -## 3. Open questions (need a decision before/while implementing) - -1. **Tree-source integration (§2a)**: does `data_plane` depend on - `asap_plan`/`asap_sketch` directly, or call through a `control_plane` - seam? Leaning (2) above but unconfirmed. -2. **`SummaryMerge` semantics for sketch families**: `evaluate_exact_agg`'s - `(group, window)` bucketing is the closest precedent, but it needs a - param-equality filter on top that `Capability` alone can't provide - (§1c) — `control_plane::sketch_algebra::Capability` is family-level - only (`SketchKindHandle`, no `SummaryParams`), so `SummaryMerge`'s - child-sid selection needs a *stricter* check than - `instances_matching` + `is_satisfied_by` already gives the plain - `SummaryAgg` leaf case: exact `(SummaryKind, SummaryParams)` agreement - across every sid folded into one merge. Open sub-questions: what - happens to a sid that matches on `Capability` but not on params (drop - it and report partial coverage? `CapabilityMiss` the whole group? no - resize/downsample path exists in any sketch library used here today); - does merging change the `SummaryEstimate`'s accuracy math (a merged - multi-sid sketch can have different error bounds than a single-sid - one — is that tracked anywhere, or a new gap); and does the same - `(group, window)` bucketing generalize cleanly to every sketch family - `data_plane` needs to merge here, or do some (e.g. CMS-with-heap, where - merging matrices is well-defined but merging *heaps* isn't the same - operation) need family-specific merge logic beyond a generic fold? -3. **Where does "rank-and-slice a readout" live?** Corrected in §1d: - `topk`/`bottomk` ranking by a non-`count_over_time` measure (the shape - `try_topk_over_rate_fallback` handles) lowers to `QueryExpr::Sort { - QueryExpr::Limit { } }`, which `implement_tree_in_with` - doesn't decompose today (stops at the outer `Sort`, whole subtree - stays `Logical`) — but the inner `Aggregate` would bind fine on its - own if reached. Options: (a) leave the raw-AST fallback exactly as-is, - `data_plane`-side, unchanged; (b) add a `control_plane`-side pre-pass - (same shape as Step B's `lower.rs` pre-passes) that strips `Sort`/ - `Limit`, binds the inner `Aggregate` for real, and carries the - rank/limit as metadata *alongside* the resulting `L4Node` (not a new - `SummaryExpr` variant — this is an L5/execution-time concern, not an - L4 "what to build" concern) for `data_plane`'s executor to apply after - evaluating that subtree; (c) something upstream in `asap_plan` - (unlikely — this is deployment-specific PromQL-surface shape - recognition, the same category of thing Step B's `lower.rs` - pre-passes handle locally rather than pushing upstream). (b) is more - work than (a) but turns a permanent raw-AST special case into a real, - general "any `Sort{Limit{...}}}`-wrapped bindable aggregate" capability - — worth deciding deliberately rather than defaulting to (a) just - because it's less work right now. -4. **Rollout**: land behind a parallel path (mirroring - `USE_TYPED_STAGE_SPLIT`/`USE_TYPED_SKETCH_ALGEBRA`'s env-var-gated - parallel-path pattern from Step A/B) so the new executor can be - compared against the existing flat dispatcher before it becomes the - only path, or replace `execute()`'s loop directly once confident? - Given §1c/§1d's gaps are real, currently-shipping-behavior questions - (not just refactor risk), a parallel/comparable rollout seems safer - than a hard cutover, but this changes the shape of the work - noticeably (need to keep both paths correct simultaneously for a - while) and should be confirmed before starting. +| `SummaryAgg` → sid lookup | Porting (consolidation) | +| `SummaryEstimate` → readout | Porting (`evaluate_core`'s decode blocks already exist) | +| `SummaryMerge` for `ExactAgg` | Porting (`evaluate_exact_agg`'s fold) | +| `SummaryMerge` for sketch families | **New** — plus the param-match filter (gap 3) | +| `topk`-over-non-frequency-measure | Not tree-shaped today; a control-plane pre-pass could make the inner aggregate bindable (see open questions) | + +## Open questions + +1. **Tree source**: does `data_plane` call `asap_plan::bind::implement_tree_in_with` + directly, or through a `control_plane`-side seam (matching how it already + depends on `control_plane::sketch_algebra::Capability` rather than + `asap_sketch` directly)? Leaning toward the seam. +2. **`SummaryMerge` param-match**: use `AccumulatorSpec` to filter each + group's candidate sids to exact `(kind, params)` agreement before + folding. What happens to a sid that satisfies `Capability` but not + params — drop it (partial coverage) or miss the whole group? No + resize/downsample path exists for any sketch family here. +3. **`topk`/`rate` post-processing**: keep the raw-AST fallbacks as-is, or + add a `control_plane`-side pre-pass that strips `Sort{Limit{...}}`, + binds the inner aggregate for real, and carries rank/limit as metadata + alongside the `L4Node` for the executor to apply after evaluation? +4. **Rollout**: parallel path behind an env-var gate (matching + `USE_TYPED_STAGE_SPLIT`), or replace `execute()`'s loop directly? From 443dd73026d3a35925a59b5012e02cdbdab0eed7 Mon Sep 17 00:00:00 2001 From: zz_y Date: Wed, 22 Jul 2026 11:20:33 -0600 Subject: [PATCH 3/6] docs(data_plane): adopt ASAPController's SummaryExecutor interface ASAPController#155 landed SummaryExecutor + execute() -- the generic recursive walk over L4Node, with the merge/nesting structural rules enforced upstream instead of reinvented here. Rewrites the "proposed executor" section around implementing that trait (Handle=sid, find_candidates via AccumulatorSpec-based exact param matching, merge_states via asap_sketchlib per family) instead of a bespoke pseudocode walk, and adds a section on which nested shapes actually occur in this deployment's three-stage topology (SummaryAgg-of- SummaryAgg, SummaryMerge-of-SummaryMerge, and the still-open SummaryAgg-over-SummaryEstimate question). Co-Authored-By: Claude Sonnet 5 --- .../docs/l4node-plan-executor-design.md | 100 +++++++++++------- 1 file changed, 63 insertions(+), 37 deletions(-) diff --git a/data_plane/docs/l4node-plan-executor-design.md b/data_plane/docs/l4node-plan-executor-design.md index b7a73699..be4e76a7 100644 --- a/data_plane/docs/l4node-plan-executor-design.md +++ b/data_plane/docs/l4node-plan-executor-design.md @@ -50,52 +50,78 @@ The canonical tree (`asap_sketch::{SummaryExpr, L4Node}`) already exists and is built control-plane-side (`implement_promql_for_asap_tier`, Step A/B) — `data_plane` has never consumed it. -## Proposed executor - -```rust -fn execute_node(node: &L4Node, window: TimeRange, ctx: &ExecCtx) -> Result { - match &node.expr { - SummaryAgg { sketch, params, by, .. } => { - // one consolidated sid lookup, replacing ~4 duplicated call sites - let sids = ctx.index.instances_matching(&ctx.metric, by) - .filter(|s| required_capability(sketch, params).is_satisfied_by(s.capability)); - ExecResult::SketchState { sids, by: by.clone() } - } - SummaryEstimate { sketch_input, query } => { - readout(execute_node(sketch_input, window, ctx)?, query) - } - SummaryMerge { children } => { - let states = children.iter().map(|c| execute_node(c, window, ctx)).collect::,_>>()?; - merge_by_group(states) // must filter to exact (kind, params) match per group first - } - Logical(_) => Err(EngineError::CapabilityMiss(..)), // same meaning as "no candidate bound" today - SummaryJoin | SummarySubtract | SummaryDelete => unimplemented!(), // no Bind* produces these yet - } -} -``` +## The executor is now a common interface, not a bespoke walk -`execute()` calls this once at the root, then applies the existing -coverage-aware archive stitch once on the result (not per-node). +ASAPController#155 (`crates/sketch/src/exec.rs`) adds `SummaryExecutor` + +`execute()` upstream: a deployment implements the trait, `asap-sketch` +does the recursive walk and enforces the structural rules generically +(which nestings are valid, that `SummaryMerge` children must agree on +`(SummaryKind, SummaryParams)`, propagated correctly through arbitrary +nesting depth). See `docs/l4node-execution-model.md` in ASAPController for +the full design; this doc only covers what's specific to `data_plane`. -| Piece | New or porting | +`data_plane`'s job is one `impl SummaryExecutor for ASAPQueryEngine` (or a +small wrapper around it): + +| `SummaryExecutor` method | `data_plane` implementation | |---|---| -| `SummaryAgg` → sid lookup | Porting (consolidation) | -| `SummaryEstimate` → readout | Porting (`evaluate_core`'s decode blocks already exist) | -| `SummaryMerge` for `ExactAgg` | Porting (`evaluate_exact_agg`'s fold) | -| `SummaryMerge` for sketch families | **New** — plus the param-match filter (gap 3) | -| `topk`-over-non-frequency-measure | Not tree-shaped today; a control-plane pre-pass could make the inner aggregate bindable (see open questions) | +| `Handle` | `u64` (sid) | +| `State` | decoded sketch bytes / `Box`, per family | +| `find_candidates(sketch, params, col, by, child)` | walk `child` to recover the metric (same shape as today's `extract_edge_facts`), then `instances_matching(metric, by)` filtered to sids whose `AccumulatorSpec` is exactly `(sketch, params)` — **not** the looser family-only `Capability::is_satisfied_by` check `data_plane` uses today; `AccumulatorSpec` (landed) is what makes the exact check possible | +| `merge_states` | `AggregateCore::merge_with` for `ExactAgg` kinds; per-family `asap_sketchlib` merge for sketch kinds (KLL/DDSketch/CMS/HLL) — the part that's genuinely new (gap 2) | +| `readout` | `evaluate_core`'s existing per-family decode blocks, re-homed | +| `logical` | `Err(CapabilityMiss)` — same meaning as "no candidate bound" today, lets `EngineRouter` fail over to archive | + +Because `find_candidates` is now contractually required to return only +exact-`(kind, params)` matches (`asap-sketch`'s trait doc), gap 3 (nothing +checks param agreement today) is resolved by construction for anything +routed through `execute()` — `data_plane` no longer needs its own +merge-precondition check, `asap-sketch`'s does it for `SummaryMerge` +children, and `find_candidates`'s contract does it for a single +`SummaryAgg`'s candidate set. + +## Nested queries in this deployment's topology + +`execute()`'s recursion handles arbitrary nesting depth already (see the +ASAPController doc) — the deployment-specific question is what nesting +actually *occurs* in a three-stage (edge/gateway/backend) topology: + +- **`SummaryAgg`-of-`SummaryAgg`** (`quantile(0.9, sum by (job) (m))`): + edge builds per-job sums; backend's KLL is built over that sum stream. + Already a valid, tested shape upstream — no new work here beyond + `find_candidates` correctly walking past the inner `SummaryAgg` to find + the metric. +- **`SummaryMerge`-of-`SummaryMerge`**: a plausible real shape once + merging isn't `ExactAgg`-only — e.g. per-zone edge sketches merge at a + regional gateway, regional merges merge again at the backend. `execute()` + already handles this (the `(kind, params)` check is transitive), so this + is a rollout/topology question, not a design gap: does this deployment's + stage allocator ever actually *produce* a two-level merge cascade today, + or does everything currently collapse to one gateway hop? Worth checking + against `physical/colored_dag`'s stage-allocation output before assuming + the two-level case needs dedicated testing. +- **`SummaryAgg`-over-`SummaryEstimate`**: flagged upstream as an open, + unresolved question (building a new summary from another summary's + query-time readout). `data_plane`'s `SketchStore` only ever serves + summaries built at ingest time from raw samples — if `find_candidates` + ever receives a `child` whose subtree bottoms out in a `SummaryEstimate` + rather than raw `Logical`/`SummaryAgg`, that's this shape, and the + right answer today is `Err` (unsupported), not a guess. -## Open questions +## Remaining open questions 1. **Tree source**: does `data_plane` call `asap_plan::bind::implement_tree_in_with` directly, or through a `control_plane`-side seam (matching how it already depends on `control_plane::sketch_algebra::Capability` rather than `asap_sketch` directly)? Leaning toward the seam. -2. **`SummaryMerge` param-match**: use `AccumulatorSpec` to filter each - group's candidate sids to exact `(kind, params)` agreement before - folding. What happens to a sid that satisfies `Capability` but not - params — drop it (partial coverage) or miss the whole group? No - resize/downsample path exists for any sketch family here. +2. **`SummaryMerge` for sketch families**: per-family merge (KLL/DDSketch/ + CMS/HLL) via `asap_sketchlib` is genuinely new work, not a port. Does + merging change a `SummaryEstimate`'s accuracy math (a merged multi-sid + sketch can have different error bounds than a single-sid one)? What + happens when `find_candidates` can't find *any* sid with exactly + matching params for part of a group — drop it (partial coverage) or + miss the whole group? No resize/downsample path exists for any sketch + family here. 3. **`topk`/`rate` post-processing**: keep the raw-AST fallbacks as-is, or add a `control_plane`-side pre-pass that strips `Sort{Limit{...}}`, binds the inner aggregate for real, and carries rank/limit as metadata From ba7135affbcd5c848182b6bb57c882aeb0a79740 Mon Sep 17 00:00:00 2001 From: zz_y Date: Sat, 25 Jul 2026 09:50:18 -0600 Subject: [PATCH 4/6] docs: sync Step C design doc with what #411 actually implemented The design table and open questions described the target shape before implementation started; several turned out to differ once real code got written. Updates: - Status note: SummaryExecutor is implemented and tested for quantile/cardinality (DDSketch/Kll/Hll), both cumulative and per-window, in #411 (unmerged). Frequency family (CMS/CountSketch) in progress; ExactAgg readout and wiring execute() into the live serving path still not started. - Table: Handle is SidHandle (sid + already-fetched series + decode params), not a bare u64 -- find_candidates needs the series anyway for the group key, so it's threaded through rather than re-fetched. merge_states/fetch_state are deliberately lazy; the real merge math lives in readout, which is the only place that knows cumulative-vs-per-window mode. - Open question 1 (tree source): resolved as both -- the control_plane seam for planning-time tree construction, plus a direct asap-sketch/asap-ir dependency for everything serving-time (implementing the trait / matching its types requires importing the defining crate directly, not just consuming a function that returns those types). - Open question 2: partially resolved -- KLL/DDSketch/HLL merge done (via a generalized existing HLL-only primitive, not built from scratch), CMS/CountSketch still open. Partial-group-coverage question resolved (fold whatever's present). Accuracy-math and resize/downsample questions remain genuinely open. - Questions 3-4 unchanged -- neither addressed yet. Co-Authored-By: Claude Sonnet 5 --- .../docs/l4node-plan-executor-design.md | 89 +++++++++++++------ 1 file changed, 62 insertions(+), 27 deletions(-) diff --git a/data_plane/docs/l4node-plan-executor-design.md b/data_plane/docs/l4node-plan-executor-design.md index be4e76a7..327a0ff4 100644 --- a/data_plane/docs/l4node-plan-executor-design.md +++ b/data_plane/docs/l4node-plan-executor-design.md @@ -2,7 +2,18 @@ Planning doc, not an implementation — same spirit as [`sketchindex-sid-unification-plan.md`](./sketchindex-sid-unification-plan.md). -Status: not started. + +> **Status (2026-07-25).** `impl SummaryExecutor for QueryExecutionContext` +> is implemented and tested (#411, unmerged pending review) for +> quantile/cardinality over the DDSketch/Kll/Hll families, both cumulative +> (instant) and per-window (matrix/range) queries. Real cross-sid merging +> is wired in for both modes. Not yet covered: the Frequency family +> (`TopK`/`PointCount`, i.e. CMS/CountSketch — in progress) and `ExactAgg` +> readout (`Sum`/`Rate`/`Increase`/`MinMax`/exact `Count`, which never +> reaches `SummaryExecutor::readout` at all — see below). `execute()` +> isn't wired into `ASAPQueryEngine::execute()`'s live serving path yet; +> the sections below describe the target design, some of which turned out +> to differ from what actually got built — see the inline corrections. ## Today @@ -60,17 +71,32 @@ does the recursive walk and enforces the structural rules generically nesting depth). See `docs/l4node-execution-model.md` in ASAPController for the full design; this doc only covers what's specific to `data_plane`. -`data_plane`'s job is one `impl SummaryExecutor for ASAPQueryEngine` (or a -small wrapper around it): +`data_plane`'s implementation (`summary_executor.rs`) is +`impl SummaryExecutor for QueryExecutionContext`, a small per-query +context — not `ASAPQueryEngine` directly, since the trait carries no +time-range parameter and `ASAPQueryEngine` is called concurrently; +`QueryExecutionContext` is constructed fresh per query with `t0_ms`/ +`t1_ms`/`is_cumulative` as plain fields: | `SummaryExecutor` method | `data_plane` implementation | |---|---| -| `Handle` | `u64` (sid) | -| `State` | decoded sketch bytes / `Box`, per family | -| `find_candidates(sketch, params, col, by, child)` | walk `child` to recover the metric (same shape as today's `extract_edge_facts`), then `instances_matching(metric, by)` filtered to sids whose `AccumulatorSpec` is exactly `(sketch, params)` — **not** the looser family-only `Capability::is_satisfied_by` check `data_plane` uses today; `AccumulatorSpec` (landed) is what makes the exact check possible | -| `merge_states` | `AggregateCore::merge_with` for `ExactAgg` kinds; per-family `asap_sketchlib` merge for sketch kinds (KLL/DDSketch/CMS/HLL) — the part that's genuinely new (gap 2) | -| `readout` | `evaluate_core`'s existing per-family decode blocks, re-homed | -| `logical` | `Err(CapabilityMiss)` — same meaning as "no candidate bound" today, lets `EngineRouter` fail over to archive | +| `Handle` | `SidHandle` — a sid plus its already-fetched `Rc` and decode params, not a bare `u64`. `find_candidates` needs to fetch the series anyway (to read label values for the group key), so the handle carries it forward instead of `fetch_state`/`readout` re-fetching the same `(sid, t0, t1)` range a second time. | +| `GroupKey` | `BTreeMap` — the sid's own label values projected onto the query's `by` columns. | +| `State` | `GroupState` — the group's accumulated `SidHandle`s plus the shared decode kind. Decode/merge is deliberately lazy: `fetch_state`/`merge_states` just assemble the candidate list; the actual `RollingState` reconstruction and merge happens in `readout`, which is where cumulative-vs-per-window mode is known. | +| `find_candidates(sketch, params, col, by, child)` | walks `child` down to a `Scan{source: Source::TimeSeries{metric}, ..}` to recover the metric, then `instances_matching(metric, by)` filtered to sids whose `(SketchKindHandle, SketchConfig)` is exactly `(sketch, params)` — not the looser family-only `Capability::is_satisfied_by` check the legacy analyzer path uses. | +| `merge_states` | Lazy — see `State` above. | +| `readout` | Real cross-sid merge via `delta_apply::cumulative_rolling_state`/`per_window_rolling_states` + `RollingState::merge_same_family`, covering the DDSketch/Kll/Hll families. CMS/CountSketch (Frequency family) and `ExactAgg` are not covered — see the status note above. | +| `logical` | `Err(SummaryExecutorError::Logical)` — same meaning as "no candidate bound" today, lets `EngineRouter` fail over to archive. | + +`AggregateCore::merge_with`/per-family `asap_sketchlib` merge turned out to +be two genuinely separate things, not one shared mechanism: `ExactAgg` +readout never reaches `SummaryExecutor::readout` at all (`asap_plan::bind` +never wraps an `ExactAccumulator` implementation in a `SummaryEstimate`, +so `execute()` on such a tree returns `ExecOutcome::State` at the root — +a caller-side concern, not something this trait implementation handles), +while the sketch-family merge (gap 2) is what's actually implemented here, +via `RollingState::merge_same_family` rather than `asap_sketchlib` calls +made directly in this module. Because `find_candidates` is now contractually required to return only exact-`(kind, params)` matches (`asap-sketch`'s trait doc), gap 3 (nothing @@ -110,21 +136,30 @@ actually *occurs* in a three-stage (edge/gateway/backend) topology: ## Remaining open questions -1. **Tree source**: does `data_plane` call `asap_plan::bind::implement_tree_in_with` - directly, or through a `control_plane`-side seam (matching how it already - depends on `control_plane::sketch_algebra::Capability` rather than - `asap_sketch` directly)? Leaning toward the seam. -2. **`SummaryMerge` for sketch families**: per-family merge (KLL/DDSketch/ - CMS/HLL) via `asap_sketchlib` is genuinely new work, not a port. Does - merging change a `SummaryEstimate`'s accuracy math (a merged multi-sid - sketch can have different error bounds than a single-sid one)? What - happens when `find_candidates` can't find *any* sid with exactly - matching params for part of a group — drop it (partial coverage) or - miss the whole group? No resize/downsample path exists for any sketch - family here. -3. **`topk`/`rate` post-processing**: keep the raw-AST fallbacks as-is, or - add a `control_plane`-side pre-pass that strips `Sort{Limit{...}}`, - binds the inner aggregate for real, and carries rank/limit as metadata - alongside the `L4Node` for the executor to apply after evaluation? -4. **Rollout**: parallel path behind an env-var gate (matching - `USE_TYPED_STAGE_SPLIT`), or replace `execute()`'s loop directly? +1. **Tree source — resolved, and it's both.** `implement_promql_for_asap_tier` + (Step A) is the seam for planning-time tree construction, as leaned + toward. But `data_plane` turned out to need a *direct* `asap-sketch` + (and `asap-ir`) dependency too, pinned to match `control_plane`'s exactly + — implementing `SummaryExecutor` and matching `L4Node`/`SketchQuery` + variants requires importing their defining crate directly; consuming a + function that merely returns those types isn't enough for Rust's trait/ + pattern-matching rules. So this isn't purely "through the seam" as + originally envisioned — it's the seam for tree construction, plus a + direct dependency for everything serving-time. +2. **`SummaryMerge` for sketch families — partially resolved.** + KLL/DDSketch/HLL merge is implemented via `RollingState::merge_same_family` + (generalized from an existing HLL-only global-cardinality special case, + not built from scratch against raw `asap_sketchlib` calls). CMS/CountSketch + still open (Frequency family, in progress as a follow-up). Partial + coverage — a group missing a sid for one part — is resolved as "fold + whatever's present," not "miss the whole group" (mirrors + `SummaryMerge`'s own semantics, ASAPController#159/#161). Accuracy-math + and resize/downsample questions remain genuinely open, not yet + investigated. +3. **`topk`/`rate` post-processing**: still open, unrelated to what's been + built so far — the Frequency-family follow-up doesn't address the + `try_topk_over_rate_fallback`/`try_rate_over_frequency_fallback` raw-AST + paths in `engine.rs`. +4. **Rollout**: still open. `execute()` isn't wired into `ASAPQueryEngine`'s + live serving path yet at all — this question doesn't arise until that + wiring is attempted. From 12f937bbd9f2e555a98772fd7b84dd811665d212 Mon Sep 17 00:00:00 2001 From: zz_y Date: Mon, 27 Jul 2026 09:14:52 -0600 Subject: [PATCH 5/6] docs: sync Step C design doc with #416/#417/#418 + shadow-mode rollout plan Rebased onto current main (was 27 commits behind, predating #416/#417/#418) and updated the doc's content to match: - Status note: SummaryExecutor now covers the Frequency family and ExactAgg(Sum/Increase) candidate matching + coverage tracking, not just quantile/cardinality -- these landed since the doc was last synced. - "Today"/gap list: gap 2 (merge only existed for ExactAgg) and gap 3 (nothing checked param agreement) are now resolved on the new path, marked accordingly rather than left as open problems. - SidHandle/GroupState table entries: updated to describe the actual enum shape (Sketch/ExactAgg variants), not the original sketch-only struct design. - New "Architecture reference" section citing ASAPController design.md's "Serving-time execution" section directly -- the planning-vs-serving split this doc's Rollout section builds on. - "Rollout" section: replaces "still open" with an actual plan. Corrects a real error in the previous version -- it named `implement_promql_for_asap_tier` as "the seam" for tree construction, but that function uses the naive DefaultCostModel and has a documented, tracked gap where it can't realize the Frequency intent at all. The correct seam is `sketch_algebra::lower::bind_query_expr` (ControlPlaneCostModel, what main.rs's real production pipeline uses). Also documents why rate()/topk-over-rate/outer-agg-fold must be excluded from the shadow comparison entirely (not just deprioritized) and why shadow mode -- not a cutover -- is this phase's actual scope. Co-Authored-By: Claude Sonnet 5 --- .../docs/l4node-plan-executor-design.md | 261 ++++++++++++------ 1 file changed, 183 insertions(+), 78 deletions(-) diff --git a/data_plane/docs/l4node-plan-executor-design.md b/data_plane/docs/l4node-plan-executor-design.md index 327a0ff4..4dc54511 100644 --- a/data_plane/docs/l4node-plan-executor-design.md +++ b/data_plane/docs/l4node-plan-executor-design.md @@ -3,17 +3,62 @@ Planning doc, not an implementation — same spirit as [`sketchindex-sid-unification-plan.md`](./sketchindex-sid-unification-plan.md). -> **Status (2026-07-25).** `impl SummaryExecutor for QueryExecutionContext` -> is implemented and tested (#411, unmerged pending review) for -> quantile/cardinality over the DDSketch/Kll/Hll families, both cumulative -> (instant) and per-window (matrix/range) queries. Real cross-sid merging -> is wired in for both modes. Not yet covered: the Frequency family -> (`TopK`/`PointCount`, i.e. CMS/CountSketch — in progress) and `ExactAgg` -> readout (`Sum`/`Rate`/`Increase`/`MinMax`/exact `Count`, which never -> reaches `SummaryExecutor::readout` at all — see below). `execute()` -> isn't wired into `ASAPQueryEngine::execute()`'s live serving path yet; -> the sections below describe the target design, some of which turned out -> to differ from what actually got built — see the inline corrections. +> **Status (2026-07-27).** `impl SummaryExecutor for QueryExecutionContext` +> (`summary_executor.rs`) now covers quantile/cardinality (DDSketch/Kll/Hll), +> the Frequency family (CMS/CountSketch/CmsWithHeap/CountSketchWithHeap — +> bare total, per-item point lookup, top-k), both cumulative (instant) and +> per-window (matrix/range) queries, real cross-sid merging for all of the +> above, per-group coverage tracking (`SummaryValue::coverage`), and +> `AggKind::ExactAgg` candidate matching for `Sum`/`Increase` (#411, #412, +> #414, #415, #417 — all merged). `ExactAgg` readout still never reaches +> `SummaryExecutor::readout`/`SketchQuery` at all (unchanged from the +> original design below) — `GroupState::exact_value` is the accessor a +> caller uses instead. `MinMax`/`Count`/`Rate` are deliberately NOT matched +> for `ExactAgg` (no direction info survives to `AggKind::ExactAgg`'s +> metadata for `MinMax`; no `AggregationType` resolves to `Count`/`Rate` +> today) — see `summary_executor.rs`'s own module doc and +> `exact_agg_kind_match`. +> +> `execute()` is **still not wired into `ASAPQueryEngine::execute()`'s live +> serving path** — see "Rollout" below, which now has a concrete plan +> (shadow mode) rather than being an open question. The sections below +> describe the target design; some turned out to differ from what actually +> got built — see the inline corrections. + +## Architecture reference: planning vs. serving-time execution + +This doc is the `data_plane`-specific instance of what ASAPController's own +design doc calls out as a first-class split — see +[ASAPController `docs/design.md` § "Serving-time execution"](https://github.com/ProjectASAP/ASAPController/blob/097f440079f851a560cc6927f50eb7f56a49e6c/docs/design.md#serving-time-execution): + +> Everything above (L1-L5) is the **planning** pipeline: turning a query +> string into a plan. This section is different in kind — it's what +> actually **answers** a query at request time, using whatever plan L1-L5 +> already decided... From a code standpoint, planning and serving are two +> separate interfaces a downstream deployment implements against: +> `asap_plan` (planning-time binder) and `SummaryExecutor` (serving-time +> executor). + +Concretely, for this deployment: + +- **Planning** (`L3 QueryExpr → L4 L4Node`) is `control_plane`'s job — + `asap_plan::bind`/`implement_tree_in_with`, plugged with a `CostModel`. + `control_plane` runs **in-process** with `data_plane` in this deployment + (the "Phase 9" comment in `data_plane/Cargo.toml`), so this is a + same-binary library call, not a network hop or a second planning + implementation living in `data_plane`. +- **Serving** (`L4Node → Value`, "walk the already-decided tree against + whatever is actually materialized right now") is `data_plane`'s job — + `impl SummaryExecutor for QueryExecutionContext` (`summary_executor.rs`), + covered by the rest of this doc. + +The one thing this deployment's topology adds that the upstream doc doesn't +need to say: because both halves run in the same process, `data_plane` CAN +call `control_plane`'s planning function directly at request time (see +"Rollout" below) rather than needing a wire protocol to receive an +already-built plan from a separately-deployed control plane. This is a +deployment-specific convenience, not something `asap-plan`/`asap-sketch` +assume. ## Today @@ -29,37 +74,34 @@ for candidate in candidates { } ``` -Four real gaps this leaves: +Four real gaps this left (as of the original version of this doc): 1. **No fold.** The loop overwrites instead of combining — fine today because real queries only ever produce ≤1 candidate, but there's no existing multi-node behavior to be "faithful to" once that's no longer true. -2. **Merge only exists for `ExactAgg`.** `evaluate_exact_agg` folds sids by - `(group, window)` via `AggregateCore::merge_with` — the one real - precedent. HLL has a global-only special case. Every other grouped sketch - case (`quantile by (zone) (...)`, per-group `topk`/`frequency`) silently - emits duplicate un-merged series today when two sids project to the same - group — a live bug, not a gap in a working feature. -3. **Sketch merge needs exact param match, and nothing checks that today.** - `SummaryMerge` requires every child to agree on `(SummaryKind, - SummaryParams)`, not just family — but `Capability` (what - `is_satisfied_by` matches on) is family-level only. `AccumulatorSpec` - (`asap_types::accumulator_spec`, landed) now carries the params - `data_plane` needs for this check; nothing wires it into merge-candidate - selection yet. + Still true of the legacy `SketchReducer` path — unaffected by anything + below, since none of it has been wired into `engine.rs` yet. +2. **Merge only existed for `ExactAgg`** on the legacy path. ~~Every other + grouped sketch case silently emits duplicate un-merged series today.~~ + **Resolved on the new path**: `summary_executor.rs`'s `readout_cumulative`/ + `readout_per_window` do real cross-sid merging generically for every + sketch family via `SummaryState::merge_same_family`, not an `ExactAgg`-only + special case. Still an open bug on the legacy `SketchReducer` path itself, + which this doc's plan doesn't touch. +3. **Sketch merge needs exact param match, and nothing checked that on the + legacy path.** ~~`AccumulatorSpec` now carries the params `data_plane` + needs for this check; nothing wires it into merge-candidate selection + yet.~~ **Resolved on the new path** — see "resolved by construction" + below. 4. **Two raw-AST fallbacks exist** (`try_topk_over_rate_fallback`, `try_rate_over_frequency_fallback`) for compositions the flat `Capability` - vocabulary can't express. The first is `topk(K, sum by(gbk)(rate(m[r])))` - — PromQL classifies this as `RankingMeasure::NonAdditive` (only - `topk(K, count_over_time(...))` maps to the real `AggIntent::TopK` - sketch), which lowers to `QueryExpr::Sort{Limit{Aggregate}}`. - `implement_tree_in_with` only recurses through `Aggregate`, so this whole - shape stays one opaque `Logical` blob today even though the inner - aggregate would bind fine on its own. + vocabulary can't express. Still true, still unaddressed by anything on + this doc's plan — see "Remaining open questions" #3. The canonical tree (`asap_sketch::{SummaryExpr, L4Node}`) already exists and -is built control-plane-side (`implement_promql_for_asap_tier`, Step A/B) — -`data_plane` has never consumed it. +can be built control-plane-side — see "Remaining open questions" #1 below +for exactly which function does this correctly (it turned out to matter +which one). ## The executor is now a common interface, not a bespoke walk @@ -68,7 +110,7 @@ ASAPController#155 (`crates/sketch/src/exec.rs`) adds `SummaryExecutor` + does the recursive walk and enforces the structural rules generically (which nestings are valid, that `SummaryMerge` children must agree on `(SummaryKind, SummaryParams)`, propagated correctly through arbitrary -nesting depth). See `docs/l4node-execution-model.md` in ASAPController for +nesting depth). See `docs/l4-summary-bound-ir.md` in ASAPController for the full design; this doc only covers what's specific to `data_plane`. `data_plane`'s implementation (`summary_executor.rs`) is @@ -80,12 +122,12 @@ time-range parameter and `ASAPQueryEngine` is called concurrently; | `SummaryExecutor` method | `data_plane` implementation | |---|---| -| `Handle` | `SidHandle` — a sid plus its already-fetched `Rc` and decode params, not a bare `u64`. `find_candidates` needs to fetch the series anyway (to read label values for the group key), so the handle carries it forward instead of `fetch_state`/`readout` re-fetching the same `(sid, t0, t1)` range a second time. | +| `Handle` | `SidHandle` — an **enum**: `Sketch { series: Rc, kind: DeltaSketchKind }` or `ExactAgg { windows: Rc>>, agg_type: AggregationType }`. `find_candidates` needs to fetch the series/windows anyway (to read label values for the group key), so the handle carries it forward instead of `fetch_state`/`readout` re-fetching the same `(sid, t0, t1)` range a second time. | | `GroupKey` | `BTreeMap` — the sid's own label values projected onto the query's `by` columns. | -| `State` | `GroupState` — the group's accumulated `SidHandle`s plus the shared decode kind. Decode/merge is deliberately lazy: `fetch_state`/`merge_states` just assemble the candidate list; the actual `RollingState` reconstruction and merge happens in `readout`, which is where cumulative-vs-per-window mode is known. | -| `find_candidates(sketch, params, col, by, child)` | walks `child` down to a `Scan{source: Source::TimeSeries{metric}, ..}` to recover the metric, then `instances_matching(metric, by)` filtered to sids whose `(SketchKindHandle, SketchConfig)` is exactly `(sketch, params)` — not the looser family-only `Capability::is_satisfied_by` check the legacy analyzer path uses. | -| `merge_states` | Lazy — see `State` above. | -| `readout` | Real cross-sid merge via `delta_apply::cumulative_rolling_state`/`per_window_rolling_states` + `RollingState::merge_same_family`, covering the DDSketch/Kll/Hll families. CMS/CountSketch (Frequency family) and `ExactAgg` are not covered — see the status note above. | +| `State` | `GroupState` — also an enum, mirroring `SidHandle`'s two variants. Decode/merge is deliberately lazy for the `Sketch` variant: `fetch_state`/`merge_states` just assemble the candidate list; the actual `SummaryState` reconstruction and merge happens in `readout`, which is where cumulative-vs-per-window mode is known. `ExactAgg`'s value extraction (`GroupState::exact_value`) is eager-ish but still only runs when a caller asks. | +| `find_candidates(sketch, params, col, by, child)` | walks `child` down to a `Scan{source: Source::TimeSeries{metric}, ..}` to recover the metric, then `instances_matching(metric, by)` filtered to sids whose `AggKind` is either `Sketch{kind, config, ..}` with `(SketchKindHandle, SketchConfig)` exactly `(sketch, params)`, or `ExactAgg{agg_type, ..}` matching `exact_agg_kind_match(sketch, params, agg_type)` — not the looser family-only `Capability::is_satisfied_by` check the legacy analyzer path uses. | +| `merge_states` | Lazy for `Sketch` — see `State` above. Errors (`UnsupportedFamily`) if a group somehow mixes `Sketch` and `ExactAgg` entries (shouldn't happen; `find_candidates`'s per-`(kind,params)` exactness makes this defensive, not a real path). | +| `readout` | Real cross-sid merge via `delta_apply::cumulative_summary_state`/`per_window_summary_states` + `SummaryState::merge_same_family`, covering DDSketch/Kll/Hll/Cms/CmsWithHeap/CountSketch/CountSketchWithHeap. Returns `SummaryExecutorError::UnsupportedFamily` for an `ExactAgg` state (defensive — see below, `readout` is never actually called for one in practice). | | `logical` | `Err(SummaryExecutorError::Logical)` — same meaning as "no candidate bound" today, lets `EngineRouter` fail over to archive. | `AggregateCore::merge_with`/per-family `asap_sketchlib` merge turned out to @@ -94,17 +136,22 @@ readout never reaches `SummaryExecutor::readout` at all (`asap_plan::bind` never wraps an `ExactAccumulator` implementation in a `SummaryEstimate`, so `execute()` on such a tree returns `ExecOutcome::State` at the root — a caller-side concern, not something this trait implementation handles), -while the sketch-family merge (gap 2) is what's actually implemented here, -via `RollingState::merge_same_family` rather than `asap_sketchlib` calls -made directly in this module. +while the sketch-family merge is what's actually implemented in `readout`, +via `SummaryState::merge_same_family` rather than `asap_sketchlib` calls +made directly in this module. `GroupState::exact_value(&self, key)` is the +`ExactAgg` analog of `readout_cumulative` — folds every window/sid in the +group via `AggregateCore::merge_with`, then reads out the statistic +`agg_type` implies — called directly by whatever future caller reads an +`ExecOutcome::State`'s contents (see "Rollout" below for the first such +caller). Because `find_candidates` is now contractually required to return only -exact-`(kind, params)` matches (`asap-sketch`'s trait doc), gap 3 (nothing -checks param agreement today) is resolved by construction for anything -routed through `execute()` — `data_plane` no longer needs its own -merge-precondition check, `asap-sketch`'s does it for `SummaryMerge` -children, and `find_candidates`'s contract does it for a single -`SummaryAgg`'s candidate set. +exact-`(kind, params)` matches (`asap-sketch`'s trait doc), gap 3 above +(nothing checked param agreement on the legacy path) is resolved by +construction for anything routed through `execute()` — `data_plane` no +longer needs its own merge-precondition check, `asap-sketch`'s does it for +`SummaryMerge` children, and `find_candidates`'s contract does it for a +single `SummaryAgg`'s candidate set. ## Nested queries in this deployment's topology @@ -125,41 +172,99 @@ actually *occurs* in a three-stage (edge/gateway/backend) topology: stage allocator ever actually *produce* a two-level merge cascade today, or does everything currently collapse to one gateway hop? Worth checking against `physical/colored_dag`'s stage-allocation output before assuming - the two-level case needs dedicated testing. + the two-level case needs dedicated testing. Still unresolved — no new + information this round. - **`SummaryAgg`-over-`SummaryEstimate`**: flagged upstream as an open, unresolved question (building a new summary from another summary's query-time readout). `data_plane`'s `SketchStore` only ever serves summaries built at ingest time from raw samples — if `find_candidates` ever receives a `child` whose subtree bottoms out in a `SummaryEstimate` rather than raw `Logical`/`SummaryAgg`, that's this shape, and the - right answer today is `Err` (unsupported), not a guess. + right answer today is `Err` (unsupported), not a guess. Still unresolved. + +## Rollout + +`execute()` isn't wired into `ASAPQueryEngine`'s live serving path yet at +all. This round's plan (see the tracked plan file / PR for the actual +implementation) is a **shadow-mode** first phase, not a cutover: + +1. **Tree source, corrected.** The previous version of this doc said + `implement_promql_for_asap_tier` (`control_plane::asap_tier_implement`, + Step A) "is the seam for planning-time tree construction." That's + *incomplete* — that function uses the naive `asap_plan::DefaultCostModel` + (no real accuracy-driven parameter sizing) and has its own documented, + tracked gap: it cannot realize `AggIntent::Extension`/`Frequency` + (CMS/CountSketch) at all — falls back to `SummaryExpr::Logical` for the + entire Frequency family (see that module's own doc, "Known gap: + Extension/Frequency under-realizes"). The correct seam is + `control_plane::sketch_algebra::lower::bind_query_expr` — the function + `main.rs`'s real production planning pipeline calls, using + `ControlPlaneCostModel` (real accuracy-bound sizing, and, via + `boundary::implementation_for_with` + `realize_extension`/ + `readout_extension`, ASAPController#150, correct Frequency realization + too). `ControlPlaneCostModel::new(accuracy)` takes only an + `AccuracyTarget` — no live server/catalog state — so it's constructible + standalone from `data_plane`, matching the "same-binary library call" + framing in "Architecture reference" above. `bind_query_expr` always + returns `PhysicalExpr::Committed(L4Plan::Summary(Rc))` per its + own doc (never picks a Phase ε.1 edge/backend placement), so extracting + the tree is a simple pattern match. +2. **Sizing drift is expected, not a bug to fix first.** A freshly-computed + `SummaryParams` (width/depth/k/precision) from step 1 is NOT guaranteed + to exactly match what's actually registered in `SketchStore` right now + — `find_candidates`'s contract is an exact match, so a mismatch just + means `NoCandidates`/an empty group, not a wrong answer. This is exactly + what shadow mode is for: surface how often/how badly this happens before + ever trying to close the gap. +3. **`rate()`/topk-over-rate/outer-agg-fold are excluded from the shadow + comparison entirely**, not just deprioritized. `lower.rs`'s + `bind_recursive` rewrites `AggIntent::Rate → Increase` before binding + (so a `rate()` query DOES bind to a valid `SummaryAgg{Increase}` tree), + but `summary_executor.rs` has no rate-division logic (dividing by a + coverage-clamped range is `sketch_reducer.rs::evaluate_exact_agg_rate`/ + `evaluate_frequency_rate`'s job) — so the new path would produce a + semantically wrong (un-divided) answer for `rate()` if compared naively. + Detected and skipped before ever calling into `control_plane` for this + phase, using the same raw-AST inspection `engine.rs`'s existing + fallbacks already do. +4. **Shadow, not cutover.** Compute the new answer alongside the old + (`SketchReducer`), diff, log discrepancies, always return the old + answer — see `docs/design-sketch-db-roadmap.md` § 13.2 "Shadow mode" + for the pattern this follows (already documented there, unimplemented + until now). `ASAP_LEGACY_DUAL_WRITE` + (`data_plane/src/drivers/ingest/otel.rs:891-895`) is the closest + actually-shipped env-var mechanics to mirror for the flag itself; + `control_plane`'s `USE_TYPED_STAGE_SPLIT` is a single-path selector, not + a shadow/diff pattern, so it's the wrong template despite being more + prominent in this codebase. + +Actually switching what's served, and retiring `sketch_reducer.rs`, both +require confidence data this phase doesn't yet produce, plus a resolution +for the rate/outer-fold gap (which needs its own cross-repo design +conversation with ASAPController, not a unilateral local decision) — +neither is in scope for the shadow-mode phase. ## Remaining open questions -1. **Tree source — resolved, and it's both.** `implement_promql_for_asap_tier` - (Step A) is the seam for planning-time tree construction, as leaned - toward. But `data_plane` turned out to need a *direct* `asap-sketch` - (and `asap-ir`) dependency too, pinned to match `control_plane`'s exactly - — implementing `SummaryExecutor` and matching `L4Node`/`SketchQuery` - variants requires importing their defining crate directly; consuming a - function that merely returns those types isn't enough for Rust's trait/ - pattern-matching rules. So this isn't purely "through the seam" as - originally envisioned — it's the seam for tree construction, plus a - direct dependency for everything serving-time. -2. **`SummaryMerge` for sketch families — partially resolved.** - KLL/DDSketch/HLL merge is implemented via `RollingState::merge_same_family` - (generalized from an existing HLL-only global-cardinality special case, - not built from scratch against raw `asap_sketchlib` calls). CMS/CountSketch - still open (Frequency family, in progress as a follow-up). Partial - coverage — a group missing a sid for one part — is resolved as "fold - whatever's present," not "miss the whole group" (mirrors - `SummaryMerge`'s own semantics, ASAPController#159/#161). Accuracy-math - and resize/downsample questions remain genuinely open, not yet - investigated. -3. **`topk`/`rate` post-processing**: still open, unrelated to what's been - built so far — the Frequency-family follow-up doesn't address the - `try_topk_over_rate_fallback`/`try_rate_over_frequency_fallback` raw-AST - paths in `engine.rs`. -4. **Rollout**: still open. `execute()` isn't wired into `ASAPQueryEngine`'s - live serving path yet at all — this question doesn't arise until that - wiring is attempted. +1. **Tree source — see "Rollout" above**, now resolved with a correction + to the original answer (`bind_query_expr`, not + `implement_promql_for_asap_tier`). `data_plane` still needs a *direct* + `asap-sketch` (and `asap-ir`) dependency, pinned to match `control_plane`'s + exactly — implementing `SummaryExecutor` and matching `L4Node`/ + `SketchQuery` variants requires importing their defining crate directly; + consuming a function that merely returns those types isn't enough for + Rust's trait/pattern-matching rules. +2. **`SummaryMerge` for sketch families — resolved.** KLL/DDSketch/HLL/ + CMS/CountSketch (bare and heap-bearing) merge are all implemented via + `SummaryState::merge_same_family`. Partial coverage (a group missing a + sid for one part) is resolved as "fold whatever's present," not "miss + the whole group" (mirrors `SummaryMerge`'s own semantics, + ASAPController#159/#161). Accuracy-math and resize/downsample questions + remain genuinely open, not yet investigated. +3. **`topk`/`rate` post-processing**: still open. `try_topk_over_rate_fallback`/ + `try_rate_over_frequency_fallback` in `engine.rs` remain + `SketchReducer`-only; the shadow-mode rollout explicitly excludes these + shapes rather than attempting them (see "Rollout" #3) pending the + cross-repo design conversation on outer-agg-fold. +4. **Rollout — no longer just an open question**, see "Rollout" above for + the concrete shadow-mode plan. From 11f176f6bc9ff7dbbe4a0ba2b3d422f1f0810b8a Mon Sep 17 00:00:00 2001 From: zz_y Date: Mon, 27 Jul 2026 11:35:18 -0600 Subject: [PATCH 6/6] docs: rewrite as a pure design doc (interfaces + open design questions) Restructures the whole doc away from implementation-status tracking ("Status (date)", PR references, "resolved on the new path" narration) into a straightforward architecture/interface description: what SummaryExecutor's contract is, how data_plane's data model realizes it, what the grouping/readout/coverage/ExactAgg designs are, and what's genuinely still an open design question versus already settled. Adds one new, substantive open design question found while working on the shadow-mode rollout: grouping semantics for an empty `by` are ambiguous between "no PromQL grouping syntax exists for this shape at all" (a bare per-series range function like quantile_over_time) and "an explicit aggregation operator asked to reduce everything" (count(), sum() with no by()) -- both produce the identical SummaryAgg{by: []} tree, confirmed by inspecting both directly. Documents the two independently-correct per-family defaults already in place (ExactAgg's by=[] is unambiguous; sketch families default to the conservative never-silently-merge behavior) and the two general resolution paths for the sketch-family gap (an upstream IR signal, or a caller-supplied one mirroring engine.rs's existing outer_agg/by_labels) -- framed as a design question for a cross-repo conversation, not a local special case to hack around. Co-Authored-By: Claude Sonnet 5 --- .../docs/l4node-plan-executor-design.md | 667 +++++++++++------- 1 file changed, 421 insertions(+), 246 deletions(-) diff --git a/data_plane/docs/l4node-plan-executor-design.md b/data_plane/docs/l4node-plan-executor-design.md index 4dc54511..fe982cf0 100644 --- a/data_plane/docs/l4node-plan-executor-design.md +++ b/data_plane/docs/l4node-plan-executor-design.md @@ -1,35 +1,47 @@ -# Step C: a recursive `L4Node` plan-executor for `ASAPQueryEngine` +# L4 serving-time execution — design -Planning doc, not an implementation — same spirit as +Design doc, not an implementation log — same spirit as [`sketchindex-sid-unification-plan.md`](./sketchindex-sid-unification-plan.md). +This describes the target architecture for `data_plane`'s serving-time query +execution and the interfaces it's built against, so that work on it (and +discussion of open questions) has a stable reference independent of which +piece has landed on which day. -> **Status (2026-07-27).** `impl SummaryExecutor for QueryExecutionContext` -> (`summary_executor.rs`) now covers quantile/cardinality (DDSketch/Kll/Hll), -> the Frequency family (CMS/CountSketch/CmsWithHeap/CountSketchWithHeap — -> bare total, per-item point lookup, top-k), both cumulative (instant) and -> per-window (matrix/range) queries, real cross-sid merging for all of the -> above, per-group coverage tracking (`SummaryValue::coverage`), and -> `AggKind::ExactAgg` candidate matching for `Sum`/`Increase` (#411, #412, -> #414, #415, #417 — all merged). `ExactAgg` readout still never reaches -> `SummaryExecutor::readout`/`SketchQuery` at all (unchanged from the -> original design below) — `GroupState::exact_value` is the accessor a -> caller uses instead. `MinMax`/`Count`/`Rate` are deliberately NOT matched -> for `ExactAgg` (no direction info survives to `AggKind::ExactAgg`'s -> metadata for `MinMax`; no `AggregationType` resolves to `Count`/`Rate` -> today) — see `summary_executor.rs`'s own module doc and -> `exact_agg_kind_match`. -> -> `execute()` is **still not wired into `ASAPQueryEngine::execute()`'s live -> serving path** — see "Rollout" below, which now has a concrete plan -> (shadow mode) rather than being an open question. The sections below -> describe the target design; some turned out to differ from what actually -> got built — see the inline corrections. - -## Architecture reference: planning vs. serving-time execution - -This doc is the `data_plane`-specific instance of what ASAPController's own -design doc calls out as a first-class split — see -[ASAPController `docs/design.md` § "Serving-time execution"](https://github.com/ProjectASAP/ASAPController/blob/097f440079f851a560cc6927f50eb7f56a49e6c/docs/design.md#serving-time-execution): +## Scope and motivation + +`data_plane` today answers queries through `SketchReducer` +(`storage_engines/sketch_db/query/sketch_reducer.rs`), a flat, per-capability +dispatcher: `engine.rs` collects a `Vec` from +`analyze_promql_for_asap_tier` and calls one `SketchReducer` method per +candidate. This has three structural limitations baked into its shape, +independent of any particular bug: + +- **No real tree.** A candidate is a flat `(metric, capability, group_by_keys, + outer_fn, outer_agg, ...)` record, not a composable plan. Nested + compositions (`quantile(0.9, sum by (job) (m))`, cross-stage sketch merges) + aren't representable as a single structure the reducer walks; they're + handled — when they're handled at all — by ad hoc, PromQL-string-level + fallbacks in `engine.rs` (`try_topk_over_rate_fallback`, + `try_rate_over_frequency_fallback`, `apply_outer_agg_fold`). +- **No generic cross-sid merge.** Merging multiple sids that answer the same + logical group exists for `ExactAgg` (`evaluate_exact_agg`'s own + `AggregateCore::merge_with` fold) and, separately, as one bespoke special + case for global HLL cardinality (`evaluate_cardinality_global`). Every + other grouped sketch case has no generic merge path. +- **No compile-time contract.** Nothing enforces that candidates merged + together actually agree on sketch family and parameters; that's discovered + (or not) at read time. + +The redesign is to route serving through `asap_sketch::exec::execute()` and +the `SummaryExecutor` trait it defines — the counterpart, upstream, to the +planning-time `asap_plan::bind` interface — trading the flat-candidate model +for a real recursive tree walk with structural guarantees enforced generically +by `asap-sketch`, not re-implemented per deployment. + +## Architecture: planning vs. serving-time execution + +ASAPController's own design doc treats this as a first-class split — see +[`docs/design.md` § "Serving-time execution"](https://github.com/ProjectASAP/ASAPController/blob/097f440079f851a560cc6927f50eb7f56a49e6c/docs/design.md#serving-time-execution): > Everything above (L1-L5) is the **planning** pipeline: turning a query > string into a plan. This section is different in kind — it's what @@ -39,232 +51,395 @@ design doc calls out as a first-class split — see > `asap_plan` (planning-time binder) and `SummaryExecutor` (serving-time > executor). -Concretely, for this deployment: - -- **Planning** (`L3 QueryExpr → L4 L4Node`) is `control_plane`'s job — - `asap_plan::bind`/`implement_tree_in_with`, plugged with a `CostModel`. - `control_plane` runs **in-process** with `data_plane` in this deployment - (the "Phase 9" comment in `data_plane/Cargo.toml`), so this is a - same-binary library call, not a network hop or a second planning - implementation living in `data_plane`. -- **Serving** (`L4Node → Value`, "walk the already-decided tree against - whatever is actually materialized right now") is `data_plane`'s job — - `impl SummaryExecutor for QueryExecutionContext` (`summary_executor.rs`), - covered by the rest of this doc. - -The one thing this deployment's topology adds that the upstream doc doesn't -need to say: because both halves run in the same process, `data_plane` CAN -call `control_plane`'s planning function directly at request time (see -"Rollout" below) rather than needing a wire protocol to receive an -already-built plan from a separately-deployed control plane. This is a -deployment-specific convenience, not something `asap-plan`/`asap-sketch` -assume. - -## Today - -`ASAPQueryEngine::execute()` (`asap_query_engine/engine.rs:1264-1923`) has no -tree. It gets a flat `Vec` from -`analyze_promql_for_asap_tier` and loops: +For this deployment: + +- **Planning** (`L3 QueryExpr → L4 L4Node`) is `control_plane`'s + responsibility — `asap_plan::bind`/`implement_tree_in_with`, parameterized + by a `CostModel`. Planning has, by design, no reference to what's actually + materialized anywhere — it symbolically picks a summary family and + parameters from the query shape and an accuracy target alone. +- **Serving** (`L4Node → Value`: walk the already-decided tree against + whatever is actually materialized right now) is `data_plane`'s + responsibility — an `impl SummaryExecutor` type, covered below. Serving + needs its own error vocabulary distinct from planning's, because reality + can diverge from the plan in ways planning never sees: missing data, + multiple instances needing a merge, instances that disagree on parameters. + +`control_plane` runs **in-process** with `data_plane` in this deployment (see +`data_plane/Cargo.toml`'s dependency comment), so calling `control_plane`'s +planning entry point directly from `data_plane` at request time is a +same-binary library call, not a network hop or a second planning +implementation living in `data_plane`. That's a deployment-specific +convenience this topology affords, not something `asap-plan`/`asap-sketch` +assume of every deployment. +```mermaid +flowchart LR + Q["PromQL query string"] --> P["control_plane planning\n(bind_query_expr, ControlPlaneCostModel)"] + P --> L4["L4Node tree"] + L4 --> E["data_plane serving\n(SummaryExecutor::execute)"] + E --> V["Value"] ``` -for candidate in candidates { - sids = instances_matching(metric, group_by).filter(|sid| required.is_satisfied_by(sid.capability)); - result = evaluate_exact_agg / evaluate_for_capability(sids, ...); - combined_result = Some(result); // ← overwrites, never folds + +### Which planning entry point + +Two `control_plane` functions can turn a PromQL string into an `L4Node`, and +they are **not interchangeable**: + +- `control_plane::sketch_algebra::lower::bind_query_expr` — parameterized by + `ControlPlaneCostModel`: real accuracy-bound-driven parameter sizing, and, + via `boundary::implementation_for_with` + `realize_extension`/ + `readout_extension`, correct realization of the `Extension`/`Frequency` + intent (CMS/CountSketch). This is the function `main.rs`'s own production + planning pipeline calls. +- `control_plane::asap_tier_implement::implement_promql_for_asap_tier` — + parameterized by the naive `asap_plan::DefaultCostModel`: no accuracy-driven + sizing, and a documented inability to realize the `Frequency` intent at all + (falls back to `SummaryExpr::Logical` for the entire CMS/CountSketch + family — see that module's own doc). + +Serving-time tree construction must use `bind_query_expr`. `implement_promql_for_asap_tier` +exists for a narrower purpose (finding realizable `Aggregate` subtrees +anywhere in a query tree, not just at the root) and is not a drop-in +substitute for planning a whole query for serving. + +`bind_query_expr(expr: &QueryExpr, accuracy: AccuracyTarget) -> Result` +always returns `PhysicalExpr::Committed(L4Plan::Summary(Rc))` per its +own contract (it never picks a Phase ε.1 edge/backend placement), so +extracting the tree is a simple pattern match; any other `PhysicalExpr` shape +is a signal this deployment doesn't yet handle that binding outcome. + +`ControlPlaneCostModel::new(accuracy)` takes only an `AccuracyTarget` — no +live server or catalog state — so it's constructible standalone wherever +`control_plane` is reachable as a library. + +**Design consequence — sizing is not guaranteed to match what's stored.** A +freshly-computed `SummaryParams` (width/depth/k/precision) from planning is +not guaranteed to exactly match whatever a given sid was actually provisioned +with (registration happens at a different time, potentially under a +different accuracy target or cost-model version). Since `find_candidates`'s +contract is an *exact* `(SummaryKind, SummaryParams)` match, a mismatch here +simply yields no candidates for that sid — a capability miss, not a wrong +answer. Any rollout strategy needs to account for how often this drift +occurs before relying on serving-time re-planning as the sole tree source. + +## The `SummaryExecutor` interface + +`asap-sketch`'s `exec` module defines the shared, deployment-agnostic +contract: + +```rust +trait SummaryExecutor { + type Handle: Clone; + type State; + type Value; + type Error; + type GroupKey: Clone + Ord + Default; + + fn find_candidates( + &self, + sketch: &SummaryKind, + params: &SummaryParams, + col: &ColumnRef, + by: &[ColumnId], + child: &L4Node, + ) -> Result, Self::Error>; + + fn fetch_state(&self, handle: &Self::Handle) -> Result; + fn merge_states(&self, states: Vec) -> Result; + fn readout(&self, state: &Self::State, query: &SketchQuery) -> Result; + fn logical(&self, expr: &QueryExpr) -> Result; } ``` -Four real gaps this left (as of the original version of this doc): - -1. **No fold.** The loop overwrites instead of combining — fine today because - real queries only ever produce ≤1 candidate, but there's no existing - multi-node behavior to be "faithful to" once that's no longer true. - Still true of the legacy `SketchReducer` path — unaffected by anything - below, since none of it has been wired into `engine.rs` yet. -2. **Merge only existed for `ExactAgg`** on the legacy path. ~~Every other - grouped sketch case silently emits duplicate un-merged series today.~~ - **Resolved on the new path**: `summary_executor.rs`'s `readout_cumulative`/ - `readout_per_window` do real cross-sid merging generically for every - sketch family via `SummaryState::merge_same_family`, not an `ExactAgg`-only - special case. Still an open bug on the legacy `SketchReducer` path itself, - which this doc's plan doesn't touch. -3. **Sketch merge needs exact param match, and nothing checked that on the - legacy path.** ~~`AccumulatorSpec` now carries the params `data_plane` - needs for this check; nothing wires it into merge-candidate selection - yet.~~ **Resolved on the new path** — see "resolved by construction" - below. -4. **Two raw-AST fallbacks exist** (`try_topk_over_rate_fallback`, - `try_rate_over_frequency_fallback`) for compositions the flat `Capability` - vocabulary can't express. Still true, still unaddressed by anything on - this doc's plan — see "Remaining open questions" #3. - -The canonical tree (`asap_sketch::{SummaryExpr, L4Node}`) already exists and -can be built control-plane-side — see "Remaining open questions" #1 below -for exactly which function does this correctly (it turned out to matter -which one). - -## The executor is now a common interface, not a bespoke walk - -ASAPController#155 (`crates/sketch/src/exec.rs`) adds `SummaryExecutor` + -`execute()` upstream: a deployment implements the trait, `asap-sketch` -does the recursive walk and enforces the structural rules generically -(which nestings are valid, that `SummaryMerge` children must agree on -`(SummaryKind, SummaryParams)`, propagated correctly through arbitrary -nesting depth). See `docs/l4-summary-bound-ir.md` in ASAPController for -the full design; this doc only covers what's specific to `data_plane`. - -`data_plane`'s implementation (`summary_executor.rs`) is -`impl SummaryExecutor for QueryExecutionContext`, a small per-query -context — not `ASAPQueryEngine` directly, since the trait carries no -time-range parameter and `ASAPQueryEngine` is called concurrently; -`QueryExecutionContext` is constructed fresh per query with `t0_ms`/ -`t1_ms`/`is_cumulative` as plain fields: - -| `SummaryExecutor` method | `data_plane` implementation | +`execute(node, exec)` (upstream, not deployment code) does the recursive +walk and owns the *structural* rules generically: + +- A `SummaryAgg` leaf resolves via `find_candidates`, groups the returned + `(GroupKey, Handle)` pairs by `GroupKey`, and folds each group's handles + independently via `fetch_state`+`merge_states` — a group's state is never + combined with another group's. +- A `SummaryMerge` requires every child to agree on `(SummaryKind, + SummaryParams)` before folding their states together across children, + key-by-key. +- A `SummaryEstimate` calls `readout` on the state its child produced. +- Nesting composes: `execute()`'s own recursion handles arbitrary depth + (`SummaryAgg`-of-`SummaryAgg`, `SummaryMerge`-of-`SummaryMerge`, ...); the + deployment only ever sees one level at a time through the trait methods. + +The deployment supplies storage, summary math, and readout; `asap-sketch` +enforces which nestings are structurally valid and propagates the +`(SummaryKind, SummaryParams)` agreement check through arbitrary nesting +depth — a deployment implementing this trait does not need its own +merge-precondition check layered on top. + +## Data model + +`QueryExecutionContext<'a>` is the `SummaryExecutor` implementer — a small, +per-query, stack-local value (`index: &SketchStore`, `t0_ms`, `t1_ms`, +`is_cumulative`), constructed fresh per incoming query rather than carried on +`ASAPQueryEngine` itself: the trait carries no time-range parameter, and +`ASAPQueryEngine` is called concurrently, so threading a query's range through +shared mutable engine state would race. + +| Interface type | Design | |---|---| -| `Handle` | `SidHandle` — an **enum**: `Sketch { series: Rc, kind: DeltaSketchKind }` or `ExactAgg { windows: Rc>>, agg_type: AggregationType }`. `find_candidates` needs to fetch the series/windows anyway (to read label values for the group key), so the handle carries it forward instead of `fetch_state`/`readout` re-fetching the same `(sid, t0, t1)` range a second time. | -| `GroupKey` | `BTreeMap` — the sid's own label values projected onto the query's `by` columns. | -| `State` | `GroupState` — also an enum, mirroring `SidHandle`'s two variants. Decode/merge is deliberately lazy for the `Sketch` variant: `fetch_state`/`merge_states` just assemble the candidate list; the actual `SummaryState` reconstruction and merge happens in `readout`, which is where cumulative-vs-per-window mode is known. `ExactAgg`'s value extraction (`GroupState::exact_value`) is eager-ish but still only runs when a caller asks. | -| `find_candidates(sketch, params, col, by, child)` | walks `child` down to a `Scan{source: Source::TimeSeries{metric}, ..}` to recover the metric, then `instances_matching(metric, by)` filtered to sids whose `AggKind` is either `Sketch{kind, config, ..}` with `(SketchKindHandle, SketchConfig)` exactly `(sketch, params)`, or `ExactAgg{agg_type, ..}` matching `exact_agg_kind_match(sketch, params, agg_type)` — not the looser family-only `Capability::is_satisfied_by` check the legacy analyzer path uses. | -| `merge_states` | Lazy for `Sketch` — see `State` above. Errors (`UnsupportedFamily`) if a group somehow mixes `Sketch` and `ExactAgg` entries (shouldn't happen; `find_candidates`'s per-`(kind,params)` exactness makes this defensive, not a real path). | -| `readout` | Real cross-sid merge via `delta_apply::cumulative_summary_state`/`per_window_summary_states` + `SummaryState::merge_same_family`, covering DDSketch/Kll/Hll/Cms/CmsWithHeap/CountSketch/CountSketchWithHeap. Returns `SummaryExecutorError::UnsupportedFamily` for an `ExactAgg` state (defensive — see below, `readout` is never actually called for one in practice). | -| `logical` | `Err(SummaryExecutorError::Logical)` — same meaning as "no candidate bound" today, lets `EngineRouter` fail over to archive. | - -`AggregateCore::merge_with`/per-family `asap_sketchlib` merge turned out to -be two genuinely separate things, not one shared mechanism: `ExactAgg` -readout never reaches `SummaryExecutor::readout` at all (`asap_plan::bind` -never wraps an `ExactAccumulator` implementation in a `SummaryEstimate`, -so `execute()` on such a tree returns `ExecOutcome::State` at the root — -a caller-side concern, not something this trait implementation handles), -while the sketch-family merge is what's actually implemented in `readout`, -via `SummaryState::merge_same_family` rather than `asap_sketchlib` calls -made directly in this module. `GroupState::exact_value(&self, key)` is the -`ExactAgg` analog of `readout_cumulative` — folds every window/sid in the -group via `AggregateCore::merge_with`, then reads out the statistic -`agg_type` implies — called directly by whatever future caller reads an -`ExecOutcome::State`'s contents (see "Rollout" below for the first such -caller). - -Because `find_candidates` is now contractually required to return only -exact-`(kind, params)` matches (`asap-sketch`'s trait doc), gap 3 above -(nothing checked param agreement on the legacy path) is resolved by -construction for anything routed through `execute()` — `data_plane` no -longer needs its own merge-precondition check, `asap-sketch`'s does it for -`SummaryMerge` children, and `find_candidates`'s contract does it for a -single `SummaryAgg`'s candidate set. +| `GroupKey` | `BTreeMap` — the output row's label identity. See "Grouping semantics" below; this is the type with the least-settled design. | +| `Handle` | A sid plus its already-fetched `[t0, t1]` data, carried through from `find_candidates` so `fetch_state`/`readout` don't re-query the same range. Structured as an enum over the two payload shapes a sid can carry: a sketch series (`Rc` + a decode-parameter tag) or an exact-aggregation window map (`Rc>>` + the `AggregationType`). One sid answers one aggregation — there is no special-cased "is this exact or approximate" branch anywhere above this data-shape distinction; both payload kinds sit inside the same `find_candidates`/`fetch_state`/`merge_states` machinery. | +| `State` | Mirrors `Handle`'s two-variant shape: an accumulated list of sketch entries sharing one decode kind, or an accumulated list of exact-aggregation window maps sharing one `AggregationType`. Decode/merge for the sketch variant is deliberately lazy — `fetch_state`/`merge_states` only assemble the candidate list; reconstruction and merge happen in `readout`, which is the one place that knows cumulative-vs-per-window mode. | +| `Value` | Carries two shapes driven purely by the `SketchQuery` issued: `Points` (one scalar per timestamp) or `TopK` (one ranked `(item, value)` list per timestamp). Both variants carry the group's own observed `(min_window_end_ms, max_window_end_ms)` coverage alongside the payload — see "Coverage" below. | +| `Error` | Distinguishes: no candidate found (→ failover), an unresolved `by` column, a family this executor can't merge/decode, a decode/merge failure surfaced from lower layers, a bare `Logical` node (nothing committed at this point in the tree — same meaning as "no candidate bound," lets the caller fail over to archive), and a query shape outside this executor's covered scope. Each is a distinct, matchable variant, not a single opaque error string — callers (today: tests; eventually: `engine.rs`) need to tell these apart to decide whether to fail over to archive, log a bug, or something else. + +### `find_candidates` + +Walks the `L4Node` subtree (`child`) down to a `Scan{source: +TimeSeries{metric}, ..}` to recover the metric name (`SummaryAgg` itself +carries no metric/source field), then looks up sids by `(metric, +required_keys)` and filters to the sids whose stored kind is an *exact* +match: for a sketch-backed sid, `(SketchKindHandle, SketchConfig)` equal to +`(sketch, params)`; for an exact-aggregation-backed sid, an `AggregationType` +that maps unambiguously onto `(sketch, params)` (see "ExactAgg" below for +which mappings are unambiguous and which aren't). This is a strictly exact +match, not the family-only `Capability::is_satisfied_by` check the legacy +flat-candidate path uses — `SummaryMerge`'s precondition (every child agrees +on kind AND params) is satisfied by construction for anything this executor +produces, rather than needing a second check layered on top. + +### Grouping semantics + +`by: &[ColumnId]` names the output columns the caller wants each row keyed +by. The natural design is: project each matched sid's own label values onto +exactly those columns to get the row's `GroupKey`; sids that project to the +same key merge (via `merge_states`); sids that project to different keys stay +distinct rows. + +This is correct and sufficient whenever `by` is a genuine, resolvable +requirement — e.g. `quantile by (zone) (...)`, where the query explicitly +asks for one row per `zone`. It is **not sufficient on its own** when `by` is +empty, because an empty `by` is ambiguous between two genuinely different +intents that the current `L4Node` shape cannot distinguish: + +1. **No reduction concept applies.** A bare per-series function with no + PromQL `by(...)` and no label selector at all (e.g. `quantile_over_time(q, + m[r])`) has no grouping syntax to begin with — planning has nothing in + the query text to resolve a label column against, so the schema simply + doesn't carry one. The correct output here is one row *per underlying + series*, each keeping its own full label identity — never merging series + that happen to share no explicit `by`. +2. **An explicit, empty reduction was requested.** A genuine PromQL + aggregation operator invoked with no `by(...)` (e.g. `count(hll_metric)`, + `sum(exact_metric)`) means "reduce every matching series into one." The + correct output here is exactly one row, merging every matched sid + together, regardless of what labels they individually carry. + +Both cases produce the identical `SummaryAgg{by: []}` shape — confirmed by +inspecting the bound tree for each directly. The distinction (an aggregation +operator's own, possibly-empty `by(...)` vs. a construct with no grouping +concept at all) exists at the PromQL/L1 surface and is not preserved through +the L2→L3 canonicalization that unifies both into the same `Aggregate` +node shape. + +**Two families with different, independently-correct defaults today:** + +- **`ExactAgg`** (`Sum`/`Increase`): these `AggregationType`s map *only* from + genuine PromQL aggregation operators (`sum()`, `increase()`) — there is no + bare-range-function path into this family with the ambiguity described + above (`sum_over_time`/`avg_over_time` map to different, currently-unmatched + intents). So an empty `by` here is unambiguous: reduce fully. Projection + onto `by` (empty producing one shared key) is correct as-is. +- **Sketch families** (`DDSketch`/`Kll`/`Hll`/`Cms`/`CountSketch`, with or + without a heap): each of these families is reachable through *both* a bare + range function (case 1) and a genuine aggregation operator (case 2) — + `quantile_over_time(...)` and `quantile(...)`/`count(hll_metric)` both + bind to the same `SummaryKind`. The conservative, always-safe default is + case 1's behavior: when `by` is empty, use the sid's own full label + identity rather than collapsing to a shared empty key — this can never + silently merge two series that weren't meant to be merged, at the cost of + under-serving case 2 (a true full-reduction query gets one row per sid + instead of one merged row). + +**Open design question — resolving case 2 generally.** Under-serving case 2 +is not a narrow, single-metric special case to work around locally; every +sketch family has this same ambiguity whenever `by` is empty. A general +resolution needs one of: + +- An upstream signal on `SummaryAgg` (or an adjacent structure) distinguishing + "this by is empty because the query has no grouping syntax at all" from + "this by is empty because an aggregation operator explicitly reduced + everything" — surviving the L2→L3 canonicalization that currently discards + it. This is an `asap-ir`/`asap-plan` design question, not one `data_plane` + can resolve unilaterally. +- Equivalently, a caller-supplied signal threaded alongside the tree at + serving time (mirroring how `engine.rs`'s flat-candidate path already + carries an independent `outer_agg`/`by_labels` derived directly from the + original PromQL AST, entirely outside the `L4Node`/`Capability` vocabulary). + Note that a full reduction over a non-additive statistic (cardinality is + the concrete example: HLL registers must be merged *before* estimating, + not estimated-then-summed, or overlapping members get double-counted) is + exactly the kind of "merge these states together, then read out once" + operation this executor's own `find_candidates`→`merge_states`→`readout` + pipeline already performs for any group sharing one key — the missing + piece is purely which sids belong in that one group, not new merge math. + +Until one of these lands, full-reduction queries with an empty, ambiguous +`by` over one of the sketch families are not generally answerable through +this executor — they remain the flat legacy path's responsibility (which +resolves the ambiguity today via bespoke, capability-specific special cases, +e.g. HLL's own global-cardinality dispatch). + +### Readout + +Two modes, both doing real cross-sid merging rather than reporting one +sid's data or the first match found: + +- **Cumulative** (`quantile_over_time`/`count_distinct_over_time`-shaped + instant queries): fold each group's whole `[t0, t1]` range into one merged + state per group, then read out one scalar (or one ranked list, for + `SketchQuery::TopK`). +- **Per-window** (matrix/range queries): reconstruct each group's sids' own + per-window states, merge same-window states *across* sids, then evaluate + each window independently — one merged answer per window, not one merged + answer for the whole range. Windows union across sids: a sid missing a + particular window simply doesn't contribute to it, rather than dropping + the whole window. + +Cross-sid merge for both modes goes through a shared "merge same summary +family" operation — the sketch-family analog of `AggregateCore::merge_with`, +generalized from what was originally a single family-specific special case +into a mechanism that covers every sketch family this executor supports, +including heap-bearing (top-k) variants. Partial coverage — a group missing +a sid for one part of the range — folds whatever is present rather than +dropping the whole group, matching `SummaryMerge`'s own semantics upstream. + +### Coverage + +Each readout carries the group's own observed `(min_window_end_ms, +max_window_end_ms)` alongside its value — the signal a caller needs to +decide whether warm-tier data alone answers a query or whether an archive +tier must also be consulted and the two answers stitched. This mirrors the +legacy tier result's own coverage field, including a subtlety worth being +explicit about: despite that field's naming, no window-*start* is available +on the sketch storage path at all (samples are keyed by window-*end* only) +— both coverage bounds are folded from window-end timestamps observed across +the group's own windows, not true window starts. Coverage is folded from +*every* window observed, including any carry-in base spliced in to seed a +leading delta-only window — that base never surfaces as an output point, but +its window-end legitimately extends the group's covered range. + +### ExactAgg + +Exact-aggregation-backed sids (`Sum`, `Increase` today) participate in +`find_candidates`/`fetch_state`/`merge_states` as first-class candidates, +matched by the same one-sid-one-aggregation contract as sketches — no +"is this exact or approximate" branch anywhere in the matching logic. + +`MinMax` is deliberately **not** matched: `AggregationType` carries no +min-vs-max direction, so there is no honest way to resolve which statistic a +readout should compute from an `ExactAgg` sid's stored metadata alone — +matching it would force a guess. `Count`/`Rate` are also not matched: no +`AggregationType` resolves to either today (`Rate` in particular is reached +by rewriting to `Increase` before an accumulator is chosen at all, so the +distinct concept never reaches storage). + +`readout`/`SketchQuery` never see an `ExactAgg` state in practice: +`asap_plan::bind` never wraps an exact-accumulator implementation in a +`SummaryEstimate` (an intent's `estimate` flag is false for these), so +`execute()` on a tree rooted in one of these intents returns +`ExecOutcome::State` directly rather than reaching a `SummaryEstimate` +node — a caller-side concern, not something this trait implementation +handles. The value-extraction entry point for this case lives on the state +type itself (folding every window/sid in the group via +`AggregateCore::merge_with`, then reading out the statistic the +`AggregationType` implies) — called directly by whatever code reads an +`ExecOutcome::State`'s contents once the caller side of that path exists. ## Nested queries in this deployment's topology -`execute()`'s recursion handles arbitrary nesting depth already (see the -ASAPController doc) — the deployment-specific question is what nesting -actually *occurs* in a three-stage (edge/gateway/backend) topology: - -- **`SummaryAgg`-of-`SummaryAgg`** (`quantile(0.9, sum by (job) (m))`): - edge builds per-job sums; backend's KLL is built over that sum stream. - Already a valid, tested shape upstream — no new work here beyond - `find_candidates` correctly walking past the inner `SummaryAgg` to find - the metric. -- **`SummaryMerge`-of-`SummaryMerge`**: a plausible real shape once - merging isn't `ExactAgg`-only — e.g. per-zone edge sketches merge at a - regional gateway, regional merges merge again at the backend. `execute()` - already handles this (the `(kind, params)` check is transitive), so this - is a rollout/topology question, not a design gap: does this deployment's - stage allocator ever actually *produce* a two-level merge cascade today, - or does everything currently collapse to one gateway hop? Worth checking - against `physical/colored_dag`'s stage-allocation output before assuming - the two-level case needs dedicated testing. Still unresolved — no new - information this round. +`execute()`'s recursion handles arbitrary nesting depth already; the +deployment-specific question is what nesting actually *occurs* in a +three-stage (edge/gateway/backend) topology: + +- **`SummaryAgg`-of-`SummaryAgg`** (`quantile(0.9, sum by (job) (m))`): edge + builds per-job sums, backend's sketch is built over that sum stream. + Already a valid, structurally-supported shape — `find_candidates` walking + past the inner `SummaryAgg` to find the metric is the only requirement. +- **`SummaryMerge`-of-`SummaryMerge`**: a plausible real shape once merging + isn't `ExactAgg`-only — e.g. per-zone edge sketches merge at a regional + gateway, regional merges merge again at the backend. `execute()`'s + `(kind, params)` agreement check is transitive through this nesting, so + this is a rollout/topology question, not a structural gap: does this + deployment's stage allocator ever actually *produce* a two-level merge + cascade, or does everything currently collapse to one gateway hop? Worth + checking against the stage-allocation output before assuming the + two-level case needs dedicated testing. - **`SummaryAgg`-over-`SummaryEstimate`**: flagged upstream as an open, - unresolved question (building a new summary from another summary's - query-time readout). `data_plane`'s `SketchStore` only ever serves + unresolved question in general (building a new summary from another + summary's query-time readout). `data_plane`'s storage only ever serves summaries built at ingest time from raw samples — if `find_candidates` ever receives a `child` whose subtree bottoms out in a `SummaryEstimate` - rather than raw `Logical`/`SummaryAgg`, that's this shape, and the - right answer today is `Err` (unsupported), not a guess. Still unresolved. - -## Rollout - -`execute()` isn't wired into `ASAPQueryEngine`'s live serving path yet at -all. This round's plan (see the tracked plan file / PR for the actual -implementation) is a **shadow-mode** first phase, not a cutover: - -1. **Tree source, corrected.** The previous version of this doc said - `implement_promql_for_asap_tier` (`control_plane::asap_tier_implement`, - Step A) "is the seam for planning-time tree construction." That's - *incomplete* — that function uses the naive `asap_plan::DefaultCostModel` - (no real accuracy-driven parameter sizing) and has its own documented, - tracked gap: it cannot realize `AggIntent::Extension`/`Frequency` - (CMS/CountSketch) at all — falls back to `SummaryExpr::Logical` for the - entire Frequency family (see that module's own doc, "Known gap: - Extension/Frequency under-realizes"). The correct seam is - `control_plane::sketch_algebra::lower::bind_query_expr` — the function - `main.rs`'s real production planning pipeline calls, using - `ControlPlaneCostModel` (real accuracy-bound sizing, and, via - `boundary::implementation_for_with` + `realize_extension`/ - `readout_extension`, ASAPController#150, correct Frequency realization - too). `ControlPlaneCostModel::new(accuracy)` takes only an - `AccuracyTarget` — no live server/catalog state — so it's constructible - standalone from `data_plane`, matching the "same-binary library call" - framing in "Architecture reference" above. `bind_query_expr` always - returns `PhysicalExpr::Committed(L4Plan::Summary(Rc))` per its - own doc (never picks a Phase ε.1 edge/backend placement), so extracting - the tree is a simple pattern match. -2. **Sizing drift is expected, not a bug to fix first.** A freshly-computed - `SummaryParams` (width/depth/k/precision) from step 1 is NOT guaranteed - to exactly match what's actually registered in `SketchStore` right now - — `find_candidates`'s contract is an exact match, so a mismatch just - means `NoCandidates`/an empty group, not a wrong answer. This is exactly - what shadow mode is for: surface how often/how badly this happens before - ever trying to close the gap. -3. **`rate()`/topk-over-rate/outer-agg-fold are excluded from the shadow - comparison entirely**, not just deprioritized. `lower.rs`'s - `bind_recursive` rewrites `AggIntent::Rate → Increase` before binding - (so a `rate()` query DOES bind to a valid `SummaryAgg{Increase}` tree), - but `summary_executor.rs` has no rate-division logic (dividing by a - coverage-clamped range is `sketch_reducer.rs::evaluate_exact_agg_rate`/ - `evaluate_frequency_rate`'s job) — so the new path would produce a - semantically wrong (un-divided) answer for `rate()` if compared naively. - Detected and skipped before ever calling into `control_plane` for this - phase, using the same raw-AST inspection `engine.rs`'s existing - fallbacks already do. -4. **Shadow, not cutover.** Compute the new answer alongside the old - (`SketchReducer`), diff, log discrepancies, always return the old - answer — see `docs/design-sketch-db-roadmap.md` § 13.2 "Shadow mode" - for the pattern this follows (already documented there, unimplemented - until now). `ASAP_LEGACY_DUAL_WRITE` - (`data_plane/src/drivers/ingest/otel.rs:891-895`) is the closest - actually-shipped env-var mechanics to mirror for the flag itself; - `control_plane`'s `USE_TYPED_STAGE_SPLIT` is a single-path selector, not - a shadow/diff pattern, so it's the wrong template despite being more - prominent in this codebase. - -Actually switching what's served, and retiring `sketch_reducer.rs`, both -require confidence data this phase doesn't yet produce, plus a resolution -for the rate/outer-fold gap (which needs its own cross-repo design -conversation with ASAPController, not a unilateral local decision) — -neither is in scope for the shadow-mode phase. - -## Remaining open questions - -1. **Tree source — see "Rollout" above**, now resolved with a correction - to the original answer (`bind_query_expr`, not - `implement_promql_for_asap_tier`). `data_plane` still needs a *direct* - `asap-sketch` (and `asap-ir`) dependency, pinned to match `control_plane`'s - exactly — implementing `SummaryExecutor` and matching `L4Node`/ - `SketchQuery` variants requires importing their defining crate directly; - consuming a function that merely returns those types isn't enough for - Rust's trait/pattern-matching rules. -2. **`SummaryMerge` for sketch families — resolved.** KLL/DDSketch/HLL/ - CMS/CountSketch (bare and heap-bearing) merge are all implemented via - `SummaryState::merge_same_family`. Partial coverage (a group missing a - sid for one part) is resolved as "fold whatever's present," not "miss - the whole group" (mirrors `SummaryMerge`'s own semantics, - ASAPController#159/#161). Accuracy-math and resize/downsample questions - remain genuinely open, not yet investigated. -3. **`topk`/`rate` post-processing**: still open. `try_topk_over_rate_fallback`/ - `try_rate_over_frequency_fallback` in `engine.rs` remain - `SketchReducer`-only; the shadow-mode rollout explicitly excludes these - shapes rather than attempting them (see "Rollout" #3) pending the - cross-repo design conversation on outer-agg-fold. -4. **Rollout — no longer just an open question**, see "Rollout" above for - the concrete shadow-mode plan. + rather than raw `Logical`/`SummaryAgg`, the right answer is `Err` + (unsupported), not a guess. + +## Rollout design + +Because planning-time re-derivation can drift from what's actually stored +(see "sizing is not guaranteed to match" above) and because the grouping +ambiguity above means some query shapes aren't yet generally answerable, +cutting serving over to this executor outright is not a safe first step. +The intended rollout shape is **shadow mode**: compute the new answer +alongside whatever the legacy path already produces, diff the two, log +discrepancies, and always return the legacy answer — mirroring +`docs/design-sketch-db-roadmap.md` § 13.2's documented (previously +unimplemented) pattern for exactly this kind of migration: + +```rust +let old = legacy_path.evaluate(...); +if shadow_mode_enabled() { + let new = new_executor.execute(...); + log_diff(old, new); // never affects what's returned +} +old +``` + +A query shape known to bind successfully but answer *differently* under the +two paths (rather than simply not binding) must be excluded from the +comparison rather than compared naively — e.g. `rate()`/`irate()` bind to a +valid tree (the underlying intent is rewritten to `Increase` before +accumulator choice), but this executor has no rate-division step, so a naive +comparison would show a spurious, not a real, discrepancy. + +Actually switching what's served, and retiring the legacy reducer path +entirely, both require: confidence data from shadow mode about how often +planning-time re-derivation disagrees with what's stored, a resolution for +the grouping ambiguity above (at least for the query shapes real traffic +exercises), and a resolution for the outer-fold family of gaps below — none +of which shadow mode alone produces; it only makes the size and shape of +those gaps observable. + +## Open design questions + +1. **Grouping ambiguity for empty, sketch-family `by`** — see "Grouping + semantics" above. Affects every sketch family; needs either an upstream + IR signal or a caller-supplied one, not a per-metric special case. +2. **Outer-fold family of gaps.** Two PromQL compositions the flat + `Capability`/`by` vocabulary can't express are handled today only by + bespoke, PromQL-string-level fallbacks outside the reducer proper: + `topk(K, sum by (...) (rate(m[r])))` (ranking by a non-additive measure + over a rate) and stacking an outer exact statistic (avg/stddev/count/ + group/min/max) on top of an already-computed sketch or exact-agg readout. + Neither has an equivalent in this executor; resolving either is explicitly + out of scope for a single deployment to decide unilaterally — it needs a + cross-repo design conversation, since it's really a question about what + `asap-sketch`'s IR should be able to express, not a `data_plane`-local gap. +3. **`rate(cms_metric[r])` / bare frequency-family rate.** Same "outer fold" + category as above, specific to the Frequency family. +4. **`SummaryMerge`-of-`SummaryMerge` in practice** — see "Nested queries" + above; whether this deployment's topology ever produces the two-level + case is unconfirmed. +5. **`SummaryAgg`-over-`SummaryEstimate`** — flagged upstream as open in + general; this deployment's answer (reject, don't guess) is a local + default, not a resolution of the upstream question. +6. **Sizing drift** — how often and how badly a freshly-planned + `SummaryParams` fails to match what's actually registered; only + measurable empirically once serving-time re-planning is exercised against + real traffic.