[Bug fix] MRR type updates, deterministic line item indices, validation tests and streamlit link fixes#43
Conversation
fivetran-joemarkiewicz
left a comment
fivetran-joemarkiewicz
left a comment
There was a problem hiding this comment.
A few comments, questions, suggestions before approving.
| ) }} | ||
|
|
||
| {% set exclude_cols = var('consistency_test_exclude_metrics', []) %} | ||
| {% set exclude_cols = ['daily_charges', 'daily_credits', 'daily_discounts', 'daily_net_change', 'daily_taxes', 'rolling_account_balance', 'rolling_charge_balance', 'rolling_credit_balance', 'rolling_discount_balance', 'rolling_tax_balance'] + var('consistency_test_exclude_metrics', []) %} |
There was a problem hiding this comment.
Do we actually want to always exclude these? These seem like important columns to check in the future.
There was a problem hiding this comment.
Moved these tests over to a new consistency test designed to compare amounts. I missed some the first time around but applied them again now.
| ) }} | ||
|
|
||
| {% set exclude_cols = var('consistency_test_exclude_metrics', []) %} | ||
| {% set exclude_cols = ['current_month_mrr', 'previous_month_mrr'] + var('consistency_test_exclude_metrics', []) %} |
There was a problem hiding this comment.
Same comment about if we really want to exclude these columns in future releases?
There was a problem hiding this comment.
I created a separate consistency test to compare the amounts. Called this out more explicitly in the changelog.
| select | ||
| *, | ||
| case when current_month_mrr > previous_month_mrr then 'expansion' | ||
| when current_month_mrr < previous_month_mrr then 'contraction' | ||
| when current_month_mrr = previous_month_mrr then 'unchanged' | ||
| case when round(cast(current_month_mrr as {{ dbt.type_numeric() }}), 2) > round(cast(previous_month_mrr as {{ dbt.type_numeric() }}), 2) then 'expansion' | ||
| when round(cast(current_month_mrr as {{ dbt.type_numeric() }}), 2) < round(cast(previous_month_mrr as {{ dbt.type_numeric() }}), 2) then 'contraction' | ||
| when round(cast(current_month_mrr as {{ dbt.type_numeric() }}), 2) = round(cast(previous_month_mrr as {{ dbt.type_numeric() }}), 2) then 'unchanged' | ||
| when previous_month_mrr is null then 'new' | ||
| when (current_month_mrr = 0.0 or current_month_mrr is null) | ||
| and (previous_month_mrr != 0.0) | ||
| then 'churned' | ||
| when (previous_month_mrr = 0.0 and current_month_mrr > 0.0 | ||
| and account_month_number >= 3) | ||
| when (previous_month_mrr = 0.0 and current_month_mrr > 0.0 | ||
| and account_month_number >= 3) |
There was a problem hiding this comment.
Would it instead be more accurate to change the casts to float we apply in the staging model to be numeric in order to ensure consistent results as opposed to adjusting these case statements downstream.
In this case, numeric would be a more accurate representation of the field(s) and we should apply that at the staging layer.
There was a problem hiding this comment.
I agree changing numeric to float is probably the right approach at the staging layer. But that would increase the scope of this issue as all Recurly amounts are designed as floats and need to be updated across the model, so I opted for the band-aid. Should I add these in now or file a separate issue for future scoping?
2 participants
PR Overview
Package version introduced in this PR:
This PR addresses the following Issue/Feature(s):
Summary of changes:
Submission Checklist
Changelog