Repository navigation
feat(planner): enforce candidate SLA eligibility - #781
milindsrivastava1997 wants to merge 2 commits into
Conversation
milindsrivastava1997
left a comment
There was a problem hiding this comment.
Code review: 3 findings inline.
| AggregationType::DatasketchesKLL => measurements.get(KLL_ERROR_METRIC), | ||
| AggregationType::HLL => measurements.get(HLL_ERROR_METRIC), | ||
| AggregationType::CountMinSketchWithHeap => measurements.get(CMS_HEAP_RECALL_METRIC), | ||
| _ => return false, |
There was a problem hiding this comment.
Exact cardinality aggregators are rejected. SetAggregator and DeltaSetAggregator are exact but aren't in exact_accumulator(), so they fall through to this arm and are rejected whenever accuracy_sla > 0. Now that accuracy_sla is required to be in (0, 1], that's always, and HLL is the only remaining option for cardinality queries.
Scenario: optimizer_cli with no atomic-cost table, or none with an HLL row meeting 0.99. The cardinality query used to get SetAggregator; it now fails with UnservableItems.
Fix: add both types to exact_accumulator.
| // Build ControllerConfig, including current configs as context for the planner. | ||
| // NOTE: existing_* fields are wired through but the planner does not yet act on them. | ||
| let mut controller_config = to_controller_config(instants, ranges); | ||
| let mut controller_config = to_controller_config_with_options( |
There was a problem hiding this comment.
These SLAs never reach anything that reads them. This config goes to LocalPlannerClient, then Controller::generate, then the legacy generator::generate_plan, which doesn't read controller_options. Only the greedy optimizer (optimizer_cli / pipeline.rs / candidate_gen_dump) uses SLAs.
Meanwhile check_config (engine_config.rs) now refuses to start when query_tracker.enabled is true and accuracy_sla is left at its 0.0 default. Existing deployments that only set enabled + observation_window_secs will fail at startup. Setting accuracy_sla fixes the startup but changes nothing about the plan, even though the drop-in README says it controls the SLAs for every inferred query.
There was a problem hiding this comment.
This was generated by AI during triage.
Confirmed: the tracker feeds the legacy planner path, so these settings are inert and make existing drop-in config fail validation. Tracked separately in #782; this PR will remove the tracker-facing SLA changes.
| .validate() | ||
| .map_err(ControllerError::PlannerError)?; | ||
| } | ||
| config.warn_default_slas(); |
There was a problem hiding this comment.
The query-log path still uses accuracy_sla = 0.0, with no check and now no warning. from_query_log and from_query_log_with_schema still call query_log::to_controller_config, which builds ControllerOptions::default() (accuracy_sla = 0.0). YAML configs now reject 0.0, but this path skips that check, and removing warn_default_slas means nothing is logged either.
Eligibility treats 0.0 as unconstrained (accuracy_sla == 0.0 => true), so every group built from a query log accepts any inaccurate sketch without notice.
There was a problem hiding this comment.
This was generated by AI during triage.
Confirmed: query-log planning still constructs zero accuracy options outside YAML validation. Tracked separately in #783; this PR will not retain that inconsistent path.
| use super::cost_model::estimated_query_cpu_secs; | ||
| use super::solution::OptimizerItem; | ||
|
|
||
| const CMS_ERROR_METRIC: &str = "relative_error_mean"; |
There was a problem hiding this comment.
CMS candidates can never pass the accuracy check. relative_error_mean is checked against accuracy_sla, which must be in [0, 1]. But in real sketch-bench output (sketch-bench/out/rqe_atomic_costs.json), every cms-fastpath-vector2d entry has relative_error_mean between ~53 and ~158. That metric averages over the long tail of rare keys. sketch-bench's rqe-optimizer/examples/small_problem.rs says to use are_top100 instead.
accuracy_sla > 0 is now required, so this check always runs and CMS is never eligible. Any count/sum query that only CMS can serve fails with "no candidate satisfies accuracy_sla or latency_sla". The unit tests miss this because their fixtures use synthetic 0.01 values.
Suggest switching CMS_ERROR_METRIC to are_top100 (or another bounded metric) and adding a test fixture with realistic values.
|
Heads-up: a companion PR is about to bump asap-planner-rs's
The two PRs may conflict in 🤖 Generated with Claude Code |
|
The companion PR is up as a draft: #804. 🤖 Generated with Claude Code |
|
@zzylol you're not using this or updating the code here right? This is old stuff (from when I was trying to port MILP implementation here) and can be closed. |
Summary
Testing