PERF: Avoid redundant fetch diagnostic scans - #809
Conversation
Capture fetch diagnostics before subsequent ODBC calls replace them, preserve data-read errors and Arrow cleanup behavior, and update existing regression coverage only. The tree is identical to tested revision c1808128312c956dbf108028a9c6fb750d209a51; broader warning/platform coverage and the faster-than-pyodbc goal remain unqualified. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Unresolved native error-propagation issues can overwrite diagnostics or expose invalid metadata.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Open (1)
What changed in this PR
Optimizes fetch diagnostics by capturing ODBC records during native fetches and avoiding redundant Python-side scans.
Changes:
- Threads cursor messages through native fetch and Arrow paths.
- Preserves warning, EOF, and cleanup diagnostics.
- Updates regression tests for diagnostic forwarding and call counts.
| File | Summary |
|---|---|
tests/test_fetch_settings_cache.py |
Updates fetch bridge diagnostic assertions. |
tests/test_004_cursor.py |
Strengthens EOF-warning coverage. |
tests/test_004_cursor_arrow.py |
Verifies Arrow exhaustion diagnostics. |
mssql_python/pybind/ddbc_bindings.h |
Propagates message storage through native helpers. |
mssql_python/pybind/ddbc_bindings.cpp |
Implements native capture and cleanup; includes one critical, two moderate, and one nit finding regarding error propagation. |
mssql_python/cursor.py |
Passes message lists through fetch paths and removes redundant scans. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
📊 Code Coverage Report
Diff CoverageDiff: main...HEAD, staged and unstaged changes
Summary
mssql_python/pybind/ddbc_bindings.cppLines 1927-1937 1927 }
1928 if (stateReturn == SQL_NO_DATA)
1929 break;
1930 // Skip only this continuation record, without retrieving its message text.
! 1931 if (stateReturn == SQL_SUCCESS && std::equal(sqlState, sqlState + 6, u"01004"))
! 1932 continue;
! 1933 if (stateReturn != SQL_SUCCESS)
1934 LOG("AppendDiagRecords: SQLSTATE lookup returned %d; reading full record %d",
1935 stateReturn, recNumber);
1936 }
1937 SQLWCHAR message[SQL_MAX_MESSAGE_LENGTH_SQLSERVER] = {0};Lines 1962-1971 1962 // Format the state string
1963 std::string stateWithError = "[" + stateStr + "] (" + std::to_string(nativeError) + ")";
1964
1965 // Create the tuple with converted strings
! 1966 py::tuple record = py::make_tuple(py::str(stateWithError), py::str(msgStr));
! 1967 if (PyList_Append(records.ptr(), record.ptr()) < 0)
1968 throw py::error_already_set();
1969 }
1970 }Lines 3582-3590 3582 LOG("SQLGetData: CHAR column %d (WCHAR path) data "
3583 "truncated, using streaming LOB",
3584 i);
3585 row.append(FetchLobColumnData(hStmt, i, SQL_C_WCHAR, true, false,
! 3586 "utf-16le", messages));
3587 }
3588 } else if (dataLen == SQL_NULL_DATA) {
3589 LOG("SQLGetData: Column %d is NULL (CHAR via WCHAR)", i);
3590 row.append(py::none());Lines 3639-3647 3639 std::vector<SQLCHAR> dataBuffer(fetchBufferSize);
3640 SQLLEN dataLen;
3641 ret = SQLGetData_ptr(hStmt, i, SQL_C_CHAR, dataBuffer.data(), dataBuffer.size(),
3642 &dataLen);
! 3643 CaptureFetchDiagnostics(
3644 hStmt, ret, messages,
3645 ret == SQL_SUCCESS_WITH_INFO &&
3646 (dataLen == SQL_NO_TOTAL ||
3647 dataLen >= static_cast<SQLLEN>(dataBuffer.size())));Lines 3692-3700 3692 LOG("SQLGetData: SQL_NO_TOTAL for column %d (SQL_CHAR), "
3693 "streaming via FetchLobColumnData",
3694 i);
3695 row.append(FetchLobColumnData(hStmt, i, SQL_C_CHAR, false, false,
! 3696 effectiveCharEnc, messages));
3697 } else if (dataLen < 0) {
3698 LOG("SQLGetData: Unexpected negative data length "
3699 "for column %d - dataType=%d, dataLen=%ld",
3700 i, dataType, (long)dataLen);Lines 3802-3810 3802 case SQL_INTEGER: {
3803 SQLINTEGER intValue;
3804 SQLLEN indicator = 0;
3805 ret = SQLGetData_ptr(hStmt, i, SQL_C_LONG, &intValue, 0, &indicator);
! 3806 CaptureFetchDiagnostics(hStmt, ret, messages);
3807 if (SQL_SUCCEEDED(ret) && indicator != SQL_NULL_DATA) {
3808 row.append(static_cast<int>(intValue));
3809 } else {
3810 row.append(py::none());Lines 4403-4412 4403 template <typename Metadata>
4404 SQLRETURN FetchBatchData(SQLHSTMT hStmt, ColumnBuffers& buffers, const Metadata& columnNames,
4405 py::list& rows, SQLUSMALLINT numCols, SQLULEN& numRowsFetched,
4406 const std::vector<SQLUSMALLINT>& lobColumns,
! 4407 const std::string& charEncoding = "utf-16le", int charCtype = SQL_C_WCHAR,
! 4408 py::handle messages = {}) {
4409 PERF_TIMER("FetchBatchData");
4410 LOG("FetchBatchData: Fetching data in batches");
4411 SQLRETURN ret;
4412 {Lines 4860-4870 4860
4861 void observe(SQLRETURN ret) const {
4862 CaptureFetchDiagnostics(handle->get(), ret, py::handle(messages));
4863 CheckFetchError(handle, ret);
! 4864 }
! 4865
! 4866 void setRowArraySize(SQLULEN rowArraySize) const {
4867 observe(SQLSetStmtAttr_ptr(handle->get(), SQL_ATTR_ROW_ARRAY_SIZE,
4868 (SQLPOINTER)(intptr_t)rowArraySize, 0));
4869 }Lines 5013-5021 5013 }
5014
5015 // Initialize column buffers
5016 ColumnBuffers buffers(numCols, fetchSize);
! 5017 FetchStateGuard fetchStateGuard(StatementHandle, messages);
5018
5019 // Bind columns
5020 ret = SQLBindColums(hStmt, buffers, columnNames, numCols, fetchSize, charCtype, messages);
5021 if (!SQL_SUCCEEDED(ret)) {Lines 5337-5345 5337 SQLULEN numRowsFetched = 0;
5338 FetchStateGuard fetchStateGuard(StatementHandle, messages);
5339
5340 if (!hasLobColumns && fetchSize > 0) {
! 5341 ret = SQLBindColums(hStmt, buffers, columnNames, numCols, fetchSize, charCtype, messages);
5342 if (!SQL_SUCCEEDED(ret)) {
5343 LOG("Error when binding columns");
5344 return ret;
5345 }Lines 5549-5557 5549 ret = SQLGetData_ptr(hStmt, idxCol + 1, SQL_C_TYPE_DATE,
5550 buffers.dateBuffers[idxCol].data(),
5551 sizeof(SQL_DATE_STRUCT),
5552 buffers.indicators[idxCol].data());
! 5553 fetchStateGuard.observe(ret);
5554 if (!SQL_SUCCEEDED(ret)) {
5555 LOG("Error fetching TYPE_DATE data for column %d", idxCol + 1);
5556 return ret;
5557 }mssql_python/pybind/ddbc_bindings.h📋 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.1%
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
|
PR Performance Report✅ Performance improved3 database tasks consistently improved across 2 measured environments. No consistent slowdowns were detected. 3 IMPROVEMENTS 0 SLOWDOWNS 2/2 ENVIRONMENTS Signal fingerprint
Coverage: 2 of 2 environments completed. Advisory result; does not block merging. Measured timings
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 2022SELECT queries: py::fetchall::cpp_call +0.216 ms; ddbc::FetchAll_wrap +0.215 ms; ddbc::FetchBatchData +0.091 ms. Call changes: ddbc::AppendDiagRecords::SQLGetDiagRec_call (added, removed, or intermittent); ddbc::SQLGetAllDiagRecords (added, removed, or intermittent); py::fetchall::diag_records (added, removed, or intermittent). Unix / SQL Server 2025SELECT queries: py::fetchall::cpp_call +0.076 ms; ddbc::FetchAll_wrap +0.076 ms; ddbc::FetchBatchData +0.069 ms. Call changes: ddbc::AppendDiagRecords::SQLGetDiagRec_call (added, removed, or intermittent); ddbc::SQLGetAllDiagRecords (added, removed, or intermittent); py::fetchall::diag_records (added, removed, or intermittent). 8 additional diagnostic rows are available in the raw ADO artifacts. 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 |
Raise through the existing DB-API translator before returning a failed SQLNumResultCols output. Return a failed sql_variant probe immediately, before a later column can replace its diagnostics. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Compare ODBC SQLLEN indicators against bounded buffer sizes converted to SQLLEN. This fixes the four MSVC C4018 errors promoted by /WX without weakening compiler settings, and preserves SQL_NO_TOTAL handling. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Preserve the later upstream merge while retaining the locally validated native review fixes. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Read SQLSTATE with SQLGetDiagFieldW before materializing internal continuation diagnostics. Skip only confirmed 01004 records, retrieve every unrelated record unchanged, and retain full-record fallback if the field API is unavailable or fails. Track actual field calls with the existing profiler. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Exercise the compiled diagnostic collector in isolated subprocesses with deterministic ODBC diagnostic callbacks and a real LOB continuation. Verify warning order, internal truncation filtering, SQLSTATE fallback, full-record call counts, and the unfiltered diagnostic path. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Contain callback exceptions and assert them outside the C ABI. Learn the target statement through an unchanged diagnostic call, forward unrelated handles and EOF to saved driver addresses, and restore pointers before cursor cleanup. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Native fetch and metadata error paths can lose diagnostics or return success before the original errors are propagated.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Critical diagnostic buffer sizing and multiple bind-error diagnostic preservation issues remain unresolved.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Please re-review
Resolve the #796 metadata-cache integration while retaining immediate diagnostic capture, mixed-record filtering, and error propagation from #809. Forward the message sink through the new shared metadata-description helper and templated binding path. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Gaurav Sharma (bewithgaurav)
left a comment
Gaurav Sharma (bewithgaurav)
left a comment
There was a problem hiding this comment.
fixes lgtm, approving
Work Item / Issue Reference
Summary
Capture fetch diagnostics immediately while the ODBC records are valid, then remove redundant Python-side scans on clean fetch calls.
Validation
Linux x64, Python 3.13.15, Release builds with profiling compiled out/on separately. The final selected 274-case group, 3 isolated cases, real EOF-warning contract, 3 additional Arrow lifecycle cases and explicitly selected cancellation case passed. Nine diagnostic mocks are included in the 274-case group. Full Black check passed.
Each of 12 primary profiling observations recorded one actual helper SQLGetDiagRec call per clean 10,000-row drain; three Arrow observations recorded two calls each.
Performance
Uninstrumented 10,000-row workload medians; 20 matched pairs per primary and 10 for the Arrow-equivalent control:
The four primary medians were approximately 94–96% lower than the separately measured main baseline. This is descriptive, not a direct paired main/candidate confidence interval. All four primary workloads still lose to pyodbc. No isolated allocation or latency benefit is claimed for the positional-argument cleanup; native functions use FASTCALL|KEYWORDS.
Draft limitations
The combined performance goal is not complete, and neither ADO task is being declared closed. Perf Police scoped source review and the measured diagnostic-call budget passed; overall qualification remains HOLD. Real intermediate native-warning/mixed-record preservation, wider platform/failure-path coverage, and the full planned benchmark/noninferiority matrix remain unqualified. The scalar/small-batch warning arrived at EOF; fetchall native warning origin remains unlocalized. Intermediate native warning preservation is not established. Earlier fetchall/fetchmany(1000)/nextset observations are not final-revision measurements.