bugfix/incremental-mar-filter-removal #72
Conversation
There was a problem hiding this comment.
Hey @fivetran-joemarkiewicz, this PR looks good to me.
Just a couple questions but approving as they are not critical for release.
CHANGELOG.md
Outdated
| - Modified the logic within the `fivetran_log__mar_table_history` model to no longer filter out previous historical MAR records. Previously, these fields were filtered out as the `active_volume` source (since deprecated and replaced with `incremental_mar`) produced a cumulative daily MAR total. However, the `incremental_mar` source is not cumulative and will need to include all historical records. ([#72](https://github.com/fivetran/dbt_fivetran_log/pull/72)) | ||
|
|
||
| ## Under the Hood | ||
| - Added coalesce statements to the `paid_monthly_active_rows` and `free_monthly_active_rows` fields within the [fivetran_log__mar_table_history](https://github.com/fivetran/dbt_fivetran_log/blob/226deff6d4f10b19bad5ccff1a6bc6813e809fe7/models/fivetran_log__mar_table_history.sql) model to coalesce to 0. ([#72](https://github.com/fivetran/dbt_fivetran_log/pull/72)) |
There was a problem hiding this comment.
General question - I know there were some convos regarding referencing the github code vs the dbt Docs models, what are your opinions here?
There was a problem hiding this comment.
Good call. I will update to point to the docs.
There was a problem hiding this comment.
Updated.
| @@ -57,7 +54,6 @@ latest_mar as ( | |||
| (free_monthly_active_rows + paid_monthly_active_rows) as total_monthly_active_rows | |||
There was a problem hiding this comment.
for date(measured_date) as last_measured_at, do you think last_measured_at is a good name here? Or should it be measured_at? or just date_day?
There was a problem hiding this comment.
For this release I would recommend we keep it to measured_at otherwise this will be a breaking change and we should get the update out in a patch release.
This is something we can open an FR for to clarify in the next breaking change.
Are you a current Fivetran customer?
Fivetran created PR
What change(s) does this PR introduce?
Bug Fixes
fivetran_log__mar_table_historymodel to no longer filter out previous historical MAR records. Previously, these fields were filtered out as theactive_volumesource (since deprecated and replaced withincremental_mar) produced a cumulative daily MAR total. However, theincremental_marsource is not cumulative and will need to include all historical records.Under the Hood
paid_monthly_active_rowsandfree_monthly_active_rowsfields within the fivetran_log__mar_table_history model to coalesce to 0.Did you update the CHANGELOG?
Does this PR introduce a breaking change?
This PR is only removing an erroneous filter and will not cause 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?
Tested on the customers data to confirm the totals matched following this update.
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.