fix(sqlalchemy-spanner): handle TOKENLIST and unknown column types in introspection#16622
Conversation
There was a problem hiding this comment.
Code Review
This pull request adds support for the TOKENLIST Spanner type by mapping it to NullType and introduces a fallback mechanism in _designate_type to handle unknown types with a warning. Unit tests were also added to cover various type mapping scenarios. A review comment identified a return type inconsistency in _designate_type, where type classes were being returned instead of instances, which would lead to test failures.
| try: | ||
| return _type_map[str_repr] | ||
| except KeyError: | ||
| warnings.warn( | ||
| "Did not recognize Spanner type '%s', " | ||
| "mapping it to NullType" % str_repr | ||
| ) | ||
| return types.NullType() |
There was a problem hiding this comment.
There is an inconsistency in the return types of _designate_type. When a type is found in _type_map, the code returns the type class (e.g., types.Boolean), but the fallback returns an instance (types.NullType()).
Furthermore, the unit tests added in this PR (e.g., test_known_types and test_tokenlist_maps_to_nulltype) use isinstance(result, types.Boolean), which will fail if a class is returned instead of an instance.
To ensure consistency and fix the tests, you should instantiate the type from the map. Additionally, adding stacklevel=2 to the warning is recommended so it points to the caller of the introspection.
| try: | |
| return _type_map[str_repr] | |
| except KeyError: | |
| warnings.warn( | |
| "Did not recognize Spanner type '%s', " | |
| "mapping it to NullType" % str_repr | |
| ) | |
| return types.NullType() | |
| try: | |
| return _type_map[str_repr]() | |
| except KeyError: | |
| warnings.warn( | |
| "Did not recognize Spanner type '%s', " | |
| "mapping it to NullType" % str_repr, | |
| stacklevel=2 | |
| ) | |
| return types.NullType() |
83bf00a to
01c17c5
Compare
… in introspection Spanner full-text search uses TOKENLIST columns, but the dialect does not recognize this type — _type_map has no entry and _designate_type() crashes with KeyError on any unrecognized type string. This change: 1. Adds a first-class TOKENLIST type (TypeEngine subclass) with forward mapping (_type_map), inverse mapping (_type_map_inv), and DDL compilation (SpannerTypeCompiler.visit_TOKENLIST). This ensures schema introspection roundtrips correctly through Alembic and other DDL tools, and allows TOKENLIST columns to be referenced in SEARCH()/SCORE()/SNIPPET() queries via SQLAlchemy. 2. Adds a fallback in _designate_type() so unrecognized types return NullType with a warning instead of crashing. This future-proofs against new Spanner types. The Go client had the same gap and fixed it in googleapis/google-cloud-go#11522 (released in spanner v1.78.0), which added TOKENLIST as a first-class TypeBase. Fixes googleapis#16621
736e001 to
c2f5788
Compare
Description
SpannerDialect._designate_type()crashes withKeyError: 'TOKENLIST'when introspecting tables that haveTOKENLISTcolumns (introduced by Spanner full-text search).Root cause
_type_mapdoes not includeTOKENLIST._designate_type()performs a bare dict lookup with no fallback, so any unrecognized type crashes all introspection — not just the one column.Changes
1. First-class
TOKENLISTtype (matching the Go client's approach)Rather than mapping to
NullType(which would lose type information and roundtrip asINT64via_type_map_inv), this PR adds a properTOKENLISTclass extendingTypeEngine:_type_map["TOKENLIST"] = TOKENLIST_type_map_inv[TOKENLIST] = "TOKENLIST"SpannerTypeCompiler.visit_TOKENLIST()returns"TOKENLIST"This ensures correct roundtripping through Alembic
--autogenerateand other schema tools. It also allows TOKENLIST columns to be present in reflected models so they can be referenced inSEARCH(),SCORE(), andSNIPPET()queries via SQLAlchemy.TOKENLIST columns are generated (
Computed) andHIDDEN(excluded fromSELECT *), but they are readable by name and actively used in full-text search queries. They should be reflected in models, not filtered out.2. Unknown type fallback in
_designate_type()Added
try/except KeyErrorthat returnsNullTypewith awarnings.warn(). Future-proofs against new Spanner types being added before the dialect is updated.Prior art — Go client fix
The Go
spansqlpackage had the identical issue. The fix added TOKENLIST as a first-classTypeBaseconstant:baseTypesmap,TypeBaseenum, andSQL()method — see also the recent UUID PR which follows the same pattern.This Python PR mirrors that approach within the SQLAlchemy dialect's architecture.
Fixes #16621
Testing
Added
tests/unit/test_types.py:TestDesignateType: known types, STRING/BYTES with length, TOKENLIST →TOKENLISTtype, unknown type → NullType with warning, ARRAYTestTokenlistType: forward/inverse type map entries, type compiler roundtrip (TOKENLIST()→"TOKENLIST"), TypeEngine subclass check