-
Notifications
You must be signed in to change notification settings - Fork 123
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
Add table expiry #230
base: main
Are you sure you want to change the base?
Add table expiry #230
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hours_to_expiration
should be nested within options
, just as the now merged max_staleness
does. accordingly this new clause should be adjusted to happen within the existing options loop, as described below.
if not a CETAS options clause, to what does dbt's external.options
pertain?
@@ -4,11 +4,11 @@ | |||
{%- set external = source_node.external -%} | |||
{%- set partitions = external.partitions -%} | |||
{%- set options = external.options -%} | |||
|
|||
{%- set hours_to_expiration = external.get('hours_to_expiration') -%} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i believe this is all that is needed
{%- set hours_to_expiration = external.get('hours_to_expiration') -%} | |
{%- set hours_to_expiration = external.hours_to_expiration -%} |
@@ -45,5 +45,8 @@ | |||
{%- endif -%} | |||
{%- endfor -%} | |||
{%- endif -%} | |||
{%- if hours_to_expiration -%} | |||
, expiration_timestamp = TIMESTAMP_ADD(CURRENT_TIMESTAMP(), INTERVAL {{hours_to_expiration}} hour) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we integrate this into the already existing enumeration of options.items()
(L39-L47
)? perhaps something like the below?
this solution isn't very clean either though, imho. I thought to use the fancy jinja loop.first
to exclude the leading comma in the first example, but expiration_timestamp
will never be first in the list of options as uris
are a required option. 😵 . perhaps we could refactor so that the currently unused external
field is where uris
are given??
{%- if options is mapping -%}
{%- for key, value in options.items() if key != 'uris' %}
{%- if value is string -%}
, {{key}} = '{{value}}'
{%- else -%}
{%- if key == "hours_to_expiration" -%}
, expiration_timestamp = TIMESTAMP_ADD(CURRENT_TIMESTAMP(), INTERVAL {{hours_to_expiration}} hour)
{%- else -%}
, {{key}} = {{value}}
{%- endif -%}
{%- endfor -%}
{%- endif -%}
any progress on this @thomas-vl? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@thomas-vl have you had a chance to consider my feedback from the previous review?
This PR has been marked as Stale because it has been open with no activity as of late. If you would like the PR to remain open, please comment on the PR or else it will be closed in 7 days. |
Description & motivation
Add
hours_to_expiration
as an option to external tables in bigqueryresolves: #225
Checklist