Skip to content

feat(asap-aware-mapping): generalize summary/CSE selection into a ReplacementStrategy trait - #259

Merged
zzylol merged 50 commits into
mainfrom
feat/replacement-strategy-251
Aug 25, 2026
Merged

zzylol merged 50 commits into
mainfrom
feat/replacement-strategy-251

Conversation

@zzylol

@zzylol zzylol commented Aug 23, 2026

Copy link
Copy Markdown
Contributor

Closes #251, #252, #257, #266. Part of #33.

Why

The pre-ASAP → post-ASAP binding decision (which sketch, if any, realizes an AggIntent) used to commit to exactly one answer per node, ranked by a cost model — right for executing a query, but it throws away every alternative the cost model didn't pick. Nothing downstream (a future search, a reporting view, a human comparing options) could ever see what else was possible.

We need the opposite shape: every semantically valid alternative, at every relevant spot in the plan, kept — not collapsed early — with real costs attached, so a downstream consumer can make its own tradeoff instead of inheriting one this crate already baked in. That single requirement is what every feature below exists to satisfy, at three different scopes (one target, a whole workload, and a human trying to understand either).

What (features, and why each one)

  1. Core replacement vocabularyTargetSubDAG, Replacement, ReplacementSubDAG, ReplacementStrategy.
    Why: needs a name for "a candidate site" and "a candidate answer" that's exhaustive by contract, so any future optimization rule plugs into the same shape instead of inventing its own one-off return type.

  2. Two concrete strategiesSketchAlgorithmStrategy (every valid summary realization for one AggIntent) and SharedSubtreeStrategy (build-once-and-share vs. build-independently, at a CSE-detected shared subtree).
    Why: the two decisions this crate actually has today — which sketch, and whether to share — need one real implementation each to prove the ReplacementStrategy contract is usable, not just designed on paper.

  3. CostModel::estimate_cost — a new hook returning a real f64 per candidate.
    Why: the existing hooks (rank_candidates, cse_share_decision) only ever produce an order or a decision — enough to rank, not enough for a consumer that wants to show "candidate A ≈ 3.0, candidate B ≈ 1.0" next to each other.

  4. PlanSpace / MemoGroup / RankedGroup / search_workload — the same "generate, never prune" contract extended from one target to a whole workload, without enumerating 2^N fully-copied plans for N independently-choosable sites.
    Why: a caller with one TargetSubDAG already in hand can use ReplacementStrategy directly, but nothing discovered candidate sites across a whole workload, or represented their combined space compactly. Without this, extending "keep every candidate" to workload scale would mean either literally enumerating 2^N copied plans (infeasible past a handful of sites) or quietly dropping candidates to keep it tractable — the same trap that made a premature-commitment bind.rs tempting in the first place.

  5. Sketch type restructuringSketchKind split into family → kind → algorithm (SketchKind::Quantile(SketchAlgorithm, SketchParams), etc.), SketchFamilyStrategySketchAlgorithmStrategy.
    Why: the old flat SketchKind conflated two different questions — "what kind of summary is this" (quantile, cardinality, frequency, top-k) and "which concrete algorithm realizes it" (Kll, DDSketch, Hll, …) — into one enum. That's fine as long as nothing ever needs to reason about one axis without the other, but (4) does: ranking algorithms within an already-fixed kind, or asking "does this target's kind match a required one" regardless of algorithm, both need the two questions answered separately. A flat enum can't express "same kind, different algorithm" as a first-class fact; the nested type can.

  6. explanation.rsexplain_replacements/explain_replacements_with, returning ReplacementExplanation { kind, location, reason, node_hash } for anything in a PlanSpace beyond the trivial realization.
    Why: a PlanSpace is shaped for ranking — raw Rc pointers, every candidate's full construction detail — not for a human or a UI to read. Something has to translate "this MemoGroup has a non-trivial candidate" into a sentence, without re-deriving why that candidate is valid (a strategy already said so, in its rationale) or inventing a second, competing explanation. It isn't answering "is X applicable, yes or no" — every target already has some answer, trivial or not — it's explaining why a non-trivial replacement exists where one does.

  7. dag_export / tools/dag-viewer integration — the devtools export binary now calls explain_replacements and attaches matching explanations to graph nodes by node_hash; the viewer badges and displays them.
    Why: (6) needed a real consumer to prove it's actually useful, not just correctly shaped — this is that consumer, and it's the concrete case that motivated cutting scope in (8) below.

  8. Scope cut: no workload-level materializationbind.rs (the old "keep the first candidate, memoize by Rc identity across a workload" step) is deleted, not kept alongside PlanSpace.
    Why: building (7) surfaced the actual question — does this crate ever need to commit to one final, physically-shared answer? Picking one still requires knowing where it'll be placed, which only a downstream deployment can see. Keeping bind.rs around with no real caller of that specific commitment was speculative scope, not an MVP need.

How (design per component)

Core vocabulary — generate/choose, never mixed. ReplacementStrategy::replacements() must return every valid candidate; a CostModel may only reorder or attach a number, never remove one. This is enforced by convention plus tests (e.g. an exact AccuracyTarget correctly yields exactly one candidate — because a sketch is never valid there, not because a cost model filtered it out). Everything else in this PR is this rule applied at different scopes.

SketchAlgorithmStrategy looks up every summary candidate an AggIntent's own type allows (a static table, not a search — e.g. quantile → {Kll, DDSketch}), sizes each one via CostModel::size_params/rank_candidates, and constructs its SummaryNode directly in the same pass — deciding and constructing were always one step, so there's no separate bridge function between them (an earlier implementation.rs module that was that bridge is merged in here). Child nodes are bound independently via a small internal helper, so a choice at one target never leaks into a nested aggregate's own choice.

SharedSubtreeStrategy has zero cost logic in it — it always returns both the share and recompute-independently candidates for any target with consumer_count >= 2. Whether sharing is worth it is answered later, only by CostModel::cse_share_decision, kept as one comparison rather than a second competing implementation.

PlanSpace is a Cascades/Volcano-style MEMO: one MemoGroup per distinct TargetSubDAG (keyed by Rc pointer identity), never a flat plan list.

// One MemoGroup per distinct TargetSubDAG in the whole workload —
// never a flat list of fully-materialized plans (that's 2^N for N sites).
pub struct MemoGroup {
    pub target: Rc<QueryExpr>,
    pub consumer_count: usize,
    pub candidates: Vec<ReplacementSubDAG>,  // every alternative, unranked
}

pub struct RankedGroup<'a> {
    pub target: &'a Rc<QueryExpr>,
    pub consumer_count: usize,
    pub candidates: Vec<&'a ReplacementSubDAG>,  // same candidates, ranked
    pub costs: Vec<f64>,                         // costs[i] <-> candidates[i]
}

