PERF: Native C++ parameter detection and execute pipeline#549
Open
bewithgaurav wants to merge 26 commits into
Open
PERF: Native C++ parameter detection and execute pipeline#549bewithgaurav wants to merge 26 commits into
bewithgaurav wants to merge 26 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 171-184 171 }
172
173 // Fall back to import here because legacy paths can reach these lookups before initialize_cache() has populated globals.
174 static PyObject* get_cached_class(PyObject* cached, const char* module_name, const char* attr_name) {
! 175 if (cache_initialized && cached) return cached;
! 176 PyObject* mod = PyImport_ImportModule(module_name);
! 177 if (!mod) return nullptr;
! 178 PyObject* cls = PyObject_GetAttrString(mod, attr_name);
! 179 Py_DECREF(mod);
! 180 return cls;
181 }
182
183 // One-time cache of Python type objects and MONEY boundary constants.
184 // Called on first execute(). Uses raw CPython API (not pybind11) becauseLines 187-196 187 // unnecessary ref-count traffic on every parameter.
188 void initialize() {
189 if (!cache_initialized) {
190 PyDateTime_IMPORT;
! 191 if (PyDateTimeAPI == nullptr) {
! 192 throw py::error_already_set();
193 }
194
195 try {
196 // Cache datetime.datetime, datetime.date, datetime.time for isinstanceLines 201-210 201 datetime_class = PyObject_GetAttrString(datetime_module, "datetime");
202 date_class = PyObject_GetAttrString(datetime_module, "date");
203 time_class = PyObject_GetAttrString(datetime_module, "time");
204 Py_DECREF(datetime_module);
! 205 if (!datetime_class || !date_class || !time_class) {
! 206 throw py::error_already_set();
207 }
208
209 decimal_class = import_attr("decimal", "Decimal");
210 uuid_class = import_attr("uuid", "UUID");Lines 217-249 217 smallmoney_min = PyObject_CallFunction(Decimal, "s", "-214748.3648");
218 smallmoney_max = PyObject_CallFunction(Decimal, "s", "214748.3647");
219 money_min = PyObject_CallFunction(Decimal, "s", "-922337203685477.5808");
220 money_max = PyObject_CallFunction(Decimal, "s", "922337203685477.5807");
! 221 if (!smallmoney_min || !smallmoney_max || !money_min || !money_max) {
! 222 throw py::error_already_set();
223 }
224
225 cache_initialized = true;
! 226 } catch (...) {
! 227 Py_XDECREF(datetime_class);
! 228 Py_XDECREF(date_class);
! 229 Py_XDECREF(time_class);
! 230 Py_XDECREF(decimal_class);
! 231 Py_XDECREF(uuid_class);
! 232 Py_XDECREF(smallmoney_min);
! 233 Py_XDECREF(smallmoney_max);
! 234 Py_XDECREF(money_min);
! 235 Py_XDECREF(money_max);
! 236 datetime_class = nullptr;
! 237 date_class = nullptr;
! 238 time_class = nullptr;
! 239 decimal_class = nullptr;
! 240 uuid_class = nullptr;
! 241 smallmoney_min = nullptr;
! 242 smallmoney_max = nullptr;
! 243 money_min = nullptr;
! 244 money_max = nullptr;
! 245 throw;
246 }
247 }
248 }Lines 343-389 343 Py_XINCREF(dataPtr);
344 }
345 // Copy/move assignment and move constructor below are required for
346 // std::vector<ParamInfo> resize/reallocation and pybind11's type_caster.
! 347 // They manually manage dataPtr refcounts since ParamInfo owns a strong ref.
! 348 ParamInfo& operator=(const ParamInfo& other) {
! 349 if (this != &other) {
! 350 Py_XDECREF(dataPtr);
! 351 inputOutputType = other.inputOutputType;
! 352 paramCType = other.paramCType;
! 353 paramSQLType = other.paramSQLType;
! 354 columnSize = other.columnSize;
! 355 decimalDigits = other.decimalDigits;
! 356 strLenOrInd = other.strLenOrInd;
! 357 isDAE = other.isDAE;
! 358 dataPtr = other.dataPtr;
! 359 utf16Len = other.utf16Len;
! 360 Py_XINCREF(dataPtr);
! 361 }
! 362 return *this;
363 }
! 364 ParamInfo(ParamInfo&& other) noexcept
! 365 : inputOutputType(other.inputOutputType), paramCType(other.paramCType),
! 366 paramSQLType(other.paramSQLType), columnSize(other.columnSize),
! 367 decimalDigits(other.decimalDigits), strLenOrInd(other.strLenOrInd),
! 368 isDAE(other.isDAE), dataPtr(other.dataPtr), utf16Len(other.utf16Len) {
! 369 other.dataPtr = nullptr;
! 370 }
! 371 ParamInfo& operator=(ParamInfo&& other) noexcept {
! 372 if (this != &other) {
! 373 Py_XDECREF(dataPtr);
! 374 inputOutputType = other.inputOutputType;
! 375 paramCType = other.paramCType;
! 376 paramSQLType = other.paramSQLType;
! 377 columnSize = other.columnSize;
! 378 decimalDigits = other.decimalDigits;
! 379 strLenOrInd = other.strLenOrInd;
! 380 isDAE = other.isDAE;
! 381 dataPtr = other.dataPtr;
! 382 utf16Len = other.utf16Len;
! 383 other.dataPtr = nullptr;
! 384 }
! 385 return *this;
386 }
387 };
388 #ifdef __GNUC__
389 #pragma GCC diagnostic popLines 590-598 590 SQLDescribeParamFunc SQLDescribeParam_ptr = nullptr;
591
592 namespace {
593
! 594
595 const char* GetSqlCTypeAsString(const SQLSMALLINT cType) {
596 switch (cType) {
597 STRINGIFY_FOR_CASE(SQL_C_CHAR);
598 STRINGIFY_FOR_CASE(SQL_C_WCHAR);Lines 916-925 916 Py_ssize_t time_len = PyUnicode_GET_LENGTH(time_str);
917 info.columnSize = std::max<SQLULEN>(info.columnSize, time_len);
918 // PyList_SetItem (lowercase) decrefs the old slot before stealing the new
919 // reference; safe here because cursor.py already passed a fresh list copy.
! 920 if (PyList_SetItem(params, i, time_str) != 0) {
! 921 throw py::error_already_set();
922 }
923 continue;
924 }Lines 945-954 945 PyObject* digits_obj = PyObject_GetAttrString(as_tuple, "digits");
946 if (!digits_obj) throw py::error_already_set();
947 guard.track(digits_obj);
948
! 949 if (!PyTuple_Check(digits_obj)) {
! 950 throw py::type_error("Decimal.as_tuple().digits must be a tuple");
951 }
952
953 Py_ssize_t num_digits = PyTuple_GET_SIZE(digits_obj);
954 int exponent = static_cast<int>(PyLong_AsLong(exponent_obj));Lines 1006-1015 1006 guard.release(digits_obj);
1007 Py_DECREF(digits_obj);
1008 guard.release(as_tuple);
1009 Py_DECREF(as_tuple);
! 1010 if (PyList_SetItem(params, i, formatted) != 0) {
! 1011 throw py::error_already_set();
1012 }
1013 continue;
1014 }Lines 1026-1036 1026 info.decimalDigits = nd.scale;
1027 // Store NumericData as a Python object in the param list for the binder.
1028 py::object numeric_obj = py::cast(nd);
1029 PyObject* raw = numeric_obj.release().ptr();
! 1030 if (PyList_SetItem(params, i, raw) != 0) {
! 1031 Py_DECREF(raw);
! 1032 throw py::error_already_set();
1033 }
1034 continue;
1035 }Lines 1043-1053 1043 info.paramSQLType = SQL_GUID;
1044 info.paramCType = SQL_C_GUID;
1045 info.columnSize = 16;
1046 info.decimalDigits = 0;
! 1047 if (PyList_SetItem(params, i, bytes_le) != 0) {
! 1048 Py_DECREF(bytes_le);
! 1049 throw py::error_already_set();
1050 }
1051 continue;
1052 }Lines 1094-1103 1094 }
1095 guard.release(exponent_obj);
1096 Py_DECREF(exponent_obj);
1097
! 1098 if (!PyTuple_Check(digits_obj)) {
! 1099 throw py::type_error("Decimal.as_tuple().digits must be a tuple");
1100 }
1101
1102 // Step 5: SQL Server precision counts all stored decimal digits, while scale is just the fractional
1103 // digits. A positive exponent means trailing zeros move into the integer part; a negative exponentLines 1119-1128 1119 PyObject* py_ten = PyLong_FromLong(10);
1120 PyObject* int_val = PyLong_FromLong(0);
1121 guard.track(py_ten);
1122 guard.track(int_val);
! 1123 if (!py_ten || !int_val) {
! 1124 throw py::error_already_set();
1125 }
1126
1127 const Py_ssize_t digit_count = PyTuple_GET_SIZE(digits_obj);
1128 for (Py_ssize_t i = 0; i < digit_count; ++i) {Lines 1195-1204 1195 if (!val_bytes) throw py::error_already_set();
1196
1197 char* val_buf = nullptr;
1198 Py_ssize_t val_size = 0;
! 1199 if (PyBytes_AsStringAndSize(val_bytes, &val_buf, &val_size) == -1) {
! 1200 throw py::error_already_set();
1201 }
1202
1203 // Step 10: pack precision/scale plus the 16-byte little-endian magnitude. SQL uses sign=1 for
1204 // positive and sign=0 for negative, which is the inverse of Decimal.as_tuple().sign.Lines 1477-1486 1477 dataPtr = sqlwcharBuffer->data();
1478 bufferLength = sqlwcharBuffer->size() * sizeof(SQLWCHAR);
1479 strLenOrIndPtr = AllocateParamBuffer<SQLLEN>(paramBuffers);
1480 // Use explicit byte length instead of SQL_NTS so embedded NUL chars
! 1481 // aren't treated as string terminators.
! 1482 *strLenOrIndPtr = static_cast<SQLLEN>(sqlwcharBuffer->size() * sizeof(SQLWCHAR));
1483 }
1484 break;
1485 }
1486 case SQL_C_BIT: {Lines 1619-1627 1619 dataPtr = static_cast<void*>(sqlTimePtr);
1620 break;
1621 }
1622 case SQL_C_SS_TIMESTAMPOFFSET: {
! 1623 py::object datetimeType = PythonObjectCache::get_datetime_class_obj();
1624 if (!py::isinstance(param, datetimeType)) {
1625 ThrowStdException(MakeParamMismatchErrorStr(paramInfo.paramCType, paramIndex));
1626 }
1627 // Checking if the object has a timezoneLines 2715-2724 2715 reinterpretU16stringAsSqlWChar(utf16),
2716 utf16.size() * sizeof(SQLWCHAR),
2717 putData);
2718 if (!SQL_SUCCEEDED(rc)) {
! 2719 LOG("SQLExecute: SQLPutData failed for SQL_C_WCHAR DAE streaming");
! 2720 return rc;
2721 }
2722 } else if (matchedInfo->paramCType == SQL_C_CHAR) {
2723 // Encode the string using the specified encoding
2724 std::string encodedStr;Lines 2810-2825 2810 SQLHANDLE hStmt = statementHandle->get();
2811
2812 // Configure forward-only / read-only cursor (matches slow path semantics).
2813 if (SQLSetStmtAttr_ptr) {
! 2814 SQLSetStmtAttr_ptr(hStmt, SQL_ATTR_CURSOR_TYPE,
! 2815 (SQLPOINTER)SQL_CURSOR_FORWARD_ONLY, 0);
2816 SQLSetStmtAttr_ptr(hStmt, SQL_ATTR_CONCURRENCY,
2817 (SQLPOINTER)SQL_CONCUR_READ_ONLY, 0);
! 2818 }
! 2819
! 2820 // The encoding-settings dict has the form {"encoding": str, "ctype": int}.
! 2821 // Note: the Python layer's SQL_C_CHAR constant is numerically -8, the same
2822 // as ODBC's SQL_C_WCHAR. As a result, the only path that genuinely uses
2823 // byte-level character encoding is when the user explicitly opts in via
2824 // setencoding(..., ctype=mssql_python.SQL_CHAR) (which sends ctype=1, the
2825 // real ODBC SQL_CHAR). We default to utf-8 and only honor the dict'sLines 2829-2845 2829 if (encoding_settings.contains("ctype") && encoding_settings.contains("encoding")) {
2830 int ctype = encoding_settings["ctype"].cast<int>();
2831 if (ctype == SQL_C_CHAR /* real ODBC value: 1 */) {
2832 charEncoding = encoding_settings["encoding"].cast<std::string>();
! 2833 }
! 2834 }
! 2835
! 2836 // The cursor.py caller always passes a fresh `list(actual_params)` so this
! 2837 // function is free to mutate slots in place. Even so, every site below uses
! 2838 // PyList_SetItem (which decrefs the old slot before stealing the new ref),
! 2839 // so the function is safe regardless of who owns the list.
! 2840
! 2841 // Run DetectParamTypes BEFORE SQLPrepare so that type-detection errors
2842 // (unsupported type, NaN Decimal, precision overflow) don't leave the
2843 // cursor in a half-prepared state.
2844 std::vector<ParamInfo> paramInfos = DetectParamTypes(params.ptr());Lines 2860-2869 2860 if (!SQL_SUCCEEDED(rc)) return rc;
2861 statementHandle->clearDescribeCache();
2862 is_stmt_prepared[0] = py::bool_(true);
2863 } else {
! 2864 ThrowStdException("Cannot execute unprepared statement");
! 2865 }
2866 }
2867
2868 std::vector<std::shared_ptr<void>> paramBuffers;
2869 rc = BindParameters(*statementHandle, hStmt, params, paramInfos, paramBuffers, charEncoding);Lines 2891-2901 2891 }
2892 if (rc != SQL_NEED_DATA) break;
2893
2894 const ParamInfo* matchedInfo = nullptr;
! 2895 for (auto& info : paramInfos) {
! 2896 if (reinterpret_cast<SQLPOINTER>(const_cast<ParamInfo*>(&info)) == paramToken) {
! 2897 matchedInfo = &info;
2898 break;
2899 }
2900 }
2901 if (!matchedInfo) {Lines 2903-2919 2903 }
2904 PyObject* pyObj = matchedInfo->dataPtr;
2905 if (!pyObj || pyObj == Py_None) {
2906 py::gil_scoped_release release;
! 2907 SQLPutData_ptr(hStmt, nullptr, 0);
! 2908 continue;
! 2909 }
! 2910
! 2911 if (PyUnicode_Check(pyObj)) {
2912 if (matchedInfo->paramCType == SQL_C_WCHAR) {
2913 std::u16string u16 =
2914 py::reinterpret_borrow<py::str>(py::handle(pyObj)).cast<std::u16string>();
! 2915 rc = stream_dae_chunks(
2916 reinterpretU16stringAsSqlWChar(u16),
2917 u16.size() * sizeof(SQLWCHAR),
2918 putData);
2919 if (!SQL_SUCCEEDED(rc)) return rc;Lines 2917-2925 2917 u16.size() * sizeof(SQLWCHAR),
2918 putData);
2919 if (!SQL_SUCCEEDED(rc)) return rc;
2920 } else if (matchedInfo->paramCType == SQL_C_CHAR) {
! 2921 std::string encodedStr;
2922 py::object encoded = py::reinterpret_borrow<py::object>(py::handle(pyObj))
2923 .attr("encode")(charEncoding, "strict");
2924 encodedStr = encoded.cast<std::string>();
2925 rc = stream_dae_chunks(encodedStr.data(), encodedStr.size(), putData);Lines 2937-2945 2937 if (PyBytes_Check(pyObj)) {
2938 bytesStorage = py::reinterpret_borrow<py::bytes>(py::handle(pyObj));
2939 dataPtr = bytesStorage.data();
2940 totalBytes = bytesStorage.size();
! 2941 } else {
2942 // bytearray is mutable — copy to stable buffer before streaming
2943 bytesStorage.assign(PyByteArray_AS_STRING(pyObj),
2944 static_cast<size_t>(PyByteArray_GET_SIZE(pyObj)));
2945 dataPtr = bytesStorage.data();Lines 2953-2964 2953 }
2954 }
2955 if (!SQL_SUCCEEDED(rc) && rc != SQL_NO_DATA) return rc;
2956 }
! 2957
! 2958 if (!SQL_SUCCEEDED(rc) && rc != SQL_NO_DATA) return rc;
! 2959
! 2960 // Unbind parameter buffers before they go out of scope.
2961 // Not called on error paths — diagnostics must remain readable.
2962 SQLRETURN exec_rc = rc;
2963 SQLFreeStmt_ptr(hStmt, SQL_RESET_PARAMS);
2964 return exec_rc;Lines 3502-3510 3502
3503 // Get cached UUID class from module-level helper
3504 // This avoids static object destruction issues during
3505 // Python finalization
! 3506 py::object uuid_class = PythonObjectCache::get_uuid_class_obj();
3507 // Get cached UUID class
3508
3509 for (size_t i = 0; i < paramSetSize; ++i) {
3510 const py::handle& element = columnValues[i];Lines 5215-5223 5215 int totalMinutes = dtoValue.timezone_hour * 60 + dtoValue.timezone_minute;
5216 py::object datetime_module = py::module_::import("datetime");
5217 py::object tzinfo = datetime_module.attr("timezone")(
5218 datetime_module.attr("timedelta")(py::arg("minutes") = totalMinutes));
! 5219 py::object py_dt = PythonObjectCache::get_datetime_class_obj()(
5220 dtoValue.year, dtoValue.month, dtoValue.day, dtoValue.hour,
5221 dtoValue.minute, dtoValue.second,
5222 dtoValue.fraction / 1000, // ns → µs
5223 tzinfo);📋 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: 59.9%
mssql_python.pybind.logger_bridge.hpp: 70.8%
mssql_python.pybind.connection.connection.cpp: 76.2%
mssql_python.pybind.ddbc_bindings.cpp: 76.3%
mssql_python.__init__.py: 77.3%
mssql_python.row.py: 77.6%
mssql_python.ddbc_bindings.py: 79.6%
mssql_python.connection.py: 83.6%
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>
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>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR shifts the primary cursor.execute() pipeline from Python into native C++ by adding a raw-CPython DetectParamTypes fast path and routing calls to a single DDBCSQLExecute FFI entrypoint when setinputsizes isn’t active, targeting large parameter-count performance regressions (GH-500).
Changes:
- Added a native
DetectParamTypes → BindParameters → SQLExecutepipeline (DDBCSQLExecute) to avoid per-parameter Python/pybind11 overhead. - Preserved a legacy path (
DDBCSQLExecuteLegacy) forsetinputsizesusers and updated Python routing accordingly. - Added parity and regression tests covering fast/slow path equivalence, subclass handling, DAE streaming, and refcount safety.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| tests/test_023_fast_path_parity.py | Adds fast-vs-legacy parity tests and regression coverage for the new native execute pipeline. |
| tests/test_010_pybind_functions.py | Updates exposed-function expectations to include DDBCSQLExecuteLegacy. |
| mssql_python/pybind/ddbc_bindings.cpp | Implements native type detection, new execute entrypoints, and various binding/DAE handling updates. |
| mssql_python/cursor.py | Routes execute() to DDBCSQLExecute for the primary path and to DDBCSQLExecuteLegacy when setinputsizes overrides are present. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+2772
to
+2775
| } else { | ||
| dataPtr = PyByteArray_AS_STRING(pyObj); | ||
| totalBytes = static_cast<size_t>(PyByteArray_GET_SIZE(pyObj)); | ||
| } |
…E safety - 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>
- 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>
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>
…_class, import_attr - 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>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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>
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 primary execute path.DDBCSQLExecute(formerlyDDBCSQLExecuteFast) — single C++ pipeline: detect → bind → execute. ParamInfo never crosses the pybind11 boundary.DDBCSQLExecuteLegacy(formerlyDDBCSQLExecute) — retained forsetinputsizesusers only.PythonObjectCachestores all type objects as rawPyObject*(notpy::object), eliminating pybind11 wrapper overhead on every cache hit.Py_DECREFcleanup andPyObject_IsInstanceerror handling on all paths.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)
Bottom line
Checklist