Limitations in timezone handling in Velox

[svm1]

Hello team. I was working on implementing a scalar function, to_iso8601(), when the state of velox's handling of timezones came to my attention.

One of the signatures of this function takes in a TimestampWithTimezone, and should return an ISO8601-formatted timestamp string, including an appropriate UTC offset based on the input timezone. Velox is currently taking care of timezones and offsets with TimeZoneDatabase.cpp - a hardcoded set of English timezone identifiers as well as UTC offsets. I was able to use this existing implementation to dump UTC offsets in ISO-formatted strings; however, comparing to Presto's Java implementation revealed many gaps.

The Java implementation was built using the joda time library - this is a powerful library that catalogued detailed timezone info, including DST offsets, DST periods, and historical variations in timezones - all of which vary for most every timezone. With the existing Velox implementation, the only knowledge present is the "standard" UTC offset - even accounting for DST is not possible. At the same time, recreating this complex functionality does not seem very efficient, so I believe it may be worth bringing a similar library into Velox, to handle all timezone/offset operations.

There doesn't seem to be any actively maintained port of Java's joda time - though I have found a fairly capable open-source library from google, cctz: https://github.com/google/cctz. I've set it up locally as a bundled dependency in Velox and was able to verify that it is able to account for DST, one of the key gaps identified.

Example output:

• PST timestamp with standard UTC offset (-08:00)

presto:tpch> select ( to_iso8601(cast (shipdate as timestamp)) ) from lineitem where orderkey=4387 and partkey=1769;
             _col0             
-------------------------------
 2001-12-22T03:04:05.000-08:00 
                               
(1 row)

• PST timestamp with DST offset, as the date falls in August (-07:00)

presto:tpch> select ( to_iso8601(cast (shipdate as timestamp)) ) from lineitem where orderkey=4387 and partkey=1769;
             _col0             
-------------------------------
 2001-08-22T03:04:05.000-07:00 
                               
(1 row)

I've spoken to @aditi-pandit about this, who advised me to initiate a discussion here. I'd like to hear what the other team members think about this. I've just included the cctz library as a potentially viable example, to show a working proof-of-concept, but I am willing to discuss other options as well. Please let me know!

[aditi-pandit]

Thanks Sujit.

@pedroerp @Yuhta @laithsakka @mbasmanova : wdyt ? Do you have recommendations for other Meta internal libraries over cctz.

[mbasmanova]

CC: @gggrace14

[mbasmanova]

CC: @assignUser @kgpai

[mbasmanova]

CC: @kevinwilfong @xiaoxmeng

[assignUser]

Arrow uses a vendored and slightly modified version of https://github.com/HowardHinnant/date which seems to be similar to cctz (also based on <chrono>) but I don't have any practical insights why one might be better than the other. From a build perspective both support cmake, though date recommends use of fetchContent so it might be easier to use...

[pedroerp]

This is the same one as we have in velox/external/date

Cc: @svm1 @mbasmanova

[pedroerp]

@svm1 I might be missing something, but AFAIK we only use the internal velox/type/tz/TimeZoneDatabase.cpp for the timezone names. This list was actually imported from the Presto Java codebase, and just there for compatibility with Presto. Note that the offsets in that file are not real offsets, they are just alternate names for these timezones.

For the actual timezone conversions, we rely on std::chrono, which, I presume will end up making a syscall to let the operating system do the correct translation considering daylight savings time and other nuances, given the installed tz database.

Can you also print the value of shipdate in your example? We should also check if the session timezone is being applied correctly.

[svm1]

@pedroerp Apologies for the late response. I was trying to connect Hive server and Hadoop to my local working instance of Prestissimo to demonstrate querying timestamps and invoking toISO8601() through the Presto coordinator, but was unable to get them all setup. Therefore, I am relying on Velox unit tests to validate the function's behavior at the moment.

I understand that TimeZoneDatabase.cpp does not contain real offsets - that is the very issue I am attempting to address. I too may be missing something - but as per my understanding, std::chrono seems insufficient for this purpose. The operating system may be able to handle times and offsets relative to the current system timezone, but there doesn't seem to be any way to account for other time zones and their offset variations, whether due to DST or historic changes.

shipdate in my example is actually just a string - hence I had to cast it as a timestamp just for the sake of demonstrating the example.

Passing test cases demonstrating successful handling of the DST offset (passing date as milliseconds since epoch into toISO8601() to simulate a timestamp):

  EXPECT_EQ(
    "2020-02-05T22:31:07.000-08:00",
    toISO8601(
        (18297 * kSecondsInDay + 22 * 3600 + 31 * 60 + 7) * 1000,
        "America/Los_Angeles"));

  EXPECT_EQ(
    "2020-08-26T22:31:07.000-07:00",
    toISO8601(
        (18500 * kSecondsInDay + 22 * 3600 + 31 * 60 + 7) * 1000,
        "America/Los_Angeles"));

Here is a test case demonstrating handling of historical offsets:

EXPECT_EQ(
    "1878-04-13T01:50:23.000-07:52",
    toISO8601(
        (-33500 * kSecondsInDay + 1 * 3600 + 50 * 60 + 23) * 1000,
        "America/Los_Angeles"));

[svm1]

@pedroerp I should've been more clear - these are test cases I created after successfully implementing timezone conversion, using the cctz library. I just wanted to share these working test examples (which would've failed with no code changes). Ideally, I'd want to demonstrate invoking the function through Presto queries, but as that wasn't possible, I figured this was the next best alternative! :)

The key is that these test cases would otherwise fail - they would not return ISO formatted strings with the correct offsets, because there is no knowledge of offsets to pull from, other than the "fake" constant offsets used for identification in TimeZoneDatabase.cpp.

[pedroerp]

Could you check if these same test cases fail if you use the functions tested in this test suite?

https://github.com/facebookincubator/velox/blob/main/velox/type/tests/TimestampConversionTest.cpp#L266

[mbasmanova]

@pedroerp Pedro, FYI, there is a very nice explanation in https://github.com/google/cctz#fundamental-concepts

[pedroerp]

Thanks Masha. I remember looking into these concepts a while ago. I think my point is that, at least as far as I can understand, these concepts are supported in std::chrono starting in C++20:

https://en.cppreference.com/w/cpp/chrono/time_zone

also:

https://en.cppreference.com/w/cpp/chrono/tzdb

For now, since we can't rely on C++20, we dropped in an implementation compatible with this standard here:

https://github.com/facebookincubator/velox/tree/main/velox/external/date

Whenever we update to require C++20, we can remove this version. Internally, it uses the operating system timezone database, tzdata, which is based on the IANA Timezone Database, the source authority for these things. All in all, my understanding is that these things are already supported and that cctz is a pre-C++20 thing.

If that's not the case, I would like to understand the reason before we go ahead and add a new a dependency.

Cc: @amitkdutta @svm1

[svm1]

Thanks a lot for the thorough explanation and links Pedro. I was not aware that this date library has already been brought into Velox - I was operating under the assumption that no such library had been made available yet, though I should've realized this was already here. Apologies for the confusion.

I will look into this date library and adapt my code changes to invoke it, rather than cctz - now understand what you meant about the redundancy. Thanks again for clarifying my misconception.