PERF: route setinputsizes through the native C++ execute pipeline - #736
Open
Gaurav Sharma (bewithgaurav) wants to merge 6 commits into
Open
PERF: route setinputsizes through the native C++ execute pipeline#736Gaurav Sharma (bewithgaurav) wants to merge 6 commits into
Gaurav Sharma (bewithgaurav) wants to merge 6 commits into
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot started reviewing on behalf of
Gaurav Sharma (bewithgaurav)
September 1, 2026 19:12
View session
Contributor
There was a problem hiding this comment.
Pull request overview
Moves setinputsizes handling into the native execute pipeline to reduce parameter-binding overhead.
Changes:
- Adds native input-size override handling, normalization, and numeric clamping.
- Removes
DDBCSQLExecuteLegacyand routes execution through native bindings. - Expands execute-path parity tests.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
mssql_python/cursor.py |
Routes parameterized execution through the native path. |
mssql_python/pybind/ddbc_bindings.cpp |
Removes legacy execution and accepts input sizes natively. |
mssql_python/pybind/param_detect.hpp |
Applies native type overrides and value normalization. |
mssql_python/pybind/py_type_cache.hpp |
Simplifies access to initialized type caches. |
tests/test_010_pybind_functions.py |
Updates expected native exports. |
tests/test_023_execute_path_parity.py |
Tests native override behavior and edge cases. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
reject invalid Decimal subclass formatting before replacing the parameter so native execution preserves the Python format contract. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
📊 Code Coverage Report
Diff CoverageDiff: main...HEAD, staged and unstaged changes
Summary
mssql_python/pybind/ddbc_bindings.cppLines 1898-1906 1898
1899 // Run DetectParamTypes BEFORE SQLPrepare so that type-detection errors
1900 // (unsupported type, NaN Decimal, precision overflow) don't leave the
1901 // cursor in a half-prepared state.
! 1902 std::vector<ParamInfo> paramInfos = DetectParamTypes(params.ptr(), input_sizes.ptr());
1903
1904 RETCODE rc;
1905 bool already_prepared = is_stmt_prepared[0].cast<bool>();mssql_python/pybind/param_detect.hppLines 184-193 184 inline bool PyLongGreaterThan(PyObject* value, long long threshold) {
185 int overflow = 0;
186 long long result = PyLong_AsLongLongAndOverflow(value, &overflow);
187 if (result == -1 && PyErr_Occurred()) {
! 188 throw py::error_already_set();
! 189 }
190 return overflow > 0 || (overflow == 0 && result > threshold);
191 }
192
193 inline PyObject* FormatDecimalParam(PyObject* params, Py_ssize_t index, PyObject* value) {Lines 196-205 196 if (!PyUnicode_Check(formatted.ptr())) {
197 throw py::type_error("Decimal.__format__() must return a str");
198 }
199 if (PyList_SetItem(params, index, formatted.release().ptr()) != 0) {
! 200 throw py::error_already_set();
! 201 }
202 return PyList_GET_ITEM(params, index);
203 }
204
205 inline void NormalizeTimeParam(PyObject* params, Py_ssize_t index, SQLULEN& columnSize) {Lines 210-219 210 throw py::type_error("datetime.time.isoformat() must return a str");
211 }
212 columnSize = std::max<SQLULEN>(columnSize, PyUnicode_GET_LENGTH(formatted.ptr()));
213 if (PyList_SetItem(params, index, formatted.release().ptr()) != 0) {
! 214 throw py::error_already_set();
! 215 }
216 }
217
218 inline void ApplyInputSizeOverride(PyObject* params, PyObject* inputSize, Py_ssize_t index,
219 ParamInfo& info) {mssql_python/pybind/py_type_cache.hppLines 62-72 62 uuid_class = uuid_cls.release().ptr();
63 cache_initialized = true;
64 }
65
! 66 inline PyObject* get_datetime_class() { return datetime_class; }
! 67 inline PyObject* get_date_class() { return date_class; }
! 68 inline PyObject* get_time_class() { return time_class; }
69 inline PyObject* get_decimal_class() { return decimal_class; }
70 inline PyObject* get_uuid_class() { return uuid_class; }
71
72 inline py::object get_datetime_class_obj() { return borrow(datetime_class); }📋 Files Needing Attention📉 Files with overall lowest coverage (click to expand)mssql_python.pybind.logger_bridge.cpp: 58.9%
mssql_python.pybind.ddbc_bindings.h: 61.5%
mssql_python.pybind.logger_bridge.hpp: 70.8%
mssql_python.pybind.ddbc_bindings.cpp: 77.5%
mssql_python.__init__.py: 77.6%
mssql_python.row.py: 77.6%
mssql_python.pybind.connection.connection_pool.cpp: 81.6%
mssql_python.pybind.connection.connection.cpp: 84.4%
mssql_python.logging.py: 85.5%
mssql_python.connection.py: 85.9%🔗 Quick Links
|
Gaurav Sharma (bewithgaurav)
added a commit
that referenced
this pull request
Sep 2, 2026
### Work Item / Issue Reference > ADO Work Item: Fixed [AB#47799](https://sqlclientdrivers.visualstudio.com/c6d89619-62de-46a0-8b46-70b92a84d85e/_workitems/edit/47799) ------------------------------------------------------------------- ### Summary add a dedicated `setinputsizes` scenario to the existing 100K-row insertmanyvalues benchmark. both mssql-python and pyodbc declare all 2,000 parameter types before every execute, so main publishes a normalized baseline for #736 and later binding changes. the existing insertmanyvalues scenario remains the no-`setinputsizes` control. ------------------------------------------------------------------- ### Local Baseline macOS arm64, Python 3.13, SQL Server 2022, 10 iterations with the first discarded as warmup: | Driver | Median | |--------|--------| | mssql-python | 1.8498s | | pyodbc | 0.3288s | normalized score: **5.63x slower than pyodbc**. Co-authored-by: Gaurav Sharma <223556219+Copilot@users.noreply.github.com>
Gaurav Sharma (bewithgaurav)
marked this pull request as ready for review
September 3, 2026 06:15
Copilot started reviewing on behalf of
Gaurav Sharma (bewithgaurav)
September 3, 2026 06:53
View session
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
Native ODBC binding changes affect cross-platform type conversion, memory ownership, and DAE streaming and require final human validation.
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Gaurav Sharma (bewithgaurav)
pushed a commit
that referenced
this pull request
Sep 3, 2026
add rules learned reviewing #736: verify native-vs-legacy divergence by running both paths, cite call frequency before assigning a zone, distrust normalized scores when pyodbc moved across runners, stay in the given checkout, and label unconfirmed findings as unverified. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Reconcile #742 (bind Decimal as SQL_NUMERIC regardless of value, GH-740) with the native setinputsizes migration: - param_detect.hpp: drop the automatic MONEY/SMALLMONEY VARCHAR shortcut; every finite Decimal binds SQL_NUMERIC natively. FormatDecimalParam stays for the setinputsizes DECIMAL override only. - cursor.py: _create_parameter_types_list forwards decimal_as_numeric to _map_sql_type; the parameterless else-branch keeps DDBCSQLExecDirect and drops the deleted DDBCSQLExecuteLegacy block (GH-740 fix now happens in native detection). - test_023: keep both suites; narrow test_decimal_format_must_return_string to the setinputsizes override, the only path that still formats Decimals after GH-740. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Gaurav Sharma (bewithgaurav)
dismissed
gargsaumya’s stale review
via
September 3, 2026 13:22
bbeabbd
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
move
setinputsizesoverrides into the native detect, bind, and execute path and removeDDBCSQLExecuteLegacy.Performance Results
macOS arm64, Python 3.13, SQL Server 2022, release build
mainlegacy paththe issue #500 workload runs 100 executions with 2,000 declared parameters each (1,000
INT/VARCHARrows), callingsetinputsizes()before every execute. results are the median of three runs and were confirmed with the base/PR execution order reversed.Cross-platform CI
mainmainresults are from build 171915. PR results are from build 172065, except macOS SQL2022, which is from the last completed run in build 171939; the rerun reached the 20-minute benchmark timeout before that scenario completed.these are raw medians. the existing pyodbc-normalized score is not used here because the Windows pyodbc main baseline did not reproduce across hosted runners.