feat(asap-aware-mapping): CSE stage 4 — Volcano/Cascades cost-based sharing (#237, #223, #212) - #243
Conversation
|
Fixed per feedback: after CSE, a Added Recalibrated New test demonstrating the fix directly: a
|
|
@milindsrivastava1997 FYI for your candidate selection model of asap mapping |
#237) Records the decision for issue #237: how CostModel's still-deferred stage-4 CSE-credit wiring (issue #223's landing plan) should decide whether a detected, legality-gated common subexpression is actually worth sharing. Decision: a hybrid, per #237's own suggestion — unconditional sharing below a cheap-recompute threshold, a real cost comparison above it. Neither pure Volcano/Cascades (this repo has no plan-enumeration/DP search anywhere; new infrastructure disproportionate to a binary share-or-don't decision) nor pure System R (ignores the real cost of continuously maintaining a shared summary for a rarely-read or cheap-to-recompute subtree) fits on its own. The hybrid also matches this crate's own existing pattern for the structurally analogous sketch-vs-exact decision (rank_candidates/ size_params: a cheap built-in default, overridable by a deployment's real cost knowledge) and is layering-forced, not just preferred: detection (asap-types::pre_asap::cse) sits below this crate and cannot consult a CostModel, so it must stay cost-agnostic (System R-style default), pushing the cost-aware override downstream into this crate. Also sketches the concrete shape for a future stage-4 PR: a new CostModel::cse_share_decision trait method (default preserves today's unconditional-share behavior byte for byte), a CseCandidate input carrying the shared subtree/bound summary/consumer count, and the call site (implement_workload_with's memo-hit branch in bind.rs). Documentation only — no CSE-credit logic implemented, per #237's own scope ("not asking for an implementation yet"). Also updates cse.rs's landing-plan note to point at this decision instead of just flagging stage 4 as deferred. Related to #223, #212. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…t-based sharing) Replaces the previous doc-comment-only decision for #237 with a real, working implementation, per review feedback: - The design discussion moves out of code comments into docs/cse-cost-model-decision.md. - CostModel gains CseCandidate/ShareDecision/cse_share_decision, plus two overridable cost-estimation hooks (cse_recompute_cost, cse_shared_maintenance_cost) with concrete default heuristics. The default cse_share_decision body performs a genuine Volcano/Cascades- style cost comparison — share iff the estimated cost of maintaining one shared summary is no greater than the estimated total cost of recomputing it independently at every consumer — rather than a fixed System R-style rule. - bind::implement_workload_with now computes each shared subtree's true consumer_count via a whole-workload pre-pass, decides ShareDecision once per shared subtree (from full knowledge of the workload's sharing structure), and applies it consistently to every occurrence. This closes out #223's stage 4 and #212's original "add CSE" tracking issue. Stage 3 (structural-hash unification) landed separately in #244. New tests: cost_model.rs unit tests pinning the default cost comparison's behavior in both directions and confirming custom CostModel overrides are actually consulted; a bind.rs test confirming a CostModel that declines sharing produces genuinely independent SummaryNodes even for two roots that are the same Rc<QueryExpr>. All pre-existing CSE tests (including the DefaultCostModel-sharing integration tests) still pass unchanged — the default cost weights were calibrated against a real lowered query so today's sharing behavior for realistic subtrees is preserved. Verified: cargo build --workspace --all-targets, cargo test --workspace, cargo fmt --all -- --check, cargo clippy --workspace --all-targets --all-features -- -D warnings, cargo doc --workspace --no-deps (no new broken-link warnings). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…not tree-shaped default_cse_recompute_cost measured a subtree's cost via serde_json::to_string(subtree).len() — but a CseCandidate's subtree, by definition, is something CSE already found sharing in, so it's generally a DAG, not a tree. Rc's Serialize impl serializes the pointee's value at every occurrence, not once by identity, so a naive full serialization re-counts any descendant the subtree already shares internally (e.g. from single-query CSE, x op x collapsing both branches onto one Rc) once per parent that references it — silently inflating the cost estimate relative to the DAG's real size. Adds asap_types::pre_asap::cse::dag_node_count(root) -> usize: counts unique nodes reachable from root, deduplicated by Rc pointer identity, using the same operator-child traversal scope share_common_subtrees itself uses (mirrors rebuild_children's field enumeration, as a separate read-only traversal since rebuild_children consumes and rebuilds its input). default_cse_recompute_cost now calls this instead of serde_json. Recalibrates default_cse_shared_maintenance_cost's UNIT constant (60.0 -> 1.0) to match the new magnitude: node counts are small integers (typically 1-20), not byte lengths (typically 200-500) — the weight table's relative ordering (ExactAggregate cheapest, StatModel priciest) is unchanged, only the absolute scale. Verified this preserves existing default-sharing behavior: all pre-existing CSE tests, including crates/integration-tests/tests/cse.rs's DefaultCostModel-sharing assertions, pass unchanged. New tests: - cse.rs: dag_node_count is the naive count when nothing is shared; correctly dedupes an internally-shared subtree (BinaryOp with both branches pointing at the same Rc: 3 unique nodes, not 5); dedupes correctly across two workload roots sharing one subtree. - cost_model.rs: default_recompute_cost_does_not_double_count_an_internally_shared_descendant — a Join with two independent Scan children costs 3, the same Join with both children pointing at one shared Scan costs 2, not 3 (the bug this fix addresses, demonstrated directly at the layer that consumes dag_node_count). Verified: cargo build --workspace --all-targets, cargo test --workspace (444 passed, 0 failed), cargo fmt --all -- --check, cargo clippy --workspace --all-targets --all-features -- -D warnings, cargo doc --workspace --no-deps (no new broken-link warnings). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
f79be19 to
f2ceb1e
Compare
…dge rename Rebase fixup: #246 (merged after this branch was forked) collapsed QueryExpr::PromqlScalar(f64) into PromqlScalarBridge(Rc<QueryExpr>), matching rebuild_children's existing treatment (never descended into, interned as a single unit). dag_node_count's count_unique needed the same update - it still referenced the removed PromqlScalar variant, which fails to compile against current main. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…des like SharedSubexpressionRule does collect_sketch_findings (SketchApplicabilityRule) walked the workload as a tree, with no Rc-identity tracking - unlike SharedSubexpressionRule's register_site, which explicitly dedupes by Rc::as_ptr. A bindable Aggregate reachable via two paths after share_common_subtrees (two different roots, or two branches of one root, e.g. median(x) / median(x)) was reported as two separate "sketch-approximation is applicable" findings, even though it's bound exactly once - one CostModel::cse_share_decision per shared Rc, per bind::implement_workload_with, not one per path that reaches it. Overstated the number of independent opportunities. for_each_operator_child now passes an Option<usize> Rc pointer alongside each child (None only for Concat's by-value branches, which have no Rc identity of their own) so collect_sketch_findings can thread a `visited` HashSet across the whole evaluate() call - shared across all roots, not reset per root - and skip re-descending into an already-visited pointer, mirroring register_site's exact dedup rationale. New test: a_shared_sketchable_aggregate_is_reported_only_once, the same median(x)/median(x) shape pre_asap::cse's own single_query_shares_its_own_repeated_subtree test uses, pinning that a single shared Aggregate produces exactly one finding. Found while auditing other open PRs for the same tree-vs-DAG issue fixed in PR #243 (default_cse_recompute_cost's naive serde_json-length proxy over-counting internally-shared subtrees). Verified: cargo build --workspace --all-targets, cargo test --workspace (444 passed, 0 failed), cargo fmt --all -- --check, cargo clippy --workspace --all-targets --all-features -- -D warnings. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…des like SharedSubexpressionRule does collect_sketch_findings (SketchApplicabilityRule) walked the workload as a tree, with no Rc-identity tracking - unlike SharedSubexpressionRule's register_site, which explicitly dedupes by Rc::as_ptr. A bindable Aggregate reachable via two paths after share_common_subtrees (two different roots, or two branches of one root, e.g. median(x) / median(x)) was reported as two separate "sketch-approximation is applicable" findings, even though it's bound exactly once - one CostModel::cse_share_decision per shared Rc, per bind::implement_workload_with, not one per path that reaches it. Overstated the number of independent opportunities. for_each_operator_child now passes an Option<usize> Rc pointer alongside each child (None only for Concat's by-value branches, which have no Rc identity of their own) so collect_sketch_findings can thread a `visited` HashSet across the whole evaluate() call - shared across all roots, not reset per root - and skip re-descending into an already-visited pointer, mirroring register_site's exact dedup rationale. New test: a_shared_sketchable_aggregate_is_reported_only_once, the same median(x)/median(x) shape pre_asap::cse's own single_query_shares_its_own_repeated_subtree test uses, pinning that a single shared Aggregate produces exactly one finding. Found while auditing other open PRs for the same tree-vs-DAG issue fixed in PR #243 (default_cse_recompute_cost's naive serde_json-length proxy over-counting internally-shared subtrees). Verified: cargo build --workspace --all-targets, cargo test --workspace (444 passed, 0 failed), cargo fmt --all -- --check, cargo clippy --workspace --all-targets --all-features -- -D warnings. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Summary
Implements #223's stage 4 for real:
CostModel::cse_share_decisiondecideswhether a CSE-detected shared subtree is actually worth sharing, via a real
Volcano/Cascades-style cost comparison rather than a fixed rule.
The design discussion for #237 (rule-based vs. cost-based framework) now
lives in
docs/cse-cost-model-decision.md, not in code comments. See thatdoc for the full reasoning: why cost-based, why not a full plan-search
engine, and the layering constraint that forces CSE detection
(
asap_types::pre_asap::cse) to stay cost-agnostic while the cost-awaredecision lives downstream in
asap-aware-mapping.Core algorithm — NOT dynamic programming
Worth being explicit about this: nothing here is a DP recurrence over a
state space. It's two separate, simpler things — a hash-consing pass
(pre-existing, #235) and a per-candidate greedy cost comparison (this PR).
Detection (
share_common_subtrees, already merged in #235, unchangedby this PR) — bottom-up hash-consing / value-numbering:
Sharing decision (this PR,
bind::implement_workload_with+CostModel::cse_share_decision) — a single left-to-right pass with apre-count, then one independent cost comparison per shared candidate,
cached so every later occurrence reuses the same answer:
Why not DP: each
CseCandidate's decision is evaluated independently —no candidate's
Share/RecomputeIndependentlychoice depends on, orconstrains, any other candidate's choice, and there's no recurrence
combining subproblem solutions toward one globally-optimal objective (the
shape
OPT[i] = f(OPT[i-1], …)takes). The memo above is answer-cachingfor efficiency (decide once per unique shared subtree, not once per
occurrence) — necessary because a naive per-occurrence decision could
disagree with itself across occurrences of the same subtree, but caching a
computed answer isn't sufficient by itself to make an algorithm DP. This is
also why a full Volcano/Cascades-style plan-enumeration search was
explicitly rejected in
docs/cse-cost-model-decision.md: this repo has noDP-based plan memo/search space anywhere, and the binary "share or don't"
question this PR answers doesn't need one — a direct cost comparison is
the whole algorithm.
What changed in code (not just docs)
CostModelgains:CseCandidate— a detected, legality-gated shared subtree with itsbound
SummaryNodeand trueconsumer_count.ShareDecision—ShareorRecomputeIndependently.cse_recompute_cost/cse_shared_maintenance_cost— overridable costhooks, with concrete default heuristics.
cse_share_decision— a real default body: share iff the estimatedshared-maintenance cost is no greater than the estimated total
recompute cost (
cse_recompute_cost * consumer_count). A genuine costcomparison, not a hardcoded rule.
bind::implement_workload_withnow computes each shared subtree's trueconsumer_countvia a whole-workload pre-pass, decidesShareDecisiononce per shared subtree, and applies it consistently to every occurrence.
cse_recompute_cost's default (default_cse_recompute_cost) isDAG-aware, not tree-shaped: it calls the new
asap_types::pre_asap::cse::dag_node_count, which counts unique nodesreachable from a subtree (deduplicated by
Rcpointer identity), ratherthan a naive
serde_jsonserialization length — aCseCandidate'ssubtree is, by definition, something CSE already found sharing in, so
it's generally a DAG, not a tree, and a naive full serialization would
re-count any descendant it already shares internally once per parent
that references it.
Tests
cost_model.rs: unit tests pinning the default cost comparison'sbehavior in both directions, a test confirming a custom
CostModel'soverridden cost hooks are actually consulted, and a test proving the
DAG-vs-tree fix directly (a
Joinwith two independentScanchildrencosts 3 unique nodes; the same
Joinwith both children pointing at oneshared
Scancosts 2, not 3).cse.rs: unit tests fordag_node_count— the naive count when nothingis shared, correct dedup of an internally-shared subtree, and correct
dedup across two workload roots sharing one subtree.
bind.rs: a test confirming aCostModelthat declines sharing producesgenuinely independent (non-
Rc::ptr_eq)SummaryNodes.DefaultCostModel-sharing assertions, pass unchanged — default costweights were calibrated against a real lowered query so today's sharing
behavior for realistic subtrees is preserved.
Test plan
cargo build --workspace --all-targetscargo test --workspace(444 passed, 0 failed)cargo fmt --all -- --checkcargo clippy --workspace --all-targets --all-features -- -D warningscargo doc --workspace --no-deps(no new broken-intra-doc-link warnings)Closes #237. Closes #223. Closes #212.
🤖 Generated with Claude Code