Restore WithResultSet as a mixin with sync and async sibling bases - #909
Conversation
| self.result_set.close() | ||
|
|
||
|
|
||
| class WithFetch(WithResultSet, BaseCursor, CursorIterator): |
There was a problem hiding this comment.
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/cancelmoves toWithFetch.__abstractmethods__is empty for all 12. __init__chain:aio/common.pydefines no__init__, soWithResultSet.__init__→BaseCursor.__init__→CursorIterator.__init__runs in the same order as on master. Instantiating all 12 cursors with and withoutarraysize=5000gives identical instance attributes (names, order, values),arraysize/rowcount/rownumber/query_id/result_set,ProgrammingErrorfor the capped cursors, androwcountafterclose(), on master and this branch.- Callers: nothing in
pyathena/usesisinstance/issubclasswith these classes, soWithResultSetno longer subclassingBaseCursordoes not affect a code path. - Test quality:
test_result_set.pyfails on master (WithFetchimport) 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>
7d9ba8f to
85f0baa
Compare
|
|
||
|
|
||
| class WithAsyncFetch(AioBaseCursor, WithResultSet): | ||
| class WithAsyncFetch(WithResultSet, AioBaseCursor, CursorIterator): |
There was a problem hiding this comment.
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:and51532ed7:ofresult_set.pyandaio/common.pymatch the table. - "Members
WithFetchandWithAsyncFetchduplicated before Share the pure cursor-base logic between the sync and asyncio cursors #883":51532ed7defines__init__,arraysize,result_set,query_id,rownumber,rowcount,closein both; all are now inWithResultSetonly. - "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 MROTypeErroron master and works on this branch and on v3.37.0; aWithFetchsubclass works on this branch and v3.37.0 (itsarraysize=7result 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_SIZEinWithResultSet.__init__, "sync iteration" inWithFetch(fromCursorIterator), "subclasses implement the fetch methods as coroutines" (all 5 aio cursors define asyncfetchone/fetchmany/fetchall): true. - Docs: no prose mentions
WithFetch/WithResultSetbeyond theautoclassentries;docs/api/connection.rstnow renders fewer inherited members for the mixin.just docs buildnot run (docs sources unchanged).
Findings and repairs:
- 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. - PR body said the base order is "the only structural difference from v3.37.0", ignoring the members moved into
WithResultSet. Narrowed.
| 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. |
There was a problem hiding this comment.
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.
WHAT
Restores the mixin design that #883 replaced, keeping the deduplication #883 achieved.
WithResultSetWithResultSet(BaseCursor, CursorIterator), base of all SQL cursorsWithFetchWithFetch(BaseCursor, CursorIterator, WithResultSet)WithFetch(WithResultSet, BaseCursor, CursorIterator)WithAsyncFetchWithAsyncFetch(AioBaseCursor, CursorIterator, WithResultSet)WithAsyncFetch(AioBaseCursor, WithResultSet), overrides sync membersWithAsyncFetch(WithResultSet, AioBaseCursor, CursorIterator)WithResultSetholds the members thatWithFetchandWithAsyncFetchduplicated before Share the pure cursor-base logic between the sync and asyncio cursors #883:__init__(with thearraysizeargument from SQL cursors ignore the arraysize keyword argument #897), theresult_set/query_idstorage,arraysize,rownumber,rowcount, andclose, plus the result set properties it always had.WithFetchholds the sync fetch methods,executemany, andcancel.WithAsyncFetchholds the asyncexecutemany/cancel, theTypeErroron 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.WithResultSetfirst, so itsarraysize,rownumber,rowcount, andclosetake precedence over those ofCursorIterator(cappedarraysize) andBaseCursor(abstractclose). Apart from the members moved intoWithResultSet, this order is the only difference in these classes' structure from v3.37.0.WithFetchagain.No behavior change.
WHY
Part of #880.
With*classes are mixins by design. #883 turnedWithResultSetinto a base class and removedWithFetchwithout that design being agreed. This PR restores the design and keeps the shared members in one place.Compared with v3.37.0,
WithFetchandWithAsyncFetchkeep their names and roles, so the 4.0.0 release-note items from #883 ("WithFetchis removed", MROTypeErrorwhen listingBaseCursor/CursorIteratorbeforeWithResultSet) no longer apply. A class that composes the mixin as v3.37.0'sWithFetchdid,class MyCursor(BaseCursor, CursorIterator, WithResultSet)with its ownresult_set/query_id, fails with an MROTypeErroron master and works on this branch as on v3.37.0 (checked with a local script on all three). What remains is thatWithResultSet.result_setandquery_idare concrete instead of abstract, andWithResultSetprovides 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).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/cancelmoved fromWithResultSettoWithFetch. No aio cursor member changed its defining class.__abstractmethods__is empty for all 12.tests/pyathena/test_result_set.pypins the structure:WithResultSethas no base class, andWithFetch/WithAsyncFetchlist it before their cursor base andCursorIterator.--noconftest, dummy env):tests/pyathena/test_result_set.py,tests/pyathena/aio/test_common.py, and thearraysize/ iteration tests intests/pyathena/test_connection.py,tests/pyathena/aio/test_connection.py,tests/pyathena/aio/test_result_set.py: passed.test / run (3.14)passed.test-sqla/test-sqla-asyncwere skipped by the path filter (nosqlalchemypaths 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