refactor: consolidate Capability into sketch_algebra; warm_tier_analysis becomes facade (Step 2a) - #129
Merged
Conversation
…sis becomes facade
Step 2a of the controller architectural cleanup per design.md §5.
Before this change there were FOUR overlapping sketch-capability tables:
1. controller/sketch_capabilities.yml (66-line YAML runtime override)
2. controller/src/algebra/optimizer.rs::sketch_capability() (compiled defaults)
3. controller/src/sketch_algebra/params.rs::SketchKind (enum)
4. controller/src/warm_tier_analysis.rs::{Capability, SketchKindHandle}
(invented duplicate from PR #128)
Plus warm_tier_analysis.rs matched directly on PromQL function name
strings (count_distinct_over_time, cardinality_estimate, count_distinct)
which exist in NEITHER PromQL nor MetricsQL — invented aliases.
## What landed
### New: controller/src/sketch_algebra/capability.rs (~680 lines)
Single source of truth for warm-tier dispatch:
- Capability enum (was duplicated in warm_tier_analysis.rs + asap-query-engine sketch_index.rs)
- SketchKindHandle enum (incl. CmsWithHeap + Any wildcard)
- SketchCapability struct (was in algebra/optimizer.rs)
- SupportedIntent enum (was in algebra/optimizer.rs)
- capability_for(&AggIntent) -> Option<Capability> <- THE missing function
- default_capability_table() (was in algebra/optimizer.rs::sketch_capability)
- load_capability_overrides(path) (was in algebra/optimizer.rs::load_sketch_capabilities)
- Capability::is_satisfied_by(&Capability) with SketchKindHandle::Any wildcard
- 22 unit tests covering every AggIntent variant + satisfaction wildcards
AggIntent → Capability mapping (final):
| AggIntent | Returns |
|---|---|
| Quantile{accuracy:Epsilon/EpsilonDelta} | Some(QuantileApprox(Any)) |
| Cardinality / Count {accuracy:Epsilon/EpsilonDelta} | Some(CardinalityApprox) |
| TopK / Frequency {accuracy:Epsilon/EpsilonDelta} | Some(FrequencyTopk(CmsWithHeap)) |
| {*}{accuracy:Exact} | None — warm tier doesn't carry exact intents |
| Sum, Min, Max, Avg, Rate, Increase | None |
### warm_tier_analysis.rs (1015 → 607 lines)
Now a thin facade. Pipeline:
query_parser::parse_query(metricsql) -> ParsedQuery
intent_algebra::lower::lower_parsed_query(&parsed, AccuracyTarget::Epsilon(0.01)) -> QueryExpr
walk QueryExpr for Aggregate nodes:
capability_for(&agg.intent) -> Some(cap) | None
Some -> WarmTierCandidate
None -> UnsupportedAggIntent(...)
No direct PromQL function-name matching here anymore. The lowerer is
the single owner of "what does this PromQL mean"; sketch_algebra is
the single owner of "what sketch answers this intent".
Capability + SketchKindHandle now re-exported from sketch_algebra (no
duplicate definitions).
### algebra/optimizer.rs (-199 lines)
Imports SketchCapability + SupportedIntent + default_capability_table +
load_capability_overrides from sketch_algebra. Duplicate definitions
deleted. Cost-model logic stays — relocation is Step 2c.
### asap-query-engine side
- stores/sketch_db/sketch_index.rs: local Capability/SketchKindHandle
enums replaced with `pub use controller::sketch_algebra::{...}`.
From<> adapters from PR #128 deleted (no longer needed — one type).
is_satisfied_by impl moved to sketch_algebra::capability.
- engines/warm_tier/sketch_reducer.rs: kept legacy function-name
aliases in function_to_family for back-compat with PR #128 reducer
tests + recording-rule emission in config/precompute.rs. They no
longer drive analysis-side dispatch.
- drivers/ingest/otel.rs + engines/simple/engine.rs: import-path
adjustments.
## Build + test
- cargo build --release -p controller: clean (5+21 pre-existing warnings)
- cargo build --release -p query_engine_rust: clean (3 pre-existing warnings)
- cargo test -p controller --lib -- sketch_algebra::capability: 22/22 pass (new)
- cargo test -p controller --lib -- warm_tier_analysis: 19/19 pass (PR #128
had 24; 5 invented-name tests dropped + replaced with real PromQL idioms)
- cargo test -p controller --lib: 633/633 pass
- cargo test -p query_engine_rust --lib -- engines::warm_tier: 13/13 pass
## TODOs / known gaps
1. Quantile accuracy-target awareness in capability_for is coarse —
Epsilon(0.0001) and Epsilon(0.05) both return QuantileApprox(Any).
The L4 binder picks DDSketch vs KLL based on ε budget.
2. The intent_algebra lowerer doesn't emit AggIntent::TopK for the
`topk` query-expr wrapper today; capability_for sees the inner
`count_over_time` as Cardinality. PR #128's test that asserted
FrequencyTopk(CmsWithHeap) loosened to just-assert-a-candidate.
Proper fix: extend the lowerer to emit AggIntent::TopK.
3. Legacy aliases `count_distinct_over_time` / `cardinality_estimate`
remain in sketch_reducer.rs::function_to_family + config/precompute.rs
recording-rule emission. They no longer drive analysis-side
dispatch but are accepted for back-compat. Step 2c can revisit.
Diff: 7 files modified + 1 new file
+1154 / -933 = net +221 (the new capability.rs is 680 lines)
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
4 tasks
zzylol
added a commit
that referenced
this pull request
May 11, 2026
…/+analyzer.rs (#130) Per design.md §5 target layout. Restructure controller/src/ from the historic layout (algebra/ + planner/ + config/ as ill-defined catch-alls mixed with stage_split.rs + analyzer.rs scattered top-level files) into the design.md modular split: query_parser/ (L1) → language_logical_plan/ (L2) → intent_algebra/ (L3) → sketch_algebra/ (L4 IR) → optimizer/ (L4 framework) → physical/ (L5 framework) → emit/ (L5 emitters) → pipeline.rs (L1→L5 driver). This is Step 2b/c/d/e/f from the architectural cleanup chain that started with #129 (Step 2a — capability consolidation). ## Old → new path mapping | Old | New | |---|---| | `algebra/expr.rs` | `intent_algebra/legacy_expr.rs` (kept; 91 consumers — unification is a follow-up) | | `algebra/lower.rs` | `intent_algebra/legacy_lower.rs` | | `algebra/{directory,physical,allocator,plan}.rs` | `physical/{sketch_catalog,planner,allocator,plan}.rs` | | `algebra/optimizer.rs` | `optimizer/engine.rs` | | `planner/{cost_model,delta,online,pareto,tco,wire_cost}.rs` | `optimizer/cost/{mod,delta,online,pareto,tco,wire}.rs` | | `planner/rules.rs` | `optimizer/rules/mod.rs` | | `planner/baseline_planner.rs` | `optimizer/baseline.rs` | | `planner/stage_split.rs` | `physical/stage_split.rs` | | `analyzer.rs` | `pipeline.rs` | | `stage_split/` | `physical/colored_dag/` (with colored_dag.rs → dag.rs) | | `query_language/` | `query_parser/language/` | | `config/workloads.rs` | `workload.rs` | | `config/{agent,backend,asapquery_backend,precompute,stage_config,stage_config_otap,stage_config_telegraf}.rs` | `emit/{agent,backend,asapquery_backend,precompute,stage_config,otap,telegraf}.rs` | | (new) | `physical/topology.rs`, `emit/trait_def.rs`, `optimizer/trait_def.rs`, `deployment_model.rs` | ## Pragmatic concessions (documented TODOs) - **`algebra/expr.rs` (1,230 lines) → `intent_algebra/legacy_expr.rs`**: the brief assumed only one stray consumer of `algebra::expr::AggIntent` existed, but reality is ~91 sites across query_parser, language_logical_plan, planner, algebra, and config use the legacy `QueryExpr` / `AggIntent` / `WindowSpec` IR. Treating these as "duplicates to delete" would have required rewriting every consumer in one PR. Moved the file with a doc-comment marking it as legacy IR pending unification with the canonical `intent_algebra::{agg_intent,query_expr,schema}` types. Same for `algebra/lower.rs` → `intent_algebra/legacy_lower.rs`. - **`config/stage_config.rs` (3,020 lines)** moved whole as `emit/stage_config.rs` rather than split into the design.md-prescribed `emit/{opamp,streaming_config,inference_config}.rs`. The monolith mixes OTel-collector YAML emit, ASAPQuery-backend JSON emit, storage-routing JSON emit, and many shared internals; a clean split needs ownership reorganisation, not file renames. - **Back-compat module aliases** in `lib.rs` (`pub use intent_algebra:: legacy_expr as algebra;` etc.) — `controller/src/main.rs` references `crate::stage_split::*`, `crate::algebra::*`, `crate::planner::*`, `crate::config::*`, `crate::analyzer::*` (the bin crate, not the lib). Keeping shims in `lib.rs` lets main.rs continue compiling without per-line edits. Remove the shims once main.rs migrates. - **`hll_accuracy()` / `countmin_accuracy()`** moved from `legacy_expr.rs` → `sketch_algebra/capability.rs` (their structural home — sketch-family error bounds). `legacy_expr.rs` retains thin `pub use` re-exports so in-file callers (e.g. `AggIntent::default_cardinality`) keep compiling. ## docs/design.md updates - §3 layer table — rows rewritten with new `controller/src/*` paths + italicised `*Refactor 2026-05 absorbed ...*` notes. - §5 Target repo layout — prepended blockquote: multi-crate split deferred until ≥2 deployment models ship; lookup table mapping `crates/core/*` targets to current single-crate paths. - §6 Core crate details — prepended blockquote: 16-row lookup table. - **§16 NEW** — ADR section. §16.1 documents PR #129's capability consolidation; §16.2 documents this PR's retirement plan with the full mapping table and four TODOs. ## Build + test - `cargo build --release -p controller` → clean (4 warnings, pre-existing) - `cargo build --release -p query_engine_rust` → clean (3 warnings) - `cargo test -p controller --lib` → **633/633 pass** (unchanged from #129) - `cargo test -p query_engine_rust --lib -- engines::warm_tier` → **13/13 pass** ## Diff: 55 files, +568 / -309 (file renames + import-path rewires) ## Follow-ups 1. Migrate consumers of `intent_algebra::legacy_expr::*` onto canonical `intent_algebra::{query_expr,agg_intent,schema}` types; delete the `legacy_*.rs` files. 2. Split `emit/stage_config.rs` (3,020 lines) per design.md §5. 3. Implement the placeholder traits (`OptimizerRule`, `PlanEmitter`, `DeploymentModelRegistry`). Existing free-function emitters + rule loops keep behaviour stable. 4. Migrate `controller/src/main.rs` off the back-compat module aliases in `lib.rs` (`crate::algebra::*` etc.); remove the shims. 5. (Separate task #32) `sketch_algebra/` touch-up: drop `AggIntent::HistogramQuantile` from semantic IR; map Min/Max to QuantileApprox; consolidate SketchCapability vs SketchCapabilities; rename params.rs → sketch_params.rs; add `FrequencyEstimate` capability variant + `CountSketchWithHeap` handle. 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>
5 tasks
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
Step 2a of the controller architectural cleanup per
docs/design.md§5. Consolidates the four overlapping sketch-capability tables into a single source of truth atcontroller/src/sketch_algebra/capability.rs, and shrinkswarm_tier_analysis.rs(PR #128) into a thin facade that uses the controller's existingintent_algebra::lowerpipeline instead of matching directly on PromQL function-name strings.Before: 4 overlapping capability tables + invented function names
controller/sketch_capabilities.ymlcontroller/src/algebra/optimizer.rs::sketch_capability()controller/src/sketch_algebra/params.rs::SketchKindcontroller/src/warm_tier_analysis.rs::{Capability, SketchKindHandle}Plus the analyzer matched directly on PromQL function-name strings —
count_distinct_over_time,cardinality_estimate,count_distinct— that exist in neither PromQL nor MetricsQL.After: one definition, no invented names
What landed
New:
controller/src/sketch_algebra/capability.rs(~680 lines, 22 tests)Capabilityenum (was duplicated in 2 places)SketchKindHandleenum (incl.CmsWithHeap+Anywildcard)SketchCapabilitystruct +SupportedIntentenum (moved fromalgebra/optimizer.rs)capability_for(&AggIntent) -> Option<Capability>— the new comprehensive functiondefault_capability_table()(moved fromalgebra/optimizer.rs::sketch_capability)load_capability_overrides(path)(moved fromalgebra/optimizer.rs::load_sketch_capabilities)Capability::is_satisfied_by(&Capability)withSketchKindHandle::AnywildcardAggIntent→Capabilitymapping:Quantile{accuracy: Epsilon/EpsilonDelta}Some(QuantileApprox(Any))Cardinality / Count {accuracy: Epsilon/EpsilonDelta}Some(CardinalityApprox)TopK / Frequency {accuracy: Epsilon/EpsilonDelta}Some(FrequencyTopk(CmsWithHeap))*{accuracy: Exact}NoneSum,Min,Max,Avg,Rate,IncreaseNonewarm_tier_analysis.rs(1015 → 607 lines)Pure facade. No direct function-name matching. Uses
query_parser+intent_algebra::lower+sketch_algebra::capability_for.Capability+SketchKindHandlere-exported fromsketch_algebra.algebra/optimizer.rs(−199 lines)Imports from
sketch_algebra; duplicate definitions deleted.asap-query-engine/stores/sketch_db/sketch_index.rs: localCapability/SketchKindHandleenums dropped →pub use controller::sketch_algebra::{...}. PR feat: unify extract_promql_call with controller — single PromQL → Capability owner #128'sFrom<>adapters deleted.engines/warm_tier/sketch_reducer.rs: kept legacy function-name aliases for back-compat with PR feat: unify extract_promql_call with controller — single PromQL → Capability owner #128 reducer tests + recording-rule emission inconfig/precompute.rs. They no longer drive analysis-side dispatch.drivers/ingest/otel.rs+engines/simple/engine.rs: import-path adjustments.Build + test
cargo build --release -p controller: cleancargo build --release -p query_engine_rust: cleancargo test -p controller --lib -- sketch_algebra::capability: 22/22 pass (new)cargo test -p controller --lib -- warm_tier_analysis: 19/19 pass (PR feat: unify extract_promql_call with controller — single PromQL → Capability owner #128 had 24; 5 invented-name tests dropped + replaced with real PromQL idioms)cargo test -p controller --lib: 633/633 passcargo test -p query_engine_rust --lib -- engines::warm_tier: 13/13 passDiff stat
Follow-ups (out of scope for 2a)
Quantileaccuracy-target awareness incapability_foris coarse.Epsilon(0.0001)andEpsilon(0.05)both returnQuantileApprox(Any). The L4 binder picks DDSketch vs KLL based on ε budget — that's the proper home for this granularity.topkaggregate gap: theintent_algebralowerer doesn't emitAggIntent::TopKfor thetopkquery-expr wrapper today.capability_forsees the innercount_over_timeasCardinalityinstead. PR feat: unify extract_promql_call with controller — single PromQL → Capability owner #128'sFrequencyTopk(CmsWithHeap)test loosened to just-assert-a-candidate. Proper fix: extend the lowerer.count_distinct_over_time/cardinality_estimateremain accepted insketch_reducer.rs::function_to_family+config/precompute.rsrecording-rule emission for back-compat. Step 2c can revisit.Up next in this refactor chain
algebra/legacy module; createphysical/,optimizer/,emit/modules per design.md §5; absorbquery_language/intoquery_parser/docs/design.md"Today's locations" column to match the new layout🤖 Generated with Claude Code