Let execute() override cursor read options and accept them in the AsyncPandasCursor constructor - #1022
Conversation
| chunksize: int | None = None, | ||
| result_reuse_enable: bool = False, | ||
| result_reuse_minutes: int = CursorIterator.DEFAULT_RESULT_REUSE_MINUTES, | ||
| block_size: int | None = None, |
There was a problem hiding this comment.
Self-review round one (behavior, failure paths, data/resource boundaries, simplicity and test quality) — FINDINGS
Scope: git diff 785be364e380a7088b82ecf3b5bcb3a5bf01b9ca..bb006761a94daa69f858347479d6d5b03934e295 — all 12 files: the six pandas/Polars cursors and their test files.
Finding (repaired in d542edb): the new block_size and cache_type parameters were inserted before result_reuse_enable, so a caller passing result_reuse_enable or result_reuse_minutes positionally to AsyncPandasCursor(...) would have set block_size/cache_type instead. All three new parameters are now appended after result_reuse_minutes; keyword callers (including connect(cursor_kwargs=...)) are unaffected either way.
Checked without findings:
- Each changed
kwargs.pop()reads a per-call dict (execute()'s own**kwargs, or the dict submitted to the executor forAsyncPandasCursor/AsyncPolarsCursor), so popping does not leak into later calls. - The popped keys reach the same explicit result-set parameters they reached through
**kwargsbefore, so per-call values that already worked (e.g.AsyncPandasCursor.execute(block_size=...)) behave the same. AsyncPandasCursor/AsyncPolarsCursor:max_workersgiven toexecute()sets only the S3 read workers; the executor keeps the constructor size.execute(block_size=None)now overrides the cursor value with None, matching the existingPandasCursorpop semantics.- Tests: the offline
test_read_optionscases fail on the base (5TypeErrors, plus theAsyncPandasCursorconstructor case) and assert the actual result-set arguments; the livetest_auto_optimize_chunksizecovers the real chunked read throughcursor_kwargs.
| cache_type=self._cache_type, | ||
| max_workers=self._max_workers, | ||
| chunksize=self._chunksize, | ||
| block_size=kwargs.pop("block_size", self._block_size), |
There was a problem hiding this comment.
Self-review round one — out-of-scope note (not changed in this PR): the same "cursor attribute plus **kwargs" collision exists outside the pandas/Polars scope agreed for this PR:
pyathena/arrow/cursor.py:213-214,pyathena/arrow/async_cursor.py:174-175,pyathena/aio/arrow/cursor.py:185-186: per-callconnect_timeout/request_timeoutraiseTypeError.pyathena/s3fs/cursor.py:206,pyathena/s3fs/async_cursor.py:175,pyathena/aio/s3fs/cursor.py:185: per-callcsv_readerraisesTypeError.pyathena/polars/result_set.py:458,488,506,650,681: a per-callstorage_optionscollides with the result set's ownstorage_optionsat read time.
To be filed as a separate issue after the maintainer confirms.
| **kwargs: Additional pandas read_csv/read_parquet parameters. | ||
| ``engine``, ``chunksize``, ``block_size``, ``cache_type``, and | ||
| ``auto_optimize_chunksize`` override the cursor's values for this query. | ||
| ``max_workers`` sets the number of S3 read workers for this query. |
There was a problem hiding this comment.
Self-review round two (claims, existing callers, documentation, evidence) — FINDINGS (PR description only; no code change)
Scope: git diff 785be364e380a7088b82ecf3b5bcb3a5bf01b9ca..d542edb21cef28aab2636523585efd1aee5df814 (full pass), PR body, commit messages, changed docstrings, docs/pandas.md, docs/polars.md, docs/aio.md.
Claims checked:
- "Constructor values were silently lost": on the base the offline constructor case fails with
KeyError: 'block_size'(the value never reachesAthenaPandasResultSet);BaseCursor.__init__documents**kwargs: Ignored(pyathena/common.py). - "Per-call values raised
TypeError": on the base, PandasCursor/PolarsCursor/AsyncPolarsCursor fail withgot multiple values for keyword argument, and AioPandasCursor/AioPolarsCursor fail the same way insideasyncio.to_thread(). - "
max_workerssizes the thread pool" (this docstring):pyathena/async_cursor.py:112-113uses it forThreadPoolExecutor, so the per-call value only sets S3 read workers. - "
auto_optimize_chunksize... CSV result file":_auto_determine_chunksizeis called only from_read_csv(pyathena/pandas/result_set.py:560-562). - Existing callers: the round-one parameter-order repair keeps positional calls; per-call keys that already worked keep reaching the same result-set parameters; no other per-call key changes destination.
- Docs: no statement in
docs/says AsyncPandasCursor lacks these options or that per-call overrides fail; no doc change needed.
Findings (repaired in the PR description):
- The live-test claim was overstated:
-kfiltered the aio files down to the offlinetest_read_options, so the Polars and aio execute paths had no live run. Ran at the head: both aio files in full (36 passed) and-k chunkover the Polars cursor files (15 passed). - "Tested commit" named bb00676 after the round-one repair; it now names the head and attributes the pandas live run to bb00676.
Limits: the rest of the pandas/Polars suites and just test pyathena run only in CI after Ready.
| }, | ||
| ], | ||
| ) | ||
| def test_read_options(self, execute_kwargs): |
There was a problem hiding this comment.
Independent review (relayed): FINDINGS (one P2, pre-existing test-infrastructure scope) → not changed
Reviewer: Codex CLI 0.160.0, model gpt-6-astra, reasoning effort high, sandbox read-only, session 01a10201-a44c-7562-b6df-80f20295d29b. Static review only. Snapshot: detached worktree at head d542edb21cef28aab2636523585efd1aee5df814, base 785be364e380a7088b82ecf3b5bcb3a5bf01b9ca. The prompt contained the literal diff and the intended behavior, without the PR number, description, or prior findings. Afterwards, the snapshot and the PR worktree were clean at d542edb2.
Covered (reviewer): all six cursor implementations (constructor defaults, per-call overrides, positional compatibility, worker-count semantics); connect(cursor_kwargs=...), result-set consumers, failure propagation, futures, and asyncio execution; docstrings, fixtures, regression sensitivity against the base, and the observable chunk assertions. No production-code defect found; the reviewer confirmed the regression cases detect the base's missing defaults and duplicate keywords, and the live case asserts actual chunk contents.
Finding: P2, this line and the other five test_read_options additions. Run normally, these tests still pass through the session hook in tests/pyathena/conftest.py:15, which uploads S3 data and creates Athena tables before the mocks apply; the reviewer calls the hook behavior pre-existing.
Author disposition: not changed. The tests themselves make no AWS calls, which is all their docstrings claim. They follow the existing pattern for such tests in this repository (tests/pyathena/test_cursor.py _offline_cursor and the "(no AWS)" passthrough tests sit under the same conftest), and they run without AWS through --noconftest, as recorded in the PR description. Making the session setup lazy for mock-only tests is test-infrastructure work outside this PR.
laughingman7743
changed the title
| self._column_names_cache: list[str] = self._get_column_names() | ||
| self._iterrows = self._df_iter.iterrows() | ||
|
|
||
| def _read_kwargs(self, **defaults: Any) -> dict[str, Any]: |
There was a problem hiding this comment.
Self-review round one (expanded scope) — CLEAN
The maintainer asked to include the Arrow, S3FS, and Polars storage_options collisions, which expands the contract, so this is a full pass over the new commit d542edb21cef28aab2636523585efd1aee5df814..f904c4c709a32309e74c8c487ed09754c68cfddc (13 source/test files) and its callers; base 785be364e380a7088b82ecf3b5bcb3a5bf01b9ca.
Covered:
- Arrow (3) and S3FS (3) cursors:
kwargs.popreads the per-call dict (the executor-submitted dict for the concurrent.futures cursors), so nothing leaks into later calls; the result sets do not forward**kwargsto any read function, so no other key changes destination. AthenaPolarsResultSet._read_kwargs()(this line): all four read calls (read_csv,scan_csv,read_parquet,scan_parquet) now take{**defaults, **self._kwargs}; other per-call Polars arguments (e.g.n_rows) still reach the same functions. The UNLOAD schema read takes onlystorage_optionsfrom the per-call arguments, as before it took none.- The default storage options are still computed when replaced (one
get_credentials()call); no behavior depends on it. - A per-call
has_headerorschema_overridesreplaces the result set's choice wholesale (e.g. the converter dtypes); this is the agreed "execute() wins" contract and is stated in the docstrings. - Tests: the new cursor cases and the five
storage_optionscases fail ond542edb21cef28aab2636523585efd1aee5df814withTypeError;test_csv_read_kwargs_replace_defaultsreads a real local CSV through both CSV paths; the livetest_as_polars_with_read_kwargspassesschema_overridesand fsspecstorage_options(withskip_instance_cache) through a real Athena result.
| lazy_df = pl.scan_parquet( | ||
| self._unload_location, | ||
| storage_options=self._parquet_storage_options, | ||
| storage_options=self._kwargs.get("storage_options", self._parquet_storage_options), |
There was a problem hiding this comment.
Self-review round two (expanded scope) — FINDINGS (PR description only; no code change)
Claims checked at f904c4c709a32309e74c8c487ed09754c68cfddc:
- "Non-chunked CSV is read through fsspec, chunked CSV and UNLOAD through object store" (docstring): in the locked Polars 1.44.2,
polars/io/csv/functions.pyroutes S3 paths inread_csvthroughprepare_file_arg→fsspec.open(the object-store dispatch is only forhf://orPOLARS_FORCE_ASYNC), and the result set already usesscan_csv/scan_parquetwith object-store options. - Arrow/S3FS docstrings:
block_size,connect_timeout,request_timeout, andcsv_readerare explicitAthenaArrowResultSet/AthenaS3FSResultSetparameters. - Docs:
docs/describes none of these per-call arguments; no update needed.
Finding: the PR description and the commit message said storage_options replaces PyAthena's settings "as in the pandas cursors". That holds only for pandas CSV results (pyathena/pandas/result_set.py:577-589); pandas UNLOAD reads always pass filesystem=self._fs (pyathena/pandas/result_set.py:791-796). The PR description now says "as the pandas cursors do for CSV results"; the commit message is left as published. Out of scope and reported to the maintainer: a per-call filesystem in the pandas UNLOAD path collides the same way.
| self._column_names_cache: list[str] = self._get_column_names() | ||
| self._iterrows = self._df_iter.iterrows() | ||
|
|
||
| def _storage_options(self, default: Callable[[], dict[str, Any]]) -> Any: |
There was a problem hiding this comment.
Independent review (relayed), expanded scope: FINDINGS (one P2) → repaired in 0db626f
Reviewer: Codex CLI 0.160.0, model gpt-6-astra, reasoning effort high, sandbox read-only, session 01a10221-5efc-7122-88ef-235556a40864. Static review only. Snapshot: detached worktree at f904c4c709a32309e74c8c487ed09754c68cfddc; reviewed change d542edb21cef28aab2636523585efd1aee5df814..f904c4c709a32309e74c8c487ed09754c68cfddc (the contract expansion, so a full review of it). The prompt contained the literal diff and the intended behavior, without the PR number, description, or prior findings. Afterwards, the snapshot and the PR worktree were clean at f904c4c7.
Covered (reviewer): the six Arrow/S3FS cursor overrides (sync, futures, asyncio, defaults, error propagation); Polars eager/chunked CSV, Parquet, UNLOAD schema discovery, and argument forwarding; tests, docstrings, and the locked Polars 1.44.2 / fsspec 2026.9.0 / PyArrow 25.0.1 pins. The override tests fail on the previous head and assert read arguments or actual CSV values.
Finding: P2, pyathena/polars/result_set.py:700 (also 507, 524, 669 at f904c4c7). _parquet_storage_options was evaluated before a per-call storage_options replaced it, and it calls get_frozen_credentials(). A chunked UNLOAD read with valid explicit S3 credentials, started after the session credentials expire, would raise OperationalError when refreshing them fails, before Polars receives the replacement. The tests mocked the property with a successful return.
Minor: has_header replacement had no direct assertion.
Pre-existing (not changed, as recorded in the earlier independent review): normal pytest invocation runs the AWS session setup even for the mocked/local tests.
Author disposition: verified and repaired. _storage_options() (this method) calls the default only when execute() was not given storage_options, and all five read calls pass the default as a callable. test_storage_options_replace_defaults now makes both default properties raise; all 5 cases fail at f904c4c7 and pass at 0db626f1. test_csv_read_kwargs_replace_defaults now reads a header-less file with has_header=False. Offline: tests/pyathena/polars/test_result_set.py 9 passed; live: -k "as_polars or chunk" over the three Polars cursor files, 28 passed.
There was a problem hiding this comment.
Self-review of the repair (git range-diff: one new commit, f904c4c709a32309e74c8c487ed09754c68cfddc..0db626f1e32b2f8133c9bf54d8d044a3b3900730; both old objects present; base unchanged at 785be364e380a7088b82ecf3b5bcb3a5bf01b9ca) — CLEAN
Round one (behavior): the default is still computed inside each read's try, so a failing default raises the same OperationalError as before when no storage_options is given; for the chunk generators it is still computed on first iteration. A given storage_options=None counts as given (Polars then uses its own credential lookup), matching the pandas CSV rule "given storage_options, even None". Other per-call read arguments are merged exactly as before; storage_options is written last with the same value.
Round two (claims): the commit message and the PR description now state that replaced options are not computed; the test evidence in the PR description names the commit each run used. No docs mention these internals.
There was a problem hiding this comment.
Independent follow-up (relayed): CLEAN
Reviewer: Codex CLI 0.160.0, model gpt-6-astra, reasoning effort high, sandbox read-only, session 01a10228-c80d-7083-b414-b5e3a8de2204. Static review only. Snapshot: detached worktree at 0db626f1e32b2f8133c9bf54d8d044a3b3900730; reviewed f904c4c709a32309e74c8c487ed09754c68cfddc..0db626f1e32b2f8133c9bf54d8d044a3b3900730 with the finding and intended behavior, without prior conclusions. Afterwards, the snapshot and the PR worktree were clean at 0db626f1.
Covered (reviewer): all five Polars read sites including the UNLOAD schema scan (defaults evaluated only when storage_options is absent; an explicit None is kept); sync, futures, and asyncio callers; separator/has_header/schema_overrides replacement; default evaluation, reads, schema collection, and chunk consumption stay inside the OperationalError handlers; the five storage cases fail on the previous head, and the CSV test checks actual column names and values.
Reviewer notes, no action: the CSV replacement test passes on the previous head too (that behavior already worked there; it guards the replacement, not the repair), and the explicit-None case is verified from source without a dedicated test.
There was a problem hiding this comment.
Rebase record: 0db626f1e32b2f8133c9bf54d8d044a3b3900730 → 16a15eaf24c96930fc33b3e169f9d55cac4c9b5e, onto fe21250a (master had conflicting test additions). git range-diff 785be364..0db626f1 origin/master..16a15eaf: commits 1, 2, 4 unchanged; commit 3 differs only in tests/pyathena/arrow/test_cursor.py (kept #1023's new test_fetch_all_rows body) and tests/pyathena/polars/test_result_set.py (kept #1029's new TestPolarsDataFrameIterator after the new TestAthenaPolarsResultSet cases).
Upstream source changes checked against this PR's contracts: #1023 (AthenaArrowResultSet._convert_rows) and #1029 (PolarsDataFrameIterator.close()) do not touch the per-call or read arguments; the filesystem and SQLAlchemy changes are unrelated.
At the rebased head: just lint passed; offline 35 passed (24 cursor test_read_options + Polars result set incl. #1029's iterator test); live -k "fetch_all_rows or timeout or as_polars" over the Arrow and Polars cursor files, 19 passed.
0db626f to
16a15ea
Compare
| kwargs: dict[str, Any] = {"use_threads": True, **self._kwargs} | ||
| # Given storage_options, even None, pandas opens the files itself, | ||
| # as for CSV results. | ||
| if "filesystem" not in kwargs and "storage_options" not in kwargs: |
There was a problem hiding this comment.
Self-review round one (pandas UNLOAD filesystem/storage_options) — CLEAN
The maintainer asked to include the pandas UNLOAD filesystem collision and to apply the CSV rule to storage_options; this expands the contract, so this is a full pass over 16a15eaf24c96930fc33b3e169f9d55cac4c9b5e..24b56a95830cc0eb4eb142600f29093449194453 (4 source files, 2 test files).
Covered:
- Default path (no
filesystem/storage_optionsgiven): unchanged call — PyAthena's_fs, scheme-lessbucket/keypath,use_threads=True, then the per-call arguments (which can still overrideuse_threads). - Given
filesystem(fsspec or pyarrow): passed as is with the scheme-less path, which both kinds expect.filesystem=None: thes3://location, so pandas resolves the filesystem. - Given
storage_options(evenNone) withoutfilesystem: nofilesystemargument and thes3://location, the same rule as_read_csv()(pyathena/pandas/result_set.py"Given storage_options, even None"). - Both given: passed together; pandas' own validation then raises (fsspec:
ValueError, pyarrow FS:NotImplementedError), surfaced asOperationalErrorlike any read failure. - The manifest read and
_read_parquet_schema()keep PyAthena's filesystem; they read the same UNLOAD location the connection wrote to. - Tests: the four override cases of
test_read_parquet_filesystemfail at16a15eaf24c96930fc33b3e169f9d55cac4c9b5eand assert the exactread_parquetcall; the livetest_unload_with_filesystem_optionsreads a real UNLOAD result with each option, usingskip_instance_cacheso no filesystem is left in fsspec's cache.
| result_set_type_hints: Athena type signatures for complex-type columns, | ||
| keyed by column name (case-insensitive) or zero-based column index. | ||
| **kwargs: Additional arguments passed to pandas.read_csv/read_parquet. | ||
| A given ``storage_options``, even None, replaces PyAthena's S3 filesystem |
There was a problem hiding this comment.
Self-review round two (pandas UNLOAD) — FINDINGS (PR description only)
Claims checked at 24b56a95830cc0eb4eb142600f29093449194453:
- "A non-empty
storage_optionsraisedValueError" (commit message): reproduced with pandas 3.0.6 —read_parquet("bucket/unload/", engine="pyarrow", filesystem=<fsspec fs>, storage_options={"anon": True})raisesValueError: storage_options passed with buffer, or non-supported URL(pandas/io/parquet.py_get_path_or_handle). - Same call with
storage_options=None: no error, PyAthena's filesystem was used and the option ignored. - This docstring: CSV results already replace PyAthena's filesystem for a given
storage_options, evenNone(_read_csv()); UNLOAD now follows it; the manifest/schema statement matches_read_parquet_schema().
Finding: the per-call storage_options=None case is a behavior change for UNLOAD (ignored before; pandas now resolves the filesystem itself). It was missing from the PR description; added to the WHAT section and the release-note items.
| **kwargs: Additional arguments passed to pandas.read_csv/read_parquet. | ||
| A given ``storage_options``, even None, replaces PyAthena's S3 filesystem | ||
| for reading the result files, and so does ``filesystem`` for UNLOAD results. | ||
| The UNLOAD manifest is still read with the connection's S3 client, and the |
There was a problem hiding this comment.
Independent review (relayed), pandas UNLOAD change: FINDINGS (P2 pre-existing, P3 docstring) → P3 repaired in c8d4f2d
Reviewer: Codex CLI 0.160.0, model gpt-6-astra, reasoning effort high, sandbox read-only, session 01a1026a-68d1-75d0-8baf-c02e0494558b. Static review only. Snapshot: detached worktree at 24b56a95830cc0eb4eb142600f29093449194453; reviewed 16a15eaf24c96930fc33b3e169f9d55cac4c9b5e..24b56a95830cc0eb4eb142600f29093449194453 (the contract expansion, full review) with the intended behavior, without the PR number, description, or prior findings. Afterwards, the snapshot and the PR worktree were clean at 24b56a95.
Covered (reviewer): keyword forwarding and exception propagation in all three pandas cursors; default and overridden Parquet reads; CSV parity; manifest/schema reads; the locked pandas 3.0.6 and pyarrow 25.0.1 sources; both new test groups. No functional defect in the argument selection; the four non-default mocked cases and both live cases fail on the previous head, and the tests assert the pandas call boundary and the returned records.
Findings:
- P2, pre-existing:
tests/pyathena/pandas/test_result_set.py:93— run normally, the mocked test still passes through the AWS session hook intests/pyathena/conftest.py:15. Author disposition: not changed, as for the same finding recorded earlier on this PR — the tests make no AWS calls themselves, follow the existing pattern, and run offline with--noconftest. - P3: this docstring said the UNLOAD manifest is read with PyAthena's filesystem;
AthenaResultSet._read_data_manifest()(pyathena/result_set.py:777-778) reads it with the connection's S3 client. Author disposition: repaired (this line). The same wrong statement in the commit message of24b56a95stays as published; the PR description is corrected.
There was a problem hiding this comment.
Self-review of the repair (24b56a95830cc0eb4eb142600f29093449194453..c8d4f2d642bb5e4b6f360844d1ca6cb64d39bc4c, docstring only) — CLEAN
Round one: no code change; just lint (incl. ruff pydocstyle and mypy) passed at c8d4f2d6. Round two: "manifest ... connection's S3 client" matches _read_data_manifest() (self._client.get_object with the result set's retry config); "schema with PyAthena's filesystem" matches _read_parquet_schema() (ParquetDataset(..., filesystem=self._fs)). The PR description now states the same; no other prose in the PR or docs makes the manifest claim.
There was a problem hiding this comment.
Independent follow-up (relayed): CLEAN
Reviewer: Codex CLI 0.160.0, model gpt-6-astra, reasoning effort high, sandbox read-only, session 01a1026d-dab5-72f0-bd79-4e457c6cc409. Static review only. Snapshot: detached worktree at c8d4f2d642bb5e4b6f360844d1ca6cb64d39bc4c; reviewed 24b56a95830cc0eb4eb142600f29093449194453..c8d4f2d642bb5e4b6f360844d1ca6cb64d39bc4c with the finding. Result: the repaired docstring matches both implementations (manifest via the connection's S3 client, schema via PyAthena's filesystem); no other inaccuracies. The snapshot and the PR worktree were clean at c8d4f2d6 afterwards.
There was a problem hiding this comment.
Rebase record: c8d4f2d642bb5e4b6f360844d1ca6cb64d39bc4c → b88146b0778819b87aff1daab599e8191d5e9122, onto a73c6db4 (via 8446e872, where master's #1010 and #1031 tests conflicted with this PR's test additions).
git range-diff fe21250a..c8d4f2d6 origin/master..b88146b0: commits 2, 4, 5, 6 unchanged; commits 1 and 3 differ only in test files —test_fetch_all_rowsin the Arrow, pandas, and Polars cursor tests (kept master's new body and assertions, then this PR'stest_read_options), and in the S3FS cursor test both master'stest_fetch_all_rows_custom_converterand its imports and this PR'stest_read_optionsand imports are kept. The step from8446e872toa73c6db4applied without conflicts.- Upstream source changes checked against this PR's contracts: JSON-as-text conversion (Return Athena JSON values from SQLAlchemy JSON columns and the pandas, Arrow, and S3FS cursors #1010) in the Arrow/Polars/S3FS result sets and
AthenaResultSet._json_converters(), the Arrow CSVignore_empty_lineschange (Keep NULL rows of single-column CSV results in ArrowCursor #1031), and the filesystem lookup parameters (Send the lookup parameters of a file with its lookups #1024) do not touch the per-call arguments,storage_options,filesystem,csv_reader, or the timeouts. - At
20f85eb6(series on8446e872):just lintpassed; offline 48 passed; live 33 passed over the conflict areas (one pandasUserWarningfromtest_fetch_all_rows[default], reproduced on mastera73c6db4). Atb88146b0:just lintpassed; offline 18 passed.
AsyncPandasCursor now takes block_size, cache_type, and auto_optimize_chunksize in its constructor, like PandasCursor and AioPandasCursor. Before, these arguments fell into **kwargs, which BaseCursor ignores, so a value given to the constructor or through connect(cursor_kwargs=...) was silently lost. Values given to execute() now override the cursor's values in every pandas and Polars cursor. PandasCursor and AioPandasCursor raised TypeError for a per-call auto_optimize_chunksize, and the three Polars cursors raised TypeError for a per-call block_size, cache_type, max_workers, or chunksize, because the cursor value and the same key in **kwargs were both passed to the result set. Closes #938 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Inserting them before result_reuse_enable shifted the positions of the existing parameters for positional callers. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The Arrow cursors passed connect_timeout and request_timeout, and the S3FS cursors passed csv_reader, both from the cursor and from the execute() keyword arguments, so giving one to execute() raised TypeError. The cursor value is now the default and execute() overrides it, as in the pandas and Polars cursors. AthenaPolarsResultSet passed separator, has_header, schema_overrides, and storage_options to the Polars read functions together with the execute() keyword arguments, so giving one of them raised TypeError. The execute() arguments now replace the ones the result set chooses. A given storage_options replaces PyAthena's S3 settings as a whole, as in the pandas cursors, and also applies to the UNLOAD schema read. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The default storage options were evaluated before a storage_options given to execute() replaced them, so a replaced default still fetched the session credentials, and a failed credential refresh broke a read that had valid explicit options. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
AthenaPandasResultSet read UNLOAD results with pandas.read_parquet, always passing PyAthena's filesystem and a path without the scheme. A filesystem given to execute() therefore raised TypeError, and a given storage_options made pandas raise ValueError for the scheme-less path. A filesystem given to execute() now replaces PyAthena's. A given storage_options, even None, makes pandas open the s3:// location itself, as it already did for CSV results. The manifest and schema reads keep using PyAthena's filesystem. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…tring The manifest is read with the connection's S3 client, not with PyAthena's filesystem. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
c8d4f2d to
b88146b
Compare
1 participant
WHAT
AsyncPandasCursoracceptsblock_size,cache_type, andauto_optimize_chunksizein its constructor (appended afterresult_reuse_minutes, so positional callers are unaffected), stores them likePandasCursor, and uses them as defaults for the result sets it creates.max_workersstill sizes the cursor's thread pool there, so the S3 read worker count stays a per-callexecute()option.execute()override the cursor's values instead of raisingTypeError: got multiple values for keyword argument:PandasCursor/AioPandasCursor:auto_optimize_chunksize.PolarsCursor/AsyncPolarsCursor/AioPolarsCursor:block_size,cache_type,max_workers,chunksize.ArrowCursor/AsyncArrowCursor/AioArrowCursor:connect_timeout,request_timeout.S3FSCursor/AsyncS3FSCursor/AioS3FSCursor:csv_reader.AthenaPolarsResultSet: Polars read arguments given toexecute()replace the ones the result set chooses (separator,has_header,schema_overrides,storage_options); before, each of them raisedTypeError.A given
storage_optionsreplaces PyAthena's S3 settings as a whole, as the pandas cursors do for CSV results, and is also used for the UNLOAD schema read. PyAthena's own options (which fetch the session credentials) are then not computed.Non-chunked CSV results are read through fsspec (in the locked Polars 1.44.2,
polars.read_csvopens S3 paths withfsspec.open), and chunked CSV and UNLOAD results through Polars' native object store, so the valid keys depend on the read path; the docstring says so.AthenaPandasResultSetUNLOAD reads: afilesystemgiven toexecute()replaces PyAthena's (before:TypeError), and a givenstorage_options, evenNone, makes pandas open thes3://location itself, as it already did for CSV results (before: a non-empty value raisedValueError: storage_options passed with buffer, or non-supported URLfor the scheme-less path, andNonewas ignored).filesystem=Nonealso passes thes3://location. The manifest is still read with the connection's S3 client and the schema with PyAthena's filesystem (the commit message says both use the filesystem; the docstring is corrected).execute()docstrings list which keyword arguments override the cursor's values.Release-note items:
AsyncPandasCursor(block_size=..., cache_type=..., auto_optimize_chunksize=...), including throughconnect(cursor_kwargs=...), now takes effect; before, the values were silently ignored.TypeError.execute(storage_options=...)replaces PyAthena's storage options for that query.unload=True:execute(filesystem=...)andexecute(storage_options=...)now read the UNLOAD result; a per-callstorage_options=None, which was ignored, now lets pandas resolve the filesystem itself, as for CSV results.WHY
Closes #938.
The
TypeErrorfor per-call values in the other pandas, Polars, Arrow, and S3FS cursors, and in the Polars read arguments, has the same cause (a value passed both explicitly and through**kwargs) and is fixed here at the maintainer's request, withstorage_optionsreplacing PyAthena's settings as the maintainer chose (the commit message says "as in the pandas cursors"; that holds for pandas CSV results only, since pandas UNLOAD reads always pass PyAthena's filesystem).TEST
Head: b88146b, rebased onto a73c6db.
The two rebases (onto fe21250 and onto a73c6db) changed only test files, keeping both sides where master added tests next to this PR's;
git range-diffshows the source patches unchanged. Earlier commits are named where a run used them.At the head:
just lint: passed.uv run --env-file .env pytest --noconftest): the Arrowtest_read_optionscases and both result-set test files, 18 passed.At 20f85eb6 (the same series rebased onto 8446e87):
test_read_optionscases (pandas, Polars, Arrow, S3FS × sync/async/aio) and the Polars and pandas result-set tests.-n 1:-k "fetch_all_rows or as_polars_with_read_kwargs or unload_with_filesystem or timeout or reader or auto_optimize"over the Arrow, pandas, Polars, and S3FS cursor files and the AsyncPandasCursor file, 33 passed. The one pandasUserWarning(fromtest_fetch_all_rows[default]) also occurs on master.Earlier, per change:
TestAthenaPandasResultSet::test_read_parquet_filesystem(default,filesystem,filesystem=None,storage_options,storage_options=None; the four override cases fail at the previous commit); live-k unloadover the pandas cursor file 8 passed, includingtest_unload_with_filesystem_options(both options read a real UNLOAD result), and the default-path UNLOAD/CSV reads (-k "test_fetchone or test_fetchall or test_as_pandas or test_complex"over the pandas and aio pandas cursor files) 30 passed.test_storage_options_replace_defaultsmakes PyAthena's default options raise; all 5 cases fail at f904c4c. Live-k "as_polars or chunk"over the three Polars cursor files, 28 passed.-k "as_polars or timeout or reader"over the Polars, Arrow, and S3FS cursor files 28 passed, includingtest_as_polars_with_read_kwargs(schema_overridesandstorage_optionsper call); both aio Arrow/S3FS files 33 passed.-k chunkover the Polars cursor files 15 passed; the pandasauto_optimizetests, includingTestAsyncPandasCursor::test_auto_optimize_chunksize, passed.pyathena/changes reverted, every new per-call case fails:TypeError, a different mockedread_parquetcall (pandas UNLOADfilesystem=None/storage_options), orKeyError: 'block_size'(theAsyncPandasCursorconstructor case).Not run locally: the remaining cursor suites and the full
just test pyathenasuite; they run in CI once the PR is Ready.🤖 Generated with Claude Code