FEAT: Implementing and Integrating AQE API's - execute, executemany, fetch, fetchall and fetchmany

[Subrata (subrata-ms)]

Work Item / Issue Reference

AB#47195,47195,47997,47198

GitHub Issue: #<ISSUE_NUMBER>

---

Summary

This pull request refactors the asynchronous query layer to improve clarity, error handling, and code organization. The main changes include renaming internal variables for clarity, extracting statement execution and result fetching logic into dedicated modules, enhancing exception translation, and updating tests to reflect improved error handling. These updates make the async API more robust and maintainable.

Refactoring and Code Organization:

• Renamed internal attributes from native_connection/native_cursor to py_core_async_connection/py_core_async_cursor in both AsyncConnection and AsyncCursor for improved clarity and future maintainability. (mssql_python/async_query/async_connection.py, mssql_python/async_query/async_cursor.py) [1] [2]
• Extracted statement execution and result fetching logic from AsyncCursor into new helper modules: async_execute.py and async_fetch.py, leading to cleaner, more modular code. (mssql_python/async_query/async_execute.py, mssql_python/async_query/async_fetch.py, mssql_python/async_query/async_cursor.py) [1] [2] [3]

Error Handling Improvements:

• Enhanced exception translation in exception_translator.py to classify and translate lower-level driver errors into appropriate public exceptions (e.g., OperationalError, ProgrammingError), and updated the async connection tests to verify these translations. (mssql_python/async_query/exception_translator.py, tests/AsyncTest/test_002_async_connection.py) [1] [2] [3] [4]

Behavioral and API Improvements:

• Improved fetch tracking in AsyncCursor, including accurate rowcount reporting after fetch operations and resetting fetch state on nextset. (mssql_python/async_query/async_cursor.py) [1] [2]
• Updated column name handling in AsyncCursor.description to support automatic lowercasing based on settings, and ensured that row wrapping consistently produces Row objects with proper metadata. (mssql_python/async_query/async_cursor.py, mssql_python/async_query/async_fetch.py) [1] [2]

Testing:

• Updated and expanded tests to cover the improved exception translation and internal API changes, ensuring correct error propagation and behavior after connection closure. (tests/AsyncTest/test_002_async_connection.py, tests/AsyncTest/test_003_async_exceptions.py) [1] [2] [3]

[Copilot]

🟡 Changes recommended

Unresolved moderate and critical findings remain in result-set state, parameter binding, fetch handling, and credential-redaction coverage.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Refactors the async query API by separating execution and fetching helpers, improving error translation and row handling, and expanding test coverage.

Changes:

• Extracts async execution and fetch operations into dedicated modules.
• Improves exception translation, logging, row wrapping, and fetch tracking.
• Expands tests for execution, fetching, errors, cursor behavior, and logging.

File summaries

File	Reviewed changes	Final review findings
tests/AsyncTest/test_007_async_fetch.py	Adds fetch, row, type, and navigation coverage.	—
tests/AsyncTest/test_006_async_execute.py	Adds execute and executemany coverage.	—
tests/AsyncTest/test_005_async_cursor.py	Updates cursor property and lifecycle tests.	—
tests/AsyncTest/test_004_async_logging.py	Adds operation logging coverage.	Critical (2 votes): Preserve broad credential-redaction assertions or verify known secrets are absent.
tests/AsyncTest/test_003_async_exceptions.py	Expands exception translation coverage.	—
tests/AsyncTest/test_002_async_connection.py	Updates connection error and lifecycle tests.	—
mssql_python/async_query/exception_translator.py	Classifies and translates async errors.	—
mssql_python/async_query/async_fetch.py	Implements fetching, row wrapping, and metadata handling.	Moderate (1 vote): Snapshot native_uuid; short-circuit non-positive fetch sizes. Nit (3 votes): Cache row metadata maps instead of rebuilding them per row.
mssql_python/async_query/async_execute.py	Implements async execution helpers.	Moderate (2 votes): Normalize Row parameter sequences like the synchronous path.
mssql_python/async_query/async_cursor.py	Delegates operations and tracks fetch state.	Moderate (2 votes): Snapshot lowercase/UUID settings and row metadata per result set.
mssql_python/async_query/async_connection.py	Renames connection internals and integrates logging.	—

Review details

Suppressed comments (2)

mssql_python/async_query/async_fetch.py:62

• The wrapper passes a non-positive size through to py-core. The synchronous cursor short-circuits size <= 0 before its native call (mssql_python/cursor.py:2842-2843), and this PR's test requires fetchmany(-1) to return []; a Rust binding using an unsigned size can reject -1 instead. Short-circuit requested_size <= 0 before calling py-core.

    with translate_py_core_exceptions():
        if size is None:
            rows = await _get_py_core_async_cursor(cursor).fetchmany()
        else:
            rows = await _get_py_core_async_cursor(cursor).fetchmany(size)

mssql_python/async_query/async_fetch.py:30

• This consults the mutable global native_uuid at fetch time, so changing it between execute() and fetch*() changes the representation of an already-executed result. The synchronous cursor snapshots UUID conversion indices at execute (mssql_python/cursor.py:1404-1423) and tests cover this (tests/test_004_cursor.py:16785-16815); store the setting/result-set metadata when execute or nextset completes.

    uuid_str_indices = (
        tuple(index for index, column in enumerate(description) if column[1] is uuid.UUID)
        if not get_settings().native_uuid
        else None

• Files reviewed: 11/11 changed files
• Comments generated: 4
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[github-actions]

📊 Code Coverage Report

🔥 Diff Coverage 100%

🎯 Overall Coverage 84%

📈 Total Lines Covered: 8802 out of 10458 📁 Project: mssql-python

---

Diff Coverage

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

• mssql_python/async_query/async_connection.py (100%)
• mssql_python/async_query/async_cursor.py (100%)
• mssql_python/async_query/async_execute.py (100%)
• mssql_python/async_query/async_fetch.py (100%)
• mssql_python/async_query/exception_translator.py (100%)

Summary

• Total: 208 lines
• Missing: 0 lines
• Coverage: 100%

---

📋 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.3%
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

⚙️ Build Summary	📋 Coverage Details
View Azure DevOps Build	Browse Full Coverage Report

[Copilot]

🟡 Changes recommended

Unresolved compatibility, correctness, and performance findings remain.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (4)

mssql_python/async_query/async_cursor.py:107

• This property applies the global lowercase setting on every access, so changing it after execute() changes the metadata and row key casing for an already-established result set. The synchronous cursor snapshots this at execution (tests/test_004_cursor.py:3592-3608); retain the effective casing with the async result metadata instead.

        if description is None:
            return None
        lowercase = get_settings().lowercase
        return [
            ((column[0].lower() if lowercase else column[0]), *column[1:]) for column in description

mssql_python/async_query/async_execute.py:27

• The new async path only unwraps tuple/list parameters, but the added test_execute_accepts_dbapi_row passes a Row and expects its columns to bind as individual parameters. The synchronous implementation explicitly normalizes Row to a tuple because the downstream binder otherwise treats the whole row as one value (mssql_python/cursor.py:1739-1747); apply the same normalization before forwarding to PyAsyncCursor.execute, or that new async contract will fail.

    cursor._reset_fetch_tracking()  # pyright: ignore[reportPrivateUsage]
    if len(parameters) == 1 and isinstance(parameters[0], (tuple, list)):
        parameters = tuple(parameters[0])

mssql_python/async_query/async_fetch.py:30

• native_uuid is read while each row is wrapped, so toggling the module setting after execute() but before fetch*() changes the type of rows from one result set. The synchronous cursor snapshots this setting at execution (tests/test_004_cursor.py:16336-16357); cache the UUID conversion indices when async result metadata is established.

    uuid_str_indices = (
        tuple(index for index, column in enumerate(description) if column[1] is uuid.UUID)
        if not get_settings().native_uuid
        else None

mssql_python/async_query/async_fetch.py:23

• _wrap_row rebuilds the column map and, when enabled, the lowercase map for every returned row. fetchmany() and fetchall() therefore repeat O(column_count) metadata work for every row; the synchronous cursor precomputes these maps once per result set, so cache and reuse them for large async result sets.

def _wrap_row(cursor: "AsyncCursor", values: tuple[Any, ...]) -> Row:
    description = cursor.description or ()
    column_map = {column[0]: index for index, column in enumerate(description)}
    column_map_lower = (
        {name.lower(): index for name, index in column_map.items()}

• Files reviewed: 11/11 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

A critical test indentation error blocks collection, and fetch behavior, performance, and documentation require fixes.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (3)

mssql_python/async_query/async_fetch.py:24

• _wrap_row is called for every row by fetchone, fetchmany, and fetchall, but this rebuilds the description-derived maps for every row, including a fresh cursor.description list and dictionaries. That makes large result sets pay O(rows × columns) Python metadata work and can make one result set change representation if lowercase/native_uuid is changed between fetches; the synchronous cursor caches these maps/settings once per result set (mssql_python/cursor.py:1838-1848). Cache the row metadata when the result set is established and reuse it here.

    description = cursor.description or ()
    column_map = {column[0]: index for index, column in enumerate(description)}
    column_map_lower = (
        {name.lower(): index for name, index in column_map.items()}
        if get_settings().lowercase

mssql_python/async_query/async_fetch.py:60

• Non-positive sizes are still passed to the native cursor. The synchronous cursor returns immediately for size <= 0 (mssql_python/cursor.py:2839-2843), and the new async test expects fetchmany(-1) to return []; passing -1 to a native usize-like parameter can instead raise before the wrapper's tracking guard runs.

    requested_size = cursor.arraysize if size is None else size
    logger.debug("AsyncCursor.fetchmany: starting; requested_size=%s", requested_size)
    with translate_py_core_exceptions():
        if size is None:
            rows = await _get_py_core_async_cursor(cursor).fetchmany()

mssql_python/async_query/exception_translator.py:98

• The new fallback at this line translates allowlisted built-in RuntimeError and TypeError instances, but the surrounding docstrings still say non-py-core errors are returned unchanged and that the context manager translates only py-core failures. Please update that documented contract so callers are not misled about which native Python errors are wrapped.

    return _translate_known_builtin_error(error)

• Files reviewed: 11/11 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

Five moderate issues remain unresolved.

Review details

Suppressed comments (5)

Previously missed (1) — in code that hasn't changed since the last review.

mssql_python/async_query/async_fetch.py:62

• The synchronous cursor returns [] immediately for size <= 0 (mssql_python/cursor.py:2839-2843), but this path passes zero and negative sizes into mssql_py_core. That makes the new fetchmany(0/-1) contract depend on backend validation and can raise instead of returning the expected empty list; short-circuit non-positive sizes before the native call.

mssql_python/async_query/async_cursor.py:87

• After a fetch sets _fetch_rowcount, rowcount bypasses the native cursor on every later read. Because close() does not clear that cache, a fetched cursor still returns its old count after closing instead of following the native closed-cursor behavior used by the other properties and operations. Clear fetch tracking when close succeeds before logging completion.

            await self._py_core_async_cursor.close()

mssql_python/async_query/async_cursor.py:62

• This removes the existing public use_prepare keyword from AsyncCursor.executemany; callers that used the previous signature now fail with TypeError, and the native helper no longer receives the flag. Retain and forward the keyword (as execute still does), or deliberately version/document this breaking API change.

        seq_of_parameters: Sequence[Sequence[Any]] | Sequence[Mapping[str, Any]],
    ) -> None:

mssql_python/async_query/async_execute.py:27

• This unwraps only tuples/lists, so a Row passed as the sole parameter is forwarded to PyAsyncCursor.execute as one bound value. The new test_execute_accepts_dbapi_row expects the row's two values to bind to two ? markers; mirror the synchronous cursor's Row-to-tuple normalization (mssql_python/cursor.py:1742-1747) before calling the native API.

    if len(parameters) == 1 and isinstance(parameters[0], (tuple, list)):
        parameters = tuple(parameters[0])

mssql_python/async_query/async_fetch.py:30

• _wrap_row runs once for every returned row, but each invocation rereads description, rebuilds both column maps, and rescans all columns for UUIDs. Large fetchall()/fetchmany() results therefore repeat O(column_count) metadata work and allocate a map per row; cache these result-set maps on AsyncCursor when execution/nextset() changes and reuse them here.

    description = cursor.description or ()
    column_map = {column[0]: index for index, column in enumerate(description)}
    column_map_lower = (
        {name.lower(): index for name, index in column_map.items()}
        if get_settings().lowercase
        else None
    )
    uuid_str_indices = (
        tuple(index for index, column in enumerate(description) if column[1] is uuid.UUID)
        if not get_settings().native_uuid
        else None

• Files reviewed: 11/11 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Subrata (subrata-ms)]

🔵 Needs a closer look

Five moderate issues remain unresolved.

Review details

All five moderate issues were valid and are now resolved:

fetchmany(size <= 0) returns [] without calling py-core, while preserving closed-cursor errors.
Successful close() clears cached fetch rowcount.
executemany(..., use_prepare=...) is restored and forwarded.
A sole DB-API Row is normalized to positional parameters.
Row metadata/maps are cached once per result set and refreshed on nextset().

[Copilot]

🔵 Needs a closer look

Five moderate findings remain unresolved.

Review details

Suppressed comments (5)

mssql_python/async_query/async_execute.py:65

• len(seq_of_parameters) is evaluated before the cursor's closed state is checked. A closed cursor passed an iterator (or another object without len) therefore raises a raw TypeError instead of the ProgrammingError expected for closed-cursor operations and used by the synchronous cursor, which checks closure before inspecting parameters. Check the cursor before computing batch_count.

    batch_count = len(seq_of_parameters)
    cursor._reset_fetch_tracking()  # pyright: ignore[reportPrivateUsage]
    cursor._clear_result_metadata()  # pyright: ignore[reportPrivateUsage]

mssql_python/async_query/async_fetch.py:47

• Because this branch only checks the wrapper's _closed flag, it never consults the native cursor or connection. After AsyncConnection.close() the existing AsyncCursor wrapper is not marked closed, so await cursor.fetchmany(0) returns [] while the other cursor operations surface the closed-connection error. Keep cursor lifecycle state synchronized with connection close or perform an equivalent closed-state check before returning here.

    if requested_size <= 0:
        cursor._check_closed()  # pyright: ignore[reportPrivateUsage]
        logger.debug("AsyncCursor.fetchmany: completed; row_count=0; rowcount=%d", cursor.rowcount)
        return []

mssql_python/async_query/async_fetch.py:45

• For a closed cursor, fetchmany("invalid") reaches requested_size <= 0 before _check_closed() and leaks Python's comparison TypeError rather than translating the operation to ProgrammingError. The synchronous cursor checks closure before validating the size, so perform the closed check before reading arraysize/evaluating the requested size.

    requested_size = cursor.arraysize if size is None else size
    logger.debug("AsyncCursor.fetchmany: starting; requested_size=%s", requested_size)
    if requested_size <= 0:
        cursor._check_closed()  # pyright: ignore[reportPrivateUsage]

mssql_python/async_query/exception_translator.py:35

• The translation table only maps the exact RuntimeError("Cursor is closed") case. Fetching from an open cursor before any result set is another py-core built-in runtime error, so fetchone/fetchall/fetchmany can still escape as RuntimeError even though the new test_fetch_without_result_set_raises_programming_error expects ProgrammingError. Add the py-core no-result-set message/prefix (or expose it as a native ProgrammingError) here.

_PROGRAMMING_RUNTIME_ERRORS = ("Cursor is closed",)

tests/AsyncTest/test_004_async_logging.py:53

• This changes the redaction regression guard from rejecting any password text to rejecting only the password= spelling. Since this PR now forwards the default logger to py-core, a diagnostic formatted as password: ... or another non-= form could include a credential and still pass; assert the actual secret is absent or cover all sensitive-key formats instead.

    assert "password=" not in messages.lower()

• Files reviewed: 11/11 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

Copilot review overview

🟡 Changes recommended

Async Row._mapping does not preserve canonical ordering for duplicate column names.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1

Open (1)

• Preserve duplicate-column order in async row mappings · New

[Copilot]

Copilot review overview

🟡 Changes recommended

Failed-execution reconciliation can mask the original translated database exception when native cursor state is unavailable.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1

Open (1)

• Prevent metadata reconciliation from masking execution errors · New

Resolved since last review (1)

• Preserve duplicate-column order in async row mappings

[Copilot]

Copilot review overview

🔵 Needs a closer look

The broad async state and native-driver integration changes require successful live SQL Server validation before approval.

Review effort: Balanced
Findings: None

Resolved since last review (1)

• Prevent metadata reconciliation from masking execution errors

[Copilot]

Copilot review overview

🟡 Changes recommended

Some fetch validation and pre-execution property errors bypass the intended public exception contract.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 2

Open (2)

• Translate cursor property getter errors in execute paths · New
• Validate active result set before fetchmany(0) fast path · New

[Copilot]

Copilot review overview

🔵 Needs a closer look

Cancellation currently bypasses execution-state reconciliation, potentially leaving stale cursor metadata and row counts.

Review effort: Balanced
Findings: None

Resolved since last review (2)

• Validate active result set before fetchmany(0) fast path
• Translate cursor property getter errors in execute paths

Previously missed (1)

In code that hasn't changed since last review

Reconcile cursor and metadata when cancellation interrupts await

mssql_python/​async_query/​async_execute.py:71

asyncio.CancelledError inherits from BaseException, not Exception, so cancelling this await bypasses reconciliation. If py-core has already reset or advanced the native cursor, the wrapper retains the previous description and fetch counters and can wrap later rows with stale metadata. Reconcile on cancellation as well before re-raising.

This issue also appears on line 108 of the same file.

[Copilot]

Copilot review overview

🔵 Needs a closer look

Cross-language async state and live-database behavior require final human and cross-platform validation.

Review effort: Balanced
Findings: None

[Sumit Sarabhai (sumitmsft)]

PR #792: Preserve active-result state when navigation is rejected.

nextset() resets fetch tracking and clears result metadata before native ownership validation. If it is called while fetchall() is running, py-core can reject navigation with a busy error without changing the native result, but the Python metadata and counters have already been erased. The ongoing fetch then loses named-column access and UUID conversion, and its cumulative rowcount restarts.

Please preserve the existing metadata and counters when navigation is rejected without changing the result. Reconcile state after actual advancement or result disposal, and add a regression that attempts busy navigation while fetching pending rows.

[Sumit Sarabhai (sumitmsft)]

PR #792: Bind fetch completion to the result that produced its rows.

After awaiting native fetching, this code updates counters and wraps rows using the cursor's current mutable metadata. A concurrent nextset() can successfully advance and replace those caches while the earlier fetch is still materializing its rows.

Old-result rows can consequently receive the next result's column names or UUID indices, contaminate its rowcount, or raise IndexError when the new indices exceed the old row width, preventing delivery of the consumed batch.

Please capture the originating result's metadata before awaiting and associate fetch accounting with that result's generation, or serialize public operations through row publication. Apply this consistently across fetch methods and cover concurrent successful navigation between result sets with different column layouts.

[Saurabh Singh (saurabh500)]

While these are internal methods, can we still add comments to all these methods to describe what and why?