From f8e455677f64eb558a9ecc6c0ff3a8721cbaca9e Mon Sep 17 00:00:00 2001 From: Jahnvi Thakkar Date: Thu, 17 Sep 2026 19:55:55 +0530 Subject: [PATCH 1/3] REFACTOR: Optimize checked temporal fetch construction 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> --- CHANGELOG.md | 4 + mssql_python/pybind/ddbc_bindings.cpp | 38 ++-- mssql_python/pybind/fetch_temporal.hpp | 59 ++++++ tests/test_038_fetch_temporal.py | 262 +++++++++++++++++++++++++ 4 files changed, 340 insertions(+), 23 deletions(-) create mode 100644 mssql_python/pybind/fetch_temporal.hpp create mode 100644 tests/test_038_fetch_temporal.py diff --git a/CHANGELOG.md b/CHANGELOG.md index aab046b3a..efab12f4d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -57,6 +57,10 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), does not change the default provider or ship any Rust driver binaries. ### Changed +- DATE, TIME, and TIMESTAMP fetch conversion uses checked CPython constructors + for the standard datetime types, while preserving cached substitute constructors, + their positional arguments and exceptions, and fractional-second truncation. + DATETIMEOFFSET, UUID, and Decimal conversion are unchanged. - `mssql-python` now depends on `mssql-python-rs==0.1.0` for `mssql_py_core` instead of embedding files owned by that separately published distribution. - **GH-769 deprecation policy:** The misplaced `GetInfoConstants` members diff --git a/mssql_python/pybind/ddbc_bindings.cpp b/mssql_python/pybind/ddbc_bindings.cpp index 35a3b6da4..db1447081 100644 --- a/mssql_python/pybind/ddbc_bindings.cpp +++ b/mssql_python/pybind/ddbc_bindings.cpp @@ -12,6 +12,7 @@ #include "param_detect.hpp" #include "py_ref.hpp" #include "py_type_cache.hpp" +#include "fetch_temporal.hpp" #include "utf_utils.h" #include // std::min @@ -3758,8 +3759,8 @@ SQLRETURN SQLGetData_wrap(SqlHandlePtr StatementHandle, SQLUSMALLINT colCount, p ret = SQLGetData_ptr(hStmt, i, SQL_C_TYPE_DATE, &dateValue, sizeof(dateValue), NULL); if (SQL_SUCCEEDED(ret)) { - row.append(PyTypeCache::get_date_class_obj()(dateValue.year, dateValue.month, - dateValue.day)); + row.append( + FetchTemporal::date(dateValue.year, dateValue.month, dateValue.day)); } else { row.append(py::none()); } @@ -3771,7 +3772,7 @@ SQLRETURN SQLGetData_wrap(SqlHandlePtr StatementHandle, SQLUSMALLINT colCount, p SQLLEN indicator = 0; ret = SQLGetData_ptr(hStmt, i, SQL_C_SS_TIME2, &t2, sizeof(t2), &indicator); if (SQL_SUCCEEDED(ret) && indicator != SQL_NULL_DATA) { - row.append(PyTypeCache::get_time_class_obj()( + row.append(FetchTemporal::time( t2.hour, t2.minute, t2.second, t2.fraction / 1000)); // ns to µs } else { if (!SQL_SUCCEEDED(ret)) { @@ -3790,7 +3791,7 @@ SQLRETURN SQLGetData_wrap(SqlHandlePtr StatementHandle, SQLUSMALLINT colCount, p ret = SQLGetData_ptr(hStmt, i, SQL_C_TYPE_TIMESTAMP, ×tampValue, sizeof(timestampValue), NULL); if (SQL_SUCCEEDED(ret)) { - row.append(PyTypeCache::get_datetime_class_obj()( + row.append(FetchTemporal::datetime( timestampValue.year, timestampValue.month, timestampValue.day, timestampValue.hour, timestampValue.minute, timestampValue.second, timestampValue.fraction / 1000 // Convert back ns to µs @@ -4428,32 +4429,23 @@ SQLRETURN FetchBatchData(SQLHSTMT hStmt, ColumnBuffers& buffers, py::list& colum case SQL_TYPE_TIMESTAMP: case SQL_DATETIME: { const SQL_TIMESTAMP_STRUCT& ts = buffers.timestampBuffers[col - 1][i]; - PyObject* datetimeObj = PyTypeCache::get_datetime_class_obj()( - ts.year, ts.month, ts.day, ts.hour, ts.minute, - ts.second, ts.fraction / 1000) - .release() - .ptr(); - PyList_SET_ITEM(row, col - 1, datetimeObj); + py::object datetimeObj = FetchTemporal::datetime( + ts.year, ts.month, ts.day, ts.hour, ts.minute, ts.second, + ts.fraction / 1000); + PyList_SET_ITEM(row, col - 1, datetimeObj.release().ptr()); break; } case SQL_TYPE_DATE: { - PyObject* dateObj = - PyTypeCache::get_date_class_obj()(buffers.dateBuffers[col - 1][i].year, - buffers.dateBuffers[col - 1][i].month, - buffers.dateBuffers[col - 1][i].day) - .release() - .ptr(); - PyList_SET_ITEM(row, col - 1, dateObj); + const SQL_DATE_STRUCT& value = buffers.dateBuffers[col - 1][i]; + py::object dateObj = FetchTemporal::date(value.year, value.month, value.day); + PyList_SET_ITEM(row, col - 1, dateObj.release().ptr()); break; } case SQL_SS_TIME2: { const SQL_SS_TIME2_STRUCT& t2 = buffers.timeBuffers[col - 1][i]; - PyObject* timeObj = - PyTypeCache::get_time_class_obj()(t2.hour, t2.minute, t2.second, - t2.fraction / 1000) // ns to µs - .release() - .ptr(); - PyList_SET_ITEM(row, col - 1, timeObj); + py::object timeObj = + FetchTemporal::time(t2.hour, t2.minute, t2.second, t2.fraction / 1000); + PyList_SET_ITEM(row, col - 1, timeObj.release().ptr()); break; } case SQL_SS_TIMESTAMPOFFSET: { diff --git a/mssql_python/pybind/fetch_temporal.hpp b/mssql_python/pybind/fetch_temporal.hpp new file mode 100644 index 000000000..473069117 --- /dev/null +++ b/mssql_python/pybind/fetch_temporal.hpp @@ -0,0 +1,59 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT license. + +#pragma once + +#include +#include + +#include "py_ref.hpp" +#include "py_type_cache.hpp" + +namespace FetchTemporal { + +// datetime.h keeps PyDateTimeAPI per translation unit, so these helpers must too. +static inline void ensure_datetime_api() { + if (PyDateTimeAPI == nullptr) { + PyDateTime_IMPORT; + if (PyDateTimeAPI == nullptr) throw py::error_already_set(); + } +} + +static inline py::object date(int year, int month, int day) { + ensure_datetime_api(); + // Cached substitutes must still receive the original constructor call. + if (PyTypeCache::get_date_class() != + reinterpret_cast(PyDateTimeAPI->DateType)) { + return PyTypeCache::get_date_class_obj()(year, month, day); + } + py::object result = steal(PyDate_FromDate(year, month, day)); + if (!result) throw py::error_already_set(); + return result; +} + +static inline py::object time(int hour, int minute, int second, int microsecond) { + ensure_datetime_api(); + if (PyTypeCache::get_time_class() != + reinterpret_cast(PyDateTimeAPI->TimeType)) { + return PyTypeCache::get_time_class_obj()(hour, minute, second, microsecond); + } + py::object result = steal(PyTime_FromTime(hour, minute, second, microsecond)); + if (!result) throw py::error_already_set(); + return result; +} + +static inline py::object datetime(int year, int month, int day, int hour, int minute, int second, + int microsecond) { + ensure_datetime_api(); + if (PyTypeCache::get_datetime_class() != + reinterpret_cast(PyDateTimeAPI->DateTimeType)) { + return PyTypeCache::get_datetime_class_obj()(year, month, day, hour, minute, second, + microsecond); + } + py::object result = + steal(PyDateTime_FromDateAndTime(year, month, day, hour, minute, second, microsecond)); + if (!result) throw py::error_already_set(); + return result; +} + +} // namespace FetchTemporal diff --git a/tests/test_038_fetch_temporal.py b/tests/test_038_fetch_temporal.py new file mode 100644 index 000000000..074a324d5 --- /dev/null +++ b/tests/test_038_fetch_temporal.py @@ -0,0 +1,262 @@ +"""Temporal fetch parity, including cached constructors in fresh interpreters.""" + +import datetime +from decimal import Decimal +import gc +import json +import os +from pathlib import Path +import subprocess +import sys +import uuid + +import pytest + +MODES = ("default", "custom", "date", "time", "datetime", "uuid") +APIS = ("fetchone", "fetchmany", "fetchall", "iteration") + + +@pytest.mark.parametrize("mode", MODES) +def test_fetch_temporal_constructors(mode): + if not os.environ.get("DB_CONNECTION_STRING"): + pytest.skip("DB_CONNECTION_STRING is required") + result = subprocess.run( + [sys.executable, str(Path(__file__).resolve()), mode], + capture_output=True, + text=True, + encoding="utf-8", + timeout=120, + env={**os.environ, "PYTHONIOENCODING": "utf-8"}, + ) + assert result.returncode == 0, result.stdout + result.stderr + report = json.loads(result.stdout) + assert report["mode"] == mode + assert report["checks"] == 24 + + +def _drain(cursor, method): + if method == "iteration": + return [tuple(row) for row in cursor] + result = [] + while True: + if method == "fetchone": + row = cursor.fetchone() + batch = [] if row is None else [row] + elif method == "fetchmany": + batch = cursor.fetchmany(2) + else: + batch = cursor.fetchall() + if not batch: + return result + result.extend(tuple(row) for row in batch) + + +def _probe(mode, root): + originals = { + "date": datetime.date, + "time": datetime.time, + "datetime": datetime.datetime, + "uuid": uuid.UUID, + } + replacements = dict(originals) + calls = [] + state = {"record": False, "raise": None} + + class ConstructorFailure(Exception): + pass + + failure = ConstructorFailure("cached constructor failure") + + def record(name, positional, keywords): + if state["record"]: + calls.append((name, positional, keywords)) + if state["raise"] == name: + raise failure + + if mode != "default": + + def substitute(name, original): + class RecordingTemporal(original): + def __new__(cls, *positional, **keywords): + record(name, positional, keywords) + return original.__new__(cls, *positional, **keywords) + + return RecordingTemporal + + for name in ("date", "time", "datetime"): + replacements[name] = substitute(name, originals[name]) + setattr(datetime, name, replacements[name]) + + class RecordingUUID(originals["uuid"]): + def __init__(self, *, bytes): + record("uuid", (), {"bytes": bytes}) + super().__init__(bytes=bytes) + + replacements["uuid"] = RecordingUUID + uuid.UUID = RecordingUUID + + # Substitutions must precede native module initialization, not just connection creation. + sys.path.insert(0, str(root)) + import mssql_python + + assert Path(mssql_python.ddbc_bindings.module.__file__).resolve().is_relative_to(root) + mssql_python.native_uuid = True + query = """ + SELECT CAST(v.d AS date) AS d, CAST(v.t AS time(7)) AS t, + CAST(v.ts AS datetime2(7)) AS ts, CAST(v.u AS uniqueidentifier) AS u, + CAST(v.n AS nvarchar(64)) AS n, CAST(v.v AS varchar(64)) AS v, + CAST(v.b AS varbinary(64)) AS b, + CASE WHEN v.id = 4 THEN NULL ELSE CAST(123.4567 AS decimal(12,4)) END AS dec, + CASE WHEN v.id = 4 THEN NULL ELSE + CAST('2024-02-29T12:34:56.1234567+05:30' AS datetimeoffset(7)) END AS dto + {extra} + FROM (VALUES + (1, '0001-01-01', '00:00:00.0000000', '0001-01-01T00:00:00.0000000', + '00112233-4455-6677-8899-aabbccddeeff', N'A' + NCHAR(0) + N'\U0001f642', 'abc', 0x00FF), + (2, '2000-02-29', '12:34:56.1234567', '2000-02-29T12:34:56.1234567', + 'ffffffff-ffff-ffff-ffff-ffffffffffff', N'', '', 0x), + (3, '9999-12-31', '23:59:59.9999999', '9999-12-31T23:59:59.9999999', + '00000000-0000-0000-0000-000000000000', N'caf\u00e9', 'xyz', 0x000100), + (4, NULL, NULL, NULL, NULL, NULL, NULL, NULL) + ) AS v(id, d, t, ts, u, n, v, b) + ORDER BY v.id + """ + date, time, timestamp, guid = (originals[k] for k in ("date", "time", "datetime", "uuid")) + dto = timestamp( + 2024, 2, 29, 12, 34, 56, 123456, datetime.timezone(datetime.timedelta(minutes=330)) + ) + expected = [ + ( + date(1, 1, 1), + time(), + timestamp(1, 1, 1), + guid("00112233-4455-6677-8899-aabbccddeeff"), + "A\0\U0001f642", + "abc", + b"\0\xff", + ), + ( + date(2000, 2, 29), + time(12, 34, 56, 123456), + timestamp(2000, 2, 29, 12, 34, 56, 123456), + guid("ffffffff-ffff-ffff-ffff-ffffffffffff"), + "", + "", + b"", + ), + ( + date(9999, 12, 31), + time(23, 59, 59, 999999), + timestamp(9999, 12, 31, 23, 59, 59, 999999), + guid("00000000-0000-0000-0000-000000000000"), + "caf\u00e9", + "xyz", + b"\0\1\0", + ), + ] + expected = [row + (Decimal("123.4567"), dto) for row in expected] + [(None,) * 9] + checks = 0 + recovery_checks = 0 + with mssql_python.connect(os.environ["DB_CONNECTION_STRING"]) as connection: + # The ASCII VARCHAR control also exercises SQL_CHAR without the Windows UTF-8 upgrade. + for encoding, ctype in ( + ("utf-16le", mssql_python.SQL_WCHAR), + ("latin-1", mssql_python.SQL_CHAR), + ): + connection.setdecoding(mssql_python.SQL_CHAR, encoding=encoding, ctype=ctype) + for forced_lob in (False, True): + extra = ", CAST(N'lob' AS nvarchar(max)) AS force_lob" if forced_lob else "" + wanted = [row + (("lob",) if forced_lob else ()) for row in expected] + for method in APIS: + with connection.cursor() as cursor: + cursor.execute(query.format(extra=extra)) + if mode not in ("default", "custom"): + state.update(record=True, **{"raise": mode}) + try: + _drain(cursor, method) + except ConstructorFailure as error: + assert error is failure + else: + raise AssertionError("Constructor failure was swallowed") + finally: + state.update(record=False, **{"raise": None}) + recovery_checks += 1 + cursor.execute(query.format(extra=extra)) + calls.clear() + state["record"] = True + try: + actual = _drain(cursor, method) + finally: + state["record"] = False + assert actual == wanted, (method, encoding, forced_lob, actual) + for row in actual[:3]: + for index, name in enumerate(("date", "time", "datetime", "uuid")): + assert type(row[index]) is replacements[name], (method, name) + for index in (1, 2): + assert row[index].tzinfo is None and row[index].fold == 0 + assert type(row[7]) is Decimal + assert type(row[8]) is replacements["datetime"] + assert row[8].utcoffset() == datetime.timedelta(minutes=330) + assert row[8].fold == 0 + assert all(type(row[i]) is str for i in (4, 5)) + assert type(row[6]) is bytes + if mode != "default": + for name, count in (("date", 3), ("time", 4), ("datetime", 7)): + recorded = [ + call for call in calls if call[0] == name and len(call[1]) != 8 + ] + assert len(recorded) == 3, (method, name, recorded) + assert all(len(p) == count and not k for _, p, k in recorded) + dto_calls = [call for call in calls if len(call[1]) == 8] + assert len(dto_calls) == 3 + assert all(n == "datetime" and not k for n, _, k in dto_calls) + uuid_calls = [call for call in calls if call[0] == "uuid"] + assert len(uuid_calls) == 3 + assert [k["bytes"] for _, _, k in uuid_calls] == [ + row[3].bytes for row in wanted[:3] + ] + assert all(not p and set(k) == {"bytes"} for _, p, k in uuid_calls) + checks += 1 + gc.collect() + + legacy_query = """ + SELECT CAST(v.dt AS datetime), CAST(v.small AS smalldatetime) {extra} + FROM (VALUES + (1, '1753-01-01T00:00:00.000', '1900-01-01T00:00:00'), + (2, '9999-12-31T23:59:59.997', '2079-06-06T23:59:00'), + (3, NULL, NULL) + ) AS v(id, dt, small) ORDER BY v.id + """ + legacy = [ + (timestamp(1753, 1, 1), timestamp(1900, 1, 1)), + (timestamp(9999, 12, 31, 23, 59, 59, 997000), timestamp(2079, 6, 6, 23, 59)), + (None, None), + ] + for forced_lob in (False, True): + extra = ", CAST(N'lob' AS nvarchar(max))" if forced_lob else "" + wanted = [row + (("lob",) if forced_lob else ()) for row in legacy] + for method in APIS: + with connection.cursor() as cursor: + cursor.execute(legacy_query.format(extra=extra)) + calls.clear() + state["record"] = True + try: + actual = _drain(cursor, method) + finally: + state["record"] = False + assert actual == wanted + for row in actual[:2]: + for value in row[:2]: + assert type(value) is replacements["datetime"] + assert value.tzinfo is None and value.fold == 0 + if mode != "default": + assert len(calls) == 4 + assert all(n == "datetime" and len(p) == 7 and not k for n, p, k in calls) + checks += 1 + assert recovery_checks == (16 if mode not in ("default", "custom") else 0) + print(json.dumps({"mode": mode, "checks": checks, "recovery_checks": recovery_checks})) + + +if __name__ == "__main__": + root = Path(sys.argv[2]).resolve() if len(sys.argv) > 2 else Path(__file__).resolve().parents[1] + _probe(sys.argv[1], root) From bc47daa4be12926321741ac8e20a43d92f03285f Mon Sep 17 00:00:00 2001 From: Jahnvi Thakkar Date: Mon, 21 Sep 2026 15:26:57 +0530 Subject: [PATCH 2/3] FIX: Validate temporal test native provenance for installed wheels Preserve strict source-snapshot checks while allowing isolated wheel tests to validate the native module against its imported package. Add focused source and installed layout regression coverage. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- tests/test_038_fetch_temporal.py | 76 ++++++++++++++++++++++++++++++-- 1 file changed, 73 insertions(+), 3 deletions(-) diff --git a/tests/test_038_fetch_temporal.py b/tests/test_038_fetch_temporal.py index 074a324d5..79e3a954c 100644 --- a/tests/test_038_fetch_temporal.py +++ b/tests/test_038_fetch_temporal.py @@ -16,6 +16,71 @@ APIS = ("fetchone", "fetchmany", "fetchall", "iteration") +def _assert_native_origin(package_file, native_file, root, *, require_source=False): + package_path = Path(package_file).resolve() + native_path = Path(native_file).resolve() + assert package_path.is_file(), f"Imported package does not exist: {package_path}" + assert native_path.is_file(), f"Imported native module does not exist: {native_path}" + assert ( + native_path.parent == package_path.parent + ), f"Native module {native_path} is outside imported package {package_path.parent}" + assert native_path.name.startswith("ddbc_bindings.") and native_path.suffix in ( + ".so", + ".pyd", + ), f"Expected the compiled native module, not its Python loader: {native_path}" + source_package = (root / "mssql_python" / "__init__.py").resolve() + # Explicit snapshots and source checkouts must not silently import an installed wheel. + if require_source or source_package.is_file(): + assert ( + package_path == source_package + ), f"Expected source package {source_package}, imported {package_path}" + + +@pytest.mark.parametrize("layout", ("source", "explicit-source", "installed-wheel")) +@pytest.mark.parametrize("suffix", (".so", ".pyd")) +def test_native_origin_accepts_package_layout(tmp_path, layout, suffix): + root = tmp_path / "tests-root" + location = root if layout != "installed-wheel" else tmp_path / "site-packages" + package = location / "mssql_python" / "__init__.py" + package.parent.mkdir(parents=True) + package.touch() + native = package.with_name(f"ddbc_bindings.cp313-test{suffix}") + native.touch() + _assert_native_origin(package, native, root, require_source=layout == "explicit-source") + + +@pytest.mark.parametrize( + "problem, message", + ( + ("missing-native", "native module does not exist"), + ("foreign-native", "outside imported package"), + ("python-loader", "Expected the compiled native module"), + ("shadowed-source", "Expected source package"), + ("missing-explicit-source", "Expected source package"), + ), +) +def test_native_origin_rejects_mismatched_package(tmp_path, problem, message): + root = tmp_path / "tests-root" + package = tmp_path / "site-packages" / "mssql_python" / "__init__.py" + package.parent.mkdir(parents=True) + package.touch() + native = package.with_name("ddbc_bindings.cp313-test.so") + if problem == "foreign-native": + native = tmp_path / native.name + elif problem == "python-loader": + native = package.with_name("ddbc_bindings.py") + if problem != "missing-native": + native.touch() + if problem == "shadowed-source": + source_package = root / "mssql_python" / "__init__.py" + source_package.parent.mkdir(parents=True) + source_package.touch() + with pytest.raises(AssertionError, match=message): + _assert_native_origin( + package, native, root, require_source=problem == "missing-explicit-source" + ) + + @pytest.mark.parametrize("mode", MODES) def test_fetch_temporal_constructors(mode): if not os.environ.get("DB_CONNECTION_STRING"): @@ -51,7 +116,7 @@ def _drain(cursor, method): result.extend(tuple(row) for row in batch) -def _probe(mode, root): +def _probe(mode, root, *, require_source=False): originals = { "date": datetime.date, "time": datetime.time, @@ -99,7 +164,12 @@ def __init__(self, *, bytes): sys.path.insert(0, str(root)) import mssql_python - assert Path(mssql_python.ddbc_bindings.module.__file__).resolve().is_relative_to(root) + _assert_native_origin( + mssql_python.__file__, + mssql_python.ddbc_bindings.module.__file__, + root, + require_source=require_source, + ) mssql_python.native_uuid = True query = """ SELECT CAST(v.d AS date) AS d, CAST(v.t AS time(7)) AS t, @@ -259,4 +329,4 @@ def __init__(self, *, bytes): if __name__ == "__main__": root = Path(sys.argv[2]).resolve() if len(sys.argv) > 2 else Path(__file__).resolve().parents[1] - _probe(sys.argv[1], root) + _probe(sys.argv[1], root, require_source=len(sys.argv) > 2) From c76ce3eb12e6aaa80ab93d82f63321209fc69ac2 Mon Sep 17 00:00:00 2001 From: Gaurav Sharma Date: Tue, 22 Sep 2026 12:54:53 +0530 Subject: [PATCH 3/3] STYLE: Highlight PR Performance Report signals (#805) ### Work Item / Issue Reference > [AB#44819](https://sqlclientdrivers.visualstudio.com/c6d89619-62de-46a0-8b46-70b92a84d85e/_workitems/edit/44819) ------------------------------------------------------------------- ### Summary Improve the deterministic PR Performance Report presentation while preserving its existing measurements, thresholds, and advisory behavior. - Add explicit success, regression, review, unavailable, and clean verdict headings. - Summarize signal counts with compact badges. - Add a task-by-environment signal fingerprint without directional arrows or a spread column. - Keep exact before/after values in the expandable measured-timings table. - Rename and emphasize the diagnostic, complete-results, and build-detail sections. - Run same-repository report workflows from PR code so formatter changes can be validated end to end. Fork pull requests continue to use trusted base-branch code. This PR is stacked on #795. When merged into that branch, #795 will rerun its performance publisher using this formatter. After #795 reaches `main`, other pull requests receive the format when they synchronize with `main`. **Validation** - 118 profiler CI contract tests passed. - Workflow YAML parsing, Black, Flake8, and diff checks passed. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .github/workflows/pr-profiler-report.yml | 12 +- eng/profiler_benchmarks/README.md | 12 +- eng/profiler_benchmarks/report.py | 151 ++++++++++++++--------- tests/test_036_profiler_ci.py | 90 ++++++++------ 4 files changed, 164 insertions(+), 101 deletions(-) diff --git a/.github/workflows/pr-profiler-report.yml b/.github/workflows/pr-profiler-report.yml index d14dd5770..1e0130d2d 100644 --- a/.github/workflows/pr-profiler-report.yml +++ b/.github/workflows/pr-profiler-report.yml @@ -1,7 +1,10 @@ name: PR Performance Report -# Privileged reporting only. No PR checkout, builds, or artifact execution here. +# Same-repo PRs may exercise their formatter directly. Forks use trusted base code. on: + pull_request: + branches: [main] + types: [opened, synchronize, reopened, ready_for_review] pull_request_target: branches: [main] types: [opened, synchronize, reopened, ready_for_review] @@ -16,12 +19,17 @@ concurrency: jobs: report: + if: >- + (github.event_name == 'pull_request' && + github.event.pull_request.head.repo.full_name == github.repository) || + (github.event_name == 'pull_request_target' && + github.event.pull_request.head.repo.full_name != github.repository) runs-on: ubuntu-latest timeout-minutes: 230 steps: - uses: actions/checkout@11d5960a326750d5838078e36cf38b85af677262 # v4.4.0 with: - ref: ${{ github.event.pull_request.base.sha }} + ref: ${{ github.event_name == 'pull_request' && github.event.pull_request.head.sha || github.event.pull_request.base.sha }} persist-credentials: false - uses: actions/setup-python@a26af69be951a213d495a4c3e4e4022e16d87065 # v5.6.0 with: diff --git a/eng/profiler_benchmarks/README.md b/eng/profiler_benchmarks/README.md index 8192be78b..689fc5b29 100644 --- a/eng/profiler_benchmarks/README.md +++ b/eng/profiler_benchmarks/README.md @@ -30,12 +30,12 @@ Partial results never produce a verdict. Two environments publish raw samples: Unix on Ubuntu with SQL Server 2022/2025. Routine Windows and macOS profiling is intentionally excluded because neutral PRs showed platform variance above the regression threshold, while both platforms -remain covered by functional CI. The privileged publisher runs -trusted base code, selects the exact PR-head ADO build, and validates bounded -artifacts as data. It publishes as soon as both profiler artifacts exist, -without waiting for unrelated matrix legs. After build completion, missing -artifacts receive a two-minute propagation grace before a partial result is -published. A failed aggregate build can still publish usable profiler artifacts. +remain covered by functional CI. Same-repository PRs run their formatter directly; +fork PRs retain the trusted-base publisher. Both select the exact PR-head ADO build +and validate bounded artifacts as data. Publication begins as soon as both profiler +artifacts exist, without waiting for unrelated matrix legs. After build completion, +missing artifacts receive a two-minute propagation grace before a partial result +is published. A failed aggregate build can still publish usable profiler artifacts. Exact-head reports may finalize after merge; stale heads are ignored. Missing, malformed, canceled, incomplete, or invalid data remains unavailable. diff --git a/eng/profiler_benchmarks/report.py b/eng/profiler_benchmarks/report.py index cfe19bb88..6188894cc 100644 --- a/eng/profiler_benchmarks/report.py +++ b/eng/profiler_benchmarks/report.py @@ -42,6 +42,7 @@ MAX_BYTES = 8 * 1024 * 1024 MAX_COMMENT_CHARS = 60000 MAX_DIAGNOSTIC_ROWS = 20 +MAX_FINGERPRINT_TASKS = 4 MARKER = "" THRESHOLD = 0.20 MIN_DELTA_MS = 1.0 @@ -376,52 +377,43 @@ def render(reports, head, build_id, issues=()): ] missing = len(LEGS) - len(completed) - if len(regressions) == 1: - leg, row = regressions[0] - opening = ( - f"This PR consistently slows {TASK_NAMES[row['name']].lower()} on " - f"{environment_name(leg)} by {row['change_pct']:.1f}%." - ) - elif regressions: + highlighted = [ + (leg, row) for leg, (_, rows) in completed.items() for row in rows if row["status"] != "ok" + ] + if regressions: tasks = len({row["name"] for _, row in regressions}) environments = len({leg for leg, _ in regressions}) opening = ( - f"This PR has {len(regressions)} consistent slowdown signals across " - f"{tasks} database tasks and {environments} environments." + f"{tasks} database task{'s' if tasks != 1 else ''} consistently slowed down across " + f"{environments} measured environment{'s' if environments != 1 else ''}." ) + verdict = "⚠️ Performance regression detected" elif noisy: - if len(noisy) == 1: - leg, row = noisy[0] - opening = ( - f"{TASK_NAMES[row['name']]} was slower on {environment_name(leg)}, " - "but the repeated comparisons were inconsistent." - ) - else: - tasks = len({row["name"] for _, row in noisy}) - environments = len({leg for leg, _ in noisy}) - opening = ( - f"No consistent slowdowns detected. {len(noisy)} inconsistent comparisons " - f"need review across {tasks} database tasks and {environments} environments." - ) - elif len(improvements) == 1: - leg, row = improvements[0] + tasks = len({row["name"] for _, row in noisy}) + environments = len({leg for leg, _ in noisy}) opening = ( - f"This PR consistently makes {TASK_NAMES[row['name']].lower()} faster on " - f"{environment_name(leg)} by {abs(row['change_pct']):.1f}%." + f"{tasks} database task{'s' if tasks != 1 else ''} produced inconsistent slowdown " + f"signals across {environments} measured environment" + f"{'s' if environments != 1 else ''}." ) + verdict = "🔍 Performance needs review" elif improvements: tasks = len({row["name"] for _, row in improvements}) environments = len({leg for leg, _ in improvements}) opening = ( - f"This PR has {len(improvements)} consistent improvement signals across " - f"{tasks} database tasks and {environments} environments." + f"{tasks} database task{'s' if tasks != 1 else ''} consistently improved across " + f"{environments} measured environment{'s' if environments != 1 else ''}. " + "No consistent slowdowns were detected." ) + verdict = "✅ Performance improved" elif not completed: opening = ( "Performance could not be assessed because no environment produced a complete result." ) + verdict = "⛔ Performance unavailable" elif not missing: opening = f"No consistent slowdowns detected across all {len(LEGS)} environments." + verdict = "✅ No regression detected" else: completed_label = "environment" if len(completed) == 1 else "environments" missing_label = "environment" if missing == 1 else "environments" @@ -429,22 +421,56 @@ def render(reports, head, build_id, issues=()): f"No consistent slowdowns in the {len(completed)} completed {completed_label}. " f"No result is available for {missing} {missing_label}." ) + verdict = "✅ No regression detected" - lines = [MARKER, "## PR Performance Report", "", f"**{opening}**", ""] - highlighted = regressions or noisy or improvements - if highlighted: - if not regressions and noisy: - lines += ["Inconsistent slowdowns to review:", ""] + improvement_tasks = len({row["name"] for _, row in improvements}) + regression_tasks = len({row["name"] for _, row in regressions}) + lines = [ + MARKER, + "## PR Performance Report", + "", + f"### {verdict}", + "", + f"**{opening}**", + "", + f"{improvement_tasks} IMPROVEMENT" + f"{'S' if improvement_tasks != 1 else ''} " + f"{regression_tasks} SLOWDOWN" + f"{'S' if regression_tasks != 1 else ''} " + f"{len(completed)}/{len(LEGS)} ENVIRONMENTS", + "", + ] + if noisy: + noisy_tasks = len({row["name"] for _, row in noisy}) lines += [ - "| Environment | Affected task | Before | After | Change |", - "|---|---|---:|---:|---:|", + f"{noisy_tasks} INCONSISTENT SLOWDOWN" f"{'S' if noisy_tasks != 1 else ''}", + "", ] - for leg, row in highlighted: - lines.append( - f"| {environment_name(leg)} | {TASK_NAMES[row['name']]} | " - f"{row['base_ms']:.3f} ms | {row['candidate_ms']:.3f} ms | " - f"{row['change_pct']:+.1f}% |" - ) + affected_tasks = [name for name in CASES if any(row["name"] == name for _, row in highlighted)] + if highlighted and len(affected_tasks) <= MAX_FINGERPRINT_TASKS: + affected_legs = [leg for leg in LEGS if any(item_leg == leg for item_leg, _ in highlighted)] + by_signal = {(leg, row["name"]): row for leg, row in highlighted} + lines += [ + "### Signal fingerprint", + "", + "| Database task | " + + " | ".join(environment_name(leg) for leg in affected_legs) + + " |", + "|---|" + "|".join("---:" for _ in affected_legs) + "|", + ] + for name in affected_tasks: + cells = [] + for leg in affected_legs: + row = by_signal.get((leg, name)) + if row is None: + cells.append("No signal") + elif row["status"] == "improvement": + cells.append(f"**{abs(row['change_pct']):.1f}% faster**") + elif row["status"] == "regression": + cells.append(f"**{abs(row['change_pct']):.1f}% slower**") + else: + cells.append(f"**{abs(row['change_pct']):.1f}% inconsistent**") + lines.append(f"| {escape(TASK_NAMES[name])} | " + " | ".join(cells) + " |") lines.append("") if regressions: lines.append( @@ -461,24 +487,37 @@ def render(reports, head, build_id, issues=()): lines += [ f"**Coverage:** {len(completed)} of {len(LEGS)} environments completed. " "Advisory result; does not block merging.", - "", - "| Environment | Status |", - "|---|---|", ] - for leg in LEGS: - report = by_leg.get(leg) - status = ( - "Completed" - if leg in completed - else f"No result available ({escape(issue_reason(leg, issues))})" - ) - lines.append(f"| {environment_name(leg)} | {status} |") + unavailable_legs = [ + f"{environment_name(leg)} ({escape(issue_reason(leg, issues))})" + for leg in LEGS + if leg not in completed + ] + if unavailable_legs: + lines += ["", "Unavailable: " + "; ".join(unavailable_legs) + "."] + + if highlighted: + lines += [ + "", + "
", + "Measured timings", + "", + "| Environment | Database task | Before | After | Change |", + "|---|---|---:|---:|---:|", + ] + for leg, row in highlighted: + lines.append( + f"| {environment_name(leg)} | {TASK_NAMES[row['name']]} | " + f"{row['base_ms']:.3f} ms | {row['candidate_ms']:.3f} ms | " + f"**{row['change_pct']:+.1f}%** |" + ) + lines += ["", "
"] diagnostics_start = len(lines) lines += [ "", "
", - "Affected phases and call counts", + "Performance diagnostics", "", "Phase times are inclusive diagnostics and must not be added together. " "They identify where measured time changed, not why it changed.", @@ -516,7 +555,7 @@ def render(reports, head, build_id, issues=()): lines += [ "", "
", - "All database tasks and timings", + "All database tasks and timings", ] for leg, (report, rows) in completed.items(): @@ -542,7 +581,7 @@ def render(reports, head, build_id, issues=()): "
", "", "
", - "Build, commits and measurement details", + "Build and measurement details", "", ] lines += [ @@ -591,7 +630,7 @@ def render(reports, head, build_id, issues=()): lines[diagnostics_start:diagnostics_end] = [ "", "
", - "Affected phases and call counts", + "Performance diagnostics", "", f"{total_diagnostics} diagnostic rows are available in the raw ADO artifacts.", "", diff --git a/tests/test_036_profiler_ci.py b/tests/test_036_profiler_ci.py index e72ad2c2b..5bbf99f86 100644 --- a/tests/test_036_profiler_ci.py +++ b/tests/test_036_profiler_ci.py @@ -115,11 +115,12 @@ def test_consistent_slowdown_is_advisory_regression(report): row["status"] == "regression" and row["change_pct"] == pytest.approx(30) for row in rows ) body = reporting.render([report], "c" * 40, 42) - assert "20 consistent slowdown signals" in body + assert "### ⚠️ Performance regression detected" in body + assert "20 database tasks consistently slowed down" in body assert "| Unix / SQL Server 2022 | Connection opening |" in body - assert "| Unix / SQL Server 2025 | No result available" in body - assert body.index("consistent slowdown signals") < body.index( - "Build, commits and measurement details" + assert "Unavailable: Unix / SQL Server 2025 (incomplete benchmark)." in body + assert body.index("consistently slowed down") < body.index( + "Build and measurement details" ) @@ -248,8 +249,8 @@ def test_render_bounds_schema_valid_diagnostics(report): body = reporting.render(reports, "c" * 40, 42) assert len(body) <= 60000 assert "20 additional diagnostic rows are available in the raw ADO artifacts" in body - assert "All database tasks and timings" in body - assert "Build, commits and measurement details" in body + assert "All database tasks and timings" in body + assert "Build and measurement details" in body @pytest.mark.parametrize("invalid", ["source commit", "base commit"]) @@ -288,14 +289,13 @@ def test_impact_summary_handles_single_inconsistent_and_complete_clean_results(r for pair, scale in zip(clean["pairs"], (1.3, 1.3, 1.3, 0.8, 0.8)): pair["candidate"]["scenarios"]["fetchone"]["wall_ms"] *= scale noisy = reporting.render([clean], "c" * 40, 42) - assert ( - "**Row-by-row fetching was slower on Unix / SQL Server 2022, " - "but the repeated comparisons were inconsistent.**" - ) in noisy - assert "Inconsistent slowdowns to review:" in noisy + assert "### 🔍 Performance needs review" in noisy + assert "1 database task produced inconsistent slowdown signals" in noisy + assert "1 INCONSISTENT SLOWDOWN" in noisy complete = [set_leg(clear_slowdowns(copy.deepcopy(report)), leg) for leg in reporting.LEGS] clean_body = reporting.render(complete, "c" * 40, 42) + assert "### ✅ No regression detected" in clean_body assert "**No consistent slowdowns detected across all 2 environments.**" in clean_body assert "**Coverage:** 2 of 2 environments completed." in clean_body @@ -305,12 +305,11 @@ def test_impact_summary_handles_single_regression_partial_and_no_results(report) for pair in single["pairs"]: pair["candidate"]["scenarios"]["fetchone"]["wall_ms"] *= 1.3 body = reporting.render([single], "c" * 40, 42) - assert ( - "**This PR consistently slows row-by-row fetching on Unix / SQL Server 2022 " "by 30.0%.**" - ) in body - assert "Affected phases and call counts" in body - assert "All database tasks and timings" in body - assert "Build, commits and measurement details" in body + assert "### ⚠️ Performance regression detected" in body + assert "**1 database task consistently slowed down across 1 measured environment.**" in body + assert "Performance diagnostics" in body + assert "All database tasks and timings" in body + assert "Build and measurement details" in body assert "median of paired before-and-after ratios" in body partial = reporting.render( @@ -321,7 +320,7 @@ def test_impact_summary_handles_single_regression_partial_and_no_results(report) ) assert "No consistent slowdowns in the 1 completed environment." in partial assert "No result is available for 1 environment." in partial - assert "| Unix / SQL Server 2025 | No result available (missing) |" in partial + assert "Unavailable: Unix / SQL Server 2025 (missing)." in partial assert "pending" not in partial.lower() unavailable = reporting.render([], "c" * 40, 42, ["Linux-SQL2022 (invalid artifact)"]) @@ -330,21 +329,30 @@ def test_impact_summary_handles_single_regression_partial_and_no_results(report) def test_impact_summary_reports_consistent_improvements(report): - single = clear_slowdowns(copy.deepcopy(report)) - for pair in single["pairs"]: - pair["candidate"]["scenarios"]["fetchall"]["wall_ms"] *= 0.7 - pair["candidate"]["scenarios"]["fetchall"]["cpp"]["ddbc::query"] = dict( - calls=1, total_us=500, min_us=500, max_us=500 - ) - rows = reporting.comparisons(single) + reports = [set_leg(clear_slowdowns(copy.deepcopy(report)), leg) for leg in reporting.LEGS] + for item, scales in zip(reports, ((0.7, 0.6), (0.72, 0.61))): + for pair in item["pairs"]: + pair["candidate"]["scenarios"]["fetchall"]["wall_ms"] *= scales[0] + pair["candidate"]["scenarios"]["setinputsizes"]["wall_ms"] *= scales[1] + pair["candidate"]["scenarios"]["fetchall"]["cpp"]["ddbc::query"] = dict( + calls=1, total_us=500, min_us=500, max_us=500 + ) + rows = reporting.comparisons(reports[0]) assert rows[4]["status"] == "improvement" assert rows[4]["phases"] == [(-0.5, "ddbc::query")] - body = reporting.render([single], "c" * 40, 42) + body = reporting.render(reports, "c" * 40, 42) + assert "### ✅ Performance improved" in body assert ( - "**This PR consistently makes fetch-all queries faster on Unix / SQL Server 2022 " - "by 30.0%.**" + "**2 database tasks consistently improved across 2 measured environments. " + "No consistent slowdowns were detected.**" ) in body - assert "| Unix / SQL Server 2022 | Fetch-all queries |" in body + assert "2 IMPROVEMENTS 0 SLOWDOWNS 2/2 ENVIRONMENTS" in body + assert "| Fetch-all queries | **30.0% faster** | **28.0% faster** |" in body + assert ( + "| Insertion with explicit input sizes | **40.0% faster** | " "**39.0% faster** |" + ) in body + assert "Spread" not in body + assert "Measured timings" in body assert "| Fetch-all queries |" in body and "| consistent improvement |" in body assert "ddbc::query -0.500 ms" in body @@ -355,7 +363,8 @@ def test_regression_headline_keeps_precedence_over_improvement(report): pair["candidate"]["scenarios"]["fetchall"]["wall_ms"] *= 0.7 pair["candidate"]["scenarios"]["fetchone"]["wall_ms"] *= 1.3 body = reporting.render([mixed], "c" * 40, 42) - assert "**This PR consistently slows row-by-row fetching" in body + assert "### ⚠️ Performance regression detected" in body + assert "1 IMPROVEMENT 1 SLOWDOWN" in body def test_inconsistent_slowdown_keeps_precedence_over_improvement(report): @@ -365,8 +374,9 @@ def test_inconsistent_slowdown_keeps_precedence_over_improvement(report): for pair, scale in zip(mixed["pairs"], (1.3, 1.3, 1.3, 0.8, 0.8)): pair["candidate"]["scenarios"]["fetchone"]["wall_ms"] *= scale body = reporting.render([mixed], "c" * 40, 42) - assert "**Row-by-row fetching was slower" in body - assert "Inconsistent slowdowns to review:" in body + assert "### 🔍 Performance needs review" in body + assert "1 IMPROVEMENT 0 SLOWDOWNS" in body + assert "1 INCONSISTENT SLOWDOWN" in body @pytest.mark.parametrize( @@ -1232,13 +1242,13 @@ def corrupt_deflate(raw): if corrupt in ("zip", "timeout", "scenarios", "recursion", "deflate"): assert "### Unix / SQL Server 2025" in posted[1] assert reporting.escape("Linux-SQL2022 (invalid artifact)") in posted[1] - assert "| Unix / SQL Server 2022 | No result available (invalid artifact) |" in posted[1] - assert posted[1].count("20 consistent slowdown signals") == 1 + assert "Unavailable: Unix / SQL Server 2022 (invalid artifact)." in posted[1] + assert posted[1].count("20 database tasks consistently slowed down") == 1 else: assert "**Coverage:** 2 of 2 environments completed." in posted[1] assert "### Unix / SQL Server 2022" in posted[1] assert "### Unix / SQL Server 2025" in posted[1] - assert posted[1].count("40 consistent slowdown signals") == 1 + assert posted[1].count("20 database tasks consistently slowed down") == 1 def test_publisher_waits_for_newer_run_after_exact_head_build_is_canceled(report, monkeypatch): @@ -1673,10 +1683,16 @@ def test_profiler_documentation_preserves_standalone_benchmarks_and_failed_build assert "failed aggregate build can still publish" in contract -def test_comment_workflow_executes_only_trusted_base_code(): +def test_comment_workflow_separates_same_repo_and_fork_trust(): workflow = (ROOT / ".github/workflows/pr-profiler-report.yml").read_text(encoding="utf-8") + assert "pull_request:" in workflow assert "pull_request_target:" in workflow - assert "ref: ${{ github.event.pull_request.base.sha }}" in workflow + assert "github.event.pull_request.head.repo.full_name == github.repository" in workflow + assert "github.event.pull_request.head.repo.full_name != github.repository" in workflow + assert ( + "github.event_name == 'pull_request' && github.event.pull_request.head.sha || " + "github.event.pull_request.base.sha" + ) in workflow assert "persist-credentials: false" in workflow assert "actions/checkout@11d5960a326750d5838078e36cf38b85af677262" in workflow assert "actions/setup-python@a26af69be951a213d495a4c3e4e4022e16d87065" in workflow