feat(plan): let CostModel offer sketches for exact TopK - #293
Closed
zzylol wants to merge 3 commits into
Closed
Conversation
…etch 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 <noreply@anthropic.com>
…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<AccuracyTarget>`. `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<AccuracyTarget> }`. 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 <noreply@anthropic.com>
zzylol
changed the title
Contributor
Author
|
Closing this approach because |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #151.
Problem
TopK { accuracy: Exact }currently short-circuits toPassThrough. Unlike exactCount, top-k has no exact mergeable accumulator, but some deployments still want to offer the closest available sketch approximation.The planner previously had no cost-model extension point for expressing that policy.
Changes
CostModel::topk_exact_accuracy_target, defaulting toNone.None: onlyPassThroughis offered.CmsWithHeapandCountSketchWithHeapcandidates alongsidePassThrough.Exact, which would saturate CMS width at1 << 26and could create multi-gigabyte sketch instances.Some(AccuracyTarget::Exact)as a cost-model contract violation before sizing.Countaccumulator.Compatibility
The hook has a provided default implementation returning
None, so existingCostModelimplementations require no changes and preserve the currentTopK { accuracy: Exact }behavior.Validation
cargo build --workspacecargo test --workspacecargo clippy --workspace --all-targets -- -D warningscargo fmt --all -- --checkFocused coverage verifies default pass-through behavior, opted-in candidate ranking and sizing, retention of
PassThrough, rejection of anExactsizing target, explanation output, and no behavior change for exactCount.