search

databricks / dbt-databricks

A dbt adapter for Databricks.
https://databricks.com
Apache License 2.0
226 stars 119 forks source link

Change merge alias defaults #840

Closed benc-db closed 3 weeks ago

benc-db commented 3 weeks ago

Resolves #841

Description

During canary usage of the 1.9.0b2, we discovered that some users had overwritten some existing macros to rely on the previous aliases (since they are used through dbt to interact with predicates). This PR changes the defaults to the previous aliases.

Checklist

benc-db commented 3 weeks ago

Reminder to Ben, need to update the docs on the dbt site.

@mi-volodin for viz.

mi-volodin commented 3 weeks ago

@benc-db thanks for fixing this. If there are any further issues with these merge capabilities - you can ping me, I can take over.

However, I didn't get the justification of a "bug" statement in #841. Some users assumed the aliases to be DBT_INTERNAL_X, but to the best of my knowledge, these aliases are not fixed in any design document for dbt.

If that is correct, then making an assumption and binding with the internal implementation - is an abstraction leak. With exactly such consequences. So it looks not like the bug for me, because internal implementation should not be constrained by user expectations. It sounds more like the feature request to tolerate specific user-side design decisions.

Just out of curiosity - have you considered the opposite option? I mean to actually motivate such users to migrate to specifically defined aliases as part of the upgrade?

UPD. I re-read and it felt a little like an objection 😬 It's not, I am just "tuning" my vision. Will be really helpful in case of probable further contributions from my side. I think I could have avoided this extra work if I had had a better understanding of these expectations before: this "default" aliases topic was surfaced during the tests. So the decision to keep other aliases was not an accident.

benc-db commented 3 weeks ago

@benc-db thanks for fixing this. If there are any further issues with these merge capabilities - you can ping me, I can take over.

However, I didn't get the justification of a "bug" statement in #841. Some users assumed the aliases to be DBT_INTERNAL_X, but to the best of my knowledge, these aliases are not fixed in any design document for dbt.

If that is correct, then making an assumption and binding with the internal implementation - is an abstraction leak. With exactly such consequences. So it looks not like the bug for me, because internal implementation should not be constrained by user expectations. It sounds more like the feature request to tolerate specific user-side design decisions.

Just out of curiosity - have you considered the opposite option? I mean to actually motivate such users to migrate to specifically defined aliases as part of the upgrade?

UPD. I re-read and it felt a little like an objection 😬 It's not, I am just "tuning" my vision. Will be really helpful in case of probable further contributions from my side. I think I could have avoided this extra work if I had had a better understanding of these expectations before: this "default" aliases topic was surfaced during the tests. So the decision to keep other aliases was not an accident.

Hi Dmitry. There are some external constraints that inform rolling back as opposed to treating this like users just depending on internal details: 1.) dbt has made a promise to users that when they select 'always latest' they will not be broken by updates 2.) the nature of dbt is that overriding macros is quite common, which means there's kind of no such thing as 'internal' when it comes to Jinja macros 3.) I think the previous mechanism for specifying incremental predicates pretty much required users to reference these 'internal' alias names.

Number 2 bites me from time to time, and not being aware of number 3 was a big miss on my part that I should have made you aware of during the PR.