fix(tesseract): resolve rollup-join keys declared as time dimensions, and name the failing hop

[igorlukanin]

Warning

Superseded — do not merge. Review proved this emits an unexecutable join: both planners
render ON "td_dates__day" = "td_facts__day" while the leg rollups only ever materialize
td_facts__day_day. Master rejected such a schema with an actionable No rollups found; this
branch accepts it and produces SQL that fails at execution — a worse failure mode than the one
it replaced. Back to CORE-724 for re-planning as an ON-clause alias-resolution fix.
Details, the executed before/after, and the open granularity question are in the planning doc.
The docs PR #11470 is independent and unaffected.

What this was trying to fix

The ticket reported that a rollup_join over a 4-cube chain fails where the 3-cube one matches, and
inferred a chain-depth limit in Tesseract. There is no depth limit — 4- and 5-cube chains match on
master on the same terms a 3-cube chain does. The inherited regression test failed for two reasons of
its own: its interior rollup declared only [dim_x], not dim_w (the key on its own side of the new
hop), and it asserted the rollupJoin's own id appears in preAggregationsDescription(), which never
happens for a matched rollupJoin — you get the leg rollups, with the join named on
preAggregationForQuery.

A real defect was found next door: a join key declared as a rollup's time dimension was invisible to
the lookup that matches a hop to its leg rollups, so such a chain never resolved at any length. Both
planners had their own copy — Tesseract's find_pre_aggregation_for_join matched dimensions only,
and the JS preAggObjForJoin checked references.dimensions, which excludes timeDimensions.

Why the fix is wrong

Widening the match predicate lets the hop resolve, but nothing teaches the ON-clause renderer that
the key lives in a granularity-suffixed column. The JS sqlResolveFn in
PreAggregations.rollupPreAggregation returns the bare member.aliasName(), and on the Rust side the
join-members loop in PreAggregation::all_dimensions_refererences runs after the time-dimensions
loop and overwrites td_facts.day -> td_facts__day_day with td_facts__day — which also corrupts the
query's own time dimension, rendered as date_trunc('day', "td_facts__day") "td_facts__day_day".

No test caught it because the time-dimension test — the only one covering the actual fix — asserts
matching (preAggregationsDescription(), preAggregationName) and not rendering. The 4-/5-cube chain
tests in this PR do assert the SQL assembles; that one didn't meet the same bar.

What is still worth keeping

• The refuted premise and the fixture diagnosis.
• The Tesseract error message, which now names the failing hop, the unmatched members, and which
  rollup_join to fix — a bare No rollups found that can be used for rollup join is why this was
  filed as a depth bug. Sound on its own merits, independent of the matching change.
• The 4- and 5-cube chain tests, which assert the SQL assembles rather than only that matching
  succeeded.

Refs #11362

[claude]

Claude finished @igorlukanin's task in 3m 44s —— View job

---

Review complete — the fix is right in principle; 1 high, 1 medium, 2 low, 1 general note.

Full review

• Gather context (diff, Rust planner, JS counterpart, SQL rendering path)
• Review Rust planner change
• Review tests & fixtures
• Post review

Summary

The diagnosis is convincing and the write-up is unusually good. build_join_source collects join hints from dimensions + time_dimensions + segments while find_pre_aggregation_for_join matched against dimensions alone — that asymmetry is a genuine bug, and the argument that widening the predicate can't regress a working schema (disjoint schema keys; the only reachable new outcome is Multiple rollups on a schema that was already failing) holds up. Comparing through base_symbol() is the correct unwrap. The double red-check on the Rust test, and the JS chain tests asserting the SQL assembles rather than just that matching succeeded, are the right instincts.

My concerns are all downstream of the match, not about the match itself.

#	Severity	Where	Issue
1	high	pre_aggregations_compiler.rs:558-575	Hop resolves, but the granularity-suffixed rollup column (td_facts__day_day) isn't what the join member's alias resolves to — and all_dimensions_refererences overwrites the suffixed entry with the unsuffixed one. No SQL-level test covers the newly enabled path.
2	medium	pre_aggregations_compiler.rs:570	Granularity is discarded entirely; a month-grained rollup satisfies a raw-day join key, and the two sides of a hop are matched independently so mismatched granularities are accepted.
3	low	pre_aggregations_compiler.rs:565	Chaining segments can never match (expr:-prefixed MemberExpressions) — reads as support that isn't there.
4	low	pre_aggregations_compiler.rs:526-535	The error still can't say which side of the hop failed; both calls emit identical text.

Details are in the four inline comments.

General note — divergence from the JS planner

PreAggregations.ts:1093 still filters on p.references.dimensions only, and JS keeps references.timeDimensions separate. So after this change a schema whose rollup_join key is declared as a time_dimension works under Tesseract and still fails under CUBEJS_TESSERACT_SQL_PLANNER=false. Given the legacy planner is deprecated but still selectable, worth either mirroring the predicate there or stating the divergence in the PR description — right now the description says the error message matches JS, which is true, but the matching behaviour now deliberately doesn't.

Things I checked and found fine

• The rollup_join_missing_key.yaml fixture is minimal and its comment points at the exact omission — good fixture hygiene.
• The JS chain tests assert preAggregationsDescription reports the leg rollups while the join is named on preAggregationForQuery; that distinction is easy to get wrong and the comment records it.
• Threading rollup_join_name through by reference adds no allocation on the success path; the format! calls are all inside error branches.
• No security or performance surface here — this is compile-time planning over an already-loaded schema, and the added predicate is O(rollup members) inside an existing filter.

Not verified

I couldn't run cargo test (no build cache in this environment and the build wasn't permitted), so finding #1 is reasoned from the code path rather than observed. It's the one worth confirming before merge — a build_sql assertion on the time-dimension fixture would settle it either way.

• branch `igor/core-724-rollup_join-doesnt-match-over-a-4-cube-join-chain-3-hops`

[claude]

The hop now resolves, but I don't think the SQL it produces can reference the right column.

A rollup that declares day as a time_dimension materializes it granularity-suffixed — the pre-agg table column is td_facts__day_day (see build_pre_agg_query_yaml in test_context.rs, and the suffix logic in PreAggregation::all_dimensions_refererences, logical_plan/pre_aggregation.rs:113-129). The join key resolved from {CUBE.day} = {td_facts.day} is the bare dimension symbol, whose alias() is td_facts__day.

Two consequences follow from all_dimensions_refererences:

1. The join-members loop (pre_aggregation.rs:136-142) inserts td_facts.day -> td_facts__day after the time-dimensions loop inserted td_facts.day -> td_facts__day_day into the same HashMap — so the later, unsuffixed entry wins, and even the query's own selected time dimension would then render against a column that isn't in the rollup table.
2. The on_sql for the hop renders through the same reference map, so the ON clause would reference td_facts__day too.

The new Rust test asserts only the logical structure (join.items[0].to == td_facts_rollup), so neither of these would show up. That's the one gap against the bar this PR sets for itself elsewhere — the JS chain tests deliberately assert the SQL assembles rather than just that matching succeeded. Could the time-dimension fixture get the same treatment (a build_sql assertion, or an integration-postgres run that actually executes the join)? If the SQL does come out right, that test is cheap insurance; if it doesn't, the alias mapping needs to resolve a time-dimension-backed join key to its suffixed column.

Fix this →

[igorlukanin]

Confirmed, including the overwrite. Executed on both planners — the leg rollup only ever materializes td_facts__day_day, and the join renders ON "td_dates__day" = "td_facts__day". The projection shows the map corruption directly: Tesseract emits date_trunc('day', "td_facts__day") "td_facts__day_day", so the query's own time dimension gets mis-mapped exactly as you said, because the join-members loop in all_dimensions_refererences runs after the time-dimensions loop and wins.

Details and the before/after in the JS thread. Kicking the ticket back rather than growing this PR into an alias-resolution change across both planners.

[claude]

Granularity is discarded, and the two sides of a hop are matched independently.

base_symbol() == member accepts a rollup whose stored values are truncated to any granularity. A rollup declaring time_dimension: created_at, granularity: month will satisfy a join key on created_at even though the column holds month-truncated timestamps, and the two legs of a hop are resolved by two separate find_pre_aggregation_for_join calls — nothing checks they agree. A month-grained leg joined to a day-grained leg matches, and the join silently produces the wrong rows.

The fixture can't distinguish this: both legs are granularity: day and the source data is already day-aligned, so a wrong-granularity match would still produce the same rows.

Even the matching-granularity case is a semantic choice worth stating: joining on truncated buckets is only equivalent to joining on the raw key when the key is already bucket-aligned. If that's the intended contract, the doc comment is the place to say so; if not, the two sides need their granularities compared before the hop is accepted.

[igorlukanin]

Valid, and it's the part that needs a product decision rather than a patch. Joining on truncated buckets is only equivalent to joining on the raw key when the key is already bucket-aligned, and nothing compares the two legs' granularities — so a month leg joined to a day leg matches and returns wrong rows. Recorded on CORE-724 as an open question for reassessment along with the alias gap; whether mismatched granularities should be rejected or documented as the contract isn't mine to settle inside a bug fix.

[claude]

The message is a real improvement over the bare string, but it still can't tell the author which side of the hop failed. find_pre_aggregation_for_join is called twice per hop — once for from_members, once for to_members — and both calls emit the identical text listing both sides. In the new rollup_join_missing_key fixture it's the to side (mk_customers) that lacks the key, yet the message reads the same as it would if mk_orders were the broken one.

Since members is already passed separately from join_item, a one-word side label threaded from make_pre_aggregation_join_item ("no rollup covers the from side …") would make the message point at exactly one rollup to fix. The JS planner has the same ambiguity, so this isn't a regression — just the remaining half of the diagnosis this PR is adding.

[igorlukanin]

Agreed, and cheap — a side label threaded from make_pre_aggregation_join_item would point at one rollup instead of two. Leaving it with the rest of the reassessment rather than pushing a message-only change onto a PR that's being reworked.

[claude]

Claude finished @igorlukanin's task in 3m 4s —— View job

---

Re-review of the JS-planner commit — 1 high, 1 medium new; 3 prior threads still stand, 1 resolved.

Full review

• Gather context (new commit e6d1f52, existing review threads)
• Review JS planner change (PreAggregations.ts)
• Re-check prior findings against current diff
• Resolve stale threads, post new inline comments

What changed since the last review

e6d1f52 does three things, and two of them are direct answers to the last round:

1. Mirrors the fix in the legacy JS planner — this was my "General note — divergence from the JS planner." It's addressed properly: preAggObjForJoin now also checks references.timeDimensions, and cubesHintsFromPreAggregation includes time-dimension cubes. The commit message notes both were verified independently by reverting one at a time, and the new test runs it.each over both planners. That's the right way to close it — the two matchers really are independent code, and testing the pair prevents them drifting again.
2. Drops the segments branch from the Rust predicate — my low-severity finding ResultSet pivot methods documentation  #3. Correct reasoning in the commit message (MemberSymbol equality includes the enum discriminant, so a dimension can never compare equal to a Segment), and the extracted pre_aggregation_covers_join_member with its doc comment reads better than the inline chain did. Thread resolved.
3. Extracts the predicate into a named helper with a doc comment.

New findings

#	Severity	Where	Issue
5	high	PreAggregations.ts:1096	The JS side inherits the Rust alias gap: the rollupJoin ON clause resolves join keys via newDimension(path).aliasName() → td_facts__day, but a leg rollup declaring the key as a timeDimension materializes it as td_facts__day_day. The new test asserts matching only, so it can't see this.
6	medium	PreAggregations.ts:1147	The cubesHintsFromPreAggregation widening is not covered by the PR's "cannot regress a working schema" argument — it grows existingJoins, which can only shrink nonExistingJoins, and an empty nonExistingJoins is a hard Nothing to join in rollup join throw.

Details inline. On #5 I was able to trace the JS path statically end to end (ON clause bypasses renderedReference, which is the suffixed map, and goes straight to aliasName()), which also firms up the Rust twin — all_dimensions_refererences (logical_plan/pre_aggregation.rs:136-142) demonstrably runs its join-members loop after the time-dimensions loop and inserts into the same HashMap, so td_facts.day -> td_facts__day_day is overwritten by td_facts.day -> td_facts__day.

Prior threads re-affirmed (not re-posted)

• pre_aggregations_compiler.rs:567 — SQL-level assertion missing on the time-dimension fixture / alias-suffix mismatch. Still applies verbatim; the new commit didn't touch the alias mapping, and finding Add support for AWS Athena in JDBC driver #5 is the same defect on the other planner.
• pre_aggregations_compiler.rs:563 — granularity discarded, and the two sides of a hop matched independently. Still applies; the new doc comment explains the base_symbol() unwrap but doesn't state a granularity contract.
• pre_aggregations_compiler.rs:535 — the error still can't name which side of the hop failed. Unchanged.

Things I checked and found fine

• references.timeDimensions is always an array from evaluatePreAggregationReferences (CubeEvaluator.ts:1138-1155), so no undefined guard is needed in the new .find(...) — consistent with the unguarded references.timeDimensions.length === 0 already at line 1479.
• td.dimension is a dotted string including any join path, so .split('.').slice(0, -1) in cubesHintsFromPreAggregation yields the same shape as the existing dimensions handling. R.uniq over arrays-of-arrays does structural comparison — correct here.
• Running both planners through it.each in one test is a good pattern for this class of bug and worth keeping.
• No security or performance surface: compile-time planning over an already-loaded schema; the added predicate is O(rollup members) inside an existing filter, the added hints are O(time dimensions).

Not verified

I could not execute anything this round — the monorepo has no installed dependencies in this environment and yarn install wasn't permitted, and there's no Rust build cache. Findings #5 and the re-affirmed Rust alias thread are both read from the code path rather than observed. They're the pair worth confirming before merge, and one buildSqlAndParams() assertion on the time-dimension fixture would settle both at once.

• branch `igor/core-724-rollup_join-doesnt-match-over-a-4-cube-join-chain-3-hops`

[claude]

The JS side inherits the same alias gap the Rust side has — and here I can trace it end to end.

The hop now matches, but the rollupJoin ON clause is rendered with a sqlResolveFn that returns member.aliasName() for a plain dimension (rollupPreAggregation, line 1592-1599):

const member = this.query.cubeEvaluator.isMeasure(path)
  ? this.query.newMeasure(path)
  : this.query.newDimension(path);
return member.aliasName();

For td_dates.day / td_facts.day that yields td_dates__day and td_facts__day. But a leg rollup that declares the key as a timeDimension materializes it granularity-suffixed — timeDimensionsRenderedReference (line 1725) builds the column as timeDimension.unescapedAliasName(rollupGranularity), i.e. td_dates__day_day / td_facts__day_day. Neither td_dates__day nor td_facts__day is a column of either leg table, so the emitted ON should reference columns that don't exist.

The outer projection is fine — it goes through renderedReference, which is the suffixed map. It's specifically the ON clause that bypasses it by resolving straight to aliasName().

The new rollupJoin whose join key is a time dimension test (pre-aggregations.test.ts:1563-1584) asserts only preAggregationsDescription and preAggregationForQuery.preAggregationName — matching, not rendering — so it wouldn't surface this. That's the same bar the 4-/5-cube chain tests in this PR deliberately set for themselves ("assert the SQL assembles, not merely that matching succeeded"); this fixture should get it too. A query.buildSqlAndParams() assertion on the ON text would settle it in either direction.

If it does render unsuffixed, the fix is for the rollupJoin sqlResolveFn to resolve a key backed by a leg rollup's timeDimension through that rollup's time-dimension alias (granularity suffix included) rather than the bare dimension alias.

I could not run this — the monorepo has no installed dependencies in this environment and install wasn't permitted — so this is read from the code path, not observed. Given the Rust twin has the same shape (see the unresolved thread on pre_aggregations_compiler.rs, where all_dimensions_refererences demonstrably overwrites td_facts.day -> td_facts__day_day with td_facts__day because the join-members loop runs last), it's worth confirming before merge.

Fix this →

[igorlukanin]

You're right, and it renders unsuffixed — I ran it on both planners.

The leg rollup materializes exactly two columns:

CREATE TABLE td_facts_td_facts_rollup AS SELECT
  date_trunc('day', ("td_facts".day::timestamptz AT TIME ZONE 'America/Los_Angeles')) "td_facts__day_day",
  sum("td_facts".amount) "td_facts__total_amount"

There is no td_facts__day. Yet the ON clause is:

-- JS planner
LEFT JOIN td_facts_td_facts_rollup AS "td_facts__td_facts_rollup"
  ON "td_dates__day" = "td_facts__day"

-- Tesseract
LEFT JOIN  td_facts_td_facts_rollup  AS "td_facts__td_facts_rollup"
  ON "td_dates__day" = "td_facts__day"

Both sides reference columns neither leg table has. And the Rust overwrite you predicted in the sibling thread is visible in the projection too — Tesseract emits date_trunc('day', "td_facts__day") "td_facts__day_day", re-truncating a column that doesn't exist instead of reading the stored one.

So the net effect of this half of the PR is worse than what it replaced: master rejected the schema with an actionable No rollups found, and with the widened predicate it is accepted and emits unexecutable SQL. Confirmed by reverting the predicate alone — the rejection comes back.

Not fixing it inside this PR. Making it correct means resolving a join key backed by a leg rollup's timeDimension through that rollup's suffixed alias in both planners (the JS sqlResolveFn here, and the map ordering in all_dimensions_refererences on the Rust side), which is a different change from widening a lookup. Kicked back to CORE-724 for reassessment with your traces recorded.

[claude]

This one isn't covered by the "cannot regress a working schema" argument — that argument was made for the matching predicate, and it holds there. This hint widening has a different blast radius.

cubesHintsFromPreAggregation feeds existingJoins (line 1457-1461), which is subtracted from targetJoins to produce nonExistingJoins — and if that comes out empty, rollupJoin throws outright:

if (!nonExistingJoins.length) {
  throw new UserError(`Nothing to join in rollup join. ...`);
}

Adding time-dimension cubes to the hints makes each leg's join tree larger, so existingJoins grows and nonExistingJoins can only shrink. For a leg rollup whose timeDimension carries a join path (td_dates.td_facts.day), a hop that was previously treated as still-to-be-joined is now treated as already inside that leg. I think that's the more correct reading — the hop genuinely is inside the leg rollup — but it is a behaviour change on schemas that were previously matching, and the failure mode is a hard throw rather than a silent fallback.

Worth either a test pinning a leg rollup with a join-path time dimension (does it still produce a non-empty nonExistingJoins?), or a line in the PR description acknowledging that this half of the change can move a previously-matching schema, unlike the predicate half.

[igorlukanin]

Fair distinction — the "cannot regress a working schema" argument was about the matching predicate and doesn't cover the hint widening, which can only shrink nonExistingJoins and fails hard rather than falling back. Untested either way right now. Folded into the CORE-724 reassessment; the leg-rollup-with-join-path-time-dimension case needs pinning before any version of this lands.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 79.45%. Comparing base (3728d62) to head (e6d1f52).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #11469      +/-   ##
==========================================
- Coverage   79.45%   79.45%   -0.01%     
==========================================
  Files         480      480              
  Lines       98810    98812       +2     
  Branches     3636     3636              
==========================================
  Hits        78511    78511              
- Misses      19778    19780       +2     
  Partials      521      521              

Flag	Coverage Δ
cube-backend	59.10% <100.00%> (-0.01%)	⬇️
cubesql	83.95% <ø> (-0.01%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.