Skip to content

Commit 9ae4522

Browse files
zzylolclaude
andcommitted
fix(planner): address #804's second review: m from the lookback, checked heap
- heap_size takes m from the plan, the most windows a served query's lookback merges (query_instance_count), and emits max(planned heap, m · max(TOPK_K, largest literal k)); the row's heap can be a measured size above m · TOPK_K. No division by TOPK_K, checked m · k (HeapSize error), and a row without a heap is MissingParam, not a panic. - require_shapes only fires for a sketch family that has cost rows and is a candidate; others are left to the Unservable path. - GroupEntry's doc states the MissingShape behavior; DuplicateLabels is checked before shape validation; DataShape is built from the locals. - Tests: m from the lookback under a larger measured heap, overflow and a missing heap, and require_shapes skipping a family without rows. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FhcWJExZcmVqS6r6rEtjis
1 parent b93ddf7 commit 9ae4522

3 files changed

Lines changed: 102 additions & 32 deletions

File tree

‎asap-planner-rs/src/optimizer/milp.rs‎

Lines changed: 23 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -70,7 +70,7 @@ pub fn solve_milp(
7070
allow_undeployable_families: bool,
7171
accuracy: &Accuracy,
7272
) -> Result<MilpSolution, MilpError> {
73-
require_shapes(workload, facts, allow_undeployable_families)?;
73+
require_shapes(workload, facts, costs, allow_undeployable_families)?;
7474
let deployments = build_all_candidates(
7575
&workload.raqes,
7676
costs,
@@ -108,18 +108,22 @@ pub fn solve_milp(
108108
Ok(solution)
109109
}
110110

111-
/// A Raqe sketches may serve reads their accuracy off the curves at its
112-
/// grouping's `shape`; without one it could only be reported unservable.
111+
/// A Raqe a sketch family with cost rows may serve reads its accuracy off
112+
/// the curves at its grouping's `shape`; without one it could only be
113+
/// reported unservable. Families without rows are left to that path.
113114
fn require_shapes(
114115
workload: &MilpWorkload,
115116
facts: &WorkloadFacts,
117+
costs: &[AtomicCostEntry],
116118
allow_undeployable_families: bool,
117119
) -> Result<(), MilpError> {
118120
for raqe in &workload.raqes {
119121
let sketch_served = raqe
120122
.capability
121123
.candidate_families(allow_undeployable_families)
122-
.any(|family| !family_properties(family).exact);
124+
.any(|family| {
125+
!family_properties(family).exact && costs.iter().any(|row| row.sketch == family)
126+
});
123127
let has_shape = facts
124128
.get(&raqe.metric)
125129
.is_some_and(|m| m.data_shape.contains_key(&raqe.grouping_labels));
@@ -610,6 +614,21 @@ metrics:
610614
if raqe.contains(quantile) && *grouping == labels(&["instance", "job"])),
611615
"{err:?}"
612616
);
617+
// No sketch rows for quantiles: left to the Unservable path.
618+
let exact_rows: Vec<_> = costs()
619+
.into_iter()
620+
.filter(|row| row.sketch == "exact-sum")
621+
.collect();
622+
let err = solve_milp(
623+
&w,
624+
&facts,
625+
&exact_rows,
626+
Objective::default(),
627+
false,
628+
&table_accuracy,
629+
)
630+
.unwrap_err();
631+
assert!(matches!(err, MilpError::Unservable(_)), "{err:?}");
613632
let exact_only = config(&group(sum, 0.99));
614633
let w = build_milp_workload(&exact_only, &facts, SCRAPE_MS).unwrap();
615634
assert!(solve_milp(

‎asap-planner-rs/src/optimizer/milp_output.rs‎

Lines changed: 64 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,12 @@ pub enum MilpOutputError {
4343
InvalidDdsAlpha { alpha: f64 },
4444
#[error("topk query {0:?} has no literal k")]
4545
TopkWithoutK(String),
46+
#[error("{variant}: can't size the heap for a {lookback_ms} ms lookback: {reason}")]
47+
HeapSize {
48+
variant: String,
49+
lookback_ms: u64,
50+
reason: &'static str,
51+
},
4652
#[error("query {0:?} is served by two deployments; a query string can name only one")]
4753
QueryOnTwoDeployments(String),
4854
#[error(transparent)]
@@ -242,14 +248,29 @@ fn missing_param(variant: &str, param: &'static str) -> MilpOutputError {
242248
}
243249
}
244250

245-
/// `m · k` for the `m` windows a query merges, so the merged heaps still hold
246-
/// the top k. The plan priced `m · TOPK_K` (sketch-bench's `heap` param); a
247-
/// literal k above `TOPK_K` scales it to `m · k`, which the plan doesn't price.
251+
/// `m · k` for the most windows `m` a served query merges, so the merged
252+
/// heaps still hold the top k, and never below the heap the plan priced
253+
/// (sketch-bench's `heap` param, which can be a measured size above
254+
/// `m · TOPK_K`). A literal k above `TOPK_K` is one the plan doesn't price.
248255
fn heap_size(deployment: &Deployment, items: &[&OptimizerItem]) -> Result<u64, MilpOutputError> {
249-
let planned =
250-
rqe_optimizer::heap_capacity(&deployment.config).expect("cms-heap-topk has a heap");
256+
let variant = deployment.config.sketch.as_str();
257+
let planned = rqe_optimizer::heap_capacity(&deployment.config)
258+
.ok_or_else(|| missing_param(variant, "heap"))?;
251259
let k = max_topk_k(items)?.max(rqe_optimizer::TOPK_K);
252-
Ok(planned / rqe_optimizer::TOPK_K * k)
260+
let mut heap = planned;
261+
for item in items {
262+
let lookback_ms = item.requirements.data_range_ms;
263+
let error = |reason| MilpOutputError::HeapSize {
264+
variant: variant.to_string(),
265+
lookback_ms,
266+
reason,
267+
};
268+
let m = deployment
269+
.query_instance_count(lookback_ms)
270+
.ok_or_else(|| error("the lookback isn't whole windows"))?;
271+
heap = heap.max(m.checked_mul(k).ok_or_else(|| error("m · k overflows"))?);
272+
}
273+
Ok(heap)
253274
}
254275

255276
/// One heap serves every topk query on the deployment, so it is sized for
@@ -440,20 +461,50 @@ mod tests {
440461
}
441462
}
442463

443-
/// A deployment merging `m` = 4 windows (heap 4 · TOPK_K) gets `4 · k`:
444-
/// the planned heap for a k within `TOPK_K`, scaled for a larger one.
464+
/// `query`'s planned top-k deployment, re-windowed to 15 s so its 1 m
465+
/// lookback merges m = 4 windows, with the cost row's `heap`.
466+
fn merging_deployment(query: &str, heap: serde_json::Value) -> (MilpWorkload, Deployment) {
467+
let (_, workload, solution) = plan(&group(query, 0.99));
468+
let mut deployment = solution.deployments[0].deployment.clone();
469+
(deployment.window_ms, deployment.slide_ms) = (15_000, 15_000);
470+
deployment.config.sketch_config["params"]["heap"] = heap;
471+
(workload, deployment)
472+
}
473+
474+
/// m comes from the lookback, not the row's heap: a measured heap of 2048
475+
/// (above 4 · TOPK_K) stays the floor, and a large k needs 4 · k.
445476
#[test]
446-
fn heap_size_is_merged_windows_times_k() {
447-
for (k, heap) in [(5, 4 * rqe_optimizer::TOPK_K), (4096, 4 * 4096)] {
477+
fn heap_size_takes_m_from_the_lookback() {
478+
for (k, heap) in [(5, 2048), (4096, 4 * 4096)] {
448479
let query = format!("topk({k}, sum_over_time(http_requests_total[1m]))");
449-
let (_, workload, solution) = plan(&group(&query, 0.99));
450-
let mut deployment = solution.deployments[0].deployment.clone();
451-
deployment.config.sketch_config["params"]["heap"] = json!(4 * rqe_optimizer::TOPK_K);
480+
let (workload, deployment) = merging_deployment(&query, json!(2048));
452481
let items: Vec<&OptimizerItem> = workload.items.iter().collect();
453482
assert_eq!(heap_size(&deployment, &items).unwrap(), heap, "k = {k}");
454483
}
455484
}
456485

486+
#[test]
487+
fn heap_size_reports_overflow_and_a_missing_heap() {
488+
let query = "topk(4611686018427387904, sum_over_time(http_requests_total[1m]))";
489+
let (workload, deployment) = merging_deployment(query, json!(2048));
490+
let items: Vec<&OptimizerItem> = workload.items.iter().collect();
491+
assert!(matches!(
492+
heap_size(&deployment, &items),
493+
Err(MilpOutputError::HeapSize {
494+
reason: "m · k overflows",
495+
..
496+
})
497+
));
498+
let small = "topk(5, sum_over_time(http_requests_total[1m]))";
499+
let (workload, mut deployment) = merging_deployment(small, json!(2048));
500+
deployment.config.sketch = "cms-fastpath-vector2d".into();
501+
let items: Vec<&OptimizerItem> = workload.items.iter().collect();
502+
assert!(matches!(
503+
heap_size(&deployment, &items),
504+
Err(MilpOutputError::MissingParam { param: "heap", .. })
505+
));
506+
}
507+
457508
/// A k above the plan's `TOPK_K` gets `m · k`, so its merged heaps still
458509
/// hold the top k.
459510
#[test]

‎asap-planner-rs/src/optimizer/workload_facts.rs‎

Lines changed: 15 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -59,8 +59,8 @@ struct GroupEntry {
5959
labels: Vec<String>,
6060
cardinality: u64,
6161
/// The data one group's sketch sees, fitted by the caller as the worst
62-
/// case over windows and groups. A grouping without one has no sketch
63-
/// accuracy, so only exact accumulators serve it.
62+
/// case over windows and groups. A grouping a sketch family with cost
63+
/// rows would serve must carry one, or solving fails with `MissingShape`.
6464
#[serde(default)]
6565
shape: Option<ShapeEntry>,
6666
}
@@ -113,6 +113,15 @@ pub fn parse_workload_facts(
113113
let mut data_shape = BTreeMap::new();
114114
for group in entry.groups {
115115
let labels: LabelSet = group.labels.into_iter().collect();
116+
if cardinality
117+
.insert(labels.clone(), group.cardinality)
118+
.is_some()
119+
{
120+
return Err(WorkloadFactsError::DuplicateLabels {
121+
metric: entry.metric,
122+
labels,
123+
});
124+
}
116125
if let Some(shape) = group.shape {
117126
let ShapeEntry {
118127
zipf_s,
@@ -129,23 +138,14 @@ pub fn parse_workload_facts(
129138
});
130139
}
131140
data_shape.insert(
132-
labels.clone(),
141+
labels,
133142
DataShape {
134-
zipf_s: shape.zipf_s,
135-
distinct_keys: shape.distinct_keys,
136-
tail_index: shape.tail_index,
143+
zipf_s,
144+
distinct_keys,
145+
tail_index,
137146
},
138147
);
139148
}
140-
if cardinality
141-
.insert(labels.clone(), group.cardinality)
142-
.is_some()
143-
{
144-
return Err(WorkloadFactsError::DuplicateLabels {
145-
metric: entry.metric,
146-
labels,
147-
});
148-
}
149149
}
150150
tracing::debug!(
151151
metric = %entry.metric,

0 commit comments

Comments
 (0)