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

Provide AT ZONE-equivalent functions where zone name is not constant #135

Closed
findepi opened this issue Feb 1, 2019 · 23 comments
Closed
Assignees
Labels
enhancement New feature or request good first issue Good for newcomers
Milestone

Comments

@findepi
Copy link
Member

findepi commented Feb 1, 2019

make at_timezone it public (see @dain 's prestodb/presto#5162 (comment)) and add documentation

This is required to perform AT TIME ZONE conversion when zone is not a constant

See #135 (comment) comment below for what should be implemented in this issue.

Ref prestodb/presto#5162

@findepi findepi added enhancement New feature or request good first issue Good for newcomers labels Feb 1, 2019
@martint
Copy link
Member

martint commented Feb 2, 2019

We should pick a proper name for these functions. I don’t think it be just named at_timezone. What these functions do is convert an unzoned time or timestamp to a zoned one. We have other places in the function library where we do conversions, so we should try to stay consistent.

@findepi
Copy link
Member Author

findepi commented Feb 2, 2019

What these functions do is convert an unzoned time or timestamp to a zoned one.

@martint we also need a function to convert timestamp with time zone to a different zone, just like twtz AT TIME ZONE ... does.
This conversion can be done

  • keeping instant the same (as ... AT TIME ZONE ... does; i.e. as in java.time.ZonedDateTime#withZoneSameInstant),
  • or keeping local time the same (as (CAST twtz AS timestamp) AT TIME ZONE ... does; i.e. as in java.time.ZonedDateTime#withZoneSameLocal).

Shall we have 2 functions then? The second one is clearly only a shorthand, but might be a useful one.

@findepi
Copy link
Member Author

findepi commented Feb 2, 2019

Also, at_timezone is not public but is accessible and I know some people are using it.
When we introduce new functions, we should keep at_timezone name around for a while (behind a compatibility switch, might even be initially disabled by default).

@dain
Copy link
Member

dain commented Feb 2, 2019

There are a bunch of references to this function in stackoverflow, so at the very least we would need to deprecate the old name.

@ebyhr
Copy link
Member

ebyhr commented Feb 4, 2019

JFYI, other database's function/syntax are:
convert_tz(dt, from_tz, to_tz): mysql
at time zone 'timezone_name': postgres, sql server, oracle
from_utc_timestamp(dt, 'timezone_name'): hive
convert_timezone(['source_timezone',] 'target_timezone', 'timestamp'): redshift
datetime(timestamp_expression, timezone): bigquery
from_tz( timestamp_value, time_zone_value ): oracle

@findepi
Copy link
Member Author

findepi commented Feb 4, 2019

@ebyhr thanks for listing these. Do you know which of these databases have standards-compliant timestamp with time zone data type?

@ebyhr
Copy link
Member

ebyhr commented Feb 4, 2019

As far as I searched, postgres, oracle, redshift and bigquery have it. SQL server holds as datetimeoffset type. MySQL and hive don't have it.

@findepi told me on slack, postgres doesn't have it, also redshift and bigquery may be same. Thank you for the information.

@dain
Copy link
Member

dain commented Feb 4, 2019

@martint do you have thoughts on the above function names from other systems?

@martint
Copy link
Member

martint commented Feb 7, 2019

To recap, what we (possibly?) need are these behaviors:

  1. Make a TIMESTAMP WITH TIME ZONE from a TIMESTAMP and a timezone.
    function(ts::TIMESTAMP, timezone::VARCHAR) :: TIMESTAMP WITH TIME ZONE

  2. Change the timezone component of a TIMESTAMP WITH TIME ZONE while preserving the instant in time.
    function(ts:TIMESTAMP WITH TIME ZONE, target_timezone::VARCHAR) :: TIMESTAMP WITH TIME ZONE

  3. Change the timezone offset associated with a TIMESTAMP WITH TIMEZONE while preserving the local timestamp.
    function(ts:TIMESTAMP WITH TIME ZONE, target_timezone::VARCHAR) :: TIMESTAMP WITH TIME ZONE

1 and 2 seem straightforward to explain and understand given the semantics of those two types. While 3 sounds convenient, I'm concerned about being able to come up with a name and explanation of the behavior that's easy and intuitive. I'd prefer not to add it unless there's demand for it.

For 1, some possible names: to_timestamp_with_timezone, with_timezone, to_zoned, at_timezone. Oracle, confusingly, calls this function from_tz (https://docs.oracle.com/cd/B19306_01/server.102/b14200/functions059.htm).

For 2, it might make sense to keep the name as at_timezone. Oracle doesn't have an equivalent to this other than via the AT TIME ZONE syntax.

@findepi
Copy link
Member Author

findepi commented Feb 8, 2019

@martint what about at_timezone_keep_local or at_timezone_same_local for 3.?

@dain dain removed the good first issue Good for newcomers label Feb 20, 2019
@martint
Copy link
Member

martint commented Feb 21, 2019

what about at_timezone_keep_local or at_timezone_same_local for 3.?

I'd say, let's leave that one out. It seems an uncommon use case with a reasonable workaround. If there's demand later, we can consider adding such a function.

@findepi
Copy link
Member Author

findepi commented Feb 21, 2019

@martint what about using at_timezone for 1 and 2?

@martint
Copy link
Member

martint commented Feb 21, 2019

at_timezone makes a lot of sense for 2. I'm concerned about using the same name for 1 given that it would introduce an overload with variants that have completely different semantics. Specifically, what happens if there's a type with an implicit cast to both TIMESTAMP and TIMESTAMP WITH TIME ZONE? For instance, what would at_timezone(DATE '2019-01-01', 'PST') produce if the session timezone is anything other than PST?

While it's tempting to say "having both functions named the same is consistent with the AT TIMEZONE syntax", it's important to note that the spec disallows AT TIMEZONE if the declared type of the argument is of type DATE. Built-in language constructs can afford to have special rules about coercibility that general functions can't.

If the declared type of the <datetime primary> is DATE, then shall not be specified.

As a side note, we have a function named from_unixtime(unix_epoch, timezone). So maybe we just name it from_timestamp(timestamp, timezone) to be consistent?

@findepi
Copy link
Member Author

findepi commented Feb 21, 2019

i am fine with from_timestamp(timestamp, timezone). Note however the signature will be from_timestamp(timestamp, varchar) -- can you think of any other meaning or conversion we could want to name "from_timestamp" that would take varchar as the second parameter? (if you can think of any, with_timezone might be a safer choice)

@martint
Copy link
Member

martint commented Feb 22, 2019

Yeah, from_timestamp is kind of generic. It doesn't convey what it's doing. A name that indicates to_XXX or with_XXX is more obvious. Ok, so with_timezone, then?

@findepi
Copy link
Member Author

findepi commented Feb 22, 2019

Summing up:

what we (possibly?) need are these behaviors:

  1. Make a TIMESTAMP WITH TIME ZONE from a TIMESTAMP and a timezone.
    function(ts::TIMESTAMP, timezone::VARCHAR) :: TIMESTAMP WITH TIME ZONE
  2. Change the timezone component of a TIMESTAMP WITH TIME ZONE while preserving the instant in time.
    function(ts:TIMESTAMP WITH TIME ZONE, target_timezone::VARCHAR) :: TIMESTAMP WITH TIME ZONE
  3. Change the timezone offset associated with a TIMESTAMP WITH TIMEZONE while preserving the local timestamp.
    function(ts:TIMESTAMP WITH TIME ZONE, target_timezone::VARCHAR) :: TIMESTAMP WITH TIME ZONE
  • with_timezone for (1)
  • at_timezone for (2)
  • currently no function for (3)

@findepi
Copy link
Member Author

findepi commented Feb 25, 2019

@martint thanks. Looks like we reached conclusion.

@ebyhr you already started doing something around this in #143.
However, this now became a bit bigger change to be done.
Are you interested in working on this?

@findepi findepi changed the title Make at_timezone function public Provide AT ZONE-equivalent functions where zone name is not constant Feb 25, 2019
@ebyhr
Copy link
Member

ebyhr commented Mar 2, 2019

@findepi Sorry for my late reply. Probably, I don't have enough time to work on that, therefore hope someone develop it.

@martint martint added the good first issue Good for newcomers label May 21, 2019
@RosterIn
Copy link

RosterIn commented Jul 2, 2019

It's really weird that presto doesn't have ANY way to convert UTC timestamp to another timezone but without having the timezone string.
https://stackoverflow.com/questions/48633900/presto-cast-timestamp-w-tz-to-plain-timestamp-without-converting-to-utc

@findepi
Copy link
Member Author

findepi commented Jul 2, 2019

@RosterIn I am not sure I fully understand your needs. I invite you to Presto community Slack (https://prestosql.io/community.html), I will be happy if I can help in your particular use-case.

@RosterIn
Copy link

RosterIn commented Jul 12, 2019

@findepi
This is what I want to do: timestamp with timezone -> timestamp

The problem:
the act of casting timestamp with timezone to a timestamp is resulted in the timestamp being converted back to UTC time. so there is no direct way to do what I want.
Now, say that I take this UTC and want to cast it to my timezone.
I can't do that. It will always add me the 'Australia/Melbourne' so I get back timestamp with timezone . It's a deadlock!

@findepi
Copy link
Member Author

findepi commented Jul 15, 2019

@RosterIn If I understand this correctly, you want timestamp to represent local date/time with no implicit relation to any time zone.
This is covered by #37. Today, the engine supports this, but some connectors do not, so it cannot be enabled by default.
Depending on which connectors you use, you can experiment with what standard compliant timestamps by setting deprecated.legacy-timestamp=false (admittedly, the property name is far from ideal...).
Let's continue this discussion on Slack (https://prestosql.io/community.html). There is a dedicated #timestamps channel.

@martint
Copy link
Member

martint commented Oct 5, 2019

Fixed by #1612

@martint martint closed this as completed Oct 5, 2019
rice668 pushed a commit to rice668/trino that referenced this issue Jan 31, 2023
rice668 pushed a commit to rice668/trino that referenced this issue Jul 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Development

No branches or pull requests

7 participants