feat(planner): --planner saved for SQL by milindsrivastava1997 · Pull Request #866 · ProjectASAP/ASAPQuery

milindsrivastava1997 · 2026-10-14T14:05:23Z

Closes #856.

Summary

--planner saved --query-language sql writes a sketch-bench plan for the multigroup/all tables' SQL queries, as #828 does for PromQL. The e2e eval (#862) runs the all workload's multigroup queries this way.

Entries: an e2e query is written in promql or sql (exactly one; both or neither is an error). --query-language picks which are planned (promql or sql only, checked once in saved_plan_output), and a table with none in that language is an error. This also keeps PromQL --planner saved working on all tables, whose multigroup entries have no promql.
SQL config: each records metric becomes a table (labels → metadata_columns, values → value_columns, time column ts), with --planner milp's table checks and empty values refused. The sql strings, written by sketch-bench, are used as written, one query group per interval_secs.
Mapping: a SQL query is one leaf that may read several RQEs (compared as a set). All of their picks must be served by one deployment with one sketch, or the plan is refused.
Sub-second windows: a plan's window_secs / slide_secs may be fractional. Since sketch-bench#215 an instant query's window is one scrape (0.1 s at 10 samples/s), which the old whole-second parse refused for PromQL too. They are converted to whole ms; a fraction of a ms is refused.

Testing

