-
Notifications
You must be signed in to change notification settings - Fork 23
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
Update/rename package #81
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fivetran-jamie this looks great! I do have a few minor comments and suggestions in line before this is fully good go.
In addition to the in line comments, would you be able to show a validation example of the incremental strategy loading in only new rows on an incremental run. This will help us ensure the strategy is sound. You can do this my artificially filtering the source data on the full refresh, and then removing the filter on the next incremental run.
Finally, I feel this section (below) in the README would be the best place to inform Snowflake, Redshift, and Postgres users to periodically run a full refresh to ensure their incremental models capture updates. Would you be able to add a sub section there detailing this to the customers?
Co-authored-by: Joe Markiewicz <[email protected]>
Co-authored-by: Joe Markiewicz <[email protected]>
@fivetran-joemarkiewicz added your changes! also for validation, i added some extra lines to the seed file So, running before adding new dates: I then add these new rows to the i i perform an incremental run, resulting in the following expected output (a new record for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm thanks for applying the updates!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fivetran-jamie thanks for working through this! I left a few comments and code change suggestions. Let me know if you have any questions following my review.
Co-authored-by: Joe Markiewicz <[email protected]>
Co-authored-by: Joe Markiewicz <[email protected]>
Co-authored-by: Joe Markiewicz <[email protected]>
Co-authored-by: Joe Markiewicz <[email protected]>
Co-authored-by: Joe Markiewicz <[email protected]>
Co-authored-by: Joe Markiewicz <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One small comment in the README, but other than that - this looks good to go!
Co-authored-by: Joe Markiewicz <[email protected]>
…bt_fivetran_log into update/rename-package
PR Overview
This PR will address the following Issue/Feature:
height task T-390568
#83
This PR will result in the following new package version:
considering we are renaming the package, v1.0.0?
Please detail what change(s) this PR introduces and any additional information that should be known during the review of this PR:
fivetran_log
prefix on all variables and models tofivetran_platform
_stg_fivetran_platform
and_fivetran_platform
fivetran_platform
_fivetran_synced
in some sourcesaccount_membership
table which was deprecated - [Feature] totally deprecateaccount_membership
table #83what this does NOT do:
PR Checklist
Basic Validation
Please acknowledge that you have successfully performed the following commands locally:
Before marking this PR as "ready for review" the following have been applied:
Detailed Validation
Please acknowledge that the following validation checks have been performed prior to marking this PR as "ready for review":
ran the two versions of the package simultaneously (since they have different project + model names). I looked at various models, but here I will focus on the audit table model, since i updated the the Bigquery + Spark incremental strategy for that model
everything checks out there -- should i validate any other way.. ?
Standard Updates
Please acknowledge that your PR contains the following standard updates:
dbt Docs
Please acknowledge that after the above were all completed the below were applied to your branch:
If you had to summarize this PR in an emoji, which would it be?
📛