PERF: Optimize checked temporal fetch construction - #795
Open
Jahnvi Thakkar (jahnvi480) wants to merge 4 commits into
Open
Jahnvi Thakkar (jahnvi480) wants to merge 4 commits into
Jahnvi Thakkar (jahnvi480) wants to merge 4 commits into
Conversation
Use direct CPython date/time/datetime construction for exact cached standard types, preserving substituted constructors and exception behavior. Cover row-wise and batch fetch contracts in isolated subprocesses. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot started reviewing on behalf of
Jahnvi Thakkar (jahnvi480)
September 17, 2026 14:27
View session
Jahnvi Thakkar (jahnvi480)
changed the title
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
Native extension changes span multiple fetch paths, with limited platform and validation coverage.
Pull request overview
Optimizes native SQL temporal fetch construction while preserving custom constructors and conversion behavior.
Changes:
- Adds checked datetime construction helpers.
- Integrates them into six temporal fetch paths.
- Adds regression tests and changelog documentation.
File summaries
| File | Reviewed changes |
|---|---|
tests/test_038_fetch_temporal.py |
Temporal parity, constructor, exception, and fetch API coverage |
mssql_python/pybind/fetch_temporal.hpp |
Translation-unit-local checked construction helpers |
mssql_python/pybind/ddbc_bindings.cpp |
Integration into row-wise and batch temporal fetch paths |
CHANGELOG.md |
Documents the optimization and preserved behavior |
Review details
- Files reviewed: 4/4 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 4446-4455 4446 }
4447 case SQL_SS_TIME2: {
4448 const SQL_SS_TIME2_STRUCT& t2 = buffers.timeBuffers[col - 1][i];
4449 py::object timeObj =
! 4450 FetchTemporal::time(t2.hour, t2.minute, t2.second, t2.fraction / 1000);
! 4451 PyList_SET_ITEM(row, col - 1, timeObj.release().ptr());
4452 break;
4453 }
4454 case SQL_SS_TIMESTAMPOFFSET: {
4455 SQLULEN rowIdx = i;mssql_python/pybind/fetch_temporal.hppLines 13-23 13
14 // datetime.h keeps PyDateTimeAPI per translation unit, so these helpers must too.
15 static inline void ensure_datetime_api() {
16 if (PyDateTimeAPI == nullptr) {
! 17 PyDateTime_IMPORT;
! 18 if (PyDateTimeAPI == nullptr) throw py::error_already_set();
! 19 }
20 }
21
22 static inline py::object date(int year, int month, int day) {
23 ensure_datetime_api();📋 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.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.fetch_temporal.hpp: 92.1%🔗 Quick Links
|
Jahnvi Thakkar (jahnvi480)
marked this pull request as ready for review
September 18, 2026 03:55
Jahnvi Thakkar (jahnvi480)
requested review from
Gaurav Sharma (bewithgaurav) and
Sumit Sarabhai (sumitmsft)
September 18, 2026 03:55
PR Performance ReportNo consistent slowdowns detected. 2 inconsistent comparisons need review across 2 database tasks and 1 environments. Inconsistent slowdowns to review:
Coverage: 4 of 4 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. Windows / SQL Server 2025Legacy 100,000-row insertion: py::execute::cpp_call +71.059 ms; ddbc::SQLExecute_wrap +70.540 ms; ddbc::BindParameters +34.841 ms. All database tasks and timingsWindows / SQL Server 2022
Windows / SQL Server 2025
Unix / SQL Server 2022
Unix / SQL Server 2025
Build, commits and measurement detailsPR head:
A consistent slowdown requires more than 20% median paired slowdown, at least 1 ms between the median runtimes, and at least 80% of pairs exceeding the relative threshold. An inconsistent slowdown crosses the first two thresholds without enough pair agreement. 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 |
Copilot started reviewing on behalf of
Jahnvi Thakkar (jahnvi480)
September 18, 2026 07:48
View session
Copilot started reviewing on behalf of
Jahnvi Thakkar (jahnvi480)
September 18, 2026 09:22
View session
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The isolated Linux wheel-validation job fails on a source-tree-relative module path assertion.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Lite
| sys.path.insert(0, str(root)) | ||
| import mssql_python | ||
|
|
||
| assert Path(mssql_python.ddbc_bindings.module.__file__).resolve().is_relative_to(root) |
Copilot started reviewing on behalf of
Jahnvi Thakkar (jahnvi480)
September 18, 2026 10:32
View session
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Work Item / Issue Reference
Summary
Optimize the construction of Python DATE, TIME, and TIMESTAMP objects during fetching. The previous implementation returned correct temporal values but paid generic Python-call overhead for every non-NULL value. This change removes that intermediate work for standard datetime types while preserving the original route for substituted constructors.
1. Before: what was slow?
ODBC already provides the date/time fields as native numbers. The driver nevertheless invoked the cached Python constructor through pybind11 for every value:
PyTypeCache::get_date_class_obj()(year, month, day)The constructor was already cached; repeated imports were not the problem. The remaining work was converting C++ numbers to Python arguments, arranging positional arguments, dispatching a generic callable, parsing those arguments, and managing their references. DATE uses three arguments, TIME four, and TIMESTAMP seven. Small integers may be reused, so not every argument requires a fresh allocation. The standard datetime constructor itself is implemented in C; the avoidable cost is its general-purpose calling route.
flowchart TD A["ODBC temporal structure"] --> B["Existing success and NULL checks"] B -->|"Non-NULL"| C["Native numeric fields"] C --> D["Cached constructor through pybind11"] D --> E["Convert fields into Python arguments"] E --> F["Generic callable dispatch and argument parsing"] F --> G["Validate fields and allocate temporal object"] G --> H["Existing result-row insertion"] B -->|"NULL"| N["Existing None handling"]2. After: what changed?
The new
fetch_temporal.hpphelper checks the identity of the actual cached constructor. For the exact standard Python type, it constructs the object directly from native integers using:PyDate_FromDatePyTime_FromTimePyDateTime_FromDateAndTimeIf the cached constructor has been substituted, the original callable still runs with the original arguments, return-object handling, and propagated exceptions.
flowchart TD A["Same ODBC temporal structure"] --> B["Unchanged success and NULL checks"] B -->|"Non-NULL"| C["Same native fields and fraction / 1000"] C --> D["Ensure this translation unit's datetime API is initialized"] D --> E{"Cached constructor is the exact standard type?"} E -->|"Yes: fast path"| F["Pass native integers directly to CPython construction API"] F --> G["Validate fields and allocate temporal object"] G --> H["Check result and own the new reference"] E -->|"No: compatibility path"| I["Original cached callable with 3/4/7 positional arguments"] H --> J["Existing result-row insertion"] I --> J B -->|"NULL"| N["Unchanged None handling"]Why this is faster: standard-type conversion no longer packages native fields as Python call arguments or performs generic callable dispatch/argument parsing. Field validation and the final Python object allocation still happen. A small saving per value repeats thousands of times; bulk fetching amortizes other per-call overhead, whereas repeated
fetchonestill pays that surrounding overhead for every row.For example, the frozen 4,000-row x 8-column DATE workload, with every seventh row NULL, contains 27,432 non-NULL DATE values. That is a calculated workload count, not a native instrumentation result. Each value still produces one Python object; only its construction route changes. There is no claim of fewer SQL executions, ODBC fetches, or network round trips.
3. Files changed and behavior preserved
mssql_python/pybind/fetch_temporal.hppmssql_python/pybind/ddbc_bindings.cppSQLGetDataconversion and batch-buffer conversion.tests/test_038_fetch_temporal.pyCHANGELOG.mdExisting GIL scopes, SQL/NULL checks, buffer layouts, fraction/1000 nanosecond-to-microsecond truncation, naive timezone/fold behavior, and result positioning are unchanged.
steal()/py::objectown new references; batch insertion releases each exactly once. Construction failures propagate rather than returning an invalid object. DATETIMEOFFSET, UUID, Decimal, text, Arrow, Row construction, and settings/converter caches are unchanged.4. Measured improvement: actual baseline versus this change
Platform/build: Windows x64; CPython 3.13.15; pybind11 3.0.1; SQL Server 15.0.4382.1; ODBC 18.6.2.1. Both builds use matching MSVC Release x64/C++17 settings,
/O2 /Ob2 /DNDEBUG, andENABLE_PROFILING=OFF.Revisions: pristine baseline
e279a4f6c12241603b978602acf01e64520a819f; measured temporal-only candidate subsequently committed unchanged asf8e455677f64eb558a9ecc6c0ff3a8721cbaca9e. These measurements are not from a combined optimization branch.Method: 12 counterbalanced paired rounds, nine samples per case per round, one untimed warmup per case, and fresh worker processes. Every result's values, types, and row count are checked outside timing. All ordered raw samples are retained, without outlier removal. One predeclared primary confirmation followed noisy calibration. The table reports complete fetch/drain time excluding execute and validation; repeated fetchone includes the caller's collection loop. Positive reduction means less time.
Every listed workload returns 4,000 rows, with every seventh row NULL. Mixed temporal has four columns: DATE, TIME(7), DATETIME2(7), and unchanged DATETIMEOFFSET. Pure-type workloads have eight columns. Requested rows/call is a public API setting, not a claim about internal native rowset size.
Point reductions are ratios of medians. Intervals use paired per-round median reductions with 5,000 bootstrap resamples and seed 558, a distinct estimator. Supplemental cases use the same builds and 12x9 protocol but are separate measurements, not another primary confirmation. Their percentages are not added to the primary results. DATE fetchall and pure TIME fetchall intervals include zero: those point gains remain inconclusive.
Across the confirmation's six temporal cases and nine unchanged narrow/converter controls:
These are fixed-suite sums, not one query's latency or an application-wide speedup. First-result windows are retained in the study artifacts; no first-result improvement is claimed here.
Measured constructor counts: fresh-process correctness tests assert the same fallback behavior on both builds. Each main control query makes three substituted DATE calls with three positional arguments, three TIME calls with four, and three TIMESTAMP calls with seven. Unchanged DTO makes three eight-argument calls; UUID makes three
bytes-keyword calls. NULLs invoke none. These are measured fallback-parity counts, not an instrumented count of standard-path generic-call elimination. Timing builds contain no instrumentation.Uncertainty and negative controls: identical-build A/A calibration was very noisy, with a paired score interval of 0.828x to 1.951x. Unchanged confirmation controls include negative point reductions: narrow fetchone -1.19%, many(1) -1.14%, fetchall -2.79%, converter many -4.63%, and converter all -2.39%; their intervals span zero. No further timing retries were used to select a favorable outcome. The results support scoped temporal gains, but do not establish a blanket no-regression guarantee, universal speedup, or performance improvement on other operating systems.
Local validation
Retained validation limitations