-
Notifications
You must be signed in to change notification settings - Fork 36
FIX: Cursor.describe invalid data #355
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
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 46-54 Lines 1016-1024 mssql_python/cursor.pyLines 130-138 Lines 148-156 Lines 1122-1130 Lines 1469-1477 Lines 2537-2545 📋 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.5%
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.__init__.py: 84.9%
mssql_python.cursor.py: 85.2%🔗 Quick Links
|
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
Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
mssql_python/connection.py:952
- The
add_output_converterdocstring says non-NULL values passed to converters “will be a bytes object”, but the Row conversion logic only encodesstrto UTF-16LE bytes; other types (e.g.,int,decimal.Decimal,datetime) are passed through as their native Python objects. Please update the docstring to reflect the actual value types converters may receive (bytes for strings/binary, native objects otherwise).
Args:
sqltype (int, SQLTypeCode, or type): The integer SQL type value to convert, which can be
one of the defined standard constants (e.g. SQL_VARCHAR) or a
database-specific value (e.g. -151 for the SQL Server 2008
geometry data type). Also accepts SQLTypeCode objects (from
cursor.description) or Python types (e.g., str, int) for
backward compatibility.
func (callable): The converter function which will be called with a single parameter,
the value, and should return the converted value. If the value is NULL
then the parameter passed to the function will be None, otherwise it
will be a bytes object.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…UDT against SQL_NO_TOTAL, update hash test
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
Copilot reviewed 10 out of 10 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.
saurabh500
left a comment
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.
Looks pretty good to me.
Do we want to consider doing something about testing with the stable release of libraries which are using the driver?
saurabh500
left a comment
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.
Looks pretty good to me.
- Improve __hash__ error message with helpful guidance for dict key usage - Move spatial tests (geography/geometry/hierarchyid) to test_017_spatial_types.py - Add binary parameter round-trip tests for geography and geometry - Update type stub and unit test for new hash behavior
This PR is already way larger than it probably should have been. I'd do a PR for each library test once the libraries to test with are defined. |
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
Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (2)
tests/test_004_cursor.py:1
- Changed from catching generic
Exceptionto specificmssql_python.Error. This is more precise and follows best practices for error handling in tests.
"""
tests/test_004_cursor.py:1
- The date '2025-11-26' is in the future. Consider using a past date or current date to avoid potential timezone-related test failures.
"""
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Improve __hash__ error message with helpful guidance for dict key usage - Move spatial tests (geography/geometry/hierarchyid) to test_017_spatial_types.py - Add binary parameter round-trip tests for geography and geometry - Update type stub and unit test for new hash behavior
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
Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (3)
tests/test_004_cursor.py:1
- This change corrects the exception type from a generic
Exceptiontomssql_python.Error, which is more specific and accurate for database errors. This is already fixed in the updated code.
"""
tests/test_004_cursor.py:13063
- Simplified cleanup by removing unnecessary try-except block around DROP TABLE statement. This is already fixed in the updated code.
cursor.execute(f"DROP TABLE IF EXISTS {table_name}")
tests/test_004_cursor.py:1
- Added proper encoding handling for string comparison in output converter test. The driver passes string values as UTF-16LE encoded bytes to output converters, so the test now correctly encodes the comparison value. This is already fixed in the updated code.
"""
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
SqlTypeCode should be used instead of SQLTypeCode. I am assuming this is a public type and the naming would matter more. PascalCase for Python classes is recommended and applicable to acronyms also. |
Work Item / Issue Reference
Summary
This PR fixes
cursor.descriptionto properly return SQL type codes per DB-API 2.0 specification, while maintaining backwards compatibility with libraries like pandas that compare against Python types.Key Changes
New
SQLTypeCodeclass - A dual-compatible type code that compares equal to both:desc[1] == -9) for DB-API 2.0 compliancedesc[1] == str) for pandas/library compatibilitySQL Server spatial type support - Added handling for:
geography(e.g., GPS coordinates, regions)geometry(e.g., shapes, planar coordinates)hierarchyid(e.g., org charts, tree structures)ODBC 3.x date/time type codes - Added support for:
SQL_TYPE_DATE(91)SQL_TYPE_TIME(92)SQL_TYPE_TIMESTAMP(93)SQL_TYPE_TIMESTAMP_WITH_TIMEZONE(95)Hash/equality contract fix - Made
SQLTypeCodeunhashable since__eq__compares to multiple types with different hash valuesFiles Changed
mssql_python/cursor.pySQLTypeCodeclass, updated_initialize_description()mssql_python/constants.pySQL_SS_UDT,SQL_SS_TIME2,SQL_SS_XMLconstantsmssql_python/pybind/ddbc_bindings.cppmssql_python/__init__.pySQLTypeCodemssql_python/mssql_python.pyiSQLTypeCodetests/test_002_types.pySQLTypeCodeunit teststests/test_004_cursor.pyUsage Examples
SQLTypeCode - Dual Compatibility
Spatial Types
Testing
_column_metadataSQLTypeCodeunit testsCopilot Review Comments Addressed
_type_map__hash__ = None(unhashable)geo_colBreaking Changes
None. The
SQLTypeCodeclass maintains full backwards compatibility:cursor.description[i][1] == strstill works ✅cursor.description[i][1] == -9also works ✅