Follow fsspec's get_file() contract for directories and file objects - #990
Conversation
| if os.path.isdir(lpath): | ||
| if outfile is None and isfilelike(lpath): | ||
| outfile = lpath | ||
| elif outfile is None and self.isdir(rpath): |
There was a problem hiding this comment.
Self-review round one (behavior and implementation)
Base aa0fc91, head fa93630.
Covered: S3FileSystem.get_file() and cp_file(), AioS3FileSystem._cp_file() and its delegated _get_file(), fsspec 2026.9.0 get()/_get()/copy()/_copy()/mv() callers, and the new offline and live tests.
Result: FINDINGS (one simplification, see the test helper thread).
Checked without findings:
- Directory detection costs no request for files:
isdir(rpath)heads the object and caches it, andS3File.__init__reuses the cached entry (s3.py:2297). Mocked count: 1 HeadObject + 1 GetObject, as on master. cp_file()skips only wheninfo()reports a directory, i.e. no object at the key. A key that is both an object and a prefix is still copied as an object.- A missing object now raises from
open()before the local file is created, so no empty local file is left behind. - Concurrent
_get()entries rely onos.makedirs(..., exist_ok=True), which tolerates races. ExitStackcloses the local file and the remote file on errors.isfilelikeis infsspec.utilsof every fsspec release this code otherwise supports (the module already needsfsspec.callbacks._DEFAULT_CALLBACK).
Limitation, not changed: get_file(rpath, lpath, version_id=...) checks isdir(rpath) on the latest version, so it sends one more HeadObject than before. AbstractFileSystem.isdir() takes no version, and fsspec's get() does not pass version_id, so I kept the fsspec flow.
| ) | ||
|
|
||
| @staticmethod | ||
| def _directory_object(key): |
There was a problem hiding this comment.
Self-review round one finding (simplicity): this helper duplicates the static S3FileSystem._directory_object() (pyathena/filesystem/s3.py:279) that info() returns for prefixes, and the async test builds the same entry inline. Using the production builder keeps the fake directory entry identical to the real one.
There was a problem hiding this comment.
Repaired in 957fbf9: the sync tests use fs._directory_object("bucket", "dir") and the async test fs._sync_fs._directory_object("bucket", "dir"); the test helper is removed. just lint passes, and the 5 offline tests pass.
| return | ||
|
|
||
| with open(lpath, "wb") as local, self.open(rpath, "rb", **kwargs) as remote: | ||
| with ExitStack() as stack: |
There was a problem hiding this comment.
Self-review round two (claims, callers, operational effects)
Base aa0fc91, head 957fbf9.
Result: FINDINGS (one missing test, two PR-body wording corrections), repaired.
Claims checked against mocked S3 responses on this branch and on master:
- Master
mv(recursive=True):S3FileSystemraisesNoSuchKey: srcwith nothing moved;AioS3FileSystemraises after copyingdst/aanddst/sub/b, keepingsrc/*. Confirmed. - Master recursive
get()raisesNotADirectoryError. Confirmed. - A file
rpathinto an existing local directory: master returns with no error and no data; this branch raisesIsADirectoryError. Confirmed. The PR body said this only affects directget_file()calls, but a recursiveget()also reaches it when a local directory already exists where a remote file goes. Body corrected. - A missing object: master leaves an empty local file; this branch raises
FileNotFoundErrorand creates nothing. Confirmed, but no test pinned it. Finding. - The PR body said fsspec strips the trailing slash from expanded paths. In fact PyAthena's
expand_path()returns directory paths without one (bucket/src,bucket/src/sub). Body corrected. - Request counts in the PR table come from mocked responses, not AWS traffic, and the table says so.
isfilelikeand_DEFAULT_CALLBACKboth exist in fsspec 2023.9.0 and 2023.12.0. Checked withuv run --with fsspec==....- Callers: the positional order
(rpath, lpath, callback, outfile)is unchanged, andAioS3FileSystem._get_file()forwardsoutfilethrough**kwargs.docs/filesystem.mdanddocs/aio.mdsay nothing aboutget()/copy()/mv()directory handling, so no docs need changing. - AWS: no retry or request change for files; directory entries in
cp_file()stop sending a failingCopyObject.
There was a problem hiding this comment.
Repaired in 0032114: test_get_file_missing checks that get_file() raises FileNotFoundError for a missing object and creates no local file. It fails with the pyathena/ changes reverted (6 of 6 offline tests fail, 6 pass with them). The two PR-body claims are corrected in the description.
| raise ValueError("Cannot copy buckets.") | ||
|
|
||
| info1 = self.info(path1) | ||
| if info1.get("type") == S3ObjectType.S3_OBJECT_TYPE_DIRECTORY: |
There was a problem hiding this comment.
Independent review (relayed): Codex CLI 0.160.0, model gpt-6-astra, reasoning effort max, codex exec -s read-only --ephemeral, session 01a100cf-455a-77a1-b98b-a5df09930e21.
Base aa0fc91, head 0032114, clean detached snapshot; given the literal diff, AGENTS.md and the fsspec 2026.9.0 sources, without the PR text, commit messages or prior findings. Static review: no tests run. Snapshot and PR worktree verified unchanged afterwards.
Covered (reviewer's words): sync/async downloads and copies; fsspec 2026.9.0 get_file/get/_get/copy/_copy/mv callers; metadata, versioning, request costs, resource ownership, and the added regression tests.
Result: FINDINGS (3).
Finding 1 (P1), relayed: Recursive moves into a descendant delete both source and destination. In an unversioned bucket containing src/a, fs.mv("bucket/src", "bucket/src/archive", recursive=True): the directory skip now lets the copy succeed, then the inherited mv() recursively removes src, whose listing now includes src/archive/a, so both are deleted. Before, copying the directory raised before removal. The inherited algorithm is pre-existing, but this diff newly enables the outcome (also s3_async.py:258). Reviewer suggestion: reject descendant destinations before moving.
Author verification: CONFIRMED with mocked S3 responses: on this head both objects are gone; on master the call raises NoSuchKey: src and keeps src/a. The glob form mv("bucket/src/*", "bucket/src/archive/", recursive=True) already deletes both on master, so the hazard is fsspec's mv() (copy, then re-expand and remove) and pre-exists; this PR makes the non-glob form reach it.
There was a problem hiding this comment.
Deferred to a maintainer decision, not changed here. A guard means overriding fsspec's mv(), e.g. rejecting a destination under the source, and that design choice is the maintainer's. The same destructive result already happens on master with the glob form mv("bucket/src/*", "bucket/src/archive/", recursive=True) (verified with mocked responses), so the hazard is pre-existing in the mv() algorithm; this PR makes the non-glob form reach it. The PR body records it under "Not changed: mv() into a descendant".
There was a problem hiding this comment.
Maintainer decision (relayed by the coordinator): guard it in this PR. Done in e17a8cf and f7b8fc3: S3FileSystem.mv() and AioS3FileSystem.mv() raise ValueError before any copy or removal when a recursive move would copy the source onto itself or into a path under it. See the self-review records on the mv() lines and the PR body section "Guarding mv() into the source".
| if os.path.isdir(lpath): | ||
| if outfile is None and isfilelike(lpath): | ||
| outfile = lpath | ||
| elif outfile is None and self.isdir(rpath): |
There was a problem hiding this comment.
Finding 2 (P2), relayed: Directory detection ignores the requested object version. If key has a readable version v1, its current version is deleted, and key/child exists, get_file("bucket/key", "/tmp/out", version_id="v1") takes the current prefix as a directory, creates /tmp/out and downloads nothing. Before, open() received version_id and downloaded v1. Async _get_file() inherits it.
Finding 3 (P2), relayed: With S3FileSystem(use_listings_cache=False), downloading an object sends one HeadObject through isdir() and another in S3File initialization; before, one. Reviewer suggestion: reuse the metadata obtained for directory detection.
Author verification: Finding 2 CONFIRMED by code: AbstractFileSystem.isdir() calls info(rpath) without the version. Finding 3 CONFIRMED with mocked responses: 2 HeadObject + 1 GetObject on this head, 1 + 1 on master.
There was a problem hiding this comment.
Repaired in dfd0588: get_file() opens rpath first and takes the type from remote.details, the info() lookup that open() makes with the requested version_id. A bucket rpath, which S3File rejects for having no key, is still checked with isdir(). This was found while checking the repair: with only open-first, a recursive get("s3://bucket", ...) raised ValueError. test_get_file_directory now covers the bucket case.
Self-review of the repair, behavior: with mocked responses, get_file() of a file sends 1 HeadObject + 1 GetObject with or without use_listings_cache=False (0032114 sent 2 HeadObject without the cache). Recursive get() without the cache: 5 HeadObject (0032114: 7). A directory rpath with a file-like lpath writes nothing and fetches nothing, as fsspec does. ExitStack closes the opened directory S3File on the early return. New test_get_file_version_id asserts a single info("bucket/key?versionId=v1", version_id="v1"); it fails on 0032114.
Self-review of the repair, claims: the PR body detection section and table now describe the open-first lookup. The earlier round-one statement that isdir() heads and caches the object no longer applies. just lint passes; offline tests: 8 passed; live: full tests/pyathena/filesystem/ 286 passed, and the targeted get/copy/move tests 33 passed on dfd0588.
There was a problem hiding this comment.
Update: 6e4da7e replaces the dfd0588 repair after the independent follow-up review (path-like rpath and eager cache_type regressions). Finding 2 stays fixed: get_file() skips isdir() when version_id is passed. Finding 3 is deferred as a documented limitation: with use_listings_cache=False, a download sends 2 HeadObject, as fsspec's default get_file() would; it is 1 with the default cache.
| if os.path.isdir(lpath): | ||
| if outfile is None and isfilelike(lpath): | ||
| outfile = lpath | ||
| elif outfile is None and not self.parse_path(rpath)[1] and self.isdir(rpath): |
There was a problem hiding this comment.
Independent follow-up review (relayed): Codex CLI 0.160.0, model gpt-6-astra, reasoning effort max, codex exec -s read-only --ephemeral, session 01a100e3-91da-7943-997b-eff187671021.
Scope: the repair 0032114..dfd0588 (merge-base aa0fc91), clean detached snapshot, with the affected get_file() contracts listed and no author conclusions. Static review: no tests run. Snapshot and PR worktree verified unchanged afterwards.
Covered (reviewer's words): repair and tests; fsspec 2026.9.0 sync/async callers; directories and whole buckets; parent creation; output ownership; both version-ID forms; request counts with caching enabled/disabled; resource cleanup.
Result: FINDINGS (2). The reviewer judged the version-specific lookup and the removal of the extra per-file lookup sound.
Finding 1 (P2), relayed: get_file(Path("bucket/key"), "/tmp/out") raises TypeError, because parse_path() passes the Path straight to a regex. master and 0032114 reached fsspec's normalization through open()/info(). Sync get() and async _get() pass such inputs unchanged when both arguments are lists. Introduced by the repair.
Author verification: CONFIRMED by code (parse_path() gets the raw rpath).
There was a problem hiding this comment.
Verified empirically on the dfd0588 snapshot with mocked responses: get_file(Path("bucket/src/a"), ...) raises TypeError: expected string or bytes-like object, got 'PosixPath', and get("s3://bucket/src", ..., recursive=True, cache_type="all") raises NoSuchKey: src.
Repaired in 6e4da7e, which replaces the open-first lookup instead of patching it. get_file() goes back to the isdir(rpath) check of fsspec's get_file(), and skips it when version_id is passed, because a version always names an object (Finding 2 of the first review stays fixed). isdir() normalizes path-like inputs through info(), handles buckets, and runs before any S3File or cache exists. Both scenarios above now succeed.
Trade-off: with use_listings_cache=False, a download sends 2 HeadObject again (Finding 3 of the first review). fsspec's default get_file() makes the same two lookups, and with the default cache it is still 1. The PR body records this as a limitation.
Self-review of the repair. Behavior: test_get_file_version_id still asserts a single info("bucket/key?versionId=v1", version_id="v1") and fails on 0032114; the bucket case of test_get_file_directory passes. Claims: the PR body detection section, table and TEST section now match 6e4da7e. Validation: just lint passes; offline tests: 8 passed; live targeted get/copy/move tests: 33 passed on 6e4da7e. The full live filesystem suite is running on 6e4da7e; its result goes in the PR body.
| if outfile is None: | ||
| # The type comes from the info() that open() looked up for | ||
| # the requested version, so no other request is needed. | ||
| if remote.details.get("type") == S3ObjectType.S3_OBJECT_TYPE_DIRECTORY: |
There was a problem hiding this comment.
Finding 2 (P2), relayed: With bucket/dir/file and no object named dir, get_file("s3://bucket/dir", out, cache_type="all") fails before this check: fsspec's AllBytes constructor (fsspec/caching.py:694) fetches (0, 0) at once, so it sends GetObject for the missing directory key. Recursive sync and async downloads with this cache fail. 0032114 created the directory without opening it. The eager fetch is older than the repair, but the repair exposes directory downloads to it.
Author verification: CONFIRMED by code (S3File construction for a directory rpath runs the cache constructor).
There was a problem hiding this comment.
Repaired in 6e4da7e (see the reply on the first thread of this review): the directory check runs through isdir() before open(), so no S3File or cache is constructed for a directory rpath. get("s3://bucket/src", out, recursive=True, cache_type="all") now downloads out/a and out/sub/b with mocked responses.
| if os.path.isdir(lpath): | ||
| if outfile is None and isfilelike(lpath): | ||
| outfile = lpath | ||
| elif outfile is None and not kwargs.get("version_id") and self.isdir(rpath): |
There was a problem hiding this comment.
Independent follow-up review 2 (relayed): Codex CLI 0.160.0, model gpt-6-astra, reasoning effort max, codex exec -s read-only --ephemeral, session 01a100ef-9d18-7ef3-8863-434ecdc2d2a0.
Scope: the net repair 0032114..6e4da7e (merge-base aa0fc91), clean detached snapshot, with the affected get_file() contracts listed and no author conclusions. Static review: no tests run. Snapshot and PR worktree verified unchanged afterwards.
Result: no new defect in the repair; FINDINGS are pre-existing only. Reviewer's result, verbatim apart from shortened local paths:
Surfaces covered: the net repair and tests; fsspec 2026.9.0 get_file()/get(); AioS3FileSystem._get_file() → _get(); version resolution, path-like and bucket paths, caches, resource cleanup, and request counts.
FINDINGS — pre-existing issues only. No new defect found in the net repair. The keyword-version repair is sound. Directory creation, parent creation, path normalization, and ExitStack cleanup are sound; caller-owned output streams remain open.
-
P2 — Some cache types issue invalid empty-range requests. s3.py:2592: downloading a four-byte object with
cache_type="first"reaches EOF and calls_fetch_range(4, 4), producingRange: bytes=4-3.cache_type="all"similarly requestsbytes=0--1when opening an empty object._fetch_range()needs to handle empty ranges. Present in master and the previously reviewed head. -
P2 —
outfilealone cannot be used. s3.py:1549:fs.get_file("bucket/key", outfile=buffer)raisesTypeErrorbecauselpathremains mandatory; fsspec defaults it toNone. Supplying a dummylpathworks. Present in master and the previously reviewed head. -
P2 — Bulk callers can still skip an explicitly requested historical version. fsspec/spec.py:1045, fsspec/asyn.py:781: if version
v1exists but the latest key is deleted andkey/childexists, scalarget(..., version_id="v1")and_get(...)classify the latest path as a directory and return before calling the repaired method. Pre-existing caller behavior. -
P2 — Bulk expansion interprets path-qualified versions as globs. fsspec/spec.py:1259, fsspec/asyn.py:1050: scalar
get("bucket/key?versionId=v1", ...)treats?as a wildcard and ordinarily raisesFileNotFoundErrorduring expansion. Directget_file()handles the version correctly. Pre-existing caller behavior.
Request counts against supplied master base aa0fc914, for an ordinary unversioned object, cold metadata cache, local pathname destination, and no retries:
| Call | Listing cache | Master | Repaired |
|---|---|---|---|
get_file() |
Default | 1 HEAD + G GETs | Same |
get_file() |
Disabled | 1 HEAD + G GETs | 2 HEADs + G GETs |
Scalar nonrecursive get() / _get() |
Default | 1 HEAD + G GETs | Same |
Scalar nonrecursive get() / _get() |
Disabled | 2 HEADs + G GETs | 3 HEADs + G GETs |
G is unchanged; with default block size, workers, and byte cache, it is ceil(size / 5 MiB). The extra HEAD predates this net repair. Direct keyword-version downloads now use one version-specific HEAD with either listing-cache setting, matching master. Recursive traversal adds separate listing/directory requests.
Static inspection only; no tests, builds, writes, or network access. HEAD remains 6e4da7e9, with a clean worktree.
Author disposition: all four items are pre-existing and left out of this PR. (1) The empty-range requests from cache_type="first"/"all" are range handling in S3File._fetch_range(), the area of #970. (2) outfile without lpath is a signature gap: get_file()/_get_file() require lpath, while fsspec defaults it to None. (3) and (4) come from fsspec's get()/_get() expansion around versions, before get_file() is reached. The extra HEAD with use_listings_cache=False is the trade-off recorded in the PR body.
6e4da7e to
3f64905
Compare
| for destination in destinations: | ||
| parts = cls._strip_protocol(destination).split("/") | ||
| if any( | ||
| cls._matches_path_or_parent(pattern, candidate) |
There was a problem hiding this comment.
Self-review of the mv() guard, perspective one (behavior and implementation), for the maintainer decision relayed by the coordinator.
Base 6f25850 (merge-base of the rebased branch), repair range 3f64905..f7b8fc3 (e17a8cf + f7b8fc3).
Result: FINDINGS (one, repaired in f7b8fc3).
- fsspec 2026.9.0:
AbstractFileSystem.mv()copies withon_error="raise", thenrm(path1, recursive=recursive).AsyncFileSystemhas no_mv(), soAioS3FileSysteminherits the same syncmv(), andmove()/rename()callmv(). Both classes overridemv()only. - Finding (repaired): e17a8cf guarded only destinations at or under the source. An ad hoc live run on S3 showed that
mv(".../src/sub", ".../src", recursive=True)also deletes the data. fsspec copies a source into an existing directory under its base name, sosrc/sub/awas copied onto itself, and the removal left nothing. On master, the directory entry's failingCopyObjectstopped it, so this PR newly enables it as well. f7b8fc3 also checks the destination joined with the source's last segment. That coversmv("bucket/data/*.csv", "bucket/data/", recursive=True), which already deleted the files on master. - The check sends no request.
_strip_protocol()normalizess3:///s3a://, trailing slashes and path-like inputs (checked withPurePosixPath). Segment comparison keepsbucket/src2outsidebucket/src. - Still allowed (tested): the sibling prefix
src2,data/*.csvtodata/archive/,a/b/ctoa, and non-recursive moves. Conservative rejections, documented in the PR body: the contents movesrc/sub/tosrc/, which loses a nestedsrc/sub/sub/xwithout the guard (verified with mocked responses on 3f64905), and rare layouts such asa/*/x.csvtoa/b. - Behavior change:
mv(p, p, recursive=True)now raises instead of fsspec's debug no-op. Release-noted. - Tests: the rejection cases assert that neither
copy()norrm()is called. With mocked_call, an unguardedmv()would loop in pagination. The allowed cases assert delegation withon_error="raise". All 12 rejection cases fail without the guard.
| self.invalidate_cache(path) | ||
| return object_.to_dict() | ||
|
|
||
| def mv(self, path1, path2, recursive=False, maxdepth=None, **kwargs) -> None: |
There was a problem hiding this comment.
Self-review of the mv() guard, perspective two (claims, callers, operations), same range 3f64905..f7b8fc3.
Result: CLEAN after the PR-body corrections listed here.
- Claim: the plain forms raised
NoSuchKeybefore this PR, and the glob forms deleted the files on master. Checked with mocked responses on master forsrc/subtosrc(FileNotFoundError,src/sub/akept),data/*.csvtodata/(both files gone) andsrc/*tosrc/archive/(gone). - Claim:
fsspec.utils.glob_translateonly exists since fsspec 2023.12, and PyAthena imports with fsspec 2023.1. Checked withuv run --with fsspec==2023.9.0/2023.12.0/2024.2.0and an import of this branch on 2023.1.0/2023.6.0/2023.9.0. So the guard uses stdlibfnmatch.fnmatchcaseper segment and does not raise the dependency floor. - Claim: no S3 requests. The check is pure string work before
super().mv(). - Callers: the signature matches fsspec's
mv(path1, path2, recursive=False, maxdepth=None, **kwargs), and lists forpath1/path2are checked element by element.docs/filesystem.mdanddocs/aio.mddo not describemv()semantics, so no docs change. - Evidence: offline 17
mv()tests and 8get_file/cp_filetests pass. Full livetests/pyathena/filesystem/: 328 passed on f7b8fc3. Live ad hoc: thesrc/subtosrcmove raisedValueErrorand kept the object. - PR body: added the release-note item (new
ValueError, including the same-path case) and the section "Guardingmv()into the source", replacing the earlier "Not changed" section.
| for destination in destinations: | ||
| parts = cls._strip_protocol(destination).split("/") | ||
| if any( | ||
| cls._matches_path_or_parent(pattern, candidate) |
There was a problem hiding this comment.
Independent review of the mv() guard (relayed): Codex CLI 0.160.0, model gpt-6-astra, reasoning effort max, codex exec -s read-only --ephemeral, session 01a1011f-e09a-73a1-9b52-1002ba369a80.
Scope: 3f64905..f7b8fc3 (e17a8cf + f7b8fc3), clean detached snapshot, with the required contract stated and no author conclusions. Static review: no tests run. Snapshot and PR worktree verified unchanged afterwards.
Result: FINDINGS. Reviewer's result, verbatim apart from shortened local paths:
Surfaces covered: plain paths, trailing slashes, s3:///s3a://, bucket boundaries, glob expansion including ** and character classes, lists, path-like inputs, destination mapping, removal, async mirroring, aliases, and added tests.
FINDINGS
-
P1 — The guard misses destinations created by copying descendants. s3.py:1222, shared by s3_async.py:251.
Checking only the destination and destination plus source basename does not cover fsspec’s
other_paths()mapping. These concrete cases pass the guard withrecursive=True:Source → destination Existing object → copied object bucket/p/a/a→bucket/pbucket/p/a/a/a/f→bucket/p/a/a/fbucket/p/x/y/→bucket/p/bucket/p/x/y/x/y/f→bucket/p/x/y/fbucket/p/[ab]→bucket/p/bucket/p/a/b/f→bucket/p/b/fbucket/p/*/leaf→bucket/p/bucket/p/a/leaf/b/leaf/f→bucket/p/b/leaf/fEach row can start with only the stated object. Copying succeeds to a different key; subsequent recursive removal selects both the original and its copy. The character-class case also exposes that appending literal
[ab]does not represent an actual matching basename.This affects both filesystems. It is an incomplete guard against data loss already possible at
3f64905f, rather than newly introduced data loss. -
P2 — Exact same-argument moves now raise instead of remaining harmless no-ops. s3.py:1191, also the async override.
mv("bucket/src", "bucket/src", recursive=True)now raises. Installed fsspec checkspath1 == path2and performs neither copy nor removal. The new check unnecessarily rejects this provably safe case. -
P2 — Explicit destination lists are incorrectly treated as directories. s3.py:1215.
With an object at
bucket/dir/file,mv("bucket/dir/file", ["bucket/dir"], recursive=True)is safe: fsspec uses the list entry literally, copies to object keydir, and removes onlydir/file. The guard instead inventsbucket/dir/fileby appending the basename and rejects it. This regression affects both filesystems. -
P2 —
fnmatchcase()incorrectly lets a standalone*match an empty path component. s3.py:1255.Given objects
bucket/p/aandbucket/p/b, movingbucket/p/*tobucket/p//archive/recursively is safe: copies land beneath the empty component, which fsspec’s glob does not match. The guard rejects it becausefnmatchcase("", "*")succeeds. Installed fsspec explicitly requires a nonempty component for standalone*; interior repeated slashes survive normalization. -
P2 — Delegation tests assume a minimum fsspec version that PyAthena does not require. test_s3.py:1146, test_s3_async.py:237.
These assertions require
on_error="raise". With the oldermv()contract explicitly accommodated by the existing compatibility shim, delegation instead passesonerror="raise", so the new tests fail despite correct superclass delegation. This is a new test compatibility defect.
Two additional data-loss cases are pre-existing at 3f64905f, not introduced by this addition:
- P1 — Async list-to-list directory moves can delete everything without copying files. At s3_async.py:279,
mv(["bucket/src"], ["bucket/dst"], recursive=True)skips the source directory because fsspec bypasses expansion when both arguments are lists._rm()then expands and deletessrc/f. The new guard still permits this. The sync counterpart instead encounters its existingparse_path(list)failure. - P1 — Depth-limited moves remove uncopied deeper files. fsspec/spec.py:1301 omits
maxdepthwhen removing. With onlybucket/src/sub/f, movingbucket/srctobucket/dstwithrecursive=True, maxdepth=1copies no files and then deletesf.
The guard itself makes no S3 requests and correctly handles sibling-prefix boundaries and protocol normalization. move() and rename() dynamically call self.mv() in both classes; async mirroring does not replace the override, and there is no _mv. Path-like values normalize in the guard, but downstream sync rm() and async expansion already have path-like limitations.
The rejection tests would fail without the guard and assert observable behavior: ValueError before either mocked operation. They do not exercise actual destination expansion or explicitly assert zero S3 calls. Conservative rejection of some parent moves is justified because nested descendants can map back beneath the source.
Static review only; no tests or builds run. HEAD remains f7b8fc387eb593e81d1dc1c51db85e9a16d9f408, with a clean working tree.
Author verification: with mocked responses on f7b8fc3, every row of finding 1 and both cases the reviewer labels pre-existing ended with no objects left, on both filesystems (the sync list-to-list case fails with TypeError first). Relative to master, the maxdepth and list-to-list cases are not pre-existing: on master the directory entry's failing CopyObject stopped them before the removal, so this PR's directory skip newly enables them. Findings 2-5 were confirmed by code.
There was a problem hiding this comment.
Repaired in 68f6499.
- (P1) A static check cannot predict where fsspec's
other_paths()places each copy, so the guard now rejects any recursive move whose destination overlaps the source in either direction: the destination is, lies under, or holds a path that the source can match, compared per segment. When neither holds, every copy (at or under the destination) is outside every path the removal expands. All four rows now raise before copying (checked with mocked responses on both filesystems). The basename join and the literal[ab]problem are gone with it. - Kept fsspec's no-op: the check runs only when
path1 != path2;test_mv_not_overlappingcoversmv(p, p, recursive=True). - A destination list holding an ancestor of the source is still rejected, now as an overlap (ancestor destinations are rejected in general). Recorded as a conservative rejection in the PR body.
- A segment must be non-empty to match a wildcard pattern segment;
p/*top//archive/is allowed and tested. - The delegation tests check the positional paths and
recursive, not the name of theon_errorkeyword.
Newly enabled relative to master, also guarded now:maxdepthwithrecursive=Trueraises (fsspec's removal ignores it), and so does a list-to-list recursive move (lists are copied unexpanded but removed expanded).
Validation on 68f6499: just lint passes; offline 25 mv() tests pass, and all 19 rejection cases fail on 3f64905; full live tests/pyathena/filesystem/ 336 passed; live ad hoc src/sub to src raised and kept the object. Both self-review perspectives on this repair follow on the new mv() lines; an independent follow-up comes after them.
| for source in sources: | ||
| pattern = cls._strip_protocol(source).split("/") | ||
| for destination in destinations: | ||
| if cls._overlaps(pattern, cls._strip_protocol(destination).split("/")): |
There was a problem hiding this comment.
Self-review of the guard rework, perspective one (behavior), range f7b8fc3..68f6499 (base 6f25850).
Result: CLEAN.
- Completeness: every copy fsspec makes for a string destination is at or under it (
other_paths()joins ontopath2), and list-to-list moves are rejected. The removal deletes the paths that the source matches when it re-expands it after copying, with their contents. So a copy can be removed only if the destination and one of those paths are on one line of ancestry, which_overlaps()checks in both directions. Segment matching accepts whatever fsspec'sglob()can match, so it errs only towards rejecting. - Codex rows
p/a/a→p,p/x/y/→p/,p/[ab]→p/andp/*/leaf→p/raise, as domaxdepth=1and["src"]→["dst"], on both filesystems with mocked responses. Allowed moves still complete:src→src2,data/*.csv→data/archive/, and the livetest_move_recursive(base/src→base/dst). path1 != path2keeps fsspec's no-op for identical arguments, including identical lists. Normalized-equal spellings (s3://b/srcvss3a://b/src/) are still rejected, because fsspec would copy onto the source and then remove it.- The empty-segment rule
(bool(parts[0]) or not pattern[0])lets only an empty pattern segment match an empty path segment. - No request is made before
super().mv(), and the async override is a single delegating call. - Tests: the 19 rejection cases assert that
copy()/rm()are not called, and all fail on 3f64905. The allowed cases assert delegation without depending on theon_errorkeyword name.
| self.invalidate_cache(path) | ||
| return object_.to_dict() | ||
|
|
||
| def mv(self, path1, path2, recursive=False, maxdepth=None, **kwargs) -> None: |
There was a problem hiding this comment.
Self-review of the guard rework, perspective two (claims, callers, operations), same range.
Result: CLEAN after the PR-body rewrite.
- PR body: the release note now lists the three rejection reasons and the identical-argument no-op. The "Guarding
mv()" section has a results table, measured with mocked responses except the livesrc/sub→srcrow, and lists the conservative rejections with the full-destination workaround. Theglob_translatefloor claim is unchanged and was checked earlier. - Callers:
move()/rename()reach these overrides;AsyncFileSystemmirrors no_mv(). The rejected combinations did not work for directory sources on master either: they failed withNoSuchKey, or the async filesystem copied the files and then raised. Glob or file sources withmaxdepthor list-to-list did run on master and are newly rejected; the release note says so. - Docs:
docs/filesystem.mdanddocs/aio.mddo not describemv(). - Operations: no added S3 requests. The live filesystem suite passed (336) on 68f6499, and the ad hoc live
src/sub→srcmove raised and kept the object.
68f6499 to
156d460
Compare
| os.makedirs(lpath, exist_ok=True) | ||
| return | ||
|
|
||
| # The remote file is opened first so that no local file is created |
There was a problem hiding this comment.
Rebase record and self-review of the integration: 68f6499 rebased onto master 4950996 (after #986 rm(), #987 ranges/open(), #988 aio parity, #999), new head 156d460.
Result: FINDINGS (one contract conflict, resolved as below).
Perspective one (behavior):
- Conflicts:
get_file()(Clamp byte ranges to the object in cat_file() and S3File reads #987 had already moved the remoteopen()before the local file; kept, with its comment, around this PR'sExitStack), imports (fnmatch,io.BytesIO), and test positions intest_s3_async.py(both sides kept). - Contract conflict: Clamp byte ranges to the object in cat_file() and S3File reads #987's
test_get_file_directorypinnedget_file()of a prefix to raiseFileNotFoundErrorand leave no local file. Recursive get(), copy() and mv() mishandle directory entries #974 requires fsspec's behavior: a directoryrpathcreates the local directory, which recursiveget()relies on. Resolved by keeping the Recursive get(), copy() and mv() mishandle directory entries #974 behavior and removing Clamp byte ranges to the object in cat_file() and S3File reads #987's test in 156d460. Its intent, that no empty file is written for a prefix, still holds: this PR'stest_get_file_directoryasserts a directory and no requests. A prefix with a file-likelpath/outfilenow raisesFileNotFoundErrorfrom Clamp byte ranges to the object in cat_file() and S3File reads #987'sopen()check instead of writing nothing. - With Fix rm() bulk deletes in S3FileSystem and AioS3FileSystem #986's
rm(), a sync list-to-list recursive move now also deletes data without the guard (mocked), so the guard's list rejection covers both filesystems. Non-recursive list moves work on both. AioS3FileSystem._get_file()still delegates to the syncget_file(), and the Align AioS3FileSystem with S3FileSystem in transactions, open() and touch() #988 transaction changes do not touchmv()/cp_file().
Perspective two (claims):
- PR body: the release note about no empty local file for a missing object is now Clamp byte ranges to the object in cat_file() and S3File reads #987's, and was replaced by a note that this PR supersedes Clamp byte ranges to the object in cat_file() and S3File reads #987's
get_file(prefix)behavior. Removed the stale sync-listTypeErrorremarks. Tested commit and counts updated. - Evidence on 156d460:
just lintpasses; offline 33 tests pass, 25 fail on master 4950996 (the 8 that pass are explained in the PR body); full livetests/pyathena/filesystem/424 passed; the ad hoc livesrc/sub→srcmove raised and kept the object.
| raise ValueError("Cannot copy buckets.") | ||
|
|
||
| info1 = self.info(path1) | ||
| if info1.get("type") == S3ObjectType.S3_OBJECT_TYPE_DIRECTORY: |
There was a problem hiding this comment.
Independent follow-up review of the full rebased PR (relayed): Codex CLI 0.160.0, model gpt-6-astra, reasoning effort max, codex exec -s read-only --ephemeral, session 01a1013b-99f9-7823-aee3-2a675268b7d0.
Scope: merge-base 4950996..156d460 (full diff, since the guard was reworked and the branch rebased over #986/#987/#988), clean detached snapshot, with the intended contracts stated and no author conclusions. Static review: no tests run. Snapshot and PR worktree verified unchanged afterwards.
Result: FINDINGS. Reviewer's result, verbatim apart from shortened local paths:
FINDINGS
Surfaces covered: both filesystem implementations and changed tests; fsspec 2026.9.0’s copy mapping, glob translation, expansion, move, and download paths; PyAthena’s info(), find(), open(), deletion expansion, and cache invalidation. Static review only; no tests, imports, or network operations were run.
-
P1 — Bracket patterns can bypass the overlap guard. s3.py:1394
Withbucket/p/qzx/fpresent, this passes the guard:fs.mv("bucket/p/q[!a]x/**", "bucket/p/q/x/archive", recursive=True)
fsspec translates
[!a]into[^a], which can match/. Consequently, removal matches the newbucket/p/q/x/archive/fas well as the original. Copying invalidates the shared listing cache, so removal discovers and deletes both. The segment-wise guard misses that match. This affects both implementations and becomes destructive with the new directory skip. -
P1 — List-to-string moves can overwrite copies before deleting their originals. s3.py:1356
Supposesrc/aandsrc/sub/acontain different bytes:fs.mv(["bucket/src"], "bucket/dst", recursive=True)
fsspec uses
other_paths(..., flatten=True)for list sources, mapping both files todst/a. One copy overwrites the other, then removal deletes both originals. The guard permits this. Flattening is pre-existing; directory skipping newly allows this directory-source scenario to complete destructively. -
P1 — Real objects ending in
/can be skipped and then deleted. s3.py:1440, s3_async.py:343
Store nonempty data atbucket/src/a/, whichpipe_file()supports, then movebucket/srcrecursively tobucket/dst.find()lists the actual object, butinfo()strips its trailing slash and reportssrc/aas a directory. The new branch skips its copy. Removal retains the original listed key and deletessrc/a/. This introduces data loss even for disjoint, plain source and destination paths. -
P2 — Missing explicit versions can silently become successful directory operations. s3.py:1859
Ifbucket/key/childexists but versionv1ofkeydoes not,get_file("bucket/key?versionId=v1", local)creates a local directory and returns successfully. The bypass checks only the keyword argument;info()falls back to the existing prefix after the version lookup fails. The rebased implementation raisedFileNotFoundError, and the keyword-version form still does. Async downloads inherit this regression. The new copy-directory branches likewise suppress the corresponding missing-version copy error.
The added tests assert useful observable behavior: downloaded bytes, directory creation, stream ownership, and prevention of copy/removal calls. They do not cover these four scenarios. The integration moves use distinct filenames and plain string paths. test_get_file_missing already succeeds against the base: it opened the remote first. Comments describing prefixes becoming empty files also describe older behavior; the rebased open() already rejected prefixes.
Pre-existing limitations remain separate: bucket copying raises before the directory skip, and deletion of a scalar PathLike still fails at s3.py:957. These were not introduced by this change.
Deliberate conservative rejections, not defects, are ancestor/descendant overlaps, glob-overlapping destinations, any recursive maxdepth, and non-identical list-to-list recursive moves. Identical arguments retain the no-op. The guard itself makes no S3 requests.
The snapshot remains clean at 156d460c42e230e514793a89ac7dd5278f871fa2.
Author verification: findings 1-3 were confirmed with mocked responses on 156d460. (1) glob_translate("bucket/p/q[!a]x/**") matches bucket/p/q/x/archive/f. (2) src/a was lost to the overwrite. (3) The src/a/ object's data was lost. Finding 4 was confirmed by code: isdir() gets the versioned path and info() falls back to the prefix. Finding 3 is a regression from the directory skip: master copied src/a/ as an object.
There was a problem hiding this comment.
Repaired in c6782a5.
- In the overlap check, a glob segment with
[...]matches any number of segments, as**does, because fsspec's character classes can match/.p/q[!a]x/**→p/q/x/archiveis rejected (test case added). - Any list source is rejected for recursive moves: a list to a list copies directories unexpanded, and a list to a single destination flattens onto base names.
["src"]→dstis rejected in both filesystems; non-recursive list moves are unchanged and tested. cp_file()/_cp_file()skip a directory only when the key does not end with/.expand_path()lists bothbucket/src/a(the prefix) andbucket/src/a/(the object); only the first is skipped. Live on S3:mv(".../src", ".../dst", recursive=True)with asrc/a/object holdingDATAmoved it todst/a/with its data.get_file()skipsisdir()for a version in the path as well as inversion_id, andcp_file()/_cp_file()never skip a source with a version. A missing version raises fromopen()/CopyObject instead of turning into a directory.
Also removedtest_get_file_missing, since master already opens the remote first, and reworded thetest_get_file_directorycomment, which described pre-Clamp byte ranges to the object in cat_file() and S3File reads #987 behavior.
Self-review of the repair, behavior: 8 new or extended test cases fail on 156d460 and pass on c6782a5; 41 offline tests pass. Mocked end-to-end checks of all earlier cases still give the same results: the guard rejects them, allowed moves complete, and recursive get() and the cache_type="all" and Path cases still work. Full live tests/pyathena/filesystem/: 432 passed on c6782a5.
Self-review of the repair, claims: the PR body now covers the list-source rejection, the bracket rule, the /-key and version exceptions in cp_file(), both version forms in get_file(), the new table rows, and recounted tests. The claim that these tests keep master's behavior was checked: the 6 version//-key cases pass on master 4950996 and fail on 156d460.
get_file() wrote a directory rpath as an empty local file, did not create the parent directories of lpath, rejected a file-like lpath and ignored outfile, so a recursive get() failed. A directory or bucket rpath now creates the local directory, the parent directories of a local file are created, and a file-like lpath or outfile is written to and left open. A requested version, as an argument or in the path, always names an object, so it skips the directory check. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
c6782a5 to
2a68d34
Compare
laughingman7743
changed the title
| """ | ||
| if os.path.isdir(lpath): | ||
| _, _, path_version_id = self.parse_path(self._strip_protocol(rpath)) | ||
| if outfile is None and isfilelike(lpath): |
There was a problem hiding this comment.
Self-review round one after the rewrite to get_file() only (implementation behavior): CLEAN
Base 28826be7d37983cb54050e849b32a4fe2331ea7d, head 2a68d3481253291aef5089c1d1c8e56f59d9da83. Full pass over git diff 28826be7d37983cb54050e849b32a4fe2331ea7d..2a68d3481253291aef5089c1d1c8e56f59d9da83 (2 files). The earlier review threads on this PR refer to the previous head c6782a50, which included cp_file() and mv(). That part moved to #1008.
Covered:
- fsspec contract: the branching matches fsspec 2026.9.0
AbstractFileSystem.get_file()(spec.py:981-1007):- a file-like
lpathbecomesoutfile; - the directory check is skipped when
outfileis given; makedirs(lpath)is called for a directory;- the parent directories of a local
lpathare created; - an
outfilefrom the caller is not closed.
- a file-like
- Intentional differences from fsspec:
- The remote file is opened before the local file and its parents are created, so a missing object leaves nothing behind (master's ordering, kept).
- A requested version skips
isdir().
- Failure paths:
- A missing
rpathmakesisdir()returnFalse(OSErroris caught), andopen()then raisesFileNotFoundErrorbefore any local file or parent directory is created. - A file
rpathwith an existing local directory aslpathraisesIsADirectoryError(release-noted).
- A missing
- Callers:
AioS3FileSystem._get_file()passesrpath,lpathand**kwargs(includingoutfile) to the syncget_file().- fsspec's sync
get()and async_get()callget_file()once for each expanded path, directories included.
- Requests: a file
rpathadds anisdir()→info()lookup. When the listings cache is on, the HeadObject result is cached and reused byopen(). When it is off, a download sends 2 HeadObject requests, as fsspec's ownget_file()does. - Tests: 4 of the 6 new offline cases fail on master. The 2
test_get_file_version_idcases guard the new version skip and pass on master. Master'stest_get_file_directory, which pinnedFileNotFoundError, is replaced.
Pre-existing and out of scope: lpath stays a required argument, while fsspec defaults it to None for outfile-only calls.
| # A requested version always names an object, while isdir() | ||
| # would look up the latest version, or the prefix of the same | ||
| # name when the version does not exist. | ||
| os.makedirs(lpath, exist_ok=True) |
There was a problem hiding this comment.
Self-review round two (claims and operational behavior): FINDINGS (PR description only, corrected)
Base 28826be7d37983cb54050e849b32a4fe2331ea7d, head 2a68d3481253291aef5089c1d1c8e56f59d9da83. Full claim pass over the PR body, the commit message and the changed docstring.
- "A directory rpath creates the local directory instead of raising FileNotFoundError": master's
test_get_file_directoryassertedFileNotFoundErrorfor a directoryrpath; this PR replaces that test. - "A file rpath with an existing local directory as lpath now raises IsADirectoryError; before, it returned without downloading": master's
get_file()returned early onos.path.isdir(lpath). The new code reachesopen(lpath, "wb"). - Corrected: the draft said async recursive
get()worked on master because_get()"creates the directories itself". In factAsyncFileSystem._get()creates only the parent directory of each target (fsspec/asyn.py:808), and master'sisdir(lpath)early return then skipped the directory targets that already existed. The PR body now states this. - "Item 2 moved to Recursive copy() and mv() send CopyObject for directories, and mv() can delete overlapping copies #1008": Recursive copy() and mv() send CopyObject for directories, and mv() can delete overlapping copies #1008 records the live S3 measurements on master 28826be that I ran for this split:
- files-only
mv("src/*", "src/archive/", recursive=True)andmv("data/*.csv", "data/", recursive=True)delete all data; - plain directory
mv()raisesFileNotFoundError.
- files-only
- Evidence scope: the 441 passed, including the live S3 tests, are from a local run on
2a68d348. AWS CI runs only after Ready. - AWS operator:
test_get_recursivecreates 2 small objects under a unique prefix and does not delete them; many live filesystem tests do the same, and the CI bucket expires objects after 1 day (cloudformation/github_actions_oidc.yaml:329-333).
…lone os.path.abspath() resolves ".." before symlinks, so a path such as "link/../out/key" created the parent next to the symlink instead of the one that open() writes to. Use the parent of lpath as given. lpath now defaults to None as in fsspec, so get_file() accepts outfile alone. Test that a missing object leaves nothing behind, and give the directory test the cache type that open() needs. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| if os.path.isdir(lpath): | ||
| _, _, path_version_id = self.parse_path(self._strip_protocol(rpath)) | ||
| if outfile is None and isfilelike(lpath): | ||
| outfile = lpath |
There was a problem hiding this comment.
Independent review (relayed): FINDINGS
- Reviewer: Codex CLI 0.160.0, model
gpt-6-astra, reasoning effortmax, run with--sandbox read-only --ephemeral, session01a1016c-9dab-7160-994a-725258653bd1. - Scope: base
28826be7d37983cb54050e849b32a4fe2331ea7d, head2a68d3481253291aef5089c1d1c8e56f59d9da83, in a clean detached snapshot. - Package: the literal diff and the intended behavior, plus read-only access to the installed fsspec 2026.9.0 sources. The PR number, description, commit messages and self-review findings were not included.
- Constraints: static review only. The reviewer made no edits and ran no tests, network or GitHub calls. The snapshot and the PR worktree were unchanged afterwards.
Reviewer output (verbatim):
Covered surfaces: the exact diff, metadata/cache/version handling, stream ownership and failure paths, sync get(), async _get(), AioS3FileSystem._get_file(), installed fsspec 2026.9.0, tests, and docstring conventions. Static review only; no tests or network access.
FINDINGS
-
P2 — Introduced regression: parent creation can target the wrong directory. pyathena/filesystem/s3.py:1853
abspath()collapses..lexically, whereasopen(lpath)resolves symlinks first. Suppose/a/linkpoints to/b/sub,/b/outis a directory, and/a/outis a regular file. Downloading to/a/link/../out/datapreviously succeeded at/b/out/data; the newmakedirs("/a/out")raisesFileExistsError. Preserve the original path’s symlink/..semantics when creating its parent. -
P2 — Pre-existing contract gap remains:
outfilealone is rejected. pyathena/filesystem/s3.py:1817
fs.get_file("s3://bucket/key", outfile=stream)raisesTypeErrorbecauselpathremains mandatory. Installed fsspec declareslpath=None, allowing this call. The newly implementedoutfilesupport still requires a dummy destination argument. DefaultlpathtoNoneand cover the omitted-argument call. This is retained incompatibility, not a regression.
Other review results:
- Requests: For unversioned local destinations, caching normally avoids an additional HEAD. Disabled caching adds one HEAD; an uncached missing key repeats the HEAD/ListObjectsV2 lookup. Explicit versions and supplied streams bypass the new directory probe.
- Failures: Remote-open failures precede local creation.
ExitStackcloses owned handles while leaving supplied streams open. Async_get()already creates parents before delegation, so its missing-object failures can leave directories independently of this change. - Tests: Parent-directory, stream, and recursive tests assert observable fixes and would fail the base. Version tests also pass the base; they preserve existing behavior. Missing-object and exception-cleanup behavior lacks direct coverage. The directory test’s baseline failure is additionally affected by its fake filesystem lacking
default_cache_type. - Documentation/conventions: The docstring otherwise describes the implemented branches accurately, uses Google style, and introduces no import-placement or name-mangling violations.
There was a problem hiding this comment.
Repair in a87a59e, with both self-review perspectives applied
Patch series 2a68d348..a87a59e2. Base unchanged (28826be7d37983cb54050e849b32a4fe2331ea7d).
-
Finding 1 (P2, introduced regression): verified and fixed. The scenario reproduced locally: a symlink
a/link → b/sub, a directoryb/out, a regular filea/out, andlpath = a/link/../out/data.makedirs(dirname(abspath(lpath)))raisedFileExistsError.makedirs(dirname(lpath))letopen()writeb/out/data.
get_file()now createsos.path.dirname(lpath), skipping the call when the parent is empty, as for a bare file name. New test:test_get_file_parent_through_symlink. -
Finding 2 (P2, pre-existing): folded in as a contained fix.
lpathnow defaults toNoneas in fsspec 2026.9.0, and the docstring says it is not needed withoutfile. Covered intest_get_file_file_like. -
Test notes:
test_get_file_directorynow setsdefault_cache_type, so on master it fails becauseopen()raisesFileNotFoundError, not because of a missing fixture attribute.- New
test_get_file_missingpins that a missing object leaves no local file or parent directory.
Round one (behavior):
- A
PathLikelpathworks withos.path.dirname(). lpath=NonewithoutoutfileraisesTypeError, as in fsspec, and only afteropen()has checked that the remote object exists.AioS3FileSystem._get_file()is unchanged and still passesoutfilethrough**kwargs.
Round two (claims):
- The code comment "abspath() would resolve
..before symlinks" matchesposixpath.abspath(), which normalizes lexically. - The PR body now records the symlink handling, the
lpath=Nonedefault, the tested commit a87a59e, and the new test results.
Validation on a87a59e:
just lintpassed.uv run --env-file .env pytest -n 2 -p no:cacheprovider -q tests/pyathena/filesystem/gave 443 passed.- The new and changed offline tests give 8 passed. With master's
get_file(), 5 fail; with 2a68d34's, 2 fail (symlink andoutfile-only).
A Codex follow-up on 2a68d348..a87a59e2 follows.
There was a problem hiding this comment.
Independent follow-up on the repair (relayed): CLEAN
- Reviewer: Codex CLI 0.160.0, model
gpt-6-astra, reasoning effortmax, run with--sandbox read-only --ephemeral, session01a10177-b108-7c53-91d9-42705ab40353. - Scope: patch series
2a68d3481253291aef5089c1d1c8e56f59d9da83..a87a59e2, base28826be7d37983cb54050e849b32a4fe2331ea7d. - Constraints: static review only. The snapshot and the PR worktree were unchanged afterwards.
- Note: the reviewer labels finding 1 as pre-existing. It was introduced by this PR's first head, 2a68d34, relative to master; master never created parent directories.
Reviewer output (verbatim):
Covered surfaces: the exact repair, fsspec 2026.9.0 compatibility, parent creation, relative paths, PathLike inputs, bare filenames, lpath=None, stream ownership, missing objects, and regression tests.
CLEAN — both prior defects are resolved; no introduced regression found.
- Pre-existing symlink/
..defect: resolved. s3.py:1855 preserves filesystem path resolution. The symlink test blocks the incorrectly normalized destination with a regular file and verifies the correct destination, catching the original failure. - Pre-existing required-
lpathdefect: resolved. s3.py:1817 permits omission. The outfile test directly exercises it. - Relative paths retain their intended resolution; PathLike inputs remain supported; bare filenames skip empty-parent creation. Explicit
Noneworks withoutfile; without either destination, it remains invalid. - Relative paths, direct PathLike inputs, bare filenames, and
Nonewithoutoutfilelack dedicated regression tests. The two reported defects are directly guarded.
Static review only; no tests run. Checkout remained clean at a87a59e2.
1 participant
WHAT
Make
S3FileSystem.get_file()follow fsspec'sAbstractFileSystem.get_file()contract, so recursiveget()works.rpathcreates the local directorylpathinstead of writing an empty file.lpathare created, from the path as given so that..after a symlink resolves asopen()resolves it.lpath, oroutfile, is written to and left open. As in fsspec,lpathdefaults toNone, soget_file(rpath, outfile=f)works.version_idor as?versionId=in the path, always names an object. In that caseget_file()skips the directory check, becauseisdir()would look up the latest version.AioS3FileSystem._get_file()delegates to this method.Behavior changes for the release notes:
outfileis now honored.rpathwith an existing local directory aslpathnow raisesIsADirectoryError. Before, this call returned without downloading anything.rpathcreates the local directory instead of raisingFileNotFoundError.WHY
Fixes item 1 of #974. Item 2,
cp_file()sendingCopyObjectfor directory entries, moves to #1008.Skipping directory entries in
cp_file()would let fsspec's copy-then-removemv()delete overlapping copies, so how far to guardmv()needs its own design decision.Closes #974.
TEST
Tested commit: a87a59e on master 28826be.
just lint: passed.uv run --env-file .env pytest -n 2 -p no:cacheprovider -q tests/pyathena/filesystem/: 443 passed, including the live S3 tests.New offline tests:
test_get_file_directory[dir|None], which replaces master's test that pinnedFileNotFoundError.test_get_file_creates_parent_directories,test_get_file_parent_through_symlinkandtest_get_file_file_like, which includes anoutfile-only call.All five fail on master. The symlink case and the
outfile-only call also fail on the first head of this PR, 2a68d34.test_get_file_missingpasses on master as well. It pins that a missing object leaves no local file or parent directory.test_get_file_version_id[...](2 cases) passes on master as well. It guards the new directory check against looking up a requested version as a directory.New live test
test_get_recursivecreates two objects under a unique prefix.Not covered:
get(). On master,AsyncFileSystem._get()creates the parent directory of every target before any download (fsspec/asyn.py:808), so the directory targets already exist and master'sos.path.isdir(lpath)early return skipped them.cp_file()andmv()(Recursive copy() and mv() send CopyObject for directories, and mv() can delete overlapping copies #1008).🤖 Generated with Claude Code