Address time zone localization issue #368

john-bodley · 2023-04-30T11:46:52Z

Description

This PR addresses #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 · 2023-04-30T15:20:51Z

tests/integration/test_types_integration.py

-
-
-def query_time_with_timezone(trino_connection, tz_str: str):
+@pytest.mark.parametrize(


Same logic as before but more aligned with how pytest was intended to be used.

Please make preparatory commits for optimisations not related to the change.

john-bodley · 2023-04-30T15:22:46Z

trino/client.py

- from zoneinfo import ZoneInfo # type: ignore
-
-except ModuleNotFoundError:
- from backports.zoneinfo import ZoneInfo # type: ignore


Previously there was no requirement in setup.py for the backports.zoneinfo package.

john-bodley · 2023-05-01T22:05:26Z

tests/integration/test_types_integration.py

@@ -300,114 +306,114 @@ def query_time_with_timezone(trino_connection, tz_str: str):
 # min supported time(3)
 .add_field(
 sql=f"TIME '00:00:00 {tz_str}'",
- python=time(0, 0, 0).replace(tzinfo=tz))
+ python=time(0, 0, 0, tzinfo=tz))


Clearer/cleaner to specify the time zone when creating the object.

are there any other occurrences of this in production code as well?

mdesmet

LGTM

mdesmet · 2023-04-30T17:21:02Z

tests/integration/test_types_integration.py

-
-
-def query_time_with_timezone(trino_connection, tz_str: str):
+@pytest.mark.parametrize(


Please make preparatory commits for optimisations not related to the change.

john-bodley · 2023-05-02T21:24:01Z

@mdesmet I've addressed your comments.

john-bodley · 2023-05-02T21:28:45Z

@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 · 2023-05-03T09:15:15Z

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.

hashhar

Commit "[preparatory] Use @pytest.mark.parameterize".

The project avoids prefixing commit messages, so something just like Parameterize time and timestamp with time zone tests is good enough.

hashhar

commit "[preparatory] Specify tzinfo during datetime/time object creation" -> "Specify tzinfo during datetime/time creation in tests"

hashhar

commit " [fix] Address timezone localization issue ".

Can you add a short explanation in the commit message body about the issue? Alternatively I think it'd also be self-explanatory if you can add a test which showcases the issue and then in next commit add the fix and adjust the test.

hashhar · 2023-05-03T09:20:00Z

setup.py

@@ -78,7 +78,12 @@
 "Topic :: Database :: Front-Ends",
 ],
 python_requires='>=3.7',
- install_requires=["pytz", "requests", "tzlocal"],
+ install_requires=[
+ "backports.zoneinfo;python_version<'3.9'",


missing zoneinfo require should be a separate commit, it's not related to the fix itself.

hashhar · 2023-05-03T09:21:06Z

trino/client.py

@@ -1041,7 +1037,7 @@ def new_instance(self, value: datetime, fraction: Decimal) -> TimestampWithTimeZ
 return TimestampWithTimeZone(value, fraction)

 def normalize(self, value: datetime) -> datetime:
- if isinstance(self._whole_python_temporal_value.tzinfo, BaseTzInfo):
+ if tz.datetime_ambiguous(value):


john-bodley · 2023-05-03T23:55:28Z

@hashhar I've addressed your comments.

Consider the following example using the canonical way to add zones to datetime objects: >>> import pytz >>> import datetime >>> import zoneinfo >>> datetime.datetime(2023, 1, 1, tzinfo=pytz.timezone("America/Los_Angeles")).isoformat() '2023-01-01T00:00:00-07:53' >>> datetime.datetime(2023, 1, 1, tzinfo=zoneinfo.ZoneInfo("America/Los_Angeles")).isoformat() '2023-01-01T00:00:00-08:00' pytz does eager timezone evaluation and uses the local-mean-time since the instant in time is not known. It requires an additional `localize` call to get the correct zone like so: >>> pytz.timezone("America/Los_Angeles").localize(datetime.datetime(2023, 1, 1)).isoformat() '2023-01-01T00:00:00-08:00' This increases chances of introducing bugs when writing idiomatic Python. The only reason to use pytz was because it allowed to control what happens with ambiguous datetimes but the standard library also allows provides control over that since 3.9 (and is available as backports.zoneinfo for older versions).

cla-bot bot added the cla-signed label Apr 30, 2023

john-bodley mentioned this pull request Apr 30, 2023

fix: Address time zone localization issue #367

Closed

john-bodley requested review from mdesmet and aalbu April 30, 2023 11:54

john-bodley commented Apr 30, 2023

View reviewed changes

john-bodley changed the title ~~fix: Address timezone localization issue~~ fix: Address time zone localization issue May 1, 2023

john-bodley requested review from hashhar and hovaesco May 1, 2023 22:00

john-bodley commented May 1, 2023

View reviewed changes

john-bodley force-pushed the john-bodley--fix-issue-366 branch from d120739 to 013853c Compare May 2, 2023 06:53

mdesmet approved these changes May 2, 2023

View reviewed changes

john-bodley force-pushed the john-bodley--fix-issue-366 branch from 013853c to e184409 Compare May 2, 2023 20:43

hashhar reviewed May 3, 2023

View reviewed changes

john-bodley added 3 commits May 3, 2023 16:22

Parameterize time and timestamp with time zone tests

fecd653

Specify tzinfo during datetime/time creation in tests

1ec5032

Add missing backports.zoneinfo requirement

cd30eed

john-bodley force-pushed the john-bodley--fix-issue-366 branch from e184409 to cbd4c6b Compare May 3, 2023 23:45

john-bodley changed the title ~~fix: Address time zone localization issue~~ Address time zone localization issue May 3, 2023

hashhar approved these changes May 5, 2023

View reviewed changes

hashhar force-pushed the john-bodley--fix-issue-366 branch from cbd4c6b to 7ff12c7 Compare May 8, 2023 13:03

hashhar merged commit 2b9ca0c into trinodb:master May 8, 2023

hashhar mentioned this pull request May 15, 2023

Timestamp with time zone not mapped correctly #366

Closed

1 task

hashhar mentioned this pull request May 15, 2023

Release notes for 0.324.0 #370

Closed

okayhooni mentioned this pull request May 24, 2024

BUG) Superset get wrong value of TIMESTAMP WITH TIME ZONE type field on Trino client apache/superset#28685

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Address time zone localization issue #368

Address time zone localization issue #368

john-bodley commented Apr 30, 2023 •

edited

Loading

john-bodley Apr 30, 2023

mdesmet Apr 30, 2023

john-bodley Apr 30, 2023

john-bodley May 1, 2023

hashhar May 3, 2023

mdesmet left a comment

mdesmet Apr 30, 2023

john-bodley commented May 2, 2023

john-bodley commented May 2, 2023

hashhar commented May 3, 2023

hashhar left a comment

hashhar left a comment

hashhar left a comment

hashhar May 3, 2023

hashhar May 3, 2023

john-bodley commented May 3, 2023



		def query_time_with_timezone(trino_connection, tz_str: str):
		@pytest.mark.parametrize(

Address time zone localization issue #368

Address time zone localization issue #368

Conversation

john-bodley commented Apr 30, 2023 • edited Loading

Description

Before

After

Non-technical explanation

Release notes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdesmet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

john-bodley commented May 2, 2023

john-bodley commented May 2, 2023

hashhar commented May 3, 2023

hashhar left a comment

Choose a reason for hiding this comment

hashhar left a comment

Choose a reason for hiding this comment

hashhar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

john-bodley commented May 3, 2023

john-bodley commented Apr 30, 2023 •

edited

Loading