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

Change merge alias defaults #840

Merged
merged 3 commits into from
Oct 31, 2024
Merged

Change merge alias defaults #840

merged 3 commits into from
Oct 31, 2024

Conversation

benc-db
Copy link
Collaborator

@benc-db benc-db commented Oct 30, 2024

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

  • I have run this code in development and it appears to resolve the stated issue
  • This PR includes tests, or tests are not required/relevant for this PR
  • I have updated the CHANGELOG.md and added information about my change to the "dbt-databricks next" section.

@benc-db
Copy link
Collaborator Author

benc-db commented Oct 30, 2024

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

@mi-volodin for viz.

@mi-volodin
Copy link
Contributor

mi-volodin commented Oct 31, 2024

@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
Copy link
Collaborator Author

benc-db commented Oct 31, 2024

@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.

@benc-db benc-db merged commit 6cb6eaa into main Oct 31, 2024
19 of 21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants