CHORE: TEST PR Performance Report control path - #799
Gaurav Sharma (bewithgaurav) wants to merge 24 commits into
Conversation
… on SUCCESS, __slots__ Row, and C++ Row construction - Cache decoding encoding strings in cursor __init__ to avoid 2 method calls + 2 dict.get() per fetch - Skip DDBCSQLGetAllDiagRecords on SQL_SUCCESS (ODBC spec: zero records on SUCCESS) - Replace param.encode('ascii') try/except with str.isascii() (C-level check) - Class-level _SQL_TO_C_TYPE lookup table (built once, shared across cursors) - Add __slots__ to Row class (eliminates per-instance __dict__, ~232 bytes/row savings) - Add Row._fast_create static method (bypasses __init__ for common case) - Add C++ construct_rows function (builds Row objects in tight C loop, avoiding Python loop overhead) - Zero-copy Row fast path when no converters/UUID processing needed Benchmark results (5-run average, richbench repeat=5 number=5): - Fetch one: -1.7x -> -1.4x (18% improvement) - Fetch many: -1.7x -> -1.3x (24% improvement) - 100 inserts: 4.9x -> 5.6x (14% faster) - SELECT: -1.1x -> -1.0x (on par with pyodbc) Profiler wall clock (50K rows): - fetchall: 176.7ms -> 158.1ms (11% faster) - fetchmany: 166.6ms -> 138.6ms (17% faster) No overlap with PR #549 (execute fast path) or PR #526 (simdutf).
Preserve late output converter fallback behavior and UUID conversion while retaining the no-converter fast path. Add regression and cache operation-count coverage for all fetch APIs. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Resolve fetch and Row conflicts while retaining current-main converter dispatch, lowercase column maps, profiling scopes, and CHAR decoding ctype. Extend fetch fast paths and regression coverage for the merged behavior. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Validate the native row type argument, restore unconditional diagnostic retrieval, raise fetch errors before row processing, and retain fast paths when registered converters do not match. Add isolated crash and fetch contract regressions. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Preserve scalar SQL NULL values without suppressing fetch errors. Cover fixed-width types, LOB and bound fetch paths, literal NULL and OBJECT_ID results, and cursor recovery. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Create a disposable documentation-only change to validate the PR Performance Report end to end. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
PR Performance ReportThis PR has 4 consistent improvement signals across 2 database tasks and 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 2022Insertion with explicit input sizes: py::execute::cpp_call -35.979 ms; ddbc::SQLExecute_wrap -35.461 ms; ddbc::BindParameters -9.503 ms. Unix / SQL Server 2025Insertion with explicit input sizes: py::execute::cpp_call -39.267 ms; ddbc::SQLExecute_wrap -38.696 ms; ddbc::BindParameters -5.345 ms. 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 |
There was a problem hiding this comment.
🟢 Approval recommended
No unresolved issues; the change is limited to a non-rendered README marker.
Pull request overview
Adds an invisible README marker to validate the profiler report control path without changing product code.
Changes:
- Adds an HTML comment canary to
README.md.
File summaries
| File | Description |
|---|---|
README.md |
Adds the invisible profiler control marker. |
Review details
- Files reviewed: 1/1 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.
📊 Code Coverage Report
Diff CoverageDiff: main...HEAD, staged and unstaged changes
Summary
mssql_python/pybind/ddbc_bindings.cppLines 3630-3640 3630 case SQL_SMALLINT: {
3631 SQLSMALLINT smallIntValue;
3632 SQLLEN indicator = 0;
3633 ret = SQLGetData_ptr(hStmt, i, SQL_C_SHORT, &smallIntValue, 0, &indicator);
! 3634 if (SQL_SUCCEEDED(ret) && indicator == SQL_NULL_DATA) {
! 3635 row.append(py::none());
! 3636 break;
3637 }
3638 if (SQL_SUCCEEDED(ret)) {
3639 row.append(static_cast<int>(smallIntValue));
3640 } else {Lines 3646-3659 3646 break;
3647 }
3648 case SQL_REAL: {
3649 SQLREAL realValue;
! 3650 SQLLEN indicator = 0;
! 3651 ret = SQLGetData_ptr(hStmt, i, SQL_C_FLOAT, &realValue, 0, &indicator);
! 3652 if (SQL_SUCCEEDED(ret) && indicator == SQL_NULL_DATA) {
3653 row.append(py::none());
3654 break;
! 3655 }
3656 if (SQL_SUCCEEDED(ret)) {
3657 row.append(realValue);
3658 } else {
3659 LOG("SQLGetData: Error retrieving SQL_REAL for column %d - "📋 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.5%
mssql_python.pybind.connection.connection_pool.cpp: 82.3%
mssql_python.pybind.connection.connection.cpp: 82.5%
mssql_python.logging.py: 86.2%
mssql_python.pooling.py: 90.1%
mssql_python.pybind.py_type_cache.hpp: 91.6%🔗 Quick Links
|
Merge main 2a86fc1 while preserving fetch cache and NULL fixes. Share canonical Row mapping snapshots through Python and native fast constructors, including legacy constructor defaults. Retain main's pooled transaction cleanup changes. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Replace process-static Python attribute names with scoped owning handles outside the row loop. Cover repeated batch construction and reference cleanup when slot assignment fails. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The PR scope does not match its stated canary purpose, and Row changes may break existing public API behavior.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Open (1)
| @@ -1,4 +1,5 @@ | |||
| # Microsoft Python Driver for SQL Server | |||
| <!-- profiler-report-control-canary: no product code changed --> | |||
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
|
||
| # __slots__ eliminates per-instance __dict__ (~232 bytes/row savings), | ||
| # and makes attribute access ~30% faster (array index vs dict lookup). | ||
| __slots__ = ("_values", "_column_map", "_cursor", "_column_map_lower", "_column_names") |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The scope is not README-only, report coverage is incomplete, and the Row changes may break public API compatibility.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
| f"<kbd>{improvement_tasks} IMPROVEMENT" | ||
| f"{'S' if improvement_tasks != 1 else ''}</kbd> " | ||
| f"<kbd>{regression_tasks} SLOWDOWN" | ||
| f"{'S' if regression_tasks != 1 else ''}</kbd> " | ||
| f"<kbd>{len(completed)}/{len(LEGS)} ENVIRONMENTS</kbd>", |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Critical CI coverage changes and unresolved fetch-performance and Row compatibility regressions must be addressed.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 2
| jobs: | ||
| - job: CodeQLAnalysis | ||
| displayName: 'CodeQL Security Analysis' | ||
| condition: false |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>



Work Item / Issue Reference
Summary
Disposable control canary for the merged PR Performance Report. This branch changes only an invisible README marker, so the report should complete across five environments without detecting a performance regression. This PR will not be merged.