Skip to content

fix(core): reject collapsed relationship joins - #2697

Open
primorLee wants to merge 1 commit into
Canner:mainfrom
primorLee:fix/relationship-join-collapse
Open

fix(core): reject collapsed relationship joins#2697
primorLee wants to merge 1 commit into
Canner:mainfrom
primorLee:fix/relationship-join-collapse

Conversation

@primorLee

@primorLee primorLee commented Aug 23, 2026

Copy link
Copy Markdown

Summary

  • Detect different DatasetLink values when calculated-column graphs merge the same model pair.
  • Return a planning error containing both join conditions instead of silently keeping whichever relationship was merged first.
  • Preserve deduplication when the incoming relationship is genuinely identical.

This is the short-term containment described in #2688. It prevents plausible-but-wrong SQL while full first-class relationship aliases remain a larger architectural change.

What failure does this repair?

The regression test builds bill-to and ship-to edges from orders to addresses with different join conditions, then merges the two graphs.

Before this change:

cargo test -p wren-semantic-core --lib rejects_distinct_relationships_between_the_same_models -- --nocapture

failed with:

distinct relationships must not collapse into one join: ()

The unit value shows merge_graph returned Ok and silently discarded the incoming ship-to edge. In the role-playing-dimension query from #2688, that causes both calculated cities to use the bill-to join.

After this change, the same graph merge returns a plan error that names both conflicting conditions.

How is it tested?

  • Targeted regression: 1 passed.
  • Full core library suite: 153 passed, 0 failed.
  • cargo clippy -p wren-semantic-core --all-targets -- -D warnings: passed.
  • The test is a normal core library unit test and runs in the default Rust test target.

Duplicate check

Searched open PRs for #2688, relationship handles, and distinct join aliases; the issue still has no linked PR. #976 was also reviewed and is an unrelated, service-only semantics-enrichment alias change that does not touch wren-core or relationship planning.

Addresses #2688.

Summary by CodeRabbit

  • Bug Fixes
    • Improved query planning when multiple relationships connect the same datasets.
    • Distinct join conditions are now preserved and reported as planning errors instead of being silently ignored.

@github-actions github-actions Bot added rust Pull requests that update rust code core labels Aug 23, 2026
@coderabbitai

coderabbitai Bot commented Aug 23, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 49dde44a-a85c-439e-9ec8-c4f009604fd2

📥 Commits

Reviewing files that changed from the base of the PR and between f2841bc and 5eed46a.

📒 Files selected for processing (1)
  • core/wren-core/core/src/logical_plan/analyze/plan.rs

Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.


Walkthrough

merge_graph now preserves identical relationship edges and reports conflicting relationships between the same dataset nodes. A regression test verifies that the error includes both distinct join conditions.

Changes

Relationship merge validation

Layer / File(s) Summary
Detect and test conflicting dataset relationships
core/wren-core/core/src/logical_plan/analyze/plan.rs
merge_graph skips identical relationships and returns a planning error for different relationships between the same dataset nodes. A unit test verifies that both join conditions appear in the error.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Merge Risk: ⚪ Minimal · up to 5eed4

This change rejects conflicting relationship joins instead of silently producing potentially incorrect SQL, while preserving identical-relationship deduplication. No actionable merge-blocking risk remains after normal checks and review.

Suggested reviewers: goldmedal

Poem

I’m a rabbit guarding every link,
No hidden joins shall slip or shrink.
Same paths merge with a gentle hop,
Conflicting paths make planning stop.
Two join clues now shine in sight—
Thump, thump, the graph is right!

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly identifies the core fix: rejecting collapsed relationship joins.
Description check ✅ Passed The description includes the required summary, observed failure and output, testing details, and duplicate check.
Docstring Coverage ✅ Passed Docstring check was indeterminate for this PR — some files could not be analyzed in time. Not blocking.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

core rust Pull requests that update rust code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant