Skip to content

docs(data_plane): Step C design doc — recursive L4Node plan-executor - #409

Closed
zzylol wants to merge 6 commits into
mainfrom
docs/l4node-plan-executor-design
Closed

zzylol wants to merge 6 commits into
mainfrom
docs/l4node-plan-executor-design

Conversation

@zzylol

@zzylol zzylol commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

Summary

Design doc for Step C of the plan-shaped-serving migration (Step A: #407, Step B: #408) — planning doc, not an implementation, same spirit as data_plane/docs/sketchindex-sid-unification-plan.md.

Grounds the design in what ASAPQueryEngine::execute() actually does today (read, not assumed): a flat Vec<ASAPTierCandidate> loop that overwrites rather than folds multiple candidates, real cross-sid merge only for ExactAgg (plus a global-only HLL special case — every other grouped sketch case silently produces duplicate un-merged output series today), and two raw-AST fallbacks (try_topk_over_rate_fallback, try_rate_over_frequency_fallback) that exist because today's flat Capability vocabulary can't express those compositions.

Proposes replacing the flat loop with a real recursive walk over asap_sketch::L4Node — already built control-plane-side since Step A/B (implement_promql_for_asap_tier/implement_tree_in_with), never consumed by data_plane today — and flags four open questions that need a decision before/while implementing:

  1. Where the tree comes from (data_plane depends on asap_plan/asap_sketch directly, or calls through a control_plane seam)
  2. SummaryMerge's exact-param-match requirement for sketch families — Capability matching is family-level only (SketchKindHandle, no SummaryParams), so it can't see whether two candidate sids actually share the same k/width/depth
  3. Where rank-and-slice post-processing (topk ranking by a non-count_over_time measure) lives — turns out the reason it doesn't tree-ify today is narrower than an earlier draft claimed (AggIntent::TopK can compose with an inner aggregate, just only for the RankingMeasure::Frequency case; the NonAdditive case lowers to Sort{Limit{Aggregate}}, which implement_tree_in_with just doesn't decompose yet)
  4. Rollout shape (parallel-path env-var gate vs. hard cutover)

Test plan

N/A — docs only, no code changes.

🤖 Generated with Claude Code

@zzylol
zzylol force-pushed the docs/l4node-plan-executor-design branch from 058e1bd to 45ef4ee Compare July 22, 2026 17:01
zzylol added a commit that referenced this pull request Jul 24, 2026
This doc and data_plane/docs/l4node-plan-executor-design.md (Step C,
#409, itself following Step A/#407 and Step B/#408) were written four
days apart and never cross-referenced each other. §6's RoutingIndex
assumed a flat one-query-to-one-materialization model that predates
L4Node's tree shape and doesn't account for SummaryExecutor::find_candidates
(ASAPController#155, the serving-time counterpart to this section).

As originally written, §6 would reintroduce the exact bug AccumulatorSpec
(#401) was built to close: step 5's Capability::is_satisfied_by is
family-level only and can't guarantee two candidates actually share
(SketchKind, SketchParams), which SummaryMerge requires.

Corrects three things:
- Granularity: RoutingIndex's Tier-2 lookup must be invocable per L4Node
  leaf (find_candidates is called once per SummaryAgg, possibly several
  times for one nested query), not only once per whole query.
- Match precision: find_candidates needs exact (SummaryKind,
  SummaryParams) matching via AccumulatorSpec, not family-level
  Capability -- required for anything that can feed a SummaryMerge.
- Selection semantics: find_candidates must return every exact match for
  merge_states to fold, not rank-and-pick-one like the original
  whole-query mode.

Both consumption modes can share the same columnar/interned Tier-2
structure (§6.1) -- only match precision and return shape differ by
caller. No code changes; this is a design-doc correction so
implementation (of either this or #409) doesn't have to be redone once
the two are compared.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
zzylol added a commit that referenced this pull request Jul 24, 2026
Phase 1 wasn't done as scoped (#391 closed unmerged; #392 "Phase 1b"
substituted a bigger structural fix -- direct git-dep on ASAPController's
IR crates instead of an in-tree vocabulary copy-merge). Phase 3 is
substantially done already (capability_for() routes Sum/Min/Max/Rate/
Increase to exact-agg on main) but not via this plan's sequencing, and
its own documented blocker (missing analyzer_parity_tests corpus) is
still unresolved. Phases 4-5 haven't started.

Also flags an unplanned parallel thread (#407/#408 Step A/B, merged;
#409 Step C, open) that adopts asap_plan::bind::implement_tree /
asap_sketch::L4Node directly and overlaps with what Phases 4-5 were
meant to deliver -- cross-referenced against the RoutingIndex
reconciliation just landed on design-backend-plan-wire-format.md (#389)
so Phases 4-5 get re-scoped against what that thread actually ships
before anyone executes them as originally written.

No process/plan changes here beyond recording status -- this is the
same kind of staleness correction this doc already applied to
ASAPController/docs/migration-plan.md.

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

zzylol commented Jul 25, 2026

Copy link
Copy Markdown
Contributor Author

While starting the SummaryExecutor implementation for this design: filed ASAPController#159 — asap_sketch::exec::execute() has no grouping model (SummaryAgg's by field is passed to find_candidates but never used structurally; fold_states merges everything find_candidates returns into one State). For a grouped query (quantile by (zone) (...), the normal case here, not an edge case), calling execute() naively once per query would silently merge across groups instead of producing one series per group.

Doesn't block this doc (still correctly describes the target shape), but the "proposed executor" section's find_candidates table entry should note that data_plane's implementation needs a per-group enumeration loop outside execute() (calling it once per distinct group value, with find_candidates scoped to that group via whatever per-call context data_plane's Self closes over) until/unless ASAPController#159 lands a first-class answer. Same applies to time-range: the trait carries no t0_ms/t1_ms, so data_plane needs a fresh per-query context struct (not shared mutable state on ASAPQueryEngine, which is called concurrently) rather than implementing the trait directly on the engine.

🤖 Noted with Claude Code.

zzylol added a commit that referenced this pull request Jul 25, 2026
…families

Adds RollingState::merge_same_family and a generic
cumulative_rolling_state, following the exact decode/merge/apply_delta
pattern cumulative_hll_state already used for the HLL-only global
cardinality rollup -- generalized to DD/KLL too. cumulative_hll_state
itself now delegates to the generic function (behaviorally identical:
same decode_full/merge/apply_delta_bytes calls, just reachable for any
RollingState family instead of hardcoded to Hll).

This is the cross-sid merge building block SummaryExecutor::merge_states
needs (Step C, #409): reconstruct each candidate sid's own RollingState
over the query range via cumulative_rolling_state, then fold them
together via merge_same_family before reading out one cross-sid answer
-- the same real bug evaluate_cardinality_global already fixed for the
HLL-global special case (issue: every other grouped sketch case still
emits duplicate un-merged series today), generalized so it isn't
HLL-only anymore.

Also picks up asap_sketchlib's updated Cargo.lock entry (serde_bytes
dependency, from the msgpack wire format work already on asap_sketchlib
main) via the local path-patch sibling checkout.

Verified: cargo test -p data_plane (sketch_db module) -- 294 passed, 0
failed, including the existing HLL global-cardinality tests this
refactor touches indirectly.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
zzylol added a commit that referenced this pull request Jul 25, 2026
…ardinality

Step C of the plan-shaped-serving migration
(data_plane/docs/l4node-plan-executor-design.md), scoped to cumulative
(instant) quantile/cardinality queries over the DDSketch/Kll/Hll
families -- the ones `RollingState` (delta_apply.rs, generalized in the
prior commit) covers. See the module doc for exactly what's in and out
of scope for this first cut (per-window/matrix output and the Frequency
family are explicitly deferred, not silently mishandled).

`QueryExecutionContext` implements `asap_sketch::exec::SummaryExecutor`
-- constructed fresh per incoming query (never shared, never mutated),
carrying `t0_ms`/`t1_ms`/`is_cumulative` as plain fields. The trait
itself has no time-range parameter and `ASAPQueryEngine` is called
concurrently, so this is the safe alternative to threading the range
through shared mutable state on the engine.

- `find_candidates`: walks the `SummaryAgg`'s child subtree down to a
  `Scan { source: Source::TimeSeries { metric }, .. }` to recover the
  metric (mirrors `control_plane::asap_tier_implement::collect_aggregate_roots`'s
  recursion style over the same `QueryExpr` type), resolves `by`
  `ColumnId`s to names against the child's `L4Schema`, and filters
  candidate sids by EXACT `(SummaryKind, SummaryParams)` match (not the
  family-level `Capability::is_satisfied_by` the legacy analyzer path
  uses) -- required so `SummaryMerge`'s precondition holds by
  construction.
- `fetch_state`/`merge_states` are deliberately lazy: they just
  accumulate a group's sid list. The real cross-sid merge (the actual
  fix for the "duplicate un-merged series" gap #409 flagged) happens in
  `readout`, via `cumulative_rolling_state` + `RollingState::merge_same_family`
  -- reusing the exact primitives the HLL-only global-cardinality rollup
  already proved correct, generalized to DD/KLL too.
- `logical`: errors (matches today's CapabilityMiss-and-fail-over-to-archive
  contract).

Adds `asap-sketch`/`asap-ir` as direct `data_plane` dependencies
(pin-matched to `control_plane`'s, same rule as the existing
`crates/asap_types/Cargo.toml` pin comment) -- required even though
`control_plane::asap_tier_implement` already returns `Vec<Rc<L4Node>>`,
because implementing a trait on / matching variants of a type requires
importing its defining crate directly.

## Test plan
- [x] `cargo test -p data_plane --lib summary_executor` -- 6 new tests,
      all passing:
  - `single_kll_sid_quantile_readout` -- basic correctness.
  - `two_sids_same_group_actually_merge_not_just_first` -- proves real
    merge: median of two sids' disjoint value ranges lands between
    both, not at either one alone (would fail if merge silently
    dropped a sid).
  - `two_sids_different_groups_produce_two_series_not_one_merged_blob`
    -- the ASAPController#159 fix, exercised end-to-end through
    `asap_sketch::exec::execute()`: two zones produce two independent
    series with correct per-zone values, not one merged blob.
  - `hll_cardinality_readout`, `no_matching_sid_is_no_candidates`,
    `mismatched_params_does_not_match` (exact-param-match, not
    family-only).
- [x] `cargo test -p data_plane` -- full suite, only pre-existing,
      unrelated failures (verified identical on the parent commit
      before this file existed): the same
      `optimizer::rules::tests::invalid_sketch_type_override_falls_back_to_default`
      failure from the rev-bump commits, plus 4 pre-existing
      `e2e_controller_plans_and_backend_serves` CMS/CountSketch
      failures (verified identical with `git stash` against the parent
      commit).
- [x] `cargo build --workspace` -- clean.
- [x] `cargo clippy -p data_plane --lib -- -D warnings` -- zero
      warnings in this file (pre-existing warnings elsewhere in the
      crate untouched).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
zzylol and others added 5 commits July 27, 2026 09:11
Planning doc, not an implementation (same spirit as
sketchindex-sid-unification-plan.md). Grounds the design in what
ASAPQueryEngine::execute() actually does today: a flat Vec<ASAPTierCandidate>
loop that overwrites rather than folds multiple candidates, real
cross-sid merge only for ExactAgg (+ a global-only HLL special case),
and two raw-AST fallbacks (topk-over-rate, rate-over-frequency) that
exist because today's flat Capability vocabulary can't express those
compositions.

Proposes replacing the flat loop with a real recursive walk over
asap_sketch::L4Node (already built control-plane-side since Step A/B,
never consumed by data_plane), and flags four open questions that need
a decision before/while implementing: where the tree comes from,
SummaryMerge's param-equality requirement for sketch families (Capability
matching is family-level only, doesn't see SummaryParams), where
rank-and-slice post-processing (topk-over-non-frequency-measure) lives
now that the reason it doesn't tree-ify today turns out to be narrower
than it first looked, and rollout shape.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Cuts the doc from ~420 to ~100 lines -- drops exhaustive file:line
citations, quoted source blocks, and the meta-commentary correcting an
earlier draft's wrong topk-over-rate claim. Keeps the four real gaps,
the executor sketch, and the four open questions; nothing substantive
lost, just the supporting evidence that mattered for getting the design
right, not for reading it afterward.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
ASAPController#155 landed SummaryExecutor + execute() -- the generic
recursive walk over L4Node, with the merge/nesting structural rules
enforced upstream instead of reinvented here. Rewrites the "proposed
executor" section around implementing that trait (Handle=sid,
find_candidates via AccumulatorSpec-based exact param matching,
merge_states via asap_sketchlib per family) instead of a bespoke
pseudocode walk, and adds a section on which nested shapes actually
occur in this deployment's three-stage topology (SummaryAgg-of-
SummaryAgg, SummaryMerge-of-SummaryMerge, and the still-open
SummaryAgg-over-SummaryEstimate question).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The design table and open questions described the target shape before
implementation started; several turned out to differ once real code got
written. Updates:

- Status note: SummaryExecutor is implemented and tested for
  quantile/cardinality (DDSketch/Kll/Hll), both cumulative and
  per-window, in #411 (unmerged). Frequency family (CMS/CountSketch) in
  progress; ExactAgg readout and wiring execute() into the live serving
  path still not started.
- Table: Handle is SidHandle (sid + already-fetched series + decode
  params), not a bare u64 -- find_candidates needs the series anyway
  for the group key, so it's threaded through rather than re-fetched.
  merge_states/fetch_state are deliberately lazy; the real merge math
  lives in readout, which is the only place that knows
  cumulative-vs-per-window mode.
- Open question 1 (tree source): resolved as both -- the
  control_plane seam for planning-time tree construction, plus a
  direct asap-sketch/asap-ir dependency for everything serving-time
  (implementing the trait / matching its types requires importing the
  defining crate directly, not just consuming a function that returns
  those types).
- Open question 2: partially resolved -- KLL/DDSketch/HLL merge done
  (via a generalized existing HLL-only primitive, not built from
  scratch), CMS/CountSketch still open. Partial-group-coverage question
  resolved (fold whatever's present). Accuracy-math and
  resize/downsample questions remain genuinely open.
- Questions 3-4 unchanged -- neither addressed yet.

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

Rebased onto current main (was 27 commits behind, predating #416/#417/#418)
and updated the doc's content to match:

- Status note: SummaryExecutor now covers the Frequency family and
  ExactAgg(Sum/Increase) candidate matching + coverage tracking, not just
  quantile/cardinality -- these landed since the doc was last synced.
- "Today"/gap list: gap 2 (merge only existed for ExactAgg) and gap 3
  (nothing checked param agreement) are now resolved on the new path,
  marked accordingly rather than left as open problems.
- SidHandle/GroupState table entries: updated to describe the actual
  enum shape (Sketch/ExactAgg variants), not the original sketch-only
  struct design.
- New "Architecture reference" section citing ASAPController design.md's
  "Serving-time execution" section directly -- the planning-vs-serving
  split this doc's Rollout section builds on.
- "Rollout" section: replaces "still open" with an actual plan. Corrects
  a real error in the previous version -- it named
  `implement_promql_for_asap_tier` as "the seam" for tree construction,
  but that function uses the naive DefaultCostModel and has a documented,
  tracked gap where it can't realize the Frequency intent at all. The
  correct seam is `sketch_algebra::lower::bind_query_expr`
  (ControlPlaneCostModel, what main.rs's real production pipeline uses).
  Also documents why rate()/topk-over-rate/outer-agg-fold must be
  excluded from the shadow comparison entirely (not just deprioritized)
  and why shadow mode -- not a cutover -- is this phase's actual scope.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@zzylol
zzylol force-pushed the docs/l4node-plan-executor-design branch from 4ef29fd to 12f937b Compare July 27, 2026 15:14
@zzylol

zzylol commented Jul 27, 2026

Copy link
Copy Markdown
Contributor Author

Rebased onto current main (this branch was 27 commits behind, predating #416/#417/#418) and updated the doc's content:

  • Status note + gap list brought up to date: SummaryExecutor now covers the Frequency family and ExactAgg(Sum/Increase) candidate matching + coverage tracking (landed since this doc was last synced), not just quantile/cardinality. Merge-only-existed-for-ExactAgg and no-param-agreement-check are both resolved on the new path now.
  • New "Architecture reference" section citing ASAPController docs/design.md's "Serving-time execution" section directly, to anchor this doc's planning-vs-serving split in the upstream framing.
  • "Rollout" section replaces "still open" (question docs: design for SimpleMapStore persistence (mem limit + disk flush) #4) with an actual plan — and corrects a real error from the original version: it named implement_promql_for_asap_tier as "the seam" for tree construction, but that function uses the naive DefaultCostModel and has its own documented, tracked gap where it can't realize the Frequency intent at all. The correct seam is sketch_algebra::lower::bind_query_expr (ControlPlaneCostModel, what main.rs's real production pipeline actually uses). Also documents why rate()/topk-over-rate/outer-agg-fold must be excluded from the shadow comparison entirely, and why shadow mode (not a cutover) is the actual scope of the next phase.

🤖 Generated with Claude Code

Restructures the whole doc away from implementation-status tracking
("Status (date)", PR references, "resolved on the new path" narration)
into a straightforward architecture/interface description: what
SummaryExecutor's contract is, how data_plane's data model realizes it,
what the grouping/readout/coverage/ExactAgg designs are, and what's
genuinely still an open design question versus already settled.

Adds one new, substantive open design question found while working on
the shadow-mode rollout: grouping semantics for an empty `by` are
ambiguous between "no PromQL grouping syntax exists for this shape at
all" (a bare per-series range function like quantile_over_time) and "an
explicit aggregation operator asked to reduce everything" (count(),
sum() with no by()) -- both produce the identical SummaryAgg{by: []}
tree, confirmed by inspecting both directly. Documents the two
independently-correct per-family defaults already in place (ExactAgg's
by=[] is unambiguous; sketch families default to the conservative
never-silently-merge behavior) and the two general resolution paths for
the sketch-family gap (an upstream IR signal, or a caller-supplied one
mirroring engine.rs's existing outer_agg/by_labels) -- framed as a
design question for a cross-repo conversation, not a local special case
to hack around.

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

zzylol commented Jul 27, 2026

Copy link
Copy Markdown
Contributor Author

Rewrote this doc entirely as a pure design/interface doc, dropping the implementation-status-tracking framing ("Status (date)", PR references, "resolved on the new path" narration). It now describes: the SummaryExecutor trait contract, how data_plane's data model realizes it (Handle/State/Value/GroupKey/Error), the grouping/readout/coverage/ExactAgg designs, the nested-query topology questions, and the shadow-mode rollout design — with a clean "Open design questions" list at the end instead of scattered inline status notes.

Added one new, substantive open question found while working on the shadow-mode rollout (#419): grouping semantics for an empty by are ambiguous between "no PromQL grouping syntax exists for this shape at all" (a bare per-series range function like quantile_over_time) and "an explicit aggregation operator asked to reduce everything" (count(), sum() with no by()) — both produce the identical SummaryAgg{by: []} tree (confirmed by inspecting both directly). This affects every sketch family, not just HLL/cardinality, so it's written up as a design question needing either an upstream IR signal or a caller-supplied one — not a per-metric special case to hack around locally.

🤖 Generated with Claude Code

@zzylol

zzylol commented Jul 29, 2026

Copy link
Copy Markdown
Contributor Author

Superseded by #425, which grounds this same territory (planning-time vs. serving-time, the SummaryExecutor rollout) in ASAPController's now-merged official ## Interface docs (ASAPController#169) rather than this doc's own proposals/open questions from before those interfaces were nailed down.

In particular, open question 2 here ("SummaryMerge's exact-param-match requirement... Capability matching is family-level only") and the broader grouping-ambiguity problem this doc's later revisions documented are addressed by #169's Reduction::{Reduce(GroupKeys), PerEntity} type — a real upstream IR signal, not something this deployment needs to solve unilaterally.

This PR's own analysis (the flat-loop/no-generic-merge/two-fallback critique of ASAPQueryEngine::execute()) is still accurate and not restated in #425 — worth keeping in mind during the actual SummaryExecutor cutover, just not carried forward as a live design doc.

@zzylol zzylol closed this Jul 29, 2026
zzylol added a commit that referenced this pull request Jul 29, 2026
…-time (#425)

Written independent of what's currently implemented, grounded entirely
in ASAPController's own current interfaces -- specifically the
`## Interface` sections added to docs/l1-query-language.md through
l5-physical-plan.md in ASAPController#169 (every signature there
verified against ASAPController main at cc18c98, 2026-07-28).

Core claim: control_plane should be a thin planning-time shell around
asap-ir/asap-l2/asap-plan/asap-sketch, contributing exactly two things
ASAPController doesn't ship -- an L5 physical planner (ASAPController
has no asap-physical crate; L5 is explicitly speculative there, real
here) and deployment-specific L4 extension points (CostModel, Matcher).
data_plane should be a thin serving-time shell implementing
SummaryExecutor once.

The gap table (S4) finds L2-L4 substantially already at this target
(thin re-export shims, ControlPlaneCostModel, SummaryFamilyMatcher) --
the two genuinely open items are L1 (adopt asap-frontend-promql,
retiring query_parser/ outright) and the serving-time cutover
(SummaryExecutor is fully implemented but not yet the live path).
L5 should NOT shrink -- it's this deployment's own permanent
contribution, not legacy debt, precisely because no asap-physical
crate exists upstream.

S5 reconciles this with data_plane/docs/l4node-plan-executor-design.md
(PR #409)'s open questions -- PR #169's Reduction::{Reduce(GroupKeys),
PerEntity} type looks like it resolves the grouping-ambiguity question
that doc flagged as blocking, via a real upstream IR signal rather than
a per-deployment heuristic.

Supersedes #409 with a version grounded in ASAPController's now-merged
official interface docs rather than proposing/guessing at them.

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.

1 participant