PERF: Optimize fetchone, fetchmany(1) and fetchval paths - #829
Jahnvi Thakkar (jahnvi480) wants to merge 7 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::AppendDiagRecords::SQLGetDiagRec_call +0.000 ms; 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::AppendDiagRecords::SQLGetDiagRec_call +0.001 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. Raw samples and logs are attached to the ADO run as |
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>
📊 Code Coverage Report
Diff CoverageDiff: main...HEAD, staged and unstaged changes
Summary
mssql_python/pybind/ddbc_bindings.cppLines 5424-5432 5424 FetchStateGuard fetchStateGuard(StatementHandle, messages);
5425
5426 if (!hasLobColumns && fetchSize > 0) {
5427 ret = SQLBindColums(StatementHandle, buffers, columnNames, numCols, fetchSize, charCtype,
! 5428 messages);
5429 if (!SQL_SUCCEEDED(ret)) {
5430 LOG("Error when binding columns");
5431 return ret;
5432 }📋 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.ddbc_bindings.h: 62.6%
mssql_python.pybind.logger_bridge.hpp: 70.8%
mssql_python.pybind.ddbc_bindings.cpp: 80.5%
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.pooling.py: 90.1%
mssql_python.pybind.fetch_temporal.hpp: 92.1%🔗 Quick Links
|
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
github-actions
Bot
added
pr-size: large
Jahnvi Thakkar (jahnvi480)
changed the title
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
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.