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

[CT-2855] [Feature] dbt_utils.date_spine should be added to core given new MetricFlow requirements #8172

Closed
3 tasks done
Tracked by #8125
graciegoheen opened this issue Jul 20, 2023 · 5 comments · Fixed by #8616, dbt-labs/dbt-bigquery#943, dbt-labs/dbt-redshift#617 or dbt-labs/dbt-snowflake#787
Assignees
Labels
enhancement New feature or request user docs [docs.getdbt.com] Needs better documentation utils Cross-database building blocks

Comments

@graciegoheen
Copy link
Contributor

Is this your first time submitting a feature request?

  • I have read the expectations for open source contributors
  • I have searched the existing issues, and I could not find an existing issue for this feature
  • I am requesting a straightforward extension of existing dbt functionality, rather than a Big Idea better suited to a discussion

Describe the feature

The new metric spec requires a Time Spine model - documented here.

The metricflow_time_spine model to copy/paste into your project uses the dbt_utils date_spine macro. Given that this macro will be used by all dbt users who want to define metrics, should we bring it into dbt-core?

Additionally, should we handle casting within the macro itself? Currently, the example requires that you input appropriate casting to a date data type:

    {{dbt_utils.date_spine('day'
    , "to_date('01/01/2000','mm/dd/yyyy')"
    , "to_date('01/01/2027','mm/dd/yyyy')"
    )
    }}

But not all warehouses have the to_date function. If we handled date casting within the date_spine macro, we could have a single metricflow_time_spine example that would work in all data warehouses.

Describe alternatives you've considered

We keep date_spine in dbt_utils and require that people install dbt_utils if they want to use metric flow.

Who will this benefit?

dbt users who want to define metrics in their project

Are you interested in contributing this feature?

No response

Anything else?

No response

@graciegoheen graciegoheen added enhancement New feature or request triage labels Jul 20, 2023
@github-actions github-actions bot changed the title [Feature] should dbt_utils.date_spine be added to core given new MetricFlow requirements [CT-2855] [Feature] should dbt_utils.date_spine be added to core given new MetricFlow requirements Jul 20, 2023
@jtcohen6 jtcohen6 added the utils Cross-database building blocks label Jul 20, 2023
@dbeatty10
Copy link
Contributor

Keen observation @graciegoheen 🧠

In addition to moving date_spine into dbt-core (and/or copying and modifying it), are you thinking that we'd define a default metricflow_time_spine macro in dbt-core (along with relevant test cases) and then expect adapters (like dbt-postgres, etc) to override and customize the default implementation as-needed?

Maybe something like this (completely untested)?

{% macro default__metricflow_time_spine(start_date="2000-01-01", end_date="2030-01-01") -%}

{# -- Do some kind of checking here to make sure the provided date strings are in valid ISO 8601 / RFC 3339 format #}

with days as (

    {{
        dbt.date_spine(
            'day',
            cast({{ dbt.string_literal(start_date) }} as {{ api.Column.translate_type("date")) }}),
            cast({{ dbt.string_literal(end_date) }} as {{ api.Column.translate_type("date")) }})
        )
    }}

)

select cast(date_day as {{ api.Column.translate_type("date")) }}) as date_day
from days

Note: we don't have a cross-database data type macro for type_date, hence the api.Column.translate_type("date")) above. But we could always implement type_date really easily if we want!

We also don't have a well-known cross-database cast macro, but dbt-labs/dbt-adapters#84 could add that.

@graciegoheen
Copy link
Contributor Author

graciegoheen commented Aug 7, 2023

Notes from TR:

@graciegoheen graciegoheen changed the title [CT-2855] [Feature] should dbt_utils.date_spine be added to core given new MetricFlow requirements [CT-2855] [Feature] dbt_utils.date_spine should be added to core given new MetricFlow requirements Aug 7, 2023
@dbeatty10
Copy link
Contributor

Will need to add a deprecation warning in dbt-utils if folks are using a version of dbt-core that has this macro natively

@graciegoheen could you open an issue for this in dbt-utils (and/or open a PR)?

@dbeatty10 dbeatty10 removed the triage label Aug 7, 2023
@graciegoheen
Copy link
Contributor Author

graciegoheen commented Aug 11, 2023

@dbeatty10 Do you want me to do that now? Or wait until this is picked up?

@dbeatty10
Copy link
Contributor

@dbeatty10 Do you want me to do that now? Or wait until this is picked up?

Ideally, whoever picks up this one would also pick up the dbt-utils one at the same time. Alternatively, we'd need to make sure someone else is available to pick up the dbt-utils work at the same time.

Would probably be worth opening the issue in dbt-utils now so that it's ready either way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request user docs [docs.getdbt.com] Needs better documentation utils Cross-database building blocks
Projects
None yet
4 participants