feat(control_plane): adopt asap_plan::bind::implement_tree (Step A) - #407
Merged
Merged
Conversation
Plan-shaped-serving scoping, Step A: asap_tier_analysis's collect_agg_intents flattens every AggIntent anywhere in a query tree into one list, then checks each independently against capability_for -- it never composes multiple accumulators into a multi-stage plan (Avg = Sum/Count stays archive-only for exactly this reason). asap_plan::bind::implement_tree_in_with already builds a genuinely compositional plan (Rc<L4Node>/SummaryExpr), but is deliberately conservative about where it looks for a realizable Aggregate: hitting any non-Aggregate node (Filter, Window, Project, ...) wraps the whole subtree as Logical with no attempt to recurse past it -- "rewriting through logical parents is the L4 rule engine's job", a deployment-specific decision asap-plan explicitly doesn't model. Adds asap_tier_implement.rs: collect_aggregate_roots mirrors collect_agg_intents's exact recursion through every QueryExpr variant, but collects Aggregate subtree roots instead of flattening AggIntents, so implement_tree_in_with can be handed each one independently. implement_promql_for_asap_tier ties parsing + this walk + implementation together, returning one Rc<L4Node> per root. Known, tracked gap (not wired into the live serving path for this reason): asap-plan has no opinion on AggIntent::Extension (control_plane's deployment-specific Frequency intent rides in one) -- it always returns Implementation::PassThrough, so a Frequency-shaped query under-realizes to Logical here today, unlike capability_for's as_frequency special case on the flat path. Pinned by implement_frequency_as_agg_test rather than silently left as a trap. Closing it is Step A's remaining scope: either an asap-plan extension hook, or a hand-rolled SummaryAgg/SummaryEstimate construction here (today impossible without reimplementing asap_plan::bind's private L4Node-building helpers). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This was referenced Jul 22, 2026
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
First step of the plan-shaped-serving scoping (control_plane/data_plane's query-serving model is currently a flat lookup -- "does some pre-registered accumulator answer this one AggIntent" -- never composing multiple accumulators into a multi-stage plan;
Avg = Sum/Countstays archive-only for exactly this reason).asap_plan::bind::implement_tree_in_withalready exists upstream and builds a genuinely compositional plan (Rc<L4Node>/SummaryExpr) from an L3QueryExpr-- but it's deliberately conservative about where it looks for a realizableAggregate: any non-Aggregatenode (Filter/Window/Project/...) immediately wraps the whole subtree asLogical, with no attempt to recurse past it. Perasap-plan's own module doc, that's a deliberate deployment-specific decision it doesn't model.control_plane/src/asap_tier_implement.rs:collect_aggregate_rootsmirrorsasap_tier_analysis::collect_agg_intents's exact recursion through everyQueryExprvariant, but collectsAggregatesubtree roots instead of flatteningAggIntents, soimplement_tree_in_withcan realize each independently.implement_promql_for_asap_tierties parsing + this walk + implementation together.asap-planhas no opinion onAggIntent::Extension(control_plane's deployment-specificFrequencyintent), so aFrequency-shaped query under-realizes toLogicalhere today, unlikecapability_for'sas_frequencyspecial case on the flat path. Pinned byimplement_frequency_as_agg_testrather than left as a silent trap. Closing it is the remaining scope before this can replace the flat analyzer -- either anasap-planextension hook, or a hand-rolled L4Node construction here (today impossible without reimplementingasap_plan::bind's private helpers).Test plan
cargo build --workspace-- cleancargo test -p control_plane --lib asap_tier_implement-- 8/8 passed, each grounded against the flat path's own existing tests (bare selector, sum, quantile, avg_over_time, nested aggregate-under-aggregate, filter-wrapped aggregate, the frequency gap)cargo test -p asap_types -p control_plane -p data_plane --lib-- control_plane 777 passed / 1 pre-existing unrelated failure (confirmed identical onmain), data_plane/asap_types unaffectedrustfmtapplied to touched files only🤖 Generated with Claude Code