Change experimental_python_types to legacy_primitive_types. Invert lo… #308
Conversation
damian3031
requested review from
aalbu,
hashhar,
hovaesco and
mdesmet
and removed request for
hashhar
README.md
Outdated
|
|
||
| conn = trino.dbapi.connect( | ||
| experimental_python_types=True, | ||
| legacy_primitive_types=False, |
There was a problem hiding this comment.
This doesn't really make sense as this is the default. I would just remove this code fragment
| rows = cur.fetchall() | ||
|
|
||
| if expected: | ||
| if not expected: |
There was a problem hiding this comment.
Maybe it makes sense to rename expected into something more explicit, eg legacy_primitive_types_expected
There was a problem hiding this comment.
renamed to expected_legacy_primitive_types
|
|
||
| def test_decimal_query_param(trino_connection): | ||
| cur = trino_connection.cursor(experimental_python_types=True) | ||
| cur = trino_connection.cursor(legacy_primitive_types=False) |
There was a problem hiding this comment.
Please omit the parameter as the default is tested in test_legacy_primitive_types_with_connection_and_cursor
damian3031
force-pushed
the
enable-experimental_python_types-by-default
branch
from
898ad81 to
4653c60
Compare
damian3031
force-pushed
the
enable-experimental_python_types-by-default
branch
from
4653c60 to
fd28548
Compare
README.md
Outdated
| "session_properties": {'query_max_run_time': '1d'}, | ||
| "client_tags": ["tag1", "tag2"], | ||
| "experimental_python_types": True, | ||
| "legacy_primitive_types": True, |
There was a problem hiding this comment.
Remove these entries, these code parts tend to be copied by users and we would rather not let them set this to True.
README.md
Outdated
| 'session_properties={"query_max_run_time": "1d"}' | ||
| '&client_tags=["tag1", "tag2"]' | ||
| '&experimental_python_types=true' | ||
| '&legacy_primitive_types=true' |
README.md
Outdated
| port=8080, | ||
| client_tags=["tag1", "tag2"], | ||
| experimental_python_types=True | ||
| legacy_primitive_types=True |
damian3031
force-pushed
the
enable-experimental_python_types-by-default
branch
from
fd28548 to
665f8d3
Compare
|
@hashhar would you have some time to look at this PR? Please approve if you are ok with these changes 🙂 |
hashhar
left a comment
hashhar
left a comment
There was a problem hiding this comment.
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
| exits the *with* context and the queries succeed, otherwise | ||
| `trino.dbapi.Connection.rollback()` will be called. | ||
|
|
||
| ## Improved Python types |
There was a problem hiding this comment.
| ## Improved Python types | |
| ## Legacy Primitive types |
since this section now describes how to use the legacy behaviour.
damian3031
force-pushed
the
enable-experimental_python_types-by-default
branch
from
665f8d3 to
eb72e00
Compare
damian3031
force-pushed
the
enable-experimental_python_types-by-default
branch
from
eb72e00 to
b925efe
Compare
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
force-pushed
the
enable-experimental_python_types-by-default
branch
from
b925efe to
b0a9e0d
Compare
|
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? |
4 participants
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: