Allow ? in keys and address versions in listings, files and copies

[laughingman7743]

WHAT

Stacked on #1052 (S3Path); the base branch is refactor/1049-s3-path.

1. Keys containing ?. S3Path.PATTERN treats only a version ID query at the end of a path (?versionId=, ?versionID=, ?versionid=, ?version_id=, with a non-empty ID without ?) as a version, and any other ? as part of the key. The new S3Path.split_version_id() applies the same rule to any string, such as the names fsspec builds. invalidate_cache() now uses it in place of splitting at the first ?. info(), exists(), open(), cat(), rm(recursive=True) and the other methods accept keys such as dir/what?.txt, which ls()/find() already returned.
2. Version paths are not globs.
   • S3FileSystem.expand_path() and AioS3FileSystem._expand_path() return a path with a version as given. With recursive, the path is kept only if the version exists (a key prefix of the same name does not count, see 5; lookup errors other than a missing object are raised), and nothing below it is expanded.
   • copy() and get() (sync, plus aio _copy()/_get()) pair sources that have a version with destinations named after the key without the version, through the new _copy_paths().
   • _move_paths() (mv()) uses the same pairing. Other sources still go through fsspec's own copy()/get().
3. Pinned versions of files. With version_aware=True, S3File carries the version it pins at open time in its path (bucket/key?versionId=<id>), as an explicit version_id already does since Accept version_id in S3FileSystem.open() and cat_file() #958. metadata(), getxattr() and url() of the file therefore describe and sign that version, and an unpickled file reads it again.
4. Listed versions. ls(versions=True) names each version bucket/key?versionId=<id> in both the strings and the name of the detail=True entries, except for the null version, which keeps the plain key. This is the same as s3fs's _fill_info(). The names can be passed back to open()/info()/copy().
5. Missing versions. info() no longer falls back to the key-prefix lookup when HeadObject does not find an explicitly requested version. A version names an object, so the path does not exist.
6. fsspec>=2026.9.0. The requirement is raised so that get()/_get() check the destinations they name with fsspec's own check_contained(), which fsspec 2026.9.0 added, and the onerror workaround for fsspec < 2026.6.0 in cp_file()/_cp_file() is removed. S3Path.has_version_id() (public) tells whether a path string ends with a version ID query.
7. docs/filesystem.md describes the version rule, the pinned path, the listing names and versioned copies.

Release notes (4.0.0):

• PyAthena requires fsspec>=2026.9.0 (it had no lower bound).
• Keys containing ? can be used. A path such as s3://bucket/key?foo=bar, which used to raise ValueError: Invalid S3 path format, is now the key key?foo=bar. A key that itself ends with a version ID query, such as key?versionId=x, is still read as a version.
• ls(versions=True) returns version-qualified names (bucket/key?versionId=<id>, except for null) instead of the plain key once per version.
• With version_aware=True, S3File.path of a file opened for reading includes ?versionId=<pinned id> when S3 reports a version, and metadata(), getxattr() and url() use that version instead of the latest.
• expand_path(), copy(), mv() and get() accept a single path with a version ID, which used to fail with FileNotFoundError after a ListObjectsV2 request.
• info() of a path with a version that does not exist raises FileNotFoundError (and exists() returns False) even when its key is also a key prefix, which used to be reported as a directory. The ListObjectsV2 request that checked for the prefix is no longer sent for a missing version.

When get() pairs the paths itself (a source with a version and a str or PathLike lpath; a sequence of destinations is left to fsspec), it checks that every destination it names lies under lpath, with fsspec's check_contained(), as fsspec's get() does for the destinations it names. A key such as a/.. therefore raises ValueError. Sources without a version keep fsspec's pairing unchanged. rm() keeps deleting versioned paths as given, without a lookup.

AioS3FileSystem._expand_path() passes the paths without a version to fsspec's async _expand_path(), so their globs keep listing only the keys under the stem before the first wildcard (prefix=). A source list of aio _copy()/_get() that contains a version path is paired by the synchronous _copy_paths(), whose globs list the directory of the stem, as S3FileSystem does.

WHY

Closes #979, including the case in its comment (version paths globbed by expand_path()/copy()/mv()/get()).

Maintainer decisions on 2026-10-04: s3fs naming for the listed versions, the pinned version carried in S3File.path rather than new version_id arguments on metadata()/getxattr()/sign(), and the copy/get/mv pairing fixed in this PR.

TEST

Tested commit 491e567, on master after #1052 was merged (77892e8).

• just lint: passed. just docs lint: 0 errors.
• New offline tests in tests/pyathena/filesystem/:
  • test_question_mark_keys: listing, info(), exists() and recursive rm() of ? keys.
  • test_invalidate_cache_question_mark_key.
  • test_expand_path_version and test_expand_path_recursive_missing_version.
  • test_copy_version: copy()/mv() to a key, to a directory, from a list, and with a ? key.
  • test_get_version: to a file, a directory and from a list.
  • test_open_version_aware_pins_version_in_path: the requests of metadata()/getxattr()/url() carry VersionId.
  • aio test_copy_version, test_get_version and test_get_version_path_destination.
  • test_info_missing_version_is_not_a_prefix (path query and version_id=); test_info_version_spellings_share_cache now expects no ListObjectsV2 after a missing version.
  • TestS3Path.test_has_version_id (strings and Paths) replaces the test of the removed module helper.
  • Added after the independent follow-up: test_expand_path_recursive_version_lookup_error (a PermissionError is raised, not taken for a missing version) and a tuple-destination case of test_get_version.
• uv lock only records the new specifier; the locked fsspec stays 2026.9.0, which CI uses. CI has no lowest-version job.
  • Added after the independent review: test_get_version_outside_destination, test_has_version_id (Path sources), the Path cases of test_get_version, aio test_expand_path_glob_lists_stem_prefix, a stronger test_expand_path_recursive_missing_version (a key prefix of the same name is not a version), and parser cases for newlines and bucket/?versionId=.
  • The updated test_ls_versions cases, test_parse_path_invalid (sync and aio), and S3Path parse and split_version_id cases.
• Measured before the fix, offline with mocked S3 responses: overriding only expand_path() made copy()/mv() raise Cannot copy to a versioned file (the version ended up in the destination), and made get() write the local file f.txt/b?versionId=v1. The final pairing copies to out, d/b, d/b.
• Offline run of tests/pyathena/filesystem/ (--noconftest, dummy credentials): the same 118 AWS-dependent tests fail as on the base branch, and all other tests pass.
• Live AWS run: uv run --env-file .env pytest -n 4 tests/pyathena/filesystem/ → 717 passed on 491e567 (and on b3b7d2f; 715 on a16f063, 713 on 43ab8f9, and 698 on 460c4cb before the review repairs). This includes the new test_question_mark_keys_and_null_version, which writes, lists, reads and deletes a real what?.txt key, and copies and downloads its ?versionId=null path on the unversioned CI bucket.
• Not covered live: versions other than null, because the CI bucket is unversioned and the CI role lacks s3:DeleteObjectVersion. Those cases are covered offline.

🤖 Generated with Claude Code

[laughingman7743]

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

Base 1dc7a6b (#1052 head), head 460c4cb.

Covered:

• S3Path.PATTERN/split_version_id(): a trailing query only, an empty ID, an ID containing ?, and strings that are not S3 paths.
• expand_path(): fsspec's own recursion re-enters it with lists and assume_literal in kwargs, which are forwarded. The FileNotFoundError is the same as fsspec's when nothing matches. recursive makes one lookup per version.
• _copy_paths() (here) against fsspec 2026.9.0 copy()/get()/_copy()/_get(): the non-recursive directory filter, exists (fsspec's has_magic except for a version), other_paths on names without the version, make_path_posix and the local isdir for get(). Sources without a version still take fsspec's path, including check_contained.
• mv() through _move_paths(); the aio mirrors; rm()'s versioned paths (unchanged, no lookup).
• invalidate_cache() for ? keys and for versions.
• The S3File version pin: only when version_aware and S3 reports a version, appended to the path given to the base class, as Accept version_id in S3FileSystem.open() and cat_file() #958's explicit version is.
• ls(versions=True) names, including null.

Tests: each new offline test fails on the base. The base raises ValueError for ? keys and FileNotFoundError for version paths, metadata() sends no VersionId, and ls() returns plain names. The live test exercises a real ? key and ?versionId=null.

No findings.

[laughingman7743]

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

The repair is 460c4cb..43ab8f9. The merge-base with master is still 1dc7a6b, now that #1052 is merged as 77892e8.

• Implementation:
  • PATTERN with re.DOTALL/\Z and the bare / first: re-checked against every earlier parse case (bucket, bucket/, bucket//, trailing-slash keys, ? keys, ?versionId= empty, two queries, every spelling).
  • _check_contained(): the root itself is allowed, which is the single-file case.
  • aio _expand_path(): fsspec's recursion re-enters it with assume_literal, which is forwarded. _isfile() swallows lookup errors as fsspec's does.
  • _split_version_paths() is shared by sync and aio.
  • isfile() replaces exists() for version paths.
  • get() pairs any non-list lpath after make_path_posix(), which accepts PathLike.
• Claims: the PR body no longer says that aio delegates to the sync expansion. It describes the containment check (ValueError for a/.., replacing the earlier measured IsADirectoryError note), the isfile rule and the prefix= behavior, and records the evidence on 43ab8f9: just lint passed, the offline failures are the same 118 AWS-dependent tests plus the new live test, and the live run passed 713 tests.

[laughingman7743]

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

Same base and head as round one.

Finding: the PR body said a destination of a versioned source "cannot leave lpath". For a key whose last segment is .. (a/..?versionId=v1), the destination is lpath/... I measured it offline: get() raises IsADirectoryError and writes nothing, so the conclusion holds but the wording was wrong. Repaired by rewording the PR body to the measured behavior. No code change.

Claims checked:

• The pinned path follows Accept version_id in S3FileSystem.open() and cat_file() #958: git log -S finds the explicit-version carry in bbf3759, which is part of Accept version_id in S3FileSystem.open() and cat_file() #958.
• On the base, get() and copy()/mv()/expand_path() of a version path raise FileNotFoundError (measured offline on 1dc7a6b). On the branch they copy to out, d/b and d/b.
• ls(versions=True) naming matches s3fs 2026.9.0 _fill_info() (read in the s3fs wheel).
• The release note about s3://bucket/key?foo=bar is covered by the updated test_parse_path_invalid (sync and aio).
• Existing callers: copy/get keep fsspec's signatures and their on_error, callback and batch_size handling, because the paired lists are passed to super(). Only the version-aware S3File.path and the listing names change visibly, and both are in the release notes.
• Evidence: just lint passed; the offline failures are the same 118 AWS-dependent tests as on the base; the live pytest -n 4 tests/pyathena/filesystem/ run passed 698 tests on this head. Versions other than null were not run live (unversioned CI bucket).

[laughingman7743]

Independent review (relayed): FINDINGS (1 P1, 9 P2).

Reviewer: Codex CLI 0.160.0, model gpt-6-astra, codex exec -s read-only (session 01a1044e-66d8-7770-9b75-e897c4e723fe). This was a static review that read the local fsspec 2026.9.0 sources, on a detached snapshot of head 460c4cb, base 1dc7a6b; the snapshot was unchanged afterward. The prompt had the diff and the intended behavior, without the PR text or the self-review records. Repairs are in 43ab8f9. Per-finding verdicts are in this thread and the threads below.

Finding 1 (P1), anchored here: the generated pairs entered fsspec's explicit-list branch of get(), which skips check_contained(). On Windows, a mixed list such as [r"bucket/..\victim.txt", "bucket/ok?versionId=v1"] could write above lpath. Confirmed. Repaired: get() and aio _get() check every destination they name against lpath (_check_contained(), the same rule as fsspec's). test_get_version_outside_destination covers a/..?versionId=v1 and a mixed list on POSIX.

Finding 4 (P2): a version-free Path source raised TypeError in copy()/get() because _has_version_id() iterated it. Confirmed; repaired (os.PathLike and os.fspath), see test_has_version_id.

Finding 5 (P2): a Path destination of aio _get() (and sync get()) skipped the pairing, so the file went to out.bin/key?versionId=v1. Confirmed; repaired by pairing any non-list lpath after make_path_posix(), see aio test_get_version_path_destination and the Path cases of test_get_version.

[laughingman7743]

Independent follow-up 1 (relayed): FINDINGS (3 P2).

Reviewer: Codex CLI 0.160.0, model gpt-6-astra, codex exec -s read-only (session 01a10464-6390-7661-bcad-4c6dbcb1e432). Static review of the repair 460c4cb..43ab8f9 on the detached snapshot, which was unchanged afterward. It confirmed that original findings 1-4 and the scenario of 9 are resolved, that 5's Path case is fixed, and that the rejections of 7, 8 and 10 are sound.

New findings and dispositions; the repairs are in b3b7d2f:

1. isfile()/_isfile() swallow every exception, so a PermissionError on one version path silently dropped it from a recursive expansion. Confirmed. Repaired by using exists()/_exists() again. Since a16f063, info() no longer takes a key prefix for a missing version, so the prefix case stays excluded and other lookup errors are raised. Test: test_expand_path_recursive_version_lookup_error.
2. An aio _copy()/_get() source list that mixes a version path with a glob is paired by the synchronous _copy_paths(), so its glob lists the directory of the stem rather than prefix=. Not changed: that is how S3FileSystem.copy()/get() expand the same list, and it matters only for such a mixed list under a policy that allows listing only the stem prefix. Unversioned aio expansions keep fsspec's async prefix= (finding 6). The PR body states this.
3. A tuple lpath raised TypeError in _check_contained(). Confirmed. Repaired: get()/_get() pair only a str or PathLike lpath, and a sequence is left to fsspec as before. Test: a tuple case of test_get_version.

Both perspectives on the repair: CLEAN. The new tests fail on 43ab8f9, where the expansion returns only a and abspath(tuple) fails. just lint passed, the offline failures are the same AWS-dependent set, and the live pytest -n 4 tests/pyathena/filesystem/ run passed 717 tests on b3b7d2f.

[laughingman7743]

Independent follow-up 2 (relayed): FINDINGS (1).

Reviewer: Codex CLI 0.160.0, model gpt-6-astra, codex exec -s read-only (session 01a10470-7e65-7f13-a7c0-ce3ba33b1561). Static review of the patch 43ab8f9..b3b7d2f on the detached snapshot, which was unchanged afterward.

Coverage reported: the info() change and its callers (exists, isdir, _copy_file/cp_file, open/S3File, cat_file, expand_path, mv/_move_paths, get_file), the sync and async get destination pairing, and the updated test expectations. It confirmed that follow-up 1's findings 1 and 3 are repaired.

Finding: with exists(), a recursive expansion keeps a version-qualified bucket path (s3://bucket/?versionId=x) when the bucket exists, and a copy of it then fails with ValueError("Cannot copy buckets.").

Not changed. A non-recursive expansion keeps the same path as given and reaches the same explicit ValueError, so the two modes agree. A version on a bucket path names nothing, and exists() on a bucket path checks only the bucket, as before. Nothing is copied or overwritten. The review found no other actionable regression.

[laughingman7743]

Maintainer-requested change: fsspec>=2026.9.0 and S3Path.has_version_id(), in 491e567. Self-review from both perspectives: CLEAN.

• Implementation:
  • S3Path.has_version_id(path) takes a single str/PathLike. The "path or list" convention stays in the fsspec-facing copy/get/_copy/_get, which apply any() to their sources.
  • The removed module helper _check_contained() is replaced by fsspec.utils.check_contained(). I checked the published fsspec wheels: that function is absent in 2025.12.0 through 2026.7.0 (2026.5.0 and 2026.8.0 could not be downloaded) and present in 2026.9.0, hence the new fsspec>=2026.9.0. Its ValueError names the escaping path, and test_get_version_outside_destination still matches outside.
  • The kwargs.pop("onerror") in cp_file()/_cp_file() only served fsspec < 2026.6.0's mv(). PyAthena's own mv()/_mv() never pass it, so the pop is removed.
• Claims: the PR body gains a release note (fsspec>=2026.9.0, which had no lower bound) and describes the change. uv lock changes only the recorded specifier; the locked fsspec was already 2026.9.0. Evidence on 491e567: just lint passed, the offline failures are the same AWS-dependent set, and the live pytest -n 4 tests/pyathena/filesystem/ run passed 717 tests.

[laughingman7743]

Independent follow-up 3 (relayed): CLEAN.

Reviewer: Codex CLI 0.160.0, model gpt-6-astra, codex exec -s read-only (session 01a1048a-c3e3-7141-b30d-8ef993703ed1). This was a static review of the patch b3b7d2f..491e567 on the detached snapshot, which was unchanged afterward.

Coverage reported:

• Sync and aio copy/get/mv, with their sync wrappers, versioned and unversioned sources, destination mapping, and kwargs and error propagation. No supported move path generates the removed onerror.
• S3Path.has_version_id() with the callers' any() preserves the previous predicate.
• fsspec 2026.9.0 check_contained(): correct (root, paths) arguments, and equivalent normalization, root equality, separator boundaries and ValueError; only the message wording differs.
• The dependency requirement, the lock metadata, the tests and the docs: no remaining dependence on older fsspec.

No files were changed, nothing was built or tested, and there was no network access.

[laughingman7743]

Independent review (relayed), findings 2 and 3 (P2): confirmed and repaired.

• Finding 2: .+? excluded newlines. bucket/a\nb raised ValueError, and bucket/a\n parsed as key a because $ matched before the final newline. Both keys parsed intact on the base. The pattern now uses re.DOTALL and \Z.
• Finding 3: bucket/?versionId=v1 parsed as the key ?versionId=v1, whereas the base read it as a version of the bucket path. A bare / is now tried before a key.

Both are covered by new TestS3Path.test_parse cases (a newline inside, at the end, and before a version).

[laughingman7743]

Independent review (relayed), finding 6 (P2): confirmed and repaired. Delegating aio _expand_path() to the sync expand_path() lost fsspec's async glob prefix= (the stem before the first wildcard), so bucket/reports/2026-*.csv listed reports/ instead of reports/2026-.

The aio override now splits off the version paths with the shared _split_version_paths() and passes the others to fsspec's async _expand_path(). test_expand_path_glob_lists_stem_prefix asserts prefix="2026-".

[laughingman7743]

Independent review (relayed), finding 9 (P2, pre-existing cause): with only dir/child present, a recursive expansion of bucket/dir?versionId=missing kept the path, because exists() → info() falls back to the prefix lookup.

Repaired for the expansion: a version path is kept only if isfile(). test_expand_path_recursive_missing_version now covers this, and it fails on the base.

The underlying info() behavior predates this PR and is not changed here: a version path whose object is missing is reported as a directory when the key is also a prefix (s3.py prefix fallback in info(), outside the diff).

Finding 10 (P2, pre-existing): expand_path("bucket/key?versionId=v/") drops the trailing / of the version ID. Not changed: fsspec's _strip_protocol() does this for every operation on the base, and the review notes the same.

[laughingman7743]

Follow-up at the maintainer's request: the pre-existing info() cause is fixed in this PR as well, in a16f063. Self-review from both perspectives: CLEAN.

• Implementation: when HeadObject does not find an explicitly requested version, info() now raises FileNotFoundError instead of falling back to the ListObjectsV2 key-prefix check. This covers both the path query and the version_id= argument; version_id is the merged value. The callers follow:
  • exists() returns False.
  • isdir() returns False.
  • _copy_file() raises FileNotFoundError instead of skipping the path as a directory, which copy(on_error=...) handles.
  • open() raised FileNotFoundError for a directory already, so it is unchanged.
  • cat_file()'s negative-offset lookup raises as before.
    Version paths that are found, and paths without a version, take the same requests as before.
• Claims: the PR body adds the release note (FileNotFoundError/False, and one ListObjectsV2 fewer for a missing version). test_info_version_spellings_share_cache now expects 3 requests instead of 5, because the two prefix checks are gone. test_info_missing_version_is_not_a_prefix fails on the previous head, where info() returned a directory. Evidence on a16f063: just lint passed, the offline failures are the same AWS-dependent set, and the live pytest -n 4 tests/pyathena/filesystem/ passed 715 tests.

[laughingman7743]

Independent review (relayed), findings 7 and 8 (P2): not changed, with these reasons.

• Finding 7: copying ["bucket/a/?versionId=v1", "bucket/b/?versionId=v2"] into bucket/out/ names both destinations bucket/out/, because fsspec's flattened basename of a trailing-slash key is empty. Flattening a list already maps sources with the same basename onto one destination in fsspec's copy() (for example ["bucket/a/x", "bucket/b/x"] → bucket/out/x twice), and mv() rejects such collisions in _move_paths(). Versions of directory-marker keys follow the same rule.
• Finding 8: copying bucket/a?versionId=x?versionId=v1 (key a?versionId=x) into a directory names the destination bucket/out/a?versionId=x, which _copy_file() rejects with ValueError: Cannot copy to a versioned file. A key that itself ends with a version query cannot be named as a destination path, as the PR body and docs state, and the error is explicit; nothing is overwritten.