gouline / dbt-metabase

dbt + Metabase integration
https://pypi.org/project/dbt-metabase/
MIT License
442 stars 63 forks source link

feat: Enable using dbt meta to set FK relationships #92

Closed soltanianalytics closed 2 years ago

soltanianalytics commented 2 years ago

Rationale for this PR: I've had my fair share of issues with setting the PK/FK relationships via the dbt relationships test. My current issue is that the existing logic is a two-way street and which field in a relationship is set as PK and which as FK is basically random.

This is because the manifest's child_map contains relationships for both the tested field as well as the related model. This is sort-of OK because if the fields are named differently (e.g. orders.customer_id vs customers.id), the un-intended relationship is not actually set because it can't find the field. However, if following a different convention (i.e. orders.customer_id and customers.customer_id), this isn't the case anymore, and the PK and FK fields may be switched up.

Instead of trying to fix the whole thing in the code base, I thought it wouldn't harm to just allow setting relationships via meta instead of forcing the user to set relationship tests (among other things, because a user may not want to use those tests in production to begin with for various reasons). Hence, this PR.

gouline commented 2 years ago

I'm ok with that approach, what do you think @z3z1ma?

z3z1ma commented 2 years ago

Yes I think its more pluses than downsides. It also solves for self referencing relationships being able to be propagated to Metabase. I also have had instances where I want to relationship in Metabase (which in and of itself expresses the relationship more as an unrealized/lazy join and is useful in that way) but dont want it in my CICD.

:+1:

gouline commented 2 years ago

What happens if you specify both, a relationship test and meta fields? Is what happens now the intended behaviour? Is it worth documenting which takes precedence?

soltanianalytics commented 2 years ago

Looks to me like the meta fields supercede the relationship test. I would consider that preferable this way, too. Happy to add a line in the docs to indicate this 👍