FIX: bind executemany money-range Decimals as SQL_NUMERIC - #752
FIX: bind executemany money-range Decimals as SQL_NUMERIC#752VyrnSynx (vyrnsynx) wants to merge 1 commit into
Conversation
executemany auto-detect skipped the money-range VARCHAR shortcut by deriving a batch-wide NUMERIC precision/scale, matching execute() so comparisons against smaller numeric columns no longer overflow. setinputsizes DECIMAL/NUMERIC string binding is unchanged. Fixes microsoft#745
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
|
VyrnSynx (@vyrnsynx) please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.
Contributor License AgreementContribution License AgreementThis Contribution License Agreement (“Agreement”) is agreed to by the party signing below (“You”),
|
There was a problem hiding this comment.
🟡 Changes recommended
The new batch precision/scale path needs stricter validation and the current executemany numeric override still conflates numeric precision with string buffer sizing in a way that can produce invalid SQL Server precision (>38) for some Decimal shapes.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR fixes the remaining executemany() auto-detection path where MONEY/SMALLMONEY-range Decimal values could still be bound as SQL_VARCHAR, causing SQL Server to overflow when comparing against smaller numeric/decimal columns. The change aligns executemany() with the already-fixed execute() behavior by binding auto-detected Decimal columns as SQL_NUMERIC using a batch-wide precision/scale, while still sending values as SQL_C_CHAR strings.
Changes:
- Update
executemany()auto-detect to bindDecimalcolumns asSQL_NUMERIC(skipping the MONEY-range VARCHAR shortcut) and compute a batch-wide(precision, scale). - Add unit/integration tests covering the GH-745 overflow regression and mixed-sign batch behavior.
- Document the behavior change in the changelog and update explanatory test/module docs.
File summaries
| File | Description |
|---|---|
mssql_python/cursor.py |
Adds batch-wide Decimal precision/scale derivation and applies it to executemany() auto-detect numeric binding. |
tests/test_020_money_smallmoney.py |
Adds DB integration coverage for GH-745 (no overflow on executemany comparisons; mixed-sign batch still works). |
tests/test_004_cursor.py |
Adds unit tests for batch precision/scale derivation and verifies executemany binds money-range Decimals as SQL_NUMERIC. |
CHANGELOG.md |
Records GH-745 fix and clarifies setinputsizes behavior remains unchanged. |
Review details
Suppressed comments (1)
mssql_python/cursor.py:2729
- executemany’s SQL_NUMERIC/SQL_DECIMAL override reuses ParamInfo.columnSize both as the numeric precision (passed as cbColDef to SQLBindParameter) and as the max SQL_C_CHAR buffer length. Setting columnSize to max_decimal_len (which includes sign and decimal point) can push the declared numeric precision above SQL Server’s max 38 (e.g., scale-38 values format to 40 chars), causing bind failures unrelated to the actual numeric precision/scale. Consider decoupling numeric precision from string buffer sizing (e.g., compute buffer sizes from actual encoded string lengths in BindParameterArray when SQLType is NUMERIC/DECIMAL but keep cbColDef=precision<=38).
# Ensure columnSize also accommodates the longest string form
# (mixed-sign batches, GH-557).
if max_decimal_len > paraminfo.columnSize:
paraminfo.columnSize = max_decimal_len
- 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.
| max_scale = 0 | ||
| max_int_digits = 0 | ||
| found = False | ||
| for value in column: | ||
| if not isinstance(value, decimal.Decimal): | ||
| continue | ||
| try: | ||
| precision, scale = self._decimal_sql_precision_scale(value) | ||
| except ValueError: | ||
| continue | ||
| found = True | ||
| max_scale = max(max_scale, scale) | ||
| max_int_digits = max(max_int_digits, precision - scale) | ||
| if not found: | ||
| return 0, 0 | ||
| return max(max_int_digits + max_scale, 1), max_scale |
Work Item / Issue Reference
Summary
executemanyauto-detect still used the MONEY/SMALLMONEY-range VARCHAR shortcut after #742 fixedexecute(). A money-rangeDecimalcompared against a smaller numeric column could still overflow on that path.This change binds auto-detected Decimal columns as
SQL_NUMERICwith a batch-wide precision/scale (values still go through the existingSQL_C_CHARstring conversion). ThesetinputsizesDECIMAL/NUMERIC string path (GH-503) and mixed-sign VARCHAR sizing (GH-557) are left alone.Fixes #745