fix(schema-compiler): a cube that extends another broke the parent's multi-stage measures - #11641
Conversation
`extends` hands the extending cube the very definitions of the cube it extends: `allDefinitions` merges the parent's member objects by reference, and a view's `default_filters` reach the extending view through the prototype. References resolved per cube were written into those shared objects, so the last cube prepared won and every other cube ended up carrying member paths of a cube that is not its own. Multi-stage members were hit through their nested `grain` and `filter` objects — `prepareMembers` copies the member itself, but not what hangs off it — which broke every multi-stage measure of the extended cube, including queries that never mention the extending one. A parent's `grain.include: [d]` came out as `["child.d"]`, and planning then either found no join path to that cube or reported the grain dimension as unreachable, naming a dimension the model already declares. The same aliasing hit two more places: a view's default filter resolved to a member of the view extending it, and a pre-aggregation's `outputColumnTypes` names were scoped to the extending cube. Copy each of them before writing the resolved references. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The report for a dimension a multi-stage member reads outside its grain named the member, the dimension, and the declaration that fixes it — all of which the model already says — so a grain holding something else was indistinguishable from a grain the reader believed was right. Spell out the grain the member is computed at. Granularities are named separately (`orders.created_at (month)`) instead of being folded into the symbol name, and the one case a listing cannot disambiguate — the grain carrying the read dimension only under a granularity — gets a sentence of its own. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @waralexrom's task in 5m 37s —— View job Review complete — correct root-cause fix; no blocking issues. 3 low-severity notes (1 architectural suggestion, 1 Rust readability/consistency, 1 test coverage).Full review
VerdictThe diagnosis holds up under reading. I confirmed the fixed sites are the complete set of cube-scoped writes reachable from Notes (all inline)
Architectural suggestion (non-blocking)This is the third distinct place the same latent bug has surfaced, and the fix is per-site. Anyone who later adds a cube-scoped reference write onto another inherited nested object reintroduces it silently — and, as this PR shows, it fails only at query time on a cube the user didn't even mention. Fixing it once at the source (deep-clone inherited definitions in Praise
Not verifiedI did not run the test suites — this checkout has no installed dependencies ( No security or performance concerns: the new Rust allocations are on the error path only, and the JS changes add a handful of shallow copies at compile time. |
| if (preAggregation.outputColumnTypes) { | ||
| preAggregation.outputColumnTypes.forEach(column => { | ||
| column.name = this.evaluateReferences(cube.name, column.member, { originalSorting: true }); | ||
| }); | ||
| cube.preAggregations[preAggregationName] = { | ||
| ...preAggregation, | ||
| outputColumnTypes: preAggregation.outputColumnTypes.map(column => ({ | ||
| ...column, | ||
| name: this.evaluateReferences(cube.name, column.member, { originalSorting: true }), | ||
| })), | ||
| }; | ||
| } |
There was a problem hiding this comment.
This fix works (the preAggregations getter in CubeSymbols.createCube memoizes a fresh merged map per cube object, so writing the key back only affects this cube), but it's worth noting two things:
- It writes into a map whose setter is explicitly
set preAggregations(_v) { /* Dont allow to modify */ }. Mutating the object the getter hands out sidesteps that intent and depends on an invariant declared in another file. A short reference to that invariant in the comment ("the map returned by the getter is per-cube, only its values are shared") would keep this from looking wrong to the next reader. - The copy is conditional on
outputColumnTypes. Everything above (lines 731–783) still mutates the shared pre-aggregation object in place —delete preAggregation.timeDimension,refreshRangeStart = buildRangeStart, etc. Those happen to be idempotent and not cube-scoped, so no correctness bug today, but thebuildRangeStart/refreshRangeStartwarning now fires only for whichever cube is prepared first. If you copied the entry unconditionally at the top of the loop body instead, the whole loop would operate on a cube-owned object and the class of bug this PR is fixing couldn't come back here.
Minor: object spread drops accessors/non-enumerable properties. This file itself attaches a non-enumerable maskSql getter to members (line 847), so the pattern isn't hypothetical — if a pre-aggregation ever grows one, it would silently vanish for the extending cube.
| let grain = Self::grain_members(grain_state, parent_state); | ||
| // A granularity of the very dimension the sql reads looks like a match in | ||
| // the listing, so the one case where reading the grain is not enough to | ||
| // tell them apart is spelled out. | ||
| let target = dimension.clone().resolve_reference_chain().full_name(); | ||
| let hint = if grain | ||
| .iter() | ||
| .any(|m| Self::granular_time_dimension_base(m).as_ref() == Some(&target)) | ||
| { | ||
| format!( | ||
| " The grain carries {target} at a granularity, which is a value of its own and not \ | ||
| the dimension itself." | ||
| ) | ||
| } else { | ||
| String::new() | ||
| }; | ||
| let grain = if grain.is_empty() { | ||
| "no dimensions".to_string() | ||
| } else { | ||
| grain | ||
| .iter() | ||
| .map(Self::describe_grain_member) | ||
| .collect::<Vec<_>>() | ||
| .join(", ") | ||
| }; | ||
| Err(CubeError::user(format!( | ||
| "Multi-stage member {member} reads dimension {dimension}, which is not part of the \ | ||
| grain it is computed at. Add {dimension} to `grain.include` of {member}, or remove \ | ||
| it from the member's sql.", | ||
| grain it is computed at ({grain}).{hint} Add {dimension} to `grain.include` of \ | ||
| {member}, or remove it from the member's sql.", | ||
| member = member.full_name(), | ||
| dimension = dimension.full_name(), | ||
| ))) |
There was a problem hiding this comment.
Two small things in the new message building:
-
grainis bound twice with different types (Vec<Rc<MemberSymbol>>thenString). It reads fine here because the format arg wants the string, but shadowing a collection with its own rendering is easy to misread —grain_description(or inlining the render into theformat!) would be clearer. -
Path convention differs inside one sentence: the message prints
dimension = dimension.full_name()whilehintprintstarget = dimension.resolve_reference_chain().full_name(). For a view-scoped dimension those are different paths for the same thing, so the user could read "reads dimensionv.created_at… The grain carriesorders.created_atat a granularity" and think two dimensions are involved. Note also thatdimension_is_reachable(line 1260) uses the unresolvedfull_name()as its target, so the extraresolve_reference_chain()here is both redundant (the caller at line 371 already passes a resolved symbol) and inconsistent with the check it explains. Usingdimension.full_name()fortargetaligns all three.
| it('resolves grain references against the cube that declares the member', async () => { | ||
| const { cubeEvaluator } = await compile(true); | ||
|
|
||
| expect(cubeEvaluator.evaluatedCubes.base_fact.measures.linked.grain?.includeReferences) | ||
| .toEqual(['base_fact.d']); | ||
| expect(cubeEvaluator.evaluatedCubes.child_fact.measures.linked.grain?.includeReferences) | ||
| .toEqual(['child_fact.d']); | ||
| }); | ||
|
|
||
| it('resolves filter references against the cube that declares the member', async () => { | ||
| const { cubeEvaluator } = await compile(true); | ||
|
|
||
| expect(cubeEvaluator.evaluatedCubes.base_fact.measures.v_sum_all_dates.filter?.excludeReferences) | ||
| .toEqual(['base_fact.d']); | ||
| expect(cubeEvaluator.evaluatedCubes.child_fact.measures.v_sum_all_dates.filter?.excludeReferences) | ||
| .toEqual(['child_fact.d']); | ||
| }); |
There was a problem hiding this comment.
Good shape — asserting the parent's plan is identical with and without the extending cube is the right regression invariant, since "last cube prepared wins" is exactly what the bug was.
Two coverage gaps worth considering, both cheap to add to the existing model string:
- Sibling cubes (
child_a extends base_fact,child_b extends base_fact). With one child, the failure mode is "child overwrote base"; with two, it's also "child_b overwrote child_a", which is the case a per-site copy could still get wrong if any copy were shallow one level too few. - A chain (
grandchild extends child_fact). The prototype chain is two deep there, and the map merge happens per level.
Also, the view-default-filter and pre-aggregation blocks only assert the resolved reference strings, not that a query/rollup match still builds — an expect(buildSqlAndParams()).toBeDefined() on the child view would make those two mirror the strength of this block.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #11641 +/- ##
===========================================
+ Coverage 59.36% 79.98% +20.61%
===========================================
Files 229 489 +260
Lines 18241 102706 +84465
Branches 3679 3676 -3
===========================================
+ Hits 10829 82147 +71318
- Misses 6861 20008 +13147
Partials 551 551
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:
|
Summary
A cube that
extendsanother broke all multi-stage measures of the cube it extends — including queries that never mention the extending cube — with eitherCan't find join path to join '<child>', '<parent>'orMulti-stage member … reads dimension …, which is not part of the grain it is computed at, the latter naming a dimension the model already declares ingrain.include. Both models compile cleanly and/metais clean, so this only surfaces at query time. Reproduced on v1.7.5 and v1.7.19, so it is not a recent regression.Root cause is in
CubeEvaluator, not in the planner:extendshands the extending cube the very definitions of the cube it extends, and references resolved per cube were written into those shared objects, so the last cube prepared won.Changes
CubeEvaluator.evaluateMultiStageReferences: copy a member's nestedgrain/filterbefore writing the resolved references.prepareMemberscopies the member object itself, but not what hangs off it, so a parent'sgrain.include: [d]came out as["child.d"]— which pulls a cube into the query that is not part of it, hence either the missing join path or the unreachable grain dimension.CubeEvaluator.prepareViewFilters: same aliasing for a view extending another view —default_filtersreach it through the prototype, sobase_view's own default filter resolved tochild_view.currency, a memberbase_viewdoes not include.CubeEvaluator.preparePreAggregations: same aliasing foroutputColumnTypes— the base cube's rollup got column names scoped to the extending cube (child.id).orders.created_at (month)) instead of being folded into the symbol name, and the one case a listing cannot disambiguate gets a sentence of its own:The legacy planner is unaffected in effect: nothing outside the native planner reads
excludeReferences/keepOnlyReferences/includeReferences, and legacy has nograin:at all.Testing
packages/cubejs-schema-compiler/test/unit/extends-shared-definitions.test.ts, which fails on the unfixed code with exactly the two reported messages. It asserts the resolved references per cube, and that the plan of the parent's multi-stage measure is identical with and without the extending cube — under both planners (useNativeSqlPlanner: true/false), plus the view-default-filter and pre-aggregation cases. Reverting each fix hunk was verified to fail the matching test.cubesqlplannerintegration tests inmulti_stage/dimension_deps.rsfor the report, including a grain declared at another cube's dimension.cargo test --features integration-postgresacross the workspace: 1277 passed, 0 failed.cargo fmtclean.jest dist/test/unitincubejs-schema-compiler: 782/784, the two failures are a pre-existing ANSI-colour snapshot inerror-reporter.test.ts(passes underFORCE_COLOR=1).extensions,multi-stage,multi-stage-grain,multi-stage-filter,multi-stage-member-to-alias,multi-stage-order-by-view,multi-stage-time-shift-filter-params,pre-aggregations-multi-stage,pre-aggregations,views,view-default-value-filters.