feat(control_plane): adopt asap_sketch::L4Node for the L4 IR (Step B) - #408
Merged
Merged
Conversation
Retires control_plane's own locally-defined, flat PhysicalExpr L4 algebra
(Logical/SketchAgg/SketchEstimate/SketchMerge/ExactAgg) in favor of
ASAPController's canonical L4 IR, asap_sketch::{SummaryExpr, L4Node} --
the same move Step 3 of the enum-unification made for
SketchKind -> SummaryKind, one layer up.
PhysicalExpr is now a thin L5 wrapper: `Committed(L4Plan)` for the
common case, plus the two Phase eps.1 placement variants. L4Plan keeps
only what asap_sketch genuinely doesn't have -- named LetBinding/Ref
fan-in sharing (asap_sketch's own DAG sharing is structural, via Rc, but
this crate's rule-firing walk still needs a name to thread a bound value
across sibling calls).
The 7 bind_kll_quantile/bind_ddsketch_quantile/bind_hll_cardinality/
bind_cms_count/bind_cms_topk/bind_exact_agg/bind_archive_only Rule
structs are retired -- their selection/sizing policy (KLL's k rungs,
DDSketch-over-KLL priority, TopK recall tiers, wire-cost tie-breaks) is
preserved verbatim in the new ControlPlaneCostModel, plugged into
asap_plan::bind::implement_tree_in_with via the CostModel trait
(rank_candidates + the new size_params hook, ASAPController#146) instead
of a bespoke dispatcher -- so schema derivation, col/by computation, and
DAG construction are asap_plan::bind's, not a forked copy.
Three node shapes get a small local pre-pass in sketch_algebra::lower
before delegating, because asap_plan::boundary::implementation_for
actively binds them to something this deployment's data plane can't (or
deliberately shouldn't) serve:
- AggIntent::Count{accuracy: Exact} would bind SummaryKind::Count, which
has no data-plane accumulator (PR #200/#201 already established this
is wrong -- reverted, stays on archive).
- AggIntent::Rate would bind its own SummaryKind::Rate; this deployment
represents Rate as an Increase accumulator (rate = increase / window,
a query-time division, not a separate accumulator).
- AggIntent::Extension (Frequency) and TopK{accuracy: Exact} both
decline to bind at all (asap_plan's Extension/exact-TopK coverage gaps
-- filed upstream as ASAPController#150 and #151); this is a real,
accepted behavior change from the retired bind_cms_count/bind_cms_topk
rules, not a bug -- see the updated tests in optimizer/rules/mod.rs
and emit/mod.rs.
Also fixes a real bug surfaced by the migration: PhysicalExpr now
carries Rc<L4Node> (asap_sketch's own DAG-sharing mechanism), so holding
a bound PhysicalExpr across an .await point made handle_plan's generated
Future !Send, breaking axum::Handler. Scoped the Rc-bearing computation
into a synchronous block that resolves down to Send-safe StageConfig
output before the first await.
Pins ASAPQuery-backend's ASAPController dependency to
12482fd77945ab5771a021c6112750fd9284f8ed (ASAPController PR #146,
CostModel::size_params + bind::logical).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
ASAPController#146 (CostModel::size_params) and #154 (bind::logical visibility) are both merged. Re-pin from the feat/costmodel-size-params-hook branch tip to main's current commit now that both land there. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Step B of the plan-shaped-serving migration (Step A: #407). Retires control_plane's own locally-defined, flat
PhysicalExprL4 algebra in favor of ASAPController's canonicalasap_sketch::{SummaryExpr, L4Node}— the same move Step 3 of the enum-unification made forSketchKind → SummaryKind, one layer up.PhysicalExprbecomes a thin L5 wrapper (Committed(L4Plan)+ the two Phase ε.1 placement variants).L4Plankeeps onlyLetBinding/Ref(asap_sketch has no equivalent — its own DAG sharing is structural, viaRc).bind_*.rsRulestructs (KLL/DDSketch/HLL/CMS-count/CMS-topk/exact-agg/archive-only) are retired. Their real selection/sizing policy is preserved verbatim in a newControlPlaneCostModel, plugged intoasap_plan::bind::implement_tree_in_withvia theCostModeltrait (rank_candidates+ the newsize_paramshook — ASAPController#146) instead of a bespoke dispatcher. Schema derivation /col/bycomputation / DAG construction now come fromasap_plan::bind, not a forked copy.sketch_algebra::lowerbefore delegating, becauseasap_plan::boundary::implementation_foractively binds them to something this deployment can't or shouldn't serve:Count{Exact}(no data-plane Count accumulator — matches the already-reverted PR feat(sid): wire ExactAgg routing — capability_for + sid metadata #200/feat(sid): L4 binder rule emits PhysicalExpr::ExactAgg for Sum/Rate/Increase/Count #201),Rate(represented as anIncreaseaccumulator, matching what the data plane actually implements), and — as real, accepted coverage regressions filed upstream —Extension/Frequency andTopK{Exact}now decline to bind at all (ASAPController#150, feat(asap_types): make aggregationId optional; backend derives from content #151).PhysicalExprnow carriesRc<L4Node>, so holding a bound value across an.awaitinhandle_planmade its Future!Send, breakingaxum::Handler. Scoped theRc-bearing computation into a sync block resolving toSend-safeStageConfigoutput before the first.await.Depends on ASAPController#146 (not yet merged) — this branch pins
ASAPControllerto12482fd77945ab5771a021c6112750fd9284f8ed, the tip of that PR's branch. Needs re-pinning tomainonce #146 merges.Test plan
cargo build --workspace— cleancargo test -p control_plane --lib— 752 passed, 1 failed (invalid_sketch_type_override_falls_back_to_default— verified failing identically on the pre-Step-B base commit, a pre-existing bug in thesketch_type_overridere-derivation logic, unrelated to this migration)cargo test -p data_plane -p asap_types --lib— unaffected (data_plane: 876 passed / 5 pre-existing failures unrelated to control_plane; asap_types: 36 passed)cargo clippy -p control_plane --all-targets— no new warnings from this change🤖 Generated with Claude Code