fix(planner): consistent latency_sla_ms defaults and stricter config 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>

Author: milindsrivastava1997

asap-planner-rs/src/config/input.rs

@@ -226,6 +226,7 @@ pub struct HllParams {
 }
 
 #[derive(Debug, Clone, Deserialize)]
+#[serde(deny_unknown_fields)]
 pub struct SQLControllerConfig {
     pub query_groups: Vec<SQLQueryGroup>,
     pub tables: Vec<TableDefinition>,
@@ -235,6 +236,7 @@ pub struct SQLControllerConfig {
 }
 
 #[derive(Debug, Clone, Deserialize)]
+#[serde(deny_unknown_fields)]
 pub struct SQLQueryGroup {
     pub id: Option<u32>,
     pub queries: Vec<String>,
@@ -252,13 +254,15 @@ pub struct TableDefinition {
 }
 
 #[derive(Debug, Clone, Deserialize)]
+#[serde(deny_unknown_fields)]
 pub struct ElasticDSLControllerConfig {
     pub query_groups: Vec<ElasticDSLQueryGroup>,
     pub sketch_parameters: Option<SketchParameterOverrides>,
     pub aggregate_cleanup: Option<AggregateCleanupConfig>,
 }
 
 #[derive(Debug, Clone, Deserialize)]
+#[serde(deny_unknown_fields)]
 pub struct ElasticDSLQueryGroup {
     pub id: Option<u32>,
     pub queries: Vec<String>,
@@ -323,8 +327,7 @@ query_groups:
         );
     }
 
-    // The unitless `latency_sla` key was never enforced; reject it so stale
-    // configs fail loudly instead of silently losing their latency limit.
+    // Reject the unitless `latency_sla` key so it can't be silently ignored.
     #[test]
     fn rejects_legacy_latency_sla_key() {
         let yaml = r#"
@@ -362,6 +365,40 @@ query_groups:
         );
     }
 
