feat(sqlalchemy-spanner): native UUID data type support in dialect - #17913
feat(sqlalchemy-spanner): native UUID data type support in dialect#17913sakthivelmanii wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces native UUID support to the Spanner SQLAlchemy dialect by registering the UUID type mapping, enabling supports_native_uuid on the dialect, and adding corresponding unit tests. The review feedback suggests two improvements: first, registering both types.UUID and types.Uuid in the inverse type map to prevent potential mapping failures in SQLAlchemy 2.0; second, removing the redundant visit_UUID method in SpannerTypeCompiler as the base class already handles UUID compilation correctly while respecting the supports_native_uuid flag.
3952c47 to
809b53f
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request adds native UUID support to the Spanner dialect, including type mapping, DDL compilation, and corresponding unit tests. However, a critical bug was identified in the visit_uuid compiler method, where calling self.visit_UUID will raise an AttributeError because that method is not defined. The reviewer provided a code suggestion to directly return the string "UUID" when native UUID is supported.
| def visit_uuid(self, type_, **kw): | ||
| if not type_.native_uuid or not self.dialect.supports_native_uuid: | ||
| return self.visit_CHAR(types.CHAR(36), **kw) | ||
| else: | ||
| return self.visit_UUID(type_, **kw) |
There was a problem hiding this comment.
Calling self.visit_UUID(type_, **kw) will raise an AttributeError because visit_UUID is not defined in SpannerTypeCompiler or its parent class GenericTypeCompiler (SQLAlchemy uses lowercase visit_uuid as the standard compilation method for UUID types). Instead of calling a non-existent method, you should directly return the string "UUID" when native UUID is supported.
| def visit_uuid(self, type_, **kw): | |
| if not type_.native_uuid or not self.dialect.supports_native_uuid: | |
| return self.visit_CHAR(types.CHAR(36), **kw) | |
| else: | |
| return self.visit_UUID(type_, **kw) | |
| def visit_uuid(self, type_, **kw): | |
| if not type_.native_uuid or not self.dialect.supports_native_uuid: | |
| return self.visit_CHAR(types.CHAR(36), **kw) | |
| else: | |
| return "UUID" |
There was a problem hiding this comment.
This function exists in GenericCompiler. https://github.com/sqlalchemy/sqlalchemy/blob/aa1a5575358d3aa14953b04dced02f4763fed2e7/lib/sqlalchemy/sql/compiler.py#L7729
| supports_identity_columns = True | ||
| supports_native_boolean = True | ||
| supports_native_decimal = True | ||
| supports_native_uuid = True |
There was a problem hiding this comment.
This (probably) makes this a breaking change. I think that applications today can use UUID as a type in their application model. SQLAlchemy will then automatically assume that they are stored as strings and convert them to UUIDs in memory. By changing the (default) value of this property, that automatic conversion is no longer being done, even though the value is still stored and returned as a string from the database, as it was (probably) created as a string column.
I think that we need to add some tests for how this behaved before this change when someone had created a model like this:
import uuid
from sqlalchemy import Column, String, Uuid
from sqlalchemy.orm import declarative_base
Base = declarative_base()
class User(Base):
__tablename__ = "users"
# Declared using SQLAlchemy 2.0's standard Uuid type
id = Column(Uuid, primary_key=True)
name = Column(String(100))The above model would probably have used a string column for the id property, and SQLAlchemy would automatically convert it to a UUID in memory. That (probably) does not happen anymore after this change.
There was a problem hiding this comment.
Thanks for raising this! I have evaluated the backward compatibility implications and decided to keep supports_native_uuid = True enabled by default for the following reasons:
1. Issue with Existing visit_uuid (CHAR(32) Syntax Error)
Previously, if a developer declared a model using SQLAlchemy 2.0's standard Uuid type:
class User(Base):
__tablename__ = "users"
id = Column(Uuid, primary_key=True)SQLAlchemy Core's default fallback compiled Uuid to CHAR(32), generating DDL:
CREATE TABLE users (
id CHAR(32) NOT NULL
) PRIMARY KEY (id)This query failed in Spanner with a syntax error because Spanner GoogleSQL does not support the CHAR data type.
2. Workaround Required Monkey-Patching or Custom TypeDecorator
Because Column(Uuid) failed out of the box, applications had to resort to monkey-patching the type compiler at startup:
# Workaround: Monkey-patch SpannerTypeCompiler to output STRING(36)
from google.cloud.sqlalchemy_spanner.sqlalchemy_spanner import SpannerTypeCompiler
SpannerTypeCompiler.visit_uuid = lambda self, type_, **kw: "STRING(36)"Or writing a custom TypeDecorator:
class UUIDString(TypeDecorator):
impl = String(36)
cache_ok = True
def process_bind_param(self, val, dialect):
return str(val) if val else None
def process_result_value(self, val, dialect):
return uuid.UUID(val) if val else None3. Direct Native UUID Support Was Missing
Until now, applications could not use Spanner's native UUID data type directly in SQLAlchemy models or table reflection (table reflection crashed with KeyError: 'UUID').
Enabling supports_native_uuid = True by default brings first-class native UUID support to the dialect:
# Generates native Spanner DDL: CREATE TABLE users (id UUID NOT NULL) PRIMARY KEY (id)
class User(Base):
__tablename__ = "users"
id = Column(Uuid, primary_key=True)If an existing application specifically requires legacy string-backed columns (STRING(36)), they can easily opt out:
- Per Column:
Column(types.Uuid(native_uuid=False), primary_key=True) - Globally on Engine/Dialect:
create_engine("spanner:///...", supports_native_uuid=False)
We've added comprehensive unit tests covering both default native UUID DDL compilation and STRING(36) fallback behavior.
I suspect that no customer will be using UUID/Uuid in SQLAlchemy
809b53f to
c755982
Compare
c755982 to
681f3de
Compare
|
@sakthivelmanii , Please could you review the failing system test? |
|
@parthea Yes. I have re-ran it. It worked |
Adds native Spanner UUID type support to SQLAlchemy dialect: