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

Allow start/end dates in metrics to be simple strings and function calls #28

Conversation

alexberryman
Copy link

@alexberryman alexberryman commented Mar 31, 2022

Description of the changes

As discussed within Issue #27. This PR dynamically detects the contents in start_date and end_date and decides if it needs to wrap the user input in single quotes.

  • If start_date contains a function call, then leave it un-quoted so the call will be executed
    • compiled unquoted SQL: cast('dateadd(day, -1, date_trunc("day", getdate()))' as date) as upper_bound
  • Else if state_date contains only a basic date string, wrap it in quotes
    • compiled qutoed SQL: cast('202201-01' as date) as upper_bound
  • Else act like start_date isn't defined
    • compiled SQL existing else-block: max(case when has_data then period end) over () {% endif %} as upper_bound

How were the changes tests

I tested this locally on my branch using a mix of simple strings and function calls for start_date and end_date. I then select * from metric; on a Snowflake table and view.

Metric Definition

##metric_test.sql
select 
    *
from {{ metrics.metric(
    metric_name='new_opportunities',
    grain='month',
    dimensions=['lead_source'],
    start_date="2022-01-01",
    end_date='dateadd(day, -1, date_trunc("day", getdate()))'
) }}

Compiled SQL

bounded as (
    select 
        *,
         cast('2021-01-01' as date)
         as lower_bound,
         /* assume any end_date that is defined and contains an opening parentheses is likely a function call and does not need to be quoted */ cast(dateadd(day, -1, date_trunc("day", getdate())) as date)
         as upper_bound
    from joined 
),

Signed Individual Contributor Agreement?

  • Yes

Additional Context

Possible improvements

  • Better detection of "is a function call being used?" Currently I just look for ( since most databases I have used call functions like get_date()
  • Remove inline comment in Compiled SQL

@cla-bot cla-bot bot added the cla:yes The CLA has been signed label Mar 31, 2022
@joellabes
Copy link
Contributor

joellabes commented Apr 8, 2022

Thanks for opening this @alexberryman! I agree with your diagnosis of the problem (it should be possible to use functions as proxies for dates), but not your proposed solution.

An easier, more stable, and more consistent with prior art approach would be to remove the enclosing quotes and instead require callers with a fixed date to double quote their entries. This means that your example would instead look like:

--metric_test.sql
select 
    *
from {{ metrics.metric(
    metric_name='new_opportunities',
    grain='month',
    dimensions=['lead_source'],
    start_date="'2022-01-01'", --note the extra quotes here
    end_date='dateadd(day, -1, date_trunc("day", getdate()))'
) }}

This would line up with how other macros in dbt_utils work, e.g. dateadd, datediff, split_part.

I happened to talk a lot about a similar problem in this thread last week: dbt-labs/dbt-utils#528 (comment)

If you wanted to change this PR to remove the existing quotes and update the documentation to reflect the new best practice, we'd be happy to get that merged in!

@callum-mcdata callum-mcdata added the enhancement New feature or request label Jun 1, 2022
@callum-mcdata callum-mcdata linked an issue Jun 1, 2022 that may be closed by this pull request
@callum-mcdata
Copy link
Contributor

callum-mcdata commented Jun 2, 2022

Hi @alexberryman! I'm circling back on this PR as the new member of the developer experience team who's going to be tackling ownership of this package. I agree @joellabes's points above in how we'd want to handle the date logic. Having it align with dbt_utils is the ideal design choice.

Are you interested in making the above changes that he calls out?

@callum-mcdata
Copy link
Contributor

Hi @alexberryman! I'm going to close out this PR as it is many versions behind main. If you're interested in adding this functionality to future releases, more than happy to re-open as we bring it back up!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla:yes The CLA has been signed enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to use function calls in start_date and end_date
3 participants