-
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 |
One test fails: 2024-12-09T22:33:25.9146899Z ● YAMLCompiler › should correctly handle extensions with joins
2024-12-09T22:33:25.9147339Z 'second_prop' not found for path 'active_users.second_prop'
2024-12-09T22:33:25.9147649Z 624 |
2024-12-09T22:33:25.9148059Z 625 | if (!this.evaluatedCubes[cubeAndName[0]][type][cubeAndName[1]]) {
2024-12-09T22:33:25.9148588Z > 626 | throw new UserError(`'${cubeAndName[1]}' not found for path '${path}'`);
2024-12-09T22:33:25.9148949Z | ^
2024-12-09T22:33:25.9149158Z 627 | }
2024-12-09T22:33:25.9149350Z 628 |
2024-12-09T22:33:25.9149676Z 629 | return this.evaluatedCubes[cubeAndName[0]][type][cubeAndName[1]];
2024-12-09T22:33:25.9150155Z at CubeEvaluator.byPath (src/compiler/CubeEvaluator.ts:626:13)
2024-12-09T22:33:25.9150626Z at CubeEvaluator.parsePath (src/compiler/CubeEvaluator.ts:634:10)
2024-12-09T22:33:25.9151083Z at BaseDimension.path (src/adapter/BaseDimension.ts:127:37)
2024-12-09T22:33:25.9151451Z at src/adapter/BaseQuery.js:1916:15
2024-12-09T22:33:25.9151807Z at _map (../../node_modules/ramda/src/internal/_map.js:7:19)
2024-12-09T22:33:25.9152179Z at map (../../node_modules/ramda/src/map.js:83:14)
2024-12-09T22:33:25.9152568Z at ../../node_modules/ramda/src/internal/_dispatchable.js:50:15
2024-12-09T22:33:25.9152967Z at ../../node_modules/ramda/src/internal/_curry2.js:26:18
2024-12-09T22:33:25.9153357Z at f1 (../../node_modules/ramda/src/internal/_curry1.js:19:17)
2024-12-09T22:33:25.9153738Z at ../../node_modules/ramda/src/internal/_pipe.js:3:14
|
#7137 - the docs specify this is supported. So adding the support for it 🤷
Check List