Support variable precision time timestamp by lpoulain · Pull Request #300 · trinodb/trino-python-client

lpoulain · 2022-12-13T01:13:05Z

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.

mdesmet

LGTM % comments.

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

mdesmet · 2022-12-13T09:04:28Z

trino/client.py

            In case the supplied value exceeds the specified precision,
            the value needs to be rounded.
        """
+        precision = min(precision, 6)


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

tests/integration/test_dbapi_integration.py

mdesmet · 2022-12-15T09:44:47Z

trino/client.py

    PROXIES = {}

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


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

tests/integration/test_dbapi_integration.py

mdesmet · 2022-12-15T09:47:52Z

trino/client.py

            raise exceptions.TrinoUserError("Query has been cancelled", self.query_id)

-        response = self._request.post(self._sql, additional_http_headers)
+        if self._experimental_python_types:


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

Shouldn't this be part of another PR?

mdesmet · 2022-12-16T10:02:13Z

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

hashhar · 2022-12-20T09:18:16Z

tests/integration/test_dbapi_integration.py

    rows = cur.fetchall()

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


same for time with time zone?

Added similar assertion to test_time_with_time_zone_negative_offset() and test_time_with_time_zone_positive_offset()

hashhar · 2022-12-20T09:19:13Z

tests/integration/test_types_integration.py

-        #     python=time(0, 0, 0))  # PARAMETRIC_DATETIME not enabled
-        # round down
        .add_field(
-            sql="TIME '00:00:00.000100'",


where did this case go?

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

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 · 2022-12-20T09:19:39Z

tests/integration/test_types_integration.py

-            sql="TIME '00:00:00.0000001'",
-            python=time(0, 0, 0))  # PARAMETRIC_DATETIME not enabled


where did this go?

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

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

hashhar · 2022-12-20T09:27:27Z

trino/client.py

+        http_headers = {constants.HEADER_CLIENT_CAPABILITIES: 'PARAMETRIC_DATETIME'}
+        if additional_http_headers:
+            http_headers.update(additional_http_headers)


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

hashhar

LGTM % move header setting code to http_headers method

hashhar

Please rebase/squash and resolve conflict. LGTM otherwise.

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 · 2022-12-27T11:41:59Z

rebased and reworded

cla-bot bot added the cla-signed label Dec 13, 2022

lpoulain requested review from ebyhr, hashhar and mdesmet December 13, 2022 01:13

ebyhr removed their request for review December 13, 2022 01:57

mdesmet reviewed Dec 13, 2022

View reviewed changes

lpoulain force-pushed the support-variable-precision-time-timestamp branch from c237202 to d254794 Compare December 14, 2022 20:20

mdesmet reviewed Dec 15, 2022

View reviewed changes

mdesmet approved these changes Dec 16, 2022

View reviewed changes

lpoulain force-pushed the support-variable-precision-time-timestamp branch from bcd075a to 37bcf0d Compare December 16, 2022 14:02

hovaesco approved these changes Dec 19, 2022

View reviewed changes

hashhar reviewed Dec 20, 2022

View reviewed changes

hashhar approved these changes Dec 21, 2022

View reviewed changes

lpoulain force-pushed the support-variable-precision-time-timestamp branch 2 times, most recently from f4b1103 to 18fdea1 Compare December 21, 2022 15:36

hashhar force-pushed the support-variable-precision-time-timestamp branch from 18fdea1 to 8d5e9d0 Compare December 27, 2022 11:41

hashhar approved these changes Dec 27, 2022

View reviewed changes

hashhar mentioned this pull request Dec 27, 2022

Release notes for 0.321.0 #298

Closed

hashhar merged commit 6d08841 into trinodb:master Dec 27, 2022

bitsondatadev mentioned this pull request Dec 27, 2022

Release notes for 0.321.0 #304

Merged

nineinchnick mentioned this pull request Jan 3, 2023

Support times better trinodb/trino-go-client#64

Merged

hashhar mentioned this pull request Jul 11, 2023

Support variable precision time, timestamp, time with time zone, timestamp with time zone types #42

Closed

		sql="TIME '00:00:00.0000001'",
		python=time(0, 0, 0)) # PARAMETRIC_DATETIME not enabled

Conversation

lpoulain commented Dec 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Non-technical explanation

Release notes

Uh oh!

mdesmet left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mdesmet commented Dec 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hashhar left a comment

Choose a reason for hiding this comment

Uh oh!

hashhar left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hashhar commented Dec 27, 2022

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

lpoulain commented Dec 13, 2022 •

edited

Loading

mdesmet commented Dec 16, 2022 •

edited

Loading

hashhar left a comment •

edited

Loading