fix(tesseract): keep time_shift when a pre-aggregation serves the query - #11599
Conversation
A multi_stage measure with time_shift returned NULL for every row when queried through a view while a pre-aggregation was matched. The shifted leaf scanned a partition set that could not contain its rows. Time shifts are keyed by the fully resolved cube member: QueryProperties builds them from all_time_members(), which peels the TimeDimension wrapper and follows the reference chain. extract_date_range probed that map with BaseFilter::member_name(), which resolves neither, so a view-qualified filter never found its shift and the range was left un-widened. Add TimeShiftState::get_for_symbol, which normalizes the probe the same way the keys are built, and route the lookup sites through it. Preferred over a fallback second lookup so the key-normalization rule lives in one place instead of being re-derived per call site; member_name() is left alone because its other callers compare against query-level names, where view qualification is consistent. TimeShiftSqlNode keeps its own probe: it is guarded on a non-reference symbol, and resolving there would apply the shift twice. Covered by a view-level test sitting next to the existing cube-level one, asserting the shifted and unshifted usages carry different date ranges. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…m a rollup A time_shift declared on a time dimension that wraps another cube's time dimension was lost entirely once a pre-aggregation served the query: the shifted leaf read the same rows as the unshifted one, so the shifted measure silently repeated the current period instead of the previous one. The same query without a pre-aggregation was correct. Dimension-specific shifts are keyed by the owned member the declared dimension wraps, because that is where the interval lands when the member's SQL is expanded. Two things then went wrong when the rollup materialized the derived dimension instead: - extract_date_range probed only the chain-resolved name, so the range was never widened. get_for_symbol now probes the owned child too, covering both ways a key is built. - The rollup column is substituted for the dimension, so its SQL is never expanded and the recursion that normally carries the shift down to the owned member never happens. TimeShiftSqlNode now applies the shift to the column itself, but only for dimensions it knows are substituted — an evaluated dimension must still wait for the recursion, or the interval would be added twice. Covered by a test asserting the widened range, a single shift on the rollup column, and — on a seed holding a period before the queried range — the executed values. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…review Fall through instead of unwrapping a shift with no interval, so TimeShiftSqlNode treats it as "no shift" like the other two consumers of the same lookup rather than panicking. Assert the single-application invariant in the derived-dimension test by requiring every rendered interval to sit directly on the rollup column, instead of matching one exact textual form of a doubled shift. Record what the view test's executed rows do and do not cover: the widened range only selects rollup partitions, which the harness does not emulate — it loads each rollup whole — so only the assertions on the usages guard the widening. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
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
VerdictThe diagnosis holds up under reading. Both lookups (
Findings
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). TestsGood 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:
What I could not verify
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| let input = if !self.time_shifts.is_empty() { | ||
| TimeShiftSqlNode::new(self.time_shifts.clone(), input) | ||
| TimeShiftSqlNode::new( | ||
| self.time_shifts.clone(), | ||
| self.pre_aggregation_dimensions_references.clone(), | ||
| input, | ||
| ) | ||
| } else { | ||
| input | ||
| }; |
There was a problem hiding this comment.
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 Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #11599 +/- ##
========================================
Coverage 79.58% 79.59%
========================================
Files 480 485 +5
Lines 99544 100011 +467
Branches 3636 3675 +39
========================================
+ Hits 79224 79599 +375
- Misses 19801 19865 +64
- Partials 519 547 +28
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 shift entry reaching the renderer without an interval was rendered unshifted, turning a state the map calls shifted into silently wrong numbers. Return an error instead. Also record why the first probe is by exact name: a dimension that gets evaluated picks its shift up when the recursion reaches the owned member it wraps, so matching it at the outer level too would add the interval twice. Only a substituted dimension, never expanded, resolves through the chain. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A time dimension built from several members, only some of which the shift covers, has no valid offset of its stored column: moving the column would carry along the rows the shift must leave in place. The rollup was matched anyway and the shift was dropped, so the shifted measure silently repeated the unshifted one. Reject such a pre-aggregation during matching. The unrewritten leaf then triggers the existing rollback of the whole multi-stage rewrite and the query falls back to base SQL, which computes the shift correctly. The gate is tied to the shift lookup rather than re-deriving reachability: reject exactly when a shift is involved but cannot be attributed to the stored column. Re-deriving the rule would add a second place obliged to stay in step with the lookup. The test's expected values were captured from the same query with pre-aggregations disabled, before the gate existed. They differ from what offsetting the stored column would produce, which is what rules that approach out. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
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 notesFull review
What changed since my last pass
Both now degrade to "correct but slower" instead of "silently wrong". Resolved that thread too. The reasoning comment on The new test asserts New finding
Low notes (no inline comment)
Re-affirmed prior thread
What I could not verify
|
… shift The gate scanned only a pre-aggregation's time dimensions, but dimensions and segments are substituted by column just the same. A dimension built from a partially shifted member slipped through, and the shifted leaf then read it computed from unshifted values: both leaves rendered identically, so the shifted measure repeated the unshifted one. Check every member the pre-aggregation stores. The type is not what matters — any stored column computed from a shifted member is wrong when read unshifted — so dimensions and segments are checked whatever they hold. Tests cover all three ways such a member reaches a rollup: as its time dimension, under dimensions, and through a segment. Each was confirmed to fail with its own part of the gate removed. Their snapshots pin what makes the stored column unusable: the row the shift leaves in place lands on the same key in both stages, which no offset of a single column reproduces. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @waralexrom's task in 2m 14s —— View job Re-review of
|
| 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—TimeShiftSqlNodestill clones the wholeRenderReferencesmap to callcontains_key. Thread left open, unchanged.has_shift_underhas 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 dayswhere base SQL givesdate_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.
dimensionsandsegmentsare far more commonly populated than a rollup'stime_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_underreturningfalsefor 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_ataddition strongly suggests they were written that way, but I can't observe it.
• branch tesseract-time-shift-view-preagg-date-range •
Three ways the gate reached the wrong verdict, each confirmed by comparing the rollup plan against the same query without one. A measure was not examined at all. One whose SQL reads the shifted dimension is stored aggregated from unshifted values, and no offset recovers it, because a shift changes which rows feed an aggregate rather than the value itself. Measures are therefore rejected outright whenever a shift reaches them, never merely attributed — attribution succeeds for a measure with a single dependency and would have admitted exactly the broken case. Only the measures the match consumes are examined. A stored reference to the shifted dimension was admitted although the renderer resolves a reference through to what it points at instead of offsetting the column, leaving it unshifted. The gate asked whether a shift could be attributed while the renderer asked whether it would apply one; the two are now the same question, asked through `shift_for_substituted_column`, so they cannot disagree again. A member the query never reads could reject the whole rollup. Nothing renders such a column, so it cannot make the stored data wrong; the gate now looks only at the members the node actually reads, matching how measures were already treated. Tests cover all five ways a shift reaches a rollup — time dimension, dimension, segment, measure, reference — plus the case that must stay matched. Each was confirmed to flip with its own part of the gate removed, and the kept-rollup values were checked against a base-SQL run rather than against the rollup that produced them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A FILTER_PARAMS column bound to a time dimension derived from another cube's is offset in the shifted stage and left bare in the unshifted one. Both forms are asserted, so losing the offset and applying a spurious one are equally caught. Without the shift lookup resolving through the derivation, the shifted stage filtered the source rows by unshifted bounds while grouping by shifted values. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @waralexrom's task in 4m 20s —— View job Re-review of
|
| Sev | Where | What |
|---|---|---|
| Low | optimizer.rs:516 |
Inserting read_member_names between the rationale comment and can_carry_time_shifts re-parented all three paragraphs onto the wrong function — the one they explain now has no comment at all, 30 lines down. read_member_names' own one-liner is also glued to the measures paragraph with no blank line. Plus a dropped word: Only the measures matching consumed are examined. |
| Low | optimizer.rs:186 |
read_member_names is an eager argument, so it runs before can_carry_time_shifts reaches its is_empty() early return — full symbol resolution and a String per member, discarded on every no-shift match (including the whole-query path at line 104, which always passes TimeShiftState::default()). Suggestion inline. |
| Low | integration_multi_dep_time_dim_shift_pre_agg.yaml:19-23 |
happened_at_ref was inserted between effective_at's comment and effective_at, so "built from two owned time dimensions" now sits directly above a single-dependency plain reference. Suggestion inline. |
One more note, no comment posted: read_member_names omits filter.measures_filter. Harmless as far as I can tell — a HAVING measure reaches the gate through matched_measures instead, and a dimension read only through a measure filter isn't a shape I can construct — but the omission is silent, and the three lists it does chain make it look exhaustive.
Prior threads
- Resolved
optimizer.rs:562(measures excluded from the gate) — addressed bye7e3970. - Still open, not reposted:
factory.rs:297,TimeShiftSqlNodetaking a fullRenderReferencesclone to callcontains_key. Unchanged. - Carried forward:
has_shift_understill has no visited set (now also called over matched measures, so a little wider); and the sub-granularity-interval-vs-stored-granularity question from my second pass, which predates this PR and is worth confirming separately.
Tests
The three new integration tests are well chosen — two rejections and, importantly, one acceptance (..._keeps_rollup_when_unshiftable_member_unused) that pins the new permissiveness with assert_eq!(pre_aggrs.len(), 2). Worth noting for the record that this test's value snapshot cannot discriminate: the shift is on happened_at, nothing in the query reads it, so total_prev_month == total on every row whether or not the rollup is used. The count assertion is the whole test. That's the correct assertion for the claim, just don't read the snapshot as independent confirmation.
The ..._on_measure_reading_shifted_dimension snapshot does discriminate nicely — row 1 NULL, row 2 NULL/2000.00 — values a column aggregated from unshifted rows can't reproduce.
What I could not verify
cargo isn't in this job's allowlist, so cargo test -p cubesqlplanner and clippy were not run here; the PR body's counts are taken as reported. The one thing I'd want from a local run is that ..._keeps_rollup_when_unshiftable_member_unused is red without e7e3970 — it's the only new test asserting a match, so it's the only one that could pass for the wrong reason.
| // matching consumed are examined, since the rest are never read. | ||
| // Resolved names of every member the query reads, so a stored member no | ||
| // one reads cannot decide anything. | ||
| fn read_member_names(schema: &LogicalSchema, filter: &LogicalFilter) -> HashSet<String> { |
There was a problem hiding this comment.
Inserting read_member_names here re-parented the whole rationale block. All three paragraphs — "A stored member is shifted by offsetting its column…", the grouping-members paragraph, and the measures paragraph — now read as the doc comment for read_member_names, which is a name-collection helper they say nothing about. can_carry_time_shifts, the function they actually explain, is now 30 lines below with no comment of its own. The one-line doc that does belong to read_member_names ("Resolved names of every member the query reads…") is glued to the end of the measures paragraph with no blank line, so it reads as a continuation of it.
Moving read_member_names below can_carry_time_shifts (or the comment block down onto it) restores the pairing. Also, Only the measures matching consumed are examined looks like a dropped word — reads as if matching is the noun; Only the measures matching actually consumed them or Only the measures the match consumed are examined is presumably the intent.
| if !Self::can_carry_time_shifts( | ||
| pre_aggregation, | ||
| &matched_measures, | ||
| &Self::read_member_names(&query.schema(), &query.filter()), |
There was a problem hiding this comment.
read_member_names is an eagerly-evaluated argument, so it runs before can_carry_time_shifts gets to its time_shifts.is_empty() early return. Every successful pre-aggregation match therefore walks the full schema plus every filter's member evaluators, calls resolve_base_symbol().resolve_reference_chain() on each, and allocates a String per member into a fresh HashSet — then discards all of it, because time_shifts is empty for essentially every non-multi-stage query and for the whole-query rewrite path at line 104 (&TimeShiftState::default()).
Not hot enough to be a real problem (once per match, not per candidate), but it's free to avoid:
| &Self::read_member_names(&query.schema(), &query.filter()), | |
| if !time_shifts.is_empty() | |
| && !Self::can_carry_time_shifts( | |
| pre_aggregation, | |
| &matched_measures, | |
| &Self::read_member_names(&query.schema(), &query.filter()), | |
| time_shifts, | |
| ) | |
| { |
(the is_empty() check inside can_carry_time_shifts can stay as the invariant for direct callers).
| # Time dimension built from two owned time dimensions. The shift | ||
| # below covers only one of them, so the stored rollup column | ||
| # cannot represent the shifted values. | ||
| # Plain reference to the shifted dimension. Rendered through to what | ||
| # it points at, so a stored copy of it is never offset. |
There was a problem hiding this comment.
happened_at_ref was inserted between effective_at's comment and effective_at, so the two comments are now stacked and the first one describes the dimension two entries below it. A reader hitting line 19 reads "built from two owned time dimensions" immediately above a single-dependency plain reference.
| # Time dimension built from two owned time dimensions. The shift | |
| # below covers only one of them, so the stored rollup column | |
| # cannot represent the shifted values. | |
| # Plain reference to the shifted dimension. Rendered through to what | |
| # it points at, so a stored copy of it is never offset. | |
| # Plain reference to the shifted dimension. Rendered through to what | |
| # it points at, so a stored copy of it is never offset. | |
| - name: happened_at_ref | |
| type: time | |
| sql: "{CUBE.happened_at}" | |
| # Time dimension built from two owned time dimensions. The shift | |
| # below covers only one of them, so the stored rollup column | |
| # cannot represent the shifted values. |
2 participants
Summary
A
multi_stagemeasure withtime_shiftsilently 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:
NULL. The identical query against the cube was correct.Changes
TimeShiftStateand 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.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
cargo test -p cubesqlplanner: 1225 passed.--features integration-cubestore): 1216 passed. The 9switch_rollingfailures are pre-existing on this branch's base — verified against a baseline run with the fixes reverted — and come from the older releasedcubestoredbinary 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.