feat(query): outer aggregation operators on function results (closes #296) - #297
Merged
zzylol merged 1 commit intoMay 18, 2026
Merged
Conversation
…296) 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>
This was referenced May 19, 2026
fix(emit): inject cumulativetodelta upstream of agent routing for Counter metrics (closes #298)
#299
Merged
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
Closes #296. The asap PromQL engine now composes
max/min/avg/count/group/stddev/stdvar by (...)wrapping a function result (quantile_over_time,rate,sum_over_time, ...). Previously these errored with{"error":"No result for query"}on asap tier while baselines (VictoriaMetrics) answered them 100%.Changes
OuterAggenum (control_plane/src/sketch_algebra/capability.rs) — taxonomy mirroringOuterFnfrom refactor(query): preserve rate/sum_over_time distinction in analyzer (kill PromQL string re-parse from PR #292) #295. Variants for Max/Min/Avg/Count/Group/Stddev/Stdvar, each carryingby-labels.sumintentionally excluded (has its ownExactAgg(Sum)dispatch).outer_aggfield onASAPTierCandidate+ analyzer extraction.extract_outer_agglifts the outermost aggregation operator. Key fix: whenouter_agg.is_some(), the walker descends INTO the aggregate's inner expression (via newunwrap_outermost_aggregatehelper) so it captures the INNER function name (quantile_over_time), not the outer operator name (max). Without this descent the engine's reducer dispatched against"max"as a function name and CapabilityMissed to archive.apply_outer_agg_foldhelper + engine dispatch. After the inner reducer emits per-row results, projects each row's labels onto theOuterAgg.by_labels(), groups, folds. Wired into bothexecute(&str)instant path andhandle_range_query_promqlrange path.Identity case (asap's per-zone sketches: each
by-group has 1 row) falls out naturally — fold returns the single value unchanged.Coverage
cargo test -p control_plane --lib: 775 passed (+30 new analyzer/capability tests)cargo test -p data_plane --lib: 752 passed (+7 new outer_agg integration/fold tests)data_source: asap_query:Out of scope (follow-up)
quantile()instant aggregator over function results — needs per-group sketch merging, not a scalar fold.without (labels)modifier — analyzer maps toOuterAgg::None(falls through to archive); proper handling needs inner-result label-universe knowledge.count by (...)overquantile_over_timeroutes tothanos_queryarchive via the storage router (count-of-quantile is semantically cardinality, dispatched separately) — that's working as intended for the storage routing, not a bug here.max by (a) (avg by (b) (X))) — captures only the outermost agg.Closes #296