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

redundant to include incremental materialization #102

Merged
merged 2 commits into from
Feb 15, 2021

Conversation

dataders
Copy link
Collaborator

@dataders dataders commented Feb 5, 2021

Marie Kondo inspo... if it works with the global project, why not just rely on that? This was we automatically inherit upgrades. The downside is that we'll have to accommodate any breaking changes, but we can always re-implement at that time?

@mikaelene
Copy link
Collaborator

I guess it was there for a reason, but maybe the global_project adjusted along the way. @jtcohen6 how well are incremental materialisations tested in the adapter-tests pytest package?

But it really looks like it works. It is always better to relay on the global_project, so I am leaning to that there are no reasons to not make this merge...

@jtcohen6
Copy link
Contributor

jtcohen6 commented Feb 5, 2021

Definitely in favor of this! The less code, the less surface area, the better. If you need to reimplement/override because of breaking changes in a future release, so be it. (And hopefully you'd be able to test the beta/RC and find it first, so we can rewrite the SQL in a T-SQL-friendlier way.)

The test_dbt_incremental sequence is pretty good, but I believe it tests append-only behavior (i.e. an incremental model without unique_key). I'd just say, you should sure it works right with a unique_key as well.

@dataders
Copy link
Collaborator Author

@mikaelene this is ready to be merged!

@mikaelene
Copy link
Collaborator

@swanderz do you want to add anything in the changeling for this? You can add it in master-branch if you don't want to reopen this.

@dataders
Copy link
Collaborator Author

@mikaelene thanks for the heads up! I merged #106 to do exactly that. We're ready for a v0.19.0.1 release now I think.

@sdebruyn sdebruyn deleted the posssibly_drop_incremental branch June 4, 2022 08:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants