feat(query-engine): execute nested PromQL aggregation DAGs by milindsrivastava1997 · Pull Request #770 · ProjectASAP/ASAPQuery

milindsrivastava1997 · 2026-10-04T03:04:42Z

Closes #738

Summary

plan PromQL aggregation chains as explicit query-time stages
execute each range stage as a visible AggregateVector DAG node
carry the resulting label schema through the native DAG to response formatting
preserve plain-selector TopK metric-name labels through nested stages
keep grouped instant TopK buckets contiguous and ranked within each bucket
fall back to Prometheus when a later by-stage requires a label removed by an earlier stage

Verification

cargo test -p query_engine_rust
full pre-commit suite
make run DATASET=../datasets/nested-aggregations.yaml SUITE=../suites/nested-aggregations.yaml (90/90 passed)

Docker compliance matrix

Current make run-all result (2026-10-04):

Suite	Result	Notes
single-rate-temporal	Fails	Pre-existing window mismatch
single-rate-off-grid-rate	Fails	Pre-existing off-grid native-rate behavior
aggregations-native-dag	Passes
aggregations	Fails	Pre-existing unsupported rate/nested temporal shapes and avg_over_time
topk-ordering	Passes	Confirms grouped instant TopK bucket contiguity and within-bucket ordering against Prometheus
quantiles	Passes
olly-bench	Fails as expected	Non-CI suite

The command exits non-zero because it includes the listed known failures; the new TopK ordering suite passes.

milindsrivastava1997 · 2026-10-04T03:10:11Z

+                    return Ok(None);
+                };
+                let Some((anchor_labels, anchor_result)) =
+                    self.execute_context_result(context, false, false)?


The anchor runs with topk limiting and formatting both off (false, false). If the planned subquery is itself a topk (e.g. sum(topk(3, x)) → anchor topk(3, x) + sum), it's never truncated to k, so the outer sum adds every candidate series instead of the top 3, and it groups over unformatted labels. The same applies to the range path at L1454.

milindsrivastava1997 · 2026-10-04T03:10:11Z

+                else {
+                    return Ok(None);
+                };
+                let output = self.execute_observed_range_query_pipeline(


This calls execute_observed_range_query_pipeline(...)? directly and skips map_local_execution_outcome, which the instant path and the non-nested range path both go through. When the local store has no data for the window, NoLocalData becomes an HTTP 500 instead of Ok(None), so the request is never forwarded to the fallback Prometheus.

milindsrivastava1997 · 2026-10-04T03:10:11Z

+                labels: labels.labels.clone(),
+            }
+        }
+        _ => QueryTimeGrouping {


without () with no labels falls through to QueryTimeGroupingMode::All, but in PromQL it keeps every label. For example, sum without () (sum by (job, instance) (x)) should return one series per (job, instance), but here it returns a single summed series. Map it to Without with an empty list, or mark it unsupported.

milindsrivastava1997 · 2026-10-04T03:10:11Z

+
+    let labels = match aggregation.grouping.mode {
+        QueryTimeGroupingMode::All => Vec::new(),
+        QueryTimeGroupingMode::By => aggregation.grouping.labels.clone(),


A by label that isn't in the input label set is treated as an error here, but in Prometheus it's valid and produces an empty label value. As a result, sum by (job) (sum by (instance) (x)) passes planning but fails when the plan is compiled. The client gets a 500 instead of one series with job="", and the request isn't forwarded to Prometheus.

milindsrivastava1997 · 2026-10-04T13:56:30Z

+                            planned_subquery = %config.planned_subquery,
+                            "configured query-time aggregation anchor does not parse: {error}"
+                        );
+                        return Ok(None);


Triaged: agreed. A matching configured query with an unparsable planned_subquery is a corrupt static execution plan, not a capability miss. The instant and range paths should return QueryExecutionError::Native, with regressions for both.

milindsrivastava1997 · 2026-10-04T13:56:51Z

+            group.truncate(k);
+            results.extend(group);
+        }
+        results.sort_by(|left, right| left.labels.labels.cmp(&right.labels.labels));


Triaged: agreed. The final label sort discards the descending-value order established above. Query-time TopK must return selected instant-vector elements in descending value order, using the full label set only as the tie-breaker. Range correctness remains per timestamp; a matrix cannot encode a different global rank order at each timestamp.

milindsrivastava1997 · 2026-10-04T14:09:44Z

+            else {
+                return None;
+            };
+            if number.val < 0.0 || number.val.fract() != 0.0 {


Triaged: the loadability issue is real. The planner must omit nested plans for topk(0) and quantile values outside [0, 1], so those queries fall back to Prometheus. However, without () is valid PromQL and explicitly supported by #738: it must remain Without([]). The correct companion fix is to allow Without([]) in QueryTimeAggregation validation, not map it to All.

milindsrivastava1997 · 2026-10-04T14:22:26Z

+        "max" => QueryTimeAggregationOperator::Max,
+        "quantile" => QueryTimeAggregationOperator::Quantile,
+        "topk" => QueryTimeAggregationOperator::Topk,
+        _ => return None,


Triaged: no change for sort_desc. The #738 contract defines the supported query-time operators as sum, count, avg, min, max, quantile, and topk; sort_desc is not included and is a vector-sorting function rather than an aggregation. The separate TopK ordering defect is being fixed.

milindsrivastava1997 · 2026-10-04T15:29:26Z

+                    return Ok(None);
+                };
+                let Some((anchor_labels, anchor_result)) =
+                    self.execute_context_result(context, true, false)?


When the anchor is a topk over a bare metric (e.g. topk(3, transfer_events)), anchor_labels starts with __name__, but running it with formatting off yields rows without a __name__ value, so labels and row values are misaligned by one.

sum by (srcip) (topk(3, transfer_events)) / sum without (...): index lookup for srcip hits position 1 on a 1-value row → "result labels do not match the configured label schema" error instead of an answer or Prometheus fallback.

topk(1, topk(3, transfer_events)): convert_query_result_to_prometheus zips them, giving __name__="10.0.0.x" and no srcip.

Same issue on the range path (query_plan.rs:155). The new test only covers plain sum(...), which drops all labels and so masks this — worth adding a by (...) case.

milindsrivastava1997 · 2026-10-04T15:29:26Z

