Skip to content

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

Merged
graingert merged 2 commits into
sqlalchemy-redshift:masterfrom
bouk:fix-now
Aug 19, 2015
Merged

Use SYSDATE when func.NOW() is used#15
graingert merged 2 commits into
sqlalchemy-redshift:masterfrom
bouk:fix-now

Conversation

@bouk

@bouk bouk commented Aug 19, 2015

Copy link
Copy Markdown
Contributor

@graingert

graingert commented Aug 19, 2015

Copy link
Copy Markdown
Member

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

graingert
graingert reviewed Aug 19, 2015
View reviewed changes
Comment thread tests/test_compiler.py Outdated

@graingert graingert Aug 19, 2015

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

just instantiate the dialect in the test function

@graingert

graingert commented Aug 19, 2015

Copy link
Copy Markdown
Member

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

@graingert

graingert commented Aug 19, 2015

Copy link
Copy Markdown
Member

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

@bouk

bouk commented Aug 19, 2015

Copy link
Copy Markdown
Contributor Author

@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

graingert commented Aug 19, 2015

Copy link
Copy Markdown
Member

Great, can you add a changelog entry, please?

@graingert

graingert commented Aug 19, 2015

Copy link
Copy Markdown
Member

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

@bouk

bouk commented Aug 19, 2015

Copy link
Copy Markdown
Contributor Author

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

@bouk

bouk commented Aug 19, 2015

Copy link
Copy Markdown
Contributor Author

Changelog added

graingert
graingert reviewed Aug 19, 2015
View reviewed changes
Comment thread CHANGES.rst Outdated

@graingert graingert Aug 19, 2015

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

@bouk bouk Aug 19, 2015

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

🙉 done

graingert added a commit that referenced this pull request Aug 19, 2015
@graingert
Merge pull request #15 from bouk/fix-now
232ea93 
Use SYSDATE when func.NOW() is used
@graingert graingert merged commit 232ea93 into sqlalchemy-redshift:master Aug 19, 2015
@bouk bouk deleted the fix-now branch August 19, 2015 17:44
haleemur pushed a commit to haleemur/redshift_sqlalchemy that referenced this pull request Sep 2, 2015
@graingert
Merge pull request sqlalchemy-redshift#15 from bouk/fix-now
b84c1cc 
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

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@bouk @graingert