Skip to content

Support variable precision time timestamp #300

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Dec 27, 2022
Merged

Support variable precision time timestamp #300

merged 1 commit into from
Dec 27, 2022

Conversation

lpoulain
Copy link
Member

@lpoulain lpoulain commented Dec 13, 2022 •
edited
Loading

Description

Set X-Trino-Client-Capabilities to PARAMETRIC_DATETIME. Update the unit and integration tests accordingly.

Non-technical explanation

When querying Trino, set the X-Trino-Client-Capabilities HTTP header to PARAMETRIC_DATETIME. This instructs Trino to return full-precision datetimes and times.

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:

* The client is now requesting PARAMETRIC_DATETIME, leading Trino to return full-precision datetimes and times.

@cla-bot cla-bot bot added the cla-signed label Dec 13, 2022
@ebyhr ebyhr removed their request for review December 13, 2022 01:57
mdesmet
mdesmet reviewed Dec 13, 2022
View reviewed changes
Copy link
Contributor

@mdesmet mdesmet left a comment

Choose a reason for hiding this comment

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

LGTM % comments.

Also there is a merge commit, could you instead rebase on current master?

trino/client.py Outdated
@@ -912,6 +919,7 @@ def round_to(self, precision: int) -> TemporalType:
In case the supplied value exceeds the specified precision,
the value needs to be rounded.
"""
precision = min(precision, 6)
Copy link
Contributor

Choose a reason for hiding this comment

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

Please create a constant with a descriptive name for the max python temporal precision.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@lpoulain lpoulain force-pushed the support-variable-precision-time-timestamp branch from c237202 to d254794 Compare December 14, 2022 20:20
mdesmet
mdesmet reviewed Dec 15, 2022
View reviewed changes
trino/client.py Outdated
@@ -66,6 +66,7 @@
PROXIES = {}

_HEADER_EXTRA_CREDENTIAL_KEY_REGEX = re.compile(r'^\S[^\s=]*$')
PYTHON_MAX_DECIMAL_PRECISION = 6
Copy link
Contributor

Choose a reason for hiding this comment

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

Rename to MAX_PYTHON_TEMPORAL_PRECISION_POWER, move to line 77 and reuse in constant MAX_PYTHON_TEMPORAL_PRECISION.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

mdesmet
mdesmet reviewed Dec 15, 2022
View reviewed changes
trino/client.py Outdated
@@ -747,7 +748,14 @@ def execute(self, additional_http_headers=None) -> TrinoResult:
if self.cancelled:
raise exceptions.TrinoUserError("Query has been cancelled", self.query_id)

response = self._request.post(self._sql, additional_http_headers)
if self._experimental_python_types:
Copy link
Contributor

Choose a reason for hiding this comment

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

Actually you can always enable PARAMETRIC_DATETIME. This is also how the JDBC driver works. Please also adjust the commit message and PR description.

Copy link
Member Author

Choose a reason for hiding this comment

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

Shouldn't this be part of another PR?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@mdesmet
Copy link
Contributor

mdesmet commented Dec 16, 2022 •
edited
Loading

@lpoulain: Please squash the commits, also adapt PR description and commit message (remove the experimental_python_types reference).

@lpoulain lpoulain force-pushed the support-variable-precision-time-timestamp branch from bcd075a to 37bcf0d Compare December 16, 2022 14:02
hashhar
hashhar reviewed Dec 20, 2022
View reviewed changes
tests/integration/test_dbapi_integration.py
@@ -461,6 +461,7 @@ def test_time_query_param(trino_connection):
rows = cur.fetchall()

assert rows[0][0] == params
assert cur.description[0][1] == "time(6)"
Copy link
Member

Choose a reason for hiding this comment

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

same for time with time zone?

Copy link
Member Author

Choose a reason for hiding this comment

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

Added similar assertion to test_time_with_time_zone_negative_offset() and test_time_with_time_zone_positive_offset()

hashhar
hashhar reviewed Dec 20, 2022
View reviewed changes
tests/integration/test_types_integration.py
.add_field(
sql="TIME '00:00:00.000100'",
Copy link
Member

Choose a reason for hiding this comment

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

where did this case go?

Copy link
Member Author

Choose a reason for hiding this comment

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

Do we still need this considering there is no rounding involved here?

Copy link
Member

Choose a reason for hiding this comment

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

Good catch, the rounding only happenned when parametric timestamp was not implemented. 00:00:00.000100 has 1 at 4th decimal digit and without parametric timestamp support it gets rounded to 00:00:00.

hashhar
hashhar reviewed Dec 20, 2022
View reviewed changes
tests/integration/test_types_integration.py
sql="TIME '00:00:00.0000001'",
python=time(0, 0, 0)) # PARAMETRIC_DATETIME not enabled
Copy link
Member

Choose a reason for hiding this comment

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

where did this go?

Copy link
Member Author

Choose a reason for hiding this comment

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

Isn't this moved a few lines below (233)?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, the diff on GitHub looked a bit wacky. I've re-reviewed locally in IDE and everything looks good.

hashhar
hashhar reviewed Dec 20, 2022
View reviewed changes
trino/client.py Outdated
Comment on lines 751 to 753
http_headers = {constants.HEADER_CLIENT_CAPABILITIES: 'PARAMETRIC_DATETIME'}
if additional_http_headers:
http_headers.update(additional_http_headers)
Copy link
Member

Choose a reason for hiding this comment

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

this must go into the http_headers method instead where we already do the request header setting logic.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

hashhar
hashhar reviewed Dec 20, 2022
View reviewed changes
Copy link
Member

@hashhar hashhar left a comment

Choose a reason for hiding this comment

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

LGTM % move header setting code to http_headers method

hashhar
hashhar approved these changes Dec 21, 2022
View reviewed changes
Copy link
Member

@hashhar hashhar left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Please rebase/squash and resolve conflict. LGTM otherwise.

@lpoulain lpoulain force-pushed the support-variable-precision-time-timestamp branch 2 times, most recently from f4b1103 to 18fdea1 Compare December 21, 2022 15:36
@lpoulain @hashhar
Add support for parametric datetimes
8d5e9d0 
This change starts sending the PARAMETRIC_DATETIME client capability in
the X-Trino-Client-Capabilities request headers so that Trino sends back
temporal types in their original precision instead of always sending
back results with millisecond precision.
@hashhar hashhar force-pushed the support-variable-precision-time-timestamp branch from 18fdea1 to 8d5e9d0 Compare December 27, 2022 11:41
@hashhar
Copy link
Member

hashhar commented Dec 27, 2022

rebased and reworded

@hashhar hashhar mentioned this pull request Dec 27, 2022
Closed
@hashhar hashhar merged commit 6d08841 into trinodb:master Dec 27, 2022
@bitsondatadev bitsondatadev mentioned this pull request Dec 27, 2022
Merged
@nineinchnick nineinchnick mentioned this pull request Jan 3, 2023
Merged
@hashhar hashhar mentioned this pull request Jul 11, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mdesmet mdesmet mdesmet approved these changes

@hashhar hashhar hashhar approved these changes

@hovaesco hovaesco hovaesco approved these changes

Assignees
No one assigned
Labels
cla-signed
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@lpoulain @mdesmet @hashhar @hovaesco