feat(pre-asap): unify dag_export's structural_hash with cse's (stage 3) - #244
Conversation
…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>
…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>
…haring (#237, #223, #212) (#243) * docs(asap-aware-mapping): decide rule-based vs. cost-based CSE sharing (#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> * feat(asap-aware-mapping): implement CSE stage 4 (Volcano/Cascades cost-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> * fix(asap-aware-mapping): CSE recompute-cost proxy must be DAG-aware, 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> * fix(types): update dag_node_count for PromqlScalar -> PromqlScalarBridge 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> --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
9984c04 to
347b3d0
Compare
|
Rebased onto latest main (twice — #246 then #243 merged in the meantime) and addressed the tree-vs-DAG issue you flagged: Rewrote it to be memoized: each node's contribution is now its own tag + non-child scalar fields (still This mirrors New tests prove memoization doesn't change the answer, only the work: hashing a value gives the same result regardless of cache warmth; a tree with internal sharing hashes identically to an unshared-but-structurally-equal tree; and a direct proof that hashing a shared-branch
|
Implements stage 3 of issue #223's landing plan: dag_export's per-node `hash` is now computed by calling `pre_asap::cse::structural_hash` directly (exposed as `pub(crate)`) instead of maintaining an independent reimplementation that happened to use the same trick. `push_node` now takes the actual `QueryExpr` subtree it represents and hashes it via that one canonical function; the old `structural_hash(kind, detail, children, nodes)` helper in dag_export.rs is removed. Adds tests in dag_export.rs asserting the exported hash for a node is literally equal to `cse::structural_hash` applied to the same subtree, at both the root and an interior node. Updates tools/dag-viewer/README.md's "shared subtree highlighting" caveat: the hash the viewer highlights on is no longer a parallel reimplementation, but it is still a proxy for real CSE for two independent reasons — the viewer never re-runs the PartialEq + Schema::has_unique_key legality check InternTable::intern performs before actually sharing an Rc, and no real Rc identity crosses this tool's process boundaries (each query is lowered/exported independently, and the viewer merges JSON from separate invocations). Closing that fully is left as a follow-up, noted in the PR description. Part of #223. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
structural_hash computed a coarse candidate-bucket hash by fully serializing a node's whole subtree via serde_json - correct, but not DAG-aware: called once per node in a bottom-up pass (InternTable::intern during CSE detection, and dag_export::push_node during export), a node near the root re-serializes - re-walks - everything below it, including any descendant already hashed while processing an earlier node. That's O(subtree size) per node instead of O(1) amortized: quadratic-or-worse for a deep chain, and compounds further with any real internal Rc sharing (structural_hash's whole reason for existing operates on trees that, after CSE, generally aren't trees). Rewrites structural_hash to hash each node's own tag and non-child scalar fields (small, still serde_json-based - safe for the f64s some fields carry) combined with each Rc-backed child's hash, looked up in a new HashCache (keyed by Rc pointer) if already computed there instead of recursed into again. InternTable now owns a hash_cache field persisted across its whole lifetime, so a bottom-up pass hashes each node exactly once - O(N) total for N nodes, not O(N) per node. dag_export::export threads the same HashCache through its own build() recursion for the identical reason. This mirrors dag_node_count's DAG-vs-tree fix (issue #212/#223/#237 stage 4) applied to hashing instead of counting - same underlying bug class, found while auditing other open PRs after that fix landed. Exhaustive per-variant match, kept in sync with rebuild_children's existing one (both must be extended together for a new QueryExpr variant to compile). New tests: structural_hash_is_stable_across_cache_states (the hash of a value doesn't depend on cache warmth), _matches_the_unshared_equivalent (memoization doesn't change the answer, only the work), and _memoizes_a_shared_descendant_exactly_once (direct proof: hashing a BinaryOp with both branches pointing at one shared Rc populates the cache with exactly 2 entries - the shared branch's own node count - not more). Verified: cargo build --workspace --all-targets, cargo test --workspace (453 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 - confirmed none of the pre-existing warnings fall within the changed code). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
347b3d0 to
116537c
Compare
…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 — 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 search.rs adds on top — nothing here re-implements replacement.rs/bind.rs, which now come from #259 itself. - search.rs's two implement_tree_with call sites replaced with the established take-the-first-candidate pattern (SketchFamilyStrategy::replacements(...).into_iter().next(), falling back to bind::logical) — the same helper shape bind.rs's own tests and every external caller of this crate now use. - structural_hash's signature grew a `cache: &mut HashCache` parameter since this branch forked (issue #244, CSE stage 3, landed on main in the meantime) — is_duplicate_rewrite now threads a fresh HashCache through its one pairwise comparison. - Made HashCache pub (was pub(crate)) alongside structural_hash, for search.rs's cross-crate reuse — same rationale already documented for structural_hash itself. - lib.rs: added the `search` module doc bullet, `pub mod search;`, and its public re-exports; fixed two stale `boundary::` doc references in search.rs itself. Verified: cargo build --workspace --all-targets, cargo test --workspace (0 failures, including all 12 search:: 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>
Summary
Implements stage 3 of issue #223's 4-stage landing plan:
dag_export'sstructural_hashbecomes literally the same function stage 1'sInternTableuses, instead of a parallel reimplementation.crates/types/src/pre_asap/cse.rs'sstructural_hashis nowpub(crate)(was module-private), with its doc comment updated to describe the new
cross-module caller instead of "unifying the two is stage 3, not done
here."
crates/types/src/dag_export.rsno longer has its ownstructural_hash(kind, detail, children, nodes)helper.push_nodenowtakes the actual
QueryExprsubtree each flattenedDagNoderepresentsand computes
hashby callingpre_asap::cse::structural_hash(expr)directly — the exact same function call
InternTable::internmakes forthat same subtree during real CSE.
dag_export.rsassert this literally, not just "hashesagree by coincidence":
graph.nodes[root].hash == structural_hash(&root),and the same equality checked again at an interior node of a multi-level
tree, not just the root.
tools/dag-viewer/README.md's "shared subtree highlighting is a proxy"caveat is rewritten to describe the new, accurate state.
What's still a proxy, and why
tools/dag-viewer's "shared subtree" highlighting is not full real CSEafter this change, even though the hash it highlights on is now genuinely
the same hash
share_common_subtrees'sInternTableuses internally (nota lookalike). Two independent reasons a hash match in the viewer still
isn't a guarantee of real sharing:
PartialEq+ legality gate isn't re-run.structural_hashisdeliberately only a coarse bucketing filter —
InternTable::internalways follows a hash match with a
PartialEqcheck and aSchema::has_unique_key()legality gate before actually sharing anRc. The viewer only has the hash, so e.g. an ungroupedAggregate(no provable unique key, never actually hoistable) can still show up
highlighted as "shared" there.
Rcidentity crosses this tool's process boundaries. Eachquery given to the
dag_exportbinary is lowered and exportedindependently (
share_common_subtreesis never invoked in that path),and the viewer's own multi-file-load feature merges JSON produced by
entirely separate
dag_exportinvocations. There's noRc<QueryExpr>for the viewer to compare pointer identity on — hash equality is the
only signal available to it by construction, regardless of how well the
hash itself is unified with the real pass.
I looked at closing this fully — threading real
Rc::ptr_eqidentity froman in-process
share_common_subtreescall throughdag_export'snode-flattening into the exported JSON, plus having the
dag_exportbinaryactually call
share_common_subtreesacross all queries given in oneinvocation, plus a viewer highlighting-logic change to prefer that signal
over the hash when present. That's a reasonable follow-up, but it's a
materially bigger, separable change (new export API + binary change + JS
change) than a hash-unification stage should responsibly carry, and it
still couldn't help the viewer's primary "load several independently
generated JSON files" workflow (no shared process, so no shared
Rcregardless). Left as a future issue rather than folded into this PR.
Per the issue (#223 stage 4) and the task scope for this PR,
cost_model.rsand any CSE-credit/cost-based wiring are untouched — that remains blocked on
#237 and is being handled separately.
Test plan
cargo build --workspace --all-targetscargo test --workspace(all green, including the existing 6cseunit tests, the existing 3
crates/integration-tests/tests/cse.rsintegration tests, and 2 new
dag_exportunit tests asserting hashparity with
cse::structural_hash)cargo fmt --all -- --checkcargo clippy --workspace --all-targets --all-features -- -D warningsPart of #223.
🤖 Generated with Claude Code