Accept version_id in S3FileSystem.open() and cat_file()

[laughingman7743]

WHAT

S3FileSystem.open(), AioS3FileSystem.open() and S3FileSystem.cat_file() (and AioS3FileSystem.cat_file(), which delegates to it) now accept version_id.
get_file() takes it too, through open().

• open() hands version_id to S3File, which keeps its existing check: a version_id that differs from the ?versionId= in the path raises ValueError.
• S3File carries a version_id argument in its path as ?versionId=, so it behaves like the suffix form; for example, an unpickled file is reopened at the same version.
• S3File raises ValueError when a version, in either form, is given for writing or appending. pipe_file(), touch() and copying already reject a versioned target.
• cat_file() resolves the version like info(): the version in the path takes precedence. With start/end, the range is measured against the size of that version.
• S3File now looks up the object before calling the fsspec base initializer and passes the size to it. The base initializer took the read size from info(path) without the version, so a version given only as an argument would have been read with the size of the latest version. This also removes the second info() call when opening for reading.

Release note (4.0.0):

• Bug fix: open(), cat_file() and get_file() accept version_id instead of raising TypeError. (S3FileSystem.open() and cat_file() raise TypeError when given version_id #936)
• Breaking: open() for writing or appending (wb, ab, xb) raises ValueError when the path has ?versionId=. Before, the version was ignored for writing, and appending started from the content of that version.

Depends on #957 (#932) for the info() cache of explicit versions, which open() and cat_file() now reach with version_id: on master, the second info() of a ?versionId= path on one filesystem instance returns a directory, and info(path, version_id=...) caches that version under the plain path, where later lookups of the latest version find it.

WHY

Closes #936.
_open() passed version_id=None to S3File and also forwarded **kwargs, and cat_file() passed the version parsed from the path to _get_object() along with **kwargs, so a version_id keyword raised TypeError: ... got multiple values for keyword argument 'version_id'.
docs/filesystem.md already documents the version_id argument, so the guide is now correct as written.
Behavior when the path and the argument disagree follows each method's existing neighbor (S3File raises, info() lets the path win), and versioned writes are rejected, as agreed with the maintainer.

TEST

Tested commit: bbf3759

• just format and just lint: passed.
• uv run --env-file .env pytest -n 0 tests/pyathena/filesystem/test_s3.py tests/pyathena/filesystem/test_s3_async.py -k version_id: 12 passed.
  Without the pyathena/ changes, the new tests fail: TypeError for open()/cat_file(), the size lookup without the version, the version missing from the path, and no ValueError for versioned writes.
• uv run --env-file .env pytest -n 4 tests/pyathena/filesystem/: 253 passed against AWS.
• The new integration test reads the "null" version of an object in the unversioned staging bucket and checks that S3 rejects an unknown version (so the version reaches GetObject).
  Reading an older version with a different size is covered only by the unit tests, since the CI bucket is not versioned.

🤖 Generated with Claude Code

[laughingman7743]

Self-review round 1 (implementation behavior) — FINDINGS, repaired

Scope: git diff e0e85da09435257a18c8b8832b3e10ba7840b697..5d9482b19cdb0135e499936c442160305c6773d9 (initial pass), repair in 40f8c4c.
Covered: S3FileSystem._open/AioS3FileSystem._open, cat_file (and aio _cat_file, get_file → open), S3File.__init__ read/append/write paths, info()/_head_object/_ls_from_cache cache behavior for versions, unit + integration tests.

Findings:

1. S3File.__init__ (pyathena/filesystem/s3.py:2214 at head): fsspec's base initializer took the read size from info(path) without the version, so open(path, version_id=old) would read with the latest version's size. Found while writing the test (info() called twice, first without the version). Repaired in 5d9482b by looking the object up first and passing size.
2. info() (this line): a version given as an argument used the cache of the latest version (cached object entry or parent listing → latest metadata, so open() would send the latest ETag as IfMatch), and HeadObject results for a version were cached under the object path, poisoning later latest-version lookups. The suffix form returned a directory (size 0) on its second lookup and raised FileNotFoundError with a cached parent listing. Reproduced offline on master code; pre-existing, but directly reached by the new open()/cat_file() version_id. Repaired in 40f8c4c: versioned lookups skip _ls_from_cache and _head_object caches under the version-qualified path (the suffix's key). test_info_version_id (6 cases) fails without the repair.

Checked, no change: raising before super().__init__() is safe for __del__ (fsspec closed defaults to True); the unused executor is not shut down in that case, but ThreadPoolExecutor starts no threads before a submit. info() does not forward **kwargs, so its conditional pop is safe. Write/append with version_id behaves as with the ?versionId= suffix (pre-existing, unchanged).
Validation: just lint; tests/pyathena/filesystem/ 253 passed against AWS at 40f8c4c.

[laughingman7743]

Repair: the info() change was reverted in dffe1e5 per the maintainer, since #957 (#932) owns explicit-version caching; #958 now depends on #957. Finding 1 (S3File size from the versioned lookup) remains in this PR. The "cached parent listing" scenario in finding 2 is not produced by current code: _ls_dirs caches listings under (path, delimiter) tuple keys.

[laughingman7743]

Self-review round 2 (claims, callers, operations) — CLEAN

Scope: full git diff e0e85da09435257a18c8b8832b3e10ba7840b697..40f8c4ca3a059e741e73345eb05281b151d23ce3, PR body, both commit messages, changed docstrings/comments, docs/filesystem.md versioning section.

Claims checked:

• get_file() accepts version_id through open() (s3.py get_file → self.open(rpath, "rb", **kwargs)): offline run at head sends VersionId=v1 on HeadObject and GetObject.
• AioS3FileSystem.cat_file() delegates to the sync cat_file(): offline run at head sends VersionId=v1.
• fsspec base initializer reads the size from info(self.path) (fsspec 2026.9.0 AbstractBufferedFile.__init__/details), so the reorder in S3File.__init__ is needed; open now calls info() once (asserted in test_open_version_id).
• info() suffix-form directory/FileNotFoundError and kwarg-form stale/poisoned cache claims: reproduced offline on the pre-repair code; test_info_version_id fails there (6/6).
• docs/filesystem.md:139-140 ("Explicit versions can always be read with the ?versionId= suffix or the version_id argument") is now true for open()/cat_file()/get_file(); no other docs mention the argument.
• Test counts in the PR body (12 -k version_id, 253 filesystem against AWS) re-run at this head.

Callers/operations: unversioned info() lookups are unchanged; versioned lookups now issue one HeadObject per version (cached under the version-qualified key) instead of reading latest-version cache entries, which were wrong. Version entries are immutable, and deleting a version via rm(path?versionId=) already invalidates that key. Read-mode open drops one info() call.
Limits: the CI bucket is unversioned, so reading an older version with a different size is covered by unit tests only; the integration test reads the "null" version and checks that S3 rejects an unknown one.

[laughingman7743]

Independent review (relayed) — Codex CLI 0.160.0, model gpt-6-astra, reasoning effort high, sandbox read-only, session 01a10067-f3d3-75c1-8e58-0382a9615c1c. Static review only (no tests/builds/network). Snapshot: detached worktree at 40f8c4c, diff e0e85da09435257a18c8b8832b3e10ba7840b697..40f8c4ca; prompt omitted PR number/body, commit messages and prior findings; snapshot unchanged afterwards.

Reviewer result (verbatim):

Covered the full diff, sync/async open, cat_file and get_file, fsspec initialization and serialization, read/write/append modes, version pinning, cache invalidation, listings, docstrings, and added tests. Static review only; no tests, builds, network access, or modifications.

FINDINGS

Introduced regression

1. P2 — Version-qualified metadata survives object overwrite. s3.py:651
   In an unversioned bucket, call info(path, version_id="null"), then overwrite that object through pipe_file(path, new_data). The new cache key is path?versionId=null, but invalidation removes only the plain path and ancestors. Subsequent versioned info returns the old size/ETag; buffered reads can fail their stale IfMatch, and ranged cat_file can calculate incorrect offsets. Previously, the keyword lookup cached under the plain path and was invalidated. Invalidate affected version entries, particularly the mutable "null" version.

Pre-existing implementation defects exposed by the newly accepted keyword

2. P2 — Append selects versioned metadata but reads latest-version content. s3.py:2250
   Given historical v1 = b"old" and latest v2 = b"new", open(path, "ab", version_id="v1") followed by write(b"!") writes b"new!". Metadata uses v1, but cat(self.path) has neither the suffix nor the argument. Multipart append likewise copies the unqualified path at s3.py:2294. Reject version arguments for append or propagate the selected version to every source read/copy. The underlying omission predates this diff; filesystem open previously rejected this keyword.

3. P2 — Pickling loses the selected version. s3.py:2228
   version_id is absent from both the stored path and the kwargs passed to fsspec. Its inherited __reduce__ reconstructs the handle using those fields. With equally sized historical and current objects, pickle/unpickle of open(path, "rb", version_id="v1") therefore silently reads the current object. Preserve version identity in reconstruction. This omission already affected directly constructed S3File instances; the diff exposes it through filesystem open.

The added tests do not cover these scenarios. The integration test reads a single "null" version, and the mocked open tests verify metadata selection without reading distinct historical/latest payloads.

---

Author verification and disposition:

1. Confirmed (the null version is mutable in unversioned/suspended buckets). Version caching in info() belongs to Cache each explicit version of an object separately in info() #957 (S3FileSystem.info(version_id=...) returns cached metadata of another version #932), which does not cache null; per the maintainer, the overlapping info() change was reverted here (dffe1e5) and Accept version_id in S3FileSystem.open() and cat_file() #958 depends on Cache each explicit version of an object separately in info() #957.
2. Confirmed: the argument form left self.path unversioned, so cat(self.path) / copy_source=self.path used the latest version. Per the maintainer, S3File now rejects a version (either form) for wb/ab/xb (bbf3759, this line). Release-noted as breaking.
3. Confirmed: fsspec __reduce__ reopens with self.path and self.kwargs. S3File now carries the argument version in the path (bbf3759); test_open_version_id asserts f.path and __reduce__() carry it. The version pinned by version_aware is still not carried (pre-existing, unchanged).

[laughingman7743]

Self-review round 1 (repair 40f8c4ca..bbf3759dee3bf1f4f9dea4fe7a26084972c06b07) — CLEAN

Covered: the revert (dffe1e5 restores info() to master; git diff e0e85da09435257a18c8b8832b3e10ba7840b697..bbf3759dee3bf1f4f9dea4fe7a26084972c06b07 no longer touches info()), and S3File.__init__ (bbf3759): rejection runs before any request (asserted with _call raising); self.version_id at that point comes only from the argument or path (version_aware pinning is later and read-only); the folded path feeds info(), fsspec self.path, cat/sign/metadata/getxattr, all of which already accept the suffix form; put_file()/get_file() route through open().
Tests: test_open_version_id_for_writing (6 cases) fails fast without the change (DID NOT RAISE / unexpected request); test_open_version_id and the aio test fail without it (path lacks the version).
Validation: just lint; -k version_id 12 passed at bbf3759; tests/pyathena/filesystem/ 253 passed against AWS before the last test-only tweak (_call.side_effect).

[laughingman7743]

Self-review round 2 (repair, claims) — CLEAN

Claims checked: pipe_file(), touch() and copy reject versioned targets (s3.py:1318, touch, _copy: "Cannot copy to a versioned file."); suffix-form append started from that version (cat(self.path) and CopySource="bucket/key?versionId=..." passed through unchanged by _upload_part_copy); fsspec 2026.9.0 AbstractBufferedFile.__reduce__ → reopen(fs, self.path, ..., self.kwargs).
PR body corrected: info() bullets and release note removed; breaking release note for versioned writes added; dependency on #957 stated (master's info() returns a directory on the second lookup of a ?versionId= path and caches a kwarg version under the plain path).
Correction to round 1 above: the "cached parent listing" case does not occur with current code, because _ls_dirs caches listings under (path, delimiter) keys (pointed out by the #957 session).

[laughingman7743]

Independent follow-up review (relayed) — Codex CLI 0.160.0, model gpt-6-astra, effort high, sandbox read-only, session 01a1007f-1822-7323-9838-18948f7c5926. Static review only. Snapshot: detached worktree at bbf3759 (unchanged afterwards); full diff e0e85da0..bbf3759d plus repair diff 40f8c4ca..bbf3759d.

Reviewer result (verbatim):

Surfaces covered: full and repair diffs; sync/aio argument forwarding; version precedence and mismatches; read/write/append/xb modes; text wrapping; version_aware pinning; size initialization; pickle reopening; put_file/get_file; transaction commit/rollback; changed docstrings, comments, and tests.

CLEAN. No introduced defects found by static inspection.

All three repairs are verified:

1. info() caching changes are absent from the full diff; existing caching behavior was excluded.
2. Explicit versions, supplied by argument or suffix, are rejected for wb, ab, and xb before metadata or write requests.
3. Argument versions are preserved in S3File.path, which fsspec’s pickle reopening mechanism uses.

Test coverage remains limited: the pickle assertion checks the serialized path without reopening; range tests use positive bounds that do not exercise version-dependent size; the integration test reads the sole "null" version rather than distinct historical content. These limit regression evidence but do not establish an implementation defect.

No files changed, tests/builds run, or network access performed.

---

Author disposition: no action. The coverage limits match those stated in the PR body (the CI bucket is unversioned); the cat_file unit test asserts that the size lookup carries the version, which is the version-dependent part of a range read.