FIX: bind Decimal as SQL_NUMERIC regardless of value - #742
FIX: bind Decimal as SQL_NUMERIC regardless of value#742Gaurav Sharma (bewithgaurav) wants to merge 5 commits into
Conversation
The standard execute path chose a Decimal's bind type from its value: anything in the MONEY/SMALLMONEY range was sent 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 just 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 APD record number in SQLSetDescField 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. Scoped to the single execute() path; executemany still string-binds decimals (GH-503) and is a separate follow-up. (GH-740) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🔵 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
Decimalparameters asSQL_NUMERICinDetectParamTypes(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.
📊 Code Coverage Report
Diff CoverageDiff: main...HEAD, staged and unstaged changes
Summary
mssql_python/pybind/ddbc_bindings.cppLines 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 SQL_NUMERIC_STRUCT from the Decimal object. Store as a pybind11-castable | ||
| // object in the param list so BindParameters can extract it as NumericData. | ||
| // Bind every finite Decimal as SQL_NUMERIC using its own precision and scale, |
There was a problem hiding this comment.
The identical money→VARCHAR shortcut still lives in the Python _map_sql_type, which this PR doesn't touch, and that path is still reachable from a real execute(). If a caller uses setinputsizes() for only some positions, the un-sized params fall back to _map_sql_type and GH-740 reproduces again. executemany() has the same problem for WHERE numeric_col = ?.
Can we either mirror this change in _map_sql_type or explicitly scope/track the remaining path?
| SQL_NUMERIC using its own precision and scale, regardless of value. Binding no longer | ||
| depends on whether the value falls in the MONEY/SMALLMONEY range, so an in-range value | ||
| compared against a smaller numeric column returns no match instead of a varchar->numeric | ||
| overflow (GH-740). executemany still string-binds Decimals (SQL_VARCHAR) to preserve |
There was a problem hiding this comment.
Since native binding is now NUMERIC, test_023_execute_path_parity.py::test_money_range_decimal_binds_wide (L515) is misleading — it's a native-path test whose docstring says money-range decimals bind as text, but it only asserts a value round-trip, so it still passes even though the C type changed. The file's whole "native == _map_sql_type reference" premise is now violated for this case. Worth updating it here to assert the native base type is numeric so we keep real parity coverage instead of a green-but-vacuous test.
| db_connection.commit() | ||
|
|
||
|
|
||
| def test_gh740_numeric_param_not_in_first_position(cursor, db_connection): |
There was a problem hiding this comment.
These use a NULL as the earlier param. Since the old bug also clobbered whatever was bound at record 1, a stronger guard would put a non-null value first (e.g. (some_int_or_str, Decimal(...))) and assert that first value round-trips intact - that pins the collateral corruption of position 1, not just the numeric's own misplacement.
test_money_range_decimal_binds_wide only round-tripped the value, so it stayed green after the native C type changed to NUMERIC. Assert the declared base type via sql_variant, and note that _map_sql_type still text-binds money-range Decimals to protect executemany's string binding. (GH-740) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The position test used a NULL first param, which masked the old record-1 bug (record 1 held no data). Use a non-null value first and assert it round-trips intact, so the test pins the collateral corruption of the earlier parameter, not just the numeric's own misplacement. Verified it fails against the pre-fix binder. (GH-740) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
| // bind type from the value alone, ignoring the target column, so an in-range value | ||
| // compared against a smaller numeric column triggered a server-side varchar->numeric | ||
| // overflow instead of simply not matching (GH-740). | ||
| info.paramSQLType = SQL_NUMERIC; |
There was a problem hiding this comment.
This scopes the fix to the native execute path. When setinputsizes() is active but supplies fewer entries than there are parameters (which only raises a warning, not an error), any Decimal past the end of the list falls through to the legacy _map_sql_type (cursor.py:1313-1316), which still binds MONEY/SMALLMONEY-range Decimals as VARCHAR (cursor.py:796-818) — so GH-740 can still reproduce there.
Example:
cur.setinputsizes([(SQL_INTEGER, 0, 0)]) # 1 entry
cur.execute("SELECT ... WHERE v = ?", [5, Decimal("12345.6789")]) # 2 params
# the Decimal at index 1 is uncovered -> _map_sql_type -> bound as VARCHAR -> overflowCan we close this so the fix holds for every execute() path? e.g. make uncovered execute-time Decimals use numeric binding in the legacy path too (without changing executemany's GH-503 string binding), and add a partial-setinputsizes regression test. If it's intentionally out of scope, please add a short note here and in the PR description and file a follow-up so it's explicit.
Work Item / Issue Reference
Summary
The standard execute path chose a
Decimal's bind type from its value, sendinganything 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 notmatching. Bind every finite
DecimalasSQL_NUMERICwith its own precision andscale, 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.
Scoped to the single
execute()path;executemanystill string-binds decimals(GH-503) and is a separate follow-up.