feat(planner): --planner saved reads RQE trees and writes rqe_configs by milindsrivastava1997 · Pull Request #888 · ProjectASAP/ASAPQuery

milindsrivastava1997 · 2026-10-14T22:51:53Z

Closes #882. Part of #886. Stacked on #887 (review that first; this PR's base is its branch).

--planner saved reads an e2e query's expr (RQE tree) and writes a new rqe_configs section to inference_config.yaml: per RQE id, the aggregations to read plus any query-time roll-up stage, so the engine (#883) can answer a leaf without parsing.

rqe_configs:
  <rqe id>:
    aggregations: [{aggregation_id: 7, num_aggregates_to_retain: 11}]
    query_time_aggregations: []   # e.g. [{operator: sum, grouping: {mode: by, labels: [job]}, parameter: null}] for an exact roll-up

with expr, a query's RQE ids come from the tree (rqes optional; must agree if given); each leaf is checked against its plan pick like a text leaf
a query with only expr gets its language from its leaves' metrics (records → SQL) and is planned on its own (no query_configs entry)
roll-ups follow the query_configs rules (sketch merge: no stage; exact: sum/min/max stage; top-k/increase refused), built from the leaf rather than PromQL
engine InferenceConfig loads rqe_configs (unused until feat(query-engine): RQE tree query route #883)
tables without expr convert as before
--expr-only: plan only the tree leaves of the e2e queries that have expr, writing no query_configs. Queries without expr (e.g. rate) and query text get no deployment. This is the plan the RQE frontend (feat(asap-tools): rqe query protocol and frontend switch for e2e runs #890) requires.
a query with promql/sql must list rqes; only a tree-only query may omit it
retention: StreamingConfig takes rqe_configs references into account as well as query_configs (max retention, summed read thresholds), so aggregations read only by trees are bounded

Tests: planner unit tests for each case and refusal; an integration test on the real e2e-rate10 all table + plans (copied from sketch-bench#225) checks every leaf gets an entry matching its query's query_configs aggregations. Manually, all 64 classic/all/multigroup × method × SLA conversions give identical streaming YAML and identical inference YAML apart from rqe_configs.

Known, unchanged by this PR: streaming_config.yaml's metrics: order isn't stable across runs.

🤖 Generated with Claude Code

Add `asap_types::rqe_expr`: a serde type for RQE expression trees (rqe leaves, agg, binary, scalar nodes), a `validate` check run at load, and `leaves` / `output_labels` helpers. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011gKTCAEz3sJ1Hzde16xxU1

An e2e query's optional `expr` (an RQE expression tree, validated at load) now names the plan RQEs it reads. Each tree leaf becomes one more optimizer item and Raqe, served by its plan pick like a text leaf, and gets an entry in a new `rqe_configs` section of the inference config: RQE id -> the aggregations it reads and, for an exact roll-up, the query-time stage that regroups to the RQE's grouping. `query_configs` are unchanged; a table without trees converts as before and writes no `rqe_configs`. A query may have only a tree: its language is that of the metrics its leaves read (records -> SQL). The engine's `InferenceConfig` loads `rqe_configs` (empty when absent); nothing reads it yet. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011gKTCAEz3sJ1Hzde16xxU1

An RQE leaf may name `topk_by` labels: like PromQL `topk by (...)`, a top-k leaf then ranks its input independently within each value combination of those labels, while `grouping` stays its output labels. Absent, the whole input is one bucket. Validation allows `topk_by` only on `topk_by_value` leaves, non-empty and within `grouping`. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011gKTCAEz3sJ1Hzde16xxU1

…tion' into 882-featplanner-planner-saved-reads-rqe-trees-and-writes-rqe_configs

A top-k leaf's optimizer item now carries its `topk_by` labels as `topk_by_labels`, as a PromQL `topk by (...)` query's does, so its Raqe is grouped by the buckets (the plan's top-k RQE grouping) and its heap is one per bucket, keyed by the remaining labels. Without `topk_by` the Raqe is a single bucket and can't share the deployment of a `topk by (...)` query. Tests run the pilot random workload (seed-862/workload-01-pilot) and its saved plans: `topk by (region, service)` leaves read the same heaps as their PromQL queries. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011gKTCAEz3sJ1Hzde16xxU1

milindsrivastava1997

Code review: 7 findings.

milindsrivastava1997 · 2026-10-15T01:23:13Z

    pub query_configs: Vec<QueryConfig>,
    pub cleanup_policy: CleanupPolicy,
+    /// RQE id -> how to answer that RQE leaf; empty unless planned from RQE trees.
+    pub rqe_configs: HashMap<String, RqeConfig>,


Aggregation retention is still computed only from query_configs (StreamingConfig::from_yaml_data walks inference_config.query_configs). The new rqe_configs references, with their num_aggregates_to_retain and read_count_threshold, never feed retention_map or read_count_threshold_map.

Failure scenario: A tree-only query (e.g. the plans_a_tree_only_query / writes_sql_rqe_configs_with_an_exact_roll_up latency_sum case) gets an aggregation no query_configs entry references, so retention_map.get(id) is None and the store grows unbounded. Also, a tree leaf needing a longer lookback than text queries sharing its aggregation is capped at the smaller retention and reads fewer windows than its rqe_configs entry asks for.

Fixed in 33ddb10 (the commit message doesn't mention it): StreamingConfig::from_yaml_data now walks the aggregation references of both query_configs and rqe_configs. Retention takes the max and read-count thresholds sum, as for shared text references. Tests: rqe_only_reference_bounds_retention, query_and_rqe_references_keep_the_largest_retention, query_and_rqe_references_sum_read_based_thresholds.

milindsrivastava1997 · 2026-10-15T01:23:13Z

+        };
+        let leaves: Vec<&str> = expr.leaves().iter().map(|l| l.id.as_str()).collect();
+        ensure!(
+            rqes.is_empty() || rqes == leaves,


rqe_ids requires rqes == leaves in exact order with the same duplicates, even for SQL queries, whose rqes is documented as unordered (the caller sorts it).

Failure scenario: A SQL query with rqes: ["cnt", "sum"] and expr sum / cnt is refused with "names RQEs [cnt, sum] but its expr reads [sum, cnt]". A tree reading one leaf twice (x - x) can't match a deduplicated rqes list.

Fixed in 33ddb10: rqes and the tree's leaf ids are compared as sets (test: reads_a_tree_whose_rqes_are_reordered_or_repeated).

milindsrivastava1997 · 2026-10-15T01:23:13Z

+                tree_leaves.insert(&leaf.id, (leaf, interval_ms))
+            {
+                ensure!(
+                    other == leaf && other_interval_ms == interval_ms,


A tree leaf whose RQE id appears in queries with different interval_secs is refused as "two different leaves". The text path allows this (rqes_of is keyed by (leaf, interval)), and the rqe_configs entry doesn't depend on the interval.

Failure scenario: The same tree run every 1 s and every 10 s converts fine as text, but adding expr fails with RQE "x" is two different leaves: ... every 1000 ms and ... every 10000 ms.

Fixed in 33ddb10: one RQE id can appear at several intervals, and its rqe_configs entry is written once. Different leaves under one id are still refused (test: plans_one_rqe_read_at_two_intervals).

milindsrivastava1997 · 2026-10-15T01:23:13Z

        match (&self.promql, &self.sql) {
-            (Some(_), Some(_)) | (None, None) => bail!(
+            (Some(_), Some(_)) => bail!(
                "e2e query reading {:?} needs exactly one of `promql` and `sql`",


Error/context messages identify a query by self.rqes, which is now optional (#[serde(default)]), so tree queries without rqes print [].

Failure scenario: A tree query with both promql and sql fails with "e2e query reading [] needs exactly one of promql and sql"; same for the "invalid expr" and "mixes records" errors, leaving no way to tell which query is at fault.

Fixed in 33ddb10: E2eQuery::name() identifies a query by its text, else its RQE or leaf ids.

milindsrivastava1997 · 2026-10-15T01:23:13Z

+//! `--planner saved` on real sketch-bench eval tables and their saved plans:
+//! the `all` workload of sketch-bench's `rqe-optimizer/results/e2e-rate10`
+//! (sketch-bench c1fd812), whose e2e queries carry RQE trees, and the pilot
+//! random workload `seed-862/workload-01-pilot` (sketch-bench PR #226), whose


The module doc comment cites "sketch-bench PR #226". AGENTS.md: "Code comments should not include git history, or issue numbers or historical facts." Describe what the fixture contains instead.

Fixed in 33ddb10: the doc comment now describes the fixture instead of citing PRs.

milindsrivastava1997 · 2026-10-15T01:23:13Z

+    let mut tree_leaves: BTreeMap<&str, (&RqeLeaf, u64)> = BTreeMap::new();
+    for (q, expr) in &trees {
+        // A tree-only query's `rqes` must agree with its tree too.
+        q.rqe_ids()?;


q.rqe_ids() is called again for every tree query only to validate it, and the result is discarded; for queries with both text and a tree it already ran at line 373. Validate once while building selected/trees.

Fixed in 33ddb10: rqe_ids() is computed once per query (TextQuery).

milindsrivastava1997 · 2026-10-15T01:23:13Z

+                    })?;
+                    kinds.push(metric.kind == RECORDS_METRIC_TYPE);
+                }
+                kinds.dedup();


is_sql builds a Vec of booleans, calls dedup() (consecutive only) and matches on length — a roundabout 'all leaves same kind' check. Take the first leaf's kind and check every other leaf matches.

Fixed in 33ddb10: is_sql takes the first leaf's kind and requires the others to match.

Require every subtree that feeds the result (root, agg operand, one side of each binary op) to read at least one RQE leaf; allow a repeated leaf id only for identical leaves; reject duplicate labels in grouping, by and topk_by; reject an empty id or metric and a zero lookback. Document that label lists are sets matched by name. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011gKTCAEz3sJ1Hzde16xxU1

…tion' into 882-featplanner-planner-saved-reads-rqe-trees-and-writes-rqe_configs

- Compare a query's `rqes` with its tree's leaves as sets. - Allow one RQE leaf at several repeat intervals: each interval is a Raqe, and the interval-independent rqe_configs entry is written once. Different leaves under one id are still refused. - Name a query in errors by its text, else its tree's RQE ids. - Compute and check a query's RQE ids once. - Check a tree's leaves read one kind of metric without dedup(). - Describe the e2e-tables fixture instead of citing its PR. - --expr-only (with --planner saved) plans only the e2e queries that have an RQE tree; the others (e.g. rate) get no deployment. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011gKTCAEz3sJ1Hzde16xxU1

milindsrivastava1997

Automated code review: 4 finding(s), unverified candidates.

milindsrivastava1997 · 2026-10-15T04:16:30Z

+        };
+        let leaves = leaf_ids(expr);
+        if rqes.is_empty() {
+            return Ok(leaves);


When a PromQL query omits rqes, rqe_ids returns the tree's leaf ids left-to-right and they are zipped positionally with decompose_to_leaves(text); nothing checks that tree leaf order matches text leaf order.

Failure scenario: promql quantile_over_time(0.5, x[5m]) / quantile_over_time(0.9, x[5m]) with expr {op:'/', lhs: p90-leaf, rhs: p50-leaf} and no rqes: text leaf 0 (p50) gets RQE p90 and vice versa. Downstream checks then fail confusingly or silently cross-attach the sketches.

(from automated /code-review; unverified)

Fixed in 2f18617: a query with promql/sql must list rqes; only a tree-only query may leave them out.

milindsrivastava1997 · 2026-10-15T04:16:30Z

    for q in &e2e.queries {
-        if let Some(text) = q.text(sql)? {
-            selected.push((q, text));
+        if expr_only && q.expr.is_none() {


With --expr-only, a query with both text and expr still has its text planned as a query_configs item alongside its tree leaves, though the rqe frontend never sends that text.

Failure scenario: If the text decomposes into an item with a different identifying_key than its tree leaf (e.g. topk-of-sum text with inner key labels vs a topk_by_value leaf), both aggregations are deployed; ASAP maintains a sketch nothing reads, skewing the cost --expr-only is meant to measure.

(from automated /code-review; unverified)

Fixed in 2f18617: --expr-only plans tree leaves only, with no query_configs. The real-table test asserts that the deployed aggregations are exactly those some rqe_configs entry reads (PromQL and SQL).

milindsrivastava1997 · 2026-10-15T04:16:30Z

+        }
+        if let Some(expr) = &q.expr {
+            expr.validate()
+                .with_context(|| format!("e2e query {}: invalid `expr`", q.name()))?;


Every query's expr is validated (and is_sql resolves its leaves' metrics) before the language filter, so a bad tree on a query of the other language aborts the run.

Failure scenario: --query-language promql on a mixed table whose SQL-only entry has a bad agg by or typo'd metric fails the whole PromQL plan, though that query would have been skipped.

(from automated /code-review; unverified)

Declining. A bad tree anywhere in a table is a generator bug, so failing the whole run loudly is intended, even for a query in the other language.

milindsrivastava1997 · 2026-10-15T04:16:30Z

+            leaf.metric
+        );
+    }
+    ensure!(leaf.lookback_ms > 0, "RQE {id:?} looks back 0 ms");


leaf_item re-checks lookback_ms > 0, already enforced by expr.validate() (ZeroLookback); its metric lookup also duplicates the one in E2eQuery::is_sql.

Failure scenario: Redundant validation that can drift from RqeLeaf::validate; the same metrics.get + 'reads unknown metric' error is written twice.

(from automated /code-review; unverified)

Fixed in 2f18617: the duplicated lookback check is removed (validate() owns it), and leaf_metric() is shared by is_sql and leaf_item.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011gKTCAEz3sJ1Hzde16xxU1

…tion' into 882-featplanner-planner-saved-reads-rqe-trees-and-writes-rqe_configs

- A query with `promql`/`sql` must name its `rqes`: text leaves map to plan RQEs by `rqes`, never by the tree's leaf order. Only a tree-only query may omit them. - `--expr-only` no longer plans query text: it writes only the tree leaves' deployments and `rqe_configs`, and no `query_configs`. - `leaf_item` drops the lookback check `RqeExpr::validate` owns, and shares one metric lookup (`leaf_metric`) with `is_sql`. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011gKTCAEz3sJ1Hzde16xxU1

…qe-expression-tree-type-and-validation

…on-tree-type-and-validation' into 882-featplanner-planner-saved-reads-rqe-trees-and-writes-rqe_configs # Conflicts: # asap-planner-rs/src/optimizer/milp_output.rs

…er-saved-reads-rqe-trees-and-writes-rqe_configs

milindsrivastava1997 and others added 2 commits October 14, 2026 18:29

This was referenced Oct 14, 2026

feat(query-engine): answer RQE expression trees on POST /api/v1/rqe_query #889

Merged

feat(asap-tools): rqe query protocol and frontend switch for e2e runs #890

Merged

Tracking: RQE frontend for the paper eval #886

Closed

milindsrivastava1997 and others added 3 commits October 14, 2026 20:16

Merge branch '880-featasap-common-rqe-expression-tree-type-and-valida…

c25fd46

…tion' into 882-featplanner-planner-saved-reads-rqe-trees-and-writes-rqe_configs

milindsrivastava1997 commented Oct 15, 2026

View reviewed changes

milindsrivastava1997 and others added 3 commits October 14, 2026 22:32

Merge branch '880-featasap-common-rqe-expression-tree-type-and-valida…

7ef4f7c

…tion' into 882-featplanner-planner-saved-reads-rqe-trees-and-writes-rqe_configs

milindsrivastava1997 commented Oct 15, 2026

View reviewed changes

milindsrivastava1997 and others added 6 commits October 15, 2026 00:23

style(asap-common): state behaviour in duplicate-label test comment

60a7844

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011gKTCAEz3sJ1Hzde16xxU1

Merge branch '880-featasap-common-rqe-expression-tree-type-and-valida…

995f16a

…tion' into 882-featplanner-planner-saved-reads-rqe-trees-and-writes-rqe_configs

Merge remote-tracking branch 'origin/main' into 880-featasap-common-r…

c2da7dc

…qe-expression-tree-type-and-validation

Merge remote-tracking branch 'origin/880-featasap-common-rqe-expressi…

292f30d

…on-tree-type-and-validation' into 882-featplanner-planner-saved-reads-rqe-trees-and-writes-rqe_configs # Conflicts: # asap-planner-rs/src/optimizer/milp_output.rs

Merge remote-tracking branch 'origin/main' into 882-featplanner-plann…

57ce5f5

…er-saved-reads-rqe-trees-and-writes-rqe_configs

milindsrivastava1997 changed the base branch from 880-featasap-common-rqe-expression-tree-type-and-validation to main October 15, 2026 14:04

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

feat(planner): --planner saved reads RQE trees and writes rqe_configs #882

Closed

milindsrivastava1997 merged commit 8ced299 into main Oct 15, 2026
7 of 8 checks passed

Conversation

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

Uh oh!

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!

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!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

milindsrivastava1997 commented Oct 14, 2026 •

edited

Loading