-
Notifications
You must be signed in to change notification settings - Fork 178
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
Use timestamp_tz type in microbatch delete
DDL
#1257
Conversation
…sql incremental_predicates
@@ -58,10 +58,10 @@ | |||
|
|||
{#-- Add additional incremental_predicates to filter for batch --#} | |||
{% if model.config.get("__dbt_internal_microbatch_event_time_start") -%} | |||
{% do incremental_predicates.append("DBT_INTERNAL_TARGET." ~ model.config.event_time ~ " >= TIMESTAMP '" ~ model.config.__dbt_internal_microbatch_event_time_start ~ "'") %} | |||
{% do incremental_predicates.append("DBT_INTERNAL_TARGET." ~ model.config.event_time ~ " >= to_timestamp_tz('" ~ model.config.__dbt_internal_microbatch_event_time_start ~ "')") %} |
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.
Interestingly, just using the raw timestamp (that includes UTC offset, which model.config.__dbt_internal_microbatch_event_time_start
does) works well. E.g. just removing TIMESTAMP
would have been enough here. However, the to_timestamp_tz
piece feels more intentional so I've opted for that.
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 like how using TIMESTAMP
was causing the issue 🫠 I like th emove to using to_timestamp_tz
instead of just removing TIMESTAMP
👍
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.
You are crushiiiing it
@@ -58,10 +58,10 @@ | |||
|
|||
{#-- Add additional incremental_predicates to filter for batch --#} | |||
{% if model.config.get("__dbt_internal_microbatch_event_time_start") -%} | |||
{% do incremental_predicates.append("DBT_INTERNAL_TARGET." ~ model.config.event_time ~ " >= TIMESTAMP '" ~ model.config.__dbt_internal_microbatch_event_time_start ~ "'") %} | |||
{% do incremental_predicates.append("DBT_INTERNAL_TARGET." ~ model.config.event_time ~ " >= to_timestamp_tz('" ~ model.config.__dbt_internal_microbatch_event_time_start ~ "')") %} |
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 like how using TIMESTAMP
was causing the issue 🫠 I like th emove to using to_timestamp_tz
instead of just removing TIMESTAMP
👍
New commit 238bae2 just cleans up some extraneous configuration in test code, that was accidentally left in from development. No functional changes. |
(cherry picked from commit 5cc5d22) Co-authored-by: Michelle Ark <[email protected]>
resolves #1256
Problem
When specifying a timestamp like
TIMESTAMP '2024-11-05 00:00:00+00:00'
, snowflake uses the system timezone, even if the timestamp has offsets. This causes issues in microbatch because thedelete
DDL statement is leveraging theTIMESTAMP
typing, and is deleting the wrong records, potentially causing duplicate rows to be inserted.Solution
to_timestamp_tz
when computing incremental predicates for microbatch DDLChecklist