Skip to content

PERF: Keep fetchmany column metadata native and call-local - #796

Draft
Jahnvi Thakkar (jahnvi480) wants to merge 1 commit into
mainfrom
jahnvi/perf-small-fetch-native-metadata
Draft

Jahnvi Thakkar (jahnvi480) wants to merge 1 commit 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

Pending maintainer-confirmed work item / issue reference (draft).


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"]
    end
Loading

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

Operation Before After Median change
fetchmany(1) 722.44 ms 252.47 ms -65.05%
fetchall() control 47.02 ms 61.84 ms +31.52%

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

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

Copy link
Copy Markdown

📊 Code Coverage Report

🔥 Diff Coverage

94%


🎯 Overall Coverage

83%


📈 Total Lines Covered: 8472 out of 10128
📁 Project: mssql-python


Diff Coverage

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

  • mssql_python/pybind/ddbc_bindings.cpp (94.8%): Missing lines 3059,3061-3062,3113

Summary

  • Total: 77 lines
  • Missing: 4 lines
  • Coverage: 94%

mssql_python/pybind/ddbc_bindings.cpp

Lines 3055-3066

  3055 
  3056 SQLULEN GetFetchColumnSize(const FetchColumnMetadata& column) {
  3057     return column.columnSize;
  3058 }
! 3059 
  3060 std::string GetFetchColumnName(const py::dict& column) {
! 3061     return column["ColumnName"].cast<std::string>();
! 3062 }
  3063 
  3064 std::string GetFetchColumnName(const FetchColumnMetadata& column) {
  3065     return column.name.cast<std::string>();
  3066 }

Lines 3109-3117

  3109 
  3110 }  // namespace
  3111 
  3112 // Wrap SQLDescribeCol
! 3113 SQLRETURN SQLDescribeCol_wrap(SqlHandlePtr StatementHandle, py::list& ColumnMetadata) {
  3114     PERF_TIMER("SQLDescribeCol_wrap");
  3115     return DescribeColumns(StatementHandle, [&](FetchColumnMetadata column) {
  3116         ColumnMetadata.append(
  3117             py::dict("ColumnName"_a = column.name, "DataType"_a = column.dataType,


📋 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: 61.5%
mssql_python.pybind.logger_bridge.hpp: 70.8%
mssql_python.row.py: 77.6%
mssql_python.pybind.ddbc_bindings.cpp: 77.9%
mssql_python.pybind.connection.connection_pool.cpp: 81.8%
mssql_python.logging.py: 86.2%
mssql_python.pooling.py: 90.1%
mssql_python.pybind.py_type_cache.hpp: 91.6%

🔗 Quick Links

⚙️ Build Summary 📋 Coverage Details

View Azure DevOps Build

Browse Full Coverage Report

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants