From a3c95fdc2a9e42dc3dc83dbbaa10011807feb6b9 Mon Sep 17 00:00:00 2001 From: zz_y Date: Wed, 26 Aug 2026 13:43:59 -0600 Subject: [PATCH 1/3] feat(plan): let CostModel opt AggIntent::TopK{accuracy:Exact} into sketch candidates (#151) `implementations_for_with`'s `AccuracyTarget::Exact` arm previously short-circuited every approximate-capable intent straight to `exact_realization`, which for `TopK` always meant `PassThrough` -- there's no exact mergeable top-k accumulator, so no `CostModel` hook was ever consulted for that shape. Add `CostModel::topk_exact_offers_sketch(&self, intent) -> bool` (default `false`) as the opt-in gate. When a deployment's `CostModel` returns `true` for a `TopK { accuracy: Exact, .. }` intent, `implementations_for_with` routes it through the same `sketch_implementations` pipeline an approximate `TopK` request uses -- `summary_candidates` for the CmsWithHeap/CountSketchWithHeap family, ranked via `rank_candidates` and sized via `size_params` at the tightest budget `AccuracyTarget::Exact` degrades to -- instead of inventing a parallel candidate-generation path. Default-preserves-behavior guarantee: the new method has a provided default body (`false`), so every existing `CostModel` implementation (including ones that only override the required `rank_candidates`) keeps today's `PassThrough` behavior for `TopK { accuracy: Exact, .. }` byte for byte, with no changes required on their part. `AggIntent::Count { accuracy: Exact }` and the rest of the Exact arm are untouched -- only `TopK` is gated by the new hook. Tests (crates/asap-aware-mapping/src/replacement.rs): - topk_exact_default_cost_model_still_passes_through: (a) no regression - topk_exact_opted_in_cost_model_offers_sketch_candidates: (b) opt-in CostModel gets a real, ranked, sized sketch candidate - count_exact_unaffected_by_topk_exact_opt_in: (c) Count{Exact} unaffected, even against a CostModel that opts every intent in Co-Authored-By: Claude Sonnet 5 --- crates/asap-aware-mapping/src/cost_model.rs | 28 ++++ crates/asap-aware-mapping/src/replacement.rs | 154 ++++++++++++++++++- 2 files changed, 178 insertions(+), 4 deletions(-) diff --git a/crates/asap-aware-mapping/src/cost_model.rs b/crates/asap-aware-mapping/src/cost_model.rs index 82a3e97..939cf9a 100644 --- a/crates/asap-aware-mapping/src/cost_model.rs +++ b/crates/asap-aware-mapping/src/cost_model.rs @@ -223,6 +223,34 @@ pub trait CostModel { crate::replacement::default_size_params(kind, intent, eps, delta) } + /// Whether an `AggIntent::TopK { accuracy: Exact, .. }` request should + /// still be offered [`summary_candidates`](crate::replacement::summary_candidates)'s + /// sketch family (`CmsWithHeap`/`CountSketchWithHeap`) via + /// [`rank_candidates`](Self::rank_candidates)/[`size_params`](Self::size_params) + /// — sized at the tightest accuracy budget `AccuracyTarget::Exact` + /// resolves to (see `replacement::accuracy_budget`) — instead of + /// `implementations_for_with` declining straight to + /// `Implementation::PassThrough` (issue #151). + /// + /// There is no *exact* mergeable top-k accumulator (unlike `Count`), so + /// `asap-plan` itself has no way to decide between "no summary at all" + /// and "the closest available approximation" for an `Exact`-accuracy + /// top-k request — that's a deployment policy call. A deployment that + /// wants the latter overrides this to return `true` (optionally only + /// for the specific `TopK` shapes it cares about); `rank_candidates` + /// and `size_params` are then consulted exactly as they are for any + /// other top-k sketch candidate, so a `CostModel` gets one place to + /// answer both "should this even be considered" and "which one, sized + /// how" instead of reimplementing the same pre-pass client-side. + /// + /// Default: `false` — preserves today's `PassThrough` behavior for + /// every deployment that doesn't override this, including one whose own + /// `rank_candidates` override doesn't special-case `TopK`'s `Exact` + /// case (it is never consulted here unless this method also says yes). + fn topk_exact_offers_sketch(&self, _intent: &AggIntent) -> bool { + false + } + /// Estimated number of distinct subpopulations produced by `target`'s /// grouping keys. `None` means the deployment has no cardinality estimate; /// grouping alternatives remain legal but keep their discovery order. diff --git a/crates/asap-aware-mapping/src/replacement.rs b/crates/asap-aware-mapping/src/replacement.rs index e18a9a5..117f64a 100644 --- a/crates/asap-aware-mapping/src/replacement.rs +++ b/crates/asap-aware-mapping/src/replacement.rs @@ -668,6 +668,20 @@ pub(crate) fn implementations_for_with( | AggIntent::Cardinality { accuracy, .. } | AggIntent::Count { accuracy } | AggIntent::TopK { accuracy, .. } => match accuracy { + // `TopK` is the one approximate-capable intent whose `Exact` + // case a `CostModel` may still want a sketch candidate for + // (issue #151, see `CostModel::topk_exact_offers_sketch`'s + // docs) — there's no *exact* mergeable top-k accumulator + // (unlike `Count`), so "closest available approximation" is a + // real, deployment-chosen alternative to `PassThrough`, not a + // fallback core can pick on its own. Quantile/Cardinality/Count + // still route through `exact_realization` unconditionally. + AccuracyTarget::Exact + if matches!(intent, AggIntent::TopK { .. }) + && cost_model.topk_exact_offers_sketch(intent) => + { + sketch_implementations(intent, accuracy, cost_model) + } AccuracyTarget::Exact => vec![exact_realization(intent)], _ => sketch_implementations(intent, accuracy, cost_model), }, @@ -758,7 +772,10 @@ pub(crate) fn implementations_for_with( /// Exact realization of an approximate-capable intent whose target is /// `AccuracyTarget::Exact`. `Count` has a mergeable exact accumulator; exact /// quantile / top-k / cardinality have no single-value summary form (they -/// need the full multiset / heap / set) and pass through. +/// need the full multiset / heap / set) and pass through by default. A +/// `CostModel` can opt a `TopK { accuracy: Exact, .. }` request out of this +/// function entirely — see [`CostModel::topk_exact_offers_sketch`] and +/// [`implementations_for_with`]'s `AccuracyTarget::Exact` arm (issue #151). fn exact_realization(intent: &AggIntent) -> Implementation { match intent { AggIntent::Count { .. } => exact_accumulator(intent, ExactKind::Count, ExactParams::Count), @@ -781,9 +798,11 @@ fn exact_accumulator(intent: &AggIntent, kind: ExactKind, params: ExactParams) - /// this crate's own sizing — one place this resolution happens, so nothing /// can drift apart on it. /// -/// `Exact` is unreachable via [`implementations_for_with`] (which routes -/// `Exact` to [`exact_realization`] instead); degrades to the tightest -/// parameters for a caller that resolves it directly anyway. +/// `Exact` is unreachable via [`implementations_for_with`] for every +/// approximate-capable intent except `TopK` under an opted-in `CostModel` +/// (issue #151, [`CostModel::topk_exact_offers_sketch`]) — that path sizes +/// its sketch candidate(s) at the tightest budget this degrades to, the same +/// as a caller that resolves `Exact` directly. pub fn accuracy_budget(accuracy: &AccuracyTarget) -> (f64, f64) { match accuracy { AccuracyTarget::Exact => (f64::MIN_POSITIVE, DEFAULT_DELTA), @@ -3181,6 +3200,133 @@ mod tests { assert_eq!(kinds, vec![SketchAlgorithm::Kll, SketchAlgorithm::DDSketch]); } + // ── TopK{accuracy: Exact} sketch opt-in (issue #151) ──────────────── + + /// A `CostModel` that opts every `TopK { accuracy: Exact, .. }` request + /// into the sketch family via `topk_exact_offers_sketch`, otherwise + /// behaving exactly like `DefaultCostModel`. + struct OffersSketchForExactTopK; + + impl CostModel for OffersSketchForExactTopK { + fn rank_candidates( + &self, + _intent: &AggIntent, + candidates: &[SketchAlgorithm], + ) -> Vec { + candidates.to_vec() + } + + fn topk_exact_offers_sketch(&self, intent: &AggIntent) -> bool { + matches!(intent, AggIntent::TopK { .. }) + } + } + + fn topk_exact(k: usize) -> AggIntent { + AggIntent::TopK { + k, + accuracy: AccuracyTarget::Exact, + } + } + + /// (a) No regression: with no `CostModel` overriding + /// `topk_exact_offers_sketch`, `TopK { accuracy: Exact, .. }` still + /// resolves to `PassThrough` — `implementations_for_with`'s + /// `AccuracyTarget::Exact` arm now *consults* a `CostModel` hook before + /// declining, but `DefaultCostModel`'s default (`false`) declines + /// exactly like the old unconditional short-circuit did. + #[test] + fn topk_exact_default_cost_model_still_passes_through() { + assert_eq!(preferred(&topk_exact(10)), Implementation::PassThrough); + // Exhaustive too, not just the head of the list. + assert_eq!( + implementations_for_with(&topk_exact(10), &DefaultCostModel), + vec![Implementation::PassThrough] + ); + } + + /// (b) A `CostModel` that opts in via `topk_exact_offers_sketch` gets a + /// real sketch candidate — ranked via `rank_candidates` and sized via + /// `size_params` exactly like an approximate `TopK` request, at the + /// tightest budget `AccuracyTarget::Exact` resolves to (issue #151). + #[test] + fn topk_exact_opted_in_cost_model_offers_sketch_candidates() { + let intent = topk_exact(10); + let candidates = implementations_for_with(&intent, &OffersSketchForExactTopK); + // Same candidate family/order as an approximate TopK request would + // get: [CmsWithHeap, CountSketchWithHeap]. + let kinds: Vec = candidates + .iter() + .map(|implementation| match implementation { + Implementation::Sketch(kind) => kind.algorithm().clone(), + other => panic!("expected Sketch, got {other:?}"), + }) + .collect(); + assert_eq!( + kinds, + vec![ + SketchAlgorithm::CmsWithHeap, + SketchAlgorithm::CountSketchWithHeap + ] + ); + // The preferred (first) candidate is a real, heap-sized sketch — + // not PassThrough. + match implementations_for_with(&intent, &OffersSketchForExactTopK) + .into_iter() + .next() + .unwrap() + { + Implementation::Sketch(kind) => { + assert_eq!(kind.algorithm(), &SketchAlgorithm::CmsWithHeap); + let SketchParams::CmsWithHeap { heap_size, .. } = kind.params() else { + unreachable!("SketchKind validates CmsWithHeap params") + }; + assert_eq!(*heap_size, 10); + } + other => panic!("expected Sketch, got {other:?}"), + } + } + + /// (c) `Count { accuracy: Exact }` is unaffected by the `TopK`-specific + /// opt-in: it keeps routing through `exact_realization`'s mergeable + /// accumulator regardless of what a `CostModel` says about + /// `topk_exact_offers_sketch` (which only ever gates the `TopK` arm). + #[test] + fn count_exact_unaffected_by_topk_exact_opt_in() { + let intent = AggIntent::Count { + accuracy: AccuracyTarget::Exact, + }; + assert_eq!( + implementations_for_with(&intent, &DefaultCostModel), + vec![Implementation::ExactAggregate { + kind: ExactKind::Count, + params: ExactParams::Count, + }] + ); + // Even a CostModel that unconditionally opts every intent into the + // TopK-exact sketch path leaves Count's exact accumulator alone — + // `topk_exact_offers_sketch` is never even consulted for it. + struct OptsEverythingIn; + impl CostModel for OptsEverythingIn { + fn rank_candidates( + &self, + _intent: &AggIntent, + candidates: &[SketchAlgorithm], + ) -> Vec { + candidates.to_vec() + } + fn topk_exact_offers_sketch(&self, _intent: &AggIntent) -> bool { + true + } + } + assert_eq!( + implementations_for_with(&intent, &OptsEverythingIn), + vec![Implementation::ExactAggregate { + kind: ExactKind::Count, + params: ExactParams::Count, + }] + ); + } + #[test] fn degenerate_epsilon_saturates_to_tightest_params() { let intent = AggIntent::Quantile { From 853fe84c9907e40e7093cbbe7617e4d77d533063 Mon Sep 17 00:00:00 2001 From: zz_y Date: Thu, 27 Aug 2026 09:28:44 -0600 Subject: [PATCH 2/3] fix(plan): size opted-in TopK{Exact} sketches at a real budget, keep PassThrough (#151) Code review of #293 found two bugs in the CostModel opt-in for AggIntent::TopK { accuracy: Exact, .. }: 1. Sketches were sized via accuracy_budget(Exact), which resolves to (f64::MIN_POSITIVE, DEFAULT_DELTA) -- cms_width saturates that to its clamp maximum (1<<26). A CmsWithHeap/CountSketchWithHeap candidate under that budget is 67,108,864 x 5 = 335,544,320 counters (~1.3-2.7 GB) per instance, multiplied per subpopulation for a grouped top-k. Both this crate's SQL and PromQL frontends default to Exact when no accuracy is specified, so every un-annotated topk query in an opted-in deployment would have gotten the largest possible sketch. 2. The opt-in replaced PassThrough instead of adding sketch candidates alongside it, pruning a candidate implementations_for_with's own docs (and rank_candidates' contract) call exhaustive/never-prune. Fix: - Replaced `CostModel::topk_exact_offers_sketch(&self, intent) -> bool` with `CostModel::topk_exact_accuracy_target(&self, intent) -> Option`. `None` (the default) preserves today's PassThrough-only behavior exactly. `Some(target)` supplies the real budget to size at -- the hook itself documents why `target` must never be `AccuracyTarget::Exact` again (same clamp-saturation problem, now an explicit deployment choice instead of a silent default). - `topk_exact_realization` (new, replacing the inline match arm) now offers the target-sized, rank_candidates-ranked sketch candidate(s) *ahead of* PassThrough, not instead of it -- PassThrough stays in the returned Vec unconditionally. - Updated cost_model.rs/replacement.rs doc comments accordingly. Tests (crates/asap-aware-mapping/src/replacement.rs): - topk_exact_default_cost_model_still_passes_through: now also checks an explicit `None` behaves identically to the default. - topk_exact_opted_in_cost_model_offers_sketch_candidates: sizes at a sane eps=0.1 target, asserts PassThrough is still present (not pruned) and last, and pins width=28/depth=5 explicitly so a regression back to the clamp-saturated (1<<26, ..) is caught by a failing assertion instead of going unpinned. - count_exact_unaffected_by_topk_exact_opt_in: unchanged coverage, updated to the new hook signature. - Consolidated the two near-duplicate test CostModel impls into one parameterized `TopkExactCostModel { target: Option }`. Also addressed: - explanation.rs: added topk_exact_opted_in_cost_model_reports_a_sketch_applicability_finding, confirming an opted-in TopK{Exact} candidate is correctly reported as a SketchApproximation finding (no code change needed -- the module's existing "does this candidate list contain anything beyond the trivial realization" framing already covers this once the candidate is generated); scoped exact_quantile_is_not_a_sketch_applicability_finding's doc comment to note this one exception. - docs/developer_docs/ASAP-aware-mapping-developer-guide.md: added `topk_exact_accuracy_target` to the CostModel hook table, a full "Which hook" subsection, the quick-reference table, and corrected the "single candidate" claim for exact/pass-through realizations. - docs/user-guide/user-guide.md: added the new hook to the CostModel hooks mentioned there. cargo build --workspace, cargo test --workspace, cargo clippy --workspace --all-targets -- -D warnings, and cargo fmt --all -- --check all pass clean. Co-Authored-By: Claude Sonnet 5 --- crates/asap-aware-mapping/src/cost_model.rs | 52 +++-- crates/asap-aware-mapping/src/explanation.rs | 58 ++++++ crates/asap-aware-mapping/src/replacement.rs | 192 ++++++++++++------ .../ASAP-aware-mapping-developer-guide.md | 41 +++- docs/user-guide/user-guide.md | 2 +- 5 files changed, 262 insertions(+), 83 deletions(-) diff --git a/crates/asap-aware-mapping/src/cost_model.rs b/crates/asap-aware-mapping/src/cost_model.rs index 939cf9a..2a7fa27 100644 --- a/crates/asap-aware-mapping/src/cost_model.rs +++ b/crates/asap-aware-mapping/src/cost_model.rs @@ -55,6 +55,7 @@ use asap_types::post_asap::{ use asap_types::pre_asap::agg_intent::AggIntent; use asap_types::pre_asap::expr_ir::ColumnRef; use asap_types::pre_asap::query_expr::QueryExpr; +use asap_types::types::AccuracyTarget; use crate::replacement::{ realize_child, Implementation, Replacement, ReplacementSubDAG, TargetSubDAG, @@ -223,32 +224,45 @@ pub trait CostModel { crate::replacement::default_size_params(kind, intent, eps, delta) } - /// Whether an `AggIntent::TopK { accuracy: Exact, .. }` request should - /// still be offered [`summary_candidates`](crate::replacement::summary_candidates)'s - /// sketch family (`CmsWithHeap`/`CountSketchWithHeap`) via - /// [`rank_candidates`](Self::rank_candidates)/[`size_params`](Self::size_params) - /// — sized at the tightest accuracy budget `AccuracyTarget::Exact` - /// resolves to (see `replacement::accuracy_budget`) — instead of - /// `implementations_for_with` declining straight to - /// `Implementation::PassThrough` (issue #151). + /// The accuracy budget to size a sketch candidate at for an + /// `AggIntent::TopK { accuracy: Exact, .. }` request, or `None` to + /// decline entirely (issue #151). /// /// There is no *exact* mergeable top-k accumulator (unlike `Count`), so /// `asap-plan` itself has no way to decide between "no summary at all" /// and "the closest available approximation" for an `Exact`-accuracy - /// top-k request — that's a deployment policy call. A deployment that - /// wants the latter overrides this to return `true` (optionally only - /// for the specific `TopK` shapes it cares about); `rank_candidates` - /// and `size_params` are then consulted exactly as they are for any - /// other top-k sketch candidate, so a `CostModel` gets one place to - /// answer both "should this even be considered" and "which one, sized - /// how" instead of reimplementing the same pre-pass client-side. + /// top-k request — that's a deployment policy call, and if it opts in, + /// *what budget to size at* is also a deployment call: `AccuracyTarget + /// ::Exact` itself resolves (via `replacement::accuracy_budget`) to the + /// tightest parameters every sketch family's sizing formula clamps to + /// (e.g. `cms_width`'s `1 << 26`), which is not a meaningful target for + /// any real deployment — sizing an opted-in candidate at that clamp + /// would silently produce the largest possible sketch (hundreds of MB + /// to GB per instance, multiplied per subpopulation for a grouped + /// top-k) for every un-annotated top-k query, since both this crate's + /// SQL and PromQL frontends default to `Exact` when no accuracy is + /// specified. Returning `Some` here instead requires the deployment to + /// state the budget it actually wants (e.g. + /// `Some(AccuracyTarget::Epsilon(1.0 / k as f64))`, or a fixed + /// deployment-wide epsilon) — [`rank_candidates`](Self::rank_candidates) + /// and [`size_params`](Self::size_params) are then consulted exactly as + /// they are for any other top-k sketch candidate, sized against that + /// returned target rather than `Exact`'s own degenerate one. /// - /// Default: `false` — preserves today's `PassThrough` behavior for + /// The sketch candidate(s) this produces are offered *alongside* + /// `Implementation::PassThrough`, not instead of it — `PassThrough` + /// stays in the exhaustive candidate set `implementations_for_with` + /// returns; ranking (via `rank_candidates`, and ultimately whichever + /// candidate a caller commits to) decides which one wins, the same as + /// every other exhaustive-then-ranked candidate set this crate builds. + /// + /// Default: `None` — preserves today's `PassThrough`-only behavior for /// every deployment that doesn't override this, including one whose own /// `rank_candidates` override doesn't special-case `TopK`'s `Exact` - /// case (it is never consulted here unless this method also says yes). - fn topk_exact_offers_sketch(&self, _intent: &AggIntent) -> bool { - false + /// case (it is never consulted here unless this method also returns + /// `Some`). + fn topk_exact_accuracy_target(&self, _intent: &AggIntent) -> Option { + None } /// Estimated number of distinct subpopulations produced by `target`'s diff --git a/crates/asap-aware-mapping/src/explanation.rs b/crates/asap-aware-mapping/src/explanation.rs index bcfee70..2cd0bd3 100644 --- a/crates/asap-aware-mapping/src/explanation.rs +++ b/crates/asap-aware-mapping/src/explanation.rs @@ -541,6 +541,13 @@ mod tests { ); } + /// True for every `AggIntent` this crate has no opt-in sketch path for + /// under an `Exact` accuracy target, and for `TopK { accuracy: Exact, + /// .. }` specifically under the *default* `CostModel` (no + /// `topk_exact_accuracy_target` override) — see + /// `topk_exact_opted_in_cost_model_reports_a_sketch_applicability_finding` + /// below for the one case (an opted-in `CostModel`, issue #151) where an + /// `Exact` accuracy target *does* report sketch-applicability. #[test] fn exact_quantile_is_not_a_sketch_applicability_finding() { let q = agg( @@ -561,6 +568,57 @@ mod tests { ); } + /// The one exception to the invariant above (issue #151): a `CostModel` + /// that opts a `TopK { accuracy: Exact, .. }` request into a sketch + /// candidate via `CostModel::topk_exact_accuracy_target` makes that + /// candidate a real, non-trivial alternative in the `TargetSubDAG`'s + /// candidate list (alongside `PassThrough`) — exactly the shape this + /// module's own framing ("does this candidate list contain anything + /// other than the trivial, no-op realization?") reports as a + /// `SketchApproximation` finding. No special-casing was needed in this + /// module for that: `sketch_finding_reason` already reads whatever + /// `crate::replacement` decided, and the finding's `reason` is that + /// candidate's own rationale, which names the sketch kind explicitly — + /// a reviewer reading the finding can already tell an approximation was + /// used, even though the query asked for `Exact`. + #[test] + fn topk_exact_opted_in_cost_model_reports_a_sketch_applicability_finding() { + struct OffersSketchForExactTopK; + impl crate::cost_model::CostModel for OffersSketchForExactTopK { + fn rank_candidates( + &self, + _intent: &AggIntent, + candidates: &[asap_types::post_asap::SketchAlgorithm], + ) -> Vec { + candidates.to_vec() + } + fn topk_exact_accuracy_target(&self, _intent: &AggIntent) -> Option { + Some(AccuracyTarget::Epsilon(0.1)) + } + } + + let q = agg( + vec![2], + AggIntent::TopK { + k: 10, + accuracy: AccuracyTarget::Exact, + }, + metric_scan(&["job"]), + ); + let cost_model = OffersSketchForExactTopK; + let strategies = replacement::default_strategies_with(&cost_model); + let findings = explain_replacements_with(vec![("exact_top10", q)], &strategies); + let sketch = findings + .iter() + .find(|f| f.kind == ExplanationKind::SketchApproximation) + .expect("an opted-in CostModel should report a sketch finding for TopK{Exact}"); + assert!( + sketch.reason.to_lowercase().contains("cmswithheap"), + "reason should name the sketch kind actually offered, got {:?}", + sketch.reason + ); + } + #[test] fn nested_aggregate_still_finds_the_inner_sketchable_node() { // avg(quantile(0.9, sum by (job) (m))) shaped test isn't representable diff --git a/crates/asap-aware-mapping/src/replacement.rs b/crates/asap-aware-mapping/src/replacement.rs index 117f64a..0ee422e 100644 --- a/crates/asap-aware-mapping/src/replacement.rs +++ b/crates/asap-aware-mapping/src/replacement.rs @@ -670,17 +670,14 @@ pub(crate) fn implementations_for_with( | AggIntent::TopK { accuracy, .. } => match accuracy { // `TopK` is the one approximate-capable intent whose `Exact` // case a `CostModel` may still want a sketch candidate for - // (issue #151, see `CostModel::topk_exact_offers_sketch`'s + // (issue #151, see `CostModel::topk_exact_accuracy_target`'s // docs) — there's no *exact* mergeable top-k accumulator // (unlike `Count`), so "closest available approximation" is a // real, deployment-chosen alternative to `PassThrough`, not a // fallback core can pick on its own. Quantile/Cardinality/Count // still route through `exact_realization` unconditionally. - AccuracyTarget::Exact - if matches!(intent, AggIntent::TopK { .. }) - && cost_model.topk_exact_offers_sketch(intent) => - { - sketch_implementations(intent, accuracy, cost_model) + AccuracyTarget::Exact if matches!(intent, AggIntent::TopK { .. }) => { + topk_exact_realization(intent, cost_model) } AccuracyTarget::Exact => vec![exact_realization(intent)], _ => sketch_implementations(intent, accuracy, cost_model), @@ -772,10 +769,12 @@ pub(crate) fn implementations_for_with( /// Exact realization of an approximate-capable intent whose target is /// `AccuracyTarget::Exact`. `Count` has a mergeable exact accumulator; exact /// quantile / top-k / cardinality have no single-value summary form (they -/// need the full multiset / heap / set) and pass through by default. A -/// `CostModel` can opt a `TopK { accuracy: Exact, .. }` request out of this -/// function entirely — see [`CostModel::topk_exact_offers_sketch`] and -/// [`implementations_for_with`]'s `AccuracyTarget::Exact` arm (issue #151). +/// need the full multiset / heap / set) and pass through by default. For +/// `TopK` specifically, `implementations_for_with` doesn't even call this — +/// see [`topk_exact_realization`] and [`CostModel::topk_exact_accuracy_target`] +/// (issue #151) — because `PassThrough` there is only ever the *default*, +/// not the only possible answer, the way it is for the other two variants +/// this function handles. fn exact_realization(intent: &AggIntent) -> Implementation { match intent { AggIntent::Count { .. } => exact_accumulator(intent, ExactKind::Count, ExactParams::Count), @@ -793,16 +792,54 @@ fn exact_accumulator(intent: &AggIntent, kind: ExactKind, params: ExactParams) - Implementation::ExactAggregate { kind, params } } +/// `AggIntent::TopK { accuracy: Exact, .. }`'s realization (issue #151) — +/// `implementations_for_with`'s dedicated arm for this one shape, kept +/// separate from [`exact_realization`] because unlike every other intent +/// that function handles, `PassThrough` here is never the *only* legal +/// answer, just the default one. +/// +/// `Implementation::PassThrough` always stays in the returned list — this +/// crate has no *exact* mergeable top-k accumulator, so it is always a +/// legal candidate, and [`implementations_for_with`]'s exhaustive, +/// never-prune contract (the same one [`CostModel::rank_candidates`] itself +/// is bound by) means opting into a sketch candidate must never remove it. +/// [`CostModel::topk_exact_accuracy_target`] decides whether a real sketch +/// candidate joins it: `None` (the default) leaves `PassThrough` as the +/// only candidate — today's behavior, unchanged; `Some(target)` sizes the +/// same `CmsWithHeap`/`CountSketchWithHeap` family an approximate `TopK` +/// request would get (via [`sketch_implementations`], ranked via +/// `rank_candidates` exactly as usual) against the deployment-supplied +/// `target` instead of `Exact`'s own degenerate one, and puts those ranked +/// candidates ahead of `PassThrough` — a deployment that opted in this +/// explicitly prefers its approximation over no summary at all, but +/// `PassThrough` remains available for a caller (e.g. +/// [`CostModel::estimate_cost`]) that wants to weigh it against the rest. +fn topk_exact_realization(intent: &AggIntent, cost_model: &dyn CostModel) -> Vec { + match cost_model.topk_exact_accuracy_target(intent) { + Some(target) => { + let mut candidates = sketch_implementations(intent, &target, cost_model); + candidates.push(Implementation::PassThrough); + candidates + } + None => vec![Implementation::PassThrough], + } +} + /// Resolve an [`AccuracyTarget`] into the `(eps, delta)` budget /// [`CostModel::size_params`] needs. Shared by [`sketch_implementations`] and /// this crate's own sizing — one place this resolution happens, so nothing /// can drift apart on it. /// -/// `Exact` is unreachable via [`implementations_for_with`] for every -/// approximate-capable intent except `TopK` under an opted-in `CostModel` -/// (issue #151, [`CostModel::topk_exact_offers_sketch`]) — that path sizes -/// its sketch candidate(s) at the tightest budget this degrades to, the same -/// as a caller that resolves `Exact` directly. +/// `Exact` is unreachable via [`implementations_for_with`] — every +/// approximate-capable intent either has its own exact realization +/// ([`exact_realization`]) or, for `TopK`, resolves a deployment-supplied +/// replacement target first ([`topk_exact_realization`], +/// [`CostModel::topk_exact_accuracy_target`], issue #151) rather than ever +/// sizing against `Exact` itself. Still degrades to the tightest parameters +/// here for a caller that resolves `Exact` directly anyway (or a +/// `CostModel` that deliberately returns `Some(AccuracyTarget::Exact)` from +/// `topk_exact_accuracy_target` — see that method's docs for why that's a +/// pitfall, not a use case). pub fn accuracy_budget(accuracy: &AccuracyTarget) -> (f64, f64) { match accuracy { AccuracyTarget::Exact => (f64::MIN_POSITIVE, DEFAULT_DELTA), @@ -3203,11 +3240,17 @@ mod tests { // ── TopK{accuracy: Exact} sketch opt-in (issue #151) ──────────────── /// A `CostModel` that opts every `TopK { accuracy: Exact, .. }` request - /// into the sketch family via `topk_exact_offers_sketch`, otherwise - /// behaving exactly like `DefaultCostModel`. - struct OffersSketchForExactTopK; + /// into the sketch family via `topk_exact_accuracy_target`, sizing at + /// `target` (a sane, deployment-chosen budget — never `Exact`'s own + /// degenerate one), otherwise behaving exactly like `DefaultCostModel`. + /// `target: Option` so + /// `topk_exact_default_cost_model_still_passes_through` below can reuse + /// this same struct with `None` instead of a second near-identical impl. + struct TopkExactCostModel { + target: Option, + } - impl CostModel for OffersSketchForExactTopK { + impl CostModel for TopkExactCostModel { fn rank_candidates( &self, _intent: &AggIntent, @@ -3216,8 +3259,8 @@ mod tests { candidates.to_vec() } - fn topk_exact_offers_sketch(&self, intent: &AggIntent) -> bool { - matches!(intent, AggIntent::TopK { .. }) + fn topk_exact_accuracy_target(&self, _intent: &AggIntent) -> Option { + self.target.clone() } } @@ -3229,11 +3272,12 @@ mod tests { } /// (a) No regression: with no `CostModel` overriding - /// `topk_exact_offers_sketch`, `TopK { accuracy: Exact, .. }` still - /// resolves to `PassThrough` — `implementations_for_with`'s - /// `AccuracyTarget::Exact` arm now *consults* a `CostModel` hook before - /// declining, but `DefaultCostModel`'s default (`false`) declines - /// exactly like the old unconditional short-circuit did. + /// `topk_exact_accuracy_target` (default: `None`), `TopK { accuracy: + /// Exact, .. }` still resolves to `PassThrough`, and *only* + /// `PassThrough` — `implementations_for_with`'s `AccuracyTarget::Exact` + /// arm now consults a `CostModel` hook before declining, but + /// `DefaultCostModel`'s default (`None`) declines exactly like the old + /// unconditional short-circuit did, byte for byte. #[test] fn topk_exact_default_cost_model_still_passes_through() { assert_eq!(preferred(&topk_exact(10)), Implementation::PassThrough); @@ -3242,54 +3286,88 @@ mod tests { implementations_for_with(&topk_exact(10), &DefaultCostModel), vec![Implementation::PassThrough] ); + // A `CostModel` that explicitly declines (`None`, same as the + // default) behaves identically — the opt-in really is off unless a + // real target is supplied. + assert_eq!( + implementations_for_with(&topk_exact(10), &TopkExactCostModel { target: None }), + vec![Implementation::PassThrough] + ); } - /// (b) A `CostModel` that opts in via `topk_exact_offers_sketch` gets a - /// real sketch candidate — ranked via `rank_candidates` and sized via - /// `size_params` exactly like an approximate `TopK` request, at the - /// tightest budget `AccuracyTarget::Exact` resolves to (issue #151). + /// (b) A `CostModel` that opts in via `topk_exact_accuracy_target` with + /// a sane budget (eps=0.1, not `Exact`'s own degenerate one) gets a + /// real, correctly-sized sketch candidate *alongside* `PassThrough` — + /// ranked via `rank_candidates` and sized via `size_params` exactly + /// like an approximate `TopK` request, against the supplied target + /// (issue #151). Pins width/depth explicitly so a regression back to + /// `accuracy_budget(Exact)`'s clamp-saturated `(width=1<<26, ..)` is + /// caught immediately, not silently. #[test] fn topk_exact_opted_in_cost_model_offers_sketch_candidates() { let intent = topk_exact(10); - let candidates = implementations_for_with(&intent, &OffersSketchForExactTopK); - // Same candidate family/order as an approximate TopK request would - // get: [CmsWithHeap, CountSketchWithHeap]. - let kinds: Vec = candidates + let cost_model = TopkExactCostModel { + target: Some(AccuracyTarget::Epsilon(0.1)), + }; + let candidates = implementations_for_with(&intent, &cost_model); + + // Exhaustive, never-prune: PassThrough is still in the set, not + // replaced by the sketch candidates. + assert!(candidates.contains(&Implementation::PassThrough)); + + let sketches: Vec<&SketchKind> = candidates .iter() - .map(|implementation| match implementation { - Implementation::Sketch(kind) => kind.algorithm().clone(), - other => panic!("expected Sketch, got {other:?}"), + .filter_map(|implementation| match implementation { + Implementation::Sketch(kind) => Some(kind), + _ => None, }) .collect(); assert_eq!( - kinds, + sketches + .iter() + .map(|k| k.algorithm().clone()) + .collect::>(), vec![ SketchAlgorithm::CmsWithHeap, SketchAlgorithm::CountSketchWithHeap ] ); - // The preferred (first) candidate is a real, heap-sized sketch — - // not PassThrough. - match implementations_for_with(&intent, &OffersSketchForExactTopK) - .into_iter() - .next() - .unwrap() - { + + // The preferred (first) candidate is the ranked sketch, sized at + // eps=0.1 (not Exact's clamp-to-maximum) — width=⌈e/0.1⌉=28, + // depth=⌈ln(1/DEFAULT_DELTA)⌉=5, a world away from the + // 1<<26 = 67_108_864 the old `accuracy_budget(Exact)` fallback + // would have produced. + match &candidates[0] { Implementation::Sketch(kind) => { assert_eq!(kind.algorithm(), &SketchAlgorithm::CmsWithHeap); - let SketchParams::CmsWithHeap { heap_size, .. } = kind.params() else { + let SketchParams::CmsWithHeap { + width, + depth, + heap_size, + } = kind.params() + else { unreachable!("SketchKind validates CmsWithHeap params") }; + assert_eq!( + *width, 28, + "width must track the eps=0.1 target, not Exact's clamp" + ); + assert_eq!(*depth, 5); assert_eq!(*heap_size, 10); } other => panic!("expected Sketch, got {other:?}"), } + // PassThrough is last: an opted-in deployment prefers its own + // approximation over no summary, but PassThrough is still offered + // for a caller that wants to weigh it (e.g. `estimate_cost`). + assert_eq!(candidates.last(), Some(&Implementation::PassThrough)); } /// (c) `Count { accuracy: Exact }` is unaffected by the `TopK`-specific /// opt-in: it keeps routing through `exact_realization`'s mergeable /// accumulator regardless of what a `CostModel` says about - /// `topk_exact_offers_sketch` (which only ever gates the `TopK` arm). + /// `topk_exact_accuracy_target` (which only ever gates the `TopK` arm). #[test] fn count_exact_unaffected_by_topk_exact_opt_in() { let intent = AggIntent::Count { @@ -3304,22 +3382,12 @@ mod tests { ); // Even a CostModel that unconditionally opts every intent into the // TopK-exact sketch path leaves Count's exact accumulator alone — - // `topk_exact_offers_sketch` is never even consulted for it. - struct OptsEverythingIn; - impl CostModel for OptsEverythingIn { - fn rank_candidates( - &self, - _intent: &AggIntent, - candidates: &[SketchAlgorithm], - ) -> Vec { - candidates.to_vec() - } - fn topk_exact_offers_sketch(&self, _intent: &AggIntent) -> bool { - true - } - } + // `topk_exact_accuracy_target` is never even consulted for it. + let cost_model = TopkExactCostModel { + target: Some(AccuracyTarget::Epsilon(0.1)), + }; assert_eq!( - implementations_for_with(&intent, &OptsEverythingIn), + implementations_for_with(&intent, &cost_model), vec![Implementation::ExactAggregate { kind: ExactKind::Count, params: ExactParams::Count, diff --git a/docs/developer_docs/ASAP-aware-mapping-developer-guide.md b/docs/developer_docs/ASAP-aware-mapping-developer-guide.md index e87fff2..6bafd6a 100644 --- a/docs/developer_docs/ASAP-aware-mapping-developer-guide.md +++ b/docs/developer_docs/ASAP-aware-mapping-developer-guide.md @@ -424,6 +424,7 @@ The crate cannot hardcode real deployment costs: `asap-aware-mapping` depends on |---|---|---| | `rank_candidates` | Order valid sketch algorithms | No | | `size_params` | Convert an accuracy target into sketch parameters | Yes | +| `topk_exact_accuracy_target` | Opt a `TopK{accuracy: Exact}` request into a sketch candidate, and supply the budget to size it at | Yes (`None`: stays `PassThrough`-only) | | `realize_extension` | Map a custom intent to an implementation | Yes | | `readout_extension` | Query a custom extension summary | Panics until paired with a custom realization | | `cse_recompute_cost` | Estimate independent recomputation | Yes | @@ -443,6 +444,16 @@ The crate cannot hardcode real deployment costs: `asap-aware-mapping` depends on fn size_params(&self, kind: SketchAlgorithm, intent: &AggIntent, eps: f64, delta: f64) -> SketchParams; ``` +- **`topk_exact_accuracy_target`** — opt an `AggIntent::TopK { accuracy: Exact, .. }` request into a real sketch candidate, and supply the accuracy budget to size it at. There is no exact mergeable top-k accumulator (unlike `Count`), so `implementations_for_with` cannot decide on its own between "no summary" and "the closest available approximation"—that policy call, and the budget to size it against, belong to the deployment. The default (`None`) declines: the candidate list stays `PassThrough`-only, unchanged from before this hook existed. + + ```rust + fn topk_exact_accuracy_target(&self, intent: &AggIntent) -> Option { + None + } + ``` + + Returning `Some(target)` does **not** replace `PassThrough`—`implementations_for_with` keeps it in the candidate list and adds a ranked, `target`-sized sketch (via `rank_candidates`/`size_params`, the same hooks an approximate `TopK` request already uses) ahead of it. Size at a real budget, never `AccuracyTarget::Exact` itself: `Exact` resolves to the tightest parameters every sketch family's sizing formula clamps to (e.g. `cms_width`'s `1 << 26`), which is not a meaningful target for any real deployment—both this crate's SQL and PromQL frontends default to `Exact` when no accuracy is specified, so sizing at that clamp would put the largest possible sketch behind every un-annotated `topk` query. Prefer something budget-shaped instead, such as `AccuracyTarget::Epsilon(1.0 / k as f64)` or a fixed deployment-wide epsilon. + - **`realize_extension`** — map a deployment-defined `AggIntent::Extension` to a post-ASAP `Implementation`. The default is `Implementation::PassThrough`. Use `AggIntent::Extension { ext_kind, payload }` for intent shapes that only your deployment needs. Core treats both fields as opaque. For example, a deployment can tag an approximate-frequency intent with `ext_kind: "frequency"` and recognize it in `realize_extension`: @@ -850,7 +861,7 @@ flowchart LR C --> F["Output Vec<ReplacementSubDAG>
each entry contains a constructed SummaryNode and rationale;
all candidates retained in preferred order"] ``` -For an approximate quantile, the candidate list includes both KLL and DDSketch even though the cost model ranks one ahead of the other. When only one realization is legal, such as an exact accumulator or pass-through, the strategy returns that single candidate. +For an approximate quantile, the candidate list includes both KLL and DDSketch even though the cost model ranks one ahead of the other. When only one realization is legal, such as an exact accumulator or pass-through, the strategy returns that single candidate—except `TopK { accuracy: Exact, .. }` under a `CostModel` that overrides `topk_exact_accuracy_target`: `pass-through` stays a candidate, but a ranked, sized sketch candidate joins it (see the `CostModel` hook table above). --- @@ -1211,6 +1222,33 @@ flowchart LR --- +#### `topk_exact_accuracy_target` + +Use when you want an `AggIntent::TopK { accuracy: Exact, .. }` request to still get a real sketch candidate—the "closest available approximation" policy—instead of only `PassThrough`. + +Signature: + +```rust +fn topk_exact_accuracy_target(&self, intent: &AggIntent) -> Option; +``` + +Return `None` (the default) to decline—`PassThrough` stays the only candidate, unchanged from before this hook existed. Return `Some(target)` to opt in: `implementations_for_with` adds a `target`-sized, `rank_candidates`-ranked `CmsWithHeap`/`CountSketchWithHeap` candidate ahead of `PassThrough`, which stays in the list. + +`target` must be a real budget your deployment is willing to size against—never `AccuracyTarget::Exact` itself, which resolves to the tightest parameters every sizing formula clamps to (e.g. `1 << 26` for CMS width). A reasonable choice scales with `k`: + +```rust +fn topk_exact_accuracy_target(&self, intent: &AggIntent) -> Option { + match intent { + AggIntent::TopK { k, .. } => Some(AccuracyTarget::Epsilon(1.0 / *k as f64)), + _ => None, + } +} +``` + +This hook is only ever consulted for `TopK`'s `Exact` case; every other approximate-capable intent (`Quantile`, `Cardinality`) and `Count { accuracy: Exact }` are unaffected by it. + +--- + #### `realize_extension` Use for extension-defined implementation kinds. @@ -1546,6 +1584,7 @@ Use this table to find the right place for a change. | Add a new built-in sketch candidate | `replacement.rs`'s summary-candidate mapping | | Prefer one sketch algorithm over another | `CostModel::rank_candidates` | | Change sketch sizing for an accuracy target | `CostModel::size_params` | +| Offer a sketch alternative for an `Exact`-accuracy top-k request | `CostModel::topk_exact_accuracy_target` | | Add extension-defined implementation behavior | `CostModel::realize_extension` | | Add extension-defined readout behavior | `CostModel::readout_extension` | | Change CSE recomputation cost | `CostModel::cse_recompute_cost` | diff --git a/docs/user-guide/user-guide.md b/docs/user-guide/user-guide.md index 558323f..b68b123 100644 --- a/docs/user-guide/user-guide.md +++ b/docs/user-guide/user-guide.md @@ -163,7 +163,7 @@ else { a deployment that wants its own candidate ranking or parameter sizing instead of this crate's built-in static preference order (`DefaultCostModel` — what `default_cost_model()` uses). See the `CostModel` trait doc in `crates/asap-aware-mapping/src/cost_model.rs` for its overridable -hooks (`rank_candidates`, `size_params`, `realize_extension`, …). +hooks (`rank_candidates`, `size_params`, `topk_exact_accuracy_target`, `realize_extension`, …). To see every root of a whole workload at once — including the candidates CSE-shared subtrees get (a shared subtree's `MemoGroup` carries both the "share" and "recompute independently" options, From c91cda33d9cb4128a76f38a7c7cc48681d550d71 Mon Sep 17 00:00:00 2001 From: zz_y Date: Fri, 28 Aug 2026 04:38:38 -0600 Subject: [PATCH 3/3] fix(plan): reject exact TopK sketch sizing target --- crates/asap-aware-mapping/src/cost_model.rs | 8 ++++-- crates/asap-aware-mapping/src/replacement.rs | 30 ++++++++++++++++---- 2 files changed, 30 insertions(+), 8 deletions(-) diff --git a/crates/asap-aware-mapping/src/cost_model.rs b/crates/asap-aware-mapping/src/cost_model.rs index 2a7fa27..1ec84f6 100644 --- a/crates/asap-aware-mapping/src/cost_model.rs +++ b/crates/asap-aware-mapping/src/cost_model.rs @@ -244,10 +244,12 @@ pub trait CostModel { /// specified. Returning `Some` here instead requires the deployment to /// state the budget it actually wants (e.g. /// `Some(AccuracyTarget::Epsilon(1.0 / k as f64))`, or a fixed - /// deployment-wide epsilon) — [`rank_candidates`](Self::rank_candidates) + /// deployment-wide epsilon). Returning `Some(AccuracyTarget::Exact)` is + /// a contract violation and is rejected before sizing, preventing the + /// clamp-saturated allocation described above. [`rank_candidates`](Self::rank_candidates) /// and [`size_params`](Self::size_params) are then consulted exactly as - /// they are for any other top-k sketch candidate, sized against that - /// returned target rather than `Exact`'s own degenerate one. + /// they are for any other top-k sketch candidate, sized against the + /// returned approximate target rather than `Exact`'s own degenerate one. /// /// The sketch candidate(s) this produces are offered *alongside* /// `Implementation::PassThrough`, not instead of it — `PassThrough` diff --git a/crates/asap-aware-mapping/src/replacement.rs b/crates/asap-aware-mapping/src/replacement.rs index 0ee422e..6896890 100644 --- a/crates/asap-aware-mapping/src/replacement.rs +++ b/crates/asap-aware-mapping/src/replacement.rs @@ -817,6 +817,11 @@ fn exact_accumulator(intent: &AggIntent, kind: ExactKind, params: ExactParams) - fn topk_exact_realization(intent: &AggIntent, cost_model: &dyn CostModel) -> Vec { match cost_model.topk_exact_accuracy_target(intent) { Some(target) => { + assert!( + !matches!(target, AccuracyTarget::Exact), + "CostModel::topk_exact_accuracy_target must return an approximate accuracy \ + target, not AccuracyTarget::Exact" + ); let mut candidates = sketch_implementations(intent, &target, cost_model); candidates.push(Implementation::PassThrough); candidates @@ -835,11 +840,9 @@ fn topk_exact_realization(intent: &AggIntent, cost_model: &dyn CostModel) -> Vec /// ([`exact_realization`]) or, for `TopK`, resolves a deployment-supplied /// replacement target first ([`topk_exact_realization`], /// [`CostModel::topk_exact_accuracy_target`], issue #151) rather than ever -/// sizing against `Exact` itself. Still degrades to the tightest parameters -/// here for a caller that resolves `Exact` directly anyway (or a -/// `CostModel` that deliberately returns `Some(AccuracyTarget::Exact)` from -/// `topk_exact_accuracy_target` — see that method's docs for why that's a -/// pitfall, not a use case). +/// sizing against `Exact` itself; [`topk_exact_realization`] rejects that +/// value as a `CostModel` contract violation. Still degrades to the tightest +/// parameters here for a caller that resolves `Exact` directly anyway. pub fn accuracy_budget(accuracy: &AccuracyTarget) -> (f64, f64) { match accuracy { AccuracyTarget::Exact => (f64::MIN_POSITIVE, DEFAULT_DELTA), @@ -3364,6 +3367,23 @@ mod tests { assert_eq!(candidates.last(), Some(&Implementation::PassThrough)); } + /// Returning `Exact` from the opt-in hook would size each sketch at the + /// clamp maximum, recreating the allocation hazard the explicit target + /// is intended to prevent. Reject it at the hook boundary rather than + /// relying on every downstream `CostModel` author to notice the warning. + #[test] + #[should_panic( + expected = "CostModel::topk_exact_accuracy_target must return an approximate accuracy target" + )] + fn topk_exact_opt_in_rejects_exact_sizing_target() { + implementations_for_with( + &topk_exact(10), + &TopkExactCostModel { + target: Some(AccuracyTarget::Exact), + }, + ); + } + /// (c) `Count { accuracy: Exact }` is unaffected by the `TopK`-specific /// opt-in: it keeps routing through `exact_realization`'s mergeable /// accumulator regardless of what a `CostModel` says about