Skip to content

feat(planner): add basic scaffolding for optimization problem in planner#407

Merged
milindsrivastava1997 merged 9 commits into
mainfrom
405-feat-optimization-based-sketch-config-selection
Jun 23, 2026
Merged

feat(planner): add basic scaffolding for optimization problem in planner#407
milindsrivastava1997 merged 9 commits into
mainfrom
405-feat-optimization-based-sketch-config-selection

Conversation

@milindsrivastava1997

Copy link
Copy Markdown
Contributor
  • feat(optimizer): scaffold OptimizerSolution type and translator
  • feat(optimizer): add AQE extractor with GCD/min/sum frequency tracking
  • feat(optimizer): wire all-EXACT end-to-end pipeline (Phase 1)

milindsrivastava1997 and others added 7 commits June 19, 2026 17:05
Adds asap-planner-rs/src/optimizer/ with:
- Aqe: atomic query expression wrapper (QueryRequirements + query strings + f_a)
- QueryMethod: Neither / Merge{num_windows} / Subtract / Exact
- AqeAssignment: maps an AQE to a deployed config + query method + cost estimate
- OptimizerSolution: full planner output (deployed configs + assignments + cost)
  with all_exact() constructor for Phase 1 scaffolding
- translator: translate() -> (StreamingConfig, InferenceConfig); Phase 1 stub
  with TODO for Phase 2 query config population

Also adds design doc for the optimization formulation (#405).

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Adds optimizer/aqe_extractor.rs:
- Rqe struct (query_string + t_repeat_secs)
- extract_aqes(): decomposes RQEs into deduplicated AQEs via recursive
  binary-op splitting and PromQL pattern matching
- Computes three frequency values per AQE:
  - query_frequency_hz: Σ 1/T_r (total query load for MIP objective)
  - min_t_repeat_secs: min(T_r) (freshness bound on W ≤ min_t)
  - t_repeat_gcd_secs: GCD(T_r) (natural slide interval S for candidate gen)
- Hand-rolled Euclidean GCD (num-integer not in workspace)
- TODO noting duplication with build_query_requirements_promql in query-engine

Updates Aqe struct in solution.rs to carry all three frequency fields.
5 unit tests covering single queries, binary splits, scalar arms, dedup.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Adds optimizer/pipeline.rs with run_all_exact_pipeline():
  ControllerConfig + PromQLSchema
    → config_to_rqes() (flatten QueryGroups)
    → extract_aqes()
    → OptimizerSolution::all_exact()
    → translate() → (StreamingConfig, InferenceConfig)

No streaming configs deployed in this path — every AQE falls back to
raw data. Validates end-to-end plumbing before Phase 2 sketch selection.

2 tests: empty streaming config for all-EXACT, group flattening.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… generation

Adds sketch_properties.rs (mergeable/subtractable/subpopulation_aware per
AggregationType) and candidate_gen.rs (enumerates candidate streaming configs
per AQE across agg type, param grid, window size, and ingest type). Sliding
candidates sweep slide interval S as well as the forced W=range_a, trading
ingest cost for freshness.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…rates

Adds cost_model.rs implementing IngestCost(g) and QueryCost(a,g) from the
design doc's formulas, using stub AtomicCosts (real values come from
sketch-bench in Phase 3). Includes an EXACT fallback query cost so the
greedy/MIP objective has something non-zero to compare sketches against.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Adds greedy.rs (picks each AQE's independently-cheapest candidate via the
cost model), run_greedy_pipeline() in pipeline.rs, and populates
InferenceConfig.query_configs in translator.rs for non-Exact assignments.

Rebalances CostWeights::default() so memory and CPU costs are on comparable
scales (RAM-held-over-time is ~1e6x cheaper per unit than CPU-time in real
cloud pricing) — the prior equal weighting made EXACT always win regardless
of workload.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… pipeline

Adds asap-optimizer-cli (src/bin/optimizer_cli.rs), a second binary that runs
run_greedy_pipeline against a workload YAML and prints the resulting deployed
configs and query configs. Lets the optimizer be exercised against real
configs without touching Controller::generate(), which still goes through
the existing hardcoded generator::generate_plan() path unconditionally.

Also adds a doc comment on generate_plan() flagging that the optimizer
module exists as a not-yet-wired-in alternative, for anyone reading that
file without other context.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@milindsrivastava1997

Copy link
Copy Markdown
Contributor Author

Code review findings (ranked, verified against source)

Correctness — all confirmed by reading the actual code:

  1. aqe_extractor.rs:83entry.2 += 1.0 / rqe.t_repeat_secs as f64 with no zero-check. t_repeat_secs comes straight from QueryGroup.repetition_delay: u64 (config/input.rs:51) with zero validation anywhere in the load path. A config with repetition_delay: 0 silently produces +inf query frequency that poisons cost comparisons for that AQE.
  2. aqe_extractor.rs:85-89 — the GCD accumulator uses entry.4 == 0 as both "uninitialized" and a legitimate value. A zero t_repeat_secs merging into a bucket causes later values to overwrite instead of GCD-combine, corrupting t_repeat_gcd_secs for that whole AQE.
  3. promql_utilities/src/query_logics/parsing.rs:71,81,84 (called from aqe_extractor.rs extract_requirements) — get_statistics_to_compute panic!s on any matched-but-unrecognized statistic/function name, contradicting aqe_extractor.rs:64-65's doc comment that unsupported queries are "skipped with a warning." Any dashboard query matching a pattern but using a statistic outside the current enum crashes the whole optimizer_cli run instead of skipping that one leaf.
  4. greedy.rs:38-40min_by comparator does .partial_cmp(...).unwrap(). optimizer_cli's --rho is parsed as a bare f64 with no validation; --rho nan propagates NaN through every cost, making partial_cmp return None and panicking instead of failing cleanly.
  5. translator.rs:60 vs candidate_gen.rsretention_count_for_assignment hardcodes 1 for QueryMethod::Subtract, while candidate_gen.rs sets the deployed config's actual retained-window count to n_windows. Self-documented as a Phase-3 placeholder, but worth flagging now since it's a silent mismatch waiting to bite once wired up.

Reuse/cleanup (lower severity, all confirmed):

  1. aqe_extractor.rs:250 (decompose_to_leaves/arm_to_query_string/strip_parens) — duplicates SingleQueryProcessor::get_binary_arm_queries/strip_parens already in planner/promql.rs:31-145. Two copies of PromQL binary-arm decomposition will drift.
  2. aqe_extractor.rs:298 (extract_requirements) — duplicates SimplePromQLEngine::build_query_requirements_promql (query-engine crate); the author's own TODO comment already flags this.
  3. greedy.rs:35-46total_cost_rate is recomputed twice per min_by comparison, then a 3rd/4th time afterward to recover the cost breakdown for best. Cheap fix: map once into (candidate, cost) pairs, min_by_key, reuse.
  4. pipeline.rsrun_all_exact_pipeline and run_greedy_pipeline are near-identical shells differing only in the solving step; a Phase-3 MIP pipeline will likely triple this boilerplate. Consider one run_pipeline(config, solver) now.

Skipped as non-findings: the derive_sub_type empty-string case (inert under current required_sub_type contract), param_grid's bool-slice plumbing, and the num_sketch_served/num_exact_fallback redundant getters — all real but cosmetic, not worth the budget here. No CLAUDE.md exists in this repo, so no conventions violations to report.

Findings from manual code review (we need to prioritize these)

Some of these may be big enough that we need to solve them in different issues after merging this. Some of these may be big enough to warrant an entire session/discussion.

solution.rs:

  • can we change sec to millisec (in other files too)
  • can we rename QueryMethod::Neither? it sounds weird
  • do we/can we eforce num_windows > 1 for QueryMethod::Merge
  • what is f_a?
  • Can we capitalize AQE and RQE structs?
  • why doesnt optimizersolution have estimate query cost per second

sketch_properties.rs:

  • what are SingleSubpopulation and MultipleSubpopulation types?
  • Should we up-level sketch properties as traits? Should they live in asap_sketchlib?

candidate_gen.rs:

  • we should put constants in a separate file
  • what should we do for multiple statistics? What happens right now? If unsupported, create an issue for this
  • CMSWithHeap -- we should remove count_events from the parameters, and instead have it as a subtype. That's what "sum" vs "count" does for CountMinSketch
  • What are the semantics of the "range_a_secs" parameter? Is this from a PromQL range query (where a query itself is repeated multipole times) or is it the time range of data queries by a single query instance? The "spatial query has None range" comment makes me question this. I guess technically, spatial query should have range = scrape interval? It's quite possible this is an issue with code outside this PR. Nonetheless, we should investigate and log issues as necessary
  • For sliding windows, grid search, the size of sliding window can also change right? If/when we do this, this may also need changes to the determine_query_method function
  • Here is my high-level logic for window param searching. I am curious to know if the code implements this or something different:
    • For tumbling, we need window size as multiples of scrape interval, and window size <= t_repeat
    • For sliding, we need both size and slide as multiple of scrape interval. window size <= time range of query. window slize <= t_repeat

aqe_extractor.rs:

  • can we replace the gcd function with a library call?

translator.rs:

  • for number of aggregates to retain for QueryMethod::Subtract, 1 makes sense but we also need to retain another aggregate that represents the sketch used for the previous query instance. It's possible this calculation/parameter is being used in 2 places and must be dismbiguated into 2 different parameters

cost_model.rs:

  • up level constants to a different file
  • can we rename: rho_g, n, g, a, f_a, etc
  • what is n? what is N(s, g)? We need more descriptive names
  • mem_retain for tumbling windows -- is this being double counted because it is also included in the query_memory?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant