Skip to content

docs: pre-aggregation matching for switch dimensions (follow-up to #11346) - #11441

Open
igorlukanin wants to merge 4 commits into
masterfrom
igor/core-668-docs-pre-agg-matching-switch-dims
Open

docs: pre-aggregation matching for switch dimensions (follow-up to #11346)#11441
igorlukanin wants to merge 4 commits into
masterfrom
igor/core-668-docs-pre-agg-matching-switch-dims

Conversation

@igorlukanin

Copy link
Copy Markdown
Member

Summary

Pre-aggregations can match queries that use switch dimensions without storing them, but nothing in the docs covered the interaction — switch/calc-group dimensions had zero mentions on the pre-aggregation pages.

  • docs/pre-aggregations/matching-pre-aggregations.mdx — new ### Matching switch dimensions section: a rollup doesn't need to include a switch dimension (and stays smaller for leaving it out), plus the two modeling constraints that apply when case measures span cubes — define the switch on one shared cube, and key every rollup on the time dimension the query groups by. Also qualifies the "contains all dimensions" matching bullet, which was stated twice and is now stated once.
  • reference/data-modeling/dimensions.mdxswitch was the only dimension type without a code example; added one to both variants of the type <CodeGroup>, and documented the values sub-parameter.
  • recipes/data-modeling/dynamic-rolling-windows.mdx — new ## Pre-aggregations section; the recipe had no rollup example, and its "always include the switch dimension in the query" note is where the rollup question arises.

Test plan

  • Every example compiles: both the YAML and JavaScript variant of all three changes, via prepareYamlCompiler / prepareJsCompiler. The JS rollup in the recipe needed string member names — bare CUBE[...] references don't resolve inside a transpiled measures array.
  • mint broken-links --check-anchors: zero failures in the touched pages; the 16 reported are pre-existing in embedding/iframe/.
  • All three pages render (HTTP 200) under mint dev.

@igorlukanin
igorlukanin requested a review from keydunov as a code owner July 31, 2026 22:19
@claude

claude Bot commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

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


Review: 1 high, 2 medium, 2 low — core claims verified against the planner, one rollup example is a no-op

The central claim of the PR is correct and well-grounded: DimensionMatcher::try_match_dimension returns MatchState::Full for is_calc_group() dimensions, and stored calc groups keep resolving to the rollup column, so "existing definitions are unaffected" holds too. The main issue is that both new rollup examples list rolling_window measures, which the measure matcher rejects unconditionally.

Full review — findings, verification notes, checklist

Checklist

  • Read PR diff
  • Verify technical claims against the Tesseract planner source
  • Verify cross-reference anchors resolve
  • Check consistency with existing docs
  • Post review (4 inline comments)

What I verified in source

Claim Verdict Evidence
A rollup need not include a switch dimension ✅ correct pre_aggregation/dimension_matcher.rs:203-209is_calc_group()MatchState::Full
Rollups that do include it keep matching ✅ correct physical_plan_builder/processors/query.rs:72-91 — stored calc groups resolve to the rollup column "for backward compatibility"
switch uses values instead of sql ✅ correct at the model level CubeValidator.ts:1011-1014 permits only type + values; the Rust SwitchDimension supports a sql variant but it isn't expressible in the data model
Per-cube switch dims → extra rows ✅ correct query.rs:93-118 — unpinned calc groups with >1 value get cross-joined
Matching is all-or-nothing across subqueries ✅ correct pre_aggregation/optimizer.rs:288-303 — any unrewritten leaf rolls back the whole attempt
inDateRange filter "prevents matching" ⚠️ overstated conflicts with query-format.mdx:355-363
Rollup should list the per-window measures ❌ no-op measure_matcher.rs:34-39is_cumulative() (= is_rolling_window()) returns false before the pre-agg measure list is checked

Anchors all resolve: #time-dimensions-format, #indaterange, #rollup, #multi_stage, #case, #type, and the new #matching-switch-dimensions.

Findings

  1. High — dynamic-rolling-windows.mdx:418-439: the rollup lists r{3,6,9,12}_gross_sales; rolling-window measures can never be matched from a rollup, so only the base gross_sales at month granularity does any work. The prose ("which is what actually gets aggregated") inverts the actual mechanism. inline comment with suggested rewrite
  2. Medium — matching-pre-aggregations.mdx:125-128: same problem in the illustrative rollup (r3_amount, ytd_amount); plainly additive measures keep the snippet on-message.
  3. Medium — matching-pre-aggregations.mdx:180-186: the inDateRange note reads as an absolute blocker, while the reference doc says it's applied as a generic filter needing the time dimension in dimensions. The recipe's version additionally equates a SQL-API WHERE clause with timeDimensions, which would make every SQL-API date filter a blocker.
  4. Low — matching-pre-aggregations.mdx:164-178: the second modelling bullet is general multi-stage behaviour, not switch-specific; it belongs in "Matching multi-fact and multi-stage queries", which currently reads purely optimistically.
  5. Low — dimensions.mdx:930/964: values is required (and sql rejected) for switch — worth stating; the three claims packed into one table cell would read better as a short ### values section; and naming the example dimension rolling_window collides with the rolling_window measure parameter.

Nice work

  • Removing the duplicated "does every member exist" bullet and qualifying the surviving one is the right call — the exception is now stated exactly where the algorithm is described.
  • Deriving the guidance from actual planner behaviour (row multiplication, all-or-nothing rollback) rather than restating the feature is what makes the new section useful.
  • No security, performance, or test-coverage concerns: docs-only change, no runtime code touched.
• branch `igor/core-668-docs-pre-agg-matching-switch-dims`

Comment thread docs-mintlify/recipes/data-modeling/dynamic-rolling-windows.mdx Outdated
Comment thread docs-mintlify/docs/pre-aggregations/matching-pre-aggregations.mdx
Comment thread docs-mintlify/docs/pre-aggregations/matching-pre-aggregations.mdx Outdated
Comment thread docs-mintlify/docs/pre-aggregations/matching-pre-aggregations.mdx Outdated
Comment thread docs-mintlify/reference/data-modeling/dimensions.mdx Outdated
…ove the all-or-nothing caveat to multi-fact matching
@claude

claude Bot commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

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


Re-review of 40cba45 — my two rolling-window findings were wrong; 1 new medium; 3 prior findings resolved

@igorlukanin's counter-evidence holds and I've withdrawn both measure-matcher findings — new_unrolling() preserves the measure's compiled_path, so the rollup does have to list the r{n}_gross_sales names. The one new item is the JavaScript variant of that same rollup: string member names never register as symbol dependencies, so Tesseract compiles it with an empty measure list.

Full review — verification, findings, thread bookkeeping

Checklist

  • Read the new commit's diff
  • Verify @igorlukanin's counter-evidence against the planner
  • Check whether the inDateRange and all-or-nothing findings are addressed
  • Resolve my own stale threads (all 5)
  • Post updated review (1 new inline comment)

Where I was wrong

My claim was that MeasureMatcher::try_match returning false for is_cumulative() measures makes listing r{n}_gross_sales a no-op. Tracing the leaf construction shows otherwise:

// multi_stage_query_planner.rs:704
let base_member = MemberSymbol::new_measure(measure.new_unrolling());

new_unrolling() (measure_symbol.rs:156-186) clones the symbol with rolling_window: None, multi_stage: None but keeps compiled_path. So the leaf inside the rolling-window CTE is gross_sales.r3_gross_sales with is_cumulative() == false — exactly the name the pre-agg must list. The code comment I quoted ("only their base (leaf) measures inside the CTE can match") means the unrolled same-named measure, not the referenced base measure. That also explains the empirical result: base-only → no match; per-window → match; and the build SQL materialising sum(amount) per rolling measure rather than computing windows at build time.

Consequently r3_amount/ytd_amount in the matching-pre-aggregations.mdx snippet are fine as written too.

New finding — medium

dynamic-rolling-windows.mdx:450-453 — the JS rollup's measures: ['gross_sales', ...windows.map(...)]. Tesseract ignores the JS-evaluated measureReferences and instead compiles the measures function, taking sql_call.get_dependencies() (pre_aggregations_compiler.rs:114symbols_from_ref:554-569). Those dependencies come from symbolPaths, populated only by memberReferenceProxy's get trap (MemberSqlTemplateCompiler.js:62-80, collected at :249-266). String literals touch no proxy → symbolPaths: [] → empty CompiledPreAggregation.measures → nothing matches, with no error anywhere on the path. Legacy has the mirror problem: evaluateReferences maps .toString() (CubeSymbols.ts:1233), producing unqualified "gross_sales".

CUBE[\r${months}_gross_sales`]records the path on access and keeps the dynamic generation — [suggested diff in the inline comment](https://github.com/cube-js/cube/pull/11441#discussion_r3693740636). Caveat stated there: source reading only,npx jestisn't runnable in this sandbox, so apreAggregationsDescription()` dump of the JS variant would settle it.

Prior findings now addressed

Finding Status
inDateRange overstated as "prevents matching" ✅ now mirrors query-format.mdx — "applied as a generic dimension filter … matches only when that time dimension is also listed in the pre-aggregation's dimensions"
Recipe conflated a SQL-API WHERE with timeDimensions ✅ scoped to "When querying through the REST (JSON) API"
All-or-nothing caveat filed under switch dimensions ✅ moved to "Matching multi-fact and multi-stage queries", where a reader debugging a dropped rollup lands; the time-dimension-keying cause reads well there
values described as an alternative to sql ✅ "Requires a values sub-parameter and doesn't accept sql" matches SwitchDimension in CubeValidator.ts:1011-1014 (min(1).required(), no sql key)
rolling_window example dimension name collided with the measure parameter ✅ renamed to growth_window, consistent with the recipe

The one suggestion not taken — hoisting the values explanation out of the table cell into its own ### section — was stylistic; the trimmed cell reads fine.

All five of my earlier threads are resolved: two withdrawn as incorrect, three addressed by the commit. Docs-only change; no security, performance, or test-coverage concerns.

• branch `igor/core-668-docs-pre-agg-matching-switch-dims`

Comment thread docs-mintlify/recipes/data-modeling/dynamic-rolling-windows.mdx Outdated
@claude

claude Bot commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

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


Re-review of 422071a — fix confirmed correct, 3 low nits, nothing blocking

The CUBE[...] change is right and the dropped base measure is consistent across both variants. Remaining items are placement and precision, not correctness.

Full review — verification, findings, thread bookkeeping

Checklist

  • Read the new commit's diff
  • Verify the CUBE[...] fix and the dropped base measure
  • Check YAML/JS variant parity across all three pages
  • Resolve stale threads (1 remaining, now resolved)
  • Post review (3 inline comments)

The fix

measures: windows.map(months => CUBE[r${months}_gross_sales]) — correct, and your preAggregationsDescription() dump settles the question the source reading couldn't. Dropping gross_sales matches the YAML variant, and the collision you hit ('undefined' not found for path 'gross_sales' when the measure shares the cube's name) is a real constraint of this model, not a workaround. Prose now says "it's those that the rollup has to name to be matched", which is the mechanism rather than the earlier hand-wave.

Checked for regressions from the change: the model section still defines r{3,6,9,12}_gross_sales via windows.reduce, so the rollup's generated names line up; all 17 [ref-*] links in the recipe resolve to definitions; the v1.7.0 Tesseract warning wording matches the other 13 occurrences across the docs.

Findings — all low

# Location Issue
1 matching-pre-aggregations.mdx:173-181 The reworded inDateRange note is accurate but sits at the end of "Matching switch dimensions" while being general time-dimension guidance. ### Matching time dimensions (lines 65-88) is where a reader who lost a rollup to inDateRange will look, and it doesn't mention it. Same misfiling you fixed for the all-or-nothing caveat in 40cba45.
2 dynamic-rolling-windows.mdx:449 Now the only computed-CUBE[...] pre-agg measures list in the docs; every other JS example uses bare identifiers. One sentence on why strings don't work would save the next person the silent no-match.
3 dimensions.mdx:930 "doesn't accept sql" understates it — SwitchDimension (CubeValidator.ts:1011-1014) is a closed object with only type + values, so title/description/public/meta are rejected as well. Source-read only; dist/ isn't built here, so worth confirming before rewording.

Threads

My last open thread (the JS string-names finding) is resolved — fixed by this commit. All six of my threads on this PR are now closed: two withdrawn as wrong, four addressed.

Docs-only change; no security, performance, or test-coverage concerns.

• branch `igor/core-668-docs-pre-agg-matching-switch-dims`

Comment thread docs-mintlify/recipes/data-modeling/dynamic-rolling-windows.mdx
Comment thread docs-mintlify/docs/pre-aggregations/matching-pre-aggregations.mdx Outdated
Comment thread docs-mintlify/reference/data-modeling/dimensions.mdx Outdated
@claude

claude Bot commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

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


Re-review of 1fb9332 — all three prior items addressed, no new findings; ready to merge

Full review — verification, thread bookkeeping

Checklist

  • Read the new commit's diff
  • Verify the inDateRange move, the CUBE[...] note, and the values rewording
  • Check the touched sections for regressions (link defs, anchors, YAML/JS parity)
  • Resolve stale threads (3 remaining, now resolved)
  • Post review (no inline comments — nothing left to flag)

The three fixes

Item Status
inDateRange note misfiled under "Matching switch dimensions" ✅ moved to the end of ### Matching time dimensions (matching-pre-aggregations.mdx:95-103), right after the custom-granularity paragraph. "the granularity matching rules above don't apply to it" now has real referents — the three bullets immediately preceding it. The recipe keeps its own in-context copy, which is the right split.
CUBE[...] refs unexplained in the recipe dynamic-rolling-windows.mdx:422-425 — "Cube resolves member references by recording them as they're accessed, so a string never registers as one — the rollup compiles with an empty measure list and silently matches nothing." That's the mechanism (symbolPaths populated by the proxy's get trap), stated in one sentence without dragging the reader into internals, and it names the failure mode that makes it dangerous: silent.
switch constraint understated ✅ "Takes only a values sub-parameter — no sql, and no other parameters such as title or public" matches SwitchDimension in CubeValidator.ts:1011-1014 — a closed two-key object that DimensionsSchema routes to instead of the BaseDimension alternatives. Your compile check confirms what the source reading suggested.

Regression checks on this commit

  • All eight link definitions used by the moved/edited prose resolve: ref-time-dimensions-formatquery-format#time-dimensions-format, ref-in-date-range#indaterange, plus ref-switch, ref-case, ref-rollup, ref-multi-stage, ref-matching-switch-dimensions, link-tesseract.
  • Removing the <Note> leaves "Matching switch dimensions" ending on the multi-cube modelling paragraph, which is the section's own subject — no dangling transition.
  • The recipe's new paragraph sits before the <CodeGroup> and is scoped to "In the JavaScript model", so it doesn't read as a caveat on the YAML variant, which is unaffected.
  • JS/YAML parity holds on all three pages; the switch-section snippet still uses bare identifiers (measures: [total, r3_amount, ytd_amount]), which is correct — bare identifiers transpile to recorded member refs, so no contradiction with the recipe's string-vs-CUBE[...] note.
  • growth_window example in dimensions.mdx:964/1004 matches the recipe's naming.

Threads

The three that remained open are now resolved. Across the PR, all nine of my threads are closed: two withdrawn as incorrect (the measure-matcher findings — new_unrolling() keeps compiled_path, so the per-window measures do have to be listed), seven addressed by commits.

Docs-only change; no security, performance, or test-coverage concerns. Nothing blocking.

• branch `igor/core-668-docs-pre-agg-matching-switch-dims`

@mintlify

mintlify Bot commented Jul 31, 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 Jul 31, 2026, 11:10 PM

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

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