refactor(legacy_expr): step α — legacy AggIntent → canonical AggIntent at field level - #138
Merged
zzylol merged 1 commit intoMay 11, 2026
Conversation
…t at field level
Unifying first move per the revised migration plan. After Batch 2's
A-lifts, the 3d/3e agents surfaced an architectural wall: legacy and
canonical QueryExpr are unrelated types; consumer migration requires
Schema threading first.
Step α takes a different cut: at the TYPE-OF-FIELD level, replace
`legacy_expr::AggIntent` with canonical `intent_algebra::agg_intent::AggIntent`
in every site that carried legacy AggIntent. The wrapping
`legacy_expr::QueryExpr` variants (SketchAgg, WindowedAgg, etc.) stay
intact — they now carry canonical AggIntent as their `op`/`agg` field.
This unblocks Step β (Schema threading) and Step γ (variant-by-variant
consumer migration) because all sketch-bound intents now have a unified
shape.
## Translation strategy: option (a) — re-export + PerPartitionWrap
`legacy_expr.rs` now does:
```rust
pub use crate::intent_algebra::agg_intent::AggIntent;
pub struct PerPartitionWrap { inner: AggIntent, keys: Vec<String> }
```
The legacy `AggIntent` enum + `ExactAgg` helper enum are **deleted**.
Canonical `AggIntent` covers Min/Max/Sum/Count/Avg natively; only
`PerPartition` needed a separate carrier (1 live construction site).
Free-function helpers replace the old `AggIntent::method()` API:
- `agg_is_mergeable(&AggIntent) -> bool`
- `agg_is_exact(&AggIntent) -> bool`
- `agg_to_legacy_agg_type(&AggIntent) -> AggType`
- `agg_quantiles(&AggIntent) -> Vec<f64>`
- `agg_accuracy(&AggIntent) -> f64`
- `accuracy_target_from_legacy(f64) -> AccuracyTarget`
- `default_quantile/cardinality/frequency()` constructors
## Translation table (executed)
| Legacy | Canonical |
|---|---|
| `Quantile { quantiles: Vec<f64>, accuracy: f64 }` | fan-out: N × `Quantile { q, accuracy: AccuracyTarget }` |
| `Cardinality { accuracy: f64 }` | `Cardinality { accuracy: Exact if 0.0 else Epsilon(eps) }` |
| `Frequency { accuracy: f64 }` | `Frequency { accuracy: ... }` (same) |
| `Extrema { min: true, max: false }` | `Min` |
| `Extrema { min: false, max: true }` | `Max` |
| `Extrema { min: true, max: true }` | fan-out: `Min` + `Max` (not reached at any caller) |
| `Exact(ExactAgg::Sum)` | `Sum` |
| `Exact(ExactAgg::Count)` | `Count { accuracy: Exact }` |
| `Exact(ExactAgg::Avg/Min/Max)` | `Avg/Min/Max` |
| `PerPartition { inner, keys }` | `PerPartitionWrap { inner: canonical, keys }` (legacy-only carrier; collapses in Step γ) |
## Fan-out strategy: F1 — Merge-wrap
`legacy_lower::agg_func_to_intents` returns `Vec<AggIntent>`;
`lower_aggregate` wraps siblings in `QueryExpr::Merge { inputs: [SketchAgg|WindowedAgg, …] }`
when multi-intent results come back. SketchAgg/WindowedAgg shapes
unchanged; consumers keep matching single-intent `op: AggIntent`.
Two active fan-out paths:
- `AggFunc::StdDev` / `AggFunc::Variance` → Merge of 2 Quantile siblings (q=0.25, 0.75)
- R6 HistogramQuantileFusion when φ ≠ existing q → Merge of 2 Quantile siblings
## Sites migrated
- **13 construction sites** across legacy_lower / legacy_expr / physical/{allocator,planner,stage_split} / optimizer/engine
- **9 match sites** in sketch_catalog / allocator / stage_split / query_parser / optimizer/engine
- **2 fan-out emit cases** (above)
## R9 HydraConversion DISABLED
Multi-key `Partition + sketch` tree fusion (R9) cannot survive Step α
because it inlined `PerPartition` into `SketchAgg::op` — a shape
canonical AggIntent doesn't support. R9's `try_rewrite` returns `None`;
original logic preserved under `#[allow(dead_code)] mod hydra_conversion_legacy`
with a Step γ TODO. Disabling a cost-driven rule preserves correctness
(unfused tree still produces right result, just less efficient).
## Build + test
- `cargo build --release -p controller` — clean
- `cargo build --release -p query_engine_rust` — clean
- `cargo test -p controller --lib` — **666 / 666 pass** (was 666 after Batch 2; net 0 change — one R6 test rewritten, one legacy_expr test renamed)
- `cargo test -p controller --bin controller` — **27 / 27 pass**
## Verification grep
- `::AggFunc\b` → 4 hits, all in `query_parser/mod.rs::collect_agg_func*` — L2 SQL/PromQL parse-time helpers (intentional retention)
- `legacy_expr::AggIntent\b | ExactAgg\b` → 1 live hit (the `pub use` re-export in stage_split.rs) + comments
- `Extrema {` → 1 hit, `EvalFunc::Extrema { ... }` in physical/planner.rs (unrelated L5 enum, not legacy AggIntent)
## 4 TODOs flagged for Step γ
1. **R9 HydraConversion re-enable**: Step γ re-emits as canonical `Aggregate { by: keys, aggs: [inner] }`
2. **R6 smarter dedup**: today emits Merge of siblings; Step γ should consider Merge-aware fusion that dedups same-q siblings
3. **`sketch_op_to_promql` fall-throughs**: archive-only / TopK / Rate / Increase currently emit `count_over_time` (legacy `Exact(_)` fallback). Step γ routes each kind through its dedicated PromQL template
4. **`agg_to_legacy_agg_type` taxonomy**: collapses all non-card/freq onto `AggType::Quantile`. Step γ should plumb canonical kind directly into the planner
## Diff: 8 files, +556 / -337
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
4 tasks
zzylol
added a commit
that referenced
this pull request
May 12, 2026
… stack (#139) Plumbing-only refactor. After Step α (PR #138) gave us canonical AggIntent inside legacy QueryExpr variants, the next blocker for variant-by-variant consumer migration (Step γ) is that consumers can't resolve `ColumnRef::Named("host")` → `ColumnId(2)` without a Schema. Step β threads Schema through every consumer-side walker via approach (c) — traversal-environment parameter (lightest touch; mirrors how canonical QueryExpr already passes schema downward). ## What landed ### New: `controller/src/intent_algebra/column_resolution.rs` (256 lines, 8 tests) ```rust pub fn resolve_column_ref(col: &ColumnRef, schema: &Schema) -> Result<ColumnId, ResolveError>; pub fn resolve_column_refs(cols: &[ColumnRef], schema: &Schema) -> Result<Vec<ColumnId>, ResolveError>; pub fn infer_schema_for_root(expr: &QueryExpr) -> Schema; pub fn infer_source_schema(metric_name: &str) -> Schema; ``` `ResolveError` enumerates `NotFound { name, available }`, `NoSampleValue { available }`, `WildcardNotPositional`. `infer_source_schema` synthesises the default time-series shape: - `Column { name: "ts", dtype: Timestamp, nullable: false }` at index 0 - `Column { name: "value", dtype: Float64, nullable: false }` at index 1 - Conservative `unique_keys: vec![]` (no catalog to assert primary key) `infer_schema_for_root` walks to the outermost `Source` leaf via the existing `QueryExpr::source_name()` helper and applies `infer_source_schema`; falls back to `Schema::default()` for trees rooted at a bare `Ref(_)`. ### Functions/methods that gained a `parent_schema: &Schema` parameter - `optimizer::engine::RewriteRule::try_rewrite` — trait + 12 impls - `optimizer::engine::QueryOptimizer::{apply_all, recurse_children, apply_rules_at}` - `physical::allocator::SketchAllocator::{alloc_node, alloc_sketch_agg}` - `physical::planner::plan_node` (free fn) - `physical::stage_split::{walk, promql_from_qe}` (free fns) - `query_parser::mod::QeCollector::visit` - `intent_algebra::legacy_lower::lower_to_sketch_algebra_with_schema` (new sibling — old `lower_to_sketch_algebra` now delegates after deriving schema via `infer_schema_for_root`) Public entry points (`SketchAllocator::allocate`, `plan`, `split_expr_by_stage`, `expr_to_promql`, `lower_to_sketch_algebra`, `qe_to_parsed_query`, `QueryOptimizer::optimize`) keep their existing signatures; each derives the root schema and threads it inward. Backwards compatibility intact. ### Usage proof (physical/stage_split.rs:Distinct arm) ```rust QueryExpr::Distinct { cols, input } => { plan.backend.has_dedup = true; for c in cols { if let ColumnRef::Named(_) = c { match resolve_column_ref(c, parent_schema) { Ok(_id) => { /* Step γ TODO: distinct_cols: Vec<ColumnId> */ } Err(e) => plan.deferral_log.push(format!( "Distinct col resolution deferred to Step γ: {e}")), } } } walk(input, plan, budgets, parent_schema); } ``` ## Build + test - `cargo build --release -p controller` — clean - `cargo build --release -p query_engine_rust` — clean (pre-existing `e2e_modified_otlp_sketch_path` test compile error in `query_engine_rust --lib` was already on main; out of scope) - `cargo test --release -p controller --lib` — **674 passed** (was 666 after Step α; +8 new column_resolution tests) - `cargo test --release -p controller --bin controller` — 27 passed ## 7 TODOs flagged for Step γ 1. `Aggregate { keys: Vec<String> }` → `Vec<ColumnId>` — needs schema transformation as we descend into Aggregate's child 2. `Distinct { cols }` consumer in `physical::stage_split` logs `ColumnId`s instead of using them — `BackendSubPlan` needs a `distinct_cols: Vec<ColumnId>` field 3. `Project { cols }` reshapes columns canonically; legacy passes parent schema through verbatim 4. `LetBinding` / `Ref` don't carry a `BindingScope` analogue in legacy; Step γ needs to plumb a scope map alongside Schema 5. Label sets (`*labels` open-set) aren't representable in canonical `Schema` today — needs a `DataType::LabelSet` or parallel side-channel before full Source→Scan migration 6. `CostModel::estimate(&self, expr: &QueryExpr)` doesn't take Schema (inspects only outermost node). Future cost estimators may need the signature extended 7. `optimizer::engine::HydraConversion` (disabled in Step α) could now consult the schema to emit canonical `Aggregate { by, aggs: [inner] }` instead of inlining into `SketchAgg.op` ## Diff: 8 files, +579 / -151 (256-line new file + 7 modified) 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
Unifying first move in the revised legacy_expr migration plan. After Batch 2's A-lifts, the 3d/3e agents surfaced an architectural wall: legacy and canonical
QueryExprare unrelated types; consumer migration requires Schema threading first.Step α takes a different cut: at the type-of-field level, replace
legacy_expr::AggIntentwith canonicalintent_algebra::agg_intent::AggIntentin every field that carried legacy AggIntent. The wrappinglegacy_expr::QueryExprvariants (SketchAgg,WindowedAgg, etc.) stay intact — they now carry canonical AggIntent as theirop/aggfield.Unblocks Step β (Schema threading) and Step γ (variant-by-variant consumer migration).
Translation strategy: option (a) — re-export +
PerPartitionWraplegacy_expr.rsnow does:The legacy
AggIntentenum +ExactAgghelper enum are deleted. Free-function helpers (agg_is_mergeable,agg_is_exact,agg_to_legacy_agg_type,agg_quantiles,agg_accuracy, etc.) replace the oldAggIntent::method()API.Translation table
Quantile { quantiles: Vec<f64>, accuracy: f64 }Quantile { q, accuracy: AccuracyTarget }Cardinality/Frequency { accuracy: f64 }accuracy: Exact if 0.0 else Epsilon(eps)Extrema { min: true, max: false }MinExtrema { min: false, max: true }MaxExtrema { min: true, max: true }Min+MaxExact(ExactAgg::Sum/Count/Avg/Min/Max)PerPartition { inner, keys }PerPartitionWrap { inner: canonical, keys }(carrier; collapses in Step γ)Fan-out: F1 — Merge-wrap
legacy_lower::agg_func_to_intentsreturnsVec<AggIntent>;lower_aggregatewraps siblings inQueryExpr::Merge { inputs: [SketchAgg|WindowedAgg, …] }. SketchAgg/WindowedAgg shapes unchanged; consumers keep matching single-intentop: AggIntent.Active fan-out paths:
AggFunc::StdDev/AggFunc::Variance→ Merge of 2 Quantile siblings (q=0.25, 0.75)R9 HydraConversion: DISABLED
R9's multi-key
Partition + sketchfusion inlinedPerPartitionintoSketchAgg::op— a shape canonical AggIntent doesn't support. R9'stry_rewritereturnsNone; original logic preserved under#[allow(dead_code)] mod hydra_conversion_legacywith a Step γ TODO.Disabling a cost-driven rule preserves correctness (unfused tree still produces the right result, just less efficient for multi-key Partition + sketch cases).
Sites migrated
legacy_lower,legacy_expr,physical/{allocator,planner,stage_split},optimizer/enginesketch_catalog,allocator,stage_split,query_parser,optimizer/enginelegacy_expr::AggFunckept — it's L2 (AggItem.func) per Plan agent's guidance. 4 references all inquery_parser/mod.rs::collect_agg_func*.Build + test
cargo build --release -p controller— cleancargo build --release -p query_engine_rust— cleancargo test -p controller --lib— 666 / 666 pass (no regression)cargo test -p controller --bin controller— 27 / 27 passVerification grep
::AggFunc\b→ 4 hits, all inquery_parser/mod.rs(L2 intentional)legacy_expr::AggIntent\b | ExactAgg\b→ 1 hit (thepub usere-export instage_split.rs) + commentsExtrema {→ 1 hit,EvalFunc::Extremainphysical/planner.rs(unrelated L5 enum)4 TODOs flagged for Step γ
Aggregate { by: keys, aggs: [inner] }sketch_op_to_promqlfall-throughs: route archive-only / TopK / Rate / Increase through their dedicated PromQL templates instead ofcount_over_timefallbackagg_to_legacy_agg_typetaxonomy collapse: γ plumb canonical kind directly into planner; remove the coarseAggTypeenumDiff: 8 files, +556 / -337
🤖 Generated with Claude Code