feat(control_plane): Phase 1b -- depend on asap-ir for real, not a copy - #392
Merged
Merged
Conversation
…ontroller Adopts ASAPController's current AggIntent as the base (~40 variants vs control_plane's pre-merge 25), per docs/migration-plan-backend-plan.md Phase 1. Two deliberate deviations from a byte-for-byte port, both documented in agg_intent.rs's module docs: - `window: Duration` stays on the time-series-derivative variants instead of moving to QueryExpr::TimeRange -- that requires verifying query_expr.rs/lower.rs thread it correctly everywhere, which is Phase 2 scope. - `AggIntent::Frequency` is kept, not folded into ASAPController's RankingMeasure::Frequency -- they're unrelated concepts that share a name (standalone point-frequency-via-CMS query vs. a TopK ranking-measure classifier). This corrects the decision recorded in PR #389/#390; RankingMeasure is still adopted additively. Also folds `irate` into `Rate` per the agreed tie-break (ASAPController's approach): `AggIntent::Irate` is removed. This was already a no-op on real query behavior -- the PromQL walk already lowered "rate"|"irate" to the same AggFunc::Rate with no AggFunc::Irate ever existing, so AggIntent::Irate was unreachable from real parsing; only unit tests constructed it directly. New intent families added (histogram accessors, math/trig transforms, time/calendar accessors, presence functions, Group/CountValues, extended range-vector reducers) are all archive-only for now -- none has a Bind* rule yet, consistent with existing policy for intents with no ASAP-tier sketch binding. Renames to match ASAPController: Idelta -> IDelta, HoltWinters -> DoubleExpSmoothing, Absent -> {Absent, AbsentOverTime}, Present -> PresentOverTime. Adds col: Option<ColumnId> to the single-column reducers (Sum/Min/Max/Avg/Quantile/Cardinality/StdDev/Variance), defaulting to None everywhere today (PromQL sample-value convention) -- plumbing for the Phase 0 "control_plane gains SQL support" decision, populated once frontend-sql lands in Phase 2. Verified: full workspace builds clean (cargo build --workspace); control_plane's 837-test suite passes unchanged (1 pre-existing failure, confirmed identical on unmodified main via git stash, unrelated to this change). The analyzer-parity-matrix.md corpus this phase's testing bar was meant to gate on no longer exists under its documented name (data_plane/.../engine.rs::analyzer_parity_tests) -- flagging for Phase 3, since that phase's plan explicitly depends on it as the acceptance test for the capability_for() fix.
Supersedes #391. Phase 1 copied ASAPController's AggIntent vocabulary into control_plane's own agg_intent.rs -- that's not a merge, it's a second independently-maintained copy that will drift again on the next change either side makes. This adds a real git dependency on ASAPController's asap-ir crate (pinned to a commit SHA -- no tagged releases exist yet; re-pin as needed) and deletes the local AggIntent definition entirely. Two coupling issues surfaced only once depending on the real external type (impossible to see from a copy, since a copy can freely add fields): - AggIntent's own methods are typed against asap_ir's Column/DataType and AccuracyTarget, not control_plane's local versions. ColumnId is a bare `usize` alias on both sides (no-op), but Column/DataType and AccuracyTarget are real, incompatible types. Column/DataType: converted at the agg_intent.rs boundary rather than merging schema.rs wholesale (that cascades into Schema/QueryExpr, ~38 Column{} literals -- Phase 2 scope). AccuracyTarget: swapped control_plane's local type for asap_ir's directly (types_v2::AccuracyTarget is now `pub use asap_ir::types:: AccuracyTarget`) -- no external YAML/JSON persists the old wire shape (only one in-Rust test fixture needed updating), so converting at every one of ~400 AggIntent call sites wasn't worth it just to keep a differently-shaped local copy. - Rust's orphan rules don't allow inherent impls on a foreign type, so `.archive_only()` / `.output_column()` (control_plane-only concepts) become free functions `archive_only(&intent)` / `output_column(&intent, &col)`. `asap_ir`'s own inherent methods (`.input_col()`, `.is_per_series()`) keep method syntax -- those aren't control_plane- specific and need no wrapper. Frequency (control_plane's standalone point-frequency-via-CMS query, not the same thing as RankingMeasure::Frequency -- see ASAPController#137) is now carried through the shared AggIntent as `Extension { ext_kind: "frequency", payload }`, via `frequency()`/`as_frequency()` helpers. window: Duration on Rate/Increase/Changes/Delta/IDelta/Deriv/Resets -- deferred in Phase 1 by adding the field to the local copy -- is gone for real now; you can't add a field to a variant you don't own. Grepped for actual reads (not constructions) first: exactly one real consumer existed (sketch_algebra/rules/bind_exact_agg.rs's zero-window rejection guard), which now reads the window off the enclosing QueryExpr::Window node it already has in scope. Verified: full workspace builds clean (cargo build --workspace); control_plane's 828-test suite passes unchanged (the same 1 pre-existing failure as #391, confirmed identical on unmodified main). Needs: .cargo/config.toml (net.git-fetch-with-cli = true) at the workspace root -- Cargo's bundled libgit2 doesn't use the configured SSH agent the way the system git CLI does; data_plane already carries this same setting locally for its own git deps.
This was referenced Jul 18, 2026
zzylol
added a commit
that referenced
this pull request
Jul 21, 2026
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.
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
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
Supersedes #391. Please close #391 in favor of this one -- Phase 1's
approach (copy ASAPController's
AggIntentvocabulary intocontrol_plane's ownagg_intent.rs) turned out not to be a merge atall: it's a second, independently-maintained copy that drifts again the
moment either side changes anything, which is exactly the "two-repo
split forces coordinated PRs" problem
design-controller-into-backend.mdalready flagged. This PR adds a real git dependency on ASAPController's
asap-ircrate (pinned toc73c2a5, ASAPController#137's merge commit --no tagged releases exist yet) and deletes the local
AggIntentdefinition.
What only showed up once depending on the real type
A local copy can freely grow extra fields; a dependency on a type you
don't own can't. Two coupling issues surfaced that Phase 1 had no way to
see:
Column/DataType/AccuracyTarget.AggIntent's own methods aretyped against
asap_ir's versions of these, not control_plane's localones.
ColumnIdis a bareusizealias on both sides (free). ForColumn/DataTypeI convert at theagg_intent.rsboundary ratherthan merging
schema.rswholesale -- that cascades intoSchema/QueryExpr(~38Column{}literals across the repo), which is Phase 2scope, not this PR's. For
AccuracyTargetI went the other way andswapped control_plane's local type for
asap_ir's directly (types_v2:: AccuracyTargetis nowpub use asap_ir::types::AccuracyTarget) --checked first that no external YAML/JSON persists the old wire shape
(
#[serde(tag="kind", content="value")]with anepsfield vs.asap_ir's default tagging with
epsilon); only one in-Rust testfixture needed updating, so converting at ~400 call sites just to keep
a differently-shaped local copy wasn't worth it.
.archive_only()/.output_column()(control_planeconcepts, not shared ones) can't be inherent methods on a foreign type
anymore -- they're free functions now (
archive_only(&intent),output_column(&intent, &col)).asap_ir's own methods(
.input_col(),.is_per_series()) keep method syntax.Frequency(control_plane's point-frequency-via-CMS query -- seeASAPController#137 for why it's not the same thing as
RankingMeasure::Frequency) is now carried asAggIntent::Extension { ext_kind: "frequency", payload }viafrequency()/as_frequency()helpers.
window: DurationonRate/Increase/Changes/Delta/IDelta/Deriv/Resets-- Phase 1 deferred this by just adding the field to itslocal copy -- is gone for real; you can't add a field to a variant you
don't own. I grepped for actual reads of the field (not constructions)
before assuming this needed a big restructure: there was exactly one
real consumer (
bind_exact_agg.rs's zero-window rejection guard), whichnow reads the window off the enclosing
QueryExpr::Windownode italready has in scope.
Also needed
.cargo/config.toml(net.git-fetch-with-cli = true) at the workspaceroot -- Cargo's bundled libgit2 doesn't use the configured SSH agent the
way the system
gitCLI does.data_planealready carries this samesetting locally for its own git path/deps; this just makes it apply
workspace-wide so
control_plane's new git dependency resolves too.Verification
cargo build --workspace-- cleancargo test -p control_plane --lib-- 828 passed, 1 pre-existingfailure (
invalid_sketch_type_override_falls_back_to_default),confirmed identical on unmodified
main-- not caused by this change(same failure feat(control_plane): Phase 1 -- merge AggIntent vocabulary from ASAPController #391 already flagged)
rustfmtapplied to all changed filesStill flagging from #391:
analyzer-parity-matrix.md's named acceptancetest doesn't exist in current
data_planeunder that name -- Phase 3needs to locate or rebuild it before starting.
Test plan
cargo build --workspacecargo test -p control_plane --lib🤖 Generated with Claude Code