fix(data_plane): stop conflating CountSketch with CMS on ingest - #416
Merged
Merged
Conversation
`SummaryKind::CountSketch`/`CountSketchWithHeap` silently reused the CMS
accumulators on both ingest paths:
- `accumulator_factory.rs`'s raw-metric dispatch collapsed
`(Cms|CountSketch)` and `(CmsWithHeap|CountSketchWithHeap)` into shared
match arms, building `CmsAccumulatorUpdater`/`CmsHeapAccumulatorUpdater`
(min-of-rows CMS math) for CountSketch-family sids.
- `otel.rs`'s modified-OTLP decode/delta-bootstrap/delta-apply for
heap-bearing `SketchKind::CountSketch` frames used
`CountMinSketchWithHeap`/`CountMinSketchWithHeapAccumulator` — the two
families share a byte-identical wire envelope (heap is the
distinguishing payload), which let this compile and round-trip, but
reads a real CountSketch matrix through CMS's min-of-rows estimator.
Adds `CountSketchAccumulator::{query_key, MultipleSubpopulationAggregate,
MergeableAccumulator}` (previously missing; its `query_statistic`'s
keyed lookup also used a hand-rolled, non-hash-spec-compatible hasher —
now routes through the real `asap_sketchlib::CountSketch::estimate`) and
a new `CountSketchWithHeapAccumulator` (structural port of
`CountMinSketchWithHeapAccumulator` wrapping the real
`asap_sketchlib::CountSketchWithHeap`), then wires both through
dedicated `CountSketchAccumulatorUpdater`/`CountSketchWithHeapAccumulatorUpdater`
and fixes all four otel.rs call sites to use them.
The two known pre-existing e2e failures
(`controller_plan_to_query_full_roundtrip_{cms,count_sketch}_with_heap_topk`)
are unaffected — confirmed identical on origin/main before this change,
so they're a separate routing issue, not this bug.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
5 tasks
zzylol
added a commit
that referenced
this pull request
Jul 27, 2026
…t plan Rebased onto current main (was 27 commits behind, predating #416/#417/#418) and updated the doc's content to match: - Status note: SummaryExecutor now covers the Frequency family and ExactAgg(Sum/Increase) candidate matching + coverage tracking, not just quantile/cardinality -- these landed since the doc was last synced. - "Today"/gap list: gap 2 (merge only existed for ExactAgg) and gap 3 (nothing checked param agreement) are now resolved on the new path, marked accordingly rather than left as open problems. - SidHandle/GroupState table entries: updated to describe the actual enum shape (Sketch/ExactAgg variants), not the original sketch-only struct design. - New "Architecture reference" section citing ASAPController design.md's "Serving-time execution" section directly -- the planning-vs-serving split this doc's Rollout section builds on. - "Rollout" section: replaces "still open" with an actual plan. Corrects a real error in the previous version -- it named `implement_promql_for_asap_tier` as "the seam" for tree construction, but that function uses the naive DefaultCostModel and has a documented, tracked gap where it can't realize the Frequency intent at all. The correct seam is `sketch_algebra::lower::bind_query_expr` (ControlPlaneCostModel, what main.rs's real production pipeline uses). Also documents why rate()/topk-over-rate/outer-agg-fold must be excluded from the shadow comparison entirely (not just deprioritized) and why shadow mode -- not a cutover -- is this phase's actual scope. 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
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.
Summary
accumulator_factory.rs's raw-metric dispatch collapsedSummaryKind::{Cms,CountSketch}and{CmsWithHeap,CountSketchWithHeap}into shared match arms, silently building CMS (min-of-rows) accumulators for CountSketch-family sids. Split into dedicated arms + newCountSketchAccumulatorUpdater/CountSketchWithHeapAccumulatorUpdater.otel.rs's modified-OTLP decode / delta-bootstrap / delta-apply for heap-bearingSketchKind::CountSketchframes usedCountMinSketchWithHeap(Accumulator)— the two families share a byte-identical wire envelope (heap is the distinguishing payload) so this compiled and round-tripped, but read a real CountSketch matrix through CMS's min-of-rows estimator. Fixed all four call sites.CountSketchAccumulatorgainedMultipleSubpopulationAggregate/MergeableAccumulatorimpls (previously missing) and its keyedquery_statisticlookup — previously a hand-rolled, non-hash-spec-compatible hasher — now routes through the realasap_sketchlib::CountSketch::estimate.CountSketchWithHeapAccumulator(structural port ofCountMinSketchWithHeapAccumulator) wrapping the realasap_sketchlib::CountSketchWithHeap.Test plan
cargo build --workspace— cleancargo test -p data_plane --lib— 929 passed (0 failed)cargo test -p data_plane --test e2e_controller_plans_and_backend_serves— 9/11 pass; the 2 known pre-existing failures (controller_plan_to_query_full_roundtrip_{cms,count_sketch}_with_heap_topk) reproduce identically on origin/main before this change, confirming they're an unrelated pre-existing routing issuecargo clippy -p data_plane --lib— no new warnings vs. baseline (one pre-existing warning fixed as a side effect of deleting the dead hand-rolled hasher)rustfmton touched files only🤖 Generated with Claude Code