Keep the whole pandas and Polars results reusable - #1005
Conversation
Without chunksize, PandasCursor.as_pandas() and PolarsCursor.as_polars() and as_arrow() read the same iterator as the fetch methods and consumed it, so a second call returned an empty frame and later fetches returned nothing. The result sets now keep the whole DataFrame and return it on every call. The fetch methods read a shallow copy (a clone for Polars), so changes to the returned DataFrame do not reach them, and iter_chunks() returns a new iterator over the DataFrame on each call. Chunked reads keep their single-pass iterator. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| import pandas as pd | ||
|
|
||
| # The whole result when it was not read in chunks. | ||
| self._df: DataFrame | None = None |
There was a problem hiding this comment.
Self-review round one (implementation behavior): CLEAN
Base a18ebdaa80c0c85466eacbdb3eb3df76d7ed135a, head f41ca3c05cf61338ab2ab408541efdb6cf13b9fc.
Covered: AthenaPandasResultSet / AthenaPolarsResultSet construction for the S3 output, GetQueryResults fallback, and failed-state paths; as_pandas(), as_polars(), as_arrow(), iter_chunks(), close(); the sync/async/aio cursor wrappers (no subclasses; PandasCursor.iter_chunks() closes the iterator it receives, which is now a fresh one in non-chunked mode); explicit chunksize and auto_optimize_chunksize chunked paths (unchanged single-pass); the two new tests.
Checks:
- Hypothesis: eager
_trunc_datein__init__could now raise on results that were never read. Refuted live: zero-rowtime, all-NULLtime, and mixedtime/NULL results convert identically on master and this head (master only differs in returning[]fromfetchall()afteras_pandas(), which is the bug). - Mutation isolation verified offline (pandas 3.0.6 CoW, polars 1.44.2): without the shallow copy / clone,
df["a"] = 0, in-placerename, anddf[0, "a"] = 0leaked into fetched rows; with them, they do not. - Both new tests fail on the original code.
Limitation (not a defect): in non-chunked mode the result set now holds the DataFrame until close() or the next execute(), as before v2.13.0 for pandas, before v3.24.0 for Polars, and like ArrowCursor. Previously a fetch-only caller released it once the single chunk was exhausted.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| When chunksize is specified, or ``auto_optimize_chunksize`` chose a chunk size | ||
| for a large CSV result, it yields DataFrames in chunks for memory-efficient | ||
| processing. Otherwise, it yields the entire result as a single DataFrame. | ||
| When a CSV result is read in chunks, because chunksize is specified or |
There was a problem hiding this comment.
Self-review round two (claims, callers, operations): FINDINGS → repaired in a26c763
Base a18ebdaa80c0c85466eacbdb3eb3df76d7ed135a, head f41ca3c05cf61338ab2ab408541efdb6cf13b9fc. Full pass over the PR body, the commit message, and the changed docstrings and comments.
Claims checked:
- "pandas since v2.13.0; Polars since v3.24.0":
git tag --containsputs d5cb976 first in v2.13.0 and 6f7addc first in v3.24.0. Before v3.24.0,as_polars()returnedself._df. - Shallow-copy isolation relies on Copy-on-Write:
pyproject.tomlrequirespandas>=3.0.0(CoW is always on) andpolars>=1.39.0; verified offline in round one. - "async/aio variants share the result sets":
pandas/async_cursor.py,polars/async_cursor.py, andaio/{pandas,polars}/cursor.pyconstructAthenaPandasResultSet/AthenaPolarsResultSet. PandasCursor.iter_chunks()closes its iterator (with result_set.iter_chunks() as chunks); on master that closed the shared fetch iterator.
Findings (repaired):
pyathena/pandas/result_set.pyiter_chunks(): the new sentence said chunks withchunksizecome from the fetch iterator, but_read_parquetignoreschunksize. A pandas UNLOAD result withchunksize=1000gets the stored DataFrame and a fresh iterator. The docstring now conditions on the CSV result being read in chunks.pyathena/polars/result_set.pyas_polars()/as_arrow()/iter_chunks(): "when chunksize is set ... a later call returns an empty DataFrame" was false for results without an S3 result file (the GetQueryResults path stores the DataFrame even withchunksize). The wording now conditions on the result file being read in chunks.- The PR body now states the pandas UNLOAD/
chunksizecase.
Callers and operations: no change in S3 or Athena requests (same reads, same places). No user guide in docs/ describes the old consuming behavior. #927 wrote the guides for the intended behavior. Behavior changes are listed in the release note. Limitation from round one (DataFrame retained until close()) still stands.
| if self._df is not None: | ||
| # A shallow copy keeps changes to the DataFrame from as_pandas() | ||
| # out of the rows that the fetch methods return. | ||
| self._df_iter = PandasDataFrameIterator(self._df.copy(deep=False), _no_trunc_date) |
There was a problem hiding this comment.
Independent review (relayed): FINDINGS
Reviewer: Codex CLI 0.160.0, model gpt-6-astra, reasoning effort high, sandbox read-only, session 01a1015d-691c-71d1-8c1a-e2e76abd7851. Static review only (no builds, tests, or network). Snapshot: detached worktree at head a26c7633d81164fed18507525713ecb950385fc4, base a18ebdaa80c0c85466eacbdb3eb3df76d7ed135a. The prompt contained the literal diff and the intended behavior, without the PR number, description, commits, or prior findings. Afterwards, both the snapshot and the PR worktree were clean and still at a26c7633.
Covered (reviewer): S3 CSV, UNLOAD Parquet, API fallback, empty and non-SUCCEEDED construction; whole-result access, explicit/automatic chunking, fetch positions, close/reset, CSV streams and readers; mutation isolation, memory retention, pandas time truncation, docs; sync, future-based async, and aio wrappers; both new tests.
Findings and author verification:
- Diff-related, P2
pyathena/pandas/result_set.py:364: the shallow copy does not isolate mutable cell contents. WithCAST(ARRAY[1, 2] AS JSON),df.at[0, "j"].append(3)changes the laterfetchone(). Verified (object cells are shared Python objects). Action: narrow the comment and the PR body to assignments to the DataFrame. A deep copy would double the memory. - Pre-existing, made worse by the diff, P2
pyathena/pandas/result_set.py:358: in the GetQueryResults fallback, TIME values are alreadydatetime.time, so_trunc_date's.dt.timeraisesAttributeError. Verified live with the managed work group: master raises it onfetchone(), while this head raises it inexecute(). Action: fix in this PR. - Pre-existing, P2
pyathena/polars/result_set.py:138: the fallback DataFrame already holds converted values, anditerrows()converts them again. Verified live: TIMEfetchone()raisesTypeError: strptime() argument 1 must be str, not datetime.timeon both master and head. Action (maintainer chose to fold in): fix in this PR. - Pre-existing, P2
pyathena/polars/result_set.py:359: chunked UNLOAD keeps the UNLOAD command metadata. Verified live:unload=True, chunksize=10gives description[('rows', 'bigint', ...)]andfetchone()(None,); without chunksize,(1, 'x'). Action (maintainer): separate issue. - Pre-existing, P2
pyathena/polars/result_set.py:711:close()replaces_df_iterwithout closing it (pandas closes it). Verified by reading the code. Action (maintainer chose to fold in): fix in this PR.
There was a problem hiding this comment.
Repair: c857dd3 (on top of a26c763; same base a18ebda)
- Finding 1: narrowed both comments ("assignments to the DataFrame"; pandas notes that mutable cell values such as JSON lists stay shared) and the PR body. No deep copy.
- Finding 2: the pandas GetQueryResults fallback no longer applies
_trunc_date.DefaultTypeConverteralready returnsdatetime.timefortime.time with time zonestays a string, as inCursor; the old.dt.timealso raised on it. - Finding 3: the Polars fallback uses no row converters (
_df_converters = {}), for the fetch iterator and foriter_chunks(). - Finding 4: filed as PolarsCursor with unload=True and chunksize returns the UNLOAD statement's metadata and None rows #1007.
- Finding 5:
AthenaPolarsResultSet.close()callsself._df_iter.close()before replacing it. For a whole-result iterator,close()is a no-op; a chunk generator getsGeneratorExit, which theexcept Exceptionin_iter_*_chunksdoes not catch.
Validation: just lint passed. test_fetch_all_rows[managed] (pandas and Polars) now selects a TIME column, and the new test_close_stops_chunks covers finding 5. With the source changes of c857dd3 reverted, these 3 fail; with them, they pass. The related pandas, Polars, and aio subsets: 177 passed.
Self-review of the repair (round one, behavior): no findings. The fallback DataFrame types are unchanged, and only the fetch conversion and the truncation differ. Round two (claims): the commit message, comments, and PR body were rechecked against the measured errors (AttributeError in execute() on a26c763 and in fetchone() on master; TypeError from strptime()); no findings.
There was a problem hiding this comment.
Independent follow-up review (relayed): no regressions; 2 pre-existing findings
Reviewer: Codex CLI 0.160.0, gpt-6-astra, effort high, read-only, session 01a1016a-31a5-7862-ac29-da0c5dd01643. Static review of a26c7633..c857dd3e06a2d5a466e2997f110ea533a2267db5 (range-diff: earlier commits unchanged) traced into both result sets, the cursor wrappers, _fetch_all_rows, and the converters. Afterwards, the snapshot and the PR worktree were clean at c857dd3e.
The reviewer resolved all four earlier comments: assignment isolation is documented with the shared mutable cells; fallback TIME bypasses .dt.time (also with chunksize); Polars fallback rows are not converted again, for fetch and iter_chunks().iterrows(); and the stored CSV/Parquet chunk generators are closed. By source tracing, the managed TIME tests and test_close_stops_chunks fail without the fix.
Pre-existing findings and author disposition:
- P2
pyathena/converter.py:481:DefaultTypeConverterhas notime with time zonemapping, so fallback values stay strings. That is the existingCursorbehavior, out of scope. It did show that the new comment atpyathena/pandas/result_set.py:358("time columns hold times") was too broad. Repaired in 7ce2bf4 (comment only: "already converted and need no time truncation").just lintpassed. - P3
pyathena/polars/result_set.py:124:PolarsDataFrameIterator.close()does nothing for a whole-DataFrame reader, unlike the pandas iterator. Deferred: this is pre-existing, the iterator only holds a DataFrame reference, andAthenaPolarsResultSet.close()replaces the iterator anyway.
There was a problem hiding this comment.
Independent follow-up (relayed): CLEAN for c857dd3e..7ce2bf4ca262003d4b3708a77161a7c03c0382e6 (comment only).
Reviewer: Codex CLI 0.160.0, gpt-6-astra, effort high, read-only, session 01a1016e-7848-7901-ac2a-c2eb4a17074f. Result: the comment at pyathena/pandas/result_set.py:358 is accurate. With no output location, DefaultTypeConverter turns time into datetime.time and leaves time with time zone as a string; neither needs _trunc_date(). Static review only.
With no S3 result file, the rows from GetQueryResults are already converted. PandasCursor no longer applies the time truncation to them, which raised AttributeError for TIME columns (now in execute(), before in the fetch methods), and PolarsCursor no longer converts them again in the fetch methods, which raised TypeError for TIME columns. AthenaPolarsResultSet.close() now closes the chunk iterator, as the pandas result set does. The shallow copy comments now say that mutable cell values stay shared. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- _to_json returns None for an empty string, which CSV results use for SQL NULL, so PandasCursor and ArrowCursor no longer raise JSONDecodeError on a NULL json value. - ArrowCursor applies its converters only to tables read from S3. With managed query result storage, the GetQueryResults rows are already converted, and converting them again raised TypeError for JSON objects, TIME, and VARBINARY and decoded JSON string scalars a second time. PolarsCursor got the same fix in #1005. - S3FSCursor converts GetQueryResults rows with its own converter, as it does for the CSV result file, instead of DefaultTypeConverter. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
WHAT
Without
chunksize, the pandas and Polars result sets keep the whole result as a DataFrame and no longer hand out the iterator that the fetch methods read.PandasCursor.as_pandas(),PolarsCursor.as_polars(), andPolarsCursor.as_arrow()(and the async/aio variants, which share the result sets) return the whole result on every call.as_pandas()/as_polars()return the same DataFrame object each time;as_arrow()converts it.copy(deep=False)under pandas 3 Copy-on-Write,clone()for Polars), so assignments to the returned DataFrame do not change the fetched rows, and callingas_*()does not move the fetch position. Mutable values inside cells, such as lists from JSON columns, stay shared; a deep copy would double the memory.iter_chunks()without chunking returns a new iterator over the stored DataFrame on each call, so it no longer drains the fetch iterator, andPandasCursor.iter_chunks()closing its iterator no longer empties the result.chunksize, or a chunk size chosen byauto_optimize_chunksize) keep their single-pass iterator shared with the fetch methods; the docstrings now state this.chunksize(pandas UNLOAD results, whichread_parquetreads whole, and results without an S3 result file) also gets the stored DataFrame; for pandas,as_pandas()withchunksizethen returns a new iterator over it on each call.PandasCursorno longer applies the time truncation to them (a TIME column raisedAttributeErroron fetch/as_pandas()), andPolarsCursorno longer converts them again in the fetch methods (a TIME column raisedTypeError: strptime() argument 1 must be str, not datetime.time). Found by the independent review; the pandas case would otherwise have moved intoexecute()with this change.AthenaPolarsResultSet.close()closes the chunk iterator before replacing it, as the pandas result set does, so a retainediter_chunks()iterator stops.Release notes (4.0.0):
chunksize, a repeatedas_pandas()/as_polars()/as_arrow()returns the whole result instead of an empty frame, and fetches after them return the rows;as_pandas()/as_polars()return the same object on each call, and the result set keeps it untilclose()or the nextexecute().PandasCursorand inPolarsCursorfetches.PolarsCursorresult set closes its chunk iterator.WHY
Closes #935.
as_pandas()returnednext()/ joined the iterator thatfetchone()/fetchmany()/fetchall()also read, so a second call returned an empty DataFrame and a fetch afterwards returned nothing (pandas since v2.13.0; Polars since v3.24.0, which replaced the stored_dfwith the iterator wrapper).Maintainer decisions: the
auto_optimize_chunksizechunked case keeps the single-pass behavior from #950 (no caching, no S3 re-read), the non-chunkediter_chunks()is aligned in this PR, and the GetQueryResults fallback conversion and Polarsclose()defects found in review are fixed here. The chunked Polars UNLOAD metadata defect found in the same review is #1007.TEST
Tested commit: c857dd3 (7ce2bf4 changes one comment only;
just lintpassed)just lint: passed.TestPandasCursor::test_pandas_cursor_whole_result_reusedandTestPolarsCursor::test_whole_result_reusedcover repeatedas_*(), assignment isolation, fetch position acrossas_*()/iter_chunks(), and repeatediter_chunks(). Both fail on the original code (as_pandas()/as_polars()returned an empty frame on the second call).test_fetch_all_rows[managed](pandas and Polars) now selects a TIME column, and the newTestPolarsCursor::test_close_stops_chunkschecks that a retained chunk iterator stops afterclose(). With the source changes of c857dd3 reverted, these three fail (AttributeError,TypeError, and remaining chunks); with them, they pass.uv run --env-file .env pytest -n 4 tests/pyathena/pandas tests/pyathena/polars tests/pyathena/aio/pandas tests/pyathena/aio/polars -k "as_pandas or as_polars or as_arrow or iter_chunks or fetch or close or managed or Iterator or whole_result": 177 passed (sync, async, and aio cursors, including the managed work group).df["a"] = 0, an in-placerename, ordf[0, "a"] = 0on the returned DataFrame changed the rows that the fetch iterator later returned; with them, it did not.just test pyathenasuite; it runs in CI when the PR is marked Ready.🤖 Generated with Claude Code