Skip to content
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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

pauldheinrichs
Copy link

@pauldheinrichs pauldheinrichs commented Nov 19, 2024

#7137 - the docs specify this is supported. So adding the support for it 🤷

image

Check List

  • Tests has been run in packages where changes made if available
  • Linter has been run for changed code
  • Tests for the changes have been added if not covered yet
  • Docs have been added / updated if required

@pauldheinrichs pauldheinrichs requested a review from a team as a code owner November 19, 2024 17:01
Copy link

vercel bot commented Nov 19, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

8 Skipped Deployments
Name Status Preview Comments Updated (UTC)
examples-angular-dashboard ⬜️ Ignored (Inspect) Visit Preview Nov 19, 2024 5:19pm
examples-react-d3 ⬜️ Ignored (Inspect) Visit Preview Nov 19, 2024 5:19pm
examples-react-dashboard ⬜️ Ignored (Inspect) Visit Preview Nov 19, 2024 5:19pm
examples-react-data-table ⬜️ Ignored (Inspect) Visit Preview Nov 19, 2024 5:19pm
examples-react-highcharts ⬜️ Ignored (Inspect) Visit Preview Nov 19, 2024 5:19pm
examples-react-material-ui ⬜️ Ignored (Inspect) Visit Preview Nov 19, 2024 5:19pm
examples-react-pivot-table ⬜️ Ignored (Inspect) Visit Preview Nov 19, 2024 5:19pm
examples-vue-query-builder ⬜️ Ignored (Inspect) Visit Preview Nov 19, 2024 5:19pm

@github-actions github-actions bot added the pr:community Contribution from Cube.js community members. label Nov 19, 2024

joins:
- name: base_user_joins
sql: "{CUBE}.user_id = {base_user_joins}.user_id"
Copy link
Member

@igorlukanin igorlukanin Nov 22, 2024

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.

Copy link
Member

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

@igorlukanin
Copy link
Member

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

@KSDaemon
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr:community Contribution from Cube.js community members.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants