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.

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

Support UUID type #354

Merged
merged 1 commit into from
Apr 11, 2023
Merged

Support UUID type #354

merged 1 commit into from
Apr 11, 2023

Conversation

lpoulain
Copy link
Member

@lpoulain lpoulain commented Apr 6, 2023

Description

Map Trino UUID type to Python's

Non-technical explanation

Support Trino UUID type

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:

* Maps Trino UUID type into Python's UUID

@cla-bot cla-bot bot added the cla-signed label Apr 6, 2023
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.

Missing prepared statement handling

trino/client.py Outdated
Comment on lines 1191 to 1192
try:
return uuid.UUID(values)
except:
return None
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the exception handling, I don't think we would like to silently discard values.

Suggested change
try:
return uuid.UUID(values)
except:
return None
return uuid.UUID(values)

Copy link
Member Author

Choose a reason for hiding this comment

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

The test currently expects that CAST(NULL AS UUID) returns None in Python. Should we expect a different behavior or a specific case should be applied when values is None?

Copy link
Contributor

Choose a reason for hiding this comment

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

I get it but we should not let None go into UUID, similar like all the other mappers.

@@ -1,5 +1,6 @@
import math
import re
import uuid
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
import uuid
from uuid import UUID

Copy link
Member Author

Choose a reason for hiding this comment

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

The reason why I did this is because I saw elsewhere in the code was importing uuid instead of uuid.UUID. But I can change this.

@lpoulain lpoulain force-pushed the support-uuid-type branch from 64e9ec5 to c67655c Compare April 6, 2023 14:59
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 % minor comment

trino/client.py Outdated
@@ -1185,6 +1186,13 @@ def map(self, values: Any) -> Optional[Dict[Any, Optional[Any]]]:
}


class UuidValueMapper(ValueMapper[uuid.UUID]):
def map(self, values: Any) -> Optional[uuid.UUID]:
Copy link
Contributor

Choose a reason for hiding this comment

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

change into value instead of values

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-uuid-type branch from c67655c to d92863c Compare April 6, 2023 15:06
@lpoulain lpoulain force-pushed the support-uuid-type branch from d92863c to 9529547 Compare April 6, 2023 16:03
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.

Please move all sqlalchemy related changes to a separate commit. Also I don't see any tests related to that change.

Support UUID type
@lpoulain lpoulain force-pushed the support-uuid-type branch from 9529547 to ac33c54 Compare April 6, 2023 22:34
@hashhar
Copy link
Member

hashhar commented Apr 10, 2023

does this work with sqla too? If yes then please add tests. Otherwise looks good to go.

i.e. does this fix #331?

@lpoulain
Copy link
Member Author

It does not work yet with sqla. And this does not fix #331 which seems to be a sqla issue (I've added comments to this issue to detail what I've found)

@hashhar hashhar merged commit cc65a03 into trinodb:master Apr 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants