-
Notifications
You must be signed in to change notification settings - Fork 1.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(schema-compiler) support join inheritance #8970
base: master
Are you sure you want to change the base?
fix(schema-compiler) support join inheritance #8970
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 8 Skipped Deployments
|
|
||
joins: | ||
- name: base_user_joins | ||
sql: "{CUBE}.user_id = {base_user_joins}.user_id" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would/should happen if the current cube is referenced by its name instead of CUBE
here? If it fails, would the error be comprehensible enough?
I feel like it would be great to have a test for this case as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, if the error is not very comprehensible and it's not that easy to fix, it might help to leave a note on join inheritance on this page: https://cube.dev/docs/product/data-modeling/concepts/code-reusability-extending-cubes
Hi @pauldheinrichs 👋 Thanks a ton for this contribution! I'll let my team mates review this PR. And I feel like this, while not being formally a breaking change (since it was documented and there's a bug filed), is still a somewhat radical deviation from the behavior that has been part of Cube for at least a couple of years already. I believe merging this can lead to data model breakages, so it look like we should consider this for a minor, no a patch, release. Cc @paveltiunov |
#7137 - the docs specify this is supported. So adding the support for it 🤷
Check List
Issue Reference this PR resolves
[For example #12]
Description of Changes Made (if issue reference is not provided)
[Description goes here]