Repository navigation
feat(ir): derive summary coverage as definition + selection - #646
Merged
Merged
Conversation
This was referenced Oct 6, 2026
zzylol
force-pushed
the
stack/528-02b-merge-structure
branch
from
October 6, 2026 20:45
52b4003 to
8f58489
Compare
zzylol
force-pushed
the
stack/528-02c-coverage-derivation
branch
from
October 6, 2026 20:48
996294c to
764088d
Compare
zzylol
marked this pull request as ready for review
October 6, 2026 20:48
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>
Selvomega
requested changes
Oct 6, 2026
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
force-pushed
the
stack/528-02c-coverage-derivation
branch
from
October 6, 2026 21:49
b727827 to
34c43f0
Compare
Selvomega
requested changes
Oct 7, 2026
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
force-pushed
the
stack/528-02b-merge-structure
branch
from
October 7, 2026 14:55
a5eb728 to
7ce0660
Compare
zzylol
force-pushed
the
stack/528-02c-coverage-derivation
branch
from
October 7, 2026 15:02
34c43f0 to
fea4cc0
Compare
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
force-pushed
the
stack/528-02c-coverage-derivation
branch
from
October 7, 2026 23:07
3721101 to
ebd2a93
Compare
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
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
This was referenced Oct 8, 2026
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>
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
force-pushed
the
stack/528-02c-coverage-derivation
branch
from
October 9, 2026 15:58
ebd2a93 to
1bbd2a0
Compare
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
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
This was referenced Oct 10, 2026
Selvomega
requested changes
Oct 10, 2026
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
Selvomega
approved these changes
Oct 11, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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
SummaryMergestructurally valid, but structure cannot tell a KLL overlatencyfrom one oversize, 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.
After this PR: nothing is declared. Both states derive
selection {region ∈ {us}}over the samedefinition, so the merge fails withPossibleOverlap.What
Coverage = definition (what the state computes) + selection (which output rows of that computation it took):
Filter, a rangeTimeRange, aTimeShiftwithout@, and direct-columnProjectitems up to theSummaryAgg(includingSummaryAgg.filterandScan.predicates), and it is a value set (=,!=,IN,NOT IN,ORof equalities) or an interval (<,<=,>,>=) on one column. Columns are identified by(table, name)as theSummaryAggreads them, soshipping.regionandbilling.regionstay distinct, and aProjectrenames (region AS rgivesr ∈ {us}). A column whose(table, name)is not unique in the agg child (two items aliasedk) is not lifted, and value sets compare literals by type, so1and1.0are never proven different. One rangeTimeRange(w)overTimeShift(s)gives the relative window(−(s+w), −s]; PromQL ranges are left-open.SummaryAggwith 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.timingandguarantee, 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 throughAggregate/window/ratepartition columns, and SQL timestamp intervals (the IR has no timestamp literal yet).Key code interfaces
summary_coverage.rs,node.rsOperatorNode::newandvalidate_structurereject an invalidSummaryMerge; the derived coverage is cached on the node, so it is not derived again.with_coverage,requires_coverage,CoverageError::Missing, and the coverage fields of CSE keys andFlatNode.Tests
crates/types/tests/summary_coverage.rscovers 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 byvalidate_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_examplereproduces the SQL worked example of #573 §4.2.2 (renamingProject,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 aSummaryAgg. Workspace tests,cargo fmtand workspace/all-targets Clippy with warnings denied pass.Base:
main· Next: #539 · Tracker: #528 · Design: #573🤖 Generated with Claude Code