update stripe package with running total model #60
Conversation
|
fivetran-reneeli
requested review from
fivetran-catfritz
and removed request for
fivetran-joemarkiewicz
There was a problem hiding this comment.
I was able to reproduce the resources exceeded bug by running the main branch, and running the new branch fixed it! So from that aspect I would say it's all working great!
Now that we've solved the issue, and thinking about what a new user might experience, I'm curious why this was moved to an int model? It seems like the final model is a straight select * from only the new int model, and since we're running int models as tables instead of ephemeral, we're generating two identical tables in the output (int_stripe__account_running_total is the same as stripe__daily_overview).
If the reason is to keep the final model code clean, I think all we need to do is add a {{ config(materialized='view') }} to the int_stripe__account_running_total model. I think this could help since the other final models get the benefit of most of the int models being generated as tables, while here we avoid generating duplicate tables in the user's warehouse. I tested it out locally with a full refresh, and it panned out with our internal data, so let me know what you think!
Lastly just some small things. We need to update the installation version in the README, and I also left a few suggestions in in-line comments.
CHANGELOG.md
Outdated
| # dbt_stripe v0.10.0 | ||
| [#60](https://github.com/fivetran/dbt_stripe/pull/60) includes the following changes: | ||
| ## 🚨 Breaking Changes 🚨: | ||
| - - Introduces a `int_stripe__account_running_totals` model to alleviate runtime errors resulting from the `stripe__daily_overview` model exceeding disc capacity upon running. |
There was a problem hiding this comment.
| - - Introduces a `int_stripe__account_running_totals` model to alleviate runtime errors resulting from the `stripe__daily_overview` model exceeding disc capacity upon running. | |
| - Introduces a `int_stripe__account_running_totals` model to alleviate runtime errors resulting from the `stripe__daily_overview` model exceeding disc capacity upon running. |
Just a small edit removing extra bullet.
There was a problem hiding this comment.
I think it would help to offer a little explanation for how this change is beneficial. Maybe highlight you shifted logic from the final to the int and maybe add an "under the hood" section. Doesn't have to be too much but maybe something about how/why changing the int models from ephemeral to tables helps with the runs.
There was a problem hiding this comment.
Good point, added more to the changelog!
| @@ -25,6 +25,7 @@ with date_spine as ( | |||
| ), final as ( | |||
|
|
|||
| select | |||
| date_spine.account_id, | |||
There was a problem hiding this comment.
Was this added to help with the join? I would add the reason for this addition to the changelog.
There was a problem hiding this comment.
true, added!
|
|
||
| - name: int_stripe__incomplete_charges | ||
| description: Each record represents a charge that is incomplete. | ||
|
|
There was a problem hiding this comment.
This was helpful to understand what the different int models do!
|
Thanks for the review @fivetran-catfritz! |
| account_id, | ||
| {{ dbt_utils.generate_surrogate_key(['account_id','date_day']) }} as account_daily_id, |
There was a problem hiding this comment.
Sorry I didn't catch this before, but I'm getting a test failure from
combination_of_columns:
- date_day
- source_relationGuessing this test needs to be adjusted?
There was a problem hiding this comment.
ah, added account_id, forgot I included that field now. Should be good now, thanks for catching!
CHANGELOG.md
Outdated
| - Introduces a `int_stripe__account_running_totals` model to alleviate runtime errors resulting from the `stripe__daily_overview` model exceeding disc capacity upon running. | ||
| - Add `account_id` in `int_stripe__account_rolling_totals` for use as part of the join in the case where more than 1 `account_id` exists. | ||
|
|
||
| ## Under the Hood | ||
| - The previous logic from `stripe__daily_overview` has been moved to the intermediate `int_stripe__account_running_totals` model to help optimize runtime. |
There was a problem hiding this comment.
I don't think some of this applies anymore.
There was a problem hiding this comment.
And the README install sections still needs to be adjusted for the breaking change version.
Are you a current Fivetran customer?
Internal
What change(s) does this PR introduce?
running_totalmodel to alleviate runtime errors resulting from the daily_overview model exceeding disc capacity upon running.Did you update the CHANGELOG?
Does this PR introduce a breaking change?
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)
Is this PR in response to a previously created Bug or Feature Request
How did you test the PR changes?
Select which warehouse(s) were used to test the PR
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.