Skip to content

feat(ir): derive summary coverage as definition + selection - #646

Merged
zzylol merged 7 commits into
mainfrom
stack/528-02c-coverage-derivation
Oct 11, 2026
Merged

zzylol merged 7 commits into
mainfrom
stack/528-02c-coverage-derivation

Conversation

@zzylol

@zzylol zzylol commented Oct 6, 2026 •

Copy link
Copy Markdown
Contributor

Closes #570.

Why

A summary state's schema says what kind of state it is (KLL, k=200, by job), not what it summarizes. #560 (merged) makes SummaryMerge structurally valid, but structure cannot tell a KLL over latency from one over size, nor show that two inputs share no row. This PR derives what each summary state covers from its sub-DAG and uses it to decide whether a merge is valid. The design is in #573 (§4.2), based on Goldstein & Larson's view matching (SIGMOD 2001).

Before this PR (main + #560): coverage was declared by whoever built the node and never checked against the sub-DAG.

A = KLL(latency) over Filter(region = 'us', Scan t)   declared {region: eu}   ← wrong
B = KLL(latency) over Filter(region = 'us', Scan t)   declared {region: us}
SummaryMerge(A, B) → accepted; every US row counted twice

After this PR: nothing is declared. Both states derive selection {region ∈ {us}} over the same definition, so the merge fails with PossibleOverlap.

What

Coverage = definition (what the state computes) + selection (which output rows of that computation it took):

KLL(latency) by[job] over Filter(region = 'us' AND latency < 100, Scan t)
  definition: KLL(latency) by[job] over Scan t
  selection:  [{ region ∈ {us}, latency ∈ (−∞, 100) }]

KLL(value) over TimeRange(1m, range) over TimeShift(2m) over Scan m{job="api"}
  definition: KLL(value) over Scan m
  selection:  [{ job ∈ {api}, relative time (−3m, −2m] }]
  • Selection: a predicate conjunct is lifted when it passes Filter, a range TimeRange, a TimeShift without @, and direct-column Project items up to the SummaryAgg (including SummaryAgg.filter and Scan.predicates), and it is a value set (=, !=, IN, NOT IN, OR of equalities) or an interval (<, <=, >, >=) on one column. Columns are identified by (table, name) as the SummaryAgg reads them, so shipping.region and billing.region stay distinct, and a Project renames (region AS r gives r ∈ {us}). A column whose (table, name) is not unique in the agg child (two items aliased k) is not lifted, and value sets compare literals by type, so 1 and 1.0 are never proven different. One range TimeRange(w) over TimeShift(s) gives the relative window (−(s+w), −s]; PromQL ranges are left-open.
  • Definition: the SummaryAgg with what was lifted removed. Everything else (arithmetic, regex, rate, instant selectors, …) stays in it as a residual, so states that differ there do not merge.
  • Merge: valid only when all inputs have structurally equal definitions (ignoring timing and guarantee, so ingestion-time and query-time panes can merge) and pairwise disjoint selections. Its coverage is the shared definition and the union of the selections; boxes that differ in one dimension join when the union is again one constraint (touching time windows, touching value ranges, value sets of one column), as in the paper's one range per column; gaps stay separate boxes. Merges nest.

Not in this PR (each follows when a caller needs it; see #573 §4.2.2 and §5.6): SummaryMerge { group_by } (rollup), family-specific overlap rules (HLL may overlap; #592), hash-partition constraints, lifting through Aggregate/window/rate partition columns, and SQL timestamp intervals (the IR has no timestamp literal yet).

Key code interfaces

summary_coverage.rs, node.rs

impl OperatorNode {
    /// `Some` for a SummaryAgg and a valid SummaryMerge; derived on first use.
    pub fn coverage(&self) -> Option<&SummaryCoverage>;
}

impl SummaryCoverage {
    /// The single entry point for every summary node.
    pub fn derive(node: &OperatorNode) -> Result<Self, CoverageError>;
}

pub struct SummaryCoverage {
    pub definition: Rc<OperatorNode>,
    pub selection: Vec<SelectionBox>,
}

pub struct SelectionBox {
    pub columns: BTreeMap<ColumnIdentity, Constraint>,
    pub relative_time: Option<(Bound<i64>, Bound<i64>)>,
}

pub enum Constraint {
    In(Vec<ScalarValue>),
    NotIn(Vec<ScalarValue>),
    Interval { lower: Bound<ScalarValue>, upper: Bound<ScalarValue> },
}

pub enum CoverageError { NotSummary, EmptyMerge, DefinitionMismatch, PossibleOverlap }
  • OperatorNode::new and validate_structure reject an invalid SummaryMerge; the derived coverage is cached on the node, so it is not derived again.
  • The node's coverage field is now a private cache: ignored by equality, skipped by serde, emptied on clone.
  • Removed: with_coverage, requires_coverage, CoverageError::Missing, and the coverage fields of CSE keys and FlatNode.

Tests

crates/types/tests/summary_coverage.rs covers each rule: filters and scan predicates lifting, population and value-range merges, overlap rejection, different inputs and different scan schemas, qualified columns, time panes (join, gap, overlap), touching value ranges joining, residuals, instant selectors, nested merges, timing, a forged merge caught by validate_structure, and three regressions from an independent review (ambiguous column names, literals of different types, partly lifted scan predicates), each confirmed failing before the fix. design_doc_worked_example reproduces the SQL worked example of #573 §4.2.2 (renaming Project, Scan.predicates, residual on an expression, SummaryAgg.filter). Tests that start from SQL/PromQL query strings follow in #542, the first PR where the planner lowers a query into a SummaryAgg. Workspace tests, cargo fmt and workspace/all-targets Clippy with warnings denied pass.


Base: main · Next: #539 · Tracker: #528 · Design: #573

🤖 Generated with Claude Code

@zzylol
zzylol force-pushed the stack/528-02b-merge-structure branch from 52b4003 to 8f58489 Compare October 6, 2026 20:45
@zzylol
zzylol force-pushed the stack/528-02c-coverage-derivation branch from 996294c to 764088d Compare October 6, 2026 20:48
@zzylol
zzylol marked this pull request as ready for review October 6, 2026 20:48
@zzylol
zzylol requested a review from Selvomega October 6, 2026 20:49
zzylol added a commit that referenced this pull request Oct 6, 2026
…low multi-source summaries

Review of #560/#646 found four problems:

1. Population names came from each node's schema, which `with_schema` may
   rename. A scan whose `tier` column is named "region" made `tier = 'eu'`
   read as `{region: eu}`, so a merge with a real `{region: us}` state was
   accepted and double-counted. Columns are now named from the Scan
   operator's own schema, and a path that renames a field leaves the
   population unknown.
2. For the same reason a merge could mix states of different columns
   (`Named("latency")` reading `size` on a renamed scan). An unknown
   population only merges with the same input, so this is rejected too.
3. `OperatorNode::map_children` dropped a SummaryAgg's coverage, so
   rebuilding a merge (e.g. in canonicalize) failed. A rebuild now keeps the
   declared time bounds and reads source and population again.
4. A SummaryAgg over two sources (a join, an IN subquery over another
   table) could never validate. It now carries no coverage and cannot be
   merged.

Adds summary_coverage_derivation.rs; the four regression tests fail
before this change.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@zzylol zzylol changed the title feat(ir): check summary coverage source and population against the subtree feat(ir): read summary coverage source and population from the subtree Oct 6, 2026
Comment thread crates/types/src/ir/summary_coverage.rs Outdated
Comment thread crates/types/src/ir/summary_coverage.rs Outdated
Comment thread crates/types/src/ir/summary_coverage.rs Outdated
Comment thread crates/types/src/ir/node.rs Outdated
zzylol added a commit that referenced this pull request Oct 6, 2026
…low multi-source summaries

Review of #560/#646 found four problems:

1. Population names came from each node's schema, which `with_schema` may
   rename. A scan whose `tier` column is named "region" made `tier = 'eu'`
   read as `{region: eu}`, so a merge with a real `{region: us}` state was
   accepted and double-counted. Columns are now named from the Scan
   operator's own schema, and a path that renames a field leaves the
   population unknown.
2. For the same reason a merge could mix states of different columns
   (`Named("latency")` reading `size` on a renamed scan). An unknown
   population only merges with the same input, so this is rejected too.
3. `OperatorNode::map_children` dropped a SummaryAgg's coverage, so
   rebuilding a merge (e.g. in canonicalize) failed. A rebuild now keeps the
   declared time bounds and reads source and population again.
4. A SummaryAgg over two sources (a join, an IN subquery over another
   table) could never validate. It now carries no coverage and cannot be
   merged.

Adds summary_coverage_derivation.rs; the four regression tests fail
before this change.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@zzylol
zzylol force-pushed the stack/528-02c-coverage-derivation branch from b727827 to 34c43f0 Compare October 6, 2026 21:49
Comment thread crates/types/src/ir/summary_coverage.rs Outdated
zzylol added a commit that referenced this pull request Oct 7, 2026
…sketches

SummaryMerge no longer derives or checks coverage: of_merge,
UnknownInput and MergeOutputMismatch are removed and summary_coverage.rs
matches main. Coverage for all summary nodes will be derived by one
function in #646. The heap-based sketch restriction is also dropped.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
zzylol added a commit that referenced this pull request Oct 7, 2026
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>
zzylol added a commit that referenced this pull request Oct 7, 2026
SummaryCoverage records which columns a state summarizes (input,
group_by) as well as which rows. Update the SummaryAgg and SummaryMerge
examples to #560: merges compare coverage columns, carry no coverage
until #646, and merged_coverage is gone.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
zzylol added a commit that referenced this pull request Oct 7, 2026
…sketches

SummaryMerge no longer derives or checks coverage: of_merge,
UnknownInput and MergeOutputMismatch are removed and summary_coverage.rs
matches main. Coverage for all summary nodes will be derived by one
function in #646. The heap-based sketch restriction is also dropped.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
zzylol added a commit that referenced this pull request Oct 7, 2026
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>
@zzylol
zzylol force-pushed the stack/528-02b-merge-structure branch from a5eb728 to 7ce0660 Compare October 7, 2026 14:55
@zzylol
zzylol force-pushed the stack/528-02c-coverage-derivation branch from 34c43f0 to fea4cc0 Compare October 7, 2026 15:02
@zzylol zzylol changed the title feat(ir): read summary coverage source and population from the subtree feat(ir): derive summary coverage from the subtree with one SummaryCoverage::derive Oct 7, 2026
@zzylol zzylol changed the title feat(ir): derive summary coverage from the subtree with one SummaryCoverage::derive feat(ir): derive summary coverage from the subtree with SummaryCoverage::derive Oct 7, 2026
zzylol added a commit that referenced this pull request Oct 7, 2026
Mark the summary-operator part as implemented, show the SummaryCoverage
fields, list merge_disjoint/validate as private to derive, and replace
the dropped same-input merge rule and "checked declarations" with what
#646 does: an unreadable population has no coverage and cannot merge, and
coverage is never written by hand.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
zzylol added a commit that referenced this pull request Oct 7, 2026
Remove the coverage columns (input, group_by), check_columns and the
merge's coverage checks. SummaryMerge now only checks structure: at least
one input, every input is State with one state column and an identical
schema. Whether a structurally valid merge is semantically valid (same
computation, disjoint selections) is decided by summary coverage in #646,
following the design in #573.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01W7qG9aFyPij5uWsyAJCxDW
@zzylol
zzylol force-pushed the stack/528-02c-coverage-derivation branch from 3721101 to ebd2a93 Compare October 7, 2026 23:07
zzylol added a commit that referenced this pull request Oct 7, 2026
…e interface in #646

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01W7qG9aFyPij5uWsyAJCxDW
@zzylol zzylol changed the title feat(ir): derive summary coverage from the subtree with SummaryCoverage::derive feat(ir): derive summary coverage as definition + selection Oct 7, 2026
zzylol added a commit that referenced this pull request Oct 7, 2026
…export

Coverage is derived from the node (#646), so timed copies no longer carry
it and PhysicalASAPDAGNode drops its coverage field.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01W7qG9aFyPij5uWsyAJCxDW
zzylol added a commit that referenced this pull request Oct 7, 2026
Planned summary states no longer declare coverage: it is derived from the
node (#646). Node copies use clone + field updates (the coverage cache is
private), physical DAG fixtures drop the removed coverage field, and the
design example uses with_new_children (#648).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01W7qG9aFyPij5uWsyAJCxDW
@zzylol
zzylol requested a review from Selvomega October 8, 2026 13:41
zzylol added a commit that referenced this pull request Oct 9, 2026
* feat(ir): define compatible logical summary merges

* feat(ir): require disjoint coverage for summary merges

* refactor(ir): use SummaryCoverage names in summary merges

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* feat(ir): check merge update/reduction on producers; test documented coverage examples

SummaryCoverage no longer repeats input/reduction, so SummaryMerge compares
them through OperatorNode::summary_update. summary_coverage_examples.rs builds
each example in docs/develop_docs/summary-coverage.md as a SummaryAgg ->
SummaryMerge plan.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* test(ir): point coverage examples at the design document

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* test(ir): point coverage examples at the ASAP primitive schema design doc

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* feat(ir): reject summary merges of heap-based sketches

A merged top-k heap can miss an item that is heavy in only one input, so
CmsWithHeap, CountSketchWithHeap and UnivMon states do not merge. Also
correct the merge_disjoint doc: SummaryMerge checks schema, update,
reduction and heap families, not accuracy.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* refactor(ir): compute merge coverage with SummaryCoverage::of_merge

`ASAPOp::merged_coverage` only applied to SummaryMerge and returned an error
for every other operator. Replace it with `SummaryCoverage::of_merge`, which
takes the merge's inputs; the callers already have them. Also explain what
`OperatorNode::summary_update` returns and why merging compares it.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* refactor(ir): leave summary merge coverage to #646; allow heap-based sketches

SummaryMerge no longer derives or checks coverage: of_merge,
UnknownInput and MergeOutputMismatch are removed and summary_coverage.rs
matches main. Coverage for all summary nodes will be derived by one
function in #646. The heap-based sketch restriction is also dropped.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* refactor(ir): rename summary_update to summary_input_data

It reads the producing SummaryAgg's update expression and reduction; it
does not update the summary.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* 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>

* docs(ir): attach the merge_disjoint doc comment to merge_disjoint

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* refactor(ir): keep SummaryMerge structural; leave coverage to #646

Remove the coverage columns (input, group_by), check_columns and the
merge's coverage checks. SummaryMerge now only checks structure: at least
one input, every input is State with one state column and an identical
schema. Whether a structurally valid merge is semantically valid (same
computation, disjoint selections) is decided by summary coverage in #646,
following the design in #573.

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>
Base automatically changed from stack/528-02b-merge-structure to main October 9, 2026 15:49
zzylol and others added 2 commits October 9, 2026 15:50
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
…ections

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
@zzylol
zzylol force-pushed the stack/528-02c-coverage-derivation branch from ebd2a93 to 1bbd2a0 Compare October 9, 2026 15:58
zzylol added a commit that referenced this pull request Oct 9, 2026
Column identity follows the SummaryAgg's input schema (a Project renames
and drops the table); mark §6.1/§6.6 as #646 and per-family overlap as #592;
panes come from window composition (Pass 2); add IRate; three selection
shapes; current SummaryEstimate top-k output; time row of the definition
table; drop unchecked paper section numbers.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01W7qG9aFyPij5uWsyAJCxDW
zzylol and others added 2 commits October 9, 2026 21:23
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
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
zzylol added a commit that referenced this pull request Oct 9, 2026
Merge coverage is cached at construction (#646); full per-type overlap
table from #592; k mismatch is a schema rejection; #579 top-k shape;
tighter cost bounds; walk-order caveat for combining conditions; Project
qualifier; absolute time marked later; Example B scope; one-walk wording;
define by[job] and closed schema; link Pass 2; window notation.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01W7qG9aFyPij5uWsyAJCxDW
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
zzylol added a commit that referenced this pull request Oct 10, 2026
…s them

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01W7qG9aFyPij5uWsyAJCxDW
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
Comment thread crates/types/src/ir/node.rs Outdated
Comment thread crates/types/src/ir/summary_coverage.rs
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
@zzylol
zzylol merged commit 66a9952 into main Oct 11, 2026
3 checks passed
@zzylol
zzylol deleted the stack/528-02c-coverage-derivation branch October 11, 2026 16:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Check declared summary coverage population against subtree filters

2 participants