refactor(legacy_expr): batch 2 — lift 10 A-variants into canonical L3 + typed Predicate - #136
Merged
Merged
Conversation
… + typed Predicate Additive lift of all 10 A-classified legacy variants into the canonical `intent_algebra::query_expr::QueryExpr`, plus the minimal typed `Predicate` IR for `Filter.pred` covering the 4 used-and-cleanly-shaped ScalarExpr variants. ## What landed ### Canonical `QueryExpr` grew from 5 → 15 variants Pre-existing (PR #130 / canonical): `Scan`, `Window`, `Aggregate`, `LetBinding`, `Ref`. New (this PR): `Filter`, `Project`, `Partition`, `Distinct`, `Merge`, `Join`, `SetOp`, `Sort`, `Limit`, `BinaryOp`. Naming convention: single-input variants use `child:` (matches existing canonical `Window`/`Aggregate`/`LetBinding`). Multi-input variants follow design.md §6 shapes exactly: `Merge { children }`, `Join { kind, pred, left, right }`, `SetOp { kind, all, left, right }`, `BinaryOp { op, lhs, rhs, vector_match }`. ### Supporting types lifted alongside `ColumnRef`, `PartitionKeys`, `BinaryOpKind`, `JoinKind`, `SetOpKind`, `SortKey`, `VectorMatch`/`VectorMatchKind`/`VectorGrouping`/`GroupSide`, `LiteralValue`, `ProjectItem`. All re-exported from `intent_algebra::mod.rs`. ### New typed `Predicate` ```rust pub enum Predicate { Column(ColumnRef), Literal(LiteralValue), BinaryOp { op: BinaryOpKind, lhs: Box<Predicate>, rhs: Box<Predicate> }, IsNull { expr: Box<Predicate>, negated: bool }, } ``` Plus `Predicate::from_legacy_scalar(&legacy::ScalarExpr) -> Result<Predicate, QueryExprError>`. Translates the 4 supported variants (Column / Literal / BinaryOp / IsNull); returns `QueryExprError::UnsupportedLegacyScalar(name)` for the 4 E-deferred (FunctionCall / ScalarSubquery / InList / Between). Per user decision, those stay in legacy until a real consumer demands the typed shape. Translation handles `LiteralValue::Duration → Int(nanos)` fold (canonical LiteralValue is deliberately narrower). ### Consumer migration deferred to subsequent batches **Zero consumer-site redirects in this PR** — and that's the right call. Every legacy-A-variant consumer (`query_parser/`, `physical/ {stage_split,planner,allocator}.rs`, `optimizer/engine.rs`, `legacy_lower.rs`) simultaneously matches A-variants AND C-variants (`Source`, `Aggregate`, `Window`, `SketchAgg`, `WindowedAgg`, `TopK`, `HistogramQuantile`, `PromQLSubquery`) AND constructs `ScalarExpr` predicates with E-deferred variants. Migrating them mid-pipeline would require lifting C-variants and E-variants in the same PR — explicit out-of-scope per the batch plan. The consumers migrate one C-batch at a time: - Batch 3a (PR #11): `AggFunc → AggIntent` - Batch 3b (PR #10): `TopK` split - Batch 3c (PR #8): `WindowedAgg` un-fusion - Batch 3d (PR #9): `SketchAgg → Aggregate@L3` - Batch 3e (PR #12): PromQL parser `histogram_quantile → Quantile` - Batch 13: retire `legacy_expr.rs` + `legacy_lower.rs` ### Catch-all arms added in 6 canonical-side consumers Files: `optimizer/cost/mod.rs`, `physical/colored_dag/{allocator,emitter}.rs`, `sketch_algebra/lower.rs`, `warm_tier_analysis.rs`. These previously matched canonical `QueryExpr` exhaustively over the 5 pre-existing variants; now they need conservative handling for the 10 new ones (Edge stage / Logical wrap / child-walk / zero-cost) with TODO markers for the follow-up reshape batches. ### Serde tag note Initially gave `ColumnRef`/`LiteralValue`/`PartitionKeys`/`Predicate` internally-tagged `#[serde(tag = "kind", rename_all = "snake_case")]` attrs, but serde rejects internally-tagged newtype variants containing primitives (e.g. `Named(String)`). Switched to externally-tagged `#[serde(rename_all = "snake_case")]`. Caught by the round-trip test. ## Build + test - `cargo build --release -p controller` — clean - `cargo build --release -p query_engine_rust` — clean - `cargo test -p controller --lib` — **666 passed** (was 655 after Batch 1; +11 from new typed-Predicate + schema tests) - `cargo test -p query_engine_rust --lib -- engines::warm_tier` — 13/13 ## Diff: 8 files, +794 / -8 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
Additive lift of all 10 A-classified legacy variants into the canonical
intent_algebra::query_expr::QueryExpr, plus the minimal typedPredicateIR forFilter.pred. Zero consumer-site redirects — those happen in subsequent C-reshape batches.What landed
Canonical
QueryExprgrew 5 → 15 variantsScan,Window,Aggregate,LetBinding,RefFilter,Project,Partition,Distinct,Merge,Join,SetOp,Sort,Limit,BinaryOpNaming: single-input variants use
child:(canonical convention). Multi-input variants follow design.md §6 shapes verbatim.Supporting types
ColumnRef,PartitionKeys,BinaryOpKind,JoinKind,SetOpKind,SortKey,VectorMatch{,Kind},VectorGrouping,GroupSide,LiteralValue,ProjectItem— all lifted into canonical + re-exported fromintent_algebra::mod.rs.New typed
PredicatePlus
Predicate::from_legacy_scalar(&legacy::ScalarExpr) -> Result<Predicate, QueryExprError>. Translates the 4 supported variants; returnsQueryExprError::UnsupportedLegacyScalar(name)for the 4 E-deferred (FunctionCall / ScalarSubquery / InList / Between) per user decision.Handles
LiteralValue::Duration → Int(nanos)fold (canonicalLiteralValuedeliberately narrower).Why zero consumer redirects in this PR
Agent's honest finding: every legacy-A-variant consumer (
query_parser/,physical/{stage_split,planner,allocator}.rs,optimizer/engine.rs,legacy_lower.rs) simultaneously matches A-variants AND C-variants (Source,Aggregate,Window,SketchAgg,WindowedAgg,TopK, etc.) AND constructsScalarExprpredicates with E-deferred variants. Migrating them mid-pipeline would require lifting C-variants and E-variants in the same PR — out of scope per the batch plan.Consumers migrate one C-batch at a time:
AggFunc → AggIntent(medium)TopKsplit intoAggIntent::TopKvsSort+Limit(medium)WindowedAggun-fusion — 20+ consumers; HIGH riskSketchAgg → Aggregate@L3+ L4 binding reads canonical; HIGH riskhistogram_quantile → Quantilesubstitution + drop legacyHistogramQuantile/PromQLSubquery(low/mechanical)legacy_expr.rs+legacy_lower.rsCatch-all arms in 6 canonical-side consumers
Files:
optimizer/cost/mod.rs,physical/colored_dag/{allocator,emitter}.rs,sketch_algebra/lower.rs,warm_tier_analysis.rs. These previously matched canonicalQueryExprexhaustively over the 5 pre-existing variants; now they handle the 10 new ones conservatively (Edge stage / Logical wrap / child-walk / zero-cost) with TODO markers.Serde caveat
Initially used
#[serde(tag = "kind")]onColumnRef/LiteralValue/PartitionKeys/Predicate, but serde rejects internally-tagged newtype variants containing primitives. Switched to externally-tagged#[serde(rename_all = "snake_case")]. Caught bya_variant_serde_roundtrip_filter.Build + test
cargo build --release -p controller— cleancargo build --release -p query_engine_rust— cleancargo test -p controller --lib— 666 passed (was 655 after Batch 1; +11 new tests covering schema pass-through + Predicate translation + serde round-trip)cargo test -p query_engine_rust --lib -- engines::warm_tier— 13/13 passDiff: 8 files, +794 / -8
🤖 Generated with Claude Code