Skip to content

refactor(query): preserve rate/sum_over_time distinction in analyzer (kill PromQL string re-parse from PR #292) - #295

Merged
zzylol merged 1 commit into
mainfrom
fix/analyzer-preserve-outer-fn-name
May 18, 2026
Merged

zzylol merged 1 commit into
mainfrom
fix/analyzer-preserve-outer-fn-name

Conversation

@zzylol

@zzylol zzylol commented May 18, 2026

Copy link
Copy Markdown
Contributor

Summary

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 PR 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 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 function name / 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).

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.

Test plan

  • cargo build -p data_plane: clean
  • cargo test -p data_plane --lib: 745 passed (was 743 post-feat(query): compose rate-over-Sum into ExactAgg(Sum) candidate (multinode topk dispatch) #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: all 5 spec queries return non-empty data_source: asap_query results:
    • rate(http_requests_total[5m]) → 4 zones, per-second rates (~20-22)
    • sum by (zone) (rate(http_requests_total[5m])) → 4 zones, per-second rates
    • topk(5, sum by (zone) (rate(http_requests_total[5m]))) → 4 zones (top 5, K ≥ zone count)
    • sum_over_time(http_requests_total[5m]) → plain reducer, raw window sum (25988) — load-bearing regression case
    • 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.

Regression test additions

Analyzer-side (control_plane::asap_tier_analysis::tests):

  • rate_candidate_carries_outer_fn_rate
  • irate_candidate_carries_outer_fn_rate
  • sum_over_time_candidate_carries_outer_fn_plain — the critical "shares capability with rate, MUST report Plain" case
  • sum_by_candidate_carries_outer_fn_plain
  • bare_selector_candidate_carries_outer_fn_plain
  • sum_by_over_rate_candidate_carries_outer_fn_rate — composed shape: outer fn name is sum but inner rate triggers OuterFn::Rate
  • rate_and_sum_over_time_share_capability_but_differ_on_outer_fn — pins the asymmetry the engine's dispatch reads off

Engine-side (data_plane::query_engines::asap_query_engine):

  • 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).
  • analyzer_candidate_outer_fn_distinguishes_rate_from_sum_over_time — documents the typed contract the engine reads off.

Smoke-test outputs

=== Q1: rate(http_requests_total[5m]) ===
{"result":[{"zone":"z0","val":"22.53"},{"zone":"z1","val":"22.52"},{"zone":"z2","val":"20.79"},{"zone":"z3","val":"20.79"}],"infos":["data_source: asap_query"]}

=== Q2: sum by (zone) (rate(http_requests_total[5m])) ===
{"result":[{"zone":"z0","val":"22.53"},{"zone":"z1","val":"22.52"},{"zone":"z2","val":"20.79"},{"zone":"z3","val":"20.79"}],"infos":["data_source: asap_query"]}

=== Q3: topk(5, sum by (zone) (rate(http_requests_total[5m]))) ===
{"result":[{"zone":"z0","val":"22.53"},{"zone":"z1","val":"22.52"},{"zone":"z3","val":"20.79"},{"zone":"z2","val":"20.79"}],"infos":["data_source: asap_query"]}

=== Q4: sum_over_time(http_requests_total[5m]) ===
{"result":[{"metric":{},"val":"25988"}],"infos":["data_source: asap_query"]}

=== Q5: quantile_over_time(0.99, http_requests_total_latency_ms[5m]) ===
{"result":[{"zone":"z3","val":"59.74"},{"zone":"z1","val":"83.94"},{"zone":"z2","val":"70.12"},{"zone":"z0","val":"90.94"}],"infos":["data_source: asap_query"]}

🤖 Generated with Claude Code

