PERF: Reuse stable native column metadata per result set - #796
Conversation
Remove native metadata dictionary roundtrips while preserving eager Unicode names and fresh per-call descriptions. Add behavior and profiling regression coverage. Performance acceptance remains unresolved after the bounded local study. Co-authored-by: Copilot App <[email protected]>
There was a problem hiding this comment.
🟡 Changes recommended
The declared performance acceptance and no-regression gates remain unresolved, including failed A/A stability results.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Refactors fetchmany() metadata handling to avoid Python dictionary round-trips while preserving public descriptions and existing fetch behavior.
Changes:
- Adds call-local native metadata structures and shared description logic.
- Adds comprehensive fetch, metadata, lifecycle, and profiling tests.
- Documents the behavior change in the changelog.
File summaries
| File | Description |
|---|---|
mssql_python/pybind/ddbc_bindings.cpp |
Uses native metadata for fetchmany(). |
tests/test_040_fetch_native_metadata.py |
Adds regression and profiling coverage. |
CHANGELOG.md |
Documents the metadata refactor. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Jahnvi Thakkar (jahnvi480)
changed the title
📊 Code Coverage Report
Diff CoverageDiff: main...HEAD, staged and unstaged changes
Summary
mssql_python/pybind/ddbc_bindings.cppLines 1597-1605 1597 }
1598
1599 SQLRETURN SqlHandle::freeHandle() {
1600 PERF_TIMER("SqlHandle::free");
! 1601 resultMetadata.clear();
1602 bool pythonShuttingDown = is_python_finalizing();
1603 bool skipDuringShutdown = _type == SQL_HANDLE_STMT || _type == SQL_HANDLE_DBC;
1604 #ifdef _WIN32
1605 // The static ENV is destroyed during DLL_PROCESS_DETACH, after PythonLines 1840-1848 1840 SQLRETURN SQLColumns_wrap(SqlHandlePtr StatementHandle, const py::object& catalogObj,
1841 const py::object& schemaObj, const py::object& tableObj,
1842 const py::object& columnObj) {
1843 PERF_TIMER("SQLColumns_wrap");
! 1844 StatementHandle->resultMetadata.clear();
1845 if (!SQLColumns_ptr) {
1846 ThrowStdException("SQLColumns function not loaded");
1847 }Lines 3090-3099 3090 }
3091
3092 if (SQL_SUCCEEDED(retcode)) {
3093 auto name = dupeSqlWCharAsUtf16Le(
! 3094 ColumnName, std::min(static_cast<size_t>(NameLength),
! 3095 (sizeof(ColumnName) / sizeof(SQLWCHAR)) - 1));
3096 appendColumn(std::move(name), DataType, ColumnSize, DecimalDigits, Nullable);
3097 } else {
3098 return retcode;
3099 }Lines 3103-3115 3103
3104 } // namespace
3105
3106 // Wrap SQLDescribeCol
! 3107 SQLRETURN SQLDescribeCol_wrap(SqlHandlePtr StatementHandle, py::list& ColumnMetadata) {
3108 PERF_TIMER("SQLDescribeCol_wrap");
3109 SQLRETURN ret = SQL_ERROR;
3110 ResultMetadataFailureGuard metadataFailure(StatementHandle->resultMetadata, ret);
! 3111 ret = DescribeColumns(StatementHandle, [&](std::u16string name, SQLSMALLINT type,
3112 SQLULEN size, SQLSMALLINT digits,
3113 SQLSMALLINT nullable) {
3114 ColumnMetadata.append(
3115 py::dict("ColumnName"_a = name, "DataType"_a = type, "ColumnSize"_a = size,Lines 3113-3121 3113 SQLSMALLINT nullable) {
3114 ColumnMetadata.append(
3115 py::dict("ColumnName"_a = name, "DataType"_a = type, "ColumnSize"_a = size,
3116 "DecimalDigits"_a = digits, "Nullable"_a = nullable));
! 3117 });
3118 return ret;
3119 }
3120
3121 SQLRETURN SQLSpecialColumns_wrap(SqlHandlePtr StatementHandle, SQLSMALLINT identifierType,Lines 3369-3377 3369 : nullptr;
3370 auto pending = metadata ? nullptr : std::make_shared<ResultMetadata>();
3371 bool complete = true;
3372 if (pending) {
! 3373 pending->columns.reserve(colCount);
3374 }
3375
3376 for (SQLSMALLINT i = 1; i <= colCount; ++i) {
3377 SQLWCHAR uncachedColumnName[256];Lines 3389-3404 3389 columnName = reinterpretU16stringAsSqlWChar(column.name);
3390 ret = SQL_SUCCESS;
3391 } else {
3392 {
! 3393 PERF_TIMER("SQLDescribeCol::driver_call");
! 3394 ret = SQLDescribeCol_ptr(hStmt, i, uncachedColumnName,
! 3395 sizeof(uncachedColumnName) / sizeof(SQLWCHAR),
! 3396 &columnNameLen, &dataType, &columnSize, &decimalDigits,
! 3397 &nullable);
! 3398 }
! 3399 if (!SQL_SUCCEEDED(ret)) {
! 3400 LOG("SQLGetData: Error retrieving metadata for column %d - "
3401 "SQLDescribeCol SQLRETURN=%d",
3402 i, ret);
3403 complete = false;
3404 row.append(py::none());Lines 3402-3412 3402 i, ret);
3403 complete = false;
3404 row.append(py::none());
3405 continue;
! 3406 }
! 3407 if (pending) {
! 3408 // Capture declared metadata before probing a variant's current value.
3409 pending->columns.push_back({
3410 dupeSqlWCharAsUtf16Le(
3411 uncachedColumnName, std::min(static_cast<size_t>(columnNameLen),
3412 std::size(uncachedColumnName) - 1)),Lines 4360-4369 4360
4361 {
4362 PERF_TIMER("FetchBatchData::cache_column_metadata");
4363 for (SQLUSMALLINT col = 0; col < numCols; col++) {
! 4364 const auto& columnMeta = GetFetchColumnMetadata(columnNames, col);
! 4365 columnInfos[col].dataType = GetFetchColumnType(columnMeta);
4366 columnInfos[col].columnSize = GetFetchColumnSize(columnMeta);
4367 columnInfos[col].isLob =
4368 std::find(lobColumns.begin(), lobColumns.end(), col + 1) != lobColumns.end();
4369 columnInfos[col].processedColumnSize = columnInfos[col].columnSize;Lines 4642-4651 4642 PyList_SET_ITEM(row, col - 1, uuid_obj.release().ptr());
4643 break;
4644 }
4645 default: {
! 4646 const auto& columnMeta = GetFetchColumnMetadata(columnNames, col - 1);
! 4647 std::string columnName = GetFetchColumnName(columnMeta);
4648 std::ostringstream errorString;
4649 errorString << "Unsupported data type for column - " << columnName.c_str()
4650 << ", Type - " << dataType << ", column ID - " << col;
4651 LOG("FetchBatchData: %s", errorString.str().c_str());Lines 5023-5032 5023
5024 // An overly large fetch size doesn't seem to help performance
5025 int fetchSize = 64;
5026
! 5027 SQLRETURN ret = SQL_ERROR;
! 5028 ResultMetadataFailureGuard metadataFailure(StatementHandle->resultMetadata, ret);
5029 SQLHSTMT hStmt = StatementHandle->get();
5030 // Retrieve column count
5031 SQLSMALLINT numCols = SQLNumResultCols_wrap(StatementHandle);
5032 if (numCols <= 0) {📋 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: 64.1%
mssql_python.pybind.logger_bridge.hpp: 70.8%
mssql_python.pybind.ddbc_bindings.cpp: 78.7%
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✅ 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 2022SELECT queries: ddbc::SQLExecDirect_wrap +0.011 ms; py::fetchall::cpp_call +0.011 ms; py::execute::cpp_call +0.011 ms. Call changes: ddbc::SQLDescribeCol::driver_call (added, removed, or intermittent). Unix / SQL Server 2025SELECT queries: py::fetchall::cpp_call +0.006 ms; ddbc::FetchAll_wrap +0.006 ms; ddbc::SQLDescribeCol_wrap +0.002 ms. Call changes: ddbc::SQLDescribeCol::driver_call (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 |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The shared metadata wrapper adds intermediate work to fetchall, Arrow, and execute paths, with the reported fetchall regression unresolved.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Open (1)
Co-authored-by: Copilot App <[email protected]>
Co-authored-by: Copilot App <[email protected]>
Jahnvi Thakkar (jahnvi480)
changed the title
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The implementation persists metadata across fetch calls despite the PR description promising call-local metadata and unchanged fresh ODBC descriptions.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Open (1)
Resolved since last review (1)
Jahnvi Thakkar (jahnvi480)
changed the title
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Arrow multi-batch fetches bypass the cache, and focused generation/invalidation coverage is still needed.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Open (1)
Resolved since last review (1)
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
A teardown deadlock risk, connection-operation scalability concern, and missing native failure/lifetime coverage remain unresolved.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Gaurav Sharma (bewithgaurav)
left a comment
Gaurav Sharma (bewithgaurav)
left a comment
There was a problem hiding this comment.
the cache direction looks sound, and I did not reproduce a runtime regression.
please retain focused cache-lifetime regression coverage and complete the current-head performance qualification before merging.
requesting changes.
Co-authored-by: Copilot App <[email protected]>
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The cursor-GC regression test retains cursor references through fetched rows and must detach scalar values before forcing garbage collection.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Open (1)
Resolved since last review (2)
Pleasere-review
Preserve the existing fetch-buffer reuse changes and integrate the merged metadata optimization plus current main without rewriting branch history. Co-authored-by: Copilot App <[email protected]>
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 <[email protected]>
Work Item / Issue Reference
Summary
Keep internal fetch metadata native and reuse stable column descriptions within the current result set. This removes the
fetchmany()Python-dictionary roundtrip and repeated descriptions infetchone()/iteration, smallfetchmany()calls, and row-wise MAX/LOB fetching.The cache is statement-owned and invalidated on execution, result transitions, relevant connection operations and cleanup. Public descriptions stay fresh and Unicode-name validation timing is preserved. Every declared
SQL_VARIANTretains per-row descriptions and per-value probes. No persistent fetch buffers, binding reuse, hidden prefetch, fetch-size changes or cached decoding/converter results are introduced.flowchart LR subgraph Before B1["Each fetch / row"] --> B2["Repeated descriptions; dict setup for fetchmany"] --> B3["Bind / fetch"] end subgraph After A1["First fetch in result set"] --> A2["Owned native metadata"] --> A3["Reuse stable fields; same bind / fetch"] endCurrent scope and validation
The
65081f06scope correction, retained inbe3efbb6, leaves five runtime files +332 / -56 (25 fewer net runtime lines thand0af2acc) and the related changelog entry. There is zero effective build/CI diff. Review follow-upbe3efbb6strengthens two existing test functions (+21 / -7) for metadata invalidation across re-execution/result transitions and row lifetime across connection/cursor cleanup, without changing production code. No test files, test functions or parameterized cases are added. PR-only test files, native CMake/workflow scaffolding, and added test guidance remain removed.Fresh scoped Linux Release/OFF builds of pre-cleanup
d0af2accand candidate65081f06each passed 339 ordinary checks plus 3 isolated checks, with no skips, using the same existing base-test selection, not the removed metadata/native fixtures. This evidence qualifies the tested cleanup revision, not the later whole4616f2c5main merge, currentbe3efbb6assertions, or a full repository suite. Current-head execution of the strengthened tests, performance qualification and reviewer signoff remain pending. Windows/macOS qualification, ON/count checks and latency measurements were not included. The older results below remain historical, not current-head qualification.Historical mechanism and correctness
Historical ON instrumentation at
252e9b69counts actual ODBC descriptions for stable, error-free 10,000-row/24-column drains:8fb3c3b1252e9b69fetchmany(1)fetchone()fetchall()fetchall()controlThese are driver calls, not SQL network round-trips or elapsed-time savings. Mixed/NULL variant cases retained all 2,000 per-value NULL probes and 1,715 non-NULL subtype probes. Explicit public-description controls still performed fresh descriptions.
Historical OFF validation at
252e9b69: 338 passed, 9 skipped per arm across five invocations (metadata, temporal constructors, settings/NULL, Arrow/interleaving, lifetime). Candidate ON metadata/count checks: 55 passed, 3 skipped. Three cases required cursor preservation not advertised by this driver.Historical follow-up
c5fe1425added the now-removed native test/CI scaffolding. Its bounded checks against252e9b69passed on both arms: OFF metadata/interleaving 52 passed, 9 skipped, OFF lifetime 10 passed, and ON metadata/counts 55 passed, 3 skipped, in three separate invocations per arm. Six candidate-only native cases passed locally on Linux and Windows with active Release assertions and deliberately failing assertion controls; the historical native CI matrix passed all six on Linux, Windows, and macOS. These were limited checks, not a full repository suite, and neither the tests nor that workflow remain in the effective PR diff.The previous Windows timings and the user-cancelled earlier incremental study are not reused. The completed OFF study applies only to
252e9b69/tree3187b527, not the current head: all 15 A/A gates failed and 13 A/B no-regression bounds remained unresolved. Performance acceptance remains on HOLD; no general-speedup or no-regression signoff is claimed.