Repository navigation
Conversation
3ab6be7 to
77628c8
Compare
77628c8 to
02a6a1b
Compare
60a9e4f to
cb00197
Compare
cb00197 to
700838b
Compare
f2b7b3f to
3f5d369
Compare
e5e4f67 to
fc8eec9
Compare
3f5d369 to
52b4003
Compare
…ts fields Name the metadata coverage to match #560 and the docs; drop the single-variant multiplicity and deployment-specific revision; rename grouping to reduction to match SummaryAgg; report failures through SchemaDerivationError::Coverage; revert the unrelated PaneCoverageError rename. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
bb3bf69 to
4750e71
Compare
* feat(ir): define joint summary observation coverage * refactor(ir): name represented observations ObservationExtent * refactor(ir): rename observation extent to SummaryCoverage and trim its fields Name the metadata coverage to match #560 and the docs; drop the single-variant multiplicity and deployment-specific revision; rename grouping to reduction to match SummaryAgg; report failures through SchemaDerivationError::Coverage; revert the unrelated PaneCoverageError rename. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * docs(ir): describe coverage source as any observation data source Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * feat(ir): require coverage on summary nodes; allow regions without time bounds validate_structure rejects a SummaryAgg without coverage (CoverageError::Missing). CoverageRegion time bounds become optional so tabular sources without a time column can declare coverage. Population stays trusted; #570 tracks checking it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * docs: explain the summary coverage problem with examples Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * refactor(ir): keep only time and population in SummaryCoverage Given required coverage on summary nodes, input and reduction duplicated the producing SummaryAgg fields; drop them along with ProducerMismatch. Type source as Source, matching Scan. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * refactor(cost): rename SourceCoverage to ScanSelection SourceCoverage names the rows a physical scan reads for cost comparison, not which observations a summary state holds; rename it so it is not confused with SummaryCoverage. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * docs: turn summary coverage into a design document Move it to docs/design_docs/proposals with problem and motivation, requirements, design, alternatives and key code interfaces. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * docs: design doc for ASAP primitive schema and summary semantics Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Update asap-primitive-schema.md * Update asap-primitive-schema.md * Update asap-primitive-schema.md * Update asap-primitive-schema.md * docs: add code interfaces and per-operator examples to the ASAP primitive schema design Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * docs: move the ASAP primitive schema design to #573 The design document and the docs it consolidates are reviewed separately on main. This PR keeps code, tests and the ScanSelection rename in docs. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * docs(ir): say coverage time is absolute and population is Utf8 equality Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * feat(ir): carry summary coverage through CSE and flat DAGs Coverage is part of a summary state's identity: two states over different observations are never shared, so CSE hashes and compares it, and a flat node keeps it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
52b4003 to
8f58489
Compare
…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>
| pub fn merged_coverage(&self) -> Result<SummaryCoverage, SchemaDerivationError> { | ||
| let ASAPOp::SummaryMerge { children } = self else { | ||
| return Err(SchemaDerivationError::InvalidScalarSignature( | ||
| "coverage merge requires SummaryMerge".into(), | ||
| )); | ||
| }; | ||
| let inputs = children | ||
| .iter() | ||
| .map(|child| child.coverage.clone().ok_or(CoverageError::UnknownInput)) | ||
| .collect::<Result<Vec<_>, _>>()?; | ||
| Ok(SummaryCoverage::merge_disjoint(&inputs)?) | ||
| } |
There was a problem hiding this comment.
Looks weird. Why merged_coverage, something specific to SummaryMerge node only, will be a method for ASAPOp, a much broader type?
There was a problem hiding this comment.
Agreed, it only makes sense for SummaryMerge and had to return an error for every other operator. I replaced it with SummaryCoverage::of_merge(inputs) in summary_coverage.rs, which takes the merge's inputs directly; the three callers already have them from ASAPOp::SummaryMerge { children }.
There was a problem hiding this comment.
No I still feel this unsolved. Please check my comments in code later
There was a problem hiding this comment.
Addressed in 42bc2f7 and e201d5a. merged_coverage and its replacement of_merge are both gone. ASAPOp has no coverage method, and this PR derives no coverage at all: the merged node carries none. SummaryMerge validation only reads each input's existing coverage, requiring it to be present (UnknownInput) and to have the same columns (ColumnMismatch, see below). Deriving coverage for every summary node, merges included, is one SummaryCoverage::derive in #646.
| match self.asap()? { | ||
| ASAPOp::SummaryAgg { | ||
| input, reduction, .. | ||
| } => Some((input, reduction)), | ||
| ASAPOp::SummaryMerge { children } => children.first()?.summary_update(), | ||
| _ => None, | ||
| } | ||
| } |
There was a problem hiding this comment.
What does this function do?
There was a problem hiding this comment.
It returns what a summary state was built from: the update expression (input) and grouping (reduction) of the SummaryAgg that produced it. For a SummaryMerge it returns its first input's, which merge validation requires every input to share; for any other node it returns None.
SummaryMerge uses it to require that all inputs summarize the same expression with the same grouping. Equal schemas alone don't show that: KLL over latency and KLL over size, both grouped by job, have the same schema. I expanded the doc comment to say this.
There was a problem hiding this comment.
Give me a few minutes to further understand this.
There was a problem hiding this comment.
Figured out with Claude code. I feel the naming is off. I thought this is a function updating the summary
There was a problem hiding this comment.
This function is now removed (e201d5a). What it returned (the producing SummaryAgg's input and reduction) is part of SummaryCoverage instead, as its columns, next to the existing rows:
pub struct SummaryCoverage {
pub source: Source, // rows
pub regions: Vec<CoverageRegion>, // rows: time × population
pub input: SummaryUpdate, // columns: = SummaryAgg.input (e.g. latency)
pub group_by: Reduction, // columns: = SummaryAgg.reduction (e.g. by job)
}with_coverage rejects a SummaryAgg declaration whose columns differ from the node's own (ColumnMismatch), and a merge requires identical columns across inputs, so a KLL over latency and one over size (same schema) cannot merge.
…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>
|
|
||
| /// Coverage of a `SummaryMerge` over `inputs`: the disjoint union of their | ||
| /// coverage. Fails when an input has none or two inputs may overlap. | ||
| pub fn of_merge( |
There was a problem hiding this comment.
This is still bad design. Since in #646 we introduced for_summary function to calculate the coverage, and that function is effectively only working for SummaryAgg nodes.
Taking an union of #646 and this PR on coverage deriving, we can see that we have
of_mergecalculating coverage forSummaryMergefor_summarycalculating coverage forSummaryAgg
This is very bad for the following 2 reasons- Ideally we should only have 1 function deriving coverage for all summary-generating nodes, where we have relatively consistent and unified behavior, now we have 2
- Even for the current 2 in these PRs, neither their name nor their behavior match
I do hate the shitty patches agents are relentlessly making.
I propose here, we intendedly not implementing the coverage deriving functionality in this PR, since this is only to introduce the SummaryMerge node, and SummaryCoverage, by now, doesn't have any automatically inferred version. Then we do that in a unified way in #646, with one func
There was a problem hiding this comment.
Thanks for the review. This is a great idea.
There was a problem hiding this comment.
Done in 42bc2f7: this PR no longer derives or checks coverage. of_merge, UnknownInput and MergeOutputMismatch are removed, summary_coverage.rs matches main, and SummaryMerge only checks structure. #646 will derive coverage for every summary node through a single SummaryCoverage::derive(node, time_range).
There was a problem hiding this comment.
Follow-up in e201d5a: SummaryCoverage now also records the columns a state summarizes (input, group_by), so all merge compatibility beyond the schema goes through coverage. This PR still derives nothing; callers declare coverage, and with_coverage only checks the declared columns against the SummaryAgg's own fields. #646 derives rows and columns for SummaryAgg and SummaryMerge through one SummaryCoverage::derive. The design doc in #573 is updated to match.
| /// requires every input to share. `None` for any other node. Merging | ||
| /// compares it so that all inputs summarize the same expression with the | ||
| /// same grouping. | ||
| pub fn summary_update(&self) -> Option<(&SummaryUpdate, &Reduction)> { |
There was a problem hiding this comment.
Suggest renaming if this function is not used to actually update the summary.
There was a problem hiding this comment.
Renamed to summary_input_data in 4ff8b98. It reads the producing SummaryAgg's update expression (input, e.g. the latency column) and grouping (reduction, e.g. by job); it updates nothing. The doc comment now says this and that which rows were included is coverage, not this.
There was a problem hiding this comment.
Update: summary_input_data is removed in e201d5a; the same information is now SummaryCoverage.input and SummaryCoverage.group_by (see the thread above).
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>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…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>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… doc Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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>
`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>
…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>
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>
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>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
a5eb728 to
7ce0660
Compare
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
Why
Builds on #645 (generic IR, CSE) and #567 (summary coverage), both merged. Extracts structural summary merge support from #555.
Before this PR: constructing
ASAPOp::SummaryMergefails because the operation is reserved.After this PR: two structurally compatible KLL states can form a typed logical merge with no execution-phase assignment. Merge inputs must be nonempty, carry state with exactly one state field, and have identical family/parameter/grouping schemas. Empty, raw and mismatched states fail.
This PR is structural only and does not touch
SummaryCoverage. Equal schemas cannot tell a KLL overlatencyfrom a KLL oversize, and cannot show that the inputs cover disjoint rows. Both are decided by summary coverage, derived for every summary node by oneSummaryCoverage::derivein #646, following the design in #573.Schema/result-kind/state identity and regression tests are included. Runtime execution, materialization and derived timing belong to the later physical scopes. Subtract/delete remain reserved.
Key code interface
SummaryMergeis a logical ASAP operator whose inputs are existing operator nodes producing partial state:It uses the same node construction and validation APIs as other operators:
Here
pane_aandpane_bareRc<OperatorNode>state producers.new_sharedreturnsResult<Rc<OperatorNode>, SchemaDerivationError>and derives the output schema/result kind.validate_structurechecks the complete reachable DAG, including the producers. No timing assignment is required.The key
ASAPOpmethods are:For
SummaryMerge:validate_inputsenforces the compatibility rules below.output_schemavalidates inputs, then clones the first input's complete schema.output_kindisOperatorResultKind::State.produced_statereturns the non-plain field type from the first input. This is a metadata accessor, not a runtime merge or an independent validation step.Grouped merge keeps groups apart:
job='api'andjob='worker'stay separate states. A laterSummaryEstimateis the readout boundary that turns state into a plain value.Requirements for merging two summaries
This PR enforces these structural requirements:
childrenlist is rejected. The API is n-ary; a single compatible state is structurally allowed.result_kind == State; raw relations/vectors are rejected.Plainfield. Identical schemas enforce this on every other input too. Plain grouping fields may accompany it.FieldDataType. KLLk=200andk=300, or KLL and CMS, cannot merge here.time_index,unique_keysandclosedmust match as well. Matching only the state algorithm is insufficient.validate_structurechecks each producer's own contracts.Compatibility is deliberately strict: even differently named but otherwise equivalent schemas need an explicit normalization before this interface accepts them.
These checks establish typed structural compatibility only. They do not prove that the inputs summarize the same computation or cover disjoint rows (#646), that they cover the intended population/window, that a runtime implements the family's merge operation, or that the merged result meets an accuracy requirement. Those belong to #646, the logical composition rule and subsequent physical planning/selection. For example, overlapping frequency panes must not silently double-count observations; matching schemas alone cannot establish that.
When SummaryMerge can be used
During logical planning: a composition rule can construct
SummaryMergewhen it needs to combine compatible partial summary states—for example, several tumbling-window KLL panes answering one larger query window, or compatible partition summaries feeding a coarser computation. The planner must establish the intended input coverage and grouping semantics. The node can be constructed and structurally validated before choosing materialization.During execution: a physical implementation can merge the state contents once its inputs are available and the selected family/runtime supports the operation. Timing follows the materialization choice in #509. For example, a query can merge previously ingested/stored panes, or merge states rebuilt for that query. #560 itself adds no runtime kernel, storage, retention, window generation or execution-phase policy, and does not imply runtime support for every declared state family.
Source:
asap.rs, with regressions incrates/types/tests/summary_merge_structure.rs.Validation: workspace tests,
cargo fmt, and workspace/all-target Clippy with warnings denied all pass.Base: main · Next: #646 · Tracker: #528
Order: #645 (merged) → #567 (merged) → #560 → #646 → #539 → #540 → #541 → #542 → #543.
🤖 Generated with Claude Code