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

fix: Address time zone localization issue #367

Closed
wants to merge 1 commit into from

Conversation

john-bodley
Copy link
Contributor

@john-bodley john-bodley commented Apr 28, 2023

Description

This PR addresses #366.

Note @mdesmet et al. it doesn't address the time zone conundrum with fractional times or times (as opposed to timestamps). Additionally I sense there's an issue with how the TimeWithTimeZoneValueMapper works (or doesn't work).

Specifically, the following Trino query,

SELECT TIME '01:00' AT TIME ZONE 'America/Los_Angeles'

returns 18:00:00-07:00 when run on 2023-04-28 when daylights savings is in effect for said time zone. In order to resolve this query (I presume) the Trino server has had to taken has taken into account the current date on the server to "anchor" the time, i.e., if one were to run the same query on 2023-01-01 when daylights savings in not in effect the result would be 17:00:00-08:00.

The issue is the mapping that @mdesmet outlined is running on the Trino client—which is pseudo agnostic* of the server's timezone—and may be in a different timezone (and thus date) than the Trino server. The problem seems ill-defined, i.e., when mapping this to a Python object localized to a timezone without a numerical offset, i.e., America/Los_Angeles, one first needs to know the underlying date.

* Granted the client isn't completely agnostic of the current date on the server, i.e., it can be determined via SELECT current_date, however this requires an additional query and there's no guarantee this will be executed on the same date as the primary query.

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.
( ) Release notes are required, with the following suggested text:

* Fix localization of timestamp with time zone. ({[366](https://github.com/trinodb/trino-python-client/issues/366)})

python=datetime(2001, 8, 23, 0, 0, 0, 0).replace(tzinfo=tz))
# ce
.add_field(
sql=f"TIMESTAMP '0001-01-01 01:23:45.123 {tz_str}'",
Copy link
Contributor Author

@john-bodley john-bodley Apr 29, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a non-representative date for the US/Eastern and Asia/Kolkata time zones. It's unclear how the DB-API should handle such cases, i.e., should it raise an exception?

@john-bodley
Copy link
Contributor Author

Per this article I wonder if there's merit in removing pytz in favor of dateutil.tz which since Python 3.6 remedies ambiguous datetimes due to daylight saving time transition.

@john-bodley
Copy link
Contributor Author

Closing in favor of #368.

@john-bodley john-bodley changed the title fix(dbapi): Address timestamp with time zone localization issue fix: Address time zone localization issue Apr 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

1 participant