docs: fix stale crate names, wrong TopK shape, and undefined L3/L4 jargon - #211
Merged
Merged
Conversation
…rgon - tools/dag-viewer: `asap-lower` no longer exists; dag_export moved to asap-devtools. Fix the crate name in README.md, RUNNING.md, and generate-sample.sh so the documented commands actually run, and update the stale crates/ir/src/dag_export.rs path to crates/types/. - README.md / docs/parse_and_canonicalize.md: TopK isn't a QueryExpr node, it's an AggIntent measure on Aggregate. Fix the worked examples to show the real shape instead of a node that doesn't exist. - Drop undefined L3/L4 layer jargon from user-guide.md, pre-asap-ir.md, and dag-viewer/README.md in favor of pre-ASAP IR / post-ASAP IR, which are the terms actually defined in this repo's docs. - README.md: add a Building section (cargo build / cargo test --workspace) since none existed; drop the open question about TimeWindow as an explicit node, since TimeRange already answers it; fix a duplicated ASAPPlanner->ASAPPlanner typo in the Immediate TODOs list.
zzylol
approved these changes
Aug 15, 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 was referenced Aug 23, 2026
zzylol
added a commit
that referenced
this pull request
Aug 23, 2026
…, open issue #33 sub-issues (#258) * docs(asap-aware-mapping): flesh out ReplacementStrategy/search design, link issue #33 sub-issues docs/asap_aware_mapping.md already stubbed TargetSubDAG/ReplacementSubDAG/ ReplacementStrategy/CostModel and a Volcano/Cascades-style search pseudocode as "not yet implemented" (since #206/#211). This turns that stub into an actionable design and ties each piece to a tracked sub-issue of #33: - #251: generalize today's single-pick decisions (boundary::implementation_for, cse::share_common_subtrees) into real ReplacementStrategy impls that report every valid candidate instead of collapsing to one. - #252: the Cascades/Volcano-style search engine itself — MEMO-based candidate plan space (not a flat plan list), deduped via pre_asap::cse's existing structural-hash/InternTable machinery, sorted by CostModel. Explicitly reconciled against docs/cse-cost-model-decision.md (#237), which scoped a narrower binary share/don't-share decision away from full search infrastructure — #252 is the multi-axis case that decision itself flagged as the reason a real engine would eventually be needed. - #253: semantic-equivalent rewriting (avg -> sum/count) as a ReplacementStrategy, no longer needing its own bespoke before/after-CSE heuristic once a real search exists to let both forms compete on cost directly. - #254: group-by-lattice roll-up reuse (AHA vs. independent per-subpopulation treatment) as a ReplacementStrategy, gated on agg_is_mergeable and Schema::unique_keys the same way CSE's own legality gate is. - #256: GroupingStrategy::{PerSubpopulationInstance, SharedMultiSubpopulation} (Hydra) as a new axis orthogonal to SketchKind/SamplingKind/..., named for sharing across a query's own subpopulations (not multi-tenant deployment isolation). - #257: rebuild applicability.rs (#247) as a view over the search's candidate space instead of a parallel tree-walking system. Also resolves the doc's stale "(TODO: is this implemented?)" on parameter sizing — boundary::implementation_for's bind_summary_with already sizes every candidate via CostModel::size_params. No code change. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * changes for readability * docs(asap-aware-mapping): plain-English rewrite per Milind's PR #258/#259 comments - MEMO-sharing bullet: spell out why flat plan lists blow up and how Rc-sharing avoids it, without leaning on jargon. - Drop the docs/cse-cost-model-decision.md (#237) history/scope-change framing; keep just the forward-looking design rationale. - Applicability-reporting paragraph: explain MEMO groups concretely instead of the abstract 'candidate-plan space' framing. * Revise ASAP-aware mapping documentation for clarity Updated the documentation for ASAP-aware mapping to improve clarity and structure. Enhanced sections on key concepts, goals, and design principles, and corrected formatting issues. * Enhance glossary and definitions in ASAP-aware mapping Added glossary terms and clarified definitions related to ASAP-aware mapping, including distinctions between alternatives and candidate plans. --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com> Co-authored-by: Milind Srivastava <milindsrivastava1997@gmail.com>
zzylol
added a commit
that referenced
this pull request
Aug 24, 2026
…in the merged search engine ## Why search.rs's own vocabulary (inherited from the design doc's old pseudocode, `plan.bindable_sites()`) used the word "site" throughout -- function names (discover_sites, discover_new_descendant_sites), doc-comment prose, and local variables -- for the exact same concept TargetSubDAG already names: a discovered site is immediately wrapped as `TargetSubDAG::with_consumer_count(&target, consumer_count)` to call a ReplacementStrategy. Same terminology-drift category as the earlier implementation/bind/Logical renames this session -- there shouldn't be two names for one concept. ## What - discover_sites -> discover_targets, discover_new_descendant_sites -> discover_new_descendant_targets (definitions and all call sites). - Local variables in search_workload_with's round loop: the TargetSubDAG-typed local (previously `site`) is now `target`; the Rc<QueryExpr> it wraps (previously also `target`, colliding) is renamed `root` to make room -- matches the root/target naming TargetSubDAG's own fields and this crate's other fixtures already use. `sites_before`/`new_sites` -> `targets_before`/`new_targets`. - Doc-comment prose describing the real implementation (module docs' "Where 'for site in plan.bindable_sites()' comes from" section -> "Where TargetSubDAG discovery comes from", MemoGroup's "one distinct site" -> "one distinct TargetSubDAG", PlanSpace/discovery/dedup/ termination prose throughout replacement.rs, plus the search-engine prose in lib.rs's Status/Terminology and the developer guide that was written earlier in this same session) all now say "target"/`TargetSubDAG` instead of "site". - The one place "site" stays: the module doc's literal quoted pseudocode block (`for site in plan.bindable_sites(): ...`) and one sentence explicitly introducing it as a historical quote -- that block is explicitly framed as what the design doc "has stubbed out since #206/#211", not current API, so it stays verbatim. - Left alone as unrelated: cost_model.rs's two pre-existing "site" uses (a physical "site count" in a list of example cost dimensions, and "a single use site" in cse_recompute_cost's doc) -- both predate this session and mean something else (a deployment's execution site / a CSE consumer location), not TargetSubDAG. ## Output $ cargo build --workspace --all-targets && cargo test --workspace \ && cargo fmt --all -- --check \ && cargo clippy --workspace --all-targets --all-features -- -D warnings (all clean; asap-aware-mapping: 69 passed, 0 failed) 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
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
tools/dag-viewer:asap-lowerno longer exists in the workspace (dag_exportmoved toasap-devtools) — fixed the crate name in README.md, RUNNING.md, and generate-sample.sh so the documented commands actually run, and fixed the stalecrates/ir/src/dag_export.rspath tocrates/types/.TopKisn't aQueryExprnode, it's anAggIntentmeasure onAggregate— fixed the worked examples to show the real shape instead of a node that doesn't exist.L3/L4layer jargon from user-guide.md, pre-asap-ir.md, and dag-viewer/README.md in favor of pre-ASAP IR / post-ASAP IR, the terms this repo's docs actually define.cargo build/cargo test --workspace) since none existed; removed the open question aboutTimeWindowas an explicit node sinceTimeRangealready answers it; fixed a duplicatedASAPPlanner→ASAPPlannertypo in the Immediate TODOs list.Test plan
cargo build— passescargo test --workspace— passes./tools/dag-viewer/generate-sample.sh— now runs end-to-end and writesdag.json(previously failed immediately withpackage(s) 'asap-lower' not found)