Skip to content

Commit c8d6564

Browse files
zzylolclaude
andcommitted
feat(ir): record summarized columns in SummaryCoverage
SummaryCoverage now records which columns a state summarizes as well as which rows: `input` (the SummaryAgg update expression) and `group_by` (its reduction). with_coverage rejects a SummaryAgg declaration whose columns differ from the node's own (ColumnMismatch), and SummaryMerge requires coverage on every input (UnknownInput) with identical columns. merge_disjoint checks columns too. summary_input_data is removed. A nested SummaryMerge carries no coverage until #646, so it is rejected. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
1 parent cf3fc26 commit c8d6564

7 files changed

Lines changed: 125 additions & 52 deletions

File tree

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

Lines changed: 8 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ use std::rc::Rc;
66
use serde::{Deserialize, Serialize};
77

88
use super::node::{OperatorNode, OperatorResultKind};
9+
use super::summary_coverage::CoverageError;
910
use crate::ir::operator_properties::Reduction;
1011
use crate::ir::SchemaDerivationError;
1112
use crate::post_asap::maintained_population::{MaintainedPopulation, PopulationStatistic};
@@ -487,17 +488,13 @@ impl ASAPOp {
487488
));
488489
}
489490
}
490-
// Equal schemas do not prove the states summarize the same
491-
// expression with the same grouping; the producers do.
492-
let input_data = first.summary_input_data();
493-
if input_data.is_none()
494-
|| children
495-
.iter()
496-
.any(|c| c.summary_input_data() != input_data)
497-
{
498-
return Err(SchemaDerivationError::InvalidScalarSignature(
499-
"summary merge inputs must share update expression and reduction".into(),
500-
));
491+
// Equal schemas cannot tell a KLL over `latency` from one over
492+
// `size`; the columns in each input's coverage can. A nested
493+
// `SummaryMerge` carries no coverage yet (#646), so it is rejected.
494+
let columns = first.coverage.as_ref().ok_or(CoverageError::UnknownInput)?;
495+
for child in children {
496+
let coverage = child.coverage.as_ref().ok_or(CoverageError::UnknownInput)?;
497+
coverage.check_columns(columns)?;
501498
}
502499
Ok(())
503500
}

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

Lines changed: 10 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -10,12 +10,10 @@ use serde::{Deserialize, Serialize};
1010

1111
use super::asap::ASAPOp;
1212
use super::non_asap::NonASAPOp;
13-
use super::operator_properties::Reduction;
1413
use super::summary_coverage::{CoverageError, SummaryCoverage};
1514
use crate::ir::SchemaDerivationError;
1615
use crate::post_asap::execution_data_state::ExecutionTiming;
1716
use crate::post_asap::guarantee::ResultGuarantee;
18-
use crate::post_asap::SummaryUpdate;
1917
use crate::pre_asap::schema::Schema;
2018

2119
/// The output category of an operator, derived from the operation and its
@@ -148,7 +146,8 @@ impl OperatorNode {
148146
}
149147

