refactor: delete crates/l2 entirely — both front ends emit canonical QueryExpr directly - #213
Merged
Merged
Conversation
crates/l2 (asap-l2) no longer exists as a crate — its five modules (relational, binder, column_resolution, canonicalize, lower) move into asap-types::pre_asap verbatim (only intra-crate `use` paths change), satisfying #179's concrete deliverable: "crates/l2 itself should not survive this migration as a crate." binder/column_resolution/canonicalize land here permanently, per #179's own rationale — after the front-end migration below, they have no front-end-specific logic left and operate directly on this crate's own QueryExpr. relational/lower are explicitly marked "legacy, pending deletion" in their module docs: they're the L2 relational tree and its L2→L3 converter (convert_root), which only go away once both front ends emit canonical QueryExpr directly during their own `interpret` step instead of building a pre-canonical shape first — a separate, larger change with its own open design questions, deliberately not attempted in this pass. Both front ends (frontend-promql, frontend-sql) drop their asap-l2 dependency and import convert_root/relational/Binder/ConvertError from asap_types::pre_asap instead — a pure import-path change, zero behavior difference. Stale doc-only asap_l2:: references in asap-aware-mapping and devtools fixed to match. Verified: cargo build --workspace, cargo clippy --workspace --all-targets (zero warnings), cargo test --workspace all green (470+ tests, 0 failed), cargo doc --no-deps introduces no new unresolved-link warnings beyond what pre-existed.
…state (#179, #205) Resolves the open design question #179 flags but leaves unresolved: front ends need a canonical-shaped tree they can build with unresolved column references, before the Binder has run. Both QueryExpr and AggIntent (wherever it names an input column) now take a type parameter C, defaulting to ColumnId so every existing use of the bare `QueryExpr` / `AggIntent` name — the entire rest of the workspace — keeps compiling and behaving identically, unchanged: - QueryExpr<C: ColState = ColumnId>, plus Predicate<C>, ProjectItem<C>, SortKey<C>, GroupKeys<C>, Reduction<C> alongside it. - AggIntent<C = ColumnId> for the ~8 single-column-reducer variants (Sum/Min/Max/Avg/StdDev/Variance/Quantile/Cardinality) that carry `col: Option<C>` — SQL's `SUM(bytes_in)` needs to name an unresolved column exactly the same way a WHERE/projection expression does. - L3QueryExpr = QueryExpr<ColumnId> (the canonical, resolved tree — what `QueryExpr` has always meant) and L2QueryExpr = QueryExpr<ColumnRef> (front-end-emitted, unresolved) aliases, named the same way L2Expr/L3Expr already are. - New ColState trait: the one place the two states differ in *shape*, not just in which type fills C, is Scan.schema — always-known Schema once bound (ColumnId), vs Option<Schema> before binding (ColumnRef; Some for a catalog-backed SQL leaf, None for PromQL, deferred to the Binder). ColState::ScanSchema picks it. - output_schema (QueryExpr) and requires/is_per_series/output_column (AggIntent) stay on the concrete ColumnId instantiation rather than going generic over C: like the open question #205 raises about per-node typing, these are schema-shaped properties only meaningful once binding has picked a column identity. input_col (AggIntent) is the one method that hands `col` back to the caller, so it alone is generic. Not yet used: no front end constructs QueryExpr<ColumnRef>/L2QueryExpr yet (still emitting the separate relational::QueryExpr, per the previous commit's note) — this is the type-level foundation for that follow-up, landed and verified on its own first. Verified: cargo build -p asap-types, cargo test -p asap-types --lib (67/67, zero test-code changes needed for QueryExpr — every existing construction sits inside a function with an explicit `-> QueryExpr` return type, so C = ColumnId is inferred same as before); cargo build --workspace and cargo test --workspace both still fully green with zero changes needed anywhere outside asap-types, confirming the default type parameter kept every existing consumer's behavior byte-for-byte identical.
The PromQL front end no longer builds the legacy relational::QueryExpr tree at all. promql.rs constructs the canonical L2QueryExpr (QueryExpr<ColumnRef>) directly during its own interpret step, folding in the "local, context-free structural rewriting" #179 describes as not needing a separate converter: heavy-hitter topk recognition, Window/TimeRange fusion, the PerEntity-vs-Reduce reduction decision (reduction_for), Filter-over-Source -> Scan.predicates folding, and the AggFunc->AggIntent mapping (now inline: inner_intent/outer_intent) all happen right here, at parse time, using primitives the front end already has in scope -- no relational tree ever gets built as an intermediate step. New in asap-types: Binder::bind_query_expr(_with_inherited), the canonical-tree counterpart to bind(_with_inherited), and resolve_root/resolve (crates/types/src/pre_asap/resolve.rs) -- the "single generic, shape-preserving walk that resolves every ColumnRef to a Binder-computed ColumnId" #179 calls for, replacing convert_root for a front end that already emits canonical shape. Every L2QueryExpr variant maps 1:1 to the identical L3QueryExpr variant -- no more per-variant structural translation, since the front end has already picked the right shape. Two additions to AggIntent<C> beyond the prior generic-foundation commit, both surfaced by actually wiring this up: - is_per_series/requires/output_column moved back to the generic impl<C> block (from the concrete-only restriction in the prior commit): a front end building AggIntent<ColumnRef> directly needs is_per_series() pre-binding to decide the reduction shape itself (reduction_for) -- the "only meaningful post-binding" assumption that motivated restricting them was wrong for this exact case. - resolve_reduction (resolve.rs) uses the lenient resolve_group_keys_promql, not the strict resolve_group_keys, for Aggregate.reduction specifically -- PromQL's absent-label semantics (issue #53: a key absent from a closed schema is dropped, not rejected) -- mirroring lower::convert's per-series-fused branch, the only one a canonical-shape-emitting front end's Aggregate nodes ever take (always single-measure, always HAVING-less). Caught by outer_group_key_absent_from_nested_aggregate_is_dropped failing. Accuracy threading moved from a post-hoc convert_root(&l2, &accuracy) pass to construction time: PromqlLowerer::lower now takes accuracy: &AccuracyTarget and installs it as an ambient thread-local for the duration of lowering (AccuracyGuard, same shape as the existing histogram::CatalogGuard) so the ~30 mutually-recursive walk functions don't all need a new parameter -- only the handful of call sites building an accuracy-bearing AggIntent consult it. crates/l2's relational tree and lower::convert_root are NOT deleted: frontend-sql still depends on both (SQL migration is an explicit follow-up, per the issue's own "multi-PR migration" guidance) -- resolve.rs and the new Binder methods are purely additive. Verified: cargo build --workspace, cargo clippy --workspace --all-targets (zero warnings), cargo test --workspace fully green (500+ tests, 0 failed) -- including all 100 promql_conformance cases, all 13 awesome_prometheus_alerts cases, the o11y_bench corpus, and every promql_lowering/promql_equivalence test, with zero test-code changes required anywhere.
…e legacy relational tree and converter entirely (#179) Completes #179: crates/l2's relational tree and L2->L3 converter are now fully gone, not just the crate boundary (landed earlier) -- both front ends construct canonical QueryExpr directly, so nothing constructs the old shape anymore. SQL front end (sql/mod.rs): builds L2QueryExpr (QueryExpr<ColumnRef>) directly during LogicalPlan lowering, same as the PromQL migration. Unlike PromQL, SQL needs no reduction-shape decision at construction time -- DataFusion's Aggregate plan node is always Reduction::Reduce, never PromQL's per-series PerEntity (SQL has no windowed/subquery-child concept) -- so lower_aggregate/lower_grouping_sets always build Reduce(GroupKeys::by(keys)) directly. Two things SQL's own front end now has to do itself, since a converter no longer does it implicitly: - filter_or_fold: fold a WHERE directly over a bare Scan onto Scan.predicates (canonical's "a Filter never sits directly over a Scan" invariant) -- lower_filter no longer emits a bare Filter over a Source unconditionally. - An unconditional JOIN/EXISTS (no ON/correlation predicate) now builds Predicate(Literal(Boolean(true))) explicitly, replacing the Option<L2Expr>::None convention the old converter's Join arm resolved implicitly. lower_agg_item -> lower_agg_intent maps a DataFusion aggregate expression straight to AggIntent<ColumnRef> (no AggFunc intermediate); accuracy threads through the same ambient-thread-local AccuracyGuard pattern PromQL's migration introduced (installed after the lowerer's only .await point, since lower_plan itself is synchronous -- no further suspension point that could move the task to a different OS thread out from under a thread-local set beforehand). Verified narrowly first: cargo test -p asap-frontend-sql passed fully on the first real attempt (68/68: bgp_analytics, netflow, sql_lowering's 57 cases including filter-fold and unconditional-join cases, synthetic_packet_trace) -- no fix-up round needed, unlike PromQL's migration which needed two real corrections. Final cleanup, once both front ends no longer referenced them: - Deleted crates/types/src/pre_asap/relational.rs and lower.rs entirely (convert/convert_root/ConvertError/AggFunc/AggItem/ SourceSpec/L2ProjectItem/L2SortKey all gone with them) -- verified nothing outside those two files depended on anything but their own doc comments first. - binder.rs: removed the relational-tree-based bind/bind_with_inherited and collect_referenced_columns, renamed bind_query_expr(_with_inherited) to bind(_with_inherited) -- there is only one Binder API now, no reason for the _query_expr suffix. Ported the 4 unit tests that exercised the old relational-tree bind methods to build canonical L2QueryExpr fixtures instead, rather than net-losing coverage. - column_resolution.rs: removed infer_schema_for_root/infer_source_schema (relational::QueryExpr::source_name-based, no callers left); the 5 tests that used infer_source_schema as a schema-building convenience now use a small test-local ts_value_schema() helper instead. - Removed 5 lower.rs-specific unit tests that exercised convert/ convert_root directly -- equivalent coverage now comes from the front-end integration tests (promql_lowering.rs, sql_lowering.rs), which exercise the same scenarios end-to-end through resolve_root. - Fixed every doc comment/intra-doc link across asap-types, asap-frontend-promql, asap-frontend-sql, and asap-aware-mapping that referenced relational::/convert_root/lower::convert/AggFunc/AggItem/ SourceSpec by name. Verified: cargo build --workspace, cargo clippy --workspace --all-targets (zero warnings), cargo doc --workspace --no-deps (zero new warnings beyond what pre-existed before this migration), cargo test --workspace fully green (61 asap-types + 68 SQL + 180 PromQL + everything else, 0 failed). Test count differences from the prior commit are fully accounted for: -1 (removed source_schema_has_ts_and_value, tested the now-deleted infer_source_schema) and -5 (removed lower.rs's own convert/ convert_root unit tests, superseded by end-to-end front-end coverage).
milindsrivastava1997
approved these changes
Aug 18, 2026
Fixes the format-and-lint CI check on #213. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
zzylol
changed the title
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
2 participants
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
Closes #179. Four commits, each independently buildable/testable:
258e94d—crates/l2deleted as a crate;relational/lower/binder/column_resolution/canonicalizefolded intoasap-types::pre_asapverbatim (import-path changes only).relational/lowermarked "legacy, pending deletion" at this point — both front ends still built the old shape.7f315e8—QueryExpr<C>andAggIntent<C>made generic over the column-reference state (ColumnIdonce bound,ColumnRefbefore), via a newColStatetrait that handlesScan.schema's shape difference between the two states. Default type parameter keeps every existing consumer's behavior byte-for-byte identical — verified zero changes needed anywhere outsideasap-types. This resolves the open design question both Delete crates/l2 (asap-l2) entirely — the original L2 relational tree + its binder/lower/canonicalize plumbing; front ends emit canonical QueryExpr shapes directly #179 and Eliminate L2Expr/L3Expr as a separate scalar-expression type; fold into QueryExpr, generic over column-reference state #205 flag.a2e7e9c—asap-frontend-promqlbuilds the canonicalL2QueryExpr(QueryExpr<ColumnRef>) directly during its owninterpretstep instead of the oldrelational::QueryExpr. The "local, context-free structural rewriting" Delete crates/l2 (asap-l2) entirely — the original L2 relational tree + its binder/lower/canonicalize plumbing; front ends emit canonical QueryExpr shapes directly #179 describes (heavy-hittertopkrecognition,Window→TimeRangefusion, thePerEntity/Reducereduction decision,Filter-over-Source→Scan.predicatesfolding,AggFunc→AggIntentmapping) now happens at parse time inpromql.rs, using primitives already in scope — no intermediate relational tree. Newresolve.rsreplacesconvert_rootwith a much smaller generic substitution walk: since the input is already canonical-shaped, everyL2QueryExprvariant maps 1:1 to the identicalL3QueryExprvariant — no more per-variant translation, justColumnRef→ColumnIdresolution.49adc0f—asap-frontend-sqlgets the same treatment (passed its full test suite on the first real attempt). With both front ends migrated,relational.rs/lower.rsare deleted entirely, along with the now-dead parts ofbinder.rs/column_resolution.rs, and every stale doc reference across the touched crates is fixed.Verification
Every commit was individually verified with:
cargo build --workspacecargo clippy --workspace --all-targets(zero warnings throughout)cargo test --workspace(fully green at every step — final state: 61 asap-types + 68 SQL + 180 PromQL + everything else, 0 failed)cargo doc --workspace --no-deps(zero new warnings beyond what pre-existed)Two real bugs were caught by the test suite during the PromQL migration (both fixed, both explained in commit
a2e7e9c's message):is_per_series/requires/output_columnneeded to move back toAggIntent's genericimpl<C>block (a front end constructingAggIntent<ColumnRef>directly needsis_per_series()pre-binding), andAggregate.reductionneeded the lenientresolve_group_keys_promqlrather than the strict resolver (PromQL's absent-label semantics, issue #53).Explicitly out of scope
Issue #205 (folding the scalar
Expr<C>tree intoQueryExpr<C>itself, making it one tree instead of an operator tree wrapping a scalar tree) is not done here. This PR's generic-Cfoundation is a prerequisite for it, but #205 also needs its own open question resolved (how to keep it a compile error to nest a relational subtree in a scalar position) — left for a separate PR, per #205's own sequencing note.🤖 Generated with Claude Code