+        .iter()
+        .map(|index| {
+            index.map_or_else(
+                || Ok(String::new()),


A grouping label missing from the inner result becomes "" and is emitted as a real label. E.g. max by (instance) (sum by (job) (x)) returns {instance=""}, whereas Prometheus returns {}. This will diverge in Prometheus comparisons (e.g. the compliance suite). Missing labels should be dropped from the output series rather than set to empty.

…regations # Conflicts: # asap-planner-rs/src/optimizer/translator.rs

milindsrivastava1997

Code review: 3 findings inline (1 high, 1 medium, 1 low).

milindsrivastava1997 · 2026-10-05T13:27:13Z

-            results.sort_by(|a, b| {
-                Self::cmp_topk_value_desc(a.value, &a.labels.labels, b.value, &b.labels.labels)
-            });
+            Self::sort_instant_topk_results(


High: grouped bare-selector topk fails (or sorts the wrong column) when enable_topk_formatting=false.

For a bare-selector topk (keep_metric_name), query_output_labels starts with __name__, but with enable_topk_formatting=false the metric name is never added to the result values, so every grouping-label position is off by one.

E.g. topk by (job) (3, x): labels [__name__, instance, job] put job at index 2, but the values are [instance, job], so get(2) is None → "Topk result labels do not match the configured output schema". If the grouping label isn't last, there's no error, but the sort uses the wrong column.

Callers that pass false:

the binary-arm instant path (promql.rs:510, e.g. topk by (job) (3, x) + 0), which worked before this PR

the new nested instant path (execute_context_result(context, true, false), e.g. sum(topk by (job) (3, x)))

Fix: strip the leading __name__ before computing positions, as topk_row_label_order already does.

milindsrivastava1997 · 2026-10-05T13:27:13Z

+                let planned_subquery = query_data
+                    .get("planned_subquery")
+                    .and_then(|v| v.as_str())
+                    .ok_or_else(|| anyhow::anyhow!("Missing planned_subquery field"))?


Medium: making planned_subquery / query_time_aggregations required breaks the checked-in inference configs.

These configs have neither field and now fail with "Missing planned_subquery field":

asap-query-engine/examples/promql/inference_config.yaml

asap-query-engine/examples/sql/inference_config.yaml

asap-tools/execution-utilities/elastic-asap-benchmarking/inference_config.yaml

src/bin/test_e2e_precompute.rs loads the PromQL example directly, so that binary breaks right away. Older planner outputs fail the same way.

Fix: add the fields to the configs, or default them (planned_subquery = query, empty pipeline).

milindsrivastava1997 · 2026-10-05T13:27:13Z

+    let indices = label_indices(input_labels, &grouping_labels);
+    let mut groups: BTreeMap<Vec<String>, Vec<InstantVectorElement>> = BTreeMap::new();
+    for element in input {
+        if !element.value.is_finite() {


Low: a NaN/Inf from the inner query fails the whole request.

Here and in aggregate_values (line 108), any non-finite input returns an error. That reaches the caller as QueryExecutionError::Native, not as a "no local data" result that falls back to Prometheus.

E.g. if one series of sum by (job) (...) or a topk sketch yields NaN or ±Inf, max(...) or topk(3, ...) over it fails. Prometheus would return a result (NaN goes through sum/max, and topk ranks it).

Fix: pass NaN/Inf through the way PromQL does, or return Ok(None) so the query falls back.

milindsrivastava1997 added 15 commits October 3, 2026 21:54

feat(planner): add explicit query execution plans

690282d

feat(planner): emit nested aggregation plans

8d0de9d

feat(query-engine): validate query-time aggregation configs

c42ef98

test(planner): cover nested aggregation stages

827414c

feat(query-engine): execute query-time aggregation pipelines

ad9182f

test(query-engine): add nested aggregation e2e tracer

3b2763d

test(query-engine): cover nested topk e2e

77b6d9c

test(query-engine): cover nested aggregation operators e2e

a95971c

test(query-engine): assert nested aggregation e2e values

467d149

test(promql): add nested aggregation differential suite

cf48026

test(query-engine): cover query-time aggregation grouping

f7c3b87

fix(query-config): reject zero topk limits

0f74902

test(query-engine): cover query-time aggregation boundaries

1313532

test(query-engine): cover query-time topk ties

ff280a0

feat(query-engine): execute nested aggregation DAG nodes

bbc7f0a

milindsrivastava1997 commented Oct 4, 2026

View reviewed changes

milindsrivastava1997 added 3 commits October 3, 2026 23:17

fix(query-engine): preserve missing query-time labels

b1e5cc9

fix(planner): preserve empty without aggregations

cd5f31e

fix(query-engine): apply nested topk anchors first

35a424f

milindsrivastava1997 commented Oct 4, 2026

View reviewed changes

fix(query-engine): validate nested aggregation plans

072ef1e

milindsrivastava1997 commented Oct 4, 2026

View reviewed changes

milindsrivastava1997 added 4 commits October 4, 2026 13:20

fix(query-engine): preserve nested topk labels

b0a14f9

Merge remote-tracking branch 'origin/main' into 738-nested-promql-agg…

c0eeb1d

…regations # Conflicts: # asap-planner-rs/src/optimizer/translator.rs

fix(query-engine): keep grouped topk buckets contiguous

353d828

Merge remote-tracking branch 'origin/main' into 738-nested-promql-agg…

80abed1

…regations # Conflicts: # asap-planner-rs/src/optimizer/translator.rs

milindsrivastava1997 commented Oct 5, 2026

View reviewed changes

fix(query-engine): align raw topk labels

d450f5c

milindsrivastava1997 merged commit cbabfd6 into main Oct 5, 2026
10 checks passed

milindsrivastava1997 deleted the 738-nested-promql-aggregations branch October 5, 2026 14:12

Conversation

milindsrivastava1997 commented Oct 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Verification

Docker compliance matrix

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

milindsrivastava1997 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

milindsrivastava1997 commented Oct 4, 2026 •

edited

Loading