Skip to content

Update documentation for parametric TIME WITH TIME ZONE #5316#7912

Merged
electrum merged 5 commits intotrinodb:masterfrom
meggpie:issue-5316
Jun 4, 2021
Merged

Update documentation for parametric TIME WITH TIME ZONE #5316#7912
electrum merged 5 commits intotrinodb:masterfrom
meggpie:issue-5316

Conversation

@meggpie
Copy link
Copy Markdown
Contributor

@meggpie meggpie commented May 13, 2021

Updated description and example timezones to numeric format.
Screen Shot 2021-05-13 at 1 33 15 PM

@cla-bot
Copy link
Copy Markdown

cla-bot bot commented May 13, 2021

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to cla@trino.io. For more information, see https://github.com/trinodb/cla.

@meggpie
Copy link
Copy Markdown
Contributor Author

meggpie commented May 13, 2021

@mosabua @Ordinant @rosewms @m57lyra PR for review. I emailed my CLA last night.

@mosabua
Copy link
Copy Markdown
Member

mosabua commented May 14, 2021

FIxes #5316

Copy link
Copy Markdown
Member

@mosabua mosabua left a comment

Choose a reason for hiding this comment

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

Looks great.

@mosabua mosabua requested review from Ordinant, m57lyra and rosewms May 14, 2021 05:25
@mosabua mosabua added the docs label May 14, 2021
@mosabua
Copy link
Copy Markdown
Member

mosabua commented May 14, 2021

CLA has been submitted via email @martint

Time zones are expressed as the numeric UTC offset value.

Example: ``TIME '01:02:03.456 America/Los_Angeles'``
Example: ``TIME '01:02:03.456 -08.00'``
Copy link
Copy Markdown
Member

@hashhar hashhar May 14, 2021

Choose a reason for hiding this comment

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

This doesn't seem to work. This works though TIME '01:02:03.456 -08:00'

Same in the other example.

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.

Good catch ... note the : instead of . @meggpie

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.

Got it! Will make the updates.

Copy link
Copy Markdown
Member

@mosabua mosabua left a comment

Choose a reason for hiding this comment

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

Great find from @hashhar needs fixing ;-)

Time zones are expressed as the numeric UTC offset value.

Example: ``TIME '01:02:03.456 America/Los_Angeles'``
Example: ``TIME '01:02:03.456 -08.00'``
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.

Good catch ... note the : instead of . @meggpie


Time of day (hour, minute, second, millisecond) with a time zone.
Values of this type are rendered using the time zone from the value.
Time zones are expressed as the numeric UTC offset value.
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.

Lets change this to our default example setup that has the sql query and the output as comment so you can actually cut and paste to try

Time zones are expressed as the numeric UTC offset value::

    SELECT TIME '01:02:03.456 -08.00';
    == 1:02:03.456-08:00

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.

@mosabua Is the query meant to have the : instead of the . ? Or just the output?

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.

Both the query and the output.

type are rendered using the time zone from the value.

Example: ``TIMESTAMP '2001-08-22 03:04:05.321 America/Los_Angeles'``
Example: ``TIMESTAMP '2001-08-22 03:04:05.321 -08.00'``
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.

Same as above

Updates as per requested changes
@cla-bot cla-bot bot added the cla-signed label May 17, 2021
@hashhar hashhar requested a review from mosabua May 17, 2021 16:12
@mosabua
Copy link
Copy Markdown
Member

mosabua commented May 19, 2021

Ideally squash the commits into one commit, but this is good essentially.


Example: ``TIME '01:02:03.456 America/Los_Angeles'``
SELECT TIME '01:02:03.456 -08:00';
== 1:02:03.456-08:00
Copy link
Copy Markdown
Member

@mosabua mosabua May 20, 2021

Choose a reason for hiding this comment

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

I am so sorry @meggpie .. I messed up. it should be -- not ==

can you please fix

@mosabua
Copy link
Copy Markdown
Member

mosabua commented May 28, 2021

@meggpie did you want to fix this up as requested?

@meggpie
Copy link
Copy Markdown
Contributor Author

meggpie commented May 28, 2021

@mosabua yes, it's on my to-do list for today!

@meggpie
Copy link
Copy Markdown
Contributor Author

meggpie commented May 28, 2021

@mosabua I made the changes but I'm stull trying to figure out how to squash the commits, sorry

@mosabua
Copy link
Copy Markdown
Member

mosabua commented Jun 2, 2021

you need to do a git rebase of the last three commits and then a force push

Something like this

https://stackoverflow.com/questions/5189560/squash-my-last-x-commits-together-using-git

ping me on trino slack if you need live help

@electrum electrum merged commit 00080c8 into trinodb:master Jun 4, 2021
@electrum
Copy link
Copy Markdown
Member

electrum commented Jun 4, 2021

Thanks!

Instant in time that includes the date and time of day with ``P`` digits of
precision for the fraction of seconds and with a time zone. Values of this
type are rendered using the time zone from the value.
Time zones are expressed as the numeric UTC offset value::
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.

This turned out to be understood as indicating that Trino supports numeric offset zones, but not named zones.
see trinodb/trino-python-client#229 (comment) cc @jhlodin

we need to improve the wording to clarify named zones are supported

cc @mosabua

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.

Probably we need to mention this separately both TIME and TIMESTAMPTZ since TIME doesn't support named zones (since they don't make sense there) while TIMESTAMPTZ does.

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.

Whatever supports timezone should be consistent .. is that the case now.. I am not sure to be honest.

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.

It cannot be consistent since TIME has no concept of named zones. What does TIME '01:01:01' America/Los_Angeles mean? Depending on what day it was America/Los_Angeles can mean different things.

We need to explicitly make it more clear that TIME with timezone supports only offsets and not named zones while TIMESTAMP with timezone supports both named offsets and numbered offsets.

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.

What does TIME '01:01:01' America/Los_Angeles mean?

This is a wrong question to ask. We don't need to go into details why.

TIME has no concept of named zones.

yes, it does not.

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.

time(p) with time zone is in the SQL standard. It's defined as a time with a time zone displacement containing an hour and a minute component in the range -14:00 to +14:00. That's the only concept of timezones supported by the specification, even for the timestamp(p) with time zone type. Support for political time zones is an extension to the standard.

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.

So .. to simplify for users and consistency .. should we remove the support for political time zones everywhere?

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.

No, political timezones are useful and necessary for timestamp(p) with time zone.

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.

Okay ... I think we have enough info now to run with a doc update PR .. @jhlodin will submit soon.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Proposed type docs change here: #13927

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

9 participants