Coalesce Incremental Load Fields in shopify__customer_cohorts
#86
Conversation
|
Thanks @advolut-team for opening this! Our team will work on this in our upcoming sprint! |
There was a problem hiding this comment.
Hi @advolut-team , thanks for submitting this PR! I have one question before moving forward with approval and merging it into a release branch for our internal processes.
Would you also be able to do the following:
- Bump the version in both
dbt_project.ymlandintegration_tests/dbt_project.ymlto 0.13.1 - Add the changes you made in the CHANGELOG. You can use the "Detail what changes this PR introduces" section of the PR and elaborate if you'd like.
Let me know if you have any questions.
models/shopify__customer_cohorts.sql
Outdated
| backfill_lifetime_sums.previous_total_price_lifetime + windows.total_price_lifetime as total_price_lifetime, | ||
| backfill_lifetime_sums.previous_order_count_lifetime + windows.order_count_lifetime as order_count_lifetime, | ||
| backfill_lifetime_sums.previous_line_item_count_lifetime + windows.line_item_count_lifetime as line_item_count_lifetime, | ||
| coalesce(backfill_lifetime_sums.previous_cohort_month_number, 0) + windows.cohort_month_number as cohort_month_number, |
There was a problem hiding this comment.
Do you think we should coalesce windows.cohort_month_number in the rarer (but possible) case that this value returns null on an incremental run? There could be an incremental period where no orders are brought in during the window and thus the partition is run on an empty table.
There was a problem hiding this comment.
Definitely a rare case, but I think there's no harm adding a coalesce statement to maintain data quality :) I've tested locally by running dbt run with coalesce(windows. cohort_month_number) and I have the same result. Let me add that in my next commit, along with the CHANGELOG and version bump
|
Thanks for reviewing and providing feedback @fivetran-avinash. I've updated my PR, let me know if I missed anything else |
There was a problem hiding this comment.
Awesome, thanks for replying to all my comments @advolut-team ! I'll go ahead and merge these changes into a development branch and will hopefully have this live in a new release on the dbt hub soon.
1cc9b03
into
fivetran:bugfix/coalesce-incremental-fields
* Coalesce Incremental Load Fields in `shopify__customer_cohorts` (#86) * coalesce incremental load values * coalesce windows columns * validation tests + docs * CHANGELOG --------- Co-authored-by: advolut-team <161306391+advolut-team@users.noreply.github.com>
Please provide your name and company
Link the issue/feature request which this PR is meant to address
Issue
Detail what changes this PR introduces and how this addresses the issue/feature request linked above.
Coalesces the previous lifetimes fields from incremental loads. This fixes the issue of NULL values in the lifetime columns in
shopify__customer_cohortstable.How did you validate the changes introduced within this PR?
Ran
dbt runlocally and I no longer see NULLs in my Fivetran connector warehouse.Which warehouse did you use to develop these changes?
AWS Redshift
Did you update the CHANGELOG?
Did you update the dbt_project.yml files with the version upgrade (please leverage standard semantic versioning)? (In both your main project and integration_tests)
Provide an emoji that best describes your current mood
🏃
Feedback
We are so excited you decided to contribute to the Fivetran community dbt package! We continue to work to improve the packages and would greatly appreciate your feedback on our existing dbt packages or what you'd like to see next.
PR Template
Community Pull Request Template (default)
Maintainer Pull Request Template (to be used by maintainers)