Skip to content

docs(pre-aggregations): state that every rollup must declare its own joins' keys - #11470

Open
igorlukanin wants to merge 1 commit into
masterfrom
igor/core-724-docs-rollup-join-keys
Open

docs(pre-aggregations): state that every rollup must declare its own joins' keys#11470
igorlukanin wants to merge 1 commit into
masterfrom
igor/core-724-docs-rollup-join-keys

Conversation

@igorlukanin

@igorlukanin igorlukanin commented Aug 4, 2026

Copy link
Copy Markdown
Member

Summary

The rollup_join reference never stated that every rollup listed in rollups must declare the dimensions its own joins are on — including keys that no query selects. The page's own example includes CUBE.user_id in orders_rollup for exactly that reason, but never says why, so the requirement was only discoverable by hitting the error.

It matters most on a longer chain: a cube in the middle takes part in two joins and has to declare both keys, one of which belongs to the upstream hop and appears in no query. Missing it fails with No rollups found that can be used for a rollup join, which reads like a product limitation rather than a schema gap — that's how #11362 came to be reported as a join-chain-depth limit.

Adds a Warning callout under rollup_join stating the rule and pointing at the existing example.

Test plan

  • Wording verified against the error string both planners actually emit on master; the
    quoted substring is the part they share, so it matches whichever planner the reader is on.
  • Mintlify preview renders the callout.

@igorlukanin
igorlukanin requested a review from keydunov as a code owner August 4, 2026 04:38
@mintlify

mintlify Bot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Preview deployment for your docs. Learn more about Mintlify Previews.

Project Status Preview Updated (UTC)
cubed3 🟢 Ready View Preview Aug 4, 2026, 4:40 AM

💡 Tip: Enable Workflows to automatically generate PRs for you.

@claude

claude Bot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Claude finished @igorlukanin's task in 54s —— View job


Review: docs-only, accurate on the core rule — 1 medium issue (error string doesn't match the default planner)

The rule the callout states is correct and well-placed: preAggObjForJoin in packages/cubejs-schema-compiler/src/adapter/PreAggregations.ts:1091-1092 filters candidate rollups to those whose references.dimensions contain every join member, so a rollup that omits a join key is silently excluded — exactly the failure mode described. The Tesseract equivalent (pre_aggregations_compiler.rs:504-514) does the same filter per join, which also backs the "one join at a time / both keys in the middle of a chain" explanation. One medium finding, posted inline.

Details

Todos

  • Gather context (PR diff, surrounding docs section)
  • Verify the stated rule against the planner implementations
  • Verify the quoted error string
  • Post review

Findings

Severity Location Issue
Medium pre-aggregations.mdx:438-442 Quoted error string and "naming the join" claim only match the legacy planner. Tesseract (the default) emits No rollups found that can be used for rollup join — no a, no join details. A user on the default planner searching for their exact error won't match the doc text. Inline comment

Verified as correct

  • orders_rollup in the example does include CUBE.user_id while the query selects only users.name / orders.status — the callout's reference to the example checks out.
  • Placement between the rollup_join example <CodeGroup> and the following <Info> reads naturally; <Warning> with blank lines around the content follows the repo's Mintlify conventions.
  • Correct docs site (/docs-mintlify, not the deprecated /docs); no docs.json change needed since no new page.
  • No code, security, performance, or test surface touched.

Not done

  • Did not render the Mintlify preview locally (no dev server run); the syntax is plain <Warning> + prose, and the bot preview build covers it.
• Branch: `igor/core-724-docs-rollup-join-keys`

Comment thread docs-mintlify/reference/data-modeling/pre-aggregations.mdx Outdated
@claude

claude Bot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Claude finished @igorlukanin's task in 1m 5s —— View job


Re-review after reword: error-string finding resolved — 1 low nit

The reworded callout now quotes No rollups found that can be used for, which is a verbatim prefix of both planners' messages (pre_aggregations_compiler.rs:513 bare Tesseract form, PreAggregations.ts:1095 legacy form), so my earlier medium finding no longer applies. One low nit posted inline.

Details

Todos

  • Diff the updated commit against origin/master
  • Re-verify the quoted error string against both planners
  • Re-verify the "one join at a time / key on each side" claim
  • Check prior review threads for staleness/duplication
  • Post review

Findings

Severity Location Issue
Low pre-aggregations.mdx:435-436 "queries only ask for users.name and orders.status" — orders.status isn't exposed by orders_with_users_rollup (dimensions: [users.name]), so it isn't something a query on this rollup asks for. Slightly blurs the very distinction the callout draws. Inline comment

Verified as correct

  • Quoted prefix No rollups found that can be used for matches both:
    • Tesseract: "No rollups found that can be used for rollup join" (pre_aggregations_compiler.rs:513)
    • Legacy: `No rollups found that can be used for a rollup join from "…"` (PreAggregations.ts:1095)
  • "one join at a time … a rollup carrying that join's key on each side" matches both implementations: preAggObjForJoin is called once for fromMembers and once for toMembers per join (PreAggregations.ts:1074-1075), and find_pre_aggregation_for_join likewise (pre_aggregations_compiler.rs:484-486). Each filters to rollups whose declared dimensions contain every member of that side.
  • Hence the middle-of-a-chain conclusion holds: such a cube's rollup appears as the to side of one join and the from side of the next, and must satisfy both filters.
  • "The error does not point at the key that is missing" — true on the default (Tesseract) planner, which emits no detail. The legacy message does list fromMembers/toMembers, but as the join's full member set, not the omitted one, so the advice to check each rollup by hand still holds either way.
  • My prior thread on the error string is already resolved (by the author's reword); nothing stale left to resolve. No duplicate inline comments — the new nit is on a different line with a different root cause.
  • Correct docs site (/docs-mintlify); <Warning> with surrounding blank lines follows repo Mintlify conventions; no docs.json change needed.
  • Docs-only diff — no code, security, performance, or test surface touched.

Not done

  • Did not render the Mintlify preview locally; the bot's preview build covers rendering.

• Branch: igor/core-724-docs-rollup-join-keys

Comment thread docs-mintlify/reference/data-modeling/pre-aggregations.mdx Outdated
@claude

claude Bot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Claude finished @igorlukanin's task in 1m 2s —— View job


Re-review after the orders.count reword: prior nit resolved — 1 low nit

The callout now says orders_with_users_rollup "only exposes users.name and orders.count", which matches the example verbatim (measures: [CUBE.count], dimensions: [users.name]), so my previous nit about orders.status is addressed. One remaining low nit posted inline, on the closing instruction's scope.

Details

Todos

  • Diff the updated commit against origin/master
  • Re-verify the users.name / orders.count claim against the example
  • Re-verify the quoted error prefix against both planners
  • Check the "its own joins" scoping claim against the resolver
  • Check prior threads for staleness/duplication
  • Post review

Findings

Severity Location Issue
Low pre-aggregations.mdx:443-444 "check each rollup … against the joins its own cube takes part in" is wider than the rule. The checked join set is derived from the rollup_join's own join tree (PreAggregations.ts:1055), not every join on the cube — an unrelated ordersproducts join needs no key here. Inline comment

Verified as correct

  • "only exposes users.name and orders.count" — exact match for orders_with_users_rollup (measures: [CUBE.count], dimensions: [users.name]). Previous nit resolved.
  • CUBE.user_id is indeed the ordersusers join key (sql: ${CUBE.user_id} = ${users.id}), and it is declared in orders_rollup while not exposed by the rollup_join — the example does illustrate the rule.
  • Quoted prefix No rollups found that can be used for is a verbatim prefix of both planners:
    • Tesseract (default): "No rollups found that can be used for rollup join" (pre_aggregations_compiler.rs:513)
    • Legacy: `No rollups found that can be used for a rollup join from "…"` (PreAggregations.ts:1095)
  • "one join at a time … a rollup carrying that join's key on each side" — preAggObjForJoin is invoked once per side per join (PreAggregations.ts:1074-1075), each filtering to rollups whose references.dimensions contain every member of that side. The middle-of-a-chain conclusion follows.
  • "The error does not point at the key that is missing" — true; Tesseract emits no detail, and the legacy message lists the join's full member sets, not the omitted one.
  • Both of my earlier threads are already resolved by the author's rewords; nothing stale left. The new nit is on a different line with a different root cause, so no duplication.
  • Correct docs site (/docs-mintlify, not the deprecated /docs); <Warning> with surrounding blank lines follows repo Mintlify conventions; no docs.json change needed (no new page).
  • Docs-only diff — no code, security, performance, or test surface touched.

Not done

  • Did not render the Mintlify preview locally; the bot's preview build covers rendering.

• Branch: igor/core-724-docs-rollup-join-keys

Comment thread docs-mintlify/reference/data-modeling/pre-aggregations.mdx Outdated
@igorlukanin
igorlukanin force-pushed the igor/core-724-docs-rollup-join-keys branch from d06b245 to a050a44 Compare August 4, 2026 05:03
@claude

claude Bot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Claude finished @igorlukanin's task in 36s —— View job


Re-review after the path-scoping reword: all three prior findings resolved — no new issues

The callout now reads accurately end to end: the rule, the example reference, the quoted error prefix, and the scoping of which joins to check all match the implementation and the example on this page. Nothing new to flag; docs-only, ready from my side.

Details

Todos

  • Diff the updated commit against origin/master
  • Re-verify the quoted error prefix against both planners
  • Re-verify the "each side" claim against the example's users_rollup
  • Re-verify the narrowed "path between the cubes being joined" scoping
  • Check prior threads for staleness/duplication
  • Post review

Findings

None.

Verified as correct

  • Closing instruction now says "against the joins on the path between the cubes being joined" (pre-aggregations.mdx:443-444), which matches how the join set is actually derived — from the rollup_join's own join tree (targetJoins, PreAggregations.ts:1055) rather than every join on the cube. My prior low nit is addressed.
  • "each one needs a rollup carrying that join's key on each side" is borne out by the example itself: users_rollup declares CUBE.id and orders_rollup declares CUBE.user_id — the two sides of ${CUBE.user_id} = ${users.id}. Matches preAggObjForJoin being invoked once per side per join (PreAggregations.ts:1074-1075) and find_pre_aggregation_for_join likewise (pre_aggregations_compiler.rs:484-486), each filtering to rollups whose declared dimensions contain every member of that side.
  • Quoted prefix No rollups found that can be used for is verbatim in both planners:
    • Tesseract (default): "No rollups found that can be used for rollup join" (pre_aggregations_compiler.rs:513)
    • Legacy: `No rollups found that can be used for a rollup join from "…"` (PreAggregations.ts:1095)
      Phrasing it as "an error starting …" is the right hedge given the two messages diverge after that point.
  • "only exposes users.name and orders.count" still matches orders_with_users_rollup exactly (measures: [CUBE.count], dimensions: [users.name]).
  • "The error does not point at the key that is missing" — true; Tesseract emits no detail, and the legacy message lists the join's full member sets, not the omitted one.
  • All three of my earlier threads are resolved (and outdated) after the author's rewords — nothing stale to resolve, no duplicate comments to skip.
  • Correct docs site (/docs-mintlify, not the deprecated /docs); <Warning> with surrounding blank lines follows repo Mintlify conventions; no docs.json change needed (no new page).
  • Docs-only diff — no code, security, performance, or test surface touched.

Not done

  • Did not render the Mintlify preview locally; the bot's preview build covers rendering.

• Branch: igor/core-724-docs-rollup-join-keys

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant