FIX: dispatch output converters on integer ODBC SQL type codes (#684) by jahnvi480 · Pull Request #690 · microsoft/mssql-python

jahnvi480 · 2026-07-28T06:59:38Z

Work Item / Issue Reference

[AB#46648]

GitHub Issue: #684

Summary

Connection.add_output_converter(sqltype, func) documents (and pyodbc accepts) an
integer 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 in
cursor.description[i][1] (via _map_data_type), so integer-keyed converters were stored
but 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 built
in _initialize_description, but it was discarded. _build_converter_map then looked up
converters using description[i][1] (a Python type), which can never match an integer key.

Fix

cursor.py
- _initialize_description now also stores self._column_sql_types — the raw ODBC SQL
  type codes, parallel to description (reset to None when there are no columns).
- _build_converter_map dispatches in pyodbc-compatible order: (1) integer SQL type
  code, (2) Python type in description[i][1], (3) the legacy WVARCHAR catch-all.
  This is purely additive — existing Python-type converters keep working.
- _prepare_metadata_result_set now builds the converter map so catalog metadata result
  sets (columns(), tables(), …) honor converters consistently with normal result sets.
- Defensive self._column_sql_types = None resets alongside the bare self.description = None
  assignments (execute/executemany describe-fail paths and nextset).
connection.py — add_output_converter signature is now sqltype: Union[int, type]; the
docstring is corrected to describe both key styles (integer key takes precedence) and the
value semantics (already-materialized Python object; str passed 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_DECIMAL and SQL_NUMERIC are
distinct (they are not collapsed to decimal.Decimal), matching pyodbc.

Tests

Added tests/test_003_connection.py::test_output_converter_integer_sql_type_key_gh684
covering: integer key fires; exact-type dispatch (SQL_INTEGER does not touch DECIMAL,
but fires on INT); integer-key precedence over a Python-type key; distinct DECIMAL vs
NUMERIC converters; NULL stays None; the SQL_WVARCHAR string path (converter receives
UTF‑16LE bytes); and metadata result sets building a converter map.

Validation

black --check --line-length=100 clean on all changed files.
Output-converter suite: 14 passed (incl. the new regression test) against a live SQL Server.

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.

Copilot

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_WVARCHAR fallback 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.py doc/signature and mssql_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.

github-actions · 2026-07-28T09:57:36Z

📊 Code Coverage Report

🔥 Diff Coverage 100%	🎯 Overall Coverage 81%	📈 Total Lines Covered: `7083` out of `8689` 📁 Project: `mssql-python`

Diff Coverage

Diff: main...HEAD, staged and unstaged changes

mssql_python/connection.py (100%)
mssql_python/cursor.py (100%)

Summary

Total: 17 lines
Missing: 0 lines
Coverage: 100%

📋 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

⚙️ Build Summary	📋 Coverage Details
View Azure DevOps Build	Browse Full Coverage Report

…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).

subrata-ms · 2026-07-30T04:54:38Z

        return cursor

-    def add_output_converter(self, sqltype: int, func: Callable[[Any], Any]) -> None:
+    def add_output_converter(self, sqltype: Union[int, type], func: Callable[[Any], Any]) -> None:


Looks like Union[int, type] Is Very Broad. It seems we are now accepting any python type like list ,dict ,tuple ,set, object, ect.
Are all types actually supported?
What happens when user call/implement : add_output_converter(set, func) ?
is it the API is exposing more than it can realistically support?
Please check.

subrata-ms · 2026-07-30T05:15:16Z

    db_connection.clear_output_converters()


+def test_output_converter_integer_sql_type_key_gh684(db_connection):


As this is a API change, should we include all possible data type for testing the change?
like object, sets ect?

One Python converter applies to multiple SQL types ( CAST(10.5 AS DECIMAL(10,2)),3 CAST(20.5 AS NUMERIC(10,2)),4 CAST(30.5 AS MONEY),5 CAST(40.5 AS SMALLMONEY))

Datetime converter registration

Bytes converter registration

NULL handling

Mixed-result-set validation ( int + decimal + str + datetime)

API testing for edge cases -
a. conn.add_output_converter(set, conv)
b. conn.add_output_converter(object, converter)
c. custom instance ( class MyDecimal(decimal.Decimal))

Cached converter map behavior.Fetch multiple rows (cursor.fetchall()) and Verify converter selection remains correct after converter-map caching.

test coverage for MONEY/SMALLMONEY

Copilot AI review requested due to automatic review settings July 28, 2026 06:59

github-actions Bot added the pr-size: medium Moderate update size label Jul 28, 2026

Copilot started reviewing on behalf of jahnvi480 July 28, 2026 07:00 View session

Copilot AI reviewed Jul 28, 2026

View reviewed changes

Comment thread tests/test_003_connection.py Outdated

jahnvi480 mentioned this pull request Jul 28, 2026

BUG: SQL_WVARCHAR output converter is applied as an unconditional catch-all to non-string columns (INT/DECIMAL/DATE) #691

Open

jahnvi480 added 2 commits July 28, 2026 15:21

bewithgaurav requested review from subrata-ms and sumitmsft July 29, 2026 09:17

subrata-ms reviewed Jul 30, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

FIX: dispatch output converters on integer ODBC SQL type codes (#684) - #690

FIX: dispatch output converters on integer ODBC SQL type codes (#684)#690
jahnvi480 wants to merge 4 commits into
mainfrom
jahnvi/fix-684-output-converter-int-key

jahnvi480 commented Jul 28, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

github-actions Bot commented Jul 28, 2026 •

edited

Loading

🔥 Diff Coverage

100%

🎯 Overall Coverage

81%

Uh oh!

subrata-ms Jul 30, 2026 •

edited

Loading

Uh oh!

subrata-ms Jul 30, 2026 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		db_connection.clear_output_converters()


		def test_output_converter_integer_sql_type_key_gh684(db_connection):

Uh oh!

Conversation

jahnvi480 commented Jul 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Work Item / Issue Reference

Summary

Root cause

Fix

Exact-type dispatch

Tests

Validation

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

github-actions Bot commented Jul 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

📊 Code Coverage Report

🔥 Diff Coverage

100%

🎯 Overall Coverage

81%

Diff Coverage

Diff: main...HEAD, staged and unstaged changes

Summary

📋 Files Needing Attention

🔗 Quick Links

Uh oh!

subrata-ms Jul 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

subrata-ms Jul 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jahnvi480 commented Jul 28, 2026 •

edited

Loading

github-actions Bot commented Jul 28, 2026 •

edited

Loading

subrata-ms Jul 30, 2026 •

edited

Loading

subrata-ms Jul 30, 2026 •

edited

Loading