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

SNOW-227101: fix to datetime columns being created without timezone #202

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Nazarii
Copy link

@Nazarii Nazarii commented Nov 19, 2020

Original ticket.

Commit 28e8031 introduced a bug where all sqlalchemy.DateTime(timezone=True) columns were created as DATETIME in Snowflake which is an alias to TIMESTAMP_NTZ.
This commit reverts datetime logic back so if the timezone is defined in datetime column it will create TIMESTAMP_TZ and TIMESTAMP_NTZ otherwise.
This bug was introduced in v1.1.18

@WeiGuHS
Copy link

WeiGuHS commented Nov 20, 2021

@Nazarii do you know when this commit can be merged? and also is a reason that take out these TIMESTAMP_TZ from v1.1.8?

@Nazarii
Copy link
Author

Nazarii commented Nov 20, 2021

@WeiGuHS no idea, this PR has no reviewers assigned, the same with the original ticket. I have no permissions to assign PR, I'll try to ping other contributors to review. It looks like the support of TIMESTAMP_TZ was removed by a mistake.

@github-actions
Copy link

github-actions bot commented Apr 20, 2022

CLA Assistant Lite bot:
Thank you for your submission, we really appreciate it. Like many open-source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution. You can sign the CLA by just posting a Pull Request Comment same as the below format.


I have read the CLA Document and I hereby sign the CLA


You can retrigger this bot by commenting recheck in this Pull Request

@Nazarii
Copy link
Author

Nazarii commented Apr 20, 2022

I have read the CLA Document and I hereby sign the CLA

@Nazarii
Copy link
Author

Nazarii commented Apr 20, 2022

Not sure why but this PR shows there are conflicts. Diff does not show any merge conflicts or maybe there are some other conflicts?

@sfc-gh-mkeller
Copy link
Collaborator

We will revisit this issue shortly.
In the meantime TIMESTAMP is an alias that is configurable actually, please see our docs to see how to change what it points at.

@philippeboyd
Copy link

philippeboyd commented May 12, 2022

@sfc-gh-mkeller any news on this?

conflicts comes from GEOGRAPHY being added at the same spot in the file as your modifications

      def visit_TIMESTAMP(self, type_, **kw):
++<<<<<<< HEAD
 +        return "TIMESTAMP"
 +
 +    def visit_GEOGRAPHY(self, type_, **kw):
 +        return "GEOGRAPHY"
++=======
+         is_local = kw.get('is_local', False)
+         timezone = kw.get('timezone', type_.timezone)
+         return "TIMESTAMP %s%s" % (
+             (timezone and "WITH" or "WITHOUT") + (
+                     is_local and " LOCAL" or "") + " TIME ZONE",
+             "(%d)" % type_.precision if getattr(type_, 'precision',
+                                                None) is not None else ""
+         )
++>>>>>>> 9620a0a (SNOW-227101: fix to datetime columns being created without timezone (TIMESTAMP_NTZ).)```

…TIMESTAMP_NTZ).

Commit 28e8031 introduced a bug where all sqlalchemy.DateTime(timezone=True) columns were created as DATETIME in Snowflake which is an alias to TIMESTAMP_NTZ.
This commit reverts datetime logic back so if the timezone is defined in datetime column it will create TIMESTAMP_TZ and TIMESTAMP_NTZ otherwise.

(cherry picked from commit e180b23)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants