FIX: bind executemany money-range Decimals as SQL_NUMERIC

[VyrnSynx (vyrnsynx)]

Work Item / Issue Reference

GitHub Issue: #745

---

Summary

executemany auto-detect still used the MONEY/SMALLMONEY-range VARCHAR shortcut after #742 fixed execute(). A money-range Decimal compared against a smaller numeric column could still overflow on that path.

This change binds auto-detected Decimal columns as SQL_NUMERIC with a batch-wide precision/scale (values still go through the existing SQL_C_CHAR string conversion). The setinputsizes DECIMAL/NUMERIC string path (GH-503) and mixed-sign VARCHAR sizing (GH-557) are left alone.

Fixes #745

[azure-pipelines]

Azure Pipelines:
There may be pipelines that require an authorized user to comment /azp run to run.

[Copilot]

🟡 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 bind Decimal columns as SQL_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.

[Sumit Sarabhai (sumitmsft)]

Reviewed the change that routes executemany auto-detected Decimal columns to SQL_NUMERIC (GH-745). The core approach is sound and the new _decimal_sql_precision_scale / _batch_decimal_precision_scale helpers correctly derive a batch-wide precision/scale (and fix a latent sample-only sizing gap). Verified the precision/scale math matches _get_numeric_data, and black --check passes.

One blocking issue: the retained max_decimal_len override inside the NUMERIC block sets the NUMERIC precision from a formatted string length, which can push precision past 38 and break near-max-precision money-range Decimals that previously worked. Two smaller reliability points below.

Recommendation: Request changes.

[Sumit Sarabhai (sumitmsft)]

For a SQL_NUMERIC param, columnSize is the numeric precision passed to SQLBindParameter (ddbc_bindings.cpp, 6th arg), not a string length. The max_decimal_len > columnSize override just below counts the decimal point and sign, so it sets precision to batch_precision + 1/2. Now that money-range Decimals flow into this NUMERIC block, a full-scale value like Decimal("1E-38") or Decimal("0."+"1"*38) becomes NUMERIC(40,38) and the driver rejects it (max precision 38), a regression from the prior VARCHAR path and inconsistent with the execute() fix (GH-740). batch_precision already sizes precision correctly; the string-length need is handled separately in the SQL_VARCHAR block below. Please drop this override in the NUMERIC branch and add an executemany test for Decimal("1E-38") into NUMERIC(38,38).

[Sumit Sarabhai (sumitmsft)]

batch_precision is applied to columnSize without a <=38 check. The sample-level guard in _map_sql_type doesn't cover the whole batch, so a mixed batch (many integer digits in one row, many fractional in another) can produce batch_precision > 38 even when each value is individually storable, and the driver then fails with an opaque message. Consider raising the existing "precision too high" ValueError when batch_precision > 38. Don't clamp, since that would silently truncate.

[Sumit Sarabhai (sumitmsft)]

Deriving decimal_as_numeric from just the sample type forces the whole column onto the NUMERIC/string-decimal path. A heterogeneous column (Decimal sample plus a stray non-numeric value) that previously bound as VARCHAR will now raise in the Decimal conversion loop below. Consider gating this on "all non-NULL values are Decimal," or documenting heterogeneous columns as unsupported on this path.

[VyrnSynx (vyrnsynx)]

Sumit Sarabhai (@sumitmsft) Thanks for the review — addressed in the latest commit:

1. Dropped the max_decimal_len override inside the NUMERIC branch so columnSize stays as numeric precision (not string length).
2. Raise ValueError when batch precision exceeds 38 (no clamp).
3. Force decimal_as_numeric only when every non-NULL value in the column is a Decimal.

Also added unit coverage for near-max precision (1E-38), batch precision >38, heterogeneous columns, and non-finite Decimals.

[VyrnSynx (vyrnsynx)]

@microsoft-github-policy-service agree

[Copilot]

🔵 Needs a closer look

It changes core executemany() parameter binding semantics in a DB driver, so a final human review is needed to confirm no subtle regressions across ODBC/SQL Server edge cases.

Review details

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

[Gaurav Sharma (bewithgaurav)]

/azp run

[azure-pipelines]

Azure Pipelines:
Successfully started running 1 pipeline(s).

[Gaurav Sharma (bewithgaurav)]

VyrnSynx (@vyrnsynx) - requesting you to please look into the failing tests, let us know if you need any help

[Gaurav Sharma (bewithgaurav)]

requesting changes. the issue comparison still errors, while normal decimal batches that work on the merge base now fail before reaching sql server. I reproduced this locally, and the same regression appears in the linux and windows CI jobs.

[Gaurav Sharma (bewithgaurav)]

this uses sql numeric precision as the array's character-buffer width too. precision only counts digits, but the fixed-point string also needs space for the sign, decimal point, leading zero, and terminator. Decimal("1E-38") gets (38, 38) here but formats to 40 characters, so PyStringArrayDataBuffer rejects it before sql server sees it.

I ran the same cases on this head and the exact merge base. Decimal("1E-38") and [-12.34, 56.78] both complete on the base; this head raises RuntimeError: Input string exceeds allowed column size at parameter index 0. the linux and windows jobs show the same 13 decimal failures too.

can we keep sql precision and scale separate from the SQL_C_CHAR buffer stride, sizing the buffer from the longest encoded fixed-point value plus its terminator?

[VyrnSynx (vyrnsynx)]

Gaurav Sharma (@bewithgaurav) thanks for catching this — you were right that we were treating SQL numeric precision as the CHAR array width.

Pushed a follow-up that keeps columnSize / decimalDigits as SQL precision/scale for SQLBindParameter, and sizes the SQL_C_CHAR array stride separately (bufferSize) from the longest fixed-point encoding in the batch (with a precision+3 fallback in the binder). That covers Decimal("1E-38") and the mixed-sign short-precision case like [-12.34, 56.78].

Added/updated unit coverage for those, plus an executemany numeric(38,38) round-trip test.

[Copilot]

🔵 Needs a closer look

It changes core executemany type inference and native array-binding buffer sizing in C++, which is regression-prone and warrants final human review despite strong new test coverage.

Review details

Suppressed comments (2)

mssql_python/cursor.py:2632

• executemany() builds a full non_null_values list just to decide whether all non-NULL values are Decimal. For large batches this doubles per-column memory and does extra work; a single-pass check avoids allocating another list.

                non_null_values = [v for v in column if v is not None]
                decimal_as_numeric = bool(non_null_values) and all(
                    isinstance(v, decimal.Decimal) for v in non_null_values
                )

mssql_python/pybind/ddbc_bindings.cpp:2418

• This bounds check is against dataWidth (array buffer stride), but the thrown message still refers to "column size". Since columnSize can represent NUMERIC precision rather than string length, the message should reference buffer width to match the check.

                            if (encodedStr.size() > dataWidth) {
                                LOG("BindParameterArray: String/binary too "
                                    "long - param_index=%d, row=%zu, size=%zu, "
                                    "max=%zu",
                                    paramIndex, i, encodedStr.size(), dataWidth);
                                ThrowStdException("Input exceeds column size at index " +
                                                  std::to_string(i));

• Files reviewed: 6/6 changed files
• Comments generated: 2
• Review effort level: Lite

[Gaurav Sharma (bewithgaurav)]

/azp run

[azure-pipelines]

Azure Pipelines:
Successfully started running 1 pipeline(s).

[Gaurav Sharma (bewithgaurav)]

the original decimal buffer issue is fixed. requesting changes for a new regression in how conversion errors are handled, detailed inline.

[Gaurav Sharma (bewithgaurav)]

with explicit numeric input sizes, failed conversions can now escape as raw exceptions and expose value-bearing error messages in the traceback, instead of the existing ValueError with row/column details.

this sizing pass converts non-Decimal inputs before the protected conversion loop. its catch misses MemoryError and RuntimeError.

the existing test_setinputsizes_sql_decimal_non_decimal_exception_no_leak fails here with raw MemoryError for "1e999999999999999999". it passes on the exact merge base.

I also reproduced the message exposure with a synthetic marker:

class ExplodingStr:
    def __str__(self):
        raise RuntimeError("synthetic-private-parameter")

cursor.execute("CREATE TABLE #pr752_error_boundary (v DECIMAL(18,2))")
cursor.setinputsizes([(mssql_python.SQL_DECIMAL, 18, 2)])
cursor.executemany(
    "INSERT INTO #pr752_error_boundary VALUES (?)",
    [(ExplodingStr(),)],
)

head 977654f raises RuntimeError: synthetic-private-parameter. base cb0bd6c raises the sanitized ValueError identifying row 0, column 0, with the marker absent from the formatted traceback.

please derive the buffer width from text produced through the existing protected conversion path, rather than converting independently here, and cover both error cases.