fix(core): emit all relationship JOINs for multi-relationship calculated columns#2450
Conversation
… cols
A model with calculated columns spanning two or more distinct
relationships failed to plan with "No field named __relation__1.<col>"
— only the first relationship's JOIN was emitted.
Two interacting causes:
1. `ModelPlanNodeBuilder::merge_graph` (plan.rs) blindly added every
node from each per-calc-col sub-graph into the directed graph without
deduping. With calc cols spanning two relationships, the base model
ended up as multiple disconnected nodes in the merged graph.
2. `RelationChain::with_chain` (relation_chain.rs) walked the merged
graph as a linear chain via `find_edge(start, next)`, updating
`start = next` each iteration. Combined with the duplicate base-model
nodes, the walk hit a duplicate as `next`, `find_edge` returned None,
and the chain was silently truncated after the first relationship.
Fix:
- `merge_graph` dedupes nodes by `Dataset` value (Dataset derives
Eq + Hash) and skips parallel edges between the same pair.
- The fallback `add_node` for the base model in `build()` is now
conditional on the model not already being in the graph.
- `with_chain` tracks `prev` separately from the original `start` and
prefers `find_edge(prev, next)`, falling back to `find_edge(start,
next)` for star fan-out from the base model. Linear-chain behaviour is
preserved; star fan-out (A -> B, A -> C in one model) now works.
Adds `test_multiple_relationship_calculated_columns`: a base model with
two relationships and three calc cols (two through one relationship, one
through the other) transforms successfully. Without the fix the test
fails with `FieldNotFound { __relation__1.item_num }`. The test runs on
a dedicated 8 MiB-stack thread — the analyzer's recursion can exceed the
2 MiB default test-thread stack at several chained joins; production
tokio workers have a larger stack and are unaffected.
WalkthroughModifies model-plan graph construction to reuse existing base dataset nodes and avoid duplicate destination nodes/edges during graph merging. Updates RelationChain traversal to prefer edges from the previously visited node, falling back to the origin node, supporting star fan-out traversal. Adds a regression test covering multiple relationships with calculated columns. ChangesMulti-relationship calculated column fix
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
core/wren-core/core/src/mdl/mod.rs (1)
2760-2775: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winSubstring assertions are largely redundant with the model/table name and don't add much verification beyond the
.expect().
"market_retailer"(the relationship target's model/table name, which appears in the generated SQL regardless of whether the calculated columns are correctly projected) already contains both substrings"market"and"retailer", so those two assertions can pass even if themarket/retailercalculated columns were dropped from the outer projection. Similarly,item_numis also a physical column name on theproductmodel and will appear in inner subqueries independent of whether it correctly bubbles up through the calculated-column chain. The real regression protection here comes entirely fromresult.expect(...)not erroring; these threeassert!calls add little additional signal.Consider asserting on the qualified/aliased output form (e.g.
"dc_inventory".market, or the final outer projection list) or switching toassert_snapshot!like the rest of this test module, so a future regression that drops a column from the final projection — whilemarket_retailer/productstill appear internally — would actually be caught.Example tightened assertions
- assert!( - transformed.contains("market"), - "transformed SQL missing 'market': {transformed}" - ); - assert!( - transformed.contains("retailer"), - "transformed SQL missing 'retailer': {transformed}" - ); - assert!( - transformed.contains("item_num"), - "transformed SQL missing 'item_num': {transformed}" - ); + // Assert the outer projection (SELECT list before the first FROM) + // actually contains the calculated columns, not just that the + // related table/model names appear somewhere in the query. + let outer_select = transformed.split(" FROM ").next().unwrap_or_default(); + for col in ["market", "retailer", "item_num"] { + assert!( + outer_select.contains(col), + "outer projection missing '{col}': {transformed}" + ); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/wren-core/core/src/mdl/mod.rs` around lines 2760 - 2775, The current assertions in the multi-relationship calc cols test are too weak because they match substrings that also appear in model/table names or inner subqueries. Update the test around the transformed SQL produced from the `result.expect(...)` path to assert on the final outer projection form instead of raw substrings, such as checking qualified/aliased calculated columns or using a snapshot like the rest of `core/src/mdl/mod.rs`. This will ensure the test fails if `market`, `retailer`, or `item_num` are dropped from the final SELECT while still appearing elsewhere in the generated SQL.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@core/wren-core/core/src/mdl/mod.rs`:
- Around line 2760-2775: The current assertions in the multi-relationship calc
cols test are too weak because they match substrings that also appear in
model/table names or inner subqueries. Update the test around the transformed
SQL produced from the `result.expect(...)` path to assert on the final outer
projection form instead of raw substrings, such as checking qualified/aliased
calculated columns or using a snapshot like the rest of `core/src/mdl/mod.rs`.
This will ensure the test fails if `market`, `retailer`, or `item_num` are
dropped from the final SELECT while still appearing elsewhere in the generated
SQL.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d3ee46a2-94ee-4fdb-9474-30d30a726ccf
📒 Files selected for processing (3)
core/wren-core/core/src/logical_plan/analyze/plan.rscore/wren-core/core/src/logical_plan/analyze/relation_chain.rscore/wren-core/core/src/mdl/mod.rs
| // Production tokio worker threads have a larger stack and are | ||
| // unaffected; this is not infinite recursion. | ||
| std::thread::Builder::new() | ||
| .stack_size(8 * 1024 * 1024) |
There was a problem hiding this comment.
Minor / non-blocking: consider following the repo test convention here instead of the bespoke thread.
Every other test in mdl/mod.rs is a plain #[tokio::test], and the suite is already run under RUST_MIN_STACK=8388608 (see wren-core/CLAUDE.md and the CI test step). So the hand-rolled std::thread::Builder::stack_size(8 MiB) + current-thread runtime shouldn't be necessary — a plain #[tokio::test] should get the enlarged stack from the env var like the rest of the tests, and drops ~20 lines of boilerplate.
Could you switch test_multiple_relationship_calculated_columns to a plain #[tokio::test] (inlining _inner) to stay consistent? If it turns out the enlarged stack genuinely isn't picked up in that context, keeping the explicit thread is fine — just leave the justification comment as-is.
2 participants
Summary
A wren-core model with calculated columns spanning two or more distinct relationships fails to plan with
No field named __relation__1.<col>— only the first relationship's JOIN is emitted. This surfaces whenever a narrow fact model joins to multiple dimensions and projects an attribute from each (e.g. a DC-inventory fact withmarket/retailervia a market-retailer relationship anditem_numvia a product relationship).Root cause
Two interacting bugs:
ModelPlanNodeBuilder::merge_graph(plan.rs) blindly added every node from each per-calc-col sub-graph into the directed graph without deduping. With calc cols spanning two relationships, the shared base model ended up as multiple disconnected nodes in the merged graph.RelationChain::with_chain(relation_chain.rs) walked the merged graph as a linear chain viafind_edge(start, next), updatingstart = nexteach iteration. Combined with the duplicate base-model nodes, the walk hit a duplicate node asnext,find_edgereturnedNone, and the join chain was silently truncated after the first relationship.Fix
merge_graphdedupes nodes byDatasetvalue (Datasetalready derivesEq + Hash) and skips parallel edges between the same pair.add_nodefor the base model inbuild()is now conditional on the model not already being present.with_chaintracksprevseparately from the originalstartand prefersfind_edge(prev, next), falling back tofind_edge(start, next)for star fan-out from the base model. Linear-chain behaviour is preserved (prev == starton the first step); star fan-out (A → B and A → C in one model) now works.Test
Adds
test_multiple_relationship_calculated_columns: a base model with two relationships and three calculated columns (two through one relationship, one through the other), assertingSELECT *transforms successfully and projects all three. Without the fix the test fails withFieldNotFound { __relation__1.item_num }.The test runs on a dedicated 8 MiB-stack thread — the analyzer's recursion can exceed the 2 MiB default test-thread stack at several chained joins; production tokio worker threads have a larger stack and are unaffected (this is not infinite recursion).
Context
This is one of a small set of patches ZURU has carried on a wren-core fork; we are upstreaming them as we migrate our pin onto the current
Canner/WrenAImonorepo. Companion to #2449 (CLS wildcard-prune).Summary by CodeRabbit
Bug Fixes
Tests