Repository navigation
feat(planner): plan saved SQL queries over gauge and counter tables - #906
milindsrivastava1997 wants to merge 2 commits into
Conversation
--planner saved --query-language sql defined SQL tables only for records metrics, so a SQL query over a gauge (e.g. a pooled grouped quantile of `value`) could not be planned. Gauges and counters are now tables too, with the one value column `value`, as the ClickHouse workload tables have. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
milindsrivastava1997
left a comment
milindsrivastava1997
left a comment
There was a problem hiding this comment.
Automated code review (Claude Code). Main issue: a label-less, PromQL-only gauge/counter now fails the whole SQL plan.
| !m.values.is_empty(), | ||
| "records metric {name:?} has no values" | ||
| ); | ||
| for (name, m) in metrics { |
There was a problem hiding this comment.
Regression: every metric in e2e.metrics now becomes a SQL table, including gauges/counters no SQL query reads. A label-less one (e.g. up with labels: []) produces a table with metadata_columns: [], and MilpConfig::hints rejects it with TableWithoutMetadataColumns("up") — so --planner saved --query-language sql now fails on workloads it previously planned. Consider only emitting tables the SQL queries reference, or skipping label-less ones.
Also: counters get raw cumulative value columns, so SQL QUANTILE/SUM/AVG over a counter table sketches running totals rather than per-interval increments. Nothing rejects or flags SQL over a counter.
There was a problem hiding this comment.
Fixed in 158b2bd. Only gauges and counters that a selected SQL query reads (FROM <name>) become tables now; records metrics are tables as before. Test: a_gauge_only_promql_reads_is_no_sql_table adds a label-less up gauge to the mixed table, and it fails with the TableWithoutMetadataColumns error without the fix. The counter-semantics point isn't addressed here.
| ); | ||
| m.values.keys().cloned().collect() | ||
| } else { | ||
| vec![SQL_SERIES_VALUE_COLUMN.to_string()] |
There was a problem hiding this comment.
Any type other than records silently gets value — including histogram or typos like gauges, which asap-tools rejects (PROMETHEUS_METRIC_TYPES = ("gauge", "counter"); see test_paced_feeder.py:209). Likewise a non-records metric carrying a values map has it silently ignored. Per the code-design checklist this should fail loudly: match gauge | counter explicitly and error otherwise.
| value_columns, | ||
| metadata_columns: m.labels.iter().map(|l| l.name.clone()).collect(), | ||
| }); | ||
| } |
There was a problem hiding this comment.
Nothing checks that a gauge/counter label isn't named value (or ts). With a value label, metadata and value columns collide, and the engine's resolve_sample_value reads the label before the wire scalar — it uses the label string as the sample value (panicking if non-numeric).
|
|
||
| /// The metric type whose metrics are SQL tables. | ||
| /// The value column of a gauge's or counter's table. | ||
| const SQL_SERIES_VALUE_COLUMN: &str = "value"; |
There was a problem hiding this comment.
SQL_SERIES_VALUE_COLUMN = "value" duplicates VALUE_COLUMN in sketch_bench_workloads.py (and the values-or-value rule of its value_columns()), so the two must be kept in sync by hand. Ideally the column name comes from the table or one shared definition.
| } | ||
|
|
||
| /// A SQL query over a gauge reads its table's one value column, `value`. | ||
| #[test] |
There was a problem hiding this comment.
Only a gauge happy path is tested. Missing: a counter, a label-less gauge in a mixed table (which would catch the TableWithoutMetadataColumns regression — MIXED_TABLE's gauge happens to have labels), and an unknown metric type.
| let plan = json!({"plans": [{"method": "asap", "weights": "cpu", "kind": "cheapest", | ||
| "chosen": [pick("p90", 0, "dd", json!({"alpha": 0.01}), 300)]}]}); | ||
| let output = run_in(table, plan.to_string().as_bytes(), None, QueryLanguage::sql).unwrap(); | ||
|
|
There was a problem hiding this comment.
This asserts only (table_name, value_column); it never checks grouping labels (job), unlike the records test. A wrong metadata_columns mapping for non-records tables would go unnoticed.
Every gauge and counter became a SQL table, so a label-less one only PromQL reads (e.g. `up`) failed the whole SQL plan with TableWithoutMetadataColumns. Only the ones a selected SQL query reads (FROM <name>) are tables now. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
1 participant
Problem:
asap-planner --planner saved --query-language sqlbuilt SQL tables only forrecordsmetrics (e2e_sql_config). A SQL query over a gauge or counter, e.g. the pooled grouped quantileSELECT region, QUANTILE(0.9, value) FROM data ... GROUP BY regionthat the random-workload pilot (#879) needs, named a table the planner didn't know, so it couldn't be planned.Fix: every metric of the table's
e2e.metricsis a SQL table, its labels the metadata columns. Arecordsmetric keeps its value fields as value columns; a gauge or counter has the one value columnvalue, the column sketch-bench's ClickHouse workload tables give it (sketch_bench_workloads.py).On the engine side (read, not yet run), a remote-write sample named
datareaches a SQL aggregation whosetable_nameisdata(ingest_source.rs:232), so a gauge fed by the paced feeder should feed these aggregations unchanged.Tests:
optimizer::saved_plan::tests::writes_a_sql_query_over_a_gauge_table; the planner's 180 library tests pass.🤖 Generated with Claude Code