FIX: Cursor.description returns dual-compatible SqlTypeCode type codes#355
FIX: Cursor.description returns dual-compatible SqlTypeCode type codes#355dlevy-msft-sql wants to merge 1 commit intomicrosoft:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes issue #352 by implementing two main improvements: (1) correcting cursor.description to return SQL type codes (integers) instead of Python type objects, ensuring DB-API 2.0 specification compliance, and (2) adding support for SQL Server spatial data types (geography, geometry, hierarchyid) by handling the SQL_SS_UDT type code (-151).
Key changes:
- Fixed
cursor.description[i][1]to return SQL type integer codes (e.g., 4, -9) instead of Python types (int, str) per DB-API 2.0 spec - Added SQL_SS_UDT (-151) support for SQL Server User-Defined Types including spatial data types
- Updated output converter lookup to use SQL type codes consistently throughout the codebase
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| mssql_python/cursor.py | Changed cursor.description to return SQL type codes instead of Python types; added _column_metadata storage; updated _build_converter_map to use SQL type codes; added mappings for UDT, XML, DATETIME2, SMALLDATETIME types |
| mssql_python/constants.py | Added SQL_SS_UDT = -151 constant for SQL Server User-Defined Types |
| mssql_python/pybind/ddbc_bindings.cpp | Added C++ constants for SQL_SS_UDT, SQL_DATETIME2, SQL_SMALLDATETIME; implemented UDT handling in SQLGetData_wrap, FetchBatchData, FetchMany_wrap, and FetchAll_wrap for LOB streaming |
| tests/test_004_cursor.py | Added lob_wvarchar_column to test schema; updated test_cursor_description to expect SQL type codes; added comprehensive geography type test suite (14 new tests); separated LOB and non-LOB fetch tests; fixed output converter test for UTF-16LE encoding |
| tests/test_003_connection.py | Simplified converter integration tests to use SQL type constants directly instead of dynamic type detection |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
3f52dd7 to
646ef04
Compare
📊 Code Coverage Report
Diff CoverageDiff: main...HEAD, staged and unstaged changes
Summary
mssql_python/connection.pyLines 1003-1011 mssql_python/cursor.pyLines 1015-1023 Lines 1362-1370 Lines 2430-2438 mssql_python/type_code.pyLines 87-95 Lines 111-116 📋 Files Needing Attention📉 Files with overall lowest coverage (click to expand)mssql_python.pybind.logger_bridge.hpp: 58.8%
mssql_python.pybind.logger_bridge.cpp: 59.2%
mssql_python.pybind.ddbc_bindings.cpp: 69.3%
mssql_python.pybind.ddbc_bindings.h: 69.7%
mssql_python.pybind.connection.connection.cpp: 75.3%
mssql_python.row.py: 79.5%
mssql_python.ddbc_bindings.py: 79.6%
mssql_python.pybind.connection.connection_pool.cpp: 79.6%
mssql_python.connection.py: 84.2%
mssql_python.cursor.py: 84.9%🔗 Quick Links
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 17 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 17 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 17 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 17 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Add SQL_SS_XML (-152) constant to Python constants.py (was incorrectly using SQL_XML = 241) - Add SQL_SS_TIME2 (-154) constant for SQL Server TIME(n) type - Update cursor type map to use SQL_SS_XML and add SQL_SS_TIME2 mapping - Add sync comment in C++ to prevent future constant drift Constants verified against Microsoft Learn ODBC documentation: - SQL_SS_TIME2: -154 (SQLNCLI.h) - SQL_SS_TIMESTAMPOFFSET: -155 (SQLNCLI.h) - SQL_SS_XML: -152 (SQL Server ODBC driver) - SQL_SS_UDT: -151 (SQL Server ODBC driver) Addresses Copilot review feedback on PR microsoft#355
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 13 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 13 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
48a5042 to
a61d87e
Compare
6002dd0 to
f2d2c33
Compare
f2d2c33 to
e734a2f
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def __eq__(self, other): | ||
| """Compare equal to both Python types and SQL integer codes.""" | ||
| if isinstance(other, type): | ||
| return self.python_type == other | ||
| if isinstance(other, int): |
There was a problem hiding this comment.
SqlTypeCode.__eq__ only guarantees SqlTypeCode(x) == x, but the reverse (x == SqlTypeCode(x)) will typically be False because int.__eq__ doesn’t defer to the RHS. This also breaks common patterns like type_code in (SQL_WVARCHAR, SQL_VARCHAR) (membership compares int == SqlTypeCode). If the goal is “dual-compatible” with integer codes, consider making SqlTypeCode an int subclass (via __new__) and layering the Python-type comparison on top; also be careful that bool is an int (so SqlTypeCode(1) == True would currently be True).
| if isinstance(sqltype, SqlTypeCode): | ||
| sqltype = sqltype.type_code | ||
| with self._converters_lock: | ||
| if sqltype in self._output_converters: | ||
| del self._output_converters[sqltype] | ||
| # Pass to the underlying connection if native implementation supports it |
There was a problem hiding this comment.
remove_output_converter() unwraps SqlTypeCode to its integer code, but doesn’t remove a converter that was registered under the corresponding Python type key (e.g., add_output_converter(str, fn) then remove_output_converter(desc[i][1])). Since get_output_converter() includes a Python-type fallback for SqlTypeCode, removal should be symmetric—consider deleting both keys when a SqlTypeCode is provided.
| #define SQL_SS_XML (-152) | ||
| #define SQL_SS_UDT (-151) // SQL Server User-Defined Types (geometry, geography, hierarchyid) | ||
| #ifndef SQL_DATETIME2 | ||
| #define SQL_DATETIME2 (42) | ||
| #endif |
There was a problem hiding this comment.
SQL_SS_UDT is defined unconditionally; if the build environment’s ODBC headers already define it, this can cause macro redefinition warnings/errors. Mirror the SQL_DATETIME2 / SQL_SMALLDATETIME pattern by wrapping the SQL_SS_UDT define in an #ifndef SQL_SS_UDT guard (same applies to any other SQL Server-specific defines added here).
| # If all entries are None, return None so that Row can fall back to the | ||
| # non-optimized path, preserving legacy behavior and fallbacks. | ||
| if not any(converter_map): | ||
| return None | ||
|
|
There was a problem hiding this comment.
In _build_converter_map, returning None when all entries are None forces Row to fall back to the legacy per-cell lookup path (because converter_map is falsy), which adds unnecessary get_output_converter() calls for every row/column when converters are registered but don’t apply to this result set. Prefer returning the full converter_map list (even if it’s all None) or adjust Row to distinguish between “no optimized map” vs “optimized map with no converters” (e.g., check converter_map is not None).
| # If all entries are None, return None so that Row can fall back to the | |
| # non-optimized path, preserving legacy behavior and fallbacks. | |
| if not any(converter_map): | |
| return None | |
| # Always return the constructed converter_map list, even if all entries are None. | |
| # This allows callers to distinguish between "no optimized map" (None) and | |
| # "optimized map present but no converters apply" (list of Nones), avoiding | |
| # unnecessary fallback to legacy per-cell lookup paths. |
| def __hash__(self): | ||
| """ | ||
| SqlTypeCode is intentionally unhashable because __eq__ allows | ||
| comparisons to both Python types and integer SQL codes, and | ||
| there is no single hash value that can be consistent with both. | ||
| """ | ||
| raise TypeError( | ||
| "SqlTypeCode is unhashable. Use int(type_code) or type_code.type_code " | ||
| "as a dict key instead. Example: {int(desc[1]): handler}" | ||
| ) |
There was a problem hiding this comment.
This method always raises TypeError - use hash = None instead.
| def __hash__(self): | |
| """ | |
| SqlTypeCode is intentionally unhashable because __eq__ allows | |
| comparisons to both Python types and integer SQL codes, and | |
| there is no single hash value that can be consistent with both. | |
| """ | |
| raise TypeError( | |
| "SqlTypeCode is unhashable. Use int(type_code) or type_code.type_code " | |
| "as a dict key instead. Example: {int(desc[1]): handler}" | |
| ) | |
| # Intentionally unhashable: __eq__ supports comparison to both Python types | |
| # and integer SQL codes, so there is no single hash value that can remain | |
| # consistent with all equality semantics. Use int(type_code) or | |
| # type_code.type_code as a dict key instead (for example: {int(desc[1]): handler}). | |
| __hash__ = None |
Work Item / Issue Reference
Summary
Fixes
cursor.descriptionto returnSqlTypeCodeinstances as type codes instead of raw Python types.SqlTypeCodeis dual-compatible: it compares equal to both SQL integer type codes (DB-API 2.0) and Python types (pandas/polars compatibility), so existing code continues to work unchanged.The original issue was reported when
polars pl.read_database()failed with:Core change — SqlTypeCode class (
type_code.py):SqlTypeCodewraps an ODBC integer type code with.nameand.python_typeattributes__eq__matches bothintvalues and Pythontypeobjects, sodesc[1] == stranddesc[1] == -9both work__int__returns the raw SQL type code for DB-API 2.0 consumers__hash__raisesTypeError) because equality spans multiple hash domains — error message guides users toint(type_code)for dict keysCursor internals:
_column_metadatastores per-column(col_name, sql_type, col_size, precision, nullable)tuples fromDDBCSQLDescribeCol_build_converter_maprefactored to use stored metadata instead of re-querying_initialize_descriptionbuildsSqlTypeCodefrom stored metadataConstants and C++ bindings:
SQL_SS_TIME2,SQL_SS_UDT,SQL_SS_XMLconstants#defineguards inddbc_bindings.cppforSQL_DATETIME2andSQL_SMALLDATETIMEConverter API:
add_output_converter,get_output_converter,remove_output_converterinconnection.pyacceptUnion[int, SqlTypeCode, type]isinstance(sqltype, SqlTypeCode)to unwrap to.type_codebefore forwarding to native layerTesting:
SqlTypeCodeunit tests (equality, hashing, repr, pandas compatibility)SqlTypeCode-aware assertionstest_018_polars_integration.pywith 5 integration tests (basic types, dates, nulls, large results, all common types)Files Changed
mssql_python/type_code.pySqlTypeCodeclass extracted to own modulemssql_python/cursor.py_column_metadatatracking,_build_converter_maprefactor,SqlTypeCodeintegrationmssql_python/connection.pySqlTypeCodeviaisinstancemssql_python/constants.pySQL_SS_TIME2,SQL_SS_UDT,SQL_SS_XML)mssql_python/pybind/ddbc_bindings.cpp#defineguardsmssql_python/__init__.pySqlTypeCodemssql_python/mssql_python.pyiSqlTypeCodeCHANGELOG.mdtests/test_002_types.pySqlTypeCodeteststests/test_004_cursor.pytests/test_018_polars_integration.pyBreaking Changes
None. Full backward compatibility maintained:
cursor.description[i][1] == strstill works (Python type comparison)cursor.description[i][1] == -9also works (SQL type code comparison)int(cursor.description[i][1])returns the raw SQL integer type code