fix(schema-compiler): keep a view's independent join_path root off its own longer paths - #11817
waralexrom wants to merge 3 commits into
Conversation
A view can declare a cube twice: once at the root of its join tree (`join_path: boards`) and once as the tail of a longer path (`join_path: locations.boards`) for another member. A query over the members of the independent root alone must be planned against that root. Adds coverage on both planners for the shape and for its counterpart - the member declared under the longer path still walking it: - a schema-compiler unit test on leaf measure paths, multiplied measures and rollup matching; - Tesseract tests on the join hints collected for each of the view's members, and on the SQL and pre-aggregation the query plans to. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…s own longer paths A view records the join path of every cube it includes through more than one segment, and a bare join hint into any cube on such a path is rewritten to the prefix of that path. That is what makes a member proxied across cubes follow the route the view declares rather than a shorter one the join graph would find on its own. The rewrite has no way of telling which of the view's entries a hint came from, so it also caught the cube the same view includes at the root of its join tree. A view declaring both `join_path: boards` and `join_path: locations.boards` had every bare `boards` hint moved onto `locations.boards`: a query over the members of the independent root alone was planned as if it needed the fan-out join, its leaf measures resolved to the dotted form, it counted as having multiplied measures, and a rollup declared on `boards` alone stopped matching - the query fell back to scanning `locations LEFT JOIN boards`, a cube it never references. Views now also record the cubes they include at the root of their join tree, and a bare hint into one of those is left alone instead of being moved onto a longer path of the same view. A hint that already sits at the head of a path is unaffected, so the route of a member declared under a longer path does not change. Both planners derive the rewrite from the same view metadata and both needed the fix. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The root cubes were pooled across every view the query touches, so one view's root cube could keep another view's join path from being applied to a bare hint. They now sit in the same per-view bucket as that view's join paths, which is how Tesseract already carries them. Also makes the tests for the other side of the rule discriminating. They covered a dimension proxied straight from the cube, which never yields a bare hint for the rule to act on, so they held either way. They now cover a calculated measure declared under the longer path, whose components do yield bare hints, and assert the hints and the resolved leaves rather than the presence of a table name. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
github-actions
Bot
added
rust
|
Claude finished @waralexrom's task in 5m 46s —— View job Review complete — no blockers; 1 medium, 3 low, posted inlineFull review
VerdictThe change is sound and the two halves agree. I traced the ordering semantics on both sides and they match: JS iterates per view then per path with an early return, Rust's Findings
Notes, no action needed
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| JoinHintItem::Single(cube_name) => { | ||
| if let Some(path) = join_map.path_to(cube_name) { | ||
| *hint = JoinHintItem::Vector(path.to_vec()); | ||
| } |
There was a problem hiding this comment.
This rewrite pass mutates through iter_mut(), so it bypasses the redundancy rule JoinHints::push enforces — join_hints.rs:34-38 explicitly drops a Single(name) that directly follows a Vector(v) with v.last() == name. Leaving the bare hint in place is exactly what now produces that shape, and it is a new shape rather than a pre-existing one.
Concretely, kpi.scrap_pct in the new fixture. Before this change the bare hint was rewritten to the same vector and collapsed; after it, test_join_hints_view_dotted_path_measure asserts two items: [["locations","boards"], "boards"]. The "boards" entry is redundant against the vector that already ends in it, and it is not inert — JoinGraph.buildJoin (JoinGraph.ts:186) tries every hint as a candidate root and keeps the tree with the fewest joins. So this adds boards as a candidate root for any query whose only boards member is declared under the dotted path.
In this fixture the candidate loses (nothing joins boards → locations, so that root can't cover the other hint) and the integration test confirms the plan is unchanged. But in a model where the two cubes are joinable both ways, the extra root becomes a tie-break the query didn't previously have.
Suggest re-normalising after the pass — rebuild via JoinHints::push instead of from_items, so a bare hint that is the tail of the preceding path is dropped the same way it would have been at collection time. The JS half has the same shape (enrichHintsWithJoinMap feeding R.uniq, which won't collapse 'boards' against ['locations','boards']); view-independent-join-path-roots.test.ts:168 pins the three-hint result, so both sides would need the same treatment.
| /// The prefix of a declared join path that ends at `cube_name`, to be used | ||
| /// in place of a bare hint into that cube. `None` when no declared path | ||
| /// leads there, or when the view reaches the cube at its root as well. | ||
| pub fn path_to(&self, cube_name: &String) -> Option<&[String]> { | ||
| for path in self.paths.iter() { | ||
| if let Some(index) = path.iter().position(|part| part == cube_name) { | ||
| // A cube the view also includes at the root of its join tree is | ||
| // reachable on its own, so a bare hint into it must not be moved | ||
| // onto a longer path - that path serves the members included | ||
| // under it. | ||
| if index > 0 && self.root_cubes.contains(cube_name) { |
There was a problem hiding this comment.
The doc comment on path_to already states the rule ("None when … the view reaches the cube at its root as well"), and the four-line inline comment below restates it in longer form. One of the two carries the meaning; the inline one is the redundant copy. Same text also appears verbatim in BaseQuery.js:592-595, where it is load-bearing since there's no doc comment there.
Separate, non-blocking: ViewJoinMap defines "root of the view" as "declared with a single-segment join_path", while fallback_hints_for_measure (multi_fact_join_groups.rs:570-578) derives roots as "path heads that no other path reaches" and ignores root_cubes entirely. Two definitions of the same concept, neither aware of the other. The PR body explains why the second is left alone, which is fair, but a one-line pointer from one to the other would stop the next reader from assuming they agree.
|
|
||
| return hints.map(hint => { | ||
| if (Array.isArray(hint)) { | ||
| return hint; | ||
| } | ||
|
|
||
| for (const path of allPaths) { | ||
| const hintIndex = path.indexOf(hint); | ||
| if (hintIndex !== -1) { | ||
| return path.slice(0, hintIndex + 1); | ||
| for (const { paths, rootCubes } of views) { | ||
| for (const path of paths) { | ||
| const hintIndex = path.indexOf(hint); | ||
| if (hintIndex !== -1) { | ||
| // A cube this view also includes at the root of its join tree is | ||
| // reachable on its own, so a bare hint into it must not be moved | ||
| // onto a longer path - that path serves the members included | ||
| // under it. | ||
| if (hintIndex > 0 && rootCubes.has(hint)) { | ||
| return hint; | ||
| } | ||
| return path.slice(0, hintIndex + 1); | ||
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Worth noting the residual order-sensitivity this leaves, since the fix removes it for one shape but not its sibling. path.indexOf(hint) takes the first path that mentions the cube, so for
- join_path: locations.boards # listed first
includes: [product]
- join_path: boards.tags
includes: [tag_name]boards heads a path but has no single-segment entry, so it isn't in rootCubes; a bare boards hint matches locations.boards at index 1 and gets rewritten. List the two entries the other way round and it matches boards.tags at index 0 and stays bare. Same model, different answer.
Not introduced here — the flattened version had it too — and the rootCubes guard actually makes the declared-root case order-independent. But if the intent is "a cube the view can reach at its root is left alone", the head-of-a-path case is the same situation with the declaration implicit rather than explicit. Fine to leave; flagging so it's a known edge rather than a surprise.
| - name: kpi | ||
| cubes: | ||
| - join_path: locations | ||
| includes: | ||
| - count | ||
| - pos | ||
| - join_path: locations.boards | ||
| includes: | ||
| - product | ||
| - scrap_pct | ||
| - join_path: boards | ||
| includes: | ||
| - yield_pct | ||
| - board_id |
There was a problem hiding this comment.
Coverage gap: nothing pins the index > 0 half of the guard.
With this view, root_cubes is {locations, boards} and paths is [[locations, boards]]. Delete index > 0 from path_to (and from enrichHintsWithJoinMap) and the only behaviour that changes is the locations hint: it stops becoming Vector(["locations"]) and stays Single("locations"). No test in this PR asserts the hints for kpi.count or kpi.pos, so the guard can be removed silently.
Cheap fix: a test_join_hints_view_root_measure asserting collect_join_hints(kpi.count).items() == [v(&["locations"])], alongside the four new cases. Worth having given the Single vs single-element Vector distinction is exactly what the comment above raises about candidate roots.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #11817 +/- ##
==========================================
- Coverage 84.67% 84.66% -0.01%
==========================================
Files 261 261
Lines 86648 86648
==========================================
- Hits 73366 73362 -4
- Misses 13282 13286 +4
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:
|
1 participant
Fixes #11680.
Problem
A view can reach the same cube two ways: at the root of its join tree
(
join_path: boards) and as the tail of a longer path declared for anothermember (
join_path: locations.boards). A query that touches only the membersof the independent root was planned as if it needed the fan-out join through
locations:locations.boards.good_countinstead of
boards.good_count);on the strict path that requires the dimensions to be equal;
boardsalone therefore stopped matching, and the queryfell back to scanning
SELECT DISTINCT ... FROM locations LEFT JOIN boards-a cube it never references.
The same query straight against the cube, or through a view with independent
roots only, matched the rollup fine.
Cause
A view records the join path of every cube it includes through more than one
segment, and a bare join hint into any cube on such a path is rewritten to the
prefix of that path. That is what makes a member proxied across cubes follow the
route the view declares rather than a shorter one the join graph would find on
its own - see
views-join-order-join-maps.test.ts.The rewrite has no way of telling which of the view's entries a hint came from,
so it also caught the cube the same view includes at its root: every bare
boardshint was moved ontolocations.boards,locationsbecame the root ofthe join tree, and
boardswas marked multiplied.What changed
Views now also record the cubes they include at the root of their join tree -
the entries whose
join_pathis a single segment - and a bare hint into one ofthose is left alone instead of being moved onto a longer path of the same view.
A hint that already sits at the head of a path is unaffected, so the route of a
member declared under a longer path does not change.
Both planners derive the rewrite from the same view metadata and both needed the
fix: with only the JS half, the leaf measures and the multiplied flag came out
right but Tesseract still built the
locations LEFT JOIN boardskeys subqueryand still refused the rollup.
How it was verified
view-independent-join-path-roots.test.tscovers the shape end to end:leaf measure paths, the multiplied flag, and the rollup actually being used.
members, and the SQL and pre-aggregation the query plans to.
the longer path, whose components do yield bare hints, still walking that path
and still fanning out.
cargo test -p cubesqlplanner: 1378 passed.cubejs-schema-compilerunit suite: 955 passed, with only the pre-existingerror-reporterANSI-colour snapshot failures, which fail onmastertoo.cubejs-schema-compilerPostgres integration suite against a real databasewith a locally built native planner: 51/51 suites, 577 tests.
Risks
does not reproduce on current
master; it resolves correctly and uses therollup. The reporter's real model almost certainly also declares a path into
the same cube, which is the shape fixed here. Worth confirming with them.
under a longer path, and is queried for both aliases at once now plans as two
join trees. The outer projection identifies a measure by its leaf, so both
aliases read the same group and the other one is computed and discarded. That
is the planner's measure-identity model rather than something this change can
settle locally - two aliases of one cube measure are one measure, so one of
the two contradictory answers has to lose. Before this change the multiplied
group won for both; now the root group does.
fallback_hints_for_measureinmulti_fact_join_groups.rsstill derives aview's roots from the declared paths alone, so a hint-less member expression
on a view with two unrelated roots still resolves to one of them instead of
raising the "don't share a single root" error it has for that case. Left alone
on purpose: it is a pre-existing gap in a different feature, and closing it
turns queries that answer today into hard errors.
🤖 Generated with Claude Code