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.

Sign up for GitHub

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

Jump to bottom

merge exclude columns for incremental models #5457

Merged
merged 6 commits into from Sep 26, 2022
Merged

merge exclude columns for incremental models #5457

merged 6 commits into from Sep 26, 2022

Conversation

dave-connors-3
Copy link
Contributor

@dave-connors-3 dave-connors-3 commented Jul 11, 2022 •
edited

resolves #5260

Description

This PR allows a user to specify a list of columns not to update during an incremental run using the merge strategy. This is achieved via a new macro get_merge_update_columns that will take into account user input from either merge_update_columns or the new merge_exclude_columns config to return the correct set of columns to update.

This will need to be dispatched to accomodate slightly different behavior in the dbt-spark repository.

Checklist

@cla-bot cla-bot bot added the cla:yes label Jul 11, 2022
@github-actions
Copy link
Contributor

Thank you for your pull request! We could not find a changelog entry for this change. For details on how to document a change, see the contributing guide.

@dave-connors-3 dave-connors-3 changed the title exlcude cols like in dbt_utils.star merge exclude columns for incremental models Jul 11, 2022
@dave-connors-3 dave-connors-3 mentioned this pull request Jul 11, 2022
Merged
4 tasks
@jtcohen6 jtcohen6 added Team:Adapters Issues designated for the adapter area of the code ready_for_review Externally contributed PR has functional approval, ready for code review from Core engineering labels Jul 18, 2022
@jtcohen6
Copy link
Contributor

Thanks @dave-connors-3! I'm tagging this ready_for_review, and adding it to the review queue for the Adapters team.

In order to merge this PR, we'll want a functional test for merge_exclude_columns that lives in dbt-tests-adapter, and runs successfully when pulled into dbt-snowflake + dbt-bigquery + dbt-spark.

It would make sense for this new test to live alongside the existing tests for merge_update_columns. Currently, those tests live only in each adapter plugin, and not yet converted to use the new pytest framework:

This was referenced Jul 20, 2022
Merged
Merged
@dave-connors-3 dave-connors-3 marked this pull request as ready for review July 20, 2022 18:00
@dave-connors-3 dave-connors-3 requested a review from a team July 20, 2022 18:00
@dave-connors-3 dave-connors-3 requested review from a team as code owners July 20, 2022 18:00
@dave-connors-3
Copy link
Contributor Author

hey @jtcohen6! I think we should be good to go on added tests --

Spark
BQ
Snowflake

I likely missed something, so let me know what else needs to be done!

@kd2718
Copy link

kd2718 commented Aug 24, 2022

Our team would love to have this feature. What is the status of this PR?

@leahwicz
Copy link
Contributor

@nathaniel-may could this be a PR that you prioritize the team to get some eyes on and review please?

@colin-rogers-dbt colin-rogers-dbt self-assigned this Sep 20, 2022
gshank
gshank approved these changes Sep 26, 2022
View reviewed changes
Copy link
Contributor

@gshank gshank left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code here is straightforward and does not look risky. This does not implement adapter zone tests, however, so that should be done in a separate ticket/pr.

@gshank gshank merged commit 38ada8a into main Sep 26, 2022
@gshank gshank deleted the feature/merge_exclude_columns branch September 26, 2022 18:49
@jtcohen6 jtcohen6 mentioned this pull request Oct 12, 2022
Closed
1 task
@findinpath findinpath mentioned this pull request Oct 13, 2022
Closed
1 task
@dave-connors-3 dave-connors-3 mentioned this pull request Jan 23, 2023
Closed
@dpguthrie dpguthrie mentioned this pull request Mar 23, 2023
Merged
2 tasks
dpguthrie added a commit to dbt-labs/docs.getdbt.com that referenced this pull request Mar 24, 2023
@dpguthrie
Include a section for merge_exclude_columns (#3069)
2732c84 
## What are you changing in this pull request and why?

Adding a section for `merge_exclude_columns` to go with the similar
config `merge_update_columns`. This is functionality that was added
[here](dbt-labs/dbt-core#5457)

Closes #2197 

<!---
Describe your changes and why you're making them. If linked to an open
issue or a pull request on dbt Core, then link to them here! 

To learn more about the writing conventions used in the dbt Labs docs,
see the [Content style
guide](https://github.com/dbt-labs/docs.getdbt.com/blob/current/contributing/content-style-guide.md).
-->

## Checklist
<!--
Uncomment if you're publishing docs for a prerelease version of dbt
(delete if not applicable):
- [ ] Add versioning components, as described in [Versioning
Docs](https://github.com/dbt-labs/docs.getdbt.com/blob/current/contributing/versioningdocs.md)
- [ ] Add a note to the prerelease version [Migration
Guide](https://github.com/dbt-labs/docs.getdbt.com/tree/current/website/docs/guides/migration/versions)
-->
- [x] Review the [Content style
guide](https://github.com/dbt-labs/docs.getdbt.com/blob/current/contributing/content-style-guide.md)
and [About
versioning](https://github.com/dbt-labs/docs.getdbt.com/blob/current/contributing/single-sourcing-content.md#adding-a-new-version)
so my content adheres to these guidelines.
- [x] Add a checklist item for anything that needs to happen before this
PR is merged, such as "needs technical review" or "change base branch."
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gshank gshank gshank approved these changes

@McKnight-42 McKnight-42 Awaiting requested review from McKnight-42

@VersusFacit VersusFacit Awaiting requested review from VersusFacit

@stu-k stu-k Awaiting requested review from stu-k

Assignees

@colin-rogers-dbt colin-rogers-dbt

Labels
cla:yes ready_for_review Externally contributed PR has functional approval, ready for code review from Core engineering Team:Adapters Issues designated for the adapter area of the code
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[CT-656] [Feature] Allow for merge exclude columns
6 participants
@dave-connors-3 @jtcohen6 @kd2718 @leahwicz @gshank @colin-rogers-dbt