feat(asap-aware-mapping): reuse tighter sketches across accuracy targets - #294
Merged
Merged
Conversation
…CSE sharing (#273) Adds AccuracyReconciliationStrategy: a workload-aware ReplacementStrategy, wired into search_workload_with the same way RollupStrategy/ TopKLimitReuseStrategy already are, that recognizes near-duplicate AggIntents differing only in their AccuracyTarget and proposes reading a strictly-tighter sibling's build instead of an independent, looser copy. - Near-duplicate: same bindable single-measure, no-HAVING Aggregate shape, same reduction/output_names/child, same AggIntent variant and non-accuracy fields (col/q/k), differing only in accuracy. - Safety of tightening: both sides resolve through the existing accuracy_budget() to concrete (eps, delta) numbers; every shipped sizing formula (kll_k/cms_width/cms_depth/hll_precision/kmv_k/DDSketch's alpha) is monotonic in those numbers, so a build sized to a Pareto-dominating (eps, delta) always satisfies a looser one too. AccuracyTarget::Exact is excluded on either side (routes through a different Implementation family entirely, not just a tighter budget). - Additive only: share_common_subtrees's own exact-equality merge is untouched — accuracy still participates in exact structural equality everywhere else. This strategy's Replacement::Rewrite candidates sit alongside each consumer's own independently-sized SketchAlgorithmStrategy candidate; CostModel-driven ranking picks between them, never forced. Tests cover: the issue's own quantile(0.99, x) at epsilon=0.01 vs 0.05 scenario, Pareto-domination edge cases (EpsilonDelta needing both dimensions tighter, Exact never participating, equal resolved budgets not being strictly tighter), non-matches (different column/grouping), an end-to-end search_workload() integration test, and an explicit regression test that share_common_subtrees still never merges differing-accuracy aggregates. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…273 Addresses code review findings on PR #294 (feat/cross-consumer-accuracy- reconciliation-273): 1. Sign-inverted cost incentive (accuracy_reconciliation.rs:307, cost_model.rs). AccuracyReconciliationStrategy's Rewrite candidate was falling through to DefaultCostModel::estimate_cost's generic Rewrite arm, which prices "recompute target independently, once per consumer" — correct for SharedSubtreeStrategy's CseRecompute and for Rollup/TopKLimitReuse's LogicalRewrite candidates (which really do rebuild target from a different source), but wrong for this candidate, which never rebuilds target at all — it reads a sibling that's already being built regardless. That formula scales with consumer_count, so it made the candidate *more* expensive exactly when more consumers stood to benefit from sharing (verified: 2 vs 3 for a single consumer — chosen for the wrong reason; 4 vs CseShare's 1 for two consumers — never chosen, in precisely the scenario this feature targets). Fix: added ReplacementProvenance::AccuracyReconciliation (distinct from LogicalRewrite, since it needs distinct cost treatment, not just a distinct label) and a dedicated estimate_cost arm that prices it as a cse_shared_maintenance_cost "read" against the sibling's own bound summary — flat, and no longer scaling with the reader's own consumer_count. New regression test (estimate_cost_does_not_scale_with_the_readers_own_consumer_count) pins this directly; two new cost_sorted/global_selection integration tests cover the single-consumer and shared-consumer cases end to end. 2. Bypassed CSE's unique-key legality gate (accuracy_reconciliation.rs:272). tighter_sources' value-equality child-matching fallback didn't check has_unique_key() on the tighter candidate's own output schema, unlike RollupStrategy's identical check (rollup.rs:231, reusing the same legality rule share_common_subtrees itself applies). A global or without(...)-grouped aggregate (no provable unique key) could have been proposed as a reconciliation source even though row identity isn't stable across reads for it. Fix: added the same has_unique_key() check RollupStrategy uses, with two new tests (Reduction::by(vec![]) and without(...) fixtures) mirroring Rollup's own no_unique_key_on_the_finer_side_does_not_roll_up test. Also fixed: sort-direction doc comment (was "tightest-last", sort is ascending i.e. tightest-first), and replaced a hardcoded strategy-name string literal with self.name(). Deferred (documented as a known limitation in the module docs and PR body, not silently dropped): PlanSpace::global_selection's cross-group DP doesn't add a reference edge from a looser target to the sibling it reads via this strategy, so the sibling's own effective_consumer_count doesn't yet reflect reconciled readers. Real surgery on global_selection's reference graph (a new edge kind the topological-sort/cycle-freedom argument doesn't currently model), not a local fix to this strategy. All 162 tests pass in asap-aware-mapping (5 new); full workspace build, test, clippy -D warnings, and fmt --check all clean. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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
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.
Summary
Closes #273. Part of #33.
This PR adds
AccuracyReconciliationStrategy, a workload-aware replacement strategy that lets otherwise-identical aggregate consumers share the result built for the strictest compatible accuracy target.For example, two
quantile(0.99, x)aggregates requestingepsilon=0.01andepsilon=0.05can use one build satisfyingepsilon=0.01, instead of maintaining two independent sketches.Pre-ASAP CSE remains exact:
AccuracyTargetstill participates in structural equality, andshare_common_subtreesis unchanged. Reconciliation is added as an optionalReplacement::Rewritecandidate and remains subject to cost-based selection.Matching and safety
A tighter sibling is eligible only when both aggregates:
HAVINGclause;AggIntentvariant and the same non-accuracy fields (col,q, ork);(epsilon, delta)budgets are in a strict Pareto-dominance relationship; andAccuracyTarget::Exactis excluded because it uses a different implementation family rather than a tighter point on the same approximate-accuracy budget.The tighter implementation may use any compatible sketch algorithm.
CostModel::size_paramsnow explicitly requires returned parameters to satisfy the supplied accuracy budget, including for custom sizing models.Planning and costing
ReplacementProvenance::AccuracyReconciliationso these candidates can be identified without inferring semantics from expression shape.effective_consumer_count, after which the tighter MemoGroup propagates uses through its own selected implementation.Tests
Coverage includes:
epsilon=0.01versusepsilon=0.05quantile scenario;Epsilon/EpsilonDeltaPareto-ordering edge cases;without(...)aggregations;Verification: