trinodb / trino-python-client

Python client for Trino
Apache License 2.0
307 stars 151 forks source link

Address time zone localization issue #368

Closed john-bodley closed 1 year ago

john-bodley commented 1 year ago

Description

This PR addresses https://github.com/trinodb/trino-python-client/issues/366.

Per this article, there seemed to be merit in removing pytz in favor of dateutil.tz given that in Python 3.6 the datetime module remedied the ambiguous datetimes due to daylight saving time transition.

Regrettably though one cannot obtain the IANA name from a dateutil.tz time zone—which is required when transpiling datetime.time/datetime.datetime objects to Trino SQL when provided as operation parameters. Instead we use the zoneinfo package (added to Python’s standard library in Python 3.9 and back ported) which does provide a mechanism of obtaining the specified IANA name from a datetime.datetime/datetime.time object, i.e.,

>>> from datetime import time
>>> from zoneinfo import ZoneInfo
>>>
>>> midnight = time(0, tzinfo=ZoneInfo("America/Los_Angeles"))
>>> midnight.tzinfo.key 
'America/Los_Angeles'

The fix is rather simple as simply switching out the pytz package for the zoneinfo package remedies the issue:

Before

>>> from datetime import datetime
>>> import pytz
>>>
>>> datetime(2023, 1, 1, tzinfo=pytz.timezone("America/Los_Angeles")).isoformat()
'2023-01-01T00:00:00-07:53' 

After

>>> from datetime import datetime
>>> import zoneinfo
>>>
>>> datetime(2023, 1, 1, tzinfo=zoneinfo.ZoneInfo("America/Los_Angeles")).isoformat()
'2023-01-01T00:00:00-08:00'

Non-technical explanation

Release notes

( ) This is not user-visible or docs only and no release notes are required. ( ) Release notes are required, please propose a release note for me. (x) Release notes are required, with the following suggested text:

* Fixes time zone localization. The `pytz` package for encoding/decoding time zones has been deprecated in favor of `zoneinfo`. Time zones should now be encoded (when specifying execution parameters) using the `zoneinfo.ZoneInfo` method as opposed to `pytz.timezone` method. ({[issue](https://github.com/trinodb/trino-python-client/issues/366.)}`366`)
john-bodley commented 1 year ago

@mdesmet I've addressed your comments.

john-bodley commented 1 year ago

@aalbu or @hashhar would either of you mind merging this given than @mdesmet has approved the change?

Additionally what would be a rough estimate (days, weeks, or months) before this change would be released? The reason I'm asking is we likely need to give our Superset users—which leverages the Trino DB-API—a heads up of the issue and would ideally like to provide them with an rough ETA as to when it will be fixed.

hashhar commented 1 year ago

I'll take a look tomorrow. I'm leaving some quick editorial comments for now.

Since this is a correctness issue I'd like to do a release as soon as we have this merged.

john-bodley commented 1 year ago

@hashhar I've addressed your comments.