Skip to content

Commit 7cd4ee0

Browse files
committed
feat(ir): require disjoint coverage for summary merges
1 parent 10da5e4 commit 7cd4ee0

3 files changed

Lines changed: 92 additions & 4 deletions

File tree

‎crates/types/src/ir/asap.rs‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -207,6 +207,29 @@ impl ASAPOp {
207207
}
208208
}
209209

210+
/// Derive joint observation coverage; unknown or overlapping inputs fail closed.
211+
pub fn merged_coverage(
212+
&self,
213+
) -> Result<super::summary_coverage::SummaryCoverage, SchemaDerivationError> {
214+
let ASAPOp::SummaryMerge { children } = self else {
215+
return Err(SchemaDerivationError::InvalidScalarSignature(
216+
"coverage merge requires SummaryMerge".into(),
217+
));
218+
};
219+
let inputs = children
220+
.iter()
221+
.map(|child| {
222+
child.summary_coverage.clone().ok_or_else(|| {
223+
SchemaDerivationError::InvalidScalarSignature(
224+
"summary merge requires known observation coverage".into(),
225+
)
226+
})
227+
})
228+
.collect::<Result<Vec<_>, _>>()?;
229+
super::summary_coverage::SummaryCoverage::merge_disjoint(&inputs)
230+
.map_err(|error| SchemaDerivationError::InvalidScalarSignature(error.to_string()))
231+
}
232+
210233
/// Output schema derived from the operator and its children. Summary
211234
/// planning may retain a more specific schema (evaluation column naming)
212235
/// through [`OperatorNode::with_schema`]; all structural metadata must
@@ -477,6 +500,7 @@ impl ASAPOp {
477500
));
478501
}
479502
}
503+
self.merged_coverage()?;
480504
Ok(())
481505
}
482506
SummaryEstimate {

‎crates/types/src/ir/node.rs‎

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -105,7 +105,11 @@ impl OperatorNode {
105105
/// ASAP operator, ...).
106106
pub fn new(operator: Operator) -> Result<Self, SchemaDerivationError> {
107107
let schema = operator.output_schema()?;
108-
Ok(Self::with_schema(operator, schema))
108+
let mut node = Self::with_schema(operator, schema);
109+
if let Some(op @ ASAPOp::SummaryMerge { .. }) = node.asap() {
110+
node.summary_coverage = Some(op.merged_coverage()?);
111+
}
112+
Ok(node)
109113
}
110114

111115
/// Build a node with caller-supplied output names and qualifiers. For
@@ -321,6 +325,13 @@ impl OperatorNode {
321325
.clone()
322326
.with_summary_coverage(coverage.clone())?;
323327
}
328+
if let Some(op @ ASAPOp::SummaryMerge { .. }) = node.asap() {
329+
if node.summary_coverage.as_ref() != Some(&op.merged_coverage()?) {
330+
return Err(SchemaDerivationError::InvalidScalarSignature(
331+
"retained merge coverage disagrees with input union".into(),
332+
));
333+
}
334+
}
324335
node.operator.validate_inputs()?;
325336
if node.result_kind != node.operator.output_kind() {
326337
return Err(SchemaDerivationError::InvalidScalarSignature(

‎crates/types/tests/summary_merge_structure.rs‎

Lines changed: 56 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@ fn state(k: u32) -> Rc<OperatorNode> {
1515
schema: Schema::new(vec![Field::plain("value", DataType::Float64, false)]),
1616
}))
1717
.unwrap();
18-
OperatorNode::new_shared(Operator::ASAP(ASAPOp::SummaryAgg {
18+
let summary = OperatorNode::new(Operator::ASAP(ASAPOp::SummaryAgg {
1919
child: scan,
2020
family: FieldDataType::Sketch(
2121
SketchKind::new(SketchAlgorithm::Kll, SketchParams::Kll { k }),
@@ -26,17 +26,35 @@ fn state(k: u32) -> Rc<OperatorNode> {
2626
grouping: GroupingStrategy::default(),
2727
filter: None,
2828
}))
29-
.unwrap()
29+
.unwrap();
30+
std::rc::Rc::new(
31+
summary
32+
.with_summary_coverage(asap_types::ir::summary_coverage::SummaryCoverage {
33+
source: "latencies:timestamp".into(),
34+
revision: "fixture-1".into(),
35+
input: SummaryUpdate::column(ColumnRef::SampleValue),
36+
grouping: Reduction::by(vec![]),
37+
multiplicity:
38+
asap_types::ir::summary_coverage::ObservationMultiplicity::OncePerObservation,
39+
regions: vec![asap_types::ir::summary_coverage::CoverageRegion {
40+
start_ms: 0,
41+
end_ms: 1,
42+
population: Default::default(),
43+
}],
44+
})
45+
.unwrap(),
46+
)
3047
}
3148
/// Two KLL panes compose into one typed logical state without timing assignment.
3249
#[test]
3350
fn compatible_panes_merge_structurally() {
3451
let root = OperatorNode::new_shared(Operator::ASAP(ASAPOp::SummaryMerge {
35-
children: vec![state(200), state(200)],
52+
children: vec![state(200), shifted_state(200, 1, 2)],
3653
}))
3754
.unwrap();
3855
root.validate_structure().unwrap();
3956
assert_eq!(root.schema.fields.len(), 1);
57+
assert_eq!(root.summary_coverage.as_ref().unwrap().regions[0].end_ms, 2);
4058
}
4159
/// An empty merge, raw rows and differently sized state cannot masquerade as compatible panes.
4260
#[test]
@@ -51,3 +69,38 @@ fn incompatible_merge_inputs_fail() {
5169
);
5270
}
5371
}
72+
73+
fn shifted_state(k: u32, start: i64, end: i64) -> Rc<OperatorNode> {
74+
let mut node = (*state(k)).clone();
75+
let region = &mut node.summary_coverage.as_mut().unwrap().regions[0];
76+
region.start_ms = start;
77+
region.end_ms = end;
78+
Rc::new(node)
79+
}
80+
/// Schema equality cannot authorize overlapping or unknown observation coverage.
81+
#[test]
82+
fn unsafe_coverage_merge_is_rejected() {
83+
let mut unknown = (*state(200)).clone();
84+
unknown.summary_coverage = None;
85+
for children in [
86+
vec![state(200), state(200)],
87+
vec![state(200), Rc::new(unknown)],
88+
] {
89+
assert!(
90+
OperatorNode::new_shared(Operator::ASAP(ASAPOp::SummaryMerge { children })).is_err()
91+
);
92+
}
93+
}
94+
95+
/// Gapped time coverage remains disconnected, and forged output metadata is rejected.
96+
#[test]
97+
fn merge_derives_coverage_and_validates_retained_metadata() {
98+
let root = OperatorNode::new_shared(Operator::ASAP(ASAPOp::SummaryMerge {
99+
children: vec![state(200), shifted_state(200, 2, 3)],
100+
}))
101+
.unwrap();
102+
assert_eq!(root.summary_coverage.as_ref().unwrap().regions.len(), 2);
103+
let mut forged = (*root).clone();
104+
forged.summary_coverage.as_mut().unwrap().regions[0].end_ms = 2;
105+
assert!(Rc::new(forged).validate_structure().is_err());
106+
}

0 commit comments

Comments
 (0)