Skip to content

Conversation

ericharmeling
Copy link
Contributor

Fixes #5964.

  • Added TIMETZ to v20.1 docs (time.md and data-types.md).
  • Updated example output for TIME data type.

@otan for technical review.

@ericharmeling ericharmeling requested a review from otan January 29, 2020 16:31
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Contributor

@otan otan left a comment

Choose a reason for hiding this comment

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

Looking good - few minor comments.

I think it's also worth calling out that since TIMETZ stores timezone (and TIME does not), TIMETZ is not affected by session scoped timestamps (unlike TIMESTAMPTZ), e.g.:

root@127.0.0.1:57800/defaultdb> create table example (a timestamptz, b timetz);
CREATE TABLE

Time: 3.046ms

root@127.0.0.1:57800/defaultdb> set time zone -3;
SET

Time: 403µs

root@127.0.0.1:57800/defaultdb> insert into example values ('2000-01-01 01:00+01:00', '01:00+01:00');
INSERT 1

Time: 3.157ms

root@127.0.0.1:57800/defaultdb> select * from example;
              a             |             b
----------------------------+----------------------------
  1999-12-31 21:00:00-03:00 | 0000-01-01 01:00:00+01:00
(1 row)

Time: 584µs

root@127.0.0.1:57800/defaultdb> set time zone +4;
SET

Time: 457µs

root@127.0.0.1:57800/defaultdb> select * from example;
              a             |             b
----------------------------+----------------------------
  2000-01-01 04:00:00+04:00 | 0000-01-01 01:00:00+01:00
(1 row)

Time: 537µs

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @ericharmeling and @otan)


v20.1/time.md, line 21 at r1 (raw file):

negative timezones being ranked higher

maybe "timezones with lesser values are ranked higher"? i'm not sure about the best way to phrase it :)


v20.1/time.md, line 44 at r1 (raw file):

