Skip to content

Share one S3 client per connection across result sets and filesystems - #1058

Merged
laughingman7743 merged 10 commits into
masterfrom
feature/1011-shared-s3-client
Oct 4, 2026
Merged

laughingman7743 merged 10 commits into
masterfrom
feature/1011-shared-s3-client

Conversation

@laughingman7743

@laughingman7743 laughingman7743 commented Oct 4, 2026 •

Copy link
Copy Markdown
Member

WHAT

A connection now builds one S3 client on first use and shares it, instead of each result set and filesystem building its own.

  • Connection.s3_client (new public property) builds the client on first use from the connection's session, region, client arguments and s3_config. A lock makes concurrent first uses build one client, as GlueMetadataClient does.
  • Users of the shared client:
    • AthenaResultSet (HeadObject for the output, GetObject for the data manifest). It reads connection.s3_client only when it makes these requests, so the result sets of Cursor, DictCursor and their async versions, which never make them, build no S3 client. The AthenaResultSet._client attribute is removed.
    • S3FileSystem(connection=...), including the filesystems that the pandas, Polars and S3FS result sets create and the internal filesystem of AioS3FileSystem(connection=...). Filesystems that users create with connection= share it too.
    • The Spark cursors. They still access the client before starting a session, so a client that fails to build leaves no session behind.
  • connect(s3_config=Config(...)) (new, added as the last parameter so positional arguments keep their positions): botocore Config options for the S3 client only, merged over config with Config.merge(), so the S3 client keeps config's other options. Without it, the S3 client uses config as before. PyAthena's user agent is added to the merged config as well.
  • The S3 client leaves out the connection's endpoint_url and api_version, which are Athena's, as the Glue client already does. Before, an Athena VPC endpoint in endpoint_url received the S3 requests, so HeadObject failed with 404 (Using VPC endpoint and PandasCursor together #576), and an Athena api_version made the S3 client fail to build (DataNotFoundError: Unable to load data for: s3/2017-05-18/service-2). The S3 endpoint can be set with botocore's service-specific endpoint settings, such as AWS_ENDPOINT_URL_S3.
  • pyathena.pandas.util.to_sql() builds its own S3 resource and upload clients; they now take the same arguments as the shared client (new private Connection._s3_client_kwargs, next to _client_kwargs). Before, an Athena VPC endpoint received its PutObject requests.
  • SQLAlchemy URLs: use_ssl is now parsed as a boolean, as verify already was. The string "false" reached botocore unchanged, which treats it as true (Session.client("athena", use_ssl="false") → https://...; use_ssl=False → http://..., checked locally), so use_ssl=false had no effect. Found by the independent review; included at the maintainer's request.
  • Connection.close() closes the Athena client, and the Glue and S3 clients if they were built. It did nothing before. botocore re-creates the connection pool when a closed client is used again (checked locally), so cursors, result sets and filesystems used after close() keep working and open new connections.
  • docs/usage.md gets an "S3 client" section: what shares the client, s3_config, the pool size, and close(). docs/filesystem.md links to it from the connection constructor.

The design questions in #1011 were settled by the maintainer before implementation: a public Connection.s3_client; close() closes all three clients; user-created S3FileSystem(connection=...) shares the client; a new S3-only s3_config with the pool size unchanged by default.

Behavior changes (release-note candidates):

  • One S3 connection pool per connection instead of one per result set and filesystem. The pool keeps max_pool_connections connections per host (botocore default 10, from config unless s3_config sets it). Concurrent requests beyond that do not block: urllib3 opens more connections, closes them after use, and logs a "Connection pool is full" warning (a log record from the urllib3.connectionpool logger, not a Python warning). This was already the case within one filesystem whose max_workers exceeds the pool size; now the result sets of one connection share the limit.
  • Connection.close() now closes the network connections of its clients.
  • Breaking: endpoint_url passed to connect() no longer applies to S3. Setups that serve Athena and S3 from one endpoint (for example LocalStack) need AWS_ENDPOINT_URL_S3 or AWS_ENDPOINT_URL. This includes the result sets, S3FileSystem(connection=...), Polars storage_options, the Spark cursors and to_sql().
  • SQLAlchemy URLs with use_ssl=false now really disable TLS for the Athena and S3 clients, as connect(use_ssl=False) does; before, the option was ignored. Against AWS's own endpoints this fails: a plain-HTTP connection to athena.us-west-2.amazonaws.com did not connect within 10 s (curl, 2026-10-04), while HTTPS answered. A URL that still carries use_ssl=false for such endpoints has to drop it.
  • New connect() argument s3_config and property Connection.s3_client.

Not changed: ArrowCursor, and PolarsCursor for Parquet (unload=True) and chunked results, read the results through their libraries' own S3 clients, so only their HeadObject/manifest requests use the shared client.

WHY

Closes #1011. Fixes the problem reported in #576 (closed without a fix).

Since #1001, the result-set filesystems are no longer kept in fsspec's instance cache, so a pandas, Polars or S3FS query built two S3 clients and opened new HTTPS connections each time.

Measured on 2026-10-04 from a laptop to the CI region, 10 × SELECT 1 AS x per cursor on one connection, time from building the result set to fetchall() returning:

Cursor S3 clients, before → after Median, before → after
Cursor 10 → 0 236 → 221 ms
PandasCursor 20 → 1 1318 → 664 ms
ArrowCursor 10 → 1 1291 → 975 ms
PolarsCursor 20 → 1 1337 → 674 ms
S3FSCursor 20 → 1 1320 → 680 ms

The first query of each connection still pays for a new connection. The gain inside the region was not measured and should be smaller.

TEST

Rebased onto 8bbf8cb (master after #1060 S3Core); current head bc0d251. The only conflict was S3FileSystem.__init__, which now passes connection.s3_client to S3Core. After the rebase: just lint, just docs lint, and the connection, Spark, SQLAlchemy dialect, new live, test_s3_core.py and filesystem test_init.py tests passed (224).

AWS CI on bc0d251 (run 37174891344): test and test-sqla-async passed. test-sqla failed once in TextTest::test_literal_quoting with Athena's Iceberg table to be created already exists for CREATE TABLE t at the worker schema's shared t/ location (DDL through the REST cursor, which makes no S3 requests from PyAthena; not covered by the rerun filters). It was the first such failure in the last 40 Test runs; the rerun of that job passed (attempt 2), and the next run on the same head after marking Ready (37176167498) passed all of test, test-sqla and test-sqla-async on the first attempt.

First tested commit 3d6f7b8 (base a4cf604). Later commits change a Spark comment, the position of s3_config in the signature, and docs; after them, just lint, just docs lint, tests/pyathena/test_connection.py (48 passed) and tests/pyathena/spark/test_common.py (86 passed) were rerun.

  • just lint: passed. just docs lint: 0 errors.
  • Live, against the CI test account: uv run --env-file .env pytest -n 2 tests/pyathena/test_connection.py tests/pyathena/test_glue.py tests/pyathena/spark/test_common.py tests/pyathena/s3fs/test_cursor.py tests/pyathena/pandas/test_cursor.py tests/pyathena/test_cursor.py::TestCursor tests/pyathena/aio/test_cursor.py: 533 passed, 1 skipped (test_executemany of S3FS, skipped on master too).
  • New tests:
    • TestCursor::test_builds_no_s3_client and TestPandasCursor::test_result_sets_share_s3_client fail on the original source (one S3 client for the default cursor; four for two pandas queries instead of one) and pass with the change.
    • Commit 041913b (endpoint): TestPandasCursor::test_athena_endpoint_url (a query with endpoint_url=https://athena.<region>.amazonaws.com) fails on the source without the change with OperationalError: An error occurred (404) when calling the HeadObject operation: Not Found, as in Using VPC endpoint and PandasCursor together #576, and passes with it. TestConnection::test_s3_client_leaves_out_athena_endpoint and test_s3_client_uses_s3_endpoint_setting check the S3 client's endpoint and API version, and that AWS_ENDPOINT_URL_S3 applies to S3 only. After this commit: just lint, just docs lint, and pytest -n 2 tests/pyathena/test_connection.py tests/pyathena/spark/test_common.py tests/pyathena/s3fs/test_cursor.py tests/pyathena/pandas/test_cursor.py: 344 passed, 1 skipped.
  • Commit 8ec954b (to_sql()): test_to_sql_athena_endpoint_url fails without the change (ClientError ... when calling the PutObject operation) and passes with it.
  • Commit 38f36ba (SQLAlchemy use_ssl): TestAthenaDialect::test_conn_str_use_ssl (REST and aio dialects, false/true) fails without the change and passes with it.
  • After these commits: just lint, just docs lint, and pytest -n 2 tests/pyathena/test_connection.py tests/pyathena/pandas/test_util.py tests/pyathena/sqlalchemy/test_base.py::TestAthenaDialect: 115 passed. The S3 endpoint unit tests use an isolated_aws_config fixture (no AWS_* variables, empty config and credentials files) since 7c394d1; on that head, tests/pyathena/test_connection.py passes (50), also with AWS_S3_US_EAST_1_REGIONAL_ENDPOINT=regional and AWS_IGNORE_CONFIGURED_ENDPOINT_URLS=true set.
  • TestConnection: one client across 8 threads, S3FileSystem(connection=...) uses it, s3_config defaulting to and merging over config (with the user agent), and close() closing only built clients (mocked close).
    • test_init_does_not_start_session_when_s3_client_fails (Spark) now makes s3_client fail instead of session.client.
  • Not run locally: the filesystem, Arrow, Polars and SQLAlchemy suites, and Spark cursors against Athena. They are left to the AWS CI. The Spark cursor's client sharing is covered only by reading the code and the unit test above.

🤖 Generated with Claude Code

Comment thread pyathena/connection.py
return self._client

@property
def s3_client(self) -> BaseClient:

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Self-review round one (behavior and implementation): FINDINGS (1, repaired)

Base a4cf604, head 3d6f7b8 (full diff, 10 files).

Covered:

  • Behavior and failure paths: s3_client builds once under the lock (unit test with 8 threads). Every caller (AthenaResultSet._get_content_length()/_read_data_manifest(), S3FileSystem.__init__, SparkBaseCursor.__init__) only reads the client. Nothing in pyathena/ registers event handlers on it or closes it (grep meta.events, _client.close), so sharing cannot leak state between result sets or filesystems. The result sets of Cursor/DictCursor/AsyncCursor/AioCursor (AthenaResultSet, AthenaAioResultSet) never call the two S3 helpers, so they build no client (live test).
  • After a result set is closed, self.connection is None, so the lazy connection.s3_client access would fail. Every caller of the two helpers runs during result-set construction, or reads through self._fs/storage_options built with the same connection, which already fail after close. No new failure path.
  • close(): botocore re-creates its pools when a closed client is used again (checked locally), so cursors used after close() keep working. The Glue and S3 clients are not built just to close them (unit test).
  • s3_config: Config.merge() returns a new object, so the user's s3_config is not mutated; the user agent is added to the merged config (unit test).
  • Data/resource boundaries: no change to conversions or streams. Framework contracts: fsspec behavior is unchanged except for the client's origin; AioS3FileSystem passes connection to its internal S3FileSystem and shares the client too.
  • Simplicity and tests: the two live regression tests assert client-build counts and fail on the original source.

Finding: the Spark comment pyathena/spark/common.py:108 said the client was "Created" before the session; with a shared client it may already exist. Repaired in 3c666d1 (comment only).

@laughingman7743
laughingman7743 force-pushed the feature/1011-shared-s3-client branch from ac676e1 to 543996a Compare October 4, 2026 02:02
Comment thread pyathena/connection.py
on_start_query_execution: Callable[[str], None] | None = None,
on_poll: OnPollCallback | None = None,
glue_metadata_fallback: bool = True,
s3_config: Config | None = None,

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Self-review round two (claims, callers, operations): FINDINGS (3, repaired)

Base a4cf604, head 3c666d1 (full pass: PR body, commit messages, docstrings, docs/usage.md, related docs).

Claims checked:

  • "Two S3 clients per pandas/Polars/S3FS query" and the latency table: measured on 2026-10-04 (20 → 1 clients per 10 queries; script times result-set construction to fetchall()).
  • "botocore default 10": botocore.endpoint.MAX_POOL_CONNECTIONS == 10 (botocore 1.43.102).
  • "Requests beyond the pool do not block": botocore's PoolManager sets no block, and urllib3 2.8.0 _put_conn() closes and discards the connection when the queue is full.
  • "botocore re-creates pools after close()": URLLib3Session.close() only clears the pool managers; a HEAD request after close() succeeded locally.
  • "ArrowCursor reads through pyarrow's S3 filesystem": AthenaArrowResultSet._read_csv()/_read_parquet() use self._fs (pyarrow), and only _get_content_length()/_read_data_manifest() use the shared client.
  • "Default cursors build no S3 client": Cursor/AsyncCursor use AthenaResultSet, AioCursor uses AthenaAioResultSet; neither calls the S3 helpers (live test for Cursor).
  • test_executemany skip: unconditional @pytest.mark.skip, so it is skipped on master too.
  • AWS operator: retries unchanged; the S3 client keeps config's retry settings unless s3_config overrides them, and retry_api_call still wraps the result-set requests.

Findings and repairs (543996a):

  1. Existing caller: s3_config was inserted after config, which moved result_reuse_enable and the later parameters by one position for positional callers. It is now the last parameter, as glue_metadata_fallback was added.
  2. Documentation: the pool sentence said urllib3 closes the extra connections "with a warning"; it is a log record from urllib3.connectionpool, not a Python warning. Reworded in docs/usage.md:774 and the PR body.
  3. Documentation: docs/filesystem.md did not say that a filesystem built from a connection uses its S3 client; it now links to the new section (plain page link, since myst_heading_anchors is not enabled).

After the repairs: just lint, just docs lint, tests/pyathena/test_connection.py (48 passed) and tests/pyathena/spark/test_common.py (86 passed).

Comment thread pyathena/connection.py
return self._client

@property
def s3_client(self) -> BaseClient:

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Independent review (relayed): CLEAN, static review.

  • Reviewer: OpenAI Codex CLI 0.160.0, model gpt-6-astra, reasoning effort high, sandbox read-only, session 01a104a6-8b99-78a1-a0b0-1bdcc7363c2a. A different model from the author (Claude).
  • Scope: git diff a4cf60427f3027db084a51883a59cfea871475f4..543996ac2fb1020706b772c1c9649c6ebce5c25f in a detached snapshot worktree at 543996a, with no .env. The prompt had no PR number, description, commit messages or earlier findings. Constraints: no edits, builds, tests, installs, network or GitHub access.
  • Covered (reviewer's list): lazy S3 construction, locking, construction failures, and all previous client-construction sites; client ownership, event handlers, configuration mutation and close calls across result sets, filesystems and Spark; Connection.close() through sync, asyncio, SQLAlchemy and Spark callers, including later client use; s3_config precedence, user agent, and positional/keyword signature compatibility; the standard, pandas, Polars, Arrow and S3FS result paths, AioS3FileSystem, error translation and retries; tests, docs and docstrings against the dependency sources pinned in uv.lock.
  • Result: "No actionable regressions found in the specified diff. The added tests meaningfully distinguish shared/lazy construction from the previous implementation. Close tests verify delegation and avoid constructing unused clients; they do not establish runtime behavior after close."
  • On that limit: use after close() relies on botocore re-creating its pools. That was checked with a local botocore script (HEAD request after client.close()), not with a PyAthena test.
  • The snapshot and the PR worktree were unchanged after the review (HEAD 543996a, clean status).

@laughingman7743
laughingman7743 marked this pull request as ready for review October 4, 2026 02:13
@laughingman7743
laughingman7743 marked this pull request as draft October 4, 2026 02:39
Comment thread pyathena/connection.py Outdated
**{
k: v
for k, v in self._client_kwargs.items()
if k not in ("endpoint_url", "api_version")

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Self-review round one (behavior), endpoint scope expansion: CLEAN

Scope: 543996a..041913b (added at the maintainer's request: leave Athena's endpoint_url/api_version out of the S3 client).

  • The remaining client arguments still reach S3: credentials, use_ssl, verify (custom CA bundles and proxies keep working). region_name and config are named Connection parameters, never in _kwargs, so nothing is passed twice.
  • Every S3 user of the connection goes through s3_client: AthenaResultSet, S3FileSystem(connection=...) (pandas, S3FS, Polars storage_options, AioS3FileSystem), and the Spark cursors, whose GetObject for calculation output also went to Athena's endpoint before. S3FileSystem without a connection and ArrowCursor's pyarrow filesystem never used endpoint_url.
  • Regression coverage: the live test_athena_endpoint_url reproduces Using VPC endpoint and PandasCursor together #576's 404 on the source without the change and passes with it. The unit tests clear AWS_ENDPOINT_URL/AWS_ENDPOINT_URL_S3 so that a developer's environment cannot change them.

Comment thread docs/usage.md Outdated
`ArrowCursor` reads the query results through pyarrow's own S3 filesystem and uses the shared client for its other S3 requests.

The S3 client is built from the connection's session, region, and client arguments, with the botocore `config` merged with `s3_config`.
It does not use the connection's `endpoint_url` and `api_version`, which are Athena's.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Self-review round two (claims, callers), endpoint scope expansion: CLEAN

Scope: 543996a..041913b.

  • "An Athena api_version makes the S3 client fail to build": Session.client("s3", api_version="2017-05-18") raises DataNotFoundError: Unable to load data for: s3/2017-05-18/service-2 (botocore 1.43.102, checked locally).
  • "AWS_ENDPOINT_URL_S3 sets the S3 endpoint only": checked locally and in test_s3_client_uses_s3_endpoint_setting. Service-specific endpoints exist since botocore 1.31.x, below the botocore>=1.43.31 floor. The linked AWS SDK reference page returns 200.
  • Existing caller: a setup that relied on one endpoint_url for both Athena and S3 (e.g. LocalStack) now sends S3 requests to AWS's S3 endpoint and needs AWS_ENDPOINT_URL_S3 or AWS_ENDPOINT_URL. Recorded as a breaking change in the PR body; there is no in-repo test or doc that used endpoint_url for S3.
  • Docs: the Glue paragraph (docs/usage.md:749) already says Glue does not use endpoint_url, consistent with the new S3 sentence.

Comment thread tests/pyathena/test_connection.py Outdated
assert all(client is clients[0] for client in clients)
assert clients[0].meta.service_model.service_name == "s3"

def test_s3_client_leaves_out_athena_endpoint(self):

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Independent follow-up review (relayed): FINDINGS, static review.

  • Reviewer: OpenAI Codex CLI 0.160.0, model gpt-6-astra, reasoning effort high, sandbox read-only, session 01a104d6-0a23-7002-869b-685eb45777a3.
  • Scope: 543996ac2fb1020706b772c1c9649c6ebce5c25f..041913b8a714497b0ed34d628217c36a727b504c (the endpoint expansion), in a detached snapshot at 041913b with no .env; no PR text or earlier findings in the prompt; no edits, builds, tests, network or GitHub access.
  • Covered (reviewer's list): Connection.s3_client argument filtering, config merging, credentials, botocore endpoint/API-version resolution; REST/dict, pandas, Arrow, Polars, S3FS cursors incl. threaded and asyncio variants; S3FileSystem/AioS3FileSystem(connection=...), Spark cursors, pandas.util.to_sql; SQLAlchemy URL handling, tests, docs and docstrings against boto3/botocore 1.43.102, Polars 1.44.2, PyArrow 25.0.1, fsspec 2026.9.0.
  • Reviewer's verdict on the change: the filter excludes both Athena arguments from every shared-client consumer; credentials, verify, use_ssl, region and merged config still apply; the new tests detect the old behavior.

Findings:

  1. (Introduced, P2) The endpoint unit tests depended on ambient AWS configuration: AWS_S3_US_EAST_1_REGIONAL_ENDPOINT=regional changes the hard-coded https://s3.amazonaws.com, and AWS_IGNORE_CONFIGURED_ENDPOINT_URLS=true or a shared-config endpoint changes the second test. Verified and repaired in 3f1f48c: the first test asserts the S3 service and an endpoint different from Athena's instead of a fixed URL; the second points AWS_CONFIG_FILE to an empty temp path and sets AWS_IGNORE_CONFIGURED_ENDPOINT_URLS=false. Both pass with those two variables set to the hostile values and fail on the source without the fix.
  2. (Pre-existing, P2) to_sql() builds its own S3 resource and worker clients from conn._client_kwargs (pyathena/pandas/util.py:261, :298), so it still sends Athena's endpoint_url/api_version to S3. Verified; not in this commit. Reported to the maintainer as a scope question (fold in or separate issue).
  3. (Pre-existing, P2) A SQLAlchemy URL use_ssl=false stays the string "false", which botocore treats as true (pyathena/sqlalchemy/base.py:302; only verify is converted). Verified; unrelated to this PR, to be filed separately.
  • The reviewer also notes that Arrow's pyarrow filesystem and Polars' native Parquet/chunked readers do not use the shared client; the docs already say so for Arrow, and the S3 client section describes the shared client only.

Comment thread pyathena/pandas/util.py
session_kwargs = deepcopy(conn._session_kwargs)
session_kwargs.update({"profile_name": conn.profile_name})
client_kwargs = deepcopy(conn._client_kwargs)
client_kwargs = deepcopy(conn._s3_client_kwargs)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Self-review round one (behavior), follow-up scope: CLEAN

Scope: 041913b..HEAD (3f1f48c test isolation, cacf8d8 Polars docs, 8ec954b to_sql(), 38f36ba SQLAlchemy use_ssl; the last two added at the maintainer's request after the independent review).

  • to_sql(): the bucket resource (:262, DROP + object deletes for if_exists="replace") and the upload workers (:301, passed to to_parquet() and pickled for a ProcessPoolExecutor) get Connection._s3_client_kwargs, a new dict per access with the same keys as _client_kwargs minus endpoint_url/api_version; region_name is still added explicitly. Credentials and verify still apply.
  • use_ssl (pyathena/sqlalchemy/base.py:309): parsed with strtobool like kill_on_interrupt; an invalid value raises ValueError from create_connect_args() as those options do. All dialects (REST, pandas, Arrow, Polars, S3FS and their aio versions) go through _create_connect_args(). A use_ssl given through connect_args is not in the URL query and is untouched.
  • Behavior consequence verified: use_ssl=false in a URL now reaches botocore as False and switches AWS endpoints to http://; Athena's public endpoint did not accept a plain-HTTP connection within 10 s (curl). Stated in the PR body as a behavior change.
  • Tests: test_to_sql_athena_endpoint_url (live) and test_conn_str_use_ssl (offline, REST + aio, false/true) fail on the source without the respective change; the S3 endpoint unit tests pass with hostile AWS_S3_US_EAST_1_REGIONAL_ENDPOINT/AWS_IGNORE_CONFIGURED_ENDPOINT_URLS values.

with contextlib.suppress(ValueError):
verify = bool(strtobool(verify))
opts.update({"verify": verify})
if "use_ssl" in opts:

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Self-review round two (claims, callers), follow-up scope: CLEAN

Scope: 041913b..HEAD.

  • "botocore treats the string \"false\" as true": Session.client("athena", use_ssl="false").meta.endpoint_url is https://athena.us-west-2.amazonaws.com, and with False it is http://... (botocore 1.43.102, local check).
  • "An Athena VPC endpoint received to_sql()'s PutObject requests": test_to_sql_athena_endpoint_url, with Athena's regional endpoint as endpoint_url, fails without 8ec954b with ClientError ... when calling the PutObject operation.
  • Docs: docs/usage.md now names to_sql() next to the shared client, and the Arrow/Polars sentence matches AthenaPolarsResultSet._parquet_storage_options and the scan_csv/scan_parquet chunk paths, which use Polars' object_store. There is no URL option list in docs/sqlalchemy.md to update.
  • Existing callers: to_sql() signature unchanged; URL users of use_ssl=false are the affected group, documented in the PR body.

Comment thread tests/pyathena/test_connection.py Outdated
def test_s3_client_uses_s3_endpoint_setting(self, monkeypatch, tmp_path):
# Only the environment variables below configure the endpoints.
monkeypatch.setenv("AWS_CONFIG_FILE", str(tmp_path / "config"))
monkeypatch.delenv("AWS_PROFILE", raising=False)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Independent follow-up review (relayed): FINDINGS (1, repaired), static review.

  • Reviewer: OpenAI Codex CLI 0.160.0, model gpt-6-astra, reasoning effort high, sandbox read-only, session 01a104e8-f25c-7c43-a442-0961001e1825.
  • Scope: 041913b8a714497b0ed34d628217c36a727b504c..38f36ba48837f1001cb2dcbcb22d17544ea236e8 in a detached snapshot at 38f36ba with no .env; no PR text or earlier findings in the prompt; no edits, builds, tests, network or GitHub access. Snapshot unchanged afterwards.
  • Covered (reviewer's list): to_sql() resource creation, ListObjects/DeleteObjects for replacement, PutObject in partitioned/unpartitioned workers incl. process pools (all exclude Athena's endpoint_url/api_version and keep credentials, use_ssl, verify, region); all five sync and five async SQLAlchemy drivers (shared boolean parser, invalid strings raise ValueError, explicit endpoint schemes keep precedence); tests and their AWS-configuration dependence; both docs changes; installed boto3/botocore, SQLAlchemy, Polars, PyArrow sources matching uv.lock.

Finding (introduced, P2): test_s3_client_uses_s3_endpoint_setting pointed AWS_CONFIG_FILE to an empty path without clearing AWS_PROFILE/AWS_DEFAULT_PROFILE, so a developer with a config-only profile selected gets ProfileNotFound. Verified and repaired in 4341706: the test clears both variables. A standalone script with the test's steps raises ProfileNotFound without the two deletions and resolves http://localhost:4566 with them (pytest itself cannot run with such a profile here, because the conftest session setup needs the real AWS configuration).

Noted by the reviewer as pre-existing and unchanged: to_sql()'s S3 resource and workers do not receive the connection's config/s3_config. Not addressed in this PR.



@pytest.fixture
def isolated_aws_config(monkeypatch, tmp_path):

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Independent follow-up reviews of the test repairs (relayed), static reviews by OpenAI Codex CLI 0.160.0, model gpt-6-astra, effort high, sandbox read-only, detached snapshots without .env, unchanged afterwards.

  1. Session 01a104f1-ff18-7233-ba71-7db7499401f0, scope 38f36ba4..43417069: FINDINGS. The endpoint tests still depended on the shared credentials file, inherited SDK settings and AWS_ENDPOINT_URL/profile endpoint settings. Repaired in 7c394d1 with the isolated_aws_config fixture: it removes every AWS_* variable and points AWS_CONFIG_FILE/AWS_SHARED_CREDENTIALS_FILE to empty temporary paths; both endpoint tests use it, and the first now asserts the exact default S3 endpoint and API version. Both pass with AWS_S3_US_EAST_1_REGIONAL_ENDPOINT=regional and AWS_IGNORE_CONFIGURED_ENDPOINT_URLS=true set, and both fail when the filter in Connection._s3_client_kwargs is emptied. (AWS_MAX_ATTEMPTS=0 or AWS_ENDPOINT_URL in the environment already break the conftest session setup, before any test.)
  2. Session 01a104f6-e9bd-7050-9fc8-6299a831658c, scope 43417069..7c394d13: FINDINGS, rejected with reason. It names non-AWS_* inputs: BOTOCORE_EXPERIMENTAL__PLUGINS, developer endpoint/service models under ~/.aws/models, and an invalid SSLKEYLOGFILE. Each of these changes or breaks every botocore client in the suite, including the conftest session setup that all tests in tests/pyathena/ require, so they are environment preconditions of the suite, not a dependence specific to these tests. The fixture isolates the inputs a normal developer setup has (AWS variables, profiles, config and credentials files). The reviewer confirms that, with those inputs excluded, both assertions catch forwarding Athena's endpoint.

laughingman7743 and others added 10 commits October 4, 2026 12:36
A connection now builds one S3 client on first use and shares it with
the result sets of its cursors, S3FileSystem(connection=...) and the
Spark cursors. Each result set and its filesystem used to build their
own client, so every query opened new connections to S3. The result
sets of the default cursors never used theirs and now build none.

connect() takes s3_config, botocore Config options merged over config
for the S3 client only, such as max_pool_connections.
Connection.close() now closes the Athena client and, if built, the
Glue and S3 clients.

Closes #1011

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The S3 client received the connection's client arguments, including
Athena's endpoint_url and api_version. With an Athena VPC endpoint,
HeadObject went to that endpoint and failed with 404 (#576), and an
Athena api_version made the S3 client fail to build. The S3 client now
leaves them out, as the Glue client does; botocore's service-specific
endpoint settings, such as AWS_ENDPOINT_URL_S3, set its endpoint.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ests

to_sql() builds its own S3 resource and upload clients from the
connection's client arguments, so an Athena VPC endpoint received its
PutObject requests. It now uses the same arguments as the shared S3
client, Connection._s3_client_kwargs.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The URL value stayed a string, and botocore treats "false" as true, so
use_ssl=false had no effect.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@laughingman7743
laughingman7743 force-pushed the feature/1011-shared-s3-client branch from 7c394d1 to bc0d251 Compare October 4, 2026 03:38
Comment thread pyathena/filesystem/s3.py
config=connection.config,
**connection._client_kwargs,
)
client = connection.s3_client

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rebase record: rebased from base a4cf604 / head 7c394d1 onto 8bbf8cb (master after #1044, #1055, #1056, #1057, #1060); new head bc0d251.

  • git range-diff: every commit is unchanged except the first, where the conflict with Add S3Core with typed listing entries and list/HEAD operations #1060 (S3Core) was resolved here: S3FileSystem.__init__ now passes connection.s3_client to S3Core instead of building a client from _client_kwargs. S3FileSystem._client is now core.client, so test_s3_filesystem_uses_connection_s3_client still checks the shared client.
  • Upstream contract check: no new S3 client creation from a connection was added upstream (grep '"s3"', _client_kwargs); the s3fs-compatible path (_get_client_compatible_with_s3fs) is unchanged; docs/filesystem.md merged without conflict.
  • After the rebase: just lint, just docs lint, and pytest -n 2 of tests/pyathena/test_connection.py, spark/test_common.py, sqlalchemy/test_base.py::TestAthenaDialect, the new live tests (pandas share/endpoint, default cursor, to_sql), filesystem/test_s3_core.py and filesystem/test_init.py: 193 + 31 passed. Full coverage is left to the AWS CI on this head.

@laughingman7743
laughingman7743 marked this pull request as ready for review October 4, 2026 03:43
@laughingman7743
laughingman7743 marked this pull request as draft October 4, 2026 03:54
@laughingman7743
laughingman7743 marked this pull request as ready for review October 4, 2026 04:09
@laughingman7743
laughingman7743 merged commit 911492c into master Oct 4, 2026
28 of 29 checks passed
@laughingman7743
laughingman7743 deleted the feature/1011-shared-s3-client branch October 4, 2026 04:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Share one S3 client per connection across result sets and filesystems

1 participant