search_workload_with runs the pre-ASAP CSE pass once, discovers every target across every root's whole DAG (a shared node three levels under an unshared Filter is exactly as real a site as a shared root — root-level-only discovery would miss it), then asks every registered strategy to a fixpoint (a documented iteration cap catches a hypothetically ill-behaved future strategy; both shipped strategies converge in one round). PlanSpace::cost_sorted dispatches each group by candidate shape — an all-Rewrite pair goes to cse_share_decision, an all-Summary sketch group goes to rank_candidates, everything also gets estimate_cost — rather than inventing one comparison that tries to cover every shape at once, and returns RankedGroups: same candidate count in as out, just ranked and costed.

explanation.rs does no new decision-making at all: for each MemoGroup with something beyond the trivial candidate, it emits one ReplacementExplanation whose reason is literally the matching candidate's own rationale string. node_hash is structural_hash on the target — the same function dag_export::DagNode::hash already computes for the identical subtree — so a downstream consumer can attach the two by exact match instead of guessing from text.

dag_export stays a strictly lower crate than asap-aware-mapping (asap_types::dag_export::DagNode gained a generic notes: Vec<DagNote> field it never populates itself); the devtools binary, which already depends on both, does the actual correlation after the fact. This keeps the layering invariant (dependencies point one way) intact while still letting a higher layer annotate a lower layer's export.

Architecture

flowchart TB
  classDef gen fill:#2b6f6a,color:#fff,stroke:none
  classDef choose fill:#ad7127,color:#fff,stroke:none
  classDef report fill:#5b6b78,color:#fff,stroke:none
  classDef store stroke-dasharray: 3 3

  TSD["TargetSubDAG"]:::gen --> RS{{"ReplacementStrategy"}}:::gen
  RS --> SAS["SketchAlgorithmStrategy"]:::gen
  RS --> SSS["SharedSubtreeStrategy\n(no cost logic at all)"]:::gen

  CM(["CostModel\nrank_candidates / size_params / estimate_cost / cse_share_decision"]):::choose
  CM -. sizes + orders while generating .-> SAS

  SAS --> CAND["ReplacementSubDAG candidates\n(unranked, nothing pruned)"]
  SSS --> CAND
  CAND --> PS["PlanSpace / MemoGroup\none group per TargetSubDAG, whole workload"]:::store

  PS --> DISPATCH{"cost_sorted\ndispatch by candidate shape"}:::choose
  DISPATCH -- "all Rewrite" --> CM
  DISPATCH -- "all Summary" --> CM
  DISPATCH -- "every candidate" --> CM
  DISPATCH --> RANKED["RankedGroup\nranked + real cost per candidate"]:::choose

  RANKED --> EXP["explanation.rs\nexplain_replacements"]:::report
  EXP --> DAGEXP["dag_export\n(node_hash match)"]:::report
  DAGEXP --> VIEWER(["tools/dag-viewer\nbadge + click-to-explain"]):::report
Loading

Deliberately absent from this diagram: any node that commits to one final, physically-shared answer for a whole workload. That step — bind::implement_workload in an earlier version of this crate — is removed; picking a candidate and placing it is a downstream deployment's call this crate has no visibility into.

Output

A real, captured run (not illustrative numbers) — two independently-built, structurally identical Sum by (job) queries and one Quantile(0.99) by (job) query on a different metric, through search_workload + DefaultCostModel:

--- shared target: consumer_count=2, candidates=3 ---
  [0] cost=3.000  exact Sum accumulator
  [1] cost=1.000  build once and share            <- ranks first
  [2] cost=4.000  build independently

--- p99 target: consumer_count=1, candidates=2 ---
  [0] cost=3.000  Kll sketch
  [1] cost=3.000  DDSketch sketch

explain_replacements:
  CommonSubexpressionReuse @ root "dash_a", root "dash_b"
  SketchApproximation      @ root "p99"

And real dag_export JSON, showing an explain_replacements finding attached to a graph node by hash:

"notes": [
  {
    "kind": "SketchApproximation",
    "reason": "count realizes as a Cms sketch — one of summary_candidates' alternatives for this intent (...); count realizes as a CountSketch sketch — one of summary_candidates' alternatives for this intent (...)"
  }
]

A fuller architecture map + this same worked example, interactive: https://claude.ai/code/artifact/f16b39a4-9d50-4e33-b1dc-54238ae766ae

Known limitation — accuracy is per-target only, not reconciled across the workload

Candidate sizing (accuracy_target/accuracy_budget/default_size_params) always comes from the one AggIntent a TargetSubDAG wraps — correct at single-target granularity. But AggIntent derives PartialEq over every field, including accuracy, so pre-ASAP CSE only ever merges two subtrees onto one shared Rc when their AccuracyTargets match exactly. Two otherwise-identical aggregates that differ only in accuracy (e.g. quantile(0.99, x) at epsilon=0.01 vs. the same query at epsilon=0.05) are simply never recognized as shareable — SharedSubtreeStrategy never even sees them as one target, so this crate never proposes "build once at the tighter bound, serve both." Tracked as future work: #273.

Checks

cargo build --workspace --all-targets, cargo test --workspace, cargo fmt --all -- --check, and cargo clippy --workspace --all-targets --all-features -- -D warnings all pass clean. tools/dag-viewer's test_render.py (12/12) also passes.

🤖 Generated with Claude Code

@milindsrivastava1997

Copy link
Copy Markdown
Collaborator

@zzylol Can you create dev docs for this? This is going to be a critical piece of code since anyone contributing to ASAPPlanner probably needs to understand the data structures and traits here.

@milindsrivastava1997 milindsrivastava1997 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zzylol Can you create dev docs for this? This is going to be a critical piece of code since anyone contributing to ASAPPlanner probably needs to understand the data structures and traits here.

zzylol added a commit that referenced this pull request Aug 23, 2026
…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.
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 23, 2026
Milind's review comment on #259: this is going to be critical code for
anyone contributing to ASAPPlanner, and needs docs beyond the module's
own rustdoc (which already covers what each type is and why).

docs/replacement-strategy-dev-guide.md is the narrower companion: how a
contributor actually writes a new ReplacementStrategy -- the contract's
non-obvious rules (exhaustive not ranked, don't panic on non-match, wrap
existing decision logic instead of inventing new), a full worked
walkthrough of SharedSubtreeStrategy, a testing checklist, and explicit
non-goals. Aimed at whoever picks up #253/#254/#256/#257 next.
@zzylol

zzylol commented Aug 23, 2026

Copy link
Copy Markdown
Contributor Author

@milindsrivastava1997 Added a dev guide: docs/ASAP-aware-mapping-developer-guide.md

Covers how to actually extend this code: the ReplacementStrategy/CostModel/Matcher contracts, worked examples (SketchFamilyStrategy, SharedSubtreeStrategy, ForceSketchKind), common mistakes, a testing checklist, and an extension-point lookup table.

🤖 Generated with Claude Code

Comment thread crates/asap-aware-mapping/src/replacement.rs Outdated

@milindsrivastava1997 milindsrivastava1997 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

