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

sql/parser: add timestamptz alias for timestamp data type #5893

Merged
merged 2 commits into from
Apr 6, 2016

Conversation

dt
Copy link
Member

@dt dt commented Apr 6, 2016

Our current 'timestamp' datatype has the same behavior as postgres' timestamptz type:
it accepts strings containing an offset and includes that offset when calculating a utc timestamp, and when returned to the client has an offset (usually 0).

Fixes #5865.


This change is Reviewable

@dt dt assigned danhhz Apr 6, 2016
@petermattis
Copy link
Collaborator

:lgtm:


Review status: 0 of 4 files reviewed at latest revision, 1 unresolved discussion.


sql/parser/sql.y, line 2722 [r1] (raw file):
Some of the other *Type structures contain a Name field so that we can perfectly round-trip the SQL. See FloatType and StringType for examples.


Comments from Reviewable

@RaduBerinde
Copy link
Member

:lgtm:


Review status: 0 of 4 files reviewed at latest revision, 1 unresolved discussion, some commit checks pending.


Comments from Reviewable

@dt
Copy link
Member Author

dt commented Apr 6, 2016

Review status: 0 of 4 files reviewed at latest revision, 1 unresolved discussion, some commit checks pending.


sql/parser/sql.y, line 2722 [r1] (raw file):
hm, do we need this to roundtrip? As indicated in the test, I was thinking we'd just normalize it to TIMESTAMP all the time, at least as long as it is just an alias (in the future we might find a client actually wants a timestamp with no offset returned and it'll actually be different behavior)


Comments from Reviewable

@petermattis
Copy link
Collaborator

Review status: 0 of 4 files reviewed at latest revision, 1 unresolved discussion, some commit checks pending.


sql/parser/sql.y, line 2722 [r1] (raw file):
I don't have a specific use case for needing the round-tripping, yet I think we should err on the side of doing so when it isn't difficult.


Comments from Reviewable

Our current 'timestamp' datatype has the same behavior as postgres' timestamptz type:
it accepts strings containing an offset and includes that offset when calculating
a utc timestamp, and when returned to the client has an offset (usually 0).

Fixes cockroachdb#5865.
@dt
Copy link
Member Author

dt commented Apr 6, 2016

Review status: 0 of 6 files reviewed at latest revision, 1 unresolved discussion.


sql/parser/sql.y, line 2722 [r1] (raw file):
Done.


Comments from Reviewable

@petermattis
Copy link
Collaborator

Review status: 0 of 6 files reviewed at latest revision, 1 unresolved discussion.


sql/parser/sql.y, line 2722 [r1] (raw file):
I was imagining TimestampType{Name: "TIMESTAMPTZ"}, but this works too.


Comments from Reviewable

@dt dt merged commit a74e831 into cockroachdb:master Apr 6, 2016
@dt dt deleted the tz branch April 6, 2016 17:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants