refactor(legacy_expr): step β — thread Schema through legacy planning stack - #139
Merged
Merged
Conversation
… stack 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 was referenced Sep 11, 2026
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
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 traversal-environment parameter (approach c — lightest touch; mirrors how canonical
QueryExpralready passes schema downward).What landed
New:
controller/src/intent_algebra/column_resolution.rs(256 lines, 8 tests)infer_source_schemasynthesises the default time-series shape(ts: Timestamp, value: Float64)per the deferred SchemaCatalog decision.infer_schema_for_rootwalks to the outermostSourceleaf via existingQueryExpr::source_name()and applies the default; falls back toSchema::default()for bareRef(_)trees.Threading applied to
optimizer::engine::RewriteRule::try_rewrite— trait + 12 implsoptimizer::engine::QueryOptimizer::{apply_all, recurse_children, apply_rules_at}physical::allocator::SketchAllocator::{alloc_node, alloc_sketch_agg}physical::planner::plan_nodephysical::stage_split::{walk, promql_from_qe}query_parser::mod::QeCollector::visitintent_algebra::legacy_lower::lower_to_sketch_algebra_with_schema(new sibling; old fn delegates after deriving schema)Public entry points (
SketchAllocator::allocate,plan,split_expr_by_stage,expr_to_promql,lower_to_sketch_algebra,qe_to_parsed_query,QueryOptimizer::optimize) keep existing signatures — each derives the root schema and threads it inward. Backwards compatibility intact.Build + test
cargo build --release -p controllercleancargo build --release -p query_engine_rustcleancargo test --release -p controller --lib— 674 passed (was 666 after Step α; +8 newcolumn_resolutiontests)cargo test --release -p controller --bin controller— 27 passed7 TODOs flagged for Step γ
Aggregate { keys: Vec<String> }→Vec<ColumnId>migration — needs schema transformation as we descend into Aggregate's childDistinct { cols }consumer inphysical::stage_splitcurrently logs ColumnIds without using them —BackendSubPlanneeds adistinct_cols: Vec<ColumnId>fieldProject { cols }reshapes columns canonically; legacy passes parent schema through verbatimLetBinding/Refneed aBindingScopeanalogue plumbed alongside Schema*labels) aren't representable in canonical Schema today — needs aDataType::LabelSetor parallel side-channel before fullSource → ScanmigrationCostModel::estimate(&self, expr: &QueryExpr)doesn't take Schema (inspects only outermost node). Future cost estimators may need the signature extendedoptimizer::engine::HydraConversion(disabled in Step α) could now consult the schema to emit canonicalAggregate { by, aggs: [inner] }instead of inlining intoSketchAgg.opDiff: 8 files, +579 / -151
🤖 Generated with Claude Code