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

feat: add round timestamp macro #84

Merged
merged 4 commits into from
Oct 13, 2022

Conversation

jpmmcneill
Copy link
Contributor

Small macro which I find helpful.

It's quite similar to the typical sql date_trunc in that it returns a date, but it returns the closest date to a given timestamp.

@clausherther
Copy link
Contributor

Hi @jpmmcneill, thanks for this! (And sorry for only getting to this now.)
This looks very useful indeed. I'll take a look and let you know if I have any questions.

@clausherther
Copy link
Contributor

clausherther commented Oct 13, 2022

@jpmmcneill looks like we have a test failure here: https://app.circleci.com/pipelines/github/calogica/dbt-date/21/workflows/19108182-7ac3-440f-8e7c-27be40fc8df6/jobs/16?invite=true#step-104-70
I don't think we need the double quotes around the datepart maybe?

@clausherther clausherther self-requested a review October 13, 2022 15:49
@clausherther
Copy link
Contributor

FYI, getting similar errors on BigQuery, but not SF (I assume that's where you tested this?).

@clausherther
Copy link
Contributor

Oe more thing: even if I remove the quotes, the test fails on BigQuery b/c the integration test is comparing a date to the result of the date_trunc op which is a timestamp.
No matching signature for operator = for argument types: DATE, TIMESTAMP

where not(rounded_timestamp = 
    timestamp_trunc(
        cast(

        datetime_add(
            cast( time_stamp as datetime),
        interval 12 hour
        )

 as timestamp),
        day
    )
)

Since the macro is called round_timestamp, I think it's correct to return a timestamp and not a date, so maybe we want to adjust the integration test?

@clausherther
Copy link
Contributor

@jpmmcneill fyi, pushed a couple of commits that I think address my comments above.

Copy link
Contributor

@clausherther clausherther left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jpmmcneill
Copy link
Contributor Author

jpmmcneill commented Oct 13, 2022

Thanks @clausherther! Appreciate the commits. I think that this is good to be merged now

@clausherther clausherther merged commit 762effb into calogica:main Oct 13, 2022
@clausherther
Copy link
Contributor

I'll do releases for dbt-date and dbt-expectations this weekend.

@jpmmcneill jpmmcneill deleted the james/round-timestamp branch December 19, 2022 11:05
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 this pull request may close these issues.

2 participants