refactor(controller): trait impls + main.rs alias removal (PR #130 TODO 3+4) - #132
Merged
Merged
Conversation
…DO 3+4) Two of the four PR #130 follow-up TODOs land here. The other two are blocked by architectural reality (see below). ## TODO 3 — Placeholder traits made functional ✅ ### `OptimizerRule` trait - `controller/src/optimizer/trait_def.rs`: extended trait with `name()`, `category()`, `apply()`. Added two new `RuleCategory` variants (`Cse`, `Decorrelate`) beyond the original four. - `controller/src/optimizer/engine.rs`: per-rule `OptimizerRule` impls for all 12 engine rules (delegating to the existing `RewriteRule` free fns). New `default_rules_as_optimizer_rules()` helper returns `Vec<Box<dyn OptimizerRule>>` for callers that want the trait surface. - `controller/src/optimizer/mod.rs`: blanket impl for every `sketch_algebra::rules::Rule` (lives outside sketch_algebra/ to respect Step #32's territory boundary). ### `PlanEmitter` trait - `controller/src/emit/trait_def.rs`: kept the associated-type shape; added 4 concrete impls — `OpampEmitter`, `OpampGatewayEmitter`, `StreamingConfigEmitter`, `InferenceConfigEmitter` — each wraps the existing free-function emitter. `InferenceConfigInput<'a>` bundles per-tenant routing inputs (tenant id + metric plans + Mode-3 metrics). `emit_borrowed` variant avoids forcing `'static` on the trait-method path. ### `DeploymentModelRegistry` - `controller/src/deployment_model.rs`: extended from stub `HashMap<DeploymentModelId, ()>` to a real `HashMap<…, DeploymentModel>` where `DeploymentModel` owns `(id, rules: Vec<Box<dyn OptimizerRule>>, emitters: EmitterSet)`. `Default::default()` registers `asaplifecycle` with the engine's 12-rule library + the 4 demo emitter names so `pipeline::run_pipeline` doesn't need manual registration. ## TODO 4 — main.rs migrated off back-compat aliases ✅ Rewrote all 52 reference sites in `controller/src/main.rs`: - `algebra::*` → `optimizer::engine::*` / `physical::*` / `intent_algebra::legacy_expr::*` (depending on context) - `analyzer::*` → `pipeline::*` - `config::*` → `emit::*` / `workload::*` - `planner::*` → `optimizer::*` / `physical::stage_split` / `optimizer::cost::online as online_cost_model` - `stage_split::*` → `physical::colored_dag::*` Removed all back-compat shims from `lib.rs`: - `pub use emit as config` - `pub use pipeline as analyzer` - `pub mod algebra { … }` - `pub use physical::colored_dag as stage_split` - `pub mod planner { … }` One leftover internal ref (`crate::planner::rules::bind_workload_typed` inside `emit/mod.rs::collect_metric_to_family`) retargeted to `crate::optimizer::rules::bind_workload_typed`. `lib.rs` shrank from 184 → 95 lines. ## TODO 1 — legacy_expr migration: BLOCKED 🚫 The canonical `intent_algebra::{query_expr,agg_intent,schema}` is NOT a structural superset of the legacy `intent_algebra::legacy_expr`. The canonical `QueryExpr` has 5 variants (`Scan`, `Window`, `Aggregate`, `LetBinding`, `Ref`); legacy has ~26 (`Source`, `Filter`, `Project`, `Aggregate`, `Window`, `SketchAgg`, `WindowedAgg`, `Partition`, `Dedup`, `TopK`, `Merge`, `Join`, `JoinSketch`, `SetOp`, `Sort`, `Limit`, `Subquery`, `LetBinding`, `WindowFunc`, `HistogramQuantile`, `PromQLSubquery`, `BinaryOp`, `Ref`) plus a full `ScalarExpr` IR (Column, Literal, BinaryOp, UnaryOp, FunctionCall, ScalarSubquery, InList, InSubquery, Between, IsNull, Case, Cast, VectorBinaryOp). The canonical IR's own module docs flag this: *"the full design.md list is larger; they are deferred to follow-up phases as the planner grows consumers for them"*. Migrating consumers requires first GROWING the canonical IR to absorb these variants — a substantial separate refactor, not a follow-up cleanup. 50+ consumer sites in query_parser/, language_logical_plan/, optimizer/engine.rs, physical/{allocator,planner,plan,stage_split}.rs depend on the legacy variant set. Recommend retitling the follow-up as "grow canonical L3 to absorb legacy variants" rather than "migrate consumers". The legacy_expr module stays in place pending that work. ## TODO 2 — stage_config.rs split: DEFERRED 🟡 The 3,020-line `emit/stage_config.rs` monolith mixes OTel YAML emit, backend JSON emit, and storage-routing emit with intricately shared helpers (`sketch_kind_to_processor_name`, `build_otlp_exporter`, `sketch_kind_tag`, …) and 1,580 lines of tightly-interleaved tests (line 1441 onward). The wrapper structs from TODO 3 above already provide the polymorphic `PlanEmitter` surface (`OpampEmitter` / `StreamingConfigEmitter` / `InferenceConfigEmitter`), so the file-level split is no longer load-bearing for the trait work. Recommend addressing as a separate PR with dedicated time budget if/when the monolith becomes a review-bottleneck. ## Build + test - `cargo build --release -p controller` — clean - `cargo build --release -p query_engine_rust` — clean - `cargo test --release -p controller --lib` — **646 passed** (was 633; +13 trait tests) - `cargo test --release -p controller --bin controller` — **27 passed** - `cargo test --release -p query_engine_rust --lib -- engines::warm_tier` — **13 passed** ## Caveat — `controller/src/store/` PR #130 left `lib.rs` declaring `pub mod store;` but `controller/src/store/` is in `.gitignore` (line ~10 of repo root .gitignore — the broad `store/` pattern). The directory is untracked; this needs a separate decision on whether it should be tracked or remain a runtime artifact. The build works because the directory exists locally on the dev box; fresh clones would fail. Not in scope for this PR; flagging as follow-up. ## Diff: 8 files, +785 / -207 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Merged
4 tasks
zzylol
added a commit
that referenced
this pull request
May 11, 2026
… + dedupe (#133) User-flagged cleanup of sketch_algebra/ after PR #130's layered refactor. ## 1. Drop `AggIntent::HistogramQuantile` `histogram_quantile()` is a PromQL/MetricsQL language-level operator, NOT a semantic intent. Removed the variant from `intent_algebra::agg_intent::AggIntent`. The PromQL operator still lives on as `intent_algebra::legacy_expr::QueryExpr::HistogramQuantile` (unreachable from canonical L3 — it was never produced by the lowerer, but PR #130's legacy_expr preservation keeps it for back-compat until that module is fully retired in a separate follow-up). ## 2. `AggIntent::Min` + `AggIntent::Max` → `QuantileApprox(Any)` Quantile sketches (DDSketch, KLL) answer min = quantile(0) and max = quantile(1) directly. Previously these returned `None` from `capability_for`; now they map to warm-tier-answerable. ## 3. Consolidate `SketchCapabilities` ↔ `SketchCapability` (Option A) Renamed `controller/src/sketch_algebra/schema.rs::SketchCapabilities` → `SketchStateMetadata` to remove the name collision with `capability.rs::SketchCapability`. Both files keep their roles: - `schema.rs::SketchStateMetadata` = L4 type-system role (wraps `SketchStateSchema` with sketch-family metadata per design.md §6.4). - `capability.rs::SketchCapability` = perf/feasibility/intent-routing metadata (consumed by optimizer + cost model). ## 4. Rename `params.rs` → `sketch_params.rs` Naming clarity. The file holds `SketchParams` + its KLL/DDSketch/Hll/ Cms/CountSketch variants. `sketch_algebra/mod.rs` retains `pub use sketch_params as params;` back-compat alias so the ~10 in-tree `crate::sketch_algebra::params::*` call sites keep compiling without further migration. ## 5. Frequency Capability surface: split FrequencyTopk vs FrequencyEstimate User-clarified MetricsQL surface mapping: | MetricsQL surface | AggIntent | Capability | |---|---|---| | `sum by (item) (rate(m[r]))` w/ Epsilon | `Frequency{accuracy}` | `FrequencyEstimate(Any)` | | `topk(k, sum by (item) (rate(m[r])))` | `TopK{k, accuracy}` | `FrequencyTopk(CmsWithHeap)` | | (Exact) | (any) | `None` — warm-tier doesn't carry exact | Concrete changes in `capability.rs`: - Added `Capability::FrequencyEstimate(SketchKindHandle)` (NEW — heap-LESS; answers bare per-key frequency without top-k extraction). - Existing `Capability::FrequencyTopk(SketchKindHandle)` stays (heap-BEARING; answers top-k). - Added `SketchKindHandle::CountSketchWithHeap` variant (alongside existing `CmsWithHeap`). - Updated `capability_for`: - `Frequency{!Exact}` → `Some(FrequencyEstimate(Any))` (was `FrequencyTopk(CmsWithHeap)` — wrong, that's for top-k only) - `TopK{Exact}` → `None` (was `FrequencyTopk` — exact top-k uses HashAgg+Heap, not a sketch) - Updated `is_satisfied_by`: - `FrequencyTopk(req)`: indexed must be `FrequencyTopk(have)` with `have ∈ {CmsWithHeap, CountSketchWithHeap}`. Rejects heap-less. - `FrequencyEstimate(req)`: indexed `FrequencyEstimate` with any frequency-family handle OR `FrequencyTopk` with heap-bearing handle (heap is additional info — underlying matrix answers the point query). ## Backend wire-in - `asap-query-engine/src/engines/warm_tier/sketch_reducer.rs`: new `QueryFamily::FrequencyEstimate` dispatch arm + heap-bearing variant routing. New `decode_frequency_total` helper emits per-window row-0 sum of the CMS/CountSketch matrix as the total-frequency scalar. - `asap-query-engine/src/drivers/ingest/otel.rs`: ingest-side capability mapping at sid registration time — heap-less sketches classify as `FrequencyEstimate`, heap-bearing as `FrequencyTopk`. ## Build + test - `cargo build --release -p controller` — clean - `cargo build --release -p query_engine_rust` — clean - `cargo test --release -p controller --lib` — **643/643 pass** (was 633 in PR #132; +10 new tests covering Min/Max, FrequencyEstimate semantics, is_satisfied_by wildcard + heap rules) - `cargo test --release -p query_engine_rust --lib -- engines::warm_tier` — **13/13 pass** (FrequencyEstimate ingest+reducer arm landed but no new fixture; the existing arms already exercise the Capability-keyed dispatch) ## Diff: 12 files, +546 / −140 ## Follow-ups (out of scope) - **FrequencyEstimate per-key lookup**: today's `decode_frequency_total` returns the row-0 sum (i.e., total frequency across the entire CMS matrix). A real per-key `frequency(metric, key)` reducer needs the key as an arg, which today's `function_args: &[f64]` signature can't carry. Documented inline. - **`AggIntent::HistogramQuantile` ghost in legacy_expr**: until legacy_expr is fully retired (PR #130 TODO 1), the variant lives on as a PromQL-operator placeholder. Removal happens with the legacy retirement. Co-authored-by: zz_y <zz_y@node0.zz-y-304941.softmeasure-pg0.clemson.cloudlab.us> Co-authored-by: Claude Opus 4.7 (1M context) <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
Two of the four PR #130 follow-up TODOs land here. The other two are blocked by architectural reality — surfaced explicitly so the follow-up plan can be reshaped.
TODO 3 — Placeholder traits made functional ✅
OptimizerRuletrait extended (addedCse,Decorrelatecategories). All 12 engine rules get per-ruleOptimizerRuleimpls inoptimizer/engine.rs. Newdefault_rules_as_optimizer_rules()returnsVec<Box<dyn OptimizerRule>>. Blanket impl for everysketch_algebra::rules::Ruleinoptimizer/mod.rs.PlanEmittertrait gets 4 concrete impls inemit/trait_def.rs:OpampEmitter,OpampGatewayEmitter,StreamingConfigEmitter,InferenceConfigEmitter. Each wraps the existing free-function emitter.InferenceConfigInput<'a>bundles per-tenant inputs.DeploymentModelRegistryextended from stub to a realHashMap<DeploymentModelId, DeploymentModel>.Default::default()registersasaplifecyclewith the engine's 12-rule library + 4 demo emitters.13 new unit tests across the three trait files.
TODO 4 — main.rs migrated off back-compat aliases ✅
Rewrote all 52 reference sites:
algebra::*→optimizer::engine::*/physical::*/intent_algebra::legacy_expr::*analyzer::*→pipeline::*config::*→emit::*/workload::*planner::*→optimizer::*/physical::stage_split/optimizer::cost::onlinestage_split::*→physical::colored_dag::*Removed all 5 back-compat shims from
lib.rs(pub use emit as config,pub use pipeline as analyzer,pub mod algebra,pub use physical::colored_dag as stage_split,pub mod planner).lib.rsshrank 184 → 95 lines.TODO 1 — legacy_expr migration: BLOCKED 🚫
The canonical
intent_algebra::{query_expr,agg_intent,schema}is NOT a structural superset of the legacyintent_algebra::legacy_expr. The canonicalQueryExprhas 5 variants (Scan, Window, Aggregate, LetBinding, Ref); legacy has ~26 plus a fullScalarExprIR with no canonical equivalent.The canonical IR's own module docs flag this: "the full design.md list is larger; they are deferred to follow-up phases as the planner grows consumers for them".
Migrating consumers requires first growing the canonical IR to absorb the variant set — substantial separate refactor work, not a follow-up cleanup. 50+ consumer sites depend on the legacy variants. Left intact.
Recommendation: retitle the follow-up from "migrate consumers" to "grow canonical L3 to absorb legacy variants" so the scope is honest.
TODO 2 — stage_config.rs split: DEFERRED 🟡
3,020-line
emit/stage_config.rsmixes OTel YAML / backend JSON / storage-routing emit with intricately shared helpers and 1,580 lines of interleaved tests.Critically: TODO 3's wrapper structs (
OpampEmitter,StreamingConfigEmitter,InferenceConfigEmitter) already provide the polymorphicPlanEmittersurface. The file-level split is no longer load-bearing for the trait work — it would be purely cosmetic at this point.Recommendation: address as a separate PR with dedicated time budget if/when the monolith becomes a review bottleneck.
Build + test
cargo build --release -p controller— clean (4 main + 19 lib warnings, all pre-existing)cargo build --release -p query_engine_rust— cleancargo test --release -p controller --lib— 646 passed (was 633; +13 trait tests)cargo test --release -p controller --bin controller— 27 passedcargo test --release -p query_engine_rust --lib -- engines::warm_tier— 13 passedDiff: 8 files, +785 / -207
controller/src/deployment_model.rscontroller/src/emit/mod.rscontroller/src/emit/trait_def.rscontroller/src/lib.rscontroller/src/main.rscontroller/src/optimizer/engine.rscontroller/src/optimizer/mod.rscontroller/src/optimizer/trait_def.rsCaveat —
controller/src/store/(out of scope, flagging)lib.rsdeclarespub mod store;butcontroller/src/store/is in repo root.gitignore(broadstore/pattern). The directory is untracked; build works because it exists locally on dev boxes. Fresh clones would fail. Needs a separate decision: track the module or remove the declaration.🤖 Generated with Claude Code