Skip to content

feat(sketch): SummaryExecutor — serving-time execution model for L4Node - #155

Merged
zzylol merged 3 commits into
mainfrom
feat/summary-executor-interface
Jul 22, 2026
Merged

feat(sketch): SummaryExecutor — serving-time execution model for L4Node#155
zzylol merged 3 commits into
mainfrom
feat/summary-executor-interface

Conversation

@zzylol

@zzylol zzylol commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

Summary

asap-sketch defines the L4 tree shape and asap-plan defines how to build one from L3, but nothing said what it means to run one against already-materialized state at query time — every deployment answering queries would otherwise reinvent the same recursive walk and merge preconditions independently, with no shared vocabulary to catch divergence.

  • SummaryExecutor — the trait a deployment implements to plug in its own storage/lookup/sketch-math/readout (Handle/State/Value/Error associated types, find_candidates/fetch_state/merge_states/readout/logical methods).
  • execute() — the generic recursive walk over L4Node that calls into it. asap-sketch owns the structural rules: which nestings are valid (only SummaryAgg/SummaryMerge can be a SummaryMerge child — not a SummaryEstimate or Logical, which have already collapsed to a value), and that a SummaryMerge's children must agree on (SummaryKind, SummaryParams), checked generically and propagated through arbitrary nesting depth via ExecOutcome::State rather than re-derived per node from L4Schema.
  • The deployment owns everything requiring actual sketch math (this crate has none — no KLL/CMS/HLL/DDSketch algorithm lives here) or actual storage.

Full design rationale in docs/l4node-execution-model.md; the module's own doc comments (crates/sketch/src/exec.rs) are the authoritative detail, including the deliberately-left-open question of whether a SummaryAgg can ever validly sit above a SummaryEstimate (building a new summary from another summary's query-time-derived readout, vs. only from ingest-time raw samples).

Motivated by ASAPQuery-backend's Step C (data_plane/docs/l4node-plan-executor-design.md), which will implement this trait against its sid catalog rather than defining its own ad hoc executor.

Test plan

  • cargo test -p asap-sketch — 9 new tests: single-leaf readout, multi-candidate merge, nested SummaryAgg (quantile-of-sum shape), merge-of-merges, and the three structural error cases (empty merge, a value-producing merge child, mismatched kind/params across merge children)
  • cargo build --workspace — clean
  • cargo clippy -p asap-sketch --all-targets — clean

🤖 Generated with Claude Code

@zzylol @claude
feat(sketch): SummaryExecutor -- serving-time execution model for L4Node
d17a069 
asap-sketch defines the L4 tree shape and asap-plan defines how to build
one from L3, but nothing said what it means to *run* one against
already-materialized state at query time -- every deployment answering
queries would otherwise reinvent the same recursive walk and merge
preconditions independently.

Adds SummaryExecutor (the trait a deployment implements to plug in its
own storage/lookup/sketch-math/readout) and execute() (the generic
recursive walk over L4Node that calls into it). asap-sketch owns the
structural rules -- which nestings are valid, that a SummaryMerge's
children must agree on (SummaryKind, SummaryParams), propagated through
arbitrary nesting depth via ExecOutcome::State rather than re-derived
per node. The deployment owns everything requiring actual sketch math
(this crate has none) or actual storage.

9 tests cover single-leaf readout, multi-candidate merge, nested
SummaryAgg (quantile-of-sum shape), merge-of-merges, and the three
structural error cases (empty merge, a Value-producing merge child,
mismatched kind/params across merge children).

Design writeup in a stacked follow-up PR (docs/l4node-execution-model.md);
this module's own doc comments are the authoritative detail.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@zzylol
zzylol force-pushed the feat/summary-executor-interface branch from 6230460 to d17a069 Compare July 22, 2026 17:39
@zzylol zzylol mentioned this pull request Jul 22, 2026
Merged
zzylol and others added 2 commits July 22, 2026 11:48
@zzylol @claude
docs(sketch): trim exec.rs's comments -- covered by l4node-execution-…
8f03015 
…model.md

The design rationale (planning vs. serving L4, nested-composition rules,
why each error variant exists) now lives in the stacked doc PR, not
duplicated in-line. Keeps short, standard API docs; points to
docs/l4node-execution-model.md for the reasoning instead of restating it.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@zzylol @claude
docs(sketch): L4 — planning-time vs. serving-time design writeup (#156)
42b82eb 
* docs(sketch): L4 -- planning-time vs. serving-time design writeup

Explains the split ASAPController#155 (SummaryExecutor/execute) adds:
planning-time L4 (asap_plan::bind, QueryExpr -> L4Node, a decision made
symbolically once per query shape) vs. serving-time L4 (this module,
L4Node -> Value, a lookup done on every query against whatever's
actually materialized). Covers the trait split of responsibility, the
nested-composition rules, what's out of scope, and the planned
data_plane consumer.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

* docs(sketch): point l4node-execution-model.md at exec.rs instead of duplicating it

The trait signature, nested-composition rules, and out-of-scope notes
were already verbatim in crates/sketch/src/exec.rs's module doc comments
-- this doc now just frames planning-time vs. serving-time L4 and points
there instead of keeping a second copy in sync.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

* Revert "docs(sketch): point l4node-execution-model.md at exec.rs instead of duplicating it"

This reverts commit d434e20.

* docs: move the L4 IR struct definitions from design.md into l4node-execution-model.md

design.md's "asap-sketch -- Layer 4 IR" section carried the SketchExpr
struct definition (the pre-shipped placeholder name), the per-node
schema table, and the two type-system invariants -- all planning-time
L4 content that belongs with the rest of the L4 story now that
l4node-execution-model.md exists. Moves it there, rewritten against the
actual shipped names (SummaryExpr/L4Node/L4Schema/L4DataType, real
crates/sketch/src/expr.rs field shapes) instead of the old SketchExpr/
SketchKind/GroupKey placeholders and their translation footnote.
design.md keeps a short pointer plus the two Source/data-model notes
that aren't really about the IR shape itself.

Doesn't chase every other SketchExpr mention elsewhere in design.md
(the layer table, the glossary, the worked example) -- scoped to the
one section that duplicated the actual struct definitions; broader
terminology consistency is a separate cleanup.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

* docs(sketch): list currently-supported L4 operators and SummaryKinds

SummaryMerge/SummaryJoin/SummarySubtract/SummaryDelete are real types
exec.rs (mostly) knows how to run, but nothing produces them yet --
worth being explicit about which half of "defined" vs "actually
happens" each is in, rather than leaving it implicit across the doc.
Same for SummaryKind: all 14 variants are wired into
boundary::implementation_for, split into DefaultCostModel's pick vs.
what only a custom CostModel reaches.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

* docs(sketch): rename section to "Supported operators"

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 Jul 22, 2026
@zzylol @claude
docs: split design.md's L1/L2/L3/L5 sections into their own docs
4e2809f 
Same move as l4node-execution-model.md (#155/#156) for L4: each layer
gets its own doc instead of living as a subsection of the 1500+ line
design.md, which design.md now links to instead of duplicating.

- l1-query-language.md, l2-logical-plan.md: short, these layers barely
  have design content beyond "here's the parser."
- l3-intent-algebra.md: the real content -- QueryExpr/AggIntent design
  rules, schema flow, the three-metadata-sources table. Rewritten
  against a pointer to the real source files instead of reproducing
  the full struct definitions inline, since the old inline copy here
  had already drifted stale (stale AggIntent::TopK/Rate/Increase field
  shapes, same kind of drift l4node-execution-model.md's L4 section
  had before this round of cleanup) -- points at
  crates/ir/src/intent_algebra/*.rs as the source of truth instead of
  re-copying it, so it can't drift the same way again.
- l5-physical-plan.md: moved close to verbatim -- already correctly
  labeled "planned, not yet built" from an earlier reconciliation
  pass, no staleness to fix, just relocated.

design.md keeps a two-line pointer per layer. Section 6's intro no
longer claims "(layers 1-3 + driver)" now that the layer IRs
themselves live elsewhere -- points to all five layer docs instead.
Doesn't chase every remaining cross-reference to the old section
numbers/content scattered elsewhere in design.md (glossary, worked
examples, open-questions) -- those still resolve (the §6 subsection
headers stay, now as pointers), just not rewritten in place.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@zzylol
zzylol merged commit fc09c3a into main Jul 22, 2026
1 check passed
zzylol added a commit that referenced this pull request Jul 22, 2026
@zzylol @claude
docs: split design.md's L1/L2/L3/L5 sections into their own docs
638e965 
Same move as l4node-execution-model.md (#155/#156) for L4: each layer
gets its own doc instead of living as a subsection of the 1500+ line
design.md, which design.md now links to instead of duplicating.

- l1-query-language.md, l2-logical-plan.md: short, these layers barely
  have design content beyond "here's the parser."
- l3-intent-algebra.md: the real content -- QueryExpr/AggIntent design
  rules, schema flow, the three-metadata-sources table. Rewritten
  against a pointer to the real source files instead of reproducing
  the full struct definitions inline, since the old inline copy here
  had already drifted stale (stale AggIntent::TopK/Rate/Increase field
  shapes, same kind of drift l4node-execution-model.md's L4 section
  had before this round of cleanup) -- points at
  crates/ir/src/intent_algebra/*.rs as the source of truth instead of
  re-copying it, so it can't drift the same way again.
- l5-physical-plan.md: moved close to verbatim -- already correctly
  labeled "planned, not yet built" from an earlier reconciliation
  pass, no staleness to fix, just relocated.

design.md keeps a two-line pointer per layer. Section 6's intro no
longer claims "(layers 1-3 + driver)" now that the layer IRs
themselves live elsewhere -- points to all five layer docs instead.
Doesn't chase every remaining cross-reference to the old section
numbers/content scattered elsewhere in design.md (glossary, worked
examples, open-questions) -- those still resolve (the §6 subsection
headers stay, now as pointers), just not rewritten in place.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This was referenced Jul 22, 2026
Merged
Merged
zzylol added a commit that referenced this pull request Jul 25, 2026
@zzylol @claude
fix(sketch): SummaryExecutor::execute() gets a grouping model (#161)
d4c1756 
Fixes #159.

`SummaryAgg`'s `by: Vec<ColumnId>` field was passed to `find_candidates`
but never used structurally -- `fold_states` merged *everything*
`find_candidates` returned into one `State`, and `execute` read out one
`Value` per tree. There was no concept anywhere in the walker of "one
output series per distinct group value" -- the existing test suite only
exercised ungrouped, single-scalar cases.

For a deployment whose real traffic is dominated by grouped queries
(`quantile by (zone) (...)`, the normal case, not an edge case -- the
motivating example is ASAPQuery-backend's `data_plane`), calling
`execute()` as originally specified would silently merge every group's
state into one blob instead of erroring, since nothing checked for it.

Adds `SummaryExecutor::GroupKey: Clone + Ord + Default` -- an opaque,
deployment-chosen per-group identity (e.g. a label-value map).
`find_candidates` now tags each handle it returns with the group it
belongs to; `execute` groups them itself before folding
(`fold_states`/`merge_states` only ever combine same-group states), and
every `ExecOutcome` (`State` or `Value`) is now a per-group list rather
than a single value. The ungrouped case (`by` empty, or a `Logical` leaf)
is simply a list of one entry under `GroupKey::default()` -- same shape,
not a special case.

`SummaryMerge`'s `(SummaryKind, SummaryParams)` agreement check stays
global across every group from every child (a planning-time property of
the node, fixed before any group value is known); the actual state
folding happens within each group independently, mirroring how
`evaluate_exact_agg`'s existing per-(group, window) fold already works
downstream.

No consumer depends on the pre-#159 shape yet (#155 just merged, nothing
in this workspace or ASAPQuery-backend's `control_plane`/`data_plane`
implements the trait yet), so this is a clean breaking change rather
than a migration.

## Test plan
- [x] `cargo test -p asap-sketch` -- 11 tests (9 existing, adapted to the
      new per-group-list shape via a small `only()` helper for the
      single-group assertions, + 2 new: grouped SummaryAgg produces one
      series per group; grouped SummaryMerge folds within a group, not
      across groups -- proven by asserting the merged values don't equal
      the cross-group sum).
- [x] `cargo build --workspace` / `cargo test --workspace` -- clean, no
      other crate touches this trait yet.
- [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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@zzylol