Model S3 paths with an S3Path value class

[laughingman7743]

WHAT

Add pyathena.filesystem.s3_path.S3Path, a public frozen dataclass for an S3 path, and use it in place of the string handling at the call sites of S3FileSystem and AioS3FileSystem.

• S3Path(bucket, key=None, version_id=None):
  • parse() accepts what parse_path() accepts and raises the same ValueError; PATTERN holds the regular expression, unchanged.
  • is_bucket: no key, or a key of only slashes (bucket//), which rm() already treated as the bucket.
  • name: bucket/key (the key as parsed, including a trailing slash) or bucket.
  • str(): the name with ?versionId=<id> when there is a version. uri: the same with s3://.
  • with_version_id().
• The 30 internal parse_path() calls in s3.py and 3 in s3_async.py now use S3Path.parse(). The hand-built version paths in _head_object(), _delete_objects_path() and _move_target(), and the URIs in the log messages that carry a version, now come from S3Path.
• S3FileSystem.parse_path(), AioS3FileSystem.parse_path() and S3FileSystem.PATTERN_PATH remain, built on S3Path.
• S3Path is added to the API reference (docs/api/filesystem.rst).

No behavior change for S3FileSystem and AioS3FileSystem themselves. The debug log messages no longer print ?versionId=None for paths without a version, and the bucket-level log messages print s3://bucket instead of s3://bucket/None.

Left as they are:

• invalidate_cache() keeps splitting at the first ?. It accepts strings that S3Path.parse() rejects, and Keys containing '?' are rejected, and pinned or listed versions are not addressable #979 replaces this splitting when it changes the version rule.
• S3File.__init__() keeps appending ?versionId= to the path it was given. Building it from S3Path would drop an s3:// scheme from S3File.path for a file constructed directly.
• Internal helpers that take bucket/key arguments (_copy_object(), _put_object(), the multipart helpers) keep their signatures, so that the open PRs touching them (Move the write-and-close helpers from S3FileSystem to S3File #1035, Copy metadata, tags and annotations in multipart copies #1036, Compress pipe_file() values up front, and route and key them consistently #1038, Keep a multipart upload whose abort fails so that discard() retries it #1047) conflict less.
• fsspec's _strip_protocol() and _parent() still build the names and cache keys. That belongs to the fsspec adapter layering, which is tracked separately.

Compatibility note (release note for 4.0.0): the internal call sites no longer go through self.parse_path() or PATTERN_PATH, so a subclass that overrides parse_path() or reassigns PATTERN_PATH no longer changes how S3FileSystem parses paths. Calling parse_path() directly still returns the same tuple. Neither was a documented extension point, and nothing in the repository overrides them; the maintainer chose not to keep them as one.

Deviation from the issue: name keeps a trailing slash in the key (bucket/dir/), whereas the issue described it as what _strip_protocol() returns. _move_target() compares a/ and a as different keys, so dropping the slash would change move conflict detection.

WHY

Closes #1049. It is a prerequisite of #979, which changes the version rule (only a trailing version query is a version, so keys may contain ?) and should change it in one place.

TEST

Tested commit 1dc7a6b, rebased onto master 8c9d676 (after #1033, #1035, #1036, #1038 and #1043). The rebase conflicted only where master changed code around the converted lines: #1036's _COPY_METADATA_PARAMS next to PATTERN_PATH, and the try/finally around the copies in _copy_file(). #1036 also added four debug log lines with hand-built version URIs (two in _get_multipart_copy_kwargs(), one each in _list_object_annotations() and _copy_object_annotation()), which now use S3Path.uri as well. No upstream change added a parse_path() call.

• just lint: passed (ruff, format, mypy, cfn-lint, license headers).
• just docs lint: 0 errors.
• uv run pytest tests/pyathena/filesystem/test_s3_path.py: the new unit tests for S3Path (parse with each scheme and version spelling, bucket paths including bucket/ and bucket//, trailing-slash keys, invalid paths, is_bucket, name/str()/uri, with_version_id(), immutability), and its doctest example (python -m doctest).
• Offline run of tests/pyathena/filesystem/ (--noconftest, dummy credentials) before the rebase, on master 28ec68d and on this branch: the same 118 AWS-dependent tests fail on both, and every other test passes.
• Live AWS run: uv run --env-file .env pytest -n 4 tests/pyathena/filesystem/ → 665 passed on 1dc7a6b (642 on 93c1fbd and 607 on 6f9b958 before the rebases).

🤖 Generated with Claude Code

[laughingman7743]

Self-review round one (implementation behavior): CLEAN.

Base 28ec68d, head 11e785a (the tree of 6f9b958 is identical; only the commit message changed).

Covered: every converted call site in s3.py (30) and s3_async.py (3), S3Path itself, and the API reference entry.

• _head_object(): str(s3_path.with_version_id(...)) equals the former path.partition('?')[0] + '?versionId=' for its callers (ls(), info(), _move_paths()), which all pass _strip_protocol() or _move_target() output, so the cache keys are unchanged.
• _move_target() (here): the null version maps to name, and any other version maps to str(), as before. name keeps a trailing slash, as the former f"{bucket}/{key}" did.
• _delete_objects_path(): same string for every non-empty Key.
• rm()'s bucket check moved into is_bucket unchanged.
• Sync and aio _copy_file() take the same source and destination checks.
• PATTERN is a ClassVar, not a dataclass field; the tests construct S3Path("bucket") positionally.
• The doctest example in S3Path passes (python -m doctest).

No findings. Left unchanged and stated in the PR body: invalidate_cache() splitting (#979), S3File.__init__() path suffix, and the helper signatures that open PRs touch.

[laughingman7743]

Repair after the independent review, both self-review perspectives: CLEAN.

The old range was 28ec68d..6f9b958. The new range is ec5323e..93c1fbd: rebased onto master, plus 93c1fbd for the docstring.

• Implementation: git range-diff shows 49aa18b identical to 6f9b958 apart from the import context, where master now imports override. I checked the upstream filesystem changes from Compress pipe_file() values up front, and route and key them consistently #1038 and Convert time zone values to aware times and empty TIME/JSON text to NULL #1033: they add _strip_protocol() calls to pipe_file() and no parse_path() call or hand-built version path, so no new call site needed converting. 93c1fbd only adds the Attributes: section, and its wording matches parse() (a trailing slash is kept) and is_bucket (bucket//).
• Claims: the PR body now records the subclass compatibility note, the rebase, and the evidence on 93c1fbd: just lint passed and the live pytest -n 4 tests/pyathena/filesystem/ passed 642 tests. The offline comparison with master is stated as having been run before the rebase.

[laughingman7743]

Second rebase, both self-review perspectives: CLEAN.

The old range was ec5323e..93c1fbd. The new range is 8c9d676..1dc7a6b, after #1035, #1036 and #1043.

• Implementation: the conflicts were Copy metadata, tags and annotations in multipart copies #1036's _COPY_METADATA_PARAMS next to PATTERN_PATH, and its try/finally around the copies in the sync and aio _copy_file(). Both keep master's structure with the source/destination names. Copy metadata, tags and annotations in multipart copies #1036 added four debug log lines with hand-built version URIs, which now use S3Path.uri (two in _get_multipart_copy_kwargs(), one each in _list_object_annotations() and _copy_object_annotation()). No upstream change added a parse_path() call. The helpers that take bucket/key arguments keep them.
• Claims: the PR body describes the rebase and these log lines, and the evidence on 1dc7a6b: just lint passed and the live pytest -n 4 tests/pyathena/filesystem/ run passed 665 tests.

[laughingman7743]

Self-review round two (claims and callers): FINDINGS, repaired.

Base 28ec68d, head 11e785a, repaired in 6f9b958.

Finding: the commit message said "36 parse_path() calls", and issue #1049 said 31 and 5. Those were grep line counts that included the definitions and the aio wrapper. On master there are 30 calls in s3.py and 3 in s3_async.py. Repaired: amended the commit message (pushed with --force-with-lease; the tree is unchanged) and corrected the issue body. The PR body already said 30 and 3.

Claims checked:

• parse_path() (here) and PATTERN_PATH keep their return shape and exception, because S3Path.PATTERN is the former regular expression and parse() raises the same message.
• "No behavior change" holds for requests, cache keys and exceptions. Only the debug log text changed, as the PR body states (?versionId=None dropped, s3://bucket/None → s3://bucket).
• The reason given for keeping invalidate_cache() holds: it accepts strings that parse() rejects (e.g. bucket/k?x=1).
• The reason given for keeping S3File.__init__() holds: S3File(fs, "s3://bucket/key", version_id=...) keeps the scheme in path.
• Evidence: the same 118 AWS-dependent failures offline on master and on the branch; the live run pytest -n 4 tests/pyathena/filesystem/ passed 607 tests on the same tree.

[laughingman7743]

Independent review (relayed): FINDINGS.

Reviewer: Codex CLI 0.160.0, model gpt-6-astra, reasoning effort max, codex exec -s read-only (session 01a102f7-a971-7e70-9dda-6f6d7300ead6). This was a static review with no edits, builds, tests or network access. It ran on a detached snapshot of head 6f9b958, merge-base 28ec68d, and that snapshot was unchanged afterward. The prompt contained the diff and the intended contract, without the PR text or prior findings. The first launch waited on stdin until its time limit and produced no review; this is the rerun with stdin closed.

Coverage reported: all changed sync/async call sites; bucket-only, bucket//, trailing-slash, every version spelling, null; public parser compatibility; docstrings and conventions; tests.

Finding 1 (P2), anchored here at rm_file(): the internal call sites bypass a subclass override of parse_path() and a reassigned PATTERN_PATH. For example, a subclass whose parse_path("bucket/key") returns tenant/key used to make rm_file() delete tenant/key, and now it deletes key.

Verdict: confirmed as a contract narrowing for subclasses. Nothing in the repository overrides either, and neither is documented as an extension point. The maintainer chose not to keep them as one, so the PR body now states this in a compatibility note for the 4.0.0 release notes. No code change.

[laughingman7743]

Independent follow-up (relayed): CLEAN.

Reviewer: Codex CLI 0.160.0, model gpt-6-astra, codex exec -s read-only (session 01a10438-dfc0-7e31-ad3e-2b6995d15a11). This was a static review of head 93c1fbd on the detached snapshot. Its package was the range-diff from 28ec68d..6f9b958 to ec5323e..93c1fbd, the upstream pyathena/filesystem/ diff 28ec68d..ec5323e, and the docstring commit, together with the narrowed subclass contract.

Coverage reported: no parsing or version-path construction added upstream was left unconverted; pipe_file (its normalization, compression, buffered writes, bucket and version rejection, request arguments and cache invalidation) and the async delegation and transactions are consistent with the rebased baseline; and the S3Path field documentation is accurate. No files were changed, nothing was built or tested, and there was no network access.

[laughingman7743]

Independent follow-up 2 (relayed): CLEAN.

Reviewer: Codex CLI 0.160.0, model gpt-6-astra, codex exec -s read-only (session 01a1044a-2417-76e1-b3ea-59bb86729a47). This was a static review of head 1dc7a6b on the detached snapshot, which was unchanged afterward. Its package was the range-diff ec5323e..93c1fbd → 8c9d676..1dc7a6b, the upstream pyathena/filesystem/ diff ec5323e..8c9d676, and the full diff against 8c9d676.

Coverage reported: the sync and async _copy_file() resolutions (validation, request arguments, the directory return, and the upstream try/finally invalidation, including annotation copy failures); the PATTERN_PATH alias and the adjacent constants and imports; the upstream metadata, tag and annotation additions and the relocated S3File write helpers (no missed conversion); and the four S3Path.uri log lines (explicit versions including null kept, absent versions omitted, requests unchanged). No files were changed, nothing was built or tested, and there was no network access.

[laughingman7743]

Independent review (relayed), finding 2 (P3): _head_object("s3://bucket/key?versionID=v1") used to cache under s3://bucket/key?versionId=v1, and now caches under bucket/key?versionId=v1.

Verdict: rejected as unreachable. _head_object() is private, and its three callers pass paths without a scheme: ls() uses _strip_protocol(path).rstrip("/"), info() uses _strip_protocol(path), and _move_paths() uses _move_target()'s bucket/key form. For those inputs this line produces the same string as the former path.partition('?')[0] + '?versionId=' + version_id.

Pre-existing, reported by the same review and not changed here: info("s3://bucket?versionId=v1") passes Bucket="bucket?versionId=v1" to HeadBucket (s3.py:843, outside the diff); the merge-base behaves the same. A version on a bucket path names nothing, so it is only recorded.

[laughingman7743]

Independent review (relayed), minor findings:

• The class had no field documentation comparable to ExecuteOptions. Confirmed; fixed in 93c1fbd with this Attributes: section.
• tests/pyathena/filesystem/test_s3_path.py:13 groups pure unit tests in class TestS3Path instead of standalone functions. Rejected: the maintainer prefers unit tests that target one class to be grouped in a Test<ClassName> class.

The review found the frozen dataclass, replace(), ClassVar, @override and the docstrings consistent with the conventions, and the tests meaningful apart from the compatibility cases above.