Consolidate date macros into timestamps.sql #5838

colin-rogers-dbt · 2022-09-14T16:22:44Z

resolves #5521

related PRs:

Description

Checklist

I have read the contributing guide and understand what's expected of me
I have signed the CLA
I have run this code in development and it appears to resolve the stated issue
This PR includes tests, or tests are not required/relevant for this PR
I have opened an issue to add/update docs, or docs changes are not required/relevant for this PR
I have run changie new to create a changelog entry

github-actions · 2022-09-14T16:23:08Z

Thank you for your pull request! We could not find a changelog entry for this change. For details on how to document a change, see the contributing guide.

gshank

Looks good.

McKnight-42

Looks good!

jtcohen6

Very very happy to have these macros living in one place! Even if inconsistency must remain, to support backwards compatibility, we should aim to concentrate and cordon off that inconsistency as much as possible, highlight the contradictions, and provide clear guidance for net-new projects going forward.

I like the idea of trying to also incorporate the changes I recommended in #5521 (comment), which would enable the full removal of dbt_utils.current_timestamp by dbt-utils v1.0.

What do you think about deprecating date_function + sql_now()? That's the last tine in our fork of inconsistency.

Specifically, we could:

Stop making the base date_function an abstract method that every adapter must implement
Add a deprecation warning for anyone who calls sql_now in their project code. The deprecation message can say something like: "{{ sql_now }} and {{ adapter.date_function }} are deprecated, use {{ current_timestamp() }} or {{ now() }} instead." The sql_now function is very very old (Feature/first class pg #174), I don't know of anyone who actually uses it, but we're better safe than sorry. Our deprecations module shows a warning to the user, and it also fires an anonymous tracking event that can confirm for us whether anyone is actually using the thing.

Lastly, we'll want companion PRs in all our adapter plugins, and to test each of them thoroughly. I don't think our existing suite of functional tests is sensitive enough to catch subtly breaking changes in these macros.

@dbeatty10 - do you have thoughts on how we might write more sensitive automated tests? How did you pursue the work of comparing dbt.current_timestamp() and dbt_utils.current_timestamp() in dbt-labs/dbt-utils#597 (comment)?

core/dbt/include/global_project/macros/adapters/timestamps.sql

colin-rogers-dbt · 2022-09-19T18:04:48Z

I like the idea of trying to also incorporate the changes I recommended in #5521 (comment), which would enable the full removal of dbt_utils.current_timestamp by dbt-utils v1.0.

Will pull those changes in as well

What do you think about deprecating date_function + sql_now()? That's the last tine in our fork of inconsistency.

I like the idea but should probably be a follow on pr

Lastly, we'll want companion PRs in all our adapter plugins, and to test each of them thoroughly. I don't think our existing suite of functional tests is sensitive enough to catch subtly breaking changes in these macros.

I'm thinking I'll add a functional test into core and we can copy that one into the adapters

dbeatty10

@colin-rogers-dbt I was working on some pytests in a totally separate PR and saw the code that Jerco mentioned to use yesterday that does schema comparisons. Linked inline.

tests/functional/timestamps/test_timestamps.py

jtcohen6

Very very happy to have this in for v1.3, and smooth the path for dbt-utils v1.0!

core/dbt/include/global_project/macros/adapters/timestamps.sql

tests/functional/timestamps/test_timestamps.py

core/dbt/include/global_project/macros/adapters/timestamps.sql

tests/adapter/dbt/tests/adapter/basic/test_timestamps.py

colin-rogers-dbt · 2022-09-29T17:55:08Z

One easy way to test the backwards compatibility is just to compare the new SQL to the old SQL. If they are the same, then we are done.

Good idea, I've added an assertion to our functional tests to validate the compiled SQL

tests/adapter/dbt/tests/adapter/utils/test_timestamps.py

plugins/postgres/dbt/include/postgres/macros/adapters.sql

Co-authored-by: Doug Beatty <[email protected]>

dbeatty10

There are two things I'd appreciate an additional set of eyes on before final approval (in order of importance):

New implementation for default__current_timestamp discussed here
Convention for test class names here and here

@jtcohen6 are you willing to give these a look (or suggest someone else)?

Other than that, this behavior looks as desired across postgres, snowflake, redshift, bigquery, spark, and databricks.

There are slight differences in the rendered SQL for the *_backcompat macros for the latter three vs. dbt_utils, but I don't think they make a functional difference.

Output for the curious

dbt-postgres

dbt compile --target postgres

postgres dbt.current_timestamp():
    now()

postgres dbt_utils.current_timestamp(): 
    current_timestamp::TIMESTAMP

postgres dbt.current_timestamp_backcompat(): 
    current_timestamp::TIMESTAMP

postgres dbt_utils.current_timestamp_in_utc(): 
    (current_timestamp at time zone 'utc')::TIMESTAMP

postgres dbt.current_timestamp_in_utc_backcompat(): 
    (current_timestamp at time zone 'utc')::TIMESTAMP

dbt-snowflake

dbt compile --target snowflake

snowflake dbt.current_timestamp():
    convert_timezone('UTC', current_timestamp())

snowflake dbt_utils.current_timestamp(): 
    current_timestamp::TIMESTAMP

snowflake dbt.current_timestamp_backcompat(): 
  current_timestamp::TIMESTAMP

snowflake dbt_utils.current_timestamp_in_utc(): 
    convert_timezone('UTC', 
    current_timestamp::TIMESTAMP
)::TIMESTAMP

snowflake dbt.current_timestamp_in_utc_backcompat(): 
  convert_timezone('UTC', 
  current_timestamp::TIMESTAMP
)::TIMESTAMP

dbt-redshift

dbt compile --target redshift

redshift dbt.current_timestamp():
    getdate()

redshift dbt_utils.current_timestamp(): 
    getdate()

redshift dbt.current_timestamp_backcompat(): getdate()
redshift dbt_utils.current_timestamp_in_utc(): 
    
    getdate()


redshift dbt.current_timestamp_in_utc_backcompat(): getdate()

dbt-bigquery

dbt compile --target bigquery

bigquery dbt.current_timestamp():
    CURRENT_TIMESTAMP()

bigquery dbt_utils.current_timestamp(): 
    current_timestamp

bigquery dbt.current_timestamp_backcompat(): CURRENT_TIMESTAMP()
bigquery dbt_utils.current_timestamp_in_utc(): 
    
    current_timestamp


bigquery dbt.current_timestamp_in_utc_backcompat(): CURRENT_TIMESTAMP()

dbt-spark

dbt compile --target spark

spark dbt.current_timestamp():
    current_timestamp()

spark dbt_utils.current_timestamp(): 
    current_timestamp::timestamp

spark dbt.current_timestamp_backcompat(): current_timestamp()
spark dbt_utils.current_timestamp_in_utc(): 
    
    current_timestamp::timestamp


spark dbt.current_timestamp_in_utc_backcompat(): current_timestamp()

dbt-databricks

dbt compile --target databricks

databricks dbt.current_timestamp():
    current_timestamp()

databricks dbt_utils.current_timestamp(): 
    current_timestamp::timestamp

databricks dbt.current_timestamp_backcompat(): current_timestamp()
databricks dbt_utils.current_timestamp_in_utc(): 
    
    current_timestamp::timestamp


databricks dbt.current_timestamp_in_utc_backcompat(): current_timestamp()

core/dbt/tests/util.py

…labs/dbt-core into consolidate_timestamp_logic

dbeatty10

@colin-rogers-dbt Here's what we still need to do prior to approving this:

Restore the implementation of default__current_timestamp. Rationale provided in the comments.
PRs in dbt-snowflake and dbt-redshift need to be approved. There are code suggestions provided to unblock each.
@gshank question for you: this PR has a new class named BaseCurrentTimestamps. There are PRs to add this as a test case for dbt-snowflake and dbt-redshift, but there are not PRs that add this test for dbt-bigquery and dbt-spark. Are there scenarios in which we don't add new test classes to all adapters?

Once those are done, I'll re-run this manual testing across the following adapters as additional confirmation: postgres, snowflake, redshift, bigquery, spark, and databricks.

dbeatty10 · 2022-09-30T19:19:19Z

core/dbt/include/global_project/macros/adapters/timestamps.sql

+{% macro default__current_timestamp() %}
+    current_timestamp
+{% endmacro %}


Unblocking this PR

I don't think your change is needed to accomplish the purposes of this back compatibility PR, so let's leave the existing implementation as-is (but you can move it from the old file to its new file) like so:

Suggested change

{% macro default__current_timestamp() %}

current_timestamp

{% endmacro %}

{% macro default__current_timestamp() -%}

{{ exceptions.raise_not_implemented(

'current_timestamp macro not implemented for adapter '+adapter.type()) }}

{%- endmacro %}

Providing a path for innovation and change

You proposed change to current_timestamp seems like a reasonable one to propose, and you can raise it in a stand-alone PR.

gshank · 2022-09-30T20:21:53Z

Yes, we won't be implementing all adapter zone tests for all adapters. We did expect that everything in the "basic" directory would be a requirement for all adapters, but tests in the adapter zone outside of that are optional. Not all adapters implement all features.

dbeatty10

Looks good across all adapters 🎉

Also confirmed manually:

Output

dbt-postgres

dbt compile --target postgres

postgres dbt.current_timestamp():
    now()

postgres dbt_utils.current_timestamp(): 
    current_timestamp::TIMESTAMP

postgres dbt.current_timestamp_backcompat(): 
    current_timestamp::TIMESTAMP

postgres dbt_utils.current_timestamp_in_utc(): 
    (current_timestamp at time zone 'utc')::TIMESTAMP

postgres dbt.current_timestamp_in_utc_backcompat(): 
    (current_timestamp at time zone 'utc')::TIMESTAMP

dbt-snowflake

dbt compile --target snowflake

snowflake dbt.current_timestamp():
    convert_timezone('UTC', current_timestamp())

snowflake dbt_utils.current_timestamp(): 
    current_timestamp::TIMESTAMP

snowflake dbt.current_timestamp_backcompat(): 
  current_timestamp::TIMESTAMP

snowflake dbt_utils.current_timestamp_in_utc(): 
    convert_timezone('UTC', 
    current_timestamp::TIMESTAMP
)::TIMESTAMP

snowflake dbt.current_timestamp_in_utc_backcompat(): 
  convert_timezone('UTC', 
  current_timestamp::TIMESTAMP
)::TIMESTAMP

dbt-redshift

dbt compile --target redshift

redshift dbt.current_timestamp():
    getdate()

redshift dbt_utils.current_timestamp(): 
    getdate()

redshift dbt.current_timestamp_backcompat(): getdate()
redshift dbt_utils.current_timestamp_in_utc(): 
    
    getdate()


redshift dbt.current_timestamp_in_utc_backcompat(): getdate()

dbt-bigquery

dbt compile --target bigquery

bigquery dbt.current_timestamp():
    CURRENT_TIMESTAMP()

bigquery dbt_utils.current_timestamp(): 
    current_timestamp

bigquery dbt.current_timestamp_backcompat(): current_timestamp
bigquery dbt_utils.current_timestamp_in_utc(): 
    
    current_timestamp


bigquery dbt.current_timestamp_in_utc_backcompat(): current_timestamp

dbt-spark

dbt compile --target spark

spark dbt.current_timestamp():
    CURRENT_TIMESTAMP()

spark dbt_utils.current_timestamp(): 
    current_timestamp::timestamp

spark dbt.current_timestamp_backcompat(): 
    current_timestamp::timestamp

spark dbt_utils.current_timestamp_in_utc(): 
    
    current_timestamp::timestamp


spark dbt.current_timestamp_in_utc_backcompat(): 
    current_timestamp::timestamp

dbt-databricks

dbt compile --target databricks

databricks dbt.current_timestamp():
    CURRENT_TIMESTAMP()

databricks dbt_utils.current_timestamp(): 
    current_timestamp::timestamp

databricks dbt.current_timestamp_backcompat(): 
    current_timestamp::timestamp

databricks dbt_utils.current_timestamp_in_utc(): 
    
    current_timestamp::timestamp


databricks dbt.current_timestamp_in_utc_backcompat(): 
    current_timestamp::timestamp

* Consolidate date macros into dates.sql * rename to timestamps.sql * fix whitespace + add changie * cleanup macros and add testing * fix whitespace * remove now macro * fix functional test * remove local config * make snowflake backwards compat return utc * move timestamps to adaptor base tests * move backcompat macros to respective adapters * change timestamp param to source_timestamp * move timestamps.py to utils * update changie.yaml * make expected schema a fixture * formatting * add debug message to assert * fix changie.yaml * Update tests/adapter/dbt/tests/adapter/utils/test_timestamps.py Co-authored-by: Doug Beatty <[email protected]> * Update plugins/postgres/dbt/include/postgres/macros/timestamps.sql Co-authored-by: Doug Beatty <[email protected]> * Update .changie.yaml * add backcompat utc * remove current_timestamp_in_utc * remove convert_timezone * add _in_utc_backcompat * fix macro_calls typo * add expected sql validation to test_timestamps * make expected_sql optional * improve sql check string comparison test * remove extraneous test file * add timestamp casting back * Update plugins/postgres/dbt/include/postgres/macros/adapters.sql Co-authored-by: Doug Beatty <[email protected]> * add check_relation_has_expected_schema to comments * fix whitespace * remove default impl of current_timestamp Co-authored-by: Doug Beatty <[email protected]> (cherry picked from commit a79960f)

* Consolidate date macros into dates.sql * rename to timestamps.sql * fix whitespace + add changie * cleanup macros and add testing * fix whitespace * remove now macro * fix functional test * remove local config * make snowflake backwards compat return utc * move timestamps to adaptor base tests * move backcompat macros to respective adapters * change timestamp param to source_timestamp * move timestamps.py to utils * update changie.yaml * make expected schema a fixture * formatting * add debug message to assert * fix changie.yaml * Update tests/adapter/dbt/tests/adapter/utils/test_timestamps.py Co-authored-by: Doug Beatty <[email protected]> * Update plugins/postgres/dbt/include/postgres/macros/timestamps.sql Co-authored-by: Doug Beatty <[email protected]> * Update .changie.yaml * add backcompat utc * remove current_timestamp_in_utc * remove convert_timezone * add _in_utc_backcompat * fix macro_calls typo * add expected sql validation to test_timestamps * make expected_sql optional * improve sql check string comparison test * remove extraneous test file * add timestamp casting back * Update plugins/postgres/dbt/include/postgres/macros/adapters.sql Co-authored-by: Doug Beatty <[email protected]> * add check_relation_has_expected_schema to comments * fix whitespace * remove default impl of current_timestamp Co-authored-by: Doug Beatty <[email protected]> (cherry picked from commit a79960f) Co-authored-by: colin-rogers-dbt <[email protected]> Co-authored-by: leahwicz <[email protected]>

Consolidate date macros into dates.sql

977b841

colin-rogers-dbt requested a review from a team as a code owner September 14, 2022 16:22

colin-rogers-dbt requested a review from McKnight-42 September 14, 2022 16:22

cla-bot bot added the cla:yes label Sep 14, 2022

rename to timestamps.sql

b0a6eb4

colin-rogers-dbt changed the title ~~Consolidate date macros into dates.sql~~ Consolidate date macros into timestamps.sql Sep 14, 2022

colin-rogers-dbt self-assigned this Sep 14, 2022

fix whitespace + add changie

bd4b705

colin-rogers-dbt requested a review from a team as a code owner September 14, 2022 16:56

colin-rogers-dbt requested a review from gshank September 14, 2022 16:56

gshank approved these changes Sep 14, 2022

View reviewed changes

McKnight-42 approved these changes Sep 16, 2022

View reviewed changes

jtcohen6 previously requested changes Sep 19, 2022

View reviewed changes

colin-rogers-dbt added 2 commits September 19, 2022 18:28

cleanup macros and add testing

580d70f

fix whitespace

cd439ba

colin-rogers-dbt requested a review from jtcohen6 September 20, 2022 17:44

colin-rogers-dbt added 3 commits September 20, 2022 16:15

remove now macro

44c1809

fix functional test

3c47be6

remove local config

9404bba

dbeatty10 reviewed Sep 21, 2022

View reviewed changes

tests/functional/timestamps/test_timestamps.py Outdated Show resolved Hide resolved

make snowflake backwards compat return utc

471a63b

jtcohen6 reviewed Sep 22, 2022

View reviewed changes

move timestamps to adaptor base tests

d29dfb9

colin-rogers-dbt commented Sep 22, 2022

View reviewed changes

tests/adapter/dbt/tests/adapter/basic/test_timestamps.py Outdated Show resolved Hide resolved

colin-rogers-dbt added 4 commits September 22, 2022 10:14

move backcompat macros to respective adapters

c44b2ae

change timestamp param to source_timestamp

2bcee96

move timestamps.py to utils

33cd449

Merge branch 'main' into consolidate_timestamp_logic

fd07bd0

Merge branch 'main' into consolidate_timestamp_logic

d8bea3e

dbeatty10 reviewed Sep 29, 2022

View reviewed changes

tests/adapter/dbt/tests/adapter/utils/test_timestamps.py Outdated Show resolved Hide resolved

dbeatty10 reviewed Sep 29, 2022

View reviewed changes

plugins/postgres/dbt/include/postgres/macros/adapters.sql Outdated Show resolved Hide resolved

colin-rogers-dbt and others added 2 commits September 29, 2022 14:56

add timestamp casting back

345fea1

Update plugins/postgres/dbt/include/postgres/macros/adapters.sql

a8b5bfe

Co-authored-by: Doug Beatty <[email protected]>

dbeatty10 reviewed Sep 30, 2022

View reviewed changes

gshank reviewed Sep 30, 2022

View reviewed changes

core/dbt/tests/util.py Show resolved Hide resolved

Merge branch 'consolidate_timestamp_logic' of https://github.com/dbt-…

c8e8f99

…labs/dbt-core into consolidate_timestamp_logic

colin-rogers-dbt requested a review from dbeatty10 September 30, 2022 16:16

add check_relation_has_expected_schema to comments

efc339e

colin-rogers-dbt requested a review from gshank September 30, 2022 16:19

fix whitespace

c97cead

dbeatty10 mentioned this pull request Sep 30, 2022

consolidate timestamp macros dbt-labs/dbt-snowflake#273

Merged

6 tasks

dbeatty10 requested changes Sep 30, 2022

View reviewed changes

remove default impl of current_timestamp

2664dfc

dbeatty10 self-requested a review September 30, 2022 22:30

dbeatty10 approved these changes Sep 30, 2022

View reviewed changes

colin-rogers-dbt merged commit a79960f into main Sep 30, 2022

colin-rogers-dbt deleted the consolidate_timestamp_logic branch September 30, 2022 22:33

leahwicz added the backport 1.3.latest label Oct 3, 2022

github-actions bot mentioned this pull request Oct 3, 2022

[Backport 1.3.latest] Consolidate date macros into timestamps.sql #5985

Merged

findinpath mentioned this pull request Oct 13, 2022

Analyze whether the migration of dbt_utils current_timestamp macros in dbt 1.3.0 affects dbt-trino starburstdata/dbt-trino#156

Closed

1 task

jtcohen6 mentioned this pull request Nov 1, 2022

[CT-1457] Move dbt-postgres into its own repository #6189

Closed

dbeatty10 mentioned this pull request Jan 10, 2023

Remove duplicated changelog entry for 5521 #6563

Merged

dbeatty10 mentioned this pull request May 15, 2023

Cross-database current_timestamp() macro dbt-labs/docs.getdbt.com#3370

Closed

1 task

aranke mentioned this pull request Jul 19, 2023

Hotfix for INC-372: Use JSONEncoder in json.dumps #8149

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Consolidate date macros into timestamps.sql #5838

Consolidate date macros into timestamps.sql #5838

colin-rogers-dbt commented Sep 14, 2022 •

edited

Loading

github-actions bot commented Sep 14, 2022

gshank left a comment

McKnight-42 left a comment

jtcohen6 left a comment

colin-rogers-dbt commented Sep 19, 2022

dbeatty10 left a comment

jtcohen6 left a comment

colin-rogers-dbt commented Sep 29, 2022

dbeatty10 left a comment

dbeatty10 left a comment

dbeatty10 Sep 30, 2022

gshank commented Sep 30, 2022

dbeatty10 left a comment

-{% macro default__current_timestamp() %}
-    current_timestamp
-{% endmacro %}
+{% macro default__current_timestamp() -%}
+  {{ exceptions.raise_not_implemented(
+    'current_timestamp macro not implemented for adapter '+adapter.type()) }}
+{%- endmacro %}

Consolidate date macros into timestamps.sql #5838

Consolidate date macros into timestamps.sql #5838

Conversation

colin-rogers-dbt commented Sep 14, 2022 • edited Loading

Description

Checklist

github-actions bot commented Sep 14, 2022

gshank left a comment

Choose a reason for hiding this comment

McKnight-42 left a comment

Choose a reason for hiding this comment

jtcohen6 left a comment

Choose a reason for hiding this comment

colin-rogers-dbt commented Sep 19, 2022

dbeatty10 left a comment

Choose a reason for hiding this comment

jtcohen6 left a comment

Choose a reason for hiding this comment

colin-rogers-dbt commented Sep 29, 2022

dbeatty10 left a comment

Choose a reason for hiding this comment

dbt-postgres

dbt-snowflake

dbt-redshift

dbt-bigquery

dbt-spark

dbt-databricks

dbeatty10 left a comment

Choose a reason for hiding this comment

dbeatty10 Sep 30, 2022

Choose a reason for hiding this comment

Unblocking this PR

Providing a path for innovation and change

gshank commented Sep 30, 2022

dbeatty10 left a comment

Choose a reason for hiding this comment

dbt-postgres

dbt-snowflake

dbt-redshift

dbt-bigquery

dbt-spark

dbt-databricks

colin-rogers-dbt commented Sep 14, 2022 •

edited

Loading