Skip to content

Commit f2ceb1e

Browse files
zzylolclaude
andcommitted
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>
1 parent 5da7995 commit f2ceb1e

3 files changed

Lines changed: 247 additions & 27 deletions

File tree

‎crates/asap-aware-mapping/src/cost_model.rs‎

Lines changed: 73 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -85,33 +85,39 @@ pub enum ShareDecision {
8585
}
8686

8787
/// Default [`CostModel::cse_recompute_cost`]: a structural-size proxy — the
88-
/// length of `subtree`'s canonical JSON serialization (the same style of
89-
/// serialization `asap_types::pre_asap::cse`'s own structural hashing uses
90-
/// to compare subtrees). Cheap to compute, needs no tree traversal of its
91-
/// own, and scales with the subtree's real structural complexity — a genuinely
92-
/// tiny leaf costs little to recompute, a deep multi-join subtree costs a
93-
/// lot. A deployment with real per-row/per-update cost knowledge should
88+
/// number of *unique* nodes in `subtree`'s DAG
89+
/// ([`asap_types::pre_asap::cse::dag_node_count`], the same module this
90+
/// candidate's sharing was detected in). Deliberately **not** a raw
91+
/// `serde_json` serialization length: after CSE, `subtree` is generally a
92+
/// DAG, not a tree (a `CseCandidate` only exists because something got
93+
/// shared), and a naive full serialization re-serializes — over-counts —
94+
/// any descendant `subtree` already shares internally, once per parent
95+
/// that references it, instead of once for the whole DAG. `dag_node_count`
96+
/// dedupes by `Rc` pointer identity, so it charges each unique node's
97+
/// contribution exactly once regardless of how many places within
98+
/// `subtree` reference it. Cheap to compute (one pass, no serialization),
99+
/// and still scales with real structural complexity — a genuinely tiny
100+
/// leaf costs little to recompute, a deep multi-join subtree costs a lot.
101+
/// A deployment with real per-row/per-update cost knowledge should
94102
/// override [`CostModel::cse_recompute_cost`] instead of relying on this.
95103
pub fn default_cse_recompute_cost(subtree: &QueryExpr) -> f64 {
96-
serde_json::to_string(subtree)
97-
.map(|s| s.len() as f64)
98-
.unwrap_or(1.0)
99-
.max(1.0)
104+
asap_types::pre_asap::cse::dag_node_count(subtree) as f64
100105
}
101106

102107
/// Default [`CostModel::cse_shared_maintenance_cost`]: a small
103108
/// per-[`SummaryFamilyType`] weight, scaled to the same order of magnitude
104-
/// as [`default_cse_recompute_cost`]'s typical output, reflecting that
105-
/// families differ in how expensive they are to keep *continuously
106-
/// updated* for the life of a workload — an exact accumulator is the
107-
/// cheapest (an O(1) merge), sketches/samples cost more (a whole data
108-
/// structure to update per new row), wavelets/fitted models cost the most
109-
/// (coefficient/parameter maintenance). These weights are illustrative, not
110-
/// measured — a deployment with real memory/update-cost numbers should
111-
/// override [`CostModel::cse_shared_maintenance_cost`] instead of relying on
112-
/// this table.
109+
/// as [`default_cse_recompute_cost`]'s typical output (a small node
110+
/// count, not a byte length), reflecting that families differ in how
111+
/// expensive they are to keep *continuously updated* for the life of a
112+
/// workload — an exact accumulator is the cheapest (an O(1) merge),
113+
/// sketches/samples cost more (a whole data structure to update per new
114+
/// row), wavelets/fitted models cost the most (coefficient/parameter
115+
/// maintenance). These weights are illustrative, not measured — a
116+
/// deployment with real memory/update-cost numbers should override
117+
/// [`CostModel::cse_shared_maintenance_cost`] instead of relying on this
118+
/// table.
113119
pub fn default_cse_shared_maintenance_cost(family: &SummaryFamilyType) -> f64 {
114-
const UNIT: f64 = 60.0;
120+
const UNIT: f64 = 1.0;
115121
let weight = match family {
116122
SummaryFamilyType::Plain(_) => 1.0,
117123
SummaryFamilyType::ExactAggregate(..) => 1.0,
@@ -423,6 +429,49 @@ mod tests {
423429
assert!(default_cse_recompute_cost(&nested) > default_cse_recompute_cost(&leaf));
424430
}
425431

432+
/// The DAG-awareness this proxy exists for: a subtree that internally
433+
/// re-references one shared descendant (e.g. after single-query CSE,
434+
/// `x op x` collapsing both branches onto one `Rc`) must cost the same
435+
/// as if that descendant only appeared once — not double, the way a
436+
/// naive tree-shaped size measure (a full serialization, or an
437+
/// identity-blind recursive walk) would count it.
438+
#[test]
439+
fn default_recompute_cost_does_not_double_count_an_internally_shared_descendant() {
440+
use asap_types::pre_asap::expr_ir::ScalarValue;
441+
use asap_types::pre_asap::query_expr::{JoinKind, Predicate};
442+
443+
let true_pred = || {
444+
Predicate(std::rc::Rc::new(QueryExpr::Literal(ScalarValue::Boolean(
445+
true,
446+
))))
447+
};
448+
let shared_leaf = std::rc::Rc::new(scan());
449+
let no_sharing = QueryExpr::Join {
450+
kind: JoinKind::Inner,
451+
pred: true_pred(),
452+
left: std::rc::Rc::new(scan()),
453+
right: std::rc::Rc::new(scan()),
454+
};
455+
let with_sharing = QueryExpr::Join {
456+
kind: JoinKind::Inner,
457+
pred: true_pred(),
458+
left: std::rc::Rc::clone(&shared_leaf),
459+
right: std::rc::Rc::clone(&shared_leaf),
460+
};
461+
assert_eq!(
462+
default_cse_recompute_cost(&no_sharing),
463+
3.0,
464+
"no sharing: Join + 2 independent Scans = 3 unique nodes"
465+
);
466+
assert_eq!(
467+
default_cse_recompute_cost(&with_sharing),
468+
2.0,
469+
"internal sharing: Join + 1 shared Scan (referenced twice) = \
470+
2 unique nodes, not 3 — a tree-shaped size measure would \
471+
wrongly charge for the shared Scan twice"
472+
);
473+
}
474+
426475
#[test]
427476
fn default_shared_maintenance_cost_orders_families_cheapest_to_priciest() {
428477
let exact = default_cse_shared_maintenance_cost(&SummaryFamilyType::ExactAggregate(
@@ -468,10 +517,10 @@ mod tests {
468517
family: "gaussian_mixture".into(),
469518
},
470519
)),
471-
// A single, cheap-to-recompute leaf against an
472-
// expensive-to-maintain family: maintenance should dominate
473-
// (scan()'s recompute cost, 264, is well under StatModel's
474-
// maintenance cost, 360).
520+
// A single, cheap-to-recompute leaf (scan() alone is 1 DAG
521+
// node, recompute_total = 1) against an expensive-to-maintain
522+
// family (StatModel, maintenance cost 6.0): maintenance should
523+
// dominate.
475524
consumer_count: 1,
476525
};
477526
assert_eq!(

‎crates/types/src/pre_asap/cse.rs‎

Lines changed: 165 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -164,6 +164,110 @@ fn structural_hash(node: &QueryExpr) -> u64 {
164164
hasher.finish()
165165
}
166166

167+
/// Count of *unique* nodes reachable from `root`, deduplicated by `Rc`
168+
/// pointer identity (`Rc::as_ptr`) — the real size of the DAG rooted at
169+
/// `root`, not a tree-walk count.
170+
///
171+
/// After [`share_common_subtrees`] runs (or even before it, for a tree a
172+
/// front end already built with internal `Rc` sharing — e.g. re-running
173+
/// CSE, or a single-query repeated subexpression), `root` is generally a
174+
/// **DAG**, not a tree — that is this whole module's premise. Anything that
175+
/// walks `root` as if every reference were a fresh subtree (a naive
176+
/// recursive walk with no identity tracking, or a naive full
177+
/// `serde_json` serialization — `Rc`'s `Serialize` impl serializes the
178+
/// pointee's *value* at every occurrence, it does not dedupe by identity)
179+
/// re-visits/re-counts an already-shared descendant once per parent that
180+
/// references it, over-counting relative to the actual work of holding it
181+
/// in memory or recomputing it once. This function is the DAG-correct
182+
/// alternative: each unique node is counted exactly once, regardless of
183+
/// how many places within `root` reference it.
184+
///
185+
/// `pub` so cost-aware callers outside this crate (e.g.
186+
/// `asap_aware_mapping::CostModel::cse_recompute_cost`'s default) have a
187+
/// DAG-correct structural-size proxy available, instead of reaching for
188+
/// something tree-shaped like a raw serialization length.
189+
///
190+
/// Same operator-child traversal scope as [`share_common_subtrees`] itself
191+
/// (see the module doc's "Algorithm" section, and this module's private
192+
/// `rebuild_children`) — a scalar subexpression embedded in a wrapper
193+
/// position (`Predicate`, `ProjectItem.expr`, `Aggregate.having`, …) is not
194+
/// separately visited, matching this module's own stated scope; it's
195+
/// counted as part of its owning operator node, the same node
196+
/// `rebuild_children` treats as a single opaque leaf for interning
197+
/// purposes.
198+
pub fn dag_node_count(root: &QueryExpr) -> usize {
199+
let mut seen: std::collections::HashSet<*const QueryExpr> = std::collections::HashSet::new();
200+
count_unique(root, &mut seen)
201+
}
202+
203+
/// One node's own contribution (`1`) plus each *not-yet-seen* operator
204+
/// child's contribution — exhaustive over every `QueryExpr` variant,
205+
/// enumerating the same fields [`rebuild_children`] does (kept as a
206+
/// separate, read-only traversal rather than threaded through
207+
/// `rebuild_children` itself, since that function consumes and rebuilds
208+
/// its input while this one only ever reads it).
209+
fn count_unique(node: &QueryExpr, seen: &mut std::collections::HashSet<*const QueryExpr>) -> usize {
210+
use QueryExpr::*;
211+
212+
/// Visit one `Rc`-held child: counts (and recurses into) it only the
213+
/// first time its pointer is seen, `0` on every later occurrence —
214+
/// this is the actual dedup step.
215+
fn visit(
216+
child: &Rc<QueryExpr>,
217+
seen: &mut std::collections::HashSet<*const QueryExpr>,
218+
) -> usize {
219+
if seen.insert(Rc::as_ptr(child)) {
220+
count_unique(child, seen)
221+
} else {
222+
0
223+
}
224+
}
225+
226+
1 + match node {
227+
Scan { .. } | PromqlScalar(_) | QueryTimestamp => 0,
228+
PromqlVectorFromScalar(c) | PromqlScalarFromVector(c) => visit(c, seen),
229+
PromqlRelabel { child, .. }
230+
| PromqlInfoEnrich { child, .. }
231+
| PromqlSeriesSample { child, .. }
232+
| Filter { child, .. }
233+
| Project { child, .. }
234+
| Aggregate { child, .. }
235+
| Dedup { child, .. }
236+
| Sort { child, .. }
237+
| Limit { child, .. }
238+
| PromqlSubquery { child, .. }
239+
| TimeRange { child, .. }
240+
| TimeShift { child, .. }
241+
| SQLWindowFunc { child, .. } => visit(child, seen),
242+
// `Concat`'s branches are stored by value (`Vec<QueryExpr>`, not
243+
// `Rc<QueryExpr>` — see `rebuild_children`'s `intern_owned` use for
244+
// this variant), so a branch has no `Rc` identity of its own to
245+
// dedup on at this position; still recurse into each in case an
246+
// `Rc`-shared descendant appears further down.
247+
Concat { children } => children.iter().map(|c| count_unique(c, seen)).sum(),
248+
Join { left, right, .. } | SetOp { left, right, .. } => {
249+
visit(left, seen) + visit(right, seen)
250+
}
251+
BinaryOp { lhs, rhs, .. } => visit(lhs, seen) + visit(rhs, seen),
252+
// Scalar variants (issue #205) — never descended into, matching
253+
// `rebuild_children`'s own scope exactly (see its trailing match
254+
// arm and this module's "Algorithm" section).
255+
Column(_)
256+
| Literal(_)
257+
| Compare { .. }
258+
| BoolAnd(_)
259+
| BoolOr(_)
260+
| Not(_)
261+
| IsNull(_)
262+
| IsNotNull(_)
263+
| Cast { .. }
264+
| InList { .. }
265+
| FunctionCall { .. }
266+
| Arithmetic { .. }
267+
| Case { .. } => 0,
268+
}
269+
}
270+
167271
/// Recurse into `child`, then intern the result. `Rc::try_unwrap` recovers
168272
/// the owned node without cloning in the overwhelmingly common case — a
169273
/// tree freshly built by a front end / `resolve_root`, not yet shared by any
@@ -516,6 +620,67 @@ mod tests {
516620
);
517621
}
518622

623+
// ── dag_node_count ───────────────────────────────────────────────────
624+
625+
#[test]
626+
fn dag_node_count_is_the_naive_count_when_nothing_is_shared() {
627+
// scan() alone: 1 node.
628+
assert_eq!(dag_node_count(&scan()), 1);
629+
// quantile_agg's own child is a fresh, unshared scan(): 2 nodes.
630+
assert_eq!(dag_node_count(&quantile_agg(vec![1], Some(2), 0.5)), 2);
631+
}
632+
633+
#[test]
634+
fn dag_node_count_deduplicates_an_internally_shared_subtree() {
635+
// Same shape as `single_query_shares_its_own_repeated_subtree`: a
636+
// BinaryOp whose two branches are the *same* Rc after
637+
// `share_common_subtrees` (2 nodes: Scan + Aggregate) — the root
638+
// itself makes 3 unique nodes total (BinaryOp, Aggregate, Scan),
639+
// not 5 (which a tree-walk / naive serialization, counting the
640+
// shared branch's 2 nodes twice, would report).
641+
let agg = quantile_agg(vec![1], Some(2), 0.5);
642+
let root = QueryExpr::BinaryOp {
643+
op: BinaryOpKind::Compare(crate::pre_asap::expr_ir::CompareOpKind::Eq),
644+
lhs: Rc::new(agg.clone()),
645+
rhs: Rc::new(agg),
646+
vector_match: None,
647+
};
648+
let shared = share_common_subtrees(vec![("q", root)]);
649+
let [(_, root)] = shared.as_slice() else {
650+
panic!("expected 1 root");
651+
};
652+
assert_eq!(
653+
dag_node_count(root),
654+
3,
655+
"the shared branch's 2 nodes must be counted once, not once per \
656+
occurrence — got {} for {root:?}",
657+
dag_node_count(root)
658+
);
659+
}
660+
661+
#[test]
662+
fn dag_node_count_deduplicates_across_two_workload_roots() {
663+
// Two workload roots sharing one Aggregate after
664+
// `share_common_subtrees` (the `duplicate_workload_queries_...`
665+
// shape from `crates/integration-tests/tests/cse.rs`, built
666+
// directly here): each root's own `dag_node_count` must report the
667+
// shared subtree's real size once, not double-count anything —
668+
// there's nothing *to* double-count from a single root's own count
669+
// in this case (no root references the shared node twice), so this
670+
// pins the simpler, more common case that a per-candidate cost
671+
// proxy (`CseCandidate::subtree` in `asap-aware-mapping`) actually
672+
// exercises: counting one occurrence's own reachable DAG size.
673+
let a = quantile_agg(vec![1], Some(2), 0.5);
674+
let b = quantile_agg(vec![1], Some(2), 0.5);
675+
let shared = share_common_subtrees(vec![("a", a), ("b", b)]);
676+
let [(_, ra), (_, rb)] = shared.as_slice() else {
677+
panic!("expected 2 roots");
678+
};
679+
assert!(Rc::ptr_eq(ra, rb), "fixture sanity: the two roots merged");
680+
assert_eq!(dag_node_count(ra), 2);
681+
assert_eq!(dag_node_count(rb), 2);
682+
}
683+
519684
#[test]
520685
fn dedup_gates_sharing_the_same_as_aggregate() {
521686
// `Dedup { cols }` adds `cols` as a unique key — so two identical

‎docs/cse-cost-model-decision.md‎

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -80,9 +80,15 @@ independently (`RecomputeIndependently`) per that one decision.
8080

8181
## Defaults
8282

83-
`cse_recompute_cost`'s default is a structural-size proxy (the length of the
84-
same canonical-JSON serialization `cse::structural_hash` already computes for
85-
the subtree — no second traversal). `cse_shared_maintenance_cost`'s default
83+
`cse_recompute_cost`'s default is a structural-size proxy: `cse::dag_node_count`,
84+
the number of *unique* nodes in the subtree's DAG (deduplicated by `Rc`
85+
pointer identity), not a raw serialization length. This distinction matters
86+
here specifically — a `CseCandidate`'s subtree is, by definition, something
87+
CSE already found sharing in, so it's generally a DAG, not a tree; a naive
88+
tree-shaped size measure (a full `serde_json` serialization, or a recursive
89+
walk with no identity tracking) would re-count any descendant the subtree
90+
already shares internally once per parent that reaches it, over-stating the
91+
real cost of holding or recomputing it once. `cse_shared_maintenance_cost`'s default
8692
is a small per-`SummaryFamilyType` weight table (exact accumulators cheapest,
8793
sketches/samples/wavelets/stat-models progressively more expensive to keep
8894
continuously updated) scaled to the same order of magnitude as typical

0 commit comments

Comments
 (0)