docs: post-asap-ir and asap-aware-mapping - #206
Merged
Merged
Conversation
milindsrivastava1997
marked this pull request as ready for review
August 15, 2026 13:45
Added additional summary properties for modeling.
Clarified the operation of SummaryJoin in ASAP-specific nodes.
zzylol
approved these changes
Aug 15, 2026
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.
No description provided.