feat(planner): collapse topk(sum by (l) (sum_over_time)) into one CMS heap by milindsrivastava1997 · Pull Request #849 · ProjectASAP/ASAPQuery

milindsrivastava1997 · 2026-10-13T02:57:07Z

Before this PR

topk(k, sum by (l) (f(m[w]))) had no single-sketch plan.

MILP and --planner saved: they rejected it with no supported query pattern matched. That includes the docs(planner): plan the AutoSketch vs. ASAPQuery planner evaluation (§6.3) #777 classic workload's 4 topk(32, sum by (label_0) (sum_over_time(data[w]))) queries, so the saved plan's cms-heap-topk-fastpath-vector2d deployment couldn't be converted.
Legacy planner: it planned an exact sum by (l) anchor with a query-time topk.

After this PR

When f is linear under both sum and topk (sum_over_time, count_over_time), the query collapses into one CountMinSketchWithHeap keyed by l. The weighting is sum for sum_over_time and count for count_over_time, and no query-time stage runs.

promql_utilities: build_topk_of_sum_patterns() matches the three-level shape. The inner sum is collected as inner_aggregation. topk's output labels, and the labels a topk without (...) buckets over, now come from the inner sum instead of all labels. Every other shape is unchanged, because only these patterns produce the inner token.
Planner: build_patterns includes the new patterns, so both the legacy and MILP planners emit the heap. The MILP Raqe is a global TopKByValue (grouping []) with k from the outer topk. Its aggregated labels are l.
Engine: registers the same patterns, so it matches the planned query and answers from the heap.

Behavior change: the legacy planner no longer plans this shape as exact sum plus query-time topk. It's now approximate (CMS) and uses less memory. #744 described this collapse as the optimization.

Still unsupported (scope agreed on #827): sum by (job) (rate(data_total[w])) and topk(32, sum by (label_0) (rate(data_total[w]))). Both need per-series deltas of the cumulative counter before grouping (#496). They still fail loudly.

Validation

New tests:
- promql_utilities: output and bucket labels for by, without, and no inner modifier.
- Planner patterns: sum_over_time and count_over_time match; rate shapes and a non-sum inner reducer don't.
- Planner integration: one heap with aggregated labels [job], planned subquery equal to the query, empty query-time pipeline.
- MILP: the Raqe described above.
- Engine e2e (e2e_topk_of_sum_ranks_groups_from_one_planned_heap): planner config → precompute → query. The data makes the per-series top (c = 4) differ from the per-job top (api = 2 + 3). The test fails without the engine change.
cargo test for promql_utilities, asap_types, asap_planner and query_engine_rust: all pass. clippy adds no warnings and cargo fmt is clean.
--planner saved end to end: I merged this branch with feat(planner): asap-planner --planner saved writes a sketch-bench eval plan without re-solving #828 in a scratch worktree and ran it on sketch-bench's synthetic-templatesclassic-shared1 (ASAP, cpu, cheapest), with only the 8 rate queries removed (46 of 54). It wrote 6 aggregations, versus 5 in feat(planner): asap-planner --planner saved writes a sketch-bench eval plan without re-solving #828 with all 12 removed. The new one is CountMinSketchWithHeap sum, depth 3, width 1024, heap 46080, aggregated [label_0], matching the plan's deployment. All 4 sum_over_time topk queries route to it.

Part of #827. Blocks #824.

🤖 Generated with Claude Code

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…ketchWithHeap For f linear under both sum and topk (sum_over_time, count_over_time), plan one heap keyed by the inner sum's labels instead of an exact sum anchor with a query-time topk. Both the legacy and MILP planners use it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

… heap Register the shared topk-of-sum patterns so the engine matches the planner's collapsed CountMinSketchWithHeap and answers with the inner sum's labels. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

milindsrivastava1997 · 2026-10-13T03:12:13Z

+        ));
+        let r = &w.raqes[0];
+        assert_eq!(r.capability, Capability::TopKByValue);
+        assert_eq!(r.grouping_labels, LabelSet::new());


MILP: topk-of-sum Raqes are indistinguishable from other topk Raqes on the same metric, so one deployment can serve queries needing different heap keys.

build_raqe sets grouping_labels to topk_by_labels or [], and Raqe carries no aggregated/key labels. topk(32, sum by (label_0) (sum_over_time(data[15m]))) and topk(5, sum by (job) (sum_over_time(data[15m]))) (or a plain topk(5, sum_over_time(data[15m]))) all become TopKByValue, grouping [], same metric/lookback/filter. The solver can put them on one deployment, and the roll-up guard in plan_to_planner_output only compares grouping_labels, so it passes. aggregation_config then takes aggregated_labels from items.first(), so the heap is keyed by [label_0] and the by (job) query is routed to it and returns wrong results with no error. Before this PR every topk Raqe with the same grouping needed the same keys, so sharing was safe. Fix: add the key labels (req.grouping_labels) to the Raqe identity, or reject a deployment whose items disagree on them.

milindsrivastava1997 · 2026-10-13T03:12:14Z

@@ -237,7 +311,7 @@ pub fn get_topk_by_labels(
        AggregationModifierType::By => KeyByLabelNames::new(modifier.labels.clone()),


topk by (l) over an inner sum uses l as heap buckets even when the inner sum already removed l.

topk by (instance) (3, sum by (job) (sum_over_time(m[5m]))): in Prometheus every input series has no instance, so there is a single bucket and the result is the global top 3 jobs. The Without branch now intersects with topk_input_labels, but the By branch still returns [instance]. set_subpopulation_labels then plans grouping [instance] and aggregated [job], so precompute builds one heap per raw instance value holding per-(instance, job) partial sums. The query gets per-instance partial sums instead of per-job totals. Fix: intersect the by labels with topk_input_labels(match_result, all_labels).

milindsrivastava1997 · 2026-10-13T03:12:15Z

+        ));
+        let r = &w.raqes[0];
+        assert_eq!(r.capability, Capability::TopKByValue);
+        assert_eq!(r.grouping_labels, LabelSet::new());


The MILP prices and sizes the topk-of-sum CMS heap from the grouping [] data shape, not from the cardinality of the inner sum by keys.

require_shapes and the accuracy curves look up data_shape[raqe.grouping_labels], which is [] for every global topk. The CMS for topk(32, sum by (label_0) ...) holds one key per label_0 value, not one per series. The accuracy/cost estimate, and so the chosen depth/width/heap, is the same as for a per-series heap. That can over-provision a few-key heap or under-provision a many-key heap without any warning.

milindsrivastava1997 · 2026-10-13T03:12:17Z

-    // Patching: When the query is topk, we should always return all labels
+    // topk is a selector: it keeps its input's labels, which are all labels
+    // unless an inner `sum by (...)` reduced them first.
    if aggregation_token.op.parse::<AggregationOperator>() == Ok(AggregationOperator::Topk) {


Doc comments still say topk output labels are always all labels.

get_topk_by_labels's doc (line ~288) still says get_spatial_aggregation_output_labels "always returns all labels for topk regardless of any modifier", and the engine's extract_topk_param doc says topk is "excluded from every collapsable spatial-of-temporal pattern". Both are wrong after this PR and will mislead the next person who changes the topk label logic.

milindsrivastava1997 · 2026-10-13T03:12:18Z

+/// CountMinSketchWithHeap keyed by `l`.
+pub fn build_topk_of_sum_patterns() -> Vec<PromQLPattern> {
+    [
+        PromQLFunction::Rate,


build_topk_of_sum_patterns adds a third copy of the temporal-function list and the pattern-building code that the planner and engine already duplicate.

planner/patterns.rs and simple_engine/mod.rs each hard-code the same [Rate, Increase, SumOverTime, ...] list and the same collapsable aggregation(function(matrix_selector)) builder. This PR moves only the new pattern into promql_utilities (and into query_logics/parsing, a module for parsing helpers, not pattern builders), leaving three lists to keep in sync. Moving the full ONE_TEMPORAL_ONE_SPATIAL and TOPK_OF_SUM builders into one shared promql_utilities function would let the planner and engine share them, so they can't drift apart on which shapes they accept.

milindsrivastava1997 · 2026-10-13T03:12:18Z

-    /// than the bucketing labels (#714).
+    /// *output* label set (all labels, or an inner `sum by (...)`'s labels;
+    /// unaffected by topk's own modifier) rather than the bucketing labels
+    /// (#714).


Changed comments keep issue-number references, which AGENTS.md (pulled in by CLAUDE.md) forbids.

The repo CLAUDE.md says "See AGENTS.md for process conventions". AGENTS.md says: "Code comments should not include git history, or issue numbers or historical facts." The rewritten comments still carry (#714) (query_requirements.rs:49) and (collapsable, see #508) (query_requirements.rs:101).

`topk by (instance) (k, sum by (job) (...))` ranks series that no longer carry `instance`, so it is one bucket, not a heap per raw `instance` of partial sums. Intersect `by` labels with topk's input labels, and report no bucket labels left as a single ranking. Also correct doc comments that still said topk outputs all labels, and drop issue references from them. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…abels A Raqe doesn't carry a keyed sketch's key labels, so the solver can put topk(sum by (job) ...) and topk(sum by (instance) ...) on one heap deployment; the config then keyed the heap by the first query's labels and answered the other wrongly. Derive each served query's labels and error when they disagree. Also note that the heap is priced from the bucket grouping's shape, not its key count. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…ner and engine The planner and the engine each built the same ONE_TEMPORAL_ONE_SPATIAL patterns from their own copy of the temporal function list, and the topk-of-sum builder sat in the parsing module as a third copy. Move both builders into ast_matching::collapsable_patterns, so the two sides can't drift apart on which shapes they accept. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…-the-777-evals-sum-of-rate-and-nested-topk-queries

plan_to_planner_output now takes a MilpConfig on main. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

milindsrivastava1997 · 2026-10-13T13:13:29Z

+            modifier
+                .labels
+                .iter()
+                .filter(|label| input_labels.labels.contains(label))


Regression for plain topk by. This now drops any by label not in the input labels, so plain topk by (job) (5, x) on a metric missing from the schema gets topk_by_labels = None (it was Some([job]) before). build_query_requirements_promql uses empty labels for unknown metrics, and MILP's schema comes from config.schema_from_hints(), which is empty when metrics: is omitted. item_to_raqe then emits a global TopK Raqe with grouping [], and the query silently returns the global top 5 instead of the top 5 per job. That contradicts "every other shape is unchanged". Suggest intersecting only when there is an inner aggregation, or only when input_labels is non-empty.

Fixed in 3ab6e4a. Only a topk-of-sum intersects by labels with the inner sum's output. Plain topk by keeps its labels as written, so a metric missing from the schema still buckets by job. Regression test: plain_topk_by_keeps_its_labels_for_a_metric_missing_from_the_schema.

milindsrivastava1997 · 2026-10-13T13:13:29Z

+    // bucket grouping's shape. Key count is bounded by the series count, but
+    // accuracy curves measured per series may not match summed values; needs
+    // key labels in sketch-bench's Raqe.
    let grouping_labels: LabelSet = match capability {


The solver can merge differently keyed topk queries. A topk-of-sum Raqe has grouping [] and no key labels, so topk(5, sum by (job) (sum_over_time(m[1m]))) and topk(5, sum by (instance) (sum_over_time(m[1m]))) (or one of them plus a plain topk(5, sum_over_time(m[1m]))) produce Raqes that look identical apart from their id. The solver shares one heap because that is cheaper, then aggregation_config fails the whole plan with DeploymentLabelsDisagree, so MILP / --planner saved emits no configs. The new check guards the symptom. The root fix is to put the key labels into the Raqe identity (or split deployments by key labels at output) instead of erroring.

Fixed in ec2fda3. Instead of failing the plan, the output now builds one aggregation per (deployment, labels a served query needs). Differently keyed topk-of-sum queries that the solver put on one deployment each get their own correctly keyed heap. The remaining gap is that the plan prices one sketch where two are deployed. The root fix, key labels in the Raqe, is tracked in #854. Test: topk_of_sum_queries_needing_different_heap_keys_get_separate_heaps.

milindsrivastava1997 · 2026-10-13T13:13:29Z

+    let requirements = &item.requirements;
+    let mut grouping_labels = KeyByLabelNames::empty();
+    let mut aggregated_labels = KeyByLabelNames::empty();
+    set_subpopulation_labels(


(Applies to planner/labels.rs:27, which is not in this diff.) For topk by (job) (3, sum by (job) (sum_over_time(m[5m]))), the bucket labels equal the inner sum labels, so grouping is [job] and aggregated is []. Each bucket holds exactly one series, so topk does nothing, yet we allocate a full depth x width CMS plus a k-sized heap per job value. Memory scales with job cardinality for no benefit. When the bucket labels cover all the inner labels, collapse to a single global heap keyed by the inner labels, or to an exact sum.

Agreed, but it's a degenerate shape: topk by (l) over sum by (l) returns every group. Plain topk by (<all labels>) already has the same waste. I've noted it in #855 for later; when it matters, plan it as an exact sum by (l).

milindsrivastava1997 · 2026-10-13T13:13:29Z

+/// `topk(k, sum by (l) (func(metric[w])))`, one per `func` collapsable under
+/// both `sum` and `topk`: such a query collapses into one
+/// CountMinSketchWithHeap keyed by `l`.
+pub fn build_topk_of_sum_patterns() -> Vec<PromQLPattern> {


This routes topk(k, sum by (l) (sum_over_time|count_over_time)) unconditionally to an approximate CountMinSketchWithHeap, with no exact fallback, while topk(k, sum by (l) (m)) without a temporal function stays exact (sum anchor + query-time topk). A legacy-planner workload that was exact for topk-of-sum now silently gets CMS over-estimates and heap misses, and two near-identical shapes carry different exactness guarantees. Should this be opt-in, or gated on an accuracy/size config?

Intended: this is the collapse #744 asked for, and the PR description calls it out as a behaviour change. The PromQL legacy planner has no exact/approximate switch for top-k, and plain topk(k, m) is already always CMS+heap. So topk-of-sum now matches plain topk, and topk(sum by (l) (m)) without a temporal function is the shape still on the old path. Gating it would need its own config design; I'm leaving it out of this PR.

milindsrivastava1997 · 2026-10-13T13:13:29Z

        ];
-        controller_patterns.extend(one_temporal_one_spatial_collapsable_patterns);
+        controller_patterns.extend(build_one_temporal_one_spatial_patterns());
+        controller_patterns.extend(build_topk_of_sum_patterns());


Only the collapsable patterns moved to the shared module. The ONLY_TEMPORAL and ONLY_SPATIAL patterns are still built separately here (temporal_pattern_blocks / spatial_pattern) and in the planner (patterns.rs build_patterns). The PR's reason for sharing is that the planner and engine must accept the same shapes, but those lists can still drift. Consider moving the whole ordered build_patterns() into promql_utilities and calling it from both sides.

Agreed. The ONLY_TEMPORAL/ONLY_SPATIAL patterns are built differently in the engine (from a block map), so sharing them needs its own equivalence check. Filed as #855 rather than growing this PR.

…them Intersecting by labels with topk's input labels turned plain topk by (job) on a metric missing from the schema (no known labels) into a global top k. Only a topk-of-sum's inner sum removes labels, so intersect only there. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…ailing the plan A MILP deployment shared by topk-of-sum queries keyed by different labels used to fail the whole plan. Build one aggregation per (deployment, labels a served query needs), so each query reads a correctly keyed heap. The plan still prices only one sketch for such a deployment. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…-the-777-evals-sum-of-rate-and-nested-topk-queries # Conflicts: # asap-planner-rs/src/optimizer/milp_output.rs

milindsrivastava1997 and others added 3 commits October 12, 2026 22:41

feat(promql-utils): derive topk-of-sum output labels from the inner sum

d24b6a1

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

milindsrivastava1997 linked an issue Oct 13, 2026 that may be closed by this pull request

feat(planner): serve the #777 eval's sum-of-rate and nested topk queries #827

Closed

milindsrivastava1997 commented Oct 13, 2026

View reviewed changes

milindsrivastava1997 and others added 3 commits October 13, 2026 08:57

milindsrivastava1997 mentioned this pull request Oct 13, 2026

MILP: Raqe lacks a keyed sketch's key labels (topk-of-sum heap pricing and sharing) #854

Open

milindsrivastava1997 and others added 2 commits October 13, 2026 09:05

Merge remote-tracking branch 'origin/main' into 827-featplanner-serve…

5ad1d37

…-the-777-evals-sum-of-rate-and-nested-topk-queries

fix(planner): pass MilpConfig in the shared-heap-deployment test

bc5fdfc

plan_to_planner_output now takes a MilpConfig on main. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

milindsrivastava1997 commented Oct 13, 2026

View reviewed changes

milindsrivastava1997 and others added 2 commits October 13, 2026 09:17

milindsrivastava1997 mentioned this pull request Oct 13, 2026

Share the full PromQL pattern list between planner and engine #855

Open

Merge remote-tracking branch 'origin/main' into 827-featplanner-serve…

72a22e3

…-the-777-evals-sum-of-rate-and-nested-topk-queries # Conflicts: # asap-planner-rs/src/optimizer/milp_output.rs

milindsrivastava1997 merged commit b101a35 into main Oct 13, 2026
7 checks passed

milindsrivastava1997 mentioned this pull request Oct 14, 2026

Tracker: run #777 eval plans end to end from saved plans #824

Closed

6 tasks done

		@@ -237,7 +311,7 @@ pub fn get_topk_by_labels(
		AggregationModifierType::By => KeyByLabelNames::new(modifier.labels.clone()),

Conversation

milindsrivastava1997 commented Oct 13, 2026

Before this PR

After this PR

Validation

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!

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!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant