Skip to content

Restore WithResultSet as a mixin with sync and async sibling bases - #909

Merged
laughingman7743 merged 1 commit into
masterfrom
refactor/880-restore-mixins
Oct 1, 2026
Merged

laughingman7743 merged 1 commit into
masterfrom
refactor/880-restore-mixins

Conversation

@laughingman7743

@laughingman7743 laughingman7743 commented Oct 1, 2026 •

Copy link
Copy Markdown
Member

WHAT

Restores the mixin design that #883 replaced, keeping the deduplication #883 achieved.

v3.37.0 / before #883 master (after #883) This PR
WithResultSet mixin, no base class WithResultSet(BaseCursor, CursorIterator), base of all SQL cursors mixin, no base class
WithFetch WithFetch(BaseCursor, CursorIterator, WithResultSet) removed WithFetch(WithResultSet, BaseCursor, CursorIterator)
WithAsyncFetch WithAsyncFetch(AioBaseCursor, CursorIterator, WithResultSet) WithAsyncFetch(AioBaseCursor, WithResultSet), overrides sync members WithAsyncFetch(WithResultSet, AioBaseCursor, CursorIterator)
  • WithResultSet holds the members that WithFetch and WithAsyncFetch duplicated before Share the pure cursor-base logic between the sync and asyncio cursors #883: __init__ (with the arraysize argument from SQL cursors ignore the arraysize keyword argument #897), the result_set / query_id storage, arraysize, rownumber, rowcount, and close, plus the result set properties it always had.
  • WithFetch holds the sync fetch methods, executemany, and cancel. WithAsyncFetch holds the async executemany / cancel, the TypeError on sync iteration (Plain for loops over asyncio cursors never end and yield coroutines #898), and the async protocol. The two are siblings again; the async base no longer overrides sync members it inherits.
  • Both list WithResultSet first, so its arraysize, rownumber, rowcount, and close take precedence over those of CursorIterator (capped arraysize) and BaseCursor (abstract close). Apart from the members moved into WithResultSet, this order is the only difference in these classes' structure from v3.37.0.
  • The sync SQL cursors subclass WithFetch again.

No behavior change.

WHY

Part of #880. With* classes are mixins by design. #883 turned WithResultSet into a base class and removed WithFetch without that design being agreed. This PR restores the design and keeps the shared members in one place.

Compared with v3.37.0, WithFetch and WithAsyncFetch keep their names and roles, so the 4.0.0 release-note items from #883 ("WithFetch is removed", MRO TypeError when listing BaseCursor / CursorIterator before WithResultSet) no longer apply. A class that composes the mixin as v3.37.0's WithFetch did, class MyCursor(BaseCursor, CursorIterator, WithResultSet) with its own result_set / query_id, fails with an MRO TypeError on master and works on this branch as on v3.37.0 (checked with a local script on all three). What remains is that WithResultSet.result_set and query_id are concrete instead of abstract, and WithResultSet provides the members listed above.

TEST

Tested at 7d9ba8f; 85f0baa differs only in the commit message.

  • just lint: passed (ruff, format, mypy, license headers, cfn-lint).
  • Member resolution against master (809b380), for the 12 SQL cursors (Cursor, DictCursor, ArrowCursor, PandasCursor, PolarsCursor, S3FSCursor, and the 6 aio cursors): every resolved member has identical instructions (compared by value); only the defining class of the sync cursors' fetchone / fetchmany / fetchall / executemany / cancel moved from WithResultSet to WithFetch. No aio cursor member changed its defining class. __abstractmethods__ is empty for all 12.
  • Duplicated sync/aio pairs in the cursor-base area (method in the Share the pure logic duplicated between sync and asyncio cursors #880 handoff): 17 pairs / 281 aio body lines, unchanged from master.
  • New tests/pyathena/test_result_set.py pins the structure: WithResultSet has no base class, and WithFetch / WithAsyncFetch list it before their cursor base and CursorIterator.
  • Offline (--noconftest, dummy env): tests/pyathena/test_result_set.py, tests/pyathena/aio/test_common.py, and the arraysize / iteration tests in tests/pyathena/test_connection.py, tests/pyathena/aio/test_connection.py, tests/pyathena/aio/test_result_set.py: passed.
  • AWS, CI at 85f0baa: test / run (3.14) passed. test-sqla / test-sqla-async were skipped by the path filter (no sqlalchemy paths changed; see CI skips SQLAlchemy and Spark tests when only shared core modules change #896), so both suites were run locally at 85f0baa (Python 3.13, uv run --env-file .env just test sqla / sqla-async): 589 passed, 759 skipped each.

🤖 Generated with Claude Code

@laughingman7743 laughingman7743 added this to the 4.0.0 milestone Oct 1, 2026
Comment thread pyathena/result_set.py
self.result_set.close()


class WithFetch(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 (behavior and implementation): CLEAN

Base 809b38070ff7ed69b960f9f4318a82e03723fd2c, head 7d9ba8f55b82eae282567d61aae918ff797a595b.

Covered: pyathena/result_set.py (WithResultSet, WithFetch), pyathena/aio/common.py (WithAsyncFetch), the 5 sync cursor modules, tests/pyathena/test_result_set.py.

  • Behavior: for the 12 SQL cursors, every resolved member has the same instructions as on master; only the defining class of the sync fetch* / executemany / cancel moves to WithFetch. __abstractmethods__ is empty for all 12.
  • __init__ chain: aio/common.py defines no __init__, so WithResultSet.__init__ → BaseCursor.__init__ → CursorIterator.__init__ runs in the same order as on master. Instantiating all 12 cursors with and without arraysize=5000 gives identical instance attributes (names, order, values), arraysize / rowcount / rownumber / query_id / result_set, ProgrammingError for the capped cursors, and rowcount after close(), on master and this branch.
  • Callers: nothing in pyathena/ uses isinstance / issubclass with these classes, so WithResultSet no longer subclassing BaseCursor does not affect a code path.
  • Test quality: test_result_set.py fails on master (WithFetch import) and on a fold back into a base class (__bases__), and passes here.

No findings.

#883 folded WithFetch into WithResultSet and made the mixin a subclass
of BaseCursor and CursorIterator, with WithAsyncFetch overriding its sync
methods. Restore the mixin design: WithResultSet has no base class again,
WithFetch is restored for the sync fetch, executemany, and cancel, and
WithAsyncFetch is its asyncio sibling.

The members shared by both bases stay in WithResultSet, which both list
first so that these members take precedence over those of the cursor
bases and CursorIterator.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@laughingman7743
laughingman7743 force-pushed the refactor/880-restore-mixins branch from 7d9ba8f to 85f0baa Compare October 1, 2026 00:25
Comment thread pyathena/aio/common.py


class WithAsyncFetch(AioBaseCursor, WithResultSet):
class WithAsyncFetch(WithResultSet, AioBaseCursor, 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 two (claims, callers, docs): FINDINGS, repaired

Base 809b38070ff7ed69b960f9f4318a82e03723fd2c, head 7d9ba8f55b82eae282567d61aae918ff797a595b → repaired head 85f0baa6b093adfc3b262d0f9a29dbdcd71ccb78 (commit message only; tree unchanged).

Claims checked:

  • PR table (v3.37.0 / pre-Share the pure cursor-base logic between the sync and asyncio cursors #883 / master structures): git show v3.37.0: and 51532ed7: of result_set.py and aio/common.py match the table.
  • "Members WithFetch and WithAsyncFetch duplicated before Share the pure cursor-base logic between the sync and asyncio cursors #883": 51532ed7 defines __init__, arraysize, result_set, query_id, rownumber, rowcount, close in both; all are now in WithResultSet only.
  • "No behavior change": member-resolution and instance-state comparison against master (round one).
  • "Share the pure cursor-base logic between the sync and asyncio cursors #883 release-note items no longer apply": a v3.37.0-style composition class MyCursor(BaseCursor, CursorIterator, WithResultSet) raises the MRO TypeError on master and works on this branch and on v3.37.0; a WithFetch subclass works on this branch and v3.37.0 (its arraysize=7 result differs only by SQL cursors ignore the arraysize keyword argument #897, already release-noted). Added this evidence to the PR body.
  • Docstrings: precedence note, **kwargs / DEFAULT_FETCH_SIZE in WithResultSet.__init__, "sync iteration" in WithFetch (from CursorIterator), "subclasses implement the fetch methods as coroutines" (all 5 aio cursors define async fetchone / fetchmany / fetchall): true.
  • Docs: no prose mentions WithFetch / WithResultSet beyond the autoclass entries; docs/api/connection.rst now renders fewer inherited members for the mixin. just docs build not run (docs sources unchanged).

Findings and repairs:

  1. Commit message said Share the pure cursor-base logic between the sync and asyncio cursors #883 "made the mixin a base class of BaseCursor and CursorIterator"; it made it a subclass. Amended (85f0baa), pushed with --force-with-lease.
  2. PR body said the base order is "the only structural difference from v3.37.0", ignoring the members moved into WithResultSet. Narrowed.

Comment thread pyathena/result_set.py
validated by the ``arraysize`` setter. If None,
``DEFAULT_FETCH_SIZE`` is used.
**kwargs: Arguments passed to ``BaseCursor.__init__``.
**kwargs: Arguments passed to the next ``__init__`` in the MRO.

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): FINDINGS → 1 finding, rejected after verification

Reviewer: Codex CLI 0.157.1, model gpt-6-sol, sandbox read-only, session 01a0f4da-98ac-7700-affd-ec00465f1dd0. Static review only (no lint/tests run by the reviewer, as instructed). Base 809b38070ff7ed69b960f9f4318a82e03723fd2c, head 85f0baa6b093adfc3b262d0f9a29dbdcd71ccb78; the review snapshot and the PR worktree were unchanged afterwards. The prompt contained the diff, the intended structure, and the pre-refactor commit for comparison, without the PR number, description, commit message, or self-review results.

Reviewer coverage: method resolution and initialization of the SQL cursor classes (incl. Dict variants), abstract methods, arraysize and lifecycle behavior, SQLAlchemy and Spark callers, changed docstrings, the new test, repository conventions. Reviewer: "no source-level method-resolution change or newly abstract concrete SQL cursor."

Finding (P2), pyathena/result_set.py:819: with no base beyond object, mypy would check super().__init__(**kwargs) against object.__init__ and just lint would fail.

Verification: rejected. uv run mypy . (inside just lint) reports no issues at this head, uv run mypy pyathena/result_set.py pyathena/aio/common.py succeeds, and the CI lint job passed at 85f0baa. mypy does not report unpacking a dict[str, Any] (**kwargs) into object.__init__, since it cannot know the mapping is non-empty. No change.

@laughingman7743
laughingman7743 marked this pull request as ready for review October 1, 2026 00:31
@laughingman7743
laughingman7743 merged commit 468affc into master Oct 1, 2026
12 checks passed
@laughingman7743
laughingman7743 deleted the refactor/880-restore-mixins branch October 1, 2026 00:58
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