feat(control_plane): Phase 2 steps 3-9 -- merge L2/L3 onto asap-ir/asap-l2, relocate CSE, PromQL retarget, capability_for delegation - #395
Merged
Conversation
Column/ColumnId/DataType/Schema/CseError/cse_reuse_is_legal are now
re-exported from asap_ir::intent_algebra::schema instead of defined
locally. Unlike AggIntent's merge (Phase 1b), this needed no boundary-
conversion layer: fresh diff showed asap_ir's version is a purely
additive, backward-compatible superset of the pre-merge local type --
- Column gains `table: Option<String>` (SQL join qualifier) +
Column::new()/with_table() constructors.
- Schema gains `closed: bool` (schema-on-read completeness flag) +
column_id_qualified().
- DataType is byte-identical, no changes.
Both new fields are #[serde(default)], confirmed backward-compatible by
asap_ir's own tests. This is what made a full swap the right call here
instead of Phase 1b's boundary-conversion approach for Column/DataType --
deleted agg_intent.rs's to_asap_column/from_asap_column/to_asap_dtype/
from_asap_dtype helpers, no longer needed once there's only one
Column/DataType type.
Blast radius was much smaller than the migration plan's stale ~38-site
estimate: 15 real Column{}/Schema{} struct-literal construction sites
across 9 files needed the new field added (table: None / closed: false)
-- most of the original grep hits were field references or doc comments,
not constructions.
Verified: full workspace builds clean; control_plane's 820-test suite
passes unchanged (same 1 pre-existing failure as #392, confirmed
unrelated).
Remaining Phase 2 work (docs/migration-plan-backend-plan.md): expr_ir.rs,
query_expr.rs/relational.rs fresh diff, binder.rs/column_resolution.rs
fresh diff, cse.rs move to L4, lower.rs, PromQL/SQL frontend retarget.
…ext step) New file, re-exported from asap_ir::intent_algebra::expr_ir. Per the D2 decision in ASAPController's intent-algebra-reconciliation.md: one generic Expr<C> scalar IR shared across L2 (Expr<ColumnRef>) and L3 (Expr<ColumnId>), replacing control_plane's separate ad hoc Predicate (query_expr.rs) and ScalarExpr (relational.rs) types. Deliberately not re-exported into the crate::intent_algebra::* top-level surface yet -- query_expr::ColumnRef already claims that name, and nothing constructs Expr<C> until the query_expr.rs/relational.rs merge (next step) retargets Predicate/ScalarExpr onto L3Expr/L2Expr. This step only makes the type available; verified inert (cargo build clean, full test suite unchanged, 820 passed).
….rs onto asap-ir QueryExpr, Predicate, and their supporting types are no longer defined locally -- re-exported from asap_ir::intent_algebra. asap_ir's version is a real superset (~22 variants vs. the pre-merge 16); its output_schema_in is adopted via the inherent method rather than reimplemented. Three representational differences turned out not to be missing capability, just differently structured: - Predicate is now L3Expr (struct Predicate(pub L3Expr)); Between desugars to Compare(Ge) AND Compare(Le). - Partition doesn't exist in asap_ir -- folded into Aggregate.by: GroupKeys at construction time (intent_algebra::lower), eliminating the R11 PartitionElim rule and every Partition-specific arm. - ScalarSubquery is rejected at construction time rather than lowered: L3Expr::Column is strictly positional, so the old name-based Predicate::Column(ColumnRef::Named) hack for referencing a hoisted LetBinding has no equivalent, and neither this repo's parsers nor ASAPController's own L2 lowering construct it today. R7 SubqueryDecorrelation is deleted as a result (dead code -- its target shape is now built directly at construction time). Also fixes a real regression the merge surfaced: the Binder only collected column names from GROUP BY / TopK / Partition keys, since the pre-merge Predicate was name-based and needed no positional resolution. The new positional L3Expr::Column requires every predicate-referenced name to already be in scope, so Binder::collect_referenced_columns now also walks Filter/Aggregate.having/Join.pred/Project scalar trees. Preserves two pre-existing, deliberately-tested behaviors that a naive delegation to AggFunc::to_sketch_op() would have dropped: avg_over_time still approximates as a p50 quantile sketch, and Rate/Increase/Delta still collapse onto AggIntent::Sum (disambiguated via the separate outer_fn field) rather than adopting asap_ir's dedicated intents, per asap_tier_analysis's existing outer_fn dispatch design. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…asap-l2 relational.rs's QueryExpr/AggFunc/AggItem/SourceSpec/L2ProjectItem/ L2SortKey are no longer defined locally -- re-exported from the new asap-l2 crate (same git dependency/pinned commit as asap-ir). binder.rs and column_resolution.rs are likewise re-exports of asap_l2's versions, which are a strict superset of the ones they replace (broader column-reference collection, resolve_expr resolves a whole L2Expr tree in one generic pass instead of the old hand-rolled ScalarExpr walk). Also swaps control_plane's promql-parser dependency from crates.io 0.8 to the private ProjectASAP/promql-parser fork (pinned commit, matching what ASAPController's own frontend-promql already depends on) so the two repos parse PromQL identically going forward. asap_l2's own AggFunc->AggIntent mapping doesn't match two behaviors a real, tested consumer (asap_tier_analysis's outer_fn dispatch) still depends on -- avg_over_time as a p50 quantile-sketch approximation, and Rate/Increase/Delta collapsing onto AggIntent::Sum rather than adopting the dedicated intents asap_l2 would produce -- so lower.rs stays control_plane's own converter (not a re-export of asap_l2::lower) specifically to preserve that dispatch. Reconciling it to consume the dedicated intents directly is deliberately deferred to the PromQL frontend semantic-retarget step, alongside properly distinguishing changes/resets/delta/idelta/deriv/predict_linear (currently still bucketed together, matching pre-merge behavior exactly). AggFunc::Frequency and AggFunc::Custom(String) (control_plane-only extensions with no asap_l2 equivalent) are preserved as behavior rather than vocabulary: promql.rs now constructs plain AggFunc::Count for count_over_time (matching asap_l2's own frontend), and lower.rs recovers the Frequency-sketch routing via a grouped-or-windowed-Count trigger. Custom is dropped outright -- zero real construction sites. Partition (both at L2 and L3) is gone -- asap_l2's Aggregate carries `without: bool` directly, so control_plane's own Partition-folding logic (both in lower.rs and promql.rs's group-by handling) collapses into the same "fold keys into the nearest Aggregate" shape one layer up. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…adopt asap-plan's algorithm
Relocates the workload-level CSE pass from intent_algebra::cse to
optimizer::cse, per the Phase 0 decision: ASAPController places this
pass in crates/plan ("the cost-aware optimizer layer over the L3 intent
algebra"), not alongside the L3 IR type definitions. optimizer is the
matching layer here (R1-R12 in engine.rs, and this pass's real consumer,
optimizer::cost::workload_cost, which moves with it).
Adopts asap_plan::cse's algorithm directly: structural-equality candidate
scan (QueryExpr: PartialEq on a Vec) instead of the pre-merge Debug-
string-keyed HashMap ("{:?} is not a guaranteed-injective, stable
identity contract" per ASAPController's own comment -- a real shortcut
this repo's version was taking). CseWorkloadPlan and WorkloadCostPlan now
carry asap_ir's own BindingName/QueryId directly instead of a types_v2
wrapper, removing the boundary conversion every QueryExpr::Ref
construction site needed.
Regression check: optimizer::engine's R8 CommonSubexprElim and this pass
are not redundant despite both doing "CSE" -- R8 dedupes Scan leaves
across the branches of one Merge node inside a single query tree; this
pass dedupes Aggregate-child subtrees across the root queries of a
multi-query workload. Intra-tree vs. inter-tree, disjoint inputs, so
relocating this pass changes neither what R8 fires on nor when. Full
suite re-run confirms: 763 passed (2 new regression-guard tests ported
alongside the algorithm), same 1 pre-existing unrelated failure.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…rget to match ASAPController Adopts ASAPController's RankingMeasure/is_frequency_heavy_hitter gate for topk/bottomk instead of unconditionally forcing AggFunc::Count inside any topk(...): only descending topk ranking by count_over_time(...) now takes the heavy-hitter TopK path; everything else (bottomk, topk over a non-count measure) lowers to a generic Sort+Limit. Fixes a real correctness bug where e.g. topk(k, avg_over_time(...)) was silently treated as a Count/Frequency heavy-hitter aggregate. Rate/Increase/Changes/Resets/Delta/IDelta/Deriv/PredictLinear/ DoubleExpSmoothing now map to their own dedicated AggIntents instead of collapsing onto Sum/Count. This activates capability_for()'s existing Rate/Increase -> ExactAgg(Increase) mapping and correctly routes the archive-only counter-derivative family (Delta/Changes/Resets/...) to None instead of falsely claiming an exact-agg capability. asap_tier_analysis.rs's outer_fn is computed independently of required_capability (straight off the raw PromQL AST function name), so this only needed test-expectation updates, not a dispatch-logic redesign. Adds structural regression tests asserting the raw QueryExpr shape (TopK vs Sort+Limit) for topk/bottomk, not just the flattened ParsedQuery view. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…p50 approximation AggFunc::Avg now maps onto the literal AggIntent::Avg (matching asap_l2::lower's own mapping) instead of a p50 quantile-sketch approximation. capability_for(&AggIntent::Avg) already correctly returns None -- avg has no ASAP-tier sketch substitute, it needs a cross-policy Sum+Count join -- and ASAPController's own asap-plan treats AggIntent::Avg the same way (pass_through_intents_stay_logical keeps it a whole unsketched logical subtree). avg now routes through the same exact/archive path as Sum/Count/every archive-only intent; QeCollector::collect_op's existing catch-all exact_required = true arm already handles this with no dedicated Avg arm needed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…he exact path Same fix class as the avg_over_time correction: AggFunc::StdDev/Variance now map onto their literal AggIntent::StdDev/Variance (matching asap_l2::lower exactly) instead of the two-quantile [q(0.25), q(0.75)] "IQR proxy" approximation. capability_for already declared both archive-only (Avg | StdDev | Variance => None), so the proxy was silently claiming a QuantileApprox ASAP-tier capability neither this repo's own capability table nor ASAPController's asap-plan (pass_through_intents_stay_logical) actually backs with a real bind rule. The Merge-of-siblings fan-out this used to trigger collapses back to a plain single Aggregate. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ivergences on Count and Min/Max
Fixes the two real behavioral divergences found while auditing
capability_for against sketch_algebra::rules::dispatch (both claim to
answer the same AggIntent -> ASAP-tier-capability question but disagreed):
- Count{non-Exact}: capability_for claimed CardinalityApprox (HLL,
"distinct count" semantics); rules::dispatch's BindCmsOnCount treats
it as a bare frequency point-query (CMS). ASAPController's own
crates/plan/src/bind.rs::readout maps Count to SketchQuery::PointCount,
confirming CMS is correct -- the CardinalityApprox premise never had a
real caller anyway (distinct_over_time/COUNT(DISTINCT) always lower to
AggIntent::Cardinality, never Count). AggIntent::Count{non-Exact} is
unreachable via this repo's own PromQL frontend today regardless
(lower.rs only constructs Count{Exact} or the Extension-based
Frequency intent), so this is a consistency fix, not a live routing
change.
- Min/Max: capability_for claimed QuantileApprox (min = quantile(0),
max = quantile(1)), but no rule in sketch_algebra::rules actually
implements that -- bind_kll_quantile/bind_ddsketch_quantile only ever
match AggIntent::Quantile, never Min/Max, so the promised coverage
didn't exist and Min/Max silently fell through to archive regardless.
ASAPController's own crates/plan/src/boundary.rs treats Min/Max as an
exact mergeable accumulator (SummaryKind::MinMax), same tier as
Sum/Rate/Increase -- correct, since comparing two partial extrema
needs no approximation at all. bind_exact_agg.rs now binds
Min/Max -> AggregationType::MinMax (keyed -> MultipleMinMax), using
the data plane's already-fully-wired MinMaxAccumulator; capability_for
now returns ExactAgg(MinMax) to match.
Also fixes a stale doc-table line (Count{Exact} claimed to return
Some(ExactAgg(Sum)); the actual code has returned None since the
PR #200/#201 revert).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ary::implementation_for Bumps the ASAPController pin to 12b3054 (ProjectASAP/ASAPPlanner#138-140: CostModel interface, realize/Realization -> implementation_for/Implementation Cascades-terminology rename, CountSketch/CountSketchWithHeap SummaryKind variants, Implementation::is_satisfied_by) and adds asap-plan/asap-sketch as new git dependencies (same repo/rev as asap-ir/asap-l2; asap-plan depends only on asap-ir, so this pulls in no datafusion/front-end weight). capability_for no longer maintains its own hand-written AggIntent -> ASAP-tier capability match -- that was a second, parallel judgment kept in sync with asap-plan's own implementation_for by hand (and had already drifted twice: the Count/Min-Max divergences fixed in a prior commit). It now keeps only the Extension/Frequency special case (deployment-specific -- asap-plan has no opinion on an intent shape it can't see into, by design) and delegates everything else to asap_plan::boundary::implementation_for, translating the returned Implementation into this repo's coarser Capability vocabulary via the new implementation_to_capability helper. One deliberate override survives the delegation: Count{Exact} is forced to None (archive) rather than trusting implementation_for's ExactAccumulator claim, because the data plane has no working count accumulator (SumAccumulator conflates Sum and Count). All 772 existing tests pass unchanged -- confirms the delegation is behaviorally identical to the hand-written match it replaces, including the Min/Max and Count fixes from the prior commit. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Root cause of the 4 data_plane test failures surfaced while
re-verifying this rebase (execute_rate_dispatches_to_exact_agg_rate_reducer
and friends) -- bisected to Step 6 ("PromQL topk/rate semantic
retarget"), not the later capability_for delegation commit. Before
Step 6, rate()/increase() lowered to AggIntent::Sum (outer_fn tracked
the distinction separately), so a Sum-registered sid matched directly.
Step 6 correctly gave Rate/Increase their own AggIntent (matching
asap_plan::boundary::implementation_for's SummaryKind::Rate/Increase),
which now requires Capability::ExactAgg(Increase) -- but nothing
updated Capability::is_satisfied_by to let a Sum-registered sid answer
it, and the PR's test plan only ran `cargo test -p control_plane`, so
data_plane's own test suite (which is what actually exercises this
end-to-end) never caught the regression.
The data plane has no storage kind distinct from Sum for "Increase" in
the first place: evaluate_exact_agg_rate (sketch_reducer.rs) already
reduces Sum/MultipleSum/Increase/MultipleIncrease identically -- all
read as raw per-window deltas, divided by the coverage-aware elapsed
range. Confirmed via evaluate_exact_agg_rate's own unit tests (already
passing, unaffected by this bug) that the reducer layer was never the
problem -- only the capability-matching gate upstream of it was too
strict.
Adds Capability::is_satisfied_by's sum_satisfies_increase, mirroring
multi_pop_satisfies_single's single/multi-population direction (a
single-pop Sum can only serve a single- or multi-pop Increase
requirement if available is multi; MultipleIncrease required still
needs a multi-pop available). Does NOT relax the reverse (Increase
answering a required Sum / sum_over_time) -- that's already refused
explicitly elsewhere (issue #301: reconstructing cumulative-counter
sums from per-window deltas is unsound).
Also fixes two stale test/doc claims left over from Step 6 that
asserted or documented the pre-Step-6 "everything collapses onto
ExactAgg(Sum)" behavior as if it still held.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
zzylol
force-pushed
the
phase2/query-expr-relational-merge
branch
from
July 21, 2026 20:06
1727f30 to
490f3fc
Compare
zzylol
added a commit
that referenced
this pull request
Jul 21, 2026
…ryFamilyMatcher Step 4 of the sketch-identity unification (see scratchpad/artifacts/enum-unification-plan.md), scoped down after investigation found the "delete Capability/SketchKindHandle entirely" premise underestimated the blast radius: SketchInstanceMetadata.capability: Option<Capability> is a pervasive in-memory dispatch field across data_plane's query engine, otel ingest, http driver, reconcile lifecycle, benches, examples, and tests -- roughly 500 combined references across ~20 files (asap_tier_analysis.rs alone has 72, otel.rs 62, engine.rs 57, sketch_reducer.rs 52), far beyond what the plan's file list anticipated and well past what's safely verifiable in one PR. Per the plan's own risk-scoping guidance (exactly like Stage 4 of the earlier re-layering work got scoped down from 4 items to 3), this PR lands the real matching-logic consolidation without deleting the types themselves. What changed: - Capability::is_satisfied_by's four sketch-family arms (QuantileApprox, CardinalityApprox, FrequencyEstimate, FrequencyTopk) now delegate to a new sketch_algebra::matcher::sketch_family_satisfied free function -- the same family-compatibility rule SummaryFamilyMatcher already applies to asap_plan::Implementation values -- instead of the hand-rolled handles_compatible/handles_compatible_for_topk/ is_heap_bearing/is_frequency_family helpers (deleted). One rule table, not two. - SketchKindHandle::Any (never indexed against a concrete sketch instance, only ever appears on the required side) resolves to a concrete per-family stand-in before hitting the family check, since SummaryKind has no wildcard concept -- family-matching subsumes it. The FrequencyTopk stand-in is specifically the heap-bearing CmsWithHeap, not bare Cms: a bare stand-in would let a heap-less available sketch wrongly satisfy a top-k requirement, since bare Cms/CountSketch and CmsWithHeap/CountSketchWithHeap are members of the SAME family (related by the asymmetric "heap satisfies bare" rule, not equal). - Net effect is an intentional broadening for concrete (non-Any) required handles: family membership now decides satisfaction regardless of whether the requirement spelled out `Any` or a concrete kind (matching the plan's §5 table, which isn't conditioned on that distinction). Verified harmless: capability_for -- the only production constructor of a required Capability -- never emits a concrete handle, only Any, for every sketch-family variant. Two defensive/theoretical unit tests asserting the old exact-match behavior were updated in place with comments explaining the change. - Added exact_summary_kind_for(AggregationType) -> Option<SummaryKind>, a pure, tested mapping from data-plane's AggregationType (which conflates identity + keyed/unkeyed) onto SummaryKind's exact- accumulator identity axis, for Step 5 or merge-time reconciliation. Deliberately NOT wired into Capability::ExactAgg: doing so would silently drop the keyed-vs-unkeyed asymmetric rule (multi_pop_satisfies_single) that ExactAgg's own is_satisfied_by arm depends on, since SummaryKind alone can't distinguish a multi-population policy once Sum/MultipleSum collapse onto one variant. Fixing that properly needs a grouping sibling field, which is Step 5's territory (aggregation_config.rs) and out of scope here. Investigated and confirmed (not taken on faith): - Capability is never persisted. metadata.rs's SidMetaRecord::capability() derives it fresh from agg_kind on every load; the only durable artifact is the 8-string sketch_kind_to_str/from_str vocabulary in the same file, which this PR does not touch (SketchKindHandle is unchanged) -- sid_metadata.json round-tripping is unaffected. - timeline.rs's sketch_kind_byte fixed-mapping feeds AggSignatureGroup.signature_id, computed fresh at query time from live in-memory metadata (xxh64 over a canonical encoding), never read back from disk -- confirmed not a persistence concern, also untouched here since SketchKindHandle is unchanged. Deferred (not done in this PR): full deletion of Capability/ SketchKindHandle and re-pointing data_plane's re-export at asap_plan::Implementation directly. That remains real Step 4 scope for a future, separately-reviewed PR once each of the ~20 data_plane call sites (and the SketchInstanceMetadata.capability field itself) has been individually migrated and verified -- not safe to do speculatively here. Overlap note for the concurrent Step 5 branch (splitting data_plane's AggregationType into AccumulatorSpec): this PR does not touch aggregation_config.rs or accumulator_factory.rs. The only AggregationType-touching lines are the new exact_summary_kind_for mapping function and its tests in capability.rs (both pure additions, no existing call site changed) -- reconciliation should be low-friction. - cargo build -p control_plane -p data_plane -p asap_types: clean - cargo test -p control_plane: 827 passed (822 baseline + 5 new), 1 pre-existing unrelated failure (invalid_sketch_type_override_falls_back_to_default) - cargo test -p data_plane --lib: 881 passed, 0 failed (matches baseline exactly) Rebased onto main post-PR #395 (phase2/query-expr-relational-merge, merged after this branch was opened). #395 heavily rewrote this same file: capability_for now delegates to asap_plan::boundary::implementation_for via implementation_to_capability, and a follow-up fix on main added sum_satisfies_increase so a Sum-registered sid can answer a required Increase/Rate capability. The rebase conflict was confined to the #[cfg(test)] module: both branches appended new tests directly after exact_agg_covers_each_canonical_agg_type -- main's is_satisfied_by_sum_family_answers_required_increase / is_satisfied_by_sum_family_does_not_answer_required_multi_increase_from_single_sum / is_satisfied_by_increase_does_not_answer_required_sum (covering sum_satisfies_increase) and this branch's exact_summary_kind_for_maps_bare_identity_variants / exact_summary_kind_for_maps_keyed_siblings_onto_the_same_kind / exact_summary_kind_for_rejects_sketch_shaped_variants (covering exact_summary_kind_for). Resolution kept both blocks, each with its own closing brace (git's diff had folded them under one shared trailing `}` belonging to whichever side's last test happened to end there). Outside the tests module the rebase auto-merged cleanly: capability_for's delegation to implementation_for, is_satisfied_by's ExactAgg arm (req == have || multi_pop_satisfies_single || sum_satisfies_increase), and this PR's own sketch_kinds_compatible -> sketch_family_satisfied routing for the four sketch-family arms all coexist as intended, with no further edits needed. matcher.rs's sketch_family_satisfied free function (this PR's own commit) applied cleanly with no conflict. Post-rebase: cargo build --workspace clean. cargo test -p asap_types: 36 passed. cargo test -p control_plane --lib: 774 passed, 1 failed (optimizer::rules::tests::invalid_sketch_type_override_falls_back_to_default, confirmed pre-existing and failing on main itself, unrelated to this change). cargo test -p data_plane --lib: 881 passed, 0 failed, 2 ignored. The control_plane baseline moved from 822 to a lower number purely because main's history advanced past this branch's original fork point (unrelated intervening merges); a net-tests diff against origin/main confirms zero tests were dropped by this rebase (+5 net new, 0 removed). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
zzylol
added a commit
that referenced
this pull request
Jul 22, 2026
…ryFamilyMatcher (#402) Step 4 of the sketch-identity unification (see scratchpad/artifacts/enum-unification-plan.md), scoped down after investigation found the "delete Capability/SketchKindHandle entirely" premise underestimated the blast radius: SketchInstanceMetadata.capability: Option<Capability> is a pervasive in-memory dispatch field across data_plane's query engine, otel ingest, http driver, reconcile lifecycle, benches, examples, and tests -- roughly 500 combined references across ~20 files (asap_tier_analysis.rs alone has 72, otel.rs 62, engine.rs 57, sketch_reducer.rs 52), far beyond what the plan's file list anticipated and well past what's safely verifiable in one PR. Per the plan's own risk-scoping guidance (exactly like Stage 4 of the earlier re-layering work got scoped down from 4 items to 3), this PR lands the real matching-logic consolidation without deleting the types themselves. What changed: - Capability::is_satisfied_by's four sketch-family arms (QuantileApprox, CardinalityApprox, FrequencyEstimate, FrequencyTopk) now delegate to a new sketch_algebra::matcher::sketch_family_satisfied free function -- the same family-compatibility rule SummaryFamilyMatcher already applies to asap_plan::Implementation values -- instead of the hand-rolled handles_compatible/handles_compatible_for_topk/ is_heap_bearing/is_frequency_family helpers (deleted). One rule table, not two. - SketchKindHandle::Any (never indexed against a concrete sketch instance, only ever appears on the required side) resolves to a concrete per-family stand-in before hitting the family check, since SummaryKind has no wildcard concept -- family-matching subsumes it. The FrequencyTopk stand-in is specifically the heap-bearing CmsWithHeap, not bare Cms: a bare stand-in would let a heap-less available sketch wrongly satisfy a top-k requirement, since bare Cms/CountSketch and CmsWithHeap/CountSketchWithHeap are members of the SAME family (related by the asymmetric "heap satisfies bare" rule, not equal). - Net effect is an intentional broadening for concrete (non-Any) required handles: family membership now decides satisfaction regardless of whether the requirement spelled out `Any` or a concrete kind (matching the plan's §5 table, which isn't conditioned on that distinction). Verified harmless: capability_for -- the only production constructor of a required Capability -- never emits a concrete handle, only Any, for every sketch-family variant. Two defensive/theoretical unit tests asserting the old exact-match behavior were updated in place with comments explaining the change. - Added exact_summary_kind_for(AggregationType) -> Option<SummaryKind>, a pure, tested mapping from data-plane's AggregationType (which conflates identity + keyed/unkeyed) onto SummaryKind's exact- accumulator identity axis, for Step 5 or merge-time reconciliation. Deliberately NOT wired into Capability::ExactAgg: doing so would silently drop the keyed-vs-unkeyed asymmetric rule (multi_pop_satisfies_single) that ExactAgg's own is_satisfied_by arm depends on, since SummaryKind alone can't distinguish a multi-population policy once Sum/MultipleSum collapse onto one variant. Fixing that properly needs a grouping sibling field, which is Step 5's territory (aggregation_config.rs) and out of scope here. Investigated and confirmed (not taken on faith): - Capability is never persisted. metadata.rs's SidMetaRecord::capability() derives it fresh from agg_kind on every load; the only durable artifact is the 8-string sketch_kind_to_str/from_str vocabulary in the same file, which this PR does not touch (SketchKindHandle is unchanged) -- sid_metadata.json round-tripping is unaffected. - timeline.rs's sketch_kind_byte fixed-mapping feeds AggSignatureGroup.signature_id, computed fresh at query time from live in-memory metadata (xxh64 over a canonical encoding), never read back from disk -- confirmed not a persistence concern, also untouched here since SketchKindHandle is unchanged. Deferred (not done in this PR): full deletion of Capability/ SketchKindHandle and re-pointing data_plane's re-export at asap_plan::Implementation directly. That remains real Step 4 scope for a future, separately-reviewed PR once each of the ~20 data_plane call sites (and the SketchInstanceMetadata.capability field itself) has been individually migrated and verified -- not safe to do speculatively here. Overlap note for the concurrent Step 5 branch (splitting data_plane's AggregationType into AccumulatorSpec): this PR does not touch aggregation_config.rs or accumulator_factory.rs. The only AggregationType-touching lines are the new exact_summary_kind_for mapping function and its tests in capability.rs (both pure additions, no existing call site changed) -- reconciliation should be low-friction. - cargo build -p control_plane -p data_plane -p asap_types: clean - cargo test -p control_plane: 827 passed (822 baseline + 5 new), 1 pre-existing unrelated failure (invalid_sketch_type_override_falls_back_to_default) - cargo test -p data_plane --lib: 881 passed, 0 failed (matches baseline exactly) Rebased onto main post-PR #395 (phase2/query-expr-relational-merge, merged after this branch was opened). #395 heavily rewrote this same file: capability_for now delegates to asap_plan::boundary::implementation_for via implementation_to_capability, and a follow-up fix on main added sum_satisfies_increase so a Sum-registered sid can answer a required Increase/Rate capability. The rebase conflict was confined to the #[cfg(test)] module: both branches appended new tests directly after exact_agg_covers_each_canonical_agg_type -- main's is_satisfied_by_sum_family_answers_required_increase / is_satisfied_by_sum_family_does_not_answer_required_multi_increase_from_single_sum / is_satisfied_by_increase_does_not_answer_required_sum (covering sum_satisfies_increase) and this branch's exact_summary_kind_for_maps_bare_identity_variants / exact_summary_kind_for_maps_keyed_siblings_onto_the_same_kind / exact_summary_kind_for_rejects_sketch_shaped_variants (covering exact_summary_kind_for). Resolution kept both blocks, each with its own closing brace (git's diff had folded them under one shared trailing `}` belonging to whichever side's last test happened to end there). Outside the tests module the rebase auto-merged cleanly: capability_for's delegation to implementation_for, is_satisfied_by's ExactAgg arm (req == have || multi_pop_satisfies_single || sum_satisfies_increase), and this PR's own sketch_kinds_compatible -> sketch_family_satisfied routing for the four sketch-family arms all coexist as intended, with no further edits needed. matcher.rs's sketch_family_satisfied free function (this PR's own commit) applied cleanly with no conflict. Post-rebase: cargo build --workspace clean. cargo test -p asap_types: 36 passed. cargo test -p control_plane --lib: 774 passed, 1 failed (optimizer::rules::tests::invalid_sketch_type_override_falls_back_to_default, confirmed pre-existing and failing on main itself, unrelated to this change). cargo test -p data_plane --lib: 881 passed, 0 failed, 2 ignored. The control_plane baseline moved from 822 to a lower number purely because main's history advanced past this branch's original fork point (unrelated intervening merges); a net-tests diff against origin/main confirms zero tests were dropped by this rebase (+5 net new, 0 removed). 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
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
This PR now bundles nine consecutive Phase 2 steps (all landed on this branch before review):
Step 3 --
query_expr.rs/relational.rs's L3 side ontoasap-ir(first commit):QueryExpr/Predicateand their supporting types are no longer defined locally -- re-exported fromasap_ir::intent_algebra(~22 variants vs. the pre-merge 16).Predicateis nowL3Expr;BETWEENdesugars toCompare(Ge) AND Compare(Le).Partitiondoesn't exist inasap_ir-- folded intoAggregate.by: GroupKeysat construction time.ScalarSubqueryis rejected at construction time (no equivalent in either this repo's parsers or ASAPController's own L2 lowering today).Bindernow also walksFilter/Aggregate.having/Join.pred/Projectscalar trees for column names, required oncePredicatebecame positional.avg_over_timeas a p50 quantile-sketch approximation and (at that point)Rate/Increase/Deltacollapsing ontoAggIntent::Sum(disambiguated viaouter_fn), perasap_tier_analysis's existing dispatch design -- both later corrected, see steps 6 and 6b.Step 4 --
relational.rs's L2 side onto the newasap-l2crate (second commit):QueryExpr/AggFunc/AggItem/SourceSpec/L2ProjectItem/L2SortKey,binder.rs, andcolumn_resolution.rsare no longer defined locally -- re-exported fromasap-l2(same git dependency/pinned commit asasap-ir).promql-parserfrom crates.io0.8to the privateProjectASAP/promql-parserfork (pinned commit), matching what ASAPController'sfrontend-promqlalready depends on.lower.rsstays control_plane's own converter (not a re-export ofasap_l2::lower), at that point to preserve theavg/rate/increasebehaviors above.AggFunc::Frequency/Custom(String)(control_plane-only, noasap_l2equivalent) preserved as behavior rather than vocabulary:promql.rsconstructs plainAggFunc::Count, andlower.rsrecoversFrequency-sketch routing via a grouped-or-windowed-Counttrigger.Partitionis gone at both L2 and L3 --asap_l2'sAggregatecarrieswithout: booldirectly.Step 5 -- relocate
cse.rstooptimizer(L4), adoptasap-plan's algorithm (third commit):dedupe_subtrees/CseWorkloadPlanmove fromintent_algebra::csetooptimizer::cse, matching ASAPController's layering (CSE is a cost-aware L4 optimizer decision, not an L3 intent-algebra concern).asap_ir::intent_algebra::{BindingName, QueryId}directly (notypes_v2boundary conversion), using ASAPController'sVec+PartialEqstructural-equality candidate scan instead of the old Debug-string-keyedHashMap.optimizer::cost::WorkloadCostPlanandworkload_cost()switched to the sameasap_irtypes directly, removing an earlier boundary-conversion hack.quantiles_over_different_columns_do_not_dedupe,dedupe_subtrees_no_shared_subexpr_when_unique_keys_absent) alongside the pre-existing ones.CommonSubexprElim(intra-tree, Merge-branchScandedup) and the relocated CSE pass (inter-tree, workload-level multi-root dedup) have disjoint scopes -- relocating CSE didn't require touching R8.Step 6 -- PromQL topk/rate semantic retarget to match ASAPController (fourth commit):
RankingMeasure/is_frequency_heavy_hittergate fortopk/bottomkinstead of unconditionally forcingAggFunc::Countinside anytopk(...): only descendingtopkranking bycount_over_time(...)now takes the heavy-hitterTopKpath; everything else (bottomk,topkover a non-count measure) lowers to a genericSort+Limit. Fixes a real correctness bug where e.g.topk(k, avg_over_time(...))was silently treated as aCount/Frequencyheavy-hitter aggregate.Rate/Increase/Changes/Resets/Delta/IDelta/Deriv/PredictLinear/DoubleExpSmoothingnow map to their own dedicatedAggIntents instead of collapsing ontoSum/Count. This activatescapability_for()'s existingRate/Increase->ExactAgg(Increase)mapping and correctly routes the archive-only counter-derivative family (Delta/Changes/Resets/...) toNoneinstead of falsely claiming an exact-agg capability.asap_tier_analysis.rs'souter_fnis computed independently ofrequired_capability(straight off the raw PromQL AST function name), so this only needed test-expectation updates, not a dispatch-logic redesign.QueryExprshape (TopKvsSort+Limit) fortopk/bottomk, not just the flattenedParsedQueryview.Step 6b --
avg_over_timeroutes exact, drops the p50 approximation (fifth commit):AggFunc::Avgnow maps onto the literalAggIntent::Avg, matchingasap_l2::lower's own mapping exactly, instead of the p50 quantile-sketch approximation carried since step 3.capability_for(&AggIntent::Avg)already correctly returnsNone(no ASAP-tier sketch substitute -- needs a cross-policy Sum+Count join), and ASAPController's owncrates/plan/src/bind.rstreatsAggIntent::Avgthe same way (pass_through_intents_stay_logicalkeeps it a whole unsketched logical subtree) -- confirmed by reading the pinned-commit source directly rather than assuming.avgnow routes through the exact/archive path likeSum/Count/every archive-only intent;QeCollector::collect_op's existing catch-allexact_required = truearm already handled this correctly, so the fix is purely theAggFunc::Avgmapping plus updated test expectations (aggregationsgoes from[Quantile]/quantiles: [0.5]to empty +exact_required: true).Step 6c --
stddev_over_time/stdvar_over_timeroute exact, drop the IQR-proxy approximation (sixth commit):AggFunc::StdDev/Variancenow map onto their literalAggIntent::StdDev/Variance(matchingasap_l2::lowerexactly) instead of the two-quantile[q(0.25), q(0.75)]"IQR proxy" approximation carried since step 3.capability_foralready declared both archive-only (Avg | StdDev | Variance => None), so the proxy was silently claiming aQuantileApproxASAP-tier capability neither this repo's own capability table nor ASAPController'sasap-planactually backs with a real bind rule -- same bug class, found by auditing every remainingagg_func_to_intentsarm againstasap_l2::lower's literal mapping after fixingAvg.Merge-of-siblings fan-out this used to trigger forStdDev/Variancecollapses back to a plain singleAggregate.Step 7 -- resolve
capability_for/rules::dispatchdivergences onCountandMin/Max(seventh commit):Count{non-Exact}was claimingCardinalityApprox/HLL ("distinct count" semantics) -- ASAPController's owncrates/plan/src/bind.rs::readoutmapsCounttoSketchQuery::PointCount(a frequency point-query, CMS), matching this repo's ownBindCmsOnCountrule.distinct_over_time/COUNT(DISTINCT)always lower toAggIntent::Cardinality, neverCount, so theCardinalityApproxpremise never had a real caller --AggIntent::Count{non-Exact}is unreachable via this repo's own PromQL frontend today regardless (lower.rsonly ever constructsCount{Exact}or theExtension-basedFrequencyintent), so this is a consistency fix, not a live routing change.Min/Maxwere claimingQuantileApprox(min = quantile(0), max = quantile(1)), but no rule insketch_algebra::rulesactually implemented that --bind_kll_quantile/bind_ddsketch_quantileonly ever matchAggIntent::Quantile, neverMin/Max, so the promised coverage didn't exist andMin/Maxsilently fell through to archive regardless of whatcapability_forclaimed. ASAPController's owncrates/plan/src/boundary.rstreatsMin/Maxas an exact mergeable accumulator (SummaryKind::MinMax), same tier asSum/Rate/Increase-- correct, since comparing two partial extrema needs no approximation at all.bind_exact_agg.rsnow bindsMin/Max->AggregationType::MinMax(keyed ->MultipleMinMax), using the data plane's already-fully-wiredMinMaxAccumulator;capability_fornow returnsExactAgg(MinMax)to match.Step 8 --
capability_fordelegates toasap_plan::boundary::implementation_for(eighth commit):asap-plan/asap-sketchas new git dependencies (same repo/rev asasap-ir/asap-l2;asap-plandepends only onasap-ir, so this pulls in nodatafusion/front-end weight) and bumps the pin to pick upProjectASAP/ASAPController#138-139(a newCostModeltrait for deployment-pluggable sketch ranking, and therealize/Realization->implementation_for/ImplementationCascades-terminology rename plusCountSketch/CountSketchWithHeapSummaryKindvariants).capability_forno longer maintains its own hand-writtenAggIntent-> capability match -- that was a second, parallel judgment kept in sync withasap-plan's own decision by hand, and had already drifted twice (the Count/Min-Max divergences step 7 just fixed). It now keeps only theExtension/Frequencyspecial case (deployment-specific;asap-planhas no opinion on an intent shape it can't see into, by design) and delegates everything else toasap_plan::boundary::implementation_for, translating the returnedImplementationinto this repo's coarserCapabilityvocabulary via a newimplementation_to_capabilityhelper.Count{Exact}is forced toNone(archive) rather than trustingimplementation_for'sExactAccumulatorclaim, because the data plane has no working count accumulator (SumAccumulatorconflatesSumandCount).Step 9 -- bump the ASAPController pin to pick up
Implementation::is_satisfied_by(ninth commit):ProjectASAP/ASAPController#140:asap-plan'sImplementation::is_satisfied_by(materialized-view-style matching over theSummaryKindvocabulary -- "does an already-availableImplementationsatisfy a required one", the query-optimization "answering queries using views" question, narrowed to that crate's own vocabulary).is_satisfied_byis not consumed here. It's a distinct, narrower-scope question from this repo's ownCapability::is_satisfied_by(deployment-specific index matching against an actual inventory of already-materialized sketch instances, shared wire format withdata_plane'ssketch_index) -- the two are separate types serving separate layers, not one delegating to the other. Wiring control_plane'sCapability::is_satisfied_byto build on top ofasap-plan's is tracked as a possible follow-up, not done here.Phase 2's SQL frontend merge (
frontend-sql) is out of scope for this PR per plan -- PromQL only.Test plan
cargo build -p control_plane-- clean after all nine commitscargo test -p control_plane-- 772 passed, 1 pre-existing failure (invalid_sketch_type_override_falls_back_to_default, confirmed identical viagit stashagainst the base commit, unrelated to this change)cargo fmt -p control_planeapplied each step; incidental reformatting of untouched files reverted to keep the diff scoped🤖 Generated with Claude Code