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.

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

Change experimental_python_types to legacy_primitive_types. Invert lo… #308

Merged
merged 1 commit into from
Jan 6, 2023
Merged

Change experimental_python_types to legacy_primitive_types. Invert lo… #308

merged 1 commit into from
Jan 6, 2023

Conversation

damian3031
Copy link
Member

Description

Resolves #305
Change experimental_python_types to legacy_primitive_types. Invert logic behind interpreting this parameter.

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:

* Change experimental_python_types to legacy_primitive_types. Invert logic behind interpreting this parameter. (#305 `issuenumber`)

@cla-bot cla-bot bot added the cla-signed label Dec 23, 2022
@damian3031 damian3031 requested review from mdesmet, hashhar, hovaesco and aalbu and removed request for hashhar December 23, 2022 15:08
mdesmet
mdesmet reviewed Dec 23, 2022
View reviewed changes
README.md Outdated
@@ -456,7 +457,7 @@ import pytz
from datetime import datetime

conn = trino.dbapi.connect(
experimental_python_types=True,
legacy_primitive_types=False,
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't really make sense as this is the default. I would just remove this code fragment

Copy link
Member Author

Choose a reason for hiding this comment

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

removed

tests/integration/test_dbapi_integration.py Outdated
@@ -211,7 +211,7 @@ def test_experimental_python_types_with_connection_and_cursor(
""")
rows = cur.fetchall()

if expected:
if not expected:
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it makes sense to rename expected into something more explicit, eg legacy_primitive_types_expected

Copy link
Member Author

Choose a reason for hiding this comment

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

renamed to expected_legacy_primitive_types

tests/integration/test_dbapi_integration.py Outdated
@@ -231,7 +231,7 @@ def test_experimental_python_types_with_connection_and_cursor(


def test_decimal_query_param(trino_connection):
cur = trino_connection.cursor(experimental_python_types=True)
cur = trino_connection.cursor(legacy_primitive_types=False)
Copy link
Contributor

Choose a reason for hiding this comment

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

Please omit the parameter as the default is tested in test_legacy_primitive_types_with_connection_and_cursor

Copy link
Member Author

Choose a reason for hiding this comment

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

omitted

@damian3031 damian3031 force-pushed the enable-experimental_python_types-by-default branch from 898ad81 to 4653c60 Compare December 28, 2022 18:18
@damian3031 damian3031 requested a review from mdesmet December 28, 2022 18:20
@damian3031 damian3031 force-pushed the enable-experimental_python_types-by-default branch from 4653c60 to fd28548 Compare December 28, 2022 18:53
mdesmet
mdesmet reviewed Dec 29, 2022
View reviewed changes
README.md Outdated
@@ -116,7 +116,7 @@ engine = create_engine(
connect_args={
"session_properties": {'query_max_run_time': '1d'},
"client_tags": ["tag1", "tag2"],
"experimental_python_types": True,
"legacy_primitive_types": True,
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove these entries, these code parts tend to be copied by users and we would rather not let them set this to True.

Copy link
Member Author

Choose a reason for hiding this comment

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

removed

README.md Outdated
@@ -126,7 +126,7 @@ engine = create_engine(
'trino://user@localhost:8080/system?'
'session_properties={"query_max_run_time": "1d"}'
'&client_tags=["tag1", "tag2"]'
'&experimental_python_types=true'
'&legacy_primitive_types=true'
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here.

Copy link
Member Author

Choose a reason for hiding this comment

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

removed

README.md Outdated
@@ -135,7 +135,7 @@ engine = create_engine(URL(
host="localhost",
port=8080,
client_tags=["tag1", "tag2"],
experimental_python_types=True
legacy_primitive_types=True
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here

Copy link
Member Author

Choose a reason for hiding this comment

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

removed

README.md Show resolved Hide resolved
@damian3031 damian3031 force-pushed the enable-experimental_python_types-by-default branch from fd28548 to 665f8d3 Compare December 30, 2022 09:49
@damian3031 damian3031 requested a review from mdesmet December 30, 2022 09:53
@damian3031
Copy link
Member Author

@hashhar would you have some time to look at this PR? Please approve if you are ok with these changes 🙂

hashhar
hashhar reviewed Jan 3, 2023
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.

Do we have any test with legacy_primitive_type=True for values which cannot be represented by Python types - just to make sure that the primitive code path doesn't use Python types anywhere at all.

The important cases I can think of are with parametric timestamps since the parsing logic uses Python types IIRC.

README.md Outdated
@@ -442,8 +439,9 @@ exits the *with* context and the queries succeed, otherwise

## Improved Python types
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
## Improved Python types
## Legacy Primitive types

since this section now describes how to use the legacy behaviour.

Copy link
Member Author

Choose a reason for hiding this comment

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

changed

README.md Show resolved Hide resolved
tests/integration/test_dbapi_integration.py Outdated Show resolved Hide resolved
@damian3031 damian3031 force-pushed the enable-experimental_python_types-by-default branch from 665f8d3 to eb72e00 Compare January 4, 2023 12:00
@damian3031
Copy link
Member Author

@hashhar

Do we have any test with legacy_primitive_type=True for values which cannot be represented by Python types...

I've added it here (assertion here)

@damian3031 damian3031 requested a review from hashhar January 4, 2023 12:11
@damian3031 damian3031 force-pushed the enable-experimental_python_types-by-default branch from eb72e00 to b925efe Compare January 4, 2023 21:50
@damian3031 @hashhar
Make Python types the default and add legacy_primitive_types config
b0a9e0d 
This commit makes the behaviour of experimental_python_types enabled to
be the default.
It introduces a new connection parameter legacy_primitive_types which
can be enabled to restore the old default behaviour of type mapping.
@hashhar hashhar force-pushed the enable-experimental_python_types-by-default branch from b925efe to b0a9e0d Compare January 6, 2023 12:25
@hashhar
Copy link
Member

hashhar commented Jan 6, 2023

Just reworded the commit message.

As a follow-up do you think we should add a table of what Trino types map to what Python types and vice-versa?

@hashhar hashhar merged commit 1dc80fb into trinodb:master Jan 6, 2023
@hashhar hashhar mentioned this pull request Jan 6, 2023
Closed
@bitsondatadev bitsondatadev mentioned this pull request Jan 11, 2023
Merged
@john-bodley john-bodley mentioned this pull request May 30, 2023
Merged
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hovaesco hovaesco hovaesco approved these changes

@hashhar hashhar hashhar approved these changes

@mdesmet mdesmet mdesmet approved these changes

@aalbu aalbu Awaiting requested review from aalbu

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

Successfully merging this pull request may close these issues.

Enable experimental_python_types by default and change the name
4 participants
@damian3031 @hashhar @mdesmet @hovaesco