Skip to content

PERF: Reuse stable native column metadata per result set - #796

Open
Jahnvi Thakkar (jahnvi480) wants to merge 10 commits into
mainfrom
jahnvi/perf-small-fetch-native-metadata
Open

Jahnvi Thakkar (jahnvi480) wants to merge 10 commits into
mainfrom
jahnvi/perf-small-fetch-native-metadata

Conversation

@jahnvi480

@jahnvi480 Jahnvi Thakkar (jahnvi480) commented Sep 17, 2026

Copy link
Copy Markdown
Contributor

Work Item / Issue Reference

GitHub Issue: #554

ADO Task: AB#48323


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 in fetchone()/iteration, small fetchmany() 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_VARIANT retains 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"]
    end
Loading

Mechanism and correctness

Historical ON instrumentation at 252e9b69 counts actual ODBC descriptions for stable, error-free 10,000-row/24-column drains:

Workload Main This PR at 252e9b69
fetchmany(1) 240,024 24
fetchone() 240,000 24
MAX/LOB fetchall() 240,024 24
Ordinary fetchall() control 24 24

These are driver calls, not SQL network round-trips or elapsed-time savings. Mixed/NULL variant cases retain all 2,000 per-value NULL probes and 1,715 non-NULL subtype probes. Explicit public-description controls still perform 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 require cursor preservation not advertised by this driver; native helper ownership/invalidation checks provide separate, limited coverage.

Follow-up c5fe1425 extracts the existing child-cache invalidation helper without changing its algorithm, requires scalar NULL success, and adds permanent native invariant tests with Windows/Linux/macOS CI. Bounded checks against 252e9b69 passed 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. The six new candidate-only native cases passed locally on Linux and Windows with active Release assertions and deliberately failing assertion controls. The new native CI matrix also passed all six cases on Linux, Windows, and macOS. This is not a full repository suite; broader CI remains pending.

The previous Windows timings and the user-cancelled earlier incremental study are not reused. The completed OFF study applies only to 252e9b69/tree 3187b527, not this follow-up: all 15 A/A gates failed and 13 A/B no-regression bounds remained unresolved. Performance acceptance is not met; no general-speedup or no-regression signoff is claimed.

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 <223556219+Copilot@users.noreply.github.com>
Copilot AI lite review requested due to automatic review settings September 17, 2026 15:53

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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.

@jahnvi480 Jahnvi Thakkar (jahnvi480) changed the title REFACTOR: Keep fetchmany column metadata native and call-local PERF: Keep fetchmany column metadata native and call-local Sep 17, 2026
@github-actions

github-actions Bot commented Sep 17, 2026

Copy link
Copy Markdown

📊 Code Coverage Report

🔥 Diff Coverage

89%


🎯 Overall Coverage

84%


📈 Total Lines Covered: 9030 out of 10669
📁 Project: mssql-python


Diff Coverage

Diff: main...HEAD, staged and unstaged changes

  • mssql_python/pybind/connection/connection.cpp (100%)
  • mssql_python/pybind/ddbc_bindings.cpp (87.6%): Missing lines 1601,1844,3130-3131,3143,3147,3153,3410,3413,3430-3437,3443-3445,4401-4402,5042-5043,5958
  • mssql_python/pybind/result_metadata.hpp (100%)

Summary

  • Total: 245 lines
  • Missing: 25 lines
  • Coverage: 89%

mssql_python/pybind/ddbc_bindings.cpp

Lines 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 Python

Lines 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 3126-3135

  3126         }
  3127     } else {
  3128         // Row-wise fetches originally read names without decoding them. A later
  3129         // many/all fetch must still validate those names before its first advance.
! 3130         for (const auto& column : pending->columns) {
! 3131             py::cast(column.name);
  3132         }
  3133     }
  3134     pending->namesValidated = true;
  3135     statement->resultMetadata.publish(snapshot.generation, pending);

Lines 3139-3151

  3139 
  3140 }  // namespace
  3141 
  3142 // Wrap SQLDescribeCol
! 3143 SQLRETURN SQLDescribeCol_wrap(SqlHandlePtr StatementHandle, py::list& ColumnMetadata) {
  3144     PERF_TIMER("SQLDescribeCol_wrap");
  3145     SQLRETURN ret = SQL_ERROR;
  3146     ResultMetadataFailureGuard metadataFailure(StatementHandle->resultMetadata, ret);
! 3147     ret = DescribeColumns(StatementHandle, [&](std::u16string name, SQLSMALLINT type,
  3148                                               SQLULEN size, SQLSMALLINT digits,
  3149                                               SQLSMALLINT nullable) {
  3150         ColumnMetadata.append(
  3151             py::dict("ColumnName"_a = name, "DataType"_a = type, "ColumnSize"_a = size,

Lines 3149-3157

  3149                                               SQLSMALLINT nullable) {
  3150         ColumnMetadata.append(
  3151             py::dict("ColumnName"_a = name, "DataType"_a = type, "ColumnSize"_a = size,
  3152                      "DecimalDigits"_a = digits, "Nullable"_a = nullable));
! 3153     });
  3154     return ret;
  3155 }
  3156 
  3157 SQLRETURN SQLSpecialColumns_wrap(SqlHandlePtr StatementHandle, SQLSMALLINT identifierType,

Lines 3406-3417

  3406     auto pending = metadata ? nullptr : std::make_shared<ResultMetadata>();
  3407     bool complete = true;
  3408     if (pending) {
  3409         pending->columns.reserve(colCount);
! 3410     }
  3411 
  3412     for (SQLSMALLINT i = 1; i <= colCount; ++i) {
! 3413         SQLWCHAR uncachedColumnName[256];
  3414         const SQLWCHAR* columnName = uncachedColumnName;
  3415         SQLSMALLINT columnNameLen;
  3416         SQLSMALLINT dataType;
  3417         SQLULEN columnSize;

Lines 3426-3441

  3426             ret = SQL_SUCCESS;
  3427         } else {
  3428             {
  3429                 PERF_TIMER("SQLDescribeCol::driver_call");
! 3430                 ret = SQLDescribeCol_ptr(hStmt, i, uncachedColumnName,
! 3431                                          sizeof(uncachedColumnName) / sizeof(SQLWCHAR),
! 3432                                          &columnNameLen, &dataType, &columnSize, &decimalDigits,
! 3433                                          &nullable);
! 3434             }
! 3435             if (!SQL_SUCCEEDED(ret)) {
! 3436                 LOG("SQLGetData: Error retrieving metadata for column %d - "
! 3437                     "SQLDescribeCol SQLRETURN=%d",
  3438                     i, ret);
  3439                 complete = false;
  3440                 row.append(py::none());
  3441                 continue;

Lines 3439-3449

  3439                 complete = false;
  3440                 row.append(py::none());
  3441                 continue;
  3442             }
! 3443             if (pending) {
! 3444                 // Capture declared metadata before probing a variant's current value.
! 3445                 pending->columns.push_back({
  3446                     dupeSqlWCharAsUtf16Le(
  3447                         uncachedColumnName, std::min(static_cast<size_t>(columnNameLen),
  3448                                                     std::size(uncachedColumnName) - 1)),
  3449                     dataType, dataType == SQL_SS_VARIANT ? 0 : columnSize, decimalDigits,

Lines 4397-4406

  4397 
  4398     {
  4399         PERF_TIMER("FetchBatchData::cache_column_metadata");
  4400         for (SQLUSMALLINT col = 0; col < numCols; col++) {
! 4401             const auto& columnMeta = GetFetchColumnMetadata(columnNames, col);
! 4402             columnInfos[col].dataType = GetFetchColumnType(columnMeta);
  4403             columnInfos[col].columnSize = GetFetchColumnSize(columnMeta);
  4404             columnInfos[col].isLob =
  4405                 std::find(lobColumns.begin(), lobColumns.end(), col + 1) != lobColumns.end();
  4406             columnInfos[col].processedColumnSize = columnInfos[col].columnSize;

Lines 5038-5047

  5038 
  5039     // An overly large fetch size doesn't seem to help performance
  5040     int fetchSize = 64;
  5041 
! 5042     SQLRETURN ret = SQL_ERROR;
! 5043     ResultMetadataFailureGuard metadataFailure(StatementHandle->resultMetadata, ret);
  5044     SQLHSTMT hStmt = StatementHandle->get();
  5045     // Retrieve column count
  5046     SQLSMALLINT numCols = SQLNumResultCols_wrap(StatementHandle);
  5047     if (numCols <= 0) {

Lines 5954-5962

  5954     // Retrieve column count
  5955     SQLSMALLINT numCols = SQLNumResultCols_wrap(StatementHandle);
  5956 
  5957     // Retrieve column metadata
! 5958     const auto metadataSnapshot = StatementHandle->resultMetadata.snapshot();
  5959     py::list columnNames;
  5960     ret = SQLDescribeCol_wrap(StatementHandle, columnNames);
  5961     if (!SQL_SUCCEEDED(ret)) {
  5962         LOG("FetchAll_wrap: Failed to get column descriptions - SQLRETURN=%d", ret);


📋 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: 79.3%
mssql_python.pybind.connection.connection_pool.cpp: 82.3%
mssql_python.pybind.connection.connection.cpp: 82.8%
mssql_python.logging.py: 86.2%
mssql_python.pooling.py: 90.1%
mssql_python.pybind.fetch_temporal.hpp: 92.1%

🔗 Quick Links

⚙️ Build Summary 📋 Coverage Details

View Azure DevOps Build

Browse Full Coverage Report

Copilot AI review requested due to automatic review settings September 21, 2026 08:51
@github-actions

github-actions Bot commented Sep 21, 2026

Copy link
Copy Markdown

PR Performance Report

✅ No regression detected

No 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 diagnostics

Phase times are inclusive diagnostics and must not be added together. They identify where measured time changed, not why it changed.

Unix / SQL Server 2022

SELECT queries: ddbc::FetchAll_wrap +0.019 ms; py::fetchall::cpp_call +0.017 ms; ddbc::FetchBatchData +0.005 ms. Call changes: ddbc::SQLDescribeCol::driver_call (added, removed, or intermittent).
Fetch-all queries: ddbc::FetchAll_wrap +1.104 ms; ddbc::FetchBatchData +1.101 ms; py::fetchall::cpp_call +0.952 ms. Call changes: ddbc::SQLDescribeCol::driver_call (added, removed, or intermittent).
Row-by-row fetching: ddbc::SQLNumResultCols_wrap +0.001 ms. Call changes: ddbc::SQLDescribeCol::driver_call (added, removed, or intermittent).
Batched row fetching: ddbc::FetchBatchData::SQLFetchScroll_call +0.992 ms; ddbc::FetchBatchData +0.148 ms. Call changes: ddbc::SQLDescribeCol::driver_call (added, removed, or intermittent); ddbc::SQLDescribeCol_wrap (added, removed, or intermittent).
Transaction commit and rollback: ddbc::SQLExecDirect_wrap +0.350 ms; ddbc::Connection::commit +0.336 ms; ddbc::ConnectionHandle::commit +0.336 ms. Call changes: ddbc::SQLDescribeCol::driver_call (added, removed, or intermittent).
Arrow row fetching: ddbc::SQLDescribeCol_wrap +0.000 ms. Call changes: ddbc::SQLDescribeCol::driver_call (added, removed, or intermittent).
Row fetching in batches of 100: ddbc::FetchBatchData::SQLFetchScroll_call +1.921 ms; ddbc::FetchBatchData +0.979 ms; ddbc::FetchBatchData::construct_rows +0.066 ms. Call changes: ddbc::SQLDescribeCol::driver_call (added, removed, or intermittent); ddbc::SQLDescribeCol_wrap (added, removed, or intermittent).
Row fetching in batches of 10,000: no measured phase delta. Call changes: ddbc::SQLDescribeCol::driver_call (added, removed, or intermittent); ddbc::SQLDescribeCol_wrap (added, removed, or intermittent).
Repeated positional queries: py::execute::post_execute +0.084 ms; ddbc::FetchOne_wrap +0.047 ms; ddbc::SQLGetData_wrap +0.047 ms. Call changes: ddbc::SQLDescribeCol::driver_call (added, removed, or intermittent).
Repeated named-parameter queries: py::execute::post_execute +0.158 ms; ddbc::SQLDescribeCol_wrap +0.075 ms; py::fetchone::cpp_call +0.054 ms. Call changes: ddbc::SQLDescribeCol::driver_call (added, removed, or intermittent).
Joined aggregation queries: ddbc::SQLDescribeCol_wrap +0.004 ms; py::fetchall::row_wrap +0.003 ms; ddbc::SQLBindColums +0.001 ms. Call changes: ddbc::SQLDescribeCol::driver_call (added, removed, or intermittent).
Large joined-result fetching: py::execute::cpp_call +2.395 ms; ddbc::SQLExecDirect_wrap +2.392 ms; ddbc::FetchBatchData::SQLFetchScroll_call +1.438 ms. Call changes: ddbc::SQLDescribeCol::driver_call (added, removed, or intermittent).
1.2-million-row fetching: ddbc::FetchBatchData::SQLFetchScroll_call +7.399 ms; py::fetchall::row_wrap +1.824 ms; py::execute::post_execute +0.007 ms. Call changes: ddbc::SQLDescribeCol::driver_call (added, removed, or intermittent).
Common table expression queries: py::fetchall::cpp_call +0.032 ms; ddbc::FetchAll_wrap +0.032 ms; ddbc::SQLDescribeCol_wrap +0.005 ms. Call changes: ddbc::SQLDescribeCol::driver_call (added, removed, or intermittent).

Unix / SQL Server 2025

SELECT queries: ddbc::SQLExecDirect_wrap +0.019 ms; py::execute::cpp_call +0.017 ms; ddbc::FetchAll_wrap +0.016 ms. Call changes: ddbc::SQLDescribeCol::driver_call (added, removed, or intermittent).
Fetch-all queries: py::fetchall::row_wrap +0.155 ms; ddbc::SQLDescribeCol_wrap +0.005 ms; ddbc::SQLNumResultCols_wrap +0.000 ms. Call changes: ddbc::SQLDescribeCol::driver_call (added, removed, or intermittent).
Row-by-row fetching: ddbc::SQLNumResultCols_wrap +0.009 ms. Call changes: ddbc::SQLDescribeCol::driver_call (added, removed, or intermittent).
Batched row fetching: ddbc::FetchBatchData::SQLFetchScroll_call +1.909 ms; ddbc::FetchBatchData +1.709 ms; ddbc::FetchBatchData::construct_rows +0.004 ms. Call changes: ddbc::SQLDescribeCol::driver_call (added, removed, or intermittent); ddbc::SQLDescribeCol_wrap (added, removed, or intermittent).
Transaction commit and rollback: py::execute::cpp_call +0.740 ms; ddbc::SQLExecDirect_wrap +0.652 ms; ddbc::ConnectionHandle::commit +0.313 ms. Call changes: ddbc::SQLDescribeCol::driver_call (added, removed, or intermittent).
Arrow row fetching: ddbc::SQLDescribeCol_wrap +0.001 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 timings

Unix / SQL Server 2022

Database task Before After Paired change Result
Connection opening 10.440 ms 10.304 ms -0.6% no signal
SELECT queries 1.115 ms 1.097 ms -1.7% no signal
Row insertion 34.796 ms 34.623 ms -0.5% no signal
Executemany inserts 158.601 ms 157.664 ms +0.5% no signal
Fetch-all queries 119.733 ms 122.673 ms +2.0% no signal
Row-by-row fetching 56.220 ms 54.506 ms -3.0% no signal
Batched row fetching 123.275 ms 122.068 ms -1.3% no signal
Transaction commit and rollback 114.374 ms 114.797 ms +0.5% no signal
Arrow row fetching 94.358 ms 93.998 ms -0.6% no signal
100,000-row insertion 441.461 ms 453.489 ms +1.6% no signal
Row fetching in batches of 100 171.971 ms 157.199 ms -8.6% no signal
Row fetching in batches of 10,000 137.364 ms 126.878 ms -1.8% no signal
Repeated positional queries 42.033 ms 42.388 ms -0.9% no signal
Repeated named-parameter queries 44.580 ms 43.944 ms -1.5% no signal
Legacy 100,000-row insertion 353.106 ms 350.769 ms -0.8% no signal
Insertion with explicit input sizes 484.840 ms 491.251 ms +0.7% no signal
Joined aggregation queries 177.696 ms 177.199 ms -0.2% no signal
Large joined-result fetching 181.983 ms 181.326 ms +2.0% no signal
1.2-million-row fetching 3425.681 ms 3415.269 ms -0.1% no signal
Common table expression queries 5.489 ms 5.333 ms -3.2% no signal

Unix / SQL Server 2025

Database task Before After Paired change Result
Connection opening 97.828 ms 96.346 ms -1.5% no signal
SELECT queries 1.062 ms 1.080 ms +1.7% no signal
Row insertion 34.394 ms 34.103 ms -0.5% no signal
Executemany inserts 155.827 ms 157.374 ms +2.9% no signal
Fetch-all queries 121.421 ms 120.693 ms -1.0% no signal
Row-by-row fetching 56.352 ms 55.161 ms -1.9% no signal
Batched row fetching 122.263 ms 121.654 ms -0.6% no signal
Transaction commit and rollback 113.578 ms 114.746 ms +0.2% no signal
Arrow row fetching 94.783 ms 93.198 ms -0.1% no signal
100,000-row insertion 438.972 ms 472.586 ms +4.9% no signal
Row fetching in batches of 100 174.879 ms 159.774 ms -9.4% no signal
Row fetching in batches of 10,000 140.343 ms 129.204 ms -1.6% no signal
Repeated positional queries 41.438 ms 41.553 ms +0.7% no signal
Repeated named-parameter queries 43.990 ms 44.336 ms +0.8% no signal
Legacy 100,000-row insertion 355.731 ms 354.680 ms -0.5% no signal
Insertion with explicit input sizes 494.065 ms 492.298 ms +2.4% no signal
Joined aggregation queries 165.877 ms 159.997 ms -0.5% no signal
Large joined-result fetching 182.922 ms 188.936 ms +1.5% no signal
1.2-million-row fetching 3512.228 ms 3497.874 ms -1.6% no signal
Common table expression queries 5.134 ms 5.068 ms -0.6% no signal
Build and measurement details

ADO build 177274

PR head: d0af2acce06b14236529625bc01ba63b8c642c26
Base: 171d1306e0bad385667c2484d64a05a0af4cd1d6
Measured merge: fc90a62a138516b78cae358429f7e6ef71b1faea

  • Unix / SQL Server 2022: Python 3.12.3, x86_64, SQL 16.0.4295.3; 5 paired comparisons and 1 warmup.
  • Unix / SQL Server 2025: Python 3.12.3, x86_64, SQL 17.0.5005.3; 5 paired comparisons and 1 warmup.

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 profiler-* artifacts.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

The performance-sensitive native ODBC path has unstable calibration and no established no-regression result.

Review effort: Lite
Findings: None

@github-actions github-actions Bot added the pr-size: large Substantial code update label Sep 21, 2026
Copilot AI review requested due to automatic review settings September 22, 2026 05:54

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Medium severity

Open (1)

Comment thread mssql_python/pybind/ddbc_bindings.cpp Outdated
Copilot AI review requested due to automatic review settings September 22, 2026 07:53

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

It changes the native fetch hot path and reports a significant fetchall() regression, requiring broader cross-platform performance validation.

Review effort: Lite
Findings: 1 Medium severity

Open (1)

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings September 22, 2026 09:22
@jahnvi480 Jahnvi Thakkar (jahnvi480) changed the title PERF: Keep fetchmany column metadata native and call-local PERF: Reuse native column metadata across result-set fetches Sep 22, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Medium severity

Open (1)
Resolved since last review (1)

Comment thread mssql_python/pybind/ddbc_bindings.cpp
@jahnvi480 Jahnvi Thakkar (jahnvi480) changed the title PERF: Reuse native column metadata across result-set fetches PERF: Reuse stable native column metadata per result set Sep 22, 2026
@jahnvi480
Jahnvi Thakkar (jahnvi480) marked this pull request as ready for review September 22, 2026 11:53

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No actionable findings.

Reviewed all seven changed files and affected Python/native paths, focusing on metadata ownership, cache invalidation, mixed fetch operations, Unicode handling, sql_variant, and cleanup.

The earlier comments are addressed: the public metadata wrapper no longer creates the unnecessary intermediate object, and the PR description now explicitly documents statement-owned caching.

Preserve reserve-before-locking-weak-handles and release retained handles outside the child-list mutex. Expose the unchanged native-only algorithm for direct invariant coverage.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Require successful scalar NULL handling and exact describe counts. Add production-header cache, failure, concurrency, allocation and lifetime tests with active Release assertions, plus Windows/Linux/macOS CTest CI and test guidance.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Comment thread tests/native/result_metadata_tests.cpp Fixed
Comment thread tests/native/result_metadata_tests.cpp Fixed
Comment thread tests/native/allocation_failure.cpp Fixed

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

One critical formatting issue and three moderate cache or CI coverage issues remain unresolved.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 High severity · 1 Medium severity

Open (2)
Resolved since last review (1)
Previously missed (1)

In code that hasn't changed since last review

Medium severity Reuse native metadata cache in Arrow fetches

mssql_python/​pybind/​ddbc_bindings.cpp:5042

The Arrow fetch path still calls SQLDescribeCol_wrap here and then reads Python dictionaries for every arrow_batch() invocation, so repeated Arrow batches continue doing one ODBC description per column and the Python metadata roundtrip. That bypasses the new native cache; route this path through GetResultMetadata and the native metadata accessors (and add a description-count regression test) so Arrow batches also reuse stable result metadata.

Comment thread tests/test_040_fetch_native_metadata.py
Comment thread .github/workflows/native-metadata-tests.yml
Use C++ streams for test-runner output, document the line-scoped allocator rule exception, and trigger native invariant tests for production integration header and binding changes. Keep production fetch code and the allocation-failure probes unchanged.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings September 22, 2026 15:00

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

A moderate connection-level invalidation regression remains, and broader validation is still pending.

Review effort: Lite
Findings: None

Resolved since last review (2)

Copilot AI review requested due to automatic review settings September 22, 2026 17:14

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Arrow batch, table, and reader paths still bypass the metadata cache.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 Medium severity

Open (1)

Comment thread mssql_python/pybind/ddbc_bindings.cpp
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr-size: large Substantial code update

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants