diff --git a/crates/types/src/dag_export.rs b/crates/types/src/dag_export.rs index 2f53fb7b..4bf7e066 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, HashCache}; 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, } @@ -69,19 +86,32 @@ 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 } } +/// 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, + 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(kind, &detail, &children, nodes); + let hash = structural_hash(expr, cache); nodes.push(DagNode { id, kind, @@ -93,25 +123,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(), @@ -129,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, @@ -142,7 +153,7 @@ fn build(expr: &QueryExpr, nodes: &mut Vec) -> u32 { "predicates": predicates, "schema": schema, }); - push_node(nodes, "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 @@ -153,6 +164,8 @@ fn build(expr: &QueryExpr, nodes: &mut Vec) -> u32 { let detail = serde_json::json!({ "value": inner }); push_node( nodes, + expr, + cache, "PromqlScalarBridge", format!("PromqlScalarBridge({inner:?})"), detail, @@ -161,15 +174,19 @@ 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!({}), @@ -177,9 +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!({}), @@ -187,10 +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, @@ -198,10 +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, @@ -209,10 +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, @@ -220,19 +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, "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, @@ -246,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, @@ -255,6 +290,8 @@ fn build(expr: &QueryExpr, nodes: &mut Vec) -> u32 { }); push_node( nodes, + expr, + cache, "Aggregate", format!("Aggregate({} measures)", measures.len()), detail, @@ -262,10 +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, @@ -273,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, "Concat", label, serde_json::json!({}), ids) + push_node( + nodes, + expr, + cache, + "Concat", + label, + serde_json::json!({}), + ids, + ) } QueryExpr::Join { kind, @@ -283,10 +330,18 @@ 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, "Join", format!("Join({kind:?})"), detail, vec![l, r]) + push_node( + nodes, + expr, + cache, + "Join", + format!("Join({kind:?})"), + detail, + vec![l, r], + ) } QueryExpr::SetOp { kind, @@ -294,11 +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, @@ -310,10 +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, @@ -321,19 +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, "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, @@ -341,10 +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, @@ -352,9 +423,17 @@ 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, "TimeShift", "TimeShift".into(), detail, vec![c]) + push_node( + nodes, + expr, + cache, + "TimeShift", + "TimeShift".into(), + detail, + vec![c], + ) } QueryExpr::SQLWindowFunc { func, @@ -364,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, @@ -374,6 +453,8 @@ fn build(expr: &QueryExpr, nodes: &mut Vec) -> u32 { }); push_node( nodes, + expr, + cache, "SQLWindowFunc", format!("SQLWindowFunc({func:?})"), detail, @@ -386,11 +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, @@ -552,4 +635,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, &mut HashCache::new()), + "dag_export's root hash must equal cse::structural_hash(&leaf, &mut HashCache::new()) 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, &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, &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 c807f534..0ec319d9 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 @@ -112,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(), } } @@ -126,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 @@ -146,21 +158,249 @@ 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 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 — 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 +/// 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. +/// +/// 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() } @@ -623,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] 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