Add support for TIMEZONE #252

ulisesojeda · 2022-10-07T06:52:14Z

Add support for TIMEZONE

Fixes #27

mdesmet

Thanks for this contribution, overall looks very good.

Note that this also should be documented.

mdesmet · 2022-10-07T21:07:57Z

tests/integration/test_dbapi_integration.py

+    cur = trino_connection.cursor()
+    cur.execute('SHOW TABLES FROM information_schema')
+    cur.fetchall()
+    assert cur._request.http_headers[constants.HEADER_TIMEZONE] == "Europe/Brussels"


instead of the request header testing, please check using SELECT current_timezone()

Good point. Thanks

mdesmet · 2022-10-07T21:08:52Z

tests/integration/test_dbapi_integration.py

+    cur = trino_connection.cursor()
+    cur.execute('SHOW TABLES FROM information_schema')
+    cur.fetchall()
+    assert cur._request.http_headers[constants.HEADER_TIMEZONE] == get_localzone_name()


Same as in test_set_timezone_in_connection.

mdesmet · 2022-10-07T21:10:50Z

tests/unit/test_client.py

@@ -1056,3 +1060,51 @@ def test_request_headers_role_empty(mock_get_and_post):
    req.get("URL")
    _, get_kwargs = get.call_args
    assert_headers_with_roles(post_kwargs["headers"], None)
+
+
+def assert_headers_timezone(headers: Dict[str, str], timezone: Optional[str]):


Make second parameter required instead of optional and move get_localzone_name() call to the test itself. Makes the test easier to follow.

mdesmet · 2022-10-07T21:15:19Z

trino/client.py

