refactor(planner): rename latency_sla to optional latency_sla_ms by milindsrivastava1997 · Pull Request #797 · ProjectASAP/ASAPQuery

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

Part of #790 (session 2b). Split out of A2b so the rename lands on its own.

What

controller_options.latency_sla → latency_sla_ms: Option<f64>.

The old field had no unit and was never enforced. sketch-bench's Raqe takes latency_sla_ms: Option<f64> (hard ceiling on modeled query latency), so the config now matches.
Optional; when present it must be finite and > 0. Omitted = no limit.
ControllerOptions gets #[serde(deny_unknown_fields)]: an old latency_sla: key is now a parse error instead of being silently ignored.
Internal plumbing (RQE, OptimizerItem, UnservableItem, the dedup key) carries latency_sla_ms: Option<f64>.
Every config that set the old unitless latency_sla (test/sample/quickstart/benchmark configs, asap-tools experiment YAMLs, the Python generators, the promql-compliance Go runner, doc examples) now sets an explicit latency_sla_ms: 1000 (1 s). Omitted or null means no limit everywhere; the SQL experiment generator validates the value (number, finite, > 0) at generation time.
Unknown keys are rejected on QueryGroup and on the SQL / Elastic DSL configs and query groups, and asap-tools rejects unknown SQL controller_options keys and strips only client_options from query groups before writing planner input.

Interface change (later sessions)

Workload configs must use latency_sla_ms or omit it. latency_sla no longer parses.

