Support UUID for SQLAlchemy #359
Conversation
trino/sqlalchemy/datatype.py
Outdated
| @@ -86,6 +86,11 @@ def get_col_spec(self, **kw): | |||
| return 'JSON' | |||
|
|
|||
|
|
|||
| class UUID(UserDefinedType): | |||
lpoulain
force-pushed
the
support-uuid-for-sqla
branch
from
08d5f90
to
d1d9d46
Compare
| @@ -89,13 +91,15 @@ def test_define_and_create_table(trino_connection): | |||
| sqla.Table('users', | |||
| metadata, | |||
| sqla.Column('id', sqla.Integer), | |||
| sqla.Column('guid', UUID), | |||
There was a problem hiding this comment.
I think this should be sqla.Uuid instead. See https://docs.sqlalchemy.org/en/20/core/type_basics.html#sqlalchemy.types.Uuid
There was a problem hiding this comment.
This works locally, but when running on GH it fails with AttributeError: module 'sqlalchemy' has no attribute 'Uuid'
There was a problem hiding this comment.
Uuid only works in v2 of sqlalchemy. So you will need to ensure it only works in that version. also we should probably test with release version of v2 instead of rc (maybe take care of that in a separate commit).
There was a problem hiding this comment.
So should there be two test_define_and_create_table() tests? One for v2+ and one for older versions?
lpoulain
force-pushed
the
support-uuid-for-sqla
branch
from
d1d9d46
to
fb872b4
Compare
trino/sqlalchemy/datatype.py
Outdated
| class UUID(UserDefinedType): | ||
| __visit_name__ = "UUID" | ||
|
|
||
| def get_col_spec(self, **kw): | ||
| return "UUID" | ||
|
|
||
|
|
There was a problem hiding this comment.
I think this code block should be removed as we don't need to add a custom UserDefinedType
trino/sqlalchemy/datatype.py
Outdated
| @@ -122,7 +129,7 @@ def get_col_spec(self, **kw): | |||
| # | |||
| # === Mixed === | |||
| # 'ipaddress': IPADDRESS | |||
| # 'uuid': UUID, | |||
| "uuid": UUID, | |||
There was a problem hiding this comment.
This should be mapped to the sqlalchemy uuid type if it's available
There was a problem hiding this comment.
What if the type is not available? Won't that break the import statement?
trino/sqlalchemy/compiler.py
Outdated
| def visit_UUID(self, type, **kw): | ||
| return "UUID" | ||
|
|
There was a problem hiding this comment.
This should only be added if the uuid type is available.
| def test_parse_uuid(): | ||
| actual_type = datatype.parse_sqltype("UUID") | ||
| assert type(actual_type) == UUID |
There was a problem hiding this comment.
Test should only be succesful if type is available.
lpoulain
force-pushed
the
support-uuid-for-sqla
branch
2 times, most recently
from
2ede56f
to
e5216f5
Compare
| @@ -133,6 +135,60 @@ def test_insert(trino_connection): | |||
| metadata.drop_all(engine) | |||
|
|
|||
|
|
|||
| @pytest.mark.skipif( | |||
| sqlalchemy_version() < "2.0", | |||
| reason="columns argument to select() must be a Python list or other iterable" | |||
lpoulain
force-pushed
the
support-uuid-for-sqla
branch
from
e5216f5
to
be15d0d
Compare
trino/sqlalchemy/datatype.py
Outdated
| @@ -122,13 +123,16 @@ def get_col_spec(self, **kw): | |||
| # | |||
| # === Mixed === | |||
| # 'ipaddress': IPADDRESS | |||
| # 'uuid': UUID, | |||
There was a problem hiding this comment.
Can you revert this change
|
Please also add that this resolves #331 in the PR description. |
Support UUID for SQLAlchemy
lpoulain
force-pushed
the
support-uuid-for-sqla
branch
from
be15d0d
to
45d1f4a
Compare
Description
Provide a SQLAlchemy mapping to Trino's UUID type (resolves issue #331 )
Non-technical explanation
Provide UUID support for SQLAlchemy
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: