fix(tesseract): project the requested grain into rolling-window CTEs - #11400
Conversation
github-actions
Bot
added
rust
|
Claude finished @paveltiunov's task in 5m 12s —— View job Review complete — approach looks right; 1 medium correctness question, 2 test-coverage notesFull review
What's goodThe core change is small and well-argued. Sourcing the rolling-window CTE schema from
Blast radius is genuinely small: The fixture additions are scoped correctly — The commit message and the code comment both explain why rather than what, and the JS suite documents the two remaining matcher gaps precisely (join-equal time dimensions; Findings
Details and suggested diffs are in the three inline comments. Security / performanceNothing to raise. No user input reaches new SQL construction paths (the calc-group values are model-declared literals), no new I/O, and the change removes members from a projection rather than adding them — if anything the CTEs get narrower, not wider. DocsNo user-facing behaviour is added, so no Verification I could not run
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| let sql = ctx.build_sql(query).unwrap(); | ||
|
|
||
| // The pinned switch value renders as a literal, and the calc-group | ||
| // dimension is projected by the CTEs feeding the case measure. | ||
| assert!( | ||
| sql.contains("'R7'"), | ||
| "Expected the pinned calc-group value in the generated SQL:\n{}", | ||
| sql | ||
| ); | ||
| assert!( | ||
| sql.contains("orders__window_kind"), | ||
| "Expected the calc-group dimension to be projected:\n{}", | ||
| sql | ||
| ); |
There was a problem hiding this comment.
Every other test in this file follows build_sql with try_execute_pg + insta::assert_snapshot!, so it pins the actual values. This one stops at two substring assertions, which only prove planning no longer errors — the exact failure mode the fix guards against (a dimension missing from the CTE grain, so the rolling sum fans out over the time_series join) produces plausible SQL with wrong numbers, and neither contains("'R7'") nor contains("orders__window_kind") would notice.
Adding the execute + snapshot here would give the change real coverage rather than a smoke test:
if let Some(result) = ctx.try_execute_pg(query, SEED).await {
insta::assert_snapshot!(result);
}The JS spec does assert values, but only for the shared-calc-group Postgres model — the Rust-side switch/case shape this fixture adds stays value-unverified.
| it('loses both rollups for cross-cube measures when the joined rollup is keyed on its own time dimension', () => compiler.compile().then(() => { | ||
| const query = new PostgresQuery({ joinGraph, cubeEvaluator, compiler }, MONTH_QUERY); | ||
|
|
||
| const preAggregationsDescription: any = query.preAggregations?.preAggregationsDescription(); | ||
| const sqlAndParams = query.buildSqlAndParams(); | ||
| expect(preAggregationsDescription.map((d: any) => d.tableName)).toEqual([]); | ||
| expect(sqlAndParams[0]).not.toContain('sales_perf_rolling'); | ||
| expect(sqlAndParams[0]).not.toContain('share_metrics_perf_share'); | ||
| })); |
There was a problem hiding this comment.
This spec locks in the undesirable behaviour (toEqual([]), plus two not.toContain), which is a reasonable way to pin a known gap — but the comment above it claims "Results stay correct; only the acceleration is lost" and nothing here verifies that. Since the fallback path is the one the fix actually changed (the raw-source plan is what builds the rolling CTEs), an assertion on the rows is the part with regression value; the empty-rollup-list assertion mostly guards the matcher, which this PR doesn't touch.
EXPECTED_MONTHLY_SALES_ROWS is already exported in this file, so:
return dbRunner.evaluateQueryWithPreAggregations(query).then(res => {
expect(res).toEqual(EXPECTED_MONTHLY_SALES_ROWS.map((row, i) => ({
...row,
performance_view__rolling_share_change: i < 3 ? '0.00000000000000000000' : null,
})));
});would make this spec assert the thing that matters and keep it useful after the matcher gap is closed (only the two not.toContain lines would need flipping then).
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #11400 +/- ##
==========================================
- Coverage 83.95% 79.43% -4.53%
==========================================
Files 257 480 +223
Lines 80877 98768 +17891
Branches 0 3636 +3636
==========================================
+ Hits 67904 78455 +10551
- Misses 12973 19795 +6822
- Partials 0 518 +518
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:
|
|
Claude finished @paveltiunov's task in 2m 6s —— View job Reviewed new commit
|
| # | Severity | Where | Issue |
|---|---|---|---|
| 4 | Medium | smoke-shared-calc-group.test.ts:153 |
The Cube Store optimize_projections failure means the PR's headline shape still fails end to end in a rollup-only deployment — a hard failure, not a lost optimization. It's tracked only by a FIXME in a skipped test, and the PR description's "After the fix" table reports these queries as correct, deterministic without qualifying that this holds for Postgres only. Worth an issue link and a row in "Remaining limitations". Sub-note: the cross-cube spec adds rolling_share_change to measures but monthlyAmounts() never projects it, so the two specs assert identical things. |
Details in the inline comment.
Net executed coverage added by this commit is zero — both new specs are skipped, so the only live change is the fixture re-key, validated by the four pre-existing smoke tests. That's a reasonable trade (the skips document a real engine gap with a precise repro), but it does mean the birdbox suite still has no executed month-granularity coverage of the shape this PR fixes; the Postgres schema-compiler suite carries that alone.
Prior threads — all three re-affirmed
This commit changed neither of the Rust files nor the assertion at pre-aggregations-shared-calc-group.test.ts:731, so my three earlier threads stand unresolved as-is (state can be narrower than the root query under reduce_by/grain.exclude; the Rust regression test stops at substring assertions; the "loses both rollups" spec claims correct results without asserting rows). Not re-posting them.
Note: finding #4's second bullet and prior thread #3 are the same class of gap — a spec naming a measure it never inspects. If you touch one, the other is a one-line fix in the same direction.
Security / performance
Nothing to raise. Test fixtures and skipped specs only; the SQL is model-declared literals with no user input, and CUBEJS_REFRESH_WORKER: 'false' keeps the added rollup config from changing suite runtime.
Verification I could not run
git fetch origin master and cargo/birdbox execution were not permitted in this sandbox, so I reviewed the commit contents and working tree directly rather than re-running suites. The commit's claim that the four existing smoke tests still pass with time_dimension: sales.date is not something I could confirm locally — it's the one behavioural risk in this commit, since those specs assert SHARE_ROLLUP is used and the re-key changes what the matcher sees.
| // without trouble. | ||
| // | ||
| // FIXME: unskip once Cube Store can execute this projection shape (or the | ||
| // planner stops projecting filter-pinned calc groups into these CTEs). |
There was a problem hiding this comment.
The diagnosis here is precise and valuable — but the conclusion it lands on deserves more visibility than a FIXME inside a skipped test.
This suite is the only place in the repo that runs these plans the way production does (CUBEJS_ROLLUP_ONLY: 'true', rollups stored in and queried from Cube Store). What the two skips record is that the exact shape this PR fixes — a month-granularity rolling calc-group query — plans and matches correctly but still fails at execution time in a rollup-only deployment:
Internal: Optimizer rule 'optimize_projections' failed
Schema error: No field named fk_aggregate.sales__r3_amount
That's a second, independent gap on the same user journey, and unlike the two matcher limitations in the PR description it is a hard failure rather than a lost optimization. Two things would help:
- File it and link the issue number from the
FIXME, rather than leaving the repro only in a git comment. Whether the fix belongs in Cube Store'soptimize_projectionsor in the planner (not projecting filter-pinned calc groups into these CTEs) is exactly the kind of question that needs a tracked owner — the comment already frames both options well. - Add it to the "Remaining limitations" table in the PR description. As written, that table lists only matcher gaps, and the "After the fix" table reports the month-granularity queries as
correct, deterministic— which is true for Postgres but not for Cube Store. A reader deciding whether this unblocks their dashboard would draw the wrong conclusion.
Smaller note on the specs themselves: the second one adds rolling_share_change to measures but monthlyAmounts() projects only product/date/rolling_amount, so the cross-cube measure the test is named for is unverified even once unskipped. Extending the helper (or the expected tuples) to carry it would make the two specs actually differ in what they check.
|
Claude finished @paveltiunov's task in 2m 12s —— View job Reviewed new commit
|
| # | Severity | Where | Issue |
|---|---|---|---|
| 5 | Low | integration_multi_stage.yaml:475 |
Two sub-points: (a) the stated rationale ("a 7-day window can never be served from a rollup at month granularity") doesn't apply to this test — create_context() plans against raw SQL with no pre-aggregations, so the swap is fixture parity, not a correctness fix, and the old fixture did reproduce the bug; (b) since the case entrypoints were the only consumers of rolling_sum_7d_change, no test now exercises a calc-group switch over a sub-month rolling window, where the rolling grain differs from the query grain. |
Details and a suggested fixture addition are in the inline comment.
Prior threads — all four re-affirmed, none resolved
This commit touched neither member_query_planner.rs, pre-aggregations-shared-calc-group.test.ts, nor smoke-shared-calc-group.test.ts, so all four earlier threads stand as-is and I'm not re-posting them:
member_query_planner.rs:183—state()can be narrower than the root query underreduce_by/grain.excludeon the time dimension, potentially dropping the time dimension from the CTE grain whiletime_seriesstill fans out.with_rolling_window.rs:202— still twosql.contains(...)assertions with notry_execute_pg+ snapshot. This commit strengthens the case: the regression signal is entirely thebuild_sql(...).unwrap(), since before the fix planning errored;contains("orders__window_kind")would match the outerSELECTor the filter predicate regardless of whether the CTE projects it, which is what the doc comment claims to test. And now that the fixture mirrors the JS model whose expected monthly values are known (90/120/150), a snapshot is cheaper to justify than before.pre-aggregations-shared-calc-group.test.ts:731— "loses both rollups" spec claims correct results but asserts only the empty rollup list.smoke-shared-calc-group.test.ts:153— Cube Storeoptimize_projectionsfailure is a hard end-to-end failure in a rollup-only deployment, tracked only by aFIXMEin a skipped test and absent from the PR description's limitations table.
Security / performance
Nothing to raise. YAML test fixtures and a test constant only; no new SQL construction path, no user input, no I/O.
Verification I could not run
cargo is not available in this sandbox, so I could not run cargo test -p cubesqlplanner test_rolling_window_switch_with_granular_time_dimension against the new fixture, nor re-verify the reverted-fix failure the commit message reports. I reviewed the fixture and test contents directly and cross-checked member definitions against the two JS models. The renames are the one mechanical risk and grep covers those completely.
| type: sum | ||
| sql: amount | ||
| rolling_window: | ||
| trailing: 3 month |
There was a problem hiding this comment.
The new leaf matches the JS/birdbox model exactly — trailing: 3 month with no offset, time_shift: 3 month prior, to_date year for the other branch, switch values R3/YTD (cf. pre-aggregations-shared-calc-group.test.ts:114-142 and birdbox-fixtures/shared-calc-group/schema/sales.yml:55-97). Fixture parity across the three suites is genuinely useful.
Two notes on the framing and on what the swap costs:
-
The stated rationale — "a 7-day window can never be served from a rollup stored at month granularity" — doesn't apply to this test.
create_context()plans against raw SQL with no pre-aggregations, so nothing here matches a rollup; the CTE-projection bug is grain-agnostic and reproduced equally with the 7-day window. So this is a parity/realism change, not a correctness one — the commit message reads as though the old fixture didn't exercise the bug, when it did (and the commit confirms the reverted-fix failure still reproduces). Worth saying that plainly so a future reader doesn't conclude the day-grain shape was broken. -
Because
rolling_amount_switch/rolling_change_switchwere the only consumers ofrolling_sum_7d_change, that measure is now gone and the switch/case-over-a-sub-month rolling window shape is no longer exercised anywhere. Sub-month rolling windows go through the sametime_seriesgeneration but with a different granularity than the query grain, which is exactly where a CTE-grain bug would show up differently. Keeping both entrypoints costs four fixture lines and one extra test:
- name: rolling_amount_switch_daily
type: number
multi_stage: true
case:
switch: "{CUBE.window_kind}"
when:
- value: R3
sql: "{CUBE.rolling_sum_7d}"
else:
sql: "{CUBE.total_amount_ytd}"
1357d8d to
edb61ca
Compare
|
Claude finished @waralexrom's task in 6m 42s —— View job Reviewed new commit
|
| shape | snapshot | derivation |
|---|---|---|
| R3, April | 900 |
200+300+400 |
| R12, April | 1057 |
7+50+100+200+300+400 — needs the 2023 rows |
| YTD, April | 1000 |
100+200+300+400 |
prev R3, April |
157 |
R3 at Jan 2024 = 7+50+100 |
| change, April | 743 |
900-157 |
| growth, April | 4.732484076433121 |
743/157 |
| grouped switch | R12/R3/YTD × books/toys | ordering matches the requested order |
The seed comment explaining why the 2023 rows exist ("without them the two coincide and a test could not tell them apart") is exactly the note that stops someone deleting them as noise later.
#[ignore] discipline is right. Both ignored tests carry the verbatim engine error and an explicit statement that the cause is unrelated to the calc-group grain, and both shapes are still covered by the matching-only test — so the planner-side assertion survives the engine limitation instead of being lost with it.
Prior thread #1 (reduce_by narrowing the state) is addressed and I've resolved it. rolling_sum_7d_reduce_time is exactly the shape I asked about, with try_execute_pg + snapshot. Working through the values: rolling_sum_7d is trailing: 7 day, offset: start, so over Jan 1-10 only Jan 5's 100 (electronics) ever falls inside a window, contributing to Jan 6…Jan 10 → 5 × 100 = 500, with Jan 1-5 empty. The snapshot says exactly that. So the CTE keeps the per-date grain and the time_series join does not fan out — the concern is empirically ruled out, not just argued away.
Findings
| # | Severity | Where | Issue |
|---|---|---|---|
| 6 | Medium | switch_rolling.rs:253-269 |
Doc comment on test_case_entrypoint describes the optimize_projections failure in present tense, but 1bf2dd8 (#11410) — already in master and an ancestor here — is the fix, the test is un-ignored, and its snapshot is a real CubeStore result. Reads as "this shape is broken" when the test is the regression guard. |
| 7 | Medium | smoke-shared-calc-group.test.ts:152 |
Same staleness on the JS side: the FIXME tells the reader to unskip "once Cube Store can execute this projection shape", which is now done in-repo. The actual blocker is that birdbox pins the published cubejs/cubestore:latest (birdbox.ts:294) while the Cube server is built from the tree — so it unskips after a release carrying #11410. Without that, the next person re-diagnoses a fixed bug. |
| 8 | Low | switch_rolling.rs:170-179 |
The Postgres result is computed then discarded unless the integration-cubestore feature is also on, so in the ordinary PG configuration this whole suite asserts matching only. Snapshotting normalize() of whichever result exists would give value coverage where it's cheapest. Sub-note: the separator filter starts_with('-') also silently drops any data row whose first cell is a negative number. |
| 9 | Low | integration_cubestore_switch_rolling_tables.sql:25-41 |
account_id and category are perfectly correlated (acme=books, globex=toys), so switch_rolling_joined_dimension.snap is the by-category snapshot with a renamed column — the joined-grain test can't distinguish the joined dimension from the local one. |
| 10 | Low | integration_cubestore_switch_rolling.yaml:141-168 |
Both derived entrypoints' else branches dispatch to absolute ytd_amount (under format: percent for growth), so no test covers an arithmetic layer over a to_date window — the one shape whose planning differs by having no time_series LEFT JOIN. prev_ytd_amount already exists, so it's nearly free. |
Details and suggested diffs are in the five inline comments.
Prior threads
- ReferenceError: self is not defined - Issue with whatwg-fetch when using the core lib with Node.js #1 (
member_query_planner.rs:183, state narrower than root underreduce_by) — resolved. Addressed by the new fixture measure + executed snapshot, as described above. - MUTEX for requests #2 (
with_rolling_window.rs:202, substring-only assertions) — still open, and finding Introduce Query class in ResultSet #8 reinforces it. The new suite does value-verify the switch/rolling shape, but only under--features integration-cubestore; in the plain PG run that shape is still unverified end to end. - ResultSet pivot methods documentation #3 (
pre-aggregations-shared-calc-group.test.ts:731) — still open, file untouched by this commit. - Self reference guard in schema compiler #4 (
smoke-shared-calc-group.test.ts:153) — still open; finding Make.querymethod a getter for for ResultSet #7 is adjacent but a different root cause (the fix landed; the image pin is the blocker), so it's a new thread rather than a repost. - Add support for AWS Athena in JDBC driver #5 (
integration_multi_stage.yaml, no sub-month window under a case entrypoint) — still open. Note the new CubeStore fixture is month-only too, so nothing in either Rust suite exercises a calc-group switch over a sub-month rolling window.
Security / performance
Nothing to raise: test fixtures, seed data, and snapshots only, no user input on any new SQL construction path. On runtime — 11 new #[tokio::test(flavor = "multi_thread")] tests each build their own TestContext, seed Postgres, and create/upload rollup tables to CubeStore; that's a real cost, but it's the established pattern in cubestore/basic.rs and it's gated behind a feature flag, so default runs pay only for planning.
Docs
No user-facing behaviour added by this commit, so no docs-mintlify change needed.
Verification I could not run
cargo isn't available in this sandbox, so I could not run cargo test -p cubesqlplanner --features integration-cubestore or the birdbox suite. I verified the snapshots arithmetically against the seed instead, which is what would catch a wrong-grain regression, and confirmed 1bf2dd8 is an ancestor of HEAD with git merge-base --is-ancestor — that ancestry is what findings #6 and #7 rest on.
|
Claude finished @waralexrom's task in 3m 19s —— View job Reviewed
|
| # | Finding | Resolution |
|---|---|---|
| 6 | Stale present-tense doc on test_case_entrypoint |
Past tense, names #11410, states what the test guards. Module header updated too. |
| 7 | Stale birdbox FIXME |
Correctly re-diagnosed: the blocker is the image split (cubejs/cubestore:${BIRDBOX_CUBESTORE_VERSION:-latest} published vs. server built from the tree), not the optimizer. |
| 8 | PG result computed then discarded | from_rollup.as_deref().or(from_source.as_deref()) — values now pinned in a plain --features integration-postgres run. Snapshots became engine-independent tables, which is the better artifact. |
| 8b | starts_with('-') dropping negative first cells |
Now `chars().all( |
| 9 | account_id/category perfectly correlated |
Rows 17/18 break it, and the two snapshots now genuinely differ: by-category May/June 1202/1502, by-account 1201/1501. |
| 10 | Derived else branches dispatching to an absolute sum |
({CUBE.ytd_amount} - {CUBE.prev_ytd_amount}) and a new ytd_growth, each with an executed test, plus both added to the matching-only assertion list. The early-2023 rows exist so those tests assert numbers instead of NULLs — the right supporting change. |
I re-derived all 13 snapshots from the seed by hand and every value is correct, including the ones the new rows shifted:
| shape | value | derivation |
|---|---|---|
| R12, April books | 1087 |
30(2023-05)+7+50+100+200+300+400 — was 1057, +30 from the new 2023-05 row |
| R3, May books | 1202 |
300+400+502, the 502 from row 18 |
| joined, May acme | 1201 |
300+400+501, the 501 from row 17 — differs from by-category ✓ |
| derived else, April books | 980 |
1000 − ytd(Apr 2023)=20 |
| growth else, May toys | 29.2 |
146/5 |
| derived/growth R3 | 743, 4.7324840764 |
unchanged from before ✓ |
That the new else-branch tests produce non-NULL, non-trivial numbers (49, 29.04, 41.04) is what makes them worth having — a NULL-returning test would have passed for the wrong reason.
Findings
| # | Severity | Where | Issue |
|---|---|---|---|
| 11 | Medium | ..._grouped_switch.snap:13 |
normalize_cell's unconditional .replace('T', " ") mangles the string cell YTD → Y D. Now that snapshots record normalize(result), the corrupted value is committed as the baseline; it also means the rollup/source assert_eq! can't see divergence that the substitution collapses — the same silent failure mode as the separator filter this commit just fixed. |
| 12 | Low | ..._switch_rolling_tables.sql:22 |
The block comment still claims "one row per (category, month)"; rows 17/18 deliberately break that. The surviving invariant is per (account, category, month). |
Details and suggested diffs in the two inline comments.
Prior threads — 5 resolved, 4 still open
Resolved: the two doc-staleness threads, the run_both/separator thread, the seed-correlation thread, and the derived-else thread.
Still open, none touched by this commit (not re-posting):
with_rolling_window.rs:202—test_rolling_window_switch_with_granular_time_dimensionis still twosql.contains(...)assertions with notry_execute_pg+ snapshot. Note that finding Introduce Query class in ResultSet #8's fix narrows this: the switch/rolling shape is now value-verified in the plain PG run, but by the new CubeStore suite's fixture, not this one.pre-aggregations-shared-calc-group.test.ts:731— "loses both rollups" spec claims correct results but asserts only the empty rollup list.integration_multi_stage.yaml:475— no test exercises a calc-group switch over a sub-month rolling window; the new CubeStore fixture is month-only too.smoke-shared-calc-group.test.ts(older thread) — its headline concern is moot now that#11410landed, but the sub-note stands: the cross-cube spec addsrolling_share_changetomeasureswhilemonthlyAmounts()(L185) still projects only product/date/rolling_amount, so the two skipped specs assert identical things. Left unresolved for that reason.
Security / performance
Nothing to raise: fixtures, seed data, snapshots, and doc comments. Two new #[tokio::test(flavor = "multi_thread")] tests each seed Postgres and build a rollup, so the feature-gated suite is now 13 executed + 2 ignored; default runs still pay only for planning.
Docs
No user-facing behaviour changed, so no docs-mintlify update needed. The PR description's "Remaining limitations" item 3 (Cube Store cannot execute the fixed plan) is now stale in the same way the code comments were — worth striking or annotating with #11410 before merge, since that's what a reader assessing the PR reads first.
Verification I could not run
cargo isn't available in this sandbox, so I could not run cargo test -p cubesqlplanner --features integration-cubestore or re-accept snapshots. I verified the 13 snapshots arithmetically against the seed instead — that's the check that would catch a wrong-grain regression, and it's also how I found finding #11 (the Y D cell is visible only because the snapshots are now normalized).
· branch claude/rolling-window-time-dimension-utbmy5 ·
|
Claude finished @waralexrom's task in 1m 49s —— View job Reviewed
|
| shape | value | derivation |
|---|---|---|
| books YTD May | 1502 |
100+200+300+400+(500+2) |
| books YTD June | 2102 |
+600 |
| toys YTD May | 151 |
10+20+30+40+(50+1) |
| toys YTD June | 211 |
+60 |
| books R12 May | 1559 |
7+50+1502 — 2023-05's 30 correctly outside a Jun2023–May2024 window |
| books R3 May | 1202 |
300+400+502 |
That R12 excludes the 2023-05 row while test_case_entrypoint_r12 at April includes it is a good sign the trailing bounds are right rather than coincidentally matching.
Findings
None. One cosmetic nit, not worth an inline comment: the normalize doc comment (switch_rolling.rs:120-126) now has a stranded and ratios are at the end of a line from the edit — a reflow would read better, but cargo fmt won't complain and nothing is unclear.
Prior threads — 2 resolved, 3 still open
Resolved: the Y D thread and the seed-comment thread.
Still open, none touched by this commit (not re-posting):
with_rolling_window.rs:202—test_rolling_window_switch_with_granular_time_dimensionis still twosql.contains(...)assertions. Lower stakes now that the CubeStore suite value-verifies the same shape in a plain PG run, but the assertion in this file still can't distinguish "projected into the CTE" from "appears in the outer SELECT".pre-aggregations-shared-calc-group.test.ts:731— "loses both rollups" spec claims correct results but asserts only the empty rollup list.integration_multi_stage.yaml:475— no test exercises a calc-group switch over a sub-month rolling window; both Rust fixtures are month-only.
The older smoke-test thread stays unresolved for its sub-note only: monthlyAmounts() (L185) projects product/date/rolling_amount, while both skipped specs list performance_view.rolling_share_change in measures (L100, L118, L214, L239), so the cross-cube spec doesn't check the measure it's named for even once unskipped.
Security / performance
Nothing to raise — a string-normalisation helper in test code, one snapshot, one comment. No new test executions, so suite runtime is unchanged.
Docs
No user-facing behaviour changed. Standing note from last round: the PR description's "Remaining limitations" item 3 ("Cube Store cannot execute the fixed plan") is now stale — #11410 landed and switch_rolling.rs::test_case_entrypoint runs that shape against a live CubeStore un-ignored. That's the first thing a reviewer reads, so it's worth striking or annotating with #11410 before merge; the code comments themselves were already fixed in 8cbee73.
Verification I could not run
cargo isn't available in this sandbox, so I could not run cargo test -p cubesqlplanner --features integration-cubestore or re-accept snapshots. I verified the re-accepted snapshot arithmetically against the seed and traced the normalize_cell branch by hand for both engines' timestamp renderings and for the YTD/R3/R12 string cells — which is what would catch either a wrong-grain regression or a remaining mangled cell.
· branch claude/rolling-window-time-dimension-utbmy5 ·
A `case` entrypoint dispatching between rolling-window measures on a `type: switch` calc-group dimension could not be queried with a granular time dimension: planning failed with "Alias not found for partition_by dimension <calc group>". The rolling-window CTE built its projection from the ROOT query dimensions, while the case parent adds the switch dimension to the grain of everything below it. The arithmetic layer above the rolling windows (e.g. `r3 - prev_r3`) then partitions by that switch dimension and looks it up in the CTE schema, where it was never projected. Building the CTE schema from the description's own state fixes it and also covers the `grain: include` widening of the same shape. - rolling-window CTEs project `description.state()` dimensions and time dimensions instead of the root query's - rust: fixture gains a `window_kind` switch dimension plus case entrypoints over the rolling / rolling-change measures, and a regression test building SQL for them with a month time dimension - schema-compiler: month-granularity specs on the shared calc-group Postgres suite — the sales rollup serves the query with deterministic monthly values; a new model variant keys the joined cube's rollup on the queried time dimension and both rollups are served, with the rollup result asserted equal to the raw-source result Two remaining matching limitations are pinned by the new specs: a rollup keyed on its own (join-equal) time dimension can't serve a query grouping by the other side's dimension, and losing one cube's rollup drops the whole query to the raw source; and expressing the range as an `inDateRange` filter on the time dimension instead of `timeDimensions[].dateRange` stops every rollup from matching (skipped FIXME spec). Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CmA7TSPTdq9z83Am3e7s4u
… rolling queries Adds the month-granularity time-dimension queries to the shared calc-group birdbox suite and keys the share_metrics rollup on the time dimension the view exposes (`sales.date`), so the planner can match both rollups — the cube's own `share_metrics.date` is equal by the join condition but is not a member the matcher can resolve. The four existing no-time-dimension smoke tests still pass with that fixture change. The two new queries are skipped: the plan is planned and matched correctly (covered end to end against Postgres in the schema-compiler suite) but Cube Store cannot execute it — Internal: Optimizer rule 'optimize_projections' failed Schema error: No field named fk_aggregate.sales__r3_amount The rolling-window CTE groups by (date, product, calc group) while its consumer projects a strict subset of those group-by columns — dropping the filter-pinned calc-group column but keeping the aggregate — and Cube Store's projection optimizer prunes the aggregate out of the CTE schema. A plain rolling measure at month granularity over the same rollup (no `case` entrypoint, so no calc-group column in the CTE) is served by Cube Store without trouble, and the same SQL runs on Postgres, so this is specific to the calc-group projection shape. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CmA7TSPTdq9z83Am3e7s4u
…itch fixture The switch case entrypoints dispatched between a 7-day trailing window and the year-to-date one. A 7-day window can never be served from a rollup stored at month granularity, so it does not match the shape these specs stand in for. Adds a `rolling_sum_3m` (trailing 3 month) leaf and dispatches between whole-month windows — trailing 3 month vs year-to-date — with the switch values renamed R3 / YTD accordingly, mirroring the model in the schema-compiler and birdbox suites. The regression test still fails with "Alias not found for partition_by dimension orders.window_kind" when the rolling-window CTE projection fix is reverted. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CmA7TSPTdq9z83Am3e7s4u
… end to end Every shape the calc-group switch model produces is now exercised twice: served from an external rollup in CubeStore, and straight from the source Postgres with pre-aggregations filtered out. The two results are compared cell by cell, with timestamp rendering normalised and numbers rounded (ratios are f64 in CubeStore against Postgres NUMERIC, so they differ in the last digit), which pins the rollup path as an optimization rather than a different answer. A separate always-running test asserts rollup matching for all of them, independently of whether an engine can execute the plan. New fixture integration_cubestore_switch_rolling.yaml mirrors the shapes of a production model: a `type: switch` dimension with trailing and to-date windows behind one `case` entrypoint, an entrypoint dispatching straight onto a `time_shift` measure, a difference and a ratio layer above the rolling windows, grain by an own dimension and by a joined cube's, and rollups that variously omit the calc group, store it, or carry an aggregating index. The seed carries data from the preceding year so a 12-month trailing window and year-to-date produce different sums — without it the two coincide and a test could not tell them apart. Two shapes stay ignored for CubeStore limitations unrelated to the calc-group grain, each with the reason on the test: a four-entrypoint query builds a plan too deep for CubeStore to decode (protobuf recursion limit), and a rollup with an aggregating index fails at table creation with a column-type mismatch. Also adds rolling_sum_7d_reduce_time and an executed test for it: an Aggregate parent that reduces by the time dimension makes the child rolling-window state narrower than the root query, and the rolling CTE must still project and group by the rolling time dimension so the time_series join does not fan out.
Five points from the review, all in the test layer: Stale docs. The note on `test_case_entrypoint` described the CubeStore `RollingOptimizerRule` widening in the present tense, but that test is not ignored and holds a real CubeStore snapshot — #11410 is an ancestor of this branch and fixes exactly that rewrite. Reworded to past tense, naming the fix and what the test now guards. The birdbox `FIXME` had the same problem: the Cube Store side is fixed, what still blocks those two specs is that birdbox pulls the published `cubejs/cubestore:latest` while the Cube server is built from the working tree, so only the planner fix is in the containers under test. Value coverage in the plain Postgres configuration. `run_both` computed the raw-source rows and threw them away unless CubeStore was also available, so a run without `integration-cubestore` asserted only rollup matching. It now snapshots `normalize()` of whichever engine ran; the snapshots become engine-independent tables instead of one engine's rendering, and the values are pinned in an ordinary `cargo test --features integration-postgres` run. Separator filter. `normalize()` dropped every line starting with `-`, which would silently discard a data row whose first cell is negative — both tables lose the same rows, so the comparison would still pass with reduced coverage. It now drops only lines made of the separator characters. Correlated seed columns. Each account sold exactly one category, so the by-account snapshot was the by-category one with a column renamed and the joined-grain test could not tell the two groupings apart. Each account now sells both categories, and the two results genuinely differ. Derived entrypoints on their `else` branch. Both fell back to the absolute `ytd_amount` under measures named `..._change` and `format: percent`, so no test covered an arithmetic layer over a to-date window — the shape closest to what broke originally. They now dispatch to a difference and to a new `ytd_growth`, with a test each. The seed gains early-2023 rows so the year-over-year shift has something to subtract and those tests assert numbers rather than NULLs.
`normalize_cell` replaced `T` and `Z` unconditionally to turn CubeStore's `2024-05-01T00:00:00.000Z` into a space-separated timestamp, which also rewrote string cells: the `YTD` calc-group value was recorded as `Y D` in switch_rolling_grouped_switch.snap, so the committed artifact showed a value the model does not declare, and the rollup-versus-source comparison lost fidelity because both sides dropped the same characters. The substitution now matches the timestamp shape — trailing `Z` with a `T` inside — and leaves everything else alone. Also fixes the seed comment: rows giving each account both categories mean the invariant is one row per (account, category, month), not per (category, month).
Building an aggregating index over a decimal measure used to panic before any query ran — the sum widened to `Decimal128(28, 5)` against a column declared `Decimal128(18, 5)`. Fixed in #11413, so the test runs: the rollup with an aggregating index returns exactly what the plain one does, which is the property worth pinning, and it is now the regression guard for that cast. The four-entrypoint query stays ignored; a plan that deep still exceeds Cube Store's protobuf decode recursion limit, which reproduces on a real model too.
88decbc to
2e60ba6
Compare
|
Claude finished @waralexrom's task in 2m 43s —— View job Reviewed
|
| cell | value | derivation |
|---|---|---|
| books April | 900 |
200+300+400 |
| books May | 1202 |
300+400+(500+2) — the +2 is seed row 18 |
| books June | 1502 |
400+502+600 |
| toys April | 90 |
20+30+40 |
| toys May | 121 |
30+40+(50+1) — the +1 is row 17 |
| toys June | 151 |
40+51+60 |
Worth noting why that identity is more than a coincidence of two accepted snapshots: both tests run the same query through run_both, which asserts normalize(rollup) == normalize(source) against the same Postgres source. So "the aggregating-index rollup returns exactly what the plain one does" is pinned transitively by the harness, not just by two files happening to match — a future drift in one would fail its own assert_eq! before insta review ever saw it. That's the right structure for this test.
Counting is consistent. grep finds exactly one #[ignore] left (test_four_entrypoints_in_one_query, protobuf recursion limit), matching the header's "One shape". That remaining ignore keeps the discipline established earlier in the PR: verbatim engine error, explicit statement that the cause is unrelated to the calc-group grain, and the shape still covered by test_switch_shapes_are_served_by_rollup (L222-235) so the planner-side assertion survives. rolling_by_category_agg_idx is also in that matching-only list (L249-253), so the rollup is asserted to match even where CubeStore isn't available.
No competing-rollup risk from the new fixture: every test scopes matching with only_pre_aggregations, and the agg_idx rollup only ever appears in its own context.
Findings
None in the code. One doc nit, and it's the same class as the ones fixed in 8cbee73 — the PR description is now stale in two places:
- "13 executed shapes … plus 2 shapes
#[ignore]d for unrelated Cube Store limitations" → the file now has 14 executedasynctests + 1 ignored (plus the sync matching test). - The "Remaining limitations" list doesn't mention the aggregating-index decimal cast at all, which is fine now that it's fixed — but a reader comparing the description against the file will hit the ignore-count mismatch first and wonder which one is wrong.
Since the description is what a reviewer reads before the diff, a one-line update ("14 executed shapes … 1 #[ignore]d for the protobuf recursion limit; the aggregating-index cast was fixed in #11413") is worth doing before merge, alongside the item-3 annotation already noted last round.
Prior threads — 4 still open, none touched by this commit
Not re-posting any of them:
with_rolling_window.rs:202—test_rolling_window_switch_with_granular_time_dimensionis still twosql.contains(...)assertions; the substring check can't distinguish "projected into the CTE" from "appears in the outer SELECT". Lower stakes now that this suite value-verifies the same shape.pre-aggregations-shared-calc-group.test.ts:731— "loses both rollups" spec claims correct results but asserts only the empty rollup list.integration_multi_stage.yaml:475— no test exercises a calc-group switch over a sub-month rolling window; both Rust fixtures are month-only.smoke-shared-calc-group.test.ts— open for its sub-note only:monthlyAmounts()(L185) projects product/date/rolling_amount, while both skipped specs listperformance_view.rolling_share_changeinmeasures(L100, L118, L214, L239), so the cross-cube spec still wouldn't check the measure it's named for once unskipped. Confirmed still true in the current tree.
Security / performance
Nothing to raise — a doc comment, an attribute removal, and one snapshot. One more #[tokio::test(flavor = "multi_thread")] now executes in the feature-gated run (seeds Postgres, builds a rollup with an aggregate index); default runs still pay only for planning.
Docs
No user-facing behaviour changed, so no docs-mintlify update needed.
Verification I could not run
cargo isn't available in this sandbox, so I could not run cargo test -p cubesqlplanner --features integration-cubestore — in particular I could not confirm that creating the aggregating-index table now succeeds, which is the whole point of the un-ignore. What I could check is that the fix it depends on is present in the tree, that CI runs this suite against a release cubestored built from that same tree, and that the accepted snapshot is arithmetically correct against the seed.
· · branch claude/rolling-window-time-dimension-utbmy5 ·
4 participants
Check List
Description of Changes Made
Follow-up to #11346. Adding a time dimension (month granularity) to the shared calc-group rolling-window query did not work at all — it failed during planning, with or without pre-aggregations:
Root cause: the rolling-window CTE built its projection from the root query dimensions, while a
caseparent adds the switch dimension to the grain of everything below it. The arithmetic layer above the rolling windows (r3 - prev_r3) then partitions by that switch dimension and resolves it against the CTE schema, where it was never projected. Without a query time dimension no rolling-window CTE is built at all (try_plan_rolling_windowshort-circuits when there are no time dimensions), which is why the existing specs passed.The CTE schema now comes from the description's own state instead of the root query, which also covers the
grain: includewidening of the same shape.Still needed on current master (
c3edaf0): reverting just this two-line change makestest_rolling_window_switch_with_granular_time_dimensionfail with the error above, and master carries no other change underrust/cube/cubesqlplanner.After the fix
dateRangesales_perf_rollingrolling_share_change), month +dateRangesales.datesales.datefilters[] inDateRangeinstead ofdateRangeRemaining limitations, pinned by the new specs
share_metrics.perf_shareusestime_dimension: date(share_metrics.date) while the view exposessales.date; they are equal by the join condition, but the matcher only knows stored members. Losing one cube's rollup also drops the whole query to the raw source — the sales rollup that matches on its own is discarded too.{member: date, operator: inDateRange}prevents every rollup from matching even alongsidedateRange: the filter carries a plain dimension symbol andtry_match_dimensionlooks it up only among stored dimensions, never among the rollup's time dimensions. Semantically it also differs — withoutdateRangethe rolling window is not anchored to the range, the leaves are filtered instead.Cube Store cannot execute the fixed plan.Resolved — this was theInternal: Optimizer rule 'optimize_projections' failed / Schema error: No field named fk_aggregate.sales__r3_amountfailure, fixed on master by fix(cubestore): keep the projection schema when rewriting a rolling window #11410 (keep the projection schema when rewriting a rolling window), which is an ancestor of this branch.switch_rolling.rs::test_case_entrypointnow runs that shape against a live Cube Store un-ignored. The two birdbox smoke specs stay skipped for a different reason: birdbox pins the publishedcubejs/cubestore:latestimage while the Cube server is built from the tree, so they unskip after a release carrying fix(cubestore): keep the projection schema when rewriting a rolling window #11410.Tests
packages/cubejs-schema-compiler/.../pre-aggregations-shared-calc-group.test.ts: month-granularity specs, a model variant keying the joined cube's rollup on the queried time dimension, a rollup-vs-raw-source equality assertion, deterministic monthly values, and a skipped FIXME for the filter gap. The two obsolete skipped specs are replaced.rust/.../multi_stage/with_rolling_window.rs+ fixture:window_kindswitch dimension and case entrypoints over month-based rolling / rolling-change measures (trailing 3 month vs year-to-date, matching the JS and birdbox models); verified the test fails with the exact error when the fix is reverted.rust/.../tests/integration/cubestore/switch_rolling.rs+ fixture and seed: 13 executed shapes (case entrypoint, other window, joined dimension, stored switch, grouped switch, prev / derived / growth entrypoints and theirelsebranches, date-range-only) each run twice — served from an external rollup in Cube Store and from raw Postgres — and compared cell for cell, plus 2 shapes#[ignore]d for unrelated Cube Store limitations.packages/cubejs-testing/test/smoke-shared-calc-group.test.ts+ birdbox fixture: month-granularity queries against a real Cube server with rollups in Cube Store, skipped pending a Cube Store release with fix(cubestore): keep the projection schema when rewriting a rolling window #11410. The share rollup is keyed onsales.dateso the planner can match it; the four pre-existing no-time-dimension smoke tests still pass with that change.Verification:
cargo test -p cubesqlplanner1088 passed; calc-group Postgres suite 13 passed / 1 skipped; fulltest/integration/postgressuite 501 passed with one pre-existing local-environment failure (PreAggregations › lambda cross data source…cannot resolve an unbuiltksql-driver); birdbox smoke suite 4 passed / 2 skipped against Cube Store;cargo fmt/clippyclean.Limitations 1 and 2 are left unfixed because each is a separate change with its own correctness question (join-equality resolution in the matcher, granularity safety for filter bounds).
🤖 Generated with Claude Code
https://claude.ai/code/session_01CmA7TSPTdq9z83Am3e7s4u