refactor(main): stop constructing legacy SketchStore; diagnostics on SketchIndex - #170
Merged
Merged
Conversation
…SketchIndex Phase 5 M2.3.6g step 4 — `main.rs` no longer constructs the legacy `SketchStore` at all. The 60-LOC block that built it (with optional persistence wiring) is gone; `SketchIndex::start_persistence` remains as the single persistence path. `spawn_memory_diagnostics` rewritten to log `SketchIndex` stats (`instance_count`, `series_len`, `approx_memory_bytes`) instead of the pre-M2.3 per-`agg_id` `SketchStore::diagnostic_info`. Caller now passes `sketch_index.clone()` instead of `store.clone()`. `--cleanup-policy` and `--lock-strategy` CLI flags still parse but no longer have runtime effect (kept for backwards-compat with operator scripts; subsequent cleanup PR can deprecate them). 811/811 lib tests pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
zzylol
added a commit
that referenced
this pull request
Jul 29, 2026
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>
8 tasks
zzylol
added a commit
that referenced
this pull request
Jul 29, 2026
…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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Phase 5 M2.3.6g step 4 —
main.rsno longer constructs the legacySketchStoreat all. The 60-LOC block that built it (with optional persistence wiring) is gone;SketchIndex::start_persistenceremains as the single persistence path.spawn_memory_diagnosticsrewritten to logSketchIndexstats (instance_count,series_len,approx_memory_bytes) instead of the pre-M2.3 per-agg_idSketchStore::diagnostic_info. Caller now passessketch_index.clone()instead ofstore.clone().--cleanup-policyand--lock-strategyCLI flags still parse but no longer have runtime effect (kept for backwards-compat with operator scripts; subsequent cleanup PR can deprecate them).Test plan
cargo build --bin data_planeclean.cargo test -p data_plane --lib— 811/811 pass.What's next
store/{global,per_key,common,mod}.rs+Storetrait +StoreOutputSink— nothing in production references them now; only test fixtures still constructSketchStoredirectly.SketchIndex→SketchStore(final name; ~266 refs sed).🤖 Generated with Claude Code