-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
[CT-895] [Spike] Consolidate current_timestamp & associates #5521
Comments
I have no idea what this means in terms of actual work, but I'm in. 👍 |
Couple of unordered thoughts on
|
Converting to UTC and then casting to |
* Initialize lift + shift, dateadd + datediff * Install branches of all plugins * Use dispatch for passthrough behavior * Only support dbt-core >= 1.2 * Switch to alternative dev branches * Lift and shift cross-database macros, fixtures, and tests into dbt-core and adapters * Test `current_timestamp()` adapter definition vs. original dbt-utils * Split out the `current_timestamp` work into its own separate effort (#599) * Reinstate original `current_timestamp` implementation * Reinstate original `type_*` implementations * Reinstate original `current_timestamp_in_utc` implementation * Kick out the `current_timestamp` + `type_*` tests * Preserve `dbt_utils` as one more step along the dispatch train * Remove branch identifiers Co-authored-by: Jeremy Cohen <[email protected]>
I'm going to transfer this to |
Just learned a couple of things on Snowflake:
|
Added a comparison of dbt-core+adapters implementation vs. dbt-utils in this comment. TLDR: they don't appear to yield the same data types. |
@Fleid ☝️ this is the ticket that we were discussing earlier today about inconsistent timestamp data types across adapters. |
@dbeatty10 Amazing work pulling together all the threads from your research! I think there are two potential scopes for this work:
The table you created (and included as a screenshot in your comment) really clinches it for me. We probably don't have the time, capacity, or ability to totally rationalize our approach to GOAL: Find a path that can unlock "better" (unblocking dbt-utils v1.0), even though it will not get us to "perfect" (total consistency within Here's what I think we COULD do now, to unblock step 1:
I'm poking around with the code a bit to see if that's possible. I'm not sure yet — I'll open a branch if I get there. |
I haven't had a chance to actually test any of this yet — just sharing the links here to share how I'm thinking about the narrower path I outlined above: Motivation: If there's going to be inconsistency — feels unavoidable unless we break things — better to have it all in one place. |
Awesome to see your thinking on this @jtcohen6 ! Agreed that inconsistency is unavoidable unless we break things. Also agreed that it is better to preserve that inconsistency rather than introducing breaking changes before dbt v2. What's the advantage of adding Is it so that users of dbt_utils >= 1.0.0 can update their references from If that is the case, how is that better for users rather than leaving |
Exactly my thinking.
If there's going to be inconsistency, I'd rather have it all in one place! Right now, it's spread across And we can make clear that the backcompat is just there for that reason — backward compatibility — and shouldn't be used by anyone new. That doesn't feel clear if we keep |
Here is where I ended up at for dbt-core changes: https://github.com/dbt-labs/dbt-core/pull/5838/files#diff-4ce397ee2492fb1078425ea420483cf3b55645b5bbcee3139ae696ad07c6c1bc Still need to polish/test everything but it consolidates the dbt-core logic into a single file. Can reconcile our two versions (I don't have the backwards compat). |
Split out from dbt-labs/dbt-utils#577 + dbt-labs/dbt-utils#597, resolves dbt-labs/dbt-utils#339
Describe the feature
Rewrite
current_timestamp
+current_timestamp_utc
to be built out of core/plugin functionality (e.g.date_function
) and aconvert_timezone
macro.Currently in
dbt-core
, there are three methods/macros all purporting to do the same thing. I've linked the examples from Postgres.date_function
(Python classmethod)snapshot_get_time
(macro used for snapshots)current_timestamp
(macro used nowhere, except as the default implementation ofsnapshot_get_time
— but often the two are different, mostly for data type reasons)Then, there are the two versions currently living in
dbt-utils
:dbt_utils.current_timestamp
dbt_utils.current_timestamp_in_utc
Things we need to figure out
TIMEZONE
andTIMESTAMP_TYPE_MAPPING
are both setIdeal outcome
When I close my eyes and imagine how this could be really good:
dbt-core
and implemented by each adapter plugin, that provides the SQL for getting the current timestamp. Whenever we need the current timestamp in a different data type, we use thatcurrent_timestamp
macro plus a data type conversion function (see Use built-in adapter functionality for datatypes dbt-utils#586).convert_timezone
macro, defined indbt-core
and implemented by each adapter plugin. @clausherther has already written one for thedbt-date
package—Claus, would you have any interest in kicking it upstream? :)current_timestamp_in_utc
macro that builds on top of 1+2. Whether that lives indbt-core
,dbt-utils
, or somewhere else doesn't matter so much to me. Wherever it lives, it should look a lot likedbt_date.now(tz=None)
. We'd leave thedbt_utils
one in place for backwards compatibility.Questions
Describe alternatives you've considered
Leaving all of it where it is :)
Additional context
Timezones are no fun
This stuff is pretty tricky to test. We're not really testing it currently. I had an idea in dbt-labs/dbt-utils#577 (comment), but it's not a great one. We are indirectly testing our use of
snapshot_get_time
indbt-core
functional tests (specificallytest_hard_delete_snapshot
.Who will this benefit?
dbt-date
package (I hope!)The text was updated successfully, but these errors were encountered: