feat(planner): MILP roll-ups for KLL, DDSketch and HLL by milindsrivastava1997 · Pull Request #851 · ProjectASAP/ASAPQuery

milindsrivastava1997 · 2026-10-13T03:00:27Z

Part 4/5 of #839. Stacked on #850. Stack: #847 → #848 → #850 → MILP sketches (this) → MILP exact types.

What

plan_to_planner_output no longer refuses every roll-up.

Roll-ups allowed: when a query's grouping Q is a strict subset of its deployment's grouping G and the deployment is DatasketchesKLL, DDSketch or HLL, the query_config points the query at the G-grouped aggregation. The engine infers the roll-up (feat(query-engine): roll up single-subpopulation aggregations to a coarser grouping #848).
Labels: aggregation_config now takes its labels from the deployment's grouping instead of the first item it serves. The old code would have deployed at the wrong grouping whenever the first query served was the coarse one. Top-k is the exception: it keeps its topk by handling.
Errors:
- A roll-up of any other type is still RollupUnsupported. The error now carries the aggregation type and names the types that can roll up.
- A query grouping that's not a subset of the deployment's is GroupingOutsideDeployment.

Tests

Allowed: a table-driven test for KLL, DD and HLL by (job) from a deployment grouped (instance, job).
Refused: an exact-sum roll-up, and a non-subset grouping.
Shared deployment: two queries on one deployment, one at its grouping and one rolled up, produce a single aggregation at (instance, job) in either order.
Checked against the old labels: both the sketch roll-up test and the shared-deployment test fail with the first-item labels restored.

⚠️ Note: the pinned sketch-bench never proposes roll-ups

Cargo.lock pins sketch-bench 30e725a, where candidate generation requires the deployment's grouping to equal the query's, and mergeable_across_groups doesn't exist yet. Until rqe-optimizer is bumped, these paths are reached only by tests that edit the solution, as the old rollup_is_refused did. The bump is left to a separate deps PR.

🤖 Generated with Claude Code

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

+        });
+    }
+    match aggregation_type {
+        AggregationType::DatasketchesKLL | AggregationType::DDSketch | AggregationType::HLL => {


The set of roll-up-able types is now spelled out in three places: this match arm, the RollupUnsupported message string (line 53), and the engine's ROLLUP_AGGREGATION_TYPES in simple_engine/mod.rs. If the engine adds or drops a type, the planner either keeps refusing a roll-up the engine can serve or emits configs the engine then rejects at query time, and the error text drifts too. Consider moving the list into promql_utilities (next to does_precompute_operator_support_subpopulations), using it in both crates, and formatting the message from it.

Fixed in 3859ddb. AggregationType::merges_across_groups() (in promql_utilities) is now the single list. The engine's roll-up check and the MILP's check_rollup both use it, and the RollupUnsupported message is built from it. #852 adds the exact Multiple* roll-ups by regrouping at query time, which is a separate mechanism, so they're intentionally not in this list.

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

-        "query {query:?} is grouped by {raqe:?} but served by a deployment grouped by \
-         {deployment:?}: a roll-up, which the engine can't read yet"
+        "query {query:?} is grouped by {raqe:?} but served by a {aggregation_type} deployment \
+         grouped by {deployment:?}: the engine rolls up only DatasketchesKLL, DDSketch and HLL"


This message says "the engine rolls up only DatasketchesKLL, DDSketch and HLL", but the engine's ROLLUP_AGGREGATION_TYPES also includes Sum and MinMax. It's really the planner's own rule. For a top-k or exact roll-up the message points the reader at the engine as the limit. Suggest rewording to "this planner deploys roll-ups only for …", or building the text from a shared list.

Fixed in 3859ddb. The message no longer blames the engine. It now says only aggregations that merge across groups roll up, and lists them from the shared constant.

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

    let mut queries: IndexMap<String, QueryPlanEntry> = IndexMap::new();
    for (raqe, planned) in solution.raqes.iter().enumerate() {
        let key = &deployment_keys[planned.deployment];
+        check_rollup(


check_rollup runs only after every deployment's aggregation_config has been built and deduped by identifying_key. So a plan whose query grouping isn't a subset of the deployment's (or is an unsupported roll-up) first builds a config at the wrong grouping. Any aggregation_config error, such as MissingParam or InvalidDdsAlpha, also hides the more basic grouping error. The check only needs the Raqe, the Deployment and the aggregation type, so it could run where served is built, before any aggregation is made.

Fixed in 3859ddb. The subset check (check_grouping) runs for every (Raqe, deployment) pair before any aggregation is built, so a MissingParam or InvalidDdsAlpha error can't hide a GroupingOutsideDeployment. The type check stays after, because the aggregation type comes from the variant match.

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

+                AggregationType::DDSketch,
+            ),
+            (
+                "hll",


The HLL case reuses quantile by (job) (0.99, …), so set_subpopulation_labels is called with Statistic::Quantile and AggregationType::HLL, a pairing that a real Cardinality workload never produces. Label placement depends on the statistic for some types (CountMinSketch, Topk), so this test doesn't show what happens for a real cardinality query. Consider giving the HLL row its own cardinality query string so the workload's requirements match the capability.

Fixed in 3859ddb. The HLL row is gone from the quantile table. New test hll_count_distinct_rollup_is_deployed_at_the_deployment_grouping plans a real SQL COUNT(DISTINCT requests) ... GROUP BY job (Cardinality only comes from SQL), checks the Raqe's capability and grouping, and serves it from an HLL deployment grouped by (instance, job).

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

+    // subpopulation is every label of the metric, which every item on it
+    // shares.
+    let (subpopulation_labels, topk_by_labels) = match deployment.capability {
+        Capability::TopKByValue | Capability::TopKByCount => {


For top-k, the subpopulation labels still come from items.first().requirements.grouping_labels, and the comment says every item shares them. Nothing checks that. If a top-k deployment served items whose full label sets differ (for example, different without clauses that resolve differently), aggregated_labels would silently follow whichever item sorts first. That is the same first-item dependency this PR removes for the other types. Consider taking the union, or asserting that all items match.

Addressed during the rebase onto #849. Labels are now derived per query, not from items.first(), and a top-k deployment whose queries need different heap keys gets one aggregation per key set.

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

+    }
+
+    /// The solution with its only deployment grouped by (instance, job).
+    fn widen_to_instance_job(solution: &mut MilpSolution) {


widen_to_instance_job only inserts instance, so it yields (instance, job) only if the deployment is already grouped by (job). The name and doc comment promise the end state. If a caller's deployment has some other grouping, the helper quietly produces a different set and the test assertions compare against the wrong grouping. Either assign LabelSet::from(["instance".into(), "job".into()]) directly or assert the starting grouping first.

Fixed in 3859ddb. widen_to_instance_job now assigns {instance, job} directly.

Add an optional suite-level planner_output_dir holding streaming_config.yaml and inference_config.yaml. When set, the runner validates both files at load time, skips generating the planner input, and adds a Compose override that parks the planner behind an inactive profile and mounts the directory where the query engine reads planner output. This lets the harness exercise engine configs the planner does not produce yet. Co-Authored-By: Claude Opus 5.5 <[email protected]>

The rollup suite reads KLL, DDSketch, Sum and MinMax aggregations grouped by (service, endpoint), over tumbling and sliding windows, through queries grouped by (service, endpoint), by (service) and with no grouping. The dataset includes an endpoint that starts mid-range. Co-Authored-By: Claude Opus 5.5 <[email protected]>

A MILP deployment grouped by G can serve a query grouped by Q ⊊ G. For single-subpopulation sketches (DatasketchesKLL, DDSketch, HLL) the engine merges across G-keys at query time, so the planner now writes the query_config pointing at the G-grouped aggregation instead of refusing. aggregation_config takes its labels from the deployment's grouping rather than the first served item, so a shared deployment is written at G whichever query comes first. Other families' roll-ups stay RollupUnsupported, and a query grouping outside the deployment's is a new GroupingOutsideDeployment error. Co-Authored-By: Claude Opus 5.5 <[email protected]>

milindsrivastava1997

Code review (automated, unverified). No confirmed correctness bugs; one drift risk, test gaps and cleanups below.

milindsrivastava1997 · 2026-10-13T13:40:46Z

+        });
+    }
+    match aggregation_type {
+        AggregationType::DatasketchesKLL | AggregationType::DDSketch | AggregationType::HLL => {


check_rollup hardcodes its own list of types that can roll up (DatasketchesKLL | DDSketch | HLL), separate from the engine's ROLLUP_AGGREGATION_TYPES in asap-query-engine/src/engines/simple_engine/mod.rs:196. The error message (line 51) names them a third time.

Failure scenario: If the engine list changes and the planner doesn't follow, the planner either refuses roll-ups the engine can serve or writes configs that fail at query time with "can't be merged across groups". A shared predicate such as AggregationType::can_roll_up() in promql_utilities, used by both sides and the error message, removes the drift.

Fixed in 3859ddb. Same change: one shared predicate, AggregationType::merges_across_groups(), used by the engine, the planner and the error text.

milindsrivastava1997 · 2026-10-13T13:40:46Z

+        let query = "quantile by (job) (0.99, http_requests_total)";
+        for (variant, capability, params, aggregation_type) in [
+            (
+                "kll-percall",


The HLL case of sketch_rollup_is_deployed_at_the_deployment_grouping uses a quantile query and only sets the deployment capability to Cardinality, so a real cardinality query (Statistic::Cardinality) never goes through set_subpopulation_labels or the Raqe build.

Failure scenario: A real count-distinct query rolled up from (instance, job) could get wrong labels or be wrongly refused, and this test would still pass.

Fixed in 3859ddb. See the reply above: HLL is now tested with a real SQL cardinality query.

milindsrivastava1997 · 2026-10-13T13:40:46Z

        assert!(err.contains("0 plans match"), "{err}");
    }

+    /// The exact sum is a keyed MultipleSum, which the engine doesn't roll up.


Only test-edited solutions and saved plans can reach a roll-up, yet saved_plan.rs has only the refusal test and no test where a sketch roll-up succeeds.

Failure scenario: Two Raqes on one plan deployment with different groupings go through the deployments[at] == deployment and can_serve checks, neither tested for a roll-up. A regression rejecting valid DD/KLL roll-up plans would go unnoticed.

Fixed in 3859ddb. Added writes_a_sketch_roll_up_at_the_deployment_grouping: a saved plan whose DD pick has deployment_grouping (job, label_0) serves quantile by (job) (0.5, data) with one DDSketch aggregation at (job, label_0).

milindsrivastava1997 · 2026-10-13T13:40:46Z

    for (raqe, &index) in served_by.iter().enumerate() {
        let item = workload.raqe_items[raqe];
        let key = &item_keys[&(index, item)];
+        check_rollup(


check_rollup now runs in the second loop, after aggregation_config is built for every Raqe. An invalid grouping is now reported only after building configs, and an earlier error can hide it.

Failure scenario: A deployment whose grouping isn't a superset of the query's and which also has a bad param (missing k/lg_k, invalid alpha) reports MissingParam/InvalidDdsAlpha instead of GroupingOutsideDeployment. Checking all (raqe, deployment) pairs up front, where the old loop was, fixes both.

Fixed in 3859ddb. Same fix: the grouping check runs up front, where the old loop was.

milindsrivastava1997 · 2026-10-13T13:40:46Z

    let requirements = &item.requirements;
+    let deployment_labels =
+        KeyByLabelNames::new(deployment.grouping_labels.iter().cloned().collect());
+    let (subpopulation_labels, topk_by_labels) = match deployment.capability {


For top-k, subpopulation_labels now takes topk_by_labels from deployment.grouping_labels instead of requirements.topk_by_labels. That's correct only because check_rollup later refuses top-k roll-ups; the function neither states nor checks it.

Failure scenario: If CountMinSketchWithHeap is ever allowed to roll up, or check_rollup is moved/skipped, a heap would be bucketed by the deployment's labels, ignoring the query's own topk by buckets — wrong per-bucket rankings with no error.

Fixed in 3859ddb. Top-k now takes its buckets from the query's own topk_by_labels, not the deployment's grouping. The two are equal whenever there's no roll-up, and top-k never rolls up, so there's no longer a hidden dependency on check_rollup.

milindsrivastava1997 · 2026-10-13T13:40:46Z

+            let fine_raqe = workload
+                .raqes
+                .iter()
+                .position(|r| r.id.contains(fine))


shared_deployment_takes_its_own_grouping_in_either_order puts both Raqes on the fine query's deployment without checking that its window/slide can serve the coarse query.

Failure scenario: If the coarse query needs a different window, the test still passes, though saved_plan's can_serve would refuse that pairing — the test checks a combination that can't occur in practice.

Leaving this as is. The test checks label logic: which grouping the aggregation gets when a fine and a coarse query share one deployment, regardless of order. Whether a window can serve a query is the solver's and can_serve's job, and those are tested separately; a mismatched window doesn't change the labels.

- AggregationType::merges_across_groups() is now the one list of types a roll-up can merge; the engine and the MILP output both use it, and the refusal message is built from it. - A query grouping outside its deployment's is refused before any aggregation is built, so a param error can't hide it. - A top-k heap takes its buckets from the query's own topk by labels. - Tests: HLL roll-up from a SQL COUNT(DISTINCT) query, a saved-plan DD roll-up that succeeds, and widen_to_instance_job sets the grouping. Co-Authored-By: Claude Opus 5.5 <[email protected]>

milindsrivastava1997 · 2026-10-13T14:24:29Z

+    deployment: &Deployment,
+    aggregation_type: AggregationType,
+) -> Result<(), MilpOutputError> {
+    if raqe.grouping_labels == deployment.grouping_labels || aggregation_type.merges_across_groups()


merges_across_groups() also lets Sum and MinMax through, but this PR is scoped to DatasketchesKLL / DDSketch / HLL roll-ups. If a variant is later mapped to Sum or MinMax, its roll-ups would pass planner validation with no test or review covering them. Consider an explicit allow-list of the three sketch types here instead of reusing the engine-side list.

Leaving this as the shared list on purpose. Sum and MinMax really do roll up: the engine merges them across groups (tested in #853's rollup_tests), so a planner that accepted them would be writing configs the engine serves correctly. No MILP variant maps to Sum or MinMax today, so that path isn't reachable from a real plan. A separate planner allow-list is the copy the last review asked us to remove.

milindsrivastava1997 · 2026-10-13T14:24:29Z

    item: &OptimizerItem,
    aggregation_type: AggregationType,
 ) -> (KeyByLabelNames, KeyByLabelNames) {
    let requirements = &item.requirements;


For non-top-k deployments the labels still come from statistics[0] of whichever item is served first. If one deployment ever serves items with different first statistics (e.g. Sum and Count on one exact-sum deployment), the aggregation key depends on item order, which is the order dependence this PR removes elsewhere.

There's no first-served dependence anymore. Since #849, labels are computed per (deployment, item) in subpopulation_labels, and the aggregation is deduped by its identifying key. statistics[0] only affects label placement for CountMinSketch (Sum/Count vs others) and top-k (does_precompute_operator_support_subpopulations, set_subpopulation_labels). The MILP never emits plain CountMinSketch, and Sum and Count items can't share a deployment because their capabilities differ.

- Check docker compose >= 2.24.4 (needed for !override) before using it, and document it. - Clear COMPOSE_PROFILES so an inherited value can't re-enable the planner. - Bind the fixed directory with long syntax, so a ':' in its path is safe. - Only skip planner-input generation when a Compose project is used. - LoadSuite rejects a relative planner_output_dir and validates an absolute one, so every loaded suite holds an absolute, checked path. - The checked-in fixture test also checks aggregation ids, metrics and grouping labels; an engine test parses the fixed configs with the engine's own loaders. Co-Authored-By: Claude Opus 5.5 <[email protected]>

…ketches

…tches

milindsrivastava1997 commented Oct 13, 2026

View reviewed changes

milindsrivastava1997 force-pushed the 839-3-compliance-rollup-suite branch from c460836 to 49426b5 Compare October 13, 2026 13:11

milindsrivastava1997 force-pushed the 839-4-milp-rollup-sketches branch from a9081dd to 1f88df1 Compare October 13, 2026 13:11

milindsrivastava1997 and others added 3 commits October 13, 2026 09:30

milindsrivastava1997 force-pushed the 839-3-compliance-rollup-suite branch from 49426b5 to 901cf84 Compare October 13, 2026 13:33

milindsrivastava1997 force-pushed the 839-4-milp-rollup-sketches branch from 1f88df1 to 76830c3 Compare October 13, 2026 13:33

milindsrivastava1997 mentioned this pull request Oct 13, 2026

feat(planner): --planner saved for SQL (multigroup e2e tables) #856

Closed

milindsrivastava1997 commented Oct 13, 2026

View reviewed changes

milindsrivastava1997 and others added 2 commits October 13, 2026 10:33

Merge branch '839-3-compliance-rollup-suite' into 839-4-milp-rollup-s…

8ac5766

…ketches

milindsrivastava1997 changed the base branch from 839-3-compliance-rollup-suite to main October 13, 2026 14:49

Merge remote-tracking branch 'origin/main' into 839-4-milp-rollup-ske…

8d7802b

…tches

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

Conversation

milindsrivastava1997 commented Oct 13, 2026

What

Tests

⚠️ Note: the pinned sketch-bench never proposes roll-ups

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!

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!

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