From d60585b5541ca713bbdaee12c21b51d0e84c1628 Mon Sep 17 00:00:00 2001 From: zz_y Date: Sat, 22 Aug 2026 16:22:01 -0600 Subject: [PATCH 1/2] feat(pre-asap): unify dag_export's structural_hash with cse's (stage 3) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- crates/types/src/dag_export.rs | 162 ++++++++++++++++++++++++------- crates/types/src/pre_asap/cse.rs | 28 ++++-- tools/dag-viewer/README.md | 70 ++++++++----- 3 files changed, 189 insertions(+), 71 deletions(-) diff --git a/crates/types/src/dag_export.rs b/crates/types/src/dag_export.rs index 2f53fb7b..3935e7bc 100644 --- a/crates/types/src/dag_export.rs +++ b/crates/types/src/dag_export.rs @@ -5,17 +5,30 @@ //! `QueryExpr` already derives `Serialize`, but as a Rust-shaped tagged tree //! (`Rc` children nested inside each variant's own field). This module //! flattens that into an explicit node list + child-id edges — the shape a -//! generic graph renderer wants — and additionally computes a bottom-up -//! structural hash per node, so a caller with several exported queries can -//! spot identical subtrees (a shared `Scan`, a repeated `Aggregate` shape, -//! …) by comparing hashes rather than re-implementing `QueryExpr: PartialEq` -//! structural comparison client-side. This hash is also the intended basis -//! for a real CSE pass (issue #212, #222) — today it only drives -//! `tools/dag-viewer`'s highlighting, a proxy for that, not the pass itself. +//! generic graph renderer wants — and additionally tags each node with +//! [`structural_hash`](crate::pre_asap::cse::structural_hash), so a caller +//! with several exported queries can spot identical subtrees (a +//! shared `Scan`, a repeated `Aggregate` shape, …) by comparing hashes +//! rather than re-implementing `QueryExpr: PartialEq` structural comparison +//! client-side. +//! +//! This is literally the same hashing +//! [`share_common_subtrees`](crate::pre_asap::cse::share_common_subtrees) +//! uses to bucket candidates in its `InternTable` (issue #223 stage 3) — not +//! a parallel reimplementation. `tools/dag-viewer`'s "shared subtree" +//! highlighting is still a *proxy* for real CSE, though: a hash match here +//! only means two nodes are legal `InternTable` bucket-mates (same coarse +//! hash), the same candidate-narrowing step `structural_hash` performs +//! inside `InternTable::intern` — it does not mean `share_common_subtrees` +//! actually ran on this data and merged them onto one `Rc` (that also +//! requires the `PartialEq` check `InternTable::intern` performs, and the +//! `Schema::has_unique_key` legality gate, neither of which this export +//! step evaluates). See `tools/dag-viewer/README.md` for the up-to-date +//! caveat. use serde::Serialize; -use std::hash::{Hash, Hasher}; +use crate::pre_asap::cse::structural_hash; use crate::pre_asap::query_expr::{QueryExpr, Source}; /// One flattened IR node. `detail` holds this node's own scalar fields @@ -32,8 +45,12 @@ pub struct DagNode { /// Child node ids, in the variant's field order (e.g. `Join` is /// `[left, right]`). pub children: Vec, - /// Bottom-up structural hash: two nodes hash equally iff their `kind`, - /// `detail`, and (recursively) their children's hashes all match. + /// [`structural_hash`](crate::pre_asap::cse::structural_hash) of the + /// subtree rooted at this node — the exact same function `cse`'s + /// `InternTable` uses to bucket CSE candidates, so two nodes hash + /// equally here iff they would land in the same `InternTable` bucket. + /// See the module doc for what a hash match here does and doesn't + /// guarantee. pub hash: u64, } @@ -73,15 +90,22 @@ pub fn export(expr: &QueryExpr) -> DagGraph { DagGraph { nodes, root } } +/// Push one flattened node for `expr`. `expr` is the *whole* subtree this +/// node represents (not just its own fields) — `hash` is +/// [`structural_hash(expr)`](structural_hash), the identical function and +/// the identical input `InternTable::intern` would hash for this same +/// subtree, so this node's `hash` matches what `cse::share_common_subtrees` +/// would bucket it under. fn push_node( nodes: &mut Vec, + expr: &QueryExpr, kind: &'static str, label: String, detail: serde_json::Value, children: Vec, ) -> u32 { let id = nodes.len() as u32; - let hash = structural_hash(kind, &detail, &children, nodes); + let hash = structural_hash(expr); nodes.push(DagNode { id, kind, @@ -93,25 +117,6 @@ fn push_node( id } -/// `detail.to_string()` is a stable, canonical string: `serde_json::Value`'s -/// default map (no `preserve_order` feature) is a `BTreeMap`, so object keys -/// always serialize in the same sorted order regardless of construction -/// order. -fn structural_hash( - kind: &str, - detail: &serde_json::Value, - children: &[u32], - nodes: &[DagNode], -) -> u64 { - let mut hasher = std::collections::hash_map::DefaultHasher::new(); - kind.hash(&mut hasher); - detail.to_string().hash(&mut hasher); - for &c in children { - nodes[c as usize].hash.hash(&mut hasher); - } - hasher.finish() -} - fn source_label(source: &Source) -> String { match source { Source::Table { table_ref } => table_ref.clone(), @@ -142,7 +147,7 @@ fn build(expr: &QueryExpr, nodes: &mut Vec) -> u32 { "predicates": predicates, "schema": schema, }); - push_node(nodes, "Scan", label, detail, vec![]) + push_node(nodes, expr, "Scan", label, detail, vec![]) } // The bridged child is a scalar-sub-language node (issue #220), not // an operator node `build` can recurse into — serialize it as opaque @@ -153,6 +158,7 @@ fn build(expr: &QueryExpr, nodes: &mut Vec) -> u32 { let detail = serde_json::json!({ "value": inner }); push_node( nodes, + expr, "PromqlScalarBridge", format!("PromqlScalarBridge({inner:?})"), detail, @@ -161,6 +167,7 @@ fn build(expr: &QueryExpr, nodes: &mut Vec) -> u32 { } QueryExpr::QueryTimestamp => push_node( nodes, + expr, "QueryTimestamp", "QueryTimestamp".into(), serde_json::json!({}), @@ -170,6 +177,7 @@ fn build(expr: &QueryExpr, nodes: &mut Vec) -> u32 { let c = build(child, nodes); push_node( nodes, + expr, "PromqlVectorFromScalar", "vector()".into(), serde_json::json!({}), @@ -180,6 +188,7 @@ fn build(expr: &QueryExpr, nodes: &mut Vec) -> u32 { let c = build(child, nodes); push_node( nodes, + expr, "PromqlScalarFromVector", "scalar()".into(), serde_json::json!({}), @@ -191,6 +200,7 @@ fn build(expr: &QueryExpr, nodes: &mut Vec) -> u32 { let detail = serde_json::json!({ "dst": dst, "value": value }); push_node( nodes, + expr, "PromqlRelabel", format!("PromqlRelabel(dst={dst})"), detail, @@ -202,6 +212,7 @@ fn build(expr: &QueryExpr, nodes: &mut Vec) -> u32 { let detail = serde_json::json!({ "selector": selector }); push_node( nodes, + expr, "PromqlInfoEnrich", "PromqlInfoEnrich".into(), detail, @@ -213,6 +224,7 @@ fn build(expr: &QueryExpr, nodes: &mut Vec) -> u32 { let detail = serde_json::json!({ "by": by, "kind": kind }); push_node( nodes, + expr, "PromqlSeriesSample", format!("PromqlSeriesSample({kind:?})"), detail, @@ -222,7 +234,7 @@ fn build(expr: &QueryExpr, nodes: &mut Vec) -> u32 { QueryExpr::Filter { pred, child } => { let c = build(child, nodes); let detail = serde_json::json!({ "pred": pred }); - push_node(nodes, "Filter", "Filter".into(), detail, vec![c]) + push_node(nodes, expr, "Filter", "Filter".into(), detail, vec![c]) } QueryExpr::Project { cols, @@ -233,6 +245,7 @@ fn build(expr: &QueryExpr, nodes: &mut Vec) -> u32 { let detail = serde_json::json!({ "cols": cols, "qualifier": qualifier }); push_node( nodes, + expr, "Project", format!("Project({} cols)", cols.len()), detail, @@ -255,6 +268,7 @@ fn build(expr: &QueryExpr, nodes: &mut Vec) -> u32 { }); push_node( nodes, + expr, "Aggregate", format!("Aggregate({} measures)", measures.len()), detail, @@ -266,6 +280,7 @@ fn build(expr: &QueryExpr, nodes: &mut Vec) -> u32 { let detail = serde_json::json!({ "cols": cols }); push_node( nodes, + expr, "Dedup", format!("Dedup({} cols)", cols.len()), detail, @@ -275,7 +290,7 @@ fn build(expr: &QueryExpr, nodes: &mut Vec) -> u32 { QueryExpr::Concat { children } => { let ids: Vec = children.iter().map(|c| build(c, nodes)).collect(); let label = format!("Concat({} branches)", ids.len()); - push_node(nodes, "Concat", label, serde_json::json!({}), ids) + push_node(nodes, expr, "Concat", label, serde_json::json!({}), ids) } QueryExpr::Join { kind, @@ -286,7 +301,14 @@ fn build(expr: &QueryExpr, nodes: &mut Vec) -> u32 { let l = build(left, nodes); let r = build(right, nodes); let detail = serde_json::json!({ "kind": kind, "pred": pred }); - push_node(nodes, "Join", format!("Join({kind:?})"), detail, vec![l, r]) + push_node( + nodes, + expr, + "Join", + format!("Join({kind:?})"), + detail, + vec![l, r], + ) } QueryExpr::SetOp { kind, @@ -299,6 +321,7 @@ fn build(expr: &QueryExpr, nodes: &mut Vec) -> u32 { let detail = serde_json::json!({ "kind": kind, "all": all }); push_node( nodes, + expr, "SetOp", format!("SetOp({kind:?})"), detail, @@ -314,6 +337,7 @@ fn build(expr: &QueryExpr, nodes: &mut Vec) -> u32 { let detail = serde_json::json!({ "keys": keys, "partition_by": partition_by }); push_node( nodes, + expr, "Sort", format!("Sort({} keys)", keys.len()), detail, @@ -323,7 +347,7 @@ fn build(expr: &QueryExpr, nodes: &mut Vec) -> u32 { QueryExpr::Limit { n, offset, child } => { let c = build(child, nodes); let detail = serde_json::json!({ "n": n, "offset": offset }); - push_node(nodes, "Limit", format!("Limit({n})"), detail, vec![c]) + push_node(nodes, expr, "Limit", format!("Limit({n})"), detail, vec![c]) } QueryExpr::PromqlSubquery { range, @@ -334,6 +358,7 @@ fn build(expr: &QueryExpr, nodes: &mut Vec) -> u32 { let detail = serde_json::json!({ "range": range, "resolution": resolution }); push_node( nodes, + expr, "PromqlSubquery", "PromqlSubquery".into(), detail, @@ -345,6 +370,7 @@ fn build(expr: &QueryExpr, nodes: &mut Vec) -> u32 { let detail = serde_json::json!({ "range": range }); push_node( nodes, + expr, "TimeRange", format!("TimeRange({range:?})"), detail, @@ -354,7 +380,14 @@ fn build(expr: &QueryExpr, nodes: &mut Vec) -> u32 { QueryExpr::TimeShift { shift, child } => { let c = build(child, nodes); let detail = serde_json::json!({ "shift": shift }); - push_node(nodes, "TimeShift", "TimeShift".into(), detail, vec![c]) + push_node( + nodes, + expr, + "TimeShift", + "TimeShift".into(), + detail, + vec![c], + ) } QueryExpr::SQLWindowFunc { func, @@ -374,6 +407,7 @@ fn build(expr: &QueryExpr, nodes: &mut Vec) -> u32 { }); push_node( nodes, + expr, "SQLWindowFunc", format!("SQLWindowFunc({func:?})"), detail, @@ -391,6 +425,7 @@ fn build(expr: &QueryExpr, nodes: &mut Vec) -> u32 { let detail = serde_json::json!({ "op": op.to_string(), "vector_match": vector_match }); push_node( nodes, + expr, "BinaryOp", format!("BinaryOp({op})"), detail, @@ -552,4 +587,59 @@ mod tests { g2.nodes[g2.root as usize].hash ); } + + // ── Issue #223 stage 3: dag_export's hash literally *is* cse's hash ──── + + #[test] + fn root_hash_matches_cse_structural_hash_for_the_same_node() { + // Not just "hashes equal for equal inputs" (any two consistent hash + // functions would do that) — the exported root's `hash` must be the + // literal `u64` `crate::pre_asap::cse::structural_hash` produces for + // this exact node, because it's the same function call, not a + // parallel reimplementation that happens to agree. + let leaf = scan("metrics", value_col()); + let graph = export(&leaf); + assert_eq!( + graph.nodes[graph.root as usize].hash, + structural_hash(&leaf), + "dag_export's root hash must equal cse::structural_hash(&leaf) directly" + ); + } + + #[test] + fn every_node_hash_matches_cse_structural_hash_on_its_own_subtree() { + // A multi-level tree: check the parity holds at every depth, not + // just the root — each `DagNode::hash` must equal + // `structural_hash` applied to the actual `QueryExpr` subtree that + // node represents. + let agg = QueryExpr::Aggregate { + reduction: Reduction::Reduce(GroupKeys::none()), + measures: vec![AggIntent::Count { + accuracy: AccuracyTarget::Exact, + }], + output_names: vec![], + having: None, + child: Rc::new(scan("metrics", value_col())), + }; + let root = QueryExpr::Filter { + pred: Predicate(Rc::new(QueryExpr::Literal(ScalarValue::Boolean(true)))), + child: Rc::new(agg.clone()), + }; + + let graph = export(&root); + assert_eq!( + graph.nodes[graph.root as usize].hash, + structural_hash(&root), + "Filter root hash must match cse::structural_hash(&root)" + ); + + let filter = &graph.nodes[graph.root as usize]; + let agg_node = &graph.nodes[filter.children[0] as usize]; + assert_eq!( + agg_node.hash, + structural_hash(&agg), + "the exported Aggregate node's hash must match cse::structural_hash \ + on the Aggregate subtree it represents, not just the root" + ); + } } diff --git a/crates/types/src/pre_asap/cse.rs b/crates/types/src/pre_asap/cse.rs index c807f534..dddd2bba 100644 --- a/crates/types/src/pre_asap/cse.rs +++ b/crates/types/src/pre_asap/cse.rs @@ -84,9 +84,13 @@ //! ([`asap_aware_mapping::implement_workload`]) is a real caller, wired at //! the same time so this never becomes unwired dead code again (the original //! `asap-plan::cse::dedupe_subtrees` was deleted in #192 for exactly that). -//! Stages 3 (`dag_export::structural_hash` unification) and 4 (`CostModel` -//! CSE credit) are deliberately deferred follow-ups, not attempted here. -//! Stage 4 (issue #237) is implemented in +//! Stage 3 — [`dag_export`](crate::dag_export) computing its per-node `hash` +//! by calling this module's [`structural_hash`] directly, instead of a +//! parallel reimplementation — is also done, so `tools/dag-viewer`'s +//! "shared subtree" highlighting now flags exactly the candidate pairs this +//! module's own `InternTable` would bucket together (still only a hash +//! match, not a guarantee of `share_common_subtrees`-actual sharing — see +//! `dag_export`'s module doc). Stage 4 (issue #237) is implemented in //! `asap_aware_mapping::cost_model::CostModel::cse_share_decision` and its //! caller, `asap_aware_mapping::bind::implement_workload_with` — a real, //! Volcano/Cascades-style cost comparison over what this module detects, not @@ -151,13 +155,17 @@ impl InternTable { /// /// `QueryExpr` carries `f64`s (`Literal(ScalarValue::Float64)`, `AggIntent::Quantile.q`, …), so it /// cannot derive `std::hash::Hash`. Serializing to a canonical JSON string -/// and hashing that sidesteps the `f64` problem the same way -/// `dag_export.rs`'s own `structural_hash` does — a deliberately independent -/// implementation for now (this module's stage 1; unifying the two is stage -/// 3 of issue #223's landing plan, not done here). A NaN/infinite `f64` -/// makes JSON serialization fail; falling back to a fixed hash just puts -/// every such node in one (larger, still `PartialEq`-disambiguated) bucket. -fn structural_hash(node: &QueryExpr) -> u64 { +/// and hashing that sidesteps the `f64` problem. +/// +/// `pub(crate)` (not private) so [`dag_export`](crate::dag_export) can call +/// this exact function for its exported nodes' `hash` field instead of +/// maintaining its own parallel reimplementation — issue #223 stage 3. That +/// makes `tools/dag-viewer`'s "shared subtree" highlighting reflect this +/// module's real hashing, not a lookalike computed a different way; see the +/// module doc's "Landing plan" section. A NaN/infinite `f64` makes JSON +/// serialization fail; falling back to a fixed hash just puts every such +/// node in one (larger, still `PartialEq`-disambiguated) bucket. +pub(crate) fn structural_hash(node: &QueryExpr) -> u64 { let mut hasher = std::collections::hash_map::DefaultHasher::new(); let canonical = serde_json::to_string(node).unwrap_or_default(); canonical.hash(&mut hasher); diff --git a/tools/dag-viewer/README.md b/tools/dag-viewer/README.md index 40e09fa9..be824b30 100644 --- a/tools/dag-viewer/README.md +++ b/tools/dag-viewer/README.md @@ -102,33 +102,53 @@ identity and **not** the output of a real common-subexpression-elimination pass — see the next section for why, and don't read the UI's "shared" / "merged" language as claiming otherwise. -## Shared-subtree highlighting is a proxy, not real CSE +## Shared-subtree highlighting is a real-hash proxy, not real CSE (yet) The highlight (and Compare/Union mode's notion of "shared") is computed by -hashing each node's `(kind, detail, children)` bottom-up and matching hashes -across queries — see the doc comment on `crates/types/src/dag_export.rs`. -This is **not** the same guarantee a real CSE pass gives: - -1. **No `PartialEq` + legality re-check.** A real CSE pass (once one exists - and is wired into an end-to-end multi-root planning path — see - `asap_types::pre_asap::cse::share_common_subtrees` and issue #223) follows - a hash match with a full `PartialEq` check and a legality gate (e.g. can - this actually be hoisted without changing semantics) before treating two - subtrees as the same. The viewer only has the hash, so a hash collision - or a node that hashes alike but isn't legally shareable would still show - up as "shared"/"merged" here. -2. **No real `Rc` identity crosses this tool's process boundary.** Each - query given to the `dag_export` binary is lowered and exported - independently, and the viewer's multi-file-load feature merges JSON - produced by entirely separate invocations. There's no `Rc` for - the viewer to compare pointer identity on — hash equality is the only - signal available to it, by construction, regardless of how the hash - itself is computed. - -If/when real CSE output (a `CseWorkloadPlan`'s `bindings`/`Ref`s) is -available from an end-to-end planning path, Union mode's converging-edges -view is a natural place to render that directly instead of (or alongside) -this hash-based proxy. +matching each node's `hash` across queries (a node is "shared" once its +hash shows up under ≥ 2 distinct query names). +That `hash` is no longer a viewer-only reimplementation: as of issue #223 +stage 3, `dag_export`'s per-node `hash` is computed by calling +`asap_types::pre_asap::cse::structural_hash` directly — the exact same +function, on the exact same input, that +`asap_types::pre_asap::cse::share_common_subtrees`'s `InternTable` uses to +bucket its own merge candidates (`crates/types/src/dag_export.rs`, +`crates/types/src/pre_asap/cse.rs`). Two nodes with equal `hash` here really +are exactly the pair `InternTable::intern` would go on to run its +`PartialEq` check against. + +It's still a **proxy**, though, for two independent reasons — a hash match +here does not by itself mean `share_common_subtrees` ran and actually merged +those nodes onto one `Rc`: + +1. **The `PartialEq` + legality gate isn't re-run.** `structural_hash` is + deliberately only a coarse bucketing filter (hash collisions are + possible, and never disambiguated here); the viewer trusts a hash match + as "structurally identical" without also re-checking `PartialEq` or + `Schema::has_unique_key()` the way `InternTable::intern` does before + actually sharing an `Rc`. A node with no provable unique key (e.g. an + ungrouped `Aggregate`) can still show up highlighted here even though + real CSE would never hoist it. +2. **No real `Rc` identity crosses this tool's process boundaries.** Each + `--sql`/`--promql` query given to the `dag_export` binary is lowered and + exported independently — `share_common_subtrees` is never actually + invoked in this path — and the viewer's own multi-file-load feature + merges JSON produced by entirely separate `dag_export` invocations (or + even separate machines/times). There is no `Rc` for the + viewer to compare pointer identity on; hash equality is the only signal + available to it, by construction, regardless of how the hash is + computed. + +Closing this fully — real `Rc::ptr_eq`-based highlighting reflecting an +actual `share_common_subtrees` run — would mean threading `Rc` pointer +identity from a single in-process `share_common_subtrees` call through +`dag_export`'s node-flattening and into the exported JSON (a new field +alongside `hash`), and having the `dag_export` binary actually call +`share_common_subtrees` across all queries given in one invocation before +exporting. That's a reasonable follow-up but a materially bigger change than +this hash-unification step (new export API, binary changes, and a viewer +highlighting-logic change) and orthogonal to it, so it's left for a future +issue rather than folded into #223 stage 3. ## Vendored dependencies From 116537ccf30e6d140d4e9d4646ee71c82555cc59 Mon Sep 17 00:00:00 2001 From: zz_y Date: Sat, 22 Aug 2026 18:37:26 -0600 Subject: [PATCH 2/2] fix(types): make structural_hash DAG-aware (memoized), not tree-shaped 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 --- crates/types/src/dag_export.rs | 118 ++++++++---- crates/types/src/pre_asap/cse.rs | 319 ++++++++++++++++++++++++++++++- 2 files changed, 397 insertions(+), 40 deletions(-) diff --git a/crates/types/src/dag_export.rs b/crates/types/src/dag_export.rs index 3935e7bc..4bf7e066 100644 --- a/crates/types/src/dag_export.rs +++ b/crates/types/src/dag_export.rs @@ -28,7 +28,7 @@ use serde::Serialize; -use crate::pre_asap::cse::structural_hash; +use crate::pre_asap::cse::{structural_hash, HashCache}; use crate::pre_asap::query_expr::{QueryExpr, Source}; /// One flattened IR node. `detail` holds this node's own scalar fields @@ -86,7 +86,12 @@ pub struct WorkloadGraph { /// Flatten `expr` into a [`DagGraph`]. pub fn export(expr: &QueryExpr) -> DagGraph { let mut nodes = Vec::new(); - let root = build(expr, &mut nodes); + // One cache for the whole export — persisted across every `build`/ + // `push_node` call, not reset per node, so `structural_hash` memoizes + // real work across this pass instead of re-walking an already-hashed + // shared descendant once per node that references it. + let mut cache = HashCache::new(); + let root = build(expr, &mut nodes, &mut cache); DagGraph { nodes, root } } @@ -99,13 +104,14 @@ pub fn export(expr: &QueryExpr) -> DagGraph { fn push_node( nodes: &mut Vec, expr: &QueryExpr, + cache: &mut HashCache, kind: &'static str, label: String, detail: serde_json::Value, children: Vec, ) -> u32 { let id = nodes.len() as u32; - let hash = structural_hash(expr); + let hash = structural_hash(expr, cache); nodes.push(DagNode { id, kind, @@ -134,7 +140,7 @@ fn source_label(source: &Source) -> String { /// serializes it as opaque `detail` JSON via `Predicate`/`ProjectItem`/ /// `AggIntent`'s own `Serialize` impl, same as before the merge — a scalar /// subtree was never a separate DAG node, so this doesn't change that. -fn build(expr: &QueryExpr, nodes: &mut Vec) -> u32 { +fn build(expr: &QueryExpr, nodes: &mut Vec, cache: &mut HashCache) -> u32 { match expr { QueryExpr::Scan { source, @@ -147,7 +153,7 @@ fn build(expr: &QueryExpr, nodes: &mut Vec) -> u32 { "predicates": predicates, "schema": schema, }); - push_node(nodes, expr, "Scan", label, detail, vec![]) + push_node(nodes, expr, cache, "Scan", label, detail, vec![]) } // The bridged child is a scalar-sub-language node (issue #220), not // an operator node `build` can recurse into — serialize it as opaque @@ -159,6 +165,7 @@ fn build(expr: &QueryExpr, nodes: &mut Vec) -> u32 { push_node( nodes, expr, + cache, "PromqlScalarBridge", format!("PromqlScalarBridge({inner:?})"), detail, @@ -168,16 +175,18 @@ fn build(expr: &QueryExpr, nodes: &mut Vec) -> u32 { QueryExpr::QueryTimestamp => push_node( nodes, expr, + cache, "QueryTimestamp", "QueryTimestamp".into(), serde_json::json!({}), vec![], ), QueryExpr::PromqlVectorFromScalar(child) => { - let c = build(child, nodes); + let c = build(child, nodes, cache); push_node( nodes, expr, + cache, "PromqlVectorFromScalar", "vector()".into(), serde_json::json!({}), @@ -185,10 +194,11 @@ fn build(expr: &QueryExpr, nodes: &mut Vec) -> u32 { ) } QueryExpr::PromqlScalarFromVector(child) => { - let c = build(child, nodes); + let c = build(child, nodes, cache); push_node( nodes, expr, + cache, "PromqlScalarFromVector", "scalar()".into(), serde_json::json!({}), @@ -196,11 +206,12 @@ fn build(expr: &QueryExpr, nodes: &mut Vec) -> u32 { ) } QueryExpr::PromqlRelabel { dst, value, child } => { - let c = build(child, nodes); + let c = build(child, nodes, cache); let detail = serde_json::json!({ "dst": dst, "value": value }); push_node( nodes, expr, + cache, "PromqlRelabel", format!("PromqlRelabel(dst={dst})"), detail, @@ -208,11 +219,12 @@ fn build(expr: &QueryExpr, nodes: &mut Vec) -> u32 { ) } QueryExpr::PromqlInfoEnrich { selector, child } => { - let c = build(child, nodes); + let c = build(child, nodes, cache); let detail = serde_json::json!({ "selector": selector }); push_node( nodes, expr, + cache, "PromqlInfoEnrich", "PromqlInfoEnrich".into(), detail, @@ -220,11 +232,12 @@ fn build(expr: &QueryExpr, nodes: &mut Vec) -> u32 { ) } QueryExpr::PromqlSeriesSample { by, kind, child } => { - let c = build(child, nodes); + let c = build(child, nodes, cache); let detail = serde_json::json!({ "by": by, "kind": kind }); push_node( nodes, expr, + cache, "PromqlSeriesSample", format!("PromqlSeriesSample({kind:?})"), detail, @@ -232,20 +245,29 @@ fn build(expr: &QueryExpr, nodes: &mut Vec) -> u32 { ) } QueryExpr::Filter { pred, child } => { - let c = build(child, nodes); + let c = build(child, nodes, cache); let detail = serde_json::json!({ "pred": pred }); - push_node(nodes, expr, "Filter", "Filter".into(), detail, vec![c]) + push_node( + nodes, + expr, + cache, + "Filter", + "Filter".into(), + detail, + vec![c], + ) } QueryExpr::Project { cols, qualifier, child, } => { - let c = build(child, nodes); + let c = build(child, nodes, cache); let detail = serde_json::json!({ "cols": cols, "qualifier": qualifier }); push_node( nodes, expr, + cache, "Project", format!("Project({} cols)", cols.len()), detail, @@ -259,7 +281,7 @@ fn build(expr: &QueryExpr, nodes: &mut Vec) -> u32 { having, child, } => { - let c = build(child, nodes); + let c = build(child, nodes, cache); let detail = serde_json::json!({ "reduction": reduction, "measures": measures, @@ -269,6 +291,7 @@ fn build(expr: &QueryExpr, nodes: &mut Vec) -> u32 { push_node( nodes, expr, + cache, "Aggregate", format!("Aggregate({} measures)", measures.len()), detail, @@ -276,11 +299,12 @@ fn build(expr: &QueryExpr, nodes: &mut Vec) -> u32 { ) } QueryExpr::Dedup { cols, child } => { - let c = build(child, nodes); + let c = build(child, nodes, cache); let detail = serde_json::json!({ "cols": cols }); push_node( nodes, expr, + cache, "Dedup", format!("Dedup({} cols)", cols.len()), detail, @@ -288,9 +312,17 @@ fn build(expr: &QueryExpr, nodes: &mut Vec) -> u32 { ) } QueryExpr::Concat { children } => { - let ids: Vec = children.iter().map(|c| build(c, nodes)).collect(); + let ids: Vec = children.iter().map(|c| build(c, nodes, cache)).collect(); let label = format!("Concat({} branches)", ids.len()); - push_node(nodes, expr, "Concat", label, serde_json::json!({}), ids) + push_node( + nodes, + expr, + cache, + "Concat", + label, + serde_json::json!({}), + ids, + ) } QueryExpr::Join { kind, @@ -298,12 +330,13 @@ fn build(expr: &QueryExpr, nodes: &mut Vec) -> u32 { left, right, } => { - let l = build(left, nodes); - let r = build(right, nodes); + let l = build(left, nodes, cache); + let r = build(right, nodes, cache); let detail = serde_json::json!({ "kind": kind, "pred": pred }); push_node( nodes, expr, + cache, "Join", format!("Join({kind:?})"), detail, @@ -316,12 +349,13 @@ fn build(expr: &QueryExpr, nodes: &mut Vec) -> u32 { left, right, } => { - let l = build(left, nodes); - let r = build(right, nodes); + let l = build(left, nodes, cache); + let r = build(right, nodes, cache); let detail = serde_json::json!({ "kind": kind, "all": all }); push_node( nodes, expr, + cache, "SetOp", format!("SetOp({kind:?})"), detail, @@ -333,11 +367,12 @@ fn build(expr: &QueryExpr, nodes: &mut Vec) -> u32 { partition_by, child, } => { - let c = build(child, nodes); + let c = build(child, nodes, cache); let detail = serde_json::json!({ "keys": keys, "partition_by": partition_by }); push_node( nodes, expr, + cache, "Sort", format!("Sort({} keys)", keys.len()), detail, @@ -345,20 +380,29 @@ fn build(expr: &QueryExpr, nodes: &mut Vec) -> u32 { ) } QueryExpr::Limit { n, offset, child } => { - let c = build(child, nodes); + let c = build(child, nodes, cache); let detail = serde_json::json!({ "n": n, "offset": offset }); - push_node(nodes, expr, "Limit", format!("Limit({n})"), detail, vec![c]) + push_node( + nodes, + expr, + cache, + "Limit", + format!("Limit({n})"), + detail, + vec![c], + ) } QueryExpr::PromqlSubquery { range, resolution, child, } => { - let c = build(child, nodes); + let c = build(child, nodes, cache); let detail = serde_json::json!({ "range": range, "resolution": resolution }); push_node( nodes, expr, + cache, "PromqlSubquery", "PromqlSubquery".into(), detail, @@ -366,11 +410,12 @@ fn build(expr: &QueryExpr, nodes: &mut Vec) -> u32 { ) } QueryExpr::TimeRange { range, child } => { - let c = build(child, nodes); + let c = build(child, nodes, cache); let detail = serde_json::json!({ "range": range }); push_node( nodes, expr, + cache, "TimeRange", format!("TimeRange({range:?})"), detail, @@ -378,11 +423,12 @@ fn build(expr: &QueryExpr, nodes: &mut Vec) -> u32 { ) } QueryExpr::TimeShift { shift, child } => { - let c = build(child, nodes); + let c = build(child, nodes, cache); let detail = serde_json::json!({ "shift": shift }); push_node( nodes, expr, + cache, "TimeShift", "TimeShift".into(), detail, @@ -397,7 +443,7 @@ fn build(expr: &QueryExpr, nodes: &mut Vec) -> u32 { output_name, child, } => { - let c = build(child, nodes); + let c = build(child, nodes, cache); let detail = serde_json::json!({ "func": func, "args": args, @@ -408,6 +454,7 @@ fn build(expr: &QueryExpr, nodes: &mut Vec) -> u32 { push_node( nodes, expr, + cache, "SQLWindowFunc", format!("SQLWindowFunc({func:?})"), detail, @@ -420,12 +467,13 @@ fn build(expr: &QueryExpr, nodes: &mut Vec) -> u32 { rhs, vector_match, } => { - let l = build(lhs, nodes); - let r = build(rhs, nodes); + let l = build(lhs, nodes, cache); + let r = build(rhs, nodes, cache); let detail = serde_json::json!({ "op": op.to_string(), "vector_match": vector_match }); push_node( nodes, expr, + cache, "BinaryOp", format!("BinaryOp({op})"), detail, @@ -601,8 +649,8 @@ mod tests { let graph = export(&leaf); assert_eq!( graph.nodes[graph.root as usize].hash, - structural_hash(&leaf), - "dag_export's root hash must equal cse::structural_hash(&leaf) directly" + structural_hash(&leaf, &mut HashCache::new()), + "dag_export's root hash must equal cse::structural_hash(&leaf, &mut HashCache::new()) directly" ); } @@ -629,15 +677,15 @@ mod tests { let graph = export(&root); assert_eq!( graph.nodes[graph.root as usize].hash, - structural_hash(&root), - "Filter root hash must match cse::structural_hash(&root)" + structural_hash(&root, &mut HashCache::new()), + "Filter root hash must match cse::structural_hash(&root, &mut HashCache::new())" ); let filter = &graph.nodes[graph.root as usize]; let agg_node = &graph.nodes[filter.children[0] as usize]; assert_eq!( agg_node.hash, - structural_hash(&agg), + structural_hash(&agg, &mut HashCache::new()), "the exported Aggregate node's hash must match cse::structural_hash \ on the Aggregate subtree it represents, not just the root" ); diff --git a/crates/types/src/pre_asap/cse.rs b/crates/types/src/pre_asap/cse.rs index dddd2bba..0ec319d9 100644 --- a/crates/types/src/pre_asap/cse.rs +++ b/crates/types/src/pre_asap/cse.rs @@ -116,12 +116,20 @@ use super::query_expr::QueryExpr; /// nodes just means a (harmless) linear scan of a few extra candidates. struct InternTable { buckets: HashMap>>, + /// Memoizes [`structural_hash`] per already-hashed `Rc` pointer, shared + /// across every [`intern`](Self::intern) call for the table's whole + /// lifetime — see [`structural_hash`]'s own doc on why this matters: + /// without it, hashing an `N`-node bottom-up pass costs `O(N)` work + /// *per node* (every already-interned descendant gets re-walked), not + /// `O(1)` amortized. + hash_cache: HashCache, } impl InternTable { fn new() -> Self { Self { buckets: HashMap::new(), + hash_cache: HashMap::new(), } } @@ -130,7 +138,7 @@ impl InternTable { /// sharing is legal (see "Legality" above) — return the existing `Rc` /// instead of allocating a new one. fn intern(&mut self, node: QueryExpr) -> Rc { - let hash = structural_hash(&node); + let hash = structural_hash(&node, &mut self.hash_cache); // A node with no provable unique key is never *returned* as a match // for something else — it may still go on to occupy a fresh slot in // the bucket (harmless; it just never gets found by a later @@ -150,12 +158,41 @@ impl InternTable { } } +/// [`structural_hash`]'s memoization cache: maps an already-hashed node's +/// `Rc` pointer to its computed hash. Not tied to any one `QueryExpr` — a +/// fresh, empty cache is correct to start with anywhere; what matters is +/// letting it *persist* across every node in one bottom-up pass (as +/// [`InternTable`] does via its own `hash_cache` field), rather than +/// starting a new one per call. +pub(crate) type HashCache = HashMap<*const QueryExpr, u64>; + /// Coarse structural hash used only to bucket [`InternTable::intern`]'s /// candidate search — never the actual sharing decision (`PartialEq` is). /// /// `QueryExpr` carries `f64`s (`Literal(ScalarValue::Float64)`, `AggIntent::Quantile.q`, …), so it /// cannot derive `std::hash::Hash`. Serializing to a canonical JSON string -/// and hashing that sidesteps the `f64` problem. +/// and hashing that sidesteps the `f64` problem — but only for `node`'s own +/// tag and non-child fields, *not* its children's full values: each +/// `Rc`-backed child's contribution is its own [`structural_hash`], looked +/// up in `cache` if already computed there (memoized by `Rc` pointer +/// identity) rather than recursed into again. +/// +/// This is the DAG-aware fix a naive "just serialize the whole subtree" +/// hash would get wrong: after [`share_common_subtrees`] (or even before +/// it — a front end can emit internal `Rc` sharing directly, e.g. a +/// repeated subexpression within one query), `node` is generally a DAG, +/// not a tree. A full-subtree serialization re-serializes — re-walks — +/// any descendant `node` already shares internally once per parent that +/// references it; called once per node in a bottom-up pass (as +/// [`InternTable::intern`] and [`dag_export`](crate::dag_export) both do), +/// that costs `O(subtree size)` *per node* instead of `O(1)` amortized — +/// quadratic-or-worse for a deep chain, compounding further with any real +/// internal sharing. Memoizing each child's hash by pointer identity in +/// `cache` (persisted across the whole pass by the caller, not reset per +/// node) makes each node's own contribution `O(1)` beyond its children's +/// already-known hashes, giving `O(N)` total for `N` nodes — matching +/// [`dag_node_count`]'s own DAG-vs-tree fix (issue #212/#223/#237's stage +/// 4) in spirit, applied to hashing instead of counting. /// /// `pub(crate)` (not private) so [`dag_export`](crate::dag_export) can call /// this exact function for its exported nodes' `hash` field instead of @@ -165,10 +202,205 @@ impl InternTable { /// module doc's "Landing plan" section. A NaN/infinite `f64` makes JSON /// serialization fail; falling back to a fixed hash just puts every such /// node in one (larger, still `PartialEq`-disambiguated) bucket. -pub(crate) fn structural_hash(node: &QueryExpr) -> u64 { +/// +/// Exhaustive over every `QueryExpr` variant, matching [`rebuild_children`] +/// in which fields count as an operator child (must stay in sync — a new +/// variant fails to compile in both places until both are extended). +pub(crate) fn structural_hash(node: &QueryExpr, cache: &mut HashCache) -> u64 { + use QueryExpr::*; + + fn child_hash(child: &Rc, cache: &mut HashCache) -> u64 { + let ptr = Rc::as_ptr(child); + if let Some(&h) = cache.get(&ptr) { + return h; + } + let h = structural_hash(child, cache); + cache.insert(ptr, h); + h + } + + /// Hash `own_fields` (this node's own tag and non-child scalar + /// fields — anything JSON-serializable and small, i.e. never a + /// `QueryExpr` subtree) via the same canonical-JSON-string trick the + /// whole-subtree version used, just applied to `O(1)` fields instead + /// of `O(subtree size)`. + fn hash_own_fields(hasher: &mut impl Hasher, own_fields: &impl serde::Serialize) { + serde_json::to_string(own_fields) + .unwrap_or_default() + .hash(hasher); + } + let mut hasher = std::collections::hash_map::DefaultHasher::new(); - let canonical = serde_json::to_string(node).unwrap_or_default(); - canonical.hash(&mut hasher); + match node { + Scan { + source, + predicates, + schema, + } => hash_own_fields(&mut hasher, &("Scan", source, predicates, schema)), + PromqlVectorFromScalar(c) => { + "PromqlVectorFromScalar".hash(&mut hasher); + child_hash(c, cache).hash(&mut hasher); + } + PromqlScalarFromVector(c) => { + "PromqlScalarFromVector".hash(&mut hasher); + child_hash(c, cache).hash(&mut hasher); + } + PromqlRelabel { dst, value, child } => { + hash_own_fields(&mut hasher, &("PromqlRelabel", dst, value)); + child_hash(child, cache).hash(&mut hasher); + } + PromqlInfoEnrich { selector, child } => { + hash_own_fields(&mut hasher, &("PromqlInfoEnrich", selector)); + child_hash(child, cache).hash(&mut hasher); + } + PromqlSeriesSample { by, kind, child } => { + hash_own_fields(&mut hasher, &("PromqlSeriesSample", by, kind)); + child_hash(child, cache).hash(&mut hasher); + } + Filter { pred, child } => { + hash_own_fields(&mut hasher, &("Filter", pred)); + child_hash(child, cache).hash(&mut hasher); + } + Project { + cols, + qualifier, + child, + } => { + hash_own_fields(&mut hasher, &("Project", cols, qualifier)); + child_hash(child, cache).hash(&mut hasher); + } + Aggregate { + reduction, + measures, + output_names, + having, + child, + } => { + hash_own_fields( + &mut hasher, + &("Aggregate", reduction, measures, output_names, having), + ); + child_hash(child, cache).hash(&mut hasher); + } + Dedup { cols, child } => { + hash_own_fields(&mut hasher, &("Dedup", cols)); + child_hash(child, cache).hash(&mut hasher); + } + Concat { children } => { + "Concat".hash(&mut hasher); + for c in children { + // Stored by value, not `Rc` — see `rebuild_children`'s + // `intern_owned` use for this variant — so there's no + // pointer to memoize on here; recurse directly. Any + // `Rc`-typed descendant beneath `c` still gets memoized + // once this call reaches it. + structural_hash(c, cache).hash(&mut hasher); + } + } + Join { + kind, + pred, + left, + right, + } => { + hash_own_fields(&mut hasher, &("Join", kind, pred)); + child_hash(left, cache).hash(&mut hasher); + child_hash(right, cache).hash(&mut hasher); + } + SetOp { + kind, + all, + left, + right, + } => { + hash_own_fields(&mut hasher, &("SetOp", kind, all)); + child_hash(left, cache).hash(&mut hasher); + child_hash(right, cache).hash(&mut hasher); + } + Sort { + keys, + partition_by, + child, + } => { + hash_own_fields(&mut hasher, &("Sort", keys, partition_by)); + child_hash(child, cache).hash(&mut hasher); + } + Limit { n, offset, child } => { + hash_own_fields(&mut hasher, &("Limit", n, offset)); + child_hash(child, cache).hash(&mut hasher); + } + PromqlSubquery { + range, + resolution, + child, + } => { + hash_own_fields(&mut hasher, &("PromqlSubquery", range, resolution)); + child_hash(child, cache).hash(&mut hasher); + } + TimeRange { range, child } => { + hash_own_fields(&mut hasher, &("TimeRange", range)); + child_hash(child, cache).hash(&mut hasher); + } + TimeShift { shift, child } => { + hash_own_fields(&mut hasher, &("TimeShift", shift)); + child_hash(child, cache).hash(&mut hasher); + } + SQLWindowFunc { + func, + args, + partition_by, + order_by, + output_name, + child, + } => { + hash_own_fields( + &mut hasher, + &( + "SQLWindowFunc", + func, + args, + partition_by, + order_by, + output_name, + ), + ); + child_hash(child, cache).hash(&mut hasher); + } + BinaryOp { + op, + lhs, + rhs, + vector_match, + } => { + hash_own_fields(&mut hasher, &("BinaryOp", op, vector_match)); + child_hash(lhs, cache).hash(&mut hasher); + child_hash(rhs, cache).hash(&mut hasher); + } + // `QueryTimestamp`, `PromqlScalarBridge`, and the scalar variants + // (issue #205) are all leaves for this traversal's purposes — none + // has an operator child to look up in `cache` — so hashing the + // whole node via `serde_json` in one shot is already `O(node + // size)`, not `O(subtree size)`: exactly the same cost the + // per-variant `hash_own_fields` calls above pay, just without + // needing to spell out each field individually. Matches + // `rebuild_children`'s and `dag_node_count`'s identical scope + // decision for these variants ("never descended into"). + QueryTimestamp + | PromqlScalarBridge(_) + | Column(_) + | Literal(_) + | Compare { .. } + | BoolAnd(_) + | BoolOr(_) + | Not(_) + | IsNull(_) + | IsNotNull(_) + | Cast { .. } + | InList { .. } + | FunctionCall { .. } + | Arithmetic { .. } + | Case { .. } => hash_own_fields(&mut hasher, node), + } hasher.finish() } @@ -631,6 +863,83 @@ mod tests { ); } + // ── structural_hash (DAG-aware memoization) ───────────────────────── + + #[test] + fn structural_hash_is_stable_across_cache_states() { + // The hash of a given *value* must not depend on whether its cache + // started warm or cold — memoization changes how much work is + // redone, never what a node's hash actually is. + let agg = quantile_agg(vec![1], Some(2), 0.5); + let mut cold = HashMap::new(); + let mut warm = HashMap::new(); + // Prime `warm` with an unrelated node first, so it's non-empty but + // holds nothing relevant to `agg`. + structural_hash(&scan(), &mut warm); + assert_eq!( + structural_hash(&agg, &mut cold), + structural_hash(&agg, &mut warm), + "hash must be independent of unrelated cache state" + ); + } + + #[test] + fn structural_hash_of_an_internally_shared_tree_matches_the_unshared_equivalent() { + // The same BinaryOp-with-shared-branches shape as + // `dag_node_count_deduplicates_an_internally_shared_subtree` below: + // hashing it (however the memoization internally short-circuits the + // second branch) must produce the exact same value as hashing a + // structurally-identical tree built with *no* sharing at all — the + // whole point of memoization is not changing the answer, only the + // work needed to reach it. + let agg = quantile_agg(vec![1], Some(2), 0.5); + let shared_root = QueryExpr::BinaryOp { + op: BinaryOpKind::Compare(crate::pre_asap::expr_ir::CompareOpKind::Eq), + lhs: Rc::new(agg.clone()), + rhs: Rc::new(agg.clone()), + vector_match: None, + }; + let unshared_root = QueryExpr::BinaryOp { + op: BinaryOpKind::Compare(crate::pre_asap::expr_ir::CompareOpKind::Eq), + lhs: Rc::new(agg.clone()), + rhs: Rc::new(agg), // a second, independently-allocated Rc with an equal value + vector_match: None, + }; + let mut cache = HashMap::new(); + assert_eq!( + structural_hash(&shared_root, &mut cache), + structural_hash(&unshared_root, &mut HashMap::new()), + ); + } + + #[test] + fn structural_hash_memoizes_a_shared_descendant_exactly_once() { + // Direct proof the cache is actually doing its job: hashing a + // BinaryOp whose two branches are the *same* Rc (2 underlying + // nodes: Scan + Aggregate) should populate the cache with exactly + // 2 entries — the shared branch's nodes, cached once each when + // first reached — not a fresh entry (or a fresh, redundant + // recursive walk) for the second occurrence. + let agg = quantile_agg(vec![1], Some(2), 0.5); + let shared = Rc::new(agg); + let root = QueryExpr::BinaryOp { + op: BinaryOpKind::Compare(crate::pre_asap::expr_ir::CompareOpKind::Eq), + lhs: Rc::clone(&shared), + rhs: Rc::clone(&shared), + vector_match: None, + }; + let mut cache = HashMap::new(); + structural_hash(&root, &mut cache); + assert_eq!( + cache.len(), + 2, + "expected exactly one cache entry per unique node in the shared \ + branch (Aggregate + its Scan child), got {} entries: {:?}", + cache.len(), + cache + ); + } + // ── dag_node_count ─────────────────────────────────────────────────── #[test]