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

consolidate timestamp macros #273

Merged
merged 21 commits into from
Sep 30, 2022
Merged

Conversation

colin-rogers-dbt
Copy link
Contributor

@colin-rogers-dbt colin-rogers-dbt commented Sep 22, 2022

resolves #276

Description

Checklist

@cla-bot cla-bot bot added the cla:yes label Sep 22, 2022
@github-actions
Copy link
Contributor

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 dbt-snowflake contributing guide.

@colin-rogers-dbt colin-rogers-dbt marked this pull request as ready for review September 23, 2022 18:26
tests/functional/adapter/test_timestamps.py Outdated Show resolved Hide resolved
.changes/unreleased/Breaking Changes-20220923-112314.yaml Outdated Show resolved Hide resolved
.changes/unreleased/Breaking Changes-20220923-112314.yaml Outdated Show resolved Hide resolved
dev-requirements.txt Outdated Show resolved Hide resolved
dev-requirements.txt Outdated Show resolved Hide resolved
@colin-rogers-dbt colin-rogers-dbt dismissed dbeatty10’s stale review September 27, 2022 00:17

removed, will look into dev reqs later

Copy link
Contributor

@dbeatty10 dbeatty10 left a comment

Choose a reason for hiding this comment

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

This looks good 👍

Left one comment for you to take a peek at related to casing and another related to pytest class conventions.

✅ Manually confirmed the same results that you got in your automated tests:

Output
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

Merge notes

After dbt-labs/dbt-core#5838 is merged, then you can commit an update to dev-requirements.txt in this PR prior to merging it.

tests/functional/adapter/test_timestamps.py Show resolved Hide resolved
{{current_timestamp_backcompat()}} as current_timestamp_backcompat
"""

class TestCurrentTimestampSnowflake(test_timestamps.TestCurrentTimestamps):
Copy link
Contributor

Choose a reason for hiding this comment

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

The pattern I've seen in our code is like this:

from dbt.tests.adapter.utils.test_timestamps import BaseCurrentTimestamps

class TestCurrentTimestampSnowflake(BaseCurrentTimestamps):

We can keep the discussion about this over here and here though. Just pointing this out for sake of completeness.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

Comment on lines 2 to 11
from dbt.tests.adapter.utils import test_timestamps

_MODEL_CURRENT_TIMESTAMP = """
SELECT {{current_timestamp()}} as current_timestamp,
{{current_timestamp_in_utc_backcompat()}} as current_timestamp_in_utc_backcompat,
{{current_timestamp_backcompat()}} as current_timestamp_backcompat
"""


class TestCurrentTimestampSnowflake(test_timestamps.BaseCurrentTimestamps):
Copy link
Contributor

Choose a reason for hiding this comment

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

Per @gshank's comment here, let's make this consistent with the rest of the code base.

Suggested change
from dbt.tests.adapter.utils import test_timestamps
_MODEL_CURRENT_TIMESTAMP = """
SELECT {{current_timestamp()}} as current_timestamp,
{{current_timestamp_in_utc_backcompat()}} as current_timestamp_in_utc_backcompat,
{{current_timestamp_backcompat()}} as current_timestamp_backcompat
"""
class TestCurrentTimestampSnowflake(test_timestamps.BaseCurrentTimestamps):
from dbt.tests.adapter.utils.test_timestamps import BaseCurrentTimestamps
_MODEL_CURRENT_TIMESTAMP = """
SELECT {{current_timestamp()}} as current_timestamp,
{{current_timestamp_in_utc_backcompat()}} as current_timestamp_in_utc_backcompat,
{{current_timestamp_backcompat()}} as current_timestamp_backcompat
"""
class TestCurrentTimestampSnowflake(BaseCurrentTimestamps):

Copy link
Contributor

@dbeatty10 dbeatty10 left a comment

Choose a reason for hiding this comment

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

👍

@colin-rogers-dbt colin-rogers-dbt merged commit 2ef59aa into main Sep 30, 2022
@colin-rogers-dbt colin-rogers-dbt deleted the consolidateTimestampMacros branch September 30, 2022 22:57
github-actions bot pushed a commit that referenced this pull request Oct 3, 2022
* consolidate timestamp macros

* add testing changes

* deleting extra timestamps.sql

* changie entry

* fix backcompat

* Update Breaking Changes-20220923-112314.yaml

* Update dev-requirements.txt

* remove current_timestamp_in_utc

* fix backcompat

* change test import

* fix BaseCurrentTimestamps import

* update dev-requirements

* Update change log body

* fix changie log

(cherry picked from commit 2ef59aa)
leahwicz pushed a commit that referenced this pull request Oct 3, 2022
* consolidate timestamp macros

* add testing changes

* deleting extra timestamps.sql

* changie entry

* fix backcompat

* Update Breaking Changes-20220923-112314.yaml

* Update dev-requirements.txt

* remove current_timestamp_in_utc

* fix backcompat

* change test import

* fix BaseCurrentTimestamps import

* update dev-requirements

* Update change log body

* fix changie log

(cherry picked from commit 2ef59aa)

Co-authored-by: colin-rogers-dbt <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[CT-1244] [Feature] Consolidate current_timestamp & associates
3 participants