feat(planner): build rqe-optimizer Raqes and facts from the workload config by milindsrivastava1997 · Pull Request #798 · ProjectASAP/ASAPQuery

milindsrivastava1997 · 2026-10-06T20:54:56Z

Implements A2b (inputs → Raqe) of #790 (session 2b). Builds on #797 (latency_sla_ms rename, merged).

What

optimizer/milp.rs
- build_milp_workload(config, facts, scrape_interval_ms) -> MilpWorkload { items, raqes, raqe_items } turns optimizer items into rqe_optimizer::Raqes and runs validate_facts.
- solve_milp(workload, facts, costs, objective) -> (deployments, MilpSolution) runs build_all_candidates (deployable families only), checks for unservable Raqes, then milp::minimize.
optimizer/workload_facts.rs: loads the new facts file into WorkloadFacts.
pipeline.rs: hint checking moved into a shared extract_hinted_items that greedy and MILP both use.
atomic_costs.rs: AtomicCostEntry / AtomicCostTable re-export rqe_optimizer's types instead of a local copy, so measured_at (feat(planner): accept optional measured_at on atomic-cost entries #796) is now sketch-bench's typed MeasuredAt, and rows also carry merge_accuracy. sketch-bench's type does not deny unknown fields, so feat(planner): accept optional measured_at on atomic-cost entries #796's "other unknown fields are refused" assertion is dropped; the measured_at load tests stay. The drift-canary test is removed.
asap-optimizer-cli --milp: --workload-facts, --atomic-costs <flat rqe_atomic_costs.json>, --w-cpu, --w-mem. Prints deployments, Raqe → deployment mapping, latency, and per-phase cost. Writes no YAML (that's A3).
Debug logs (-v) at each step: facts per metric, item → Raqe (all fields), candidate count, eligible count per Raqe, solve result.
Cargo.lock: sketch-bench rqe-optimizer bumped to 373ea6b (includes Explore if we can have static checking of configs generated by Utilities experiments scripts for various modules #145/QueryEngineRust should not do an exact match of an incoming query against inference_config for SQL queries #148, plus Tracking issue for OSS release #155 measured_at and Revert sketchlib-rust path dependency once it is open sourced #154 accuracy checked at the query's merge count). MetricFacts.value_range (sizes DDSketch only, not deployable) is set to None.
tests/rqe_optimizer_smoke.rs removed: it used the pre-QueryEngineRust should not do an exact match of an incoming query against inference_config for SQL queries #148 API. The new solve_* tests cover HiGHS linking.

Design decisions (also on #790)

#	Decision
1	No `[patch]` path dep: sketch-bench main already has S1/S2/S6.
2	sketch-bench has no `minimize_cost`/EC2 pricing; the objective is `Objective::AUCCost { w_cpu, w_mem }` via `--w-cpu`/`--w-mem` (default: rqe-optimizer's `Objective::default()`).
3	One Raqe per query occurrence. `Raqe` has no frequency weight and query CPU is charged once per Raqe per interval. Two dashboards running the same query → two identical Raqes (`<query>#0`, `#1`) served by one shared deployment.
4	Statistic → Capability: Sum/Count→SumOrCount, Rate/Increase→RateOrIncrease, Min→Min, Max→Max, Quantile, Cardinality, Topk→TopK.
5	`accuracy_sla` is required accuracy (e.g. 0.99), in [0, 1]. Exact families, Cardinality: `relative_error ≤ 1 − sla`. Quantile: `max_rank_err ≤ 1 − sla`. TopK: `precision_at_k ≥ sla`. Worst-case metrics throughout.
6	`latency_sla_ms` (#797) passes straight through to `Raqe.latency_sla_ms`.
7	Only non-empty spatial filters are rejected (by `validate_facts`).
8	Facts file: `metrics: [{metric, groups: [{labels, cardinality}]}]`. The all-labels entry is the series count; arrival rate = series / scrape interval (sketch-bench). Labels come from the workload `metrics:` hints, scrape interval from the CLI. New loader; the old `label_set_facts` stays for greedy until A4.
9	TopK `grouping_labels` = `topk_by_labels` (`{}` for bare `topk`), not the all-labels output set, which would cost one heap per series. `k` and `topk_count_events` are not modeled.
10	The MILP path reads sketch-bench's flat cost export directly. The profile-selecting loader stays for greedy only.

Facts example:

metrics:
  - metric: http_requests_total
    groups:
      - labels: [instance, job, path]   # all labels → series count
        cardinality: 20000
      - labels: [job]
        cardinality: 20
      - labels: []
        cardinality: 1

Interface for later sessions

Raqe ids are {item index}:{query}#{k}, unique per Raqe. OptimizerItem.occurrences is the exact leaf count; query_frequency_hz is derived from it.
Cost table: load_flat_atomic_cost_table errors on non-finite or negative rows.
Known gap (sketch-bench#161): sum and count share the SumOrCount capability, so after the avg rewrite one exact-sum deployment can serve both. Don't deploy avg plans until Sketchlib rust migration #161 splits the capability.
A3 (3a):
- MilpWorkload.raqe_items[i] is the OptimizerItem index that raqes[i] came from. Several Raqes can share one item.
- solve_milp returns the full candidate list plus MilpSolution.mapping, which indexes into that list.
- --allow-undeployable-families is not wired; solve_milp passes false.
A4+A5 (4a): --milp already exists on asap-optimizer-cli. A4 removes greedy, the --label-set-facts / --atomic-cost-workload flags, and label_set_facts.rs. extract_hinted_items still uses LabelSetFactsError's hint variants, so move those when deleting.

Smoke run

Real cost export sketch-bench/out_10_6_26_1342/rqe_atomic_costs.json, scrape 15s. Workload: sum by (job) ×2 (two groups), rate[5m], quantile_over_time[5m], topk(5), max_over_time[1h], all at T=60s, accuracy_sla: 0.99.

=== Deployments: 5 ===
  [0] SumOrCount exact-sum config={"algorithm":"exact-sum","params":{}} metric=http_requests_total grouping={"job"} window=15000ms slide=15000ms
  [4] Max exact-max config={"algorithm":"exact-max","params":{}} metric=http_requests_total grouping={"instance", "job", "path"} window=240000ms slide=60000ms
  [13] RateOrIncrease exact-increase config={"algorithm":"exact-increase","params":{}} metric=http_requests_total grouping={"instance", "job", "path"} window=60000ms slide=60000ms
  [16] Quantile kll-percall config={"algorithm":"kll-percall","params":{"k":200}} metric=http_requests_total grouping={"instance", "job", "path"} window=300000ms slide=60000ms
  [17] TopK cms-heap-topk-fastpath-vector2d config={"algorithm":"cms-heap-topk-fastpath-vector2d","params":{"cols":2048,"rows":3}} metric=http_requests_total grouping={} window=15000ms slide=15000ms

=== Raqes: 6 ===
  max_over_time(http_requests_total[1h])#0 -> [4] latency=5.597e1ms
  quantile_over_time(0.99, http_requests_total[5m])#0 -> [16] latency=1.305e2ms
  rate(http_requests_total[5m])#0 -> [13] latency=2.496e1ms
  sum by (job) (http_requests_total)#0 -> [0] latency=7.911e-3ms
  sum by (job) (http_requests_total)#1 -> [0] latency=7.911e-3ms
  topk(5, http_requests_total)#0 -> [17] latency=3.600e-3ms

objective=6.216010e-3 cpu=6.216010e-3 cpu-sec/sec memory=932.502 MB
  ingest: cpu=2.692448e-3 cpu-sec/sec memory=652.539 MB
  merge: cpu=1.070174e-3 cpu-sec/sec memory=5.773 MB
  query: cpu=2.453389e-3 cpu-sec/sec memory=0.481 MB
  storage: cpu=0.000000e0 cpu-sec/sec memory=273.709 MB

Test plan

15 new unit tests:
- one Raqe per occurrence
- per-capability metric and direction
- TopK bucket vs global grouping
- accuracy_sla > 1 rejected
- spatial filter rejected
- missing cardinality rejected
- repeated queries share a deployment
- unmet accuracy → Unservable
- one deployable family per capability
- facts parsing, duplicates, missing hint, old format
cargo clippy -p asap_planner --all-targets -D warnings, cargo test -p asap_planner (pre-commit also ran workspace check/clippy/test).
Smoke run above.

🤖 Generated with Claude Code

milindsrivastava1997

Automated review findings (Claude Code). These have not been verified by hand.

milindsrivastava1997 · 2026-10-06T21:28:38Z

+
+fn capability(statistic: Statistic) -> Capability {
+    match statistic {
+        Statistic::Sum | Statistic::Count => Capability::SumOrCount,


Sum and Count both map to Capability::SumOrCount. After avg by (job)(x) is rewritten into sum and count items with the same grouping, both land in one candidate group, so the MILP can serve both Raqes from a single exact-sum deployment. One accumulator can't hold both the sum of values and the count, so the plan under-counts ingest and memory and A3 can't deploy it.

Agreed, this is real. The fix belongs upstream: split SumOrCount into Sum and Count capabilities (as Min/Max already are), so a sum and a count on the same grouping never share a deployment. Tracked in ProjectASAP/sketch-bench#161; ASAPQuery bumps the rev once it lands. A3 must not deploy an avg plan before then.

milindsrivastava1997 · 2026-10-06T21:28:38Z

+        _ => req.grouping_labels.labels.iter().cloned().collect(),
+    };
+
+    Ok(Raqe {


The Raqe id is only query_strings.join(" | ") plus a per-item #k. Items that differ only in t_repeat_ms, accuracy_sla or latency_sla_ms get the same ids (for example, sum by (job) (x)#0 with SLA 0.99 and with SLA 0.9). The Unservable error, the CLI's Raqe→deployment output, and anything downstream keyed by id can't tell them apart.

Fixed in 5561781. Ids are now {item index}:{query}#{k}, so items differing only in T or SLA get distinct ids (same_query_at_different_cadences_gets_distinct_ids).

milindsrivastava1997 · 2026-10-06T21:28:38Z

+    let costs_path = args
+        .atomic_costs
+        .as_deref()
+        .expect("clap requires --atomic-costs with --milp");


The MILP path deserializes the cost table without row validation. The greedy loader drops non-finite or negative rows via valid_cost_entry, and rqe-optimizer doesn't check them either. A NaN or negative cost then reaches minimize, where HiGHS either fails with an opaque error or picks the negative-cost deployment. Suggest reusing valid_cost_entry.

Fixed in 5561781. New load_flat_atomic_cost_table reuses valid_cost_entry and errors on any non-finite or negative row instead of dropping it (flat_loader_rejects_non_finite_or_negative_costs).

milindsrivastava1997 · 2026-10-06T21:28:38Z

+        .expect("clap requires --atomic-costs with --milp");
+    let costs: AtomicCostTable = serde_json::from_str(&std::fs::read_to_string(costs_path)?)
+        .map_err(|e| anyhow::anyhow!("parsing cost table {}: {e}", costs_path.display()))?;
+    let Objective::AUCCost { w_cpu, w_mem } = Objective::default();


--w-cpu and --w-mem accept any f64. A negative value (for example --w-mem -1) rewards memory or makes the problem unbounded, and NaN makes every objective coefficient NaN. Suggest rejecting values that aren't finite and >= 0.

Fixed in 5561781. --w-cpu/--w-mem go through parse_weight: finite and >= 0 only (weights_must_be_finite_and_non_negative).

milindsrivastava1997 · 2026-10-06T21:28:38Z

+    let req = &item.requirements;
+    let query = item.query_strings.join(" | ");
+    let [statistic] = req.statistics.as_slice() else {
+        panic!("optimizer item {query:?} must have exactly one statistic after the avg rewrite");


This panic!s when an item has more than one statistic. get_statistics_to_compute returns [Sum, Count] for any avg, but rewrite_avg only rewrites a top-level avg or avg_over_time, so an avg the rewrite misses crashes the CLI. Suggest returning a MilpError variant instead.

Fixed in 5561781. Returns MilpError::MultipleStatistics instead of panicking (item_with_two_statistics_is_an_error_not_a_panic).

milindsrivastava1997 · 2026-10-06T21:28:38Z

-}
-
-pub type AtomicCostTable = Vec<AtomicCostEntry>;
+pub use rqe_optimizer::{AtomicCostEntry, AtomicCostTable};


Re-exporting rqe_optimizer's AtomicCostEntry drops the local #[serde(deny_unknown_fields)]. Unknown or misspelled cost fields (for example a new subtract_cpu_secs) used to fail to parse and are now silently accepted, and the canary test that guarded this was removed.

Agreed that the strictness is lost, but it belongs to sketch-bench's type now that ASAPQuery uses it directly. Tracked in ProjectASAP/sketch-bench#161 (add deny_unknown_fields to AtomicCostEntry). On the typed measured_at: sketch-bench's reduce_one always writes the full block, so the typed form matches every table it emits. Tables are only produced by sketch-bench.

milindsrivastava1997 · 2026-10-06T21:28:38Z

+
+/// `query_frequency_hz` is `count * 1000 / t_repeat_ms`.
+fn occurrence_count(item: &OptimizerItem) -> usize {
+    (item.query_frequency_hz * item.t_repeat_ms as f64 / 1000.0).round() as usize


Recovering the occurrence count by rounding query_frequency_hz counts leaves, not query evaluations. sum(x) / sum(x) adds 1000/t twice to one item and becomes 2 Raqes, so query CPU is charged twice for one evaluation. The same happens with avg by (job)(rate(x[5m])). Storing an integer count on OptimizerItem would be exact.

Fixed the float round-trip in 5561781: OptimizerItem.occurrences is an exact integer count and query_frequency_hz is derived from it. Keeping per-leaf counting, though: the engine evaluates each arm of a binary query independently, so sum(x) / sum(x) really runs the aggregate twice, and two Raqes charge that correctly.

milindsrivastava1997 · 2026-10-06T21:28:38Z

+        cost_rows = costs.len(),
+        "milp: built candidates"
+    );
+    for (raqe, eligible) in workload


Nit: this debug-only loop runs eligible_deployments_for for every Raqe on every solve, even when debug logging is off, and unservable then repeats the same O(R×D) scan. Suggest guarding it with tracing::enabled!(Level::DEBUG), or computing eligibility once.

Fixed in 5561781. Eligibility is computed once per Raqe and feeds both the debug log and the unservable check (no second scan via unservable).

milindsrivastava1997 · 2026-10-06T21:28:38Z

-    /// document. This makes the empirical workload profile explicit and avoids
-    /// mixing costs from different traces or time windows.
-    #[arg(long = "atomic-cost-workload", requires = "atomic_costs")]
+    /// Greedy only. JSON `profiles[].workload` value copied from the


Nit: --label-set-facts has no conflicts_with = "milp", while --atomic-cost-workload does, so it is silently ignored under --milp.

Fixed in 5561781. --label-set-facts now has conflicts_with = "milp".

milindsrivastava1997 · 2026-10-06T21:28:38Z

    Ok(())
 }
+
+fn run_milp(args: &Args, config: &ControllerConfig) -> anyhow::Result<()> {


Nit: workload facts load against config.metrics.unwrap_or_default() before the MissingMetricHints check runs. A config with no metrics: section fails with "metric X has facts but no metrics: hint" instead of the intended MissingMetricHints error.

Fixed in 5561781. A missing metrics: section now returns MissingMetricHints before facts are loaded, and the MILP path calls config.warn_default_slas().

…config Converts the workload's optimizer items into sketch-bench `Raqe`s (one per query occurrence), loads per-metric workload facts, and solves with `rqe_optimizer::milp::minimize`. `asap-optimizer-cli --milp` prints the chosen deployments and plan cost; it writes no configs yet. `AtomicCostEntry` is now rqe-optimizer's type instead of a local copy. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

milindsrivastava1997

Automated code review (9 findings).

milindsrivastava1997 · 2026-10-06T22:23:08Z

+
+fn capability(statistic: Statistic) -> Capability {
+    match statistic {
+        Statistic::Sum | Statistic::Count => Capability::SumOrCount,


Sum and Count share one SumOrCount deployment. Both map to Capability::SumOrCount, and a Deployment is keyed only by (capability, metric, filter, grouping, config, window), so the MILP can serve sum by (job)(x) and count by (job)(x) from one exact-sum accumulator, which can hold only one of sum-of-values or sum-of-1s.

Failure: the avg rewrite (#795) produces a sum item and a count item on the same metric and grouping. Both map to one shared deployment, so ingest and memory are undercounted by about 2x and A3 cannot deploy the plan.

Agreed, this is real. The fix belongs upstream: split SumOrCount into Sum and Count capabilities (as Min/Max already are), so a sum and a count on the same grouping never share a deployment. Tracked in ProjectASAP/sketch-bench#161; ASAPQuery bumps the rev once it lands. A3 must not deploy an avg plan before then.

milindsrivastava1997 · 2026-10-06T22:23:08Z

+    };
+
+    Ok(Raqe {
+        id: query,


Raqe ids are not unique. The id is query_strings.join(" | ") plus #k only. Items with the same query string but a different t_repeat_ms, accuracy_sla or latency_sla_ms get identical ids.

Failure: two groups running sum by (job)(x) at 30s and 60s both produce sum by (job)(x)#0. The CLI output, MilpError::Unservable, and the BTreeSet-deduped validate_facts messages then become ambiguous or merge, and A3 keying off raqe ids would see duplicates.

Fixed in 5561781. Ids are now {item index}:{query}#{k}, so items differing only in T or SLA get distinct ids (same_query_at_different_cadences_gets_distinct_ids).

milindsrivastava1997 · 2026-10-06T22:23:08Z

 }
+
+fn run_milp(args: &Args, config: &ControllerConfig) -> anyhow::Result<()> {
+    let hints = config.metrics.as_deref().unwrap_or_default();


Missing metrics: hints are silently defaulted. config.metrics.as_deref().unwrap_or_default() turns missing hints into an empty slice, which hides the intended MissingMetricHints error (code-design-review §1).

Failure: a config with no metrics: block later fails with metric "..." has facts but no metrics: hint. That points the user at the facts file instead of the config.

Separately, the MILP path never calls config.warn_default_slas(). A group without controller_options gets accuracy_sla = 0.0 (max error 1.0, or precision_at_k >= 0 for TopK), so the coarsest sketch qualifies with no warning.

Fixed in 5561781. A missing metrics: section now returns MissingMetricHints before facts are loaded, and the MILP path calls config.warn_default_slas().

milindsrivastava1997 · 2026-10-06T22:23:08Z

-}
-
-pub type AtomicCostTable = Vec<AtomicCostEntry>;
+pub use rqe_optimizer::{AtomicCostEntry, AtomicCostTable};


Re-exporting sketch-bench's AtomicCostEntry drops #[serde(deny_unknown_fields)] and makes measured_at a typed MeasuredAt.

Failure: a misspelled field such as merge_acuracy now loads silently with the data dropped, so merged accuracy is never checked. Conversely, a profile whose measured_at lacks items_per_instance (accepted as opaque in #796) now fails to load.

Agreed that the strictness is lost, but it belongs to sketch-bench's type now that ASAPQuery uses it directly. Tracked in ProjectASAP/sketch-bench#161 (add deny_unknown_fields to AtomicCostEntry). On the typed measured_at: sketch-bench's reduce_one always writes the full block, so the typed form matches every table it emits. Tables are only produced by sketch-bench.

milindsrivastava1997 · 2026-10-06T22:23:09Z

+}
+
+/// `query_frequency_hz` is `count * 1000 / t_repeat_ms`.
+fn occurrence_count(item: &OptimizerItem) -> usize {


Occurrence count is recovered through a float round-trip (query_frequency_hz * t_repeat_ms / 1000), so the integer count from extract_aqes is lost.

Failure: sum(x) + sum(x) in one dashboard adds 1000/t twice, becomes two Raqes, and charges query CPU twice. Carrying count: usize on OptimizerItem would be exact and simpler.

Fixed the float round-trip in 5561781: OptimizerItem.occurrences is an exact integer count and query_frequency_hz is derived from it. Keeping per-leaf counting, though: the engine evaluates each arm of a binary query independently, so sum(x) / sum(x) really runs the aggregate twice, and two Raqes charge that correctly.

milindsrivastava1997 · 2026-10-06T22:23:09Z

+    for (raqe, eligible) in workload
+        .raqes
+        .iter()
+        .map(|r| (r, eligible_deployments_for(r, &deployments)))


An O(R·D) eligibility scan runs only to feed a debug log. eligible_deployments_for runs for every Raqe x candidate pair even when debug logging is off, and unservable then repeats the same scan. Gate it on tracing::enabled!(Level::DEBUG), or derive unservable from the same per-Raqe lists.

Fixed in 5561781. Eligibility is computed once per Raqe and feeds both the debug log and the unservable check (no second scan via unservable).

milindsrivastava1997 · 2026-10-06T22:23:09Z

+            .then(a.accuracy_sla.total_cmp(&b.accuracy_sla))
+            .then(
+                a.latency_sla_ms
+                    .partial_cmp(&b.latency_sla_ms)


The sort comparator is inconsistent. It uses partial_cmp(...).unwrap_or(Ordering::Equal) for latency_sla_ms but total_cmp for accuracy_sla. A NaN latency (rejected by validate_facts only after sorting) makes the order non-total, so Raqe order and ids can vary between runs. Using total_cmp through the Option would make it total.

Fixed in 5561781. Both SLAs now use total_cmp; no latency limit sorts last.

milindsrivastava1997 · 2026-10-06T22:23:09Z

-    #[arg(long = "atomic-costs")]
+    /// Greedy only. YAML label-set facts: `series_count` per (metric, spatial
+    /// filter) and `cardinality` per (metric, spatial filter, grouping labels).
+    #[arg(long = "label-set-facts", required_unless_present = "milp")]


--label-set-facts is silently ignored with --milp. Clap accepts the flag but the file is never read in MILP mode. Add conflicts_with = "milp", as --atomic-cost-workload already has.

Fixed in 5561781. --label-set-facts now has conflicts_with = "milp".

- Raqe ids carry the item index, so items differing only in T or SLA get distinct ids. - `OptimizerItem.occurrences` counts merged leaves exactly; `query_frequency_hz` is derived from it. - An item with more than one statistic is a `MilpError`, not a panic. - Eligibility is computed once per Raqe for the debug log and the unservable check; the item sort is total. - The MILP cost table loader rejects non-finite or negative rows; `--w-cpu`/`--w-mem` must be finite and >= 0. - `--label-set-facts` conflicts with `--milp`; missing `metrics:` hints report `MissingMetricHints`; the MILP path warns on default SLAs. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

milindsrivastava1997

Automated code review: 2 correctness issues (deployment sharing), 4 behaviour changes, 2 nits.

🤖 Generated with Claude Code

milindsrivastava1997 · 2026-10-07T01:59:37Z

+
+fn capability(statistic: Statistic) -> Capability {
+    match statistic {
+        Statistic::Sum | Statistic::Count => Capability::SumOrCount,


Sum and Count share a deployment. Statistic::Sum and Statistic::Count both map to Capability::SumOrCount, so the MILP can serve a sum Raqe and a count Raqe from one exact-sum deployment. ASAPQuery deploys them as different configs (MultipleSum with sub_type sum vs count, per map_statistic_to_precompute_operator). The avg→sum+count rewrite from #795 always hits this, and so does sum by (job)(x) next to count by (job)(x). The result is a plan ASAPQuery can't deploy as written, and its cost counts one accumulator where two are needed.

Same as the earlier Sum/Count thread: tracked in ProjectASAP/sketch-bench#161 (split SumOrCount into Sum and Count). ASAPQuery bumps the rev once it lands; A3 must not deploy avg plans before then.

milindsrivastava1997 · 2026-10-07T01:59:37Z

+    item.latency_sla_ms.unwrap_or(f64::INFINITY)
+}
+
+fn item_to_raqe(item: &OptimizerItem) -> Result<Raqe, MilpError> {


topk_count_events is dropped. topk(5, sum_over_time(x[5m])) and topk(5, count_over_time(x[5m])) at the same cadence become identical Raqes (capability, grouping, lookback all match), so they can share one CountMinSketchWithHeap deployment. ASAPQuery needs two configs with different count_events, so one of the two queries gets wrong rankings, and the cost counts one sketch too few.

Agreed. Raqe has no way to express it, so the fix is upstream: added as §3 of ProjectASAP/sketch-bench#161 (split TopK by value vs count weighting, or add a stream-identity field). That is in progress, so no interim guard here.

milindsrivastava1997 · 2026-10-07T01:59:37Z

+    capability: Capability,
+    accuracy_sla: f64,
+) -> (&'static str, f64, AccuracyDirection) {
+    let max_error = 1.0 - accuracy_sla;


Floating-point error at the SLA boundary. 1.0 - 0.9 = 0.09999999999999998, so a cost row whose relative_error or max_rank_err is exactly 0.1 fails the <= check and is wrongly ineligible. This can make a Raqe Unservable or force a costlier deployment. Adding a small epsilon to the comparison would fix it.

Fixed in a7515f6. Tolerances get a SLA_EPSILON = 1e-9 slack in the lenient direction (accuracy_exactly_at_the_sla_boundary_passes covers 0.1 at SLA 0.9 for rank error and 0.9 for top-k precision).

milindsrivastava1997 · 2026-10-07T01:59:37Z

+
+/// Objective weights must be finite and non-negative: a negative weight
+/// rewards cost, and NaN poisons every coefficient.
+fn parse_weight(s: &str) -> Result<f64, String> {


An all-zero objective is accepted. 0 passes this check for both weights, and the default w_mem is already 0, so --w-cpu 0 makes every cost coefficient 0. HiGHS then returns an arbitrary feasible plan, which the CLI prints as optimal. Reject the case where w_cpu == 0 && w_mem == 0.

Fixed in a7515f6. The CLI rejects --w-cpu and --w-mem both being 0.

milindsrivastava1997 · 2026-10-07T01:59:37Z

-}
-
-pub type AtomicCostTable = Vec<AtomicCostEntry>;
+pub use rqe_optimizer::{AtomicCostEntry, AtomicCostTable};


This also changes the greedy loader's parsing. Re-exporting rqe_optimizer's AtomicCostEntry makes measured_at typed (MeasuredAt with a required items_per_instance) where it was opaque JSON, and it drops deny_unknown_fields. A greedy cost document that #796 accepted can now fail to parse, and misspelled optional fields (merge_acuracy, measured-at) are now silently ignored instead of rejected. Was that intended?

Intended. ASAPQuery now uses sketch-bench's type directly. sketch-bench's reduce_one always writes the full measured_at block, so the typed form matches every table it emits. Unknown-field strictness is tracked in ProjectASAP/sketch-bench#161 §2.

milindsrivastava1997 · 2026-10-07T01:59:37Z

                Ok(req) => {
                    let key = OptimizerItemKey::from_rqe(&req, rqe);
-                    let entry = acc.entry(key).or_insert_with(|| (req, Vec::new(), 0.0));
+                    let entry = acc.entry(key).or_insert_with(|| (req, Vec::new(), 0));


Repeated leaves within one query are counted twice. occurrences goes up once per matching leaf, so sum by (job)(x) / sum by (job)(x) gives occurrences = 2. build_milp_workload then emits two Raqes and charges query and merge CPU twice per interval, even though the query runs once. Should identical leaves within a single RQE be deduplicated?

Keeping two Raqes: the engine evaluates each arm separately. handle_binary_expr_promql calls evaluate_binary_arm for lhs and rhs (asap-query-engine/src/engines/simple_engine/promql.rs:562,565), each running the full fetch and merge pipeline with no memo. Range queries build and run one context per arm (promql.rs:803-828), and there is no result cache. So sum(x) / sum(x) really pays query and merge CPU twice.

milindsrivastava1997 · 2026-10-07T01:59:37Z

    pub query_frequency_hz: f64,

+    /// Leaf occurrences merged into this item, across all RQEs.
+    pub occurrences: usize,


nit: query_frequency_hz is always occurrences * 1000 / t_repeat_ms, so storing both lets them drift. The tests already set the two independently and inconsistently. A query_frequency_hz() method computed from these fields would remove the redundant state.

Leaving this one: query_frequency_hz is read only by the greedy path (greedy.rs:76, cost_model.rs:193, candidate_gen_dump.rs:81). The MILP uses occurrences. Greedy is commented out in A4 (#790), so the redundant field goes with it.

milindsrivastava1997 · 2026-10-07T01:59:38Z

+
+fn run_milp(args: &Args, config: &ControllerConfig) -> anyhow::Result<()> {
+    config.warn_default_slas();
+    let Some(hints) = config.metrics.as_deref() else {


nit: this repeats the MissingMetricHints check that extract_hinted_items already does inside build_milp_workload. Having one guard (e.g. let load_workload_facts take the config) would keep the two from diverging.

Leaving as is. The CLI guard has to run before load_workload_facts, which needs the hints, and that happens before build_milp_workload runs. Moving the check into a facts loader that takes the config would just move the duplicate.

… objective - Accuracy tolerances get a 1e-9 slack so `1 - sla` rounding doesn't reject a cost row measured exactly at the boundary. - `asap-optimizer-cli --milp` rejects `--w-cpu`/`--w-mem` both 0, which would make the solver's pick arbitrary. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

milindsrivastava1997

Code review: 8 findings (3 correctness), verified against pinned sketch-bench rqe-optimizer @ 373ea6b. Tests were not run.

milindsrivastava1997 · 2026-10-07T02:35:53Z

+
+    // TopK keeps one heap per `topk by` bucket; its `grouping_labels` is the
+    // output label set (every label), which would cost one heap per series.
+    let grouping_labels: LabelSet = match capability {


Correctness: item_to_raqe drops topk_count_events, so count-weighted and value-weighted TopK items become identical Raqes (same capability/metric/grouping). topk(5, count_over_time(x[5m])) and topk(5, sum_over_time(x[5m])) at the same cadence can both be assigned to one cms-heap-topk-fastpath-vector2d deployment, but the engine needs two distinct CountMinSketchWithHeap configs. One query is then answered with the wrong weighting, and cost is under-counted by one deployment. (The PR notes this isn't modeled, but the resulting plan can't be deployed. Consider at least making the weightings ineligible to share.)

milindsrivastava1997 · 2026-10-07T02:35:53Z

-}
-
-pub type AtomicCostTable = Vec<AtomicCostEntry>;
+pub use rqe_optimizer::{AtomicCostEntry, AtomicCostTable};


Correctness: Re-exporting sketch-bench's AtomicCostEntry loses deny_unknown_fields, and the rejection test was removed. A typo such as merge_acuracy now parses silently with merge_accuracy empty, so accuracy_at falls back to single-instance query_accuracy. KLL/HLL rows can then pass accuracy_ok_for at L/W merges while their merged error exceeds the SLA.

milindsrivastava1997 · 2026-10-07T02:35:53Z

-                    // query_frequency_hz must stay in Hz (queries per real second)
-                    // regardless of t_repeat_ms's internal unit — 1000.0 / ms, not 1.0 / ms.
-                    entry.2 += 1000.0 / rqe.t_repeat_ms as f64;
+                    entry.2 += 1;


Correctness: occurrences is incremented per leaf even when the same leaf repeats inside one query (the contains check dedups only query_strings). rate(x[5m]) / rate(x[5m]) * 100 at T=60s gives occurrences=2, so two Raqes (#0, #1) are emitted and query cost is charged twice per interval for one panel.

milindsrivastava1997 · 2026-10-07T02:35:53Z

+    let file: FactsFile = serde_yaml::from_str(yaml)?;
+    let mut facts = WorkloadFacts::new();
+    for entry in file.metrics {
+        let Some(hint) = hints.iter().find(|h| h.metric == entry.metric) else {


With duplicate metrics: hints for one metric, this find takes the first hint's labels while schema_from_hints keeps the last (add_metric overwrites). Extraction and facts then disagree on the label set, which shows up as a confusing 'not a subset' error from validate_facts or a wrong series count. Suggest rejecting duplicate hints up front.

milindsrivastava1997 · 2026-10-07T02:35:53Z

+        "milp: built candidates"
+    );
+    let mut missing = Vec::new();
+    for raqe in &workload.raqes {


This reimplements rqe_optimizer::enumerate::unservable(raqes, deployments), which minimize's doc points callers to. If upstream changes the definition, this copy drifts and a Raqe can reach minimize, which assert!s and panics instead of returning MilpError::Unservable.

milindsrivastava1997 · 2026-10-07T02:35:53Z

+    costs: &[AtomicCostEntry],
+    objective: Objective,
+) -> Result<(Vec<Deployment>, MilpSolution), MilpError> {
+    let deployments = build_all_candidates(&workload.raqes, costs, facts, false);


The full R×D eligibility matrix is computed 3× (pruning in build_all_candidates, the unservable check below, and inside minimize), and R scales with one Raqe per occurrence. With many duplicate panels, planning time grows linearly for no change in the plan. Consider computing eligibility once, or collapsing identical Raqes before the check.

milindsrivastava1997 · 2026-10-07T02:35:53Z

    pub query_frequency_hz: f64,

+    /// Leaf occurrences merged into this item, across all RQEs.
+    pub occurrences: usize,


occurrences and query_frequency_hz are two sources of truth (freq = occurrences * 1000 / t_repeat_ms). Fixtures already set them inconsistently: the greedy test helper takes an arbitrary freq_hz with occurrences: 1. Suggest deriving the frequency with a query_frequency_hz() method.

milindsrivastava1997 · 2026-10-07T02:35:53Z

+        for k in 0..count {
+            raqes.push(Raqe {
+                // The item index keeps ids unique across T and SLAs.
+                id: format!("{index}:{}#{k}", raqe.id),


Raqe ids start with the sorted item index, so adding or removing any query renumbers every later item (3:q#0 → 4:q#0). If A3 keys configs or diffs on these ids, unchanged queries will look new. A content-based key (query, T, SLAs) would keep the ids stable.

milindsrivastava1997 mentioned this pull request Oct 6, 2026

Tracking: use sketch-bench rqe-optimizer MILP in asap-planner-rs #790

Open

3 of 9 tasks

milindsrivastava1997 commented Oct 6, 2026

View reviewed changes

milindsrivastava1997 force-pushed the 790-a2b-milp-inputs branch from 1806411 to b0ce7a7 Compare October 6, 2026 22:18

milindsrivastava1997 changed the base branch from latency-sla-ms to main October 6, 2026 22:18

milindsrivastava1997 commented Oct 6, 2026

View reviewed changes

milindsrivastava1997 mentioned this pull request Oct 6, 2026

rqe-optimizer: split SumOrCount and TopK by kind; deny unknown AtomicCostEntry fields ProjectASAP/sketch-bench#161

Closed

milindsrivastava1997 commented Oct 7, 2026

View reviewed changes

milindsrivastava1997 merged commit acebd27 into main Oct 7, 2026
8 checks passed

milindsrivastava1997 deleted the 790-a2b-milp-inputs branch October 7, 2026 02:41

Conversation

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

Uh oh!

What

Design decisions (also on #790)

Interface for later sessions

Smoke run

Test plan

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!

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

milindsrivastava1997 commented Oct 6, 2026 •

edited

Loading