Repository navigation
PERF: Optimize fetchone, fetchmany(1) and fetchval paths - #829
Draft
Jahnvi Thakkar (jahnvi480) wants to merge 18 commits into
Draft
Jahnvi Thakkar (jahnvi480) wants to merge 18 commits into
Jahnvi Thakkar (jahnvi480) wants to merge 18 commits into
Conversation
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
PR Performance Report✅ No regression detectedNo consistent slowdowns detected across all 2 environments. 0 IMPROVEMENTS 0 SLOWDOWNS 2/2 ENVIRONMENTS Coverage: 2 of 2 environments completed. Advisory result; does not block merging. Performance diagnosticsPhase times are inclusive diagnostics and must not be added together. They identify where measured time changed, not why it changed. Unix / SQL Server 2022Row-by-row fetching: ddbc::SQLDescribeCol::driver_call +0.000 ms. Call changes: ddbc::FetchSingleRow::SQL_UNBIND (added, removed, or intermittent); ddbc::SQLNumResultCols_wrap (1000 -> 1 calls). Unix / SQL Server 2025Row-by-row fetching: ddbc::SQLGetData_wrap +0.065 ms; ddbc::AppendDiagRecords::SQLGetDiagRec_call +0.002 ms. Call changes: ddbc::FetchSingleRow::SQL_UNBIND (added, removed, or intermittent); ddbc::SQLNumResultCols_wrap (1000 -> 1 calls). All database tasks and timingsUnix / SQL Server 2022
Unix / SQL Server 2025
Build and measurement detailsPR head:
A consistent change requires more than 20% median paired movement, at least 1 ms between the median runtimes, and at least 80% of pairs exceeding the relative threshold in the same direction. A slowdown without enough pair agreement is reported as inconsistent. The displayed change is the median of paired before-and-after ratios. It is not recalculated from the two displayed median runtimes. Both revisions use profiling-enabled builds on the same agent and database, with alternating order and discarded warmups. Results are diagnostic and do not represent production-wheel latency. This headline uses the original 22-task profiling-enabled diagnostics; separate OFF/OFF latency and ON/OFF route measurements, when available, are retained in the raw artifacts and are not headline inputs. Raw samples and logs are attached to the ADO run as |
Copilot started reviewing on behalf of
Jahnvi Thakkar (jahnvi480)
October 1, 2026 09:00
View session
Contributor
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
The native hot-path change still requires live correctness and performance validation, as acknowledged by the draft description.
Review effort: Balanced
Findings: None
What changed in this PR
Introduces an experimental native cache to avoid repeated column-count queries during fetchone.
Changes:
- Caches full column counts by metadata generation.
- Reuses cached counts while preserving invalidation and error handling.
- Adds nine subprocess-isolated integration scenarios.
| File | Description |
|---|---|
mssql_python/pybind/ddbc_bindings.cpp |
Uses the cached count in FetchOne_wrap. |
mssql_python/pybind/result_metadata.hpp |
Stores and invalidates full column counts. |
tests/test_fetch_settings_cache.py |
Tests reuse, invalidation, failures, and mixed fetching. |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Use a separate connection for the cross-handle assertion without requiring MARS. Preserve all fetch and native call-count assertions. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot started reviewing on behalf of
Jahnvi Thakkar (jahnvi480)
October 1, 2026 10:14
View session
Copilot started reviewing on behalf of
Jahnvi Thakkar (jahnvi480)
October 1, 2026 10:17
View session
📊 Code Coverage Report
Diff CoverageDiff: main...HEAD, staged and unstaged changes
Summary
mssql_python/cursor.pyLines 2898-2906 2898 self.messages,
2899 )
2900 finally:
2901 if started:
! 2902 perf_stop("py::fetchone::cpp_call", started)
2903
2904 return self._finish_fetchone(ret, row_data)
2905 except Exception:
2906 # On error, don't increment rownumber - rethrow the errorLines 2916-2925 2916 return None
2917
2918 # Update internal position after successful fetch
2919 if self._skip_increment_for_next_fetch:
! 2920 self._skip_increment_for_next_fetch = False
! 2921 self._next_row_index += 1
2922 else:
2923 self._increment_rownumber()
2924
2925 self.rowcount = self._next_row_indexLines 2929-2940 2929 started = perf_start()
2930 try:
2931 if not converter_map and not self._uuid_str_indices:
2932 if native and not started and _native_row_eligible(Row):
! 2933 row_type = Row
! 2934 factory = row_type._fast_create
! 2935 column_names = self._cached_result_columns
! 2936 return (
2937 _NATIVE_ROW_PLAN,
2938 row_type,
2939 column_map,
2940 self,Lines 2956-2964 2956 column_names=self._cached_result_columns,
2957 )
2958 finally:
2959 if started:
! 2960 perf_stop("py::fetchone::row_wrap", started)
2961
2962 def fetchmany(self, size: Optional[int] = None) -> List[Row]:
2963 """
2964 Fetch the next set of rows of a query result.Lines 3000-3008 3000 self.messages,
3001 )
3002 finally:
3003 if started:
! 3004 perf_stop("py::fetchmany::cpp_call", started)
3005
3006 return self._finish_fetchmany(ret, rows_data, size=size)
3007 except Exception:
3008 # On error, don't increment rownumber - rethrow the errorLines 3038-3049 3038 and Row is _DEFAULT_ROW_TYPE
3039 and Row._fast_create is _DEFAULT_FAST_ROW_CREATE
3040 ):
3041 if native and not started and _native_row_eligible(Row):
! 3042 row_type = Row
! 3043 factory = row_type._fast_create
! 3044 column_names = self._cached_result_columns
! 3045 return (
3046 _NATIVE_ROW_PLAN,
3047 row_type,
3048 column_map,
3049 self,Lines 3082-3090 3082 for row_data in rows_data
3083 ]
3084 finally:
3085 if started:
! 3086 perf_stop("py::fetchmany::row_wrap", started)
3087
3088 def fetchall(self) -> List[Row]:
3089 """
3090 Fetch all (remaining) rows of a query result.mssql_python/pybind/ddbc_bindings.cppLines 3721-3731 3721 PyErr_SetString(PyExc_ValueError, "embedded null character");
3722 throw py::error_already_set();
3723 }
3724 decoded = steal(PyUnicode_Decode(
! 3725 reinterpret_cast<const char*>(dataBuffer.data()),
! 3726 static_cast<Py_ssize_t>(dataLen),
! 3727 decodeEncoding.c_str(), "strict"));
3728 if (!decoded) throw py::error_already_set();
3729 if (PyList_Append(row.ptr(), decoded.ptr()) < 0)
3730 throw py::error_already_set();
3731 LOG("SQLGetData: CHAR column %d decoded with '%s', %zu bytes "Lines 3740-3750 3740 // Preserve the existing codec-error bytes fallback.
3741 decoded = steal(PyBytes_FromStringAndSize(
3742 reinterpret_cast<const char*>(dataBuffer.data()),
3743 static_cast<Py_ssize_t>(dataLen)));
! 3744 if (!decoded) throw py::error_already_set();
! 3745 if (PyList_Append(row.ptr(), decoded.ptr()) < 0)
! 3746 throw py::error_already_set();
3747 }
3748 } else {
3749 // Buffer too small, fallback to streaming
3750 LOG("SQLGetData: CHAR column %d data truncated "Lines 3812-3822 3812 SQLWCHAR* dataBuffer;
3813 if (bufferChars <= std::size(inlineBuffer)) {
3814 std::fill_n(inlineBuffer, bufferChars, SQLWCHAR{});
3815 dataBuffer = inlineBuffer;
! 3816 } else {
! 3817 heapBuffer.resize(bufferChars);
! 3818 dataBuffer = heapBuffer.data();
3819 }
3820 SQLLEN dataLen;
3821 ret = SQLGetData_ptr(hStmt, i, SQL_C_WCHAR, dataBuffer, fetchBufferSize,
3822 &dataLen);Lines 3834-3842 3834 // null termination. This preserves embedded NULs and avoids
3835 // any risk of reading past the valid range if the driver
3836 // omits the terminator.
3837 row.append(FetchText::from_utf16_native(
! 3838 reinterpret_cast<const char*>(dataBuffer),
3839 static_cast<Py_ssize_t>(numCharsInData * sizeof(SQLWCHAR))));
3840 LOG("SQLGetData: Appended NVARCHAR string "
3841 "length=%lu for column %d",
3842 (unsigned long)numCharsInData, i);Lines 3888-3896 3888 SQLLEN indicator = 0;
3889 ret = SQLGetData_ptr(hStmt, i, SQL_C_LONG, &intValue, 0, &indicator);
3890 CaptureFetchDiagnostics(hStmt, ret, messages);
3891 if (SQL_SUCCEEDED(ret) && indicator != SQL_NULL_DATA) {
! 3892 AppendFetchedCell(row, PyLong_FromLong(intValue));
3893 } else {
3894 row.append(py::none());
3895 }
3896 break;Lines 5468-5476 5468 FetchStateGuard fetchStateGuard(StatementHandle, messages);
5469
5470 if (!hasLobColumns && fetchSize > 0) {
5471 ret = SQLBindColums(StatementHandle, buffers, columnNames, numCols, fetchSize, charCtype,
! 5472 messages);
5473 if (!SQL_SUCCEEDED(ret)) {
5474 LOG("Error when binding columns");
5475 return ret;
5476 }mssql_python/pybind/row_factory.hppLines 14-22 14 const py::handle& attr_cursor, const py::handle& attr_column_map_lower,
15 const py::handle& attr_column_names,
16 int (*set_attr)(PyObject*, PyObject*, PyObject*) = PyObject_GenericSetAttr) {
17 if (!row)
! 18 throw py::error_already_set();
19
20 if (set_attr(row.ptr(), attr_values.ptr(), row_data) < 0 ||
21 set_attr(row.ptr(), attr_column_map.ptr(), column_map.ptr()) < 0 ||
22 set_attr(row.ptr(), attr_cursor.ptr(), cursor_obj.ptr()) < 0 ||Lines 66-99 66 }
67
68 // Passive final eligibility check: do not execute newly installed class descriptors.
69 inline bool has_default_row_allocation(PyObject* row_class, const py::str& new_name,
! 70 const py::str& setattr_name) {
! 71 if (!PyUnicode_CheckExact(new_name.ptr()) || !PyUnicode_CheckExact(setattr_name.ptr())) {
! 72 throw py::type_error("Row allocation guard names must be exact strings");
! 73 }
! 74 if (!row_class || Py_TYPE(row_class) != &PyType_Type) {
! 75 return false;
! 76 }
! 77 auto* row_type = reinterpret_cast<PyTypeObject*>(row_class);
! 78 if (!row_type->tp_bases || !PyTuple_CheckExact(row_type->tp_bases) ||
! 79 PyTuple_GET_SIZE(row_type->tp_bases) != 1 ||
! 80 PyTuple_GET_ITEM(row_type->tp_bases, 0) != reinterpret_cast<PyObject*>(&PyBaseObject_Type) ||
! 81 !row_type->tp_dict) {
! 82 return false;
! 83 }
! 84 PyObject* names[] = {new_name.ptr(), setattr_name.ptr()};
! 85 for (PyObject* name : names) {
! 86 PyObject* member = PyDict_GetItemWithError(row_type->tp_dict, name);
! 87 if (member) {
! 88 return false;
! 89 }
! 90 if (PyErr_Occurred()) {
91 throw py::error_already_set();
92 }
93 }
! 94 return true;
! 95 }
96
97 // Attribute names are prepared once as binding defaults, not allocated for each row.
98 inline py::object construct_row(const py::object& values, const py::type& row_class,
99 const py::object& column_map, const py::object& cursor_obj,Lines 99-112 99 const py::object& column_map, const py::object& cursor_obj,
100 const py::object& column_map_lower, const py::object& column_names,
101 const py::tuple& attributes) {
102 if (attributes.size() != 5) {
! 103 throw py::value_error("Row construction requires five attribute names");
! 104 }
105 for (py::handle name : attributes) {
106 if (!PyUnicode_Check(name.ptr())) {
! 107 throw py::type_error("Row attribute names must be strings");
! 108 }
109 }
110 const py::object new_method = row_class.attr("__new__");
111 py::object row = steal(PyObject_CallOneArg(new_method.ptr(), row_class.ptr()));
112 initialize_row(row, values.ptr(), column_map, cursor_obj, column_map_lower, column_names,📋 Files Needing Attention📉 Files with overall lowest coverage (click to expand)mssql_python.pybind.performance_counter.hpp: 0.7%
mssql_python.pybind.logger_bridge.cpp: 57.9%
mssql_python.pybind.row_factory.hpp: 62.3%
mssql_python.pybind.ddbc_bindings.h: 62.6%
mssql_python.pybind.ddbc_bindings.cpp: 79.9%
mssql_python.pybind.connection.connection_pool.cpp: 82.3%
mssql_python.pybind.connection.connection.cpp: 83.1%
mssql_python.logging.py: 86.2%
mssql_python.pybind.logger_bridge.hpp: 87.5%
mssql_python.pooling.py: 90.1%🔗 Quick Links
|
Copilot started reviewing on behalf of
Jahnvi Thakkar (jahnvi480)
October 5, 2026 06:03
View session
Jahnvi Thakkar (jahnvi480)
marked this pull request as ready for review
October 5, 2026 07:56
Copilot started reviewing on behalf of
Jahnvi Thakkar (jahnvi480)
October 5, 2026 07:57
View session
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
github-actions
Bot
added
pr-size: large
Jahnvi Thakkar (jahnvi480)
changed the title
Copilot started reviewing on behalf of
Jahnvi Thakkar (jahnvi480)
October 5, 2026 10:55
View session
Preserve bounded CI budgets and report unavailable modes explicitly. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot started reviewing on behalf of
Jahnvi Thakkar (jahnvi480)
October 6, 2026 14:36
View session
Comment on lines
+110
to
+115
| def build(path, log, timeout=900, profiling=True): | ||
| env = dict(os.environ, ENABLE_PROFILING="1" if profiling else "0") | ||
| # build scripts find Python via PATH; keep the controller's interpreter. | ||
| env["PATH"] = str(Path(sys.executable).parent) + os.pathsep + env["PATH"] | ||
| command = ["cmd", "/c", "build.bat"] if os.name == "nt" else ["bash", "build.sh"] | ||
| run_process(command, log, timeout, cwd=path / "mssql_python/pybind", env=env) |
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot started reviewing on behalf of
Jahnvi Thakkar (jahnvi480)
October 6, 2026 16:33
View session
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Select the native bridge used by each public fetch route while retaining all failure and recovery assertions. Mark three reviewed SHA256 source fingerprints with narrow DevSkim rule-specific suppressions. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot started reviewing on behalf of
Jahnvi Thakkar (jahnvi480)
October 7, 2026 02:40
View session
Restore the original 22-task diagnostic headline and report layout. Keep separate OFF/OFF latency and ON/OFF route artifacts, validation, thresholds and collection unchanged, with an explicit scope caveat. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot started reviewing on behalf of
Jahnvi Thakkar (jahnvi480)
October 7, 2026 03:45
View session
Keep logical buffer capacity, initialization and conversion unchanged, with heap fallback for larger bounded values. Preserve existing tests and append boundary, strict-error and isolated buffer-contract cases. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot started reviewing on behalf of
Jahnvi Thakkar (jahnvi480)
October 7, 2026 06:11
View session
Consolidate essential fetch regressions into existing suites and remove PR-added benchmark-only and unified-coverage-skipped tests. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot started reviewing on behalf of
Jahnvi Thakkar (jahnvi480)
October 8, 2026 05:55
View session
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The numeric route can mask SQLGetData failures, and latency CI does not produce optimized Release builds.
3 open findings
🧠 Review effort: Balanced
Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.
Comment on lines
+5097
to
+5098
| ret = FetchSingleRow(StatementHandle, row, charEncoding, wcharEncoding, charCtype, | ||
| messages, numCols, true); |
| temporary.replace(path) | ||
|
|
||
|
|
||
| def run_ci_report(args): |
Use existing Python Row completion while retaining native fetch behavior. Keep route provenance accurate and update existing regression expectations. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot started reviewing on behalf of
Jahnvi Thakkar (jahnvi480)
October 8, 2026 08:24
View session
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Latency builds are not verified Release builds, route documentation conflicts with source, and new CI orchestration lacks automated coverage.
4 open findings
🧠 Review effort: Balanced
Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.
Comment on lines
+297
to
+301
| must record 1,001 native fetch calls. The version-1 default route contract requires | ||
| 1,000 native Row constructions for `fetchmany(1)` and none for `fetchone` or | ||
| `fetchval` in the successor. Here `1` is a built-in integer for both numeric and | ||
| mixed shapes; this is not the numeric-column-only native fetching shortcut. | ||
| Legacy f539 requires 1,000 constructions for all three APIs; main666 requires none. This is route attribution, not production latency. |
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
Not applicable; the work item above is the single reference.
Summary
fetchone(),fetchmany(1)and inheritedfetchval()/iterator optimization work in this existing PR.SQLGetDatametadata. Direct column-count calls remain uncached; all nine original count-cache regressions remain.fetchmany(1)results throughSQLFetchScrollandSQLGetData, preserving eager count/name validation and row-array configuration/cleanup. Mixed INT/NVARCHAR and other types retain their existing native paths.fetchone()overrides.fetchval(). Replace Python single-row phase context managers with equivalent paired start/stop instrumentation.Qualification status: source review accepted; source-only controls passed (17 methods, 144 fake-boundary variants and eight 10,000-row operation-count controls). These use fake native boundaries/textual checks and do not establish native correctness or latency.
Formatting:
python -m pre_commit run black-check --all-files --hook-stage pre-pushpassed, using isolated pre-commit 4.5.1 and the repository-pinned Black 26.5.1 hook. Functional isolated commit/push hooks ran the same full check successfully. Black removed one extra blank line in the new test file; all 17 source controls passed again after formatting.git diff --cached --checkalso passed before commit.Native qualification and performance testing remain pending. No combined native build/import, live SQL test or benchmark has run locally. Earlier original-head CI/profiler results do not qualify this combined change. No speedup is claimed. Fresh-main, original-PR, combined and pyodbc comparisons require a separately reviewed runtime plan; normal CI for this update is expected to provide broader regression coverage.
Normal CI update: ADO build 180509 on
4c43081abuilt Ubuntu SQL2025 Release with profiling enabled, then reported 11 failed, 5,696 passed, 132 skipped and 42 deselected. All nine original count-cache cases and both new generation-change cases passed. Ten failures used unprefixed native profiler keys; the partial-bind case incorrectly excluded the existingRuntimeErrorcontract. Commitac6e9880corrects only those test expectations and counter-name documentation; production source is unchanged. All 19 source-only controls and the pinned full Black check passed before pushing the correction. A fresh native CI run is pending; the failed run is not presented as successful qualification.