gouline / dbt-metabase

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

feat: Allow explicit null semantic types #132

Closed gouline closed 2 years ago

gouline commented 2 years ago

Providing a null metabase.semantic_type explicitly writes it to the API, omitting it leaves the existing one.

Changes behaviour in #118 and #119

Resolves #131

gouline commented 2 years ago

@willbryant Can you test and see if this works for you?

willbryant commented 2 years ago

Just testing it out right now actually. The code looks good to me but it doesn't seem to actually be doing anything.

I added a debugging line and it's definitely running and definitely setting the key to None, but nothing's changing in the Metabase admin UI.

Could be a bug in Metabase itself, have you tried it?

willbryant commented 2 years ago

Ah no sorry, I made a mistake in my testing setup, it's working.

Yep, LGTM, thanks :)

gouline commented 2 years ago

Only saw your response after debugging my Metabase instance with Chrome developer tools 😆

willbryant commented 2 years ago

😬 sorry :)

jack-cook-repo commented 2 years ago

@gouline - I'm going to confirm this next week, but our Metabase sync started showing some weird behaviour after using 0.9.5.

Numerical (integer) BigQuery columns that had no semantic type set in dbt were being assigned as categorical Source semantic type in Metabase. The columns in question had "channel" in the name which Metabase confirmed with me was the reason for it being inferred as Source when we hadn't specified a type (link to Metabase code). This caused a lot of weird behaviour such as number formatting disappearing and filters not working.

I re-ran the sync with 0.9.4 and it got rid of the issues.

In short, this may be intentional behaviour - as per https://github.com/gouline/dbt-metabase/issues/131, but for us being able to not having to set semantic types for every column and avoid Metabase's automatic inference is quite valuable as a default.

Potentially, there could be a flag to set in the sync to avoid Metabase's attempts to infer semantic types as in <=0.9.4 (this may already exist but I'm not aware of it). Or the solution could simply be for us to stick with 0.9.4 which works for the time being, but thought I'd raise this either way.

gouline commented 2 years ago

@jack-cook-repo Not sure I understand, do you want the inferred types removed by default? If so, 0.9.4 provided no way of doing that, you could only specify a non-null semantic type to replace it.

Not specifying a type (omitting the meta field) leaves the inferred type unchanged in both, 0.9.4 and 0.9.5. What's different in 0.9.5 is that you can explicitly set the semantic type to null (as opposed to omitting it), which removes the inferred type. In 0.9.4, omitting it and setting it to null had the same behaviour, both left the inferred type unchanged.

Resetting inferred types by default was removed in 0.9.2 when the regression #118 was fixed. Not sure why 0.9.4 works any different to 0.9.5 for you.

No plans to introduce another flag though (enough configs to get confused over as-is), you can set semantic types to null whenever you want the inferred type gone.

jack-cook-repo commented 2 years ago

Ah - interesting

do you want the inferred types removed by default?

Yes, we only want a column's semantic type to be set in Metabase if we provide it in the dbt schema, otherwise we want it left as No semantic type as Metabase's efforts to guess semantic types can end up with issues as we've seen with "channel" columns.

Essentially we've only seen this issue with Metabase automatically inferring a column's type (instead of just leaving it as No semantic type) this week, and as 0.9.5 was released a couple of days ago (and that we weren't pinning our package versions correctly) we'd figured it was the new release that had caused the change.

Odd that using 0.9.4 seems to fix it for us then, I'm going to put this change in place (in our codebase) next week and will let you know if it seems to be the cause.

jack-cook-repo commented 2 years ago

So this is one of our data models running with 0.9.5, where the type is being automatically inferred:

image

And I double checked we aren't specifying types in dbt:

image

After pinning dbt-metabase to 0.9.4 and rerunning the sync:

image

The issue is resolved from our end, but odd that seems to be the behaviour