feat(engine): serve from SummaryExecutor when it is provably safe - #420
Conversation
d150949 to
0f96520
Compare
|
Rebased onto the updated #419 and removed the grouping-ambiguity gate entirely — it is now obsolete rather than merely simplifiable. ( Why it can go
#165 made the reduction kind explicit, and #419 made
The flag would be unconditionally Tests inverted, not droppedThese covered exactly the behavior that changed, so each now asserts the opposite:
The e2e assertion was also tightened: it previously accepted "any positive value," which a legacy fallback also satisfied — so it could not actually observe whether the new path served the query. It now requires Verification
Note this PR now serves a shape it previously declined, so the live-serve blast radius is slightly larger than when it was first opened — that is the intended effect of the upstream fix (the shape is now provably correct rather than provably ambiguous), but worth a look during review given Depends on #419. 🤖 Generated with Claude Code |
0f96520 to
5b6c639
Compare
|
Rebased onto the updated #419 to pick up ProjectASAP/ASAPPlanner#170 (the No logic in this branch's own two commits touched a renamed field or the Verification
Depends on #419. 🤖 Generated with Claude Code |
Phase 2 of the SummaryExecutor rollout (Phase 1: shadow-mode, PR #419). Adds the actual serving cutover, gated behind ASAP_SUMMARY_EXECUTOR_LIVE (default off): when a query lowers and executes cleanly and isn't the known ASAPController#163 grouping-ambiguity shape, engine.rs now serves the answer directly from SummaryExecutor and skips the legacy SketchReducer call entirely for that candidate. Every other case falls back exactly as today, so None here is indistinguishable from Phase 1. - summary_executor.rs: GroupState::exact_coverage gives ExactAgg the same coverage story SummaryValue already has (needed so live-serve can report ASAPTierResult.coverage regardless of which family answered). - l4_readout.rs (new): shared lowering + execution + conversion into ASAPTierResult's (series, coverage) shape, with the mechanical ambiguous_merge_risk gate (empty root by + >1 group). Both shadow_compare.rs and live_serve.rs now call this instead of duplicating the conversion logic. - live_serve.rs (new): the actual cutover — flag check, ambiguity gate, Some(...) means "use this instead of the legacy reducer." - engine.rs: wired into the range- and instant-query dispatch loops at the points where the legacy reducer is called; skips the redundant apply_outer_agg_fold and shadow-mode comparison when a candidate was already served live. Verified: full lib suite (955 tests) and the e2e suite pass identically with the flag on and off, including two new e2e tests proving the live path actually answers a DDSketch quantile and correctly falls back (via legacy) on the known ambiguous multi-HLL-sid count() shape. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
`L4ReadoutOutcome::ambiguous_merge_risk` and `live_serve.rs`'s use of it
existed purely as a workaround for ASAPController#163: an empty
`by: Vec<ColumnId>` was indistinguishable between "no grouping concept
applies" (the group split is correct) and "an aggregation operator asked
to reduce everything" (the groups should have merged). Unable to tell
which, `try_serve_from_summary_executor` declined to serve ANY empty-`by`
shape that produced more than one group, falling back to the legacy path.
That ambiguity no longer exists. ASAPController#165 made the reduction
kind explicit (`Reduction::{PerEntity, Reduce(GroupKeys)}`) and the
previous commit made `summary_executor.rs::resolve_group_key` act on it,
so both branches are already resolved correctly before the gate ran:
* `PerEntity` -- the multi-group split is definitionally correct (one
row per entity, never merged). Never a "risk"; the
gate could only ever DECLINE a correct answer here.
* `Reduce([])` -- every candidate shares one group key, so the outcome
has exactly one group and the `values.len() > 1`
trigger cannot fire at all.
The flag would therefore be unconditionally `false` today. Keeping it
would mean keeping a heuristic whose only remaining effect is spurious
fallback, so it's removed rather than rewritten against `Reduction`:
the field, its computation, the `root_summary_agg_by_is_empty` tree walk
that fed it, and the `live_serve.rs` early-return are all deleted.
Tests that pinned the OLD behavior are inverted rather than dropped,
since they cover exactly the case that changed:
* `flag_on_ambiguous_shape_falls_back` ->
`flag_on_global_merge_shape_is_served_merged_not_declined`: asserted
`is_none()` (declined); now asserts the shape IS served as ONE merged
series with cardinality ~6 across both sids, not ~3.
* `ambiguous_global_merge_shape_is_flagged` ->
`global_merge_shape_now_merges_instead_of_being_declined`: asserted
the flag plus TWO unmerged series; now asserts ONE merged series.
* The e2e `live_serve_ambiguous_hll_global_count_falls_back_correctly`
-> `live_serve_hll_global_count_merges_across_sids`, and its
assertion tightened from "any positive value" (which a legacy
fallback also satisfied) to ">= 1.5", which distinguishes a real
cross-sid merge (~2) from serving only one sid's registers (~1).
Verified: `cargo check -p data_plane --all-targets` clean;
`cargo test -p data_plane --lib` 956 passed / 0 failed.
Refs ProjectASAP/ASAPPlanner#163, #164, #165
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
5b6c639 to
0848fe9
Compare
Summary
Phase 2 of the
SummaryExecutorrollout (Phase 1: shadow-mode comparison, #419, merges the new path's answer alongside the legacy one and logs a diff, never affecting what's served).This PR adds the actual serving cutover, gated behind
ASAP_SUMMARY_EXECUTOR_LIVE(default off): when a query lowers and executes cleanly throughSummaryExecutorand isn't the known ASAPController#163 grouping-ambiguity shape,engine.rsnow serves the answer directly fromSummaryExecutorand skips the legacySketchReducercall entirely for that candidate. Every other outcome falls back to the legacy path exactly as it does today —Nonefrom the new gate is indistinguishable from Phase 1's shadow-only behavior.What "safe to serve" means
A query is served from the new path only when:
lower_promql_to_l4nodesucceeds (excludesrate()/irate(), unparseable queries, anything that didn't realize to a concrete binding).asap_sketch::exec::execute()returnsOk(...).ExecOutcome::Value) outcome, if the tree's rootSummaryAgg'sbywas empty AND more than one group came back, this is exactly the ambiguous shape ASAPController#163 describes — decline to serve, fall back to legacy (which resolves it today via its own bespoke special case).Changes
summary_executor.rs:GroupState::exact_coveragegivesExactAggthe same coverage storySummaryValuealready has — needed so the live-serve path can reportASAPTierResult.coverageregardless of which family (sketch vs. exact) answered.l4_readout.rs(new): shared lowering + execution + conversion intoASAPTierResult's(series, coverage)shape, with the mechanicalambiguous_merge_riskgate. Bothshadow_compare.rsandlive_serve.rsnow call this instead of duplicating the conversion logic (de-dupe, no behavior change to Phase 1's shadow logic).live_serve.rs(new): the actual cutover module — flag check (ASAP_SUMMARY_EXECUTOR_LIVE), ambiguity gate,Some(...)means "use this instead of the legacy reducer."engine.rs: wired into the range-query and instant-query dispatch loops at the point where the legacy reducer is called; skips the now-redundantapply_outer_agg_foldand shadow-mode comparison call when a candidate was already served live (there's no separate legacy answer left to diff against).Explicitly NOT in this round
sketch_reducer.rs— the ambiguous-byshapes, rate, and topk-over-rate families still need it.ASAP_SUMMARY_EXECUTOR_LIVE's default to on.Test plan
cargo build --workspace— clean.cargo test -p data_plane --lib— 955 passed, 0 failed (flag off, no behavior change).cargo test -p data_plane --test e2e_controller_plans_and_backend_serves— 12 passed / 2 known pre-existing failures (cms_with_heap_topk,count_sketch_with_heap_topk), identical result withASAP_SUMMARY_EXECUTOR_LIVE=1set.live_serve_actually_answers_ddsketch_quantile— proves the flag-on path genuinely serves (not just shadow-compares) a correct DDSketch quantile answer.live_serve_ambiguous_hll_global_count_falls_back_correctly— two distinct-service HLL sids, ungroupedcount(), flag on — proves the ambiguity gate declines to serve and the legacy fallback still answers correctly.cargo clippy -p data_plane --lib— identical warning count to the base branch (no new warnings introduced).cargo fmton touched files only.🤖 Generated with Claude Code