refactor(legacy_expr): batch 1 — D-purge + Dedup→Distinct rename - #135
Merged
Merged
Conversation
Lowest-risk first batch of the legacy_expr retirement plan (Plan agent's
13-PR sequence). Deletes 5 QueryExpr variants + 5 ScalarExpr variants
that have ZERO constructor sites (only match arms), and renames
`QueryExpr::Dedup { col }` → `QueryExpr::Distinct { cols: Vec<ColumnRef> }`
with N-arity generalization.
## Deleted variants (zero constructor sites; only match arms)
QueryExpr family:
- `QueryExpr::JoinSketch { join_key, outer, inner }` — sketch-bound
physical alternative; design.md says L3 must be intent-only and the
sketch-bound shape belongs at L4 (`sketch_algebra::SketchExpr::SketchJoin`).
No constructor sites; 5 consumer match arms removed (query_parser,
physical/{stage_split,planner,allocator}, optimizer/engine).
- `QueryExpr::Subquery { alias, expr }` — designed for SQL nested
SELECT lowering, but no parser emits it. 6 consumer match arms.
- `QueryExpr::WindowFunc { func, partition_by, order_by, frame, input }`
— designed for SQL OVER() clauses, no parser emits it. 5 arms.
- `SketchCoverage` (legacy_expr-internal enum) — referenced only by an
in-file test. Deleted along with the test.
ScalarExpr family:
- `UnaryOp { op, input }` — 0 consumer arms.
- `InSubquery { expr, subquery, negated }` — 0 arms.
- `Case { operand, when_then, else_ }` — 0 arms.
- `Cast { expr, to }` — 0 arms.
- `VectorBinaryOp { op, lhs, rhs, vector_match }` — 0 arms (design.md
merges PromQL vector ops into `QueryExpr::BinaryOp { vector_match }`).
## Also dropped (orphaned after the D-purge)
- `UnaryOpKind` enum (only used by `UnaryOp`).
- `WindowFuncKind`, `WindowFrame`, `FrameUnit`, `FrameBound` (only used
by `WindowFunc`).
- Legacy `DataType` enum (only used by `Cast`). The canonical
`intent_algebra::schema::DataType` is untouched.
Reversible if Batch 2's canonical lift needs them.
## Dedup → Distinct rename + N-arity
`QueryExpr::Dedup { col: ColumnRef }` → `QueryExpr::Distinct { cols: Vec<ColumnRef> }`.
Per design.md §6 "What was removed" row 7: *"Single-column-only spelling
of SQL `DISTINCT`. Replacement: renamed to `Distinct { cols }`,
generalised to N columns"*.
Touched sites:
- legacy_expr.rs enum + walk/source_name match arms
- query_parser/mod.rs (sketch-mapping arm)
- physical/stage_split.rs (walk + PromQL serializer — 2 sites)
- physical/planner.rs (new `display_distinct_cols(&[ColumnRef])` helper)
- physical/allocator.rs (incl. routing doc table)
- optimizer/engine.rs (cost factor, deployment-stage map,
HLLDedupElim pattern, recursive rewriter, 2 tests)
- intent_algebra/legacy_lower.rs
HLLDedupElim rule struct name kept (semantically still describes the
rule); only test names/asserts updated.
## Canonical AggIntent verification
`controller/src/intent_algebra/agg_intent.rs` already has no
`HistogramQuantile` (removed in #133) and no `PromQLSubquery`. No
change needed.
## Build + test
- `cargo build --release -p controller` — clean
- `cargo build --release -p query_engine_rust` — clean
- `cargo test -p controller --lib` — **655 passed, 0 failed**
(was 656/656; -1 because `sketch_coverage_classification` test went
with the deleted `SketchCoverage` enum)
- `cargo test -p query_engine_rust --lib -- engines::warm_tier` — 13/13 pass
## Grep verification
- `JoinSketch | QueryExpr::Subquery | QueryExpr::WindowFunc | SketchCoverage |
ScalarExpr::{UnaryOp,InSubquery,Case,Cast,VectorBinaryOp}` → zero
matches across `controller/src/` and `asap-query-engine/src/`.
(The 6 remaining `::Subquery\b` hits are `Expr::Subquery(sq)` from
the external `promql_parser` / `sqlparser` AST libraries — not our
type; pre-existing and out of scope.)
- `QueryExpr::Dedup | Dedup {` → zero matches.
## Diff: 7 files, +66 / -323 (net -257 lines)
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
Lowest-risk first batch of the legacy_expr retirement plan (the Plan agent's 13-PR sequence). Deletes 10 unused variants + does one structural rename. Lays the groundwork for Batch 2 (A-lifts into canonical L3) and the heavier C-reshape PRs that follow.
What landed
Deleted: 5
QueryExprvariants (0 constructor sites)JoinSketch { join_key, outer, inner }sketch_algebra::SketchExpr::SketchJoin)Subquery { alias, expr }WindowFunc { …, frame, … }OVER(), no parser emits itSketchCoverageDeleted: 5
ScalarExprvariants (0 constructor sites)UnaryOp { op, input }InSubquery { …, subquery, … }Case { operand, when_then, else_ }Cast { expr, to }VectorBinaryOp { …, vector_match }QueryExpr::BinaryOp { vector_match }Orphaned supporting types also dropped
UnaryOpKind,WindowFuncKind,WindowFrame,FrameUnit,FrameBound, legacyDataType. All had no consumers after the D-purge. The canonicalintent_algebra::schema::DataTypeis untouched.Dedup→Distinctrename + N-arityQueryExpr::Dedup { col: ColumnRef }→QueryExpr::Distinct { cols: Vec<ColumnRef> }. Per design.md §6 "What was removed" row 7.Touched: enum decl, walk/source_name arms,
query_parser/mod.rs,physical/stage_split.rs(×2),physical/planner.rs(newdisplay_distinct_colshelper),physical/allocator.rs(incl. routing doc),optimizer/engine.rs(cost factor, deployment-stage map, HLLDedupElim pattern, recursive rewriter, 2 tests),intent_algebra/legacy_lower.rs.HLLDedupElimrule name kept (still semantically accurate).Canonical AggIntent already clean
intent_algebra/agg_intent.rshas noHistogramQuantile(removed in #133) and noPromQLSubquery. No change needed.Build + test
cargo build --release -p controller— cleancargo build --release -p query_engine_rust— cleancargo test -p controller --lib— 655 passed, 0 failed (was 656/656; -1 becausesketch_coverage_classificationtest went with the deletedSketchCoverageenum)cargo test -p query_engine_rust --lib -- engines::warm_tier— 13/13 passGrep verification (final)
JoinSketch | QueryExpr::Subquery | QueryExpr::WindowFunc | SketchCoverage | ScalarExpr::{UnaryOp,InSubquery,Case,Cast,VectorBinaryOp}→ zero matches.QueryExpr::Dedup | Dedup {→ zero matches.::Subquery\bhits (6) areExpr::Subquery(sq)from externalpromql_parser/sqlparserAST libraries, not our type.Diff: 7 files, +66 / -323 (net -257 lines)
Next in the chain
Batch 2 — A-lifts (next PR): Lift
Filter,Project,Partition,Distinct,Merge,Join,SetOp,Sort,Limit,BinaryOpinto canonicalintent_algebra::query_expr::QueryExpr+ a minimal typedPredicate(Column/Literal/BinaryOp/IsNull) forFilter.pred. Then Batch 3 sub-PRs cover the C-reshapes (Source→Scan,WindowedAggun-fusion,SketchAgg→Aggregate@L3,TopKsplit,AggFunc→AggIntent, PromQL parser-sidehistogram_quantile→Quantilesubstitution). Final PR retireslegacy_expr.rs+legacy_lower.rsentirely.🤖 Generated with Claude Code