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

Use SYSDATE when func.NOW() is used #15

Merged
merged 2 commits into from Aug 19, 2015
Merged

Conversation

@bouk
Copy link
Contributor

@bouk bouk commented Aug 19, 2015

@graingert
Copy link
Collaborator

@graingert graingert commented Aug 19, 2015

what's the advantage of SYSDATE over CURRENT_DATE or TRUNC(GETDATE())?

@graingert
graingert reviewed Aug 19, 2015
View changes
tests/test_compiler.py Outdated
def dialect(self):
return RedshiftDialect()

def test_func_now(self, dialect):

This comment has been minimized.

@graingert

graingert Aug 19, 2015
Collaborator

just instantiate the dialect in the test function

@graingert
graingert reviewed Aug 19, 2015
View changes
tests/test_compiler.py Outdated
from redshift_sqlalchemy.dialect import RedshiftDialect


class TestCompiler(object):

This comment has been minimized.

@graingert

graingert Aug 19, 2015
Collaborator

no need for a test class here

@@ -19,6 +19,12 @@ class RedshiftImpl(postgresql.PostgresqlImpl):
__dialect__ = 'redshift'


class RedshiftCompiler(PGCompiler):

This comment has been minimized.

@graingert

graingert Aug 19, 2015
Collaborator

is there anything that gets registered to the PGCompiler that sub-classing it might miss?

This comment has been minimized.

@bouk

bouk Aug 19, 2015
Author Contributor

I think the subclassing was an issue before because of the name being set on the dialect, it won't be an issue here

@graingert
Copy link
Collaborator

@graingert graingert commented Aug 19, 2015

NOW() is only deprecated not removed, does it have different semantics to SYSDATE? Are you sure people never want to call this function?

@graingert
Copy link
Collaborator

@graingert graingert commented Aug 19, 2015

@bouk thanks for this PR, it looks great I just have a few questions about the specifics (see above)

@bouk bouk force-pushed the bouk:fix-now branch to e2e7bf3 Aug 19, 2015
@bouk
Copy link
Contributor Author

@bouk bouk commented Aug 19, 2015

@graingert CURRENT_DATE and TRUNC(GETDATE()) are for dates, while SYSDATE is date and time (I know, it makes no sense). Seems that's an error in the Redshift documentation

The reason I picked SYSDATE over GETDATE() is because SYSDATE includes microseconds http://docs.aws.amazon.com/redshift/latest/dg/r_GETDATE.html

Here's a screenshot of the various outputs:

The reason this change is needed is because Redshift doesn't support NOW() in the context of a table, when inserting for example:

I've made the changes you noted

@graingert
Copy link
Collaborator

@graingert graingert commented Aug 19, 2015

Great, can you add a changelog entry, please?

@graingert
Copy link
Collaborator

@graingert graingert commented Aug 19, 2015

Ah I notice NOW() has a timezone "+00"

@bouk
Copy link
Contributor Author

@bouk bouk commented Aug 19, 2015

@graingert redshift doesn't even support timestamps with timezones in tables so I don't think that matters

@bouk
Copy link
Contributor Author

@bouk bouk commented Aug 19, 2015

Changelog added

@graingert
graingert reviewed Aug 19, 2015
View changes
CHANGES.rst Outdated
@@ -2,7 +2,9 @@
0.1.3 (unreleased)
------------------

- Nothing changed yet.
- Use SYSDATE instead of NOW()

This comment has been minimized.

@graingert

graingert Aug 19, 2015
Collaborator

one last little thing: missing a period here at the end of the line.

This comment has been minimized.

@bouk

bouk Aug 19, 2015
Author Contributor

🙉 done

@bouk bouk force-pushed the bouk:fix-now branch to ec8c42a Aug 19, 2015
graingert added a commit that referenced this pull request Aug 19, 2015
Use SYSDATE when func.NOW() is used
@graingert graingert merged commit 232ea93 into sqlalchemy-redshift:master Aug 19, 2015
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@bouk bouk deleted the bouk:fix-now branch Aug 19, 2015
haleemur pushed a commit to haleemur/redshift_sqlalchemy that referenced this pull request Sep 2, 2015
Use SYSDATE when func.NOW() is used
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.