High-level comments.

  • Remove section 19. "If there is one rule to keep in mind when extending this layer" is applicable for human attention, not agent(?)
  • Sec 16-18 is awesome and very helpful. We should do this for other efforts too.
  • Glossary would help. Idk what boundary and binder/binding mean.

See more detailed comments inline

Comment thread docs/developer_docs/ASAP-aware-mapping-developer-guide.md Outdated
Comment thread docs/developer_docs/ASAP-aware-mapping-developer-guide.md Outdated
Comment thread docs/developer_docs/ASAP-aware-mapping-developer-guide.md Outdated
Comment thread docs/developer_docs/ASAP-aware-mapping-developer-guide.md Outdated
Comment thread docs/developer_docs/ASAP-aware-mapping-developer-guide.md Outdated
Comment thread docs/developer_docs/ASAP-aware-mapping-developer-guide.md Outdated
Comment thread docs/developer_docs/ASAP-aware-mapping-developer-guide.md Outdated
Comment thread docs/developer_docs/ASAP-aware-mapping-developer-guide.md Outdated
Comment thread docs/developer_docs/ASAP-aware-mapping-developer-guide.md
Comment thread docs/developer_docs/ASAP-aware-mapping-developer-guide.md Outdated
zzylol added a commit that referenced this pull request Aug 23, 2026
…uide/ (#265)

design_docs/ (conceptual/design rationale, not how-to):
  asap_aware_mapping.md, cse-cost-model-decision.md,
  parse_and_canonicalize.md, post-asap-ir.md, pre-asap-ir.md
user-guide/:
  user-guide.md
developer_docs/ is created by PR #259 (ASAP-aware-mapping-developer-guide.md
lands there directly; not on main yet).

Updated every cross-reference to a moved path: README.md's doc links,
docs/user-guide/user-guide.md's internal references, and the rustdoc
comments in bind.rs/cost_model.rs/cse.rs that cite
docs/cse-cost-model-decision.md or docs/pre-asap-ir.md. cargo check
--workspace passes clean (comment-only changes in Rust source).
@zzylol
zzylol force-pushed the feat/replacement-strategy-251 branch from 7ed5345 to 0c0d0ff Compare August 23, 2026 19:40
zzylol and others added 9 commits August 23, 2026 13:58
…trategy (#251)

Implements the `TargetSubDAG`/`ReplacementSubDAG`/`ReplacementStrategy`
vocabulary `docs/asap_aware_mapping.md` stubs out under "Key concepts
(not yet implemented)", scoped to the decision this crate already owns
(summary-family/kind selection and CSE-sharing), not a new invention.

`boundary::implementation_for_with` and `bind::implement_tree_with`
each commit to exactly one answer per decision point, ranked via a
CostModel — right for binding a query, but it throws away every
alternative a cost model didn't pick. A future search/optimization
engine needs the opposite shape: every semantically valid alternative,
so a later cost-based search can explore and compare them. This adds
that shape alongside the existing single-pick functions, wrapping
rather than replacing them:

- `TargetSubDAG` — a reference (by `Rc<QueryExpr>`, not a bare
  `&QueryExpr`) to a pre-ASAP node that's a replacement candidate, plus
  how many workload locations already reference it (`consumer_count`).
- `ReplacementSubDAG` — one candidate replacement: a `Replacement`
  (`Summary(Rc<SummaryNode>)` for a binding decision, or
  `Rewrite(Rc<QueryExpr>)` for a pre-ASAP structural alternative) plus
  a human-readable rationale.
- `ReplacementStrategy` — `matches`/`replacements`, exhaustive and not
  ranked or filtered; picking the best candidate stays a CostModel's
  job.

Two real strategies, ported from existing decision logic with no new
decision procedure:

- `SketchFamilyStrategy` wraps `boundary::implementation_for_with`/
  `summary_candidates`'s exhaustive match: for a bindable `Aggregate`
  (the same single-intent, no-`HAVING` shape `bind::implement_tree_with`
  requires), returns every candidate summary realization — every
  `SketchKind` `summary_candidates` lists when the boundary decision is
  approximate, built via a `ForceSketchKind` `CostModel` adapter that
  steers `implement_tree_with`'s own decision procedure towards each
  candidate in turn rather than reimplementing any part of it; the
  single exact-accumulator/pass-through outcome when that's the only
  option.
- `SharedSubtreeStrategy` wraps `share_common_subtrees`'s sharing
  decision as an explicit build-independently-vs-build-once-and-share
  candidate pair, wherever a `TargetSubDAG` already has two or more
  consumers.

`boundary.rs`/`bind.rs`/`applicability.rs`'s existing single-pick
public behavior is unchanged; this only adds the exhaustive-candidate
view alongside it. No search/selection logic is added — that's a
separate Cascades/Volcano-style engine, tracked in #33's other
sub-issues, built on top of this trait.

`cargo build --workspace --all-targets`, `cargo test --workspace`,
`cargo fmt --all -- --check`, and
`cargo clippy --workspace --all-targets --all-features -- -D warnings`
all pass clean.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Milind's review comment on #259: this is going to be critical code for
anyone contributing to ASAPPlanner, and needs docs beyond the module's
own rustdoc (which already covers what each type is and why).

docs/replacement-strategy-dev-guide.md is the narrower companion: how a
contributor actually writes a new ReplacementStrategy -- the contract's
non-obvious rules (exhaustive not ranked, don't panic on non-match, wrap
existing decision logic instead of inventing new), a full worked
walkthrough of SharedSubtreeStrategy, a testing checklist, and explicit
non-goals. Aimed at whoever picks up #253/#254/#256/#257 next.
…tcher

Rename replacement-strategy-dev-guide.md -> extension-points-dev-guide.md.
CostModel shares ReplacementStrategy's extension-point shape (small trait,
sensible defaults, override just what you need) and deserved the same
worked-example/contract treatment: the rank_candidates/size_params/
realize_extension+readout_extension pairing, the cse_share_decision default
composing the two cost hooks (delegate to those, don't reimplement the
comparison), and the ForceSketchKind delegation pattern for adapting an
existing CostModel. Matcher gets a short pointer -- unlike the other two it
ships with no default body and no implementation in this crate at all.
…rewrite

Rewritten as a fuller how-to: mental model, glossary, worked examples for
both SketchFamilyStrategy and SharedSubtreeStrategy, a strategy contract
with numbered rules, a per-hook CostModel reference, common mistakes,
an extension checklist, and an extension-point lookup table.

Verified against current code (ForceSketchKind body, SketchFamilyStrategy
dispatch, CostModel hook signatures, CseCandidate/ShareDecision, test
names) -- no discrepancies found. On top of the original draft: normalized
heading levels (was mixing H1/H2 for sections 4-18), defined 'readout'
where it first appears, marked illustrative (My*/Prefer*) vs. real code,
linked docs/asap_aware_mapping.md, and added a Matcher section (confirmed
via grep: no impl Matcher for anywhere in the crate, so it ships with
neither a default body nor an implementation, unlike CostModel).
…the glossary

