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.

Sign up for GitHub

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

Jump to bottom

Document the TIMETZ type #3102

Merged
merged 1 commit into from
May 16, 2018
Merged

Document the TIMETZ type #3102

merged 1 commit into from
May 16, 2018

Conversation

jseldess
Copy link
Contributor

@jseldess jseldess commented May 4, 2018

Also minor tweaks to the TIMEZONE type docs.

Fixes #2982

@jseldess jseldess requested a review from windchan7 May 4, 2018 18:32
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@cockroach-teamcity
Copy link
Member

http://cockroach-docs-review.s3-website-us-east-1.amazonaws.com/7ed98610f6536caf74283857ab6c3380bd911256/

@windchan7
Copy link

Generally the timetz part looks pretty good, except that we don't recommend use of TIMETZ.
TIMETZ is a very confusing type so I would strongly discourage anyone from using it.

Review status: 0 of 3 files reviewed at latest revision, all discussions resolved, all commit checks successful.

v2.1/time.md, line 24 at r1 (raw file):

## Best Practices

We recommend always using the `TIMETZ` variant because the `TIME` variant can sometimes lead to unexpected behaviors when it ignores a session offset. However, we also recommend you avoid setting a session time for your database.

We actually do not recommend use of TIMETZ because the original design from Postgres is broken. Since we don't store the time zone internally, we do not match Postgres's behavior a hundred percent.

Say in the SQL command line, when we are executing the following query: SELECT '07:00:01-06:00':::TIMETZ > '23:00:01-04:00':::TIMETZ, the result would be false, which matches Postgres's behavior. It's because when doing a comparison, '23:00:01-04:00':::TIMETZ is considered to be a time of "next day", even though the concept of day shouldn't exist in TIMETZ.

However, if we store '07:00:01-06:00':::TIMETZ and '23:00:01-04:00':::TIMETZ in a table and do select * from TABLE order by time;, '23:00:01-04:00':::TIMETZ will appear before '07:00:01-06:00':::TIMETZ, being displayed as 13:00:01+00:00. That's because we don't store time zone information for TIMETZ so they are stored without day information.

In addition, in the original postgres doc, it doesn't recommend use of timetz either.

Comments from Reviewable

@windchan7 windchan7 requested a review from dt May 4, 2018 19:06
@jseldess
Copy link
Contributor Author

jseldess commented May 7, 2018

Thanks for the feedback, @windchan7! @rmloveland will take this over.

I incorrectly assumed that the best practices for timetz would mirror the best practices for timestamptz. Interesting that we recommend using timestamptz but not timetz. Rich can dig in more with you and @dt.

@windchan7
Copy link

@jseldess Yeah, time types are confusing. I would say we should always recommend using timestamptz because the functionalities it provides and its design is not broken.

@rmloveland
Copy link
Contributor

@windchan7, I just pushed some updates based on your feedback (summary of changes in commit message). Let me know what you think!

@cockroach-teamcity
Copy link
Member

http://cockroach-docs-review.s3-website-us-east-1.amazonaws.com/c69ba364c00701fc198dd6251c31c6e8e0a8c3ec/

@rmloveland
Copy link
Contributor

Hey @windchan7, would appreciate a review of the latest updates whenever you have cycles. Thank you!

@windchan7
Copy link

:lgtm:

Sorry I missed your message earlier.

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

Comments from Reviewable

@rmloveland rmloveland requested a review from lnhsingh May 15, 2018 19:05
@lnhsingh
Copy link
Contributor

:lgtm:

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

Comments from Reviewable

jseldess
jseldess commented May 16, 2018
View reviewed changes
Copy link
Contributor Author

@jseldess jseldess left a comment

Choose a reason for hiding this comment

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

LGTM, with 2 last nits.

v2.1/timestamp.md Outdated
@@ -53,7 +53,7 @@ ISO 8601 | `TIMESTAMP '2016-01-25T10:10:10.555555'`
To express a `TIMESTAMPTZ` value (with time zone offset from UTC), use
the following format: `TIMESTAMPTZ '2016-01-25 10:10:10.555555-05:00'`

When it is unambiguous, a simple unannotated string literal can also
When it is unambiguous, a simple unannotated [string literals](sql-constants.html#string-literals) can also
Copy link
Contributor Author

Choose a reason for hiding this comment

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

nit: string literals > string literal

Copy link
Contributor

Choose a reason for hiding this comment

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

Fixed - thanks for the catch!

v2.1/time.md Outdated
- Comparing `TIMETZ` instances using [operators such as `>`](functions-and-operators.html#operators)
- [Ordering query results](query-order.html) which include `TIMETZ` values in time order, e.g., `SELECT * FROM table ORDER BY column_with_timetz_type`.

[Like Postgres](https://www.postgresql.org/docs/current/static/datatype-datetime.html), we implement the `TIMETZ` variant for [SQL standards compliance](sql-feature-support.html), and also because it is used by ORMs like [Hibernate](build-a-java-app-with-cockroachdb-hibernate.html).
Copy link
Contributor Author

Choose a reason for hiding this comment

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

nit: extra space before ORMs.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fixed.

@jseldess @rmloveland
Document the TIMETZ type
c172539 
Fixes #2982

Summary of changes:

- Make clear that TIMETZ is not really recommended, just there for SQL
  standard and ORM support (2.1)

- Give examples of wonkiness: comparisons and ordering (2.1)

- Update SQL Feature page with TIME data type (2.0 and 2.1)

- Update example to use TIME, not TIMETZ

- Add links to other related docs as appropriate: Postgres time stuff
  and wire protocol, Cockroach SQL feature page and functions/operators

- Fixed a few small typos/copy-edits

- Also minor tweaks to the TIMEZONE type docs
@cockroach-teamcity
Copy link
Member

http://cockroach-docs-review.s3-website-us-east-1.amazonaws.com/c172539bb7eeb92d390b0495feffb26f982bfbec/

@rmloveland rmloveland merged commit 42b9042 into master May 16, 2018
@rmloveland rmloveland deleted the timetz-type branch May 16, 2018 18:59
@knz knz mentioned this pull request Jun 5, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rmloveland rmloveland rmloveland left review comments

@windchan7 windchan7 Awaiting requested review from windchan7

@dt dt Awaiting requested review from dt

@lnhsingh lnhsingh Awaiting requested review from lnhsingh

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@jseldess @cockroach-teamcity @windchan7 @rmloveland @lnhsingh