From dcb60f240c6ff6ff81a46ec344fcaff12b9f2389 Mon Sep 17 00:00:00 2001 From: Gaurav Sharma Date: Wed, 29 Apr 2026 17:16:05 +0530 Subject: [PATCH 01/29] Add C++ DetectParamTypes + SQLExecuteFast pipeline MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Move parameter type detection from Python into C++ using raw CPython type checks (PyLong_CheckExact, PyFloat_CheckExact, etc.). Merge the DetectParamTypes → BindParameters → SQLExecute pipeline into a single DDBCSQLExecuteFast call so ParamInfo never crosses the pybind11 boundary. - DetectParamTypes: handles int (range-detected), float, bool, str (unicode + geometry sniffing), bytes, datetime/date/time, Decimal (MONEY range + generic numeric), UUID, None, with fallback to string - SQLExecuteFast_wrap: single pipeline with GIL release, always uses SQLPrepare for parameterized queries - cursor.py: fast path routing when no setinputsizes overrides present; old DDBCSQLExecute path preserved for setinputsizes callers - Named constants: MAX_INLINE_CHAR, MAX_INLINE_BINARY, MAX_NUMERIC_PRECISION, MONEY/SMALLMONEY ranges, PARAM_C_TYPE_TEXT platform macro --- mssql_python/cursor.py | 82 +++-- mssql_python/pybind/ddbc_bindings.cpp | 446 ++++++++++++++++++++++++++ 2 files changed, 495 insertions(+), 33 deletions(-) diff --git a/mssql_python/cursor.py b/mssql_python/cursor.py index 05324875..783d0c69 100644 --- a/mssql_python/cursor.py +++ b/mssql_python/cursor.py @@ -1452,11 +1452,6 @@ def execute( # pylint: disable=too-many-locals,too-many-branches,too-many-state # Getting encoding setting encoding_settings = self._get_encoding_settings() - # Apply timeout if set (non-zero) - logger.debug("execute: Creating parameter type list") - param_info = ddbc_bindings.ParamInfo - parameters_type = [] - # Validate that inputsizes matches parameter count if both are present if parameters and self._inputsizes: if len(self._inputsizes) != len(parameters): @@ -1468,11 +1463,6 @@ def execute( # pylint: disable=too-many-locals,too-many-branches,too-many-state Warning, ) - if parameters: - for i, param in enumerate(parameters): - paraminfo = self._create_parameter_types_list(param, param_info, parameters, i) - parameters_type.append(paraminfo) - # Prepare caching: skip SQLPrepare when re-executing the same SQL # with parameters. The HSTMT is reused via _soft_reset_cursor, so the # server-side plan from the previous SQLPrepare is still valid. @@ -1481,30 +1471,56 @@ def execute( # pylint: disable=too-many-locals,too-many-branches,too-many-state self.is_stmt_prepared = [False] effective_use_prepare = use_prepare and not same_sql - if logger.isEnabledFor(logging.DEBUG): - for i, param in enumerate(parameters): - logger.debug( - """Parameter number: %s, Parameter: %s, - Param Python Type: %s, ParamInfo: %s, %s, %s, %s, %s""", - i + 1, - param, - str(type(param)), - parameters_type[i].paramSQLType, - parameters_type[i].paramCType, - parameters_type[i].columnSize, - parameters_type[i].decimalDigits, - parameters_type[i].inputOutputType, - ) - - ret = ddbc_bindings.DDBCSQLExecute( - self.hstmt, - operation, - parameters, - parameters_type, - self.is_stmt_prepared, - effective_use_prepare, - encoding_settings, + # Fast path: when no inputsizes override, do type detection + bind + execute + # entirely in C++. ParamInfo never crosses the pybind11 boundary. + use_fast_path = parameters and not ( + self._inputsizes and any(s is not None for s in self._inputsizes) ) + + if use_fast_path: + ret = ddbc_bindings.DDBCSQLExecuteFast( + self.hstmt, + operation, + parameters, + self.is_stmt_prepared, + effective_use_prepare, + encoding_settings, + ) + else: + # Slow path: Python-side type detection (used when setinputsizes overrides are present) + parameters_type = [] + if parameters: + param_info = ddbc_bindings.ParamInfo + for i, param in enumerate(parameters): + paraminfo = self._create_parameter_types_list( + param, param_info, parameters, i + ) + parameters_type.append(paraminfo) + + if logger.isEnabledFor(logging.DEBUG): + for i, param in enumerate(parameters): + logger.debug( + """Parameter number: %s, Parameter: %s, + Param Python Type: %s, ParamInfo: %s, %s, %s, %s, %s""", + i + 1, + param, + str(type(param)), + parameters_type[i].paramSQLType, + parameters_type[i].paramCType, + parameters_type[i].columnSize, + parameters_type[i].decimalDigits, + parameters_type[i].inputOutputType, + ) + + ret = ddbc_bindings.DDBCSQLExecute( + self.hstmt, + operation, + parameters, + parameters_type, + self.is_stmt_prepared, + effective_use_prepare, + encoding_settings, + ) # Check return code try: diff --git a/mssql_python/pybind/ddbc_bindings.cpp b/mssql_python/pybind/ddbc_bindings.cpp index 9d007653..0fc59701 100644 --- a/mssql_python/pybind/ddbc_bindings.cpp +++ b/mssql_python/pybind/ddbc_bindings.cpp @@ -163,6 +163,7 @@ struct ParamInfo { SQLLEN strLenOrInd = 0; // Required for DAE bool isDAE = false; // Indicates if we need to stream py::object dataPtr; + Py_ssize_t utf16Len = 0; // UTF-16 code unit count for string params }; #ifdef __GNUC__ #pragma GCC diagnostic pop @@ -439,6 +440,386 @@ std::string DescribeChar(unsigned char ch) { } } +// --------------------------------------------------------------------------- +// Constants for DetectParamTypes +// --------------------------------------------------------------------------- + +// Strings longer than this use data-at-execution (DAE) streaming +static constexpr int MAX_INLINE_CHAR = 4000; + +// Binary data longer than this uses DAE streaming (SQL Server max for non-MAX types) +static constexpr int MAX_INLINE_BINARY = 8000; + +// SQL Server maximum numeric precision +static constexpr int MAX_NUMERIC_PRECISION = 38; + +// MONEY range: -922,337,203,685,477.5808 to 922,337,203,685,477.5807 +static constexpr double MONEY_MIN = -922337203685477.5808; +static constexpr double MONEY_MAX = 922337203685477.5807; + +// SMALLMONEY range: -214,748.3648 to 214,748.3647 +static constexpr double SMALLMONEY_MIN = -214748.3648; +static constexpr double SMALLMONEY_MAX = 214748.3647; + +// Platform-specific text C type: unixODBC requires all text as wide chars +#if defined(__APPLE__) || defined(__linux__) +static constexpr SQLSMALLINT PARAM_C_TYPE_TEXT = SQL_C_WCHAR; +#else +static constexpr SQLSMALLINT PARAM_C_TYPE_TEXT = SQL_C_CHAR; +#endif + +// Forward declare NumericData helper used by decimal path +static py::object build_numeric_data(const py::object& decimal_param); + +// --------------------------------------------------------------------------- +// DetectParamTypes — C++ type detection for the execute() fast path. +// +// Replaces the Python-side _create_parameter_types_list() loop by doing type +// detection entirely in C++ using raw CPython type checks. +// +// ORDERING MATTERS: +// - bool before int (bool is a subclass of int in Python) +// - datetime before date (datetime is a subclass of date) +// +// Some types mutate the params list in-place via PyList_SET_ITEM: +// - time → normalized to "HH:MM:SS.ffffff" string +// - Decimal in MONEY range → formatted via __format__("f") +// - Decimal (generic) → converted to NumericData struct +// - UUID → replaced with bytes_le +// This is safe because execute() already copies the caller's param list +// (via list(actual_params)) before reaching this function. +// --------------------------------------------------------------------------- +std::vector DetectParamTypes(py::list& params) { + PythonObjectCache::initialize(); + + const Py_ssize_t n = py::len(params); + std::vector infos(n); + + PyObject* decimal_type = PythonObjectCache::get_decimal_class().ptr(); + PyObject* uuid_type = PythonObjectCache::get_uuid_class().ptr(); + PyObject* datetime_type = PythonObjectCache::get_datetime_class().ptr(); + PyObject* date_type = PythonObjectCache::get_date_class().ptr(); + PyObject* time_type = PythonObjectCache::get_time_class().ptr(); + + for (Py_ssize_t i = 0; i < n; ++i) { + ParamInfo& info = infos[i]; + info.inputOutputType = SQL_PARAM_INPUT; + info.isDAE = false; + + PyObject* obj = PyList_GET_ITEM(params.ptr(), i); + + // --- None --- + if (obj == Py_None) { + info.paramSQLType = SQL_UNKNOWN_TYPE; + info.paramCType = SQL_C_DEFAULT; + info.columnSize = 1; + info.decimalDigits = 0; + continue; + } + + // --- bool (must check before int, since bool is subclass of int) --- + if (PyBool_Check(obj)) { + info.paramSQLType = SQL_BIT; + info.paramCType = SQL_C_BIT; + info.columnSize = 1; + info.decimalDigits = 0; + continue; + } + + // --- int --- + if (PyLong_CheckExact(obj)) { + int overflow = 0; + int64_t val = PyLong_AsLongLongAndOverflow(obj, &overflow); + if (overflow == 0 && !PyErr_Occurred()) { + if (val >= 0 && val <= 255) { + info.paramSQLType = SQL_TINYINT; + info.paramCType = SQL_C_TINYINT; + info.columnSize = 3; + } else if (val >= -32768 && val <= 32767) { + info.paramSQLType = SQL_SMALLINT; + info.paramCType = SQL_C_SHORT; + info.columnSize = 5; + } else if (val >= -2147483648LL && val <= 2147483647LL) { + info.paramSQLType = SQL_INTEGER; + info.paramCType = SQL_C_LONG; + info.columnSize = 10; + } else { + info.paramSQLType = SQL_BIGINT; + info.paramCType = SQL_C_SBIGINT; + info.columnSize = 19; + } + } else { + PyErr_Clear(); + info.paramSQLType = SQL_BIGINT; + info.paramCType = SQL_C_SBIGINT; + info.columnSize = 19; + } + info.decimalDigits = 0; + continue; + } + + // --- float --- + if (PyFloat_CheckExact(obj)) { + info.paramSQLType = SQL_DOUBLE; + info.paramCType = SQL_C_DOUBLE; + info.columnSize = 15; + info.decimalDigits = 0; + continue; + } + + // --- str --- + if (PyUnicode_CheckExact(obj)) { + Py_ssize_t length = PyUnicode_GET_LENGTH(obj); + unsigned int kind = PyUnicode_KIND(obj); + + Py_ssize_t utf16_len; + if (kind <= PyUnicode_2BYTE_KIND) { + utf16_len = length; + } else { + utf16_len = 0; + const Py_UCS4* data = PyUnicode_4BYTE_DATA(obj); + for (Py_ssize_t j = 0; j < length; ++j) { + utf16_len += (data[j] > 0xFFFF) ? 2 : 1; + } + } + + bool is_unicode = (kind > PyUnicode_1BYTE_KIND) || + (PyUnicode_IS_COMPACT_ASCII(obj) == 0 && kind == PyUnicode_1BYTE_KIND + && PyUnicode_MAX_CHAR_VALUE(obj) > 127); + + if (utf16_len > MAX_INLINE_CHAR) { + info.isDAE = true; + info.columnSize = 0; + info.dataPtr = py::reinterpret_borrow(py::handle(obj)); + } else { + info.columnSize = is_unicode ? utf16_len : length; + info.utf16Len = utf16_len; + } + info.paramSQLType = is_unicode ? SQL_WVARCHAR : SQL_VARCHAR; + info.paramCType = is_unicode ? SQL_C_WCHAR : PARAM_C_TYPE_TEXT; + info.decimalDigits = 0; + + // Check geometry prefixes + if (length >= 5 && kind == PyUnicode_1BYTE_KIND) { + const char* ascii = (const char*)PyUnicode_1BYTE_DATA(obj); + if (strncmp(ascii, "POINT", 5) == 0 || + (length >= 10 && strncmp(ascii, "LINESTRING", 10) == 0) || + (length >= 7 && strncmp(ascii, "POLYGON", 7) == 0)) { + info.paramSQLType = SQL_WVARCHAR; + info.paramCType = SQL_C_WCHAR; + info.columnSize = length; + } + } + continue; + } + + // --- bytes / bytearray --- + if (PyBytes_CheckExact(obj) || PyByteArray_CheckExact(obj)) { + Py_ssize_t length = PyBytes_CheckExact(obj) ? PyBytes_GET_SIZE(obj) + : PyByteArray_GET_SIZE(obj); + info.paramSQLType = SQL_VARBINARY; + info.paramCType = SQL_C_BINARY; + info.decimalDigits = 0; + if (length > MAX_INLINE_BINARY) { + info.isDAE = true; + info.columnSize = 0; + info.dataPtr = py::reinterpret_borrow(py::handle(obj)); + } else { + info.columnSize = std::max(length, 1); + } + continue; + } + + // --- datetime (must check before date, since datetime is subclass of date) --- + if (PyObject_IsInstance(obj, datetime_type)) { + py::handle h(obj); + py::object tzinfo = h.attr("tzinfo"); + if (!tzinfo.is_none()) { + info.paramSQLType = SQL_SS_TIMESTAMPOFFSET; + info.paramCType = SQL_C_SS_TIMESTAMPOFFSET; + info.columnSize = 34; + info.decimalDigits = 7; + } else { + info.paramSQLType = SQL_TYPE_TIMESTAMP; + info.paramCType = SQL_C_TYPE_TIMESTAMP; + info.columnSize = 26; + info.decimalDigits = 6; + } + continue; + } + + // --- date --- + if (PyObject_IsInstance(obj, date_type)) { + info.paramSQLType = SQL_TYPE_DATE; + info.paramCType = SQL_C_TYPE_DATE; + info.columnSize = 10; + info.decimalDigits = 0; + continue; + } + + // --- time (normalized to string for binding) --- + if (PyObject_IsInstance(obj, time_type)) { + info.paramSQLType = SQL_TYPE_TIME; + info.paramCType = PARAM_C_TYPE_TEXT; + info.columnSize = 16; + info.decimalDigits = 6; + py::handle h(obj); + int hour = h.attr("hour").cast(); + int minute = h.attr("minute").cast(); + int second = h.attr("second").cast(); + int microsecond = h.attr("microsecond").cast(); + char buf[32]; + if (microsecond > 0) { + snprintf(buf, sizeof(buf), "%02d:%02d:%02d.%06d", hour, minute, second, microsecond); + } else { + snprintf(buf, sizeof(buf), "%02d:%02d:%02d", hour, minute, second); + } + py::str time_str(buf); + Py_ssize_t time_len = py::len(time_str); + info.columnSize = std::max(info.columnSize, time_len); + info.utf16Len = time_len; + PyList_SET_ITEM(params.ptr(), i, time_str.release().ptr()); + continue; + } + + // --- Decimal --- + if (PyObject_IsInstance(obj, decimal_type)) { + py::handle h(obj); + py::object as_tuple = h.attr("as_tuple")(); + py::object exponent_obj = as_tuple.attr("exponent"); + + if (py::isinstance(exponent_obj)) { + info.paramSQLType = SQL_NUMERIC; + info.paramCType = SQL_C_NUMERIC; + info.columnSize = MAX_NUMERIC_PRECISION; + info.decimalDigits = 0; + py::object numeric_data = build_numeric_data(py::reinterpret_borrow(h)); + PyList_SET_ITEM(params.ptr(), i, numeric_data.release().ptr()); + continue; + } + + py::tuple digits = as_tuple.attr("digits").cast(); + int num_digits = static_cast(py::len(digits)); + int exponent = exponent_obj.cast(); + int precision; + if (exponent >= 0) + precision = num_digits + exponent; + else if ((-exponent) <= num_digits) + precision = num_digits; + else + precision = -exponent; + + if (precision > MAX_NUMERIC_PRECISION) { + throw py::value_error( + "Precision of the numeric value is too high. " + "The maximum precision supported by SQL Server is " + + std::to_string(MAX_NUMERIC_PRECISION) + ", but got " + + std::to_string(precision) + "."); + } + + // SMALLMONEY/MONEY range — bind as formatted VARCHAR string + // to match SQL Server's fixed-point money semantics. + double dval = h.attr("__float__")().cast(); + if (dval >= SMALLMONEY_MIN && dval <= MONEY_MAX) { + py::str formatted = h.attr("__format__")(py::str("f")); + info.paramSQLType = SQL_VARCHAR; + info.paramCType = PARAM_C_TYPE_TEXT; + Py_ssize_t fmtLen = py::len(formatted); + info.columnSize = fmtLen; + info.utf16Len = fmtLen; + info.decimalDigits = 0; + PyList_SET_ITEM(params.ptr(), i, formatted.release().ptr()); + continue; + } + + // Generic numeric binding via SQL_NUMERIC_STRUCT + info.paramSQLType = SQL_NUMERIC; + info.paramCType = SQL_C_NUMERIC; + py::object numeric_data = build_numeric_data(py::reinterpret_borrow(h)); + NumericData nd = numeric_data.cast(); + info.columnSize = nd.precision; + info.decimalDigits = nd.scale; + PyList_SET_ITEM(params.ptr(), i, numeric_data.release().ptr()); + continue; + } + + // --- UUID --- + if (PyObject_IsInstance(obj, uuid_type)) { + py::handle h(obj); + py::bytes bytes_le = h.attr("bytes_le"); + info.paramSQLType = SQL_GUID; + info.paramCType = SQL_C_GUID; + info.columnSize = 16; + info.decimalDigits = 0; + PyList_SET_ITEM(params.ptr(), i, bytes_le.release().ptr()); + continue; + } + + // --- Fallback: convert to string (matches Python _map_sql_type default) --- + py::str str_val = py::str(obj); + Py_ssize_t length = py::len(str_val); + info.paramSQLType = SQL_WVARCHAR; + info.paramCType = SQL_C_WCHAR; + info.columnSize = length; + info.utf16Len = length; + info.decimalDigits = 0; + // Replace param in-place (safe: execute() copies the caller's list) + PyList_SET_ITEM(params.ptr(), i, str_val.release().ptr()); + } + + return infos; +} + +// Helper: build SQL_NUMERIC_STRUCT from Python Decimal +static py::object build_numeric_data(const py::object& decimal_param) { + py::object as_tuple = decimal_param.attr("as_tuple")(); + py::tuple digits = as_tuple.attr("digits").cast(); + int sign_val = as_tuple.attr("sign").cast(); + py::object exponent_obj = as_tuple.attr("exponent"); + + int exponent = 0; + if (py::isinstance(exponent_obj)) { + exponent = exponent_obj.cast(); + } + + int num_digits = static_cast(py::len(digits)); + int precision, scale; + if (exponent >= 0) { + precision = num_digits + exponent; + scale = 0; + } else { + scale = -exponent; + precision = std::max(num_digits, scale); + } + precision = std::max(1, std::min(precision, MAX_NUMERIC_PRECISION)); + scale = std::min(scale, precision); + + py::object py_zero = py::int_(0); + py::object int_val = py_zero; + for (auto d : digits) { + int_val = int_val * py::int_(10) + d.cast(); + } + if (exponent > 0) { + py::object multiplier = py::int_(1); + for (int j = 0; j < exponent; ++j) + multiplier = multiplier * py::int_(10); + int_val = int_val * multiplier; + } + + py::object abs_val = int_val.attr("__abs__")(); + py::bytes val_bytes = abs_val.attr("to_bytes")(py::int_(16), py::str("little")); + std::string val_str = val_bytes.cast(); + + NumericData nd; + nd.precision = static_cast(precision); + nd.scale = static_cast(scale); + nd.sign = (sign_val == 0) ? 1 : 0; + std::memset(&nd.val[0], 0, SQL_MAX_NUMERIC_LEN); + std::memcpy(&nd.val[0], val_str.data(), std::min(val_str.size(), (size_t)SQL_MAX_NUMERIC_LEN)); + + return py::cast(nd); +} + // Given a list of parameters and their ParamInfo, calls SQLBindParameter on // each of them with appropriate arguments SQLRETURN BindParameters(SQLHANDLE hStmt, const py::list& params, @@ -2042,6 +2423,67 @@ SQLRETURN SQLExecute_wrap(const SqlHandlePtr statementHandle, } } +// --------------------------------------------------------------------------- +// SQLExecuteFast — single C++ pipeline: DetectParamTypes → BindParameters → SQLExecute +// No ParamInfo objects cross the pybind11 boundary. +// +// Always uses SQLPrepare (not ExecDirect) because parameterized queries +// benefit from prepared plan reuse, and the fast path is only invoked +// when parameters are present. The use_prepare flag from the caller is +// acknowledged but overridden — this is a perf-only code path. +// --------------------------------------------------------------------------- +SQLRETURN SQLExecuteFast_wrap(const SqlHandlePtr statementHandle, + const std::wstring& query, + py::list params, + py::list is_stmt_prepared, + bool use_prepare, + const py::dict& encoding_settings) { + if (!statementHandle || !statementHandle->get()) { + return SQL_INVALID_HANDLE; + } + + SQLHANDLE hStmt = statementHandle->get(); + std::string charEncoding = "utf-8"; + std::string wcharEncoding = "utf-16le"; + if (encoding_settings.contains("charEncoding")) { + charEncoding = encoding_settings["charEncoding"].cast(); + } + if (encoding_settings.contains("wcharEncoding")) { + wcharEncoding = encoding_settings["wcharEncoding"].cast(); + } + + RETCODE rc; + bool already_prepared = is_stmt_prepared[0].cast(); + + // Prepare if needed (fast path always uses prepare for parameterized queries) + if (!already_prepared) { +#if defined(__APPLE__) || defined(__linux__) + std::vector queryBuffer = WStringToSQLWCHAR(query); + SQLWCHAR* queryPtr = queryBuffer.data(); +#else + SQLWCHAR* queryPtr = const_cast(query.c_str()); +#endif + { + py::gil_scoped_release release; + rc = SQLPrepare_ptr(hStmt, queryPtr, SQL_NTS); + } + if (!SQL_SUCCEEDED(rc)) return rc; + is_stmt_prepared[0] = py::bool_(true); + } + + // DetectParamTypes + BindParameters in one shot — ParamInfo stays in C++ + std::vector paramInfos = DetectParamTypes(params); + std::vector> paramBuffers; + rc = BindParameters(hStmt, params, paramInfos, paramBuffers, charEncoding); + if (!SQL_SUCCEEDED(rc)) return rc; + + { + py::gil_scoped_release release; + rc = SQLExecute_ptr(hStmt); + } + return rc; +} + SQLRETURN BindParameterArray(SQLHANDLE hStmt, const py::list& columnwise_params, const std::vector& paramInfos, size_t paramSetSize, std::vector>& paramBuffers, @@ -5803,6 +6245,10 @@ PYBIND11_MODULE(ddbc_bindings, m) { m.def("DDBCSQLExecute", &SQLExecute_wrap, "Prepare and execute T-SQL statements", py::arg("statementHandle"), py::arg("query"), py::arg("params"), py::arg("paramInfos"), py::arg("isStmtPrepared"), py::arg("usePrepare"), py::arg("encodingSettings")); + m.def("DDBCSQLExecuteFast", &SQLExecuteFast_wrap, + "Fast path: DetectParamTypes + BindParameters + SQLExecute all in C++", + py::arg("statementHandle"), py::arg("query"), py::arg("params"), + py::arg("isStmtPrepared"), py::arg("usePrepare"), py::arg("encodingSettings")); m.def("SQLExecuteMany", &SQLExecuteMany_wrap, "Execute statement with multiple parameter sets", py::arg("statementHandle"), py::arg("query"), py::arg("columnwise_params"), py::arg("paramInfos"), py::arg("paramSetSize"), py::arg("encodingSettings")); From 14ccf08f034bc3d5ecc69d5bf80e01954ad22ee5 Mon Sep 17 00:00:00 2001 From: Gaurav Sharma Date: Wed, 29 Apr 2026 18:13:16 +0530 Subject: [PATCH 02/29] Fix DAE handling, MONEY range, and TypeError for unknown types MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Add complete DAE (Data-At-Execution) loop to SQLExecuteFast_wrap: SQL_NEED_DATA → SQLParamData/SQLPutData for large str/bytes/binary, matching the existing SQLExecute_wrap logic exactly - Fix DAE type assignment: non-unicode DAE strings use SQL_C_CHAR (not PARAM_C_TYPE_TEXT which maps to SQL_C_WCHAR on macOS/Linux) - Fix MONEY range lower bound: use MONEY_MIN not SMALLMONEY_MIN so negative decimals in MONEY range bind as VARCHAR (matches Python path) - Raise TypeError for unknown param types instead of silent str conversion - Add SQLFreeStmt(SQL_RESET_PARAMS) to unbind after execute --- mssql_python/pybind/ddbc_bindings.cpp | 111 +++++++++++++++++++++++--- 1 file changed, 98 insertions(+), 13 deletions(-) diff --git a/mssql_python/pybind/ddbc_bindings.cpp b/mssql_python/pybind/ddbc_bindings.cpp index 0fc59701..fd4ab530 100644 --- a/mssql_python/pybind/ddbc_bindings.cpp +++ b/mssql_python/pybind/ddbc_bindings.cpp @@ -588,15 +588,20 @@ std::vector DetectParamTypes(py::list& params) { && PyUnicode_MAX_CHAR_VALUE(obj) > 127); if (utf16_len > MAX_INLINE_CHAR) { + // DAE path: match slow-path types exactly. + // Non-unicode → SQL_VARCHAR + SQL_C_CHAR (encoded via Python codec in DAE loop) + // Unicode → SQL_WVARCHAR + SQL_C_WCHAR (wide-char streaming in DAE loop) info.isDAE = true; info.columnSize = 0; info.dataPtr = py::reinterpret_borrow(py::handle(obj)); + info.paramSQLType = is_unicode ? SQL_WVARCHAR : SQL_VARCHAR; + info.paramCType = is_unicode ? SQL_C_WCHAR : SQL_C_CHAR; } else { info.columnSize = is_unicode ? utf16_len : length; info.utf16Len = utf16_len; + info.paramSQLType = is_unicode ? SQL_WVARCHAR : SQL_VARCHAR; + info.paramCType = is_unicode ? SQL_C_WCHAR : PARAM_C_TYPE_TEXT; } - info.paramSQLType = is_unicode ? SQL_WVARCHAR : SQL_VARCHAR; - info.paramCType = is_unicode ? SQL_C_WCHAR : PARAM_C_TYPE_TEXT; info.decimalDigits = 0; // Check geometry prefixes @@ -720,7 +725,7 @@ std::vector DetectParamTypes(py::list& params) { // SMALLMONEY/MONEY range — bind as formatted VARCHAR string // to match SQL Server's fixed-point money semantics. double dval = h.attr("__float__")().cast(); - if (dval >= SMALLMONEY_MIN && dval <= MONEY_MAX) { + if (dval >= MONEY_MIN && dval <= MONEY_MAX) { py::str formatted = h.attr("__format__")(py::str("f")); info.paramSQLType = SQL_VARCHAR; info.paramCType = PARAM_C_TYPE_TEXT; @@ -755,16 +760,9 @@ std::vector DetectParamTypes(py::list& params) { continue; } - // --- Fallback: convert to string (matches Python _map_sql_type default) --- - py::str str_val = py::str(obj); - Py_ssize_t length = py::len(str_val); - info.paramSQLType = SQL_WVARCHAR; - info.paramCType = SQL_C_WCHAR; - info.columnSize = length; - info.utf16Len = length; - info.decimalDigits = 0; - // Replace param in-place (safe: execute() copies the caller's list) - PyList_SET_ITEM(params.ptr(), i, str_val.release().ptr()); + // --- Unknown type: raise TypeError (matches Python _map_sql_type) --- + throw py::type_error( + "Unsupported parameter type: The driver cannot safely convert it to a SQL type."); } return infos; @@ -2481,6 +2479,93 @@ SQLRETURN SQLExecuteFast_wrap(const SqlHandlePtr statementHandle, py::gil_scoped_release release; rc = SQLExecute_ptr(hStmt); } + + // DAE (Data-At-Execution) loop: when BindParameters marks a param as DAE + // (large str/bytes/binary), SQLExecute returns SQL_NEED_DATA. We must + // stream the data via SQLParamData/SQLPutData before execution completes. + if (rc == SQL_NEED_DATA) { + SQLPOINTER paramToken = nullptr; + while ((rc = SQLParamData_ptr(hStmt, ¶mToken)) == SQL_NEED_DATA) { + const ParamInfo* matchedInfo = nullptr; + for (auto& info : paramInfos) { + if (reinterpret_cast(const_cast(&info)) == paramToken) { + matchedInfo = &info; + break; + } + } + if (!matchedInfo) { + ThrowStdException("SQLExecuteFast: unrecognized paramToken from SQLParamData"); + } + const py::object& pyObj = matchedInfo->dataPtr; + if (pyObj.is_none()) { + SQLPutData_ptr(hStmt, nullptr, 0); + continue; + } + + if (py::isinstance(pyObj)) { + if (matchedInfo->paramCType == SQL_C_WCHAR) { + std::wstring wstr = pyObj.cast(); + const SQLWCHAR* dataPtr = nullptr; + size_t totalChars = 0; +#if defined(__APPLE__) || defined(__linux__) + std::vector sqlwStr = WStringToSQLWCHAR(wstr); + totalChars = sqlwStr.size() - 1; + dataPtr = sqlwStr.data(); +#else + dataPtr = wstr.c_str(); + totalChars = wstr.size(); +#endif + size_t chunkChars = DAE_CHUNK_SIZE / sizeof(SQLWCHAR); + for (size_t offset = 0; offset < totalChars; offset += chunkChars) { + size_t len = std::min(chunkChars, totalChars - offset); + rc = SQLPutData_ptr(hStmt, (SQLPOINTER)(dataPtr + offset), + static_cast(len * sizeof(SQLWCHAR))); + if (!SQL_SUCCEEDED(rc)) return rc; + } + } else if (matchedInfo->paramCType == SQL_C_CHAR) { + std::string encodedStr; + try { + py::object encoded = pyObj.attr("encode")(charEncoding, "strict"); + encodedStr = encoded.cast(); + } catch (const py::error_already_set& e) { + throw; + } + const char* dataPtr = encodedStr.data(); + size_t totalBytes = encodedStr.size(); + for (size_t offset = 0; offset < totalBytes; offset += DAE_CHUNK_SIZE) { + size_t len = std::min(static_cast(DAE_CHUNK_SIZE), + totalBytes - offset); + rc = SQLPutData_ptr(hStmt, (SQLPOINTER)(dataPtr + offset), + static_cast(len)); + if (!SQL_SUCCEEDED(rc)) return rc; + } + } else { + ThrowStdException("SQLExecuteFast: unsupported C type for str in DAE"); + } + } else if (py::isinstance(pyObj) || + py::isinstance(pyObj)) { + py::bytes b = pyObj.cast(); + std::string s = b; + const char* dataPtr = s.data(); + size_t totalBytes = s.size(); + for (size_t offset = 0; offset < totalBytes; offset += DAE_CHUNK_SIZE) { + size_t len = std::min(static_cast(DAE_CHUNK_SIZE), + totalBytes - offset); + rc = SQLPutData_ptr(hStmt, (SQLPOINTER)(dataPtr + offset), + static_cast(len)); + if (!SQL_SUCCEEDED(rc)) return rc; + } + } else { + ThrowStdException("SQLExecuteFast: DAE only supported for str or bytes"); + } + } + if (!SQL_SUCCEEDED(rc) && rc != SQL_NO_DATA) return rc; + } + + if (!SQL_SUCCEEDED(rc) && rc != SQL_NO_DATA) return rc; + + // Unbind params — buffers go out of scope after this + rc = SQLFreeStmt_ptr(hStmt, SQL_RESET_PARAMS); return rc; } From 8ab074c984b09488fa910241ae7e9ee38afac4af Mon Sep 17 00:00:00 2001 From: Gaurav Sharma Date: Thu, 30 Apr 2026 11:09:25 +0530 Subject: [PATCH 03/29] Fix MSVC warnings-as-errors: unused param and catch variable - Comment out use_prepare parameter name (C4100: unreferenced parameter) - Remove unused catch variable name (C4101: unreferenced local variable) --- mssql_python/pybind/ddbc_bindings.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/mssql_python/pybind/ddbc_bindings.cpp b/mssql_python/pybind/ddbc_bindings.cpp index fd4ab530..19c3e2f3 100644 --- a/mssql_python/pybind/ddbc_bindings.cpp +++ b/mssql_python/pybind/ddbc_bindings.cpp @@ -2434,7 +2434,7 @@ SQLRETURN SQLExecuteFast_wrap(const SqlHandlePtr statementHandle, const std::wstring& query, py::list params, py::list is_stmt_prepared, - bool use_prepare, + bool /*use_prepare*/, const py::dict& encoding_settings) { if (!statementHandle || !statementHandle->get()) { return SQL_INVALID_HANDLE; @@ -2527,7 +2527,7 @@ SQLRETURN SQLExecuteFast_wrap(const SqlHandlePtr statementHandle, try { py::object encoded = pyObj.attr("encode")(charEncoding, "strict"); encodedStr = encoded.cast(); - } catch (const py::error_already_set& e) { + } catch (const py::error_already_set&) { throw; } const char* dataPtr = encodedStr.data(); From 00c85ccbaf161e7f1925501bd1df5b1dcbafde23 Mon Sep 17 00:00:00 2001 From: Gaurav Sharma Date: Thu, 30 Apr 2026 11:23:31 +0530 Subject: [PATCH 04/29] Guard memcpy with null/length check for DevSkim DS121708 Add explicit null pointer and zero-length guards before memcpy in build_numeric_data to satisfy DevSkim code scanning rule DS121708. --- mssql_python/pybind/ddbc_bindings.cpp | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/mssql_python/pybind/ddbc_bindings.cpp b/mssql_python/pybind/ddbc_bindings.cpp index 19c3e2f3..9e904f36 100644 --- a/mssql_python/pybind/ddbc_bindings.cpp +++ b/mssql_python/pybind/ddbc_bindings.cpp @@ -813,7 +813,10 @@ static py::object build_numeric_data(const py::object& decimal_param) { nd.scale = static_cast(scale); nd.sign = (sign_val == 0) ? 1 : 0; std::memset(&nd.val[0], 0, SQL_MAX_NUMERIC_LEN); - std::memcpy(&nd.val[0], val_str.data(), std::min(val_str.size(), (size_t)SQL_MAX_NUMERIC_LEN)); + size_t copy_len = std::min(val_str.size(), static_cast(SQL_MAX_NUMERIC_LEN)); + if (copy_len > 0 && val_str.data() != nullptr) { + std::memcpy(&nd.val[0], val_str.data(), copy_len); + } return py::cast(nd); } From bad8acf37e1d50fb22bf7245b039fca4e4e2daaa Mon Sep 17 00:00:00 2001 From: Gaurav Sharma Date: Thu, 7 May 2026 12:51:08 +0530 Subject: [PATCH 05/29] STYLE: Fix black formatting in cursor.py --- mssql_python/cursor.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/mssql_python/cursor.py b/mssql_python/cursor.py index 783d0c69..8659b065 100644 --- a/mssql_python/cursor.py +++ b/mssql_python/cursor.py @@ -1492,9 +1492,7 @@ def execute( # pylint: disable=too-many-locals,too-many-branches,too-many-state if parameters: param_info = ddbc_bindings.ParamInfo for i, param in enumerate(parameters): - paraminfo = self._create_parameter_types_list( - param, param_info, parameters, i - ) + paraminfo = self._create_parameter_types_list(param, param_info, parameters, i) parameters_type.append(paraminfo) if logger.isEnabledFor(logging.DEBUG): From c5a827fbc13595ecb58f276639b21117c784825a Mon Sep 17 00:00:00 2001 From: Gaurav Sharma Date: Thu, 7 May 2026 14:04:18 +0530 Subject: [PATCH 06/29] Address PR review: encoding key, subclass support, GIL, exec_rc, cursor attrs, parity test Six review fixes for SQLExecuteFast_wrap and DetectParamTypes: 1. Encoding key: read 'encoding' from settings dict (was 'charEncoding' which never matched). Only honor when ctype==SQL_C_CHAR so the default utf-16le doesn't corrupt SQL_C_CHAR DAE/inline byte paths. 2. Subclass support: PyLong_Check/PyFloat_Check/PyUnicode_Check/PyBytes_Check instead of *_CheckExact. Fixes user-defined int/str/bytes/float subclasses that were silently rejected with TypeError. Switched PyBytes_GET_SIZE to PyBytes_Size for subclass-safe length. 3. GIL release in DAE loop: SQLParamData and SQLPutData now release the GIL during each ODBC call, matching slow-path concurrency for large blobs/strings. 4. Preserve exec_rc: stash the SQLExecute return code before SQLFreeStmt so SUCCESS_WITH_INFO and other non-success-non-error codes are not clobbered by the unbind call. 5. Shallow-copy params: params = py::list(params) at function entry so DetectParamTypes' in-place PyList_SET_ITEM cannot mutate the caller's list under any future code path that might pass it directly. 6. Cursor attrs: SQLSetStmtAttr(SQL_ATTR_CURSOR_TYPE/CONCURRENCY) at entry to match slow-path semantics regardless of prior hstmt state. Also adds tests/test_023_fast_path_parity.py covering int/str/bytes/float subclasses, caller-list non-mutation, and unsupported-type TypeError. --- mssql_python/pybind/ddbc_bindings.cpp | 97 +++++++++++------ tests/test_023_fast_path_parity.py | 151 ++++++++++++++++++++++++++ 2 files changed, 216 insertions(+), 32 deletions(-) create mode 100644 tests/test_023_fast_path_parity.py diff --git a/mssql_python/pybind/ddbc_bindings.cpp b/mssql_python/pybind/ddbc_bindings.cpp index b9909824..8d63303e 100644 --- a/mssql_python/pybind/ddbc_bindings.cpp +++ b/mssql_python/pybind/ddbc_bindings.cpp @@ -526,8 +526,8 @@ std::vector DetectParamTypes(py::list& params) { continue; } - // --- int --- - if (PyLong_CheckExact(obj)) { + // --- int (allow subclasses, but bool was already caught above) --- + if (PyLong_Check(obj)) { int overflow = 0; int64_t val = PyLong_AsLongLongAndOverflow(obj, &overflow); if (overflow == 0 && !PyErr_Occurred()) { @@ -558,8 +558,8 @@ std::vector DetectParamTypes(py::list& params) { continue; } - // --- float --- - if (PyFloat_CheckExact(obj)) { + // --- float (allow subclasses) --- + if (PyFloat_Check(obj)) { info.paramSQLType = SQL_DOUBLE; info.paramCType = SQL_C_DOUBLE; info.columnSize = 15; @@ -567,8 +567,8 @@ std::vector DetectParamTypes(py::list& params) { continue; } - // --- str --- - if (PyUnicode_CheckExact(obj)) { + // --- str (allow subclasses) --- + if (PyUnicode_Check(obj)) { Py_ssize_t length = PyUnicode_GET_LENGTH(obj); unsigned int kind = PyUnicode_KIND(obj); @@ -618,10 +618,10 @@ std::vector DetectParamTypes(py::list& params) { continue; } - // --- bytes / bytearray --- - if (PyBytes_CheckExact(obj) || PyByteArray_CheckExact(obj)) { - Py_ssize_t length = PyBytes_CheckExact(obj) ? PyBytes_GET_SIZE(obj) - : PyByteArray_GET_SIZE(obj); + // --- bytes / bytearray (allow subclasses) --- + if (PyBytes_Check(obj) || PyByteArray_Check(obj)) { + Py_ssize_t length = + PyBytes_Check(obj) ? PyBytes_Size(obj) : PyByteArray_Size(obj); info.paramSQLType = SQL_VARBINARY; info.paramCType = SQL_C_BINARY; info.decimalDigits = 0; @@ -2496,15 +2496,33 @@ SQLRETURN SQLExecuteFast_wrap(const SqlHandlePtr statementHandle, } SQLHANDLE hStmt = statementHandle->get(); - std::string charEncoding = "utf-8"; - std::string wcharEncoding = "utf-16le"; - if (encoding_settings.contains("charEncoding")) { - charEncoding = encoding_settings["charEncoding"].cast(); + + // Configure forward-only / read-only cursor (matches slow path semantics). + if (SQLSetStmtAttr_ptr) { + SQLSetStmtAttr_ptr(hStmt, SQL_ATTR_CURSOR_TYPE, + (SQLPOINTER)SQL_CURSOR_FORWARD_ONLY, 0); + SQLSetStmtAttr_ptr(hStmt, SQL_ATTR_CONCURRENCY, + (SQLPOINTER)SQL_CONCUR_READ_ONLY, 0); } - if (encoding_settings.contains("wcharEncoding")) { - wcharEncoding = encoding_settings["wcharEncoding"].cast(); + + // Match the slow path's encoding-dict contract: keys are "encoding" and "ctype". + // Only honor the user's encoding when their preferred ctype is SQL_C_CHAR; + // otherwise the default ctype is SQL_C_WCHAR and the "encoding" value is + // meant for wide-char paths (e.g. "utf-16le") and would corrupt the + // SQL_C_CHAR DAE/inline path that operates on byte data. + std::string charEncoding = "utf-8"; + if (encoding_settings.contains("ctype") && encoding_settings.contains("encoding")) { + int ctype = encoding_settings["ctype"].cast(); + if (ctype == SQL_C_CHAR) { + charEncoding = encoding_settings["encoding"].cast(); + } } + // Shallow-copy the parameter list so DetectParamTypes' in-place + // PyList_SET_ITEM never mutates the caller's list. The cost is one + // PyList_New + N refcount bumps; cheap relative to ODBC binding. + params = py::list(params); + RETCODE rc; bool already_prepared = is_stmt_prepared[0].cast(); @@ -2538,9 +2556,16 @@ SQLRETURN SQLExecuteFast_wrap(const SqlHandlePtr statementHandle, // DAE (Data-At-Execution) loop: when BindParameters marks a param as DAE // (large str/bytes/binary), SQLExecute returns SQL_NEED_DATA. We must // stream the data via SQLParamData/SQLPutData before execution completes. + // GIL is released around each ODBC call to match slow-path concurrency. if (rc == SQL_NEED_DATA) { SQLPOINTER paramToken = nullptr; - while ((rc = SQLParamData_ptr(hStmt, ¶mToken)) == SQL_NEED_DATA) { + while (true) { + { + py::gil_scoped_release release; + rc = SQLParamData_ptr(hStmt, ¶mToken); + } + if (rc != SQL_NEED_DATA) break; + const ParamInfo* matchedInfo = nullptr; for (auto& info : paramInfos) { if (reinterpret_cast(const_cast(&info)) == paramToken) { @@ -2553,6 +2578,7 @@ SQLRETURN SQLExecuteFast_wrap(const SqlHandlePtr statementHandle, } const py::object& pyObj = matchedInfo->dataPtr; if (pyObj.is_none()) { + py::gil_scoped_release release; SQLPutData_ptr(hStmt, nullptr, 0); continue; } @@ -2573,25 +2599,27 @@ SQLRETURN SQLExecuteFast_wrap(const SqlHandlePtr statementHandle, size_t chunkChars = DAE_CHUNK_SIZE / sizeof(SQLWCHAR); for (size_t offset = 0; offset < totalChars; offset += chunkChars) { size_t len = std::min(chunkChars, totalChars - offset); - rc = SQLPutData_ptr(hStmt, (SQLPOINTER)(dataPtr + offset), - static_cast(len * sizeof(SQLWCHAR))); + { + py::gil_scoped_release release; + rc = SQLPutData_ptr(hStmt, (SQLPOINTER)(dataPtr + offset), + static_cast(len * sizeof(SQLWCHAR))); + } if (!SQL_SUCCEEDED(rc)) return rc; } } else if (matchedInfo->paramCType == SQL_C_CHAR) { std::string encodedStr; - try { - py::object encoded = pyObj.attr("encode")(charEncoding, "strict"); - encodedStr = encoded.cast(); - } catch (const py::error_already_set&) { - throw; - } + py::object encoded = pyObj.attr("encode")(charEncoding, "strict"); + encodedStr = encoded.cast(); const char* dataPtr = encodedStr.data(); size_t totalBytes = encodedStr.size(); for (size_t offset = 0; offset < totalBytes; offset += DAE_CHUNK_SIZE) { size_t len = std::min(static_cast(DAE_CHUNK_SIZE), totalBytes - offset); - rc = SQLPutData_ptr(hStmt, (SQLPOINTER)(dataPtr + offset), - static_cast(len)); + { + py::gil_scoped_release release; + rc = SQLPutData_ptr(hStmt, (SQLPOINTER)(dataPtr + offset), + static_cast(len)); + } if (!SQL_SUCCEEDED(rc)) return rc; } } else { @@ -2606,8 +2634,11 @@ SQLRETURN SQLExecuteFast_wrap(const SqlHandlePtr statementHandle, for (size_t offset = 0; offset < totalBytes; offset += DAE_CHUNK_SIZE) { size_t len = std::min(static_cast(DAE_CHUNK_SIZE), totalBytes - offset); - rc = SQLPutData_ptr(hStmt, (SQLPOINTER)(dataPtr + offset), - static_cast(len)); + { + py::gil_scoped_release release; + rc = SQLPutData_ptr(hStmt, (SQLPOINTER)(dataPtr + offset), + static_cast(len)); + } if (!SQL_SUCCEEDED(rc)) return rc; } } else { @@ -2619,9 +2650,11 @@ SQLRETURN SQLExecuteFast_wrap(const SqlHandlePtr statementHandle, if (!SQL_SUCCEEDED(rc) && rc != SQL_NO_DATA) return rc; - // Unbind params — buffers go out of scope after this - rc = SQLFreeStmt_ptr(hStmt, SQL_RESET_PARAMS); - return rc; + // Preserve the execute return code (e.g. SQL_SUCCESS_WITH_INFO) — don't + // let the SQLFreeStmt return value clobber what the caller needs to see. + SQLRETURN exec_rc = rc; + SQLFreeStmt_ptr(hStmt, SQL_RESET_PARAMS); + return exec_rc; } SQLRETURN BindParameterArray(SQLHANDLE hStmt, const py::list& columnwise_params, diff --git a/tests/test_023_fast_path_parity.py b/tests/test_023_fast_path_parity.py new file mode 100644 index 00000000..09da6e00 --- /dev/null +++ b/tests/test_023_fast_path_parity.py @@ -0,0 +1,151 @@ +""" +Parity tests: assert that fast path (DetectParamTypes in C++) and slow path +(_map_sql_type in Python) produce identical query results for representative +parameter types. + +The fast path runs by default. The slow path is forced by calling setinputsizes() +with a non-None entry, which triggers cursor.execute()'s slow-path branch. +""" + +import pytest +import datetime +import decimal +import uuid +from mssql_python import connect + +import os + +CONN_STR = os.environ.get( + "DB_CONNECTION_STRING", + "Server=localhost;Database=master;Uid=sa;Pwd=Str0ng@Passw0rd123;TrustServerCertificate=yes", +) + + +@pytest.fixture +def conn(): + c = connect(CONN_STR) + yield c + c.close() + + +def _roundtrip(cursor, value): + """Round-trip a single parameter through SELECT ? and return the result.""" + cursor.execute("SELECT ?", [value]) + return cursor.fetchone()[0] + + +def _force_slow_path_roundtrip(cursor, value): + """Force slow path via setinputsizes(None for that param) — any non-empty + inputsizes list with a non-None entry triggers the legacy code path.""" + # Empty list with at least one entry that's not None forces slow path. + # Using SQL_VARCHAR(8000) as an opaque "no override" placeholder. + from mssql_python import ddbc_bindings + + # A None entry means "infer", which is fine — the slow path still runs because + # _inputsizes is set (any non-empty list with at least one non-None entry). + # We need at least one non-None entry to flip use_fast_path to False. + cursor.setinputsizes([None]) # Has at least one entry but no override + # Wait — None doesn't trigger slow path. We need a real override. + # Use SQL_VARCHAR which is identity-ish for strings. + cursor.setinputsizes([(1, 0, 0)]) # (sqlType, size, decimal) tuple + cursor.execute("SELECT ?", [value]) + cursor.setinputsizes(None) # Reset + return cursor.fetchone()[0] + + +@pytest.mark.parametrize( + "value", + [ + # int range detection + 0, + 1, + 255, + 256, + 32767, + 32768, + 2147483647, + 2147483648, + -1, + -32768, + -2147483648, + # bool + True, + False, + # float + 0.0, + 3.14, + -1.5e10, + # str (ASCII inline + DAE + unicode) + "", + "hello", + "a" * 100, + # bytes + b"", + b"\x00\x01\x02", + b"x" * 100, + ], +) +def test_fast_path_roundtrip(conn, value): + """Fast path produces identical results regardless of value type.""" + cur = conn.cursor() + result = _roundtrip(cur, value) + assert ( + result == value + ), f"Roundtrip mismatch for {type(value).__name__} {value!r}: got {result!r}" + + +def test_int_subclass(conn): + """int subclasses must work (regression test for *_CheckExact bug).""" + + class MyInt(int): + pass + + cur = conn.cursor() + assert _roundtrip(cur, MyInt(42)) == 42 + + +def test_str_subclass(conn): + """str subclasses must work.""" + + class MyStr(str): + pass + + cur = conn.cursor() + assert _roundtrip(cur, MyStr("hello")) == "hello" + + +def test_bytes_subclass(conn): + """bytes subclasses must work.""" + + class MyBytes(bytes): + pass + + cur = conn.cursor() + assert _roundtrip(cur, MyBytes(b"hello")) == b"hello" + + +def test_float_subclass(conn): + """float subclasses must work.""" + + class MyFloat(float): + pass + + cur = conn.cursor() + assert _roundtrip(cur, MyFloat(3.14)) == 3.14 + + +def test_caller_param_list_not_mutated(conn): + """DetectParamTypes must not mutate the caller's parameter list.""" + cur = conn.cursor() + params = ["hello", 42, 3.14, datetime.date(2024, 1, 1), uuid.uuid4()] + snapshot = list(params) + cur.execute("SELECT ?, ?, ?, ?, ?", params) + cur.fetchone() + assert params == snapshot, f"Caller list was mutated: {params} != {snapshot}" + + +def test_unsupported_type_raises_typeerror(conn): + """Unknown parameter types must raise TypeError, matching slow path.""" + cur = conn.cursor() + with pytest.raises(TypeError): + cur.execute("SELECT ?", [{1, 2, 3}]) # set is not supported From a38ce4e94aaffe560b8c8efb58d65ad273d9524f Mon Sep 17 00:00:00 2001 From: Gaurav Sharma Date: Thu, 7 May 2026 14:46:24 +0530 Subject: [PATCH 07/29] Address second PR review: refcount leak, geometry+DAE, NaN, parity test Eight follow-up fixes after review feedback on c5a827f. 1. Refcount leak (BLOCKER): replace PyList_SET_ITEM (uppercase, no decref of old slot) with PyList_SetItem (decrefs old slot before stealing the new reference) in DetectParamTypes time/Decimal/UUID branches. The previous shallow-copy defense via py::list(params) was a no-op because pybind11s list constructor only inc_refs an already-list argument. 2. Geometry + DAE conflict: gate the geometry-prefix override on the not-DAE branch so a long POLYGON/POINT/LINESTRING string does not end up with isDAE=true, dataPtr set, AND a non-zero columnSize. 3. Decimal NaN/Infinity: throw ValueError instead of silently binding 0 via build_numeric_data on an empty digits tuple. 4. Time format: always emit microseconds (HH:MM:SS.ffffff), matching slow path isoformat(timespec=microseconds). 5. PyObject_IsInstance: explicit equality check so a custom __instancecheck__ that raises (returns -1) does not fall through with a Python error set. 6. Dead code: removed unused SMALLMONEY_MIN/SMALLMONEY_MAX constants and the unused utf16Len assignments in DetectParamTypes. 7. Encoding-key contract: only honor encoding_settings encoding when the user explicitly opted in via setencoding(..., ctype=SQL_C_CHAR=1). The Python layer SQL_C_CHAR constant is numerically -8 (real ODBC SQL_C_WCHAR), so by default the wide-char path is taken and encoding is irrelevant. 8. Parity test rewrite: drop the dead _force_slow_path_roundtrip helper, use the project cursor fixture instead of a hard-coded conn string, and add (a) a real fast-vs-slow parity check via setinputsizes-forced slow path, (b) a refcount-leak regression test using a Decimal subclass + weakref, (c) explicit NaN-rejection coverage. --- mssql_python/pybind/ddbc_bindings.cpp | 94 ++++++------- tests/test_023_fast_path_parity.py | 191 ++++++++++++++++---------- 2 files changed, 165 insertions(+), 120 deletions(-) diff --git a/mssql_python/pybind/ddbc_bindings.cpp b/mssql_python/pybind/ddbc_bindings.cpp index 8d63303e..ae0171ac 100644 --- a/mssql_python/pybind/ddbc_bindings.cpp +++ b/mssql_python/pybind/ddbc_bindings.cpp @@ -457,11 +457,11 @@ static constexpr int MAX_NUMERIC_PRECISION = 38; static constexpr double MONEY_MIN = -922337203685477.5808; static constexpr double MONEY_MAX = 922337203685477.5807; -// SMALLMONEY range: -214,748.3648 to 214,748.3647 -static constexpr double SMALLMONEY_MIN = -214748.3648; -static constexpr double SMALLMONEY_MAX = 214748.3647; - -// Platform-specific text C type: unixODBC requires all text as wide chars +// Platform-specific text C type: unixODBC requires all text as wide chars on +// Linux/macOS. On Windows the ODBC driver accepts SQL_C_CHAR for ASCII text. +// This matches the Python slow path's behavior (its SQL_C_CHAR constant is +// numerically -8, which is ODBC's SQL_C_WCHAR — a long-standing alias used +// throughout the Python layer). #if defined(__APPLE__) || defined(__linux__) static constexpr SQLSMALLINT PARAM_C_TYPE_TEXT = SQL_C_WCHAR; #else @@ -481,7 +481,8 @@ static py::object build_numeric_data(const py::object& decimal_param); // - bool before int (bool is a subclass of int in Python) // - datetime before date (datetime is a subclass of date) // -// Some types mutate the params list in-place via PyList_SET_ITEM: +// Some types mutate the params list in-place via PyList_SetItem (which +// decrefs the old slot before stealing the new ref): // - time → normalized to "HH:MM:SS.ffffff" string // - Decimal in MONEY range → formatted via __format__("f") // - Decimal (generic) → converted to NumericData struct @@ -589,23 +590,26 @@ std::vector DetectParamTypes(py::list& params) { if (utf16_len > MAX_INLINE_CHAR) { // DAE path: match slow-path types exactly. - // Non-unicode → SQL_VARCHAR + SQL_C_CHAR (encoded via Python codec in DAE loop) - // Unicode → SQL_WVARCHAR + SQL_C_WCHAR (wide-char streaming in DAE loop) + // Non-unicode (ASCII) → SQL_VARCHAR + PARAM_C_TYPE_TEXT + // On Linux/macOS PARAM_C_TYPE_TEXT == SQL_C_WCHAR, matching + // the slow path's SQL_C_CHAR (which is numerically -8 == + // SQL_C_WCHAR — a long-standing alias in the Python layer). + // Unicode → SQL_WVARCHAR + SQL_C_WCHAR (wide-char streaming) info.isDAE = true; info.columnSize = 0; info.dataPtr = py::reinterpret_borrow(py::handle(obj)); info.paramSQLType = is_unicode ? SQL_WVARCHAR : SQL_VARCHAR; - info.paramCType = is_unicode ? SQL_C_WCHAR : SQL_C_CHAR; + info.paramCType = is_unicode ? SQL_C_WCHAR : PARAM_C_TYPE_TEXT; } else { info.columnSize = is_unicode ? utf16_len : length; - info.utf16Len = utf16_len; info.paramSQLType = is_unicode ? SQL_WVARCHAR : SQL_VARCHAR; info.paramCType = is_unicode ? SQL_C_WCHAR : PARAM_C_TYPE_TEXT; } info.decimalDigits = 0; - // Check geometry prefixes - if (length >= 5 && kind == PyUnicode_1BYTE_KIND) { + // Check geometry prefixes (only for non-DAE strings; long geometry + // values stay on the DAE path with their already-set types). + if (!info.isDAE && length >= 5 && kind == PyUnicode_1BYTE_KIND) { const char* ascii = (const char*)PyUnicode_1BYTE_DATA(obj); if (strncmp(ascii, "POINT", 5) == 0 || (length >= 10 && strncmp(ascii, "LINESTRING", 10) == 0) || @@ -636,7 +640,7 @@ std::vector DetectParamTypes(py::list& params) { } // --- datetime (must check before date, since datetime is subclass of date) --- - if (PyObject_IsInstance(obj, datetime_type)) { + if (PyObject_IsInstance(obj, datetime_type) == 1) { py::handle h(obj); py::object tzinfo = h.attr("tzinfo"); if (!tzinfo.is_none()) { @@ -654,7 +658,7 @@ std::vector DetectParamTypes(py::list& params) { } // --- date --- - if (PyObject_IsInstance(obj, date_type)) { + if (PyObject_IsInstance(obj, date_type) == 1) { info.paramSQLType = SQL_TYPE_DATE; info.paramCType = SQL_C_TYPE_DATE; info.columnSize = 10; @@ -663,9 +667,9 @@ std::vector DetectParamTypes(py::list& params) { } // --- time (normalized to string for binding) --- - if (PyObject_IsInstance(obj, time_type)) { + if (PyObject_IsInstance(obj, time_type) == 1) { info.paramSQLType = SQL_TYPE_TIME; - info.paramCType = PARAM_C_TYPE_TEXT; + info.paramCType = PARAM_C_TYPE_TEXT; // matches slow path (its SQL_C_CHAR is -8 = SQL_C_WCHAR) info.columnSize = 16; info.decimalDigits = 6; py::handle h(obj); @@ -673,34 +677,28 @@ std::vector DetectParamTypes(py::list& params) { int minute = h.attr("minute").cast(); int second = h.attr("second").cast(); int microsecond = h.attr("microsecond").cast(); + // Always include microseconds (matches Python's isoformat(timespec="microseconds")). char buf[32]; - if (microsecond > 0) { - snprintf(buf, sizeof(buf), "%02d:%02d:%02d.%06d", hour, minute, second, microsecond); - } else { - snprintf(buf, sizeof(buf), "%02d:%02d:%02d", hour, minute, second); - } + snprintf(buf, sizeof(buf), "%02d:%02d:%02d.%06d", hour, minute, second, microsecond); py::str time_str(buf); Py_ssize_t time_len = py::len(time_str); info.columnSize = std::max(info.columnSize, time_len); - info.utf16Len = time_len; - PyList_SET_ITEM(params.ptr(), i, time_str.release().ptr()); + // PyList_SetItem (lowercase) decrefs the old slot before stealing the new + // reference, so this is safe even if `params` is shared with the caller. + PyList_SetItem(params.ptr(), i, time_str.release().ptr()); continue; } // --- Decimal --- - if (PyObject_IsInstance(obj, decimal_type)) { + if (PyObject_IsInstance(obj, decimal_type) == 1) { py::handle h(obj); py::object as_tuple = h.attr("as_tuple")(); py::object exponent_obj = as_tuple.attr("exponent"); + // NaN / Infinity / sNaN: refuse rather than silently writing 0. if (py::isinstance(exponent_obj)) { - info.paramSQLType = SQL_NUMERIC; - info.paramCType = SQL_C_NUMERIC; - info.columnSize = MAX_NUMERIC_PRECISION; - info.decimalDigits = 0; - py::object numeric_data = build_numeric_data(py::reinterpret_borrow(h)); - PyList_SET_ITEM(params.ptr(), i, numeric_data.release().ptr()); - continue; + throw py::value_error( + "Cannot bind non-finite Decimal (NaN/Infinity) as SQL NUMERIC"); } py::tuple digits = as_tuple.attr("digits").cast(); @@ -728,12 +726,11 @@ std::vector DetectParamTypes(py::list& params) { if (dval >= MONEY_MIN && dval <= MONEY_MAX) { py::str formatted = h.attr("__format__")(py::str("f")); info.paramSQLType = SQL_VARCHAR; - info.paramCType = PARAM_C_TYPE_TEXT; + info.paramCType = PARAM_C_TYPE_TEXT; // matches slow path Py_ssize_t fmtLen = py::len(formatted); info.columnSize = fmtLen; - info.utf16Len = fmtLen; info.decimalDigits = 0; - PyList_SET_ITEM(params.ptr(), i, formatted.release().ptr()); + PyList_SetItem(params.ptr(), i, formatted.release().ptr()); continue; } @@ -744,19 +741,19 @@ std::vector DetectParamTypes(py::list& params) { NumericData nd = numeric_data.cast(); info.columnSize = nd.precision; info.decimalDigits = nd.scale; - PyList_SET_ITEM(params.ptr(), i, numeric_data.release().ptr()); + PyList_SetItem(params.ptr(), i, numeric_data.release().ptr()); continue; } // --- UUID --- - if (PyObject_IsInstance(obj, uuid_type)) { + if (PyObject_IsInstance(obj, uuid_type) == 1) { py::handle h(obj); py::bytes bytes_le = h.attr("bytes_le"); info.paramSQLType = SQL_GUID; info.paramCType = SQL_C_GUID; info.columnSize = 16; info.decimalDigits = 0; - PyList_SET_ITEM(params.ptr(), i, bytes_le.release().ptr()); + PyList_SetItem(params.ptr(), i, bytes_le.release().ptr()); continue; } @@ -2505,23 +2502,26 @@ SQLRETURN SQLExecuteFast_wrap(const SqlHandlePtr statementHandle, (SQLPOINTER)SQL_CONCUR_READ_ONLY, 0); } - // Match the slow path's encoding-dict contract: keys are "encoding" and "ctype". - // Only honor the user's encoding when their preferred ctype is SQL_C_CHAR; - // otherwise the default ctype is SQL_C_WCHAR and the "encoding" value is - // meant for wide-char paths (e.g. "utf-16le") and would corrupt the - // SQL_C_CHAR DAE/inline path that operates on byte data. + // The encoding-settings dict has the form {"encoding": str, "ctype": int}. + // Note: the Python layer's SQL_C_CHAR constant is numerically -8, the same + // as ODBC's SQL_C_WCHAR. As a result, the only path that genuinely uses + // byte-level character encoding is when the user explicitly opts in via + // setencoding(..., ctype=mssql_python.SQL_CHAR) (which sends ctype=1, the + // real ODBC SQL_CHAR). We default to utf-8 and only honor the dict's + // encoding when ctype == 1 (real ODBC SQL_CHAR). Otherwise the user's + // "encoding" value is meant for the wide-char path and we leave it alone. std::string charEncoding = "utf-8"; if (encoding_settings.contains("ctype") && encoding_settings.contains("encoding")) { int ctype = encoding_settings["ctype"].cast(); - if (ctype == SQL_C_CHAR) { + if (ctype == SQL_C_CHAR /* real ODBC value: 1 */) { charEncoding = encoding_settings["encoding"].cast(); } } - // Shallow-copy the parameter list so DetectParamTypes' in-place - // PyList_SET_ITEM never mutates the caller's list. The cost is one - // PyList_New + N refcount bumps; cheap relative to ODBC binding. - params = py::list(params); + // The cursor.py caller always passes a fresh `list(actual_params)` so this + // function is free to mutate slots in place. Even so, every site below uses + // PyList_SetItem (which decrefs the old slot before stealing the new ref), + // so the function is safe regardless of who owns the list. RETCODE rc; bool already_prepared = is_stmt_prepared[0].cast(); diff --git a/tests/test_023_fast_path_parity.py b/tests/test_023_fast_path_parity.py index 09da6e00..331a4700 100644 --- a/tests/test_023_fast_path_parity.py +++ b/tests/test_023_fast_path_parity.py @@ -1,62 +1,54 @@ """ -Parity tests: assert that fast path (DetectParamTypes in C++) and slow path -(_map_sql_type in Python) produce identical query results for representative -parameter types. +Parity tests: assert that fast path (C++ DetectParamTypes + DDBCSQLExecuteFast) +and slow path (Python _map_sql_type + DDBCSQLExecute) produce identical query +results for representative parameter types. -The fast path runs by default. The slow path is forced by calling setinputsizes() -with a non-None entry, which triggers cursor.execute()'s slow-path branch. +Uses the project's `cursor` fixture from conftest.py so the tests work in any +environment that runs the rest of the suite. """ -import pytest import datetime import decimal +import gc import uuid -from mssql_python import connect - -import os +import weakref -CONN_STR = os.environ.get( - "DB_CONNECTION_STRING", - "Server=localhost;Database=master;Uid=sa;Pwd=Str0ng@Passw0rd123;TrustServerCertificate=yes", -) +import pytest +from mssql_python.constants import ConstantsDDBC as ddbc_sql_const -@pytest.fixture -def conn(): - c = connect(CONN_STR) - yield c - c.close() +# --------------------------------------------------------------------------- +# Helpers +# --------------------------------------------------------------------------- -def _roundtrip(cursor, value): - """Round-trip a single parameter through SELECT ? and return the result.""" +def _fast_path_roundtrip(cursor, value): + """Default fast path: no setinputsizes.""" cursor.execute("SELECT ?", [value]) return cursor.fetchone()[0] -def _force_slow_path_roundtrip(cursor, value): - """Force slow path via setinputsizes(None for that param) — any non-empty - inputsizes list with a non-None entry triggers the legacy code path.""" - # Empty list with at least one entry that's not None forces slow path. - # Using SQL_VARCHAR(8000) as an opaque "no override" placeholder. - from mssql_python import ddbc_bindings - - # A None entry means "infer", which is fine — the slow path still runs because - # _inputsizes is set (any non-empty list with at least one non-None entry). - # We need at least one non-None entry to flip use_fast_path to False. - cursor.setinputsizes([None]) # Has at least one entry but no override - # Wait — None doesn't trigger slow path. We need a real override. - # Use SQL_VARCHAR which is identity-ish for strings. - cursor.setinputsizes([(1, 0, 0)]) # (sqlType, size, decimal) tuple - cursor.execute("SELECT ?", [value]) - cursor.setinputsizes(None) # Reset - return cursor.fetchone()[0] +def _slow_path_roundtrip(cursor, value, sql_type, column_size): + """Force the slow path by setting an explicit inputsizes entry. The fast + path is gated on `not (self._inputsizes and any(s is not None ...))`, so a + non-None tuple here flips us to the legacy Python type-detection path.""" + cursor.setinputsizes([(sql_type, column_size, 0)]) + try: + cursor.execute("SELECT ?", [value]) + return cursor.fetchone()[0] + finally: + cursor.setinputsizes(None) + + +# --------------------------------------------------------------------------- +# Fast-path coverage: representative type matrix +# --------------------------------------------------------------------------- @pytest.mark.parametrize( "value", [ - # int range detection + # int range detection (TINYINT / SMALLINT / INTEGER / BIGINT) 0, 1, 255, @@ -75,7 +67,7 @@ def _force_slow_path_roundtrip(cursor, value): 0.0, 3.14, -1.5e10, - # str (ASCII inline + DAE + unicode) + # str (ASCII inline) "", "hello", "a" * 100, @@ -85,67 +77,120 @@ def _force_slow_path_roundtrip(cursor, value): b"x" * 100, ], ) -def test_fast_path_roundtrip(conn, value): - """Fast path produces identical results regardless of value type.""" - cur = conn.cursor() - result = _roundtrip(cur, value) - assert ( - result == value - ), f"Roundtrip mismatch for {type(value).__name__} {value!r}: got {result!r}" +def test_fast_path_basic_types(cursor, value): + """Fast path round-trips representative scalar types correctly.""" + result = _fast_path_roundtrip(cursor, value) + assert result == value, ( + f"Fast-path roundtrip mismatch for {type(value).__name__} {value!r}: " f"got {result!r}" + ) + +# --------------------------------------------------------------------------- +# Subclass support — regression for the *_CheckExact bug from PR review +# --------------------------------------------------------------------------- -def test_int_subclass(conn): - """int subclasses must work (regression test for *_CheckExact bug).""" +def test_int_subclass(cursor): class MyInt(int): pass - cur = conn.cursor() - assert _roundtrip(cur, MyInt(42)) == 42 + assert _fast_path_roundtrip(cursor, MyInt(42)) == 42 -def test_str_subclass(conn): - """str subclasses must work.""" - +def test_str_subclass(cursor): class MyStr(str): pass - cur = conn.cursor() - assert _roundtrip(cur, MyStr("hello")) == "hello" - + assert _fast_path_roundtrip(cursor, MyStr("hello")) == "hello" -def test_bytes_subclass(conn): - """bytes subclasses must work.""" +def test_bytes_subclass(cursor): class MyBytes(bytes): pass - cur = conn.cursor() - assert _roundtrip(cur, MyBytes(b"hello")) == b"hello" + assert _fast_path_roundtrip(cursor, MyBytes(b"hello")) == b"hello" -def test_float_subclass(conn): - """float subclasses must work.""" - +def test_float_subclass(cursor): class MyFloat(float): pass - cur = conn.cursor() - assert _roundtrip(cur, MyFloat(3.14)) == 3.14 + assert _fast_path_roundtrip(cursor, MyFloat(3.14)) == 3.14 + +# --------------------------------------------------------------------------- +# Caller-list isolation and refcount safety +# --------------------------------------------------------------------------- -def test_caller_param_list_not_mutated(conn): + +def test_caller_param_list_not_mutated(cursor): """DetectParamTypes must not mutate the caller's parameter list.""" - cur = conn.cursor() params = ["hello", 42, 3.14, datetime.date(2024, 1, 1), uuid.uuid4()] snapshot = list(params) - cur.execute("SELECT ?, ?, ?, ?, ?", params) - cur.fetchone() + cursor.execute("SELECT ?, ?, ?, ?, ?", params) + cursor.fetchone() assert params == snapshot, f"Caller list was mutated: {params} != {snapshot}" -def test_unsupported_type_raises_typeerror(conn): - """Unknown parameter types must raise TypeError, matching slow path.""" - cur = conn.cursor() +def test_no_refcount_leak_on_in_place_replacement(cursor): + """Decimal/UUID/time params get replaced in-place inside DetectParamTypes + via PyList_SetItem. The replaced object must have its reference dropped — + a regression caught in PR review where PyList_SET_ITEM (uppercase, no + decref) leaked one reference per replaced item per execute.""" + + class TrackedDec(decimal.Decimal): + pass + + td = TrackedDec("123.45") + ref = weakref.ref(td) + params = [td] + del td # drop our local strong reference + + cursor.execute("SELECT ?", params) + cursor.fetchone() + del params # drop the list's strong reference + gc.collect() + + assert ref() is None, ( + "Decimal parameter was leaked: PyList_SetItem must decref the old " + "slot before stealing the new reference." + ) + + +# --------------------------------------------------------------------------- +# Error semantics +# --------------------------------------------------------------------------- + + +def test_unsupported_type_raises_typeerror(cursor): + """Fast path must raise TypeError for unknown parameter types — matching + the slow path's `_map_sql_type` final branch.""" with pytest.raises(TypeError): - cur.execute("SELECT ?", [{1, 2, 3}]) # set is not supported + cursor.execute("SELECT ?", [{1, 2, 3}]) # set is not bindable + + +def test_decimal_nan_rejected(cursor): + """Non-finite Decimals must raise rather than silently bind as 0.""" + with pytest.raises(Exception): # ValueError or DataError, not silent zero + cursor.execute("SELECT ?", [decimal.Decimal("NaN")]) + + +# --------------------------------------------------------------------------- +# Fast-vs-slow parity for representative types +# --------------------------------------------------------------------------- + + +@pytest.mark.parametrize( + "value, sql_type, column_size", + [ + ("hello", ddbc_sql_const.SQL_VARCHAR.value, 5), + (42, ddbc_sql_const.SQL_INTEGER.value, 0), + (3.14, ddbc_sql_const.SQL_DOUBLE.value, 0), + (b"data", ddbc_sql_const.SQL_VARBINARY.value, 4), + ], +) +def test_fast_slow_path_parity(cursor, value, sql_type, column_size): + """Same input through both paths produces the same output.""" + fast = _fast_path_roundtrip(cursor, value) + slow = _slow_path_roundtrip(cursor, value, sql_type=sql_type, column_size=column_size) + assert fast == slow, f"Fast/slow path divergence for {value!r}: fast={fast!r} slow={slow!r}" From ba65f46ec9a180f82a5460c06f8d0ba0ef2f1800 Mon Sep 17 00:00:00 2001 From: Gaurav Sharma Date: Wed, 1 Jul 2026 12:00:09 +0530 Subject: [PATCH 08/29] FIX: Address rubber-duck review findings in fast path - Honor use_prepare flag (was silently ignored, always preparing) - Move DetectParamTypes before SQLPrepare to prevent half-prepared state - Fix bytearray DAE crash (pybind11 bytes caster doesn't handle bytearray) - Replace lossy double MONEY comparison with exact Decimal arithmetic - Add SMALLMONEY range detection (was missing from fast path) - Handle PyObject_IsInstance error return (-1) with proper exception propagation - Clear describe cache on prepare (matching slow path) - Add edge case tests: large bytearray/bytes/string DAE, MONEY boundaries, Infinity rejection, embedded nulls Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- mssql_python/pybind/ddbc_bindings.cpp | 130 +++++++++++++++++++------- tests/test_023_fast_path_parity.py | 92 ++++++++++++++++++ 2 files changed, 186 insertions(+), 36 deletions(-) diff --git a/mssql_python/pybind/ddbc_bindings.cpp b/mssql_python/pybind/ddbc_bindings.cpp index f9ee77aa..3f8f17d2 100644 --- a/mssql_python/pybind/ddbc_bindings.cpp +++ b/mssql_python/pybind/ddbc_bindings.cpp @@ -486,9 +486,13 @@ static constexpr int MAX_INLINE_BINARY = 8000; // SQL Server maximum numeric precision static constexpr int MAX_NUMERIC_PRECISION = 38; -// MONEY range: -922,337,203,685,477.5808 to 922,337,203,685,477.5807 -static constexpr double MONEY_MIN = -922337203685477.5808; -static constexpr double MONEY_MAX = 922337203685477.5807; +// MONEY/SMALLMONEY ranges are compared using exact Decimal arithmetic (not +// double) to avoid boundary misclassification. These string constants are +// converted to Python Decimal objects at first use via PythonObjectCache. +static const char* SMALLMONEY_MIN_STR = "-214748.3648"; +static const char* SMALLMONEY_MAX_STR = "214748.3647"; +static const char* MONEY_MIN_STR = "-922337203685477.5808"; +static const char* MONEY_MAX_STR = "922337203685477.5807"; // Platform-specific text C type: unixODBC requires all text as wide chars on // Linux/macOS. On Windows the ODBC driver accepts SQL_C_CHAR for ASCII text. @@ -630,6 +634,7 @@ std::vector DetectParamTypes(py::list& params) { // Unicode → SQL_WVARCHAR + SQL_C_WCHAR (wide-char streaming) info.isDAE = true; info.columnSize = 0; + info.utf16Len = utf16_len; info.dataPtr = py::reinterpret_borrow(py::handle(obj)); info.paramSQLType = is_unicode ? SQL_WVARCHAR : SQL_VARCHAR; info.paramCType = is_unicode ? SQL_C_WCHAR : PARAM_C_TYPE_TEXT; @@ -673,7 +678,9 @@ std::vector DetectParamTypes(py::list& params) { } // --- datetime (must check before date, since datetime is subclass of date) --- - if (PyObject_IsInstance(obj, datetime_type) == 1) { + int is_datetime = PyObject_IsInstance(obj, datetime_type); + if (is_datetime == -1) throw py::error_already_set(); + if (is_datetime == 1) { py::handle h(obj); py::object tzinfo = h.attr("tzinfo"); if (!tzinfo.is_none()) { @@ -691,7 +698,9 @@ std::vector DetectParamTypes(py::list& params) { } // --- date --- - if (PyObject_IsInstance(obj, date_type) == 1) { + int is_date = PyObject_IsInstance(obj, date_type); + if (is_date == -1) throw py::error_already_set(); + if (is_date == 1) { info.paramSQLType = SQL_TYPE_DATE; info.paramCType = SQL_C_TYPE_DATE; info.columnSize = 10; @@ -700,7 +709,9 @@ std::vector DetectParamTypes(py::list& params) { } // --- time (normalized to string for binding) --- - if (PyObject_IsInstance(obj, time_type) == 1) { + int is_time = PyObject_IsInstance(obj, time_type); + if (is_time == -1) throw py::error_already_set(); + if (is_time == 1) { info.paramSQLType = SQL_TYPE_TIME; info.paramCType = PARAM_C_TYPE_TEXT; // matches slow path (its SQL_C_CHAR is -8 = SQL_C_WCHAR) info.columnSize = 16; @@ -723,7 +734,9 @@ std::vector DetectParamTypes(py::list& params) { } // --- Decimal --- - if (PyObject_IsInstance(obj, decimal_type) == 1) { + int is_decimal = PyObject_IsInstance(obj, decimal_type); + if (is_decimal == -1) throw py::error_already_set(); + if (is_decimal == 1) { py::handle h(obj); py::object as_tuple = h.attr("as_tuple")(); py::object exponent_obj = as_tuple.attr("exponent"); @@ -755,16 +768,36 @@ std::vector DetectParamTypes(py::list& params) { // SMALLMONEY/MONEY range — bind as formatted VARCHAR string // to match SQL Server's fixed-point money semantics. - double dval = h.attr("__float__")().cast(); - if (dval >= MONEY_MIN && dval <= MONEY_MAX) { - py::str formatted = h.attr("__format__")(py::str("f")); - info.paramSQLType = SQL_VARCHAR; - info.paramCType = PARAM_C_TYPE_TEXT; // matches slow path - Py_ssize_t fmtLen = py::len(formatted); - info.columnSize = fmtLen; - info.decimalDigits = 0; - PyList_SetItem(params.ptr(), i, formatted.release().ptr()); - continue; + // Use exact Decimal comparison (not double) to avoid boundary misclassification. + { + py::object decimal_mod = py::module_::import("decimal"); + py::object Decimal = decimal_mod.attr("Decimal"); + py::object py_param = py::reinterpret_borrow(py::handle(obj)); + py::object sm_min = Decimal(py::str(SMALLMONEY_MIN_STR)); + py::object sm_max = Decimal(py::str(SMALLMONEY_MAX_STR)); + py::object m_min = Decimal(py::str(MONEY_MIN_STR)); + py::object m_max = Decimal(py::str(MONEY_MAX_STR)); + + bool in_money_range = false; + // Check SMALLMONEY first (subset of MONEY) + if (py_param.attr("__ge__")(sm_min).cast() && + py_param.attr("__le__")(sm_max).cast()) { + in_money_range = true; + } else if (py_param.attr("__ge__")(m_min).cast() && + py_param.attr("__le__")(m_max).cast()) { + in_money_range = true; + } + + if (in_money_range) { + py::str formatted = h.attr("__format__")(py::str("f")); + info.paramSQLType = SQL_VARCHAR; + info.paramCType = PARAM_C_TYPE_TEXT; + Py_ssize_t fmtLen = py::len(formatted); + info.columnSize = fmtLen; + info.decimalDigits = 0; + PyList_SetItem(params.ptr(), i, formatted.release().ptr()); + continue; + } } // Generic numeric binding via SQL_NUMERIC_STRUCT @@ -779,7 +812,9 @@ std::vector DetectParamTypes(py::list& params) { } // --- UUID --- - if (PyObject_IsInstance(obj, uuid_type) == 1) { + int is_uuid = PyObject_IsInstance(obj, uuid_type); + if (is_uuid == -1) throw py::error_already_set(); + if (is_uuid == 1) { py::handle h(obj); py::bytes bytes_le = h.attr("bytes_le"); info.paramSQLType = SQL_GUID; @@ -2430,16 +2465,15 @@ SQLRETURN SQLExecute_wrap(const SqlHandlePtr statementHandle, const std::u16stri // SQLExecuteFast — single C++ pipeline: DetectParamTypes → BindParameters → SQLExecute // No ParamInfo objects cross the pybind11 boundary. // -// Always uses SQLPrepare (not ExecDirect) because parameterized queries -// benefit from prepared plan reuse, and the fast path is only invoked -// when parameters are present. The use_prepare flag from the caller is -// acknowledged but overridden — this is a perf-only code path. +// Honors use_prepare: when true, uses SQLPrepare + SQLExecute (benefiting from +// plan reuse). When false but already prepared, reuses the existing plan. +// When false and not prepared, throws (matching slow path behavior). // --------------------------------------------------------------------------- SQLRETURN SQLExecuteFast_wrap(const SqlHandlePtr statementHandle, const std::u16string& query, py::list params, py::list is_stmt_prepared, - bool /*use_prepare*/, + bool use_prepare, const py::dict& encoding_settings) { if (!statementHandle || !statementHandle->get()) { return SQL_INVALID_HANDLE; @@ -2476,22 +2510,33 @@ SQLRETURN SQLExecuteFast_wrap(const SqlHandlePtr statementHandle, // PyList_SetItem (which decrefs the old slot before stealing the new ref), // so the function is safe regardless of who owns the list. + // Run DetectParamTypes BEFORE SQLPrepare so that type-detection errors + // (unsupported type, NaN Decimal, precision overflow) don't leave the + // cursor in a half-prepared state. + std::vector paramInfos = DetectParamTypes(params); + RETCODE rc; bool already_prepared = is_stmt_prepared[0].cast(); - // Prepare if needed (fast path always uses prepare for parameterized queries) + // Honor use_prepare flag (matching slow path behavior): + // - use_prepare=true: prepare now (or reuse if same SQL already prepared) + // - use_prepare=false + already prepared: reuse existing plan + // - use_prepare=false + not prepared: error (cannot execute unprepared) if (!already_prepared) { - SQLWCHAR* queryPtr = reinterpretU16stringAsSqlWChar(query); - { - py::gil_scoped_release release; - rc = SQLPrepare_ptr(hStmt, queryPtr, SQL_NTS); + if (use_prepare) { + SQLWCHAR* queryPtr = reinterpretU16stringAsSqlWChar(query); + { + py::gil_scoped_release release; + rc = SQLPrepare_ptr(hStmt, queryPtr, SQL_NTS); + } + if (!SQL_SUCCEEDED(rc)) return rc; + statementHandle->clearDescribeCache(); + is_stmt_prepared[0] = py::bool_(true); + } else { + ThrowStdException("Cannot execute unprepared statement"); } - if (!SQL_SUCCEEDED(rc)) return rc; - is_stmt_prepared[0] = py::bool_(true); } - // DetectParamTypes + BindParameters in one shot — ParamInfo stays in C++ - std::vector paramInfos = DetectParamTypes(params); std::vector> paramBuffers; rc = BindParameters(*statementHandle, hStmt, params, paramInfos, paramBuffers, charEncoding); if (!SQL_SUCCEEDED(rc)) return rc; @@ -2567,10 +2612,23 @@ SQLRETURN SQLExecuteFast_wrap(const SqlHandlePtr statementHandle, } } else if (py::isinstance(pyObj) || py::isinstance(pyObj)) { - py::bytes b = pyObj.cast(); - std::string s = b; - const char* dataPtr = s.data(); - size_t totalBytes = s.size(); + // Handle bytes and bytearray separately — pybind11's bytes + // caster does not safely convert bytearray. + const char* dataPtr = nullptr; + size_t totalBytes = 0; + std::string bytesStorage; // lifetime must span the loop + + if (py::isinstance(pyObj)) { + bytesStorage = pyObj.cast(); + dataPtr = bytesStorage.data(); + totalBytes = bytesStorage.size(); + } else { + // bytearray: use raw buffer access + PyObject* ba = pyObj.ptr(); + dataPtr = PyByteArray_AS_STRING(ba); + totalBytes = static_cast(PyByteArray_GET_SIZE(ba)); + } + for (size_t offset = 0; offset < totalBytes; offset += DAE_CHUNK_SIZE) { size_t len = std::min(static_cast(DAE_CHUNK_SIZE), totalBytes - offset); diff --git a/tests/test_023_fast_path_parity.py b/tests/test_023_fast_path_parity.py index 331a4700..7aaaa6a8 100644 --- a/tests/test_023_fast_path_parity.py +++ b/tests/test_023_fast_path_parity.py @@ -194,3 +194,95 @@ def test_fast_slow_path_parity(cursor, value, sql_type, column_size): fast = _fast_path_roundtrip(cursor, value) slow = _slow_path_roundtrip(cursor, value, sql_type=sql_type, column_size=column_size) assert fast == slow, f"Fast/slow path divergence for {value!r}: fast={fast!r} slow={slow!r}" + + +# --------------------------------------------------------------------------- +# Edge case tests (issues caught in rubber-duck review) +# --------------------------------------------------------------------------- + + +def test_large_bytearray_dae(cursor): + """Large bytearray (>8000 bytes) must stream via DAE without crashing. + This catches the pybind11 bytes-cast-from-bytearray bug.""" + large_ba = bytearray(b"\xAB" * 10000) + cursor.execute( + "SELECT DATALENGTH(CAST(? AS VARBINARY(MAX)))", [large_ba] + ) + result = cursor.fetchone()[0] + assert result == 10000 + + +def test_large_bytes_dae(cursor): + """Large bytes (>8000 bytes) must stream via DAE correctly.""" + large_b = b"\xCD" * 10000 + cursor.execute( + "SELECT DATALENGTH(CAST(? AS VARBINARY(MAX)))", [large_b] + ) + result = cursor.fetchone()[0] + assert result == 10000 + + +def test_large_string_dae(cursor): + """Large string (>4000 chars) must stream via DAE correctly.""" + large_str = "x" * 5000 + cursor.execute("SELECT LEN(?)", [large_str]) + result = cursor.fetchone()[0] + assert result == 5000 + + +def test_large_unicode_string_dae(cursor): + """Large unicode string (>4000 UTF-16 code units) streams via DAE.""" + large_str = "\u00e9" * 5000 # é = 1 UTF-16 code unit each + cursor.execute("SELECT LEN(?)", [large_str]) + result = cursor.fetchone()[0] + assert result == 5000 + + +@pytest.mark.parametrize( + "value", + [ + decimal.Decimal("-922337203685477.5808"), # MONEY_MIN boundary + decimal.Decimal("922337203685477.5807"), # MONEY_MAX boundary + decimal.Decimal("-214748.3648"), # SMALLMONEY_MIN + decimal.Decimal("214748.3647"), # SMALLMONEY_MAX + decimal.Decimal("0.01"), # typical money value + ], +) +def test_decimal_money_boundary(cursor, value): + """Decimal values at MONEY/SMALLMONEY boundaries must round-trip correctly.""" + cursor.execute("SELECT CAST(? AS DECIMAL(38,4))", [value]) + result = cursor.fetchone()[0] + assert result == value, f"MONEY boundary mismatch: sent {value}, got {result}" + + +def test_decimal_outside_money_uses_numeric(cursor): + """Decimal outside MONEY range must use SQL_NUMERIC binding.""" + # One unit above MONEY_MAX + value = decimal.Decimal("922337203685477.5808") + cursor.execute("SELECT CAST(? AS DECIMAL(38,4))", [value]) + result = cursor.fetchone()[0] + assert result == value + + +def test_decimal_infinity_rejected(cursor): + """Decimal('Infinity') must raise, not silently bind as 0.""" + with pytest.raises(Exception): + cursor.execute("SELECT ?", [decimal.Decimal("Infinity")]) + + +def test_binary_with_embedded_nulls(cursor): + """Binary data with embedded null bytes must not be truncated.""" + data = b"\x00\x01\x00\x02\x00" + cursor.execute( + "SELECT DATALENGTH(CAST(? AS VARBINARY(MAX)))", [data] + ) + result = cursor.fetchone()[0] + assert result == 5 + + +def test_string_with_embedded_nulls(cursor): + """String with embedded NUL chars must not be truncated.""" + value = "hello\x00world" + cursor.execute("SELECT LEN(?)", [value]) + result = cursor.fetchone()[0] + assert result == 11 From dc1460b2b68f021ad029ff1ddb99f50476236572 Mon Sep 17 00:00:00 2001 From: Gaurav Sharma Date: Tue, 14 Jul 2026 11:04:53 +0530 Subject: [PATCH 09/29] STYLE: Fix black formatting in test_023_fast_path_parity Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- tests/test_023_fast_path_parity.py | 26 ++++++++++---------------- 1 file changed, 10 insertions(+), 16 deletions(-) diff --git a/tests/test_023_fast_path_parity.py b/tests/test_023_fast_path_parity.py index 7aaaa6a8..cbd564ba 100644 --- a/tests/test_023_fast_path_parity.py +++ b/tests/test_023_fast_path_parity.py @@ -204,20 +204,16 @@ def test_fast_slow_path_parity(cursor, value, sql_type, column_size): def test_large_bytearray_dae(cursor): """Large bytearray (>8000 bytes) must stream via DAE without crashing. This catches the pybind11 bytes-cast-from-bytearray bug.""" - large_ba = bytearray(b"\xAB" * 10000) - cursor.execute( - "SELECT DATALENGTH(CAST(? AS VARBINARY(MAX)))", [large_ba] - ) + large_ba = bytearray(b"\xab" * 10000) + cursor.execute("SELECT DATALENGTH(CAST(? AS VARBINARY(MAX)))", [large_ba]) result = cursor.fetchone()[0] assert result == 10000 def test_large_bytes_dae(cursor): """Large bytes (>8000 bytes) must stream via DAE correctly.""" - large_b = b"\xCD" * 10000 - cursor.execute( - "SELECT DATALENGTH(CAST(? AS VARBINARY(MAX)))", [large_b] - ) + large_b = b"\xcd" * 10000 + cursor.execute("SELECT DATALENGTH(CAST(? AS VARBINARY(MAX)))", [large_b]) result = cursor.fetchone()[0] assert result == 10000 @@ -241,11 +237,11 @@ def test_large_unicode_string_dae(cursor): @pytest.mark.parametrize( "value", [ - decimal.Decimal("-922337203685477.5808"), # MONEY_MIN boundary - decimal.Decimal("922337203685477.5807"), # MONEY_MAX boundary - decimal.Decimal("-214748.3648"), # SMALLMONEY_MIN - decimal.Decimal("214748.3647"), # SMALLMONEY_MAX - decimal.Decimal("0.01"), # typical money value + decimal.Decimal("-922337203685477.5808"), # MONEY_MIN boundary + decimal.Decimal("922337203685477.5807"), # MONEY_MAX boundary + decimal.Decimal("-214748.3648"), # SMALLMONEY_MIN + decimal.Decimal("214748.3647"), # SMALLMONEY_MAX + decimal.Decimal("0.01"), # typical money value ], ) def test_decimal_money_boundary(cursor, value): @@ -273,9 +269,7 @@ def test_decimal_infinity_rejected(cursor): def test_binary_with_embedded_nulls(cursor): """Binary data with embedded null bytes must not be truncated.""" data = b"\x00\x01\x00\x02\x00" - cursor.execute( - "SELECT DATALENGTH(CAST(? AS VARBINARY(MAX)))", [data] - ) + cursor.execute("SELECT DATALENGTH(CAST(? AS VARBINARY(MAX)))", [data]) result = cursor.fetchone()[0] assert result == 5 From 5cf016ec430a6a001280884f33ecffa7b482a813 Mon Sep 17 00:00:00 2001 From: Gaurav Sharma Date: Tue, 14 Jul 2026 11:15:45 +0530 Subject: [PATCH 10/29] PERF: Convert DetectParamTypes to raw CPython API - Replace pybind11 .attr()/.cast<>() with raw CPython calls throughout DetectParamTypes and build_numeric_data - datetime/date/time: use PyDateTime_Check/PyDate_Check/PyTime_Check macros and PyDateTime_TIME_GET_* accessors (requires PyDateTime_IMPORT) - Decimal: PyObject_CallMethod/GetAttrString/RichCompareBool instead of py::module_::import + py::object .attr() chains - UUID: PyObject_GetAttrString("bytes_le") instead of py::handle .attr() - Cache MONEY/SMALLMONEY Decimal bounds in PythonObjectCache (constructed once at init, not per-call) using cached Python-side constants - Replace magic int range numbers with UINT8_MAX/INT16_MIN/MAX/INT32_MIN/MAX - Proper Py_DECREF cleanup on all error paths in build_numeric_data Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- mssql_python/pybind/ddbc_bindings.cpp | 369 +++++++++++++++++++------- 1 file changed, 268 insertions(+), 101 deletions(-) diff --git a/mssql_python/pybind/ddbc_bindings.cpp b/mssql_python/pybind/ddbc_bindings.cpp index 3f8f17d2..8c6d03d2 100644 --- a/mssql_python/pybind/ddbc_bindings.cpp +++ b/mssql_python/pybind/ddbc_bindings.cpp @@ -19,6 +19,7 @@ #include // std::setw, std::setfill #include #include // std::forward +#include // CPython datetime API (PyDateTime_IMPORT, PyDateTime_GET_*, etc.) //------------------------------------------------------------------------------------------------- @@ -119,10 +120,19 @@ static py::object date_class; static py::object time_class; static py::object decimal_class; static py::object uuid_class; +static PyObject* money_min = nullptr; +static PyObject* money_max = nullptr; +static PyObject* smallmoney_min = nullptr; +static PyObject* smallmoney_max = nullptr; static bool cache_initialized = false; void initialize() { if (!cache_initialized) { + PyDateTime_IMPORT; + if (PyDateTimeAPI == nullptr) { + throw py::error_already_set(); + } + auto datetime_module = py::module_::import("datetime"); datetime_class = datetime_module.attr("datetime"); date_class = datetime_module.attr("date"); @@ -134,6 +144,23 @@ void initialize() { auto uuid_module = py::module_::import("uuid"); uuid_class = uuid_module.attr("UUID"); + PyObject* Decimal = decimal_class.ptr(); + smallmoney_min = PyObject_CallFunction(Decimal, "s", "-214748.3648"); + smallmoney_max = PyObject_CallFunction(Decimal, "s", "214748.3647"); + money_min = PyObject_CallFunction(Decimal, "s", "-922337203685477.5808"); + money_max = PyObject_CallFunction(Decimal, "s", "922337203685477.5807"); + if (!smallmoney_min || !smallmoney_max || !money_min || !money_max) { + Py_XDECREF(smallmoney_min); + Py_XDECREF(smallmoney_max); + Py_XDECREF(money_min); + Py_XDECREF(money_max); + smallmoney_min = nullptr; + smallmoney_max = nullptr; + money_min = nullptr; + money_max = nullptr; + throw py::error_already_set(); + } + cache_initialized = true; } } @@ -486,14 +513,6 @@ static constexpr int MAX_INLINE_BINARY = 8000; // SQL Server maximum numeric precision static constexpr int MAX_NUMERIC_PRECISION = 38; -// MONEY/SMALLMONEY ranges are compared using exact Decimal arithmetic (not -// double) to avoid boundary misclassification. These string constants are -// converted to Python Decimal objects at first use via PythonObjectCache. -static const char* SMALLMONEY_MIN_STR = "-214748.3648"; -static const char* SMALLMONEY_MAX_STR = "214748.3647"; -static const char* MONEY_MIN_STR = "-922337203685477.5808"; -static const char* MONEY_MAX_STR = "922337203685477.5807"; - // Platform-specific text C type: unixODBC requires all text as wide chars on // Linux/macOS. On Windows the ODBC driver accepts SQL_C_CHAR for ASCII text. // This matches the Python slow path's behavior (its SQL_C_CHAR constant is @@ -530,14 +549,11 @@ static py::object build_numeric_data(const py::object& decimal_param); std::vector DetectParamTypes(py::list& params) { PythonObjectCache::initialize(); - const Py_ssize_t n = py::len(params); + const Py_ssize_t n = PyList_GET_SIZE(params.ptr()); std::vector infos(n); PyObject* decimal_type = PythonObjectCache::get_decimal_class().ptr(); - PyObject* uuid_type = PythonObjectCache::get_uuid_class().ptr(); - PyObject* datetime_type = PythonObjectCache::get_datetime_class().ptr(); - PyObject* date_type = PythonObjectCache::get_date_class().ptr(); - PyObject* time_type = PythonObjectCache::get_time_class().ptr(); + PyObject* uuid_type = PythonObjectCache::get_uuid_class().ptr(); for (Py_ssize_t i = 0; i < n; ++i) { ParamInfo& info = infos[i]; @@ -569,15 +585,15 @@ std::vector DetectParamTypes(py::list& params) { int overflow = 0; int64_t val = PyLong_AsLongLongAndOverflow(obj, &overflow); if (overflow == 0 && !PyErr_Occurred()) { - if (val >= 0 && val <= 255) { + if (val >= 0 && val <= UINT8_MAX) { info.paramSQLType = SQL_TINYINT; info.paramCType = SQL_C_TINYINT; info.columnSize = 3; - } else if (val >= -32768 && val <= 32767) { + } else if (val >= INT16_MIN && val <= INT16_MAX) { info.paramSQLType = SQL_SMALLINT; info.paramCType = SQL_C_SHORT; info.columnSize = 5; - } else if (val >= -2147483648LL && val <= 2147483647LL) { + } else if (val >= INT32_MIN && val <= INT32_MAX) { info.paramSQLType = SQL_INTEGER; info.paramCType = SQL_C_LONG; info.columnSize = 10; @@ -621,9 +637,10 @@ std::vector DetectParamTypes(py::list& params) { } } - bool is_unicode = (kind > PyUnicode_1BYTE_KIND) || - (PyUnicode_IS_COMPACT_ASCII(obj) == 0 && kind == PyUnicode_1BYTE_KIND - && PyUnicode_MAX_CHAR_VALUE(obj) > 127); + bool is_unicode = + (kind > PyUnicode_1BYTE_KIND) || + (PyUnicode_IS_COMPACT_ASCII(obj) == 0 && kind == PyUnicode_1BYTE_KIND && + PyUnicode_MAX_CHAR_VALUE(obj) > 127); if (utf16_len > MAX_INLINE_CHAR) { // DAE path: match slow-path types exactly. @@ -662,8 +679,7 @@ std::vector DetectParamTypes(py::list& params) { // --- bytes / bytearray (allow subclasses) --- if (PyBytes_Check(obj) || PyByteArray_Check(obj)) { - Py_ssize_t length = - PyBytes_Check(obj) ? PyBytes_Size(obj) : PyByteArray_Size(obj); + Py_ssize_t length = PyBytes_Check(obj) ? PyBytes_Size(obj) : PyByteArray_Size(obj); info.paramSQLType = SQL_VARBINARY; info.paramCType = SQL_C_BINARY; info.decimalDigits = 0; @@ -678,12 +694,12 @@ std::vector DetectParamTypes(py::list& params) { } // --- datetime (must check before date, since datetime is subclass of date) --- - int is_datetime = PyObject_IsInstance(obj, datetime_type); - if (is_datetime == -1) throw py::error_already_set(); - if (is_datetime == 1) { - py::handle h(obj); - py::object tzinfo = h.attr("tzinfo"); - if (!tzinfo.is_none()) { + if (PyDateTime_Check(obj)) { + PyObject* tzinfo = PyObject_GetAttrString(obj, "tzinfo"); + if (!tzinfo) throw py::error_already_set(); + bool has_tz = (tzinfo != Py_None); + Py_DECREF(tzinfo); + if (has_tz) { info.paramSQLType = SQL_SS_TIMESTAMPOFFSET; info.paramCType = SQL_C_SS_TIMESTAMPOFFSET; info.columnSize = 34; @@ -698,9 +714,7 @@ std::vector DetectParamTypes(py::list& params) { } // --- date --- - int is_date = PyObject_IsInstance(obj, date_type); - if (is_date == -1) throw py::error_already_set(); - if (is_date == 1) { + if (PyDate_Check(obj)) { info.paramSQLType = SQL_TYPE_DATE; info.paramCType = SQL_C_TYPE_DATE; info.columnSize = 10; @@ -709,27 +723,26 @@ std::vector DetectParamTypes(py::list& params) { } // --- time (normalized to string for binding) --- - int is_time = PyObject_IsInstance(obj, time_type); - if (is_time == -1) throw py::error_already_set(); - if (is_time == 1) { + if (PyTime_Check(obj)) { info.paramSQLType = SQL_TYPE_TIME; - info.paramCType = PARAM_C_TYPE_TEXT; // matches slow path (its SQL_C_CHAR is -8 = SQL_C_WCHAR) + info.paramCType = + PARAM_C_TYPE_TEXT; // matches slow path (its SQL_C_CHAR is -8 = SQL_C_WCHAR) info.columnSize = 16; info.decimalDigits = 6; - py::handle h(obj); - int hour = h.attr("hour").cast(); - int minute = h.attr("minute").cast(); - int second = h.attr("second").cast(); - int microsecond = h.attr("microsecond").cast(); + int hour = PyDateTime_TIME_GET_HOUR(obj); + int minute = PyDateTime_TIME_GET_MINUTE(obj); + int second = PyDateTime_TIME_GET_SECOND(obj); + int microsecond = PyDateTime_TIME_GET_MICROSECOND(obj); // Always include microseconds (matches Python's isoformat(timespec="microseconds")). char buf[32]; snprintf(buf, sizeof(buf), "%02d:%02d:%02d.%06d", hour, minute, second, microsecond); - py::str time_str(buf); - Py_ssize_t time_len = py::len(time_str); + PyObject* time_str = PyUnicode_FromString(buf); + if (!time_str) throw py::error_already_set(); + Py_ssize_t time_len = PyUnicode_GET_LENGTH(time_str); info.columnSize = std::max(info.columnSize, time_len); // PyList_SetItem (lowercase) decrefs the old slot before stealing the new // reference, so this is safe even if `params` is shared with the caller. - PyList_SetItem(params.ptr(), i, time_str.release().ptr()); + PyList_SetItem(params.ptr(), i, time_str); continue; } @@ -737,28 +750,50 @@ std::vector DetectParamTypes(py::list& params) { int is_decimal = PyObject_IsInstance(obj, decimal_type); if (is_decimal == -1) throw py::error_already_set(); if (is_decimal == 1) { - py::handle h(obj); - py::object as_tuple = h.attr("as_tuple")(); - py::object exponent_obj = as_tuple.attr("exponent"); + PyObject* as_tuple = PyObject_CallMethod(obj, "as_tuple", NULL); + if (!as_tuple) throw py::error_already_set(); + + PyObject* exponent_obj = PyObject_GetAttrString(as_tuple, "exponent"); + if (!exponent_obj) { + Py_DECREF(as_tuple); + throw py::error_already_set(); + } // NaN / Infinity / sNaN: refuse rather than silently writing 0. - if (py::isinstance(exponent_obj)) { + if (PyUnicode_Check(exponent_obj)) { + Py_DECREF(exponent_obj); + Py_DECREF(as_tuple); throw py::value_error( "Cannot bind non-finite Decimal (NaN/Infinity) as SQL NUMERIC"); } - py::tuple digits = as_tuple.attr("digits").cast(); - int num_digits = static_cast(py::len(digits)); - int exponent = exponent_obj.cast(); + PyObject* digits_obj = PyObject_GetAttrString(as_tuple, "digits"); + if (!digits_obj) { + Py_DECREF(exponent_obj); + Py_DECREF(as_tuple); + throw py::error_already_set(); + } + + Py_ssize_t num_digits = PyTuple_GET_SIZE(digits_obj); + int exponent = static_cast(PyLong_AsLong(exponent_obj)); + Py_DECREF(exponent_obj); + if (exponent == -1 && PyErr_Occurred()) { + Py_DECREF(digits_obj); + Py_DECREF(as_tuple); + throw py::error_already_set(); + } + int precision; if (exponent >= 0) - precision = num_digits + exponent; + precision = static_cast(num_digits) + exponent; else if ((-exponent) <= num_digits) - precision = num_digits; + precision = static_cast(num_digits); else precision = -exponent; if (precision > MAX_NUMERIC_PRECISION) { + Py_DECREF(digits_obj); + Py_DECREF(as_tuple); throw py::value_error( "Precision of the numeric value is too high. " "The maximum precision supported by SQL Server is " + @@ -769,41 +804,53 @@ std::vector DetectParamTypes(py::list& params) { // SMALLMONEY/MONEY range — bind as formatted VARCHAR string // to match SQL Server's fixed-point money semantics. // Use exact Decimal comparison (not double) to avoid boundary misclassification. - { - py::object decimal_mod = py::module_::import("decimal"); - py::object Decimal = decimal_mod.attr("Decimal"); - py::object py_param = py::reinterpret_borrow(py::handle(obj)); - py::object sm_min = Decimal(py::str(SMALLMONEY_MIN_STR)); - py::object sm_max = Decimal(py::str(SMALLMONEY_MAX_STR)); - py::object m_min = Decimal(py::str(MONEY_MIN_STR)); - py::object m_max = Decimal(py::str(MONEY_MAX_STR)); - - bool in_money_range = false; - // Check SMALLMONEY first (subset of MONEY) - if (py_param.attr("__ge__")(sm_min).cast() && - py_param.attr("__le__")(sm_max).cast()) { - in_money_range = true; - } else if (py_param.attr("__ge__")(m_min).cast() && - py_param.attr("__le__")(m_max).cast()) { + bool in_money_range = false; + int cmp_ge = PyObject_RichCompareBool(obj, PythonObjectCache::smallmoney_min, Py_GE); + int cmp_le = PyObject_RichCompareBool(obj, PythonObjectCache::smallmoney_max, Py_LE); + if (cmp_ge == -1 || cmp_le == -1) { + Py_DECREF(digits_obj); + Py_DECREF(as_tuple); + throw py::error_already_set(); + } + if (cmp_ge == 1 && cmp_le == 1) { + in_money_range = true; + } else { + cmp_ge = PyObject_RichCompareBool(obj, PythonObjectCache::money_min, Py_GE); + cmp_le = PyObject_RichCompareBool(obj, PythonObjectCache::money_max, Py_LE); + if (cmp_ge == -1 || cmp_le == -1) { + Py_DECREF(digits_obj); + Py_DECREF(as_tuple); + throw py::error_already_set(); + } + if (cmp_ge == 1 && cmp_le == 1) { in_money_range = true; } + } - if (in_money_range) { - py::str formatted = h.attr("__format__")(py::str("f")); - info.paramSQLType = SQL_VARCHAR; - info.paramCType = PARAM_C_TYPE_TEXT; - Py_ssize_t fmtLen = py::len(formatted); - info.columnSize = fmtLen; - info.decimalDigits = 0; - PyList_SetItem(params.ptr(), i, formatted.release().ptr()); - continue; + if (in_money_range) { + PyObject* formatted = PyObject_CallMethod(obj, "__format__", "s", "f"); + if (!formatted) { + Py_DECREF(digits_obj); + Py_DECREF(as_tuple); + throw py::error_already_set(); } + info.paramSQLType = SQL_VARCHAR; + info.paramCType = PARAM_C_TYPE_TEXT; + info.columnSize = PyUnicode_GET_LENGTH(formatted); + info.decimalDigits = 0; + Py_DECREF(digits_obj); + Py_DECREF(as_tuple); + PyList_SetItem(params.ptr(), i, formatted); + continue; } // Generic numeric binding via SQL_NUMERIC_STRUCT info.paramSQLType = SQL_NUMERIC; info.paramCType = SQL_C_NUMERIC; - py::object numeric_data = build_numeric_data(py::reinterpret_borrow(h)); + Py_DECREF(digits_obj); + Py_DECREF(as_tuple); + py::object numeric_data = + build_numeric_data(py::reinterpret_borrow(py::handle(obj))); NumericData nd = numeric_data.cast(); info.columnSize = nd.precision; info.decimalDigits = nd.scale; @@ -815,13 +862,13 @@ std::vector DetectParamTypes(py::list& params) { int is_uuid = PyObject_IsInstance(obj, uuid_type); if (is_uuid == -1) throw py::error_already_set(); if (is_uuid == 1) { - py::handle h(obj); - py::bytes bytes_le = h.attr("bytes_le"); + PyObject* bytes_le = PyObject_GetAttrString(obj, "bytes_le"); + if (!bytes_le) throw py::error_already_set(); info.paramSQLType = SQL_GUID; info.paramCType = SQL_C_GUID; info.columnSize = 16; info.decimalDigits = 0; - PyList_SetItem(params.ptr(), i, bytes_le.release().ptr()); + PyList_SetItem(params.ptr(), i, bytes_le); continue; } @@ -835,17 +882,50 @@ std::vector DetectParamTypes(py::list& params) { // Helper: build SQL_NUMERIC_STRUCT from Python Decimal static py::object build_numeric_data(const py::object& decimal_param) { - py::object as_tuple = decimal_param.attr("as_tuple")(); - py::tuple digits = as_tuple.attr("digits").cast(); - int sign_val = as_tuple.attr("sign").cast(); - py::object exponent_obj = as_tuple.attr("exponent"); + PyObject* as_tuple = PyObject_CallMethod(decimal_param.ptr(), "as_tuple", NULL); + if (!as_tuple) throw py::error_already_set(); + + PyObject* digits_obj = PyObject_GetAttrString(as_tuple, "digits"); + if (!digits_obj) { + Py_DECREF(as_tuple); + throw py::error_already_set(); + } + PyObject* sign_obj = PyObject_GetAttrString(as_tuple, "sign"); + if (!sign_obj) { + Py_DECREF(digits_obj); + Py_DECREF(as_tuple); + throw py::error_already_set(); + } + PyObject* exponent_obj = PyObject_GetAttrString(as_tuple, "exponent"); + if (!exponent_obj) { + Py_DECREF(sign_obj); + Py_DECREF(digits_obj); + Py_DECREF(as_tuple); + throw py::error_already_set(); + } + + int sign_val = static_cast(PyLong_AsLong(sign_obj)); + Py_DECREF(sign_obj); + if (sign_val == -1 && PyErr_Occurred()) { + Py_DECREF(exponent_obj); + Py_DECREF(digits_obj); + Py_DECREF(as_tuple); + throw py::error_already_set(); + } int exponent = 0; - if (py::isinstance(exponent_obj)) { - exponent = exponent_obj.cast(); + if (PyLong_Check(exponent_obj)) { + exponent = static_cast(PyLong_AsLong(exponent_obj)); + if (exponent == -1 && PyErr_Occurred()) { + Py_DECREF(exponent_obj); + Py_DECREF(digits_obj); + Py_DECREF(as_tuple); + throw py::error_already_set(); + } } + Py_DECREF(exponent_obj); - int num_digits = static_cast(py::len(digits)); + int num_digits = static_cast(PyTuple_GET_SIZE(digits_obj)); int precision, scale; if (exponent >= 0) { precision = num_digits + exponent; @@ -857,31 +937,118 @@ static py::object build_numeric_data(const py::object& decimal_param) { precision = std::max(1, std::min(precision, MAX_NUMERIC_PRECISION)); scale = std::min(scale, precision); - py::object py_zero = py::int_(0); - py::object int_val = py_zero; - for (auto d : digits) { - int_val = int_val * py::int_(10) + d.cast(); + PyObject* py_ten = PyLong_FromLong(10); + PyObject* int_val = PyLong_FromLong(0); + if (!py_ten || !int_val) { + Py_XDECREF(int_val); + Py_XDECREF(py_ten); + Py_DECREF(digits_obj); + Py_DECREF(as_tuple); + throw py::error_already_set(); + } + + const Py_ssize_t digit_count = PyTuple_GET_SIZE(digits_obj); + for (Py_ssize_t i = 0; i < digit_count; ++i) { + PyObject* digit_obj = PyTuple_GET_ITEM(digits_obj, i); + long digit = PyLong_AsLong(digit_obj); + if (digit == -1 && PyErr_Occurred()) { + Py_DECREF(int_val); + Py_DECREF(py_ten); + Py_DECREF(digits_obj); + Py_DECREF(as_tuple); + throw py::error_already_set(); + } + + PyObject* multiplied = PyNumber_Multiply(int_val, py_ten); + Py_DECREF(int_val); + if (!multiplied) { + Py_DECREF(py_ten); + Py_DECREF(digits_obj); + Py_DECREF(as_tuple); + throw py::error_already_set(); + } + + PyObject* py_digit = PyLong_FromLong(digit); + if (!py_digit) { + Py_DECREF(multiplied); + Py_DECREF(py_ten); + Py_DECREF(digits_obj); + Py_DECREF(as_tuple); + throw py::error_already_set(); + } + + int_val = PyNumber_Add(multiplied, py_digit); + Py_DECREF(multiplied); + Py_DECREF(py_digit); + if (!int_val) { + Py_DECREF(py_ten); + Py_DECREF(digits_obj); + Py_DECREF(as_tuple); + throw py::error_already_set(); + } } + if (exponent > 0) { - py::object multiplier = py::int_(1); - for (int j = 0; j < exponent; ++j) - multiplier = multiplier * py::int_(10); - int_val = int_val * multiplier; + PyObject* multiplier = PyLong_FromLong(1); + if (!multiplier) { + Py_DECREF(int_val); + Py_DECREF(py_ten); + Py_DECREF(digits_obj); + Py_DECREF(as_tuple); + throw py::error_already_set(); + } + for (int j = 0; j < exponent; ++j) { + PyObject* next_multiplier = PyNumber_Multiply(multiplier, py_ten); + Py_DECREF(multiplier); + if (!next_multiplier) { + Py_DECREF(int_val); + Py_DECREF(py_ten); + Py_DECREF(digits_obj); + Py_DECREF(as_tuple); + throw py::error_already_set(); + } + multiplier = next_multiplier; + } + PyObject* scaled_val = PyNumber_Multiply(int_val, multiplier); + Py_DECREF(multiplier); + Py_DECREF(int_val); + if (!scaled_val) { + Py_DECREF(py_ten); + Py_DECREF(digits_obj); + Py_DECREF(as_tuple); + throw py::error_already_set(); + } + int_val = scaled_val; } - py::object abs_val = int_val.attr("__abs__")(); - py::bytes val_bytes = abs_val.attr("to_bytes")(py::int_(16), py::str("little")); - std::string val_str = val_bytes.cast(); + PyObject* abs_val = PyNumber_Absolute(int_val); + Py_DECREF(int_val); + Py_DECREF(py_ten); + Py_DECREF(digits_obj); + Py_DECREF(as_tuple); + if (!abs_val) throw py::error_already_set(); + + PyObject* val_bytes = PyObject_CallMethod(abs_val, "to_bytes", "is", 16, "little"); + Py_DECREF(abs_val); + if (!val_bytes) throw py::error_already_set(); + + char* val_buf = nullptr; + Py_ssize_t val_size = 0; + if (PyBytes_AsStringAndSize(val_bytes, &val_buf, &val_size) == -1) { + Py_DECREF(val_bytes); + throw py::error_already_set(); + } NumericData nd; nd.precision = static_cast(precision); nd.scale = static_cast(scale); nd.sign = (sign_val == 0) ? 1 : 0; std::memset(&nd.val[0], 0, SQL_MAX_NUMERIC_LEN); - size_t copy_len = std::min(val_str.size(), static_cast(SQL_MAX_NUMERIC_LEN)); - if (copy_len > 0 && val_str.data() != nullptr) { - std::memcpy(&nd.val[0], val_str.data(), copy_len); + size_t copy_len = std::min(static_cast(val_size), static_cast(SQL_MAX_NUMERIC_LEN)); + if (copy_len > 0 && val_buf != nullptr) { + std::memcpy(&nd.val[0], val_buf, copy_len); } + Py_DECREF(val_bytes); return py::cast(nd); } From 8e8d622b92cc194d74395ebc260465f18a6a8a07 Mon Sep 17 00:00:00 2001 From: Gaurav Sharma Date: Tue, 14 Jul 2026 11:20:04 +0530 Subject: [PATCH 11/29] FIX: Use memcpy_s on MSVC for CodeQL DS121708 compliance Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- mssql_python/pybind/ddbc_bindings.cpp | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/mssql_python/pybind/ddbc_bindings.cpp b/mssql_python/pybind/ddbc_bindings.cpp index 8c6d03d2..a8689b42 100644 --- a/mssql_python/pybind/ddbc_bindings.cpp +++ b/mssql_python/pybind/ddbc_bindings.cpp @@ -1046,7 +1046,12 @@ static py::object build_numeric_data(const py::object& decimal_param) { std::memset(&nd.val[0], 0, SQL_MAX_NUMERIC_LEN); size_t copy_len = std::min(static_cast(val_size), static_cast(SQL_MAX_NUMERIC_LEN)); if (copy_len > 0 && val_buf != nullptr) { - std::memcpy(&nd.val[0], val_buf, copy_len); +#ifdef _MSC_VER + memcpy_s(&nd.val[0], SQL_MAX_NUMERIC_LEN, val_buf, copy_len); +#else + // copy_len is bounded to SQL_MAX_NUMERIC_LEN above — safe by construction + std::memcpy(&nd.val[0], val_buf, copy_len); // NOLINT(clang-analyzer-security.insecureAPI.DeprecatedOrUnsafeBufferHandling) +#endif } Py_DECREF(val_bytes); From cbd2f00106d55822460a2c47f23011ffba3233d0 Mon Sep 17 00:00:00 2001 From: Gaurav Sharma Date: Tue, 14 Jul 2026 11:36:27 +0530 Subject: [PATCH 12/29] =?UTF-8?q?REFACTOR:=20Rename=20DDBCSQLExecuteFast?= =?UTF-8?q?=20=E2=86=92=20DDBCSQLExecute,=20old=20=E2=86=92=20DDBCSQLExecu?= =?UTF-8?q?teLegacy?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The new C++ pipeline is the primary path (99% of calls). The old function is the legacy fallback for setinputsizes users only. Naming should reflect this. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- mssql_python/cursor.py | 8 ++++---- mssql_python/pybind/ddbc_bindings.cpp | 11 ++++++----- tests/test_010_pybind_functions.py | 1 + tests/test_023_fast_path_parity.py | 4 ++-- 4 files changed, 13 insertions(+), 11 deletions(-) diff --git a/mssql_python/cursor.py b/mssql_python/cursor.py index 2c648c86..b147440d 100644 --- a/mssql_python/cursor.py +++ b/mssql_python/cursor.py @@ -1506,14 +1506,14 @@ def execute( # pylint: disable=too-many-locals,too-many-branches,too-many-state self.is_stmt_prepared = [False] effective_use_prepare = use_prepare and not same_sql - # Fast path: when no inputsizes override, do type detection + bind + execute + # Primary path: when no inputsizes override, do type detection + bind + execute # entirely in C++. ParamInfo never crosses the pybind11 boundary. use_fast_path = parameters and not ( self._inputsizes and any(s is not None for s in self._inputsizes) ) if use_fast_path: - ret = ddbc_bindings.DDBCSQLExecuteFast( + ret = ddbc_bindings.DDBCSQLExecute( self.hstmt, operation, parameters, @@ -1522,7 +1522,7 @@ def execute( # pylint: disable=too-many-locals,too-many-branches,too-many-state encoding_settings, ) else: - # Slow path: Python-side type detection (used when setinputsizes overrides are present) + # Legacy path: Python-side type detection (used when setinputsizes overrides are present) parameters_type = [] if parameters: param_info = ddbc_bindings.ParamInfo @@ -1545,7 +1545,7 @@ def execute( # pylint: disable=too-many-locals,too-many-branches,too-many-state parameters_type[i].inputOutputType, ) - ret = ddbc_bindings.DDBCSQLExecute( + ret = ddbc_bindings.DDBCSQLExecuteLegacy( self.hstmt, operation, parameters, diff --git a/mssql_python/pybind/ddbc_bindings.cpp b/mssql_python/pybind/ddbc_bindings.cpp index a8689b42..f2d7eb5c 100644 --- a/mssql_python/pybind/ddbc_bindings.cpp +++ b/mssql_python/pybind/ddbc_bindings.cpp @@ -2396,7 +2396,7 @@ SQLRETURN SQLTables_wrap(SqlHandlePtr StatementHandle, const std::u16string& cat // statement and binds the parameters. Otherwise, it executes the query // directly. 'usePrepare' parameter can be used to disable the prepare step for // queries that might already be prepared in a previous call. -SQLRETURN SQLExecute_wrap(const SqlHandlePtr statementHandle, const std::u16string& query, +SQLRETURN SQLExecuteLegacy_wrap(const SqlHandlePtr statementHandle, const std::u16string& query, const py::list& params, std::vector& paramInfos, py::list& isStmtPrepared, const bool usePrepare, const py::dict& encodingSettings) { @@ -2641,7 +2641,7 @@ SQLRETURN SQLExecute_wrap(const SqlHandlePtr statementHandle, const std::u16stri // plan reuse). When false but already prepared, reuses the existing plan. // When false and not prepared, throws (matching slow path behavior). // --------------------------------------------------------------------------- -SQLRETURN SQLExecuteFast_wrap(const SqlHandlePtr statementHandle, +SQLRETURN SQLExecute_wrap(const SqlHandlePtr statementHandle, const std::u16string& query, py::list params, py::list is_stmt_prepared, @@ -6697,11 +6697,12 @@ PYBIND11_MODULE(ddbc_bindings, m) { m.def("enable_pooling", &enable_pooling, "Enable global connection pooling"); m.def("close_pooling", []() { ConnectionPoolManager::getInstance().closePools(); }); m.def("DDBCSQLExecDirect", &SQLExecDirect_wrap, "Execute a SQL query directly"); - m.def("DDBCSQLExecute", &SQLExecute_wrap, "Prepare and execute T-SQL statements", + m.def("DDBCSQLExecuteLegacy", &SQLExecuteLegacy_wrap, + "Legacy path: accepts pre-built ParamInfo from Python (used with setinputsizes)", py::arg("statementHandle"), py::arg("query"), py::arg("params"), py::arg("paramInfos"), py::arg("isStmtPrepared"), py::arg("usePrepare"), py::arg("encodingSettings")); - m.def("DDBCSQLExecuteFast", &SQLExecuteFast_wrap, - "Fast path: DetectParamTypes + BindParameters + SQLExecute all in C++", + m.def("DDBCSQLExecute", &SQLExecute_wrap, + "Primary path: DetectParamTypes + BindParameters + SQLExecute all in C++", py::arg("statementHandle"), py::arg("query"), py::arg("params"), py::arg("isStmtPrepared"), py::arg("usePrepare"), py::arg("encodingSettings")); m.def("SQLExecuteMany", &SQLExecuteMany_wrap, "Execute statement with multiple parameter sets", diff --git a/tests/test_010_pybind_functions.py b/tests/test_010_pybind_functions.py index 106b64ca..b7374112 100644 --- a/tests/test_010_pybind_functions.py +++ b/tests/test_010_pybind_functions.py @@ -513,6 +513,7 @@ def test_all_exposed_functions_exist(self): "DDBCSetDecimalSeparator", "DDBCSQLExecDirect", "DDBCSQLExecute", + "DDBCSQLExecuteLegacy", "DDBCSQLRowCount", "DDBCSQLFetch", "DDBCSQLNumResultCols", diff --git a/tests/test_023_fast_path_parity.py b/tests/test_023_fast_path_parity.py index cbd564ba..c4f2b383 100644 --- a/tests/test_023_fast_path_parity.py +++ b/tests/test_023_fast_path_parity.py @@ -1,6 +1,6 @@ """ -Parity tests: assert that fast path (C++ DetectParamTypes + DDBCSQLExecuteFast) -and slow path (Python _map_sql_type + DDBCSQLExecute) produce identical query +Parity tests: assert that the primary path (C++ DetectParamTypes + DDBCSQLExecute) +and legacy path (Python _map_sql_type + DDBCSQLExecuteLegacy) produce identical query results for representative parameter types. Uses the project's `cursor` fixture from conftest.py so the tests work in any From fa0e866813d095b2bb8b387070e606a925297928 Mon Sep 17 00:00:00 2001 From: Gaurav Sharma Date: Tue, 14 Jul 2026 11:45:54 +0530 Subject: [PATCH 13/29] REFACTOR: Convert PythonObjectCache + ParamInfo::dataPtr to raw CPython MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Removes unnecessary pybind11 ↔ CPython round-trips in the hot path: - PythonObjectCache types stored as PyObject* (not py::object) - ParamInfo::dataPtr is raw PyObject* with explicit refcount management - DetectParamTypes takes PyObject* directly (not py::list&) - build_numeric_data returns NumericData struct (not py::object) - Added contextual comments explaining non-obvious design decisions Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- mssql_python/pybind/ddbc_bindings.cpp | 375 +++++++++++++++++--------- 1 file changed, 255 insertions(+), 120 deletions(-) diff --git a/mssql_python/pybind/ddbc_bindings.cpp b/mssql_python/pybind/ddbc_bindings.cpp index f2d7eb5c..4681c1dd 100644 --- a/mssql_python/pybind/ddbc_bindings.cpp +++ b/mssql_python/pybind/ddbc_bindings.cpp @@ -103,7 +103,8 @@ inline int EffectiveCharCtypeForFetch(int charCtype, const std::string& charEnco } namespace PythonObjectCache { -py::object get_time_class(); +PyObject* get_time_class(); +py::object get_time_class_obj(); } //------------------------------------------------------------------------------------------------- @@ -115,11 +116,13 @@ py::object get_time_class(); // embedded in macro //------------------------------------------------------------------------------------------------- namespace PythonObjectCache { -static py::object datetime_class; -static py::object date_class; -static py::object time_class; -static py::object decimal_class; -static py::object uuid_class; +// Cached as raw PyObject* to avoid pybind11 refcount wrapper overhead on every access. +// These are immortal (never DECREFed) — module-lifetime singletons, same as CPython's type objects. +static PyObject* datetime_class = nullptr; +static PyObject* date_class = nullptr; +static PyObject* time_class = nullptr; +static PyObject* decimal_class = nullptr; +static PyObject* uuid_class = nullptr; static PyObject* money_min = nullptr; static PyObject* money_max = nullptr; static PyObject* smallmoney_min = nullptr; @@ -133,18 +136,27 @@ void initialize() { throw py::error_already_set(); } - auto datetime_module = py::module_::import("datetime"); - datetime_class = datetime_module.attr("datetime"); - date_class = datetime_module.attr("date"); - time_class = datetime_module.attr("time"); - - auto decimal_module = py::module_::import("decimal"); - decimal_class = decimal_module.attr("Decimal"); - - auto uuid_module = py::module_::import("uuid"); - uuid_class = uuid_module.attr("UUID"); - - PyObject* Decimal = decimal_class.ptr(); + PyObject* datetime_module = PyImport_ImportModule("datetime"); + if (!datetime_module) throw py::error_already_set(); + datetime_class = PyObject_GetAttrString(datetime_module, "datetime"); + date_class = PyObject_GetAttrString(datetime_module, "date"); + time_class = PyObject_GetAttrString(datetime_module, "time"); + Py_DECREF(datetime_module); + if (!datetime_class || !date_class || !time_class) throw py::error_already_set(); + + PyObject* decimal_module = PyImport_ImportModule("decimal"); + if (!decimal_module) throw py::error_already_set(); + decimal_class = PyObject_GetAttrString(decimal_module, "Decimal"); + Py_DECREF(decimal_module); + if (!decimal_class) throw py::error_already_set(); + + PyObject* uuid_module = PyImport_ImportModule("uuid"); + if (!uuid_module) throw py::error_already_set(); + uuid_class = PyObject_GetAttrString(uuid_module, "UUID"); + Py_DECREF(uuid_module); + if (!uuid_class) throw py::error_already_set(); + + PyObject* Decimal = decimal_class; smallmoney_min = PyObject_CallFunction(Decimal, "s", "-214748.3648"); smallmoney_max = PyObject_CallFunction(Decimal, "s", "214748.3647"); money_min = PyObject_CallFunction(Decimal, "s", "-922337203685477.5808"); @@ -165,39 +177,97 @@ void initialize() { } } -py::object get_datetime_class() { +static py::object wrap_cached_or_imported(PyObject* obj) { + if (!obj) throw py::error_already_set(); + // Cached path returns a borrowed module-lifetime singleton. Fallback import path + // returns a fresh reference, so steal it to avoid leaking the rare edge-case object. + return cache_initialized ? py::reinterpret_borrow(py::handle(obj)) + : py::reinterpret_steal(obj); +} + +// Returns cached type. Fallback import only for edge case where cache wasn't initialized +// (e.g., called from legacy path before any fast-path execute). +PyObject* get_datetime_class() { if (cache_initialized && datetime_class) { return datetime_class; } - return py::module_::import("datetime").attr("datetime"); + PyObject* mod = PyImport_ImportModule("datetime"); + if (!mod) return nullptr; + PyObject* cls = PyObject_GetAttrString(mod, "datetime"); + Py_DECREF(mod); + return cls; // caller must check for NULL +} + +py::object get_datetime_class_obj() { + return wrap_cached_or_imported(get_datetime_class()); } -py::object get_date_class() { +// Returns cached type. Fallback import only for edge case where cache wasn't initialized +// (e.g., called from legacy path before any fast-path execute). +PyObject* get_date_class() { if (cache_initialized && date_class) { return date_class; } - return py::module_::import("datetime").attr("date"); + PyObject* mod = PyImport_ImportModule("datetime"); + if (!mod) return nullptr; + PyObject* cls = PyObject_GetAttrString(mod, "date"); + Py_DECREF(mod); + return cls; // caller must check for NULL } -py::object get_time_class() { +py::object get_date_class_obj() { + return wrap_cached_or_imported(get_date_class()); +} + +// Returns cached type. Fallback import only for edge case where cache wasn't initialized +// (e.g., called from legacy path before any fast-path execute). +PyObject* get_time_class() { if (cache_initialized && time_class) { return time_class; } - return py::module_::import("datetime").attr("time"); + PyObject* mod = PyImport_ImportModule("datetime"); + if (!mod) return nullptr; + PyObject* cls = PyObject_GetAttrString(mod, "time"); + Py_DECREF(mod); + return cls; // caller must check for NULL +} + +py::object get_time_class_obj() { + return wrap_cached_or_imported(get_time_class()); } -py::object get_decimal_class() { +// Returns cached type. Fallback import only for edge case where cache wasn't initialized +// (e.g., called from legacy path before any fast-path execute). +PyObject* get_decimal_class() { if (cache_initialized && decimal_class) { return decimal_class; } - return py::module_::import("decimal").attr("Decimal"); + PyObject* mod = PyImport_ImportModule("decimal"); + if (!mod) return nullptr; + PyObject* cls = PyObject_GetAttrString(mod, "Decimal"); + Py_DECREF(mod); + return cls; // caller must check for NULL +} + +py::object get_decimal_class_obj() { + return wrap_cached_or_imported(get_decimal_class()); } -py::object get_uuid_class() { +// Returns cached type. Fallback import only for edge case where cache wasn't initialized +// (e.g., called from legacy path before any fast-path execute). +PyObject* get_uuid_class() { if (cache_initialized && uuid_class) { return uuid_class; } - return py::module_::import("uuid").attr("UUID"); + PyObject* mod = PyImport_ImportModule("uuid"); + if (!mod) return nullptr; + PyObject* cls = PyObject_GetAttrString(mod, "UUID"); + Py_DECREF(mod); + return cls; // caller must check for NULL +} + +py::object get_uuid_class_obj() { + return wrap_cached_or_imported(get_uuid_class()); } } // namespace PythonObjectCache @@ -215,15 +285,45 @@ py::object get_uuid_class() { #pragma GCC diagnostic ignored "-Wattributes" #endif struct ParamInfo { - SQLSMALLINT inputOutputType; - SQLSMALLINT paramCType; - SQLSMALLINT paramSQLType; - SQLULEN columnSize; - SQLSMALLINT decimalDigits; + SQLSMALLINT inputOutputType = SQL_PARAM_INPUT; + SQLSMALLINT paramCType = SQL_C_DEFAULT; + SQLSMALLINT paramSQLType = SQL_UNKNOWN_TYPE; + SQLULEN columnSize = 0; + SQLSMALLINT decimalDigits = 0; SQLLEN strLenOrInd = 0; // Required for DAE bool isDAE = false; // Indicates if we need to stream - py::object dataPtr; - Py_ssize_t utf16Len = 0; // UTF-16 code unit count for string params + // Holds a strong reference to the Python object for DAE (data-at-execution) streaming. + // Raw pointer + manual Py_INCREF/DECREF avoids pybind11 wrapper overhead in the hot loop. + PyObject* dataPtr = nullptr; + Py_ssize_t utf16Len = 0; // UTF-16 code unit count for string params + + ParamInfo() = default; + ~ParamInfo() { Py_XDECREF(dataPtr); } + ParamInfo(const ParamInfo&) = delete; + ParamInfo& operator=(const ParamInfo&) = delete; + ParamInfo(ParamInfo&& other) noexcept + : inputOutputType(other.inputOutputType), paramCType(other.paramCType), + paramSQLType(other.paramSQLType), columnSize(other.columnSize), + decimalDigits(other.decimalDigits), strLenOrInd(other.strLenOrInd), + isDAE(other.isDAE), dataPtr(other.dataPtr), utf16Len(other.utf16Len) { + other.dataPtr = nullptr; + } + ParamInfo& operator=(ParamInfo&& other) noexcept { + if (this != &other) { + Py_XDECREF(dataPtr); + inputOutputType = other.inputOutputType; + paramCType = other.paramCType; + paramSQLType = other.paramSQLType; + columnSize = other.columnSize; + decimalDigits = other.decimalDigits; + strLenOrInd = other.strLenOrInd; + isDAE = other.isDAE; + dataPtr = other.dataPtr; + utf16Len = other.utf16Len; + other.dataPtr = nullptr; + } + return *this; + } }; #ifdef __GNUC__ #pragma GCC diagnostic pop @@ -525,42 +625,46 @@ static constexpr SQLSMALLINT PARAM_C_TYPE_TEXT = SQL_C_CHAR; #endif // Forward declare NumericData helper used by decimal path -static py::object build_numeric_data(const py::object& decimal_param); +static NumericData build_numeric_data(PyObject* decimal_param); // --------------------------------------------------------------------------- -// DetectParamTypes — C++ type detection for the execute() fast path. +// DetectParamTypes: Raw CPython parameter type detection for the primary execute path. // -// Replaces the Python-side _create_parameter_types_list() loop by doing type -// detection entirely in C++ using raw CPython type checks. +// Design decisions: +// 1. Operates on a COPY of the user's param list (cursor.py does list(actual_params)). +// We mutate it in-place (PyList_SetItem) for types that need pre-processing +// (time→isoformat string, Decimal→NumericData, UUID→bytes_le). +// 2. Uses CPython macros (PyLong_Check, PyDateTime_Check, etc.) instead of pybind11's +// py::isinstance<> for ~3x faster type checks (direct struct field test vs virtual call). +// 3. Integer range detection uses constants — these match the SQL Server +// storage engine's range exactly (TINYINT: 0-255, SMALLINT: -32768..32767, etc.) +// 4. String handling inspects UCS kind directly for O(1) ASCII detection rather than +// scanning content — critical for bulk insert scenarios with thousands of params. +// 5. MONEY/SMALLMONEY uses exact Decimal comparison (PyObject_RichCompareBool) to avoid +// double-precision boundary errors (e.g., 214748.3647 would round incorrectly as double). +// --------------------------------------------------------------------------- // // ORDERING MATTERS: // - bool before int (bool is a subclass of int in Python) // - datetime before date (datetime is a subclass of date) // -// Some types mutate the params list in-place via PyList_SetItem (which -// decrefs the old slot before stealing the new ref): -// - time → normalized to "HH:MM:SS.ffffff" string -// - Decimal in MONEY range → formatted via __format__("f") -// - Decimal (generic) → converted to NumericData struct -// - UUID → replaced with bytes_le -// This is safe because execute() already copies the caller's param list -// (via list(actual_params)) before reaching this function. -// --------------------------------------------------------------------------- -std::vector DetectParamTypes(py::list& params) { +// Takes a raw PyObject* (must be a list). Caller guarantees it's a fresh copy +// (cursor.py does list(actual_params)), so in-place mutation via PyList_SetItem is safe. +std::vector DetectParamTypes(PyObject* params) { PythonObjectCache::initialize(); - const Py_ssize_t n = PyList_GET_SIZE(params.ptr()); + const Py_ssize_t n = PyList_GET_SIZE(params); std::vector infos(n); - PyObject* decimal_type = PythonObjectCache::get_decimal_class().ptr(); - PyObject* uuid_type = PythonObjectCache::get_uuid_class().ptr(); + PyObject* decimal_type = PythonObjectCache::get_decimal_class(); + PyObject* uuid_type = PythonObjectCache::get_uuid_class(); for (Py_ssize_t i = 0; i < n; ++i) { ParamInfo& info = infos[i]; info.inputOutputType = SQL_PARAM_INPUT; info.isDAE = false; - PyObject* obj = PyList_GET_ITEM(params.ptr(), i); + PyObject* obj = PyList_GET_ITEM(params, i); // --- None --- if (obj == Py_None) { @@ -571,7 +675,9 @@ std::vector DetectParamTypes(py::list& params) { continue; } - // --- bool (must check before int, since bool is subclass of int) --- + // bool must be checked before int: in CPython, PyBool_Type is a subclass of + // PyLong_Type, so PyLong_Check(True) returns 1. If we hit the int branch first, + // True→1 instead of BIT. if (PyBool_Check(obj)) { info.paramSQLType = SQL_BIT; info.paramCType = SQL_C_BIT; @@ -637,12 +743,18 @@ std::vector DetectParamTypes(py::list& params) { } } + // Detect whether the string needs wide-char (NVARCHAR) or narrow (VARCHAR) binding. + // PyUnicode_IS_COMPACT_ASCII is a struct field check (O(1)), not a content scan. + // UCS-1 strings with max_char > 127 contain Latin-1 chars → need NVARCHAR. bool is_unicode = (kind > PyUnicode_1BYTE_KIND) || (PyUnicode_IS_COMPACT_ASCII(obj) == 0 && kind == PyUnicode_1BYTE_KIND && PyUnicode_MAX_CHAR_VALUE(obj) > 127); if (utf16_len > MAX_INLINE_CHAR) { + // Strings > 4000 UTF-16 code units exceed SQL Server's inline NVARCHAR(MAX) + // threshold. Switch to data-at-execution (DAE) streaming: ODBC driver pulls + // data in chunks via SQLPutData, avoiding a single massive buffer allocation. // DAE path: match slow-path types exactly. // Non-unicode (ASCII) → SQL_VARCHAR + PARAM_C_TYPE_TEXT // On Linux/macOS PARAM_C_TYPE_TEXT == SQL_C_WCHAR, matching @@ -652,7 +764,8 @@ std::vector DetectParamTypes(py::list& params) { info.isDAE = true; info.columnSize = 0; info.utf16Len = utf16_len; - info.dataPtr = py::reinterpret_borrow(py::handle(obj)); + info.dataPtr = obj; + Py_INCREF(obj); info.paramSQLType = is_unicode ? SQL_WVARCHAR : SQL_VARCHAR; info.paramCType = is_unicode ? SQL_C_WCHAR : PARAM_C_TYPE_TEXT; } else { @@ -686,7 +799,8 @@ std::vector DetectParamTypes(py::list& params) { if (length > MAX_INLINE_BINARY) { info.isDAE = true; info.columnSize = 0; - info.dataPtr = py::reinterpret_borrow(py::handle(obj)); + info.dataPtr = obj; + Py_INCREF(obj); } else { info.columnSize = std::max(length, 1); } @@ -741,8 +855,8 @@ std::vector DetectParamTypes(py::list& params) { Py_ssize_t time_len = PyUnicode_GET_LENGTH(time_str); info.columnSize = std::max(info.columnSize, time_len); // PyList_SetItem (lowercase) decrefs the old slot before stealing the new - // reference, so this is safe even if `params` is shared with the caller. - PyList_SetItem(params.ptr(), i, time_str); + // reference; safe here because cursor.py already passed a fresh list copy. + PyList_SetItem(params, i, time_str); continue; } @@ -801,8 +915,9 @@ std::vector DetectParamTypes(py::list& params) { std::to_string(precision) + "."); } - // SMALLMONEY/MONEY range — bind as formatted VARCHAR string - // to match SQL Server's fixed-point money semantics. + // MONEY/SMALLMONEY: SQL Server stores these as fixed-point integers internally. + // We bind as formatted VARCHAR (e.g., "214748.3647") because SQL_C_NUMERIC can't + // represent the exact money range without precision loss on certain ODBC drivers. // Use exact Decimal comparison (not double) to avoid boundary misclassification. bool in_money_range = false; int cmp_ge = PyObject_RichCompareBool(obj, PythonObjectCache::smallmoney_min, Py_GE); @@ -840,21 +955,22 @@ std::vector DetectParamTypes(py::list& params) { info.decimalDigits = 0; Py_DECREF(digits_obj); Py_DECREF(as_tuple); - PyList_SetItem(params.ptr(), i, formatted); + PyList_SetItem(params, i, formatted); continue; } - // Generic numeric binding via SQL_NUMERIC_STRUCT + // 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. info.paramSQLType = SQL_NUMERIC; info.paramCType = SQL_C_NUMERIC; Py_DECREF(digits_obj); Py_DECREF(as_tuple); - py::object numeric_data = - build_numeric_data(py::reinterpret_borrow(py::handle(obj))); - NumericData nd = numeric_data.cast(); + NumericData nd = build_numeric_data(obj); info.columnSize = nd.precision; info.decimalDigits = nd.scale; - PyList_SetItem(params.ptr(), i, numeric_data.release().ptr()); + // Store NumericData as a Python object in the param list for the binder. + py::object numeric_obj = py::cast(nd); + PyList_SetItem(params, i, numeric_obj.release().ptr()); continue; } @@ -868,7 +984,7 @@ std::vector DetectParamTypes(py::list& params) { info.paramCType = SQL_C_GUID; info.columnSize = 16; info.decimalDigits = 0; - PyList_SetItem(params.ptr(), i, bytes_le); + PyList_SetItem(params, i, bytes_le); continue; } @@ -881,8 +997,11 @@ std::vector DetectParamTypes(py::list& params) { } // Helper: build SQL_NUMERIC_STRUCT from Python Decimal -static py::object build_numeric_data(const py::object& decimal_param) { - PyObject* as_tuple = PyObject_CallMethod(decimal_param.ptr(), "as_tuple", NULL); +// Converts a Python Decimal into a SQL_NUMERIC_STRUCT representation. +// Takes raw PyObject* (must be a Decimal instance). Returns NumericData directly — +// the caller stores it as a Python capsule via PyList_SetItem for the binder to consume. +static NumericData build_numeric_data(PyObject* decimal_param) { + PyObject* as_tuple = PyObject_CallMethod(decimal_param, "as_tuple", NULL); if (!as_tuple) throw py::error_already_set(); PyObject* digits_obj = PyObject_GetAttrString(as_tuple, "digits"); @@ -1055,7 +1174,7 @@ static py::object build_numeric_data(const py::object& decimal_param) { } Py_DECREF(val_bytes); - return py::cast(nd); + return nd; } // GH-610: Resolve SQL type for a NULL parameter using per-handle cache. @@ -1405,7 +1524,7 @@ SQLRETURN BindParameters(SqlHandle& handle, SQLHANDLE hStmt, const py::list& par break; } case SQL_C_TYPE_DATE: { - py::object dateType = PythonObjectCache::get_date_class(); + py::object dateType = PythonObjectCache::get_date_class_obj(); if (!py::isinstance(param, dateType)) { ThrowStdException(MakeParamMismatchErrorStr(paramInfo.paramCType, paramIndex)); } @@ -1425,7 +1544,7 @@ SQLRETURN BindParameters(SqlHandle& handle, SQLHANDLE hStmt, const py::list& par break; } case SQL_C_TYPE_TIME: { - py::object timeType = PythonObjectCache::get_time_class(); + py::object timeType = PythonObjectCache::get_time_class_obj(); if (!py::isinstance(param, timeType)) { ThrowStdException(MakeParamMismatchErrorStr(paramInfo.paramCType, paramIndex)); } @@ -1439,7 +1558,7 @@ SQLRETURN BindParameters(SqlHandle& handle, SQLHANDLE hStmt, const py::list& par break; } case SQL_C_SS_TIMESTAMPOFFSET: { - py::object datetimeType = PythonObjectCache::get_datetime_class(); + py::object datetimeType = PythonObjectCache::get_datetime_class_obj(); if (!py::isinstance(param, datetimeType)) { ThrowStdException(MakeParamMismatchErrorStr(paramInfo.paramCType, paramIndex)); } @@ -1491,7 +1610,7 @@ SQLRETURN BindParameters(SqlHandle& handle, SQLHANDLE hStmt, const py::list& par break; } case SQL_C_TYPE_TIMESTAMP: { - py::object datetimeType = PythonObjectCache::get_datetime_class(); + py::object datetimeType = PythonObjectCache::get_datetime_class_obj(); if (!py::isinstance(param, datetimeType)) { ThrowStdException(MakeParamMismatchErrorStr(paramInfo.paramCType, paramIndex)); } @@ -2522,14 +2641,15 @@ SQLRETURN SQLExecuteLegacy_wrap(const SqlHandlePtr statementHandle, const std::u if (!matchedInfo) { ThrowStdException("Unrecognized paramToken returned by SQLParamData"); } - const py::object& pyObj = matchedInfo->dataPtr; - if (pyObj.is_none()) { + PyObject* pyObj = matchedInfo->dataPtr; + if (!pyObj || pyObj == Py_None) { putData(nullptr, 0); continue; } - if (py::isinstance(pyObj)) { + if (PyUnicode_Check(pyObj)) { if (matchedInfo->paramCType == SQL_C_WCHAR) { - std::u16string utf16 = pyObj.cast(); + std::u16string utf16 = + py::reinterpret_borrow(py::handle(pyObj)).cast(); size_t totalChars = utf16.size(); const SQLWCHAR* dataPtr = reinterpretU16stringAsSqlWChar(utf16); size_t offset = 0; @@ -2556,14 +2676,11 @@ SQLRETURN SQLExecuteLegacy_wrap(const SqlHandlePtr statementHandle, const std::u // Encode the string using the specified encoding std::string encodedStr; try { - if (py::isinstance(pyObj)) { - py::object encoded = pyObj.attr("encode")(charEncoding, "strict"); - encodedStr = encoded.cast(); - LOG("SQLExecute: DAE SQL_C_CHAR - Encoded with '%s', %zu bytes", - charEncoding.c_str(), encodedStr.size()); - } else { - encodedStr = pyObj.cast(); - } + py::object encoded = py::reinterpret_borrow(py::handle(pyObj)) + .attr("encode")(charEncoding, "strict"); + encodedStr = encoded.cast(); + LOG("SQLExecute: DAE SQL_C_CHAR - Encoded with '%s', %zu bytes", + charEncoding.c_str(), encodedStr.size()); } catch (const py::error_already_set& e) { LOG_ERROR("SQLExecute: DAE SQL_C_CHAR - Failed to encode with '%s': %s", charEncoding.c_str(), e.what()); @@ -2589,12 +2706,18 @@ SQLRETURN SQLExecuteLegacy_wrap(const SqlHandlePtr statementHandle, const std::u } else { ThrowStdException("Unsupported C type for str in DAE"); } - } else if (py::isinstance(pyObj) || - py::isinstance(pyObj)) { - py::bytes b = pyObj.cast(); - std::string s = b; - const char* dataPtr = s.data(); - size_t totalBytes = s.size(); + } else if (PyBytes_Check(pyObj) || PyByteArray_Check(pyObj)) { + const char* dataPtr = nullptr; + size_t totalBytes = 0; + std::string bytesStorage; // lifetime must span the loop + if (PyBytes_Check(pyObj)) { + bytesStorage = py::reinterpret_borrow(py::handle(pyObj)); + dataPtr = bytesStorage.data(); + totalBytes = bytesStorage.size(); + } else { + dataPtr = PyByteArray_AS_STRING(pyObj); + totalBytes = static_cast(PyByteArray_GET_SIZE(pyObj)); + } const size_t chunkSize = DAE_CHUNK_SIZE; for (size_t offset = 0; offset < totalBytes; offset += chunkSize) { size_t len = std::min(chunkSize, totalBytes - offset); @@ -2685,7 +2808,7 @@ SQLRETURN SQLExecute_wrap(const SqlHandlePtr statementHandle, // Run DetectParamTypes BEFORE SQLPrepare so that type-detection errors // (unsupported type, NaN Decimal, precision overflow) don't leave the // cursor in a half-prepared state. - std::vector paramInfos = DetectParamTypes(params); + std::vector paramInfos = DetectParamTypes(params.ptr()); RETCODE rc; bool already_prepared = is_stmt_prepared[0].cast(); @@ -2741,16 +2864,17 @@ SQLRETURN SQLExecute_wrap(const SqlHandlePtr statementHandle, if (!matchedInfo) { ThrowStdException("SQLExecuteFast: unrecognized paramToken from SQLParamData"); } - const py::object& pyObj = matchedInfo->dataPtr; - if (pyObj.is_none()) { + PyObject* pyObj = matchedInfo->dataPtr; + if (!pyObj || pyObj == Py_None) { py::gil_scoped_release release; SQLPutData_ptr(hStmt, nullptr, 0); continue; } - if (py::isinstance(pyObj)) { + if (PyUnicode_Check(pyObj)) { if (matchedInfo->paramCType == SQL_C_WCHAR) { - std::u16string u16 = pyObj.cast(); + std::u16string u16 = + py::reinterpret_borrow(py::handle(pyObj)).cast(); const SQLWCHAR* dataPtr = reinterpretU16stringAsSqlWChar(u16); size_t totalChars = u16.size(); size_t chunkChars = DAE_CHUNK_SIZE / sizeof(SQLWCHAR); @@ -2765,7 +2889,8 @@ SQLRETURN SQLExecute_wrap(const SqlHandlePtr statementHandle, } } else if (matchedInfo->paramCType == SQL_C_CHAR) { std::string encodedStr; - py::object encoded = pyObj.attr("encode")(charEncoding, "strict"); + py::object encoded = py::reinterpret_borrow(py::handle(pyObj)) + .attr("encode")(charEncoding, "strict"); encodedStr = encoded.cast(); const char* dataPtr = encodedStr.data(); size_t totalBytes = encodedStr.size(); @@ -2782,23 +2907,21 @@ SQLRETURN SQLExecute_wrap(const SqlHandlePtr statementHandle, } else { ThrowStdException("SQLExecuteFast: unsupported C type for str in DAE"); } - } else if (py::isinstance(pyObj) || - py::isinstance(pyObj)) { + } else if (PyBytes_Check(pyObj) || PyByteArray_Check(pyObj)) { // Handle bytes and bytearray separately — pybind11's bytes // caster does not safely convert bytearray. const char* dataPtr = nullptr; size_t totalBytes = 0; std::string bytesStorage; // lifetime must span the loop - if (py::isinstance(pyObj)) { - bytesStorage = pyObj.cast(); + if (PyBytes_Check(pyObj)) { + bytesStorage = py::reinterpret_borrow(py::handle(pyObj)); dataPtr = bytesStorage.data(); totalBytes = bytesStorage.size(); } else { // bytearray: use raw buffer access - PyObject* ba = pyObj.ptr(); - dataPtr = PyByteArray_AS_STRING(ba); - totalBytes = static_cast(PyByteArray_GET_SIZE(ba)); + dataPtr = PyByteArray_AS_STRING(pyObj); + totalBytes = static_cast(PyByteArray_GET_SIZE(pyObj)); } for (size_t offset = 0; offset < totalBytes; offset += DAE_CHUNK_SIZE) { @@ -3251,7 +3374,7 @@ SQLRETURN BindParameterArray(SqlHandle& handle, SQLHANDLE hStmt, const py::list& AllocateParamBufferArray(tempBuffers, paramSetSize); strLenOrIndArray = AllocateParamBufferArray(tempBuffers, paramSetSize); - py::object datetimeType = PythonObjectCache::get_datetime_class(); + py::object datetimeType = PythonObjectCache::get_datetime_class_obj(); for (size_t i = 0; i < paramSetSize; ++i) { const py::handle& param = columnValues[i]; @@ -3366,7 +3489,7 @@ SQLRETURN BindParameterArray(SqlHandle& handle, SQLHANDLE hStmt, const py::list& // Get cached UUID class from module-level helper // This avoids static object destruction issues during // Python finalization - py::object uuid_class = PythonObjectCache::get_uuid_class(); + py::object uuid_class = PythonObjectCache::get_uuid_class_obj(); // Get cached UUID class for (size_t i = 0; i < paramSetSize; ++i) { @@ -4333,7 +4456,7 @@ SQLRETURN SQLGetData_wrap(SqlHandlePtr StatementHandle, SQLUSMALLINT colCount, p // parsing The decimal separator only affects display // formatting, not parsing py::object decimalObj = - PythonObjectCache::get_decimal_class()(py::str(cnum, safeLen)); + PythonObjectCache::get_decimal_class_obj()(py::str(cnum, safeLen)); row.append(decimalObj); } catch (const py::error_already_set& e) { // If conversion fails, append None @@ -4383,7 +4506,7 @@ SQLRETURN SQLGetData_wrap(SqlHandlePtr StatementHandle, SQLUSMALLINT colCount, p ret = SQLGetData_ptr(hStmt, i, SQL_C_TYPE_DATE, &dateValue, sizeof(dateValue), NULL); if (SQL_SUCCEEDED(ret)) { - row.append(PythonObjectCache::get_date_class()(dateValue.year, dateValue.month, + row.append(PythonObjectCache::get_date_class_obj()(dateValue.year, dateValue.month, dateValue.day)); } else { row.append(py::none()); @@ -4396,7 +4519,7 @@ SQLRETURN SQLGetData_wrap(SqlHandlePtr StatementHandle, SQLUSMALLINT colCount, p SQLLEN indicator = 0; ret = SQLGetData_ptr(hStmt, i, SQL_C_SS_TIME2, &t2, sizeof(t2), &indicator); if (SQL_SUCCEEDED(ret) && indicator != SQL_NULL_DATA) { - row.append(PythonObjectCache::get_time_class()( + row.append(PythonObjectCache::get_time_class_obj()( t2.hour, t2.minute, t2.second, t2.fraction / 1000)); // ns to µs } else { if (!SQL_SUCCEEDED(ret)) { @@ -4415,7 +4538,7 @@ SQLRETURN SQLGetData_wrap(SqlHandlePtr StatementHandle, SQLUSMALLINT colCount, p ret = SQLGetData_ptr(hStmt, i, SQL_C_TYPE_TIMESTAMP, ×tampValue, sizeof(timestampValue), NULL); if (SQL_SUCCEEDED(ret)) { - row.append(PythonObjectCache::get_datetime_class()( + row.append(PythonObjectCache::get_datetime_class_obj()( timestampValue.year, timestampValue.month, timestampValue.day, timestampValue.hour, timestampValue.minute, timestampValue.second, timestampValue.fraction / 1000 // Convert back ns to µs @@ -4455,7 +4578,7 @@ SQLRETURN SQLGetData_wrap(SqlHandlePtr StatementHandle, SQLUSMALLINT colCount, p py::object datetime_module = py::module_::import("datetime"); py::object tzinfo = datetime_module.attr("timezone")( datetime_module.attr("timedelta")(py::arg("minutes") = totalMinutes)); - py::object py_dt = PythonObjectCache::get_datetime_class()( + py::object py_dt = PythonObjectCache::get_datetime_class_obj()( dtoValue.year, dtoValue.month, dtoValue.day, dtoValue.hour, dtoValue.minute, dtoValue.second, microseconds, tzinfo); row.append(py_dt); @@ -4562,7 +4685,7 @@ SQLRETURN SQLGetData_wrap(SqlHandlePtr StatementHandle, SQLUSMALLINT colCount, p py::bytes py_guid_bytes(guid_bytes.data(), guid_bytes.size()); py::object uuid_obj = - PythonObjectCache::get_uuid_class()(py::arg("bytes") = py_guid_bytes); + PythonObjectCache::get_uuid_class_obj()(py::arg("bytes") = py_guid_bytes); row.append(uuid_obj); } else if (indicator == SQL_NULL_DATA) { row.append(py::none()); @@ -5025,7 +5148,7 @@ SQLRETURN FetchBatchData(SQLHSTMT hStmt, ColumnBuffers& buffers, py::list& colum // parsing The decimal separator only affects display // formatting, not parsing PyObject* decimalObj = - PythonObjectCache::get_decimal_class()(py::str(rawData, decimalDataLen)) + PythonObjectCache::get_decimal_class_obj()(py::str(rawData, decimalDataLen)) .release() .ptr(); PyList_SET_ITEM(row, col - 1, decimalObj); @@ -5042,7 +5165,7 @@ SQLRETURN FetchBatchData(SQLHSTMT hStmt, ColumnBuffers& buffers, py::list& colum case SQL_TYPE_TIMESTAMP: case SQL_DATETIME: { const SQL_TIMESTAMP_STRUCT& ts = buffers.timestampBuffers[col - 1][i]; - PyObject* datetimeObj = PythonObjectCache::get_datetime_class()( + PyObject* datetimeObj = PythonObjectCache::get_datetime_class_obj()( ts.year, ts.month, ts.day, ts.hour, ts.minute, ts.second, ts.fraction / 1000) .release() @@ -5052,7 +5175,7 @@ SQLRETURN FetchBatchData(SQLHSTMT hStmt, ColumnBuffers& buffers, py::list& colum } case SQL_TYPE_DATE: { PyObject* dateObj = - PythonObjectCache::get_date_class()(buffers.dateBuffers[col - 1][i].year, + PythonObjectCache::get_date_class_obj()(buffers.dateBuffers[col - 1][i].year, buffers.dateBuffers[col - 1][i].month, buffers.dateBuffers[col - 1][i].day) .release() @@ -5063,7 +5186,7 @@ SQLRETURN FetchBatchData(SQLHSTMT hStmt, ColumnBuffers& buffers, py::list& colum case SQL_SS_TIME2: { const SQL_SS_TIME2_STRUCT& t2 = buffers.timeBuffers[col - 1][i]; PyObject* timeObj = - PythonObjectCache::get_time_class()(t2.hour, t2.minute, t2.second, + PythonObjectCache::get_time_class_obj()(t2.hour, t2.minute, t2.second, t2.fraction / 1000) // ns to µs .release() .ptr(); @@ -5079,7 +5202,7 @@ SQLRETURN FetchBatchData(SQLHSTMT hStmt, ColumnBuffers& buffers, py::list& colum py::object datetime_module = py::module_::import("datetime"); py::object tzinfo = datetime_module.attr("timezone")( datetime_module.attr("timedelta")(py::arg("minutes") = totalMinutes)); - py::object py_dt = PythonObjectCache::get_datetime_class()( + py::object py_dt = PythonObjectCache::get_datetime_class_obj()( dtoValue.year, dtoValue.month, dtoValue.day, dtoValue.hour, dtoValue.minute, dtoValue.second, dtoValue.fraction / 1000, // ns → µs @@ -5113,7 +5236,7 @@ SQLRETURN FetchBatchData(SQLHSTMT hStmt, ColumnBuffers& buffers, py::list& colum py::bytes py_guid_bytes(reinterpret_cast(reordered), 16); py::dict kwargs; kwargs["bytes"] = py_guid_bytes; - py::object uuid_obj = PythonObjectCache::get_uuid_class()(**kwargs); + py::object uuid_obj = PythonObjectCache::get_uuid_class_obj()(**kwargs); PyList_SET_ITEM(row, col - 1, uuid_obj.release().ptr()); break; } @@ -6660,7 +6783,19 @@ PYBIND11_MODULE(ddbc_bindings, m) { .def_readwrite("columnSize", &ParamInfo::columnSize) .def_readwrite("decimalDigits", &ParamInfo::decimalDigits) .def_readwrite("strLenOrInd", &ParamInfo::strLenOrInd) - .def_readwrite("dataPtr", &ParamInfo::dataPtr) + .def_property( + "dataPtr", + [](const ParamInfo& info) -> py::object { + if (!info.dataPtr) { + return py::none(); + } + return py::reinterpret_borrow(py::handle(info.dataPtr)); + }, + [](ParamInfo& info, py::object obj) { + Py_XINCREF(obj.ptr()); + Py_XDECREF(info.dataPtr); + info.dataPtr = obj.ptr(); + }) .def_readwrite("isDAE", &ParamInfo::isDAE); // Define numeric data class From 3d73824bb5cd224693ba2cac6a645f113ed9977a Mon Sep 17 00:00:00 2001 From: Gaurav Sharma Date: Tue, 14 Jul 2026 12:06:48 +0530 Subject: [PATCH 14/29] FIX: Add ParamInfo copy constructor for pybind11 legacy path compatibility pybind11's type_caster needs copy semantics for std::vector& in the legacy path. Provide a copy ctor that Py_XINCREFs dataPtr. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- mssql_python/pybind/ddbc_bindings.cpp | 26 ++++++++++++++++++++++++-- 1 file changed, 24 insertions(+), 2 deletions(-) diff --git a/mssql_python/pybind/ddbc_bindings.cpp b/mssql_python/pybind/ddbc_bindings.cpp index 4681c1dd..12ea2312 100644 --- a/mssql_python/pybind/ddbc_bindings.cpp +++ b/mssql_python/pybind/ddbc_bindings.cpp @@ -299,8 +299,30 @@ struct ParamInfo { ParamInfo() = default; ~ParamInfo() { Py_XDECREF(dataPtr); } - ParamInfo(const ParamInfo&) = delete; - ParamInfo& operator=(const ParamInfo&) = delete; + // Copy needed by pybind11 type_caster for the legacy path (std::vector&). + ParamInfo(const ParamInfo& other) + : inputOutputType(other.inputOutputType), paramCType(other.paramCType), + paramSQLType(other.paramSQLType), columnSize(other.columnSize), + decimalDigits(other.decimalDigits), strLenOrInd(other.strLenOrInd), + isDAE(other.isDAE), dataPtr(other.dataPtr), utf16Len(other.utf16Len) { + Py_XINCREF(dataPtr); + } + ParamInfo& operator=(const ParamInfo& other) { + if (this != &other) { + Py_XDECREF(dataPtr); + inputOutputType = other.inputOutputType; + paramCType = other.paramCType; + paramSQLType = other.paramSQLType; + columnSize = other.columnSize; + decimalDigits = other.decimalDigits; + strLenOrInd = other.strLenOrInd; + isDAE = other.isDAE; + dataPtr = other.dataPtr; + utf16Len = other.utf16Len; + Py_XINCREF(dataPtr); + } + return *this; + } ParamInfo(ParamInfo&& other) noexcept : inputOutputType(other.inputOutputType), paramCType(other.paramCType), paramSQLType(other.paramSQLType), columnSize(other.columnSize), From 98eb2549fca84bdc825b5bf713f07aef0023aca4 Mon Sep 17 00:00:00 2001 From: Gaurav Sharma Date: Tue, 14 Jul 2026 12:41:46 +0530 Subject: [PATCH 15/29] FIX: Use explicit byte length for string binding instead of SQL_NTS Strings with embedded NUL characters (e.g., 'hello\x00world') were truncated at the first NUL because BindParameters used SQL_NTS (null-terminated string indicator). Now passes the actual byte/char length so ODBC sees the full string. Fixes test_string_with_embedded_nulls on all platforms. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- mssql_python/pybind/ddbc_bindings.cpp | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/mssql_python/pybind/ddbc_bindings.cpp b/mssql_python/pybind/ddbc_bindings.cpp index 12ea2312..f965aa2b 100644 --- a/mssql_python/pybind/ddbc_bindings.cpp +++ b/mssql_python/pybind/ddbc_bindings.cpp @@ -1371,10 +1371,12 @@ SQLRETURN BindParameters(SqlHandle& handle, SQLHANDLE hStmt, const py::list& par std::string* strParam = AllocateParamBuffer(paramBuffers, encodedStr); - dataPtr = const_cast(static_cast(strParam->c_str())); - bufferLength = strParam->size() + 1; + dataPtr = const_cast(static_cast(strParam->data())); + bufferLength = strParam->size(); strLenOrIndPtr = AllocateParamBuffer(paramBuffers); - *strLenOrIndPtr = SQL_NTS; + // Use explicit byte length instead of SQL_NTS so embedded NUL chars + // aren't treated as string terminators (e.g., "hello\x00world"). + *strLenOrIndPtr = static_cast(strParam->size()); } break; } @@ -1439,7 +1441,9 @@ SQLRETURN BindParameters(SqlHandle& handle, SQLHANDLE hStmt, const py::list& par dataPtr = sqlwcharBuffer->data(); bufferLength = sqlwcharBuffer->size() * sizeof(SQLWCHAR); strLenOrIndPtr = AllocateParamBuffer(paramBuffers); - *strLenOrIndPtr = SQL_NTS; + // Use explicit byte length instead of SQL_NTS so embedded NUL chars + // aren't treated as string terminators. + *strLenOrIndPtr = static_cast(sqlwcharBuffer->size() * sizeof(SQLWCHAR)); } break; } From 4ee2db95369fc3b28c9947d22d7284fd28be3b4c Mon Sep 17 00:00:00 2001 From: Gaurav Sharma Date: Tue, 14 Jul 2026 15:49:43 +0530 Subject: [PATCH 16/29] TEST: Add edge-case tests and LCOV exclusions for untestable paths - Add tests: integer overflow (2**63), Decimal NaN/sNaN, precision > 38 - Add LCOV_EXCL markers on CPython import-failure and cache-fallback paths - Add contextual comments on PythonObjectCache and ParamInfo operators Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- mssql_python/pybind/ddbc_bindings.cpp | 55 ++++++++++++++++++++------- tests/test_023_fast_path_parity.py | 22 +++++++++++ 2 files changed, 64 insertions(+), 13 deletions(-) diff --git a/mssql_python/pybind/ddbc_bindings.cpp b/mssql_python/pybind/ddbc_bindings.cpp index f965aa2b..37f99357 100644 --- a/mssql_python/pybind/ddbc_bindings.cpp +++ b/mssql_python/pybind/ddbc_bindings.cpp @@ -129,39 +129,53 @@ static PyObject* smallmoney_min = nullptr; static PyObject* smallmoney_max = nullptr; static bool cache_initialized = false; +// One-time cache of Python type objects and MONEY boundary constants. +// Called on first execute(). Uses raw CPython API (not pybind11) because +// these cached PyObject* are compared via PyObject_IsInstance in the +// hot DetectParamTypes loop — wrapping them in py::object would add +// unnecessary ref-count traffic on every parameter. void initialize() { if (!cache_initialized) { PyDateTime_IMPORT; if (PyDateTimeAPI == nullptr) { - throw py::error_already_set(); + throw py::error_already_set(); // LCOV_EXCL_LINE } + // Cache datetime.datetime, datetime.date, datetime.time for isinstance + // checks in DetectParamTypes. Single import, then borrowed refs held + // for process lifetime (module-level statics, never decremented). PyObject* datetime_module = PyImport_ImportModule("datetime"); - if (!datetime_module) throw py::error_already_set(); + if (!datetime_module) throw py::error_already_set(); // LCOV_EXCL_LINE datetime_class = PyObject_GetAttrString(datetime_module, "datetime"); date_class = PyObject_GetAttrString(datetime_module, "date"); time_class = PyObject_GetAttrString(datetime_module, "time"); Py_DECREF(datetime_module); - if (!datetime_class || !date_class || !time_class) throw py::error_already_set(); + if (!datetime_class || !date_class || !time_class) + throw py::error_already_set(); // LCOV_EXCL_LINE PyObject* decimal_module = PyImport_ImportModule("decimal"); - if (!decimal_module) throw py::error_already_set(); + if (!decimal_module) throw py::error_already_set(); // LCOV_EXCL_LINE decimal_class = PyObject_GetAttrString(decimal_module, "Decimal"); Py_DECREF(decimal_module); - if (!decimal_class) throw py::error_already_set(); + if (!decimal_class) throw py::error_already_set(); // LCOV_EXCL_LINE PyObject* uuid_module = PyImport_ImportModule("uuid"); - if (!uuid_module) throw py::error_already_set(); + if (!uuid_module) throw py::error_already_set(); // LCOV_EXCL_LINE uuid_class = PyObject_GetAttrString(uuid_module, "UUID"); Py_DECREF(uuid_module); - if (!uuid_class) throw py::error_already_set(); + if (!uuid_class) throw py::error_already_set(); // LCOV_EXCL_LINE + // Pre-compute MONEY/SMALLMONEY boundary Decimals once. DetectParamTypes + // uses PyObject_RichCompareBool against these to classify Decimal params + // into MONEY vs NUMERIC — doing exact Decimal comparison (not float cast) + // avoids boundary misclassification at the edges. PyObject* Decimal = decimal_class; smallmoney_min = PyObject_CallFunction(Decimal, "s", "-214748.3648"); smallmoney_max = PyObject_CallFunction(Decimal, "s", "214748.3647"); money_min = PyObject_CallFunction(Decimal, "s", "-922337203685477.5808"); money_max = PyObject_CallFunction(Decimal, "s", "922337203685477.5807"); if (!smallmoney_min || !smallmoney_max || !money_min || !money_max) { + // Partial creation — clean up whichever succeeded before throwing. Py_XDECREF(smallmoney_min); Py_XDECREF(smallmoney_max); Py_XDECREF(money_min); @@ -170,7 +184,7 @@ void initialize() { smallmoney_max = nullptr; money_min = nullptr; money_max = nullptr; - throw py::error_already_set(); + throw py::error_already_set(); // LCOV_EXCL_LINE } cache_initialized = true; @@ -191,11 +205,13 @@ PyObject* get_datetime_class() { if (cache_initialized && datetime_class) { return datetime_class; } + // LCOV_EXCL_START PyObject* mod = PyImport_ImportModule("datetime"); if (!mod) return nullptr; PyObject* cls = PyObject_GetAttrString(mod, "datetime"); Py_DECREF(mod); return cls; // caller must check for NULL + // LCOV_EXCL_STOP } py::object get_datetime_class_obj() { @@ -208,11 +224,13 @@ PyObject* get_date_class() { if (cache_initialized && date_class) { return date_class; } + // LCOV_EXCL_START PyObject* mod = PyImport_ImportModule("datetime"); if (!mod) return nullptr; PyObject* cls = PyObject_GetAttrString(mod, "date"); Py_DECREF(mod); return cls; // caller must check for NULL + // LCOV_EXCL_STOP } py::object get_date_class_obj() { @@ -225,11 +243,13 @@ PyObject* get_time_class() { if (cache_initialized && time_class) { return time_class; } + // LCOV_EXCL_START PyObject* mod = PyImport_ImportModule("datetime"); if (!mod) return nullptr; PyObject* cls = PyObject_GetAttrString(mod, "time"); Py_DECREF(mod); return cls; // caller must check for NULL + // LCOV_EXCL_STOP } py::object get_time_class_obj() { @@ -242,11 +262,13 @@ PyObject* get_decimal_class() { if (cache_initialized && decimal_class) { return decimal_class; } + // LCOV_EXCL_START PyObject* mod = PyImport_ImportModule("decimal"); if (!mod) return nullptr; PyObject* cls = PyObject_GetAttrString(mod, "Decimal"); Py_DECREF(mod); return cls; // caller must check for NULL + // LCOV_EXCL_STOP } py::object get_decimal_class_obj() { @@ -259,11 +281,13 @@ PyObject* get_uuid_class() { if (cache_initialized && uuid_class) { return uuid_class; } + // LCOV_EXCL_START PyObject* mod = PyImport_ImportModule("uuid"); if (!mod) return nullptr; PyObject* cls = PyObject_GetAttrString(mod, "UUID"); Py_DECREF(mod); return cls; // caller must check for NULL + // LCOV_EXCL_STOP } py::object get_uuid_class_obj() { @@ -307,6 +331,10 @@ struct ParamInfo { isDAE(other.isDAE), dataPtr(other.dataPtr), utf16Len(other.utf16Len) { Py_XINCREF(dataPtr); } + // Copy/move assignment and move constructor below are required for + // std::vector resize/reallocation and pybind11's type_caster. + // They manually manage dataPtr refcounts since ParamInfo owns a strong ref. + // LCOV_EXCL_START — exercised by STL internals, not directly testable. ParamInfo& operator=(const ParamInfo& other) { if (this != &other) { Py_XDECREF(dataPtr); @@ -346,6 +374,7 @@ struct ParamInfo { } return *this; } + // LCOV_EXCL_STOP }; #ifdef __GNUC__ #pragma GCC diagnostic pop @@ -892,7 +921,7 @@ std::vector DetectParamTypes(PyObject* params) { PyObject* exponent_obj = PyObject_GetAttrString(as_tuple, "exponent"); if (!exponent_obj) { Py_DECREF(as_tuple); - throw py::error_already_set(); + throw py::error_already_set(); // LCOV_EXCL_LINE } // NaN / Infinity / sNaN: refuse rather than silently writing 0. @@ -907,7 +936,7 @@ std::vector DetectParamTypes(PyObject* params) { if (!digits_obj) { Py_DECREF(exponent_obj); Py_DECREF(as_tuple); - throw py::error_already_set(); + throw py::error_already_set(); // LCOV_EXCL_LINE } Py_ssize_t num_digits = PyTuple_GET_SIZE(digits_obj); @@ -916,7 +945,7 @@ std::vector DetectParamTypes(PyObject* params) { if (exponent == -1 && PyErr_Occurred()) { Py_DECREF(digits_obj); Py_DECREF(as_tuple); - throw py::error_already_set(); + throw py::error_already_set(); // LCOV_EXCL_LINE } int precision; @@ -947,7 +976,7 @@ std::vector DetectParamTypes(PyObject* params) { if (cmp_ge == -1 || cmp_le == -1) { Py_DECREF(digits_obj); Py_DECREF(as_tuple); - throw py::error_already_set(); + throw py::error_already_set(); // LCOV_EXCL_LINE } if (cmp_ge == 1 && cmp_le == 1) { in_money_range = true; @@ -957,7 +986,7 @@ std::vector DetectParamTypes(PyObject* params) { if (cmp_ge == -1 || cmp_le == -1) { Py_DECREF(digits_obj); Py_DECREF(as_tuple); - throw py::error_already_set(); + throw py::error_already_set(); // LCOV_EXCL_LINE } if (cmp_ge == 1 && cmp_le == 1) { in_money_range = true; diff --git a/tests/test_023_fast_path_parity.py b/tests/test_023_fast_path_parity.py index c4f2b383..77272caf 100644 --- a/tests/test_023_fast_path_parity.py +++ b/tests/test_023_fast_path_parity.py @@ -280,3 +280,25 @@ def test_string_with_embedded_nulls(cursor): cursor.execute("SELECT LEN(?)", [value]) result = cursor.fetchone()[0] assert result == 11 + + +def test_integer_overflow_detected(cursor): + """Integers beyond int64 range trigger overflow detection in C++ and bind as BIGINT. + SQL Server cannot store values outside [-2^63, 2^63-1] in BIGINT, so these raise.""" + with pytest.raises(Exception): + cursor.execute("SELECT ?", [2**63]) + with pytest.raises(Exception): + cursor.execute("SELECT ?", [-(2**63) - 1]) + + +@pytest.mark.parametrize("value", [decimal.Decimal("NaN"), decimal.Decimal("sNaN")]) +def test_decimal_nan_variants_rejected(cursor, value): + """Decimal NaN variants must raise, not silently bind as 0.""" + with pytest.raises(Exception): + cursor.execute("SELECT ?", [value]) + + +def test_decimal_precision_overflow_rejected(cursor): + """Decimals beyond SQL Server's max precision must raise.""" + with pytest.raises(Exception): + cursor.execute("SELECT ?", [decimal.Decimal("123456789012345678901234567890123456789")]) From 8941070be702f9617c2a96eda3a9d7dc4bed1f6a Mon Sep 17 00:00:00 2001 From: Gaurav Sharma Date: Tue, 14 Jul 2026 20:06:09 +0530 Subject: [PATCH 17/29] =?UTF-8?q?FIX:=20Address=20review=20comments=20?= =?UTF-8?q?=E2=80=94=20PyList=5FSetItem=20checks=20and=20bytearray=20DAE?= =?UTF-8?q?=20safety?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Check PyList_SetItem return value at all 4 call sites in DetectParamTypes - Copy mutable bytearray into std::string before DAE streaming (both paths) - Revert LCOV_EXCL markers (not processed by llvm-cov pipeline) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- mssql_python/pybind/ddbc_bindings.cpp | 72 ++++++++++++++------------- 1 file changed, 38 insertions(+), 34 deletions(-) diff --git a/mssql_python/pybind/ddbc_bindings.cpp b/mssql_python/pybind/ddbc_bindings.cpp index 37f99357..49b5a15f 100644 --- a/mssql_python/pybind/ddbc_bindings.cpp +++ b/mssql_python/pybind/ddbc_bindings.cpp @@ -138,32 +138,32 @@ void initialize() { if (!cache_initialized) { PyDateTime_IMPORT; if (PyDateTimeAPI == nullptr) { - throw py::error_already_set(); // LCOV_EXCL_LINE + throw py::error_already_set(); } // Cache datetime.datetime, datetime.date, datetime.time for isinstance // checks in DetectParamTypes. Single import, then borrowed refs held // for process lifetime (module-level statics, never decremented). PyObject* datetime_module = PyImport_ImportModule("datetime"); - if (!datetime_module) throw py::error_already_set(); // LCOV_EXCL_LINE + if (!datetime_module) throw py::error_already_set(); datetime_class = PyObject_GetAttrString(datetime_module, "datetime"); date_class = PyObject_GetAttrString(datetime_module, "date"); time_class = PyObject_GetAttrString(datetime_module, "time"); Py_DECREF(datetime_module); if (!datetime_class || !date_class || !time_class) - throw py::error_already_set(); // LCOV_EXCL_LINE + throw py::error_already_set(); PyObject* decimal_module = PyImport_ImportModule("decimal"); - if (!decimal_module) throw py::error_already_set(); // LCOV_EXCL_LINE + if (!decimal_module) throw py::error_already_set(); decimal_class = PyObject_GetAttrString(decimal_module, "Decimal"); Py_DECREF(decimal_module); - if (!decimal_class) throw py::error_already_set(); // LCOV_EXCL_LINE + if (!decimal_class) throw py::error_already_set(); PyObject* uuid_module = PyImport_ImportModule("uuid"); - if (!uuid_module) throw py::error_already_set(); // LCOV_EXCL_LINE + if (!uuid_module) throw py::error_already_set(); uuid_class = PyObject_GetAttrString(uuid_module, "UUID"); Py_DECREF(uuid_module); - if (!uuid_class) throw py::error_already_set(); // LCOV_EXCL_LINE + if (!uuid_class) throw py::error_already_set(); // Pre-compute MONEY/SMALLMONEY boundary Decimals once. DetectParamTypes // uses PyObject_RichCompareBool against these to classify Decimal params @@ -184,7 +184,7 @@ void initialize() { smallmoney_max = nullptr; money_min = nullptr; money_max = nullptr; - throw py::error_already_set(); // LCOV_EXCL_LINE + throw py::error_already_set(); } cache_initialized = true; @@ -205,13 +205,11 @@ PyObject* get_datetime_class() { if (cache_initialized && datetime_class) { return datetime_class; } - // LCOV_EXCL_START PyObject* mod = PyImport_ImportModule("datetime"); if (!mod) return nullptr; PyObject* cls = PyObject_GetAttrString(mod, "datetime"); Py_DECREF(mod); return cls; // caller must check for NULL - // LCOV_EXCL_STOP } py::object get_datetime_class_obj() { @@ -224,13 +222,11 @@ PyObject* get_date_class() { if (cache_initialized && date_class) { return date_class; } - // LCOV_EXCL_START PyObject* mod = PyImport_ImportModule("datetime"); if (!mod) return nullptr; PyObject* cls = PyObject_GetAttrString(mod, "date"); Py_DECREF(mod); return cls; // caller must check for NULL - // LCOV_EXCL_STOP } py::object get_date_class_obj() { @@ -243,13 +239,11 @@ PyObject* get_time_class() { if (cache_initialized && time_class) { return time_class; } - // LCOV_EXCL_START PyObject* mod = PyImport_ImportModule("datetime"); if (!mod) return nullptr; PyObject* cls = PyObject_GetAttrString(mod, "time"); Py_DECREF(mod); return cls; // caller must check for NULL - // LCOV_EXCL_STOP } py::object get_time_class_obj() { @@ -262,13 +256,11 @@ PyObject* get_decimal_class() { if (cache_initialized && decimal_class) { return decimal_class; } - // LCOV_EXCL_START PyObject* mod = PyImport_ImportModule("decimal"); if (!mod) return nullptr; PyObject* cls = PyObject_GetAttrString(mod, "Decimal"); Py_DECREF(mod); return cls; // caller must check for NULL - // LCOV_EXCL_STOP } py::object get_decimal_class_obj() { @@ -281,13 +273,11 @@ PyObject* get_uuid_class() { if (cache_initialized && uuid_class) { return uuid_class; } - // LCOV_EXCL_START PyObject* mod = PyImport_ImportModule("uuid"); if (!mod) return nullptr; PyObject* cls = PyObject_GetAttrString(mod, "UUID"); Py_DECREF(mod); return cls; // caller must check for NULL - // LCOV_EXCL_STOP } py::object get_uuid_class_obj() { @@ -334,7 +324,6 @@ struct ParamInfo { // Copy/move assignment and move constructor below are required for // std::vector resize/reallocation and pybind11's type_caster. // They manually manage dataPtr refcounts since ParamInfo owns a strong ref. - // LCOV_EXCL_START — exercised by STL internals, not directly testable. ParamInfo& operator=(const ParamInfo& other) { if (this != &other) { Py_XDECREF(dataPtr); @@ -374,7 +363,6 @@ struct ParamInfo { } return *this; } - // LCOV_EXCL_STOP }; #ifdef __GNUC__ #pragma GCC diagnostic pop @@ -907,7 +895,9 @@ std::vector DetectParamTypes(PyObject* params) { info.columnSize = std::max(info.columnSize, time_len); // PyList_SetItem (lowercase) decrefs the old slot before stealing the new // reference; safe here because cursor.py already passed a fresh list copy. - PyList_SetItem(params, i, time_str); + if (PyList_SetItem(params, i, time_str) != 0) { + throw py::error_already_set(); + } continue; } @@ -921,7 +911,7 @@ std::vector DetectParamTypes(PyObject* params) { PyObject* exponent_obj = PyObject_GetAttrString(as_tuple, "exponent"); if (!exponent_obj) { Py_DECREF(as_tuple); - throw py::error_already_set(); // LCOV_EXCL_LINE + throw py::error_already_set(); } // NaN / Infinity / sNaN: refuse rather than silently writing 0. @@ -936,7 +926,7 @@ std::vector DetectParamTypes(PyObject* params) { if (!digits_obj) { Py_DECREF(exponent_obj); Py_DECREF(as_tuple); - throw py::error_already_set(); // LCOV_EXCL_LINE + throw py::error_already_set(); } Py_ssize_t num_digits = PyTuple_GET_SIZE(digits_obj); @@ -945,7 +935,7 @@ std::vector DetectParamTypes(PyObject* params) { if (exponent == -1 && PyErr_Occurred()) { Py_DECREF(digits_obj); Py_DECREF(as_tuple); - throw py::error_already_set(); // LCOV_EXCL_LINE + throw py::error_already_set(); } int precision; @@ -976,7 +966,7 @@ std::vector DetectParamTypes(PyObject* params) { if (cmp_ge == -1 || cmp_le == -1) { Py_DECREF(digits_obj); Py_DECREF(as_tuple); - throw py::error_already_set(); // LCOV_EXCL_LINE + throw py::error_already_set(); } if (cmp_ge == 1 && cmp_le == 1) { in_money_range = true; @@ -986,7 +976,7 @@ std::vector DetectParamTypes(PyObject* params) { if (cmp_ge == -1 || cmp_le == -1) { Py_DECREF(digits_obj); Py_DECREF(as_tuple); - throw py::error_already_set(); // LCOV_EXCL_LINE + throw py::error_already_set(); } if (cmp_ge == 1 && cmp_le == 1) { in_money_range = true; @@ -1006,7 +996,9 @@ std::vector DetectParamTypes(PyObject* params) { info.decimalDigits = 0; Py_DECREF(digits_obj); Py_DECREF(as_tuple); - PyList_SetItem(params, i, formatted); + if (PyList_SetItem(params, i, formatted) != 0) { + throw py::error_already_set(); + } continue; } @@ -1021,7 +1013,11 @@ std::vector DetectParamTypes(PyObject* params) { info.decimalDigits = nd.scale; // Store NumericData as a Python object in the param list for the binder. py::object numeric_obj = py::cast(nd); - PyList_SetItem(params, i, numeric_obj.release().ptr()); + PyObject* raw = numeric_obj.release().ptr(); + if (PyList_SetItem(params, i, raw) != 0) { + Py_DECREF(raw); + throw py::error_already_set(); + } continue; } @@ -1035,7 +1031,10 @@ std::vector DetectParamTypes(PyObject* params) { info.paramCType = SQL_C_GUID; info.columnSize = 16; info.decimalDigits = 0; - PyList_SetItem(params, i, bytes_le); + if (PyList_SetItem(params, i, bytes_le) != 0) { + Py_DECREF(bytes_le); + throw py::error_already_set(); + } continue; } @@ -2770,8 +2769,11 @@ SQLRETURN SQLExecuteLegacy_wrap(const SqlHandlePtr statementHandle, const std::u dataPtr = bytesStorage.data(); totalBytes = bytesStorage.size(); } else { - dataPtr = PyByteArray_AS_STRING(pyObj); - totalBytes = static_cast(PyByteArray_GET_SIZE(pyObj)); + // bytearray is mutable — copy to stable buffer before streaming + bytesStorage.assign(PyByteArray_AS_STRING(pyObj), + static_cast(PyByteArray_GET_SIZE(pyObj))); + dataPtr = bytesStorage.data(); + totalBytes = bytesStorage.size(); } const size_t chunkSize = DAE_CHUNK_SIZE; for (size_t offset = 0; offset < totalBytes; offset += chunkSize) { @@ -2974,9 +2976,11 @@ SQLRETURN SQLExecute_wrap(const SqlHandlePtr statementHandle, dataPtr = bytesStorage.data(); totalBytes = bytesStorage.size(); } else { - // bytearray: use raw buffer access - dataPtr = PyByteArray_AS_STRING(pyObj); - totalBytes = static_cast(PyByteArray_GET_SIZE(pyObj)); + // bytearray is mutable — copy to stable buffer before streaming + bytesStorage.assign(PyByteArray_AS_STRING(pyObj), + static_cast(PyByteArray_GET_SIZE(pyObj))); + dataPtr = bytesStorage.data(); + totalBytes = bytesStorage.size(); } for (size_t offset = 0; offset < totalBytes; offset += DAE_CHUNK_SIZE) { From 432b52278fcb57d62ad2a4495381e75ba65b98b0 Mon Sep 17 00:00:00 2001 From: Gaurav Sharma Date: Tue, 14 Jul 2026 21:17:02 +0530 Subject: [PATCH 18/29] FIX: Harden Decimal subclass safety, RAII param cleanup, cache init leak - Add PyTuple_Check guard before PyTuple_GET_SIZE/GET_ITEM on Decimal.as_tuple().digits in DetectParamTypes and build_numeric_data - Add ParamResetGuard RAII struct to ensure SQLFreeStmt(SQL_RESET_PARAMS) fires on all exit paths after BindParameters succeeds - Wrap PythonObjectCache::initialize() in try/catch to clean up partial refs on any import failure Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- mssql_python/pybind/ddbc_bindings.cpp | 133 +++++++++++++++++--------- 1 file changed, 86 insertions(+), 47 deletions(-) diff --git a/mssql_python/pybind/ddbc_bindings.cpp b/mssql_python/pybind/ddbc_bindings.cpp index 49b5a15f..37f2eb69 100644 --- a/mssql_python/pybind/ddbc_bindings.cpp +++ b/mssql_python/pybind/ddbc_bindings.cpp @@ -141,53 +141,67 @@ void initialize() { throw py::error_already_set(); } - // Cache datetime.datetime, datetime.date, datetime.time for isinstance - // checks in DetectParamTypes. Single import, then borrowed refs held - // for process lifetime (module-level statics, never decremented). - PyObject* datetime_module = PyImport_ImportModule("datetime"); - if (!datetime_module) throw py::error_already_set(); - datetime_class = PyObject_GetAttrString(datetime_module, "datetime"); - date_class = PyObject_GetAttrString(datetime_module, "date"); - time_class = PyObject_GetAttrString(datetime_module, "time"); - Py_DECREF(datetime_module); - if (!datetime_class || !date_class || !time_class) - throw py::error_already_set(); + try { + // Cache datetime.datetime, datetime.date, datetime.time for isinstance + // checks in DetectParamTypes. Single import, then borrowed refs held + // for process lifetime (module-level statics, never decremented). + PyObject* datetime_module = PyImport_ImportModule("datetime"); + if (!datetime_module) throw py::error_already_set(); + datetime_class = PyObject_GetAttrString(datetime_module, "datetime"); + date_class = PyObject_GetAttrString(datetime_module, "date"); + time_class = PyObject_GetAttrString(datetime_module, "time"); + Py_DECREF(datetime_module); + if (!datetime_class || !date_class || !time_class) { + throw py::error_already_set(); + } - PyObject* decimal_module = PyImport_ImportModule("decimal"); - if (!decimal_module) throw py::error_already_set(); - decimal_class = PyObject_GetAttrString(decimal_module, "Decimal"); - Py_DECREF(decimal_module); - if (!decimal_class) throw py::error_already_set(); - - PyObject* uuid_module = PyImport_ImportModule("uuid"); - if (!uuid_module) throw py::error_already_set(); - uuid_class = PyObject_GetAttrString(uuid_module, "UUID"); - Py_DECREF(uuid_module); - if (!uuid_class) throw py::error_already_set(); - - // Pre-compute MONEY/SMALLMONEY boundary Decimals once. DetectParamTypes - // uses PyObject_RichCompareBool against these to classify Decimal params - // into MONEY vs NUMERIC — doing exact Decimal comparison (not float cast) - // avoids boundary misclassification at the edges. - PyObject* Decimal = decimal_class; - smallmoney_min = PyObject_CallFunction(Decimal, "s", "-214748.3648"); - smallmoney_max = PyObject_CallFunction(Decimal, "s", "214748.3647"); - money_min = PyObject_CallFunction(Decimal, "s", "-922337203685477.5808"); - money_max = PyObject_CallFunction(Decimal, "s", "922337203685477.5807"); - if (!smallmoney_min || !smallmoney_max || !money_min || !money_max) { - // Partial creation — clean up whichever succeeded before throwing. + PyObject* decimal_module = PyImport_ImportModule("decimal"); + if (!decimal_module) throw py::error_already_set(); + decimal_class = PyObject_GetAttrString(decimal_module, "Decimal"); + Py_DECREF(decimal_module); + if (!decimal_class) throw py::error_already_set(); + + PyObject* uuid_module = PyImport_ImportModule("uuid"); + if (!uuid_module) throw py::error_already_set(); + uuid_class = PyObject_GetAttrString(uuid_module, "UUID"); + Py_DECREF(uuid_module); + if (!uuid_class) throw py::error_already_set(); + + // Pre-compute MONEY/SMALLMONEY boundary Decimals once. DetectParamTypes + // uses PyObject_RichCompareBool against these to classify Decimal params + // into MONEY vs NUMERIC — doing exact Decimal comparison (not float cast) + // avoids boundary misclassification at the edges. + PyObject* Decimal = decimal_class; + smallmoney_min = PyObject_CallFunction(Decimal, "s", "-214748.3648"); + smallmoney_max = PyObject_CallFunction(Decimal, "s", "214748.3647"); + money_min = PyObject_CallFunction(Decimal, "s", "-922337203685477.5808"); + money_max = PyObject_CallFunction(Decimal, "s", "922337203685477.5807"); + if (!smallmoney_min || !smallmoney_max || !money_min || !money_max) { + throw py::error_already_set(); + } + + cache_initialized = true; + } catch (...) { + Py_XDECREF(datetime_class); + Py_XDECREF(date_class); + Py_XDECREF(time_class); + Py_XDECREF(decimal_class); + Py_XDECREF(uuid_class); Py_XDECREF(smallmoney_min); Py_XDECREF(smallmoney_max); Py_XDECREF(money_min); Py_XDECREF(money_max); + datetime_class = nullptr; + date_class = nullptr; + time_class = nullptr; + decimal_class = nullptr; + uuid_class = nullptr; smallmoney_min = nullptr; smallmoney_max = nullptr; money_min = nullptr; money_max = nullptr; - throw py::error_already_set(); + throw; } - - cache_initialized = true; } } @@ -570,6 +584,23 @@ SQLDescribeParamFunc SQLDescribeParam_ptr = nullptr; namespace { +// Ensures parameter bindings are always reset before local bound buffers go out +// of scope, preventing dangling APD pointers on error/exception paths. +struct ParamResetGuard { + SQLHSTMT hStmt; + bool active = false; + + explicit ParamResetGuard(SQLHSTMT stmtHandle) : hStmt(stmtHandle) {} + + ~ParamResetGuard() { + if (active && SQLFreeStmt_ptr) { + SQLFreeStmt_ptr(hStmt, SQL_RESET_PARAMS); + } + } + + void arm() { active = true; } +}; + const char* GetSqlCTypeAsString(const SQLSMALLINT cType) { switch (cType) { STRINGIFY_FOR_CASE(SQL_C_CHAR); @@ -929,6 +960,13 @@ std::vector DetectParamTypes(PyObject* params) { throw py::error_already_set(); } + if (!PyTuple_Check(digits_obj)) { + Py_DECREF(digits_obj); + Py_DECREF(exponent_obj); + Py_DECREF(as_tuple); + throw py::type_error("Decimal.as_tuple().digits must be a tuple"); + } + Py_ssize_t num_digits = PyTuple_GET_SIZE(digits_obj); int exponent = static_cast(PyLong_AsLong(exponent_obj)); Py_DECREF(exponent_obj); @@ -1094,6 +1132,12 @@ static NumericData build_numeric_data(PyObject* decimal_param) { } Py_DECREF(exponent_obj); + if (!PyTuple_Check(digits_obj)) { + Py_DECREF(digits_obj); + Py_DECREF(as_tuple); + throw py::type_error("Decimal.as_tuple().digits must be a tuple"); + } + int num_digits = static_cast(PyTuple_GET_SIZE(digits_obj)); int precision, scale; if (exponent >= 0) { @@ -2662,6 +2706,8 @@ SQLRETURN SQLExecuteLegacy_wrap(const SqlHandlePtr statementHandle, const std::u if (!SQL_SUCCEEDED(rc)) { return rc; } + ParamResetGuard resetGuard(hStmt); + resetGuard.arm(); { // Release the GIL during the blocking SQLExecute network call. @@ -2804,11 +2850,6 @@ SQLRETURN SQLExecuteLegacy_wrap(const SqlHandlePtr statementHandle, const std::u rc, (void*)hStmt); return rc; } - - // Unbind the bound buffers for all parameters coz the buffers' memory - // will be freed when this function exits (parambuffers goes out of - // scope) - rc = SQLFreeStmt_ptr(hStmt, SQL_RESET_PARAMS); return rc; } } @@ -2892,6 +2933,8 @@ SQLRETURN SQLExecute_wrap(const SqlHandlePtr statementHandle, std::vector> paramBuffers; rc = BindParameters(*statementHandle, hStmt, params, paramInfos, paramBuffers, charEncoding); if (!SQL_SUCCEEDED(rc)) return rc; + ParamResetGuard resetGuard(hStmt); + resetGuard.arm(); { py::gil_scoped_release release; @@ -3002,11 +3045,7 @@ SQLRETURN SQLExecute_wrap(const SqlHandlePtr statementHandle, if (!SQL_SUCCEEDED(rc) && rc != SQL_NO_DATA) return rc; - // Preserve the execute return code (e.g. SQL_SUCCESS_WITH_INFO) — don't - // let the SQLFreeStmt return value clobber what the caller needs to see. - SQLRETURN exec_rc = rc; - SQLFreeStmt_ptr(hStmt, SQL_RESET_PARAMS); - return exec_rc; + return rc; } SQLRETURN BindParameterArray(SqlHandle& handle, SQLHANDLE hStmt, const py::list& columnwise_params, From fd06b80922a7559e347b199fcf760fda8e494f1f Mon Sep 17 00:00:00 2001 From: Gaurav Sharma Date: Wed, 15 Jul 2026 11:53:54 +0530 Subject: [PATCH 19/29] =?UTF-8?q?FIX:=20Revert=20RAII=20ParamResetGuard=20?= =?UTF-8?q?=E2=80=94=20it=20wiped=20ODBC=20diagnostics=20on=20error?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ParamResetGuard called SQLFreeStmt(SQL_RESET_PARAMS) in its destructor before the caller could read SQLGetDiagRec, producing empty SQLSTATEs. Restore manual SQLFreeStmt on success-only paths. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- mssql_python/pybind/ddbc_bindings.cpp | 31 ++++++++------------------- 1 file changed, 9 insertions(+), 22 deletions(-) diff --git a/mssql_python/pybind/ddbc_bindings.cpp b/mssql_python/pybind/ddbc_bindings.cpp index 37f2eb69..d29c2ab1 100644 --- a/mssql_python/pybind/ddbc_bindings.cpp +++ b/mssql_python/pybind/ddbc_bindings.cpp @@ -584,22 +584,6 @@ SQLDescribeParamFunc SQLDescribeParam_ptr = nullptr; namespace { -// Ensures parameter bindings are always reset before local bound buffers go out -// of scope, preventing dangling APD pointers on error/exception paths. -struct ParamResetGuard { - SQLHSTMT hStmt; - bool active = false; - - explicit ParamResetGuard(SQLHSTMT stmtHandle) : hStmt(stmtHandle) {} - - ~ParamResetGuard() { - if (active && SQLFreeStmt_ptr) { - SQLFreeStmt_ptr(hStmt, SQL_RESET_PARAMS); - } - } - - void arm() { active = true; } -}; const char* GetSqlCTypeAsString(const SQLSMALLINT cType) { switch (cType) { @@ -2706,9 +2690,6 @@ SQLRETURN SQLExecuteLegacy_wrap(const SqlHandlePtr statementHandle, const std::u if (!SQL_SUCCEEDED(rc)) { return rc; } - ParamResetGuard resetGuard(hStmt); - resetGuard.arm(); - { // Release the GIL during the blocking SQLExecute network call. py::gil_scoped_release release; @@ -2850,6 +2831,10 @@ SQLRETURN SQLExecuteLegacy_wrap(const SqlHandlePtr statementHandle, const std::u rc, (void*)hStmt); return rc; } + + // Unbind parameter buffers before they go out of scope. + // Not called on error paths — diagnostics must remain readable. + SQLFreeStmt_ptr(hStmt, SQL_RESET_PARAMS); return rc; } } @@ -2933,8 +2918,6 @@ SQLRETURN SQLExecute_wrap(const SqlHandlePtr statementHandle, std::vector> paramBuffers; rc = BindParameters(*statementHandle, hStmt, params, paramInfos, paramBuffers, charEncoding); if (!SQL_SUCCEEDED(rc)) return rc; - ParamResetGuard resetGuard(hStmt); - resetGuard.arm(); { py::gil_scoped_release release; @@ -3045,7 +3028,11 @@ SQLRETURN SQLExecute_wrap(const SqlHandlePtr statementHandle, if (!SQL_SUCCEEDED(rc) && rc != SQL_NO_DATA) return rc; - return rc; + // Unbind parameter buffers before they go out of scope. + // Not called on error paths — diagnostics must remain readable. + SQLRETURN exec_rc = rc; + SQLFreeStmt_ptr(hStmt, SQL_RESET_PARAMS); + return exec_rc; } SQLRETURN BindParameterArray(SqlHandle& handle, SQLHANDLE hStmt, const py::list& columnwise_params, From 48ba475d6655bd60513a3ffcf5754d6932e870a3 Mon Sep 17 00:00:00 2001 From: Gaurav Sharma Date: Wed, 15 Jul 2026 12:12:56 +0530 Subject: [PATCH 20/29] =?UTF-8?q?REFACTOR:=20Extract=20helpers=20=E2=80=94?= =?UTF-8?q?=20PyObjGuard,=20stream=5Fdae=5Fchunks,=20get=5Fcached=5Fclass,?= =?UTF-8?q?=20import=5Fattr?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - PyObjGuard: RAII cleanup for Decimal tuple extraction in DetectParamTypes and build_numeric_data (eliminates ~15 manual decref cascades) - stream_dae_chunks(): template replacing 6 identical DAE chunking loops across legacy and fast execute paths - get_cached_class(): single helper replacing 5 copy-paste type getter functions - import_attr(): consolidates import-module-getattr-decref pattern in PythonObjectCache::initialize() Net -94 lines, no functional changes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- mssql_python/pybind/ddbc_bindings.cpp | 400 ++++++++++---------------- 1 file changed, 153 insertions(+), 247 deletions(-) diff --git a/mssql_python/pybind/ddbc_bindings.cpp b/mssql_python/pybind/ddbc_bindings.cpp index d29c2ab1..542fbf6b 100644 --- a/mssql_python/pybind/ddbc_bindings.cpp +++ b/mssql_python/pybind/ddbc_bindings.cpp @@ -107,6 +107,37 @@ PyObject* get_time_class(); py::object get_time_class_obj(); } +// RAII guard for automatic PyObject cleanup on scope exit. +// Tracks a small set of PyObject* refs and decrefs them in destructor. +class PyObjGuard { + PyObject* refs[8] = {}; + int count = 0; + +public: + void track(PyObject* obj) { + for (int i = 0; i < count; ++i) { + if (refs[i] == nullptr) { + refs[i] = obj; + return; + } + } + if (count < 8) refs[count++] = obj; + } + + void release(PyObject* obj) { + for (int i = 0; i < count; ++i) { + if (refs[i] == obj) { + refs[i] = nullptr; + return; + } + } + } + + ~PyObjGuard() { + for (int i = count - 1; i >= 0; --i) Py_XDECREF(refs[i]); + } +}; + //------------------------------------------------------------------------------------------------- //------------------------------------------------------------------------------------------------- // Logging Infrastructure: @@ -129,6 +160,24 @@ static PyObject* smallmoney_min = nullptr; static PyObject* smallmoney_max = nullptr; static bool cache_initialized = false; +static PyObject* import_attr(const char* module_name, const char* attr_name) { + PyObject* mod = PyImport_ImportModule(module_name); + if (!mod) throw py::error_already_set(); + PyObject* attr = PyObject_GetAttrString(mod, attr_name); + Py_DECREF(mod); + if (!attr) throw py::error_already_set(); + return attr; +} + +static PyObject* get_cached_class(PyObject* cached, const char* module_name, const char* attr_name) { + if (cache_initialized && cached) return cached; + PyObject* mod = PyImport_ImportModule(module_name); + if (!mod) return nullptr; + PyObject* cls = PyObject_GetAttrString(mod, attr_name); + Py_DECREF(mod); + return cls; +} + // One-time cache of Python type objects and MONEY boundary constants. // Called on first execute(). Uses raw CPython API (not pybind11) because // these cached PyObject* are compared via PyObject_IsInstance in the @@ -155,17 +204,8 @@ void initialize() { throw py::error_already_set(); } - PyObject* decimal_module = PyImport_ImportModule("decimal"); - if (!decimal_module) throw py::error_already_set(); - decimal_class = PyObject_GetAttrString(decimal_module, "Decimal"); - Py_DECREF(decimal_module); - if (!decimal_class) throw py::error_already_set(); - - PyObject* uuid_module = PyImport_ImportModule("uuid"); - if (!uuid_module) throw py::error_already_set(); - uuid_class = PyObject_GetAttrString(uuid_module, "UUID"); - Py_DECREF(uuid_module); - if (!uuid_class) throw py::error_already_set(); + decimal_class = import_attr("decimal", "Decimal"); + uuid_class = import_attr("uuid", "UUID"); // Pre-compute MONEY/SMALLMONEY boundary Decimals once. DetectParamTypes // uses PyObject_RichCompareBool against these to classify Decimal params @@ -216,14 +256,7 @@ static py::object wrap_cached_or_imported(PyObject* obj) { // Returns cached type. Fallback import only for edge case where cache wasn't initialized // (e.g., called from legacy path before any fast-path execute). PyObject* get_datetime_class() { - if (cache_initialized && datetime_class) { - return datetime_class; - } - PyObject* mod = PyImport_ImportModule("datetime"); - if (!mod) return nullptr; - PyObject* cls = PyObject_GetAttrString(mod, "datetime"); - Py_DECREF(mod); - return cls; // caller must check for NULL + return get_cached_class(datetime_class, "datetime", "datetime"); } py::object get_datetime_class_obj() { @@ -233,14 +266,7 @@ py::object get_datetime_class_obj() { // Returns cached type. Fallback import only for edge case where cache wasn't initialized // (e.g., called from legacy path before any fast-path execute). PyObject* get_date_class() { - if (cache_initialized && date_class) { - return date_class; - } - PyObject* mod = PyImport_ImportModule("datetime"); - if (!mod) return nullptr; - PyObject* cls = PyObject_GetAttrString(mod, "date"); - Py_DECREF(mod); - return cls; // caller must check for NULL + return get_cached_class(date_class, "datetime", "date"); } py::object get_date_class_obj() { @@ -250,14 +276,7 @@ py::object get_date_class_obj() { // Returns cached type. Fallback import only for edge case where cache wasn't initialized // (e.g., called from legacy path before any fast-path execute). PyObject* get_time_class() { - if (cache_initialized && time_class) { - return time_class; - } - PyObject* mod = PyImport_ImportModule("datetime"); - if (!mod) return nullptr; - PyObject* cls = PyObject_GetAttrString(mod, "time"); - Py_DECREF(mod); - return cls; // caller must check for NULL + return get_cached_class(time_class, "datetime", "time"); } py::object get_time_class_obj() { @@ -267,14 +286,7 @@ py::object get_time_class_obj() { // Returns cached type. Fallback import only for edge case where cache wasn't initialized // (e.g., called from legacy path before any fast-path execute). PyObject* get_decimal_class() { - if (cache_initialized && decimal_class) { - return decimal_class; - } - PyObject* mod = PyImport_ImportModule("decimal"); - if (!mod) return nullptr; - PyObject* cls = PyObject_GetAttrString(mod, "Decimal"); - Py_DECREF(mod); - return cls; // caller must check for NULL + return get_cached_class(decimal_class, "decimal", "Decimal"); } py::object get_decimal_class_obj() { @@ -284,14 +296,7 @@ py::object get_decimal_class_obj() { // Returns cached type. Fallback import only for edge case where cache wasn't initialized // (e.g., called from legacy path before any fast-path execute). PyObject* get_uuid_class() { - if (cache_initialized && uuid_class) { - return uuid_class; - } - PyObject* mod = PyImport_ImportModule("uuid"); - if (!mod) return nullptr; - PyObject* cls = PyObject_GetAttrString(mod, "UUID"); - Py_DECREF(mod); - return cls; // caller must check for NULL + return get_cached_class(uuid_class, "uuid", "UUID"); } py::object get_uuid_class_obj() { @@ -922,43 +927,32 @@ std::vector DetectParamTypes(PyObject* params) { if (is_decimal == 1) { PyObject* as_tuple = PyObject_CallMethod(obj, "as_tuple", NULL); if (!as_tuple) throw py::error_already_set(); + PyObjGuard guard; + guard.track(as_tuple); PyObject* exponent_obj = PyObject_GetAttrString(as_tuple, "exponent"); - if (!exponent_obj) { - Py_DECREF(as_tuple); - throw py::error_already_set(); - } + if (!exponent_obj) throw py::error_already_set(); + guard.track(exponent_obj); // NaN / Infinity / sNaN: refuse rather than silently writing 0. if (PyUnicode_Check(exponent_obj)) { - Py_DECREF(exponent_obj); - Py_DECREF(as_tuple); throw py::value_error( "Cannot bind non-finite Decimal (NaN/Infinity) as SQL NUMERIC"); } PyObject* digits_obj = PyObject_GetAttrString(as_tuple, "digits"); - if (!digits_obj) { - Py_DECREF(exponent_obj); - Py_DECREF(as_tuple); - throw py::error_already_set(); - } + if (!digits_obj) throw py::error_already_set(); + guard.track(digits_obj); if (!PyTuple_Check(digits_obj)) { - Py_DECREF(digits_obj); - Py_DECREF(exponent_obj); - Py_DECREF(as_tuple); throw py::type_error("Decimal.as_tuple().digits must be a tuple"); } Py_ssize_t num_digits = PyTuple_GET_SIZE(digits_obj); int exponent = static_cast(PyLong_AsLong(exponent_obj)); + guard.release(exponent_obj); Py_DECREF(exponent_obj); - if (exponent == -1 && PyErr_Occurred()) { - Py_DECREF(digits_obj); - Py_DECREF(as_tuple); - throw py::error_already_set(); - } + if (exponent == -1 && PyErr_Occurred()) throw py::error_already_set(); int precision; if (exponent >= 0) @@ -969,8 +963,6 @@ std::vector DetectParamTypes(PyObject* params) { precision = -exponent; if (precision > MAX_NUMERIC_PRECISION) { - Py_DECREF(digits_obj); - Py_DECREF(as_tuple); throw py::value_error( "Precision of the numeric value is too high. " "The maximum precision supported by SQL Server is " + @@ -985,21 +977,13 @@ std::vector DetectParamTypes(PyObject* params) { bool in_money_range = false; int cmp_ge = PyObject_RichCompareBool(obj, PythonObjectCache::smallmoney_min, Py_GE); int cmp_le = PyObject_RichCompareBool(obj, PythonObjectCache::smallmoney_max, Py_LE); - if (cmp_ge == -1 || cmp_le == -1) { - Py_DECREF(digits_obj); - Py_DECREF(as_tuple); - throw py::error_already_set(); - } + if (cmp_ge == -1 || cmp_le == -1) throw py::error_already_set(); if (cmp_ge == 1 && cmp_le == 1) { in_money_range = true; } else { cmp_ge = PyObject_RichCompareBool(obj, PythonObjectCache::money_min, Py_GE); cmp_le = PyObject_RichCompareBool(obj, PythonObjectCache::money_max, Py_LE); - if (cmp_ge == -1 || cmp_le == -1) { - Py_DECREF(digits_obj); - Py_DECREF(as_tuple); - throw py::error_already_set(); - } + if (cmp_ge == -1 || cmp_le == -1) throw py::error_already_set(); if (cmp_ge == 1 && cmp_le == 1) { in_money_range = true; } @@ -1007,16 +991,14 @@ std::vector DetectParamTypes(PyObject* params) { if (in_money_range) { PyObject* formatted = PyObject_CallMethod(obj, "__format__", "s", "f"); - if (!formatted) { - Py_DECREF(digits_obj); - Py_DECREF(as_tuple); - throw py::error_already_set(); - } + if (!formatted) throw py::error_already_set(); info.paramSQLType = SQL_VARCHAR; info.paramCType = PARAM_C_TYPE_TEXT; info.columnSize = PyUnicode_GET_LENGTH(formatted); info.decimalDigits = 0; + guard.release(digits_obj); Py_DECREF(digits_obj); + guard.release(as_tuple); Py_DECREF(as_tuple); if (PyList_SetItem(params, i, formatted) != 0) { throw py::error_already_set(); @@ -1028,7 +1010,9 @@ std::vector DetectParamTypes(PyObject* params) { // object in the param list so BindParameters can extract it as NumericData. info.paramSQLType = SQL_NUMERIC; info.paramCType = SQL_C_NUMERIC; + guard.release(digits_obj); Py_DECREF(digits_obj); + guard.release(as_tuple); Py_DECREF(as_tuple); NumericData nd = build_numeric_data(obj); info.columnSize = nd.precision; @@ -1075,50 +1059,33 @@ std::vector DetectParamTypes(PyObject* params) { static NumericData build_numeric_data(PyObject* decimal_param) { PyObject* as_tuple = PyObject_CallMethod(decimal_param, "as_tuple", NULL); if (!as_tuple) throw py::error_already_set(); + PyObjGuard guard; + guard.track(as_tuple); PyObject* digits_obj = PyObject_GetAttrString(as_tuple, "digits"); - if (!digits_obj) { - Py_DECREF(as_tuple); - throw py::error_already_set(); - } + if (!digits_obj) throw py::error_already_set(); + guard.track(digits_obj); PyObject* sign_obj = PyObject_GetAttrString(as_tuple, "sign"); - if (!sign_obj) { - Py_DECREF(digits_obj); - Py_DECREF(as_tuple); - throw py::error_already_set(); - } + if (!sign_obj) throw py::error_already_set(); + guard.track(sign_obj); PyObject* exponent_obj = PyObject_GetAttrString(as_tuple, "exponent"); - if (!exponent_obj) { - Py_DECREF(sign_obj); - Py_DECREF(digits_obj); - Py_DECREF(as_tuple); - throw py::error_already_set(); - } + if (!exponent_obj) throw py::error_already_set(); + guard.track(exponent_obj); int sign_val = static_cast(PyLong_AsLong(sign_obj)); + guard.release(sign_obj); Py_DECREF(sign_obj); - if (sign_val == -1 && PyErr_Occurred()) { - Py_DECREF(exponent_obj); - Py_DECREF(digits_obj); - Py_DECREF(as_tuple); - throw py::error_already_set(); - } + if (sign_val == -1 && PyErr_Occurred()) throw py::error_already_set(); int exponent = 0; if (PyLong_Check(exponent_obj)) { exponent = static_cast(PyLong_AsLong(exponent_obj)); - if (exponent == -1 && PyErr_Occurred()) { - Py_DECREF(exponent_obj); - Py_DECREF(digits_obj); - Py_DECREF(as_tuple); - throw py::error_already_set(); - } + if (exponent == -1 && PyErr_Occurred()) throw py::error_already_set(); } + guard.release(exponent_obj); Py_DECREF(exponent_obj); if (!PyTuple_Check(digits_obj)) { - Py_DECREF(digits_obj); - Py_DECREF(as_tuple); throw py::type_error("Decimal.as_tuple().digits must be a tuple"); } @@ -1136,11 +1103,9 @@ static NumericData build_numeric_data(PyObject* decimal_param) { PyObject* py_ten = PyLong_FromLong(10); PyObject* int_val = PyLong_FromLong(0); + guard.track(py_ten); + guard.track(int_val); if (!py_ten || !int_val) { - Py_XDECREF(int_val); - Py_XDECREF(py_ten); - Py_DECREF(digits_obj); - Py_DECREF(as_tuple); throw py::error_already_set(); } @@ -1148,91 +1113,70 @@ static NumericData build_numeric_data(PyObject* decimal_param) { for (Py_ssize_t i = 0; i < digit_count; ++i) { PyObject* digit_obj = PyTuple_GET_ITEM(digits_obj, i); long digit = PyLong_AsLong(digit_obj); - if (digit == -1 && PyErr_Occurred()) { - Py_DECREF(int_val); - Py_DECREF(py_ten); - Py_DECREF(digits_obj); - Py_DECREF(as_tuple); - throw py::error_already_set(); - } + if (digit == -1 && PyErr_Occurred()) throw py::error_already_set(); PyObject* multiplied = PyNumber_Multiply(int_val, py_ten); + guard.track(multiplied); + guard.release(int_val); Py_DECREF(int_val); - if (!multiplied) { - Py_DECREF(py_ten); - Py_DECREF(digits_obj); - Py_DECREF(as_tuple); - throw py::error_already_set(); - } + if (!multiplied) throw py::error_already_set(); PyObject* py_digit = PyLong_FromLong(digit); - if (!py_digit) { - Py_DECREF(multiplied); - Py_DECREF(py_ten); - Py_DECREF(digits_obj); - Py_DECREF(as_tuple); - throw py::error_already_set(); - } + if (!py_digit) throw py::error_already_set(); + guard.track(py_digit); int_val = PyNumber_Add(multiplied, py_digit); + guard.track(int_val); + guard.release(multiplied); Py_DECREF(multiplied); + guard.release(py_digit); Py_DECREF(py_digit); - if (!int_val) { - Py_DECREF(py_ten); - Py_DECREF(digits_obj); - Py_DECREF(as_tuple); - throw py::error_already_set(); - } + if (!int_val) throw py::error_already_set(); } if (exponent > 0) { PyObject* multiplier = PyLong_FromLong(1); - if (!multiplier) { - Py_DECREF(int_val); - Py_DECREF(py_ten); - Py_DECREF(digits_obj); - Py_DECREF(as_tuple); - throw py::error_already_set(); - } + if (!multiplier) throw py::error_already_set(); + guard.track(multiplier); for (int j = 0; j < exponent; ++j) { PyObject* next_multiplier = PyNumber_Multiply(multiplier, py_ten); + guard.track(next_multiplier); + guard.release(multiplier); Py_DECREF(multiplier); - if (!next_multiplier) { - Py_DECREF(int_val); - Py_DECREF(py_ten); - Py_DECREF(digits_obj); - Py_DECREF(as_tuple); - throw py::error_already_set(); - } + if (!next_multiplier) throw py::error_already_set(); multiplier = next_multiplier; } PyObject* scaled_val = PyNumber_Multiply(int_val, multiplier); + guard.track(scaled_val); + guard.release(multiplier); Py_DECREF(multiplier); + guard.release(int_val); Py_DECREF(int_val); - if (!scaled_val) { - Py_DECREF(py_ten); - Py_DECREF(digits_obj); - Py_DECREF(as_tuple); - throw py::error_already_set(); - } + if (!scaled_val) throw py::error_already_set(); int_val = scaled_val; } PyObject* abs_val = PyNumber_Absolute(int_val); + guard.track(abs_val); + guard.release(int_val); Py_DECREF(int_val); + guard.release(py_ten); Py_DECREF(py_ten); + guard.release(digits_obj); Py_DECREF(digits_obj); + guard.release(as_tuple); Py_DECREF(as_tuple); if (!abs_val) throw py::error_already_set(); PyObject* val_bytes = PyObject_CallMethod(abs_val, "to_bytes", "is", 16, "little"); + guard.track(val_bytes); + guard.release(abs_val); Py_DECREF(abs_val); if (!val_bytes) throw py::error_already_set(); char* val_buf = nullptr; Py_ssize_t val_size = 0; if (PyBytes_AsStringAndSize(val_bytes, &val_buf, &val_size) == -1) { - Py_DECREF(val_bytes); throw py::error_already_set(); } @@ -1250,11 +1194,24 @@ static NumericData build_numeric_data(PyObject* decimal_param) { std::memcpy(&nd.val[0], val_buf, copy_len); // NOLINT(clang-analyzer-security.insecureAPI.DeprecatedOrUnsafeBufferHandling) #endif } + guard.release(val_bytes); Py_DECREF(val_bytes); return nd; } +template +static SQLRETURN stream_dae_chunks(const void* data, size_t total_bytes, PutDataFn put_data_fn) { + const char* bytes = static_cast(data); + for (size_t offset = 0; offset < total_bytes; offset += DAE_CHUNK_SIZE) { + size_t len = std::min(static_cast(DAE_CHUNK_SIZE), total_bytes - offset); + SQLRETURN rc = put_data_fn( + static_cast(const_cast(bytes + offset)), static_cast(len)); + if (!SQL_SUCCEEDED(rc)) return rc; + } + return SQL_SUCCESS; +} + // GH-610: Resolve SQL type for a NULL parameter using per-handle cache. // On cache miss, calls SQLDescribeParam and stores the result. static DescribedParamInfo ResolveNullParamType(SqlHandle& handle, SQLHANDLE hStmt, int paramIndex) { @@ -2731,27 +2688,13 @@ SQLRETURN SQLExecuteLegacy_wrap(const SqlHandlePtr statementHandle, const std::u if (matchedInfo->paramCType == SQL_C_WCHAR) { std::u16string utf16 = py::reinterpret_borrow(py::handle(pyObj)).cast(); - size_t totalChars = utf16.size(); - const SQLWCHAR* dataPtr = reinterpretU16stringAsSqlWChar(utf16); - size_t offset = 0; - size_t chunkChars = DAE_CHUNK_SIZE / sizeof(SQLWCHAR); - while (offset < totalChars) { - size_t len = std::min(chunkChars, totalChars - offset); - size_t lenBytes = len * sizeof(SQLWCHAR); - if (lenBytes > - static_cast(std::numeric_limits::max())) { - ThrowStdException("Chunk size exceeds maximum " - "allowed by SQLLEN"); - } - rc = putData((SQLPOINTER)(dataPtr + offset), - static_cast(lenBytes)); - if (!SQL_SUCCEEDED(rc)) { - LOG("SQLExecute: SQLPutData failed for " - "SQL_C_WCHAR chunk - offset=%zu", - offset, totalChars, lenBytes, rc); - return rc; - } - offset += len; + rc = stream_dae_chunks( + reinterpretU16stringAsSqlWChar(utf16), + utf16.size() * sizeof(SQLWCHAR), + putData); + if (!SQL_SUCCEEDED(rc)) { + LOG("SQLExecute: SQLPutData failed for SQL_C_WCHAR DAE streaming"); + return rc; } } else if (matchedInfo->paramCType == SQL_C_CHAR) { // Encode the string using the specified encoding @@ -2768,21 +2711,10 @@ SQLRETURN SQLExecuteLegacy_wrap(const SqlHandlePtr statementHandle, const std::u throw; } - size_t totalBytes = encodedStr.size(); - const char* dataPtr = encodedStr.data(); - size_t offset = 0; - size_t chunkBytes = DAE_CHUNK_SIZE; - while (offset < totalBytes) { - size_t len = std::min(chunkBytes, totalBytes - offset); - - rc = putData((SQLPOINTER)(dataPtr + offset), static_cast(len)); - if (!SQL_SUCCEEDED(rc)) { - LOG("SQLExecute: SQLPutData failed for " - "SQL_C_CHAR chunk - offset=%zu", - offset, totalBytes, len, rc); - return rc; - } - offset += len; + rc = stream_dae_chunks(encodedStr.data(), encodedStr.size(), putData); + if (!SQL_SUCCEEDED(rc)) { + LOG("SQLExecute: SQLPutData failed for SQL_C_CHAR DAE streaming"); + return rc; } } else { ThrowStdException("Unsupported C type for str in DAE"); @@ -2802,16 +2734,11 @@ SQLRETURN SQLExecuteLegacy_wrap(const SqlHandlePtr statementHandle, const std::u dataPtr = bytesStorage.data(); totalBytes = bytesStorage.size(); } - const size_t chunkSize = DAE_CHUNK_SIZE; - for (size_t offset = 0; offset < totalBytes; offset += chunkSize) { - size_t len = std::min(chunkSize, totalBytes - offset); - rc = putData((SQLPOINTER)(dataPtr + offset), static_cast(len)); - if (!SQL_SUCCEEDED(rc)) { - LOG("SQLExecute: SQLPutData failed for " - "binary/bytes chunk - offset=%zu", - offset, totalBytes, len, rc); - return rc; - } + + rc = stream_dae_chunks(dataPtr, totalBytes, putData); + if (!SQL_SUCCEEDED(rc)) { + LOG("SQLExecute: SQLPutData failed for binary/bytes DAE streaming"); + return rc; } } else { ThrowStdException("DAE only supported for str or bytes"); @@ -2930,6 +2857,10 @@ SQLRETURN SQLExecute_wrap(const SqlHandlePtr statementHandle, // GIL is released around each ODBC call to match slow-path concurrency. if (rc == SQL_NEED_DATA) { SQLPOINTER paramToken = nullptr; + auto putData = [&](SQLPOINTER data, SQLLEN len) { + py::gil_scoped_release release; + return SQLPutData_ptr(hStmt, data, len); + }; while (true) { { py::gil_scoped_release release; @@ -2958,35 +2889,18 @@ SQLRETURN SQLExecute_wrap(const SqlHandlePtr statementHandle, if (matchedInfo->paramCType == SQL_C_WCHAR) { std::u16string u16 = py::reinterpret_borrow(py::handle(pyObj)).cast(); - const SQLWCHAR* dataPtr = reinterpretU16stringAsSqlWChar(u16); - size_t totalChars = u16.size(); - size_t chunkChars = DAE_CHUNK_SIZE / sizeof(SQLWCHAR); - for (size_t offset = 0; offset < totalChars; offset += chunkChars) { - size_t len = std::min(chunkChars, totalChars - offset); - { - py::gil_scoped_release release; - rc = SQLPutData_ptr(hStmt, (SQLPOINTER)(dataPtr + offset), - static_cast(len * sizeof(SQLWCHAR))); - } - if (!SQL_SUCCEEDED(rc)) return rc; - } + rc = stream_dae_chunks( + reinterpretU16stringAsSqlWChar(u16), + u16.size() * sizeof(SQLWCHAR), + putData); + if (!SQL_SUCCEEDED(rc)) return rc; } else if (matchedInfo->paramCType == SQL_C_CHAR) { std::string encodedStr; py::object encoded = py::reinterpret_borrow(py::handle(pyObj)) .attr("encode")(charEncoding, "strict"); encodedStr = encoded.cast(); - const char* dataPtr = encodedStr.data(); - size_t totalBytes = encodedStr.size(); - for (size_t offset = 0; offset < totalBytes; offset += DAE_CHUNK_SIZE) { - size_t len = std::min(static_cast(DAE_CHUNK_SIZE), - totalBytes - offset); - { - py::gil_scoped_release release; - rc = SQLPutData_ptr(hStmt, (SQLPOINTER)(dataPtr + offset), - static_cast(len)); - } - if (!SQL_SUCCEEDED(rc)) return rc; - } + rc = stream_dae_chunks(encodedStr.data(), encodedStr.size(), putData); + if (!SQL_SUCCEEDED(rc)) return rc; } else { ThrowStdException("SQLExecuteFast: unsupported C type for str in DAE"); } @@ -3009,16 +2923,8 @@ SQLRETURN SQLExecute_wrap(const SqlHandlePtr statementHandle, totalBytes = bytesStorage.size(); } - for (size_t offset = 0; offset < totalBytes; offset += DAE_CHUNK_SIZE) { - size_t len = std::min(static_cast(DAE_CHUNK_SIZE), - totalBytes - offset); - { - py::gil_scoped_release release; - rc = SQLPutData_ptr(hStmt, (SQLPOINTER)(dataPtr + offset), - static_cast(len)); - } - if (!SQL_SUCCEEDED(rc)) return rc; - } + rc = stream_dae_chunks(dataPtr, totalBytes, putData); + if (!SQL_SUCCEEDED(rc)) return rc; } else { ThrowStdException("SQLExecuteFast: DAE only supported for str or bytes"); } From 114d76b3dd737dd044908590b4ab744d015a3206 Mon Sep 17 00:00:00 2001 From: Gaurav Sharma Date: Wed, 15 Jul 2026 12:16:59 +0530 Subject: [PATCH 21/29] FIX: Suppress DevSkim DS121708 for bounded memcpy on non-MSVC Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- mssql_python/pybind/ddbc_bindings.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mssql_python/pybind/ddbc_bindings.cpp b/mssql_python/pybind/ddbc_bindings.cpp index 542fbf6b..44083c59 100644 --- a/mssql_python/pybind/ddbc_bindings.cpp +++ b/mssql_python/pybind/ddbc_bindings.cpp @@ -1191,7 +1191,7 @@ static NumericData build_numeric_data(PyObject* decimal_param) { memcpy_s(&nd.val[0], SQL_MAX_NUMERIC_LEN, val_buf, copy_len); #else // copy_len is bounded to SQL_MAX_NUMERIC_LEN above — safe by construction - std::memcpy(&nd.val[0], val_buf, copy_len); // NOLINT(clang-analyzer-security.insecureAPI.DeprecatedOrUnsafeBufferHandling) + std::memcpy(&nd.val[0], val_buf, copy_len); // DevSkim: ignore DS121708 #endif } guard.release(val_bytes); From 3499dbdeb31ffe433f377c25298329d3876b9283 Mon Sep 17 00:00:00 2001 From: Gaurav Sharma Date: Wed, 15 Jul 2026 12:40:55 +0530 Subject: [PATCH 22/29] DOC: Add algorithm comments to build_numeric_data and Decimal detection Explain the SQL_NUMERIC_STRUCT conversion algorithm step-by-step, precision/scale computation logic, MONEY range check rationale, and one-liners on helper utilities (PyObjGuard, stream_dae_chunks, get_cached_class). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- mssql_python/pybind/ddbc_bindings.cpp | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/mssql_python/pybind/ddbc_bindings.cpp b/mssql_python/pybind/ddbc_bindings.cpp index 44083c59..dc6a3869 100644 --- a/mssql_python/pybind/ddbc_bindings.cpp +++ b/mssql_python/pybind/ddbc_bindings.cpp @@ -124,6 +124,7 @@ class PyObjGuard { if (count < 8) refs[count++] = obj; } + // release() hands ownership back to a caller that decrefs immediately, avoiding double-decref in ~PyObjGuard(). void release(PyObject* obj) { for (int i = 0; i < count; ++i) { if (refs[i] == obj) { @@ -169,6 +170,7 @@ static PyObject* import_attr(const char* module_name, const char* attr_name) { return attr; } +// Fall back to import here because legacy paths can reach these lookups before initialize_cache() has populated globals. static PyObject* get_cached_class(PyObject* cached, const char* module_name, const char* attr_name) { if (cache_initialized && cached) return cached; PyObject* mod = PyImport_ImportModule(module_name); @@ -955,6 +957,9 @@ std::vector DetectParamTypes(PyObject* params) { if (exponent == -1 && PyErr_Occurred()) throw py::error_already_set(); int precision; + // Precision is total base-10 digits after applying Decimal's exponent: positive exponents + // add trailing zeros, in-range negative exponents keep the original digit count, and larger + // negative exponents force leading fractional zeros such as Decimal("0.001") -> precision 3. if (exponent >= 0) precision = static_cast(num_digits) + exponent; else if ((-exponent) <= num_digits) @@ -970,6 +975,8 @@ std::vector DetectParamTypes(PyObject* params) { std::to_string(precision) + "."); } + // Check SMALLMONEY first, then widen to MONEY, so common small values keep the narrowest + // exact range while still accepting larger fixed-point values supported by SQL Server. // MONEY/SMALLMONEY: SQL Server stores these as fixed-point integers internally. // We bind as formatted VARCHAR (e.g., "214748.3647") because SQL_C_NUMERIC can't // represent the exact money range without precision loss on certain ODBC drivers. @@ -1057,11 +1064,14 @@ std::vector DetectParamTypes(PyObject* params) { // Takes raw PyObject* (must be a Decimal instance). Returns NumericData directly — // the caller stores it as a Python capsule via PyList_SetItem for the binder to consume. static NumericData build_numeric_data(PyObject* decimal_param) { + // Decimal.as_tuple() exposes the canonical pieces we need: sign, coefficient digits, and exponent. PyObject* as_tuple = PyObject_CallMethod(decimal_param, "as_tuple", NULL); if (!as_tuple) throw py::error_already_set(); PyObjGuard guard; guard.track(as_tuple); + // Step 1-4: unpack Decimal(sign, digits, exponent) and validate that digits is the tuple form + // promised by Decimal.as_tuple(); the remaining logic works purely from these normalized parts. PyObject* digits_obj = PyObject_GetAttrString(as_tuple, "digits"); if (!digits_obj) throw py::error_already_set(); guard.track(digits_obj); @@ -1089,6 +1099,9 @@ static NumericData build_numeric_data(PyObject* decimal_param) { throw py::type_error("Decimal.as_tuple().digits must be a tuple"); } + // Step 5: SQL Server precision counts all stored decimal digits, while scale is just the fractional + // digits. A positive exponent means trailing zeros move into the integer part; a negative exponent + // means scale = -exponent and precision must still cover leading fractional zeros such as 0.001. int num_digits = static_cast(PyTuple_GET_SIZE(digits_obj)); int precision, scale; if (exponent >= 0) { @@ -1101,6 +1114,8 @@ static NumericData build_numeric_data(PyObject* decimal_param) { precision = std::max(1, std::min(precision, MAX_NUMERIC_PRECISION)); scale = std::min(scale, precision); + // Step 6: build the mantissa with Python bigint math so we preserve every Decimal digit; a fixed + // C++ integer would overflow long before SQL Server's 38-digit NUMERIC limit. PyObject* py_ten = PyLong_FromLong(10); PyObject* int_val = PyLong_FromLong(0); guard.track(py_ten); @@ -1134,6 +1149,8 @@ static NumericData build_numeric_data(PyObject* decimal_param) { if (!int_val) throw py::error_already_set(); } + // Step 7: a positive Decimal exponent means the tuple digits omit trailing zeros, so Decimal("123e2") + // must become integer mantissa 12300 before packing. if (exponent > 0) { PyObject* multiplier = PyLong_FromLong(1); if (!multiplier) throw py::error_already_set(); @@ -1156,6 +1173,7 @@ static NumericData build_numeric_data(PyObject* decimal_param) { int_val = scaled_val; } + // Step 8: SQL_NUMERIC_STRUCT stores magnitude and sign separately, so encode only the absolute value. PyObject* abs_val = PyNumber_Absolute(int_val); guard.track(abs_val); guard.release(int_val); @@ -1168,6 +1186,8 @@ static NumericData build_numeric_data(PyObject* decimal_param) { Py_DECREF(as_tuple); if (!abs_val) throw py::error_already_set(); + // Step 9: SQL_NUMERIC_STRUCT::val is a 16-byte little-endian unsigned integer buffer, so ask + // Python's bigint to serialize directly into that wire format instead of reimplementing base conversion. PyObject* val_bytes = PyObject_CallMethod(abs_val, "to_bytes", "is", 16, "little"); guard.track(val_bytes); guard.release(abs_val); @@ -1180,6 +1200,8 @@ static NumericData build_numeric_data(PyObject* decimal_param) { throw py::error_already_set(); } + // Step 10: pack precision/scale plus the 16-byte little-endian magnitude. SQL uses sign=1 for + // positive and sign=0 for negative, which is the inverse of Decimal.as_tuple().sign. NumericData nd; nd.precision = static_cast(precision); nd.scale = static_cast(scale); @@ -1201,6 +1223,7 @@ static NumericData build_numeric_data(PyObject* decimal_param) { } template +// The callable hides whether the caller wraps SQLPutData with GIL management; chunk sizing stays shared. static SQLRETURN stream_dae_chunks(const void* data, size_t total_bytes, PutDataFn put_data_fn) { const char* bytes = static_cast(data); for (size_t offset = 0; offset < total_bytes; offset += DAE_CHUNK_SIZE) { From d164a540cb51d119107384ca03a9a7c781518622 Mon Sep 17 00:00:00 2001 From: Gaurav Sharma Date: Wed, 15 Jul 2026 13:31:28 +0530 Subject: [PATCH 23/29] REFACTOR: Replace PyObjGuard with PyPtr (Pattern 0 from perf proposal) Introduce py_ref.hpp: PyPtr = std::unique_ptr with adopt() and incref_borrow() helpers. Zero runtime overhead via empty-base optimisation. Replaces the bespoke PyObjGuard (fixed 8-slot array, manual track/release) with standard C++ RAII throughout DetectParamTypes and build_numeric_data. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- mssql_python/pybind/ddbc_bindings.cpp | 180 ++++++++------------------ mssql_python/pybind/py_ref.hpp | 26 ++++ 2 files changed, 77 insertions(+), 129 deletions(-) create mode 100644 mssql_python/pybind/py_ref.hpp diff --git a/mssql_python/pybind/ddbc_bindings.cpp b/mssql_python/pybind/ddbc_bindings.cpp index dc6a3869..466cf3bf 100644 --- a/mssql_python/pybind/ddbc_bindings.cpp +++ b/mssql_python/pybind/ddbc_bindings.cpp @@ -8,8 +8,13 @@ #include "connection/connection.h" #include "connection/connection_pool.h" #include "logger_bridge.hpp" +#include "py_ref.hpp" #include "utf_utils.h" +using pyref::PyPtr; +using pyref::adopt; +using pyref::incref_borrow; + #include // std::min #include @@ -107,38 +112,6 @@ PyObject* get_time_class(); py::object get_time_class_obj(); } -// RAII guard for automatic PyObject cleanup on scope exit. -// Tracks a small set of PyObject* refs and decrefs them in destructor. -class PyObjGuard { - PyObject* refs[8] = {}; - int count = 0; - -public: - void track(PyObject* obj) { - for (int i = 0; i < count; ++i) { - if (refs[i] == nullptr) { - refs[i] = obj; - return; - } - } - if (count < 8) refs[count++] = obj; - } - - // release() hands ownership back to a caller that decrefs immediately, avoiding double-decref in ~PyObjGuard(). - void release(PyObject* obj) { - for (int i = 0; i < count; ++i) { - if (refs[i] == obj) { - refs[i] = nullptr; - return; - } - } - } - - ~PyObjGuard() { - for (int i = count - 1; i >= 0; --i) Py_XDECREF(refs[i]); - } -}; - //------------------------------------------------------------------------------------------------- //------------------------------------------------------------------------------------------------- // Logging Infrastructure: @@ -927,33 +900,27 @@ std::vector DetectParamTypes(PyObject* params) { int is_decimal = PyObject_IsInstance(obj, decimal_type); if (is_decimal == -1) throw py::error_already_set(); if (is_decimal == 1) { - PyObject* as_tuple = PyObject_CallMethod(obj, "as_tuple", NULL); - if (!as_tuple) throw py::error_already_set(); - PyObjGuard guard; - guard.track(as_tuple); + PyPtr as_tuple_ptr = adopt(PyObject_CallMethod(obj, "as_tuple", NULL)); + if (!as_tuple_ptr) throw py::error_already_set(); - PyObject* exponent_obj = PyObject_GetAttrString(as_tuple, "exponent"); + PyPtr exponent_obj = adopt(PyObject_GetAttrString(as_tuple_ptr.get(), "exponent")); if (!exponent_obj) throw py::error_already_set(); - guard.track(exponent_obj); // NaN / Infinity / sNaN: refuse rather than silently writing 0. - if (PyUnicode_Check(exponent_obj)) { + if (PyUnicode_Check(exponent_obj.get())) { throw py::value_error( "Cannot bind non-finite Decimal (NaN/Infinity) as SQL NUMERIC"); } - PyObject* digits_obj = PyObject_GetAttrString(as_tuple, "digits"); + PyPtr digits_obj = adopt(PyObject_GetAttrString(as_tuple_ptr.get(), "digits")); if (!digits_obj) throw py::error_already_set(); - guard.track(digits_obj); - if (!PyTuple_Check(digits_obj)) { + if (!PyTuple_Check(digits_obj.get())) { throw py::type_error("Decimal.as_tuple().digits must be a tuple"); } - Py_ssize_t num_digits = PyTuple_GET_SIZE(digits_obj); - int exponent = static_cast(PyLong_AsLong(exponent_obj)); - guard.release(exponent_obj); - Py_DECREF(exponent_obj); + Py_ssize_t num_digits = PyTuple_GET_SIZE(digits_obj.get()); + int exponent = static_cast(PyLong_AsLong(exponent_obj.get())); if (exponent == -1 && PyErr_Occurred()) throw py::error_already_set(); int precision; @@ -997,17 +964,15 @@ std::vector DetectParamTypes(PyObject* params) { } if (in_money_range) { - PyObject* formatted = PyObject_CallMethod(obj, "__format__", "s", "f"); + PyPtr formatted = adopt(PyObject_CallMethod(obj, "__format__", "s", "f")); if (!formatted) throw py::error_already_set(); info.paramSQLType = SQL_VARCHAR; info.paramCType = PARAM_C_TYPE_TEXT; - info.columnSize = PyUnicode_GET_LENGTH(formatted); + info.columnSize = PyUnicode_GET_LENGTH(formatted.get()); info.decimalDigits = 0; - guard.release(digits_obj); - Py_DECREF(digits_obj); - guard.release(as_tuple); - Py_DECREF(as_tuple); - if (PyList_SetItem(params, i, formatted) != 0) { + PyObject* raw = formatted.release(); + if (PyList_SetItem(params, i, raw) != 0) { + Py_DECREF(raw); throw py::error_already_set(); } continue; @@ -1017,10 +982,6 @@ std::vector DetectParamTypes(PyObject* params) { // object in the param list so BindParameters can extract it as NumericData. info.paramSQLType = SQL_NUMERIC; info.paramCType = SQL_C_NUMERIC; - guard.release(digits_obj); - Py_DECREF(digits_obj); - guard.release(as_tuple); - Py_DECREF(as_tuple); NumericData nd = build_numeric_data(obj); info.columnSize = nd.precision; info.decimalDigits = nd.scale; @@ -1065,44 +1026,37 @@ std::vector DetectParamTypes(PyObject* params) { // the caller stores it as a Python capsule via PyList_SetItem for the binder to consume. static NumericData build_numeric_data(PyObject* decimal_param) { // Decimal.as_tuple() exposes the canonical pieces we need: sign, coefficient digits, and exponent. - PyObject* as_tuple = PyObject_CallMethod(decimal_param, "as_tuple", NULL); + PyPtr as_tuple = adopt(PyObject_CallMethod(decimal_param, "as_tuple", NULL)); if (!as_tuple) throw py::error_already_set(); - PyObjGuard guard; - guard.track(as_tuple); // Step 1-4: unpack Decimal(sign, digits, exponent) and validate that digits is the tuple form // promised by Decimal.as_tuple(); the remaining logic works purely from these normalized parts. - PyObject* digits_obj = PyObject_GetAttrString(as_tuple, "digits"); - if (!digits_obj) throw py::error_already_set(); - guard.track(digits_obj); - PyObject* sign_obj = PyObject_GetAttrString(as_tuple, "sign"); + PyPtr digits = adopt(PyObject_GetAttrString(as_tuple.get(), "digits")); + if (!digits) throw py::error_already_set(); + PyPtr sign_obj = adopt(PyObject_GetAttrString(as_tuple.get(), "sign")); if (!sign_obj) throw py::error_already_set(); - guard.track(sign_obj); - PyObject* exponent_obj = PyObject_GetAttrString(as_tuple, "exponent"); + PyPtr exponent_obj = adopt(PyObject_GetAttrString(as_tuple.get(), "exponent")); if (!exponent_obj) throw py::error_already_set(); - guard.track(exponent_obj); - int sign_val = static_cast(PyLong_AsLong(sign_obj)); - guard.release(sign_obj); - Py_DECREF(sign_obj); + int sign_val = static_cast(PyLong_AsLong(sign_obj.get())); + sign_obj.reset(); if (sign_val == -1 && PyErr_Occurred()) throw py::error_already_set(); int exponent = 0; - if (PyLong_Check(exponent_obj)) { - exponent = static_cast(PyLong_AsLong(exponent_obj)); + if (PyLong_Check(exponent_obj.get())) { + exponent = static_cast(PyLong_AsLong(exponent_obj.get())); if (exponent == -1 && PyErr_Occurred()) throw py::error_already_set(); } - guard.release(exponent_obj); - Py_DECREF(exponent_obj); + exponent_obj.reset(); - if (!PyTuple_Check(digits_obj)) { + if (!PyTuple_Check(digits.get())) { throw py::type_error("Decimal.as_tuple().digits must be a tuple"); } // Step 5: SQL Server precision counts all stored decimal digits, while scale is just the fractional // digits. A positive exponent means trailing zeros move into the integer part; a negative exponent // means scale = -exponent and precision must still cover leading fractional zeros such as 0.001. - int num_digits = static_cast(PyTuple_GET_SIZE(digits_obj)); + int num_digits = static_cast(PyTuple_GET_SIZE(digits.get())); int precision, scale; if (exponent >= 0) { precision = num_digits + exponent; @@ -1116,87 +1070,58 @@ static NumericData build_numeric_data(PyObject* decimal_param) { // Step 6: build the mantissa with Python bigint math so we preserve every Decimal digit; a fixed // C++ integer would overflow long before SQL Server's 38-digit NUMERIC limit. - PyObject* py_ten = PyLong_FromLong(10); - PyObject* int_val = PyLong_FromLong(0); - guard.track(py_ten); - guard.track(int_val); + PyPtr py_ten = adopt(PyLong_FromLong(10)); + PyPtr int_val = adopt(PyLong_FromLong(0)); if (!py_ten || !int_val) { throw py::error_already_set(); } - const Py_ssize_t digit_count = PyTuple_GET_SIZE(digits_obj); + const Py_ssize_t digit_count = PyTuple_GET_SIZE(digits.get()); for (Py_ssize_t i = 0; i < digit_count; ++i) { - PyObject* digit_obj = PyTuple_GET_ITEM(digits_obj, i); + PyObject* digit_obj = PyTuple_GET_ITEM(digits.get(), i); long digit = PyLong_AsLong(digit_obj); if (digit == -1 && PyErr_Occurred()) throw py::error_already_set(); - PyObject* multiplied = PyNumber_Multiply(int_val, py_ten); - guard.track(multiplied); - guard.release(int_val); - Py_DECREF(int_val); + PyPtr multiplied = adopt(PyNumber_Multiply(int_val.get(), py_ten.get())); if (!multiplied) throw py::error_already_set(); - PyObject* py_digit = PyLong_FromLong(digit); + PyPtr py_digit = adopt(PyLong_FromLong(digit)); if (!py_digit) throw py::error_already_set(); - guard.track(py_digit); - - int_val = PyNumber_Add(multiplied, py_digit); - guard.track(int_val); - guard.release(multiplied); - Py_DECREF(multiplied); - guard.release(py_digit); - Py_DECREF(py_digit); + + int_val = adopt(PyNumber_Add(multiplied.get(), py_digit.get())); if (!int_val) throw py::error_already_set(); } // Step 7: a positive Decimal exponent means the tuple digits omit trailing zeros, so Decimal("123e2") // must become integer mantissa 12300 before packing. if (exponent > 0) { - PyObject* multiplier = PyLong_FromLong(1); + PyPtr multiplier = adopt(PyLong_FromLong(1)); if (!multiplier) throw py::error_already_set(); - guard.track(multiplier); for (int j = 0; j < exponent; ++j) { - PyObject* next_multiplier = PyNumber_Multiply(multiplier, py_ten); - guard.track(next_multiplier); - guard.release(multiplier); - Py_DECREF(multiplier); - if (!next_multiplier) throw py::error_already_set(); - multiplier = next_multiplier; + multiplier = adopt(PyNumber_Multiply(multiplier.get(), py_ten.get())); + if (!multiplier) throw py::error_already_set(); } - PyObject* scaled_val = PyNumber_Multiply(int_val, multiplier); - guard.track(scaled_val); - guard.release(multiplier); - Py_DECREF(multiplier); - guard.release(int_val); - Py_DECREF(int_val); - if (!scaled_val) throw py::error_already_set(); - int_val = scaled_val; + int_val = adopt(PyNumber_Multiply(int_val.get(), multiplier.get())); + if (!int_val) throw py::error_already_set(); } // Step 8: SQL_NUMERIC_STRUCT stores magnitude and sign separately, so encode only the absolute value. - PyObject* abs_val = PyNumber_Absolute(int_val); - guard.track(abs_val); - guard.release(int_val); - Py_DECREF(int_val); - guard.release(py_ten); - Py_DECREF(py_ten); - guard.release(digits_obj); - Py_DECREF(digits_obj); - guard.release(as_tuple); - Py_DECREF(as_tuple); + PyPtr abs_val = adopt(PyNumber_Absolute(int_val.get())); + int_val.reset(); + py_ten.reset(); + digits.reset(); + as_tuple.reset(); if (!abs_val) throw py::error_already_set(); // Step 9: SQL_NUMERIC_STRUCT::val is a 16-byte little-endian unsigned integer buffer, so ask // Python's bigint to serialize directly into that wire format instead of reimplementing base conversion. - PyObject* val_bytes = PyObject_CallMethod(abs_val, "to_bytes", "is", 16, "little"); - guard.track(val_bytes); - guard.release(abs_val); - Py_DECREF(abs_val); + PyPtr val_bytes = adopt(PyObject_CallMethod(abs_val.get(), "to_bytes", "is", 16, "little")); + abs_val.reset(); if (!val_bytes) throw py::error_already_set(); char* val_buf = nullptr; Py_ssize_t val_size = 0; - if (PyBytes_AsStringAndSize(val_bytes, &val_buf, &val_size) == -1) { + if (PyBytes_AsStringAndSize(val_bytes.get(), &val_buf, &val_size) == -1) { throw py::error_already_set(); } @@ -1216,9 +1141,6 @@ static NumericData build_numeric_data(PyObject* decimal_param) { std::memcpy(&nd.val[0], val_buf, copy_len); // DevSkim: ignore DS121708 #endif } - guard.release(val_bytes); - Py_DECREF(val_bytes); - return nd; } diff --git a/mssql_python/pybind/py_ref.hpp b/mssql_python/pybind/py_ref.hpp new file mode 100644 index 00000000..852221da --- /dev/null +++ b/mssql_python/pybind/py_ref.hpp @@ -0,0 +1,26 @@ +// py_ref.hpp — RAII wrapper for CPython refcount management (Pattern 0). +// +// PyPtr = std::unique_ptr +// +// Zero runtime overhead (stateless functor → empty-base optimisation). +// Use adopt() for new references, incref_borrow() for borrowed ones. + +#pragma once +#include +#include + +namespace pyref { + +struct PyDecRefDeleter { + void operator()(PyObject* p) const noexcept { Py_XDECREF(p); } +}; + +using PyPtr = std::unique_ptr; + +// Wrap a new reference (already +1 from the API that returned it). +inline PyPtr adopt(PyObject* p) noexcept { return PyPtr{p}; } + +// Extend a borrowed reference's lifetime past its borrower. +inline PyPtr incref_borrow(PyObject* p) noexcept { Py_XINCREF(p); return PyPtr{p}; } + +} // namespace pyref From b5054c03a8a11b10dc000b9b7208b4b1726a563d Mon Sep 17 00:00:00 2001 From: Gaurav Sharma Date: Fri, 17 Jul 2026 14:22:15 +0530 Subject: [PATCH 24/29] REFACTOR: Extract PythonObjectCache to header, extend PyPtr, fix double-free bugs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Extract PythonObjectCache namespace to python_object_cache.hpp - Extend PyPtr usage to Groups A/B/D/E (22→7 manual decrefs) - Fix 3 double-free bugs in PyList_SetItem error paths - Document ParamInfo.dataPtr manual refcounting rationale Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- mssql_python/pybind/ddbc_bindings.cpp | 179 ++------------------ mssql_python/pybind/python_object_cache.hpp | 111 ++++++++++++ 2 files changed, 121 insertions(+), 169 deletions(-) create mode 100644 mssql_python/pybind/python_object_cache.hpp diff --git a/mssql_python/pybind/ddbc_bindings.cpp b/mssql_python/pybind/ddbc_bindings.cpp index 466cf3bf..c943c1f5 100644 --- a/mssql_python/pybind/ddbc_bindings.cpp +++ b/mssql_python/pybind/ddbc_bindings.cpp @@ -9,6 +9,7 @@ #include "connection/connection_pool.h" #include "logger_bridge.hpp" #include "py_ref.hpp" +#include "python_object_cache.hpp" #include "utf_utils.h" using pyref::PyPtr; @@ -107,10 +108,6 @@ inline int EffectiveCharCtypeForFetch(int charCtype, const std::string& charEnco return charCtype; } -namespace PythonObjectCache { -PyObject* get_time_class(); -py::object get_time_class_obj(); -} //------------------------------------------------------------------------------------------------- //------------------------------------------------------------------------------------------------- @@ -120,164 +117,6 @@ py::object get_time_class_obj(); // Uses printf-style formatting: LOG("Value: %d", x) -- __FILE__/__LINE__ // embedded in macro //------------------------------------------------------------------------------------------------- -namespace PythonObjectCache { -// Cached as raw PyObject* to avoid pybind11 refcount wrapper overhead on every access. -// These are immortal (never DECREFed) — module-lifetime singletons, same as CPython's type objects. -static PyObject* datetime_class = nullptr; -static PyObject* date_class = nullptr; -static PyObject* time_class = nullptr; -static PyObject* decimal_class = nullptr; -static PyObject* uuid_class = nullptr; -static PyObject* money_min = nullptr; -static PyObject* money_max = nullptr; -static PyObject* smallmoney_min = nullptr; -static PyObject* smallmoney_max = nullptr; -static bool cache_initialized = false; - -static PyObject* import_attr(const char* module_name, const char* attr_name) { - PyObject* mod = PyImport_ImportModule(module_name); - if (!mod) throw py::error_already_set(); - PyObject* attr = PyObject_GetAttrString(mod, attr_name); - Py_DECREF(mod); - if (!attr) throw py::error_already_set(); - return attr; -} - -// Fall back to import here because legacy paths can reach these lookups before initialize_cache() has populated globals. -static PyObject* get_cached_class(PyObject* cached, const char* module_name, const char* attr_name) { - if (cache_initialized && cached) return cached; - PyObject* mod = PyImport_ImportModule(module_name); - if (!mod) return nullptr; - PyObject* cls = PyObject_GetAttrString(mod, attr_name); - Py_DECREF(mod); - return cls; -} - -// One-time cache of Python type objects and MONEY boundary constants. -// Called on first execute(). Uses raw CPython API (not pybind11) because -// these cached PyObject* are compared via PyObject_IsInstance in the -// hot DetectParamTypes loop — wrapping them in py::object would add -// unnecessary ref-count traffic on every parameter. -void initialize() { - if (!cache_initialized) { - PyDateTime_IMPORT; - if (PyDateTimeAPI == nullptr) { - throw py::error_already_set(); - } - - try { - // Cache datetime.datetime, datetime.date, datetime.time for isinstance - // checks in DetectParamTypes. Single import, then borrowed refs held - // for process lifetime (module-level statics, never decremented). - PyObject* datetime_module = PyImport_ImportModule("datetime"); - if (!datetime_module) throw py::error_already_set(); - datetime_class = PyObject_GetAttrString(datetime_module, "datetime"); - date_class = PyObject_GetAttrString(datetime_module, "date"); - time_class = PyObject_GetAttrString(datetime_module, "time"); - Py_DECREF(datetime_module); - if (!datetime_class || !date_class || !time_class) { - throw py::error_already_set(); - } - - decimal_class = import_attr("decimal", "Decimal"); - uuid_class = import_attr("uuid", "UUID"); - - // Pre-compute MONEY/SMALLMONEY boundary Decimals once. DetectParamTypes - // uses PyObject_RichCompareBool against these to classify Decimal params - // into MONEY vs NUMERIC — doing exact Decimal comparison (not float cast) - // avoids boundary misclassification at the edges. - PyObject* Decimal = decimal_class; - smallmoney_min = PyObject_CallFunction(Decimal, "s", "-214748.3648"); - smallmoney_max = PyObject_CallFunction(Decimal, "s", "214748.3647"); - money_min = PyObject_CallFunction(Decimal, "s", "-922337203685477.5808"); - money_max = PyObject_CallFunction(Decimal, "s", "922337203685477.5807"); - if (!smallmoney_min || !smallmoney_max || !money_min || !money_max) { - throw py::error_already_set(); - } - - cache_initialized = true; - } catch (...) { - Py_XDECREF(datetime_class); - Py_XDECREF(date_class); - Py_XDECREF(time_class); - Py_XDECREF(decimal_class); - Py_XDECREF(uuid_class); - Py_XDECREF(smallmoney_min); - Py_XDECREF(smallmoney_max); - Py_XDECREF(money_min); - Py_XDECREF(money_max); - datetime_class = nullptr; - date_class = nullptr; - time_class = nullptr; - decimal_class = nullptr; - uuid_class = nullptr; - smallmoney_min = nullptr; - smallmoney_max = nullptr; - money_min = nullptr; - money_max = nullptr; - throw; - } - } -} - -static py::object wrap_cached_or_imported(PyObject* obj) { - if (!obj) throw py::error_already_set(); - // Cached path returns a borrowed module-lifetime singleton. Fallback import path - // returns a fresh reference, so steal it to avoid leaking the rare edge-case object. - return cache_initialized ? py::reinterpret_borrow(py::handle(obj)) - : py::reinterpret_steal(obj); -} - -// Returns cached type. Fallback import only for edge case where cache wasn't initialized -// (e.g., called from legacy path before any fast-path execute). -PyObject* get_datetime_class() { - return get_cached_class(datetime_class, "datetime", "datetime"); -} - -py::object get_datetime_class_obj() { - return wrap_cached_or_imported(get_datetime_class()); -} - -// Returns cached type. Fallback import only for edge case where cache wasn't initialized -// (e.g., called from legacy path before any fast-path execute). -PyObject* get_date_class() { - return get_cached_class(date_class, "datetime", "date"); -} - -py::object get_date_class_obj() { - return wrap_cached_or_imported(get_date_class()); -} - -// Returns cached type. Fallback import only for edge case where cache wasn't initialized -// (e.g., called from legacy path before any fast-path execute). -PyObject* get_time_class() { - return get_cached_class(time_class, "datetime", "time"); -} - -py::object get_time_class_obj() { - return wrap_cached_or_imported(get_time_class()); -} - -// Returns cached type. Fallback import only for edge case where cache wasn't initialized -// (e.g., called from legacy path before any fast-path execute). -PyObject* get_decimal_class() { - return get_cached_class(decimal_class, "decimal", "Decimal"); -} - -py::object get_decimal_class_obj() { - return wrap_cached_or_imported(get_decimal_class()); -} - -// Returns cached type. Fallback import only for edge case where cache wasn't initialized -// (e.g., called from legacy path before any fast-path execute). -PyObject* get_uuid_class() { - return get_cached_class(uuid_class, "uuid", "UUID"); -} - -py::object get_uuid_class_obj() { - return wrap_cached_or_imported(get_uuid_class()); -} -} // namespace PythonObjectCache //------------------------------------------------------------------------------------------------- // Class definitions @@ -301,7 +140,9 @@ struct ParamInfo { SQLLEN strLenOrInd = 0; // Required for DAE bool isDAE = false; // Indicates if we need to stream // Holds a strong reference to the Python object for DAE (data-at-execution) streaming. - // Raw pointer + manual Py_INCREF/DECREF avoids pybind11 wrapper overhead in the hot loop. + // Raw pointer + manual Py_INCREF/DECREF (not PyPtr) because ParamInfo has custom + // copy/move semantics and is exposed to pybind11 type_caster — changing the member + // type would ripple through struct layout, copy/move operators, and property bindings. PyObject* dataPtr = nullptr; Py_ssize_t utf16Len = 0; // UTF-16 code unit count for string params @@ -843,10 +684,9 @@ std::vector DetectParamTypes(PyObject* params) { // --- datetime (must check before date, since datetime is subclass of date) --- if (PyDateTime_Check(obj)) { - PyObject* tzinfo = PyObject_GetAttrString(obj, "tzinfo"); + PyPtr tzinfo = adopt(PyObject_GetAttrString(obj, "tzinfo")); if (!tzinfo) throw py::error_already_set(); - bool has_tz = (tzinfo != Py_None); - Py_DECREF(tzinfo); + bool has_tz = (tzinfo.get() != Py_None); if (has_tz) { info.paramSQLType = SQL_SS_TIMESTAMPOFFSET; info.paramCType = SQL_C_SS_TIMESTAMPOFFSET; @@ -972,7 +812,8 @@ std::vector DetectParamTypes(PyObject* params) { info.decimalDigits = 0; PyObject* raw = formatted.release(); if (PyList_SetItem(params, i, raw) != 0) { - Py_DECREF(raw); + // PyList_SetItem steals (decrefs) the item even on failure, + // so raw is already freed — do NOT Py_DECREF here. throw py::error_already_set(); } continue; @@ -989,7 +830,7 @@ std::vector DetectParamTypes(PyObject* params) { py::object numeric_obj = py::cast(nd); PyObject* raw = numeric_obj.release().ptr(); if (PyList_SetItem(params, i, raw) != 0) { - Py_DECREF(raw); + // PyList_SetItem steals (decrefs) the item even on failure. throw py::error_already_set(); } continue; @@ -1006,7 +847,7 @@ std::vector DetectParamTypes(PyObject* params) { info.columnSize = 16; info.decimalDigits = 0; if (PyList_SetItem(params, i, bytes_le) != 0) { - Py_DECREF(bytes_le); + // PyList_SetItem steals (decrefs) the item even on failure. throw py::error_already_set(); } continue; diff --git a/mssql_python/pybind/python_object_cache.hpp b/mssql_python/pybind/python_object_cache.hpp new file mode 100644 index 00000000..1ec94c97 --- /dev/null +++ b/mssql_python/pybind/python_object_cache.hpp @@ -0,0 +1,111 @@ +// python_object_cache.hpp — One-time cache of Python type objects and MONEY boundary constants. +// +// Called on first execute(). Uses raw CPython API (not pybind11) because +// these cached PyObject* are compared via PyObject_IsInstance in the +// hot DetectParamTypes loop — wrapping them in py::object would add +// unnecessary ref-count traffic on every parameter. +// +// All cached pointers are module-lifetime singletons (never DECREFed). + +#pragma once +#include +#include +#include +#include "py_ref.hpp" + +namespace py = pybind11; +using pyref::PyPtr; +using pyref::adopt; + +namespace PythonObjectCache { + +// Module-lifetime singletons — never DECREFed, alive for the process. +inline PyObject* datetime_class = nullptr; +inline PyObject* date_class = nullptr; +inline PyObject* time_class = nullptr; +inline PyObject* decimal_class = nullptr; +inline PyObject* uuid_class = nullptr; +inline PyObject* money_min = nullptr; +inline PyObject* money_max = nullptr; +inline PyObject* smallmoney_min = nullptr; +inline PyObject* smallmoney_max = nullptr; +inline bool cache_initialized = false; + +// Import a module and extract an attribute. Returns a new reference. +inline PyObject* import_attr(const char* module_name, const char* attr_name) { + PyPtr mod = adopt(PyImport_ImportModule(module_name)); + if (!mod) throw py::error_already_set(); + PyObject* attr = PyObject_GetAttrString(mod.get(), attr_name); + if (!attr) throw py::error_already_set(); + return attr; +} + +// Return cached type, falling back to import for legacy paths that call +// before initialize() has run. +inline PyObject* get_cached_class(PyObject* cached, const char* module_name, const char* attr_name) { + if (cache_initialized && cached) return cached; + PyPtr mod = adopt(PyImport_ImportModule(module_name)); + if (!mod) return nullptr; + return PyObject_GetAttrString(mod.get(), attr_name); +} + +// One-time init. Uses local PyPtrs so exception cleanup is automatic; +// only .release() into globals after ALL acquisitions succeed. +inline void initialize() { + if (cache_initialized) return; + + PyDateTime_IMPORT; + if (PyDateTimeAPI == nullptr) throw py::error_already_set(); + + PyPtr dt_mod = adopt(PyImport_ImportModule("datetime")); + if (!dt_mod) throw py::error_already_set(); + + PyPtr dt_cls = adopt(PyObject_GetAttrString(dt_mod.get(), "datetime")); + PyPtr date_cls = adopt(PyObject_GetAttrString(dt_mod.get(), "date")); + PyPtr time_cls = adopt(PyObject_GetAttrString(dt_mod.get(), "time")); + if (!dt_cls || !date_cls || !time_cls) throw py::error_already_set(); + + PyPtr dec_cls = adopt(import_attr("decimal", "Decimal")); + PyPtr uuid_cls = adopt(import_attr("uuid", "UUID")); + + // Pre-compute MONEY/SMALLMONEY boundary Decimals for exact comparison + // in DetectParamTypes (avoids double-precision boundary errors). + PyPtr sm_min = adopt(PyObject_CallFunction(dec_cls.get(), "s", "-214748.3648")); + PyPtr sm_max = adopt(PyObject_CallFunction(dec_cls.get(), "s", "214748.3647")); + PyPtr m_min = adopt(PyObject_CallFunction(dec_cls.get(), "s", "-922337203685477.5808")); + PyPtr m_max = adopt(PyObject_CallFunction(dec_cls.get(), "s", "922337203685477.5807")); + if (!sm_min || !sm_max || !m_min || !m_max) throw py::error_already_set(); + + // Commit to globals — all acquisitions succeeded. + datetime_class = dt_cls.release(); + date_class = date_cls.release(); + time_class = time_cls.release(); + decimal_class = dec_cls.release(); + uuid_class = uuid_cls.release(); + smallmoney_min = sm_min.release(); + smallmoney_max = sm_max.release(); + money_min = m_min.release(); + money_max = m_max.release(); + cache_initialized = true; +} + +// Wrap a cached pointer as py::object (borrow for cached, steal for fallback import). +inline py::object wrap_cached_or_imported(PyObject* obj) { + if (!obj) throw py::error_already_set(); + return cache_initialized ? py::reinterpret_borrow(py::handle(obj)) + : py::reinterpret_steal(obj); +} + +inline PyObject* get_datetime_class() { return get_cached_class(datetime_class, "datetime", "datetime"); } +inline PyObject* get_date_class() { return get_cached_class(date_class, "datetime", "date"); } +inline PyObject* get_time_class() { return get_cached_class(time_class, "datetime", "time"); } +inline PyObject* get_decimal_class() { return get_cached_class(decimal_class, "decimal", "Decimal"); } +inline PyObject* get_uuid_class() { return get_cached_class(uuid_class, "uuid", "UUID"); } + +inline py::object get_datetime_class_obj() { return wrap_cached_or_imported(get_datetime_class()); } +inline py::object get_date_class_obj() { return wrap_cached_or_imported(get_date_class()); } +inline py::object get_time_class_obj() { return wrap_cached_or_imported(get_time_class()); } +inline py::object get_decimal_class_obj() { return wrap_cached_or_imported(get_decimal_class()); } +inline py::object get_uuid_class_obj() { return wrap_cached_or_imported(get_uuid_class()); } + +} // namespace PythonObjectCache From f74148f917a65082c5d94aae5eea8def9cf67f10 Mon Sep 17 00:00:00 2001 From: Gaurav Sharma Date: Fri, 17 Jul 2026 14:32:15 +0530 Subject: [PATCH 25/29] =?UTF-8?q?REFACTOR:=20Rename=20PythonObjectCache=20?= =?UTF-8?q?=E2=86=92=20PyTypeCache,=20remove=20unused=20imports?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Rename python_object_cache.hpp → py_type_cache.hpp - Rename namespace PythonObjectCache → PyTypeCache - Remove unused: incref_borrow using, , Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- mssql_python/pybind/ddbc_bindings.cpp | 57 +++++++++---------- ...hon_object_cache.hpp => py_type_cache.hpp} | 6 +- 2 files changed, 30 insertions(+), 33 deletions(-) rename mssql_python/pybind/{python_object_cache.hpp => py_type_cache.hpp} (96%) diff --git a/mssql_python/pybind/ddbc_bindings.cpp b/mssql_python/pybind/ddbc_bindings.cpp index c943c1f5..362f4650 100644 --- a/mssql_python/pybind/ddbc_bindings.cpp +++ b/mssql_python/pybind/ddbc_bindings.cpp @@ -9,20 +9,17 @@ #include "connection/connection_pool.h" #include "logger_bridge.hpp" #include "py_ref.hpp" -#include "python_object_cache.hpp" +#include "py_type_cache.hpp" #include "utf_utils.h" using pyref::PyPtr; using pyref::adopt; -using pyref::incref_borrow; #include // std::min -#include #include #include // For std::memcpy #include -#include // std::setw, std::setfill #include #include // std::forward #include // CPython datetime API (PyDateTime_IMPORT, PyDateTime_GET_*, etc.) @@ -526,13 +523,13 @@ static NumericData build_numeric_data(PyObject* decimal_param); // Takes a raw PyObject* (must be a list). Caller guarantees it's a fresh copy // (cursor.py does list(actual_params)), so in-place mutation via PyList_SetItem is safe. std::vector DetectParamTypes(PyObject* params) { - PythonObjectCache::initialize(); + PyTypeCache::initialize(); const Py_ssize_t n = PyList_GET_SIZE(params); std::vector infos(n); - PyObject* decimal_type = PythonObjectCache::get_decimal_class(); - PyObject* uuid_type = PythonObjectCache::get_uuid_class(); + PyObject* decimal_type = PyTypeCache::get_decimal_class(); + PyObject* uuid_type = PyTypeCache::get_uuid_class(); for (Py_ssize_t i = 0; i < n; ++i) { ParamInfo& info = infos[i]; @@ -789,14 +786,14 @@ std::vector DetectParamTypes(PyObject* params) { // represent the exact money range without precision loss on certain ODBC drivers. // Use exact Decimal comparison (not double) to avoid boundary misclassification. bool in_money_range = false; - int cmp_ge = PyObject_RichCompareBool(obj, PythonObjectCache::smallmoney_min, Py_GE); - int cmp_le = PyObject_RichCompareBool(obj, PythonObjectCache::smallmoney_max, Py_LE); + int cmp_ge = PyObject_RichCompareBool(obj, PyTypeCache::smallmoney_min, Py_GE); + int cmp_le = PyObject_RichCompareBool(obj, PyTypeCache::smallmoney_max, Py_LE); if (cmp_ge == -1 || cmp_le == -1) throw py::error_already_set(); if (cmp_ge == 1 && cmp_le == 1) { in_money_range = true; } else { - cmp_ge = PyObject_RichCompareBool(obj, PythonObjectCache::money_min, Py_GE); - cmp_le = PyObject_RichCompareBool(obj, PythonObjectCache::money_max, Py_LE); + cmp_ge = PyObject_RichCompareBool(obj, PyTypeCache::money_min, Py_GE); + cmp_le = PyObject_RichCompareBool(obj, PyTypeCache::money_max, Py_LE); if (cmp_ge == -1 || cmp_le == -1) throw py::error_already_set(); if (cmp_ge == 1 && cmp_le == 1) { in_money_range = true; @@ -1349,7 +1346,7 @@ SQLRETURN BindParameters(SqlHandle& handle, SQLHANDLE hStmt, const py::list& par break; } case SQL_C_TYPE_DATE: { - py::object dateType = PythonObjectCache::get_date_class_obj(); + py::object dateType = PyTypeCache::get_date_class_obj(); if (!py::isinstance(param, dateType)) { ThrowStdException(MakeParamMismatchErrorStr(paramInfo.paramCType, paramIndex)); } @@ -1369,7 +1366,7 @@ SQLRETURN BindParameters(SqlHandle& handle, SQLHANDLE hStmt, const py::list& par break; } case SQL_C_TYPE_TIME: { - py::object timeType = PythonObjectCache::get_time_class_obj(); + py::object timeType = PyTypeCache::get_time_class_obj(); if (!py::isinstance(param, timeType)) { ThrowStdException(MakeParamMismatchErrorStr(paramInfo.paramCType, paramIndex)); } @@ -1383,7 +1380,7 @@ SQLRETURN BindParameters(SqlHandle& handle, SQLHANDLE hStmt, const py::list& par break; } case SQL_C_SS_TIMESTAMPOFFSET: { - py::object datetimeType = PythonObjectCache::get_datetime_class_obj(); + py::object datetimeType = PyTypeCache::get_datetime_class_obj(); if (!py::isinstance(param, datetimeType)) { ThrowStdException(MakeParamMismatchErrorStr(paramInfo.paramCType, paramIndex)); } @@ -1435,7 +1432,7 @@ SQLRETURN BindParameters(SqlHandle& handle, SQLHANDLE hStmt, const py::list& par break; } case SQL_C_TYPE_TIMESTAMP: { - py::object datetimeType = PythonObjectCache::get_datetime_class_obj(); + py::object datetimeType = PyTypeCache::get_datetime_class_obj(); if (!py::isinstance(param, datetimeType)) { ThrowStdException(MakeParamMismatchErrorStr(paramInfo.paramCType, paramIndex)); } @@ -3151,7 +3148,7 @@ SQLRETURN BindParameterArray(SqlHandle& handle, SQLHANDLE hStmt, const py::list& AllocateParamBufferArray(tempBuffers, paramSetSize); strLenOrIndArray = AllocateParamBufferArray(tempBuffers, paramSetSize); - py::object datetimeType = PythonObjectCache::get_datetime_class_obj(); + py::object datetimeType = PyTypeCache::get_datetime_class_obj(); for (size_t i = 0; i < paramSetSize; ++i) { const py::handle& param = columnValues[i]; @@ -3266,7 +3263,7 @@ SQLRETURN BindParameterArray(SqlHandle& handle, SQLHANDLE hStmt, const py::list& // Get cached UUID class from module-level helper // This avoids static object destruction issues during // Python finalization - py::object uuid_class = PythonObjectCache::get_uuid_class_obj(); + py::object uuid_class = PyTypeCache::get_uuid_class_obj(); // Get cached UUID class for (size_t i = 0; i < paramSetSize; ++i) { @@ -4233,7 +4230,7 @@ SQLRETURN SQLGetData_wrap(SqlHandlePtr StatementHandle, SQLUSMALLINT colCount, p // parsing The decimal separator only affects display // formatting, not parsing py::object decimalObj = - PythonObjectCache::get_decimal_class_obj()(py::str(cnum, safeLen)); + PyTypeCache::get_decimal_class_obj()(py::str(cnum, safeLen)); row.append(decimalObj); } catch (const py::error_already_set& e) { // If conversion fails, append None @@ -4283,7 +4280,7 @@ SQLRETURN SQLGetData_wrap(SqlHandlePtr StatementHandle, SQLUSMALLINT colCount, p ret = SQLGetData_ptr(hStmt, i, SQL_C_TYPE_DATE, &dateValue, sizeof(dateValue), NULL); if (SQL_SUCCEEDED(ret)) { - row.append(PythonObjectCache::get_date_class_obj()(dateValue.year, dateValue.month, + row.append(PyTypeCache::get_date_class_obj()(dateValue.year, dateValue.month, dateValue.day)); } else { row.append(py::none()); @@ -4296,7 +4293,7 @@ SQLRETURN SQLGetData_wrap(SqlHandlePtr StatementHandle, SQLUSMALLINT colCount, p SQLLEN indicator = 0; ret = SQLGetData_ptr(hStmt, i, SQL_C_SS_TIME2, &t2, sizeof(t2), &indicator); if (SQL_SUCCEEDED(ret) && indicator != SQL_NULL_DATA) { - row.append(PythonObjectCache::get_time_class_obj()( + row.append(PyTypeCache::get_time_class_obj()( t2.hour, t2.minute, t2.second, t2.fraction / 1000)); // ns to µs } else { if (!SQL_SUCCEEDED(ret)) { @@ -4315,7 +4312,7 @@ SQLRETURN SQLGetData_wrap(SqlHandlePtr StatementHandle, SQLUSMALLINT colCount, p ret = SQLGetData_ptr(hStmt, i, SQL_C_TYPE_TIMESTAMP, ×tampValue, sizeof(timestampValue), NULL); if (SQL_SUCCEEDED(ret)) { - row.append(PythonObjectCache::get_datetime_class_obj()( + row.append(PyTypeCache::get_datetime_class_obj()( timestampValue.year, timestampValue.month, timestampValue.day, timestampValue.hour, timestampValue.minute, timestampValue.second, timestampValue.fraction / 1000 // Convert back ns to µs @@ -4355,7 +4352,7 @@ SQLRETURN SQLGetData_wrap(SqlHandlePtr StatementHandle, SQLUSMALLINT colCount, p py::object datetime_module = py::module_::import("datetime"); py::object tzinfo = datetime_module.attr("timezone")( datetime_module.attr("timedelta")(py::arg("minutes") = totalMinutes)); - py::object py_dt = PythonObjectCache::get_datetime_class_obj()( + py::object py_dt = PyTypeCache::get_datetime_class_obj()( dtoValue.year, dtoValue.month, dtoValue.day, dtoValue.hour, dtoValue.minute, dtoValue.second, microseconds, tzinfo); row.append(py_dt); @@ -4462,7 +4459,7 @@ SQLRETURN SQLGetData_wrap(SqlHandlePtr StatementHandle, SQLUSMALLINT colCount, p py::bytes py_guid_bytes(guid_bytes.data(), guid_bytes.size()); py::object uuid_obj = - PythonObjectCache::get_uuid_class_obj()(py::arg("bytes") = py_guid_bytes); + PyTypeCache::get_uuid_class_obj()(py::arg("bytes") = py_guid_bytes); row.append(uuid_obj); } else if (indicator == SQL_NULL_DATA) { row.append(py::none()); @@ -4925,7 +4922,7 @@ SQLRETURN FetchBatchData(SQLHSTMT hStmt, ColumnBuffers& buffers, py::list& colum // parsing The decimal separator only affects display // formatting, not parsing PyObject* decimalObj = - PythonObjectCache::get_decimal_class_obj()(py::str(rawData, decimalDataLen)) + PyTypeCache::get_decimal_class_obj()(py::str(rawData, decimalDataLen)) .release() .ptr(); PyList_SET_ITEM(row, col - 1, decimalObj); @@ -4942,7 +4939,7 @@ SQLRETURN FetchBatchData(SQLHSTMT hStmt, ColumnBuffers& buffers, py::list& colum case SQL_TYPE_TIMESTAMP: case SQL_DATETIME: { const SQL_TIMESTAMP_STRUCT& ts = buffers.timestampBuffers[col - 1][i]; - PyObject* datetimeObj = PythonObjectCache::get_datetime_class_obj()( + PyObject* datetimeObj = PyTypeCache::get_datetime_class_obj()( ts.year, ts.month, ts.day, ts.hour, ts.minute, ts.second, ts.fraction / 1000) .release() @@ -4952,7 +4949,7 @@ SQLRETURN FetchBatchData(SQLHSTMT hStmt, ColumnBuffers& buffers, py::list& colum } case SQL_TYPE_DATE: { PyObject* dateObj = - PythonObjectCache::get_date_class_obj()(buffers.dateBuffers[col - 1][i].year, + PyTypeCache::get_date_class_obj()(buffers.dateBuffers[col - 1][i].year, buffers.dateBuffers[col - 1][i].month, buffers.dateBuffers[col - 1][i].day) .release() @@ -4963,7 +4960,7 @@ SQLRETURN FetchBatchData(SQLHSTMT hStmt, ColumnBuffers& buffers, py::list& colum case SQL_SS_TIME2: { const SQL_SS_TIME2_STRUCT& t2 = buffers.timeBuffers[col - 1][i]; PyObject* timeObj = - PythonObjectCache::get_time_class_obj()(t2.hour, t2.minute, t2.second, + PyTypeCache::get_time_class_obj()(t2.hour, t2.minute, t2.second, t2.fraction / 1000) // ns to µs .release() .ptr(); @@ -4979,7 +4976,7 @@ SQLRETURN FetchBatchData(SQLHSTMT hStmt, ColumnBuffers& buffers, py::list& colum py::object datetime_module = py::module_::import("datetime"); py::object tzinfo = datetime_module.attr("timezone")( datetime_module.attr("timedelta")(py::arg("minutes") = totalMinutes)); - py::object py_dt = PythonObjectCache::get_datetime_class_obj()( + py::object py_dt = PyTypeCache::get_datetime_class_obj()( dtoValue.year, dtoValue.month, dtoValue.day, dtoValue.hour, dtoValue.minute, dtoValue.second, dtoValue.fraction / 1000, // ns → µs @@ -5013,7 +5010,7 @@ SQLRETURN FetchBatchData(SQLHSTMT hStmt, ColumnBuffers& buffers, py::list& colum py::bytes py_guid_bytes(reinterpret_cast(reordered), 16); py::dict kwargs; kwargs["bytes"] = py_guid_bytes; - py::object uuid_obj = PythonObjectCache::get_uuid_class_obj()(**kwargs); + py::object uuid_obj = PyTypeCache::get_uuid_class_obj()(**kwargs); PyList_SET_ITEM(row, col - 1, uuid_obj.release().ptr()); break; } @@ -6537,7 +6534,7 @@ void DDBCSetDecimalSeparator(const std::string& separator) { PYBIND11_MODULE(ddbc_bindings, m) { m.doc() = "msodbcsql driver api bindings for Python"; - PythonObjectCache::initialize(); + PyTypeCache::initialize(); // Add architecture information as module attribute m.attr("__architecture__") = ARCHITECTURE; diff --git a/mssql_python/pybind/python_object_cache.hpp b/mssql_python/pybind/py_type_cache.hpp similarity index 96% rename from mssql_python/pybind/python_object_cache.hpp rename to mssql_python/pybind/py_type_cache.hpp index 1ec94c97..78ed2acf 100644 --- a/mssql_python/pybind/python_object_cache.hpp +++ b/mssql_python/pybind/py_type_cache.hpp @@ -1,4 +1,4 @@ -// python_object_cache.hpp — One-time cache of Python type objects and MONEY boundary constants. +// py_type_cache.hpp — One-time cache of Python type objects and MONEY boundary constants. // // Called on first execute(). Uses raw CPython API (not pybind11) because // these cached PyObject* are compared via PyObject_IsInstance in the @@ -17,7 +17,7 @@ namespace py = pybind11; using pyref::PyPtr; using pyref::adopt; -namespace PythonObjectCache { +namespace PyTypeCache { // Module-lifetime singletons — never DECREFed, alive for the process. inline PyObject* datetime_class = nullptr; @@ -108,4 +108,4 @@ inline py::object get_time_class_obj() { return wrap_cached_or_imported(get_ inline py::object get_decimal_class_obj() { return wrap_cached_or_imported(get_decimal_class()); } inline py::object get_uuid_class_obj() { return wrap_cached_or_imported(get_uuid_class()); } -} // namespace PythonObjectCache +} // namespace PyTypeCache From 527be8fb182fb9365a7ec84282f8f0ce5e953558 Mon Sep 17 00:00:00 2001 From: Gaurav Sharma Date: Thu, 30 Jul 2026 13:15:31 +0530 Subject: [PATCH 26/29] REFACTOR: use pybind11's py::object for refcount RAII instead of custom PyPtr drops py_ref.hpp (PyPtr = unique_ptr) and uses pybind11's own RAII handle via a steal() shorthand. same ownership semantics, no behavior change: in release builds (-O3 -DNDEBUG, which is what we ship) py::object::dec_ref compiles to a bare Py_XDECREF, identical to the PyPtr deleter. the raw CPython calls in the hot detection path are untouched, only the refcount wrapper changes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- mssql_python/pybind/ddbc_bindings.cpp | 89 +++++++++++++-------------- mssql_python/pybind/py_ref.hpp | 26 -------- mssql_python/pybind/py_type_cache.hpp | 54 ++++++++-------- 3 files changed, 69 insertions(+), 100 deletions(-) delete mode 100644 mssql_python/pybind/py_ref.hpp diff --git a/mssql_python/pybind/ddbc_bindings.cpp b/mssql_python/pybind/ddbc_bindings.cpp index 362f4650..00035c01 100644 --- a/mssql_python/pybind/ddbc_bindings.cpp +++ b/mssql_python/pybind/ddbc_bindings.cpp @@ -8,14 +8,9 @@ #include "connection/connection.h" #include "connection/connection_pool.h" #include "logger_bridge.hpp" -#include "py_ref.hpp" #include "py_type_cache.hpp" #include "utf_utils.h" -using pyref::PyPtr; -using pyref::adopt; - - #include // std::min #include #include // For std::memcpy @@ -137,7 +132,7 @@ struct ParamInfo { SQLLEN strLenOrInd = 0; // Required for DAE bool isDAE = false; // Indicates if we need to stream // Holds a strong reference to the Python object for DAE (data-at-execution) streaming. - // Raw pointer + manual Py_INCREF/DECREF (not PyPtr) because ParamInfo has custom + // Raw pointer + manual Py_INCREF/DECREF (not a RAII wrapper) because ParamInfo has custom // copy/move semantics and is exposed to pybind11 type_caster — changing the member // type would ripple through struct layout, copy/move operators, and property bindings. PyObject* dataPtr = nullptr; @@ -681,9 +676,9 @@ std::vector DetectParamTypes(PyObject* params) { // --- datetime (must check before date, since datetime is subclass of date) --- if (PyDateTime_Check(obj)) { - PyPtr tzinfo = adopt(PyObject_GetAttrString(obj, "tzinfo")); + py::object tzinfo = steal(PyObject_GetAttrString(obj, "tzinfo")); if (!tzinfo) throw py::error_already_set(); - bool has_tz = (tzinfo.get() != Py_None); + bool has_tz = (tzinfo.ptr() != Py_None); if (has_tz) { info.paramSQLType = SQL_SS_TIMESTAMPOFFSET; info.paramCType = SQL_C_SS_TIMESTAMPOFFSET; @@ -737,27 +732,27 @@ std::vector DetectParamTypes(PyObject* params) { int is_decimal = PyObject_IsInstance(obj, decimal_type); if (is_decimal == -1) throw py::error_already_set(); if (is_decimal == 1) { - PyPtr as_tuple_ptr = adopt(PyObject_CallMethod(obj, "as_tuple", NULL)); + py::object as_tuple_ptr = steal(PyObject_CallMethod(obj, "as_tuple", NULL)); if (!as_tuple_ptr) throw py::error_already_set(); - PyPtr exponent_obj = adopt(PyObject_GetAttrString(as_tuple_ptr.get(), "exponent")); + py::object exponent_obj = steal(PyObject_GetAttrString(as_tuple_ptr.ptr(), "exponent")); if (!exponent_obj) throw py::error_already_set(); // NaN / Infinity / sNaN: refuse rather than silently writing 0. - if (PyUnicode_Check(exponent_obj.get())) { + if (PyUnicode_Check(exponent_obj.ptr())) { throw py::value_error( "Cannot bind non-finite Decimal (NaN/Infinity) as SQL NUMERIC"); } - PyPtr digits_obj = adopt(PyObject_GetAttrString(as_tuple_ptr.get(), "digits")); + py::object digits_obj = steal(PyObject_GetAttrString(as_tuple_ptr.ptr(), "digits")); if (!digits_obj) throw py::error_already_set(); - if (!PyTuple_Check(digits_obj.get())) { + if (!PyTuple_Check(digits_obj.ptr())) { throw py::type_error("Decimal.as_tuple().digits must be a tuple"); } - Py_ssize_t num_digits = PyTuple_GET_SIZE(digits_obj.get()); - int exponent = static_cast(PyLong_AsLong(exponent_obj.get())); + Py_ssize_t num_digits = PyTuple_GET_SIZE(digits_obj.ptr()); + int exponent = static_cast(PyLong_AsLong(exponent_obj.ptr())); if (exponent == -1 && PyErr_Occurred()) throw py::error_already_set(); int precision; @@ -801,13 +796,13 @@ std::vector DetectParamTypes(PyObject* params) { } if (in_money_range) { - PyPtr formatted = adopt(PyObject_CallMethod(obj, "__format__", "s", "f")); + py::object formatted = steal(PyObject_CallMethod(obj, "__format__", "s", "f")); if (!formatted) throw py::error_already_set(); info.paramSQLType = SQL_VARCHAR; info.paramCType = PARAM_C_TYPE_TEXT; - info.columnSize = PyUnicode_GET_LENGTH(formatted.get()); + info.columnSize = PyUnicode_GET_LENGTH(formatted.ptr()); info.decimalDigits = 0; - PyObject* raw = formatted.release(); + PyObject* raw = formatted.release().ptr(); if (PyList_SetItem(params, i, raw) != 0) { // PyList_SetItem steals (decrefs) the item even on failure, // so raw is already freed — do NOT Py_DECREF here. @@ -864,37 +859,37 @@ std::vector DetectParamTypes(PyObject* params) { // the caller stores it as a Python capsule via PyList_SetItem for the binder to consume. static NumericData build_numeric_data(PyObject* decimal_param) { // Decimal.as_tuple() exposes the canonical pieces we need: sign, coefficient digits, and exponent. - PyPtr as_tuple = adopt(PyObject_CallMethod(decimal_param, "as_tuple", NULL)); + py::object as_tuple = steal(PyObject_CallMethod(decimal_param, "as_tuple", NULL)); if (!as_tuple) throw py::error_already_set(); // Step 1-4: unpack Decimal(sign, digits, exponent) and validate that digits is the tuple form // promised by Decimal.as_tuple(); the remaining logic works purely from these normalized parts. - PyPtr digits = adopt(PyObject_GetAttrString(as_tuple.get(), "digits")); + py::object digits = steal(PyObject_GetAttrString(as_tuple.ptr(), "digits")); if (!digits) throw py::error_already_set(); - PyPtr sign_obj = adopt(PyObject_GetAttrString(as_tuple.get(), "sign")); + py::object sign_obj = steal(PyObject_GetAttrString(as_tuple.ptr(), "sign")); if (!sign_obj) throw py::error_already_set(); - PyPtr exponent_obj = adopt(PyObject_GetAttrString(as_tuple.get(), "exponent")); + py::object exponent_obj = steal(PyObject_GetAttrString(as_tuple.ptr(), "exponent")); if (!exponent_obj) throw py::error_already_set(); - int sign_val = static_cast(PyLong_AsLong(sign_obj.get())); - sign_obj.reset(); + int sign_val = static_cast(PyLong_AsLong(sign_obj.ptr())); + sign_obj = py::object(); if (sign_val == -1 && PyErr_Occurred()) throw py::error_already_set(); int exponent = 0; - if (PyLong_Check(exponent_obj.get())) { - exponent = static_cast(PyLong_AsLong(exponent_obj.get())); + if (PyLong_Check(exponent_obj.ptr())) { + exponent = static_cast(PyLong_AsLong(exponent_obj.ptr())); if (exponent == -1 && PyErr_Occurred()) throw py::error_already_set(); } - exponent_obj.reset(); + exponent_obj = py::object(); - if (!PyTuple_Check(digits.get())) { + if (!PyTuple_Check(digits.ptr())) { throw py::type_error("Decimal.as_tuple().digits must be a tuple"); } // Step 5: SQL Server precision counts all stored decimal digits, while scale is just the fractional // digits. A positive exponent means trailing zeros move into the integer part; a negative exponent // means scale = -exponent and precision must still cover leading fractional zeros such as 0.001. - int num_digits = static_cast(PyTuple_GET_SIZE(digits.get())); + int num_digits = static_cast(PyTuple_GET_SIZE(digits.ptr())); int precision, scale; if (exponent >= 0) { precision = num_digits + exponent; @@ -908,58 +903,58 @@ static NumericData build_numeric_data(PyObject* decimal_param) { // Step 6: build the mantissa with Python bigint math so we preserve every Decimal digit; a fixed // C++ integer would overflow long before SQL Server's 38-digit NUMERIC limit. - PyPtr py_ten = adopt(PyLong_FromLong(10)); - PyPtr int_val = adopt(PyLong_FromLong(0)); + py::object py_ten = steal(PyLong_FromLong(10)); + py::object int_val = steal(PyLong_FromLong(0)); if (!py_ten || !int_val) { throw py::error_already_set(); } - const Py_ssize_t digit_count = PyTuple_GET_SIZE(digits.get()); + const Py_ssize_t digit_count = PyTuple_GET_SIZE(digits.ptr()); for (Py_ssize_t i = 0; i < digit_count; ++i) { - PyObject* digit_obj = PyTuple_GET_ITEM(digits.get(), i); + PyObject* digit_obj = PyTuple_GET_ITEM(digits.ptr(), i); long digit = PyLong_AsLong(digit_obj); if (digit == -1 && PyErr_Occurred()) throw py::error_already_set(); - PyPtr multiplied = adopt(PyNumber_Multiply(int_val.get(), py_ten.get())); + py::object multiplied = steal(PyNumber_Multiply(int_val.ptr(), py_ten.ptr())); if (!multiplied) throw py::error_already_set(); - PyPtr py_digit = adopt(PyLong_FromLong(digit)); + py::object py_digit = steal(PyLong_FromLong(digit)); if (!py_digit) throw py::error_already_set(); - int_val = adopt(PyNumber_Add(multiplied.get(), py_digit.get())); + int_val = steal(PyNumber_Add(multiplied.ptr(), py_digit.ptr())); if (!int_val) throw py::error_already_set(); } // Step 7: a positive Decimal exponent means the tuple digits omit trailing zeros, so Decimal("123e2") // must become integer mantissa 12300 before packing. if (exponent > 0) { - PyPtr multiplier = adopt(PyLong_FromLong(1)); + py::object multiplier = steal(PyLong_FromLong(1)); if (!multiplier) throw py::error_already_set(); for (int j = 0; j < exponent; ++j) { - multiplier = adopt(PyNumber_Multiply(multiplier.get(), py_ten.get())); + multiplier = steal(PyNumber_Multiply(multiplier.ptr(), py_ten.ptr())); if (!multiplier) throw py::error_already_set(); } - int_val = adopt(PyNumber_Multiply(int_val.get(), multiplier.get())); + int_val = steal(PyNumber_Multiply(int_val.ptr(), multiplier.ptr())); if (!int_val) throw py::error_already_set(); } // Step 8: SQL_NUMERIC_STRUCT stores magnitude and sign separately, so encode only the absolute value. - PyPtr abs_val = adopt(PyNumber_Absolute(int_val.get())); - int_val.reset(); - py_ten.reset(); - digits.reset(); - as_tuple.reset(); + py::object abs_val = steal(PyNumber_Absolute(int_val.ptr())); + int_val = py::object(); + py_ten = py::object(); + digits = py::object(); + as_tuple = py::object(); if (!abs_val) throw py::error_already_set(); // Step 9: SQL_NUMERIC_STRUCT::val is a 16-byte little-endian unsigned integer buffer, so ask // Python's bigint to serialize directly into that wire format instead of reimplementing base conversion. - PyPtr val_bytes = adopt(PyObject_CallMethod(abs_val.get(), "to_bytes", "is", 16, "little")); - abs_val.reset(); + py::object val_bytes = steal(PyObject_CallMethod(abs_val.ptr(), "to_bytes", "is", 16, "little")); + abs_val = py::object(); if (!val_bytes) throw py::error_already_set(); char* val_buf = nullptr; Py_ssize_t val_size = 0; - if (PyBytes_AsStringAndSize(val_bytes.get(), &val_buf, &val_size) == -1) { + if (PyBytes_AsStringAndSize(val_bytes.ptr(), &val_buf, &val_size) == -1) { throw py::error_already_set(); } diff --git a/mssql_python/pybind/py_ref.hpp b/mssql_python/pybind/py_ref.hpp deleted file mode 100644 index 852221da..00000000 --- a/mssql_python/pybind/py_ref.hpp +++ /dev/null @@ -1,26 +0,0 @@ -// py_ref.hpp — RAII wrapper for CPython refcount management (Pattern 0). -// -// PyPtr = std::unique_ptr -// -// Zero runtime overhead (stateless functor → empty-base optimisation). -// Use adopt() for new references, incref_borrow() for borrowed ones. - -#pragma once -#include -#include - -namespace pyref { - -struct PyDecRefDeleter { - void operator()(PyObject* p) const noexcept { Py_XDECREF(p); } -}; - -using PyPtr = std::unique_ptr; - -// Wrap a new reference (already +1 from the API that returned it). -inline PyPtr adopt(PyObject* p) noexcept { return PyPtr{p}; } - -// Extend a borrowed reference's lifetime past its borrower. -inline PyPtr incref_borrow(PyObject* p) noexcept { Py_XINCREF(p); return PyPtr{p}; } - -} // namespace pyref diff --git a/mssql_python/pybind/py_type_cache.hpp b/mssql_python/pybind/py_type_cache.hpp index 78ed2acf..9c6c4850 100644 --- a/mssql_python/pybind/py_type_cache.hpp +++ b/mssql_python/pybind/py_type_cache.hpp @@ -11,11 +11,11 @@ #include #include #include -#include "py_ref.hpp" namespace py = pybind11; -using pyref::PyPtr; -using pyref::adopt; + +// Shorthand for adopting CPython new-references into pybind11 RAII. +inline py::object steal(PyObject* p) { return py::reinterpret_steal(py::handle(p)); } namespace PyTypeCache { @@ -33,9 +33,9 @@ inline bool cache_initialized = false; // Import a module and extract an attribute. Returns a new reference. inline PyObject* import_attr(const char* module_name, const char* attr_name) { - PyPtr mod = adopt(PyImport_ImportModule(module_name)); + py::object mod = steal(PyImport_ImportModule(module_name)); if (!mod) throw py::error_already_set(); - PyObject* attr = PyObject_GetAttrString(mod.get(), attr_name); + PyObject* attr = PyObject_GetAttrString(mod.ptr(), attr_name); if (!attr) throw py::error_already_set(); return attr; } @@ -44,12 +44,12 @@ inline PyObject* import_attr(const char* module_name, const char* attr_name) { // before initialize() has run. inline PyObject* get_cached_class(PyObject* cached, const char* module_name, const char* attr_name) { if (cache_initialized && cached) return cached; - PyPtr mod = adopt(PyImport_ImportModule(module_name)); + py::object mod = steal(PyImport_ImportModule(module_name)); if (!mod) return nullptr; - return PyObject_GetAttrString(mod.get(), attr_name); + return PyObject_GetAttrString(mod.ptr(), attr_name); } -// One-time init. Uses local PyPtrs so exception cleanup is automatic; +// One-time init. Uses local py::objects so exception cleanup is automatic; // only .release() into globals after ALL acquisitions succeed. inline void initialize() { if (cache_initialized) return; @@ -57,35 +57,35 @@ inline void initialize() { PyDateTime_IMPORT; if (PyDateTimeAPI == nullptr) throw py::error_already_set(); - PyPtr dt_mod = adopt(PyImport_ImportModule("datetime")); + py::object dt_mod = steal(PyImport_ImportModule("datetime")); if (!dt_mod) throw py::error_already_set(); - PyPtr dt_cls = adopt(PyObject_GetAttrString(dt_mod.get(), "datetime")); - PyPtr date_cls = adopt(PyObject_GetAttrString(dt_mod.get(), "date")); - PyPtr time_cls = adopt(PyObject_GetAttrString(dt_mod.get(), "time")); + py::object dt_cls = steal(PyObject_GetAttrString(dt_mod.ptr(), "datetime")); + py::object date_cls = steal(PyObject_GetAttrString(dt_mod.ptr(), "date")); + py::object time_cls = steal(PyObject_GetAttrString(dt_mod.ptr(), "time")); if (!dt_cls || !date_cls || !time_cls) throw py::error_already_set(); - PyPtr dec_cls = adopt(import_attr("decimal", "Decimal")); - PyPtr uuid_cls = adopt(import_attr("uuid", "UUID")); + py::object dec_cls = steal(import_attr("decimal", "Decimal")); + py::object uuid_cls = steal(import_attr("uuid", "UUID")); // Pre-compute MONEY/SMALLMONEY boundary Decimals for exact comparison // in DetectParamTypes (avoids double-precision boundary errors). - PyPtr sm_min = adopt(PyObject_CallFunction(dec_cls.get(), "s", "-214748.3648")); - PyPtr sm_max = adopt(PyObject_CallFunction(dec_cls.get(), "s", "214748.3647")); - PyPtr m_min = adopt(PyObject_CallFunction(dec_cls.get(), "s", "-922337203685477.5808")); - PyPtr m_max = adopt(PyObject_CallFunction(dec_cls.get(), "s", "922337203685477.5807")); + py::object sm_min = steal(PyObject_CallFunction(dec_cls.ptr(), "s", "-214748.3648")); + py::object sm_max = steal(PyObject_CallFunction(dec_cls.ptr(), "s", "214748.3647")); + py::object m_min = steal(PyObject_CallFunction(dec_cls.ptr(), "s", "-922337203685477.5808")); + py::object m_max = steal(PyObject_CallFunction(dec_cls.ptr(), "s", "922337203685477.5807")); if (!sm_min || !sm_max || !m_min || !m_max) throw py::error_already_set(); // Commit to globals — all acquisitions succeeded. - datetime_class = dt_cls.release(); - date_class = date_cls.release(); - time_class = time_cls.release(); - decimal_class = dec_cls.release(); - uuid_class = uuid_cls.release(); - smallmoney_min = sm_min.release(); - smallmoney_max = sm_max.release(); - money_min = m_min.release(); - money_max = m_max.release(); + datetime_class = dt_cls.release().ptr(); + date_class = date_cls.release().ptr(); + time_class = time_cls.release().ptr(); + decimal_class = dec_cls.release().ptr(); + uuid_class = uuid_cls.release().ptr(); + smallmoney_min = sm_min.release().ptr(); + smallmoney_max = sm_max.release().ptr(); + money_min = m_min.release().ptr(); + money_max = m_max.release().ptr(); cache_initialized = true; } From a2eb3a7dc3a4187bf1f4751318b8bce2fc92f7a7 Mon Sep 17 00:00:00 2001 From: Gaurav Sharma Date: Thu, 30 Jul 2026 16:21:36 +0530 Subject: [PATCH 27/29] PERF: build NUMERIC mantissa with fixed-width limbs instead of Python bigint build_numeric_data walked every Decimal digit through PyNumber_Multiply/PyNumber_Add, allocating three PyLongs per digit, then called PyNumber_Absolute and to_bytes(16) to serialise. it also re-entered Python for as_tuple() and the digits/exponent attributes that DetectParamTypes had already fetched. SQL Server caps NUMERIC at 38 digits and callers reject anything larger, so the mantissa always fits 128 bits. accumulate it in four uint32 limbs and write the 16 little-endian bytes directly. limbs rather than __int128 because MSVC has no __int128, and the bytes are written explicitly so host endianness does not matter. as_tuple/digits/exponent are now passed in from the caller. numeric decimal path measured 2.3x-2.9x faster (19-digit 1643 -> 711 ns/param, 38-digit 2401 -> 824). money path and all non-decimal types unchanged. 1922 tests pass, and decimal round-trip verified across sign, zero, money bounds, positive exponents, 38-digit magnitudes and scale-38 values. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- mssql_python/pybind/ddbc_bindings.cpp | 144 ++++++++++---------------- 1 file changed, 52 insertions(+), 92 deletions(-) diff --git a/mssql_python/pybind/ddbc_bindings.cpp b/mssql_python/pybind/ddbc_bindings.cpp index 00035c01..c7da95f7 100644 --- a/mssql_python/pybind/ddbc_bindings.cpp +++ b/mssql_python/pybind/ddbc_bindings.cpp @@ -492,7 +492,7 @@ static constexpr SQLSMALLINT PARAM_C_TYPE_TEXT = SQL_C_CHAR; #endif // Forward declare NumericData helper used by decimal path -static NumericData build_numeric_data(PyObject* decimal_param); +static NumericData build_numeric_data(PyObject* as_tuple, PyObject* digits, int exponent); // --------------------------------------------------------------------------- // DetectParamTypes: Raw CPython parameter type detection for the primary execute path. @@ -815,7 +815,7 @@ std::vector DetectParamTypes(PyObject* params) { // object in the param list so BindParameters can extract it as NumericData. info.paramSQLType = SQL_NUMERIC; info.paramCType = SQL_C_NUMERIC; - NumericData nd = build_numeric_data(obj); + NumericData nd = build_numeric_data(as_tuple_ptr.ptr(), digits_obj.ptr(), exponent); info.columnSize = nd.precision; info.decimalDigits = nd.scale; // Store NumericData as a Python object in the param list for the binder. @@ -853,43 +853,34 @@ std::vector DetectParamTypes(PyObject* params) { return infos; } -// Helper: build SQL_NUMERIC_STRUCT from Python Decimal -// Converts a Python Decimal into a SQL_NUMERIC_STRUCT representation. -// Takes raw PyObject* (must be a Decimal instance). Returns NumericData directly — -// the caller stores it as a Python capsule via PyList_SetItem for the binder to consume. -static NumericData build_numeric_data(PyObject* decimal_param) { - // Decimal.as_tuple() exposes the canonical pieces we need: sign, coefficient digits, and exponent. - py::object as_tuple = steal(PyObject_CallMethod(decimal_param, "as_tuple", NULL)); - if (!as_tuple) throw py::error_already_set(); - - // Step 1-4: unpack Decimal(sign, digits, exponent) and validate that digits is the tuple form - // promised by Decimal.as_tuple(); the remaining logic works purely from these normalized parts. - py::object digits = steal(PyObject_GetAttrString(as_tuple.ptr(), "digits")); - if (!digits) throw py::error_already_set(); - py::object sign_obj = steal(PyObject_GetAttrString(as_tuple.ptr(), "sign")); +// Helper: build SQL_NUMERIC_STRUCT from an already-unpacked Decimal.as_tuple(). +// +// Callers in DetectParamTypes have already called as_tuple() and pulled out the digits +// tuple and exponent, so those are passed in rather than re-entering Python to fetch +// them a second time. +// +// The mantissa is accumulated into a fixed 128-bit value held as four 32-bit limbs +// instead of Python bigint arithmetic. SQL Server caps NUMERIC precision at +// MAX_NUMERIC_PRECISION (38) digits and callers reject anything larger, so the value +// always fits the 16 bytes SQL_NUMERIC_STRUCT provides. Limbs keep this portable +// (MSVC has no __int128) and the result is written out byte-by-byte so host endianness +// does not matter. +static NumericData build_numeric_data(PyObject* as_tuple, PyObject* digits, int exponent) { + py::object sign_obj = steal(PyObject_GetAttrString(as_tuple, "sign")); if (!sign_obj) throw py::error_already_set(); - py::object exponent_obj = steal(PyObject_GetAttrString(as_tuple.ptr(), "exponent")); - if (!exponent_obj) throw py::error_already_set(); - int sign_val = static_cast(PyLong_AsLong(sign_obj.ptr())); - sign_obj = py::object(); if (sign_val == -1 && PyErr_Occurred()) throw py::error_already_set(); - int exponent = 0; - if (PyLong_Check(exponent_obj.ptr())) { - exponent = static_cast(PyLong_AsLong(exponent_obj.ptr())); - if (exponent == -1 && PyErr_Occurred()) throw py::error_already_set(); - } - exponent_obj = py::object(); - - if (!PyTuple_Check(digits.ptr())) { + if (!PyTuple_Check(digits)) { throw py::type_error("Decimal.as_tuple().digits must be a tuple"); } - // Step 5: SQL Server precision counts all stored decimal digits, while scale is just the fractional - // digits. A positive exponent means trailing zeros move into the integer part; a negative exponent - // means scale = -exponent and precision must still cover leading fractional zeros such as 0.001. - int num_digits = static_cast(PyTuple_GET_SIZE(digits.ptr())); + // SQL Server precision counts all stored decimal digits, while scale is just the + // fractional digits. A positive exponent moves trailing zeros into the integer part; + // a negative exponent means scale = -exponent and precision must still cover leading + // fractional zeros such as 0.001. + const Py_ssize_t digit_count = PyTuple_GET_SIZE(digits); + const int num_digits = static_cast(digit_count); int precision, scale; if (exponent >= 0) { precision = num_digits + exponent; @@ -901,78 +892,47 @@ static NumericData build_numeric_data(PyObject* decimal_param) { precision = std::max(1, std::min(precision, MAX_NUMERIC_PRECISION)); scale = std::min(scale, precision); - // Step 6: build the mantissa with Python bigint math so we preserve every Decimal digit; a fixed - // C++ integer would overflow long before SQL Server's 38-digit NUMERIC limit. - py::object py_ten = steal(PyLong_FromLong(10)); - py::object int_val = steal(PyLong_FromLong(0)); - if (!py_ten || !int_val) { - throw py::error_already_set(); - } + // 128-bit magnitude as four little-endian 32-bit limbs. Returns the carry out of the + // top limb, which is non-zero only if the value overflowed 128 bits. + uint32_t limb[4] = {0, 0, 0, 0}; + auto mul10_add = [&limb](uint32_t addend) -> uint64_t { + uint64_t carry = addend; + for (int k = 0; k < 4; ++k) { + uint64_t cur = static_cast(limb[k]) * 10u + carry; + limb[k] = static_cast(cur); + carry = cur >> 32; + } + return carry; + }; - const Py_ssize_t digit_count = PyTuple_GET_SIZE(digits.ptr()); + uint64_t overflow = 0; for (Py_ssize_t i = 0; i < digit_count; ++i) { - PyObject* digit_obj = PyTuple_GET_ITEM(digits.ptr(), i); + PyObject* digit_obj = PyTuple_GET_ITEM(digits, i); long digit = PyLong_AsLong(digit_obj); if (digit == -1 && PyErr_Occurred()) throw py::error_already_set(); - - py::object multiplied = steal(PyNumber_Multiply(int_val.ptr(), py_ten.ptr())); - if (!multiplied) throw py::error_already_set(); - - py::object py_digit = steal(PyLong_FromLong(digit)); - if (!py_digit) throw py::error_already_set(); - - int_val = steal(PyNumber_Add(multiplied.ptr(), py_digit.ptr())); - if (!int_val) throw py::error_already_set(); + overflow |= mul10_add(static_cast(digit)); } - - // Step 7: a positive Decimal exponent means the tuple digits omit trailing zeros, so Decimal("123e2") - // must become integer mantissa 12300 before packing. - if (exponent > 0) { - py::object multiplier = steal(PyLong_FromLong(1)); - if (!multiplier) throw py::error_already_set(); - for (int j = 0; j < exponent; ++j) { - multiplier = steal(PyNumber_Multiply(multiplier.ptr(), py_ten.ptr())); - if (!multiplier) throw py::error_already_set(); - } - int_val = steal(PyNumber_Multiply(int_val.ptr(), multiplier.ptr())); - if (!int_val) throw py::error_already_set(); + // A positive exponent means as_tuple() omitted trailing zeros, so Decimal("123e2") + // must become mantissa 12300 before packing. + for (int j = 0; j < exponent; ++j) { + overflow |= mul10_add(0); } - - // Step 8: SQL_NUMERIC_STRUCT stores magnitude and sign separately, so encode only the absolute value. - py::object abs_val = steal(PyNumber_Absolute(int_val.ptr())); - int_val = py::object(); - py_ten = py::object(); - digits = py::object(); - as_tuple = py::object(); - if (!abs_val) throw py::error_already_set(); - - // Step 9: SQL_NUMERIC_STRUCT::val is a 16-byte little-endian unsigned integer buffer, so ask - // Python's bigint to serialize directly into that wire format instead of reimplementing base conversion. - py::object val_bytes = steal(PyObject_CallMethod(abs_val.ptr(), "to_bytes", "is", 16, "little")); - abs_val = py::object(); - if (!val_bytes) throw py::error_already_set(); - - char* val_buf = nullptr; - Py_ssize_t val_size = 0; - if (PyBytes_AsStringAndSize(val_bytes.ptr(), &val_buf, &val_size) == -1) { - throw py::error_already_set(); + if (overflow != 0) { + throw py::value_error("Decimal magnitude exceeds the 16-byte SQL NUMERIC capacity"); } - // Step 10: pack precision/scale plus the 16-byte little-endian magnitude. SQL uses sign=1 for - // positive and sign=0 for negative, which is the inverse of Decimal.as_tuple().sign. NumericData nd; nd.precision = static_cast(precision); nd.scale = static_cast(scale); + // SQL uses sign=1 for positive and sign=0 for negative, the inverse of + // Decimal.as_tuple().sign. nd.sign = (sign_val == 0) ? 1 : 0; - std::memset(&nd.val[0], 0, SQL_MAX_NUMERIC_LEN); - size_t copy_len = std::min(static_cast(val_size), static_cast(SQL_MAX_NUMERIC_LEN)); - if (copy_len > 0 && val_buf != nullptr) { -#ifdef _MSC_VER - memcpy_s(&nd.val[0], SQL_MAX_NUMERIC_LEN, val_buf, copy_len); -#else - // copy_len is bounded to SQL_MAX_NUMERIC_LEN above — safe by construction - std::memcpy(&nd.val[0], val_buf, copy_len); // DevSkim: ignore DS121708 -#endif + nd.val.assign(SQL_MAX_NUMERIC_LEN, '\0'); + for (int k = 0; k < 4; ++k) { + nd.val[k * 4 + 0] = static_cast(limb[k] & 0xFF); + nd.val[k * 4 + 1] = static_cast((limb[k] >> 8) & 0xFF); + nd.val[k * 4 + 2] = static_cast((limb[k] >> 16) & 0xFF); + nd.val[k * 4 + 3] = static_cast((limb[k] >> 24) & 0xFF); } return nd; } From 2342353104cc2d8cbc8556824aaef191304af6a9 Mon Sep 17 00:00:00 2001 From: Gaurav Sharma <223556219+Copilot@users.noreply.github.com> Date: Fri, 31 Jul 2026 14:47:38 +0530 Subject: [PATCH 28/29] REFACTOR: hold ParamInfo::dataPtr in py::object instead of a hand-rolled rule of five ParamInfo kept dataPtr as a raw PyObject* and hand-wrote a destructor, copy constructor, copy assignment, move constructor and move assignment to pair Py_XINCREF with Py_XDECREF. five special members existed only to keep one refcount straight, and the copy assignment decref'd the old value before increfing the new one, which is the wrong order if the two are ever the same object. py::object already owns a refcount correctly. holding dataPtr as py::object makes the compiler-generated destructor, copy and move all correct, so the entire rule-of-five block goes away and the struct needs no special members at all. 70 lines deleted, 10 added. the two DetectParamTypes sites drop their explicit Py_INCREF, the two SQLParamData consumers read .ptr(), and the pybind property getter returns the object directly instead of reborrowing it. destruction still runs with the GIL held: the gil_scoped_release scopes in both execute paths close before the ParamInfo values die. 1922 tests pass, the refcount harness shows zero drift over 300 executes on every parameter case including str_long_dae, and the DAE path round-trips all types exactly. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- mssql_python/pybind/ddbc_bindings.cpp | 80 ++++----------------------- 1 file changed, 10 insertions(+), 70 deletions(-) diff --git a/mssql_python/pybind/ddbc_bindings.cpp b/mssql_python/pybind/ddbc_bindings.cpp index c7da95f7..63e34cba 100644 --- a/mssql_python/pybind/ddbc_bindings.cpp +++ b/mssql_python/pybind/ddbc_bindings.cpp @@ -131,65 +131,11 @@ struct ParamInfo { SQLSMALLINT decimalDigits = 0; SQLLEN strLenOrInd = 0; // Required for DAE bool isDAE = false; // Indicates if we need to stream - // Holds a strong reference to the Python object for DAE (data-at-execution) streaming. - // Raw pointer + manual Py_INCREF/DECREF (not a RAII wrapper) because ParamInfo has custom - // copy/move semantics and is exposed to pybind11 type_caster — changing the member - // type would ripple through struct layout, copy/move operators, and property bindings. - PyObject* dataPtr = nullptr; + // Strong reference to the Python object for DAE (data-at-execution) streaming. + // py::object owns the refcount, so the compiler-generated destructor, copy and + // move operations are all correct and this struct needs no rule-of-five. + py::object dataPtr; Py_ssize_t utf16Len = 0; // UTF-16 code unit count for string params - - ParamInfo() = default; - ~ParamInfo() { Py_XDECREF(dataPtr); } - // Copy needed by pybind11 type_caster for the legacy path (std::vector&). - ParamInfo(const ParamInfo& other) - : inputOutputType(other.inputOutputType), paramCType(other.paramCType), - paramSQLType(other.paramSQLType), columnSize(other.columnSize), - decimalDigits(other.decimalDigits), strLenOrInd(other.strLenOrInd), - isDAE(other.isDAE), dataPtr(other.dataPtr), utf16Len(other.utf16Len) { - Py_XINCREF(dataPtr); - } - // Copy/move assignment and move constructor below are required for - // std::vector resize/reallocation and pybind11's type_caster. - // They manually manage dataPtr refcounts since ParamInfo owns a strong ref. - ParamInfo& operator=(const ParamInfo& other) { - if (this != &other) { - Py_XDECREF(dataPtr); - inputOutputType = other.inputOutputType; - paramCType = other.paramCType; - paramSQLType = other.paramSQLType; - columnSize = other.columnSize; - decimalDigits = other.decimalDigits; - strLenOrInd = other.strLenOrInd; - isDAE = other.isDAE; - dataPtr = other.dataPtr; - utf16Len = other.utf16Len; - Py_XINCREF(dataPtr); - } - return *this; - } - ParamInfo(ParamInfo&& other) noexcept - : inputOutputType(other.inputOutputType), paramCType(other.paramCType), - paramSQLType(other.paramSQLType), columnSize(other.columnSize), - decimalDigits(other.decimalDigits), strLenOrInd(other.strLenOrInd), - isDAE(other.isDAE), dataPtr(other.dataPtr), utf16Len(other.utf16Len) { - other.dataPtr = nullptr; - } - ParamInfo& operator=(ParamInfo&& other) noexcept { - if (this != &other) { - Py_XDECREF(dataPtr); - inputOutputType = other.inputOutputType; - paramCType = other.paramCType; - paramSQLType = other.paramSQLType; - columnSize = other.columnSize; - decimalDigits = other.decimalDigits; - strLenOrInd = other.strLenOrInd; - isDAE = other.isDAE; - dataPtr = other.dataPtr; - utf16Len = other.utf16Len; - other.dataPtr = nullptr; - } - return *this; - } }; #ifdef __GNUC__ #pragma GCC diagnostic pop @@ -631,8 +577,7 @@ std::vector DetectParamTypes(PyObject* params) { info.isDAE = true; info.columnSize = 0; info.utf16Len = utf16_len; - info.dataPtr = obj; - Py_INCREF(obj); + info.dataPtr = py::reinterpret_borrow(py::handle(obj)); info.paramSQLType = is_unicode ? SQL_WVARCHAR : SQL_VARCHAR; info.paramCType = is_unicode ? SQL_C_WCHAR : PARAM_C_TYPE_TEXT; } else { @@ -666,8 +611,7 @@ std::vector DetectParamTypes(PyObject* params) { if (length > MAX_INLINE_BINARY) { info.isDAE = true; info.columnSize = 0; - info.dataPtr = obj; - Py_INCREF(obj); + info.dataPtr = py::reinterpret_borrow(py::handle(obj)); } else { info.columnSize = std::max(length, 1); } @@ -2417,7 +2361,7 @@ SQLRETURN SQLExecuteLegacy_wrap(const SqlHandlePtr statementHandle, const std::u if (!matchedInfo) { ThrowStdException("Unrecognized paramToken returned by SQLParamData"); } - PyObject* pyObj = matchedInfo->dataPtr; + PyObject* pyObj = matchedInfo->dataPtr.ptr(); if (!pyObj || pyObj == Py_None) { putData(nullptr, 0); continue; @@ -2616,7 +2560,7 @@ SQLRETURN SQLExecute_wrap(const SqlHandlePtr statementHandle, if (!matchedInfo) { ThrowStdException("SQLExecuteFast: unrecognized paramToken from SQLParamData"); } - PyObject* pyObj = matchedInfo->dataPtr; + PyObject* pyObj = matchedInfo->dataPtr.ptr(); if (!pyObj || pyObj == Py_None) { py::gil_scoped_release release; SQLPutData_ptr(hStmt, nullptr, 0); @@ -6518,13 +6462,9 @@ PYBIND11_MODULE(ddbc_bindings, m) { if (!info.dataPtr) { return py::none(); } - return py::reinterpret_borrow(py::handle(info.dataPtr)); + return info.dataPtr; }, - [](ParamInfo& info, py::object obj) { - Py_XINCREF(obj.ptr()); - Py_XDECREF(info.dataPtr); - info.dataPtr = obj.ptr(); - }) + [](ParamInfo& info, py::object obj) { info.dataPtr = std::move(obj); }) .def_readwrite("isDAE", &ParamInfo::isDAE); // Define numeric data class From 2a8f18a9d926b8fefd0b3a2187b36fbb4a7b2469 Mon Sep 17 00:00:00 2001 From: Gaurav Sharma <223556219+Copilot@users.noreply.github.com> Date: Fri, 31 Jul 2026 14:59:09 +0530 Subject: [PATCH 29/29] REFACTOR: add a borrow() counterpart to steal() and use both at every call site steal() wrapped py::reinterpret_steal but had no counterpart, so the file mixed a 6-call shorthand against 8 spelled-out py::reinterpret_borrow(py::handle(x)) calls. the dataPtr change in the previous commit added two more of the long form, so the asymmetry was growing. both helpers are now templated on the target type with py::object as the default, matching nanobind's nb::steal and nb::borrow signatures. the template is not cosmetic: four of the eight borrow sites need py::str or py::bytes rather than py::object, so a fixed-return helper would have covered only half of them. existing steal() calls are unaffected by the default argument. having the pair side by side also documents the hazard. steal on a borrowed reference (PyList_GetItem, PyTuple_GetItem, PyDict_GetItem) is a premature decref and a use-after-free, and that precondition now sits on the declaration instead of being implied by the name. no behavior change. 1922 tests pass and the refcount harness reports zero drift over 300 executes on all 15 parameter cases. all eight converted sites live in the DAE streaming paths of SQLExecuteLegacy_wrap and SQLExecute_wrap, so those were exercised directly: 7 DAE cases spanning NVARCHAR(MAX), VARCHAR(MAX) and VARBINARY(MAX), plus bytearray and the 4001-unit boundary, all round-trip byte-exact. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- mssql_python/pybind/ddbc_bindings.cpp | 16 ++++++++-------- mssql_python/pybind/py_type_cache.hpp | 22 +++++++++++++++++----- 2 files changed, 25 insertions(+), 13 deletions(-) diff --git a/mssql_python/pybind/ddbc_bindings.cpp b/mssql_python/pybind/ddbc_bindings.cpp index 63e34cba..cc908c32 100644 --- a/mssql_python/pybind/ddbc_bindings.cpp +++ b/mssql_python/pybind/ddbc_bindings.cpp @@ -577,7 +577,7 @@ std::vector DetectParamTypes(PyObject* params) { info.isDAE = true; info.columnSize = 0; info.utf16Len = utf16_len; - info.dataPtr = py::reinterpret_borrow(py::handle(obj)); + info.dataPtr = borrow(obj); info.paramSQLType = is_unicode ? SQL_WVARCHAR : SQL_VARCHAR; info.paramCType = is_unicode ? SQL_C_WCHAR : PARAM_C_TYPE_TEXT; } else { @@ -611,7 +611,7 @@ std::vector DetectParamTypes(PyObject* params) { if (length > MAX_INLINE_BINARY) { info.isDAE = true; info.columnSize = 0; - info.dataPtr = py::reinterpret_borrow(py::handle(obj)); + info.dataPtr = borrow(obj); } else { info.columnSize = std::max(length, 1); } @@ -2369,7 +2369,7 @@ SQLRETURN SQLExecuteLegacy_wrap(const SqlHandlePtr statementHandle, const std::u if (PyUnicode_Check(pyObj)) { if (matchedInfo->paramCType == SQL_C_WCHAR) { std::u16string utf16 = - py::reinterpret_borrow(py::handle(pyObj)).cast(); + borrow(pyObj).cast(); rc = stream_dae_chunks( reinterpretU16stringAsSqlWChar(utf16), utf16.size() * sizeof(SQLWCHAR), @@ -2382,7 +2382,7 @@ SQLRETURN SQLExecuteLegacy_wrap(const SqlHandlePtr statementHandle, const std::u // Encode the string using the specified encoding std::string encodedStr; try { - py::object encoded = py::reinterpret_borrow(py::handle(pyObj)) + py::object encoded = borrow(pyObj) .attr("encode")(charEncoding, "strict"); encodedStr = encoded.cast(); LOG("SQLExecute: DAE SQL_C_CHAR - Encoded with '%s', %zu bytes", @@ -2406,7 +2406,7 @@ SQLRETURN SQLExecuteLegacy_wrap(const SqlHandlePtr statementHandle, const std::u size_t totalBytes = 0; std::string bytesStorage; // lifetime must span the loop if (PyBytes_Check(pyObj)) { - bytesStorage = py::reinterpret_borrow(py::handle(pyObj)); + bytesStorage = borrow(pyObj); dataPtr = bytesStorage.data(); totalBytes = bytesStorage.size(); } else { @@ -2570,7 +2570,7 @@ SQLRETURN SQLExecute_wrap(const SqlHandlePtr statementHandle, if (PyUnicode_Check(pyObj)) { if (matchedInfo->paramCType == SQL_C_WCHAR) { std::u16string u16 = - py::reinterpret_borrow(py::handle(pyObj)).cast(); + borrow(pyObj).cast(); rc = stream_dae_chunks( reinterpretU16stringAsSqlWChar(u16), u16.size() * sizeof(SQLWCHAR), @@ -2578,7 +2578,7 @@ SQLRETURN SQLExecute_wrap(const SqlHandlePtr statementHandle, if (!SQL_SUCCEEDED(rc)) return rc; } else if (matchedInfo->paramCType == SQL_C_CHAR) { std::string encodedStr; - py::object encoded = py::reinterpret_borrow(py::handle(pyObj)) + py::object encoded = borrow(pyObj) .attr("encode")(charEncoding, "strict"); encodedStr = encoded.cast(); rc = stream_dae_chunks(encodedStr.data(), encodedStr.size(), putData); @@ -2594,7 +2594,7 @@ SQLRETURN SQLExecute_wrap(const SqlHandlePtr statementHandle, std::string bytesStorage; // lifetime must span the loop if (PyBytes_Check(pyObj)) { - bytesStorage = py::reinterpret_borrow(py::handle(pyObj)); + bytesStorage = borrow(pyObj); dataPtr = bytesStorage.data(); totalBytes = bytesStorage.size(); } else { diff --git a/mssql_python/pybind/py_type_cache.hpp b/mssql_python/pybind/py_type_cache.hpp index 9c6c4850..f8ff81ae 100644 --- a/mssql_python/pybind/py_type_cache.hpp +++ b/mssql_python/pybind/py_type_cache.hpp @@ -14,8 +14,19 @@ namespace py = pybind11; -// Shorthand for adopting CPython new-references into pybind11 RAII. -inline py::object steal(PyObject* p) { return py::reinterpret_steal(py::handle(p)); } +// Shorthand for adopting CPython references into pybind11 RAII, named after +// nanobind's nb::steal / nb::borrow. +// +// steal — takes ownership of a NEW reference without increfing. Applying it to +// a borrowed reference (PyList_GetItem, PyTuple_GetItem, PyDict_GetItem, +// PyDict_GetItemString) is a premature decref and a use-after-free. +// borrow — increfs a BORROWED reference. Safe on a new reference only if the +// caller still decrefs it, which it usually should not. +template +inline T steal(PyObject* p) { return py::reinterpret_steal(py::handle(p)); } + +template +inline T borrow(PyObject* p) { return py::reinterpret_borrow(py::handle(p)); } namespace PyTypeCache { @@ -89,11 +100,12 @@ inline void initialize() { cache_initialized = true; } -// Wrap a cached pointer as py::object (borrow for cached, steal for fallback import). +// Wrap a cached pointer as py::object. A cached class is a module-lifetime +// singleton we do not own, so borrow it; the fallback import path returns a new +// reference, so steal it. inline py::object wrap_cached_or_imported(PyObject* obj) { if (!obj) throw py::error_already_set(); - return cache_initialized ? py::reinterpret_borrow(py::handle(obj)) - : py::reinterpret_steal(obj); + return cache_initialized ? borrow(obj) : steal(obj); } inline PyObject* get_datetime_class() { return get_cached_class(datetime_class, "datetime", "datetime"); }