Note: the planner does not enforce it yet; the MILP path (#790 A2b, stacked PR) passes it to sketch-bench.

Test plan

New tests in config/input.rs: legacy key rejected; ≤ 0 / inf / NaN rejected; omitted → None.
cargo fmt, cargo clippy -p asap_planner --all-targets -D warnings, cargo test -p asap_planner (pre-commit also ran workspace check/clippy/test).
py_compile on changed Python; gofmt -l on run.go.

🤖 Generated with Claude Code

`controller_options.latency_sla` had no unit and was never enforced. The MILP planner takes a per-query latency ceiling in milliseconds, so the field becomes `latency_sla_ms: Option<f64>` (finite, > 0; omitted = no limit). `ControllerOptions` now denies unknown fields so an old `latency_sla` key fails loudly instead of being ignored. Old `latency_sla` lines are removed from configs, generators, the Go runner and docs. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

milindsrivastava1997 · 2026-10-06T21:25:45Z

            raise ValueError(f"No SQL statements found in {sql_file!r}")

        ctrl_opts = dict(group.get("controller_options") or {})
+        planner_ctrl_opts = {


This copies only accuracy_sla / latency_sla_ms out of controller_options, so a stale latency_sla key in an experiment YAML is dropped here before Rust's deny_unknown_fields ever sees it. The planner then runs with no latency ceiling and no error, which is the case rejects_legacy_latency_sla_key is meant to catch. Suggest passing ctrl_opts through unchanged, or raising on unknown keys.

Fixed in 0d3f0a8. generate_sql_planner_input now raises on any controller_options key other than accuracy_sla / latency_sla_ms, before loading the SQL file (test_sql_planner_input.py). The PromQL path already passes controller_options through unchanged, so Rust rejects a stale key there.

milindsrivastava1997 · 2026-10-06T21:25:45Z

@@ -79,11 +79,13 @@ pub struct QueryGroup {
 }



ControllerOptions now has deny_unknown_fields, but QueryGroup doesn't. A typo like controler_options: still parses, the whole SLA block falls back to its default, and the latency ceiling is silently lost. Consider adding deny_unknown_fields here too.

Fixed in 0d3f0a8. QueryGroup now has deny_unknown_fields (test rejects_misspelled_controller_options). Experiment configs keep client_options inside each query group, so the asap-tools controller-input writer now strips just that key per group. Any other unknown key still reaches the planner and fails.

milindsrivastava1997 · 2026-10-06T21:25:45Z

        for qg in &self.query_groups {
            let opts = &qg.controller_options;
-            if opts.accuracy_sla == 0.0 && opts.latency_sla == 0.0 {
+            if opts.accuracy_sla == 0.0 && opts.latency_sla_ms.is_none() {


warn_default_slas treats accuracy_sla == 0.0 && latency_sla_ms.is_none() as "controller_options missing". Omitting latency_sla_ms is now normal, so a valid explicit {accuracy_sla: 0.0} triggers a false warning. Making QueryGroup.controller_options an Option<ControllerOptions> would make this exact instead of a guess.

Leaving this as is. The false positive needs an explicit accuracy_sla: 0.0, which means "accept any error" and is almost never intended, so a warning there is arguably still useful. Making controller_options an Option would touch every caller for a log line. Happy to revisit if the warning becomes noisy.

milindsrivastava1997 · 2026-10-06T21:25:45Z

            t_repeat_ms: rqe.t_repeat_ms,
            accuracy_sla_bits: normalized_f64_bits(rqe.accuracy_sla),
-            latency_sla_bits: normalized_f64_bits(rqe.latency_sla),
+            latency_sla_ms_bits: rqe.latency_sla_ms.map(normalized_f64_bits),


The positive and finite rule for latency_sla_ms is enforced only in the serde deserializer. RQEs built in code (tests, query_log conversion, future callers) can pass Some(0.0), Some(-0.0) or Some(NaN) into the key and the MILP latency ceiling. NaN also gives otherwise-equal keys different bits. Consider validating where the RQE is built, or using a PositiveMs newtype.

Also, no test checks that items with different latency_sla_ms values (or Some vs None) stay distinct. different_slas_become_distinct_items only varies accuracy_sla, and the signed-zero test was moved to accuracy.

Added different_latency_slas_become_distinct_items in 0d3f0a8: None, Some(100) and Some(1000) stay three items. Skipping the newtype for now: every non-test RQE is built from a deserialized ControllerOptions (pipeline.rs, candidate_gen_dump.rs), and query_log uses Default (None), so Some(0.0) / NaN cannot reach the key today. We can add a PositiveMs newtype if a code-built RQE path appears.

- `QueryGroup` denies unknown fields, so a misspelled `controller_options` fails instead of silently falling back to default SLAs. - asap-tools strips only `client_options` from query groups for the planner input and rejects unknown SQL `controller_options` keys, so a stale `latency_sla` reaches an error instead of being dropped. - Test that differing `latency_sla_ms` values stay distinct items. - Docs note that `latency_sla_ms` is not enforced yet. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Configs, generators and the compliance runner that set the old unitless `latency_sla` now set an explicit 1 s `latency_sla_ms` ceiling instead of omitting it. SQL experiment planner input defaults to 1000 ms; an explicit null still means no limit. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

milindsrivastava1997 · 2026-10-06T22:07:48Z

+        planner_ctrl_opts = {
+            "accuracy_sla": float(ctrl_opts.get("accuracy_sla", 0.95)),
+        }
+        # An explicit null means no latency limit, as in the planner.


An omitted latency_sla_ms defaults to DEFAULT_LATENCY_SLA_MS (1000 ms) here, but the PromQL path (generate_controller_client_configs) and the planner treat omission as "no limit". The same experiment controller_options therefore yields a 1 s hard ceiling under SQL and none under PromQL — once the MILP enforces the SLA, SQL vs PromQL results won't be comparable. Should both paths share one default (or both mean no-limit)?

Fixed in 8d79801. The SQL generator no longer defaults it, so omitted (or null) means no limit on both paths, matching the planner. The experiment YAMLs set latency_sla_ms: 1000 explicitly, so actual runs are unchanged.

milindsrivastava1997 · 2026-10-06T22:07:48Z

+        }
+        # An explicit null means no latency limit, as in the planner.
+        latency_sla_ms = ctrl_opts.get("latency_sla_ms", DEFAULT_LATENCY_SLA_MS)
+        if latency_sla_ms is not None:


Key names are validated but the value isn't: 0/negatives pass through float() and only fail on the remote node after rsync + controller start, and latency_sla_ms: true silently becomes 1.0. Worth rejecting non-numeric/bool and <= 0 here so it fails at generation time.

Fixed in 8d79801. _positive_latency_sla_ms rejects bools, non-numbers, non-finite values and values ≤ 0 at generation time (test_invalid_latency_sla_ms_is_rejected).

milindsrivastava1997 · 2026-10-06T22:07:48Z

        }
+        # Only the query client reads `client_options`; any other unknown group
+        # key reaches the planner and is rejected there.
+        controller_only_config["query_groups"] = [


This always writes query_groups via .get("query_groups", []), so a config without the section now produces query_groups: [] instead of omitting the key. Previously the planner failed with a missing-field error; now it parses and plans nothing. Suggest only rewriting when the key is present.

Fixed in 8d79801. query_groups is rewritten only when it is present (test_controller_client_configs.py).

milindsrivastava1997 · 2026-10-06T22:07:48Z

 }

 #[derive(Debug, Clone, Deserialize)]
+#[serde(deny_unknown_fields)]


deny_unknown_fields is added to QueryGroup but not to SQLQueryGroup, ElasticDSLQueryGroup, or SQLControllerConfig. A typo'd group key (e.g. misspelled repetition_delay_ms, stray group-level latency_sla) is still silently dropped for SQL/Elastic configs while the same mistake is rejected for PromQL.

Fixed in 8d79801. SQLControllerConfig, SQLQueryGroup, ElasticDSLControllerConfig and ElasticDSLQueryGroup now deny unknown fields (sql_and_elastic_configs_reject_unknown_group_keys). Existing fixtures all still parse.

milindsrivastava1997 · 2026-10-06T22:07:48Z

        for qg in &self.query_groups {
            let opts = &qg.controller_options;
-            if opts.accuracy_sla == 0.0 && opts.latency_sla == 0.0 {
+            if opts.accuracy_sla == 0.0 && opts.latency_sla_ms.is_none() {


With omitted latency_sla_ms now a legitimate "no limit", accuracy_sla == 0.0 && latency_sla_ms.is_none() false-positives on an explicit controller_options: {accuracy_sla: 0.0}. Making controller_options: Option<ControllerOptions> and warning on None would detect omission directly instead of via sentinel values.

Same as the earlier thread on this line: leaving it. A false positive needs an explicit accuracy_sla: 0.0 ("accept any error"), and Option<ControllerOptions> would touch every caller for a log line.

milindsrivastava1997 · 2026-10-06T22:07:48Z

+        );
+    }
+
+    // The unitless `latency_sla` key was never enforced; reject it so stale


Nit: this comment narrates history ("was never enforced"). AGENTS.md asks comments to avoid historical facts — maybe just state what the test guards, e.g. "Reject the unitless latency_sla key so it can't be silently ignored."

Fixed in 8d79801. The comment now reads: "Reject the unitless latency_sla key so it can't be silently ignored."

milindsrivastava1997 · 2026-10-06T22:07:49Z

+
+class ControllerOptionKeysTest(unittest.TestCase):
+    def test_unknown_controller_option_is_rejected(self):
+        # A stale `latency_sla` used to be dropped here, so the planner ran


Nit: same as above — "used to be dropped here" is history. Suggest describing the current guarantee: unknown controller option keys must be rejected rather than dropped.

Fixed in 8d79801. It now states the guarantee: unknown keys must fail here because dropping them would hide them from the planner's strict parse.

milindsrivastava1997 · 2026-10-06T22:07:49Z

-	AccuracySLA float64 `yaml:"accuracy_sla"`
-	LatencySLA  float64 `yaml:"latency_sla"`
+	AccuracySLA  float64 `yaml:"accuracy_sla"`
+	LatencySLAMs float64 `yaml:"latency_sla_ms"`


LatencySLAMs float64 without omitempty can't express "no limit": any group that doesn't set it emits latency_sla_ms: 0, which the planner now rejects (must be > 0). Using *float64 with yaml:"latency_sla_ms,omitempty" would mirror the Rust Option<f64>.

Leaving as is: the struct's only construction site (run.go:380) always sets 1000, so 0 cannot be emitted. Will switch to *float64 + omitempty if a no-limit case is added.

…parsing - SQL experiment planner input no longer defaults `latency_sla_ms`; omitted means no limit, as on the PromQL path and in the planner. - asap-tools rejects non-numeric, boolean, non-finite or non-positive `latency_sla_ms` when generating SQL planner input. - Leave `query_groups` out of the planner input when the experiment has none, so the planner reports it missing instead of planning nothing. - SQL and Elastic DSL configs and query groups deny unknown fields, like PromQL ones. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

This was referenced Oct 6, 2026

feat(planner): build rqe-optimizer Raqes and facts from the workload config #798

Merged

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

Open

milindsrivastava1997 commented Oct 6, 2026

View reviewed changes

milindsrivastava1997 and others added 2 commits October 6, 2026 17:34

milindsrivastava1997 commented Oct 6, 2026

View reviewed changes

milindsrivastava1997 merged commit 3a27281 into main Oct 6, 2026
16 checks passed

Conversation

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

Uh oh!

What

Interface change (later sessions)

Test plan

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!

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 6, 2026 •

edited

Loading