Was a bare signature dump with only readout_extension explained. Each of
the 7 hooks now gets a one-line definition plus its default behavior (or
'no default' for rank_candidates, the one hook every CostModel must
implement) right next to its signature, so the glossary entry is useful
on its own without jumping to SS9's fuller per-hook reference.
Companion to docs/reorganize-into-folders (main) and
docs/replacement-strategy-search-design (#258), which move the design
docs into docs/design_docs/ and docs/user-guide.md into docs/user-guide/.
All three branches converge on the same final docs/ layout.

Updated the link to the design doc to point at its eventual
docs/design_docs/asap_aware_mapping.md location -- note this link is only
valid once this branch merges after (or alongside) the reorg; on this
branch alone, asap_aware_mapping.md is still at its old top-level docs/
path, since moving it is #258's change, not this one's.
… guide

Ten review comments, addressed with clarifying edits (two -- the cost-struct
suggestion and the "boundary is a weird name" rename -- are real design/code
decisions out of scope for a docs pass, noted rather than silently acted on):

- CostModel::size_params: explain why sizing counts as a cost decision, not
  just configuration -- same "real-world cost knowledge this crate can't
  hardcode" reasoning as ranking.
- realize_extension: define AggIntent::Extension (issue #131's deployment
  escape hatch) with a concrete ext_kind/payload example.
- cse_recompute_cost: note the f64-vs-struct question honestly, point at it
  as a separate future issue rather than deciding here.
- Sec 3 "How the current pieces fit together": state explicitly which path
  is production (binding) vs. additive (replacement-strategy), and connect
  ReplacementSubDAG/TargetSubDAG to the design doc's alternatives/candidates
  language.
- Sec 4.1 Guideline: disambiguate "applicability" (plain English) from
  applicability.rs's formal ApplicabilityRule/ApplicabilityFinding concept.
- ForceSketchKind: add the missing "why" paragraph before the code block --
  what problem forcing rank_candidates actually solves.
- SharedSubtreeStrategy's two alternatives: explain why the strategy must
  enumerate both even though CSE detection already makes "share" free to
  construct and CostModel::cse_share_decision already has an answer for the
  production binding path -- neither fact licenses skipping enumeration.
- Matcher: add a concrete "existing DDSketch answering a Kll request"
  example -- what question it answers and why this crate can't answer it
  itself.
…rence count, not a runtime one

Addresses the comment on TargetSubDAG.consumer_count -- 'refer or consume?
can you give an example' -- with a worked example: two queries sharing a
CSE'd rate() subtree, not execution counting.
A selective per-file checkout of tools/dag-viewer's main-side content
(previous commit) fixed the semantic conflict but not the git-level one:
tools/dag-viewer/viewer.js does not exist at the merge-base, so both this
branch and main independently "add" it with different content, which
git's merge algorithm flags as an add/add conflict regardless of how
compatible the two versions actually are — confirmed both by a real
'git merge --no-commit origin/main' locally and by PR #259 continuing to
report mergeable: CONFLICTING after the previous commit was pushed.

Resolving requires an actual merge commit (not a rebase/cherry-pick of
this branch's own history, which remains off the table): this commit
merges origin/main in, keeping this branch's already-correct viewer.js
(main's content plus the ported notes UI) for the one real conflict.
tools/dag-viewer/README.md and index.html auto-merged byte-identical to
what the previous commit had already produced by hand, confirming that
reconstruction was accurate.

The merge also pulls in main's unrelated Rust-side history this branch
never touched (crates/frontend-sql, crates/sql-function-catalog,
crates/types/src/pre_asap/query_expr.rs) — a plain fast-forward on those
paths with nothing from this branch to reconcile against.

Verified post-merge: cargo build/test/fmt/clippy all clean; python3 -m
unittest test_render passes (12/12).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
zzylol and others added 18 commits August 24, 2026 13:49
…layer references

The design doc's old L1-L4 layer numbering ("§L4 — sketch algebra")
this crate's tests referenced no longer exists there — it was
reorganized into named sections (Core Abstractions, Candidate Plan
Search, ...) a while back, leaving these two file names and their doc
comments pointing at a section heading that's been gone for some time.

- l4_binding.rs -> promql_to_post_asap.rs
- l4_binding_sql.rs -> sql_to_post_asap.rs
- local l3 variables -> pre_asap (same L-numbering leftover)
- local fn bind() -> fn realize() (this crate retired "bind" as a
  name earlier this round; the test helper missed that pass since it's
  crate-local, not part of the public API)
- fixed cross-references between the two files' doc comments to use
  the new names, and two stale mentions of already-renamed items
  (bind_summary_agg -> construct_summary, bind.rs -> replacement.rs,
  implementation::summary_candidates -> replacement::summary_candidates)

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The §3 diagram started at TargetSubDAG as if it were the crate's
top-level input. It never appears out of nowhere: it's either
constructed directly for one query already in hand (TargetSubDAG::new,
consumer_count assumed 1 — the single-query API and realize_child's
own recursion), or discovered by search_workload_with walking a whole
workload's DAGs, which is the only path that ever produces a real,
non-assumed consumer_count. That distinction matters concretely:
SharedSubtreeStrategy only meaningfully matches something reached
through the second path.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Restructure the flat 19-section developer guide into the three-part
structure the doc owner asked for: Part 1 - Code Architecture, Part 2
- Interfaces and Definitions, Part 3 - How to Add X, Y, Z (each ending
in how to verify). Content is moved, not rewritten:

Part 1 (Mental model first, per doc-owner follow-up, then a new
whole-PR architecture diagram, then "How the current pieces fit
together"):
- old #1 Mental model -> Part 1 #1
- new: whole-PR architecture diagram (TargetSubDAG's two entry points
  through ReplacementStrategy, PlanSpace/cost_sorted, explanation.rs,
  to a downstream consumer) -> Part 1 #2
- old #3 How the current pieces fit together -> Part 1 #3

Part 2:
- old Terminology's "Implementation" definition merged into the
  Glossary as one more entry (### Implementation), next to
  ReplacementStrategy
- old #2 Glossary -> Part 2 #1 (plus the merged Implementation entry
  and old #10 Matcher, retitled to match glossary-entry style)
- old #10 Matcher (implementation.rs) -> ### Matcher inside the
  Glossary; implementation.rs no longer exists, so the stale title
  is fixed
- old #19's definitional content (ReplacementExplanation/
  ExplanationKind shapes, node_hash, why there's no ExplanationRule
  trait, location-text ownership) -> Part 2 #2

Part 3:
- old #4, #5, #6, #7, #13, #14 -> Part 3 #1, Adding a new
  ReplacementStrategy (ending in Testing a new strategy)
- old #8, #9, #15 -> Part 3 #2, Adding or customizing a CostModel
  (ending in Testing a new cost model)
- old #12 -> Part 3 #3, Adding a new sketch algorithm, with its
  stale implementation.rs/binder references fixed to replacement.rs/
  construct_summary vocabulary, plus a new "Verifying a new sketch
  algorithm" close grounded in the existing coverage-matrix tests
- old #11, #16, #17, #18 -> Part 3 #4-#7 (capstone + closing
  reference material); #18's extension-map table's implementation.rs
  row fixed to replacement.rs
- old #19's "Using it"/"Adding a new kind" content -> Part 3 #8,
  Using and extending explanation.rs

cargo build --workspace --all-targets is clean (docs-only change).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…rt reorg

The reorganization into Part 1/2/3 moved every original section but
deliberately left un-enumerated internal cross-references (§8, §11,
§18, §19, #4-adding-..., etc.) untouched per its own 'reorganize, don't
rewrite' scope. Sweep those now that the structure is settled: every
old flat-numbered '§N' reference becomes 'Part X §Y' pointing at the
section's actual new location, one genuinely self-referential '(§3
above)' is dropped (the text is already inside that section), and the
one remaining 'bind.rs' source-file mention in the intro (bind.rs was
deleted this branch) is corrected to explanation.rs.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@zzylol
zzylol merged commit 0749485 into main Aug 25, 2026
3 checks passed
zzylol added a commit that referenced this pull request Aug 25, 2026
…rrent tip

This branch had been forked at the very first draft of #251
(boundary::implementation_for_with, a ForceSketchKind CostModel-wrapping
hack to steer bind::implement_tree_with) and never rebased — missing
the "make binding literally a selector over ReplacementStrategy"
refactor, and the deletion of bind::implement_tree/implement_tree_with.
It would not compile against the current tip of
feat/replacement-strategy-251. Its own copy of replacement.rs/lib.rs was
just a frozen snapshot of that first draft, reintroducing already-fixed
stale code.

Rebuilt fresh: reset this branch onto the current #259 tip (which
already carries the current, correct replacement.rs/bind.rs/lib.rs/
implementation.rs/cost_model.rs), then reapplied only the genuinely new
content this branch adds on top of it — rollup.rs (issue #254, part of

- rollup.rs itself needed no code changes at all: it never called
  bind::implement_tree/implement_tree_with or referenced boundary::
  directly — RollupStrategy/is_legal_rollup_source/build_rollup are
  self-contained around crate::replacement's TargetSubDAG/
  ReplacementSubDAG/Replacement/ReplacementStrategy vocabulary, which is
  unchanged in shape on the current #251 tip. Copied over verbatim.
- lib.rs: added `pub mod rollup;` and a module-doc bullet under
  `## Status` describing RollupStrategy/is_legal_rollup_source (the old
  branch's own lib.rs diff added no such bullet and no public
  re-exports for rollup, so none were reintroduced here either — the
  module's own `pub struct`/`pub fn` items are reachable via
  `asap_aware_mapping::rollup::*` same as before).
- Did not touch replacement.rs/bind.rs/cost_model.rs/implementation.rs:
  they already reflect #259's current, correct state.

Verified: cargo build --workspace --all-targets, cargo test --workspace
(0 failures, including all 16 rollup:: tests unchanged), cargo fmt --all
-- --check, cargo clippy --workspace --all-targets --all-features -- -D
warnings — all clean.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
zzylol added a commit that referenced this pull request Aug 25, 2026
…urrent tip

This branch had been forked at the very first draft of #251
(boundary::implementation_for_with, before the boundary.rs ->
implementation.rs rename, the "make binding literally a selector over
ReplacementStrategy" refactor, and this session's deletion of
bind::implement_tree/implement_tree_with) and never rebased — its own
frozen copy of replacement.rs/lib.rs would not compile against the
current tip of feat/replacement-strategy-251.

Rebuilt fresh: reset this branch onto the current #259 tip, then
reapplied only the genuinely new content this PR adds on top —
replacement.rs/bind.rs/implementation.rs now come from #259 itself,
unmodified.

- rewrite.rs (AvgToSumOverCountStrategy, issue #253, part of #33) had
  almost no stale-API surface to begin with: it's a pure pre-ASAP ->
  pre-ASAP QueryExpr rewrite (avg -> sum/count) that only touches
  crate::replacement's stable, unchanged vocabulary
  (Replacement/ReplacementStrategy/ReplacementSubDAG/TargetSubDAG) and
  never called implement_tree_with or touched SketchKind/Implementation
  construction. Reapplied verbatim except for three stale prose spots:
  two `boundary::implementation_for_with` doc/rationale references
  updated to `implementation::implementations_for_with` (the module
  rename), and one `docs/asap_aware_mapping.md` path reference
  corrected to `docs/design_docs/asap_aware_mapping.md` (the design
  doc's actual location on this tip). None of these were load-bearing
  for compilation.
- lib.rs: added `pub mod rewrite;`, a `## Status` doc bullet for the
  new module (mirroring the existing bullets' style; the original PR's
  own lib.rs diff never added one), and `pub use
  rewrite::AvgToSumOverCountStrategy` (also missing from the original
  PR's diff) so the strategy is actually reachable from outside the
  crate the same way its SketchFamilyStrategy/SharedSubtreeStrategy
  siblings are.

Verified: cargo build --workspace --all-targets, cargo test
--workspace (0 failures, including all 12 rewrite:: tests), cargo fmt
--all -- --check, cargo clippy --workspace --all-targets --all-features
-- -D warnings — all clean.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
zzylol added a commit that referenced this pull request Aug 25, 2026
's current tip

This branch had been forked at the very first draft of #251
(boundary::implementation_for_with, a ForceSketchKind CostModel-wrapping
hack to steer bind::implement_tree_with) and never rebased — completely
missing #259's subsequent history: the boundary.rs -> implementation.rs
rename, the "make binding literally a selector over ReplacementStrategy"
refactor, and this session's deletion of
bind::implement_tree/implement_tree_with. It would not compile against
the current tip of feat/replacement-strategy-251.

Rebuilt fresh: reset this branch onto the current #259 tip, then
reapplied only the genuinely new content this PR adds on top — nothing
here re-implements replacement.rs/bind.rs/cost_model.rs, which now come
from #259 itself.

- types crate (pure additions, brought over wholesale): `GroupingStrategy`
  gains a `grouping: GroupingStrategy` field on `SummaryExpr::SummaryAgg`
  (default `PerSubpopulationInstance`, so no existing behavior changes),
  plus the new `HydraKind`/`HydraParams` types and
  `hydra_kind_for`/`default_hydra_params` functions in
  `post_asap::sketch`, with module-doc cross-references updated for the
  current module layout.

- grouping.rs (asap-aware-mapping): `HydraGroupingStrategy`, a
  `ReplacementStrategy` offering `GroupingStrategy::SharedMultiSubpopulation`
  as an additional candidate wherever the target has a real subpopulation
  concept (non-empty `by`) and its summary family has a modeled Hydra
  variant (KLL only, today). Its own legality checks, `ReplacementStrategy`
  impl shape, and tests needed only mechanical adaptation.

- **`ForceSketchKind` was NOT resurrected.** The original draft (written
  against the first #251 draft) reused `replacement.rs`'s
  `ForceSketchKind` — a whole-recursive-bind `CostModel`-wrapping adapter
  that steers `implement_tree_with`'s decision procedure toward a forced
  `SketchKind`. That pattern was deliberately deleted from this crate as
  an anti-pattern (a real bug where the forced choice could leak into a
  target's own nested aggregates — see `replacement.rs`'s own module
  docs), and `bind::implement_tree`/`implement_tree_with` no longer exist.
  `grouping.rs` now calls `implementation::implementations_for_with(intent,
  cost_model)` to get every ranked candidate directly (already the full
  list, no forcing needed), finds the one `Implementation::Sketch { kind,
  .. }` matching the Hydra-eligible kind, and passes that exact,
  already-decided `Implementation` straight to
  `bind::bind_with_implementation` — the same first-class,
  one-candidate-at-a-time primitive `SketchFamilyStrategy` itself already
  uses. No adapter, no steering, no risk of a forced choice leaking into
  nested aggregates.

- `replacement.rs`: `describe_intent` bumped to `pub(crate)` so
  `grouping::HydraGroupingStrategy` can reuse it for rationale strings
  (`bindable_intent` needed no bump — it now lives in `bind.rs`, already
  `pub`, since #259's "unify binding and replacement-strategy paths"
  refactor moved it there independently of this branch).

- `bind.rs`/`cost_model.rs`: added the new `grouping` field to every
  `SummaryAgg` struct literal in their own test fixtures (mechanical,
  `GroupingStrategy::default()`), plus the same in
  `crates/integration-tests/tests/l4_binding.rs`/`l4_binding_sql.rs`'s
  destructuring patterns (`..`, since those tests don't assert on
  grouping).

Verified: cargo build --workspace --all-targets, cargo test --workspace
(0 failures, including all 14 grouping:: tests — the
implementations_for_with + bind_with_implementation rewrite produces the
same bound SketchKind/params the old ForceSketchKind-based code was
designed to produce), cargo fmt --all -- --check, cargo clippy
--workspace --all-targets --all-features -- -D warnings — all clean.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
zzylol added a commit that referenced this pull request Aug 25, 2026
…rrent tip

This branch had been forked at the very first draft of #251
(boundary::implementation_for_with, a ForceSketchKind CostModel-wrapping
hack to steer bind::implement_tree_with) and never rebased — missing
the "make binding literally a selector over ReplacementStrategy"
refactor, and the deletion of bind::implement_tree/implement_tree_with.
It would not compile against the current tip of
feat/replacement-strategy-251. Its own copy of replacement.rs/lib.rs was
just a frozen snapshot of that first draft, reintroducing already-fixed
stale code.

Rebuilt fresh: reset this branch onto the current #259 tip (which
already carries the current, correct replacement.rs/bind.rs/lib.rs/
implementation.rs/cost_model.rs), then reapplied only the genuinely new
content this branch adds on top of it — rollup.rs (issue #254, part of

- rollup.rs itself needed no code changes at all: it never called
  bind::implement_tree/implement_tree_with or referenced boundary::
  directly — RollupStrategy/is_legal_rollup_source/build_rollup are
  self-contained around crate::replacement's TargetSubDAG/
  ReplacementSubDAG/Replacement/ReplacementStrategy vocabulary, which is
  unchanged in shape on the current #251 tip. Copied over verbatim.
- lib.rs: added `pub mod rollup;` and a module-doc bullet under
  `## Status` describing RollupStrategy/is_legal_rollup_source (the old
  branch's own lib.rs diff added no such bullet and no public
  re-exports for rollup, so none were reintroduced here either — the
  module's own `pub struct`/`pub fn` items are reachable via
  `asap_aware_mapping::rollup::*` same as before).
- Did not touch replacement.rs/bind.rs/cost_model.rs/implementation.rs:
  they already reflect #259's current, correct state.

Verified: cargo build --workspace --all-targets, cargo test --workspace
(0 failures, including all 16 rollup:: tests unchanged), cargo fmt --all
-- --check, cargo clippy --workspace --all-targets --all-features -- -D
warnings — all clean.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
zzylol added a commit that referenced this pull request Aug 25, 2026
's current tip

This branch had been forked at the very first draft of #251
(boundary::implementation_for_with, a ForceSketchKind CostModel-wrapping
hack to steer bind::implement_tree_with) and never rebased — completely
missing #259's subsequent history: the boundary.rs -> implementation.rs
rename, the "make binding literally a selector over ReplacementStrategy"
refactor, and this session's deletion of
bind::implement_tree/implement_tree_with. It would not compile against
the current tip of feat/replacement-strategy-251.

Rebuilt fresh: reset this branch onto the current #259 tip, then
reapplied only the genuinely new content this PR adds on top — nothing
here re-implements replacement.rs/bind.rs/cost_model.rs, which now come
from #259 itself.

- types crate (pure additions, brought over wholesale): `GroupingStrategy`
  gains a `grouping: GroupingStrategy` field on `SummaryExpr::SummaryAgg`
  (default `PerSubpopulationInstance`, so no existing behavior changes),
  plus the new `HydraKind`/`HydraParams` types and
  `hydra_kind_for`/`default_hydra_params` functions in
  `post_asap::sketch`, with module-doc cross-references updated for the
  current module layout.

- grouping.rs (asap-aware-mapping): `HydraGroupingStrategy`, a
  `ReplacementStrategy` offering `GroupingStrategy::SharedMultiSubpopulation`
  as an additional candidate wherever the target has a real subpopulation
  concept (non-empty `by`) and its summary family has a modeled Hydra
  variant (KLL only, today). Its own legality checks, `ReplacementStrategy`
  impl shape, and tests needed only mechanical adaptation.

- **`ForceSketchKind` was NOT resurrected.** The original draft (written
  against the first #251 draft) reused `replacement.rs`'s
  `ForceSketchKind` — a whole-recursive-bind `CostModel`-wrapping adapter
  that steers `implement_tree_with`'s decision procedure toward a forced
  `SketchKind`. That pattern was deliberately deleted from this crate as
  an anti-pattern (a real bug where the forced choice could leak into a
  target's own nested aggregates — see `replacement.rs`'s own module
  docs), and `bind::implement_tree`/`implement_tree_with` no longer exist.
  `grouping.rs` now calls `implementation::implementations_for_with(intent,
  cost_model)` to get every ranked candidate directly (already the full
  list, no forcing needed), finds the one `Implementation::Sketch { kind,
  .. }` matching the Hydra-eligible kind, and passes that exact,
  already-decided `Implementation` straight to
  `bind::bind_with_implementation` — the same first-class,
  one-candidate-at-a-time primitive `SketchFamilyStrategy` itself already
  uses. No adapter, no steering, no risk of a forced choice leaking into
  nested aggregates.

- `replacement.rs`: `describe_intent` bumped to `pub(crate)` so
  `grouping::HydraGroupingStrategy` can reuse it for rationale strings
  (`bindable_intent` needed no bump — it now lives in `bind.rs`, already
  `pub`, since #259's "unify binding and replacement-strategy paths"
  refactor moved it there independently of this branch).

- `bind.rs`/`cost_model.rs`: added the new `grouping` field to every
  `SummaryAgg` struct literal in their own test fixtures (mechanical,
  `GroupingStrategy::default()`), plus the same in
  `crates/integration-tests/tests/l4_binding.rs`/`l4_binding_sql.rs`'s
  destructuring patterns (`..`, since those tests don't assert on
  grouping).

Verified: cargo build --workspace --all-targets, cargo test --workspace
(0 failures, including all 14 grouping:: tests — the
implementations_for_with + bind_with_implementation rewrite produces the
same bound SketchKind/params the old ForceSketchKind-based code was
designed to produce), cargo fmt --all -- --check, cargo clippy
--workspace --all-targets --all-features -- -D warnings — all clean.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
zzylol added a commit that referenced this pull request Aug 25, 2026
…262)

* feat(asap-aware-mapping): rebuild rollup.rs fresh on top of #251's current tip

This branch had been forked at the very first draft of #251
(boundary::implementation_for_with, a ForceSketchKind CostModel-wrapping
hack to steer bind::implement_tree_with) and never rebased — missing
the "make binding literally a selector over ReplacementStrategy"
refactor, and the deletion of bind::implement_tree/implement_tree_with.
It would not compile against the current tip of
feat/replacement-strategy-251. Its own copy of replacement.rs/lib.rs was
just a frozen snapshot of that first draft, reintroducing already-fixed
stale code.

Rebuilt fresh: reset this branch onto the current #259 tip (which
already carries the current, correct replacement.rs/bind.rs/lib.rs/
implementation.rs/cost_model.rs), then reapplied only the genuinely new
content this branch adds on top of it — rollup.rs (issue #254, part of

- rollup.rs itself needed no code changes at all: it never called
  bind::implement_tree/implement_tree_with or referenced boundary::
  directly — RollupStrategy/is_legal_rollup_source/build_rollup are
  self-contained around crate::replacement's TargetSubDAG/
  ReplacementSubDAG/Replacement/ReplacementStrategy vocabulary, which is
  unchanged in shape on the current #251 tip. Copied over verbatim.
- lib.rs: added `pub mod rollup;` and a module-doc bullet under
  `## Status` describing RollupStrategy/is_legal_rollup_source (the old
  branch's own lib.rs diff added no such bullet and no public
  re-exports for rollup, so none were reintroduced here either — the
  module's own `pub struct`/`pub fn` items are reachable via
  `asap_aware_mapping::rollup::*` same as before).
- Did not touch replacement.rs/bind.rs/cost_model.rs/implementation.rs:
  they already reflect #259's current, correct state.

Verified: cargo build --workspace --all-targets, cargo test --workspace
(0 failures, including all 16 rollup:: tests unchanged), cargo fmt --all
-- --check, cargo clippy --workspace --all-targets --all-features -- -D
warnings — all clean.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

* fix: preserve rollup output names

* style: format merged module declarations

* fix(rollup): integrate safe workload detection

* docs(mapping): refresh rollup integration status

---------

Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
zzylol added a commit that referenced this pull request Aug 25, 2026
's current tip

This branch had been forked at the very first draft of #251
(boundary::implementation_for_with, a ForceSketchKind CostModel-wrapping
hack to steer bind::implement_tree_with) and never rebased — completely
missing #259's subsequent history: the boundary.rs -> implementation.rs
rename, the "make binding literally a selector over ReplacementStrategy"
refactor, and this session's deletion of
bind::implement_tree/implement_tree_with. It would not compile against
the current tip of feat/replacement-strategy-251.

Rebuilt fresh: reset this branch onto the current #259 tip, then
reapplied only the genuinely new content this PR adds on top — nothing
here re-implements replacement.rs/bind.rs/cost_model.rs, which now come
from #259 itself.

- types crate (pure additions, brought over wholesale): `GroupingStrategy`
  gains a `grouping: GroupingStrategy` field on `SummaryExpr::SummaryAgg`
  (default `PerSubpopulationInstance`, so no existing behavior changes),
  plus the new `HydraKind`/`HydraParams` types and
  `hydra_kind_for`/`default_hydra_params` functions in
  `post_asap::sketch`, with module-doc cross-references updated for the
  current module layout.

- grouping.rs (asap-aware-mapping): `HydraGroupingStrategy`, a
  `ReplacementStrategy` offering `GroupingStrategy::SharedMultiSubpopulation`
  as an additional candidate wherever the target has a real subpopulation
  concept (non-empty `by`) and its summary family has a modeled Hydra
  variant (KLL only, today). Its own legality checks, `ReplacementStrategy`
  impl shape, and tests needed only mechanical adaptation.

- **`ForceSketchKind` was NOT resurrected.** The original draft (written
  against the first #251 draft) reused `replacement.rs`'s
  `ForceSketchKind` — a whole-recursive-bind `CostModel`-wrapping adapter
  that steers `implement_tree_with`'s decision procedure toward a forced
  `SketchKind`. That pattern was deliberately deleted from this crate as
  an anti-pattern (a real bug where the forced choice could leak into a
  target's own nested aggregates — see `replacement.rs`'s own module
  docs), and `bind::implement_tree`/`implement_tree_with` no longer exist.
  `grouping.rs` now calls `implementation::implementations_for_with(intent,
  cost_model)` to get every ranked candidate directly (already the full
  list, no forcing needed), finds the one `Implementation::Sketch { kind,
  .. }` matching the Hydra-eligible kind, and passes that exact,
  already-decided `Implementation` straight to
  `bind::bind_with_implementation` — the same first-class,
  one-candidate-at-a-time primitive `SketchFamilyStrategy` itself already
  uses. No adapter, no steering, no risk of a forced choice leaking into
  nested aggregates.

- `replacement.rs`: `describe_intent` bumped to `pub(crate)` so
  `grouping::HydraGroupingStrategy` can reuse it for rationale strings
  (`bindable_intent` needed no bump — it now lives in `bind.rs`, already
  `pub`, since #259's "unify binding and replacement-strategy paths"
  refactor moved it there independently of this branch).

- `bind.rs`/`cost_model.rs`: added the new `grouping` field to every
  `SummaryAgg` struct literal in their own test fixtures (mechanical,
  `GroupingStrategy::default()`), plus the same in
  `crates/integration-tests/tests/l4_binding.rs`/`l4_binding_sql.rs`'s
  destructuring patterns (`..`, since those tests don't assert on
  grouping).

Verified: cargo build --workspace --all-targets, cargo test --workspace
(0 failures, including all 14 grouping:: tests — the
implementations_for_with + bind_with_implementation rewrite produces the
same bound SketchKind/params the old ForceSketchKind-based code was
designed to produce), cargo fmt --all -- --check, cargo clippy
--workspace --all-targets --all-features -- -D warnings — all clean.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
zzylol added a commit that referenced this pull request Aug 25, 2026
…256) (#261)

* feat(types,asap-aware-mapping): rebuild grouping.rs fresh on top of #259's current tip

This branch had been forked at the very first draft of #251
(boundary::implementation_for_with, a ForceSketchKind CostModel-wrapping
hack to steer bind::implement_tree_with) and never rebased — completely
missing #259's subsequent history: the boundary.rs -> implementation.rs
rename, the "make binding literally a selector over ReplacementStrategy"
refactor, and this session's deletion of
bind::implement_tree/implement_tree_with. It would not compile against
the current tip of feat/replacement-strategy-251.

Rebuilt fresh: reset this branch onto the current #259 tip, then
reapplied only the genuinely new content this PR adds on top — nothing
here re-implements replacement.rs/bind.rs/cost_model.rs, which now come
from #259 itself.

- types crate (pure additions, brought over wholesale): `GroupingStrategy`
  gains a `grouping: GroupingStrategy` field on `SummaryExpr::SummaryAgg`
  (default `PerSubpopulationInstance`, so no existing behavior changes),
  plus the new `HydraKind`/`HydraParams` types and
  `hydra_kind_for`/`default_hydra_params` functions in
  `post_asap::sketch`, with module-doc cross-references updated for the
  current module layout.

- grouping.rs (asap-aware-mapping): `HydraGroupingStrategy`, a
  `ReplacementStrategy` offering `GroupingStrategy::SharedMultiSubpopulation`
  as an additional candidate wherever the target has a real subpopulation
  concept (non-empty `by`) and its summary family has a modeled Hydra
  variant (KLL only, today). Its own legality checks, `ReplacementStrategy`
  impl shape, and tests needed only mechanical adaptation.

- **`ForceSketchKind` was NOT resurrected.** The original draft (written
  against the first #251 draft) reused `replacement.rs`'s
  `ForceSketchKind` — a whole-recursive-bind `CostModel`-wrapping adapter
  that steers `implement_tree_with`'s decision procedure toward a forced
  `SketchKind`. That pattern was deliberately deleted from this crate as
  an anti-pattern (a real bug where the forced choice could leak into a
  target's own nested aggregates — see `replacement.rs`'s own module
  docs), and `bind::implement_tree`/`implement_tree_with` no longer exist.
  `grouping.rs` now calls `implementation::implementations_for_with(intent,
  cost_model)` to get every ranked candidate directly (already the full
  list, no forcing needed), finds the one `Implementation::Sketch { kind,
  .. }` matching the Hydra-eligible kind, and passes that exact,
  already-decided `Implementation` straight to
  `bind::bind_with_implementation` — the same first-class,
  one-candidate-at-a-time primitive `SketchFamilyStrategy` itself already
  uses. No adapter, no steering, no risk of a forced choice leaking into
  nested aggregates.

- `replacement.rs`: `describe_intent` bumped to `pub(crate)` so
  `grouping::HydraGroupingStrategy` can reuse it for rationale strings
  (`bindable_intent` needed no bump — it now lives in `bind.rs`, already
  `pub`, since #259's "unify binding and replacement-strategy paths"
  refactor moved it there independently of this branch).

- `bind.rs`/`cost_model.rs`: added the new `grouping` field to every
  `SummaryAgg` struct literal in their own test fixtures (mechanical,
  `GroupingStrategy::default()`), plus the same in
  `crates/integration-tests/tests/l4_binding.rs`/`l4_binding_sql.rs`'s
  destructuring patterns (`..`, since those tests don't assert on
  grouping).

Verified: cargo build --workspace --all-targets, cargo test --workspace
(0 failures, including all 14 grouping:: tests — the
implementations_for_with + bind_with_implementation rewrite produces the
same bound SketchKind/params the old ForceSketchKind-based code was
designed to produce), cargo fmt --all -- --check, cargo clippy
--workspace --all-targets --all-features -- -D warnings — all clean.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

* fix(asap-aware-mapping): register Hydra grouping strategy

* fix(types,asap-aware-mapping): scope Hydra to its proven CMS/Count-Sketch construction

The Hydra paper (Manousis et al., VLDB 2022) proves its accuracy bound
(Theorem 2) over one specific construction: hash each subpopulation into
a shared w×r grid of linear, mergeable frequency-vector sketches (its own
substrate is Count-Sketch; Count-Min Sketch is the same collision
algebra). The paper is explicit that this construction excludes
quantiles entirely (§4.3: "A statistic that cannot directly be estimated
by Hydra-sketch is quantiles.").

- Add HydraKind::HydraCms / HydraCountSketch, mapped from the existing
  SketchAlgorithm::Cms/CountSketch — direct instances of the paper's
  proven grid construction, with HydraParams carrying the paper's own
  (width, depth, shared_rows, shared_columns) knobs.
- Keep HydraKind::HydraKll, but document it as an unproven extension
  beyond the paper (quoting the quantile-exclusion line above) rather
  than implying it shares the same guarantee.
- Generalize per_subpopulation_k(node) -> Option<u32> into
  per_subpopulation_sketch_params(node) -> Option<SketchParams>, and
  change default_hydra_params to take the whole SketchParams and match
  per-HydraKind, instead of assuming every inner sketch has a scalar `k`
  the way KLL does. A Hydra "sketch of sketches" is a framework over any
  mergeable inner sketch, not just KLL.
- HydraUnivMon (the paper's own actual named "Hydra-sketch" — L layers of
  Count-Sketch plus a heavy-hitter heap, estimating entropy/L1/L2-norm/
  cardinality as one instance) is deliberately not added here: it needs
  new AggIntent/category vocabulary this crate doesn't have yet. Tracked
  as a follow-up on issue #256.

New/updated tests in both crates cover the two new HydraKind mappings,
default_hydra_params's per-kind dispatch (including a mismatched
kind/params pair returning None), and a grouped Count intent now
offering both HydraCms and HydraCountSketch candidates.

Verified: cargo build --workspace --all-targets, cargo test (asap-types,
asap-aware-mapping, asap-integration-tests — all passing, including the
new tests), cargo fmt --all -- --check, cargo clippy --all-targets
--all-features -- -D warnings — all clean.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

* fix(grouping): preserve Hydra accuracy and state identity

* fix(aware-mapping): cost Hydra by group cardinality

---------

Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
@zzylol
zzylol deleted the feat/replacement-strategy-251 branch September 7, 2026 15:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

asap-aware-mapping: generalize summary/CSE selection into a ReplacementStrategy trait with multiple candidates

2 participants