fix(tesseract): keep time_shift when a pre-aggregation serves the query

[waralexrom]

Summary

A multi_stage measure with time_shift silently returned wrong values whenever a pre-aggregation served the query. Time shifts are keyed by the fully resolved cube member, but the lookups that decide the shifted leaf's date range and rendering probed with an unresolved name, so the shift was dropped. Fixes CORE-767 / #11536.

Two distinct shapes were affected:

• Through a view — the shifted leaf kept the unwidened pre-aggregation date range, so it scanned a partition set that could not contain its rows and every row came back NULL. The identical query against the cube was correct.
• On a derived time dimension (a time dimension wrapping another cube's time dimension, with the rollup materializing it) — the shift was lost entirely, including from the rendered SQL, so the shifted measure repeated the current period instead of the previous one. The same query without a pre-aggregation was correct.

Changes

• Add a single normalized lookup on TimeShiftState and route the shift lookups through it, so the key-resolution rule lives in one place instead of being re-derived per call site. It probes both ways a key is built: the chain-resolved dimension, and the owned member a declared dimension wraps.
• Apply the shift to the rollup column when the pre-aggregation substitutes a dimension: its SQL is never expanded, so the recursion that normally carries the shift to the owned member never happens. Gated on dimensions known to be substituted — an evaluated dimension must still wait for the recursion, or the interval would be added twice.
• Treat a shift with no interval as "no shift" rather than unwrapping it, matching the other consumers of the same lookup.
• member_name() is deliberately left alone: its other callers compare against query-level names, where view qualification is consistent.

Only Tesseract is fixed. The legacy planner has the same defect and is untouched.

Testing

• Both fixes were landed test-first and each test was confirmed red for the stated reason before the fix — for the view case by dumping the matched usages (both matched the rollup, both with the unwidened range), for the derived case by observing the executed rows.
• Cube-level and view-level tests now sit side by side. The derived-dimension test asserts the widened range, that every rendered interval sits directly on the rollup column (so a doubled shift cannot pass), and the executed values against a seed holding a period before the queried range.
• cargo test -p cubesqlplanner: 1225 passed.
• With Postgres + CubeStore (--features integration-cubestore): 1216 passed. The 9 switch_rolling failures are pre-existing on this branch's base — verified against a baseline run with the fixes reverted — and come from the older released cubestored binary used locally.

One limitation worth recording: the view fix is pinned only by assertions on the plan. The widened range decides which rollup partitions get loaded, and the test harness builds each rollup as one whole table, so no seed can make the executed rows discriminate. Filed as CORE-805.

[claude]

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

---

Review complete — looks correct and well-tested; 0 blocking, 1 medium, 3 low (posted as 3 inline comments)

Full review

• Gather context (read diff, base at 528d755)
• Review TimeShiftState::get_for_symbol
• Review time_shift.rs / factory.rs rendering path
• Review optimizer.rs + physical base_filter.rs call sites
• Review tests & fixtures
• Post review

Verdict

The diagnosis holds up under reading. Both lookups (extract_date_range in optimizer.rs:498, filter-params in physical_plan/filter/base_filter.rs:28) probed dimensions_shifts with a name that cannot match the keys built in query_properties.rs:821 (ts.dimension.full_name(), the resolved cube member), and centralising that in get_for_symbol is the right shape. Two things I checked specifically and that come out clean:

• Node ordering. TimeShiftSqlNode is constructed outermost in dimension_processor (wrapping CalendarTimeShift → TimeDimensionNode → RenderReferencesSqlNode), so the substituted-dimension fallback yields "col" + interval rather than shifting an expression that then gets replaced. Correct by construction.
• No double shift. The ungated first probe is exact-name, and only the substituted-gated probe resolves through the chain. An evaluated derived dimension therefore still gets its interval from the recursion into the owned member, once. The shifts_on_column == shifts assertion in the new test pins this, which is the right thing to assert.
• Granularity keys. TimeDimensionSymbol::full_name() is granularity-suffixed (time_dimension_symbol.rs:79), so the chain-end probe can miss when the chain lands on a TimeDimension; the find_owned_by_cube_child fallback covers it. The two probes genuinely complement each other.

member_name() being left alone is the right call and the reasoning in the PR body matches what its other callers do.

Findings

Sev	Where	What
Medium	time_shift_state.rs:28-35	get_for_symbol probes only the chain end and the deepest owned leaf, and swallows find_owned_by_cube_child's error with .ok()?. A derived time dim with >1 dependency (COALESCE({CUBE.a}, {CUBE.b})) gets no shift from the lookup while the renderer still shifts a/b — the same silently-unwidened-range bug, one shape over. Also skips shifts keyed on an intermediate derived level.
Low	time_shift.rs:55-66	interval: None went from unwrap() panic to a silent unshifted render. Better than panicking, but a named shift escaping extract_time_shifts (the else arm at time_shift_state.rs:87 inserts it verbatim) now yields silently wrong numbers. Consider an explicit error or a why-unreachable comment.
Low	time_shift.rs:55-66	The exact-name/resolving-name asymmetry between the two probes is the no-double-shift argument, but it's only recoverable from the PR description. One line of comment would carry it.
Low	factory.rs:289-297	TimeShiftSqlNode clones the whole RenderReferences map to call contains_key on it. A HashSet<String> (or Rc<…>) says "these names are columns" more directly.

Nothing on security (no new input handling, no user-controlled strings reaching SQL that weren't already there) and nothing on docs (internal planner fix, no user-facing surface).

Tests

Good shape: cube-level and view-level cases sit side by side, the derived-dimension test asserts the widened range and that every interval lands on the rollup column, and the seed deliberately holds a period before the queried range. Two small notes, neither worth blocking:

• sql.matches("interval '1 month'") is dialect-literal. It's the assertion that gives the test teeth, so I'd keep it, but it will break silently-usefully if the Postgres template ever changes its interval rendering.
• The stated CORE-805 limitation is accurate — with the harness building each rollup as one whole table, the widened range cannot change the executed rows, so the view case really is pinned by plan assertions only. Worth keeping that ticket visible; a partitioned-rollup fixture is what would close it.

What I could not verify

cargo is not in this job's allowlist, so I could not run cargo test -p cubesqlplanner or cargo clippy myself. The reported 1225-pass / 1216-pass integration run and the pre-existing switch_rolling failures are taken from the PR body, unverified here. Re-run locally (or grant Bash(cargo test:*)) if independent confirmation matters.

• branch [`tesseract-time-shift-view-preagg-date-range`](https://github.com/cube-js/cube/tree/tesseract-time-shift-view-preagg-date-range)

[claude]

TimeShiftSqlNode only ever calls contains_key on this map, but it takes a full RenderReferences clone (a HashMap<String, RenderReferencesType>) — the third clone of the same map in this function (lines 204, 269, 292). Cheap in absolute terms, but it also couples the shift node to a rendering-substitution type it doesn't render from. A HashSet<String> of substituted names (or Rc<RenderReferences>) would express "these names are columns, not expressions" more directly and drop the copy.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 79.58%. Comparing base (528d755) to head (628e116).
⚠️ Report is 47 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff            @@
##           master   #11599    +/-   ##
========================================
  Coverage   79.58%   79.58%            
========================================
  Files         480      485     +5     
  Lines       99544   100011   +467     
  Branches     3636     3675    +39     
========================================
+ Hits        79224    79596   +372     
- Misses      19801    19868    +67     
- Partials      519      547    +28     

Flag	Coverage Δ
cube-backend	59.33% <ø> (+0.05%)	⬆️
cubesql	84.09% <ø> (+0.04%)	⬆️

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 3m 15s —— View job

---

Re-review of the 2 new commits — both prior blocking-ish concerns addressed; 1 new medium (posted inline), 2 low notes

Full review

• Fetch base, diff 2f86135..a9078f4
• Review fail loudly on a time shift with no interval (f5744b6)
• Review skip a rollup that cannot carry the query's time shift (a9078f4)
• Re-check prior unresolved threads against current code
• Post review

What changed since my last pass

f5744b6 — no-interval shift now errors. Some(shift) with interval == None returns CubeError::internal naming the dimension instead of rendering unshifted. That's what I asked for: this node is the last place that could notice, and the previous silent fallthrough turned an escaped named shift into wrong numbers. The added comment above the two probes also records the exact-name/resolving asymmetry that the no-double-shift argument rests on. Both halves of my time_shift.rs thread are closed — resolved it.

a9078f4 — unattributable shift now skips the rollup. has_shift_under walks the whole dependency tree probing full_name() at each level, so it answers "is a shift involved anywhere below this column", while get_for_symbol answers "can I attribute one to this column". Where they disagree, the pre-aggregation is skipped and the query falls back to base SQL. This is a better answer than the "walk every level in get_for_symbol" I suggested: it fixes the failure mode rather than widening the lookup, and it covers both shapes I raised —

• multi-dependency derived dimension (COALESCE({CUBE.a}, {CUBE.b}), shift on one branch) — find_owned_by_cube_child errs → skip;
• shift keyed on an intermediate derived level — has_shift_under sees it via recursion, get_for_symbol can't attribute it → skip.

Both now degrade to "correct but slower" instead of "silently wrong". Resolved that thread too. The reasoning comment on can_carry_time_shifts is unusually good — it states why the two predicates are allowed to disagree, which is the non-obvious part.

The new test asserts pre_aggrs.is_empty() with the offending name in the failure message, plus a base-SQL value snapshot. Right assertion for a "must not match" test.

New finding

Sev	Where	What
Medium	optimizer.rs:483-503	can_carry_time_shifts scans only time_dimensions, but make_pre_aggregation_source substitutes dimensions as columns too (optimizer.rs:406-412). A time-type dimension under a rollup's dimensions: that depends on a shifted member bypasses the guard, so the shifted leaf offsets one column while reading another computed from the unshifted value — same inconsistency the guard exists to prevent. Posted inline with a fixture and a two-line patch.

Low notes (no inline comment)

• has_shift_under has no memo or visited set. It re-walks shared sub-dependencies, so a diamond-shaped member graph is exponential in depth. Depths are tiny in practice and it runs once per pre-aggregation candidate, so this is a note, not a request — but if member graphs ever get deep it's the kind of thing that shows up as a mystery planning stall.
• Shift interval vs. stored granularity. The derived test confirms the interval lands on "pa_customers__return_day_month" — the granularity-truncated column. That commutes for a whole-month shift, but a sub-granularity interval (say 7 day against a month rollup) would give month_start + 7 days from the rollup where base SQL gives date_trunc('month', day + 7 days), so the stage join keys wouldn't line up. As far as I can tell this predates the PR (a plain shifted time dimension read from a rollup already offset the granularity column), so I'm recording it rather than attributing it — worth confirming that rollup matching rejects a shift that isn't a multiple of the stored granularity.

Re-affirmed prior thread

factory.rs:297 — TimeShiftSqlNode still takes a full RenderReferences clone to call contains_key. Unchanged, so the thread stays open; not reposting.

What I could not verify

cargo isn't in this job's allowlist, so cargo test -p cubesqlplanner / clippy were not run here. The PR body's counts are taken as reported. One thing worth a local check: can_carry_time_shifts sits inside the per-candidate loop, so it also runs for the whole-query rewrite path where time_shifts is TimeShiftState::default() — that early-returns true, so no existing rollup should lose its match, but a full suite run is the real confirmation.

• branch [`tesseract-time-shift-view-preagg-date-range`](https://github.com/cube-js/cube/tree/tesseract-time-shift-view-preagg-date-range)

[claude]

Claude finished @waralexrom's task in 2m 14s —— View job

---

Re-review of 628e116 — prior medium addressed (and widened past what I asked); 1 new low-medium posted inline, 1 prior thread re-affirmed

Full review

• Diff a9078f4..628e116
• Review the widened can_carry_time_shifts
• Check the remaining member kinds (measures) against the same reasoning
• Review new fixture, seed change, and the two tests
• Resolve stale threads / dedupe against prior comments
• Post review

What changed

628e116 extends can_carry_time_shifts from time_dimensions to time_dimensions ∪ dimensions ∪ segments. That's what my optimizer.rs:508 thread asked for, plus segments — which I'd missed and which is right for the same reason: make_pre_aggregation_source chains all three into the schema (optimizer.rs:406-412), and factory.rs:199-206 short-circuits segment wrapping precisely because a stored segment is already a column. A segment built over a partially shifted dimension is a boolean computed from unshifted values; no offset recovers it. Resolved that thread.

The fixture additions are the right shape. Adding batch_at as an unshifted time dimension is the key move: it makes the rollup's time dimension clean so the only thing that can reject the candidate is the stored dimensions:/segments: entry. Without that, the test would have been passing for the old reason and proved nothing. Both new tests assert pre_aggrs.is_empty() with the offending names in the message, and the base-SQL snapshots show the discriminating behaviour — row 3 (happened_at IS NULL) staying in February while every other row moves, which is exactly what no column offset can reproduce.

New finding

Sev	Where	What
Low-Medium	optimizer.rs:501-506	The comment now claims "every member the pre-aggregation stores is checked", but measures is still outside the chain. A measure whose SQL references the shifted time dimension (sql: "CASE WHEN {CUBE.happened_at} >= … THEN val END") re-enters the dimension branch of RootSqlNode when expanded, so base SQL shifts it while the rollup returns a column aggregated from unshifted values. has_shift_under/get_for_symbol already answer correctly for a measure symbol; only the iteration misses it. Posted inline with the one-line patch and two caveats (it over-rejects relative to the dimension case, and pre_aggregation.measures is the unfiltered list). Excluding measures is a defensible scope call — but then the comment should say so.

Notes carried forward (not re-posted)

• factory.rs:297 — TimeShiftSqlNode still clones the whole RenderReferences map to call contains_key. Thread left open, unchanged.
• has_shift_under has no visited set. Now called up to 3× more members per candidate, so the re-walk of shared sub-dependencies got proportionally wider. Still tiny in practice (depths are small, once per candidate), still a note rather than a request — but the constant factor moved in the wrong direction, so if member graphs ever get deep this is where it shows.
• Shift interval vs. stored granularity. Unchanged from my last pass and predates this PR: a sub-granularity interval against a coarser rollup gives month_start + 7 days where base SQL gives date_trunc('month', day + 7 days). Worth confirming separately that matching rejects a shift that isn't a multiple of the stored granularity.

What I could not verify

cargo is not in this job's allowlist, so cargo test -p cubesqlplanner and clippy were not run here. Two things I'd specifically want from a local run:

• That the widened guard doesn't cost existing rollup matches. dimensions and segments are far more commonly populated than a rollup's time_dimension, so this commit has more blast radius than the previous one — any rollup carrying a derived dimension whose graph touches a shifted member now falls back to base SQL. has_shift_under returning false for the overwhelming majority should keep that at zero, but the suite is the confirmation.
• That both new tests are red without the commit. The batch_at addition strongly suggests they were written that way, but I can't observe it.

• branch tesseract-time-shift-view-preagg-date-range •

[claude]

The comment now says "every member the pre-aggregation stores is checked", but measures is still outside the chain — and a measure column is substituted the same way (pre_aggregation_measures_references, wired at factory.rs:227 / final_measure_node_processor).

The dimension branch of RootSqlNode is where TimeShiftSqlNode lives, and a measure's SQL that references a time dimension re-enters that branch when evaluate_sql expands it. So for

measures:
  - name: total_recent
    type: sum
    sql: "CASE WHEN {CUBE.happened_at} >= '2024-02-01' THEN val END"

base SQL computes the case over happened_at + interval '1 month', while the rollup returns a column aggregated from the unshifted happened_at — the same read-unshifted-under-a-shift inconsistency the guard exists to reject, one member kind over. has_shift_under(total_recent) is true (recursion reaches happened_at) and get_for_symbol(total_recent) is None (a measure is neither the chain end nor an owned time child), so the existing predicate already answers correctly; only the iteration misses it:

    .chain(pre_aggregation.segments.iter())
    .chain(pre_aggregation.measures.iter())

Two caveats worth weighing before taking it:

• This is strictly more conservative than the dimension/segment case. A measure whose dependency graph merely touches the shifted dimension without its value depending on it (e.g. a filters: clause the rollup already applied at build time) would now also be rejected — correct, but it costs rollup hits.
• pre_aggregation.measures here is the unfiltered list; make_pre_aggregation_source narrows to matched_measures afterwards, so gating on the full list rejects on measures the query never reads. Gating on the matched set would be tighter, but can_carry_time_shifts runs before matching, so that's a larger change — the unfiltered list is the conservative-but-simple option.

Given the other two kinds are now covered, leaving measures out is a defensible scope call; if you do, a line in the comment saying measures are knowingly excluded (and why) would keep the "every member" claim honest.

Fix this →