fix(tesseract): share a rolling window's base scan across measures - #11852
Conversation
|
Claude finished @waralexrom's task in 8m 8s —— View job Review: sound design, no correctness defect found — 0 high, 2 medium, 3 low (5 inline comments)Full review — verdict, findings, what I verified and what I could not
VerdictThe shape of the change is the right one, and the two load-bearing arguments hold up under reading:
Details I checked and found correct rather than merely plausible:
Two windows differing only by FindingsMedium
Low
Also minor, not worth an inline: One thing to decide, not a defectThe pass is unconditional with no way to turn it off. Everything else in this area has an escape hatch ( What I could not run
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #11852 +/- ##
=======================================
Coverage 80.35% 80.35%
=======================================
Files 500 500
Lines 106555 106555
Branches 3881 3881
=======================================
+ Hits 85618 85619 +1
+ Misses 20387 20386 -1
Partials 550 550
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
A rolling window's base CTE aggregates one measure over the rows the window can reach, and which rows those are is decided by the frame and the query's filters rather than by the measure. Windows differing only in the column they aggregate therefore scan the fact table once each where one scan would do. Pins the wanted behaviour on three shapes — a plain query, a joined one, and two measures evaluated at row grain — and the boundaries it must not cross: a differing frame, a differing fact, a time shift (which moves the period a window covers without changing anything in the query its leaf holds), a window without a granularity (whose base CTE is the requested measure's own result and cannot be shared), and a model storing one rollup per rolling measure, which a scan carrying both measures could not be served by. The two snapshots record values a shared scan must not change: they are what the same queries answer while each measure still scans on its own. Only the sharing tests fail today; the rest are guards. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A rolling window's base CTE aggregates one measure over the rows the window can reach, and the rows it reads are decided by the window's frame and the query's filters — not by the measure. Every window still got its own CTE, so a query over several rolling measures scanned the fact table once per measure even where two windows were byte-identical apart from the column they summed. Grouped by a high-cardinality dimension, that repeats the whole (entities × window × anchors) cost per measure. Base scans that read the same rows now ride on one CTE, and each window reads its own column off it. The merge is a pass over the logical plan rather than a decision taken while planning, which is what lets it run after pre-aggregations have been matched: a rollup answers only for a query whose every measure it carries, so a scan carrying two measures could not be served by a rollup holding one of them. By the time this pass runs, each base scan either already reads a rollup — a source it declines to merge — or was left on the fact table, where nothing is lost by sharing it. Two scans read the same rows when their grain, filters, modifiers, join and evaluation context all agree. The last one is not visible in the query a leaf holds: a time shift moves the period the leaf covers and a row-grain evaluation stops it aggregating, and both live beside the query rather than in it. A shape the comparison cannot read answers "not the same", which costs a shared scan and never merges two scans that read differently. A measure two scans have in common — a switch dispatching several stages onto one — becomes a single column of the merged scan rather than two under the same alias. Sharing is confined to a CTE whose only reader is one rolling window, taking it as the measure side of its frame. Only such a reader is repointed at the scan it was folded into, so a reference of any other kind would survive into the SQL naming a CTE that is no longer in the `WITH` list. It also keeps a window without a granularity out: that has no stage on top and its base CTE is the requested measure's own result, which a CTE shared between measures cannot answer for. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Four of the five, plus the doc the fifth asked to stop repeating. The comparison deciding that two scans read the same rows is the pass's whole safety argument, and it was the one part not protected by exhaustiveness: every helper read its fields through accessors, so a field added to any of the compared types would silently widen "the same rows" rather than break the build. They destructure their left-hand argument now. `Query` and `LogicalJoin` keep their fields private and cannot, so they say so. Measures riding on one scan are deduplicated by the resolved reference chain rather than by name, which is the identity the physical side matches a column by. Two symbols resolving to the same chain render the same column; projecting both left each window reading whichever came first, and agreeing only for as long as the two rendered alike. A leaf is a candidate only when the measures it carries are the measures its query selects. The widened CTE takes one list from each, and a leaf whose member is rendered by the stage above rather than by itself has them disagree — unreachable through today's readers, but the guard that keeps it so is about who reads the CTE, not about the two lists. The two snapshots group by a dimension, so their queries now order by it as well: the recorded row order was a tie the plan is free to break, and would have failed for reasons unrelated to the merge. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
f387d55 to
c1b358f
Compare
|
Claude finished @waralexrom's task in 3m 42s —— View job Re-review of
|
| # | Finding | State |
|---|---|---|
| 1 | same_* helpers read through accessors, so a new field silently widens "reads the same rows" |
Fixed — same_evaluation_context, same_grain, same_filter, same_modifiers, same_subquery_join, same_dimension_subquery all destructure their left-hand argument now. Query and LogicalJoin keep private fields and cannot, so L36-38 says so |
| 2 | Measure dedup by full_name() vs. the physical side's resolved chain |
Fixed — member_chain_eq at optimizer.rs:108, matching Schema::find_column_for_member |
| 3 | Snapshots recorded an unpinned within-date row order | Fixed — both queries order by orders.created_at, orders.status, and both .snap files were re-recorded (34 and 38 lines changed) |
| 4 | Widened schema mixes query.schema() dimensions with leaf.measures |
Fixed, and fixed at the right place — optimizer.rs:89 makes list agreement a candidacy precondition, so the rewrite no longer assumes it. This also closes the is_without_member_leaf (Rank-shaped) hole independently of who reads the CTE |
| 5 | Pre-aggregation-ordering rationale written out three times | Fixed — the module doc is down to two sentences pointing at the call site; the load-bearing copy now lives at top_level_planner.rs:56-60, where the ordering is what an editor would move |
All five threads resolved.
Re-verified after the fixes
same_membersas a candidacy guard is order- and length-sensitive, so it rejects rather than accepts on disagreement — the conservative direction. Theis_without_member_leafcase has 1 leaf measure against 0 query measures and is now skipped on length alone.- Dedup by chain leaves the absorbed window pointing at a dropped symbol. That still resolves correctly:
resolve_member_alias→find_column_for_memberkeys onresolve_reference_chain(), the same relationmember_chain_eqnow uses, so the kept column is found rather than falling through tomember.alias()(schema.rs:37-62,multi_stage_rolling_window.rs:147). measure_for_ungroupedis abool, not a per-measure symbol — so the row-grain pair genuinely shares one scan rather than accidentally comparing equal, which is whattest_row_grain_measures_share_only_with_each_otherasserts atfact_scans == 2for three measures.widen_leaf'sQuery::builder()sets all four fields ofQuery; typed_builder makes a new non-defaulted field a compile error there.- Call-site placement is still correct: the new call sits below the
is_pre_aggregations_match_only()early return (so the refresh path is untouched) and belowtry_pre_aggregations, andoptimized_planis shadowed, so the merged plan is whatOriginalSqlCollectorandPhysicalPlanBuilderboth receive. - The new fixture measure
rolling_sum_trailing_7d_prioris amulti_stagetime-shift overrolling_sum_trailing_7d, which is the only thing that makestest_a_shifted_window_keeps_its_own_scana real boundary test rather than a tautology.
Residual (low)
The destructuring pass stopped one level short of reads_same_rows itself, which is the outermost claim and the one function that can be exhaustive — MultiStageLeafMeasure's fields are pub. Also, the "private fields, compared by hand" note names two types where four are in that position (LogicalJoinItem and Cube are read through accessors too). Both in the inline comment; neither is a defect today.
What I could not run
cargo is not permitted in this environment, so I did not compile, run the suite, or re-record the snapshots — the fix commit's claim that the re-recorded snapshots reproduce byte for byte is the author's, not reproduced here. No Docker/psql either. Everything above is from reading the changed files together with the planner and physical-builder code they depend on.
| pub fn reads_same_rows(a: &MultiStageLeafMeasure, b: &MultiStageLeafMeasure) -> bool { | ||
| same_evaluation_context(&a.evaluation_context, &b.evaluation_context) | ||
| && same_query_rows(&a.query, &b.query) | ||
| } |
There was a problem hiding this comment.
The destructuring pass stopped one level short of its own entry point. MultiStageLeafMeasure's three fields are all pub (multistage/leaf_measure.rs:11-15), so this is the one function in the module that can be made exhaustive and isn't — and it is the outermost claim, the one that says "these two leaves read the same rows". A fourth field added to the leaf is silently excluded from the comparison, which is precisely the shape of the same_evaluation_context omission that was caught pre-commit.
| pub fn reads_same_rows(a: &MultiStageLeafMeasure, b: &MultiStageLeafMeasure) -> bool { | |
| same_evaluation_context(&a.evaluation_context, &b.evaluation_context) | |
| && same_query_rows(&a.query, &b.query) | |
| } | |
| pub fn reads_same_rows(a: &MultiStageLeafMeasure, b: &MultiStageLeafMeasure) -> bool { | |
| let MultiStageLeafMeasure { | |
| evaluation_context, | |
| query, | |
| // What the merge widens, so deliberately not part of "the same rows". | |
| measures: _, | |
| } = a; | |
| same_evaluation_context(evaluation_context, &b.evaluation_context) | |
| && same_query_rows(query, &b.query) | |
| } |
Related, and only a doc fix: the note at L36-38 names Query and LogicalJoin as the types whose fields are private and so compared by hand. LogicalJoinItem (cube, on_sql, splits_rows) and Cube (cube, original_sql_pre_aggregation) are in exactly the same position — same_join_item and same_cube read them through accessors too — but a reader checking the invariant against that list would conclude those two are covered by destructuring. Worth naming all four. Fix this →
Rewriting the plan is clones and builders throughout, so neither the pass nor the leaf it widens can fail. Both said they could, which made the caller handle an error that has no way of arising. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @waralexrom's task in 1m 8s —— View job Re-review of
|
| Claim | Verified |
|---|---|
| Single call site | Yes — top_level_planner.rs:61 is the only one (grep over rust/); no ? left, and plan() still returns Result for everything around it, so no unused-Result or type mismatch |
Both match arms of the CTE map now agree in type |
Yes — widen_leaf and rename_measure_input both return Rc<LogicalMultiStageMember> |
Diverging from PreAggregationOptimizer, which does return Result, isn't an inconsistency |
That pass resolves symbols and genuinely fails; this one does not. Signature follows the code |
| Call-site ordering comment survives the edit | Yes — top_level_planner.rs:55-60, still below the is_pre_aggregations_match_only() early return and below try_pre_aggregations, with the load-bearing "merging is only safe once rollups have been matched" sentence intact |
| Nothing else in the pass shifted | Yes — the merge logic, same_rows.rs, the fixtures, the tests and both snapshots are byte-identical to c1b358f |
Open thread from last round
same_rows.rs:19 still applies unchanged: reads_same_rows reads a.evaluation_context / a.query through fields rather than destructuring MultiStageLeafMeasure, and the "private fields, compared by hand" note at L36-38 still names two of the four types in that position (LogicalJoinItem and Cube are read through accessors too). Not reposting — the thread is unresolved and says it. Low, and not a defect today.
Also minor, not worth an inline: now that optimize is infallible and &self is unused, RollingBaseScanOptimizer is a unit struct wrapping a function — a free pub fn would say the same thing with less ceremony. Purely taste; the Optimizer shape matches its neighbour.
What I could not run
cargo is not permitted in this environment, so I did not compile or run the suite — that the refactor type-checks is from reading the single call site and both changed signatures, not from cargo check. No Docker/psql either, so the snapshots are unchanged-by-inspection rather than re-recorded here.
2 participants
Part of #11770.
Problem
A rolling window's base CTE aggregates one measure over the rows the window can
reach, and the rows it reads are decided by the window's frame and the query's
filters — not by the measure. Every window still got its own CTE, so a query
over several rolling measures scanned the fact table once per measure even
where two windows were byte-identical apart from the column they summed.
Grouped by a high-cardinality dimension that repeats the whole
(entities × window × anchors) cost per measure. The reported query is three
calculated measures over five rolling sums covering two distinct windows: five
scans where two would do.
What changed
Base scans that read the same rows now ride on one CTE, and each window reads
its own column off it.
The merge is a pass over the logical plan rather than a decision taken while
planning, and it runs after pre-aggregations have been matched. That
ordering is what makes it safe to take unconditionally: a pre-aggregation
answers only for a query whose every measure it carries, so a scan carrying two
measures could not be served by a rollup holding one of them — a model storing
one rollup per rolling measure (the
partitionedRollingshape) would silentlyfall back to the fact table the moment two were queried together. By the time
this pass runs, each base scan either already reads a rollup — a source the
pass declines to merge — or was left on the fact table, where nothing is lost
by sharing it. No measure-list bookkeeping is needed to arbitrate between the
two.
Two scans read the same rows when their grain, filters, modifiers, join and
evaluation context all agree. The last one is not visible in the query a leaf
holds: a time shift moves the period the leaf covers and a row-grain evaluation
stops it aggregating, and both live beside the query rather than in it. A shape
the comparison cannot read answers "not the same", which costs a shared scan
and never merges two scans that read differently. A measure two scans have in
common — a
switchdispatching several stages onto one — becomes a singlecolumn of the merged scan rather than two under the same alias.
Sharing is confined to a CTE whose only reader is one rolling window, taking it
as the measure side of its frame. Only such a reader is repointed at the scan
it was folded into, so a reference of any other kind would survive into the SQL
naming a CTE that is no longer in the
WITHlist. It also keeps a windowwithout a granularity out: that has no stage on top and its base CTE is the
requested measure's own result, which a CTE shared between measures cannot
answer for.
Testing
cargo test -p cubesqlplanner --features integration-postgres— 1382 pass,including the existing rolling-window, multi-stage and pre-aggregation suites
against a real Postgres.
grain) and five boundaries it must not cross. Each sharing test was confirmed
to fail with the pass reverted.
disabled, i.e. while each measure still scans on its own. Re-running them
against the merged plan reproduces them byte for byte, so the merge is
value-preserving on both the grouped and the row-grain path.
switch-dispatched stages was emitted as two columns under one alias, andomitting the evaluation context from the comparison merged a time-shifted
scan with its unshifted twin, collapsing a growth ratio to zero.
Risks
join tree is built once per query and handed to every leaf, and
test_measures_over_a_joined_query_share_a_base_scanpins that; were it tochange, the pass would stop firing for joined models rather than merge
wrongly.
referenced_cte_names, which every node linkinga CTE by name has to declare. A member joined in by name through the builder
context would not be counted; today that channel carries
dimension-calculation CTEs only, and those are never candidates here.
merged with another such scan. That is a shared scan not taken, never a wrong
one.
Note for review order
#11824 currently also carries a plan-time version of this merge. That half is
superseded by this PR and will be removed from it, leaving #11824 to the
literal base-scan bounds, which are derived while planning and cannot move into
an optimizer pass.
🤖 Generated with Claude Code