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

Issue 16 in BQ date should be compared to date #17

Merged
merged 8 commits into from
Apr 3, 2021
Merged

Issue 16 in BQ date should be compared to date #17

merged 8 commits into from
Apr 3, 2021

Conversation

VasiliiSurov
Copy link
Contributor

Adding BigQuery specific macros get_base_dates

@clausherther clausherther self-requested a review April 3, 2021 14:34
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.

Thanks for this @VasiliiSurov, helps a lot! I made a couple of comments where we need to add some params but then I think this might be good to go!

@@ -1,4 +1,8 @@
{% macro get_base_dates(start_date=None, end_date=None, n_dateparts=None, datepart=None) %}
{{ adapter.dispatch('get_base_dates', packages = dbt_date._get_utils_namespaces()) (start_date, end_date) }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @VasiliiSurov, love the adapter macro version here, but I think you need to also pass the additional parameters like n_dateparts and datepart to the call to dispatch and also the subsequent macros it's calling, right?

{{ adapter.dispatch('get_base_dates', packages = dbt_date._get_utils_namespaces()) (start_date, end_date) }}
{% endmacro %}

{% macro default__get_base_dates(start_date, end_date) %}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you also need n_dateparts, datepart here

{% endmacro %}

{% macro bigquery__get_base_dates(start_date, end_date) %}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you also need n_dateparts, datepart here

@clausherther clausherther linked an issue Apr 3, 2021 that may be closed by this pull request
@VasiliiSurov
Copy link
Contributor Author

@clausherther
totally make sense, I didn't use these params so they felt out from my attention

@clausherther clausherther self-requested a review April 3, 2021 14:59
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.

Minor change and then I think we're good to go!

{{ adapter.dispatch('get_base_dates', packages = dbt_date._get_utils_namespaces()) (start_date, end_date, n_dateparts, datepart) }}
{% endmacro %}

{% macro default__get_base_dates(start_date=None, end_date=None, n_dateparts=None, datepart=None) %}
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for adding those params!
Sorry, one more nitpick: the default__get_base_dates and bigquery__get_base_dates macros don't need the None default parameter values since this get passed in from the dispatch. I'd like to leave those out so we can change the default parameters at the entry point macro without having to change them everywhere else. Thanks!

@VasiliiSurov
Copy link
Contributor Author

@clausherther
Done

@clausherther clausherther self-requested a review April 3, 2021 15:11
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.

Looks good, thanks! 👍

@clausherther clausherther merged commit e58387d into calogica:main Apr 3, 2021
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.

'dates' model produces error on BigQuery
2 participants