PERF: Keep fetchmany column metadata native and call-local - #796
Jahnvi Thakkar (jahnvi480) wants to merge 2 commits into
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 <223556219+Copilot@users.noreply.github.com>
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 3039-3050 3039 SQLULEN GetFetchColumnSize(const FetchColumnMetadata& column) {
3040 return column.columnSize;
3041 }
3042
! 3043 std::string GetFetchColumnName(const py::dict& column) {
3044 return column["ColumnName"].cast<std::string>();
! 3045 }
! 3046
3047 std::string GetFetchColumnName(const FetchColumnMetadata& column) {
3048 return column.name.cast<std::string>();
3049 }Lines 3093-3101 3093 } // namespace
3094
3095 // Wrap SQLDescribeCol
3096 SQLRETURN SQLDescribeCol_wrap(SqlHandlePtr StatementHandle, py::list& ColumnMetadata) {
! 3097 PERF_TIMER("SQLDescribeCol_wrap");
3098 return DescribeColumns(StatementHandle, [&](FetchColumnMetadata column) {
3099 ColumnMetadata.append(
3100 py::dict("ColumnName"_a = column.name, "DataType"_a = column.dataType,
3101 "ColumnSize"_a = column.columnSize, "DecimalDigits"_a = column.decimalDigits,Lines 4252-4260 4252
4253 {
4254 PERF_TIMER("FetchBatchData::cache_column_metadata");
4255 for (SQLUSMALLINT col = 0; col < numCols; col++) {
! 4256 const auto& columnMeta = GetFetchColumnMetadata(columnNames, col);
4257 columnInfos[col].dataType = GetFetchColumnType(columnMeta);
4258 columnInfos[col].columnSize = GetFetchColumnSize(columnMeta);
4259 columnInfos[col].isLob =
4260 std::find(lobColumns.begin(), lobColumns.end(), col + 1) != lobColumns.end();📋 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.4%
mssql_python.pybind.connection.connection_pool.cpp: 82.3%
mssql_python.pybind.connection.connection.cpp: 82.5%
mssql_python.row.py: 83.4%
mssql_python.logging.py: 86.2%
mssql_python.pooling.py: 90.1%🔗 Quick Links
|
PR Performance ReportNo consistent slowdowns detected across all 2 environments. Coverage: 2 of 2 environments completed. Advisory result; does not block merging.
Affected phases and call countsPhase times are inclusive diagnostics and must not be added together. They identify where measured time changed, not why it changed. Unix / SQL Server 2022Batched row fetching: ddbc::FetchBatchData::construct_rows +1.258 ms; ddbc::FetchBatchData +0.310 ms; ddbc::FetchBatchData::SQLFetchScroll_call +0.079 ms. Call changes: ddbc::SQLDescribeCol_wrap (added, removed, or intermittent). Unix / SQL Server 2025Batched row fetching: no measured phase delta. Call changes: ddbc::SQLDescribeCol_wrap (added, removed, or intermittent). All database tasks and timingsUnix / SQL Server 2022
Unix / SQL Server 2025
Build, commits 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 |
Work Item / Issue Reference
Summary
Keep
fetchmany()column metadata native within each call instead of building Python dictionaries and immediately unpacking them again in C++. Small batches repeat this setup frequently, so avoiding the roundtrip reduces driver-side work.The implementation shares one checked description loop and lets the existing binding and batch code read native fields. Fresh ODBC descriptions, eager Unicode-name conversion, public metadata output and existing fetch behavior are unchanged. There is no cross-call cache or hidden prefetch;
fetchall()and Arrow retain their existing setup.flowchart LR subgraph Before B1["Fresh descriptions"] --> B2["Python dictionaries"] --> B3["C++ field extraction"] --> B4["Bind/fetch"] end subgraph After A1["Fresh descriptions"] --> A2["Native fields + owned Unicode names"] --> A3["Same bind/fetch"] endMatched Windows x64/Python 3.13 OFF-Release measurements: 10,000 rows, 24 columns, 12 paired rounds and 9 drains per case. The table shows median fetch latency.
fetchmany(1)fetchall()controlSeparately instrumented counts for the
fetchmany(1)drain, including EOF, show metadata dictionary emissions falling from 240,024 to 0, while ODBC column descriptions remain 240,024. Owned Python Unicode names still exist.Draft, not merge-ready: same-build calibration was unstable and some controls were slower, so no-regression is not established. The wide-small-fetch result is promising, but it is not a universal-speedup claim.