Skip to content

fix(tesseract): stop a rolling window's plan fanning out across measures - #11824

Open
waralexrom wants to merge 6 commits into
masterfrom
tesseract-rolling-window-fanout
Open

fix(tesseract): stop a rolling window's plan fanning out across measures#11824
waralexrom wants to merge 6 commits into
masterfrom
tesseract-rolling-window-fanout

Conversation

@waralexrom

@waralexrom waralexrom commented Sep 9, 2026

Copy link
Copy Markdown
Member

Fixes two of the three problems behind #11770.

Problem

A query with several rolling_window measures over a high-cardinality
dimension produces a plan whose cost grows as (entities × window × anchors).
The report is three calculated measures over five rolling sums (two distinct
windows), grouped by a 4.7K-value dimension at day granularity over 33 days —
12 CTEs plus the root, ~10M intermediate rows for a result of at most 5K, and
on BigQuery a CPU guardrail hit at 133K CPU-seconds over 170MB scanned.

Two separable causes, both addressed here:

1. The base scan's date bounds were opaque. Each rolling measure's base CTE
restricts the fact table to the span its window can reach, and read both ends of
that span back off the time_series CTE:

WHERE activity_date >= (SELECT min("date_from") FROM time_series) - interval '7 day'
  AND activity_date <= (SELECT max("date_to")   FROM time_series)

No engine can eliminate partitions by a scalar sub-select, so every base scan
read the whole table. The legacy planner emitted literals here
(BaseQuery.dateFromStartToEndConditionSql takes the bounds off the series it
has already computed), so this is a Tesseract regression.

2. One base scan per rolling measure. 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 — not by the measure. Every window
still got its own CTE, so the five rolling sums produced five scans of the fact
table where only two distinct (window, filter) pairs exist.

What changed

Literal bounds. The series is derived from the time dimension's granularity
and date range, so its span is known while planning. QueryTimeSeries gained
covering_bounds_predefined / covering_bounds_custom, which derive the span
per bucket rather than by walking the series (so a wide range costs nothing),
and the regular rolling-window filter renders them as literal parameters.

The span runs one interval past the range end on purpose: a series materialized
while planning snaps its points to bucket boundaries, so its last bucket ends at
most an interval past the range end, while one generated in SQL
(generate_series, GENERATE_DATE_ARRAY, …) steps from the range start
instead, and its last point sits at most an interval before it. The span has to
cover both, and is exact wherever the range's ends are already bucket
boundaries — the common case. A wider span changes no result anyway: the rolling
join applies the exact frame on top.

The sub-select remains where the span is not derivable at plan time: a date
range that is itself a query (time_series_get_range), a granularity whose
periods come off a calendar cube, and the placeholder ranges of
pre-aggregation SQL.

Shared base scan. Measures whose base scans are equivalent now ride on one
leaf CTE, and each window reads its own column off it. Equivalence is the state
comparison the multi-stage planner already deduplicates CTEs by, narrowed three
ways:

  • to measures reading the same set of cubes — one reading a cube the scan does
    not would widen its join tree, and that is a different scan;
  • to a scan a rolling-window stage actually consumes, since a window without a
    granularity has no stage on top and its base CTE is registered as the
    measure's own result;
  • to measures no pre-aggregation separates. A pre-aggregation only answers for a
    query whose every measure it carries, so a model storing one rollup per
    rolling measure — the partitionedRolling shape — would fall back to the fact
    table for all of them the moment two were merged. That costs far more than the
    scan the merge saves, so sharing yields to it.

Windows differing in frame, filters, grain, or fact keep their own scan.

The reported query goes from 5 base scans to 2, and from 13 CTEs to 11.

Testing

  • cargo test -p cubesqlplanner — 1382 pass. The three reproduction tests from
    the issue are in rolling_window::fanout_repro; two of them are un-ignored by
    this PR (test_base_scan_date_bound_is_literal,
    test_base_table_is_not_rescanned_per_rolling_measure), and each was
    confirmed to fail with its fix reverted.
  • New unit tests pin the covering bounds against a walked series for every
    predefined granularity, and against an unaligned SQL-generated series.
  • New planner tests pin both sharing boundaries: two rolling measures over
    different facts keep their own scans, and so do two without a granularity.
  • Real Postgres under CUBEJS_TESSERACT_SQL_PLANNER=true: 112 passing in
    dataschema-compiler, pre-aggregations, pre-aggregations-multi-stage,
    pre-aggregations-calculated-measures, pre-aggregations-time,
    postgres-cumulative-measures, multi-stage-grain-rolling-window,
    rolling-window-offset-no-granularity.
  • Two /code-review high rounds raised eight findings; all eight are fixed, the
    last three commits carry them, and each fix was confirmed to fail with itself
    reverted. The pre-aggregation regression has a dedicated fixture and test
    (test_sharing_yields_to_a_rollup_per_rolling_measure).

Risks

  • The base scan reads up to one granularity bucket more than the series
    strictly needs. Negligible at day granularity; at month granularity over a
    12-month range it is ~8% more rows, traded for partition elimination that was
    entirely absent.
  • The pre-aggregation gate reads declared measure lists, not match results, so a
    rollup that would not have served this query anyway still holds the merge back.
    Such a query keeps the plan it has today; it does not gain the shared scan.
  • The remaining half of Rolling-window measures generate an unrunnable plan: time-series join has no equality predicate #11770 is untouched: the rolling CTE still joins
    time_series to its base CTE on a date range only, with no equality on the
    group-by dimension, so the engine still cannot hash-join and its row estimate
    stays badly off. That is a plan-shape change for every rolling query and is
    deliberately left for a separate PR;
    rolling_window::fanout_repro::test_rolling_join_restricts_by_dimension
    stays ignored to mark it.

🤖 Generated with Claude Code

claude and others added 3 commits September 9, 2026 13:11
Adds ignored planner tests that pin the plan shape reported in #11770:
several rolling_window measures queried with a high-cardinality dimension
scan the fact table once per measure and join time_series to each base CTE
on a date range only, with no equality predicate on the dimension that is
in the GROUP BY of both sides. On the reporter's cardinality (4.7K entities
over 33 day-anchors) that materialises ~10M intermediate rows for a 5K-row
result; Postgres plans the range-only join as a nested loop with a join
filter.

A third test covers the secondary observation: the base scan's date bound
is emitted as a scalar sub-select over time_series, where the legacy planner
emitted a literal, so engines cannot use it to eliminate partitions.

The tests assert the wanted shape and fail today, so they are #[ignore]d.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PeQG6eSzkVJzkcf49U2Uqu
A regular rolling window restricts its base scan to the span the window
can reach, and read both ends of that span back off the time series with
a scalar sub-select. The bounds are opaque to an engine that way: no
partition can be eliminated by them, so every base scan of the query
reads the whole table, and a query with several rolling measures pays
that once per measure.

The series is derived from the time dimension's granularity and date
range, so its span is known while planning. Compute it there and render
both bounds as literal parameters, falling back to the sub-select only
where the span is not derivable — a range that is itself a query, a
granularity whose periods come off a calendar cube, and the placeholder
ranges of pre-aggregation SQL.

The span covers one bucket past the range end: a series materialized
while planning snaps its points to bucket boundaries, while one generated
in SQL steps from the range start, and its last bucket can end an interval
further out. The rolling join applies the exact frame on top either way,
so a wider base scan changes no result.

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.

Measures whose base scans are equivalent now ride on one CTE: the leaf
carries them alongside its own member, and each window reads its own
column off the shared scan. Equivalence is the state comparison the
multi-stage planner already deduplicates CTEs by, narrowed further to
measures reading the same set of cubes — a measure reading one the scan
does not would widen its join tree, which is a different scan. Windows
differing in frame, filters, grain, or fact keep their own scan.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@github-actions github-actions Bot added the rust Pull requests that update Rust code label Sep 9, 2026
@claude

claude Bot commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

Claude finished @waralexrom's task in 7m 9s —— View job


Review: solid change, one likely regression for custom granularities — 1 medium, 3 low, posted inline

Full review
  • Gather context (diff, related files)
  • Review covering-bounds derivation in time_series.rs
  • Review shared-base-scan logic in multi-stage planner
  • Review filter/SQL rendering changes
  • Review test coverage
  • Post findings

What I checked

I traced both halves of the change end to end rather than reading the diff in isolation:

Literal bounds — the over-approximation is sound. I verified the tail claim against the actual dialect templates rather than taking it on faith. Postgres (generate_series), Snowflake (ARRAY_GENERATE_RANGE), MSSQL (recursive CTE) and BigQuery all step from {{ start }} = the raw range start with no truncation, so the last generated point is ≤ range_end and its bucket ends at last_point + interval - 1ms. Your bound is end_of(bucket_after(bucket_containing(range_end))), i.e. B + 2·interval - 1ms where B ≤ range_end < B + interval — so it dominates, and it dominates for month/quarter/year too since interval addition is monotone. The materialized path (generate_predefined/generate_custom) snaps to the same B for the lower bound, which is what the new unit tests pin. Precision handling also checks out: computing at precision 3 and letting format_to_date widen .999.999999 (and format_from_date .000.000000) is correct, so the precision = 3 hardcode is fine.

Shared base scan — the equivalence check is tight enough. The thing I most wanted to disprove was two windows with different frames sharing a scan. They can't: eq_as_state compares time_dimensions_filters via tree_ops::eq_with_memberBaseFilter::eq, which compares typed_filter.values(), and the rewritten RegularRollingWindowDateRange carries [from, to, trailing, leading]. So a 7-day and a 30-day window differ in state and keep their own scans. time_shifts is compared too, and is_ungrupped separately. The collect_cube_names guard is the right narrowing: since dimensions come from the shared state, equal measure-cube sets imply equal join trees, which also means equal measure multiplication — so merging can't silently turn a plain scan into a full-key-query join.

Interior mutability is safe here. add_co_measure mutates a description through a RefCell after other descriptions may already reference it, which is only sound because plan_queries builds every description before planning any (multi_stage_query_planner.rs:143). That invariant holds, and schema()/input_cte_aliases() recompute from self rather than snapshotting, so late additions are picked up. Worth noting the doc comment on co_measures calls this out explicitly — good, because it is exactly the thing a future refactor would break.

Dedup is also preserved: the new loop runs before the CTE is allocated, and add_co_measure no-ops when the measure is already the description's own member.

Findings

# Severity Where Issue
1 Medium multi_stage_query_planner.rs:1272-1279 covering_bounds_custom reaches align_to_origin's MAX_BUCKETS guard on a code path that previously never ran — new hard planning failure for fine custom granularities
2 Low time_series.rs:73 "costs the same for a range of any width" is true for the predefined path only; the custom path walks
3 Low fanout_repro.rs:20-22 Module doc says all three tests are ignored; this PR un-ignores two
4 Low (question) filter_sql_context.rs:200-203 Literal path applies apply_db_time_zone, the time_series fallback does not — divergence under a non-UTC db timezone

On #1, the short version: align_to_origin steps one interval at a time from the origin (default = start of the current year) and errors past 50 000 steps. That walk only lived inside generate_custom, which TimeSeries::to_sql never calls on drivers where support_generated_series_for_custom_td is true — i.e. Postgres, BigQuery, Snowflake, MSSQL, MySQL, Presto. rolling_series_bounds now runs it unconditionally, so a custom 5 minutes granularity queried for a November range fails to plan with "Origin alignment did not converge" where it planned fine before. The bounds are a pure optimisation with a working fallback, so degrading to Ok(None) on error rather than propagating costs nothing; suggested patch is inline.

Notes, not findings

  • The risks section is honest and matches what the code does — in particular the pre-aggregation-matching tradeoff on a merged leaf is real and correctly described, and leaving the join-shape half of Rolling-window measures generate an unrunnable plan: time-series join has no equality predicate #11770 to a separate PR with a marker test is the right call.
  • test_base_scan_date_bound_is_literal asserting on all unique params (not just the two it names) makes it double as an assertion that nothing else is parameterised. That's stricter than it looks and will fail on unrelated changes, but as a pin for this specific fix it's defensible.
  • test_rolling_measures_of_different_facts_keep_their_own_scan is the right test to have added — pinning where sharing stops is more valuable than pinning where it happens.
  • I reviewed statically; I did not run cargo test -p cubesqlplanner (build cost in this environment). The reasoning above is from reading the code and the dialect templates, so treat ReferenceError: self is not defined - Issue with whatwg-fetch when using the core lib with Node.js #1 as high-confidence-but-unexecuted — a test with a 5 minutes custom granularity over a late-in-year range should confirm or refute it quickly.
· branch `tesseract-rolling-window-fanout`

Deriving the span a rolling window's base scan is bounded by parses the
time dimension's date range. A pre-aggregation's range holds partition
placeholders rather than dates, so the derivation raised a parse error
where it should simply have no bounds to offer and leave the filter to
read them off the series.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Comment thread rust/cube/cubesqlplanner/cubesqlplanner/src/planner/time_dimension/time_series.rs Outdated
Comment on lines +200 to +203
Ok(Some((
self.format_and_allocate_from_date(from)?,
self.format_and_allocate_to_date(to)?,
)))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question, not a blocker: the two branches this now chooses between are not timezone-equivalent.

format_and_allocate_from_date/to_date run the value through apply_db_time_zone, and use_db_time_zone is !filters_ctx.use_local_tz — true in the common case. The fallback date_range_from_time_series reads min("date_from")/max("date_to") straight off the time_series CTE, whose bounds TimeSeries::to_sql emits as bare quote_string(from_date) with no tz wrapping.

So under a non-UTC CUBEJS_DB_TIME_ZONE the literal path shifts the base-scan bounds and the sub-select path does not. The literal path is arguably the correct one (it matches how every other date-range filter renders), but that also means this PR silently changes the base-scan window for those setups, and the Postgres integration runs quoted in the description would not catch it if they run in UTC. Is there a rolling-window test with a db timezone set?

@codecov

codecov Bot commented Sep 9, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 80.22%. Comparing base (fe263b9) to head (2ac4739).
⚠️ Report is 7 commits behind head on master.

Additional details and impacted files
@@             Coverage Diff             @@
##           master   #11824       +/-   ##
===========================================
+ Coverage   60.16%   80.22%   +20.05%     
===========================================
  Files         239      500      +261     
  Lines       19203   105851    +86648     
  Branches     3886     3886               
===========================================
+ Hits        11554    84920    +73366     
- Misses       7099    20381    +13282     
  Partials      550      550               
Flag Coverage Δ
cube-backend 60.16% <ø> (ø)
cubesql 84.67% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

…use it

Three defects in how the shared rolling-window base scan and its literal
bounds were derived, all found in review.

Deriving the bounds of a custom granularity's series walked its interval
from the granularity's origin through a helper that gives up after 50,000
steps — a guard against an interval that never converges, which a fine
interval and a default origin a few years off the range trips on a range
that converges perfectly well. That turned a working query into a plan
error. It now aligns to the origin the way the series' own range does,
without a step cap.

Sharing a base scan is now confined to a scan a rolling-window stage
consumes. Without a granularity there is no series to walk and no stage
on top: the base CTE is the requested measure's own result, registered
under that measure, which a CTE shared between measures cannot answer
for. Two such measures were registering the same CTE twice, joining it
twice under the second measure's name and duplicating a branch of the key
union.

The partition-range guard on the rendered bounds was unreachable — such a
range is declined while planning, before any bounds exist — and its
comment promised a protection it was not giving.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@claude

claude Bot commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

Claude finished @waralexrom's task in 5m 15s —— View job


Re-review of the two new commits: prior blocker fixed, 2 new low findings, 2 earlier threads still open

Full review
  • Diff the two new commits (e5b2952, 35990ae) against the previously reviewed head
  • Verify the MAX_BUCKETS blocker is actually gone (not just moved)
  • Check the new custom_series_bounds still covers the series the SQL generator emits
  • Check the feeds_window_stage narrowing of shared base scans
  • Check the removed is_partition_range guard is genuinely unreachable
  • Re-check the four earlier threads; resolve the ones the new commits addressed
  • Post findings

What the new commits change, and whether it holds

e5b2952 — parse guard on the date range. rolling_series_bounds now declines a range whose bounds don't parse as datetimes, which is what a pre-aggregation's __FROM_PARTITION_RANGE / __TO_PARTITION_RANGE placeholders are. Correct, and it's the guard that makes the removal in 35990ae safe.

35990ae — the MAX_BUCKETS failure is genuinely fixed. covering_bounds_custom is deleted and replaced by custom_series_bounds, which uses Granularity::align_date_to_originQueryDateTime::align_to_origin. That walk has no step cap, so the "Origin alignment did not converge" plan error I flagged can no longer happen. It also aligns the same way the series itself is placed: TimeDimensionSymbol::get_range_for_time_series (time_dimension_symbol.rs:380) already calls align_date_to_origin on the range start for every custom granularity, so the lower bound provably matches the series' first point rather than approximating it. Thread resolved.

I re-derived the upper bound against that: the SQL series steps from the aligned start, so its last point is the greatest grid point ≤ range_end — i.e. ≤ align(range_end) — and its bucket end is last + interval - 1s. The new bound is align(range_end) + 2·interval - 1s, which dominates. Sub-second origins are fine too: truncating to seconds and padding .000 widens the lower bound and .999 widens the upper.

The is_partition_range removal from date_range_literals is safe. The branch is unreachable given e5b2952's parse guard, and is_partition_range is still used by format_and_allocate_date (filter_sql_context.rs:156), so nothing became dead. The doc comment was correctly narrowed to what the code now does.

The feeds_window_stage narrowing is the right cut. The no-time-dimension call site (:925) takes the base CTE as the requested measure's own result and registers it under that member, so a CTE shared between two measures genuinely cannot answer for both — the double-join/duplicated-key-union bug the commit message describes. Only the :1030 site has a rolling stage on top, and only it opts in. Confirmed the sharing predicate still can't over-match: is_match_rolling_window_base requires Leaf(Measure) and has_aggregates_on_top, and the only other has_aggregates_on_top: true description in the file is the time series (:1118), whose member type fails the matches!. test_two_rolling_measures_without_granularity_keep_their_own_cte pins it.

Filter value indices line up. replace_regular_date_range_filter appends the series bounds after [trailing, leading], and change_date_range_filter_impl keeps the original InDateRange [from, to] in front, so values.get(4)/get(5) in typed_filter.rs is right. FilterValue::from(None) keeps indices stable when a window has no trailing/leading. And because eq_as_state compares time_dimensions_filters by value, the bounds participating in the filter means two windows with different spans can't accidentally share a scan.

Findings

# Severity Where Issue
1 Low multi_stage_query_planner.rs:1312 Second align_to_origin re-walks from the origin (≈2× the steps for a fine interval); precision honoured on the predefined branch only; the deleted covering_bounds_custom unit test left the custom path pinned only indirectly
2 Low multi_stage_query_planner.rs:1208 Two adjacent unnamed bool params — call sites read false, false / ungrouped, true; an enum would carry the meaning

Re-affirming 2 earlier threads that the new commits did not touch (left unresolved, not reposted):

  • fanout_repro.rs:20-22 — module doc still says all three tests are ignored; two are now un-ignored.
  • filter_sql_context.rs:202 — the literal path applies apply_db_time_zone and the time_series sub-select fallback does not, so the base-scan span shifts under a non-UTC CUBEJS_DB_TIME_ZONE. Still an open question; the literal path is arguably the correct one, but the behaviour change is silent and UTC-only test runs won't see it.

Notes, not findings

  • The unbounded walk is the correct trade against a hard failure, and it is not a new cost class: get_range_for_time_series already walks it for every custom-granularity query. Finding ReferenceError: self is not defined - Issue with whatwg-fetch when using the core lib with Node.js #1 just halves what this PR adds.
  • The rolling_window/custom_granularities snapshots are executed results, not SQL, so a too-narrow bound on the custom path would surface as wrong numbers there. That's why I rate the lost unit test low rather than medium.
  • Reviewed statically. There is no cargo cache in this environment, so I did not build or run cargo test -p cubesqlplanner; all reasoning above is from reading the code and the dialect templates.

· branch tesseract-rolling-window-fanout ·

granularity.align_date_to_origin(QueryDateTime::from_date_str(tz, bound)?)
};
let first = align(&date_range[0])?;
let past_last = align(&date_range[1])?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two small things about the new custom path.

The second alignment walks from the origin again. align_to_origin steps the interval one at a time from self.origin (no cap now, which is the right call — see the resolved thread). align(&date_range[1]) therefore repeats every step align(&date_range[0]) just took, plus the range's own length. For a 1 minute custom granularity with the default origin (start of the current year) and a range in November, that's ~450K steps done twice. The range end is on the same grid as first, so it can be reached from first instead of from the origin:

let first = align(&date_range[0])?;
let range_end = QueryDateTime::from_date_str(tz, &date_range[1])?;
let mut past_last = first.clone();
while past_last <= range_end {
    past_last = past_last.add_interval(interval)?;
}
past_last = past_last.add_interval(interval)?.add_duration(Duration::seconds(-1))?;

That's range-length/interval steps instead of origin-distance/interval, and it stays on exactly the same grid because add_interval from a grid point continues the same sequence the origin walk produces (including the month-clamping drift).

precision is honoured on one branch only. rolling_series_bounds computes let precision = 3 and passes it to covering_bounds_predefined, while this function hardcodes .000/.999. Today they agree, so this is only a trap for whoever changes the constant. Either take timestamp_precision here and pad with it, or drop the local and let both branches state 3 directly.

Also worth noting for coverage: covering_bounds_custom and its covering_bounds_custom_start_where_the_walked_series_does unit test were deleted, and the replacement is a private planner method with no direct test. The custom path is now pinned only indirectly, through the executed results in rolling_window::custom_granularities. A unit test asserting custom_series_bounds still starts where generate_custom starts (the assertion the deleted test made) would be cheap to keep.

Fix this →

// Whether a rolling-window stage consumes this CTE. A caller taking the
// CTE as the requested member's own result registers it under that
// member, which one shared between measures cannot answer for.
feeds_window_stage: bool,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The two adjacent bool parameters make the call sites unreadable: add_rolling_window_base(base_member, base_state, false, false, ...) at :925 and (base_member, base_rolling_state, ungrouped, true, ...) at :1030. Nothing at either site says which false is ungrouped and which is feeds_window_stage, and swapping them compiles — the failure mode being either lost sharing or an ungrouped CTE silently grouped.

Since the distinction is exactly "does a rolling stage consume this", a two-variant enum carries it at the call site and lets the doc comment go away:

enum RollingBaseConsumer {
    /// A rolling-window stage aggregates this CTE, so it may be shared.
    WindowStage,
    /// The caller takes the CTE as the requested member's own result and
    /// registers it under that member, which a shared CTE cannot answer for.
    MeasureResult,
}

Call sites then read RollingBaseConsumer::MeasureResult / ::WindowStage.

Fix this →

A pre-aggregation only answers for a query whose every measure it
carries. Sharing a base scan between rolling measures therefore made
matching all-or-nothing across the merged set: a model storing one rollup
per rolling measure — the `partitionedRolling` shape — silently fell back
to the fact table for all of them as soon as two were queried together,
which costs far more than the scan the merge saves.

Sharing now yields wherever some pre-aggregation of the query's cubes
rolls up one of the two measures but not the other. The measure lists are
read off the declarations, so a rollup that would not have matched this
query anyway also holds the merge back; that direction costs a shared scan
the query could have had, the other costs the pre-aggregation.

Also tightens the scanned span and closes two smaller gaps found in the
same review: the span now reaches one interval past the range end rather
than a whole further bucket, which is exact for a range whose ends are
already bucket boundaries; a granularity declaring a zero interval is
rejected instead of hanging the alignment it can never converge; and the
reproduction module no longer describes its two now-passing tests as
known-failing scaffolding.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@claude

claude Bot commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

Claude finished @waralexrom's task in 5m 55s —— View job


Re-review of 2ac4739: pre-aggregation gate holds up, tail bound has a sub-second gap — 2 low, posted inline

Full review
  • Diff 2ac4739 against previously reviewed head (35990ae)
  • Verify the pre-aggregation gate is sound (including transitivity across co-measures)
  • Re-derive the new tail bound against both series shapes
  • Check the is_zero extraction and the new guards
  • Re-check the four earlier threads; resolve the ones this commit addressed
  • Post findings

What the commit changes, and whether it holds

The pre-aggregation gate is correct, including the case I tried hardest to break. pre_aggregations_separate compares the incoming measure only against the description's own member_node(), never against the co-measures already merged into it — so my first question was whether a third measure could merge in and strand a rollup that was serving the second. It can't. "Agrees with X on every declared measure set" is an equivalence relation (each set induces a two-block partition; agreeing on all of them is the intersection of those partitions), so every member of a description agrees with every other member, not just with the representative. Concretely: if all rollups treat A~D alike and all treat A~C alike, then any rollup containing D contains A and C, so it still covers the merged {A,C,D} leaf. Comparing against a representative is sufficient, and cheaper than the pairwise scan.

The lazy resolution is genuinely lazy. || short-circuits, so pre_aggregations_separate is only reached once is_match_rolling_window_base and the cube-set check have both passed — i.e. only for a real sharing candidate. A single-rolling-measure query never touches declared_measures, and the RefCell<Option<Rc<…>>> memo means a multi-candidate query resolves the declarations once. declared_measures also skips measure_references() == None, which is what originalSql and rollupLambda return, matching the doc comment.

Not a new error path. symbols_from_ref propagates resolution failures, and I checked whether that could turn a tolerated broken rollup into a hard planning failure: PreAggregationOptimizer::try_optimizecompile_all_pre_aggregations already propagates with ? for every declared pre-aggregation on the query's cubes (optimizer.rs:82), so any model that would fail here already fails today. The one path where the optimizer is skipped is is_pre_aggregation_query() (top_level_planner.rs:94) — a rollup build query — and there the gate only ever costs a shared scan, never correctness. check_type_fn is passed as |_| Ok(()), so it also doesn't newly reject anything the matcher tolerates.

The tail bound got tighter, and the tightening is sound for the materialized series. Replacing end_of(bucket_after(bucket_containing(range_end))) with range_end + interval - 1s is a real improvement — it now lands exactly on the last bucket's end whenever the range end is bucket-aligned (the common case), rather than a whole bucket past it, which is what covering_bounds_predefined_stop_at_a_bucket_aligned_end pins and why the integration expectation moved from 2026-09-03T23:59:59.999 to 2026-09-02T23:59:59.999. I re-derived it per granularity: bucket ends are bucket_start + interval - ε with bucket_start ≤ floor_sec(range_end), and interval addition is monotone including the month-clamping in add_interval_to_dt (Jan 31 + 1 month → Feb 29), so it dominates for hour/day/week/month/quarter/year. 1 quarter reaching add_interval_to_dt as quarter*3 months is fine.

The double origin walk is gone. custom_series_bounds no longer aligns the range end — it steps one interval from the raw end — so the previous thread's ~2× walk cost is halved to a single align_date_to_origin for the lower bound, which is the one that has to match the series' first point exactly. The is_zero extraction onto SqlInterval (replacing the free is_zero_interval) is the right home for it.

Findings

# Severity Where Issue
1 Low time_series.rs:96 - Duration::seconds(1) + nines padding discards the range end's sub-second fraction, so a SQL-generated series whose last point carries one can end past the bound — silently short results, same shape in custom_series_bounds
2 Low time_series.rs:78 The new interval param is fully derivable from granularity; nothing enforces they agree, and a mismatched pair narrows the span rather than erroring

Finding #1 is the only one with a correctness edge, and it is narrow: it needs a dateRange whose bounds carry a sub-second fraction, on a driver that generates the series in SQL. But it is a new narrowing — the sub-select it replaces read max("date_to") exactly — and it fails as wrong numbers rather than an error, so it's worth closing while the code is being touched. The fix also folds in the precision-honoured-on-one-branch-only half of the earlier custom_series_bounds thread.

Earlier threads

Resolved: the fanout_repro module doc (PRRT_kwDOCOH19M6gooon) — this commit rewrites the paragraph to say which observation is fixed and why the join-shape test stays ignored, which is accurate.

Left unresolved, not reposted:

  • custom_series_bounds — the double walk is fixed, but two of the three points stand: precision is still hardcoded as .000/.999 while the predefined branch takes a parameter, and the deleted covering_bounds_custom_start_where_the_walked_series_does unit test still has no replacement, so the custom lower bound is pinned only indirectly through rolling_window::custom_granularities results.
  • filter_sql_context.rs:202 — the apply_db_time_zone divergence between the literal path and the time_series sub-select fallback is untouched. Still a question rather than a claim.
  • multi_stage_query_planner.rs:1215 — the two adjacent unnamed bool params are untouched.

Notes, not findings

  • The integration_rolling_window_fanout_preagg.yaml fixture is well chosen: two rollups differing only in which single rolling measure they carry is the minimal shape that distinguishes "gate present" from "gate absent", and test_sharing_yields_to_a_rollup_per_rolling_measure asserting on used_pre_aggregations rather than on SQL means it fails loudly rather than snapshot-drifting.
  • Splitting covering_bounds_predefined_snap_outwards into a start-only assertion plus a bucket-aligned-end assertion is a better decomposition than the original — the old test conflated two independent properties into one equality.
  • Reviewed statically. There is no cargo target directory in this environment, so I did not build or run cargo test -p cubesqlplanner; all reasoning above is from reading the code and the dialect templates.
· branch `tesseract-rolling-window-fanout`

/// a range of any width costs the same.
pub fn covering_bounds_predefined(
granularity: &str,
interval: &SqlInterval,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new interval parameter is fully derivable from granularity, and nothing enforces that the two agree.

Granularity::try_new_predefined sets granularity_interval = format!("1 {}", granularity).parse(), so for a predefined granularity the interval is always 1 {granularity} — the caller has no freedom here, it just re-derives what the function already knows. What it does gain is an invariant: pass ("week", "1 day") and first snaps to the Monday while past_end only reaches a day past the range, so the span ends up narrower than the series and the base scan silently drops the tail. That's a wrong-results failure with no error, guarded only by the call site happening to read both off the same Granularity.

Deriving it inside keeps the invariant unstateable:

    pub fn covering_bounds_predefined(
        granularity: &str,
        date_range: &[String; 2],
        timestamp_precision: u32,
    ) -> Result<(String, String), CubeError> {
        check_precision(timestamp_precision)?;
        if !check_predefined_granularity(granularity) { /* ... */ }
        let interval: SqlInterval = format!("1 {granularity}").parse()?;

That also removes the need for the is_zero guard at :88 and covering_bounds_predefined_reject_a_zero_interval1 X is never zero, so both are testing a state the type system would no longer permit.

Related: the unit tests pass "3 months" for quarter and "1 week" for week, while production passes "1 quarter" and "1 week" off granularity_interval(). Same arithmetic today (total_months = year*12 + quarter*3 + month), but it means no test exercises the pairing production actually uses.

Fix this →

let range_start = QueryDateTimeHelper::parse_native_date_time(&date_range[0])?;
let range_end = QueryDateTimeHelper::parse_native_date_time(&date_range[1])?;
let first = predefined_bucket(granularity, range_start, timestamp_precision)?;
let past_end = add_interval_to_dt(range_end, interval)? - Duration::seconds(1);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

- Duration::seconds(1) plus nines truncates the range end to a whole second, so a sub-second range end can put the bound inside the last bucket.

format_with_padding (:411) discards dt's sub-second field and appends nines, so writing range_end = S + f (S = floor_sec(range_end), 0 ≤ f < 1):

bound = floor_sec(range_end + interval - 1s) + .999 = S + interval - 0.001

The f is dropped, not carried. For the materialized series that's harmless — its buckets start on second-aligned boundaries B ≤ S, so B + interval - 0.001 ≤ bound, and the tests pin that. For a series generated in SQL it isn't: the generator steps from the raw range_start, so its points inherit range_start's sub-second fraction. If the last point L lands with L > S (possible whenever L ≤ range_end and both carry the same fraction), its bucket ends at L + interval - 0.001 > bound, and the base scan drops the rows in that gap — up to just under a second at the very end of the range.

Concretely, dateRange: ["2026-08-01T00:00:00.900", "2026-09-02T00:00:00.900"] at day granularity: generate_series steps to 2026-09-02T00:00:00.900, whose bucket runs to 2026-09-03T00:00:00.899, while the bound is 2026-09-02T23:59:59.999. Fact rows in [2026-09-03T00:00:00.000, .899] are excluded from the base scan, and the rolling join can't put back rows that were never scanned — so a sub-second-boundary date range gets silently short results where the old sub-select (max("date_to"), exact) did not.

custom_series_bounds (multi_stage_query_planner.rs:1365-1368) has the identical shape and the identical gap, including for the materialized custom series when the granularity carries a sub-second origin.

Subtracting one unit at the target precision and keeping the fraction removes the truncation entirely:

let past_end = add_interval_to_dt(range_end, interval)? - Duration::milliseconds(1);
Ok((first.start_str, QueryDateTimeHelper::format_to_date(&past_end, timestamp_precision)?))

Both bounds then land exactly on the series' edges for every input, and covering_bounds_predefined_stop_at_a_bucket_aligned_end keeps its current expectation.

Fix this →

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

rust Pull requests that update Rust code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants