From 11e7724ec6caf21a302de60910d4aa3e236062af Mon Sep 17 00:00:00 2001 From: zz_y Date: Wed, 29 Jul 2026 05:43:29 -0600 Subject: [PATCH] feat(data_plane): default ASAP_SUMMARY_EXECUTOR_LIVE to on Per control_plane/docs/design-target-architecture.md's Part A: the serving-time cutover's stated blocker (grouping ambiguity for empty, sketch-family `by`) is already resolved -- find_candidates already branches on the real Reduction::{Reduce(GroupKeys), PerEntity} IR signal (summary_executor.rs's resolve_group_key), not an empty-key heuristic. Both unit tests and a real HTTP-level e2e test (live_serve_hll_global_count_merges_across_sids, live_serve_actually_answers_ddsketch_quantile) already prove correctness for the shapes try_serve_from_summary_executor covers. The env var stays as a kill switch (ASAP_SUMMARY_EXECUTOR_LIVE=0/false/ off), not removed -- shapes this executor self-excludes before binding (rate()/irate(), topk(K, sum by(...)(rate(...))), keyed-CMS point-estimate) still fall through to the legacy SketchReducer path unconditionally, regardless of this flag. sketch_reducer.rs itself is untouched -- it remains the permanent fallback for those shapes, not legacy debt pending deletion. live_serve.rs's own tests updated: flag_off_never_serves -> explicit set_live_env("0") (kill-switch test, was relying on unset=off); added unset_flag_defaults_to_serving pinning the new default explicitly. Verified two pre-existing e2e failures (controller_plan_to_query_full_roundtrip_{cms,count_sketch}_with_heap_topk) are unrelated to this change -- confirmed via git stash against the unmodified base commit, same 2 failures with or without this diff. cargo build clean; data_plane 957/957 lib tests pass; live_serve:: 5/5; e2e_controller_plans_and_backend_serves 12/14 (the 2 pre-existing, unrelated failures noted above); e2e_modified_otlp_sketch_path 0/0 (6 pre-existing, unrelated ignores). Co-Authored-By: Claude Sonnet 5 --- .../docs/design-target-architecture.md | 36 +++++++------- .../asap_query_engine/live_serve.rs | 47 ++++++++++++++++--- 2 files changed, 57 insertions(+), 26 deletions(-) diff --git a/control_plane/docs/design-target-architecture.md b/control_plane/docs/design-target-architecture.md index 9574829fb..550fa1d5d 100644 --- a/control_plane/docs/design-target-architecture.md +++ b/control_plane/docs/design-target-architecture.md @@ -262,34 +262,32 @@ deployment-specific detour from it. | L3 | Zero local `QueryExpr`/`AggIntent`/`Schema` definitions | Already true — `intent_algebra/{agg_intent,query_expr,relational,schema,expr_ir}.rs` are thin re-export shims with only genuinely-local residues (`Frequency` extension helpers, `PerPartitionWrap`, PromQL-ergonomic `LabelFilter`). `intent_algebra/lower.rs` (~1000 lines) remains real local code — deliberately, for two documented reasons with no ASAPController equivalent (multi-agg fusion, the windowed-Count-as-Frequency heuristic). **Effectively closed modulo `lower.rs`'s two documented exceptions.** | | L4 | One `CostModel` impl; `Rc` used directly | `sketch_algebra::cost_model::ControlPlaneCostModel` + `sketch_algebra::lower::bind_query_expr` (delegating to `implement_tree_in_with`) already match this shape. `sketch_algebra::matcher::SummaryFamilyMatcher` is the `Matcher` impl this section's serving-time §3 depends on. **Effectively closed** — `PhysicalExpr`/`L4Plan` is a thin, acceptable L5-placement wrapper around `Rc`, not a competing L4 algebra. | | L5 | Full local `PhysicalPlanner`/`TopologyDescriptor`/`StageAllocator` impl | `physical/colored_dag/*` + `emit/*` already implement this shape structurally, just not against the trait names above (no literal `PhysicalPlanner` trait exists in this repo — the free functions/structs are the de facto impl). Low-priority gap: naming/trait-alignment, not missing functionality. | -| Serving | Single `SummaryExecutor` impl is the live path | `data_plane`'s `summary_executor.rs` implements the trait fully, but is not yet the live path — `engine.rs`'s query-serving entry point still calls the legacy flat `SketchReducer`/`capability_for`-based dispatch. `live_serve.rs`/`shadow_compare.rs` exist as the rollout mechanism (both env-flag-gated, off by default). **Rollout in progress, not complete.** | +| Serving | Single `SummaryExecutor` impl is the live path | `data_plane`'s `summary_executor.rs` implements the trait fully and is **now the default-on live path** (`ASAP_SUMMARY_EXECUTOR_LIVE` default flipped from off to on — the grouping-ambiguity blocker below is resolved via `Reduction`, and both unit + e2e tests already proved correctness for the covered shapes). `sketch_reducer.rs` remains the permanent fallback for shapes this executor self-excludes before binding (`rate()`/`irate()`, `topk(K, sum by(...)(rate(...)))`, keyed-CMS point-estimate) — **not** legacy debt pending deletion, an intentional, indefinite split. | **Net reading**: L2–L4 are substantially already at target — the earlier instinct that "`intent_algebra`/`sketch_algebra` should be unnecessary once connected to ASAPController" is correct and largely *already true* -for L2–L4, not a still-open gap. The two real, still-open items are L1 -(adopt `asap-frontend-promql`, retiring `query_parser/` outright) and the -serving-time cutover (finish the `SummaryExecutor` rollout, then retire -`sketch_reducer.rs`). L5 should **not** shrink — it's this deployment's -own, permanent responsibility per ASAPController's own "no `asap-physical` +for L2–L4, not a still-open gap. The serving-time cutover is done for the +shapes `SummaryExecutor` covers (default-on); the one real, still-open +item is L1 (adopt `asap-frontend-promql`, retiring `query_parser/` +outright). L5 should **not** shrink — it's this deployment's own, +permanent responsibility per ASAPController's own "no `asap-physical` crate" status. -## 5. Open questions (carried from `data_plane/docs/l4node-plan-executor-design.md`, still unresolved) +## 5. Open questions (carried from `data_plane/docs/l4node-plan-executor-design.md`, mostly resolved) -These block the serving-time cutover in §3, not the L1 adoption in §4 — -listed here because both docs describe the same target and shouldn't +Listed here because both docs describe the same target and shouldn't drift into two different pictures of what's still open: -1. **Grouping ambiguity for empty, sketch-family `by`.** PR #169's - `l3-intent-algebra.md` interface section resolves this at the type - level — `Reduction::{Reduce(GroupKeys), PerEntity}` is exactly the - upstream IR signal this repo's design doc flagged as missing (see - `Aggregate.reduction`'s doc: "an implementer must branch on `Reduce` - vs. `PerEntity` there, not guess from an empty key list"). **This - should now be resolvable** — `find_candidates`'s `reduction` parameter - already carries the distinction; the open work is wiring - `data_plane`'s `find_candidates` impl to actually branch on it instead - of the empty-key heuristic the current draft implementation uses. +1. **Grouping ambiguity for empty, sketch-family `by`. RESOLVED.** PR + #169's `l3-intent-algebra.md` interface section resolves this at the + type level — `Reduction::{Reduce(GroupKeys), PerEntity}` is exactly + the upstream IR signal this repo's design doc flagged as missing. + `data_plane`'s `find_candidates` implementation (`summary_executor.rs`'s + `resolve_group_key`) already branches on the real `Reduction` value, + not an empty-key heuristic — confirmed by reading the current code, + not assumed. This was the stated blocker for the serving-time cutover + in §3/Part A; it's why that cutover was safe to default-on already. 2. **Outer-fold family of gaps** (`topk(K, sum by (...) (rate(m[r])))`, stacking an outer exact statistic on a sketch/exact-agg readout) — still open, still a cross-repo IR design question per the original doc. diff --git a/data_plane/src/query_engines/asap_query_engine/live_serve.rs b/data_plane/src/query_engines/asap_query_engine/live_serve.rs index 26bf44440..a8a803905 100644 --- a/data_plane/src/query_engines/asap_query_engine/live_serve.rs +++ b/data_plane/src/query_engines/asap_query_engine/live_serve.rs @@ -21,16 +21,30 @@ const LIVE_ACCURACY: AccuracyTarget = AccuracyTarget::Epsilon(0.01); /// Whether the actual serving cutover is enabled for this process. /// Mirrors `shadow_compare::shadow_summary_executor_enabled`'s exact -/// mechanics, own flag, own default (off) — this is a materially -/// riskier switch than shadow mode (it changes what's served, not just -/// what's logged), so it must never be implied by the shadow flag. +/// mechanics and own flag — this is a materially riskier switch than +/// shadow mode (it changes what's served, not just what's logged), so +/// it must never be implied by the shadow flag. +/// +/// Default flipped to **on** (control_plane/docs/design-target-architecture.md +/// §4/Part A): both unit tests and a real HTTP-level e2e test +/// (`live_serve_hll_global_count_merges_across_sids`, +/// `live_serve_actually_answers_ddsketch_quantile`) already prove +/// correctness for the shapes `try_serve_from_summary_executor` covers, +/// and the grouping-ambiguity problem that gated this default off +/// (empty-`by` ambiguity) is resolved via the real `Reduction::{Reduce, +/// PerEntity}` IR signal, not a heuristic. The env var stays as a kill +/// switch (`ASAP_SUMMARY_EXECUTOR_LIVE=0`/`false`/`off`), not removed — +/// shapes this executor self-excludes before binding (`rate()`/`irate()`, +/// `topk(K, sum by(...)(rate(...)))`, keyed-CMS point-estimate) still +/// fall through to the legacy `SketchReducer` path unconditionally, +/// regardless of this flag. pub fn summary_executor_live_enabled() -> bool { std::env::var("ASAP_SUMMARY_EXECUTOR_LIVE") .map(|v| { let v = v.trim(); - v == "1" || v.eq_ignore_ascii_case("true") || v.eq_ignore_ascii_case("on") + !(v == "0" || v.eq_ignore_ascii_case("false") || v.eq_ignore_ascii_case("off")) }) - .unwrap_or(false) + .unwrap_or(true) } /// Try to serve `query` entirely from `SummaryExecutor`. Returns `None` @@ -195,7 +209,26 @@ mod tests { } #[test] - fn flag_off_never_serves() { + fn flag_explicitly_off_never_serves() { + // Kill switch: an explicit off-spelling still disables live-serve + // even though the default (unset) is now on. + let _guard = set_live_env("0"); + let idx = ddsketch_fixture(); + let result = try_serve_from_summary_executor( + &idx, + "quantile_over_time(0.99, latency_ms[1m])", + 1_000, + 2_000, + true, + ); + assert!(result.is_none(), "flag explicitly off must never serve"); + } + + #[test] + fn unset_flag_defaults_to_serving() { + // Default flipped to on (design-target-architecture.md §4/Part A) + // -- an unset env var must serve, not fall back to the legacy + // path, for a shape this executor already proves safe. let _guard = clear_live_env(); let idx = ddsketch_fixture(); let result = try_serve_from_summary_executor( @@ -205,7 +238,7 @@ mod tests { 2_000, true, ); - assert!(result.is_none(), "flag off must never serve"); + assert!(result.is_some(), "unset flag must default to serving"); } #[test]