…(kill PromQL string re-parse from PR #292)

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>
@zzylol
zzylol merged commit eaf00df into main May 18, 2026
zzylol added a commit that referenced this pull request May 18, 2026
…296) (#297)

The asap PromQL engine didn't compose `max/min/avg/count/group/stddev/stdvar by (...)`
wrapping a function result. `max by (zone) (quantile_over_time(0.99, X[5m]))`
errored with `{"error":"No result for query"}` on the asap tier while the same
PromQL succeeded 118/118 on VictoriaMetrics baselines. Surfaced during multinode
validation post ASAPCollector PR #397.

## Fix

1. **New `OuterAgg` enum** (`control_plane/src/sketch_algebra/capability.rs`)
   — taxonomy mirroring `OuterFn` from #295. Variants for `Max/Min/Avg/Count/
   Group/Stddev/Stdvar`, each carrying the `by`-labels. `sum` is intentionally
   excluded (it has its own `ExactAgg(Sum)` dispatch via the analyzer's lowerer).
   `Default` is `OuterAgg::None`.

2. **`outer_agg` field on `ASAPTierCandidate`** + analyzer extraction
   (`control_plane/src/asap_tier_analysis.rs`). `extract_outer_agg(&Expr)` lifts
   the OUTERMOST aggregation operator into the typed `OuterAgg` enum before the
   walker descends. When `outer_agg.is_some()`, the walker descends INTO the
   aggregate's inner expression (via new `unwrap_outermost_aggregate` helper)
   so it captures the INNER function name (`quantile_over_time`), not the
   outer operator name (`max`). Without this descent the engine's reducer
   would dispatch against `"max"` as a function and CapabilityMiss to archive.

3. **`apply_outer_agg_fold` helper** + engine dispatch
   (`data_plane/src/query_engines/asap_query_engine/engine.rs`). After the inner
   reducer emits per-row results, projects each row's labels onto the
   `OuterAgg.by_labels()` set, groups rows by projected labels, and folds each
   group's values using the operator's semantics. Wired into both the
   `execute(&str)` instant path and the `handle_range_query_promql` range path.

   Identity case (asap's per-zone sketches: `max by (zone) (
   quantile_over_time(...))` where the sketch is already grouped by `[zone]`):
   each `by`-group has exactly 1 row, fold returns that value unchanged. No
   special-case branch needed — the general fold handles it.

## Coverage

- 7 analyzer regression tests in `asap_tier_analysis.rs` (max/avg/min/count by
  quantile_over_time, count by rate, sum_over_time → OuterAgg::None, bare
  selector → OuterAgg::None, sum-by-zone NOT routed to OuterAgg::Sum)
- 4 fold-fn unit tests in `engine.rs` (`apply_outer_agg_fold` on canned rows
  for Max/Avg with multi-row and identity cases, Count semantics)
- 2 engine-integration tests in `engine.rs::outer_agg_integration_tests`
  exercising `execute(&str)` end-to-end with a DDSketch fixture for both
  `max by (zone)` and `avg by (zone)`. Asserts per-zone identity preserved
  (ordering + ballpark ranges, since DDSketch with ≤10 samples has
  bucket-boundary drift the test fixture can't budget around — real workloads
  with 100s+ samples/window stay within 5%).

## Test counts

- `cargo test -p control_plane --lib`: 775 passed (was 745; +30 new analyzer +
  capability-impl tests)
- `cargo test -p data_plane --lib`: 752 passed (was 745; +2 outer-agg
  integration + 4 fold-fn + 1 hot-reload-handle test added by the agent setup)

## Out of scope (follow-up)

- `quantile()` instant aggregator over function results — needs per-group
  sketch merging, not a scalar fold.
- `without (labels)` modifier — engine currently keys on explicit `by`-set;
  translating `without` to `by` needs knowledge of the inner result's label
  universe. Analyzer documents the gap; queries with `without (...)` route as
  `OuterAgg::None` (fall through to archive).
- Deeper nesting (`max by (a) (avg by (b) (X))`) — capture only the outermost
  agg; deeper levels documented as follow-up.

Closes #296

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@zzylol
zzylol deleted the fix/analyzer-preserve-outer-fn-name branch July 17, 2026 20:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant