PERF: Cache full column counts for fetchone - #829
Jahnvi Thakkar (jahnvi480) wants to merge 4 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: py::fetchone::row_wrap +0.034 ms; ddbc::SQLDescribeCol::driver_call +0.000 ms. Call changes: ddbc::SQLNumResultCols_wrap (1000 -> 1 calls). Unix / SQL Server 2025Row-by-row fetching: py::fetchone::row_wrap +0.016 ms; ddbc::SQLGetData_wrap +0.007 ms; ddbc::AppendDiagRecords::SQLGetDiagRec_call +0.003 ms. Call changes: 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
📋 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: 79.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
|
Work Item / Issue Reference
Summary
Draft Candidate A experiment for evaluation with the existing CI performance report; not ready to merge and no measured speedup claimed.
FetchOne_wrapwithin the existing statement metadata generation, instead of querying it after every successful row fetch.SQLGetDatametadata; publish only for the matching generation and reset on existing invalidation.Both fresh Linux x86_64 CPython 3.13 Release/profiling-OFF builds and imports succeeded. Local correctness execution and timing remain unrun because the required SQL-side FD inspection was permission-denied. The new tests and performance effect still require validation.
The CI report will be assessed per workload, including unchanged-path regressions. Profiling-enabled attribution will not be presented as a measured shipped Release-OFF speedup.