PERF: Native C++ parameter detection and execute pipeline - #549
Open
Gaurav Sharma (bewithgaurav) wants to merge 51 commits into
Open
PERF: Native C++ parameter detection and execute pipeline#549Gaurav Sharma (bewithgaurav) wants to merge 51 commits into
Gaurav Sharma (bewithgaurav) wants to merge 51 commits into
Conversation
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
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
- 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
📊 Code Coverage Report
Diff CoverageDiff: main...HEAD, staged and unstaged changes
Summary
mssql_python/pybind/ddbc_bindings.cppLines 243-251 243 SQLDescribeParamFunc SQLDescribeParam_ptr = nullptr;
244
245 namespace {
246
! 247
248 const char* GetSqlCTypeAsString(const SQLSMALLINT cType) {
249 switch (cType) {
250 STRINGIFY_FOR_CASE(SQL_C_CHAR);
251 STRINGIFY_FOR_CASE(SQL_C_WCHAR);Lines 572-581 572 dataPtr = sqlwcharBuffer->data();
573 bufferLength = sqlwcharBuffer->size() * sizeof(SQLWCHAR);
574 strLenOrIndPtr = AllocateParamBuffer<SQLLEN>(paramBuffers);
575 // Use explicit byte length instead of SQL_NTS so embedded NUL chars
! 576 // aren't treated as string terminators.
! 577 *strLenOrIndPtr = static_cast<SQLLEN>(sqlwcharBuffer->size() * sizeof(SQLWCHAR));
578 }
579 break;
580 }
581 case SQL_C_BIT: {Lines 635-643 635 case SQL_C_LONG: {
636 if (!py::isinstance<py::int_>(param)) {
637 ThrowStdException(MakeParamMismatchErrorStr(paramInfo.paramCType, paramIndex));
638 }
! 639 // Both detection paths (DetectParamTypes / _map_sql_type) reject out-of-int64
640 // ints before binding, so those callers only reach here with bindable values.
641 // A setinputsizes() override that forces SQL_C_SBIGINT on an out-of-range int
642 // skips detection; that value fails the cast below, same as before this change.
643 dataPtr = static_cast<void*>(Lines 703-711 703 dataPtr = static_cast<void*>(sqlTimePtr);
704 break;
705 }
706 case SQL_C_SS_TIMESTAMPOFFSET: {
! 707 py::object datetimeType = PyTypeCache::get_datetime_class_obj();
708 if (!py::isinstance(param, datetimeType)) {
709 ThrowStdException(MakeParamMismatchErrorStr(paramInfo.paramCType, paramIndex));
710 }
711 // Checking if the object has a timezoneLines 1806-1815 1806
1807 // LEGACY — slated for removal in a future optimization round.
1808 //
1809 // Executes the provided query using a ParamInfo list that Python already built,
! 1810 // rather than detecting parameter types in C++. Retained only for setinputsizes()
! 1811 // callers, whose explicit type overrides the native path does not yet honour.
1812 // Every parameter crosses the pybind11 boundary as a ParamInfo object here, which
1813 // is the cost SQLExecute_wrap exists to avoid. Once setinputsizes is handled
1814 // natively this function and its DDBCSQLExecuteLegacy binding both go away.
1815 //Lines 1950-1959 1950 if (matchedInfo->paramCType == SQL_C_WCHAR) {
1951 std::u16string utf16 =
1952 borrow<py::str>(pyObj).cast<std::u16string>();
1953 rc = stream_dae_chunks(
! 1954 reinterpretU16stringAsSqlWChar(utf16),
! 1955 utf16.size() * sizeof(SQLWCHAR),
1956 putData);
1957 if (!SQL_SUCCEEDED(rc)) {
1958 LOG("SQLExecute: SQLPutData failed for SQL_C_WCHAR DAE streaming");
1959 return rc;Lines 2046-2061 2046 return SQL_INVALID_HANDLE;
2047 }
2048
2049 SQLHANDLE hStmt = statementHandle->get();
! 2050
! 2051 // Configure forward-only / read-only cursor (matches slow path semantics).
2052 if (SQLSetStmtAttr_ptr) {
2053 SQLSetStmtAttr_ptr(hStmt, SQL_ATTR_CURSOR_TYPE,
! 2054 (SQLPOINTER)SQL_CURSOR_FORWARD_ONLY, 0);
! 2055 SQLSetStmtAttr_ptr(hStmt, SQL_ATTR_CONCURRENCY,
! 2056 (SQLPOINTER)SQL_CONCUR_READ_ONLY, 0);
! 2057 }
2058
2059 // The encoding-settings dict has the form {"encoding": str, "ctype": int}.
2060 // Note: the Python layer's SQL_C_CHAR constant is numerically -8, the same
2061 // as ODBC's SQL_C_WCHAR. As a result, the only path that genuinely usesLines 2065-2081 2065 // encoding when ctype == 1 (real ODBC SQL_CHAR). Otherwise the user's
2066 // "encoding" value is meant for the wide-char path and we leave it alone.
2067 std::string charEncoding = "utf-8";
2068 if (encoding_settings.contains("ctype") && encoding_settings.contains("encoding")) {
! 2069 int ctype = encoding_settings["ctype"].cast<int>();
! 2070 if (ctype == SQL_C_CHAR /* real ODBC value: 1 */) {
! 2071 charEncoding = encoding_settings["encoding"].cast<std::string>();
! 2072 }
! 2073 }
! 2074
! 2075 // The cursor.py caller always passes a fresh `list(actual_params)` so this
! 2076 // function is free to mutate slots in place. Even so, every site below uses
! 2077 // PyList_SetItem (which decrefs the old slot before stealing the new ref),
2078 // so the function is safe regardless of who owns the list.
2079
2080 // Run DetectParamTypes BEFORE SQLPrepare so that type-detection errors
2081 // (unsupported type, NaN Decimal, precision overflow) don't leave theLines 2098-2107 2098 }
2099 if (!SQL_SUCCEEDED(rc)) return rc;
2100 statementHandle->clearDescribeCache();
2101 is_stmt_prepared[0] = py::bool_(true);
! 2102 } else {
! 2103 ThrowStdException("Cannot execute unprepared statement");
2104 }
2105 }
2106
2107 std::vector<std::shared_ptr<void>> paramBuffers;Lines 2129-2139 2129 rc = SQLParamData_ptr(hStmt, ¶mToken);
2130 }
2131 if (rc != SQL_NEED_DATA) break;
2132
! 2133 // The DAE token is the ¶mInfos[i] we handed to SQLBindParameter as the
! 2134 // parameter value (see BindParameters), and paramInfos is sized up front and
! 2135 // never reallocated, so the token casts straight back to its ParamInfo instead
2136 // of scanning. Range-check it against the vector before trusting it, so a bogus
2137 // token throws rather than dereferencing arbitrary memory.
2138 const ParamInfo* matchedInfo = reinterpret_cast<const ParamInfo*>(paramToken);
2139 const ParamInfo* first = paramInfos.data();Lines 2152-2160 2152 if (matchedInfo->paramCType == SQL_C_WCHAR) {
2153 std::u16string u16 =
2154 borrow<py::str>(pyObj).cast<std::u16string>();
2155 rc = stream_dae_chunks(
! 2156 reinterpretU16stringAsSqlWChar(u16),
2157 u16.size() * sizeof(SQLWCHAR),
2158 putData);
2159 if (!SQL_SUCCEEDED(rc)) return rc;
2160 } else if (matchedInfo->paramCType == SQL_C_CHAR) {Lines 2169-2177 2169 }
2170 } else if (PyBytes_Check(pyObj) || PyByteArray_Check(pyObj)) {
2171 // matchedInfo->dataPtr holds a strong ref to pyObj for the whole loop.
2172 const char* dataPtr = nullptr;
! 2173 size_t totalBytes = 0;
2174 std::string bytesStorage; // only used for the bytearray copy below
2175
2176 if (PyBytes_Check(pyObj)) {
2177 // bytes is immutable and kept alive by the strong ref above, so streamLines 2185-2196 2185 bytesStorage.assign(PyByteArray_AS_STRING(pyObj),
2186 static_cast<size_t>(PyByteArray_GET_SIZE(pyObj)));
2187 dataPtr = bytesStorage.data();
2188 totalBytes = bytesStorage.size();
! 2189 }
! 2190
! 2191 rc = stream_dae_chunks(dataPtr, totalBytes, putData);
! 2192 if (!SQL_SUCCEEDED(rc)) return rc;
2193 } else {
2194 ThrowStdException("SQLExecute: DAE only supported for str or bytes");
2195 }
2196 }Lines 2198-2206 2198 }
2199
2200 if (!SQL_SUCCEEDED(rc) && rc != SQL_NO_DATA) return rc;
2201
! 2202 // Unbind parameter buffers before they go out of scope.
2203 // Not called on error paths — diagnostics must remain readable.
2204 SQLRETURN exec_rc = rc;
2205 SQLFreeStmt_ptr(hStmt, SQL_RESET_PARAMS);
2206 return exec_rc;Lines 2617-2625 2617 DateTimeOffset* dtoArray =
2618 AllocateParamBufferArray<DateTimeOffset>(tempBuffers, paramSetSize);
2619 strLenOrIndArray = AllocateParamBufferArray<SQLLEN>(tempBuffers, paramSetSize);
2620
! 2621 py::object datetimeType = PyTypeCache::get_datetime_class_obj();
2622
2623 for (size_t i = 0; i < paramSetSize; ++i) {
2624 const py::handle& param = columnValues[i];Lines 2732-2740 2732
2733 // Get cached UUID class from module-level helper
2734 // This avoids static object destruction issues during
2735 // Python finalization
! 2736 py::object uuid_class = PyTypeCache::get_uuid_class_obj();
2737 // Get cached UUID class
2738
2739 for (size_t i = 0; i < paramSetSize; ++i) {
2740 const py::handle& element = columnValues[i];Lines 4418-4426 4418 break;
4419 }
4420 case SQL_TYPE_DATE: {
4421 PyObject* dateObj =
! 4422 PyTypeCache::get_date_class_obj()(buffers.dateBuffers[col - 1][i].year,
4423 buffers.dateBuffers[col - 1][i].month,
4424 buffers.dateBuffers[col - 1][i].day)
4425 .release()
4426 .ptr();mssql_python/pybind/param_detect.hppLines 271-280 271 if (!as_str) {
272 // PyObject_Str can fail (e.g. CPython's int->str digit limit for a
273 // multi-thousand-digit int). Drop that error and fall back to a
274 // placeholder so we still raise our own clear ValueError.
! 275 PyErr_Clear();
! 276 }
277 std::string s = as_str ? as_str.cast<std::string>() : std::string("<int>");
278 throw py::value_error("integer " + s +
279 " is out of range for SQL BIGINT [-2^63, 2^63-1]");
280 } else {Lines 278-287 278 throw py::value_error("integer " + s +
279 " is out of range for SQL BIGINT [-2^63, 2^63-1]");
280 } else {
281 // A real Python error from PyLong_AsLongLongAndOverflow, not overflow.
! 282 throw py::error_already_set();
! 283 }
284 info.decimalDigits = 0;
285 continue;
286 }Lines 420-429 420 // so calling the same method is what keeps the two paths in agreement.
421 py::object time_obj = steal(PyObject_CallMethod(obj, "isoformat", "s", "microseconds"));
422 if (!time_obj) throw py::error_already_set();
423 if (!PyUnicode_Check(time_obj.ptr())) {
! 424 throw py::type_error("datetime.time.isoformat() must return a str");
! 425 }
426 Py_ssize_t time_len = PyUnicode_GET_LENGTH(time_obj.ptr());
427 info.columnSize = std::max<SQLULEN>(info.columnSize, time_len);
428 // PyList_SetItem (lowercase) decrefs the old slot before stealing the new
429 // reference; safe here because cursor.py already passed a fresh list copy.Lines 427-436 427 info.columnSize = std::max<SQLULEN>(info.columnSize, time_len);
428 // PyList_SetItem (lowercase) decrefs the old slot before stealing the new
429 // reference; safe here because cursor.py already passed a fresh list copy.
430 if (PyList_SetItem(params, i, time_obj.release().ptr()) != 0) {
! 431 throw py::error_already_set();
! 432 }
433 continue;
434 }
435
436 // --- Decimal ---Lines 452-461 452 py::object digits_obj = steal(PyObject_GetAttrString(as_tuple_ptr.ptr(), "digits"));
453 if (!digits_obj) throw py::error_already_set();
454
455 if (!PyTuple_Check(digits_obj.ptr())) {
! 456 throw py::type_error("Decimal.as_tuple().digits must be a tuple");
! 457 }
458
459 Py_ssize_t num_digits = PyTuple_GET_SIZE(digits_obj.ptr());
460
461 // Read the exponent at full width and range-check it BEFORE narrowing to int.Lines 465-478 465 // any precision, so treat overflow as precision overflow rather than propagating
466 // OverflowError, matching what the legacy Python path reports.
467 long long exponent_ll = PyLong_AsLongLong(exponent_obj.ptr());
468 if (exponent_ll == -1 && PyErr_Occurred()) {
! 469 PyErr_Clear();
! 470 throw py::value_error(
! 471 "Precision of the numeric value is too high. "
! 472 "The maximum precision supported by SQL Server is " +
! 473 std::to_string(MAX_NUMERIC_PRECISION) + ".");
! 474 }
475 // Bound before any arithmetic or negation. MAX_NUMERIC_PRECISION on both sides is
476 // wider than anything bindable, and keeps -exponent well clear of INT_MIN, whose
477 // negation would be signed-overflow UB.
478 if (exponent_ll > MAX_NUMERIC_PRECISION || exponent_ll < -MAX_NUMERIC_PRECISION) {Lines 503-516 503 else
504 precision = -exponent;
505
506 if (precision > MAX_NUMERIC_PRECISION) {
! 507 throw py::value_error(
! 508 "Precision of the numeric value is too high. "
! 509 "The maximum precision supported by SQL Server is " +
! 510 std::to_string(MAX_NUMERIC_PRECISION) + ", but got " +
! 511 std::to_string(precision) + ".");
! 512 }
513
514 // Check SMALLMONEY first, then widen to MONEY, so common small values keep the narrowest
515 // exact range while still accepting larger fixed-point values supported by SQL Server.
516 // MONEY/SMALLMONEY: SQL Server stores these as fixed-point integers internally.Lines 542-551 542 PyObject* raw = formatted.release().ptr();
543 if (PyList_SetItem(params, i, raw) != 0) {
544 // PyList_SetItem steals (decrefs) the item even on failure,
545 // so raw is already freed — do NOT Py_DECREF here.
! 546 throw py::error_already_set();
! 547 }
548 continue;
549 }
550
551 // Build SQL_NUMERIC_STRUCT from the Decimal object. Store as a pybind11-castableLines 559-568 559 py::object numeric_obj = py::cast(nd);
560 PyObject* raw = numeric_obj.release().ptr();
561 if (PyList_SetItem(params, i, raw) != 0) {
562 // PyList_SetItem steals (decrefs) the item even on failure.
! 563 throw py::error_already_set();
! 564 }
565 continue;
566 }
567
568 // --- UUID ---Lines 576-585 576 info.columnSize = 16;
577 info.decimalDigits = 0;
578 if (PyList_SetItem(params, i, bytes_le) != 0) {
579 // PyList_SetItem steals (decrefs) the item even on failure.
! 580 throw py::error_already_set();
! 581 }
582 continue;
583 }
584
585 // --- Unknown type: raise TypeError (matches Python _map_sql_type) ---Lines 608-617 608 int sign_val = static_cast<int>(PyLong_AsLong(sign_obj.ptr()));
609 if (sign_val == -1 && PyErr_Occurred()) throw py::error_already_set();
610
611 if (!PyTuple_Check(digits)) {
! 612 throw py::type_error("Decimal.as_tuple().digits must be a tuple");
! 613 }
614
615 // SQL Server precision counts all stored decimal digits, while scale is just the
616 // fractional digits. A positive exponent moves trailing zeros into the integer part;
617 // a negative exponent means scale = -exponent and precision must still cover leadingLines 654-663 654 for (int j = 0; j < exponent; ++j) {
655 overflow |= mul10_add(0);
656 }
657 if (overflow != 0) {
! 658 throw py::value_error("Decimal magnitude exceeds the 16-byte SQL NUMERIC capacity");
! 659 }
660
661 NumericData nd;
662 nd.precision = static_cast<SQLCHAR>(precision);
663 nd.scale = static_cast<SQLSCHAR>(scale);mssql_python/pybind/py_type_cache.hppLines 44-55 44 // type detection in Python and can therefore reach here without the cache being warm;
45 // it can be dropped once that path is removed.
46 inline PyObject* get_cached_class(PyObject* cached, const char* module_name, const char* attr_name) {
47 if (cache_initialized && cached) return cached;
! 48 py::object mod = steal(PyImport_ImportModule(module_name));
! 49 if (!mod) return nullptr;
! 50 return PyObject_GetAttrString(mod.ptr(), attr_name);
! 51 }
52
53 // One-time init. Uses local py::objects so exception cleanup is automatic;
54 // only .release() into globals after ALL acquisitions succeed.
55 inline void initialize() {📋 Files Needing Attention📉 Files with overall lowest coverage (click to expand)mssql_python.pybind.logger_bridge.cpp: 59.2%
mssql_python.pybind.ddbc_bindings.h: 61.5%
mssql_python.pybind.logger_bridge.hpp: 70.8%
mssql_python.pybind.ddbc_bindings.cpp: 75.5%
mssql_python.__init__.py: 77.6%
mssql_python.row.py: 77.6%
mssql_python.ddbc_bindings.py: 79.6%
mssql_python.pybind.connection.connection_pool.cpp: 81.4%
mssql_python.pybind.connection.connection.cpp: 84.3%
mssql_python.logging.py: 85.5%🔗 Quick Links
|
- Comment out use_prepare parameter name (C4100: unreferenced parameter) - Remove unused catch variable name (C4101: unreferenced local variable)
Add explicit null pointer and zero-length guards before memcpy in build_numeric_data to satisfy DevSkim code scanning rule DS121708.
…or 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.
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.
Resolve conflicts in ddbc_bindings.cpp from main's GH-610 work: - Keep both build_numeric_data (this PR) and ResolveNullParamType (main) - Adopt main's BindParameters/BindParameterArray signatures that take SqlHandle& handle; update the SQLExecuteFast_wrap call site to pass *statementHandle so the fast path uses the per-handle NULL describe cache - Migrate SQLExecuteFast_wrap from std::wstring + WStringToSQLWCHAR to std::u16string + reinterpretU16stringAsSqlWChar (main's uniform 16-bit query/param representation), dropping the platform #ifdef in both the prepare path and the DAE wide-char put-data loop Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- 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>
…ny-perf-detect-types
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- 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>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ecuteLegacy 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>
Gaurav Sharma (bewithgaurav)
changed the title
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>
…ility pybind11's type_caster needs copy semantics for std::vector<ParamInfo>& in the legacy path. Provide a copy ctor that Py_XINCREFs dataPtr. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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>
- 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>
… 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<T>(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>
py_ref.hpp existed earlier in this branch as the home for the custom PyPtr wrapper, and was deleted when PyPtr was replaced by py::object. steal() had to land somewhere, so it went into py_type_cache.hpp, which was already using it. borrow() then followed it there. neither belongs in that file: its own first line describes it as a cache of Python type objects and MONEY boundary constants, and these two helpers are neither. restores py_ref.hpp with the reference-adoption helpers and nothing else, and gives py_type_cache.hpp back a description that matches its contents. only ddbc_bindings.cpp includes either header, so the include change is one line. no behavior change. rebuilt and the .so is byte-identical to the previous commit (sha256 43ec909d...), with ddbc_bindings.cpp recompiled and relinked rather than served from cache. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
this PR added roughly 430 lines of new parameter-detection code into ddbc_bindings.cpp, a file that was already 6600 lines. DetectParamTypes, build_numeric_data and the types they produce are the first stage of the execute pipeline and are cohesive enough to read on their own, so they now live in their own header. ddbc_bindings.cpp drops to 6064 lines and only the pieces this PR introduced moved, so no pre-existing code shifts and no other in-flight branch gains a conflict. the SQL Server ODBC constants that sql.h does not expose move from ddbc_bindings.cpp up into ddbc_bindings.h, because both the detection path and the fetch paths need them and the header is included before either. header rather than .cpp on purpose. the build is -O3 with no LTO, so a .cpp boundary is also an inlining boundary, and these helpers run once per parameter per execute. defining them inline in a header keeps them in the including translation unit. once LTO is enabled this can become a normal .cpp. the resulting binary is not quite bit-identical and the reason is worth stating: __text grows 92 bytes and DetectParamTypes gains an out-of-line symbol. previously it sat in an anonymous namespace with exactly one call site, so the compiler inlined it into SQLExecute_wrap and deleted the original; as an inline function with vague linkage it is now emitted once and called. that is one call per execute(), not per parameter, against a roughly 300us execute. build_numeric_data, which does run per decimal parameter, was already out-of-line before this change and still is: the only difference in its symbol is the mangled name losing the anonymous-namespace prefix. no other symbol changed. 1922 tests pass, the refcount harness reports zero drift across all 15 parameter cases over 300 executes, and the 7 DAE round-trip cases remain byte-exact. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…oval the two execute entry points are DDBCSQLExecute and DDBCSQLExecuteLegacy, so the surrounding code should read standard versus legacy. cursor.py still called the non-legacy branch use_fast_path, which named a third thing that does not exist and left the reader guessing which C++ function it reached. renamed to use_standard_execute, and the parity test file follows: test_023_fast_path_parity.py becomes test_023_execute_path_parity.py, with _fast_path_roundtrip becoming _standard_roundtrip. every legacy site now says out loud that it is temporary and why it still exists. the legacy branch survives only for setinputsizes() callers, whose explicit type overrides the native path does not yet honour; that is the single thing blocking its removal, and it was not written down anywhere. annotated in cursor.py at the branch and the call, on SQLExecuteLegacy_wrap, on the DDBCSQLExecuteLegacy binding, and on the PyTypeCache import fallback that exists only because the legacy path can run before the cache is warm. _create_parameter_types_list gets a fuller docstring rather than a removal note, because it has two callers and only one of them is legacy: executemany() still needs it and will keep needing it until columnwise detection is native too. calling it simply legacy would have been wrong. left alone: the 'Fast path: Data fits in buffer' comments in ddbc_bindings.h and the ASCII-prefix fast path in test_002 and test_014. same words, unrelated concept, pre-existing. comments and identifiers only, no logic touched. 1922 tests pass and the renamed parity file runs all 51 of its tests. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Jahnvi Thakkar (jahnvi480)
requested changes
Aug 4, 2026
…t for time
two parity divergences from jahnvi480's review, both of which bound wrong data silently rather than failing.
the Decimal exponent was cast to int before it was validated. Decimal exponents are arbitrary precision, so on LP64 Decimal('1E+4294967297') truncated to 1, passed the precision <= 38 gate, and bound 10. Decimal('1E+2147483648') truncated to exactly INT_MIN and bound 0.1, and negating INT_MIN a few lines later is signed-overflow UB. the legacy Python path computes precision in arbitrary-precision ints and raises for both. the exponent is now read with PyLong_AsLongLong and bounded, along with the digit count, before any narrowing or arithmetic; an overflow from that read is reported as precision overflow rather than leaking OverflowError.
the time path hand-formatted HH:MM:SS.ffffff from the raw fields, which dropped tzinfo and ignored isoformat overrides on subclasses. an aware time whose isoformat is 01:02:03.000004+05:30 bound as 01:02:03.000004, a different time than the caller passed. it now calls isoformat(timespec='microseconds'), which is what _normalize_time_param does on the legacy side. SQL Server TIME has no offset so both paths now raise DataError for an aware time, verified against legacy auto-detection through executemany.
also finishes the fast_path rename from 2ecda91, which left four SQLExecuteFast strings in error messages and a stale comment in ddbc_bindings.cpp, plus six references in the parity test file.
12 tests added covering 2**32+1, INT_MIN, INT_MAX and their negatives, the 37 and -38 exponents that must still bind, and the aware/naive time pair. verified as real guards: reverting the header and rebuilding fails exactly the 2**32+1, INT_MIN and aware-time cases. 1934 tests pass, refcount harness reports zero drift.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ithub.com/microsoft/mssql-python into bewithgaurav/insertmany-perf-detect-types
The native detector resolved its text C type to SQL_C_WCHAR on Linux/macOS but to a real SQL_C_CHAR (1) on Windows. The legacy Python path binds text with the Python layer's SQL_C_CHAR constant, which is numerically -8, i.e. ODBC's SQL_C_WCHAR, so the legacy path has always bound text wide on every platform. Windows was therefore the only place where the two paths disagreed on C type and on the driver-side encoding path they took, and it was also the one combination CI never compared against a passing wide-bound baseline. Bind wide everywhere. Three call sites share the constant: ASCII strings (inline and DAE), datetime.time normalized to text, and MONEY-range Decimals formatted to text, so all three change on Windows only. Adds round-trip tests over ASCII, non-ASCII, inline/DAE boundary strings, NVARCHAR conversion, time and MONEY, so a reintroduced narrow binding shows up as a Windows-only failure. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The native detector raises ValueError for NaN, sNaN and Infinity. The legacy Python path instead set precision=38 and carried on, so the failure happened incidentally and with a different type each time: NaN raised decimal.InvalidOperation from the MONEY range comparison in _map_sql_type, while Infinity reached _get_numeric_data and raised TypeError from comparing a str exponent against an int. Callers writing `except ValueError` saw different behaviour depending on whether setinputsizes happened to be set. Raise ValueError with the same message in both _map_sql_type and _get_numeric_data. _get_numeric_data needs its own check because executemany's typing pass reaches it directly. Tightens the existing rejection tests from `raises(Exception)` to the exact type, and adds a parity test asserting both paths raise ValueError for all five non-finite forms. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The old parity suite claimed to compare native (C++ DetectParamTypes) against legacy (Python _map_sql_type) detection, but its "legacy" helper reached the legacy path via setinputsizes — which supplies explicit types and bypasses _map_sql_type entirely. So it compared native detection against types hardcoded in the test, never ran the Python detector, and asserted on the round-tripped value, which SQL Server coercion can mask. Every real divergence found on this PR (geometry >4000, aware time, Windows narrow binding) was found by reading code; the suite was green through all of them. Coverage confirmed it: _map_sql_type's body (lines 431-719) and _get_numeric_data sat in the Missing list. Drop the forcing. Test each path through the door real callers use: - Native path: end-to-end via cursor.execute(), unchanged. - Python detection: assert _map_sql_type(value, [value], 0) directly as a pure function returning the 5-tuple (SQL type, C type, column size, decimal digits, DAE) — no DB round-trip, so coercion can't hide a wrong type. Covers every branch: int widths, float, decimal money/numeric, uuid, ascii/unicode inline/DAE strings, geometry, binary, date/datetime/time. - _get_numeric_data: direct precision/scale and overflow assertions. - Legacy execute path (DDBCSQLExecuteLegacy): kept, reached through its only real entry point (setinputsizes), used for what it is for — user-supplied type overrides — plus a shorter-than-params case that exercises the _map_sql_type fallback in _create_parameter_types_list. The long-POLYGON case pins the legacy contract and notes the native side still diverges (a known open item, not fixed here) so the gap stays visible. Net: this file's cursor.py coverage rises 30% -> 39%; 85 -> 128 tests. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…g kinds Native geometry detection had two gaps the legacy path does not: 1. It was gated behind !info.isDAE, so a POINT/LINESTRING/POLYGON over 4000 UTF-16 units never got the geometry treatment — it fell through to the generic long-string DAE path as SQL_VARCHAR instead of NVARCHAR. 2. It only inspected the 1-byte (ASCII) storage kind, so a WKT string carrying any non-ASCII char (stored by CPython in a wider UCS-2/4 kind) was missed and bound as VARCHAR. The legacy path uses str.startswith, which is kind-independent. Detect the geometry prefix kind-agnostically (new StartsWithAscii reads code points via PyUnicode_READ) and fold the result into is_unicode BEFORE the length/DAE branch, so geometry is always NVARCHAR in both size regimes. Deliberately not a literal match to the legacy tuple: legacy _map_sql_type returns NVARCHAR with columnSize == len and DAE=false even for a 7790-char polygon, which is unbindable — SQLBindParameter rejects a non-MAX NVARCHAR precision > 4000 with "Invalid precision value". Folding into is_unicode keeps geometry wide while the existing length gate streams large values via DAE, which actually binds and round-trips. A test pins that legacy defect so it stays visible; native is verified via sql_variant BaseType (small + unicode-tagged) and a >4000 round-trip through a real geometry column. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Two changes in the native SQLExecute DAE loop (the legacy DDBCSQLExecuteLegacy loop is left alone since it is slated for removal). Zero-copy bytes: the loop copied the whole payload into a std::string before streaming, even for immutable bytes. matchedInfo->dataPtr already holds a strong ref to the object for the duration, and bytes cannot be mutated, so the buffer is stable across the GIL release. Stream straight from PyBytes_AS_STRING / PyBytes_GET_SIZE and drop the copy. This is the large-blob path, so it avoids a full payload copy per DAE bytes param. bytearray keeps its copy because it is mutable across the GIL release. Token cast-back: SQLParamData returns the ¶mInfos[i] token we handed to SQLBindParameter. paramInfos is sized up front and never reallocated, so the token casts straight back to its ParamInfo instead of a linear scan of every param per chunk. A range + alignment check keeps a bogus token throwing instead of dereferencing arbitrary memory. Verified: large bytes (incl. embedded NULs), bytearray, large unicode string, and multiple DAE params in one execute all round-trip; full suite 2083 passed. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Both are self-review cleanups on code added earlier in this PR, no behaviour change. Full suite green (2184 passed). StartsWithAscii now deduces the prefix length from the string literal via a template non-type parameter instead of a hand-passed count, so the POINT / LINESTRING / POLYGON call sites can no longer drift a length out of sync with the literal. The DAE token cast-back drops the element-alignment modulo check and keeps only the range check. SQLParamData returns the exact ¶mInfos[i] pointer we handed SQLBindParameter, so a valid token is always element-aligned; the modulo could only ever matter for an already-corrupt token, which the range check already rejects. Removing it drops three lines of defense against an unreachable state. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
|
Nice work! I tried to trace the refcounting (steal/borrow RAII), GIL release around ODBC calls, DAE streaming, and legacy parity; it all holds up. One optional, non-blocking follow-up: |
Sumit Sarabhai (sumitmsft)
previously approved these changes
Aug 13, 2026
Jahnvi Thakkar (jahnvi480)
previously approved these changes
Aug 13, 2026
gargsaumya
previously approved these changes
Aug 13, 2026
gargsaumya
left a comment
gargsaumya
left a comment
Contributor
There was a problem hiding this comment.
LGTM. Earlier reviews covered the substantive items, approved.
…both paths
A Python int past signed int64 range (e.g. 2**63) was classified as SQL BIGINT
at detect time, then failed deep in binding at param.cast<int64_t>() with an
opaque pybind11 error ("Unable to cast Python instance of type <class 'int'> to
C++ type '?'"). SQL Server has no integer type wider than BIGINT, so these can
never bind.
Reject them at detection with a clear ValueError, identical message on both
paths:
- Native DetectParamTypes: the int-overflow branch now raises
"integer <n> is out of range for SQL BIGINT [-2^63, 2^63-1]" instead of
mislabelling as BIGINT. PyObject_Str failure (CPython's int->str digit limit
for very long ints) falls back to a placeholder and clears the error, so we
still raise our own ValueError. A genuine (non-overflow) Python error from
PyLong_AsLongLongAndOverflow now propagates instead of being swallowed.
- Legacy _map_sql_type: mirrors the same check and message before the
INT -> BIGINT return, using new BIGINT_MIN/BIGINT_MAX constants. Handles the
executemany column min/max case.
Removes two dead range guards in BindParameters (signed and unsigned): each
compared an already-cast fixed-width int against its own type limits, which is
always false and unreachable because the cast throws first. A setinputsizes()
override that forces SQL_C_SBIGINT on an out-of-range int still fails the cast,
unchanged by this commit.
Tightens test_integer_overflow_detected to assert the exact ValueError message
on both paths, and adds a boundary test that +/-2^63 still bind. Full suite
2185 passed.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Gaurav Sharma (bewithgaurav)
dismissed stale reviews from gargsaumya, Jahnvi Thakkar (jahnvi480), and Sumit Sarabhai (sumitmsft)
via
August 13, 2026 10:17
9de1c9a
Jahnvi Thakkar (jahnvi480)
approved these changes
Aug 13, 2026
Sumit Sarabhai (sumitmsft)
approved these changes
Aug 13, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Work Item / Issue Reference
Summary
Moves parameter type detection and binding from Python into a native C++ pipeline using raw CPython API calls. The new
DDBCSQLExecutehandles type detection → parameter binding → SQLExecute in a single FFI crossing, eliminating per-parameter Python overhead entirely.What changed:
DetectParamTypes— C++ type detection using raw CPython API (PyLong_Check,PyDateTime_Check,PyObject_RichCompareBool, etc.) replacing the Python-side_create_parameter_types_listloop for the standard execute path.DDBCSQLExecute(formerlyDDBCSQLExecuteFast) — single C++ pipeline: detect → bind → execute. ParamInfo never crosses the pybind11 boundary.DDBCSQLExecuteLegacy(formerlyDDBCSQLExecute) — retained forsetinputsizesusers only, and annotated in-code as slated for removal once those overrides are handled natively.PyTypeCache(py_type_cache.hpp) stores all type objects as rawPyObject*(notpy::object), eliminating pybind11 wrapper overhead on every cache hit.uint32limbs instead of walking every Decimal digit throughPyNumber_Multiply/PyNumber_Add. SQL Server caps NUMERIC at 38 digits, so the mantissa always fits 128 bits. Decimal detection measured 2.3–2.9x faster on its own.ParamInfo::dataPtrholds apy::objectrather than a rawPyObject*with a hand-written rule of five, andpy_ref.hppprovidessteal/borrowso every adoption of a CPython reference states whether it is taking a new reference or borrowing one.param_detect.hpprather than inline in the 6600-lineddbc_bindings.cpp. Header-only because the build is-O3with no LTO, so a.cppboundary would also be an inlining boundary.Routing (cursor.py):
Performance Results 🚀
The Python-side type detection cost was ~2.0–2.3µs per parameter — an
isinstancecheck,ParamInfoobject construction, and a pybind11 FFI boundary crossing per parameter, per execute call. The C++ path replaces this with ~35ns/param (rawPyLong_Check+ struct field write) — a ~60x faster per-parameter detection.macOS arm64 (Apple Silicon M-series), Python 3.13
Linux aarch64 (Docker container), Python 3.13
vs pyodbc (post-PR, macOS)
Customer scenarios (end to end, macOS arm64)
The numbers above isolate driver overhead. These are whole insert workloads, so they also carry the network round trip and SQL Server actually writing the rows, which this PR does not change and which dilutes the percentage. Measured against the merge base (
d94debd) with the two builds interleaved across 3 rounds, 7 iterations each, first 2 discarded.Between them these cover every parameter type whose detection moved: int, varchar, decimal, datetime2, uuid, and the nvarchar(max) DAE streaming path.
Single-row execute does not move, and that is the expected result rather than a disappointment. At roughly 420µs per call the cost is the network round trip; detection for 4 parameters was only ever about 9µs of it. The gain scales with parameters per execute, so batched and wide-row work benefits and one-row-at-a-time work stays where it was.
Bottom line
Checklist