Skip to content

fix(schema-compiler): a cube that extends another broke the parent's multi-stage measures - #11641

Merged
waralexrom merged 2 commits into
masterfrom
tesseract-extends-breaks-multi-stage
Aug 25, 2026
Merged

fix(schema-compiler): a cube that extends another broke the parent's multi-stage measures#11641
waralexrom merged 2 commits into
masterfrom
tesseract-extends-breaks-multi-stage

Conversation

@waralexrom

Copy link
Copy Markdown
Member

Summary

A cube that extends another broke all multi-stage measures of the cube it extends — including queries that never mention the extending cube — with either Can't find join path to join '<child>', '<parent>' or Multi-stage member … reads dimension …, which is not part of the grain it is computed at, the latter naming a dimension the model already declares in grain.include. Both models compile cleanly and /meta is 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: extends hands 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 nested grain / filter before writing the resolved references. prepareMembers copies the member object itself, but not what hangs off it, so a parent's grain.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_filters reach it through the prototype, so base_view's own default filter resolved to child_view.currency, a member base_view does not include.
  • CubeEvaluator.preparePreAggregations: same aliasing for outputColumnTypes — the base cube's rollup got column names scoped to the extending cube (child.id).
  • Tesseract: the unreachable-dimension report now spells out the grain the member is computed at. It used to name only 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. Granularities are named separately (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:
Multi-stage member orders.amount_first_half_of_month reads dimension orders.created_at,
which is not part of the grain it is computed at (customers.city, orders.created_at (month)).
The grain carries orders.created_at at a granularity, which is a value of its own and not
the dimension itself. Add orders.created_at to `grain.include` of … .

The legacy planner is unaffected in effect: nothing outside the native planner reads excludeReferences / keepOnlyReferences / includeReferences, and legacy has no grain: at all.

Testing

  • New 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.
  • Two new cubesqlplanner integration tests in multi_stage/dimension_deps.rs for the report, including a grain declared at another cube's dimension.
  • cargo test --features integration-postgres across the workspace: 1277 passed, 0 failed. cargo fmt clean.
  • jest dist/test/unit in cubejs-schema-compiler: 782/784, the two failures are a pre-existing ANSI-colour snapshot in error-reporter.test.ts (passes under FORCE_COLOR=1).
  • Docker-Postgres integration files, run individually and green: 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.

waralexrom and others added 2 commits August 25, 2026 15:22
`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>
@waralexrom
waralexrom requested a review from a team as a code owner August 25, 2026 13:26
@github-actions github-actions Bot added rust Pull requests that update Rust code javascript Pull requests that update Javascript code labels Aug 25, 2026
@claude

claude Bot commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

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
  • Gather context / read diff (HEAD~2..HEAD)
  • Verify the aliasing analysis against CubeSymbols.createCube / prepareMembers
  • Review the three CubeEvaluator fixes
  • Review the Tesseract report change
  • Review tests
  • Post inline comments

Verdict

The diagnosis holds up under reading. CubeSymbols.createCube links the extending cube's object to the parent's via Object.setPrototypeOf, and allDefinitions / the preAggregations getter build a fresh map per cube whose values are the same objects. prepareMembers then does members[memberName] = { ...members[memberName], ownedByCube } — a shallow copy, so member.grain / member.filter still point at the object the parent's member also points at. Writing grain.includeReferences there is genuinely last-writer-wins, which explains both reported symptoms (a base_fact.d grain surfacing as child_fact.d pulls a cube into the query that isn't in it → missing join path, or a grain that no longer holds the dimension the SQL reads → unreachable-dimension). All three fixed sites are cube-scoped resolution written into an inherited nested object; the surrounding accessPolicy path already avoids this by R.cloneing the parent's policies, so the fixes are consistent with existing precedent.

I confirmed the fixed sites are the complete set of cube-scoped writes reachable from prepareCube: groupByReferences / reduceByReferences / addGroupByReferences / timeShiftReferences are written on the member object itself (already per-cube via prepareMembers) or into freshly built objects, and prepareAccessPolicy's filter.memberReference is safe via the R.clone above.

Notes (all inline)

Sev Where Note
Low CubeEvaluator.ts:788 The pre-agg fix mutates the map the preAggregations getter hands out, whose setter says "Dont allow to modify" — correct, but relies on a cross-file invariant worth naming. Copying the entry unconditionally at the top of the loop (rather than only when outputColumnTypes exists) would put the whole loop on a cube-owned object and close the site for good.
Low multi_stage_query_planner.rs:417 grain shadowed VecString; and the message mixes resolved (hint) with unresolved ({dimension}) paths, which for a view-scoped dimension names the same thing two ways. dimension_is_reachable uses the unresolved name, so dropping resolve_reference_chain() aligns all three.
Low extends-shared-definitions.test.ts:104 Missing sibling-cubes (a extends base, b extends base) and two-level-chain cases; the view-filter and pre-agg blocks assert reference strings only, not that a query still builds.

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 allDefinitions / the preAggregations getter, as accessPolicy already does with R.clone) would make the class of bug unreachable. Understandable if you'd rather keep this PR surgical — in that case it may be worth a comment at prepareMembers noting that member copies are shallow and nested objects are still shared.

Praise

  • The regression invariant chosen — the parent's plan must be byte-identical with and without the extending cube, under both planners — is much stronger than asserting a specific SQL string, and it's the shape that actually pins this bug.
  • Comments explain why the copy is needed rather than what the code does, and the Rust change turns a message that only restated the model into one that names the grain actually in effect. The at a granularity sentence covers the one case a flat listing can't disambiguate.
  • Confirming the legacy planner reads none of excludeReferences / keepOnlyReferences / includeReferences is the right scoping check to have made.

Not verified

I did not run the test suites — this checkout has no installed dependencies (node_modules absent, no dist/), and the schema-compiler Jest config runs against compiled dist/, so a build + install was out of scope for this review. I'm relying on the run results reported in the PR description; nothing in my reading contradicts them.

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.

· branch `tesseract-extends-breaks-multi-stage`

Comment on lines 788 to 796
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 }),
})),
};
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. 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.
  2. 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 the buildRangeStart/refreshRangeStart warning 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.

Comment on lines +417 to 448
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(),
)))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two small things in the new message building:

  1. grain is bound twice with different types (Vec<Rc<MemberSymbol>> then String). 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 the format!) would be clearer.

  2. Path convention differs inside one sentence: the message prints dimension = dimension.full_name() while hint prints target = 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 dimension v.created_at … The grain carries orders.created_at at a granularity" and think two dimensions are involved. Note also that dimension_is_reachable (line 1260) uses the unresolved full_name() as its target, so the extra resolve_reference_chain() here is both redundant (the caller at line 371 already passes a resolved symbol) and inconsistent with the check it explains. Using dimension.full_name() for target aligns all three.

Comment on lines +104 to +120
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']);
});

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

codecov Bot commented Aug 25, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 88.88889% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 79.98%. Comparing base (3b0c5b7) to head (5123f13).
⚠️ Report is 2 commits behind head on master.

Files with missing lines Patch % Lines
...bejs-schema-compiler/src/compiler/CubeEvaluator.ts 88.88% 2 Missing ⚠️
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               
Flag Coverage Δ
cube-backend 59.39% <88.88%> (+0.02%) ⬆️
cubesql 84.42% <ø> (?)

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.

@waralexrom
waralexrom merged commit 657f4e1 into master Aug 25, 2026
227 of 230 checks passed
@waralexrom
waralexrom deleted the tesseract-extends-breaks-multi-stage branch August 25, 2026 15:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

javascript Pull requests that update Javascript code rust Pull requests that update Rust code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants