Repository navigation
Conversation
52b4003 to
8f58489
Compare
996294c to
764088d
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>
| /// scanned below it and the population its filters restrict to, over | ||
| /// `time_ms`. `None` when `summary` is not a `SummaryAgg` or does not | ||
| /// read exactly one source. | ||
| pub fn for_summary(summary: &OperatorNode, time_ms: Option<Range<i64>>) -> Option<Self> { |
There was a problem hiding this comment.
The naming of time_ms seems not good. I assume you want conceptually a "range" here?
There was a problem hiding this comment.
yes, I will rename to "time_range"
There was a problem hiding this comment.
Done in fea4cc0: CoverageRegion.time_ms is now time_range, and so is the parameter of derive/with_time_range.
| source: summary.scanned_source()?, | ||
| regions: vec![CoverageRegion { | ||
| time_ms, | ||
| population: summary.derived_population().unwrap_or_default(), |
There was a problem hiding this comment.
Bug here: If derived_population() returns None, it will be converted to the default legal value of population.
The problem is in derived_population() semantic, None means I cannot handle this population so it should not be used. While this treacherous unwrap_or_default() silently convert this into a legal value!
Spotting this bug actually makes me feel better: At least it proves reading code is still somehow useful.
There was a problem hiding this comment.
Fixed in fea4cc0. An unreadable population is now an error, UnprovenPopulation, never the empty (unrestricted) map. Such a SummaryAgg is still valid but has no coverage, so it cannot be merged, and a merge with it reports UnprovenPopulation. The same-input exception for unreadable populations is dropped.
| if summary.scanned_source().as_ref() != Some(&self.source) { | ||
| return Err(CoverageError::SourceMismatch); | ||
| } | ||
| let derived = summary.derived_population().unwrap_or_default(); |
There was a problem hiding this comment.
Same. Check if there is bug here
There was a problem hiding this comment.
Same fix: check_against is removed. validate_structure compares the retained coverage with SummaryCoverage::derive, which fails with UnprovenPopulation instead of defaulting.
| } | ||
| if let (Some(coverage), Some(ASAPOp::SummaryAgg { .. })) = (&self.coverage, rebuilt.asap()) | ||
| { | ||
| let population = rebuilt.derived_population().unwrap_or_default(); |
There was a problem hiding this comment.
Seemingly a bug. Check other comments related
…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>
b727827 to
34c43f0
Compare
| /// scanned below it and the population its filters restrict to, over | ||
| /// `time_ms`. `None` when `summary` is not a `SummaryAgg` or does not | ||
| /// read exactly one source. | ||
| pub fn for_summary(summary: &OperatorNode, time_ms: Option<Range<i64>>) -> Option<Self> { |
There was a problem hiding this comment.
This function is also treacherous and shady. It is called for_summary but practically only works for SummaryAgg nodes. Some problems in PR #560 is related to this.
There was a problem hiding this comment.
Fixed in fea4cc0. for_summary is gone, and so is of_merge from #560. Coverage is now computed by one function for every summary node:
pub fn derive(node: &OperatorNode, time_range: Option<Range<i64>>) -> Result<SummaryCoverage, CoverageError>SummaryAgg → source, population and columns read from its subtree over time_range; SummaryMerge → the disjoint union of its inputs (time_range must be None); anything else → NotSummary. OperatorNode::new, with_time_range (replaces with_coverage), validate_structure and with_new_children all call it; check_against, same_input and the public scanned_source/derived_population are removed.
…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>
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>
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>
…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>
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>
a5eb728 to
7ce0660
Compare
…verage::derive SummaryCoverage::derive(node, time_range) is the only way coverage is computed, for every summary node: - SummaryAgg: the one source scanned below it, the population read from its filters, its input and reduction (grouping keys named from its input schema) as columns, over the caller's time range. - SummaryMerge: the disjoint union of its inputs' coverage, which must read the same source and columns; its time range comes from the inputs. OperatorNode::new derives coverage (a SummaryAgg starts with no time bounds), with_time_range replaces with_coverage and sets a SummaryAgg's time range, validate_structure requires the retained coverage to equal the derived one (DerivedMismatch), and with_new_children derives it again, keeping the time range. A SummaryAgg whose source or population cannot be read has no coverage and cannot be merged (NoSingleSource, UnprovenPopulation); an unreadable population is never treated as unrestricted. Merges now nest. CoverageRegion.time_ms is renamed time_range. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
34c43f0 to
fea4cc0
Compare
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…rage::derive Merging is the SummaryMerge case of derive, not a second public way to compute coverage: merge_disjoint becomes the private disjoint_union and validate becomes private. Tests that built coverage by hand and merged it directly are replaced by tests through real nodes (source mismatch, empty time range, serde); the other cases are already covered by summary_coverage_examples.rs. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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>
Closes #570.
Why
#567 records each summary state's coverage (rows: source, time range, population; columns: update expression and grouping), and #560 builds
SummaryMerge. But coverage was written by hand by whoever built the node, and nothing checked it against the node's subtree, so a wrong declaration merged silently.Before this PR:
After this PR: nobody writes coverage.
SummaryCoverage::derivecomputes it from the subtree, so A gets{region: us}and the merge fails withPossibleOverlap. A producer only supplies aSummaryAgg's time range (a pane's bounds), throughwith_time_range.How
derivecomputes coveragederive(node, time_range)SummaryAggScansource belowchild(elseNoSingleSource). population:field = 'text'conjuncts ofSummaryAgg.filter, everyFilterandScan.predicates, on a path that passes only throughFilterandTimeRange(elseUnprovenPopulation). time:time_rangefrom the caller. columns:inputas is;reductionkeys named fromchild.schema(elseUnknownColumn).SummaryMergetime_rangemust beNone(TimeRangeOnMerge). Every child must carry coverage (else the child's own derive error, orUnknownInput). Result:merge_disjointof the children's coverage: same source (SourceMismatch), same columns (ColumnMismatch), pairwise disjoint regions (PossibleOverlap). Merges nest.NotSummary; no coverage.Scan's own schema. A node on the path that renames a field (with_schema) makes the population unknown, so a renamed column cannot stand for a different source column.SummaryAggwhose source or population cannot be read (another operator on the path, a renamed field,>, regex,IN,OR, one field equal to two values, a join of two sources) is valid but has no coverage, and any merge over it fails with the reason.The design doc
summary-coverage-calculation.md(moved here from #647) gives the rule for every summary operator, the per-operator rule for non-summary operators that a follow-up will use to widen theFilter/TimeRangepath, and its open questions.Key code interfaces
crates/types/src/ir/summary_coverage.rs,crates/types/src/ir/node.rsOperatorNode::newSummaryMerge: derives, fails if not provably disjoint.SummaryAgg: derives with no time range; no coverage if unreadable.with_time_rangewith_new_childrenSummaryAgg's time range.validate_structureMissingif derivable coverage is absent,DerivedMismatchif the stored one differs (e.g. edited JSON).Removed:
with_coverage, the column check insideSummaryMerge's schema derivation (now inmerge_disjoint), and hand-written coverage in tests.Tests
summary_coverage_examples.rsbuilds every #560 example with realregion = '…'filters: time panes, populations, joint regions, a tabular source, different columns, unreadable filters.summary_coverage_derivation.rspopulation_combines_every_filter_on_the_pathSummaryAgg.filter, aFilternode andScan.predicates, through aTimeRangeunsupported_predicates_leave_the_population_unknown>,OR, one field equal to two valuesrenamed_field_does_not_fake_a_disjoint_populationtiercolumn cannot pass asregionrenamed_update_column_does_not_mergelatencyread from a renamed scan is rejectednested_merges_composerebuilding_keeps_coveragewith_new_childrenkeeps the time range and re-derives the populationsummary_over_two_sources_has_no_coveragesummary_merge_structure.rsaddsoverlapping_panes_do_not_merge,forged_coverage_is_rejected,merges_nestandunproven_population_does_not_merge.Out of scope
TimeRangeholds a relative duration, so absolute pane bounds cannot be checked in the logical IR.Project,AggregateorJoin: the doc's per-operator table; a follow-up PR.Stack and validation
Order: #645 (merged) → #567 (merged) → #560 → this PR → #539 → #540 → #541 → #542 → #543.
Workspace tests pass (1,647) and workspace/all-target/all-feature Clippy with warnings denied passes.
🤖 Generated with Claude Code