Skip to content

Commit 4f367fe

Browse files
zzylolclaude
andcommitted
fix: reject stored outputs without one consistent Planner producer
Materializations no longer carry their input filter, so an output's population comes only from the DAG node that produces it. Plan validation now rejects: - an output produced by several DAGs that disagree on the update or on the population (metric and canonical filter) they read; - a raw time-series output that has no Planner producer in a plan that carries DAGs, which would otherwise be read as unfiltered; - a bound raw producer whose input is not a source scan. Also keeps plain OTLP sketch envelopes out of shared (Hydra) states, restores the single-catalog descriptor test, and drops comments that still named removed fields. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
1 parent 931ae0e commit 4f367fe

8 files changed

Lines changed: 186 additions & 35 deletions

File tree

‎crates/asap_summary_state/src/summary_kernels/factory.rs‎

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -615,9 +615,8 @@ pub enum TopkWeight {
615615
/// (`get_topk_keys` / `topk_heap_items`), which is what `topk(...)` queries
616616
/// need.
617617
///
618-
/// The key is the configured group-by (`aggregated_labels`) value vector —
619-
/// e.g. `host` — formed by `extract_aggregated_key_from_series` in the worker,
620-
/// NOT the hardcoded metric label `item`. The accumulated quantity is selected
618+
/// The key is the Planner update's item value (e.g. the series identity for
619+
/// PromQL Top-K), not a hardcoded metric label `item`. The accumulated quantity is selected
621620
/// by [`TopkWeight`]:
622621
/// * `Value` → `inner.update(key, value)` adds the datapoint value (Σ value).
623622
/// * `Count` → `inner.update(key, 1.0)` adds one per event (Σ count).
@@ -862,7 +861,7 @@ fn cms_dims(params: &SketchParams) -> (usize, usize) {
862861
(*depth as usize, *width as usize)
863862
}
864863
other => unreachable!(
865-
"accumulator_spec() paired SketchAlgorithm::Cms/CountSketch with unexpected params: {other:?}"
864+
"SketchKind paired SketchAlgorithm::Cms/CountSketch with unexpected params: {other:?}"
866865
),
867866
}
868867
}
@@ -882,7 +881,7 @@ fn cms_heap_dims(params: &SketchParams) -> (usize, usize, usize) {
882881
heap_size,
883882
} => (*depth as usize, *width as usize, *heap_size as usize),
884883
other => unreachable!(
885-
"accumulator_spec() paired a WithHeap SketchAlgorithm with unexpected params: {other:?}"
884+
"SketchKind paired a WithHeap SketchAlgorithm with unexpected params: {other:?}"
886885
),
887886
}
888887
}

‎crates/asap_types/src/key_by_label_names.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@
33
//! Formerly `promql_utilities::data_model::key_by_label_names` — moved
44
//! here for the same reason as [`crate::Statistic`]: `asap_types`
55
//! (`PrecomputeMaterialization::grouping_labels`, `PolicyFingerprint`,
6-
//! `PolicyRegistry`, `capability_matching`) is its real center of
6+
//! `capability_matching`) is its real center of
77
//! gravity and the shared foundation both `control_plane`'s ecosystem
88
//! and `data_plane` can depend on without a cycle. Closer to a runtime
99
//! index key than a planning IR node — ASAPController's

‎crates/asap_types/src/precompute_plan.rs‎

Lines changed: 76 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -314,6 +314,11 @@ pub enum PrecomputePlanError {
314314
InvalidSchema { schema_id: String },
315315
#[error("materialization {0} uses a summary family unsupported by the runtime schema")]
316316
UnsupportedFamily(u64),
317+
#[error("materialization {materialization} has no consistent Planner computation: {reason}")]
318+
InvalidComputation {
319+
materialization: u64,
320+
reason: String,
321+
},
317322
#[error("materialization {materialization} has an invalid window layout: {reason}")]
318323
InvalidWindowLayout {
319324
materialization: u64,
@@ -466,7 +471,9 @@ impl PrecomputePlan {
466471

467472
/// The Planner SummaryAgg node that produces `output`, with the source
468473
/// expression feeding it when that input is a raw source. `None` when no
469-
/// installed DAG produces the output with a SummaryAgg.
474+
/// installed DAG produces the output with a SummaryAgg. Every DAG that
475+
/// produces the output must agree on both, or the plan is rejected: the
476+
/// runtime routes, filters and catalogs the output once.
470477
#[allow(clippy::type_complexity)]
471478
pub fn summary_producer(
472479
&self,
@@ -479,6 +486,10 @@ impl PrecomputePlan {
479486
String,
480487
> {
481488
use planner_types::post_asap::PostAsapOperatorPayload;
489+
let mut selected: Option<(
490+
planner_types::post_asap::PostAsapDagNode,
491+
Option<planner_types::pre_asap::QueryExpr>,
492+
)> = None;
482493
for installed in self.executable_dags.values() {
483494
let Some(node) = installed.binding.nodes.iter().find_map(|(node, binding)| {
484495
matches!(binding, crate::executable_plan::BackendNodeBinding::Materialization { stored_output } if *stored_output == output).then_some(*node)
@@ -508,9 +519,39 @@ impl PrecomputePlan {
508519
}),
509520
_ => None,
510521
};
511-
return Ok(Some((producer.clone(), source)));
522+
if let Some((first, first_source)) = &selected {
523+
// Shared outputs may be read by scans that project different
524+
// columns; what must agree is the update and its population.
525+
let population =
526+
|node: &planner_types::post_asap::PostAsapDagNode,
527+
source: &Option<planner_types::pre_asap::QueryExpr>| {
528+
let PostAsapOperatorPayload::SummaryAgg { family, .. } = &node.payload
529+
else {
530+
return None;
531+
};
532+
source.as_ref().map(|expression| {
533+
raw_time_series_input_contract(
534+
expression,
535+
matches!(family, SummaryFamilyType::ExactAggregate(..)),
536+
)
537+
.map(|(metric, _, filter)| {
538+
(metric, crate::utils::normalize_spatial_filter(&filter))
539+
})
540+
})
541+
};
542+
if first.payload != producer.payload
543+
|| population(first, first_source) != population(producer, &source)
544+
{
545+
return Err(format!(
546+
"stored output {} is produced by DAGs that disagree on its computation",
547+
output.as_u64()
548+
));
549+
}
550+
} else {
551+
selected = Some((producer.clone(), source));
552+
}
512553
}
513-
Ok(None)
554+
Ok(selected)
514555
}
515556

516557
/// Canonical population predicate of `config`'s input: the typed table
@@ -523,8 +564,16 @@ impl PrecomputePlan {
523564
if config.table_name.is_some() || config.derived_input.is_some() {
524565
return Ok(table);
525566
}
526-
let Some((_, Some(expression))) = self.summary_producer(config.stored_output_id)? else {
527-
return Ok(String::new());
567+
let expression = match self.summary_producer(config.stored_output_id)? {
568+
Some((_, Some(expression))) => expression,
569+
// A bound raw output whose input cannot be read must not be
570+
// treated as unfiltered.
571+
Some((_, None)) => {
572+
return Err("raw output's Planner producer does not read a source scan".into())
573+
}
574+
// Only plans without a Planner DAG for this output (imported or
575+
// fixture outputs) have no predicate to read.
576+
None => return Ok(String::new()),
528577
};
529578
let family = self.state_family(config.stored_output_id);
530579
let (_, _, filter) = raw_time_series_input_contract(
@@ -1033,6 +1082,28 @@ impl PrecomputePlan {
10331082
schema.materialization.as_u64(),
10341083
));
10351084
}
1085+
let invalid_computation = |reason: String| PrecomputePlanError::InvalidComputation {
1086+
materialization: schema.materialization.as_u64(),
1087+
reason,
1088+
};
1089+
let raw =
1090+
materialization.table_name.is_none() && materialization.derived_input.is_none();
1091+
// Once a plan carries Planner DAGs, a raw output's input predicate
1092+
// exists only on its producer; an unbound output would silently
1093+
// read every series of its metric.
1094+
if raw
1095+
&& !self.executable_dags.is_empty()
1096+
&& self
1097+
.summary_producer(schema.materialization)
1098+
.map_err(invalid_computation)?
1099+
.is_none()
1100+
{
1101+
return Err(invalid_computation(
1102+
"raw output has no Planner producer".into(),
1103+
));
1104+
}
1105+
self.population_filter(materialization)
1106+
.map_err(invalid_computation)?;
10361107
let source = materialization.table_name.as_ref().map_or_else(
10371108
|| Source::TimeSeries {
10381109
metric: materialization.metric.clone(),

‎crates/asap_types/src/summary_catalog.rs‎

Lines changed: 15 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -583,14 +583,22 @@ mod tests {
583583
GroupingStrategy::PerSubpopulationInstance,
584584
)
585585
};
586-
let a = with_family(1, 1, &[config("requests", "", 60)], &kll(100)).unwrap();
587-
let b = with_family(1, 1, &[config("requests", "", 60)], &kll(200)).unwrap();
588-
assert_ne!(a.summary_descriptors, b.summary_descriptors);
589-
assert_eq!(a.data_descriptors, b.data_descriptors);
590-
let catalog = a;
591-
assert_eq!(catalog.summary_descriptors.len(), 1);
586+
let (small, large) = (kll(100), kll(200));
587+
let output = |family: &SummaryFamilyType| {
588+
let (mut config, _) = config("requests", "", 60);
589+
config.allocate_stored_output_id(family);
590+
config
591+
};
592+
let (a, b) = (output(&small), output(&large));
593+
let catalog = SummaryCatalog::from_outputs(
594+
1,
595+
1,
596+
vec![(&a, &small, String::new()), (&b, &large, String::new())],
597+
)
598+
.unwrap();
599+
assert_eq!(catalog.summary_descriptors.len(), 2);
592600
assert_eq!(catalog.data_descriptors.len(), 1);
593-
assert_eq!(catalog.outputs.len(), 1);
601+
assert_eq!(catalog.outputs.len(), 2);
594602
}
595603

596604
// Construction order cannot affect the published snapshot bytes.

‎data_plane/src/drivers/ingest/otel.rs‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1924,6 +1924,14 @@ fn derive_sketch_policy_fp(
19241924
if bound_output.is_some_and(|output| output != config.stored_output_id)
19251925
|| config.metric != metric
19261926
|| &keys != group_by_keys
1927+
// A plain sketch envelope never fills a shared (Hydra) state.
1928+
|| matches!(
1929+
snap.state_family(config.stored_output_id),
1930+
Some(planner_types::post_asap::SummaryFamilyType::Sketch(
1931+
_,
1932+
planner_types::post_asap::GroupingStrategy::SharedMultiSubpopulation { .. }
1933+
))
1934+
)
19271935
|| snap
19281936
.agg_kind(config.stored_output_id)
19291937
.is_none_or(|installed| installed.canonical_string() != expected)

‎data_plane/src/storage_engines/types/installed_precompute_plan.rs‎

Lines changed: 71 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -262,6 +262,77 @@ impl Default for InstalledPrecomputePlan {
262262

263263
#[cfg(test)]
264264
mod tests {
265+
fn filtered_plan(job: &str) -> control_plane::physical::compiler::PrecomputePlan {
266+
let mut snapshot: serde_json::Value = serde_json::from_str(include_str!(
267+
"../../../../docs/examples/asapquery-planning-snapshot.json"
268+
))
269+
.unwrap();
270+
snapshot["query_workload"]["repeating_queries"][0]["query"] =
271+
serde_json::json!(format!("quantile_over_time(0.99, m{{job=\"{job}\"}}[1m])"));
272+
crate::tests::test_utilities::planning::quoted_snapshot(
273+
serde_json::from_value(snapshot).unwrap(),
274+
false,
275+
)
276+
.compile_promql()
277+
.unwrap()
278+
.precompute_plan
279+
}
280+
281+
/// Rebind every materialization node of `plan`'s DAGs to `output`.
282+
fn bind_all(plan: &mut control_plane::physical::compiler::PrecomputePlan, output: u64) {
283+
for installed in plan.executable_dags.values_mut() {
284+
for binding in installed.binding.nodes.values_mut() {
285+
if let asap_types::executable_plan::BackendNodeBinding::Materialization {
286+
stored_output,
287+
} = binding
288+
{
289+
*stored_output = asap_types::sds::StoredOutputId(output);
290+
}
291+
}
292+
}
293+
}
294+
295+
// Two DAGs that read different populations cannot share one stored output.
296+
#[test]
297+
fn producers_disagreeing_on_population_are_rejected() {
298+
let mut plan = filtered_plan("a");
299+
let output = plan.materializations[0].stored_output_id.as_u64();
300+
let mut other = filtered_plan("b");
301+
bind_all(&mut other, output);
302+
let (_, mut dag) = other.executable_dags.into_iter().next().unwrap();
303+
dag.document.query_id = "other-population".into();
304+
plan.executable_dags.insert("other-population".into(), dag);
305+
let error = plan.validate().unwrap_err().to_string();
306+
assert!(error.contains("disagree on its computation"), "{error}");
307+
}
308+
309+
// A raw output with no Planner producer is not read as unfiltered.
310+
#[test]
311+
fn unbound_raw_output_is_rejected() {
312+
let mut plan = filtered_plan("a");
313+
let mut unbound = plan.materializations[0].clone();
314+
unbound.metric = "unbound".into();
315+
unbound.stored_output_id = asap_types::sds::StoredOutputId(1);
316+
let mut schema = plan.schemas[0].clone();
317+
schema.materialization = unbound.stored_output_id;
318+
schema.schema_id = schema.schema_id.replace(
319+
&plan.materializations[0]
320+
.stored_output_id
321+
.as_u64()
322+
.to_string(),
323+
"1",
324+
);
325+
schema.stored_output_reference =
326+
asap_types::sds::StoredOutputReference::for_output(unbound.stored_output_id);
327+
plan.materializations.push(unbound);
328+
plan.schemas.push(schema);
329+
let error = plan.validate().unwrap_err().to_string();
330+
assert!(
331+
error.contains("raw output has no Planner producer"),
332+
"{error}"
333+
);
334+
}
335+
265336
// A raw output's input predicate, pane convention and stored state kind
266337
// come from its installed Planner producer, not from the materialization.
267338
#[test]

‎data_plane/src/storage_engines/types/precomputed_output.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -78,7 +78,7 @@ impl PrecomputedOutput {
7878
///
7979
/// `policy_fp` is the content-addressed handle on the source
8080
/// [`asap_types::PrecomputeMaterialization`]; sinks use it to look up the
81-
/// config via `PolicyRegistry::get(policy_fp)`. Construction sites
81+
/// installed output by it. Construction sites
8282
/// that lack a source config (raw-mode fast-path) pass
8383
/// [`PolicyFingerprint::UNSET`]; sinks then skip the output.
8484
pub fn new(

‎data_plane/src/tests/accumulator_fixture.rs‎

Lines changed: 10 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -41,20 +41,15 @@ fn topk_weight(update: &SummaryUpdate) -> TopkWeight {
4141
// Factory function
4242
// ---------------------------------------------------------------------------
4343

44-
/// Read the KLL `k` out of `SketchParams::Kll`. `accumulator_spec()`
45-
/// always builds a `SketchKind` whose `SketchAlgorithm::Kll` is paired with
46-
/// `SketchParams::Kll`, so the
47-
/// other arm is unreachable from a `spec` this module builds itself.
44+
/// Read the KLL `k` out of `SketchParams::Kll`; a valid `SketchKind`
45+
/// pairs `SketchAlgorithm::Kll` with no other params.
4846
#[cfg(test)]
4947
fn kll_k(params: &SketchParams) -> u16 {
5048
match params {
51-
// Lossless: `accumulator_spec()` only ever stores a value that
52-
// already fit in `u16` (via `kll_k_param`'s own `u16::try_from`
53-
// fallback) widened to `u32`.
5449
SketchParams::Kll { k } => *k as u16,
55-
other => unreachable!(
56-
"accumulator_spec() paired SketchAlgorithm::Kll with non-Kll params: {other:?}"
57-
),
50+
other => {
51+
unreachable!("SketchKind paired SketchAlgorithm::Kll with non-Kll params: {other:?}")
52+
}
5853
}
5954
}
6055

@@ -66,7 +61,7 @@ fn cms_dims(params: &SketchParams) -> (usize, usize) {
6661
(*depth as usize, *width as usize)
6762
}
6863
other => unreachable!(
69-
"accumulator_spec() paired SketchAlgorithm::Cms/CountSketch with unexpected params: {other:?}"
64+
"SketchKind paired SketchAlgorithm::Cms/CountSketch with unexpected params: {other:?}"
7065
),
7166
}
7267
}
@@ -86,7 +81,7 @@ fn cms_heap_dims(params: &SketchParams) -> (usize, usize, usize) {
8681
heap_size,
8782
} => (*depth as usize, *width as usize, *heap_size as usize),
8883
other => unreachable!(
89-
"accumulator_spec() paired a WithHeap SketchAlgorithm with unexpected params: {other:?}"
84+
"SketchKind paired a WithHeap SketchAlgorithm with unexpected params: {other:?}"
9085
),
9186
}
9287
}
@@ -97,7 +92,7 @@ fn ddsketch_alpha(params: &SketchParams) -> f64 {
9792
match params {
9893
SketchParams::DDSketch { alpha } => *alpha,
9994
other => unreachable!(
100-
"accumulator_spec() paired SketchAlgorithm::DDSketch with non-DDSketch params: {other:?}"
95+
"SketchKind paired SketchAlgorithm::DDSketch with non-DDSketch params: {other:?}"
10196
),
10297
}
10398
}
@@ -187,9 +182,8 @@ pub fn create_fixture_accumulator(
187182
// Heap-bearing top-k variant (raw-input ingest path): route to the
188183
// real `CmsHeapAccumulatorUpdater` so the per-policy top-k heap is
189184
// BUILT (heap-less CMS could not answer `topk(...)` — recall 0).
190-
// Keyed by the configured group-by `aggregated_labels` (e.g. `host`),
191-
// ranked by Σ value per key by default (`weight_mode: value`), or Σ
192-
// count for genuine frequency-top-k (`weight_mode: count`). The OTLP
185+
// Keyed by the update's item, ranked by Σ value per key, or Σ count
186+
// when the update weight is the constant 1. The OTLP
193187
// modified-sketch path builds the heap agent-side and uses
194188
// `SketchEnvelope` ingest, not this raw arm.
195189
(SummaryFamilyType::Sketch(kind, _), _)

0 commit comments

Comments
 (0)