Read a negative cat_file() start without an end as a suffix range - #998
Conversation
cat_file(path, start=-N) resolved the offset against the size from info(), which reports a key ending in "/" as a directory of size 0 and may serve a stale cached size. A negative start without an end is now sent as a suffix range (bytes=-N), which S3 resolves without the size, as s3fs does. The filesystem docs state how keys ending in "/" behave: they stay directories, following fsspec's path normalization. Closes #994. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
272986f to
91d74a7
Compare
| `s3://YOUR_S3_BUCKET/dir` are the same path. An object whose key ends in a slash, such | ||
| as a folder marker, is therefore a directory for `info`, `isfile`, and `open`, and | ||
| `open` raises `FileNotFoundError` for it. `find`, and `ls` of that directory, list the | ||
| object as a file entry. `cat_file` reads it without a range, with a non-empty range of |
There was a problem hiding this comment.
Self-review round one (implementation behavior). Base 92c9e3e67180eb3d52ef7b85579a24540a30fd64, head 91d74a700872a9e1086f0d9a7a982d17015c8387. The branch was rebased onto #988 before this round; #988 does not touch cat_file() or _format_ranges(). Result: FINDINGS (1, repaired in 91d74a7).
Covered:
- The
cat_file()branch for a negativestartwith noend. It skipsinfo()and the directory check, and the GetObject error is still translated: a prefix or missing key raisesFileNotFoundErrorfromNoSuchKey. - The other negative and empty-range paths are unchanged.
InvalidRangehandling: a suffix range on a 0-byte object returns 200b""(live), so the handler is not involved.- Version IDs are passed through.
_format_ranges()callers:_get_object(), and the copy ranges, which are always non-negative._get_object()'s empty-range guard (endisNone) and its return value: the start is used only by_fetch_range(), which never sends suffix ranges.- Async delegation and
cat_ranges()viacat_file(). - Tests: the fake implements S3's suffix semantics (
data[-N:], the whole object when shorter), and 6 cases fail on master.
Finding: the docs said "ls and find list it as a file entry". Live, ls of the parent lists only/ as the directory only; only ls of the directory itself and find return the only/ file entry. Repaired in the docs and the PR body.
Validation on 91d74a7:
just lintandjust docs lint: passed.uv run --env-file .env pytest -n 4 tests/pyathena/filesystem/: 372 passed.
| # S3 would return the whole object for an empty range. | ||
| return b"" | ||
| ranges = (start, end) | ||
| if start is not None and start < 0 and end is None: |
There was a problem hiding this comment.
Self-review round two (claims, callers, operations). Base 92c9e3e67180eb3d52ef7b85579a24540a30fd64, head 91d74a700872a9e1086f0d9a7a982d17015c8387. Result: CLEAN.
Claims checked:
- "S3 returns the last N bytes, or the whole object when it is shorter": live raw GetObject returned
bytes=-2→ 206b"89",bytes=-10andbytes=-100→ the whole object,bytes=-5on a 0-byte object → 200b"", and a missing key →NoSuchKey404. - "s3fs sends the same range": s3fs 2026.9.0
_process_limitssetsstart=""andend=-startfor a negativestartwith noend(bytes=-N). Live, s3fs readb"bc"fromslash/. - The docs paragraph: on live S3,
inforeports a directory,isfileisFalse, andopenraisesFileNotFoundError.findandlsof the directory list the key as a file.cat_filereads it without a range, with[0:1]and withstart=-2, and raises for(-2, 3),(0, -1)and(5, 5). - The release-note claim that this used to return the whole object (and, with Clamp byte ranges to the object in cat_file() and S3File reads #987,
FileNotFoundError): measured in the Objects whose keys end in '/' are listed as files but cannot be opened or read with negative offsets #994 reproduction onaa0fc914and02f8ebdd.
Callers and operations: a negative start without an end now sends one GetObject instead of info() (HeadObject, or ListObjectsV2 for a missing key, unless cached) plus GetObject. Requests decrease, retries are unchanged, and a stale cached size no longer affects the result.
A bucket path sent Key=None to GetObject, so the suffix range raised botocore's ParamValidationError instead of FileNotFoundError, as did a read without a range before. info() of a key ending in "/" describes the key without the slash, so a range resolved against it could use another object's size; such ranges now raise FileNotFoundError. The docs state which methods normalize the trailing slash and how cat_file() reads the key as written. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| # empty range sends no GetObject request that would report a | ||
| # missing object. | ||
| info = self.info(path, version_id=version_id) | ||
| if info.get("type") == S3ObjectType.S3_OBJECT_TYPE_DIRECTORY or info.key != key: |
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, session 01a10116-8794-7263-8855-6c44ece03ab5. It ran on a detached snapshot of head 91d74a700872a9e1086f0d9a7a982d17015c8387 against base 92c9e3e67180eb3d52ef7b85579a24540a30fd64. The prompt contained the literal diff and the repository conventions, without PR text, commit messages or prior findings. This is a static review. Afterwards, the snapshot and the PR worktree were unchanged. Result: FINDINGS.
Reviewer output, verbatim:
Covered the exact range, cat_file, inherited cat_ranges, async forwarding, _get_object, buffered reads, multipart-copy callers, version IDs, prefixes, missing/empty objects, slash-ending keys, docstrings, filesystem documentation, and relevant tests.
FINDINGS
-
P2 — Regression: bucket-only suffix reads leak an SDK exception.
pyathena/filesystem/s3.py:1527
For an accessible bucket,fs.cat_file("s3://bucket/", start=-1)previously obtained directory metadata and raisedFileNotFoundError. The new branch bypasses that check and sendsKey=Noneto boto3, producingParamValidationError. Neither exception handler translates it. This also affects async andcat_rangescallers. Reject bucket-only paths before issuing GetObject; the changed directory tests omit this case. -
P2 — Introduced documentation error: trailing-slash behavior is overstated.
docs/filesystem.md:101
The paths are not universally equivalent: with onlydir/containingb"abc",cat_file("s3://bucket/dir/", start=-2)returnsb"bc", while the slashless call raisesFileNotFoundError. Also,info/isfile/read-modeopencan access the actual slash-ending object throughdir/?versionId=v, because the slash before the query survives normalization. Write-modeopen(".../dir/", "wb")can create or overwritedirinstead of raising. Qualify the paragraph by method, mode, and version syntax. -
P2 — Pre-existing: bounded negative ranges can use another key’s size.
pyathena/filesystem/s3.py:1538
Supposedircontains ten bytes anddir/containsb"abc". Callingcat_file("s3://bucket/dir/", start=-2, end=-1)obtainsdir’s size through slash-strippinginfo(), then requestsbytes=8-8fromdir/. ItsInvalidRangebecomesb"", whereas the requested slice isb"b". This logic exists at the merge-base. Metadata lookup must identify the same literal key as GetObject.
The suffix header itself matches RFC 9110 and the inspected fsspec/s3fs implementation. Version precedence and buffered-read/multipart-copy callers show no further regression. Ordinary marker entries are listed as files by ls/find, as documented.
The new tests would catch the old stale-size and slash-marker suffix failures. Mocking info() means they do not establish the documentation’s classification claims; empty-object suffix handling also lacks direct coverage.
I read fsspec 2026.9.0 in the supplied .venv and s3fs 2026.9.0 in the local uv cache. Those comparisons used source, not recollection. Static review only: no edits, builds, tests, GitHub, or network access.
Author verification on live S3 (master 92c9e3e6 vs 91d74a70):
- 1:
cat_file("s3://bucket/", start=-1)raisedFileNotFoundErroron master andParamValidationErroron the branch. Confirmed regression. Without a range, both raiseParamValidationError, which is pre-existing. - 2:
cat_file("…/only/", start=-2)returnedb"bc", while"…/only"raisedFileNotFoundError.open("…/w/", "wb")created the keyw."…/only/?versionId=null"gaveinfo= file of size 3,isfile=Trueandopen=b"abc". Confirmed docs overstatement. - 3: with
both(10 bytes) andboth/(b"abc"),cat_file("…/both/", -2, -1)returnedb""on master and on the branch. Confirmed, pre-existing.
There was a problem hiding this comment.
Repaired in fbd2d0b:
- 1:
cat_file()raisesFileNotFoundErrorfor a path without a key before any request, with or without a range. Release note: an unrangedcat_file("s3://bucket")used to raiseParamValidationError. - 3: when
info()is used (a negativeend, a negativestartwith anend, or an empty range),cat_file()raisesFileNotFoundErrorunless the returned entry is a file with the same key.info()ofdir/describesdir, so its size is no longer applied todir/. A?versionId=path keeps its slash and matches, ascat_file("…/only/?versionId=null", 0, -1)returningb"ab"shows. The redundantcast(str, key)was dropped. - 2: the docs paragraph now says that
info,isfileandopennormalize the trailing slash. Reading raisesFileNotFoundError, and writingdir/writesdir. A?versionId=suffix keeps the slash.find, andlsof the directory, list the object as a file.cat_fileuses the key as written, reads it without a range, with non-empty non-negative ranges or with a suffix range, and raises for other ranges. Every statement was checked on live S3.
Validation on fbd2d0b:
just lintandjust docs lint: passed.uv run --env-file .env pytest -n 4 tests/pyathena/filesystem/: 378 passed.- New
test_cat_file_bucket(3 cases) andtest_cat_file_range_key_ending_in_slash(3 cases) fail on91d74a70. - Live S3: the bucket path raises
FileNotFoundErrorwith and without a range, andboth/raisesFileNotFoundErrorfor(-2, -1)and(5, 5). Theonly//slash/results match the docs.
Self-review of this repair:
- Round one (behavior): the key check runs only on the
info()path, so suffix ranges and non-negative ranges are unchanged. A file entry from a listing cache carries the listed key, which equals the parsed key for normal paths. The directory check is kept for plain prefixes, whose key equals the directory's key. - Round two (claims): the commit message, the
cat_file()Raises:text ("If the path has no key or the key does not exist") and the docs statements were checked against the live results above. The PR body is updated with the new release note.
open() and info() of dir/ refer to the object dir when it exists, and a ?versionId= path reads any range of the key ending in a slash. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| `dir/` for reading reads the object `dir` or raises `FileNotFoundError`, and opening it | ||
| for writing writes the object `dir`. A path with a `?versionId=` suffix keeps the slash | ||
| and refers to the object. `find`, and `ls` of the directory, list the object as a file | ||
| entry. `cat_file` uses the key as written. Without a `?versionId=` suffix, it reads |
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, session 01a10125-db3d-7510-b0ae-fc80f31122fc. The scope was the repair delta 91d74a700872a9e1086f0d9a7a982d17015c8387..fbd2d0bc3aafa1185a7b3150879fe3ee6111e0a7 (the base 92c9e3e67180eb3d52ef7b85579a24540a30fd64 did not move), with the affected contracts. This is a static review. The snapshot and the PR worktree were unchanged. Result: FINDINGS (docs only).
Reviewer output, verbatim:
Covered: the exact delta; path parsing, range branches, metadata/cache keys, version handling, prefixes, fsspec callers, async forwarding, open reads/writes, find/ls, and test helpers.
FINDINGS — two documentation issues; no code regression found.
-
P3 — introduced documentation overstatement: docs/filesystem.md:109 says other ranges raise
FileNotFoundErrorwithout excluding version-qualified paths. For an existing ten-bytedir/version,cat_file("s3://bucket/dir/?versionId=VERSION", start=-2, end=-1)successfully reads one byte;(5, 5)returnsb"".info()preserves that key, so the new check passes. Qualify this restriction as applying to trailing-slash paths without a version suffix. -
P3 — pre-existing claim retained: docs/filesystem.md:103 assumes that a trailing-slash path always becomes a directory and fails when opened for reading. When both
diranddir/exist, normalization makesinfo()return the filedir,isfile()returnTrue, andopen(..., "rb")readdir. The object lookup and new test’s setup establish this scenario. State that the directory/read-failure behavior assumes no object exists at the normalized key.
The implementation satisfies the requested guards. Ordinary keys, cached/version-aware entries, prefixes, suffix ranges, and non-empty non-negative ranges retain their prior behavior.
Static tracing indicates all six new test cases fail before and pass after for the intended reasons: bucket cases previously reached GetObject; mismatched-key cases previously returned b"" using the unrelated metadata.
No edits, builds, tests, or network access. HEAD remains fbd2d0bc…, with a clean working tree.
Author verification on live S3:
- 1:
cat_file("…/only/?versionId=null", -2, -1)returnedb"e", and(5, 5)returnedb"". Confirmed. - 2: with
bothandboth/,info("…/both/")returned a file of size 10,isfilereturnedTrue, andopen().read()returnedb"0123456789". Confirmed.
There was a problem hiding this comment.
Repaired in da5dedc (docs only):
info,isfileandopentreatdir/as "the objectdirif it exists, and otherwise the directorydir". Openingdir/for reading readsdiror raisesFileNotFoundError.- The
cat_filerange restriction now applies "without a?versionId=suffix". just docs lint: passed. Every statement in the paragraph matches the live results recorded in this thread and the earlier ones.- Self-review of the repair: round one found no behavior changes (docs only). In round two, each sentence was checked against a live result: normalization and existing
dir(both/), the directory and read failure (only/), writingw/→w,?versionId=nullaccess,find/lslisting, andcat_fileranges with and without a version suffix.
| limit raises `ValueError` and aborts its multipart upload. Multipart copies with `cp` | ||
| use parts large enough to stay within the limit. | ||
|
|
||
| Paths are normalized as in fsspec, which drops a trailing slash, so `info`, `isfile`, |
There was a problem hiding this comment.
Independent narrow follow-up review (relayed): Codex CLI 0.160.0, model gpt-6-astra, reasoning effort max, codex exec -s read-only, session 01a1012b-b219-74d0-8553-c9578ca54d61. The scope was the docs delta fbd2d0bc3aafa1185a7b3150879fe3ee6111e0a7..da5dedcfabdb057511dae7b935f28e17b6d50d57 (the base 92c9e3e67180eb3d52ef7b85579a24540a30fd64 did not move), with each statement checked against the code paths. This is a static review. The snapshot and the PR worktree were unchanged. Result: CLEAN.
Reviewer output, verbatim:
Surfaces covered, assuming dir/ exists and checking both with and without an object at dir:
| Surface | Verified behavior |
|---|---|
info, isfile |
Unsuffixed dir/ resolves to dir: file/True when that object exists; directory/False otherwise. |
open |
Reading uses dir or raises FileNotFoundError; writing targets dir. |
?versionId=V |
Preserves the slash key for metadata and reads. Version-qualified writes raise ValueError. |
find, ls of the directory |
Include the slash-key object as a file entry, including when dir also exists. |
cat_file, no suffix |
Whole-object reads, non-empty non-negative ranges—including open-ended ranges—and negative start without end read the slash key directly. |
| Other unsuffixed ranges | Empty/reversed ranges, negative end, and negative start with an explicit end raise FileNotFoundError. The key-mismatch guard also rejects an existing dir object. |
Version-qualified cat_file |
Metadata retains the slash key, allowing negative-offset normalization and empty results. Ranges starting at/past EOF return empty bytes. |
CLEAN — no actionable findings in the rewritten paragraph.
Static source review at da5dedcf, including inherited methods in local fsspec 2026.9.0, matching the lockfile. No edits, builds, tests, or network access.
WHAT
S3FileSystem.cat_file()sends a negativestartwithout anendas a suffix range (bytes=-N) instead of resolving it against the size frominfo().S3 returns the last N bytes, or the whole object when it is shorter, so no
info()lookup is needed. s3fs sends the same range.Other negative offsets (a negative
end, or a negativestartwith anend) are still resolved againstinfo(), as before.cat_file()raisesFileNotFoundErrorfor a path without a key (s3://bucket) before any request.cat_file()resolves a range withinfo()(a negativeend, a negativestartwith anend, or an empty range), it raisesFileNotFoundErrorunlessinfo()returns a file with the same key.info()ofdir/describesdir, whose size must not be used fordir/.S3File._format_ranges()formats(start, None)with a negativestartasbytes=-N.docs/filesystem.mdstates how keys ending in/behave:info,isfileandopennormalize the trailing slash:dir/is the objectdirif it exists, and otherwise the directorydir. Readingdir/readsdiror raisesFileNotFoundError, and writingdir/writesdir.?versionId=suffix keeps the slash and refers to the object, forcat_fileranges too.find, andlsof the directory, list the object as a file.cat_fileuses the key as written, and the doc lists the ranges that read it.Behavior changes for the release notes:
cat_file(path, start=-N)(andcat_ranges()with such a range) no longer callsinfo().It reads the current object even when the cached size is stale, and it reads an object whose key ends in
/. Before, it returned the whole object for such a key, and with Clamp byte ranges to the object in cat_file() and S3File reads #987,FileNotFoundError.A prefix or missing key raises
FileNotFoundErrorfrom GetObject, as before.cat_file("s3://bucket")raisesFileNotFoundErrorinstead of botocore'sParamValidationError./, ranges resolved withinfo()raiseFileNotFoundError. Before, when an object without the slash also existed, they used that object's size and could return wrong bytes.WHY
Fixes #994.
Maintainer decision for #994: keep keys ending in
/as directories, following fsspec and s3fs, and consistent with #989 (#981) and #990 (#974). Make the minimal change: readstart=-Nwith a suffix range as s3fs does, and document the limitation.ls()is unchanged; it lists such keys as files, as s3fs does.TEST
Tested commit: fbd2d0b (Python 3.13.1, fsspec 2026.9.0), based on
92c9e3e6. The head da5dedc changes only the docs (just docs lintpassed).just lint,just docs lint: passed.just docs build: succeeded with no warnings fromdocs/filesystem.md.uv run --env-file .env pytest -n 4 tests/pyathena/filesystem/: 378 passed.TestS3FileSystem. The GetObject fake now answers suffix ranges like S3.test_cat_file_rangeadds(-10, None)and(-100, None), and assertsRange=bytes=-Nwith noinfo()call for a negativestartwithout anend.test_cat_file_range_stale_sizeaddsstart=-5with a stale cached size.test_cat_file_suffix_range_key_ending_in_slash:cat_file("s3://bucket/dir/", start=-2)readsb"bc"withKey="dir/"whileinfo()reports a directory.test_cat_file_range_directoryreplaces(-5, None)with(-5, 3), which still goes throughinfo().test_format_rangesadds(-8, None).test_cat_file_bucket: a bucket path raisesFileNotFoundErrorwithout a request, with and without a range.test_cat_file_range_key_ending_in_slash:(-2, -1),(0, -1)and(5, 5)ondir/raise wheninfo()returns the filedir.ae2d2a04).bytes=-2on a 10-byte object returned 206b"89";bytes=-10andbytes=-100returned the whole object;bytes=-5on a 0-byte object returned 200b"";bytes=-2on a keyslash/returnedb"bc"; a missing key returnedNoSuchKey.only/andslash/(withslash/child):cat_file(start=-2)returnedb"bc";cat_file()and[0:1]read the object;(-2, 3),(0, -1),(5, 5)andopen()raisedFileNotFoundError;findandls("…/slash/")listed the key as a file, andlsof the parent listedonlyandslashas directories. The docs paragraph states these results.both(10 bytes) andboth/(b"abc"),cat_file("…/both/", -2, -1)and(5, 5)raiseFileNotFoundError(master:b""). A bucket path raisesFileNotFoundErrorwith and without a range.open("…/w/", "wb")writesw.…/only/?versionId=null:info= file,isfile=True,openreadsb"abc",cat_file(0, -1)=b"ab".🤖 Generated with Claude Code