+    #[test]
+    fn sql_and_elastic_configs_reject_unknown_group_keys() {
+        let sql = r#"
+tables: []
+query_groups:
+  - queries: ["SELECT 1"]
+    repetition_delay_ms: 60000
+    latency_sla: 1.0
+    controller_options:
+      accuracy_sla: 0.99
+"#;
+        let error = serde_yaml::from_str::<SQLControllerConfig>(sql)
+            .expect_err("unknown SQL group key must be rejected")
+            .to_string();
+        assert!(error.contains("unknown field `latency_sla`"), "{error}");
+
+        let elastic = r#"
+query_groups:
+  - queries: ["{}"]
+    repetition_delay_ms: 60000
+    index: i
+    time_field: t
+    controler_options:
+      accuracy_sla: 0.99
+"#;
+        let error = serde_yaml::from_str::<ElasticDSLControllerConfig>(elastic)
+            .expect_err("unknown Elastic group key must be rejected")
+            .to_string();
+        assert!(
+            error.contains("unknown field `controler_options`"),
+            "{error}"
+        );
+    }
+
     #[test]
     fn rejects_non_positive_or_non_finite_latency_sla_ms() {
         for bad in ["0.0", "-5.0", ".inf", ".nan"] {

asap-tools/experiments/CONFIG_PARAMETERS_REFERENCE.md

@@ -515,7 +515,7 @@ These parameters come from the `experiment_type` config group and are prefixed w
 
 #### `experiment_params.query_groups[].controller_options.latency_sla_ms` (float, optional)
 - **Description**: Maximum modeled query latency in milliseconds
-- **Default**: `1000` for SQL experiments (`DEFAULT_LATENCY_SLA_MS` in `experiment_utils/config.py`); for PromQL experiments, omitted means no latency limit
+- **Default**: omitted (no latency limit)
 - **Range**: finite and > 0
 - **Example**: `250`
 - **Usage**: Intended as a hard ceiling on query latency for the planner. Not yet enforced: plans are not rejected for exceeding it.

asap-tools/experiments/experiment_utils/config.py

@@ -3,6 +3,7 @@
 Contains functions for validating configs, generating controller configs, etc.
 """
 
+import math
 import os
 import copy
 import yaml
@@ -15,8 +16,6 @@
 
 # Keys the planner's `ControllerOptions` accepts.
 CONTROLLER_OPTION_KEYS = {"accuracy_sla", "latency_sla_ms"}
-# Per-query latency ceiling sent to the planner when a SQL query group omits one.
-DEFAULT_LATENCY_SLA_MS = 1000
 
 
 def validate_basic_config(
@@ -419,10 +418,11 @@ def generate_controller_client_configs(
         }
         # Only the query client reads `client_options`; any other unknown group
         # key reaches the planner and is rejected there.
-        controller_only_config["query_groups"] = [
-            {k: v for k, v in group.items() if k != "client_options"}
-            for group in controller_only_config.get("query_groups", [])
-        ]
+        if "query_groups" in controller_only_config:
+            controller_only_config["query_groups"] = [
+                {k: v for k, v in group.items() if k != "client_options"}
+                for group in controller_only_config["query_groups"]
+            ]
         with open(
             os.path.join(
                 output_dir, "{}_controller_input.yaml".format(experiment_mode["mode"])
@@ -840,6 +840,22 @@ def generate_clickhouse_client_configs(
     return modes
 
 
+def _positive_latency_sla_ms(value: Any, idx: int) -> float:
+    """Fail at generation time rather than on the remote planner."""
+    # bool is an int subclass; `true` would otherwise become 1.0.
+    if isinstance(value, bool) or not isinstance(value, (int, float)):
+        raise ValueError(
+            f"query_groups[{idx}].controller_options.latency_sla_ms must be a "
+            f"number, got {value!r}"
+        )
+    if not (math.isfinite(value) and value > 0):
+        raise ValueError(
+            f"query_groups[{idx}].controller_options.latency_sla_ms must be "
+            f"finite and > 0, got {value!r}"
+        )
+    return float(value)
+
+
 def generate_sql_planner_input(
     query_groups: Any,
     dataset_cfg: Any,
@@ -865,7 +881,7 @@ def generate_sql_planner_input(
         query_groups: ListConfig of query group dicts.
             Each entry must have ``sql_file``, ``repetition_delay_ms``, and
             ``controller_options`` (``accuracy_sla``, optional
-            ``latency_sla_ms`` defaulting to ``DEFAULT_LATENCY_SLA_MS``).
+            ``latency_sla_ms``; omitted or null means no latency limit).
         dataset_cfg: DictConfig with ``table``/``name``, and ``precompute``
             sub-config (``timestamp_col``, ``value_col``, ``label_cols``).
         sketch_parameters: Optional DictConfig/dict mirroring ``config.yaml``'s
@@ -924,10 +940,12 @@ def generate_sql_planner_input(
         planner_ctrl_opts = {
             "accuracy_sla": float(ctrl_opts.get("accuracy_sla", 0.95)),
         }
-        # An explicit null means no latency limit, as in the planner.
-        latency_sla_ms = ctrl_opts.get("latency_sla_ms", DEFAULT_LATENCY_SLA_MS)
+        # Omitted or null means no latency limit, as in the planner.
+        latency_sla_ms = ctrl_opts.get("latency_sla_ms")
         if latency_sla_ms is not None:
-            planner_ctrl_opts["latency_sla_ms"] = float(latency_sla_ms)
+            planner_ctrl_opts["latency_sla_ms"] = _positive_latency_sla_ms(
+                latency_sla_ms, idx
+            )
         planner_query_groups.append(
             {
                 "id": idx + 1,

asap-tools/experiments/tests/test_controller_client_configs.py

@@ -0,0 +1,49 @@
+"""Tests for the planner input written from experiment parameters."""
+
+import os
+import tempfile
+import unittest
+
+import yaml
+from omegaconf import OmegaConf
+
+from experiment_utils.config import generate_controller_client_configs
+
+
+def _controller_input(experiment_params):
+    with tempfile.TemporaryDirectory() as tmp:
+        generate_controller_client_configs(OmegaConf.create(experiment_params), tmp)
+        path = os.path.join(
+            tmp, "controller_client_configs", "sketchdb_controller_input.yaml"
+        )
+        with open(path) as f:
+            return yaml.safe_load(f)
+
+
+BASE = {
+    "servers": [{"name": "sketchdb", "url": "http://localhost:8088"}],
+    "experiment": [{"mode": "sketchdb", "server": "sketchdb"}],
+    "metrics": [{"metric": "m", "labels": ["job"], "exporter": "fake"}],
+}
+
+
+class ControllerInputTest(unittest.TestCase):
+    def test_client_options_are_stripped_from_query_groups(self):
+        group = {
+            "id": 1,
+            "queries": ["sum(m)"],
+            "repetition_delay_ms": 60000,
+            "client_options": {"repetitions": 1},
+            "controller_options": {"accuracy_sla": 0.99},
+        }
+        planner_input = _controller_input({**BASE, "query_groups": [group]})
+        self.assertNotIn("client_options", planner_input["query_groups"][0])
+        self.assertIn("controller_options", planner_input["query_groups"][0])
+
+    def test_missing_query_groups_stays_missing(self):
+        # An empty list would parse and plan nothing; a missing key fails loudly.
+        self.assertNotIn("query_groups", _controller_input(BASE))
+
+
+if __name__ == "__main__":
+    unittest.main()

asap-tools/experiments/tests/test_sql_planner_input.py

@@ -7,13 +7,13 @@
 import yaml
 from omegaconf import OmegaConf
 
-from experiment_utils.config import DEFAULT_LATENCY_SLA_MS, generate_sql_planner_input
+from experiment_utils.config import generate_sql_planner_input
 
 
 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
-        # with no latency limit and never saw the bad key.
+        # Unknown keys must fail here: dropping them would hide them from the
+        # planner's strict parse.
         groups = [
             {
                 "sql_file": "unused.sql",
@@ -65,11 +65,16 @@ def _planner_latency(self, controller_options):
             "latency_sla_ms"
         )
 
-    def test_omitted_latency_sla_ms_uses_default(self):
-        self.assertEqual(
-            self._planner_latency({"accuracy_sla": 0.99}),
-            float(DEFAULT_LATENCY_SLA_MS),
-        )
+    def test_omitted_latency_sla_ms_means_no_limit(self):
+        # Same meaning as the PromQL path and the planner.
+        self.assertIsNone(self._planner_latency({"accuracy_sla": 0.99}))
+
+    def test_invalid_latency_sla_ms_is_rejected(self):
+        for bad in [0, -5, float("inf"), float("nan"), True, "1000"]:
+            with self.subTest(bad=bad), self.assertRaisesRegex(
+                ValueError, "latency_sla_ms"
+            ):
+                self._planner_latency({"accuracy_sla": 0.99, "latency_sla_ms": bad})
 
     def test_explicit_latency_sla_ms_is_kept(self):
         self.assertEqual(