Skip to content

fix(sketch): SummaryAgg carries L3's Reduction directly, resolving the empty-by ambiguity - #164

Merged
zzylol merged 1 commit into
mainfrom
fix/163-summaryagg-grouping-ambiguity
Jul 28, 2026
Merged

zzylol merged 1 commit into
mainfrom
fix/163-summaryagg-grouping-ambiguity

Conversation

@zzylol

@zzylol zzylol commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

Summary

Fixes #163.

Reworked on top of #165/#166 (was previously a standalone SummaryGrouping type at L4 — see prior review discussion below). Now that Reduction::{Reduce(GroupKeys), PerEntity} exists at L3 and asap-plan::bind already decides it once at L3→L4 binding, this PR just stops flattening that decision back into an ambiguous bare Vec<ColumnId> on the way into SummaryAggSummaryAgg carries Reduction directly, verbatim, rather than introducing a parallel L4 type.

SummaryExecutor::find_candidates's by: &[ColumnId] becomes reduction: &Reduction, so every implementer (including ASAPQuery-backend's data_plane, the consumer that hit this) must handle both cases explicitly instead of guessing:

  • A bare per-series range function with no grouping syntax at all (quantile_over_time(0.99, m[10s])) — no grouping concept, must never merge across entities.
  • An aggregation operator explicitly invoked with no by(...) (count(hll_metric)) — a genuine full reduction, must merge everything into one group.

Both used to collapse to the same by: [], with nothing in the trait able to tell them apart (see the issue for a concrete two-disjoint-HLL-sids repro).

Changes

Test plan

  • Two asap-plan binding tests proving the L3 Reduction survives onto the L4 SummaryAgg node unchanged for both ambiguous cases — the classification logic itself is already covered by Design proposal: separate "what kind of computation" from "which dimensions to group by" in the intent algebra (dbt-style measure/dimension split) #165's much larger test suite (100+ conformance tests), so these are narrowly scoped to the L3→L4 passthrough.
  • New asap-sketch wiring test proving execute() passes reduction through to find_candidates unmodified.
  • Existing asap-plan/asap-sketch/e2e tests and assertions updated to the new shape.
  • cargo build --workspace / cargo test --workspace — clean.
  • cargo clippy --workspace --all-targets --all-features -- -D warnings — clean.
  • cargo fmt --all --check — clean.

🤖 Generated with Claude Code

@zzylol

zzylol commented Jul 27, 2026

Copy link
Copy Markdown
Contributor Author

I have a worry about whether this fits for SQL path. Currently it's solving a PromQL case.

…e empty-by ambiguity

Fixes #163.

Reworked on top of #165/#166: rather than introducing a parallel
SummaryGrouping type at L4 (this branch's original approach), SummaryAgg
now carries asap-ir's Reduction directly -- the same value the L3
Aggregate node it was bound from already carried. asap-plan::bind
already decides Reduce vs PerEntity once, at L3->L4 binding (#165); this
just stops flattening that decision back into an ambiguous bare
Vec<ColumnId> on the way into SummaryAgg.

`SummaryExecutor::find_candidates`'s `by: &[ColumnId]` becomes
`reduction: &Reduction`, so every implementer (including
ASAPQuery-backend's `data_plane`, the consumer that hit this) must
handle both cases explicitly instead of guessing.

Two ambiguous cases this resolves:
- A bare per-series range function with no grouping syntax at all
  (`quantile_over_time(0.99, m[10s])`) -- no grouping concept, must
  never merge across entities.
- An aggregation operator explicitly invoked with no `by(...)`
  (`count(hll_metric)`) -- a genuine full reduction, must merge
  everything into one group.

Both used to collapse to the same `by: []`, with nothing in the trait
able to tell them apart (see the issue for a concrete two-disjoint-HLL-
sids repro).

## Test plan
- [x] Two `asap-plan` binding tests proving the L3 Reduction survives
      onto the L4 SummaryAgg node unchanged for both ambiguous cases --
      the classification logic itself is already covered by #165's
      much larger test suite (100+ conformance tests), so these are
      narrowly scoped to the L3->L4 passthrough.
- [x] New `asap-sketch` wiring test proving `execute()` passes
      `reduction` through to `find_candidates` unmodified.
- [x] Existing `asap-plan`/`asap-sketch`/e2e tests and assertions
      updated to the new shape.
- [x] `cargo build --workspace` / `cargo test --workspace` -- clean.
- [x] `cargo clippy --workspace --all-targets --all-features -- -D
      warnings` -- clean.
- [x] `cargo fmt --all --check` -- clean.

Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
@zzylol
zzylol force-pushed the fix/163-summaryagg-grouping-ambiguity branch from 0dc8c4c to 0120a41 Compare July 28, 2026 20:53
@zzylol zzylol changed the title fix(sketch): SummaryAgg gets a SummaryGrouping, resolving the empty-by ambiguity fix(sketch): SummaryAgg carries L3's Reduction directly, resolving the empty-by ambiguity Jul 28, 2026
@zzylol
zzylol merged commit ea1ffcc into main Jul 28, 2026
2 of 3 checks passed
@zzylol
zzylol deleted the fix/163-summaryagg-grouping-ambiguity branch July 28, 2026 20:57
zzylol added a commit that referenced this pull request Jul 28, 2026
Rebased onto main (now past #162/#164/#165/#166) after PR #157's original
37-commit history hit real conflicts with the intervening Reduction/L4
grouping-model work -- squashed to one commit here since the PR merges
as a squash anyway, so the intermediate history wasn't worth preserving
through a 37-way conflict resolution.

`docs/design.md` was a 1500+ line document mixing layer design, migration
planning, and crate-by-crate speculation, most of it already stale
relative to the shipped code. This does the same move `l4node-execution-
model.md` already made for L4 across the rest of the pipeline: each layer
gets its own doc, `design.md` becomes a thin index into them, and every
doc is cross-checked against current code rather than copied from old
drafts.

- New per-layer docs: `l1-query-language.md`, `l2-logical-plan.md`,
  `l3-intent-algebra.md`, `l5-physical-plan.md` (l4's already existed,
  renamed to `l4-summary-bound-ir.md` to match the shipped
  `SummaryExpr`/`SummaryKind`/`L4Node` naming instead of the older
  "sketch" framing). `l1-query-language.md` also absorbs the old
  standalone `promql-lowering.md` as a section.
- `design.md` cut to a slim index: one paragraph per L1-L5 linking to its
  doc, plus a trimmed Glossary.
- Deleted `docs/migration-plan.md` and `docs/intent-algebra-
  reconciliation.md` -- both were migration-era planning artifacts, the
  latter already self-annotated as a superseded historical snapshot.
- `crates/sketch/src/exec.rs`'s doc comments repointed at the renamed
  `l4-summary-bound-ir.md`.

Known gap carried over from the original PR (not introduced by this
rebase): README.md still links to `docs/migration-plan.md` and to
`docs/design.md`'s old §3/§6/§8/§9/§11/§12 section numbers, none of
which exist after this change -- worth a follow-up.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
zzylol added a commit that referenced this pull request Jul 28, 2026
Rebased onto main (now past #162/#164/#165/#166) after PR #157's original
37-commit history hit real conflicts with the intervening Reduction/L4
grouping-model work -- squashed to one commit here since the PR merges
as a squash anyway, so the intermediate history wasn't worth preserving
through a 37-way conflict resolution.

`docs/design.md` was a 1500+ line document mixing layer design, migration
planning, and crate-by-crate speculation, most of it already stale
relative to the shipped code. This does the same move `l4node-execution-
model.md` already made for L4 across the rest of the pipeline: each layer
gets its own doc, `design.md` becomes a thin index into them, and every
doc is cross-checked against current code rather than copied from old
drafts.

- New per-layer docs: `l1-query-language.md`, `l2-logical-plan.md`,
  `l3-intent-algebra.md`, `l5-physical-plan.md` (l4's already existed,
  renamed to `l4-summary-bound-ir.md` to match the shipped
  `SummaryExpr`/`SummaryKind`/`L4Node` naming instead of the older
  "sketch" framing). `l1-query-language.md` also absorbs the old
  standalone `promql-lowering.md` as a section.
- `design.md` cut to a slim index: one paragraph per L1-L5 linking to its
  doc, plus a trimmed Glossary.
- Deleted `docs/migration-plan.md` and `docs/intent-algebra-
  reconciliation.md` -- both were migration-era planning artifacts, the
  latter already self-annotated as a superseded historical snapshot.
- `crates/sketch/src/exec.rs`'s doc comments repointed at the renamed
  `l4-summary-bound-ir.md`.

Known gap carried over from the original PR (not introduced by this
rebase): README.md still links to `docs/migration-plan.md` and to
`docs/design.md`'s old §3/§6/§8/§9/§11/§12 section numbers, none of
which exist after this change -- worth a follow-up.

Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
zzylol added a commit to ProjectASAP/ASAPQuery-backend that referenced this pull request Jul 28, 2026
…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>
zzylol added a commit that referenced this pull request Jul 29, 2026
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>
zzylol added a commit to ProjectASAP/ASAPQuery-backend 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>
zzylol added a commit that referenced this pull request Aug 5, 2026
* 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

SummaryExecutor::find_candidates: empty by is ambiguous between "no grouping concept" and "explicit full reduction"

1 participant