Skip to content

Commit 66a9952

Browse files
zzylolclaude
andauthored
feat(ir): derive summary coverage as definition + selection (#646)
* feat(ir): derive summary coverage as definition + selection Coverage is derived from the node, never declared. For a SummaryAgg, predicate conjuncts that lift through Filter, a range TimeRange, a TimeShift without @ and direct-column Project items, and are a value set or an interval on one column, form the selection; everything else stays in the definition (the SummaryAgg with the selection removed). A TimeRange(w) over TimeShift(s) gives the relative window (-(s+w), -s]. A SummaryMerge is valid only when its inputs have structurally equal definitions (ignoring timing and guarantee) and pairwise disjoint selections; OperatorNode::new and validate_structure enforce it. Its coverage is the shared definition and the union of the selections, joining adjacent windows and value sets. OperatorNode.coverage becomes a private cache behind coverage(): ignored by equality, skipped by serde, emptied on clone. with_coverage, requires_coverage and CoverageError::Missing are removed, as are the coverage fields of CSE keys and FlatNode. Design: docs/design_docs/proposals/asap-primitive-schema.md §4 (#573). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W7qG9aFyPij5uWsyAJCxDW * fix(ir): keep ambiguous columns and untyped-equal literals out of selections From an independent review of the derivation: - a column whose (table, name) is not unique in the agg child's schema cannot be named in a selection, so its conjuncts stay in the definition; - value sets compare literals by typed order, so 1 and 1.0 (or NaN) are never proven different; - a partly lifted Scan predicate is rebuilt with only its residual. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W7qG9aFyPij5uWsyAJCxDW * test(ir): cover the design doc's worked coverage example Point the module docs at #573 §4.2 and note that a Project renames columns in their identity. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W7qG9aFyPij5uWsyAJCxDW * perf(ir): cache a merge's coverage when it is checked OperatorNode::new and validate_structure derived a SummaryMerge's coverage to check it and dropped the result, so coverage() derived it again. Store it in the cache instead. Also correct two comments (Project qualifier, absolute time needs timestamp literals). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W7qG9aFyPij5uWsyAJCxDW * test(ir): worked example follows the SQL query of the design doc The #573 worked example (§4.2.2) is now a SQL query; build the DAG the SQL frontend lowers it to (WHERE folded into Scan.predicates, no time window). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W7qG9aFyPij5uWsyAJCxDW * feat(ir): join touching value ranges in a merged selection Value ranges on one column now join like touching time windows, so every selection dimension follows Goldstein & Larson's one-range-per-column form. The joined range stays explicit: neither input took NULL rows. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W7qG9aFyPij5uWsyAJCxDW * fix(ir): derive coverage on each call and keep renaming nodes Review of #646: - The coverage cache could go stale: `operator` and `schema` are public, so a node edited in place (e.g. through `Rc::make_mut`) kept the coverage of its old value. Drop the cache; `coverage()` derives on each call and returns an owned value. - A `Filter`, `TimeRange` or `TimeShift` whose schema renames columns was removed from the definition once everything was lifted out of it, taking the rename with it, so `tier = 'eu'` named `region` looked disjoint from `region = 'us'`. The walk now stops at such a node, which stays in the definition. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W7qG9aFyPij5uWsyAJCxDW --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
1 parent 0ebeb9d commit 66a9952

8 files changed

Lines changed: 1348 additions & 353 deletions

File tree

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

Lines changed: 3 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -112,7 +112,6 @@ pub fn structural_hash(node: &OperatorNode, cache: &mut HashCache) -> u64 {
112112
&node.schema,
113113
&node.guarantee,
114114
node.timing,
115-
&node.coverage,
116115
);
117116
serde_json::to_string(&own)
118117
.unwrap_or_default()
@@ -168,7 +167,6 @@ fn same_node(left: &OperatorNode, right: &OperatorNode, memo: &mut EqMemo) -> bo
168167
&& left.result_kind == right.result_kind
169168
&& left.schema == right.schema
170169
&& left.timing == right.timing
171-
&& left.coverage == right.coverage
172170
&& same_value(&left.guarantee, &right.guarantee)
173171
&& same_value(&own_fields(left), &own_fields(right))
174172
}
@@ -250,14 +248,9 @@ fn intern_bottom_up(
250248
let operator = node
251249
.operator
252250
.map_children(|child| intern_bottom_up(table, visited, child));
253-
let rebuilt = OperatorNode {
254-
operator,
255-
result_kind: node.result_kind,
256-
schema: node.schema.clone(),
257-
guarantee: node.guarantee.clone(),
258-
timing: node.timing,
259-
coverage: node.coverage.clone(),
260-
};
251+
let rebuilt = OperatorNode::with_schema(operator, node.schema.clone())
252+
.with_guarantee(node.guarantee.clone())
253+
.with_timing(node.timing);
261254
let interned = table.intern(rebuilt);
262255
visited.insert(Rc::as_ptr(node), (Rc::clone(node), Rc::clone(&interned)));
263256
interned

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

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,6 @@ use serde::{Deserialize, Serialize};
1515

1616
use super::node::{Operator, OperatorNode, OperatorResultKind};
1717
use super::query::QueryRoot;
18-
use super::summary_coverage::SummaryCoverage;
1918
use crate::post_asap::execution_data_state::ExecutionTiming;
2019
use crate::post_asap::guarantee::ResultGuarantee;
2120
use crate::pre_asap::schema::Schema;
@@ -31,8 +30,6 @@ pub struct FlatNode {
3130
pub schema: Schema,
3231
pub guarantee: Option<ResultGuarantee>,
3332
pub timing: Option<ExecutionTiming>,
34-
#[serde(default)]
35-
pub coverage: Option<SummaryCoverage>,
3633
}
3734

3835
#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)]
@@ -80,7 +77,6 @@ fn visit(
8077
schema: node.schema.clone(),
8178
guarantee: node.guarantee.clone(),
8279
timing: node.timing,
83-
coverage: node.coverage.clone(),
8480
});
8581
originals.push(Rc::clone(node));
8682
ids.insert(Rc::as_ptr(node), id);

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

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

1111
use super::asap::ASAPOp;
1212
use super::non_asap::NonASAPOp;
13-
use super::summary_coverage::{CoverageError, SummaryCoverage};
13+
use super::summary_coverage::SummaryCoverage;
1414
use crate::ir::SchemaDerivationError;
1515
use crate::post_asap::execution_data_state::ExecutionTiming;
1616
use crate::post_asap::guarantee::ResultGuarantee;
@@ -100,8 +100,6 @@ pub struct OperatorNode {
100100
pub schema: Schema,
101101
pub guarantee: Option<ResultGuarantee>,
102102
pub timing: Option<ExecutionTiming>,
103-
#[serde(default)]
104-
pub coverage: Option<SummaryCoverage>,
105103
}
106104

107105
impl OperatorNode {
@@ -110,7 +108,9 @@ impl OperatorNode {
110108
/// ASAP operator, ...).
111109
pub fn new(operator: Operator) -> Result<Self, SchemaDerivationError> {
112110
let schema = operator.output_schema()?;
113-
Ok(Self::with_schema(operator, schema))
111+
let node = Self::with_schema(operator, schema);
112+
node.check_merge()?;
113+
Ok(node)
114114
}
115115

116116
/// Build a node with caller-supplied output names and qualifiers. For
@@ -124,7 +124,6 @@ impl OperatorNode {
124124
schema,
125125
guarantee: None,
126126
timing: None,
127-
coverage: None,
128127
}
129128
}
130129

@@ -145,23 +144,20 @@ impl OperatorNode {
145144
self
146145
}
147146

148-
/// Attach caller-established coverage. Required on summary nodes; see
149-
/// [`Self::requires_coverage`].
150-
pub fn with_coverage(
151-
mut self,
152-
coverage: SummaryCoverage,
153-
) -> Result<Self, SchemaDerivationError> {
154-
coverage.validate()?;
155-
if self.result_kind != OperatorResultKind::State {
156-
return Err(CoverageError::NotState.into());
157-
}
158-
self.coverage = Some(coverage);
159-
Ok(self)
147+
/// What this summary state covers; `None` for a node that is not a
148+
/// `SummaryAgg` or a valid `SummaryMerge`. Derived on each call, since
149+
/// the public fields can be edited in place.
150+
pub fn coverage(&self) -> Option<SummaryCoverage> {
151+
SummaryCoverage::derive(self).ok()
160152
}
161153

162-
/// Summary nodes whose state can be composed must declare coverage.
163-
pub fn requires_coverage(&self) -> bool {
164-
matches!(self.asap(), Some(ASAPOp::SummaryAgg { .. }))
154+
/// A `SummaryMerge` is valid only over inputs with the same definition
155+
/// and disjoint selections.
156+
fn check_merge(&self) -> Result<(), SchemaDerivationError> {
157+
if matches!(self.asap(), Some(ASAPOp::SummaryMerge { .. })) {
158+
SummaryCoverage::derive(self)?;
159+
}
160+
Ok(())
165161
}
166162

167163
pub fn non_asap(&self) -> Option<&NonASAPOp> {
@@ -315,14 +311,8 @@ impl OperatorNode {
315311
"invalid time or identity column in schema".into(),
316312
));
317313
}
318-
match &node.coverage {
319-
Some(coverage) => {
320-
(*node.as_ref()).clone().with_coverage(coverage.clone())?;
321-
}
322-
None if node.requires_coverage() => return Err(CoverageError::Missing.into()),
323-
None => {}
324-
}
325314
node.operator.validate_inputs()?;
315+
node.check_merge()?;
326316
if node.result_kind != node.operator.output_kind() {
327317
return Err(SchemaDerivationError::InvalidScalarSignature(
328318
"retained result kind disagrees with operation".into(),

0 commit comments

Comments
 (0)