cargo test -p asap_planner: 24 saved_plan tests, 12 new: SQL over a records table with an HLL roll-up and a DDSketch; the PromQL run of a mixed table; a SQL query's RQEs on two deployments, or needing different sketches on one (both refused); reordered RQEs (accepted); a pick from another table, no query in the language, a query in both languages, a records metric without labels or values, an unsupported language (all refused); a 0.1 s window (100 ms) and a 0.5 ms one (refused). Removing the sort, or checking only a query's first RQE, fails the matching test.
Real plans: sketch-bench main (Sketchlib CountMin Support #215, 198 migrate asap h2o benchmark pipeline to asapquery #216) exported at --samples-per-sec 10 and re-planned locally (autosketch_vs_asap --runs 1, extract_autosketch_plans.py), then every asap plan converted with --data-ingestion-interval-ms 100:

Table SQL PromQL

multigroup 8 / 8 none (no PromQL queries)

all 8 / 8 8 / 8

classic none (no SQL queries) 12 / 12

Before sketch-bench#216, SQL converted 3 of 16: templates 14, 15 and 17 put one query's RQEs on two deployments.

🤖 Generated with Claude Code

An e2e query is now written in `promql` or `sql`; --query-language picks which ones are planned. In SQL mode each `records` metric is a table (labels as metadata columns, values as value columns, time column `ts`), and a SQL query is one leaf that may read several RQEs, all of which must be served by one deployment. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

milindsrivastava1997

Code review: no definite crash/wrong-output bugs; robustness, test-gap and cleanup notes inline.

milindsrivastava1997 · 2026-10-14T16:52:23Z

 #[derive(Deserialize)]
 struct E2eMetric {
+    #[serde(rename = "type")]
+    kind: String,


E2eMetric.kind (serde type) is now required with no #[serde(default)], so PromQL --planner saved refuses any e2e table whose metrics lack type — previously only labels was needed. Old tables / exporters that omit type now fail with "missing field type". Consider defaulting kind and only treating an explicit records as a SQL table.

Won't fix. Every sketch-bench e2e table writes type (gauge, counter or records) on every metric. Requiring it fails loudly; defaulting would quietly plan a metric with a missing or misspelled type as PromQL.

milindsrivastava1997 · 2026-10-14T16:52:23Z

 }

+/// The time column of the `records` tables the e2e SQL queries read.
+const SQL_TIME_COLUMN: &str = "ts";


SQL time column is hard-coded to ts, while asap-tools takes it from the dataset's timestamp_col (experiment_utils/config.py:1011). A records dataset with a different timestamp column gets a streaming config naming a nonexistent column, and nothing flags it at plan time.

Won't fix. The time column is fixed by the queries themselves: sketch-bench's sql strings say ts (WHERE ts BETWEEN ...), so a dataset with another timestamp column wouldn't match them whatever the planner wrote.

milindsrivastava1997 · 2026-10-14T16:52:23Z

                ensure!(
-                    other == rqe,
-                    "leaf {:?} every {} ms reads both {other:?} and {rqe:?}",
+                    other == rqes,


Duplicate-leaf check compares RQE lists as ordered Vecs, but for a SQL leaf the list is a set. The same SQL query listed twice at one interval_secs with rqes ["users_1pct","users_5pct"] vs ["users_5pct","users_1pct"] fails with a spurious "reads both ...". Sort/dedupe before comparing.

Fixed in 7459ec4: a SQL leaf's RQEs are sorted before comparing. Test: reads_a_repeated_sql_query_whose_rqes_are_reordered.

milindsrivastava1997 · 2026-10-14T16:52:23Z

+                name: name.clone(),
+                time_column: SQL_TIME_COLUMN.to_string(),
+                value_columns: m.values.keys().cloned().collect(),
+                metadata_columns: m.labels.iter().map(|l| l.name.clone()).collect(),


A records metric becomes a SQL table without the non-empty metadata_columns check MilpConfig::hints does (TableWithoutMetadataColumns) — the saved path never calls hints(). labels: [] passes here but --planner milp would refuse it; values: {} gives empty value_columns that only fails later with a confusing "not a value column" error.

Fixed in 7459ec4: saved_plan_output now calls MilpConfig::hints(), so a records table without labels gets the same TableWithoutMetadataColumns refusal as --planner milp, and empty values is refused by name. Test: refuses_a_records_metric_without_labels_or_values.

milindsrivastava1997 · 2026-10-14T16:52:23Z

+            sql = e2e_sql_config(&selected, &e2e.metrics);
+            MilpConfig::Sql(&sql)
+        }
+        _ => {


_ => routes every non-sql language to the PromQL arm, relying on validation in main.rs and E2eQuery::text. saved_plan_output is pub, so reordering that validation would silently plan a third language as PromQL. Match QueryLanguage::promql explicitly and fail otherwise.

Fixed in 7459ec4: saved_plan_output matches promql and sql explicitly and refuses any other language. Test: refuses_a_language_other_than_promql_or_sql.

milindsrivastava1997 · 2026-10-14T16:52:23Z

-        "--planner saved supports only --query-language promql"
+        matches!(
+            args.query_language,
+            QueryLanguage::promql | QueryLanguage::sql


Language is validated twice: this matches!(promql | sql) guard duplicates the other => bail! in E2eQuery::text (which also reruns per query). Two error strings for one condition; adding a language means editing both.

Fixed in 7459ec4: the language is checked once, at the top of saved_plan_output. The main.rs guard is gone, and E2eQuery::text only picks the field.

milindsrivastava1997 · 2026-10-14T16:52:23Z

+    }
+
+    #[test]
+    fn refuses_a_sql_query_whose_rqes_are_on_two_deployments() {


Test gap: multi-RQE SQL path is only tested with RQEs on two different deployments. Nothing covers two RQEs on the same deployment with different sketch params / deployment_grouping (should refuse with "different sketches"). If the reads loop regressed to checking only the first RQE per leaf, no test would fail.

Added in 7459ec4: refuses_a_sql_query_whose_rqes_need_different_sketches (same deployment, different lg_k). Checking only a query's first RQE fails it.

milindsrivastava1997 · 2026-10-14T16:52:23Z

+    selected: &[(&E2eQuery, &str)],
+    metrics: &BTreeMap<String, E2eMetric>,
+) -> anyhow::Result<ControllerConfig> {
    let config = json!({


PromQL config is built via json! + deserialize round-trip while the SQL config is built as a struct directly, so validation differs (e.g. PromQL gets QueryGroup's positive repetition_delay_ms check; SQL relies on later check_repeat_interval). Building both the same way would keep validation consistent.

Won't fix. Both paths refuse a zero interval (PromQL in QueryGroup's deserializer, SQL in check_repeat_interval); only the error message differs.

- A plan's window_secs/slide_secs may be fractional (an instant query's window is one scrape, 0.1 s at 10 samples/s); they are converted to whole ms, and a fraction of a ms is refused. - A SQL query's RQEs are compared as a set. - records metrics go through --planner milp's table checks; empty values are refused. - The query language is checked once, in saved_plan_output. - Test: one SQL query's RQEs needing different sketches is refused. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

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

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

Closed

milindsrivastava1997 marked this pull request as ready for review October 14, 2026 16:46

milindsrivastava1997 commented Oct 14, 2026

View reviewed changes

milindsrivastava1997 merged commit 0715889 into main Oct 14, 2026
9 checks passed

This was referenced Oct 14, 2026

eval: e2e 10/s plans with one-scrape instant RQEs and one RQE per query ProjectASAP/sketch-bench#217

Merged

test(query-engine): SQL quantile roll-up over several windows #872

Merged

asap-tools: feed the sketchdb arm in the ClickHouse runner #874

Open

Table	SQL	PromQL
multigroup	8 / 8	none (no PromQL queries)
all	8 / 8	8 / 8
classic	none (no SQL queries)	12 / 12

Conversation

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

Uh oh!

Summary

Testing

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

milindsrivastava1997 commented Oct 14, 2026 •

edited

Loading