150148
/// Attach caller-established coverage. Required on summary nodes; see
151-
/// [`Self::requires_coverage`].
149+
/// [`Self::requires_coverage`]. On a `SummaryAgg` the coverage columns
150+
/// must equal the node's `input` and `reduction`.
152151
pub fn with_coverage(
153152
mut self,
154153
coverage: SummaryCoverage,
@@ -157,28 +156,18 @@ impl OperatorNode {
157156
if self.result_kind != OperatorResultKind::State {
158157
return Err(CoverageError::NotState.into());
159158
}
159+
if let Some(ASAPOp::SummaryAgg {
160+
input, reduction, ..
161+
}) = self.asap()
162+
{
163+
if &coverage.input != input || &coverage.group_by != reduction {
164+
return Err(CoverageError::ColumnMismatch.into());
165+
}
166+
}
160167
self.coverage = Some(coverage);
161168
Ok(self)
162169
}
163170

164-
/// What a summary state summarizes from each row: the update expression
165-
/// fed into the state (`input`, e.g. the `latency` column) and the
166-
/// grouping (`reduction`, e.g. by `job`) of the `SummaryAgg` that produced
167-
/// it. Which rows were included is `coverage`, not this. For a
168-
/// `SummaryMerge` these are its first input's, which merge validation
169-
/// requires every input to share. `None` for any other node. Equal schemas
170-
/// cannot tell a KLL over `latency` from one over `size`; merging compares
171-
/// this instead.
172-
pub fn summary_input_data(&self) -> Option<(&SummaryUpdate, &Reduction)> {
173-
match self.asap()? {
174-
ASAPOp::SummaryAgg {
175-
input, reduction, ..
176-
} => Some((input, reduction)),
177-
ASAPOp::SummaryMerge { children } => children.first()?.summary_input_data(),
178-
_ => None,
179-
}
180-
}
181-
182171
/// Summary nodes whose state can be composed must declare coverage.
183172
pub fn requires_coverage(&self) -> bool {
184173
matches!(self.asap(), Some(ASAPOp::SummaryAgg { .. }))

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

Lines changed: 29 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,10 @@
1-
//! Joint time/population coverage for summary composition, independent of schema.
1+
//! What a summary state covers, independent of schema: which rows (source and
2+
//! joint time/population regions) and which columns (the update expression fed
3+
//! into the state and the grouping).
24
//! Equality predicates are a deliberately narrow proof vocabulary. Unsupported
35
//! predicates cannot be declared disjoint merely by giving them different names.
6+
use crate::ir::operator_properties::Reduction;
7+
use crate::post_asap::SummaryUpdate;
48
use crate::pre_asap::Source;
59
use serde::{Deserialize, Serialize};
610
use std::collections::BTreeMap;
@@ -13,8 +17,15 @@ pub struct SummaryCoverage {
1317
/// Observation data source, as named by `Scan`: a table or a time series.
1418
/// Region time bounds refer to its time column.
1519
pub source: Source,
16-
/// Union of joint regions; never the Cartesian product of independent bounds.
20+
/// Rows: union of joint regions; never the Cartesian product of
21+
/// independent bounds.
1722
pub regions: Vec<CoverageRegion>,
23+
/// Columns: the update expression fed into the state from each row, equal
24+
/// to the producing `SummaryAgg`'s `input` (e.g. the `latency` column).
25+
pub input: SummaryUpdate,
26+
/// Columns: the grouping, equal to the producing `SummaryAgg`'s
27+
/// `reduction` (e.g. by `job`). Compared by child-schema column index.
28+
pub group_by: Reduction,
1829
}
1930

2031
#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)]
@@ -48,6 +59,10 @@ pub enum CoverageError {
4859
NotState,
4960
#[error("summary node requires coverage")]
5061
Missing,
62+
#[error("summary coverage columns differ from the summary's input or grouping")]
63+
ColumnMismatch,
64+
#[error("summary merge requires coverage on every input")]
65+
UnknownInput,
5166
}
5267

5368
impl SummaryCoverage {
@@ -70,9 +85,17 @@ impl SummaryCoverage {
7085
}
7186

7287
/// Every observation in a region is assumed to contribute once to the state.
73-
/// Compose once-per-observation summaries only when their joint regions are
74-
/// provably disjoint. Update/reduction compatibility, family merge capability
75-
/// and accuracy are checked by `SummaryMerge`, not here.
88+
/// Compose once-per-observation summaries only when they read the same
89+
/// columns and their joint regions are provably disjoint. Family merge
90+
/// capability and accuracy are not checked here.
91+
/// Rejects `other` unless it reads the same columns as `self`.
92+
pub fn check_columns(&self, other: &Self) -> Result<(), CoverageError> {
93+
if self.input != other.input || self.group_by != other.group_by {
94+
return Err(CoverageError::ColumnMismatch);
95+
}
96+
Ok(())
97+
}
98+
7699
pub fn merge_disjoint(inputs: &[Self]) -> Result<Self, CoverageError> {
77100
let first = inputs.first().ok_or(CoverageError::EmptyMerge)?;
78101
let mut merged = first.clone();
@@ -82,6 +105,7 @@ impl SummaryCoverage {
82105
if input.source != first.source {
83106
return Err(CoverageError::SourceMismatch);
84107
}
108+
input.check_columns(first)?;
85109
merged.regions.extend(input.regions.iter().cloned());
86110
}
87111
merged.validate()?;

‎crates/types/tests/schema_rebuilding.rs‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,8 @@ fn coverage() -> SummaryCoverage {
1717
time_ms: None,
1818
population: Default::default(),
1919
}],
20+
input: SummaryUpdate::column(ColumnRef::Named("value".into())),
21+
group_by: Reduction::by(vec![0]),
2022
}
2123
}
2224
/// Rewrites clear coverage; a rewriter must declare it again for summary nodes.

‎crates/types/tests/structure_contract.rs‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,10 @@ fn whole_table() -> asap_types::ir::summary_coverage::SummaryCoverage {
2424
time_ms: None,
2525
population: Default::default(),
2626
}],
27+
input: asap_types::post_asap::SummaryUpdate::column(
28+
asap_types::pre_asap::ColumnRef::Named("x".into()),
29+
),
30+
group_by: asap_types::pre_asap::Reduction::by(vec![]),
2731
}
2832
}
2933
/// Resolved filters cannot hide invalid scalar types or out-of-scope columns.

‎crates/types/tests/summary_coverage.rs‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,8 @@ fn coverage(start: i64, end: i64, population: &[(&str, &str)]) -> SummaryCoverag
2020
.map(|(k, v)| (k.to_string(), v.to_string()))
2121
.collect(),
2222
}],
23+
input: SummaryUpdate::column(ColumnRef::Named("latency".into())),
24+
group_by: Reduction::by(vec![]),
2325
}
2426
}
2527
/// Adjacent panes coalesce; gaps remain disconnected rather than becoming a hull.
@@ -148,3 +150,14 @@ fn regions_without_time_bounds() {
148150
Err(CoverageError::PossibleOverlap)
149151
);
150152
}
153+
154+
/// Disjoint rows do not make coverage of different columns composable.
155+
#[test]
156+
fn different_columns_do_not_compose() {
157+
let mut by_job = coverage(1, 2, &[]);
158+
by_job.group_by = Reduction::by(vec![0]);
159+
assert_eq!(
160+
SummaryCoverage::merge_disjoint(&[coverage(0, 1, &[]), by_job]),
161+
Err(CoverageError::ColumnMismatch)
162+
);
163+
}

‎crates/types/tests/summary_merge_structure.rs‎

Lines changed: 59 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
//! Window composition merges compatible summary states without consuming raw rows.
22
use asap_types::{
33
ir::operator_properties::{Reduction, Source},
4+
ir::summary_coverage::CoverageError,
45
ir::{ASAPOp, NonASAPOp, Operator, OperatorNode, SchemaDerivationError},
56
post_asap::{GroupingStrategy, SketchAlgorithm, SketchKind, SketchParams, SummaryUpdate},
67
pre_asap::{ColumnRef, DataType, Field, FieldDataType, Schema},
@@ -29,7 +30,7 @@ fn state_over(family: FieldDataType, input: SummaryUpdate) -> Rc<OperatorNode> {
2930
let summary = OperatorNode::new(Operator::ASAP(ASAPOp::SummaryAgg {
3031
child: scan,
3132
family,
32-
input,
33+
input: input.clone(),
3334
reduction: Reduction::by(vec![]),
3435
grouping: GroupingStrategy::default(),
3536
filter: None,
@@ -45,6 +46,8 @@ fn state_over(family: FieldDataType, input: SummaryUpdate) -> Rc<OperatorNode> {
4546
time_ms: Some(0..1),
4647
population: Default::default(),
4748
}],
49+
input,
50+
group_by: Reduction::by(vec![]),
4851
})
4952
.unwrap(),
5053
)
@@ -84,25 +87,66 @@ fn shifted(state: &Rc<OperatorNode>, start: i64, end: i64) -> Rc<OperatorNode> {
8487
Rc::new(node)
8588
}
8689

87-
/// Equal schemas do not prove both states summarize the same expression.
90+
fn kll() -> FieldDataType {
91+
FieldDataType::Sketch(
92+
SketchKind::new(SketchAlgorithm::Kll, SketchParams::Kll { k: 200 }),
93+
Default::default(),
94+
)
95+
}
96+
97+
fn merge(children: Vec<Rc<OperatorNode>>) -> Result<Rc<OperatorNode>, SchemaDerivationError> {
98+
OperatorNode::new_shared(Operator::ASAP(ASAPOp::SummaryMerge { children }))
99+
}
100+
101+
/// Equal schemas cannot tell a KLL over one column from a KLL over another;
102+
/// the columns in their coverage can.
88103
#[test]
89-
fn different_update_expressions_do_not_merge() {
90-
let kll = || {
91-
FieldDataType::Sketch(
92-
SketchKind::new(SketchAlgorithm::Kll, SketchParams::Kll { k: 200 }),
93-
Default::default(),
94-
)
95-
};
104+
fn different_coverage_columns_do_not_merge() {
96105
let named = state_over(
97106
kll(),
98107
SummaryUpdate::column(ColumnRef::Named("value".into())),
99108
);
100-
let result = OperatorNode::new_shared(Operator::ASAP(ASAPOp::SummaryMerge {
101-
children: vec![state(200), shifted(&named, 1, 2)],
102-
}));
103109
assert!(matches!(
104-
result,
105-
Err(SchemaDerivationError::InvalidScalarSignature(message))
106-
if message.contains("update expression and reduction")
110+
merge(vec![state(200), shifted(&named, 1, 2)]),
111+
Err(SchemaDerivationError::Coverage(
112+
CoverageError::ColumnMismatch
113+
))
107114
));
108115
}
116+
117+
/// A `SummaryAgg` cannot declare columns other than its own input and grouping.
118+
#[test]
119+
fn declared_columns_must_match_the_summary() {
120+
let mut coverage = state(200).coverage.clone().unwrap();
121+
coverage.input = SummaryUpdate::column(ColumnRef::Named("value".into()));
122+
let fresh = (*state(200)).clone();
123+
assert!(matches!(
124+
fresh.clone().with_coverage(coverage.clone()),
125+
Err(SchemaDerivationError::Coverage(
126+
CoverageError::ColumnMismatch
127+
))
128+
));
129+
let mut forged = fresh;
130+
forged.coverage = Some(coverage);
131+
assert!(matches!(
132+
Rc::new(forged).validate_structure(),
133+
Err(SchemaDerivationError::Coverage(
134+
CoverageError::ColumnMismatch
135+
))
136+
));
137+
}
138+
139+
/// Every merge input needs coverage. A nested merge has none until #646, so
140+
/// it is rejected for now.
141+
#[test]
142+
fn merge_inputs_without_coverage_are_rejected() {
143+
let mut missing = (*state(200)).clone();
144+
missing.coverage = None;
145+
let nested = merge(vec![state(200)]).unwrap();
146+
for input in [Rc::new(missing), nested] {
147+
assert!(matches!(
148+
merge(vec![state(200), input]),
149+
Err(SchemaDerivationError::Coverage(CoverageError::UnknownInput))
150+
));
151+
}
152+
}

0 commit comments

Comments
 (0)