Skip to content

Share the pure cursor-base logic between the sync and asyncio cursors - #883

Merged
laughingman7743 merged 9 commits into
masterfrom
refactor/880-cursor-base
Sep 29, 2026
Merged

laughingman7743 merged 9 commits into
masterfrom
refactor/880-cursor-base

Conversation

@laughingman7743

@laughingman7743 laughingman7743 commented Sep 28, 2026 •

Copy link
Copy Markdown
Member

WHAT

First of the four area PRs for #880: the cursor base (common.py, result_set.py WithResultSet / WithFetch, aio/common.py).
Moves the logic without I/O that AioBaseCursor and WithAsyncFetch copied from their sync counterparts onto the shared base classes.
Sync and async methods remain separate for the I/O calls themselves.

  • BaseCursor._cache_search_limits (search size and expiry cutoff) and BaseCursor._match_previous_query (filter, latest-first order, expiry stop, and schema/catalog match for one page) now hold the pure parts of _find_previous_query_id.
    Both _find_previous_query_id methods keep only the paging, the _list_query_executions call, and the failure warning.
  • BaseCursor._build_execute_request formats the query and builds the StartQueryExecution request for both _execute methods.
    Each _execute still resolves its own ExecuteOptions from the legacy keyword arguments.
  • WithFetch is folded into WithResultSet, which becomes the base class of the SQL cursors that keep a result set: class WithResultSet(BaseCursor, CursorIterator).
    It now provides the result_set, query_id, arraysize, rownumber, and rowcount properties, the _query_id / _result_set initialization, the fetch methods, close, and the sync executemany / cancel.
    Previously, WithFetch and WithAsyncFetch each defined most of these verbatim, and WithResultSet was a plain mixin that declared result_set / query_id abstract.
  • The sync SQL cursors (Cursor, ArrowCursor, PandasCursor, PolarsCursor, S3FSCursor) subclass WithResultSet directly.
    WithAsyncFetch(AioBaseCursor, WithResultSet) overrides executemany / cancel with async versions, and the asyncio cursors override the fetch methods as before.
  • Because WithResultSet now precedes BaseCursor in the MRO, its close implements the abstract BaseCursor.close, and WithResultSet's members take precedence over CursorIterator's (arraysize capped at 1000, rownumber, rowcount, abstract fetch methods).
    Cursor and AioCursor still override arraysize with the cap, so the cap applies to them but not to the format cursors, as before.

Signatures of _execute, _poll, _cancel, _get_query_execution, and _find_previous_query_id are unchanged (#734).
At the maintainer's decision, this PR does not extract helpers for the one-expression response parsing in _list_databases, _list_table_metadata, and _batch_get_query_execution, which the issue listed as candidates: with docstrings, the helpers would add more lines than they remove.

Breaking changes (release note for 4.0.0):

  • pyathena.result_set.WithFetch is removed; subclass WithResultSet instead. It was added in v3.29.0 and is not in the API docs.
  • WithResultSet (in the API docs) now subclasses BaseCursor and CursorIterator. A class that lists BaseCursor or CursorIterator before WithResultSet in its bases, such as class X(BaseCursor, CursorIterator, WithResultSet), now fails with an MRO TypeError at class creation. List WithResultSet first or alone instead.

Unchanged for existing callers:

  • Every member of the 12 sync and asyncio SQL cursor classes resolves to a function with the same instructions as on master (6effaa3), except the refactored _execute / _find_previous_query_id (compared with a throwaway script; __abstractmethods__ is empty for all of them).
  • The number and order of ListQueryExecutions / StartQueryExecution requests do not change.

WHY

#880: the sync and asyncio copies differ only in await, the retry function, the sleep, and the interrupt exception, so a fix in one copy can miss the other.

Duplication in this area, counted with a throwaway script (not the counting method behind the issue's ~380 estimate): aio method bodies whose sync pair matches after removing those I/O differences, docstrings, comments, and blank lines excluded.

Duplicated pairs aio body lines
master (a86a180; unchanged at 6effaa3 apart from UTC) 30 344
This PR 17 281

The remaining 17 pairs are I/O methods: _get_query_execution, _poll / _poll_until_terminal, _cancel, _batch_get_query_execution, _list_query_executions, the listing and metadata requests, _with_glue_fallback, executemany, cancel, the paging loop left in _find_previous_query_id, and the options resolution and request left in _execute.
The issue scope keeps these as separate sync and async methods.

TEST

Commits: f78d06c (refactor), 84a46f5 (test-only repair from self-review), a4a5d61 (docstring-only corrections from self-review), ae77fbc (test and docstring repair from independent review), 7d4562a (fold WithFetch into WithResultSet, at the maintainer's request), 1b594b1, a874aed, 3e376d2 (docstring corrections and property docstrings from self-review and independent review), d4938df (merge of master 6effaa3; timezone.utc → UTC in the new code).

  • just lint on d4938df: passed (ruff, format, mypy, cfn-lint, license headers).
  • Added no-AWS tests for both cursor bases (test_cache_search_stops_at_expired_execution, test_cache_search_reads_pages_up_to_cache_size, test_cache_search_prefers_latest_execution), which cover the expiry stop, paging bounded by cache_size, and the latest match.
    They pass against master's common.py, aio/common.py, and result_set.py (checked again with the 84a46f5 tests) and on ae77fbc, together with the existing test_cache_size_different_* tests (10 passed each).
    Mutation checks: both expiry tests fail when expired is removed from the paging loops, and when the expiry comparison raises (the tests assert that no cache-search failure was logged).
  • uv run --env-file .env pytest -n 4 tests/pyathena/test_cursor.py tests/pyathena/aio/test_cursor.py tests/pyathena/test_async_cursor.py tests/pyathena/{arrow,pandas,polars,s3fs}/test_cursor.py tests/pyathena/aio/{arrow,pandas,polars,s3fs}/test_cursor.py on AWS against f78d06c: 558 passed, 1 skipped. 84a46f5–ae77fbc change only tests and docstrings. The AWS CI run 36454592891 on ae77fbc passed. For 7d4562a, the class-structure change is covered by the bytecode comparison above and the AWS CI after Ready.
  • Not run locally: the SQLAlchemy suites, Spark, and the remaining PyAthena tests; the AWS CI runs them after Ready.

🤖 Generated with Claude Code

laughingman7743 and others added 2 commits September 29, 2026 01:26
The asyncio cursor base copied the result cache search, the execute
request preparation, and the result set members of the sync cursor base.
Move the parts without I/O to the shared base classes:

- BaseCursor._cache_search_limits and _match_previous_query hold the
  cache window and the per-page match of _find_previous_query_id; both
  _find_previous_query_id methods keep only the paging and the listing.
- BaseCursor._build_execute_request formats the query and builds the
  StartQueryExecution request for both _execute methods.
- WithResultSet now provides the result_set, query_id, arraysize and
  rownumber properties and the default fetch methods that WithFetch and
  WithAsyncFetch repeated. Both mixins list WithResultSet before
  CursorIterator so that these members take precedence. close stays in
  each mixin because BaseCursor declares it abstract.

No-AWS tests pin the cache search's expiry stop, paging, and latest-match
behavior for both cursor bases.

Refs #880

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Comment thread pyathena/result_set.py Outdated
return result_set.fetchall()


class WithFetch(BaseCursor, WithResultSet, CursorIterator):

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Self-review round one (implementation behavior): FINDINGS (1, repaired)

Range: git diff a86a180ebbe5e18920cd75802ea5350ce8d33f10..f78d06c075c98ed91c756185d6500c39ff885df3

Covered surfaces:

  • BaseCursor / AioBaseCursor _find_previous_query_id: equivalence with the old loop. expired replaces next_token = None; break, datetime.now() is still computed before the try, the page match is still inside the try, and expiration_time is non-None exactly when cache_expiration_time > 0. No other class in pyathena/ overrides it; AsyncCursor (futures) reaches the sync version through BaseCursor._execute.
  • _execute (both): only the prologue moved to _build_execute_request. The legacy keyword resolution stays in each method, and the signatures are unchanged. The tests that patch _build_start_query_execution_request still reach it through the helper.
  • WithResultSet / WithFetch / WithAsyncFetch MRO: the names shared by WithResultSet and CursorIterator (__init__, arraysize, rownumber, rowcount, fetch*) were all overridden by the old mixins, so resolution is unchanged. BaseCursor and WithResultSet share only __init__; BaseCursor.close is abstract, so close stays in each mixin. __abstractmethods__ is empty for all 12 SQL cursor classes. Cursor and AioCursor still own the capped arraysize, and the format cursors still use the uncapped one. The init order still sets _query_id / _result_set after CursorIterator.__init__, and nothing reads them earlier.
  • Docs: WithResultSet and WithAsyncFetch are documented with :inherited-members:, so the moved members stay in the API docs.

Finding: see the inline comment on the expiry test. No behavior defects found in the refactor itself.

Limitation: this round did not re-run the full AWS suite for the test-only repair; the 558-test AWS run on f78d06c is unaffected by it.

Comment thread tests/pyathena/test_cursor.py Outdated
]

with patch.object(
Cursor, "_list_query_executions", return_value=("next_token", page)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FINDING (test quality): return_value returns the same page with a next_token forever. If the expiry check still returned None but stopped ending the search (for example, if query_id or next_token is None without expired), this test would page through sys.maxsize executions instead of failing. The aio counterpart has the same problem.

Repair: 84a46f5 uses a finite side_effect whose second page holds a match, so continuing past the expired execution returns next_page and fails the assertion. Verified by applying that mutation to both cursor bases: both expiry tests failed (2 failed, 8 passed); after restoring the code, 10 passed.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Comment thread pyathena/common.py
Tuple of (formatted_query, request).

Raises:
ProgrammingError: If the formatter rejects the query or its parameters.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FINDING (docstring completeness): _build_execute_request and both _execute methods run the formatter, which raises ProgrammingError for an empty query or unsupported parameters (pyathena/formatter.py:399, :418), but only DatabaseError was documented. Added in a4a5d61.

Comment thread pyathena/result_set.py Outdated
"""Mixin providing a SQL cursor's result set, its properties, and default fetch.

Shared by the sync (``WithFetch``) and asyncio (``WithAsyncFetch``) cursors.
The fetch methods call the result set synchronously; the asyncio cursors

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FINDING (docstring claim): the first version said the default fetch methods "suit result sets that load all data eagerly in __init__". That was inherited from the old mixin text, and it is wrong for the shared class: sync Cursor uses these methods with the lazily paging AthenaResultSet, and all five asyncio SQL cursors (AioCursor, Arrow, pandas, Polars, S3FS) override them. Corrected in a4a5d61.

Comment thread pyathena/aio/common.py Outdated
data eagerly in ``__init__``), and the async iteration protocol.
Adds ``close``, async ``executemany`` and ``cancel``, async iteration, and
the async context manager protocol to the properties and sync fetch
methods of ``WithResultSet``. Subclasses override the fetch methods with

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FINDING (docstring claim): the first version said "cursors whose result sets fetch lazily override the fetch methods with async versions". In fact all five subclasses override them (and __anext__), including the pandas/Arrow/Polars cursors whose result sets load eagerly. Corrected in a4a5d61.

Comment thread pyathena/aio/common.py
The query execution ID.

Raises:
ProgrammingError: If the formatter rejects the query or its parameters.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Self-review round two (claims, callers, operations): FINDINGS (3, corrected)

Range: git diff a86a180ebbe5e18920cd75802ea5350ce8d33f10..a4a5d61099f10182071e15e7cab1081e8966d270 (84a46f5 → a4a5d61 changes only docstrings)

Claims checked:

  • PR body, MRO: "Cursor / AioCursor keep the capped arraysize, format cursors the uncapped one, as before". Checked against the owning class of each member for all 12 SQL cursors on this head and on a86a180. Holds.
  • PR body, compatibility: subclasses of WithFetch / WithAsyncFetch resolve every member as before. The names shared by WithResultSet / CursorIterator were all overridden by the old mixins. Holds; added to the body.
  • PR body, AWS operator: the ListQueryExecutions / StartQueryExecution request count and order are unchanged. The paging test pins max_results 50 then 10 for cache_size=60; the matching and paging code moved without changes. Holds.
  • PR body, pinning evidence: "passed on unchanged master" predated the 84a46f5 test change. Re-ran the current tests against a86a180's common.py / aio/common.py / result_set.py: 10 passed. The body now names each commit's evidence.
  • PR body, duplication table: re-ran the script on a4a5d61, still 18 pairs / 284 lines. The list of remaining pairs had left out the I/O parts of _find_previous_query_id and _execute; corrected. Also noted that the script differs from the counting behind the issue's estimate.
  • PR body: "Following the issue discussion" misattributed the parse-helper decision, which the maintainer made for this PR; corrected.
  • Docstrings: see the inline findings.

Existing callers: signatures of _execute / _find_previous_query_id are unchanged (dbt-athena 1.x, #734). WithFetch.__init__ is gone, but super().__init__(**kwargs) from subclasses now reaches BaseCursor.__init__ with the same arguments.

Not measured: live AWS request counts; the equivalence above is static plus the mocked paging test.

Comment thread pyathena/result_set.py Outdated
override them with async versions.
"""

def __init__(self):

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Independent review finding 2 (P3, relayed): "The new test helper and the changed WithResultSet.__init__ lack the required Google-style function docstrings." The helper is tests/pyathena/util.py:54 (succeeded_query_execution).

Author verification: confirmed against the repository rule that touched functions carry Google-style docstrings. Documentation only; no behavior impact. Accepted; the repair follows in a reply.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Repair: ae77fbc adds a docstring to WithResultSet.__init__ (no arguments, so no Args section) and a Google-style Args/Returns docstring to succeeded_query_execution in tests/pyathena/util.py. Documentation only; just lint passed, and the 10 cache search tests pass.

cursor = Cursor.__new__(Cursor)
cursor._schema_name = "this_schema"
cursor._catalog_name = None
page = [

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Independent review finding 1 (P3, relayed): "The new expiry tests can pass vacuously. If the moved expiry comparison raises on the first page, _find_previous_query_id catches the exception and returns None; both tests still see the expected result and exactly one page request." Same for tests/pyathena/aio/test_cursor.py:242.

Author verification: confirmed. The except Exception in _find_previous_query_id turns a failure in _match_previous_query (for example, comparing a naive and an aware datetime) into the cache miss that the test expects. Accepted; the repair follows in a reply.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Repair: ae77fbc. Both expiry tests (sync and aio) now take caplog and assert that "Failed to check the cache" was not logged, so an exception in the expiry branch no longer passes as a cache miss.
Mutation check: with the expiry comparison made to raise (< expiration_time.replace(tzinfo=None), aware vs naive), both expiry tests failed (2 failed, 8 passed). After restoring the code: 10 passed. just lint passed.
Self-review of the repair: test-only. The warning comes from the pyathena.common logger, which propagates to caplog at WARNING, as the mutation run shows. The new comment states only what the assertion checks.

Comment thread pyathena/common.py
return execution.query_id, False
return None, False

def _find_previous_query_id(

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Independent review (relayed Codex result): FINDINGS (2 × P3)

Reviewer: OpenAI Codex CLI 0.157.1, model gpt-6-sol, reasoning effort high, codex exec -s read-only --ephemeral, session 01a0e8e4-b3a7-7f20-a371-f8847f8b14c0.
Range: git diff a86a180ebbe5e18920cd75802ea5350ce8d33f10..a4a5d61099f10182071e15e7cab1081e8966d270 on a detached snapshot at a4a5d61. The prompt omitted the PR number, description, commit messages, and prior findings. Static review only; the reviewer ran no tests. The snapshot and the PR worktree were unchanged afterwards.

Reviewer's coverage: the diff; sync and asyncio cache paging, expiry, bounds, exception handling, and returned IDs; _execute request construction and legacy signatures; the MRO and initialization path for the sync and aio cursor subclasses, including fetch, state, and arraysize resolution; the changed tests and docs. Reviewer's conclusion: "Source behavior appears equivalent."

Findings (verified by the author; both accepted): see the two inline comments.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Independent follow-up (relayed Codex result): CLEAN.

Reviewer: OpenAI Codex CLI 0.157.1, model gpt-6-sol, reasoning effort high, codex exec -s read-only --ephemeral, session 01a0e8f0-451b-71f3-95d0-9faf924f6674.
Range: repair git diff a4a5d61099f10182071e15e7cab1081e8966d270..ae77fbcb6d43ee529473e057d76694405786359d on a detached snapshot at ae77fbc. The base is unchanged (a86a180), and the repair touches only tests and one docstring, so the narrow follow-up applies. Static review; the reviewer ran no tests. The snapshot and the PR worktree were unchanged afterwards.

Reviewer's result: "Both tests check the exact warning prefix emitted by their lookup method. Those warnings are logged at WARNING and reach pytest's default caplog capture. The docstrings and comments match the inspected code."

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@laughingman7743
laughingman7743 marked this pull request as ready for review September 28, 2026 16:56
@laughingman7743
laughingman7743 marked this pull request as draft September 29, 2026 14:50
laughingman7743 and others added 2 commits September 29, 2026 23:51
After the shared members moved to WithResultSet, WithFetch held only
the sync close, executemany, and cancel. WithResultSet now subclasses
BaseCursor and CursorIterator and holds them, and the sync SQL cursors
subclass it directly. WithAsyncFetch subclasses AioBaseCursor and
WithResultSet and overrides executemany and cancel with async versions.

Because WithResultSet now precedes BaseCursor in the MRO, its close
implements the abstract BaseCursor.close, so the copy in each mixin is
gone.

Breaking change: pyathena.result_set.WithFetch is removed, and a class
that lists BaseCursor before WithResultSet in its bases can no longer
be created.

Refs #880

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Comment thread pyathena/result_set.py
class WithResultSet:
def __init__(self):
super().__init__()
class WithResultSet(BaseCursor, CursorIterator):

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Self-review round one (implementation behavior) of the WithFetch fold: CLEAN

Range: git diff ae77fbcb6d43ee529473e057d76694405786359d..1b594b13f2977cb5b165b5c767ac11b63eecaf99 (the maintainer asked to remove the near-empty WithFetch). Base unchanged (a86a180).

Covered:

  • Member resolution: for all 12 sync and asyncio SQL cursor classes, every attribute resolves to a function whose bytecode matches a86a180, except the refactored _execute / _find_previous_query_id. __abstractmethods__ is empty for all 12. (Throwaway script, both trees imported side by side.)
  • MRO: WithAsyncFetch → AioBaseCursor → WithResultSet → BaseCursor → CursorIterator. Async _execute/_poll still come from AioBaseCursor; close from WithResultSet now implements the abstract BaseCursor.close; the async executemany/cancel in WithAsyncFetch precede the sync ones.
  • __init__ chain: WithResultSet.__init__(**kwargs) → BaseCursor.__init__(**kwargs) → CursorIterator.__init__(), then _query_id/_result_set = None, the same order as the removed WithFetch.__init__.
  • References: no remaining WithFetch in pyathena/, tests/, or docs/; no isinstance checks against it.
  • mypy: the async cancel now overrides a sync method, so it carries # type: ignore[override], the same as the async executemany already did.

Duplication after the fold: 17 pairs / 281 lines (the close pair is gone).
Not run locally: the AWS suites for 7d4562a; the class change is covered by the bytecode comparison, and the AWS CI runs after Ready.

Comment thread pyathena/aio/common.py

class WithAsyncFetch(AioBaseCursor, CursorIterator, WithResultSet):
"""Mixin providing shared fetch, lifecycle, and async protocol for SQL cursors.
class WithAsyncFetch(AioBaseCursor, WithResultSet):

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Self-review round two (claims, callers) of the WithFetch fold: FINDINGS (1, corrected)

Claims checked:

  • PR body, breaking change: "a class that lists BaseCursor or CursorIterator before WithResultSet fails with an MRO TypeError". Verified with type() for (BaseCursor, CursorIterator, WithResultSet), (BaseCursor, WithResultSet), and (CursorIterator, WithResultSet), which all raise TypeError, and (WithResultSet,) and (WithResultSet, BaseCursor, CursorIterator), which work. The body's advice now says "first or alone".
  • PR body: WithFetch "was added in v3.29.0 and is not in the API docs". Checked: git tag --contains b753495 starts at v3.29.0, and docs/ has no WithFetch. WithResultSet is in docs/api/connection.rst (:members: only, under Result Sets; its placement is left as is).
  • Existing callers: from pyathena.result_set import WithFetch now raises ImportError. The maintainer chose no alias, for the 4.0.0 major release.
  • Finding: the new WithResultSet docstring (7d4562a) said WithAsyncFetch "overrides the fetch and lifecycle methods". It overrides only executemany / cancel; its subclasses override fetch; close is shared. Corrected in 1b594b1 (docstring only).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Comment thread pyathena/result_set.py

@property
@abstractmethod
def result_set(self) -> AthenaResultSet | None:

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Independent review 2 (relayed Codex result), full expanded scope after the WithFetch fold: FINDINGS (3 × P3; 1 accepted, 2 pre-existing)

Reviewer: OpenAI Codex CLI 0.157.1, codex exec -s read-only --ephemeral, configured model gpt-6-sol with reasoning effort high (from ~/.codex/config.toml, the same as independent review 1). The session ID is not available: the run's log file was removed from the shared temp directory before it was read, and --ephemeral does not persist sessions.
Range: git diff a86a180ebbe5e18920cd75802ea5350ce8d33f10..1b594b13f2977cb5b165b5c767ac11b63eecaf99 on a detached snapshot at 1b594b1. This is a full review, because removing WithFetch changes the contract. The prompt described the intended change and the accepted breaking changes, but omitted the PR number, description, commits, and prior findings. Static review; the reviewer ran no tests. The snapshot and the PR worktree were unchanged afterwards.

Reviewer's coverage: the diff against the merge-base; member resolution and initialization for all sync and asyncio SQL cursor classes; the retained private signatures; references in pyathena/, tests/, and docs/. Reviewer's conclusions: "The moved query request and cache matching logic preserves the prior results for supported inputs. The added cache tests assert paging and matching behavior and do not appear able to pass vacuously or hang."

Findings:

  1. P3: the concrete result_set / query_id and the relocated arraysize / rownumber properties have no docstrings (pyathena/result_set.py:825, :1059). Accepted; the repair follows in a reply.
  2. P3, pre-existing: "BaseCursor.__init__ calls super().__init__() without forwarding **kwargs (pyathena/common.py:191). Passing arraysize=25 when creating a cursor therefore leaves CursorIterator.__init__ to use its default of 1000." Author verification: reproduced on a86a180 (Cursor(arraysize=25, ...).arraysize == 1000) and unchanged by this PR. Deferred as out of scope; reported to the maintainer.
  3. P3, pre-existing: asyncio SQL cursors inherit the sync __next__ from CursorIterator (pyathena/common.py:114), so a plain for loop yields coroutine objects. Author verification: CursorIterator owns __next__ for AioCursor both on a86a180 and on this head. Deferred as out of scope; reported to the maintainer.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Repair for finding 1: a874aed adds Google-style docstrings to the result_set, query_id, arraysize (including the ProgrammingError its setter raises for values <= 0), and rownumber properties of WithResultSet. Documentation only; just lint passed.
Self-review of the repair: I checked each statement against the code. rownumber starts at 0 and increases per fetched row, and it can be None while a result set exists (before initialization or after close), hence "or the index is unknown". query_id and result_set are reset by _reset_state.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Independent follow-ups (relayed Codex results) on the finding 1 repair

  1. a874aed (reviewer session 01a0edb2-7db7-75b3-b95b-39021abbde4b, gpt-6-sol, effort high, read-only, range 1b594b1..a874aed): FINDINGS, 2, both verified and accepted.
    • query_id, "last started query": with cache_size / cache_expiration_time, _execute returns a previous execution's ID without starting a query.
    • arraysize, "fetches by default": execute() copies it into the new result set (AthenaResultSet.fetchmany uses self._arraysize), so a later change does not affect the current result set.
  2. Repair 3e376d2 rewrites both docstrings (documentation only; just lint passed).
  3. 3e376d2 (reviewer session 01a0edb4-9201-7632-aa0c-b586442cac9f, same settings, range a874aed..3e376d2): CLEAN. "The revised query_id and arraysize descriptions match their execution and fetch paths. The diff contains only docstring changes; no executable code changed."

All reviews were static. The snapshot and the PR worktree were unchanged after each run.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deferred findings 2 and 3 are now tracked separately, at the maintainer's agreement, as behavior fixes outside this refactor: #897 (arraysize keyword ignored) and #898 (plain for over asyncio cursors yields coroutines). Both were reproduced on master 51532ed without AWS.

laughingman7743 and others added 2 commits September 30, 2026 00:06
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…-base

# Conflicts:
#	pyathena/aio/common.py
#	pyathena/common.py
#	tests/pyathena/aio/test_cursor.py
#	tests/pyathena/test_cursor.py
Comment thread pyathena/common.py
if (
expiration_time
and execution.completion_date_time
and execution.completion_date_time.astimezone(UTC) < expiration_time

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Merge of master (d4938df) and its review: CLEAN

master advanced to 6effaa3 (#884 drops Python 3.10 and rewrites timezone.utc → datetime.UTC; #890; #870), and the PR conflicted in pyathena/common.py, pyathena/aio/common.py, and both cursor test files. Resolved by merging master into the branch (no force push). The resolution kept this PR's refactored hunks and converted its new code to UTC.

Author checks:

  • The PR diff before (a86a180..3e376d2) and after (6effaa3..d4938df) differs only in timezone.utc → UTC and the matching imports (diff of the two diffs).
  • No WithFetch reference in pyathena/, tests/, or docs/ after the merge.
  • Member resolution of all 12 SQL cursors against 6effaa3: identical except the refactored _execute / _find_previous_query_id. rownumber's raw bytecode differs only because its new docstring shifts the constant index; dis instructions compared by value are identical. __abstractmethods__ is empty.
  • just lint passed; the 10 cache search tests pass.

Independent check (relayed Codex result, session 01a0edb8-8c91-7f92-b85e-603750e2060d, gpt-6-sol, effort high, read-only, snapshot at d4938df): CLEAN. "After accounting for the UTC import and call-site rewrites and two reformatted comparisons removed by the refactor, the 773 changed lines match. No master change was lost, no changed file retains a timezone reference or unused import, and no intervening master commit depends on the changed class structure." Static review.

@laughingman7743
laughingman7743 marked this pull request as ready for review September 29, 2026 15:15
@laughingman7743
laughingman7743 merged commit f15e640 into master Sep 29, 2026
12 checks passed
@laughingman7743
laughingman7743 deleted the refactor/880-cursor-base branch September 29, 2026 15:36
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.

1 participant