feat(query): compose rate-over-Sum into ExactAgg(Sum) candidate (multinode topk dispatch) - #292
Merged
Merged
Conversation
…inode topk dispatch) Closes the rate-over-Sum gap PR #291 documented as the remaining blocker for the multinode demo's `topk(5, sum by (zone) (rate(http_requests_total[5m])))` query. The three target shapes now land on the ExactAgg(Sum) sids the control plane mints for counter metrics (no more falling over to the archive's `NoData` stub): * `rate(http_requests_total[5m])` * `sum by (zone) (rate(http_requests_total[5m]))` * `topk(5, sum by (zone) (rate(http_requests_total[5m])))` Three data-plane changes: 1. `SketchReducer::evaluate_exact_agg_rate` (sister of #291's `evaluate_exact_agg`) — folds EVERY in-window sub-window accumulator per group into ONE merged accumulator, reads `Statistic::Sum`, then divides by `range_seconds` to produce events-per-second. Emits one sample per group, timestamped at the right edge of the request window (instant-rate semantics). Guards `range_seconds == 0` and the MinMax agg-type as `UnsupportedCapability`. 2. `ASAPQueryEngine::execute` + `execute_range_promql_modern` compose rule — when a candidate is `ExactAgg(Sum-family)` with `range_seconds > 0` AND the raw PromQL contains a `rate(...)` / `irate(...)` call (walked via `promql_parser`), dispatch to `evaluate_exact_agg_rate` instead of the plain per-window `evaluate_exact_agg`. The PromQL walker is the disambiguator the analyzer can't provide on its own: `sum by (zone) (rate(metric[5m]))` and `sum by (zone) (sum_over_time(metric[5m]))` both arrive at the engine as `function="sum"` + `range_seconds=300` + `ExactAgg(Sum)` but only the first one should divide by 300. 3. `ASAPQueryEngine::try_topk_over_rate_fallback` — the control-plane analyzer rewrites `topk(K, sum by (gbk) (rate(metric[r])))` into `FrequencyTopk` + `FrequencyEstimate` candidates (PromQL `topk` lowers to `AggIntent::TopK`, which the optimizer's CMS-topk binder pins to a frequency family). Those capabilities don't match ExactAgg(Sum) sids — the analyzer- driven path would CapabilityMiss. The fallback lifts `(K, metric, group_by_keys, range_seconds, is_topk)` from the raw PromQL, finds ExactAgg(Sum) sids via `instances_matching`, runs `evaluate_exact_agg_rate`, then post-applies the descending top-K slice (or ascending bottom-K) in-engine. Plus a routing-table override: `resolve_metric_storage` in the HTTP server now flips the routing decision from `Gorilla` → `SketchStore` when the table sends a `RatePostHoc` / `Topk`-shaped query to the archive AND the sketch index has ExactAgg(Sum) sids for the metric. The control plane's `build_routing_entry` unconditionally puts `rate_post_hoc` on the archive's claim list (based on the sketch-family-only assumption that pre-dated ExactAgg sids) — flipping it back at request time avoids the control-plane churn the prompt's `DO NOT touch control_plane` constraint mandated. Test coverage: * 7 new `sketch_reducer` tests pin `evaluate_exact_agg_rate`: divide-by-range, per-group across zones, rack→zone subgroup collapse, no-gbk per-sid preservation, zero-range and MinMax unsupported-capability, empty-window NoData. * 4 new `asap_query_engine` end-to-end tests pin the engine dispatch: `rate(...)` direct, `sum by (zone) (rate(...))`, `topk(5, ...)` fallback (K≥n), `topk(2, ...)` fallback truncation. Smoke-test verification (`bash /mydata/mvp-smoke-test/run_smoke.sh`): * `rate(http_requests_total[5m])` → 4 zones × per-zone rate (~226.74, 226.74, 209.28, 209.26 events/sec) via `data_source: asap_query` (previously empty thanos_query). * `sum by (zone) (rate(http_requests_total[5m]))` → same 4 zones via `data_source: asap_query`. * `topk(5, sum by (zone) (rate(http_requests_total[5m])))` → all 4 zones (k≥n) descending via `data_source: asap_query`. * `topk(2, ...)` → top 2 entries only. * REGRESSION: `sum by (zone) (http_requests_total)` (PR #291) and `quantile_over_time(0.99, http_requests_total_latency_ms[5m])` (PR #290) — both still return correct per-zone results via `data_source: asap_query`. `cargo test -p data_plane --lib`: 743 passed (732 baseline + 7 reducer + 4 engine), 0 failed, 2 ignored. Control plane untouched (738 passed, unchanged). `irate` falls out of this for free (the PromQL walker matches both `rate` and `irate`). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This was referenced May 18, 2026
Closed
zzylol
added a commit
that referenced
this pull request
May 18, 2026
…_fp_u64() (closes #272 step 5) (#294) The `aggregation_id()` accessor was a vestigial alias from the pre-PR-5 era when `aggregation_id` was a controller-allocated counter id distinct from the content fingerprint. After PR 5 collapsed identity into `PolicyFingerprint::from_config`, the accessor's only job became `self.policy_fingerprint().as_u64()` — returning the same u64 used as the `StreamingConfig` map key (`HashMap<u64, AggregationConfig>`). The misleading name implied a counter id; the value IS the policy fingerprint. Sweep: - `pub fn aggregation_id()` -> `pub fn policy_fp_u64()` on `AggregationConfig`. Doc comment refreshed to call out the content-addressed semantic explicitly. - All 55 call sites mechanically rekeyed via sed across asap_types (capability_matching, streaming_config, policy_fingerprint, aggregation_config tests) and data_plane (drivers/ingest/otel, drivers/query/servers/http, precompute_engine/{output_sink,worker}, query_engines/asap_query_engine/engine, storage_engines/sketch_db/{backfill/{mod,processor,service}, lifecycle/eviction}, storage_engines/types/hot_reload_config, tests/test_utilities/engine_factories). - Tracing field names inside `find_compatible_aggregation` (`agg_id=`, `chosen_agg_id=`, `value_agg_id=`, `key_agg_id=`) renamed to `policy_fp=` / `chosen_policy_fp=` / `value_policy_fp=` / `key_policy_fp=` for log-side consistency. - Two test-fn renames mirror the accessor rename: `aggregation_id_accessor_equals_fingerprint_u64` in `aggregation_config.rs` and `policy_fingerprint.rs` -> the `policy_fp_u64_*` form. Out of scope (deliberate, per #272 step 5): - `AggregationIdInfo` struct fields (`aggregation_id_for_key` / `aggregation_id_for_value`): typed accessors (`policy_fp_for_key()` / `policy_fp_for_value()`) already exist; the underlying u64 field names are stable wire-adjacent surface. - `StreamingConfig::{get_aggregation_config, contains, Index}` parameter name `aggregation_id: u64`: separate public API. - `control_plane/`: rg confirms zero `.aggregation_id()` call sites and no `pub fn aggregation_id` defs there — accessor was data-plane / asap_types only. - Local `let agg_id = ...` bindings in B7.6/B7.7-settled files (output_sink, engine_factories, backfill/mod): too deeply tangled with downstream uses to rename in this PR; values are now correctly fingerprints regardless of binding name. Verification: - `cargo build --all`: clean. - `cargo test -p asap_types --lib`: 60/60 pass. - `cargo test -p data_plane --lib`: 743/743 pass (matches #292 baseline). - `cargo test -p control_plane --lib --bins`: 748 + 28 pass (matches baseline). - `rg "\.aggregation_id\(\)" .`: 0 hits. - `rg "pub fn aggregation_id" .`: 0 hits. Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
5 tasks
zzylol
added a commit
that referenced
this pull request
May 18, 2026
…(kill PromQL string re-parse from PR #292) (#295) PR #292 fixed a real bug — the engine couldn't tell `rate(metric[r])` from `sum_over_time(metric[r])` because the analyzer collapses both to `Capability::ExactAgg(Sum)`. The fix was a `query_contains_rate_call` walker in the engine that re-parsed the raw PromQL string at dispatch time to disambiguate. That worked but was a lossy-lowering smell: the analyzer is supposed to be the single source of truth for query intent, and its lowering should preserve enough info that the engine doesn't need to re-parse. This change carries the distinction through the analyzer's typed output: - Add `OuterFn { Plain, Rate }` to `sketch_algebra::capability`. `Rate` means the original PromQL had `rate(...)` / `irate(...)` anywhere in its expression tree (possibly nested inside an outer `sum by (...) (...)`). - Add `outer_fn: OuterFn` to `ASAPTierCandidate`. The analyzer's existing `trace_from_promql` walker populates it in the same pass that captures the outer-function name / scalar args / range. - Engine's reducer dispatch in both `execute()` and `execute_range_promql_modern()` reads `candidate.outer_fn` instead of calling a `query_contains_rate_call(query)` helper. - Delete `query_contains_rate_call` (~42 lines including doc). Regression tests: - Analyzer-side: `rate_and_sum_over_time_share_capability_but_differ_on_outer_fn` + 6 per-shape outer_fn assertions, including the composed `sum by (zone) (rate(metric[r]))` case where outer fn name is `sum` but `outer_fn` MUST be `Rate`. - Engine-side: `execute_sum_over_time_dispatches_to_plain_exact_agg_reducer` pins the per-window reducer's output (sum of raw window values, NOT divided by the range — if the dispatch ever regresses to "all ExactAgg(Sum) + range > 0 → rate path", the asserted value changes by a factor of `range_seconds`). - Engine-side: `analyzer_candidate_outer_fn_distinguishes_rate_from_sum_over_time` documents the typed contract the engine reads off. `try_topk_over_rate_fallback` remains — it's a different concern (shape-extraction for topk-over-rate where the analyzer emits FrequencyTopk candidates the ExactAgg(Sum) sids don't satisfy). That's a structural analyzer-side change deferred to a follow-up; this PR's scope is the lossy-lowering smell only. Verification: - `cargo build -p data_plane` clean. - `cargo test -p data_plane --lib`: 745 passed (was 743 post-#292; +2 new regression tests). 0 failures. - `cargo test -p control_plane --lib`: 745 passed (was 738; +7 new analyzer tests). 0 failures. - Smoke test (`bash /mydata/mvp-smoke-test/run_smoke.sh` + per-query curl): all 5 spec queries return non-empty `data_source: asap_query` results: * `rate(http_requests_total[5m])` → 4 zones, per-second rates * `sum by (zone) (rate(http_requests_total[5m]))` → 4 zones * `topk(5, sum by (zone) (rate(http_requests_total[5m])))` → 4 zones * `sum_over_time(http_requests_total[5m])` → plain reducer, raw sum (25988) * `quantile_over_time(0.99, http_requests_total_latency_ms[5m])` → 4 zones p99 - `rg "query_contains_rate_call" data_plane/src/` → only doc-comment references documenting the retirement; no function call sites. 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
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 the rate-over-Sum gap PR #291 documented as the remaining blocker for the multinode demo's
topk(5, sum by (zone) (rate(http_requests_total[5m])))query. The three target shapes now land on the ExactAgg(Sum) sids the control plane mints for counter metrics (no more falling over to the archive'sNoDatastub):rate(http_requests_total[5m])→ 4 zones × per-zone rate viadata_source: asap_query.sum by (zone) (rate(http_requests_total[5m]))→ same 4 zones viadata_source: asap_query.topk(5, sum by (zone) (rate(http_requests_total[5m])))→ all 4 zones (k≥n) descending viadata_source: asap_query.What changed
Three data-plane changes (control plane untouched, per scope constraint):
SketchReducer::evaluate_exact_agg_rate(sister of feat(query): dispatch sum by (...) PromQL to ExactAgg(Sum) sids (closes Option-B downstream) #291'sevaluate_exact_agg) — folds EVERY in-window sub-window accumulator per group into ONE merged accumulator, readsStatistic::Sum, then divides byrange_secondsto produce events-per-second. Emits one sample per group, timestamped att1_ms(instant-rate semantics).ASAPQueryEnginecompose rule — when a candidate isExactAgg(Sum-family)withrange_seconds > 0AND the raw PromQL contains arate(...)/irate(...)call (walked viapromql_parser), dispatch toevaluate_exact_agg_rateinstead of the plain per-windowevaluate_exact_agg. The PromQL walker is the disambiguator the analyzer can't provide on its own (sum by (zone) (rate(metric[5m]))andsum by (zone) (sum_over_time(metric[5m]))arrive at the engine with identical(function, range_seconds, capability)and need different math).ASAPQueryEngine::try_topk_over_rate_fallback— the control-plane analyzer rewritestopk(K, sum by (gbk) (rate(metric[r])))into FrequencyTopk + FrequencyEstimate candidates (PromQLtopklowers toAggIntent::TopK; the optimizer's CMS-topk binder pins it to a frequency family). Those don't match ExactAgg(Sum) sids — the analyzer-driven path would CapabilityMiss. The fallback lifts(K, metric, group_by_keys, range_seconds, is_topk)from the raw PromQL, finds ExactAgg(Sum) sids viainstances_matching, runsevaluate_exact_agg_rate, then post-applies the descending top-K (or ascending bottom-K) slice in-engine.Plus a routing-table override:
resolve_metric_storageflipsGorilla→SketchStorewhen the table sends aRatePostHoc/Topkquery to the archive AND the sketch index has ExactAgg(Sum) sids for the metric. The control plane'sbuild_routing_entryunconditionally putsrate_post_hocon the archive's claim list (predates ExactAgg sids); flipping it back at request time avoids the control-plane churn.iratefalls out for free (the PromQL walker matches bothrateandirate).Test coverage
sketch_reducertests pinevaluate_exact_agg_rate: divide-by-range, per-group across zones, rack→zone subgroup collapse, no-gbk per-sid preservation, zero-range and MinMax unsupported-capability, empty-window NoData.asap_query_engineend-to-end tests pin the engine dispatch:rate(...)direct,sum by (zone) (rate(...)),topk(5, ...)fallback (K≥n),topk(2, ...)fallback truncation.cargo test -p data_plane --lib: 743 passed (732 baseline + 7 reducer + 4 engine), 0 failed, 2 ignored. Control plane untouched (738 passed, unchanged).Smoke-test verification
Pre-fix all three new queries returned
{"result":[],"resultType":"vector","infos":["data_source: thanos_query"]}(NoData stub on the archive slot).Test plan
cargo build -p data_plane: cleancargo test -p data_plane --lib: 743 passed, 0 failed, 2 ignoredrate(...),sum by (zone) (rate(...)),topk(5, sum by (zone) (rate(...))),topk(2, ...)all return correct values viadata_source: asap_querysum by (zone) (http_requests_total)(PR feat(query): dispatch sum by (...) PromQL to ExactAgg(Sum) sids (closes Option-B downstream) #291) andquantile_over_time(0.99, http_requests_total_latency_ms[5m])(PR refactor(control_plane): unify Replanner through typed cumulative emit; retire generate_streaming_config_yaml (Option B) #290) still work🤖 Generated with Claude Code