refactor(legacy_expr): step γ1 — Aggregate bridge module + output_schema helper - #140
Merged
Merged
Conversation
…ema helper
First sub-PR of Step γ (variant-by-variant consumer migration). Adopts
the (c) bridge strategy: keep `legacy_expr::QueryExpr::Aggregate` as
the L2 emit shape; add a one-way canonical-builder helper consumers
call on demand. No construction site rewritten; no bridge enum
variant added to legacy QueryExpr.
This avoids the "child must be canonical" coupling that would otherwise
force γ1 to migrate every other legacy variant (SketchAgg, WindowedAgg,
TopK, etc.) in the same PR.
## What landed
### New: `controller/src/intent_algebra/aggregate_bridge.rs` (328 lines, 8 tests)
```rust
pub fn bridge_aggregate_to_canonical(
keys: &[ColumnRef],
aggs: &[AggItem],
having: &Option<legacy::Predicate>,
schema: &Schema,
) -> Result<BridgedAggregate, BridgeError>;
pub struct BridgedAggregate {
pub by: Vec<ColumnId>,
pub aggs: Vec<AggIntent>,
pub having: Option<HavingPredicate>,
}
pub enum BridgeError {
UnresolvedKey(ResolveError),
HavingDeferred(QueryExprError),
}
```
The `having` translation routes through `from_legacy_scalar` (Batch 2).
E-deferred ScalarExpr variants (`FunctionCall`, `ScalarSubquery`,
`InList`, `Between`) surface as `BridgeError::HavingDeferred(...)`.
Test `bridge_having_deferred_e_variant_surfaces_error` is the contract;
no in-tree construction site builds a HAVING with E-variants today.
### `column_resolution.rs` extension (+211 lines, +6 tests)
```rust
pub fn output_schema_for_aggregate(
input: &Schema,
by: &[ColumnId],
aggs: &[AggIntent],
) -> Schema;
pub fn resolve_named_keys(keys: &[ColumnRef], schema: &Schema)
-> Result<Vec<ColumnId>, ResolveError>;
```
Mirrors canonical `query_expr::QueryExpr::output_schema_in`'s Aggregate
arm: outputs `by`-columns positionally + one column per
`AggIntent::output_column(probe)`, strips `time_index`, sets
`unique_keys = [by]`. This was Step β TODO #1.
Step γ2-γ4 consumers descending into legacy `Aggregate.input` should
pass `output_schema_for_aggregate(parent_schema, &bridged.by, &bridged.aggs)`
as the inner subtree's `parent_schema`.
### Demo wire in `physical/allocator.rs::alloc_node`
The legacy `QueryExpr::Aggregate { keys, aggs, having, input }` arm
now calls the bridge to derive canonical-shape data and enrich
`NodeAnnotation.rationale` with intent kinds + group-by column count.
Emit shape stays legacy; behavior unchanged (only the rationale string
carries extra info). Proves the bridge is reachable.
## Construction-site migration: 0 (intentional)
Per strategy (c), all ~10 construction sites in
`query_parser/{promql,sql}.rs`, `legacy_lower::lower_aggregate`, and
the optimizer rewrite path still emit `legacy_expr::QueryExpr::Aggregate`.
They migrate in γ7 once no legacy `input` subtree remains (SketchAgg,
WindowedAgg, TopK migrations land first in γ2-γ4).
## Build + test
- `cargo build --release -p controller` — clean
- `cargo build --release -p query_engine_rust` — clean
- `cargo test -p controller --lib` — **688 passed** (was 674; +14 new
bridge + column_resolution tests)
- `cargo test -p controller --bin controller` — 27 passed
## Known caveats
- Canonical `QueryExpr::Aggregate.having` field is `Option<HavingPredicate>`
where `HavingPredicate(pub String)`, NOT typed `Option<Predicate>` as
the spec text suggested. Bridge renders the converted `Predicate` via
`format!("{pred:?}")`. Round-tripping from the string is a follow-up
(when canonical `having` upgrades to typed `Predicate`).
- `BridgeError` can't derive `PartialEq` (QueryExprError doesn't); tests
use `matches!`. Non-blocking.
- `optimizer::engine::HydraConversion` (Step β TODO #7) NOT migrated —
out of γ1's stated scope, deferred to a later γ sub-PR.
## Diff: 5 files, +618 / -4
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2 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
First sub-PR of Step γ (variant-by-variant consumer migration). Adopts the (c) bridge strategy: keep
legacy_expr::QueryExpr::Aggregateas the L2 emit shape; add a one-way canonical-builder helper consumers call on demand. No construction site rewritten; no bridge enum variant added to legacy QueryExpr.This avoids the "child must be canonical" coupling that would otherwise force γ1 to migrate every other legacy variant (SketchAgg / WindowedAgg / TopK / …) in the same PR. The bridge module +
output_schema_for_aggregateare the foundation γ2-γ4 will consume.What landed
New
controller/src/intent_algebra/aggregate_bridge.rs(328 lines, 8 tests)The
havingtranslation routes throughfrom_legacy_scalar(Batch 2). E-deferred ScalarExpr variants surface asBridgeError::HavingDeferred(...)— testbridge_having_deferred_e_variant_surfaces_erroris the contract.column_resolution.rsextension (+211, +6 tests)Mirrors canonical
query_expr::QueryExpr::output_schema_in's Aggregate arm. This was Step β TODO #1. γ2-γ4 consumers descending into legacyAggregate.inputshould pass the output schema as the inner subtree'sparent_schema.Demo wire in
physical/allocator.rs::alloc_nodeThe legacy Aggregate arm calls the bridge to enrich
NodeAnnotation.rationalewith intent kinds + group-by column count. Emit shape stays legacy; behavior unchanged. Proves the bridge is reachable.Construction-site migration: 0 (intentional)
Per strategy (c). γ7 flips construction sites once γ2-γ4 retire legacy
SketchAgg/WindowedAgg/TopK— so no legacyinputsubtree remains.Build + test
cargo build --release -p controllercleancargo build --release -p query_engine_rustcleancargo test -p controller --lib— 688 passed (was 674; +14 new bridge + column_resolution tests)cargo test -p controller --bin controller— 27 passedCaveats
QueryExpr::Aggregate.havingisOption<HavingPredicate>(HavingPredicate(pub String)), NOT typedOption<Predicate>as spec suggested. Bridge renders convertedPredicateviaformat!("{pred:?}"). Roundtripping is a follow-up.BridgeErrorcan't derivePartialEq(QueryExprError doesn't); tests usematches!.optimizer::engine::HydraConversion(Step β TODO test: e2e test for modified-OTLP CountMin sketch hot path (PR D) #7) NOT migrated — deferred.Diff: 5 files, +618 / -4
🤖 Generated with Claude Code