Skip to content

feat(control_plane): Phase 1 -- merge AggIntent vocabulary from ASAPController - #391

Closed
zzylol wants to merge 1 commit into
mainfrom
phase1/merge-agg-intent
Closed

zzylol wants to merge 1 commit into
mainfrom
phase1/merge-agg-intent

Conversation

@zzylol

@zzylol zzylol commented Jul 18, 2026

Copy link
Copy Markdown
Contributor

Summary

First implementation PR against #390's migration plan. Replaces
control_plane's AggIntent (25 variants) with ASAPController's current
version (~40 variants), per Phase 1.

Two corrections to decisions recorded in #389/#390, found while actually
reading how the code is used (not just diffing enum signatures):

  1. window: Duration stays on the intent (Rate/Increase/Changes/
    Delta/IDelta/Deriv/Resets/PredictLinear/DoubleExpSmoothing)
    instead of moving to QueryExpr::TimeRange as ASAPController does.
    Moving it requires verifying query_expr.rs/lower.rs thread window
    info correctly everywhere AggIntent is built or consulted -- that's
    Phase 2 (those are Phase 2 files). Reconcile there.
  2. AggIntent::Frequency is kept, not folded into RankingMeasure::Frequency.
    These are unrelated concepts that happen to share a name:
    Frequency is control_plane's standalone point-frequency-via-CMS query
    (count(*) WHERE key = k, bound in bind_cms_count.rs);
    RankingMeasure::Frequency classifies what a TopK ranks by. Folding
    one into the other would have silently deleted a real, currently-used
    capability. RankingMeasure is still adopted, additively -- it's a
    real gap control_plane had, just not a replacement for Frequency.

irate/rate fold (per the agreed tie-break) turned out to be a behavioral
no-op: the PromQL walk already lowers "rate" | "irate" to the same
AggFunc::Rate, and no AggFunc::Irate ever existed, so AggIntent::Irate
was unreachable from real query parsing -- only unit tests constructed it
directly.

New intent families (histogram accessors, math/trig, time/calendar
accessors, presence functions, Group/CountValues, extended
range-vector reducers) land as archive-only, consistent with existing
policy for intents with no Bind* rule yet.

Verification

  • cargo build --workspace -- clean.
  • cargo test -p control_plane --lib -- 837 passed, 1 pre-existing
    failure (invalid_sketch_type_override_falls_back_to_default),
    confirmed identical on unmodified main via git stash -- not caused
    by this change.
  • Flagging for Phase 3: analyzer-parity-matrix.md's corpus (the doc's
    named acceptance test, data_plane/.../engine.rs::analyzer_parity_tests)
    no longer exists under that name in current data_plane. Phase 3's
    plan explicitly depends on it as the gate for the capability_for()
    fix -- worth locating or rebuilding before that phase starts.

Test plan

🤖 Generated with Claude Code

…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.
zzylol added a commit that referenced this pull request Jul 18, 2026
Frequency: corrected to "kept, not folded" per #391's finding. Also
flags that analyzer-parity-matrix.md's named acceptance test doesn't
exist in current data_plane -- Phase 3 needs to locate or rebuild it
before starting, not assume it's there.
@zzylol

zzylol commented Jul 18, 2026

Copy link
Copy Markdown
Contributor Author

Superseded by #392 — Phase 1's copy-based approach wasn't a real merge (two independently-maintained copies of AggIntent that would drift again on the next change either side made). #392 depends on ASAPController's asap-ir crate directly instead.

@zzylol zzylol closed this Jul 18, 2026
@zzylol
zzylol deleted the phase1/merge-agg-intent branch July 18, 2026 16:51
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>
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