The string format for `TIME` is `HH:MM:SS.SSSSSS`. For example: `TIME '05:40:00.000001'`. The fractional portion is optional and is rounded to microseconds (i.e., six digits after the decimal) for compatibility with the [PostgreSQL wire protocol](https://www.postgresql.org/docs/current/static/protocol.html).

To express a `TIMETZ` value (with a time zone offset from UTC), simply add an offset to a `TIME` value. For example, `TIMETZ '10:10:10.555555-05:00'` offsets from UTC by -5.

the offset is optional -- the phrasing here makes it sound like it's required. maybe

"TIMETZ values can optionally include an offset to a TIME value".


v20.1/time.md, line 49 at r1 (raw file):

{{site.data.alerts.callout_info}}
A date of `0000-01-01` is displayed for all `TIME`/`TIMETZ` values, but is not stored in the database.

maybe add a note also that +00:00 is always added for TIME and TIMESTAMP, but are not stored in the DB either.

v20.1/time.md Outdated

Note that the fractional portion of `TIME` is optional and is rounded to microseconds (i.e., six digits after the decimal) for compatibility with the [PostgreSQL wire protocol](https://www.postgresql.org/docs/current/static/protocol.html).
{{site.data.alerts.callout_info}}
A date of `0000-01-01` is displayed for all `TIME`/`TIMETZ` values, but is not stored in the database.
Copy link
Contributor

@otan otan Jan 30, 2020

Choose a reason for hiding this comment

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

I would add for display, casting it to string will print properly, e.g. SELECT '11:00'::time::string.
Also, '24:00'::time(tz) displays as 0000-01-01 00:00:00 but is actually 24:00:00 underneath and only a ::string will catch that.

@cockroach-teamcity
Copy link
Member

Copy link
Contributor Author

@ericharmeling ericharmeling left a comment

Choose a reason for hiding this comment

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

TFTR, @otan! I'm not sure how much detail we want to go into about TIMETZ, since we actually want to discourage users from using TIMETZ. But it's better to have more detail and then cut later, as we see fit. I added an explanation for the session-scoped scenario and not a full example.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @otan)


v20.1/time.md, line 21 at r1 (raw file):

Previously, otan (Oliver Tan) wrote…

negative timezones being ranked higher

maybe "timezones with lesser values are ranked higher"? i'm not sure about the best way to phrase it :)

Yeah that original wording was weird. Updated to read:

"Time zones with lesser values are ranked higher..."


v20.1/time.md, line 44 at r1 (raw file):

Previously, otan (Oliver Tan) wrote…

the offset is optional -- the phrasing here makes it sound like it's required. maybe

"TIMETZ values can optionally include an offset to a TIME value".

Done.


v20.1/time.md, line 49 at r1 (raw file):

Previously, otan (Oliver Tan) wrote…

maybe add a note also that +00:00 is always added for TIME and TIMESTAMP, but are not stored in the DB either.

Done.


v20.1/time.md, line 49 at r1 (raw file):

Previously, otan (Oliver Tan) wrote…

I would add for display, casting it to string will print properly, e.g. SELECT '11:00'::time::string.
Also, '24:00'::time(tz) displays as 0000-01-01 00:00:00 but is actually 24:00:00 underneath and only a ::string will catch that.

Done.

@cockroach-teamcity
Copy link
Member

v20.1/time.md Outdated

The `TIME` [data type](data-types.html) stores the time of day in UTC.

The `TIMETZ` data type stores a time of day with an optional time zone offset from UTC.
Copy link
Contributor

Choose a reason for hiding this comment

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

The phrasing "with an optional time zone offset" is confusing to me. If I don't use the option, it would work just like TIME right? So when should I use the option and when should I not? Or is it that TIMETZ is the option for TIME?

Copy link
Contributor

Choose a reason for hiding this comment

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

Potentially naive question: Which time zone will TIMETZ use?

Copy link
Contributor

Choose a reason for hiding this comment

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

if you don't use it, it will default to the timezone of the session (i.e. UTC by default, SET TIME ZONE '' if set).

v20.1/time.md Outdated

### `TIMETZ`

To express a `TIMETZ` value (with a time zone offset from UTC), you can optionally add an offset to a `TIME` value. For example, `TIMETZ '10:10:10.555555-05:00'` offsets from UTC by -5.
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah..I see that this answers my previous question. I had assumed that the time zone is picked up automatically. Can we reword the earlier intro para as "specified time zone" or something similar?

{{site.data.alerts.callout_danger}}As a best practice, we recommend not using this setting and avoid setting a session time for your database. We instead recommend converting UTC values to the appropriate time zone on the client side.{{site.data.alerts.end}}

You can control your client's default time zone for the current session with <code>SET TIME ZONE</code>. This will apply a session offset to all [`TIMESTAMP WITH TIME ZONE`](timestamp.html) values.
You can control your client's default time zone for the current session with <code>SET TIME ZONE</code>. This will apply a session offset to all [`TIMESTAMPTZ`\`TIMESTAMP WITH TIME ZONE`](timestamp.html) and [`TIMETZ`\`TIME WITH TIME ZONE`](time.html) values.
Copy link
Contributor

Choose a reason for hiding this comment

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

\ is not being rendered properly. Change \ with "or".
Suggested change: [TIMESTAMPTZ or TIMESTAMP WITH TIME ZONE] and
[TIMETZ or TIME WITH TIME ZONE]

v20.1/time.md Outdated
[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). We also implement the `TIMETZ` variant for compatibility with ORMs, like [Hibernate](build-a-java-app-with-cockroachdb-hibernate.html).

{{site.data.alerts.callout_success}}
We recommend always using `TIME` instead of `TIMETZ`. To use time zone UTC offsets, convert UTC values to the appropriate time zone on the client side.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is confusing to me. Is this telling me to use TIME with calculated offsets instead of TIMETZ? Or is this instructing me on how to use TIMETZwith UTC values?

Copy link
Contributor

Choose a reason for hiding this comment

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

it should indicate the former.

v20.1/time.md Outdated

The `TIME` [data type](data-types.html) stores the time of day in UTC.

The `TIMETZ` data type stores a time of day with an optional time zone offset from UTC.
Copy link
Contributor

Choose a reason for hiding this comment

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

if you don't use it, it will default to the timezone of the session (i.e. UTC by default, SET TIME ZONE '' if set).

v20.1/time.md Outdated

- `TIMETZ`, which converts `TIME` values with an optional time zone offset from UTC.

Ordering for `TIMETZ` is done in terms of [epoch](https://en.wikipedia.org/wiki/Epoch_(computing)). Time zones with lesser values are ranked higher if times are equal. For example, `'2:00 -1' > '2:00 +0'` and `'12:00-1' > '1:00+0'`.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: one set of examples have spaces ('2:00 -1' > '2:00 +0'), one doesn't ('12:00-1' > '1:00+0'`).

v20.1/time.md Outdated
[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). We also implement the `TIMETZ` variant for compatibility with ORMs, like [Hibernate](build-a-java-app-with-cockroachdb-hibernate.html).

{{site.data.alerts.callout_success}}
We recommend always using `TIME` instead of `TIMETZ`. To use time zone UTC offsets, convert UTC values to the appropriate time zone on the client side.
Copy link
Contributor

Choose a reason for hiding this comment

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

it should indicate the former.

(2 rows)
~~~

Note that the first timestamp is UTC-05:00, which is the equivalent of EST.
Copy link
Contributor

Choose a reason for hiding this comment

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

i would mention this is because the context timezone is in UTC. if we moved this to +1, this would display 16:10

Copy link
Contributor Author

@ericharmeling ericharmeling left a comment

Choose a reason for hiding this comment

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

TFTR @Amruta-Ranade !

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @Amruta-Ranade and @otan)


v20.1/set-vars.md, line 169 at r2 (raw file):

Previously, Amruta-Ranade (Amruta Ranade) wrote…

\ is not being rendered properly. Change \ with "or".
Suggested change: [TIMESTAMPTZ or TIMESTAMP WITH TIME ZONE] and
[TIMETZ or TIME WITH TIME ZONE]

This was a typo! I meant to put "/", as we do throughout the TIME and TIMESTAMP docs.


v20.1/time.md, line 9 at r2 (raw file):

Previously, otan (Oliver Tan) wrote…

if you don't use it, it will default to the timezone of the session (i.e. UTC by default, SET TIME ZONE '' if set).

I removed the word "optional", as it's confusing. I reworded the TIMETZ syntax section so that it sounds a little less strict.


v20.1/time.md, line 21 at r2 (raw file):

Previously, otan (Oliver Tan) wrote…

nit: one set of examples have spaces ('2:00 -1' > '2:00 +0'), one doesn't ('12:00-1' > '1:00+0'`).

Good catch. Fixed.


v20.1/time.md, line 26 at r2 (raw file):

Previously, otan (Oliver Tan) wrote…

it should indicate the former.

I removed "To use time zone UTC offsets" to make this clearer.


v20.1/time.md, line 54 at r2 (raw file):

Previously, Amruta-Ranade (Amruta Ranade) wrote…

Ah..I see that this answers my previous question. I had assumed that the time zone is picked up automatically. Can we reword the earlier intro para as "specified time zone" or something similar?

I reworded the "Variants" section definition.


v20.1/timestamp.md, line 111 at r2 (raw file):

Previously, otan (Oliver Tan) wrote…

i would mention this is because the context timezone is in UTC. if we moved this to +1, this would display 16:10

I'm going to just remove this note, as I don't find it more confusing than helpful. Let me know if you have any objections.

Copy link
Contributor

@otan otan left a comment

Choose a reason for hiding this comment

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

technical aspects LGTM

| b | TIMESTAMP WITH TIME ZONE | true | NULL | | {} |
+-------------+--------------------------+-------------+----------------+-----------------------+-------------+
column_name | data_type | is_nullable | column_default | generation_expression | indices | is_hidden
+-------------+-------------+-------------+----------------+-----------------------+-----------+-----------+
Copy link
Contributor

Choose a reason for hiding this comment

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

double checking the 19.2 change was intended?

v20.1/time.md Outdated

If no time zone is specified for a `TIMETZ` value, the `timezone` [session variable](show-vars.html#supported-variables) is used. For example, if you [set the `timezone`](set-vars.html#set-time-zone) for a session using `SET TIME ZONE -2`, and you define the `TIMETZ` as `TIMETZ '10:10:10.55'`, the value will be displayed with an offset of -2 from UTC.

`TIMETZ` is not affected by session-scoped offsets (unlike [`TIMESTAMPTZ`](timestamp.html)). Time zone offsets only apply to values inserted after the offset has been set, and do not affect existing `TIMETZ` values.
Copy link
Contributor

Choose a reason for hiding this comment

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

minor nit: apply to values inserted after the offset has been set can be a bit misleading, e.g. SET TIME ZONE +3 then SELECT '12:00-03:00'::timetz will use -3 as a priority over +3.

Copy link
Contributor Author

@ericharmeling ericharmeling left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @Amruta-Ranade and @otan)


v19.2/timestamp.md, line 87 at r3 (raw file):

Previously, otan (Oliver Tan) wrote…

double checking the 19.2 change was intended?

It was - just updated the output (the output before I made this change was pre-19.2 output)


v20.1/time.md, line 58 at r3 (raw file):

Previously, otan (Oliver Tan) wrote…

minor nit: apply to values inserted after the offset has been set can be a bit misleading, e.g. SET TIME ZONE +3 then SELECT '12:00-03:00'::timetz will use -3 as a priority over +3.

Added ", or TIMETZ values with a time zone offset specified" for clarit

Copy link
Contributor Author

@ericharmeling ericharmeling left a comment

Choose a reason for hiding this comment

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

Thanks @otan!

Ready to merge with your approval, @Amruta-Ranade.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @Amruta-Ranade and @otan)

@ericharmeling ericharmeling merged commit c478621 into master Feb 6, 2020
@ericharmeling ericharmeling deleted the timetz branch February 6, 2020 15:10
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.

[NH] Support timetz datatype
4 participants