fix(schema-compiler): rollup_join cube hints from time dimensions - #11784
fix(schema-compiler): rollup_join cube hints from time dimensions#11784waralexrom wants to merge 1 commit into
Conversation
`rollup_join` derives its join tree from cube hints collected off the pre-aggregation's own references, and `cubesHintsFromPreAggregation` read only `measures` and `dimensions`. A `rollup_join` whose single reference to one of its cubes is that cube's `time_dimension` therefore produced no hint for it: the join tree covered one cube, `targetJoins` came back empty and the pre-aggregation was rejected with "Nothing to join in rollup join". Naming a dimension of that cube is not a workaround. It puts the hint in, but the rollup then only matches queries that request that dimension, and this shape cannot satisfy both requirements at once. Collect hints from `timeDimensions` as well. They are appended last, so the existing hint order - and with it the join tree root - is unchanged, and `R.uniq` absorbs cubes already covered by a measure or a dimension. This also lines the hint set up with the one the native SQL planner uses: measures, dimensions, segments and time dimensions. Both planners are affected. Under the native planner matching runs in Rust, but the pre-aggregation description - and with it the join tree - is still built here, so the same rejection surfaced through `findPreAggregationForQueryRust`. The native planner's own hint collection already covers time dimensions; a test pins that. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
github-actions
Bot
added
rust
|
Claude finished @waralexrom's task in 2m 26s —— View job Review: sound one-line fix, 0 high / 1 medium / 3 low — 3 inline comments postedFull review
VerdictThe diagnosis is right and the fix is minimal. No security or performance concerns: pure in-memory member-path collection on an already-cached path ( Findings
One claim to double-checkThe PR body says "all 4 fail without the fix", but the Rust sideThe added The acknowledged Verification caveat
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| preAggObj.references.measures.concat( | ||
| preAggObj.references.dimensions | ||
| preAggObj.references.dimensions, | ||
| (preAggObj.references.timeDimensions || []).map(td => td.dimension) |
There was a problem hiding this comment.
references.timeDimensions is non-optional in PreAggregationReferences (CubeEvaluator.ts:152) and evaluatePreAggregationReferences always sets it; other call sites read it unguarded (e.g. mergePartitionTimeDimensions at PreAggregations.ts:1388). The || [] is dead defensiveness that suggests the field can be missing.
| (preAggObj.references.timeDimensions || []).map(td => td.dimension) | |
| preAggObj.references.timeDimensions.map(td => td.dimension) |
|
|
||
| // `rollup_join` builds its join tree from cube hints collected off the | ||
| // pre-aggregation's own references. Those hints have to cover every cube the | ||
| // join is meant to span, including a cube whose single reference is its | ||
| // `time_dimension` - otherwise the join tree covers one cube, `targetJoins` | ||
| // comes back empty and the pre-aggregation is rejected with "Nothing to join in | ||
| // rollup join". | ||
| // | ||
| // The shape has no workaround: naming a dimension of that cube would put the | ||
| // hint in, but then that dimension has to be requested by every query for the | ||
| // rollup to match at all. | ||
| // | ||
| // Both planners build the pre-aggregation description here, so both are covered. |
There was a problem hiding this comment.
This 13-line header re-tells the PR description (the "no workaround" paragraph and the "both planners" note are commit-message material, not something a later editor needs in order to avoid reintroducing the bug). The load-bearing sentence is the first one; the rest can go. The two per-fixture one-liners below already say what each shape exercises.
| granularity: 'day', | ||
| dateRange: ['2026-01-01', '2026-01-31'], | ||
| }]; | ||
|
|
||
| it('builds the join when a cube is referenced only by time_dimension', async () => { | ||
| const sql = await preAggregationSql(useNativeSqlPlanner, timeDimensionOnly, { | ||
| measures: ['boards.good_count'], | ||
| timeDimensions, | ||
| }); | ||
|
|
||
| expect(sql).toContain('locations_locations_rollup'); |
There was a problem hiding this comment.
Neither test covers the other consumer of cubesHintsFromPreAggregation that this change also feeds: existingJoins (PreAggregations.ts:1058-1062). Both leg rollups here declare a same-cube time dimension, so their hint sets are unchanged by the fix. A leg rollup whose time_dimension is reached through a join now contributes an extra hint, which can cancel a target join and turn a previously-buildable rollup_join into Nothing to join in rollup join. The PR argues that cancellation is semantically right, and I agree — but it is a behavior change for existing models with no test pinning it. Worth adding one leg-side case (or explicitly noting that rollup join existing joins in the Postgres integration suite is the only coverage).
Also, the assertions only check that both leg table names appear somewhere in the concatenated loadSql; asserting on the number of descriptions or on the join SQL would fail more informatively if the rollup stops matching (an empty description array currently yields '' and a toContain failure that reads like a SQL mismatch).
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #11784 +/- ##
==========================================
- Coverage 84.67% 84.67% -0.01%
==========================================
Files 261 261
Lines 86451 86451
==========================================
- Hits 73204 73199 -5
- Misses 13247 13252 +5
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:
|
1 participant
Summary
A
rollup_joinwhose only reference to one of its cubes is that cube'stime_dimensioncould never be built: it was always rejected withNothing to join in rollup join. Cube hints for the join tree were collected from the pre-aggregation'smeasuresanddimensionsonly, never from itstimeDimensions, so the cube reached solely through a time dimension was missing from the tree.Reported alongside #11679 (this is not that issue's main claim, which does not reproduce — it is a separate limitation the reporter mentions in passing).
Changes
cubesHintsFromPreAggregationnow also collects hints fromreferences.timeDimensions. They are appended last, so the existing hint order — and with it the join tree root — is unchanged, andR.uniqabsorbs cubes already covered by a measure or a dimension.rollup-join-time-dimension-hints.test.ts, parameterized over both planners.Why there is no workaround
Adding a dimension from the anchor cube does put the hint in, but pre-aggregation matching requires the dimension set to line up, so that dimension then has to be present in every query for the rollup to match at all. The two requirements cannot both hold for this shape.
Both planners are affected
The fix is in JS, but the code is shared. Under the native SQL planner matching runs in Rust and succeeds; the pre-aggregation description — and with it the join tree — is still built in JS, so the same rejection surfaced through
findPreAggregationForQueryRust:The native planner's own hint collection already chains
time_dimensions(andsegments) and had no gap. The Rust test added here is a regression guard for that side; it was verified to actually catch the gap by temporarily dropping the.chain(time_dimensions…), which makes it fail with the same error.This fix also brings the JS hint set in line with the native one: measures, dimensions, segments and time dimensions (segments already arrive inside
references.dimensions).Testing
cargo test -p cubesqlplanner --lib: 1340 passed, 0 failed.cargo fmt --checkclean.dist/test/unit: 911 passed. The 2 failures areerror-reporter.test.tssnapshots, which fail identically onmaster— unrelated.pre-aggregations.test.js47 passed / 7 skipped (everyrollup join*/rollupJoin*case green, includingrollup join existing joins, which exercises theexistingJoinssubtraction this change also feeds);pre-aggregations-calculated-measures.test.js4 passed.Correctness of the
existingJoinssidecubesHintsFromPreAggregationserves two roles insidebuildRollupJoin: hints for the targetrollup_join(which producetargetJoins) and hints for each leg rollup (which produceexistingJoins, subtracted from the target). Leg rollups now contribute more hints too, so a leg could cancel a join it did not before — but that is correct: if a leg rollup stores a time dimension from another cube, it really was built across that join, and the edge genuinely does not need joining again. This mirrors what the native planner already does, androllup join existing joinscovers the branch.Follow-up (not in this PR)
preAggObjForJoinstill resolves join members againstp.references.dimensionsonly. A model whose leg rollup declares the join key as itstime_dimensionrather than adimensionnow reaches that filter and fails with a misleading "No rollups found … you may have forgotten to specify the full dimension paths". Not a regression — before this change the same model failed earlier, at "Nothing to join" — but it is the adjacent half of the same gap.