Skip to content
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

start_date filtering messes with secondary calculations #16

Closed
joellabes opened this issue Feb 26, 2022 · 6 comments · Fixed by #17
Closed

start_date filtering messes with secondary calculations #16

joellabes opened this issue Feb 26, 2022 · 6 comments · Fixed by #17
Assignees

Comments

@joellabes
Copy link
Contributor

joellabes commented Feb 26, 2022

{% if start_date %}
and date_day >= '{{ start_date }}'
{% endif %}
{% if end_date %}
and date_day <= '{{ end_date }}'
{% endif %}

By excluding old data so early, we can't do lookback windows etc - they'd all come through as having no data.

We could probably exclude records after the end date

@hsenger-tiger
Copy link

I don't think this works, or it may have been broken later. When I run the following:

select *
from {{ metrics.calculate(
[metric('transactions')],
grain='month',
dimensions=['company'],
secondary_calculations=[
metrics.period_over_period(comparison_strategy="ratio", interval=12, alias="yoy"),
],
start_date = '2022-01-01',
where="company!= 'My Company'",
) }}

The YoY calculation doesn't work when the prior period is is before the start_date. Is this expected?

@callum-mcdata
Copy link
Contributor

This is expected behavior in the current version of the package as we try to limit the size of the dataset that is being used by applying the date filters as early as possible. If you wanted to see the YoY value then you'll need to expand the date range.

@hsenger-tiger
Copy link

Thanks Callum. Can you help me understand whether there was a time when this did work (like after this issue first was closed) and then things were changed again so it wouldn't work later? Or am I misunderstanding what this issue was about / what the fix enabled in the first place?

@callum-mcdata
Copy link
Contributor

I am not entirely sure! This issue was from before I was at dbt Labs and this issue was during the early experimental phases of this package - it appears to have allowed for secondary cals to do this but that behavior was probably unintentionally removed in later versions as we focused more on performance & functionality of the package. The role of secondary calculations has changed since the early days of this package and now operates on the pre-aggregated metric values instead of on the entire dataset.

@hsenger-tiger
Copy link

Thanks again Callum. The use case for me is wanting to be able to use metrics.calculate in an incremental model, so that we can calculate new metrics for the latest date only rather than recalculating everything going back to the earliest comparison period we want. For example, we have metrics we compute daily that we want YoY, Yo2Y, etc comparisons for. Currently, it seems like to calculate the metrics for 1 additional date with these secondary period-over-period calculations, I need to recalculate everything going back 2 years if I want Yo2Y. If the secondary calculations now operate on the pre-aggregated metric values rather than the entire dataset, that seems like it would make this a much easier thing to accomplish! I expected to be able to accomplish it using the start_date field, but found that specifying a start_date breaks the period over period secondary calculations. Is there another method that I could use to accomplish this if using the start_date doesn't work?

@callum-mcdata
Copy link
Contributor

Unfortunately not - dbt_metrics does not support incremental models in a first-class way because the primary use case has not been materializing the datasets. If you're looking to use this in an incremental way I'd recommend doing so within the SQL and expanding the range of the start_date input to account for this behavior.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants