refactor: drop stale L1/L2/L3/L4/L5 layer-numbering jargon - #217
Merged
Merged
Conversation
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>
zzylol
added a commit
that referenced
this pull request
Aug 19, 2026
Issue raised during #217's review: `SummaryDataType::Primitive` was a confusing name (both enum arms are equally "primitive" in the trivial sense of being enum variants), and the underlying `SummaryKind`/ `SummaryParams` pair mixed two axes that don't belong in one type — exact accumulators (Sum/Count/MinMax/Rate/Increase) and approximate sketches (Kll/Cms/Hll/...) were both tagged by the same `SummaryKind`, with `SummaryKind::is_exact()` as the only thing telling them apart at runtime. This splits the vocabulary into five independent families, each with its own `(Kind, Params)` pair: - `ExactKind`/`ExactParams` — the exact, mergeable accumulators. - `SketchKind`/`SketchParams` — the approximate sketches (unchanged set: Kll/Cms/Hll/DDSketch/CmsWithHeap/Kmv/Theta/CountSketch/ CountSketchWithHeap). - `SamplingKind`/`SamplingParams` — new: sampling-based summaries (starts with `Reservoir`). - `WaveletKind`/`WaveletParams` — new: wavelet-transform summaries (starts with `Haar`). - `StatModelKind`/`StatModelParams` — new: fitted statistical/ parametric-model summaries (starts with `Parametric`, deliberately open-ended via a `family: String` the deployment interprets — same "core doesn't enumerate every deployment shape" stance `AggIntent::Extension` already takes). `SummaryDataType` is renamed `SummaryFamilyType` and grows one variant per family (`Plain(DataType)` for an ordinary readable value, plus `ExactAggregate`/`Sketch`/`Sample`/`Wavelet`/`StatModel`, each carrying that family's own `(Kind, Params)`) — so a `Sketch(Kll, ...)` can never be confused for a `Sample(...)` at the type level, and `is_exact()` disappears entirely: exactness is now which arm you're in, not a runtime flag. This is a real architecture change (not a rename) with a genuine trade-off, discussed and confirmed before implementing: `CostModel`'s `rank_candidates`/`size_params` were deliberately generic over any `SummaryKind` so a new technique needed zero new trait methods (`cost_model.rs`'s own module doc said as much). Splitting into 5 top-level families breaks that — those two methods stay scoped to `SketchKind`/`SketchParams` specifically (the only family any core `AggIntent` dispatch actually produces today), and a family other than Sketch gets its own trait methods if/when it needs ranking/sizing of its own, rather than overloading `rank_candidates`/`size_params` across incompatible types. `Implementation` (asap-aware-mapping) mirrors `SummaryFamilyType`'s split so `Sample`/`Wavelet`/`StatModel` are real, constructible outcomes — reachable today only via a deployment's own `CostModel::realize_extension` on an `AggIntent::Extension` node, since no core intent maps to them yet, same status as the exact/sketch split before this change. `bind.rs`'s `SummaryAgg`/`SummaryJoin` now carry a single `family: SummaryFamilyType` field (was two: `summary: SummaryKind, params: SummaryParams`) — the natural consolidation once every family already needs the same "is this exact (no estimate) or does it need a SummaryEstimate readout" handling regardless of which family it is. Verified: cargo build/clippy(--all-targets --all-features --locked -- -D warnings)/fmt --all -- --check/test(--locked)/doc --workspace --no-deps all clean. Test count is 60 in asap-types (was 61 — removed the now-meaningless `is_exact_matches_the_enum_declaration_split` test, since `is_exact()` no longer exists as a runtime check); every other crate's count is unchanged and green. `cargo doc` warnings dropped from 23 to 21 (asap-aware-mapping: 17 -> 15) — no new warnings, and all the new cross-links (SamplingKind, WaveletKind, StatModelKind, ...) resolve cleanly. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
zzylol
added a commit
that referenced
this pull request
Aug 21, 2026
…ry (#218) Issue raised during #217's review: `SummaryDataType::Primitive` was a confusing name (both enum arms are equally "primitive" in the trivial sense of being enum variants), and the underlying `SummaryKind`/ `SummaryParams` pair mixed two axes that don't belong in one type — exact accumulators (Sum/Count/MinMax/Rate/Increase) and approximate sketches (Kll/Cms/Hll/...) were both tagged by the same `SummaryKind`, with `SummaryKind::is_exact()` as the only thing telling them apart at runtime. This splits the vocabulary into five independent families, each with its own `(Kind, Params)` pair: - `ExactKind`/`ExactParams` — the exact, mergeable accumulators. - `SketchKind`/`SketchParams` — the approximate sketches (unchanged set: Kll/Cms/Hll/DDSketch/CmsWithHeap/Kmv/Theta/CountSketch/ CountSketchWithHeap). - `SamplingKind`/`SamplingParams` — new: sampling-based summaries (starts with `Reservoir`). - `WaveletKind`/`WaveletParams` — new: wavelet-transform summaries (starts with `Haar`). - `StatModelKind`/`StatModelParams` — new: fitted statistical/ parametric-model summaries (starts with `Parametric`, deliberately open-ended via a `family: String` the deployment interprets — same "core doesn't enumerate every deployment shape" stance `AggIntent::Extension` already takes). `SummaryDataType` is renamed `SummaryFamilyType` and grows one variant per family (`Plain(DataType)` for an ordinary readable value, plus `ExactAggregate`/`Sketch`/`Sample`/`Wavelet`/`StatModel`, each carrying that family's own `(Kind, Params)`) — so a `Sketch(Kll, ...)` can never be confused for a `Sample(...)` at the type level, and `is_exact()` disappears entirely: exactness is now which arm you're in, not a runtime flag. This is a real architecture change (not a rename) with a genuine trade-off, discussed and confirmed before implementing: `CostModel`'s `rank_candidates`/`size_params` were deliberately generic over any `SummaryKind` so a new technique needed zero new trait methods (`cost_model.rs`'s own module doc said as much). Splitting into 5 top-level families breaks that — those two methods stay scoped to `SketchKind`/`SketchParams` specifically (the only family any core `AggIntent` dispatch actually produces today), and a family other than Sketch gets its own trait methods if/when it needs ranking/sizing of its own, rather than overloading `rank_candidates`/`size_params` across incompatible types. `Implementation` (asap-aware-mapping) mirrors `SummaryFamilyType`'s split so `Sample`/`Wavelet`/`StatModel` are real, constructible outcomes — reachable today only via a deployment's own `CostModel::realize_extension` on an `AggIntent::Extension` node, since no core intent maps to them yet, same status as the exact/sketch split before this change. `bind.rs`'s `SummaryAgg`/`SummaryJoin` now carry a single `family: SummaryFamilyType` field (was two: `summary: SummaryKind, params: SummaryParams`) — the natural consolidation once every family already needs the same "is this exact (no estimate) or does it need a SummaryEstimate readout" handling regardless of which family it is. Verified: cargo build/clippy(--all-targets --all-features --locked -- -D warnings)/fmt --all -- --check/test(--locked)/doc --workspace --no-deps all clean. Test count is 60 in asap-types (was 61 — removed the now-meaningless `is_exact_matches_the_enum_declaration_split` test, since `is_exact()` no longer exists as a runtime check); every other crate's count is unchanged and green. `cargo doc` warnings dropped from 23 to 21 (asap-aware-mapping: 17 -> 15) — no new warnings, and all the new cross-links (SamplingKind, WaveletKind, StatModelKind, ...) resolve cleanly. 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
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.
What
docs/pre-asap-ir.md/docs/post-asap-ir.mddon't define an L1–L5layer 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?" → "doesL2refer topre-asap-ir?"). This drops it from the code too.
Type renames (mechanical, no behavior change)
L2QueryExpr→UnresolvedQueryExpr(QueryExpr<ColumnRef>, thefront-end-emitted, name-based tree)
L3QueryExpr→ResolvedQueryExpr(QueryExpr<ColumnId>, thepositional, resolved tree)
L4Node→SummaryNode,L4Schema→SummarySchema,L4DataType→SummaryDataType,L4Field→SummaryField(allfour already sat next to
SummaryExpr/SummaryKind/SummaryParamsin
post_asap— this makes them consistent with their own module'snaming instead of a leftover number)
The two front ends' local
L2construction alias(
UnresolvedQueryExpr as L2) becomesUnresolved. A few incidentallyadjacent 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 namedl2/l3/l4(
tree,resolved,bound,logical_leaf, …) and two test-localas L2/as L3aliases (dropped — the bare names already read fine atthose call sites).
Two real name collisions surfaced while doing this:
sql/mod.rsandsql/types.rsboth already importdatafusion::common::ScalarValueunqualified — aliased that import to
DfScalarValuein both (matchingthe
DfColumn/ArrowDataType-style convention those files alreadyuse), rather than picking a different name for the
asap-typestype.Prose
The much larger remainder was documentation: module docs, doc
comments, and
//explanatory comments across nearly every cratedescribing 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'sreferences to the deleted
asap-ir/asap-lowercrate names, anddevtools/src/lib.rs's reference to a "shared L2→L3 converter" thatissue #179 removed (each front end now calls
resolve_rootdirectly).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's the other doc's ownnumbering, not this crate's.
Verification
cargo build/clippy --all-targets --all-features --locked -- -D warnings/fmt --all -- --check/test --workspace --locked/doc --workspace --no-depsall clean. Same test counts as before, allgreen.
cargo docwarnings are one fewer than baseline (fixed theasap_irlink along the way) — no new warnings introduced. Purerename — no behavior change.
🤖 Generated with Claude Code