-
Notifications
You must be signed in to change notification settings - Fork 5
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
fix for events incremental #51
Conversation
User Campaign - First open or click event time
… first_open_or_click_event_at, update docs
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-reneeli thanks for working through this. This is looking really good I just have a few comments and suggestions before approving. Also, would you please be able to run the validation tests for this package and share the results in the PR description to confirm there are no unexpected changes between dev and prod.
CHANGELOG.md
Outdated
- Adds a field called `first_open_or_click_event_at` in the `iterable__user_campaign` model. This timestamp shows the first time a user interacted with a campaign, recording the earliest occurring event out of 'emailOpen', 'emailClick', and 'pushOpen'. ([PR #50](https://github.com/fivetran/dbt_iterable/pull/50)) | ||
|
||
## Bug Fix | ||
- Updates the incremental logic in `iterable__events` to use the `created_on` date field instead of the `created_at` timestamp. Previously, this would potentially exclude late-arriving new records from populating in the end models if they had an older `created_at` value than what was present in the model. Switching to `created_on` widens the criteria. |
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.
This should be listed in the breaking change section
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.
updated
packages.yml
Outdated
# version: [">=0.10.0", "<0.11.0"] | ||
|
||
- git: https://github.com/fivetran/dbt_iterable_source.git | ||
revision: rm_src_freshness | ||
warn-unpinned: false |
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.
Reminder to swap before merge
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.
Same
Thanks @fivetran-joemarkiewicz this is ready for re-review. I updated with the validation test results. Actually, as expected, there was one test failure on |
…/created_on definitions in yml, plus update changelog
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-reneeli thanks for working through these changes! I have a few comments before this is ready for approval. Let me know if you have questions and once this is ready for final review.
CHANGELOG.md
Outdated
# dbt_iterable v0.13.0 | ||
[PR #51](https://github.com/fivetran/dbt_iterable/pull/51) includes the following updates: | ||
|
||
## Breaking Changes |
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.
## Breaking Changes | |
## Breaking Changes (`--full-refresh` required after upgrading) |
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.
updated
Co-authored-by: Joe Markiewicz <74217849+fivetran-joemarkiewicz@users.noreply.github.com>
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-reneeli this is really close, just a few small changes before approval. Let me know once their updated. Thanks!
Co-authored-by: Joe Markiewicz <74217849+fivetran-joemarkiewicz@users.noreply.github.com>
Co-authored-by: Joe Markiewicz <74217849+fivetran-joemarkiewicz@users.noreply.github.com>
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-reneeli changes look good to me! Just a small CHANGELOG edit request and a notice that I made a small README change to close a codeblock. Once the CHANGELOG edit is made this is ready for release review.
- "emailBounce" | ||
- "inAppClick" | ||
- "pushUninstall" | ||
``` |
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.
This ending codeblock was not captured before. It seems the suggestion didn't like the ending codeblock in a codeblock. I just committed this directly to your branch to address the issue.
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.
oh gotcha, thank you!
Co-authored-by: Joe Markiewicz <74217849+fivetran-joemarkiewicz@users.noreply.github.com>
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-reneeli We should probably have the seed files match on both source and transform packages. Can you update one or the other? Not sure what works best here for more robust testing.
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.
Thanks @fivetran-avinash , I believe they're the same now
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-reneeli Nice work! A few comments before approval.
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-reneeli Oops, something went wonky here--had two tabs open with comments/suggestions and submitted both. Let me know if you have any questions.
Co-authored-by: Avinash Kunnath <108772760+fivetran-avinash@users.noreply.github.com>
Co-authored-by: Avinash Kunnath <108772760+fivetran-avinash@users.noreply.github.com>
Co-authored-by: Avinash Kunnath <108772760+fivetran-avinash@users.noreply.github.com>
Co-authored-by: Avinash Kunnath <108772760+fivetran-avinash@users.noreply.github.com>
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-reneeli Ready to merge!
PR Overview
This PR will address the following Issue/Feature:
#45 #48
This PR will result in the following new package version:
v0.13.0
Please provide the finalized CHANGELOG entry which details the relevant changes included in this PR:
Breaking Changes (
--full-refresh
required after upgrading)first_open_or_click_event_at
in theiterable__user_campaign
model. This timestamp shows the first time a user interacted with a campaign, recording the earliest occurring event out of 'emailOpen', 'emailClick', and 'pushOpen'. (PR #50)iterable__events
model to now use thecreated_on
date field instead of thecreated_at
timestamp.created_at
value than what was present in the model. Switching tocreated_on
widens the criteria.stg_iterable__user_history
model from materializing as a table to a view in order to improve performance.--full-refresh
is required after upgrading.Under the Hood
created_on
in the incremental logic initerable__events
, we introduced aiterable_lookback_window
variable to increase the window for accommodating potential late-arriving records. The default is 7 days prior to the maximumcreated_on
value present in theiterable__events
model, but you may customize this by setting the variterable_lookback_window
in your dbt_project.yml. See the Lookback Window section of the README for more details.iterable__event_metrics
variable and how to use it to specify which event metrics to pivot out.created_on
from the uniqueness test initerable__events
. Uniqueness is now tested solely onunique_event_id
, a surrogate key made up ofevent_id
(_fivetran_id
in the raw table, which is a Fivetran-created unique identifier derived from hashing campaign_id, created_at, and event_name) and_fivetran_user_id
(a Fivetran-created column derived from a hash ofuser_id
and/oremail
).event
seed data to more accurately represent real-life data, with a unique_fivetran_id
for each campaign_id, created_at, and event_name.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 share any and all of your validation steps:
This error in
consistency_user_campaign
is expected due to the addition of a new fieldfirst_open_or_click_event_at
in this branch.If you had to summarize this PR in an emoji, which would it be?
💃