refactor(intent_algebra): reconcile the two L3-construction paths; intent_algebra::lower is now the single L2→L3 pass - #232
Merged
Conversation
…o lower_to_canonical
There were two ways to build a canonical L3 `QueryExpr`:
* `convert_root` — the real path: relational L2 tree → canonical L3,
via `lower_to_canonical`. Faithful (window-swap, Partition, etc.).
* `lower_parsed_query` (`intent_algebra/lower.rs`) — reconstructed a
canonical tree from the *flat* `ParsedQuery` summary. Lossy: it
dropped exact aggregates entirely (no `Aggregate` node for
`rate`/`sum`/`increase`/bare selectors) and threaded an explicit
accuracy the real path doesn't take.
Only `asap_tier_analysis` and one `sketch_algebra` test helper used the
lossy path. Both are switched to `parse_query_expr_canonical` (the real
path) and `lower_parsed_query` + its module are deleted.
**Behavior change** (intended — see PR discussion): on the real path,
`rate`/`sum`/`increase`/bare-selector lower to `AggIntent::Sum`, and
`capability_for(&Sum)` returns `Capability::ExactAgg(Sum)` — so they are
now ASAP-tier-answerable from exact-precompute state instead of
unconditionally archive-routed. `lower_parsed_query` was *masking* the
`ExactAgg` capability by dropping those intents. The 7 `asap_tier_analysis`
tests that pinned the old "route to archive" behavior are rewritten to
pin the new ExactAgg routing.
**L4 binder fix**: feeding `sketch_algebra::bind_query_expr` the real
canonical IR exposed that its bottom-up walk didn't recurse into
`Window` — so `Window { Aggregate }` (the canonical windowed-sketch
shape) never reached the `Bind*` rules. `bind_recursive` now recognizes
`Window { Aggregate { .. } }`, pushes the window under the aggregate,
and re-dispatches — the window rides along inside the bound node's
`Logical(..)` child, exactly as it did when the aggregate sat on top.
Finally: with the old `intent_algebra/lower.rs` gone, the name is free,
so `lower_to_canonical.rs` → `lower.rs` — `intent_algebra::lower` is now
the single L2→L3 lowering, matching design.md's `core::lower`.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
3 tasks
zzylol
added a commit
that referenced
this pull request
May 14, 2026
…gg routing change (#236) PR #232 changed `control_plane::asap_tier_analysis`: `rate` / `irate` / `increase` / `sum` and bare selectors now lower to `AggIntent::Sum`, which `capability_for` maps to `Capability::ExactAgg(Sum)` — so they are ASAP-tier-answerable from exact-precompute state instead of being archive-routed. `data_plane` consumes that analyzer, and two of its tests pinned the old behaviour: * `analyzer_parity_18_query_corpus` — the 18-query golden master (PR #188). The `ctrl` rows for q04–q07, q12–q17 (and the q08 label `count_over_time` → `count`) shifted to `OK [... ExactAgg(Sum) ...]` / candidate rows. `GOLDEN` regenerated from the new actual; the `engine` rows — the actual parity contract — are unchanged. * `execute_rejects_bare_selector_via_analyzer` → renamed `execute_bare_selector_falls_over_to_archive`. A bare selector is no longer rejected with `NoCallNodeFound`; it binds to an `ExactAgg(Sum)` candidate that finds no matching policy in the test's DDSketch-only `SketchStore`, so it still `CapabilityMiss`es to the archive engine — same routing outcome, different detail. No behaviour change here — `data_plane`'s test expectations catching up to the intentional #232 analyzer change. 768 lib tests pass. Pre-existing unrelated failure: `kll_envelope_round_trip_through_backend_adapter` (KLL sketch byte serialization) fails on clean `main` too — untouched by this change. 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
1 participant
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.
PR 3 of the "de-legacy the L1/L2 path" cleanup (follows #228–#231).
The two paths
There were two ways to build a canonical L3
QueryExpr:convert_root— relational L2 tree → canonical L3 (lower_to_canonical)Partition, fusion)lower_parsed_query(intent_algebra/lower.rs) — reconstructs a tree from the flatParsedQuerysummaryrate/sum/increase/bare selector → noAggregatenode); threads an explicit accuracy the real path doesn't takeOnly
asap_tier_analysisand onesketch_algebratest helper used the lossy path. Both are switched toparse_query_expr_canonical;lower_parsed_query+ its module are deleted.Behavior change (intended)
On the real path,
rate/sum/increase/bare-selector lower toAggIntent::Sum, andcapability_for(&Sum)returnsCapability::ExactAgg(Sum)— so they're now ASAP-tier-answerable from exact-precompute state instead of unconditionally archive-routed.lower_parsed_querywas masking theExactAggcapability by dropping those intents. The 7asap_tier_analysistests that pinned the old archive-routing are rewritten to pin the new ExactAgg routing.L4 binder fix
Feeding
sketch_algebra::bind_query_exprthe real canonical IR exposed a latent bug: its bottom-up walk didn't recurse intoWindow, so the canonicalWindow { Aggregate }windowed-sketch shape never reached theBind*rules.bind_recursivenow recognizesWindow { Aggregate { .. } }, pushes the window under the aggregate, and re-dispatches — the window rides along inside the bound node'sLogical(..)child. (NoPhysicalExprIR change; localized tobind_recursive.)Rename
With the old
intent_algebra/lower.rsgone,lower_to_canonical.rs→lower.rs.intent_algebra::loweris now the single L2→L3 lowering, matching design.md'score::lower.Test plan
cargo build --workspacecleancargo test -p control_plane— 737 pass (710 + 27), 0 failures🤖 Generated with Claude Code