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

milindsrivastava1997 · 2026-10-12T21:51:47Z

Closes #834.

asap-planner --planner milp --query-language sql plans SQL workloads with the MILP, emitting SQL streaming/inference configs. elastic_sql is out of scope.

Commits

refactor: thread a language-tagged MilpConfig (Promql | Sql) through plan_milp, build_milp_workload, reject_unwritable_queries and plan_to_planner_output. No behavior change.
feat: SQL front end and output.
- planner/sql.rs: parse/match/top-k detection split out of the legacy processor (parse_sql_query, sql_statistics, sql_data_range_ms), which now calls them.
- extract_sql_aqes: metric key is table.value_column; labels come from the table's metadata_columns (must be listed, no discovery). PromQL and SQL share leaf merging.
- Top-k is accepted only when its GROUP BY lists every metadata column (ranks whole series, like PromQL topk(k, x)); k comes from LIMIT. Otherwise the planner errors.
- OptimizerItem.topk_k carries the largest literal k, replacing re-parsing PromQL in item_to_raqe.
- Output uses the SQL YAML builders with NoCleanup; windowing, sketch_parameters and aggregate_cleanup are rejected.
feat: CLI accepts --query-language sql with --planner milp; --prometheus-url and --clickhouse-* are rejected.

Testing

sql_twin_plans_like_promql: five PromQL/SQL twins (sum, count, quantile, value top-k, count top-k) give equal plan cost and identical aggregation layouts; SQL configs round-trip through the engine's config types with the right table and value column.
sql_topk_must_group_by_every_metadata_column, merged_topk_takes_the_largest_k_and_none_wins.
Manual CLI run with sketch-bench's real cost table and saturation curves: PromQL and SQL twin configs plan the same aggregations; the refusals above fire.

Not done

warn_default_slas runs only for PromQL.
bin/optimizer_cli.rs stays PromQL-only.

🤖 Generated with Claude Code

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

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

milindsrivastava1997

Review for --planner milp SQL support. The first three comments are correctness issues: they should fail early with a clear error. The rest are robustness, duplication, and efficiency cleanups.

milindsrivastava1997 · 2026-10-12T21:55:23Z

+/// models.
+fn sql_requirements(
+    query: &str,
+    tables: &[TableDefinition],


sql_requirements builds the MILP metric as table.<column> but never checks that the column is a declared value column. For COUNT(DISTINCT <metadata col>), e.g. SELECT L1, COUNT(DISTINCT L2) FROM cpu_usage ... GROUP BY L1 with metadata_columns [L1, L2], the metric becomes cpu_usage.L2, which has no hint and no facts. It then fails late in rqe_optimizer::validate_facts with a confusing missing-cardinality error, or prices a sketch with no facts behind it. The PromQL path rejects this up front with MetricsWithoutHints in extract_hinted_items, and the SQL path has no equivalent. The legacy planner handles this query.

Fixed in 553b497: sql_requirements now errors up front when the aggregated column isn't a value column of its table (e.g. COUNT reads "region", which is not a value column of t"). Tested with COUNT(DISTINCT region)insql_rejects_non_value_columns_and_avg`.

milindsrivastava1997 · 2026-10-12T21:55:23Z

+            MilpConfig::Sql(config) => {
+                let mut hints: Vec<MetricDefinition> = Vec::new();
+                for table in &config.tables {
+                    if table.metadata_columns.is_empty() {


This rejects the whole SQL config if any table has empty metadata_columns, including tables no query references. metadata_columns is #[serde(default)] and the legacy SQL planner accepts such tables. Example: tables [http (metadata [instance, job]), heartbeat (none)] with queries only on http fail with TableWithoutMetadataColumns("heartbeat"). Consider limiting the check to tables the queries actually use.

Leaving as is in this PR: the MILP path still requires metadata_columns on every table.

milindsrivastava1997 · 2026-10-12T21:55:23Z

-pub fn reject_unwritable_queries(config: &ControllerConfig) -> Result<(), MilpOutputError> {
+/// PromQL avg queries, and range-query overrides, which only size retention.
+pub fn reject_unwritable_queries(config: MilpConfig) -> Result<(), MilpOutputError> {
+    let MilpConfig::Promql(config) = config else {


This returns Ok for every SQL config, but SQL AVG maps to [Sum, Count] (get_sql_statistics). So SELECT job, AVG(v) FROM t ... GROUP BY job under --planner milp fails later in item_to_raqe with an internal-looking MultipleStatistics { statistics: [Sum, Count] } error instead of a clear "avg is not supported by --planner milp" error. The doc comment here only mentions PromQL avg.

Fixed in 553b497: a SQL query needing more than one statistic (AVG) now fails with AVG is not supported by --planner milp. Covered by sql_rejects_non_value_columns_and_avg.

milindsrivastava1997 · 2026-10-12T21:55:23Z

+    scrape_interval_ms: u64,
+) -> Result<Vec<OptimizerItem>, OptimizerError> {
+    // Only the queries' durations are used, which don't depend on it.
+    let eval_time = SystemTime::now()


eval_time comes from SystemTime::now(), so SQL parsing depends on the wall clock. The comment says only durations are used, but nothing enforces that: parse_sql_query/SQLPatternMatcher also classify absolute timestamps (e.g. time BETWEEN '2024-01-01 00:00:00' AND ...) against this time. That can make MILP output non-reproducible across runs or machines. A fixed eval time (e.g. a constant) would remove the dependency.

Fixed in 553b497: SQL queries are planned at a fixed SQL_EVAL_TIME, so output no longer depends on the clock. The CLI twin run produces byte-identical configs before and after.

milindsrivastava1997 · 2026-10-12T21:55:23Z

+                    ("sketch_parameters", config.sketch_parameters.is_some()),
+                    ("aggregate_cleanup", config.aggregate_cleanup.is_some()),
+                ])?;
+                let rqes: Vec<RQE> = config


This loop duplicates config_to_rqes, and both match arms call reject_fields with the same three fields. A new RQE field, or a new field the MILP must reject, could be added for one language and missed in the other. Consider having config_to_rqes take an iterator of (queries, repetition_delay_ms, controller_options), and calling reject_fields once before the match.

Fixed in 553b497: config_to_rqes takes (queries, repetition_delay_ms, controller_options) groups and both languages use it; reject_fields runs once before the match.

milindsrivastava1997 · 2026-10-12T21:55:23Z

        }
    }

+    // A SQL aggregation names its table and value column, and its metric is


This rebuilds the table.column -> (table, column) mapping by iterating tables × value_columns with sql_metric, which duplicates the loop (and duplicate check) in MilpConfig::hints. If the naming rules diverge, sql_columns can silently miss a metric, and the engine would get an aggregation with no table_name/value_column. A single helper on MilpConfig that returns the mapping would avoid this.

Fixed in 553b497: MilpConfig::sql_columns() is the one metric -> (table, column) mapping; hints() (with the duplicate check) and plan_to_planner_output both use it.

milindsrivastava1997 · 2026-10-12T21:55:23Z

+                    reason,
+                },
+            )?;
+            let k = topk_k(&leaf);


topk_k(&leaf) runs a full promql_parser::parse on every leaf of every RQE occurrence, including leaves that aren't top-k, and extract_requirements has already parsed each one. Before this PR, k was parsed only for top-k items in item_to_raqe. Consider getting k from the existing AST, or computing it only when the statistic is Topk.

Fixed in 553b497: topk_k now runs only for leaves whose statistic is Topk. Reusing the AST from extract_requirements would mean changing its signature, which didn't seem worth it.

milindsrivastava1997 · 2026-10-12T21:55:23Z

+    data_ingestion_interval_ms: u64,
+    query_evaluation_time: f64,
+) -> Result<(SQLQueryData, Option<SqlTopk>), ControllerError> {
+    let schema = build_sql_schema(tables);


parse_sql_query rebuilds the full schema (HashSets for every table) on every query. On the MILP SQL path, N queries over T tables means N rebuilds. Consider building the schema once in extract_sql_aqes and passing it in.

Not changing: the planner runs once offline over a handful of tables, so the rebuild is negligible, and passing the schema in would change the legacy SQL path's signature too.

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

milindsrivastava1997 and others added 3 commits October 12, 2026 14:35

refactor(planner): thread a language-tagged MilpConfig through the MI…

685ea4a

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

feat(planner): plan SQL workloads with the MILP

351d5f1

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

feat(planner): accept --query-language sql with --planner milp

1483156

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

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

asap-planner: --planner milp for SQL #834

Open

milindsrivastava1997 commented Oct 12, 2026

View reviewed changes

fix(planner): address review on --planner milp for SQL

553b497

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

zzylol mentioned this pull request Oct 13, 2026

feat(planner): asap-planner --planner saved writes a sketch-bench eval plan without re-solving #828

Open

Conversation

milindsrivastava1997 commented Oct 12, 2026

Commits

Testing

Not done

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant