Repository navigation
Separate query and S3 read worker settings - #1103
Conversation
| config=connection.s3_config.merge(Config(**overrides)), | ||
| **connection._s3_client_kwargs, | ||
| ) | ||
| self._s3_resources.callback(client.close) |
There was a problem hiding this comment.
Self-review round one — CLEAN (static behavior and implementation review).
Base: 23a54e16ecfe7cf8f64f42193d92cbb44f86cf2f
Head: 77d75ee29a65d1b9dcb4dfe8b4d7cf96502e79c3
Covered the full 30-file diff: all nine pandas/Polars/Arrow cursor constructors and execute paths, shared validation, independent query and S3 settings, repeated overrides including Arrow None, result-set and filesystem routing, Arrow CSV/UNLOAD materialization, timeout-client ownership, stream cleanup, tests, and documentation. Traced existing pandas/Polars readers, connection client configuration, the query executor, and S3File range reads and executor shutdown. Positional argument order is preserved; new parameters are keyword-only. The shared client remains connection-owned; dedicated timeout clients close on success and construction/read failure. No actionable defect found.
Evidence: local formatting/lint passed; 102 targeted offline regression cases and 140 result-set cases passed. The new AWS cases have not yet run; this static verdict does not establish live AWS behavior.
There was a problem hiding this comment.
Repair review scope: base 23a54e16ecfe7cf8f64f42193d92cbb44f86cf2f, old head 77d75ee29a65d1b9dcb4dfe8b4d7cf96502e79c3, new published head dec21943781fef9299786a54c032c14b7c3fd426. Both old objects exist; range-diff confirms the original commit is unchanged and one repair commit is added. Covered all four repaired files and the affected cursor/client/result-set callers.
Round-one repair follow-up — CLEAN. The initial pass missed benchmark callers; CI and independent review identified two obsolete argument routes. Migrated pandas/Polars cursor defaults to s3_max_workers and removed thread pandas' obsolete execute override; query-pool max_workers and result-set max_workers remain correctly routed. The factory matrix and the real AsyncPandasCursor reader test exercise the migration. Protected Arrow timeout-client creation with the existing connection S3 lock; dedicated and lazy shared clients use the same lock, which is released before filesystem construction or reads. Ownership and failure cleanup remain intact. Formatting and lint passed; benchmark suite: 108 passed, 1 intentionally skipped packaging test; focused Arrow/shared-client regression checks: 11 passed. No live AWS results yet.
There was a problem hiding this comment.
Bounded test-structure repair review.
Base: 23a54e16ecfe7cf8f64f42193d92cbb44f86cf2f
Previously reviewed head: dec21943781fef9299786a54c032c14b7c3fd426
New published head: f9e57b55fdf50545618f2586683840b73c98df06
Both old objects exist. Range-diff confirms both earlier commits are unchanged; one commit changes tests/pyathena/test_util.py only.
Self-review round one follow-up — CLEAN. TestS3WorkerOptions had no corresponding implementation class or meaningful shared setup; it only grouped validation across nine cursor types. Converted its four tests to standalone functions and its two stateless helpers to private module functions. Read the full one-file repair diff and traced construction, exception assertions, mocked _execute checks, awaitable execution, and synchronous/asynchronous close in finally. Parameter decorators, test order, assertions, and resource cleanup retain their behavior. No production code is changed. Local formatting/lint passed; complete offline test_util.py: 116 passed. Before/after collection confirms all 73 S3-worker parameter-case IDs and their order are identical after excluding the intentionally removed class qualifier.
There was a problem hiding this comment.
Round-one bounded follow-up after test organization and rebase: CLEAN.
Old merge-base: 23a54e1; previously reviewed implementation head: dec2194; test-only follow-up head: f9e57b5. Current merge-base: f3532e9; published head: 2e86370.
Verified the patch-series comparison: the first three commits retain their patches; the additional repair removes five redundant keyword-only separators introduced by combining the earlier worker patch with the new master signatures. Inspected upstream changes to all nine constructors/execute methods, BaseCursor, ExecuteOptions.resolve, util.strtobool, and their tests. All cursor options remain keyword-only as required by current master. The standalone functions preserve all 73 S3 validation cases, their parameter IDs/order, early rejection before _execute, awaitable handling, and close-in-finally; only the class-qualified pytest node IDs change. There is no corresponding S3WorkerOptions object or shared setup to justify a test class.
Current-head validation: just format and just lint passed; full offline test_util.py + test_options.py: 172 passed; keyword-only signatures and Arrow S3 adapter/ownership checks: 57 passed; explicit nine-cursor test_read_options nodes: 21 passed; just benchmark test: 108 passed, 1 packaging test intentionally skipped. An initial broad offline selector also selected five AWS fixture cases; those setup errors were resolved by selecting the intended offline nodes, without changing any tests. AWS CI and the required independent review are still pending. Static author review only.
There was a problem hiding this comment.
Round-one follow-up for independent finding F1: CLEAN.
Merge-base: f3532e9fb57fd769441e533f0b8375fd854ece69; old reviewed head: 2e863704bc638bc264abb53cfb7a6eb87d4addbb; new head: 8f1d1afc2db345bb56ac31b6c248001cd8afa973.
Verified F1 with AST parsing: five cursor files failed in each of the first three rebased commits. Folded the redundant-separator repair into the introducing implementation commit. Inspected the complete range-diff: only those five deletions move into the first commit; benchmark/locking and standalone-test patches are unchanged. All five affected cursor files now parse at every one of the three intermediate/current revisions (15 checks). The final tracked tree is exactly identical to the tested old head, tree a4a64445061620dba5e10d66ad0ebf6c319402f3, and the worktree is clean. Resource routing, validation, parameter sets/IDs/order, async behavior, and keyword-only public signatures are unchanged. Source tests were not repeated for a tree-identical history rewrite; current-head CI and the bounded independent follow-up remain required.
| | `ArrowCursor`, `AioArrowCursor` | No query pool | `None` (native PyArrow S3 filesystem) | | ||
| | `AsyncArrowCursor` | Query task pool | `None` (native PyArrow S3 filesystem) | | ||
|
|
||
| Pass `s3_max_workers` to `execute()` to override the setting for one query. |
There was a problem hiding this comment.
Self-review round two — CLEAN (static claims, compatibility, and operational review).
Base: 23a54e16ecfe7cf8f64f42193d92cbb44f86cf2f
Head: 77d75ee29a65d1b9dcb4dfe8b4d7cf96502e79c3
Audited the PR body, changed documentation/docstrings, commit claim, and full 30-file inventory. Checked constructor defaults and positional compatibility, former S3-keyword rejection before submission, query-specific override isolation, Arrow's native default and explicit None, unchanged result-set/filesystem/upload names, per-file worker limits, Polars native Parquet routing, and query submission preceding executor queueing. Related docs and SQLAlchemy cursor selection do not introduce conflicting argument routing. No actionable finding or repair.
Traced connection sessions, filtered S3 client kwargs, botocore Config.merge, PyArrow FSSpecHandler and eager dataset reads, S3Core retry ownership, and S3File executor shutdown. Explicit timeouts preserve other connection configuration without mutation and owned-client cleanup is covered on success and failure. This change makes no AWS throughput, account-wide concurrency, or elapsed-time guarantee.
Evidence boundaries: 102 offline regression cases and 140 result-set cases passed locally. A fresh worktree Sphinx build has the same 214 diagnostics as the exact base. AWS CSV/UNLOAD and full suite validation remain pending CI; core-module changes select PyAthena (including Spark/SQLAlchemy), SQLAlchemy compliance, and async compliance suites.
There was a problem hiding this comment.
Repair review scope: base 23a54e16ecfe7cf8f64f42193d92cbb44f86cf2f, old head 77d75ee29a65d1b9dcb4dfe8b4d7cf96502e79c3, new published head dec21943781fef9299786a54c032c14b7c3fd426. Both old objects exist; range-diff confirms the original commit is unchanged and one repair commit is added. Covered all four repaired files and the affected cursor/client/result-set callers.
Round-two repair follow-up — CLEAN. Checked the repair's claims against public cursor defaults, the benchmark matrix, result-set routing, Session.client concurrency, botocore configuration merge, and client lifetime. Benchmark executor_workers now explicitly supplies both pandas/Polars cursor pools while Arrow keeps its native default. The S3 creation lock serializes the new owned clients with this connection's lazy shared S3 client; this is not a global/session-wide concurrency guarantee across different connection objects. The concurrent regression uses four workers and checks ownership after creation. Existing doc/PR claims about native defaults, per-query overrides, timeout semantics, and per-file limits still hold. The initial benchmark CI failure is repaired rather than retried unchanged. Static follow-up only; AWS suites and independent repair review remain pending.
There was a problem hiding this comment.
Final runtime validation: current published/reviewed head dec21943781fef9299786a54c032c14b7c3fd426; AWS Test run 37343344427 completed successfully on Python 3.14. PyAthena: 3603 passed / 12 skipped. SQLAlchemy compliance: 589 passed / 759 skipped. Async compliance: 589 passed / 759 skipped. The workflow selected the full PyAthena directory without Spark/SQLAlchemy exclusions; the six new Arrow CSV/UNLOAD cases have no skip marks or skipping fixtures and are included in that suite. CI emits aggregate results rather than per-test reports. No AWS retries or workflow changes were needed for this PR.
Current-head benchmark, source lint, docs lint/build, and license checks also pass. Earlier Draft AWS placeholder jobs are intentionally skipped and superseded by the successful Ready jobs. PR is Ready, mergeable with CLEAN merge state, and the worktree remains clean at the reviewed head. These runtime results are author validation; both independent reviews remain static. No remaining work is required for PR delivery.
There was a problem hiding this comment.
Bounded test-structure repair review.
Base: 23a54e16ecfe7cf8f64f42193d92cbb44f86cf2f
Previously reviewed head: dec21943781fef9299786a54c032c14b7c3fd426
New published head: f9e57b55fdf50545618f2586683840b73c98df06
Both old objects exist. Range-diff confirms both earlier commits are unchanged; one commit changes tests/pyathena/test_util.py only.
Self-review round two follow-up — CLEAN. Checked the claims of unchanged coverage and grouping against AGENTS.md, all four parameter sets, helper call sites, pytest collection, and existing test selectors. The existing TestRetryConfig corresponds to RetryConfig; this removed group represented no one implementation class. There are no remaining references to TestS3WorkerOptions in the tracked source. The 73 case IDs/order are preserved; full pytest node IDs necessarily lose the class qualifier. Async pytest discovery and awaitable cleanup are retained, and all 116 offline utility tests pass. New-head independent repair review and CI remain pending; the earlier successful AWS run belongs to dec2194, not this new commit.
There was a problem hiding this comment.
Round-two bounded follow-up after test organization and rebase: CLEAN.
Old merge-base: 23a54e1; previously reviewed implementation head: dec2194; test-only follow-up head: f9e57b5. Current merge-base: f3532e9; published head: 2e86370.
Audited the claims that test grouping changes no validation behavior, s3_max_workers stays separate from thread-pool max_workers, per-query overrides preserve defaults, Arrow None retains the native path, and the rebased constructor/execute signatures retain master's keyword-only contract. The patch-series comparison and exact constructor/execute callers support these claims; the new ExecuteOptions.resolve fast path still receives shared execution fields only and leaves S3 reader kwargs in their existing routing path. Upstream bool parsing and its additional utility cases are preserved. No service-request, retry, client ownership, or concurrency behavior is changed by this follow-up.
The 73 parameter IDs/order remain, while pytest node IDs intentionally lose the class qualifier; no promise of identical full node IDs is made. Current-head offline coverage is 172 utility/options, 57 signature/Arrow, 21 routing cases, and 108 benchmark tests (1 packaging skip). Prior AWS results belong to the previous revision and are not evidence for this rebase. Updated CI is dispatched while Draft; the requested Claude Opus 5.5/Max/high review will be retried after the weekly limit resets. This is the author's second perspective, not independent review.
There was a problem hiding this comment.
Current-head runtime evidence for the rebase follow-up: published head 2e863704bc638bc264abb53cfb7a6eb87d4addbb, merge-base f3532e9fb57fd769441e533f0b8375fd854ece69.
AWS Test run 37944787630, dispatched while Draft on Python 3.14, completed successfully. PyAthena: 3739 passed / 12 skipped; synchronous SQLAlchemy compliance: 589 passed / 759 skipped; asynchronous SQLAlchemy compliance: 589 passed / 759 skipped. The lint and changes jobs also passed. The PR's docs lint/build, license check, benchmark tooling, and Draft lint checks passed. The Draft PR run intentionally skipped its AWS placeholder jobs; the dispatch ran all three expected suites. No other active AWS run was present when dispatched.
The required Claude Opus 5.5 / first-party Max / high follow-up is waiting for the weekly quota reset. The PR stays Draft until its actual review result is collected; these test results do not substitute for independent review.
There was a problem hiding this comment.
Round-two follow-up for independent finding F1: CLEAN.
Merge-base: f3532e9fb57fd769441e533f0b8375fd854ece69; old reviewed head: 2e863704bc638bc264abb53cfb7a6eb87d4addbb; new head: 8f1d1afc2db345bb56ac31b6c248001cd8afa973.
Audited the claims that the repair removes the intermediate duplicate-separator SyntaxError and changes no final runtime behavior or documented API. Per-commit AST checks verify the five constructor files at all three revisions; exact git tree identity verifies the final source/docs/tests are unchanged. The claim is about constructor syntax only; no assertion is made that every intermediate commit passes every test. Existing 73 validation cases, keyword-only constructor/execute contracts, worker defaults/overrides, and benchmark routing remain as reviewed. Prior full runtime results still belong to old SHA 2e863704bc638bc264abb53cfb7a6eb87d4addbb; current-head offline and Ready-triggered CI will be checked before delivery. This is the author's second perspective, not independent review.
There was a problem hiding this comment.
Final runtime validation and delivery: CLEAN.
Published/reviewed head: 8f1d1afc2db345bb56ac31b6c248001cd8afa973; merge-base: f3532e9fb57fd769441e533f0b8375fd854ece69.
Ready-triggered Test run 37951790920 completed successfully on Python 3.14: PyAthena 3739 passed / 12 skipped; SQLAlchemy synchronous compliance 589 passed / 759 skipped; asynchronous compliance 589 passed / 759 skipped. All three expected AWS suites, lint, and path selection passed. Final-head docs lint/build, license, and benchmark tooling checks also passed. The earlier run 37950221556 was intentionally superseded via Draft to serialize the newly overlapping shared-AWS workflow; the successful final run above began after that other workflow finished. The Draft placeholder jobs are intentionally skipped and do not replace the three completed suites.
The PR is Ready, OPEN, MERGEABLE, with CLEAN merge state; the published head matches the reviewed local head. The dedicated and main worktrees are clean. Both author review rounds and independent Opus 5.5/Max/high repair follow-ups are complete, including the resolved F1 thread. The current revision's runtime results are distinct from the read-only reviewer conclusions. No merge was performed.
| overrides["connect_timeout"] = self._connect_timeout | ||
| if self._request_timeout is not None: | ||
| overrides["read_timeout"] = self._request_timeout | ||
| with connection._s3_client_lock: |
There was a problem hiding this comment.
Relayed independent repair review — CLEAN (static).
Reviewer: Claude Code; requested and verified claude-opus-5-5, profile max, effort high; first-party Claude Max, not Enterprise. Authentication was rechecked with API key/token/provider overrides removed.
Session: b295234f-5bd8-4d9c-a178-0864072a9340.
Old/new base: 23a54e16ecfe7cf8f64f42193d92cbb44f86cf2f
Old reviewed head: 77d75ee29a65d1b9dcb4dfe8b4d7cf96502e79c3
New reviewed/published head: dec21943781fef9299786a54c032c14b7c3fd426
Read the full four-file repair diff and patch-series comparison, with API context from the full diff. Traced all pandas/Polars constructor/execute routes, independent pools, benchmark factory/matrix/reader tests and README, unchanged result-set arguments, Arrow native defaults, connection locking/configuration, owned/shared client lifetime, thread-pool and aio callers, and the concurrent regression. No introduced defect found. The benchmark routing is corrected and the S3 lock is released before reads without recursive acquisition.
Invocation retained Read/Grep/Glob only, with safe/restricted mode and hooks/skills/MCP/memory/commands/network/GitHub disabled. Snapshot hash manifest is unchanged, and the PR worktree remains clean at the reviewed head.
Reviewer notes (not defects): the existing connection comment describes shared-client lazy initialization, rather than the additional lock use; the regression's locked() assertion does not prove which thread owns a lock. Author assessment: the shared-client comment remains accurate, and the test detects omission of the dedicated-client guard while source inspection establishes mutual exclusion. No repair is required for these notes. The reviewer also identified existing Session factories using other/no locks (Glue/to_sql), predating this PR; those are outside this change and no session-wide guarantee is claimed.
Limits: static inspection only; no tests, builds, type checks, benchmarks, or live AWS execution by the reviewer; thread interleavings and dependency runtime behavior were not measured. The rest of the original diff was not re-reviewed in this bounded follow-up; its completed initial review is recorded above. Author local checks and current-head offline CI pass; AWS CI remains pending serialized execution.
f9e57b5 to
2e86370
Compare
2e86370 to
8f1d1af
Compare
| @pytest.mark.parametrize( | ||
| ("value", "error"), [(0, ValueError), (-1, ValueError), (True, TypeError), (1.5, TypeError)] | ||
| ) | ||
| def test_s3_workers_invalid_constructor(cursor_class, value, error): |
There was a problem hiding this comment.
Relayed independent static history-repair follow-up: CLEAN (no defects introduced by this bounded follow-up).
Reviewer: Claude Code, requested and verified claude-opus-5-5, max profile, effort high; first-party claude.ai Max authentication, not Enterprise, with provider/API overrides removed. Session: 9d7f8e6a-7c96-4b2d-9e02-7fb6bc2a4356. Merge-base: f3532e9fb57fd769441e533f0b8375fd854ece69; old reviewed head: 2e863704bc638bc264abb53cfb7a6eb87d4addbb; published head: 8f1d1afc2db345bb56ac31b6c248001cd8afa973.
The reviewer inspected the complete patch-series comparison, literal empty final-tree diff, all three intermediate patches, and exact intermediate snapshots of the five constructor files. Removing the second * in the introducing commit fixes F1 in every intermediate revision and preserves keyword-only S3 options. Benchmark/locking and standalone-test patches are unchanged; the current code/tests/docs remain identical to the already reviewed tree a4a64445061620dba5e10d66ad0ebf6c319402f3. Coverage also included all nine current constructors, result-set/filesystem separators, validation/routing/default overrides, the standalone tests, shared client creation lock, and benchmark callers. The preceding independent review covered the full test-organization/signature repair and relevant upstream contracts.
The reviewer also noted the existing intermediate benchmark adapter incompatibility after the implementation commit, already repaired by the preserved second commit. This is not introduced by the history follow-up and is absent at the final head. No claim is made that every intermediate commit passes every test. One factual aside in the reviewer output was incorrect: CI runs just benchmark test at .github/workflows/benchmarks.yaml:42, in addition to the packaging test at line 48; current-head benchmark CI passed. The author verified both points against the intermediate patch and actual workflow.
Static review only: Read/Grep/Glob; no edits, execution, lint/tests/builds, network, GitHub writes, memory, credentials, or delegation. Exact intermediate snapshots were supplied for the five repaired files; other intermediate contents were traced through commit patches. The old intermediate states were inferred from comparison evidence; rebase effects were outside this unchanged-base follow-up. The review snapshot and PR worktree remained unchanged. The author separately verified all 15 intermediate-file AST checks and exact final tree identity. Current-head offline CI and Ready-triggered AWS CI remain separate runtime evidence.
Arrow cursors keep the native PyArrow S3 filesystem without an s3_max_workers option, so the dedicated S3 client, the S3FileSystem client argument, and their tests are removed. Former keyword checks and value validation before query submission are removed; unknown cursor keywords are handled by #1093, and reader options by the readers. The read option tests keep the renamed argument and the independent thread pool size. The 4.0 migration notes move to usage.md. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| block_size=kwargs.pop("block_size", self._block_size), | ||
| cache_type=kwargs.pop("cache_type", self._cache_type), | ||
| max_workers=kwargs.pop("max_workers", self._max_workers), | ||
| max_workers=kwargs.pop("s3_max_workers", self._s3_max_workers), |
There was a problem hiding this comment.
Self-review round one after scope reduction — FINDINGS (one claim correction, no code repair).
Base: f3532e9fb57fd769441e533f0b8375fd854ece69
Head: c120e3011a9d20dbf8f12e27765df0794da39973
Prior reviewed head: 8f1d1afc2db345bb56ac31b6c248001cd8afa973. The new commit removes the Arrow s3_max_workers path (dedicated S3 client, S3FileSystem s3_client argument, PyArrow fsspec adapter) and _validate_s3_max_workers, so this is a full pass over the 17-file diff rather than a range-diff follow-up.
Covered: constructor and execute() routing in all six pandas/Polars cursors (sync, thread-pool, native asyncio); AsyncPolarsCursor no longer forwards its pool size; AsyncPandasCursor's new constructor default equals the result-set default it used before; Arrow cursors identical to base except the AsyncArrowCursor docstring; benchmark adapter/tests; the six test_read_options changes. The thread-pool cases construct with max_workers=1, s3_max_workers=2 and assert both values, which fails against base AsyncPolarsCursor.
Finding (fixed in the PR body): the body said a former max_workers in execute() is rejected by the reader. An offline probe with mocked _execute/_poll showed all six cursors raise TypeError: ... got multiple values for keyword argument 'max_workers' when the result set (or asyncio.to_thread) is called, i.e. after the query has run. It never silently changes behavior, so no pre-check is added; the body now states this. Constructors ignore the former name until #1102 rejects unknown keywords.
Evidence: just format, just lint, just docs lint passed; 18 test_read_options cases across the nine cursors passed; just benchmark test 108 passed / 1 packaging skip. No AWS run for this head yet.
There was a problem hiding this comment.
Round-one follow-up for the independent documentation finding — CLEAN. Old head c120e3011a9d20dbf8f12e27765df0794da39973, new head f316843f84e1dc839e1dd243e1ca3fa76b188c18, same base. The repair changes two lines of docs/usage.md only. Traced each Polars read path in pyathena/polars/result_set.py: _read_csv (non-chunked) passes _csv_storage_options with max_workers; _iter_csv_chunks (scan_csv) and the Parquet paths (read_parquet/scan_parquet) use _parquet_storage_options without it. No code changed; just docs lint passed.
There was a problem hiding this comment.
Round-one follow-up after merging master — CLEAN after one documentation repair.
Old base/head f3532e9fb57fd769441e533f0b8375fd854ece69/f316843f84e1dc839e1dd243e1ca3fa76b188c18; new merge-base dafee9a4264ae2bb765cc7c5e1e3ef842bd6cfb8 (includes #1102 and #1107); merge commit 02331086a17e04a60895e792941f2aa58927b512; head 9df509eaa4a8432db13e62837d4613948352be76.
The merge conflicted only in docs/usage.md (#1102's unknown-keyword paragraph and this PR's worker section), resolved by keeping both. Code merged without conflicts. Checked upstream contracts touching the rename: BaseCursor without **kwargs, constructor signature caching, Connection._internal_cursor cursor_kwargs filtering, pandas/Polars execute() reader forwarding, SQLAlchemy dialect cursor options, and fixtures. None names the S3 max_workers or mishandles s3_max_workers.
Probe at the merge commit (mocked _execute/_poll): the four renamed constructors raise TypeError for max_workers; execute(max_workers=2) raises duplicate-keyword TypeError after _execute in all six cursors. I added this to docs/usage.md; independent review found the thread-pool wording imprecise, and 9df509eaa4a8432db13e62837d4613948352be76 fixes it.
Validation: just lint, just docs lint; 18 test_read_options; 218 connection/options tests; just benchmark test 108 passed / 1 skip.
| query_id, future = cursor.execute("SELECT 1", s3_max_workers=3) | ||
| ``` | ||
|
|
||
| Before PyAthena 4.0, `max_workers` set the S3 read workers of `PandasCursor`, `PolarsCursor`, `AioPandasCursor`, and `AioPolarsCursor`, and of `execute()` in the pandas and Polars cursors. |
There was a problem hiding this comment.
Self-review round two after scope reduction — CLEAN after the PR-body correction recorded in round one.
Base: f3532e9fb57fd769441e533f0b8375fd854ece69
Head: c120e3011a9d20dbf8f12e27765df0794da39973
Claims checked: (1) s3_max_workers default equals the former default — (cpu_count() or 1) * 5 in the pandas/Polars cursors, the result sets, and AsyncCursor.max_workers (the value AsyncPolarsCursor used to forward). (2) AsyncPolarsCursor used max_workers for both pools — base pyathena/polars/async_cursor.py passed self._max_workers to the result set. (3) Polars UNLOAD does not use the setting — _read_parquet/scan_parquet use _parquet_storage_options (native object_store); only _csv_storage_options passes max_workers. (4) The usage example — Connection.cursor(cursor, **kwargs) forwards both arguments. (5) Arrow's native PyArrow S3FileSystem has no per-file worker option. (6) No other doc uses the renamed pandas/Polars argument; remaining max_workers examples are thread-pool cursors or to_sql.
Correction left in history: commit c120e301's message says reader options are rejected by the readers; the former execute() keyword actually fails as a duplicate result-set argument. The PR body is accurate; the commit was not rewritten.
Limits: docs example and AWS paths are not run locally; just docs build shows only the existing warnings. AWS suites run after Ready.
There was a problem hiding this comment.
Round-two follow-up for the documentation repair — CLEAN. Old head c120e3011a9d20dbf8f12e27765df0794da39973, new head f316843f84e1dc839e1dd243e1ca3fa76b188c18. Claim checked: "Polars uses it only for CSV results read without chunksize; chunked CSV reads and UNLOAD results use Polars' native readers". Supported by pyathena/polars/result_set.py:577 (_csv_storage_options) versus :637, :654, :779, and :818 (_parquet_storage_options). Results without an output file are read from GetQueryResults bytes and use no S3 reader, which the wording does not contradict. No other prose changed.
There was a problem hiding this comment.
Final runtime validation: head f316843f84e1dc839e1dd243e1ca3fa76b188c18. Ready-triggered Test run 37959031361 succeeded on Python 3.14: PyAthena 3648 passed / 12 skipped; SQLAlchemy sync compliance 589 passed / 759 skipped; async compliance 589 passed / 759 skipped; lint and changes passed. The skipped test/test-sqla/test-sqla-async entries belong to the earlier Draft run 37957549120. It started after the #1102 run 37957475564 completed, with no other Test run active. PR is Ready, MERGEABLE, CLEAN. Not merged.
There was a problem hiding this comment.
Round-two follow-up after merging master — CLEAN.
Merge-base dafee9a4264ae2bb765cc7c5e1e3ef842bd6cfb8, head 9df509eaa4a8432db13e62837d4613948352be76.
Claims: (1) "Passing max_workers to those four constructors raises TypeError" — the probe shows BaseCursor.__init__() got an unexpected keyword argument 'max_workers' for PandasCursor, PolarsCursor, AioPandasCursor, AioPolarsCursor. (2) "Passing it to execute() raises TypeError after the query has run; with AsyncPandasCursor and AsyncPolarsCursor, the returned future raises it" — the probe shows _execute called before the duplicate-keyword error in all six; the thread-pool cursors collect results in _collect_result_set (pandas/async_cursor.py:190, polars/async_cursor.py:192). (3) #1102's general statement that pandas/Polars reader options fail as OperationalError still holds for other names; max_workers is the documented exception. The PR body states the post-#1102 behavior. AWS CI for the merge commit was triggered on push; results to follow.
There was a problem hiding this comment.
Final runtime validation after the master merge: head 9df509eaa4a8432db13e62837d4613948352be76. Test run 38020038709 succeeded on Python 3.14: PyAthena 3740 passed / 12 skipped; SQLAlchemy sync compliance 589 passed / 759 skipped; async compliance 589 passed / 759 skipped; lint and changes passed. Run 38019766760 for merge commit 02331086 was cancelled when the wording repair was pushed. PR is Ready, MERGEABLE, CLEAN. Not merged.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| The pandas and Polars cursors take the number of threads that read each result file from S3 as `s3_max_workers`. | ||
| It can be set in the cursor constructor and overridden for one query in `execute()`. | ||
| The default is `(cpu_count() or 1) * 5`. | ||
| Polars uses it only for CSV results read without `chunksize`. |
There was a problem hiding this comment.
Relayed independent static review — FINDINGS (one Low documentation finding), repaired; follow-up CLEAN.
Reviewer: Codex CLI 0.161.0, model gpt-6.1-sol (a different model from the Claude author of this revision), codex exec -s read-only in a detached clean snapshot. No edits, builds, tests, network, or GitHub access; the prompt omitted the PR number, body, commits, and prior findings.
Initial review — session 01a1216d-29c9-7751-a8c5-e1630b9bb34f, base f3532e9fb57fd769441e533f0b8375fd854ece69, head c120e3011a9d20dbf8f12e27765df0794da39973. Covered all six pandas/Polars cursors (constructor settings, defaults, routing, per-query overrides), thread-pool independence, unchanged result-set/filesystem names and Arrow docstring, all repository callers (benchmarks, tests, docs, README, connection factories, sync/async SQLAlchemy dialects), regression coverage, docstrings, and simplicity. No implementation defect; it noted the tests use mocked result sets and do not test overlapping overrides.
Finding (P3): docs/usage.md said only UNLOAD bypasses s3_max_workers for Polars, but chunked CSV reads also do — pyathena/polars/result_set.py:778 passes _parquet_storage_options (native object_store, no worker value) to pl.scan_csv. Verified by the author.
An earlier delegated Codex run (thread 01a1216c-1a5d-7d02-8b74-810ca7993e7f) ended with a placeholder message and no review; it is not counted.
Repair f316843f84e1dc839e1dd243e1ca3fa76b188c18: the docs now say Polars uses the setting only for CSV results read without chunksize, and chunked CSV and UNLOAD reads use Polars' native readers.
Follow-up — session 01a12170-fac9-7bb0-88a7-61d533338321, diff c120e3011a9d20dbf8f12e27765df0794da39973..f316843f84e1dc839e1dd243e1ca3fa76b188c18: CLEAN. Verified non-chunked CSV, chunked CSV, non-chunked and chunked UNLOAD Parquet, and results without an output file; user storage_options replace the defaults, so "only for" is not overstated. The review snapshot remained clean. Static review only.
…s3-workers # Conflicts: # docs/usage.md
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
|
||
| Before PyAthena 4.0, `max_workers` set the S3 read workers of `PandasCursor`, `PolarsCursor`, `AioPandasCursor`, and `AioPolarsCursor`, and of `execute()` in the pandas and Polars cursors. | ||
| Replace it with `s3_max_workers` in those places. | ||
| Passing `max_workers` to those four constructors raises `TypeError`. |
There was a problem hiding this comment.
Relayed independent static follow-up for the master merge — FINDINGS (one Low doc wording), repaired; follow-up CLEAN.
Reviewer: Codex CLI 0.161.0, model gpt-6.1-sol, codex exec -s read-only, detached clean snapshot; no edits, builds, tests, network, or GitHub.
Merge review — session 01a123cd-4816-73d2-ac90-455f4d88b4a8, merge-base dafee9a4264ae2bb765cc7c5e1e3ef842bd6cfb8, head 02331086a17e04a60895e792941f2aa58927b512. Covered the conflict resolution, the full branch diff against master, all six pandas/Polars cursors, base keyword validation, signature inspection, public/internal connection option routing, sync/async SQLAlchemy dialects and reflection adapters, fixtures, tests, benchmarks, and worker docs. No code-level merge regression or stale S3 max_workers caller found.
Finding (P3): docs/usage.md:68 said max_workers raises in execute(); for AsyncPandasCursor/AsyncPolarsCursor execute() returns normally and future.result() raises (pandas/async_cursor.py:190, polars/async_cursor.py:192). Verified.
Repair 9df509eaa4a8432db13e62837d4613948352be76: the docs say the returned future raises it for those two cursors.
Follow-up — session 01a123d0-e10e-76d2-ad9c-92e1790f58db, diff 02331086a17e04a60895e792941f2aa58927b512..9df509eaa4a8432db13e62837d4613948352be76: CLEAN for all six cursors' constructors and execute()/future paths. Snapshot remained clean. Static review only.
1 participant
WHAT
Separate the two meanings of
max_workersin the pandas and Polars cursors.PandasCursor,PolarsCursor,AioPandasCursor, andAioPolarsCursor: the S3 read worker argument is renamed frommax_workerstos3_max_workers, in the constructor and inexecute().AsyncPandasCursorandAsyncPolarsCursor:max_workerssizes only the cursor's thread pool;s3_max_workerssets the S3 read workers in the constructor and per query inexecute().AsyncPolarsCursorno longer passes its thread pool size to the S3 readers, andAsyncPandasCursorgains a constructor default for the S3 read workers.s3_max_workersdefault stays(cpu_count() or 1) * 5, the former default.AsyncArrowCursormax_workersdocstring; they keep the native PyArrow S3 filesystem, which has no per-file worker setting.max_workersarguments keep their names and meanings.docs/usage.mddocuments the settings and the migration next to the other PyAthena 4.0 keyword changes; the benchmark adapters use the new argument.Breaking change for the 4.0.0 release note: replace
max_workerswiths3_max_workersin the pandas/Polars cursors named above and in theirexecute()calls; pass both arguments toAsyncPolarsCursorto keep its former values.The former name is not checked separately. With #1102 merged, it raises
TypeErroras an unknown keyword in the four renamed constructors. Inexecute()of all six pandas/Polars cursors it raisesTypeError(got multiple values for keyword argument 'max_workers') when the result set is created, after the query has run; docs/usage.md states both.WHY
Closes #1094.
max_workersset the query thread pool inAsyncPandasCursor, the S3 read workers inPandasCursor/PolarsCursor, and both inAsyncPolarsCursor.An earlier revision also added
s3_max_workersto the Arrow cursors, switching them to PyAthena's S3 filesystem through PyArrow's fsspec adapter when set.That was a new read path rather than a rename, so it was removed from this PR.
TEST
Tested commit:
02331086a17e04a60895e792941f2aa58927b512(merge of masterdafee9a4264ae2bb765cc7c5e1e3ef842bd6cfb8, which includes #1102, intof316843f). The merge conflicted only indocs/usage.md, resolved by keeping both sections.just format,just lint,just docs lint: passed.just docs buildshows only existing warnings.uv run --env-file .env pytest -p no:rerunfailures -q -n 1 -k test_read_optionsover the pandas, Polars, and Arrow cursor test files (sync, thread-pool, and native asyncio): 18 passed.The thread-pool pandas/Polars cases construct the cursor with
max_workers=1ands3_max_workers=2and check that the pool and the reader each receive their own value.just benchmark test: 108 passed, 1 skipped (packaging)._execute/_poll: the four renamed constructors raiseTypeErrorformax_workers, andexecute("SELECT 1", max_workers=2)on all six pandas/Polars cursors raisesTypeErrorfor duplicatemax_workersafter_executeis called.tests/pyathena/test_connection.py,tests/pyathena/aio/test_connection.py,tests/pyathena/test_options.py).9df509eaadds a two-line docs/usage.md wording repair after the merge (just docs lintpassed). Ready-triggered Test run 38020038709 at9df509ea, Python 3.14: PyAthena 3740 passed / 12 skipped; SQLAlchemy sync and async compliance each 589 passed / 759 skipped (run 38019766760 for the merge commit was superseded).🤖 Generated with Claude Code