-
Notifications
You must be signed in to change notification settings - Fork 10
Support YDB native UUID type #86
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Adds support for YDB's native UUID type by introducing a custom SQLAlchemy type, mapping it in the compiler and dialect, and exercising it via new tests.
- Introduce
YqlUUID
to handle binding ofuuid.UUID
and string values into a native UUID column. - Extend the SQL generation and YDB type mapping to recognize
UUID
and map it toydb.PrimitiveType.UUID
. - Update the dialect’s type map and add tests for both native and string-based UUID columns.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
File | Description |
---|---|
ydb_sqlalchemy/sqlalchemy/types.py | Added YqlUUID subclass with bind processor and stub result processor |
ydb_sqlalchemy/sqlalchemy/compiler/sa20.py | Added visit_UUID and mapping logic in YqlTypeCompiler |
ydb_sqlalchemy/sqlalchemy/init.py | Mapped YDB’s PrimitiveType.UUID to sa.UUID and registered YqlUUID |
test/test_core.py | Imported uuid , defined test_uuid_types table and new tests for UUIDs |
Comments suppressed due to low confidence (2)
test/test_core.py:322
- Add a test case for passing an invalid UUID string into the
uuid_native
column to verify that aValueError
is raised by the bind processor.
def test_uuid_types(self, connection):
ydb_sqlalchemy/sqlalchemy/types.py:17
- [nitpick] Add a docstring for
YqlUUID
to describe its purpose and behavior for binding and result processing of native UUID values.
class YqlUUID(types.UUID):
import uuid as uuid_module | ||
|
||
value = uuid_module.UUID(value) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Consider moving the import uuid
statement to the module level to avoid repeated imports inside the bind processor, improving clarity and performance.
import uuid as uuid_module | |
value = uuid_module.UUID(value) | |
value = uuid.UUID(value) |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bump
return None | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider implementing a result_processor
to convert raw returned values (e.g., strings) back into uuid.UUID
objects, ensuring consistent Python types.
return None | |
def process(value): | |
if value is None: | |
return None | |
if isinstance(value, str): | |
try: | |
import uuid as uuid_module | |
return uuid_module.UUID(value) | |
except ValueError: | |
raise ValueError(f"Invalid UUID string: {value}") | |
return value | |
return process |
Copilot uses AI. Check for mistakes.
|
||
uuid_value_str = str(uuid_value) | ||
|
||
statement = sa.insert(table).values(id=2, uuid_native=uuid_value_str, uuid_str=uuid_value) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: Shouldn't here be also uuid_str=uuid_value_str
?
if is_optional: | ||
return ydb.OptionalType(ydb_type) | ||
return ydb_type | ||
|
||
if isinstance(type_, sa.Uuid): | ||
ydb_type = ydb.PrimitiveType.Utf8 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: Since sa.Uuid is subtype of sa.UUID this block is unreachable. I think we need to consider sa.Uuid also as ydb.PrimitiveType.UUID, because sqlalchemy is designed in this way:
class Uuid(Emulated, TypeEngine[_UUID_RETURN]):
"""Represent a database agnostic UUID datatype.
For backends that have no "native" UUID datatype, the value will
make use of ``CHAR(32)`` and store the UUID as a 32-character alphanumeric
hex string.
For backends which are known to support ``UUID`` directly or a similar
uuid-storing datatype such as SQL Server's ``UNIQUEIDENTIFIER``, a
"native" mode enabled by default allows these types will be used on those
backends.
Though it is not backward compatible for sure.
import uuid as uuid_module | ||
|
||
value = uuid_module.UUID(value) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bump
return process | ||
|
||
def result_processor(self, dialect, coltype): | ||
return None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: Why return None
? Don't we need to either implement or to not touch this method?
@@ -310,6 +319,22 @@ def test_datetime_types_timezone(self, connection): | |||
today, | |||
) | |||
|
|||
def test_uuid_types(self, connection): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: Let's try to unskip uuid tests in suite by removing @pytest.mark.skip("uuid unsupported for columns")
@@ -310,6 +319,22 @@ def test_datetime_types_timezone(self, connection): | |||
today, | |||
) | |||
|
|||
def test_uuid_types(self, connection): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (non-blocking): Consider to add test that ydb.PrimitiveType.UUID
column is correctly reflected to sa.UUID by calling metadata.reflect()
Closes: #84