fix(tesseract): compose grain.include with rolling_window - #11639
fix(tesseract): compose grain.include with rolling_window#11639waralexrom wants to merge 4 commits into
Conversation
A multi-stage measure declaring both `grain.include` and `rolling_window` computed its inner stage at the query's granularity instead of the declared one, and lost its outer aggregation entirely when the query carried no time dimension. Nothing failed: the model compiled and the query returned a plausible wrong number. Two independent losses in the rolling path. The base member is built by unrolling the measure, which drops the multi-stage properties, so the recursion that plans the base CTE never saw the grain — extend the base rolling state with `grain.include` instead. And with no time dimension the rolling description is returned directly, so no outer stage carries the aggregation, while unrolling also collapses the aggregate kind to a calculated one; plan such a measure as its plain multi-stage self, which is what a window resolving to a single bucket computes. `grain.exclude` / `grain.keep_only` next to a rolling window were silently ignored the same way. Narrowing the reported grain needs the narrowed value broadcast back onto the query grid, which the rolling assembly has no side to do, so they are now rejected with a message rather than quietly dropped. `QueryProperties::add_dimensions` deduplicated only within `dimensions`, so a `grain.include` naming a granularity the query already groups by rendered the same column twice and the query failed as ambiguous. A time dimension's full name pins its granularity, so deduplicating added dimensions against the time dimensions by full name is exact. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…error `reduce_by` and `group_by` compile into the same lists as `grain.exclude` and `grain.keep_only`, so a model written in the older spelling hits the rejection while the message names keys that appear nowhere in it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`keep_only` is an intersection, so an empty list narrows the grain to nothing rather than meaning no key was given — `partition_filter` collapses on it. The rolling-window guard tested for a non-empty list and let it through, so the measure was computed at the unnarrowed grain and reported as if the key had been honoured. `exclude` subtracts and an empty list there really is a no-op, so only `keep_only` changes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
github-actions
Bot
added
rust
|
Claude finished @waralexrom's task in 6m 15s —— View job Review: no blockers — 0 high, 2 medium, 3 low. 4 inline comments posted.Full review
What I verified rather than took on trustThe three load-bearing claims in the description all hold:
I also swept the repo for models that combine Medium
Low
Coverage gaps (minor)Not requested, but the cheap additions: PraiseThe failure mode here — compiles clean, Not verifiedI could not run |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| pub fn add_dimensions(&mut self, dimensions: Vec<Rc<MemberSymbol>>) { | ||
| let time_dimension_names = self | ||
| .time_dimensions | ||
| .iter() | ||
| .map(|d| d.clone().resolve_reference_chain().full_name()) | ||
| .collect::<HashSet<_>>(); | ||
| let added = dimensions.into_iter().filter(|d| { | ||
| !time_dimension_names.contains(&d.clone().resolve_reference_chain().full_name()) | ||
| }); | ||
| self.dimensions = self | ||
| .dimensions | ||
| .iter() | ||
| .cloned() | ||
| .chain(dimensions.into_iter()) | ||
| .chain(added) | ||
| .unique_by(|d| d.clone().resolve_reference_chain().full_name()) |
There was a problem hiding this comment.
The dedup itself is sound — TimeDimensionSymbol::full_name() is format!("{}_{}", base.full_name(), granularity_or_day) (time_dimension_symbol.rs:79), and QueryProperties.time_dimensions only ever holds granular entries (query_properties_compiler.rs:66 filters the rest out), so a match really does mean "the same column under the same alias is already grouped".
Two things worth noting for the general-purpose setter:
-
This is now a silently dropping
add_dimensions, and the other caller is the non-rolling multi-stage path (multi_stage_query_planner.rs:677). There it runs afterset_time_dimensions(partition_filter(...)), so ordering is correct — a time dim removed bykeep_only/excludeis no longer intime_dimensionsand theincludere-adds it. That ordering dependency is load-bearing and invisible from here; a one-line note in the doc comment ("callers must apply grain narrowing totime_dimensionsbefore adding") would pin it. -
The asymmetry the doc doesn't spell out: a bare
grain.include: [returns.day]resolves toreturns.day, never toreturns.day_day, so it is never deduped against a query granularity — it adds the full-resolution column as an extra key. That's pre-existing grain semantics, but it is the reason the two fixture measures (include: returns.dayvsinclude: returns.day.day) take different code paths here, and the fixture data can't tell them apart (see my note on the seed).
| @@ -0,0 +1,16 @@ | |||
| CREATE TABLE grain_returns ( | |||
| day DATE NOT NULL, | |||
There was a problem hiding this comment.
day DATE makes grain.include: [returns.day] and grain.include: [returns.day.day] indistinguishable in the data: the raw column and its day truncation are the same value, so both fixture variants exercise the same grouping even though they take different paths through add_dimensions (raw name never dedups against returns.day_day; the qualified one always does).
The reported model almost certainly has a TIMESTAMP. Making this column TIMESTAMP with two different times inside one day would (a) keep the same expected -0.34, since the daily factor is unchanged, and (b) actually distinguish the two grain spellings — the raw include would compute a log return per timestamp and give a different number, which is the semantics a reader of these two fixtures would want confirmed one way or the other.
| fn parse_table(result: &str) -> (Vec<String>, Vec<Vec<String>>) { | ||
| let mut lines = result.lines(); | ||
| let header = lines | ||
| .next() | ||
| .expect("result has no header") | ||
| .split('|') | ||
| .map(|c| c.trim().to_string()) | ||
| .collect::<Vec<_>>(); | ||
| lines.next(); | ||
| let rows = lines | ||
| .filter(|l| !l.trim().is_empty()) | ||
| .map(|l| l.split('|').map(|c| c.trim().to_string()).collect()) | ||
| .collect(); | ||
| (header, rows) | ||
| } |
There was a problem hiding this comment.
Third hand-rolled pipe-table parser in this test tree — tests/integration/cubestore/switch_rolling.rs:146 has its own, with a comment about the separator-vs-negative-first-cell ambiguity that this one sidesteps by skipping line 2 positionally. Worth hoisting a shared parse_table (and ideally assert_measures, which is the more useful of the two) into test_fixtures/test_utils so the next value-asserting integration test doesn't write a fourth.
Not blocking — value-based assertions against Postgres are exactly the right choice for this fix, and much stronger than the SQL-shape snapshots these bugs slipped through.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #11639 +/- ##
===========================================
+ Coverage 59.34% 79.97% +20.63%
===========================================
Files 228 489 +261
Lines 18222 102701 +84479
Branches 3675 3676 +1
===========================================
+ Hits 10814 82138 +71324
- Misses 6861 20012 +13151
- Partials 547 551 +4
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:
|
The rejection fired on the keys being declared, so it also refused queries the keys never touch: `exclude` of a member the grain does not carry subtracts nothing, and `keep_only` listing everything the query groups by intersects to the same list. Those queries were answered correctly before the guard existed, and the answer is the one the measure would give with no keys at all. Reject the narrowing actually happening instead — compare the reshaped grain with the one it came from, since `partition_filter` only ever removes. The check also moves below the no-time-dimension branch. There the window has no frame to build and the measure is planned through the ordinary multi-stage path, which narrows the grain and broadcasts it back the usual way, so the combination is supported and no longer refused. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @waralexrom's task in 3m 21s —— View job Re-review of
|
| let narrows = Self::partition_filter(state.dimensions(), &grain).len() | ||
| != state.dimensions().len() | ||
| || Self::partition_filter(state.time_dimensions(), &grain).len() | ||
| != state.time_dimensions().len(); |
There was a problem hiding this comment.
The predicate is the right one, and for a reason worth recording: the plain multi-stage path decides whether to build the broadcast side with exactly this test — any_missing at line 707 compares new_state (post-partition_filter) against state and only then requests keys_input. So "a narrowing that removes nothing needs no broadcast" is not a new claim here; it's the same rule the non-rolling path already runs on. Rejecting narrows rather than "keys declared" makes the two paths agree, which is stronger than the previous version.
Two small things:
-
Short-circuit the common case. For every rolling measure with no narrowing keys at all — the overwhelming majority — this still clones both dimension vectors twice to compare lengths.
if grain.exclude.is_some() || grain.keep_only.is_some()in front makes the guard free when there is nothing to check, and reads as "only measures that declare narrowing are examined". -
statevsself.query_properties. The guard evaluates againststate, while thetime_dimensionsvec that decides theis_empty()branch and the frame above is built fromself.query_properties. For a rolling measure nested under a parent multi-stage that already narrowed the grain, those diverge:state.time_dimensions()can be empty whilequery_propertiesstill has the time dim, so the frame is built but the guard sees nothing to narrow. Checking againststateis the more defensible of the two (narrowing is relative to the grid this stage is handed), and the divergence in the frame construction is pre-existing — but the mixed use inside one function is worth a word, since the next reader will assume both sides look at the same list.
| sql: "EXP({CUBE.log_return_sum_day_granularity}) - 1" | ||
| type: number | ||
|
|
||
| # Partition-shaping grain keys have no meaning next to a window frame; |
There was a problem hiding this comment.
This comment went stale with the latest commit: partition-shaping keys do have a meaning next to a window now in two cases — when the narrowing is inert on the query's grain (weight_ytd_reduce_security, right below) and when the query carries no time dimension at all (test_rolling_narrowing_without_time_dimension). What this measure demonstrates is narrowing that actually removes a key the query groups by, which is the case with no broadcast side.
Something like "narrowing the grain the value is reported at has no answer next to a window frame — the planner must say so rather than quietly ignoring the key" keeps the intent and stays true of the three rejection fixtures specifically.
1 participant
Summary
A multi-stage measure declaring both
grainandrolling_windowsilently ignored the declared grain: the inner stage was grouped by the query's granularity instead, and with no time dimension in the query the outer aggregation was dropped entirely. Nothing failed — the model compiled,/metawas clean, and the query returned a plausible wrong number. Reported as CORE-789, reproduced standalone against Postgres.The reported model computes a time-weighted return: a per-day log return summed into a year-to-date window. Only a query asking for the time dimension exactly at the declared grain was correct;
granularity: month, a KPI tile with no time dimension, and a group-by on a non-time dimension all returned-0.0667instead of-0.34— a single factor over a fully collapsed aggregate, never linked by day.Changes
grain.includenow reaches the rolling base. The base member is built by unrolling the measure, which drops its multi-stage properties, so the recursion planning the base CTE never saw the grain. The base rolling state is extended withgrain.includeaftermake_rolling_base_state; the frame still keys off the base time dimension, so the extra grain only splits rows the outer aggregation merges back.The no-time-dimension case keeps its aggregation. That branch returns the rolling description directly — no
RollingWindownode, hence noRollingMergerender modifier, hence no outer stage to carry the aggregation — while unrolling also collapses the aggregate kind to a calculated one, sotype: sumvanished too. Newstrip_rolling_windowtransform drops only the window, and the measure is planned as its plain multi-stage self, which is what a window resolving to a single bucket computes.grain.exclude/grain.keep_onlyare honoured where they can be and rejected where they cannot. They were silently ignored next to a window. With no time dimension there is no frame to build, so the measure goes through the ordinary multi-stage path, which narrows the grain and broadcasts it back — that combination now works. With a time dimension the narrowed value would have to be broadcast onto the query grid and the rolling node has no side enumerating it, so the query is refused.The refusal is on the narrowing happening, not on the keys being declared:
excludeof a member the grain does not carry subtracts nothing, andkeep_onlylisting everything the query groups by intersects to the same list. Such queries answer exactly as a measure with no keys would, and are left alone. The message names the olderreduce_by/group_byspellings too, since those compile into the same lists and a model may contain neither of thegrain.*words. An emptykeep_onlydoes narrow — it is an intersection, sogroup_by: []collapses the grain to a grand total rather than meaning "no key given".QueryProperties::add_dimensionsdeduplicates against time dimensions. It deduplicated only withindimensions, so agrain.includenaming a granularity the query already groups by rendered the same column twice and the query failed as ambiguous. This was broken on the non-rolling path too. A time dimension'sfull_namepins its granularity, so the check is exact — a barereturns.dayandreturns.day.daystay distinct grains.Testing
Both suites were shown failing on the pre-fix code before the fix was written.
Rust —
tests/integration/multi_stage/rolling_window_grain.rs, 11 tests asserting values against Postgres, not SQL shape: all four query forms from the report (granularity: day,granularity: month, no time dimension, group-by on a non-time dimension), a granularity-qualifiedgrain.include, the ambiguous-column regression, narrowing honoured without a time dimension, two inert narrowing keys answering as a keyless measure, and the three rejection paths (keep_only,reduce_by, emptykeep_only). Before the fix, 3 of the 4 value tests failed with exactly the reported-0.0667. Full suite: 1283 passed, 0 failed.JS —
packages/cubejs-schema-compiler/test/integration/postgres/multi-stage-grain-rolling-window.test.ts, the same four forms on the same model. Verified failing by rebuilding the native addon from the reverted Rust sources: 3 of 4 failed with-0.066667. Also ranmulti-stage*,postgres-cumulative-measures,rolling-window-offset-no-granularity,bucketing,calc-groups,calendars,custom-granularities,cube-views,member-expression,multiple-join-paths,pre-aggregations*,sql-generation(135 tests),sql-generation-logicandyaml-compilerunderCUBEJS_TESSERACT_SQL_PLANNER=true— all green.On the legacy planner as an oracle: it is not one here. Under
CUBEJS_TESSERACT_SQL_PLANNER=falseall four forms are wrong including the control measure that declares no window (-0.20vs-0.34) — the v1 planner does not implementgrain:at all, which is why the existingmulti-stage-grain.test.tsskips every case on it. The new JS test follows that convention.Known gaps, deliberately not addressed here
grain.exclude/grain.keep_onlyalongside a time dimension needs a broadcast side onMultiStageRollingWindow(the node has nokeys_inputfield and its processor never reads one), or a window wrapper mirroringMultiStageCalculationWindowFunction::Window. A prototype got most of the way — narrowing the rolling description's state plus filtering the keys UNION to refs covering every key dim — but a narrowed measure queried alone still has no source for the grid. Left for a follow-up rather than shipped half-working.to_datewindow with no time dimension never bounds the date range:replace_date_range_for_rolling_window_without_granularityreturns early when bothtrailingandleadingareNone. AdateRangespanning two years is reported as year-to-date. Pre-existing and shared with plain non-multi-stage rolling measures — verified identical there — so changing it is its own decision.