FIX: bind Decimal as SQL_NUMERIC regardless of value

[Gaurav Sharma (bewithgaurav)]

Work Item / Issue Reference

GitHub Issue: #740

---

Summary

The standard execute path chose a Decimal's bind type from its value, sending
anything in the MONEY/SMALLMONEY range as a formatted VARCHAR. Comparing such a
value against a smaller numeric column made SQL Server convert varchar to numeric
and overflow, so WHERE v = ? raised an arithmetic overflow instead of simply not
matching. Bind every finite Decimal as SQL_NUMERIC with its own precision and
scale, matching pyodbc.

Removing the shortcut surfaced a second bug: the numeric parameter's descriptor
record number was hardcoded to 1, so a numeric parameter in any position other than
the first wrote its precision/scale onto the wrong record and the driver raised
"Numeric value out of range". Use the parameter's own 1-based position.

The fix covers both execute() routes: the native C++ detection path, and the Python
legacy path reached when setinputsizes() covers fewer positions than parameters (an
uncovered money-range Decimal there previously fell back to the VARCHAR shortcut and
overflowed). executemany is intentionally left on its batch VARCHAR string binding
(GH-503), so a money-range Decimal compared through executemany can still overflow;
that is a tangential finding filed separately as #745.

[Copilot]

🔵 Needs a closer look

It changes native ODBC parameter binding behavior (descriptor manipulation and Decimal typing), which warrants final human review despite good regression coverage.

Pull request overview

This PR adjusts the driver’s execute() fast path parameter detection/binding so Python Decimal values are always bound as SQL_NUMERIC (with derived precision/scale), avoiding SQL Server’s server-side varchar→numeric conversion overflow behavior seen when Decimals were previously string-bound in MONEY/SMALLMONEY ranges, and fixes descriptor-record selection for non-first numeric parameters.

Changes:

• Bind all finite Decimal parameters as SQL_NUMERIC in DetectParamTypes (removing the MONEY/SMALLMONEY VARCHAR shortcut).
• Fix numeric APD descriptor record selection to use the parameter’s own 1-based position instead of hardcoding record 1.
• Add regression tests covering GH-740 scenarios (overflow avoidance, non-first-position numeric params, multiple numerics, boundary round-trips, re-exec with changing precision/scale).

File summaries

File	Description
tests/test_020_money_smallmoney.py	Updates module-level behavior description and adds GH-740 regression tests for Decimal numeric binding and descriptor record handling.
mssql_python/pybind/py_type_cache.hpp	Removes cached MONEY/SMALLMONEY boundary Decimal objects no longer needed after eliminating range-based binding.
mssql_python/pybind/param_detect.hpp	Removes MONEY/SMALLMONEY range detection/string-binding; always constructs NumericData and sets SQL_NUMERIC binding for finite Decimals.
mssql_python/pybind/ddbc_bindings.cpp	Fixes numeric APD descriptor record number to match the actual 1-based parameter index when setting precision/scale/data ptr.

Review details

• Files reviewed: 4/4 changed files
• Comments generated: 1
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[github-actions]

📊 Code Coverage Report

🔥 Diff Coverage 60%

🎯 Overall Coverage 82%

📈 Total Lines Covered: 7772 out of 9444 📁 Project: mssql-python

---

Diff Coverage

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

• mssql_python/cursor.py (100%)
• mssql_python/pybind/ddbc_bindings.cpp (42.9%): Missing lines 879-881,919

Summary

• Total: 10 lines
• Missing: 4 lines
• Coverage: 60%

mssql_python/pybind/ddbc_bindings.cpp

Lines 875-885

  875             // The APD record number is the 1-based parameter position, matching the
  876             // SQLBindParameter call above. It was previously hardcoded to 1, so a
  877             // SQL_C_NUMERIC parameter in any position other than the first had its
  878             // precision/scale/data pointer written onto record 1 instead of its own.
! 879             // The driver then read the numeric struct with the wrong descriptor and
! 880             // raised "Numeric value out of range" (GH-740).
! 881             const SQLSMALLINT descRecNum = static_cast<SQLSMALLINT>(paramIndex + 1);
  882             SQLHDESC hDesc = nullptr;
  883             rc = SQLGetStmtAttr_ptr(hStmt, SQL_ATTR_APP_PARAM_DESC, &hDesc, 0, NULL);
  884             if (!SQL_SUCCEEDED(rc)) {
  885                 LOG("BindParameters: SQLGetStmtAttr(SQL_ATTR_APP_PARAM_DESC) "

---

Lines 915-923

  915                     paramIndex, rc);
  916                 return rc;
  917             }
  918 
! 919             rc = SQLSetDescField_ptr(hDesc, descRecNum, SQL_DESC_DATA_PTR,
  920                                      reinterpret_cast<SQLPOINTER>(numericPtr), 0);
  921             if (!SQL_SUCCEEDED(rc)) {
  922                 LOG("BindParameters: SQLSetDescField(SQL_DESC_DATA_PTR) failed "
  923                     "for param[%d] - SQLRETURN=%d",

---

---

📋 Files Needing Attention

📉 Files with overall lowest coverage (click to expand)

mssql_python.pybind.logger_bridge.cpp: 58.9%
mssql_python.pybind.ddbc_bindings.h: 61.5%
mssql_python.pybind.logger_bridge.hpp: 70.8%
mssql_python.pybind.ddbc_bindings.cpp: 75.6%
mssql_python.__init__.py: 77.6%
mssql_python.row.py: 77.6%
mssql_python.pybind.connection.connection_pool.cpp: 81.6%
mssql_python.pybind.connection.connection.cpp: 84.4%
mssql_python.logging.py: 85.5%
mssql_python.connection.py: 85.9%

---

🔗 Quick Links

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

[Jahnvi Thakkar (jahnvi480)]

This is a behavioral change on the wire (money-range Decimals now bind NUMERIC, not VARCHAR) - bare SELECT ? returns Decimal not str, sql_variant stores numeric, and since NUMERIC outranks MONEY/VARCHAR in type precedence, WHERE money_or_varchar_col = ? can flip an index seek to a CONVERT_IMPLICIT scan. Worth a CHANGELOG entry.

[Sumit Sarabhai (sumitmsft)]

Reviewed the PR for correctness, security, reliability, performance, test coverage, repository conventions, and applicable architecture and design specifications. No actionable issues were identified. The implementation is consistent with repository standards and the applicable approved design requirements.

The earlier finding on the legacy execute path (an uncovered money-range Decimal binding as VARCHAR when setinputsizes is shorter than the parameter list) is fully addressed here via decimal_as_numeric=True, with a dedicated regression test in tests/test_023_execute_path_parity.py. The executemany residual is disclosed in the description and tracked as a separate follow-up (#745).

Recommendation: Approve