FIX: dispatch output converters on integer ODBC SQL type codes (#684) - #690
Open
jahnvi480 wants to merge 4 commits into
Open
FIX: dispatch output converters on integer ODBC SQL type codes (#684)#690jahnvi480 wants to merge 4 commits into
jahnvi480 wants to merge 4 commits into
Conversation
add_output_converter documents an integer ODBC SQL type key, but dispatch keyed on the Python type in cursor.description[i][1], so integer keys silently never fired. Store per-column raw SQL type codes and dispatch on the integer code first, then Python type, then the legacy WVARCHAR catch-all. Also build the converter map for catalog metadata result sets so converters apply consistently.
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes output-converter dispatch so Connection.add_output_converter(sqltype, func) honors integer ODBC SQL type codes (pyodbc-compatible) instead of only firing for Python-type keys derived from cursor.description[i][1]. This aligns runtime behavior with the documented API and resolves the silent no-op described in #684.
Changes:
- Preserve raw ODBC SQL type codes per column and use them first when building the per-result-set output-converter dispatch map (with Python-type and legacy
SQL_WVARCHARfallback preserved). - Ensure catalog/metadata result sets (e.g.,
cursor.tables()) also build a converter map so converters apply consistently. - Update the public type surface (
connection.pydoc/signature andmssql_python.pyi) and add a regression test covering integer-key dispatch semantics and precedence.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
mssql_python/cursor.py |
Store per-column raw ODBC type codes and dispatch converters in pyodbc-compatible precedence order; build converter maps for metadata result sets. |
mssql_python/connection.py |
Broaden add_output_converter to accept Union[int, type] and correct/clarify converter semantics in the docstring. |
mssql_python/mssql_python.pyi |
Update stub signature for add_output_converter to match runtime API. |
tests/test_003_connection.py |
Add regression test covering integer SQL type keys, precedence, exact-type matching, NULL handling, string converter path, and metadata behavior. |
…ispatch test_row_output_converter_general_exception registered a converter under integer SQL type 12 (SQL_VARCHAR), which previously never fired due to the GH #684 dispatch bug. Now that integer keys dispatch correctly, the converter fires and receives UTF-16LE bytes, so the old value=="test_value" guard no longer matches. Make the converter raise unconditionally to faithfully exercise the "converter raised -> keep original value" path, and move the converter restore into finally so a failed assertion can never leak the converter onto the shared connection (which was cascading into 6 unrelated VARCHAR test failures).
Assert user-visible behavior (a catalog string column is actually passed through the registered SQL_WVARCHAR converter) instead of the private cursor._cached_converter_map cache field, which was brittle to internal refactors. tables() row string columns now come back as "CONV:..." while NULL stays None.
📊 Code Coverage Report
Diff CoverageDiff: main...HEAD, staged and unstaged changes
Summary
📋 Files Needing Attention📉 Files with overall lowest coverage (click to expand)mssql_python.pybind.logger_bridge.cpp: 59.2%
mssql_python.pybind.ddbc_bindings.h: 59.9%
mssql_python.pybind.logger_bridge.hpp: 70.8%
mssql_python.pybind.ddbc_bindings.cpp: 76.3%
mssql_python.__init__.py: 77.3%
mssql_python.row.py: 77.6%
mssql_python.ddbc_bindings.py: 79.6%
mssql_python.pybind.connection.connection_pool.cpp: 81.4%
mssql_python.pybind.connection.connection.cpp: 83.7%
mssql_python.connection.py: 84.7%🔗 Quick Links
|
…684) Adds test_execute_describe_col_exception_resets_description_and_sql_types, which patches ddbc_bindings.DDBCSQLDescribeCol to raise during execute() and asserts both self.description and self._column_sql_types are reset to None. Seeds a real SELECT first so the reset has a populated SQL-type list to clear. Covers the previously-uncovered defensive reset line in the execute() describe-failure except branch (diff coverage gap flagged by the coverage bot).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Work Item / Issue Reference
Summary
Connection.add_output_converter(sqltype, func)documents (and pyodbc accepts) aninteger ODBC SQL type code as the key — e.g.
SQL_DECIMAL,SQL_INTEGER. However,dispatch in
Cursor._build_converter_map()keyed on the Python type stored incursor.description[i][1](via_map_data_type), so integer-keyed converters were storedbut silently never fired. Only Python-type keys (e.g.
decimal.Decimal,str) worked.This makes the driver diverge from pyodbc and from its own documentation.
Root cause
The raw ODBC SQL type code (
col["DataType"]) is available when the description is builtin
_initialize_description, but it was discarded._build_converter_mapthen looked upconverters using
description[i][1](a Python type), which can never match an integer key.Fix
cursor.py_initialize_descriptionnow also storesself._column_sql_types— the raw ODBC SQLtype codes, parallel to
description(reset toNonewhen there are no columns)._build_converter_mapdispatches in pyodbc-compatible order: (1) integer SQL typecode, (2) Python type in
description[i][1], (3) the legacyWVARCHARcatch-all.This is purely additive — existing Python-type converters keep working.
_prepare_metadata_result_setnow builds the converter map so catalog metadata resultsets (
columns(),tables(), …) honor converters consistently with normal result sets.self._column_sql_types = Noneresets alongside the bareself.description = Noneassignments (execute/executemany describe-fail paths and
nextset).connection.py—add_output_convertersignature is nowsqltype: Union[int, type]; thedocstring is corrected to describe both key styles (integer key takes precedence) and the
value semantics (already-materialized Python object;
strpassed as UTF‑16LE bytes).mssql_python.pyi— stub updated to match (Union[int, type]).Exact-type dispatch
Integer keys use exact ODBC type matching, so
SQL_DECIMALandSQL_NUMERICaredistinct (they are not collapsed to
decimal.Decimal), matching pyodbc.Tests
Added
tests/test_003_connection.py::test_output_converter_integer_sql_type_key_gh684covering: integer key fires; exact-type dispatch (
SQL_INTEGERdoes not touchDECIMAL,but fires on
INT); integer-key precedence over a Python-type key; distinctDECIMALvsNUMERICconverters; NULL staysNone; theSQL_WVARCHARstring path (converter receivesUTF‑16LE bytes); and metadata result sets building a converter map.
Validation
black --check --line-length=100clean on all changed files.