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

Remove support for political time zones from TIME WITH TIME ZONE #10103

Closed
findepi opened this issue Mar 6, 2018 · 6 comments
Closed

Remove support for political time zones from TIME WITH TIME ZONE #10103

findepi opened this issue Mar 6, 2018 · 6 comments

Comments

@findepi
Copy link
Contributor

findepi commented Mar 6, 2018

TIME WITH TIME ZONE can express such (probably) useful things like 02:23:00+02:00 (= pair of 23 minutes past 2am, offset +2h).
However, it can also represent such things like 02:23:00 Europe/Warsaw, which sounds like a cool feature at first, but its semantics are problematic -- you can't reliably convert this to TIME in any zone (like UTC), since a named zone doesn't need to have fixed zone offset (due to DST and policy changes over time). (Probably for these reasons, Java Time API has LocalTime and OffsetTime but nothing like ZonedTime exists.)

As discussed offline some time ago, we should:

  • limit TIME WITH TIME ZONE to offset zones only.

Some details:

  • TIME '.... Continent/City' and TIME '.... PST' literals should fail
  • TIME / TIME TZ at time zone .... should fail for non-offset tz
  • cast(timestamp tz as time tz) should take current (as of this timestamp’s instant) tz offset and produce time with tz offset
  • cast(time as time tz) should take current session time zone’s current offset and produce time with tz offset

Relates to the on-going #7122 (doing this one before #7122 will cause conflicts but may simplify our lives significantly)

cc @losipiuk @martint @dain @electrum @haozhun

@findepi
Copy link
Contributor Author

findepi commented Jul 15, 2018

@martint some questions:

  1. Should time without political zones be guarded by legacy_timestamp? (This can simplify implementation, but complicate testing and we will have the political zones for longer, eg. in JDBC)
  2. What zone should cast(TIME as TIMESTAMP WITH TIME ZONE) add (besides adding 1970-01-01 date)? Current session zone or current session zone's offset?
  3. What should cast(TIMESTAMP as TIME WITH TIME ZONE) do when session did not observe given local timestamp? (I assume that, generally, this cast takes offset of current session zone at given local timestamp)

@findepi
Copy link
Contributor Author

findepi commented Jul 26, 2018

  1. Should time without political zones be guarded by legacy_timestamp?

i think the answer lies in https://prestodb.io/docs/current/release/release-0.206.html - we foretold the changes will be for non-legacy timestamp

@haozhun
Copy link
Contributor

haozhun commented Jul 26, 2018

(EDITED on 7/26: changed my answers materially)

Should time without political zones be guarded by legacy_timestamp? (This can simplify implementation, but complicate testing and we will have the political zones for longer, eg. in JDBC)

I don't feel strongly. But I prefer only applying it to new timestamp. I have always just assumed this, and I haven't thought about the complexity this carries. Therefore, I'm willing to change my opinion if you have a preference.

What zone should cast(TIME as TIMESTAMP WITH TIME ZONE) add (besides adding 1970-01-01 date)? Current session zone or current session zone's offset?

Prepend today's date (not 1970-01-01), and then convert to current session zone (not its offset). Let the conversion logic handle gaps and overlaps.

What should cast(TIMESTAMP as TIME WITH TIME ZONE) do when session did not observe given local timestamp? (I assume that, generally, this cast takes offset of current session zone at given local timestamp)

I assume that you are thinking about one of the following two alternatives.

Path 1. TIMESTAMP -> TIMESTAMP WITH TIME ZONE -> TIME WITH TIME ZONE
Path 2. TIMESTAMP -> TIME -> TIME WITH TIME ZONE (question: should TIME to TIME WITH TIME ZONE fail whenever local zone is a political zone?)

Both path has down sides. I prefer path 1.

cc @dain

@dain
Copy link
Contributor

dain commented Jul 26, 2018

for the last two items TIME -> TIMESTAMP w/ TZ and TIMESTAMP -> TIME w/ TZ, what does the spec say? Specifically for the first one, what date does it pick?

@haozhun
Copy link
Contributor

haozhun commented Jul 26, 2018

What the spec says about type conversion. Note that since the spec is not aware of political zone, we should only take its spirit, not its letters.

screen shot 2018-07-26 at 4 17 32 pm

@findepi
Copy link
Contributor Author

findepi commented Jul 27, 2018

Should time without political zones be guarded by legacy_timestamp? ...
... But I prefer only applying it to new timestamp. I have always just assumed this, and I haven't thought about the complexity this carries.

I am unsure what the complexity will ultimately be.

What comes to my mind is what will ResultSet.getObject(col) return? I would like it to return OffsetTime. So if we guard the change with legacy_timestamp the JDBC client will have to know the value of the switch and instantiate OffsetTime or java.sql.Time (legacy case).

let's try to make it guarded with the legacy_timestamp switch and revisit this when problems impractical to solve arise.

What zone should cast(TIME as TIMESTAMP WITH TIME ZONE) add (besides adding 1970-01-01 date)? Current session zone or current session zone's offset?

Prepend today's date (not 1970-01-01), and then convert to current session zone (not its offset). Let the conversion logic handle gaps and overlaps.

Indeed, this is what the spec suggests. Now, we fill date fields with 1970-01-01 (in all casts from TIME* to TIMESTAMP*). I will add this to the roadmap issue.

What should cast(TIMESTAMP as TIME WITH TIME ZONE) do when session did not observe given local timestamp? (I assume that, generally, this cast takes offset of current session zone at given local timestamp)

Path 1. TIMESTAMP -> TIMESTAMP WITH TIME ZONE -> TIME WITH TIME ZONE
Path 2. TIMESTAMP -> TIME -> TIME WITH TIME ZONE (question: should TIME to TIME WITH TIME ZONE fail whenever local zone is a political zone?)

Both path has down sides. I prefer path 1.

Path 1 is also what spec seems to be saying.

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

No branches or pull requests

3 participants