refactor(sketch,plan): summary/summary_input rename + Implementation::Summary merge by zzylol · Pull Request #170 · ProjectASAP/ASAPPlanner

zzylol · 2026-07-28T21:56:12Z

Summary

Implements the two code-level proposals raised during review of #169 (the L1-L5 docs' Interface sections, still open) — those comments asked about the field names and the Implementation shape but the fix was deliberately left as a follow-up rather than done as a docs-driven side effect. Landing it now, ahead of #169, since nothing else is currently mid-flight touching these exact call sites.

SummaryExpr field rename: sketch/sketch_input → summary/summary_input on SummaryAgg, SummaryJoin, SummaryDelete, SummaryEstimate. These names predated the "summary" umbrella term (an approximate sketch is one kind of summary, alongside an exact accumulator) and now match it.
Implementation variant merge: Sketch{kind,params} / ExactAccumulator{kind,params} (identical shapes) collapsed into one Summary{kind,params}. Added SummaryKind::is_exact() so bind_summary_agg can still recover "does this need a SummaryEstimate readout afterward?" from kind alone, instead of from the variant tag.

Every SummaryExecutor implementer's pattern-matching on SummaryExpr/Implementation is a breaking change by nature — this is a mechanical, non-behavioral rename/merge; no logic changed.

Test plan

cargo build --workspace
cargo test --workspace (all passing, including SummaryKind::is_exact coverage test and the existing #163/#165 regression tests)
cargo clippy --workspace --all-targets -- -D warnings
cargo fmt --check

🤖 Generated with Claude Code

…input; merge Implementation::Sketch/ExactAccumulator Both were flagged as follow-ups during PR #169's review of the L4 doc's Interface section (not yet merged): - `SummaryExpr`'s `sketch`/`sketch_input` fields predated this codebase's "summary" umbrella term (an approximate sketch is one kind of summary, alongside an exact accumulator) — renamed to `summary`/`summary_input` to match. - `Implementation::Sketch{kind,params}` and `::ExactAccumulator{kind,params}` carried identical shapes; the split existed only so `bind_summary_agg` could tell whether to wrap a `SummaryEstimate` readout. Collapsed into one `Implementation::Summary{kind,params}`, with a new `SummaryKind::is_exact()` recovering the same fact from `kind` alone. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

…mplementation merge landed Both were called out as pending follow-ups in this PR's review; #170 implemented them. Updates the code snippets and prose here to match the real, current shape instead of describing the old one as still-pending. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

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>

…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>

* docs: add an Interface section to each L1-L5 doc Fixes #168. PR #157 rewrote design.md + the per-layer docs to be pure principle/ design-level, stripped of code-level detail on purpose. This adds the promised follow-up: a concrete Rust API surface per layer, verified against current `main` rather than carried over from an old draft. - L1: no shared trait exists -- each front end exposes its own free functions (lower_promql*, lower_sql*), converging on the same L3 QueryExpr return type. Kept thin rather than padded, since that's the honest current shape. - L2: SchemaCatalog (the pluggable schema-source trait) + Binder + convert_root. - L3: QueryExpr's shape (a representative slice, not the full ~24-variant enum), Reduction + GroupKeys in full (small, central to this doc's own design principles), a representative AggIntent slice, and Schema. - L4: SummaryExpr/L4Node, the binding entry points + Implementation, the CostModel extension point (all default-having methods shown), and SummaryExecutor in full -- the real, load-bearing serving-time interface just resolved via #163/#164/#165 this session. - L5: nothing is built yet, so this is explicitly speculative -- recovered the PhysicalPlanner/TopologyDescriptor/StageAllocator sketch an earlier draft of this doc had (before PR #157's final rewrite trimmed it), updated to current summary-bound-IR naming (L4Node, SummaryKind) and kept the same "target to design against, not an API to depend on" framing so it can't be mistaken for shipped code. Every signature was checked against the actual current source in this repo (crates/frontend-promql, crates/frontend-sql, crates/l2, crates/ir, crates/sketch, crates/plan) rather than assumed from memory. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs: address PR #169 review comments on the L3/L4/L5 interface sections - L3: clarify reduction/GroupKeys/Reduction aren't tree nodes (they're plain data reached only through Aggregate.reduction), and expand the AggIntent listing from a representative slice to the full vocabulary. - L4: explain what "Implementation" means (the per-intent binding decision's own result type); note the sketch/summary field-naming tension (sketch/sketch_input predate this doc's own "summary" terminology) and the Sketch/ExactAccumulator merge question as deliberate follow-ups, not silently resolved here -- see PR discussion. - L5: fold "what does edge mean", "StageId values are deployment-owned", and "interface only in ASAPController" into one clarification at the top of the Interface section, plus reword StageId's own comment so its example values don't read as a fixed vocabulary. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs(l4): update Interface section now that sketch/summary rename + Implementation merge landed Both were called out as pending follow-ups in this PR's review; #170 implemented them. Updates the code snippets and prose here to match the real, current shape instead of describing the old one as still-pending. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs(l3): explain Group and CountValues in the AggIntent listing Review comment on #169: these two were listed with no explanation while every other category got at least a grouping comment. Both need one because they don't fit the "reduce to one number per group" shape every other reducer follows here. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs(l5): actually explain what a StageEdge/edge is, not just who owns StageId values The earlier fix for this review comment conflated two different questions — "what does an edge mean" and "who owns the concrete StageId values" — and only answered the second. Adds the missing explanation: an edge is a declared connection in the topology's connectivity graph, separate from which stages merely exist. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs: resolve 6 more PR #169 review comments — full listings, examples, constraints - l3: expand QueryExpr from an abbreviated slice to the full, real variant list (25 variants, matching crates/ir/src/intent_algebra/query_expr.rs). - l3: add a source-expression example per AggIntent variant, sourced from the accurate PromQL/SQL mappings already in agg_intent.rs's own doc comments. - l3: explain why the *OverTime range reducers can't be composed from Sort/Limit or any other generic node (no node exposes a TimeRange window's raw sample sequence as queryable rows). - l3: add a worked Schema example (sum by (job) (...)), showing the input/output shape difference an aggregate produces. - l2: add a concrete usage example for SchemaCatalog/Binder (PromQL's usage-derived default vs. a SQL catalog-backed case). - l4: list the per-SummaryExpr-node validity constraints (subtractable/ deletable/mergeable catalog flags, state-vs-value child requirements), correcting along the way which nodes actually carry a `summary` field of their own vs. which infer their kind from a child's output type. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs(l4): turn the per-SummaryExpr-node constraints into a table + a diagram Prose bullet list was hard to scan against 7 variants. Table gives a one-glance summary; the mermaid diagram visualizes the rule that's hardest to hold in your head from prose alone (SummaryMerge only accepts state-producing children, never a value-producing one). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs(l4): a taxonomy and recipe for composing summaries (bespoke) (#174) * docs(l4): design proposals for #171 and #172 composition gaps Stacked on #169's Interface section. Adds two concrete proposals: - #171 (exact <-> summary composition, either nesting order): a new SummaryFold node for an outer exact fold over an inner realized summary, computed generically by the shared execute walk with no new SummaryExecutor method; and a relaxed SummaryAgg input-column rule letting an outer summary read directly off an exact accumulator's own state (zero added error), while explicitly refusing the approximate- child case since that's #172's gap, not this one's. - #172 (nested approximate-over-approximate error propagation): a SummaryKind::implied_accuracy inverse-sizing function, a new CostModel::size_params child_accuracy parameter (opt-in, default ignores it, matching this trait's existing default-body convention), and a paired accepts_nested_approx gate so an outer approximate summary over an approximate child's estimate refuses by default instead of silently double-approximating. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs(l4): reframe #171/#172 proposal as a shared accuracy algebra, fold in #173 Replaces the earlier implementation-narrative version (which described specific match arms/functions in boundary.rs/bind.rs) with a design-level treatment: a formal (epsilon, delta) accuracy guarantee every SummaryKind already satisfies, a composition theorem (triangle inequality + union bound) for building a summary over another summary's output, and clean interfaces (Accuracy, ColumnRef::FromSummary, CostModel::size_params/ accepts_nested_approx, FoldOp/SummaryFold) derived from it. Shows #171 direction 2 (exact child) and #172 (approximate child) are the same composition formula at Accuracy::EXACT vs. Accuracy > 0, rather than two separate rules. Folds in #173: generalizes FromSummary/SummaryFold to N children for Hydra-style sketch-of-sketches, and adds a Bounded Accuracy variant (deterministic interval radius) for QTree-style range trees alongside the Probabilistic (epsilon, delta) variant. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs(l4): separate the design from which issue it solves; norm-aware composition Two changes, both from review: - Restructure into "the design" (guarantee, composition theorem, three interfaces — no issue numbers threaded through the reasoning) followed by a single "Which issue this solves" section mapping each piece back to #171/#172/#173, instead of interleaving issue callouts throughout. - The composition theorem assumed a single scalar Lipschitz constant L under an implicit pointwise/L1 error model. That's wrong for this catalog's own CountSketch/CountSketchWithHeap, whose guarantee is stated against the L2 norm of the residual frequency vector, not L1 -- a materially different, not-directly-comparable quantity on skewed data. Accuracy and compose now carry an explicit ErrorNorm; compose returns None on a norm mismatch instead of silently assuming L=1. Folded Accuracy::Bounded (deterministic radius, for #173's QTree case) into the same enum/compose function rather than a separate type. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs(l4): replace unified accuracy algebra with three grounded composition patterns Surveyed how real systems actually compose summaries (DGIM SICOMP'02, UnivMon SIGCOMM'16, Hydra VLDB'22, PromSketch VLDB'25) rather than inventing one generic accuracy algebra. Finding: there is no single law -- three structurally distinct patterns exist, only two have a governing theorem, and the pattern #171/#172 actually describe has none at all. - Pattern A (same statistic across partitions -- EH/DGIM windowing): real, DGIM Theorem 6/7 grounds it precisely, but composes the same statistic across a partition of the same data, not different statistics along a query's data-flow -- out of scope for #171/#172/#173, flagged as a separate future gap (sliding-window range-vector queries). - Pattern B (hash-collision routing -- Hydra's sketch of sketches): Theorem 2's asymmetric bound (multiplicative + additive-vs-global-total) doesn't fit the shared Accuracy shape -- Hydra and QTree each need their own SummaryKind fed directly from raw data, not a generalization of FromSummary/SummaryFold. This replaces the previous (incorrect) N-ary FromSummary framing for #173. - Pattern D (heterogeneous sequential nesting -- what #171/#172 actually ask about): no borrowed theorem exists for this shape in any of the four systems surveyed -- all four avoid it structurally. Split into D1 (inner exact, composes free -- #171 direction 2), D2 (outer exact fold over inner summary -- #171 direction 1, SummaryFold), and D3 (outer sketch over an inner statistic with no exact sub-linear form -- #172's actual remaining scope, narrower than "any nested approximation"). D3's compose()/accepts_nested_approx gate is now labeled honestly as this design's own first-principles proposal, not a literature result -- defaulting closed matches what every system surveyed actually does (none of them re-sketch another sketch's readout). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs(l4): recipe (not formula) for compound summary bounds, state not readout Reframes the design around the actual structural move every surveyed system (DGIM, UnivMon, Hydra, PromSketch) makes: never compose past a readout -- build the outer structure directly over the inner's state. Replaces the "three patterns" framing with a four-type taxonomy (same- kind merge; inner-exact; heterogeneous-with-derivable-bound; heterogeneous- undecidable) and, for the interesting type-3 case, a four-step recipe for deriving a compound bound instead of a formula: (1) treat inner's state as the outer's input schema, (2) check the outer's own construction algorithm can actually run over that state, (3) if so, re-derive -- not reuse -- the outer's own concentration argument against the composed randomness, (4) accept the resulting bound is pair-specific, not universal (DGIM's (1+e)^2*Cf^2/k+Cf-1+e and Hydra's Gi(1+/-e)+eps*GS are worked examples of the same recipe, not the same formula). Drops the generic Accuracy::compose()/Sensitivity mechanism from the previous draft -- it implied a bound could be computed from kind- independent accuracy values alone, which the DGIM/Hydra worked examples show is false. CostModel::size_params_composed now takes the child's concrete (kind, params) directly and has no default body: the deployment supplies its own pair-specific derivation, or the composition is refused (accepts_composed_state defaults false). FromSummary(Rc<L4Node>) is now constrained to state-producing nodes only (SummaryAgg/SummaryMerge), reusing the same rule this doc's SummaryMerge table already enforces, rather than allowing reference to a readout. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs(l4): restore readout-based composition as a distinct, legitimate mechanism Correction: the previous commit banned composing over a readout entirely, requiring FromSummary to reference only state-producing nodes. That over-corrected -- readout-based composition (mechanism 3a) is a real, general mechanism in its own right: any inner reporting a public (eps, delta) can be composed via a Lipschitz-style Sensitivity argument (triangle inequality + union bound), without needing access to the inner's raw state at all. It's strictly looser than state-based composition (3b, the DGIM/Hydra recipe) when 3b is available, but it's the only option when the inner's state genuinely isn't accessible (e.g. across a service boundary) or when 3b's construction-compatibility check fails. ColumnRef now has two variants -- FromReadout (value-producing node) and FromState (state-producing node) -- routing to two separate CostModel gates: accepts_readout_composition/sensitivity_for/compose_over_readout for 3a, accepts_state_composition/size_params_from_state for 3b. A deployment facing a novel (outer, inner) pair has a real choice between the two, not one closed gate. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs(l4): drop the "Which issue this solves" mapping section Requested in review. The design (four compound types, mechanisms 3a/3b) stands on its own; the issue-by-issue mapping was redundant restatement. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs(l4): tighten prose, cut redundant explanation No content change -- same taxonomy, same two mechanisms, same recipe, same interfaces. Trims repeated justification and meta-commentary about prior review rounds, shortens sentences, removes duplicate restatement of the DGIM/Hydra examples across steps. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs(l4): make the 3b recipe steps readable -- one idea per step, examples in a table The four recipe steps each interleaved DGIM and Hydra inline, forcing readers to track two examples at once inside every sentence. Splits the steps (now plain, one idea each) from the worked examples (DGIM/EH, Hydra, and a KLL-over-HLL counter-example), laid out as a comparison table keyed to steps 2 and 3. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs(l4): rename DGIM/EH to Exponential Histogram [Datar et al., SICOMP'02] Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>

zzylol merged commit cc18c98 into main Jul 29, 2026
1 check passed

zzylol deleted the refactor/rename-summary-fields-and-merge-implementation branch July 29, 2026 00:18

This was referenced Jul 29, 2026

feat(engine): shadow-mode SummaryExecutor comparison (no serving change) ProjectASAP/ASAPQuery-backend#419

Merged

feat(engine): serve from SummaryExecutor when it is provably safe ProjectASAP/ASAPQuery-backend#420

Merged

Navigation Menu

refactor(sketch,plan): summary/summary_input rename + Implementation::Summary merge - #170

refactor(sketch,plan): summary/summary_input rename + Implementation::Summary merge#170
zzylol merged 1 commit into
mainfrom
refactor/rename-summary-fields-and-merge-implementation

zzylol commented Jul 28, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

zzylol commented Jul 28, 2026

Summary

Test plan

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant