feat(query): heap-bearing CMS + CountSketch topk strict-success - #258
Merged
Merged
Conversation
Closes the FrequencyTopk strict-success path end-to-end through the gateway-less OTLP wire format. Tests 8+9 ingest msgpack-encoded heap envelopes and answer `topk(...)` queries with the heap-bearing variants (CmsWithHeap / CountSketchWithHeap). To make the strict path work, several gaps along the analyzer ↔ ingest ↔ policy match chain are closed: - `sketch_kind_handle_for` (otel.rs): mirror the existing CMS-with-heap auto-detection on the CountSketch branch. A msgpack-encoded CountSketch DP with a non-empty heap promotes the sid to `SketchKindHandle::CountSketchWithHeap`. The wire envelope is shared with CmsWithHeap (the reducer dispatches both through `decode_cms_with_heap_from_msgpack`). - `sketch_config_to_params` (otel.rs): emit canonical `w`/`d` keys to match the controller's `sketch_params_to_json`. Previously emitted `rows`/`cols`, which made `find_policy_by_content`'s content match silently miss every CMS / CountSketch sid (registered with `policy_fp = UNSET`, unreachable through `sids_for_policy`). - `Expr::VectorSelector` (promql.rs): also suppress the implicit `Aggregate(Sum)` wrapper when `ctx.topk.is_some()`. PR #255 had already done this for `outer_count`; topk has the same semantics (the inner is the population to rank, not a value to sum), and without this gate the analyzer emits a spurious ExactAgg(Sum) candidate that bails the engine's "all candidates must succeed" loop before the FrequencyTopk candidate is reached. - `capability_for(&AggIntent::TopK)` (capability.rs): return `FrequencyTopk(Any)` instead of pinning `CmsWithHeap`. The analyzer doesn't know which heap-bearing variant the ingest tier registered, and `handles_compatible_for_topk` already wildcards on `Any` — so this lets either `CmsWithHeap` or `CountSketchWithHeap` answer a `topk(...)` query. - New `AggregationType::CountSketchWithHeap` variant (promql_utilities, control_plane, data_plane) so the streaming- config can register a CountSketch-with-heap policy whose `policy_capability` returns `FrequencyTopk(CountSketchWithHeap)`. Wires through enums.rs (variants + as_str + from_str), the policy_capability bridge, the ingest-side `aggregation_type_for_sketch_handle` mapping, and the accuracy module (CountSketch ε plus heap-retention ε bound). The streaming-config emit gap (controller still writes `aggregationType: "CountMinSketch"` / `"CountSketch"` regardless of the planner's `with_heap` flag) is patched in-test for now — in-place JSON rewrite to the heap-bearing variant. The corresponding controller fix (consult `with_heap` when emitting `aggregationType`) is the natural follow-up. Two adjacent golden tests refreshed for the new analyzer output: `capability_for_topk_returns_frequency_topk_any` (was `_cms_with_heap`) and the engine-side `analyzer_parity_18_query_corpus` GOLDEN string (Q06/Q07 lose the spurious Sum candidate, FrequencyTopk now reports `Any`). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
zzylol
added a commit
that referenced
this pull request
May 16, 2026
#262) Audit follow-up from PR #258 — the new `CountSketchWithHeap` enum variant exposed two pre-existing panic / capability-miss paths that needed parallel arms: - `does_precompute_operator_support_subpopulations` (logics.rs): the catch-all `_ => panic!(...)` would fire for both `AggregationType::CountSketch` and `CountSketchWithHeap` if callers ever routed those through. Added explicit Sum/Count arms for CountSketch (mirrors the CMS arm — signed-counter equivalent) and a Topk arm for the heap-bearing variant (returns false, parallel to `CountMinSketchWithHeap`). - `compatible_agg_types(Statistic::Topk)` (capability_matching.rs): the list previously named `CountMinSketchWithHeap` and bare `CountSketch` but omitted `CountSketchWithHeap` — capability misses on a heap-bearing CountSketch policy would route the Topk query past the warm engine. Added the third entry. Test coverage in `test_does_precompute_operator_support_subpopulations` extended to assert no-panic for both new variants on their canonical statistics. Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2 tasks
zzylol
added a commit
that referenced
this pull request
May 16, 2026
…266) PR #265 wired `count_over_time(metric[range])` to `Capability::FrequencyEstimate(Any)`. This PR consumes that surface: Tests 6 + 7 promote from soft-check (assert `status` field exists) to strict-success (assert `status == "success"` + non-empty result vector). Two underlying fixes surfaced when wiring this up: 1. **`build_count_min_export` was hardcoding wire `rows`/`cols` to 0** — the policy_fp content match keys on `parameters.{d, w}` against the DP's wire dimensions, so heap-less CMS sids were always registering with `policy_fp = UNSET` (unreachable through `sids_for_policy`). Same bug as the heap-bearing helpers had before PR #258 fixed them. Added `wire_rows: i32, wire_cols: i32` parameters mirroring `build_cms_with_heap_msgpack_export`'s shape. 2. **`asap_tier_result_to_query_result(.., is_range_query=true)` returned Matrix for instant queries with range-bound inners** — `execute(&str)` is the trait surface for `/api/v1/query` only, and PromQL instant queries with a `metric[10s]` inner always return Vector (one value per series computed over `[t-range, t]`). The `any_range_candidate` signal captured the inner range, not the outer eval shape, and produced a Matrix that the Prometheus adapter's `format_success_response` rejects with a 500 / empty body. Hard-wired the `execute(&str)` site to pass `false`; left the parameter on the helper for the future `handle_range_query_promql` wire-up. Test 6 also pivoted: `top_endpoint_qps` is name-classified as TopK by the planner (always heap-bearing), so the original heap-less wire setup couldn't match. Switched to heap-bearing msgpack wire (same shape as Test 9) and a `count_over_time(...)` query — cross- coverage with Test 9's `topk(...)` on the same underlying sid. Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
3 tasks
zzylol
added a commit
that referenced
this pull request
May 16, 2026
…um (#268) Two `cms_params` helpers (accumulator_factory + accuracy) read the CMS / CountSketch dimensions from `AggregationConfig.parameters`. Pre-PR they only looked at the legacy `row_num` / `col_num` keys — the keys the control plane's `sketch_params_to_json` emits (and that PR #258's `sketch_config_to_params` standardized on for OTLP policy_fp content matching) are `w` / `d`. Result was a silent mismatch: the OTLP modified-sketch ingest path content-matched on `w` / `d`, but raw-input ingest (precompute worker + sketch_db backfill) read from `row_num` / `col_num` — meaning static streaming-config YAMLs that ship in asapcollector with `row_num` / `col_num` worked for raw-input ingest but failed policy_fp lookup on the modified-OTLP side, while controller- emitted configs with `w` / `d` worked for OTLP but defaulted to `(4, 1000)` on the raw-input side. Both helpers now read canonical `w` / `d` first and fall back to `row_num` / `col_num`. Both naming conventions work everywhere; old asapcollector configs keep working, controller-emitted configs keep working, and a follow-up asapcollector PR will migrate the static YAMLs to the canonical form. Co-authored-by: Claude Opus 4.7 (1M context) <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
topk(...)PromQL queries end-to-end.Fixes
Ingest auto-detection
sketch_kind_handle_for(otel.rs): mirrors the CMS-with-heap branch for CountSketch DPs. A msgpack-encoded CountSketch payload with a non-empty heap promotes the sid toSketchKindHandle::CountSketchWithHeap. The reducer dispatches both variants through the samedecode_cms_with_heap_from_msgpack(shared envelope).sketch_config_to_params(otel.rs): emit canonicalw/dkeys to match the controller'ssketch_params_to_json. Previously emittedrows/cols, which made every CMS/CountSketch sid register withpolicy_fp = UNSET(unreachable throughsids_for_policy).Analyzer
Expr::VectorSelector(promql.rs): also suppress the implicitAggregate(Sum)wrapper whenctx.topk.is_some()(PR fix(analyzer): align PromQL count(metric) semantics with sketch dispatch #255 had already done this forouter_count). Without this,topk(N, metric)collected a spurious ExactAgg(Sum) candidate that bailed the engine's "all candidates must succeed" loop before reaching FrequencyTopk.capability_for(&AggIntent::TopK): returnFrequencyTopk(Any)instead of pinningCmsWithHeap. The analyzer doesn't know which heap-bearing variant the ingest tier registered;handles_compatible_for_topkalready wildcards onAny.Schema
AggregationType::CountSketchWithHeapvariant (promql_utilities + control_plane + data_plane). Wires the policy_capability bridge (FrequencyTopk(CountSketchWithHeap)),aggregation_type_for_sketch_handle, and accuracy module (CS ε plus heap-retention ε).Out of scope (follow-up)
aggregationType: "CountMinSketch"/"CountSketch"regardless of the planner'swith_heapflag — Tests 8+9 patch the JSON in-place to the heap-bearing variant. The natural next step is asketch_kind_to_backend_typepatch that consultswith_heap.InstantVectorElementdrops per-element labels (label_keys_overrideonly exists onRangeVectorElement); Tests 8+9 verify count values (gamma=200) but can't verifyitem: \"gamma\"keying until that adapter gap closes.Test plan
cargo test --test e2e_controller_plans_and_backend_serves— all 9 pass (DDSketch, KLL, HLL, CountSketch, CMS roundtrips + CMS-with-heap topk strict-success + CountSketch-with-heap topk strict-success + 2 controller-only)cargo test -p control_plane --lib— 691 pass, 0 failcargo test -p data_plane --lib— 768 pass, 0 failcapability_for_topk_returns_frequency_topk_any(was_cms_with_heap) and the engine-sideanalyzer_parity_18_query_corpusGOLDEN (Q06/Q07 lose the spurious Sum candidate)🤖 Generated with Claude Code