refactor: fold L2Expr/L3Expr scalar-expression nodes into QueryExpr (#205) - #214
Merged
Merged
Conversation
…205) Issue #205: eliminate the separate L2Expr/L3Expr scalar-expression type and fold its 13 variants directly into QueryExpr, generic over the same ColState (ColumnRef vs. ColumnId) that already parameterizes the relational nodes. QueryExpr's node set now matches the design doc (docs/pre-asap-ir.md) exactly: scan is a QueryExpr variant, and so is every scalar operator. Design decisions: - The compile-time "scalar vs. relational" distinction that Expr<C> used to buy is dropped, per the design-doc steer: front ends already build only well-shaped trees (a Filter's pred is never a Scan, a Compare's operand is never an Aggregate), so nothing upstream of this merge relied on the type system to enforce it. Every consumer that only handles one half of the merged enum now has an explicit catch-all match arm for the other half — `unreachable!()` with a message naming the violated invariant where the function should structurally never see the other half (e.g. resolve_expr, the binder's column-collection walk, dag_export::build), or a graceful default (`None`, `vec![]`, a no-op) where that already matched the function's existing idiom for "not applicable here" (e.g. a relational-only tree visitor stopping at a scalar leaf). - Boxing: Predicate<C> and Relabel's `value` field name QueryExpr<C> directly (not through a Vec), which is now self-referential once Expr<C>'s variants live inside QueryExpr<C> itself — Rust's recursive-type-size check (E0072) requires heap indirection there, so both became Box<QueryExpr<C>>. ProjectItem.expr and SortKey.expr didn't need it: they're always reached through a Vec, which already provides indirection. - expr_ir.rs keeps ColumnRef, L3Scalar, CompareOp, and ArithOp — the vocabulary scalar expressions are built from — but no longer defines a tree type of its own. Mechanical fallout: because L2QueryExpr and L2/L3Expr's constructors share the same variant names, the front ends' existing `use ... as L2` aliases absorbed almost the entire rename — most call sites needed nothing beyond L2Expr/L3Expr -> L2/QueryExpr. The only recurring fix was wrapping now-boxed Predicate/Relabel arguments in Box::new(...) at their ~15 construction sites across resolve.rs, canonicalize.rs, dag_export.rs, promql.rs, sql/mod.rs, and several test files. Verified: cargo build/clippy(--all-targets)/test/doc --workspace all clean. Full test count unchanged and green (asap-types 61, asap-frontend-promql 180, asap-frontend-sql 68, asap-integration-tests 81, asap-aware-mapping 26, asap-devtools 0+7) — this is a pure type reshape, no lowering/binding/canonicalization behavior changed. The 4 pre-existing `cargo doc` warnings (unresolved/ambiguous intra-doc links in binder.rs, resolve.rs, mod.rs) predate this change, confirmed against HEAD. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
zzylol
force-pushed
the
issue-205-fold-expr-into-queryexpr
branch
from
August 18, 2026 22:37
5912b18 to
46d39ee
Compare
This was referenced Aug 18, 2026
zzylol
added a commit
that referenced
this pull request
Aug 19, 2026
The `pre-asap-ir.md`/`post-asap-ir.md` design docs don't define an L1–L5 layer numbering (#211 explicitly removed it from the docs as undefined jargon, in favor of the terms the docs actually define: "pre-ASAP IR" / "post-ASAP IR"). The source code still had it all over — both as real type names and as prose shorthand — surfaced during #214's review ("what is L3Scalar?" → "does L2 refer to pre-asap-ir?"). This drops it from the code too. Type renames (mechanical, no behavior change): - `L2QueryExpr` -> `UnresolvedQueryExpr` (`QueryExpr<ColumnRef>`, the front-end-emitted, name-based tree) - `L3QueryExpr` -> `ResolvedQueryExpr` (`QueryExpr<ColumnId>`, the positional, resolved tree) - `L4Node` -> `SummaryNode`, `L4Schema` -> `SummarySchema`, `L4DataType` -> `SummaryDataType`, `L4Field` -> `SummaryField` (all four already sat next to `SummaryExpr`/`SummaryKind`/`SummaryParams` in `post_asap` — this makes them consistent with their own module's naming instead of a leftover number) The two front ends' local `L2` construction alias (`UnresolvedQueryExpr as L2`) becomes `Unresolved`, matching the same treatment. A few incidentally-adjacent stale names got the same cleanup while touching their call sites: `df_expr_to_l2` -> `df_expr_to_unresolved`, `scalar_value_to_l3` -> `scalar_value_to_asap`, `arrow_to_l3`/`l3_to_arrow` -> `arrow_to_dtype`/`dtype_to_arrow`, `matcher_to_l3expr` -> `matcher_to_compare`, plus local variables named `l2`/`l3`/`l4` (`tree`, `resolved`, `bound`, `logical_leaf`, …) and two test-local `as L2`/`as L3` aliases (dropped — the bare names already read fine at those call sites). Two real name collisions surfaced while doing this: `sql/mod.rs` and `sql/types.rs` both already import `datafusion::common::ScalarValue` unqualified — aliased that import to `DfScalarValue` in both (same `DfColumn`/`ArrowDataType`-style convention those files already use), rather than picking a different name for the `asap-types` type. The much larger remainder was prose: module docs, doc comments, and `//` explanatory comments across nearly every crate describing a step as "L1 parse", "the L2 tree", "an L4 concern", etc. Replaced with either the descriptive phrase already used elsewhere in this repo ("pre-ASAP" / "post-ASAP" / "canonical" / "unresolved" / "resolved"/ "a deployment's own physical stage" for the L5 placeholder that was never modeled in this crate to begin with) or, where the sentence just meant "the canonical tree", dropped the label entirely. A few doubly-stale spots got fixed at the same time since they were in the exact text being rewritten: `bind.rs`'s/`show_post_asap_ir.rs`'s references to the deleted `asap-ir`/`asap-lower` crate names, and `devtools/src/lib.rs`'s reference to a "shared L2→L3 converter" that issue #179 removed (each front end now calls `resolve_root` directly). Left alone: `boundary.rs`'s "L2-norm"/"L1-norm" (real math, Count- Sketch's error bound), and two literal citations of an external design doc's own section titles ("§6 ... L3 edge", "§'L4 — sketch algebra'") in `schema.rs`/`l4_binding.rs` — that doc's own numbering, not this crate's. Verified: cargo build / clippy(--all-targets --all-features --locked -- -D warnings) / fmt --all -- --check / test(--locked) / doc --workspace --no-deps all clean. Same test counts as before, all green. `cargo doc` warnings are one *fewer* than baseline (fixed the `asap_ir` link along the way) — no new warnings introduced. Pure rename — no behavior change. Co-authored-by: Claude Sonnet 5 <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.
Closes #205.
Stacked on #213 (issue #179) — this branches from that PR's tip, so
the diff here is scoped to just the #205 change. It'll show as a clean
diff against
mainonce #213 merges; until then this PR's base is#213's branch.
What
Eliminates
L2Expr/L3Expras a separate scalar-expression type andfolds its 13 variants (
Column,Literal,Compare,BoolAnd,BoolOr,Not,IsNull,IsNotNull,Cast,InList,FunctionCall,Arith,Case) directly intoQueryExpr<C>, genericover the same
ColStatethat already parameterizes the relationalnodes.
QueryExpr's node set now matchesdocs/pre-asap-ir.mdexactly — scan is a
QueryExprvariant, and so is every scalaroperator.
Design decisions
Dropping the compile-time scalar/relational split.
Expr<C>used to make it impossible to nest a relational node in a scalar
position (or vice versa) at compile time. That guarantee is dropped
here, per the design-doc steer, since nothing upstream actually
relied on the type system to enforce it — front ends already only
ever build well-shaped trees. Every consumer that only handles one
half of the merged enum gets an explicit catch-all arm for the
other half:
unreachable!()naming the violated invariant where afunction should structurally never see the other half (e.g.
resolve_expr, the binder's column-collection walk,dag_export::build), or a graceful default (None,vec![], ano-op) where that already matched the function's existing idiom for
"not applicable here" (relational-only tree visitors stopping at a
scalar leaf).
Boxing.
Predicate<C>'s field andRelabel.valuenameQueryExpr<C>directly (not through aVec), which becomesself-referential once
Expr<C>'s variants live insideQueryExpr<C>itself — Rust's recursive-type-size check (E0072) requires heap
indirection there, so both became
Box<QueryExpr<C>>.ProjectItem.expr/SortKey.exprdidn't need it, since they'realways reached through a
Vec.expr_ir.rskeepsColumnRef,L3Scalar,CompareOp,ArithOp—the vocabulary scalar expressions are built from — but no longer
defines a tree type of its own.
Blast radius
Because
L2QueryExpr/L2Expr/L3Exprshare variant names withQueryExpr, the front ends' existinguse ... as L2aliases absorbedalmost the whole rename — most of the ~225 call sites needed nothing
beyond
L2Expr/L3Expr→L2/QueryExpr. The recurring real fix waswrapping now-boxed
Predicate/Relabelarguments inBox::new(...)at their ~15 construction sites (
resolve.rs,canonicalize.rs,dag_export.rs,promql.rs,sql/mod.rs, and several test files).Verification
cargo build/clippy --all-targets/test/docall cleanacross the workspace. Test counts unchanged and fully green:
asap-types: 61asap-frontend-promql: 180asap-frontend-sql: 68asap-integration-tests: 81asap-aware-mapping: 26This is a pure type reshape — no lowering/binding/canonicalization
behavior changed. The 4 pre-existing
cargo docwarnings (broken/ambiguous intra-doc links in
binder.rs,resolve.rs,mod.rs)predate this change (confirmed against
HEAD), not introduced by it.🤖 Generated with Claude Code