@@ -127,6 +131,7 @@ def __init__(
        self._roles = roles.copy() if roles is not None else {}
        self._prepared_statements: Dict[str, str] = {}
        self._object_lock = threading.Lock()
+        self._timezone = timezone or get_localzone_name()


This might be a breaking change, In the sense that suddenly the local timezone will be used compared to UTC.

I'll leave this to the maintainers to decide if we need to put it behind a feature flag or announce it as a breaking change.

Ok, how should we proceed?

@hashhar , @ebyhr : Please advice.

Q: Before this change the server returned values in UTC? If yes, then won't setting timezone=UTC here explicitly preserve old behaviour? If yes then it means a workaround exists to preserve old behaviour and we can mention it in release notes.

Note that in larger picture this makes Python client match JDBC client etc. So I'd consider it a bugfix which happens to be breaking change.

The current behavior is timezone=UTC as default.

mdesmet · 2022-10-07T21:19:17Z

setup.py

@@ -39,6 +39,7 @@
    "pytest",
    "pytest-runner",
    "click",
+    "tzlocal",


This can be removed as it is already included in the install_requires.

ulisesojeda · 2022-10-12T10:11:20Z

Thanks for this contribution, overall looks very good.

Note that this also should be documented.

Done the doc. Thanks for your review and comments @mdesmet !

mdesmet · 2022-10-12T10:51:19Z

README.md

@@ -357,6 +357,20 @@ conn = trino.dbapi.connect(
 )
 ```

+## Timezone
+
+Sets the time zone for the session using the time zone name. Defaults to the timezone set on your workstation.


Suggested change

Sets the time zone for the session using the time zone name. Defaults to the timezone set on your workstation.

The time zone for the session can be explicitly set using the official IANA time zone name. When not set the time zone defaults to the client side local timezone.

👍 Thanks

mdesmet · 2022-10-12T10:52:00Z

trino/client.py

@@ -127,6 +131,7 @@ def __init__(
        self._roles = roles.copy() if roles is not None else {}
        self._prepared_statements: Dict[str, str] = {}
        self._object_lock = threading.Lock()
+        self._timezone = timezone or get_localzone_name()


@hashhar , @ebyhr : Please advice.

mdesmet · 2022-10-12T10:59:13Z

tests/integration/test_dbapi_integration.py

+    session_tz = res[0][0]
+    localzone = get_localzone_name()
+    assert session_tz == localzone or \
+        (session_tz == "UTC" and localzone == "Etc/UTC") \


I conclude from this test if we set the timezone header to "Etc/UTC", Trino actually understands this but returns UTC as the current_timezone().

Is my understanding correct?

mdesmet · 2022-10-12T11:00:06Z

trino/client.py

@@ -127,6 +131,7 @@ def __init__(
        self._roles = roles.copy() if roles is not None else {}
        self._prepared_statements: Dict[str, str] = {}
        self._object_lock = threading.Lock()
+        self._timezone = timezone or get_localzone_name()


is there a way to validate the time zone using tzlocal?

Good point. I added the check with ZoneInfo

mdesmet · 2022-10-27T10:57:12Z

@ulisesojeda : Please don't merge master into your branch but rebase on master.

ulisesojeda · 2022-10-31T15:26:50Z

@ebyhr @hashhar could you please check it?

hashhar

Thanks for implementing this @ulisesojeda. I've started reviewing this and sorry about the delays here.

hashhar

Nice, thanks for a lot for adding this support.

Left some comments.

README.md

tests/integration/test_dbapi_integration.py

trino/client.py

hashhar · 2022-11-14T07:02:43Z

tests/unit/test_client.py

@@ -47,6 +48,12 @@
    _RetryWithExponentialBackoff,
 )

+try:
+    from zoneinfo._common import ZoneInfoNotFoundError  # type: ignore


Can directly import zoneinfo.ZoneInfoNotFoundError - that's the public location.

as per https://pypi.org/project/backports.zoneinfo/, with need to import the backport for Python <3.9. Did I miss something?

Oh, good point. I was checking with 3.10 only.

Can you add a TODO comment that it can be simplified once we move past 3.9?

Sorry I was saying something like this:

try: from zoneinfo import ZoneInfoNotFoundError except ModuleNotFoundError: from backports.zoneinfo._common import ZoneInfoNotFoundError

hashhar · 2022-11-14T07:05:45Z

trino/client.py

@@ -127,6 +138,9 @@ def __init__(
        self._roles = roles.copy() if roles is not None else {}
        self._prepared_statements: Dict[str, str] = {}
        self._object_lock = threading.Lock()
+        if timezone:  # Check timezone validity
+            ZoneInfo(timezone)


Just for other reviewers this throws on invalid timezones (invalidity is determined by the OS's tzdb files or the tzdata package shipped by Python itself):

ZoneInfoNotFoundError: 'No time zone found with key Foobar'

trino/client.py

hashhar

LGTM % some comments.

Thanks for this.

I plan to not merge this for 0.320.0 so that other features can be part of 0.320.0 which people might be waiting for. Then 0.321.0 can introduce this change. I hope that's ok?

While there's a way to preserve old beahviour it requires someone to read release notes so I'd like to announce on Trino community before this change.

mdesmet · 2022-11-18T08:46:21Z

.pre-commit-config.yaml

@@ -13,9 +13,4 @@ repos:
        additional_dependencies:
          - "types-pytz"
          - "types-requests"
-


The isort section should not be removed. Can you revert this?

good catch. I didn't notice this. 🙈

mdesmet · 2022-11-18T09:40:04Z

setup.py

@@ -79,8 +79,8 @@
        "Programming Language :: Python :: Implementation :: PyPy",
        "Topic :: Database :: Front-Ends",
    ],
-    python_requires=">=3.7",
-    install_requires=["pytz", "requests"],
+    python_requires='>=3.7',


Please revert the unrelated change on python_requires

ulisesojeda · 2022-11-18T19:37:16Z

LGTM % some comments.

Thanks for this.

I plan to not merge this for 0.320.0 so that other features can be part of 0.320.0 which people might be waiting for. Then 0.321.0 can introduce this change. I hope that's ok?

While there's a way to preserve old beahviour it requires someone to read release notes so I'd like to announce on Trino community before this change.

Sure. Thanks for the review @hashhar

hashhar · 2022-12-12T09:30:07Z

Rebased. Will merge once CI is done.

Co-authored-by: Ulises <[email protected]> Co-authored-by: Ashhar Hasan <[email protected]>

cla-bot bot added the cla-signed label Oct 7, 2022

ulisesojeda force-pushed the support-timezone branch from d751cb4 to 51f465b Compare October 7, 2022 07:35

ulisesojeda requested a review from mdesmet October 7, 2022 08:35

mdesmet reviewed Oct 7, 2022

View reviewed changes

ulisesojeda force-pushed the support-timezone branch 3 times, most recently from 74aa1b4 to 7972aab Compare October 12, 2022 10:07

ulisesojeda force-pushed the support-timezone branch from 7972aab to 5e7f926 Compare October 12, 2022 10:41

mdesmet reviewed Oct 12, 2022

View reviewed changes

mdesmet requested review from ebyhr and hashhar October 12, 2022 11:01

ulisesojeda force-pushed the support-timezone branch 2 times, most recently from 1bf9336 to 423e360 Compare October 19, 2022 07:58

ulisesojeda force-pushed the support-timezone branch 2 times, most recently from e611269 to 0acb5e7 Compare October 31, 2022 15:16

hashhar reviewed Nov 10, 2022

View reviewed changes

hashhar reviewed Nov 14, 2022

View reviewed changes

ulisesojeda force-pushed the support-timezone branch from 5853f72 to 70d59e7 Compare November 18, 2022 08:06

hashhar reviewed Nov 18, 2022

View reviewed changes

mdesmet reviewed Nov 18, 2022

View reviewed changes

hashhar force-pushed the support-timezone branch from 70d59e7 to bc78644 Compare December 12, 2022 09:29

Add support for TIMEZONE

03f2670

Co-authored-by: Ulises <[email protected]> Co-authored-by: Ashhar Hasan <[email protected]>

hashhar force-pushed the support-timezone branch from bc78644 to 03f2670 Compare December 12, 2022 09:59

hashhar merged commit a83dcfe into trinodb:master Dec 16, 2022

bitsondatadev mentioned this pull request Dec 20, 2022

Release notes for 0.321.0 #304

Merged

hashhar mentioned this pull request Dec 22, 2022

Release notes for 0.321.0 #298

Closed

findinpath mentioned this pull request Jan 31, 2023

Upgrade to support dbt core v141 starburstdata/dbt-trino#214

Merged

	Sets the time zone for the session using the time zone name. Defaults to the timezone set on your workstation.
	The time zone for the session can be explicitly set using the official IANA time zone name. When not set the time zone defaults to the client side local timezone.

Add support for TIMEZONE #252

Add support for TIMEZONE #252

Conversation

ulisesojeda commented Oct 7, 2022

mdesmet left a 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

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

Choose a reason for hiding this comment

ulisesojeda Nov 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ulisesojeda commented Oct 12, 2022

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdesmet commented Oct 27, 2022

ulisesojeda commented Oct 31, 2022

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hashhar left a comment

Choose a reason for hiding this comment

mdesmet Nov 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ulisesojeda commented Nov 18, 2022

hashhar commented Dec 12, 2022

ulisesojeda Nov 18, 2022 •

edited

Loading

mdesmet Nov 18, 2022 •

edited

Loading