feat(backfill): mirror replayed batches into SketchIndex by zzylol · Pull Request #165 · ProjectASAP/ASAPQuery-backend

zzylol · 2026-05-13T03:46:48Z

Summary

Phase 5 M2.3.6e — backfill replay now lands data in BOTH the legacy SketchStore AND SketchIndex, just like live precompute writes do after M2.3.4. Without this, archive replays would populate the legacy store but the M2.3.5b-era query path (which reads from SketchIndex) would still see empty windows.

Implementation

Factors a SketchIndex::ingest_precompute_for_agg_config(agg_cfg, output, accumulator) -> Option<u64> helper that owns the canonical sid-derivation + register + append_precompute path. Both SketchIndexSink (live ingest) and BackfillWindowProcessor (archive replay) call into it, so the two paths produce identical sids for the same (agg_cfg, label-values) tuple.

Builder-style with_sketch_index opt-in on both BackfillWindowProcessor and BackfillService. main.rs chains it when constructing the production BackfillService. Existing tests that don't supply a SketchIndex stay green (the field defaults to None).

Test plan

cargo build --bin data_plane clean.
cargo test -p data_plane --lib — 809/811 (2 pre-existing schema_timeline_dispatch failures unrelated).

🤖 Generated with Claude Code

Phase 5 M2.3.6e — backfill replay now lands data in BOTH the legacy `SketchStore` AND `SketchIndex`, just like live precompute writes do after M2.3.4. Without this, archive replays would populate the legacy store but the M2.3.5b-era query path (which reads from `SketchIndex`) would still see empty windows. Factors a `SketchIndex::ingest_precompute_for_agg_config(agg_cfg, output, accumulator) -> Option<u64>` helper that owns the canonical sid-derivation + register + append_precompute path; both `SketchIndexSink` (live ingest) and `BackfillWindowProcessor` (archive replay) call into it so the two paths produce identical sids for the same `(agg_cfg, label-values)` tuple. Builder-style `with_sketch_index` opt-in on both `BackfillWindowProcessor` and `BackfillService`. `main.rs` chains it when constructing the production `BackfillService`. Existing tests that don't supply a SketchIndex stay green (the field defaults to `None`). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

…l_plane Bumps asap-ir/asap-l2/asap-sketch/asap-plan from 64df20d9 to 94795092 (ASAPController main HEAD), picking up the resolution of ASAPController issue #163 across PRs #164/#165/#166: the new `Reduction` type, its placement on `QueryExpr::Aggregate` (replacing `by: GroupKeys`) and on `SummaryExpr::SummaryAgg` (replacing `by: Vec<ColumnId>`), and the updated `SummaryExecutor::find_candidates` signature. This commit is the compile-forced fallout of that bump on control_plane alone -- the actual data_plane find_candidates fix is the next commit. control_plane turned out to be in scope because its `intent_algebra` module RE-EXPORTS `asap_ir::intent_algebra` (`pub use`) rather than defining parallel local types, so the field rename cascades through its own L2->L3 lowerer, optimizer, CSE, cost model and physical planner. Sites split into three kinds, handled individually rather than by blind rename: * Pure passthrough (optimizer/engine.rs's recurse arm, optimizer/cse.rs, physical/allocator.rs's reconstructions, sketch_algebra/lower.rs): mechanical `by` -> `reduction`, no semantic content. * Real decision logic (intent_algebra/lower.rs): control_plane has its OWN parallel L2->L3 converter, so it needs its own copy of the `Reduction` decision, not just a rename. Ported ASAPController's rule (`is_per_series() || windowed` under an empty, non-`without` `by`), adapted to this repo's L3 shape convention -- here a range window is `Window` WRAPPING `Aggregate`, whereas ASAPController's current shape puts `TimeRange` in `Aggregate`'s child. Same structural marker for "bare range reduction with no grouping syntax," different node layout. `LQueryExpr::TopK` is always `Reduce` (a ranking never goes per-entity, even with an empty `by`). * Positional-`by` consumers that just need the key list for cost estimation / label-name recovery: use the non-panicking `reduction.group_keys().map(|k| k.keys()).unwrap_or(&[])`. physical/planner.rs's one site is provably always in the `Reduce` branch, so it uses `.expect_reduce()`. `QueryExpr::Sample { by, .. }` is deliberately untouched -- it's a separate field that #165 does not cover (per ASAPController's own design docs, `Sample` is not a reduction). Test fixtures updated to state their intent explicitly rather than leaning on an empty `by`: windowed no-`by` shapes (mirroring `quantile_over_time(m[r])`) become `PerEntity`; unwindowed aggregation operators and TopK become `Reduce([])`. Verified: `cargo check -p control_plane --all-targets` clean; `cargo test -p control_plane --lib` 766 passed / 1 failed, where the one failure (optimizer::rules::tests:: invalid_sketch_type_override_falls_back_to_default) is PRE-EXISTING and unrelated -- confirmed by stashing this whole changeset and reproducing the identical failure on the parent commit. Refs ProjectASAP/ASAPPlanner#163, #164, #165, #166 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

…y-by Replaces the `sketch_group_key()` heuristic (from this branch's previous commit) with the real signal now available upstream: `find_candidates` takes `reduction: &Reduction` instead of `by: &[ColumnId]`. The previous fix only closed HALF the bug. An empty `by: Vec<ColumnId>` was genuinely ambiguous -- it could mean either of two OPPOSITE things: 1. "no grouping concept was expressible at all" -- a bare per-series range function like `quantile_over_time(m[r])`, where L3/L4 planning has no reference to any label column, so `by` is empty because the query text gave it nothing to resolve, not because anyone asked to merge. Must keep distinct series distinct. 2. "a genuine cross-series reduction over zero grouping columns" -- `count(hll_metric)`-shaped. Must merge every matching candidate into ONE answer. `sketch_group_key()` could not distinguish these, because by the time it saw a bare `[]` the distinction was already gone -- it just picked behavior (1) for the whole Sketch family and left (2) broken (documented at the time as "a true global-merge aggregate, not yet modeled at all"). That's exactly the ambiguity ASAPController#165 removed at the source, by making the reduction kind explicit on the node instead of inferring it from an empty key list. So the family-specific split (`sketch_group_key` for Sketch, `project_group_key` bare for ExactAgg) collapses into ONE `resolve_group_key()` driven by `Reduction`, applied uniformly to both candidate branches: * `PerEntity` -> the sid's own FULL label map, matching legacy `sketch_reducer.rs::evaluate_core` exactly (it passes `series_label_values` through unconditionally). Case 1, behavior unchanged. * `Reduce(by)` -> project onto `by`. When `by` is empty this naturally yields the SAME `{}` key for every candidate, correctly merging them. Case 2, newly fixed. When non-empty, group as before. `sketch_group_key()` is deleted. New test `genuine_full_reduction_merges_distinct_series_unlike_per_entity` covers case 2 directly: two HLL sids with different `zone` labels and disjoint item sets under `Reduce([])` must merge to ONE group with cardinality ~10, not stay separate at ~5 each. Its sibling `bare_per_series_query_keeps_distinct_series_separate_even_with_no_by` (case 1) is retained and now states which `Reduction` it exercises, so the two tests pin the two halves against each other. shadow_compare.rs's docs updated: the `count(hll_metric)` global-merge shape they flagged as unmodeled is now handled, so the group-set check there is purely a defensive classifier for genuinely unknown shapes. Verified: `cargo check -p data_plane --all-targets` clean; `cargo test -p data_plane --lib` 948 passed / 0 failed (23/23 in summary_executor, including both ambiguity-half tests). Closes the data_plane half of ProjectASAP/ASAPPlanner#163. Refs ProjectASAP/ASAPPlanner#164, #165 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>

…ge) (#419) * feat(engine): shadow-mode SummaryExecutor comparison (no serving change) Phase 1 of wiring SummaryExecutor into engine.rs's live query path, per data_plane/docs/l4node-plan-executor-design.md's "Rollout" section: compute the new answer alongside the legacy SketchReducer path, diff the two, log discrepancies via tracing, always return the legacy answer. Mirrors docs/design-sketch-db-roadmap.md's documented (previously unimplemented) shadow-mode pattern. Default off, gated by ASAP_SHADOW_SUMMARY_EXECUTOR (mirrors ASAP_LEGACY_DUAL_WRITE's mechanics). - l4_lowering.rs: PromQL string -> asap_sketch::L4Node, via control_plane::sketch_algebra::lower::bind_query_expr (the real ControlPlaneCostModel production planner main.rs uses) rather than asap_tier_implement::implement_promql_for_asap_tier (DefaultCostModel, which has a documented, tracked gap where it can't realize the Frequency/CMS intent at all -- confirmed empirically: a test proving count_over_time(...) realizes via bind_query_expr, mirroring the exact shape asap_tier_implement.rs's own test pins as a known gap). rate()/irate() are detected and skipped before ever binding (the Rate->Increase rewrite would otherwise succeed with a semantically wrong, un-divided comparison); topk-over-rate self-excludes via SummaryExpr::Logical (confirmed empirically, no special-case needed). - shadow_compare.rs: builds a QueryExecutionContext, runs asap_sketch::exec::execute(), converts the SummaryValue/ExactAgg result into the same series+coverage shape ASAPTierResult uses, and diffs. A real e2e run surfaced one confirmed, understood noise source (not a value bug): bare per-series range functions with no PromQL by(...) get an empty group key on the new path since find_candidates projects onto the query's by columns, while the legacy path preserves the series' own labels -- detected and logged distinctly so it doesn't drown out genuine mismatches. - engine.rs: wired into the 5 shadow-eligible call sites (both evaluate_for_capability sites, evaluate_exact_agg for Sum/Increase, evaluate_cardinality_global) -- NOT the 4 rate-related sites (evaluate_exact_agg_rate x2, try_topk_over_rate_fallback, try_rate_over_frequency_fallback), which stay untouched. - New e2e test proves shadow mode is inert: enabling the flag for an existing round-trip test's duration doesn't change whether/what it serves. Verified manually too: the full e2e suite (enabled and disabled) and the full lib/control_plane suites all produce identical results to the pre-existing baseline. Explicitly not in this round (see design doc): actually serving from the new path, retiring sketch_reducer.rs, or resolving the rate/topk-over-rate/outer-agg-fold gap (needs its own ASAPController design conversation). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(summary_executor): stop silently merging distinct series under an empty by() find_candidates's group-key construction for the Sketch family (project_group_key) collapsed to `{}` whenever the query had no explicit by(...) -- which isn't just a label-display gap, it makes execute()'s own by_group: BTreeMap<GroupKey, Vec<Handle>> fold DISTINCT, unrelated series into ONE merged answer whenever more than one sid/series matches the same metric+capability. Confirmed via a real e2e shadow-mode run: for a bare per-series range function with no PromQL by(...) (e.g. quantile_over_time(m[r]) -- as opposed to a true aggregation operator like quantile(0.9, sum by (job)(m))), the L3/L4 schema derivation has no reference to any label column at all (planning happens with zero catalog knowledge), so `by` ends up empty not as a deliberate "reduce everything" choice but because there was nothing in the query text to resolve a column against. The legacy sketch_reducer.rs::evaluate_core path never has this problem because it doesn't project through `by` for this family at all -- it passes each series' own series_label_values straight through, unconditionally. New sketch_group_key(): when by is empty, use the sid's own full label map (matching evaluate_core exactly) instead of projecting onto nothing. When non-empty, project as before (an explicit grouping WAS resolvable, e.g. quantile by (zone) (...)). Deliberately does NOT touch ExactAgg's project_group_key -- Sum/Increase map from genuine PromQL aggregation operators (sum(), increase()) where an empty by() legitimately means "reduce fully," matching evaluate_exact_agg's own identical projection. Verified this is a different, independently-correct semantics, not an oversight to also fix. New test (bare_per_series_query_keeps_distinct_series_separate_even_with_no_by) directly reproduces the bug: reverting the fix makes two distinct-median series (~25 and ~75) silently merge into one wrong combined answer (50.0) -- confirmed by temporarily backing out the fix and watching the test fail with exactly that value before restoring it. shadow_compare.rs's diff logic and doc comments updated to reflect this is now fixed at the root, not a documented-but-deferred gap -- its group-set-mismatch classifier stays as a defensive check for whatever else might still produce that shape (e.g. a true global-merge aggregate like count(hll_metric), not yet modeled by summary_executor.rs at all). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * chore: bump ASAPController pin to main (Reduction) and migrate control_plane Bumps asap-ir/asap-l2/asap-sketch/asap-plan from 64df20d9 to 94795092 (ASAPController main HEAD), picking up the resolution of ASAPController issue #163 across PRs #164/#165/#166: the new `Reduction` type, its placement on `QueryExpr::Aggregate` (replacing `by: GroupKeys`) and on `SummaryExpr::SummaryAgg` (replacing `by: Vec<ColumnId>`), and the updated `SummaryExecutor::find_candidates` signature. This commit is the compile-forced fallout of that bump on control_plane alone -- the actual data_plane find_candidates fix is the next commit. control_plane turned out to be in scope because its `intent_algebra` module RE-EXPORTS `asap_ir::intent_algebra` (`pub use`) rather than defining parallel local types, so the field rename cascades through its own L2->L3 lowerer, optimizer, CSE, cost model and physical planner. Sites split into three kinds, handled individually rather than by blind rename: * Pure passthrough (optimizer/engine.rs's recurse arm, optimizer/cse.rs, physical/allocator.rs's reconstructions, sketch_algebra/lower.rs): mechanical `by` -> `reduction`, no semantic content. * Real decision logic (intent_algebra/lower.rs): control_plane has its OWN parallel L2->L3 converter, so it needs its own copy of the `Reduction` decision, not just a rename. Ported ASAPController's rule (`is_per_series() || windowed` under an empty, non-`without` `by`), adapted to this repo's L3 shape convention -- here a range window is `Window` WRAPPING `Aggregate`, whereas ASAPController's current shape puts `TimeRange` in `Aggregate`'s child. Same structural marker for "bare range reduction with no grouping syntax," different node layout. `LQueryExpr::TopK` is always `Reduce` (a ranking never goes per-entity, even with an empty `by`). * Positional-`by` consumers that just need the key list for cost estimation / label-name recovery: use the non-panicking `reduction.group_keys().map(|k| k.keys()).unwrap_or(&[])`. physical/planner.rs's one site is provably always in the `Reduce` branch, so it uses `.expect_reduce()`. `QueryExpr::Sample { by, .. }` is deliberately untouched -- it's a separate field that #165 does not cover (per ASAPController's own design docs, `Sample` is not a reduction). Test fixtures updated to state their intent explicitly rather than leaning on an empty `by`: windowed no-`by` shapes (mirroring `quantile_over_time(m[r])`) become `PerEntity`; unwindowed aggregation operators and TopK become `Reduce([])`. Verified: `cargo check -p control_plane --all-targets` clean; `cargo test -p control_plane --lib` 766 passed / 1 failed, where the one failure (optimizer::rules::tests:: invalid_sketch_type_override_falls_back_to_default) is PRE-EXISTING and unrelated -- confirmed by stashing this whole changeset and reproducing the identical failure on the parent commit. Refs ProjectASAP/ASAPPlanner#163, #164, #165, #166 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(summary_executor): resolve grouping from L3's Reduction, not empty-by Replaces the `sketch_group_key()` heuristic (from this branch's previous commit) with the real signal now available upstream: `find_candidates` takes `reduction: &Reduction` instead of `by: &[ColumnId]`. The previous fix only closed HALF the bug. An empty `by: Vec<ColumnId>` was genuinely ambiguous -- it could mean either of two OPPOSITE things: 1. "no grouping concept was expressible at all" -- a bare per-series range function like `quantile_over_time(m[r])`, where L3/L4 planning has no reference to any label column, so `by` is empty because the query text gave it nothing to resolve, not because anyone asked to merge. Must keep distinct series distinct. 2. "a genuine cross-series reduction over zero grouping columns" -- `count(hll_metric)`-shaped. Must merge every matching candidate into ONE answer. `sketch_group_key()` could not distinguish these, because by the time it saw a bare `[]` the distinction was already gone -- it just picked behavior (1) for the whole Sketch family and left (2) broken (documented at the time as "a true global-merge aggregate, not yet modeled at all"). That's exactly the ambiguity ASAPController#165 removed at the source, by making the reduction kind explicit on the node instead of inferring it from an empty key list. So the family-specific split (`sketch_group_key` for Sketch, `project_group_key` bare for ExactAgg) collapses into ONE `resolve_group_key()` driven by `Reduction`, applied uniformly to both candidate branches: * `PerEntity` -> the sid's own FULL label map, matching legacy `sketch_reducer.rs::evaluate_core` exactly (it passes `series_label_values` through unconditionally). Case 1, behavior unchanged. * `Reduce(by)` -> project onto `by`. When `by` is empty this naturally yields the SAME `{}` key for every candidate, correctly merging them. Case 2, newly fixed. When non-empty, group as before. `sketch_group_key()` is deleted. New test `genuine_full_reduction_merges_distinct_series_unlike_per_entity` covers case 2 directly: two HLL sids with different `zone` labels and disjoint item sets under `Reduce([])` must merge to ONE group with cardinality ~10, not stay separate at ~5 each. Its sibling `bare_per_series_query_keeps_distinct_series_separate_even_with_no_by` (case 1) is retained and now states which `Reduction` it exercises, so the two tests pin the two halves against each other. shadow_compare.rs's docs updated: the `count(hll_metric)` global-merge shape they flagged as unmodeled is now handled, so the group-set check there is purely a defensive classifier for genuinely unknown shapes. Verified: `cargo check -p data_plane --all-targets` clean; `cargo test -p data_plane --lib` 948 passed / 0 failed (23/23 in summary_executor, including both ambiguity-half tests). Closes the data_plane half of ProjectASAP/ASAPPlanner#163. Refs ProjectASAP/ASAPPlanner#164, #165 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * chore: bump ASAPController pin to cc18c98 (Summary variant merge, #170) Bumps asap-ir/asap-l2/asap-sketch/asap-plan from 94795092 to cc18c987 (ASAPController main HEAD), picking up ProjectASAP/ASAPPlanner#170: 1. `SummaryExpr`'s `sketch`/`sketch_input` fields renamed to `summary`/`summary_input` on `SummaryAgg`, `SummaryJoin`, `SummaryDelete`, `SummaryEstimate`. 2. `asap_plan::boundary::Implementation::Sketch{kind,params}` / `::ExactAccumulator{kind,params}` collapsed into one `Implementation::Summary{kind,params}`, with a new `SummaryKind::is_exact()` so callers recover which case they're in from `kind` alone instead of the variant tag. Fallout, split by kind: * Pure field rename (mechanical): `control_plane/src/emit/mod.rs`, `control_plane/src/physical/colored_dag/{allocator,emitter,tests}.rs`, `control_plane/src/sketch_algebra/{physical_expr,tests}.rs`, `data_plane/src/query_engines/asap_query_engine/summary_executor.rs` (mostly test-helper `SummaryAgg{sketch: ..}` literal construction). * Real logic re-expressed against the merged variant, same behavior: - `control_plane/src/sketch_algebra/capability.rs`'s `implementation_to_capability`: was two separate match arms on the variant tag (`Sketch{kind,..}` -> approx-family capabilities, `ExactAccumulator{kind,..}` -> `ExactAgg(AggregationType)`, with a deliberate `SummaryKind::Count => None` override). Now `Summary{kind,..} if kind.is_exact()` / `Summary{kind,..}` in that order, same per-kind logic in each arm, `Count => None` override preserved verbatim. - `control_plane/src/sketch_algebra/matcher.rs`'s `SummaryFamilyMatcher::is_satisfied_by`: was `(ExactAccumulator, ExactAccumulator) => kind equality` / `(Sketch, Sketch) => sketch_family_satisfied` / `_ => false` (the `_` arm covering a variant-tag mismatch, e.g. Sketch vs ExactAccumulator, among other cases). Now both sides guarded on `kind.is_exact()` agreeing -- `(Summary, Summary) if both.is_exact()` / `(Summary, Summary) if !both.is_exact()` / `_ => false`, so a mismatched-exactness pair still falls through to `false` exactly as a mismatched-variant pair did before. The asymmetric heap-topk-also-answers-bare-frequency exception inside `sketch_family_satisfied` itself is untouched. - `control_plane/src/sketch_algebra/cost_model.rs`'s `realize_extension`: constructs `Implementation::Summary` in place of `Implementation::Sketch` (Cms is always approximate, no exactness ambiguity at this call site). Verified: `cargo check -p control_plane --all-targets` and `cargo check -p data_plane --all-targets` both clean. `cargo test -p control_plane --lib`: 766 passed / 1 failed (optimizer::rules::tests::invalid_sketch_type_override_falls_back_to_default, the same pre-existing, unrelated failure flagged on this branch's previous commit -- unaffected by this change). `cargo test -p data_plane --lib`: 948 passed / 0 failed. Refs ProjectASAP/ASAPPlanner#170 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- 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>

* feat(engine): serve from SummaryExecutor when it is provably safe 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> * refactor(live_serve): remove the grouping-ambiguity gate, now obsolete `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> --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>

zzylol merged commit 5731416 into main May 13, 2026

zzylol deleted the feat/backfill-to-sketch-index branch July 17, 2026 20:05

This was referenced Jul 28, 2026

feat(engine): shadow-mode SummaryExecutor comparison (no serving change) #419

Merged

feat(engine): serve from SummaryExecutor when it is provably safe #420

Merged

This was referenced Sep 13, 2026

feat(query): maintain shared current-series quantile and TopK state #700

Merged

test(planning): pin count() to series-count, not distinct cardinality #724

Merged

Navigation Menu

feat(backfill): mirror replayed batches into SketchIndex - #165

feat(backfill): mirror replayed batches into SketchIndex#165
zzylol merged 1 commit into
mainfrom
feat/backfill-to-sketch-index

zzylol commented May 13, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

zzylol commented May 13, 2026

Summary

Implementation

Test plan

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant