fix(tesseract): never serve a calendar time shift partly from a rollup

[waralexrom]

Summary

A calendar time_shift is not arithmetic on the time column — it is a mapping held in the
calendar's own table, which Cube applies by joining the fact table on the shifted key. A
rollup stores its rows joined on the unshifted key, so replacing the fact scan with one
dropped the calendar cube from the FROM and the shift was applied nowhere: a prior-year
measure silently returned the current period's value, and disabling the pre-aggregation was
the only way to get correct numbers.

The answer is now binary. Either everything the shift reads is materialized and the query is
served entirely from pre-aggregation tables, or the whole query falls back to the source
database — never a mix of a rollup and a live calendar in one query, which could not run on
Cube Store anyway.

Changes

• shift_for_substituted_column refuses any calendar-cube time dimension: no offset of a
  stored column reproduces a table mapping.
• can_carry_time_shifts gains one exception — every member the shift's sql reads is
  stored by the pre-aggregation — which a rollup_join between the fact rollup and a rollup
  of the calendar satisfies. CalendarTimeShiftSqlNode asks the same question before
  rendering, so the gate and the renderer cannot disagree.
• Only an untruncated stored column counts as holding the mapping: a stored time dimension
  carries a period, while the shift joins on an exact value.
• extract_date_range no longer offsets a pre-aggregation's range by an interval that merely
  names a calendar mapping.
• Fixes a second defect of the same shape, not calendar-specific: a granularity defined with
  sql is read from a stored column, so it can be served neither by a finer rollup nor by a
  dimension kept at several granularities at once. Both previously rendered the cube's own
  table into a rollup scan (missing FROM-clause entry) or returned another granularity's
  column.
• Documents what a model needs for a shift to be served from rollups, and fixes the example,
  which omitted the indexes Cube Store requires to join rollups.

Testing

cargo test -p cubesqlplanner with --features integration-cubestore: 1431 passed, 0
failed; clippy clean.

• 19 planner tests covering both rules, plus controls that must keep using the rollup.
• Executed against Postgres via testcontainers, and — for the rollup-only path — against a
  live Cube Store. This is the first rollup_join in the repository exercised on Cube Store;
  every other one is plan-level or runs on Postgres, which is why the harness had to learn to
  build each referenced rollup from its own definition.
• A JS integration test covers the same ground through the path a deployment takes: YAML
  model, CubeEvaluator, the native planner, Postgres.
• The fixture calendar's first retail year is 53 weeks, so "one year back" is 371 days for
  some rows and 364 for others. No interval reproduces that, so the snapshots discriminate:
  one rollup answers 4090 -> 9 in one year and 8094 -> 4090 in the next.

Known limitation, not addressed here

A rollup declaring one time dimension at several granularities exposes a single stored column
for all of them — the reference map is keyed by member alone — so the last declaration wins
and a query for another grain can silently read the wrong column. This predates the change
and is not calendar-specific; the guard added here covers only the sql-granularity half.
Fixing it properly means making the reference map granularity-aware.

🤖 Generated with Claude Code

[mintlify]

Preview deployment for your docs. Learn more about Mintlify Previews.

Project	Status	Preview	Updated
cubed3	🟢 Ready	View Preview	Sep 23, 2026, 2:36 PM

💡 Tip: Enable Automations to automatically generate PRs for you.

[claude]

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

---

Review complete — 1 docs bug, 1 unexercised test, 2 low. Core fix looks right.

Full review

• Gather context (diff vs 9070984, changed files, planner chain)
• Review Rust planner changes
• Review tests and fixtures
• Review docs
• Handle prior review threads
• Post findings

Verdict

The shape of the fix is correct: a calendar time_shift is a table mapping, so either every column it reads is materialized or nothing is. Making CalendarTimeShiftSqlNode ask the same question as the matcher, and refusing a truncated stored column, both follow from that. The rollup_join-on-Cube-Store test with a 53-week first retail year is the right discriminator — 4090 -> 9 vs 8094 -> 4090 cannot be produced by any interval.

Findings

#	Where	Severity
1	calendar-cubes.mdx:538 — documented rollup_join omits custom_calendar.prev_year_date, so the documented model falls back	medium
2	integration_calendar_rollup_join.yaml:170 — coarse-column fixture never declares the mapped column; the new "day" guard is unexercised	medium
3	optimizer.rs:630 — hardcoded granularity == "day" assumes a day-grain calendar	low
4	time_shift_state.rs:59 — time_shift_pk_full_name().is_some() is true for every calendar-cube dimension, so plain interval shifts also lose matching	low

All four posted inline.

Not posted inline, summary only: calendar_time_shift.rs:40 says the renderer asks "the same question the matcher asks before admitting the shift, so the two cannot disagree". They can — the matcher additionally requires granularity == "day", the renderer accepts any substituted column. The direction is safe (renderer is the laxer one, and the gate runs first), but the comment claims a symmetry the code does not have.

Things I checked that are fine

• try_rewrite_schema_and_filter (optimizer.rs:227) admits a pre-aggregation without can_carry_time_shifts and with TimeShiftState::default(), which would turn the new CubeError::internal into a hard failure — but AggregateMultipliedSubquery nodes carrying an evaluation context are excluded at optimizer.rs:365, so no shifted stage reaches it.
• An undeclared interval on a calendar pk lands in interval_shifts, which makes calendar_shifts_are_stored return false — the gate and the renderer agree there.
• stored_time_dimension_column extraction keeps the reference map and the gate keyed identically (base_symbol.full_name()).
• The JS test pins useNativeSqlPlanner: true, so it is a Tesseract test and does not silently assert legacy-planner behaviour.

Review threads

Listing returned 0 of my own threads (nodes: [], single page) — no prior threads, nothing resolved, nothing re-affirmed.

Not run

cargo test -p cubesqlplanner and the Postgres/Cube Store integration suites were not run in this environment; finding 2 is from reading the fixture against calendar_shifts_are_stored, not from a failing run.

· branch `cube-calendar-preagg-wrong-results`

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 84.89%. Comparing base (3222ec0) to head (523cb66).
⚠️ Report is 9 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #11962      +/-   ##
==========================================
+ Coverage   80.52%   84.89%   +4.37%     
==========================================
  Files         508      261     -247     
  Lines      107359    87535   -19824     
  Branches     4043        0    -4043     
==========================================
- Hits        86450    74314   -12136     
+ Misses      20337    13221    -7116     
+ Partials      572        0     -572     

Flag	Coverage Δ
cube-backend	?
cubesql	84.89% <ø> (+<0.01%)	⬆️

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.

[claude]

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

---

Review complete — 1 high, 1 medium, 2 low; 4 inline comments posted

Full review

• Gather context (diff vs. base 9070984, linked model fixtures, prior threads)
• Review Rust planner changes
• Review tests and fixtures
• Review docs
• Resolve stale threads / post findings

Verdict

The core insight is right and the modelling is careful: a calendar time_shift is a table mapping, not arithmetic, so a rollup joined on the unshifted key cannot carry it. Making the answer binary — all rollups or all source — is the correct shape, and the 53-week fixture year is what makes the tests actually discriminating (4090 -> 9 in one year, 8094 -> 4090 in the next, which no interval reproduces). Executing a rollup_join against a live Cube Store is a genuine addition to the harness.

Two defects worth addressing before merge, both in pre_aggregation/optimizer.rs.

Findings

#	Severity	Where	Concern
1	high	optimizer.rs:698 (extract_date_range)	For a calendar shift the unshifted reporting range is handed to the storage layer as matchedTimeDimensionDateRange, but the scan filters on the mapped column. With partition_granularity set, the shifted measure reads partitions a year off its rows and comes back empty. No fixture declares partition_granularity, so the suite cannot see it — and calendar_shift_partition_range_agrees_with_rendered_filter pins the reporting range as correct. None (decline to prune) is the honest answer.
2	medium	optimizer.rs:669 (can_carry_time_shifts)	calendar_shifts_are_stored is a per-stage boolean OR'd into a per-member predicate, so it excuses members CalendarTimeShiftSqlNode never rewrites — a stored dimension that merely depends on the shifted calendar dimension renders unshifted. Unverified at runtime (see below); the asymmetry is visible in the code.
3	low	calendar_time_shift.rs:44-55	renders_from_stored_columns duplicates calendar_shifts_are_stored; both comments promise they cannot disagree, but nothing enforces it. One helper over a membership closure would.
4	low	time_shift_state.rs:48-54, optimizer.rs:589-593, :694-700	Explanatory comments over the repo's 3-line limit.

What I did not verify

No warm cargo target here and a cold cubesqlplanner build was out of budget, so I did not run cargo test -p cubesqlplanner or reproduce finding 2. Finding 1 is traced statically end to end — optimizer.rs:501 → planner/base_query.rs:148 → PreAggregationPartitionRangeLoader.ts:458 — but was likewise not executed. The JS test at packages/cubejs-schema-compiler/test/integration/postgres/calendar-pre-aggregations.test.ts was not run (needs Postgres).

Smaller notes, not posted inline

• PreAggregation::stored_time_dimension_column returns a bare (String, String); the one new caller reaches for .0 and the reader has to go read the body to learn which half is the member name. Two methods, or a named struct, would carry it.
• calendar_shifts_are_stored re-runs extract_time_shifts() — which clones and sorts — once per candidate pre-aggregation, though only the stored set depends on the candidate. Hoisting the extraction to the stage would make the "cheap answer first" guard unnecessary.
• Docs read well and the six-requirement list is the genuinely useful part; the hard-coded "Six things" will rot if a bullet is ever added.

Review threads

Listing returned 4 unresolved threads, all from earlier rounds of this review. All 4 resolved, each with a reply saying why — three were addressed by commits b3de9d4 and the revert 351bd6d (the docs example now declares custom_calendar.prev_year_date; the coarse-column fixture and the granularity == "day" literal no longer exist), and the fourth asked for the shift_for_substituted_column predicate to be narrowed or its breadth documented, which the new doc comment does. None re-affirmed. All 4 findings this round are new.

· branch [`cube-calendar-preagg-wrong-results`](https://github.com/cube-js/cube/tree/cube-calendar-preagg-wrong-results)

[claude]

The exception is a per-stage boolean OR'd into a per-member predicate, so it excuses members the renderer never rewrites.

shift_for_substituted_column is asked per member and answers "this member's stored column can carry its own shift". calendar_shifts_are_stored answers a different question — "the stage's calendar shifts read only stored columns" — and once true the .all() closure accepts every read grouping member with a shift under it, without asking whether that member is one the shift rewrites.

CalendarTimeShiftSqlNode rewrites only a dimension whose own full_name() is a key of shifts, i.e. the calendar's time-shift primary key (calendar_time_shift.rs:70). A stored member that merely depends on the shifted calendar dimension — a view re-export of retail_calendar.retail_date, or a fact-cube dimension declared sql: "{retail_calendar.retail_date}" and listed in the rollup — satisfies has_shift_under but is rendered by the reference branch as its stored, unshifted column. Before this change shift_for_substituted_column returned None for exactly those (reference, or non-time) and the pre-aggregation was refused; now they pass whenever some other member in the stage happens to make the boolean true.

I could not build the crate here to produce the failing snapshot, so treat the specific shape as unverified — but the asymmetry between "the stage's shifts are storable" and "this member carries its shift" is visible in the code. Scoping the exception to the member — its shift is a calendar shift whose sql dependencies are stored, i.e. it is the member the renderer rewrites — closes it and keeps the gate and the renderer asking the same question, which is what the surrounding comments promise.

Fix this →

[claude]

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

---

Review complete — 1 high, 1 medium (re-affirmed), 3 low. The rollup-only rule is right; the gate that enforces it doesn't check for the join it depends on.

Full review

• Gather context (PR diff, all 9 commits including the revert)
• Read the changed Rust planner files in full, not just the diff
• Review tests, fixtures and snapshots
• Review docs changes
• Handle prior review threads
• Post findings

Posted inline

High — optimizer.rs:591 (thread). calendar_shifts_are_stored checks only that the shift's sql dependencies appear in the pre-aggregation's declared member list, never that the pre-aggregation has a join for the shift to rewrite. The mapping is applied by CalendarTimeShiftSqlNode firing on the calendar primary key, which in the working case is rendered in the ON between the two rollup tables. A plain type: rollup declaring dimensions: [retail_calendar.prev_year_date] passes the gate with no such join, is served from one scan, and returns the current period's value for the prior-year measure — the bug this PR fixes. pre_aggregation.source is in hand; requiring PreAggregationSource::Join(_) closes it. Reasoned from the code, not executed: cargo needs approval in this environment and the crate has no prebuilt target.

Low — calendar_time_shift.rs:62 (thread). The new internal error fires only when the shifted dimension is itself substituted. Reading from a pre-aggregation that does not store the calendar pk skips the guard and renders calendar-cube columns into a rollup-only FROM — missing FROM-clause entry instead of the intended message. !self.substituted.is_empty() is the condition that matches the invariant.

Low — dimension_symbol.rs:66 (thread). Four-line doc comment whose second half repeats time_shift_state.rs:45-47 verbatim.

Not inline

PR description is stale. "Only an untruncated stored column counts as holding the mapping" describes 99e1fe1, reverted in 351bd6d. The revert message states the gap plainly — a rollup_join declaring the mapped column while a referenced rollup stores it truncated renders column "..._prev_year_date" does not exist — and that belongs under Known limitation, next to the granularity-keying one already there.

Docs, low. The sales cube in the new calendar-cubes.mdx example has no sql/sql_table, so the example the commit calls "a model that works" is not copy-pasteable. The calendar side of it is complete.

What holds up

The binary rule is the right call: no rollup scan can reproduce a table mapping, and a mixed plan could not run on Cube Store anyway. extract_date_range declining to prune a calendar-shifted stage is correct — and refusing an interval-declared calendar shift with no sql is correct too, not over-conservative, since offsetting a stored key changes the group key without changing which fact rows were rolled up.

The 53-week fixture year earns its keep: 4090 -> 9 in one year and 8094 -> 4090 in the next is a result no interval produces, so the Cube Store snapshots discriminate rather than merely record. Building each referenced rollup from its own definition (test_context.rs:766) is the right shape for a rollup_join harness. The is_sql_defined_granularity guards in dimension_matcher.rs all fail closed toward the source database.

Review threads

The listing returned 4 of my own threads. Resolved 2 — the duplicated renders_from_stored predicate (now a shared helper both call sites use) and the over-length comments flagged last round. Re-affirmed 2 as still live: the per-stage calendar_shifts_are_stored boolean OR'd into a per-member predicate at optimizer.rs:659, and the extract_date_range early return at optimizer.rs:681, where returning on the first date-range filter means the new calendar check only fires if the calendar-shifted filter happens to come first — another time dimension's range ordered ahead of it still prunes the shifted stage.

· branch `cube-calendar-preagg-wrong-results`

[claude]

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

---

Review: sound fix; 3 inline findings (1 medium, 2 low), 1 prior thread still open

Full review

• Read the Rust planner changes
• Read tests and fixtures
• Read docs changes
• Handle prior review threads
• Post findings

Verdict

The shape of the fix is right. A calendar time_shift is a mapping in the calendar's table applied by the join, so the binary rule — either every column the shift reads is materialized and a rollup_join renders the condition that applies it, or the query goes to the source — is the only correct one, and having can_carry_time_shifts and CalendarTimeShiftSqlNode ask the same question through renders_from_stored is what keeps the gate and the renderer from drifting. Refusing partition pruning on a calendar-shifted stage (optimizer.rs:696-720) follows from the same premise: no band derived from the reporting range describes the rows a mapping asks for.

The 53-week first retail year in both fixtures is what makes the tests worth their length — 4090 -> 9 in one year and 8094 -> 4090 in the next cannot be produced by any interval, so the snapshots actually discriminate. The Cube Store execution is the part that proves "rollups only" rather than "no source table in this SQL string".

Findings

#	Severity	Site	Concern
1	Medium	optimizer.rs:612-624	stored ignores the grain a time dimension is truncated to and, for a rollup_join, reads the join's declared members rather than what the referenced rollups store. Admits a shape that fails with column "..._prev_year_date" does not exist. The PR body still claims the guard that 351bd6d reverted; the gap belongs under Known limitation, with a test now that the harness can build each referenced rollup.
2	Low	optimizer.rs:628-637	joins_on ignores PreAggregationJoinMember::granularity. A key kept truncated on both sides passes check_join_members_comparable, then the shift substitutes the untruncated mapped column into the ON.
3	Low	pre_aggregation.rs:34-36	"The one place this naming is decided" — resolve_join_member re-derives the same {alias}_{granularity} at pre_aggregations_compiler.rs:649-658.

Docs read accurately against the code: the allow_non_strict_date_range_match requirement matches the new demotion refusal in dimension_matcher.rs:255-258, the six requirements each correspond to a gate in calendar_shifts_are_stored, and both new link refs resolve to real anchors.

Not raised inline: the entries.len() > 1 refusal at dimension_matcher.rs:266-270 rejects a rollup even when an exact-grain column exists — conservative, and correctly described as the half-fix it is. The test-harness Join arm rebuilds a table once per appearance rather than deduping by (cube, name) as the _ arm does, which is harmless under DROP TABLE IF EXISTS.

Review threads

The listing returned 5 of my own threads. Resolved 4 as addressed by the current diff: the hoisted calendar-shift check in extract_date_range, the joins_on requirement in the gate, the calendar_time_shift.rs:62 guard condition, and the renders_from_stored doc-comment trim. Left open 1 (optimizer.rs:674) — the || calendar_shifts_are_stored disjunct in can_carry_time_shifts is still a per-stage answer applied per-member, unchanged since it was raised. Re-affirmed 1 prior thread; no finding this round was skipped as a duplicate.

Not run: cargo test -p cubesqlplanner and clippy. The PR reports 1431 passed and clippy clean; CI covers both.

· branch `cube-calendar-preagg-wrong-results`

[claude]

The stored set counts a time dimension by its base member name whatever grain it is truncated to, and for a rollup_join it reads the join's own declared member list rather than what the referenced rollups physically store. Both halves of that were the subject of 99e1fe1 and its revert 351bd6d, and after the revert the gate admits a shape it cannot render:

- name: calendar_rollup_coarse
  time_dimensions:
    - dimension: prev_year_date   # the column the shift maps through
      granularity: month
    - dimension: date_val
      granularity: day
- name: demand_with_calendar
  type: rollup_join
  dimensions: [retail_calendar.prev_year_date]
  rollups: [demand.demand_rollup, retail_calendar.calendar_rollup_coarse]

prev_year_date is on the join's dimensions, so stored contains it and the shift is admitted; the calendar rollup only has ..._prev_year_date_month, and the query fails with column "retail_calendar__prev_year_date" does not exist. Your own revert message says the same and calls the gap live.

Two asks, neither of which is the full fix:

• The PR description still claims "Only an untruncated stored column counts as holding the mapping", which is no longer true. That sentence should go, and the rollup_join gap should be listed alongside the multi-granularity one under Known limitation — the revert commit message is the only place it is currently recorded.
• A failure that hard-errors on a legitimate-looking model deserves a test, even an #[ignore]d one, so the next change to this gate sees it. The harness can now reproduce it: the PreAggregationSource::Join arm added in test_context.rs:766 builds each referenced rollup from its own definition, which is exactly what the pre-revert fixture could not do.

[claude]

joins_on matches the primary key by symbol.full_name() and ignores PreAggregationJoinMember::granularity, which is set a few files over precisely to record that a key kept as a rollup's time dimension is truncated (pre_aggregations_compiler.rs:649-658).

check_join_members_comparable does not cover the gap: it only requires the two sides be truncated the same way. Keep the key at month on both rollups and it passes, then the shift replaces the calendar side of the ON with the untruncated mapped column, so the condition compares demand__demand_date_month to retail_calendar__prev_year_date and the shifted stage returns nothing. The mapping joins on an exact value, so this wants

.any(|member| member.symbol.full_name() == *primary_key && member.granularity.is_none())

[claude]

"The one place this naming is decided" is not true: resolve_join_member builds the same {alias}_{granularity} column independently at pre_aggregations_compiler.rs:649-658. Either call this helper there or drop the sentence — a comment asserting a single source of truth that does not exist is what lets the two drift.

[claude]

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

---

Review complete — no new inline comments; 4 prior threads re-affirmed (2 medium correctness, 2 low)

Full review

• Read the diff and changed files
• Analyze planner changes (Tesseract pre-aggregation gating)
• Review tests and docs
• Check prior review threads
• Post findings

Verdict

The shape of the fix is right. Making shift_for_substituted_column refuse every calendar-cube dimension (time_shift_pk_full_name is populated for all dimensions of a calendar cube, the pk included — dimension_symbol.rs:436), and gating the rollup path on calendar_shifts_are_stored, is binary in exactly the way the description claims. The gate and CalendarTimeShiftSqlNode share CalendarDimensionTimeShift::renders_from_stored, and the two inputs do line up: make_pre_aggregation_source (optimizer.rs:480-486) deliberately does not filter dimensions, so the reference map the renderer sees carries the same members the gate checked. The extract_date_range early return is per-stage, which calendar_shift_stage_prunes_no_partitions pins down.

The non-calendar half is also correct: rollup_granularity returns the custom granularity unchanged when it aligns with the date range, so the new granularity != *time_dimension.granularity() check in dimension_matcher.rs:256 refuses only genuinely demoted grains, not a rollup that declares the sql granularity itself.

Test coverage is unusually good — 19 planner tests with both positive controls and must-fall-back mirrors, the 53-week fixture that no interval can reproduce, a live Cube Store run, and a JS test through CubeEvaluator. Doc anchors #rollup_join and #allow_non_strict_date_range_match both resolve.

Review threads

The listing returned 4 unresolved threads, all authored by claude. 0 resolved — I read the code at each site and every concern is still live. 4 re-affirmed:

Thread	Site	Still live because
…6k8xm9	optimizer.rs:623-643	stored is keyed on stored_time_dimension_column(member).0, the base name with the grain discarded, and joins_on reads the join's own declared member list rather than what the referenced rollups store
…6k8xvw	optimizer.rs:629-637	joins_on matches on symbol.full_name() alone; PreAggregationJoinMember::granularity (compiled_pre_aggregation.rs:16) is ignored, so a truncated join key passes
…6k8LMO	optimizer.rs:674	|| calendar_shifts_are_stored is still a stage-wide boolean OR'd into a per-member predicate
…6k8x1x	pre_aggregation.rs:34	"The one place this naming is decided" is false — resolve_join_member (pre_aggregations_compiler.rs:649-657) builds format!("{}{}", member.alias(), suffix) independently

Two findings I reached independently this round — the truncated stored column at optimizer.rs:623, and the gate trusting the rollup_join member list at optimizer.rs:643 — are the same root cause as …6k8xm9, so they are folded into that thread rather than posted again.

One note on the first of those, since the PR body and the code disagree: the description's third bullet says "Only an untruncated stored column counts as holding the mapping: a stored time dimension carries a period, while the shift joins on an exact value." Commit 351bd6d reverted that guard, and nothing in the final diff replaces it. A calendar rollup declaring the mapping column under time_dimensions: with granularity: month satisfies renders_from_stored, the gate admits the rollup_join, and the shifted condition compares a month-truncated column against an exact day value. Either restore the guard or drop the bullet.

Not raised inline

• dimension_matcher.rs:268 — the entries.len() > 1 refusal covers only sql-defined granularities; the same one-column-per-member ambiguity still lets a predefined-granularity query read another grain's column. The PR body names this as a known pre-existing limitation, which is a fair scoping call.
• test_context.rs:767 — the new PreAggregationSource::Join(_) arm drops the (cube, name) dedup the _ arm does. Test-only, and a repeated CREATE TABLE is idempotent here.

· [`cube-calendar-preagg-wrong-results`](https://github.com/cube-js/cube/tree/cube-calendar-preagg-wrong-results)