Separate query and S3 read worker settings - #1103
laughingman7743 wants to merge 2 commits into
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.
| | `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.
| 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.
This branch has not been deployed
1 participant
WHAT
Keep
max_workersfor the thread-pool cursor's query-result tasks and introduces3_max_workersfor S3 reads across synchronous, thread-pool, and native asyncio pandas, Polars, and Arrow cursors.Constructor defaults and per-query overrides are independent; overrides do not change the next query's default.
Arrow keeps its native PyArrow S3 filesystem by default.
A positive
s3_max_workersselects PyAthena's S3 filesystem through PyArrow's fsspec adapter for CSV and UNLOAD results; explicitNoneselects the native path for one query.On the PyAthena path, Arrow timeouts merge into the connection's S3 configuration, and any dedicated client closes after eager materialization, including failures.
This is a breaking 4.0.0 cursor API change: former S3
max_workerskeywords raiseTypeErrorbefore query submission.Result-set, filesystem, and upload utility
max_workersarguments retain their existing meanings.S3 workers are per file, and Polars' native Parquet concurrency remains governed by Polars.
The query-result pool does not cap active Athena queries because submission precedes queueing.
Migrate the benchmark adapters to the new cursor argument and serialize dedicated Arrow S3 client creation with the connection's existing S3 creation lock.
WHY
Closes #1094.
The previous
max_workersargument had different meanings between cursor families and coupled the query pool to S3 readers inAsyncPolarsCursor.Distinct arguments make routing consistent and allow each pool to be configured separately.
TEST
Implementation revision:
77d75ee29a65d1b9dcb4dfe8b4d7cf96502e79c3; repair revision:dec21943781fef9299786a54c032c14b7c3fd426.just formatandjust lint: passed.pytest --noconftest, excluding the new AWS tests).pytest --noconftest).just benchmark test: 108 passed, 1 packaging test intentionally skipped; focused Arrow/S3-client checks: 11 passed (pytest --noconftest, selectings3_workers or s3_client). Formatting and lint passed again.just docs lint,just docs build, and a directsphinx-buildof this worktree: passed. The direct build has the same 214 existing diagnostics as the exact base; no new diagnostics.Both self-review rounds and their repair follow-ups are complete, with records inline.
Independent review used Claude Opus 5.5, Max profile, effort high (first-party Max, not Enterprise).
Its two findings were repaired, and the bounded follow-up returned CLEAN; records and repairs are inline.
Current-head offline CI passes.
The PR is Ready, and AWS Test run 37343344427 passed at the repair revision on Python 3.14: PyAthena 3603 passed / 12 skipped; SQLAlchemy compliance and async compliance each 589 passed / 759 skipped.
All three expected AWS suites ran; the older Draft run intentionally skipped its AWS placeholder jobs.