Clamp byte ranges to the object in cat_file() and S3File reads - #987
Conversation
| range_end = end | ||
|
|
||
| info = self.info(path, version_id=version_id) | ||
| if info.get("type") == S3ObjectType.S3_OBJECT_TYPE_DIRECTORY: |
There was a problem hiding this comment.
Self-review round one (implementation behavior). Base aa0fc9146f4e683d6cdf96b894f963f9dd8f7abe, head 0b0e15053a153806f4f4525deee9956c36e55aeb. Result: FINDINGS (1).
Covered: cat_file() range resolution (slice semantics; negative, None, reversed and past-the-end offsets; empty objects; a non-integer offset now raises TypeError from slice.indices instead of sending a malformed Range), delegation of cat_ranges() and AioS3FileSystem._cat_file() to cat_file(), S3File._fetch_range() clamping before _get_ranges() splits for max_workers > 1, the fsspec 2026.9.0 FirstChunkCache, MMapCache and BaseCache fetch paths, the callers of _get_object() (only cat_file() and _fetch_range(), both guarded), and whether the offline stub could pass by omitting S3 behavior (it rejects any range that S3 would answer with other than the requested bytes, and the 13 defect cases fail on master).
Finding: info() reports a prefix as a directory with size 0. With the range clamped, cat_file("s3://bucket/prefix", start=0, end=5) resolved to an empty range and returned b"". On master it raises FileNotFoundError from GetObject, with or without a range. Checked on live S3 with an object under prefix/.
Out of scope, pre-existing: fs.open(prefix, "rb").read() returns b"" on master and on this branch, because info() reports size 0. This PR does not change it.
There was a problem hiding this comment.
Repaired in 2452e4f: cat_file() raises FileNotFoundError for a directory before the range is resolved, with no request. The docstring gains a Raises: section.
- New offline test
test_cat_file_range_directorycovers(0, 5)and(5, None). - The live S3 check returns
FileNotFoundErrorfor{},start=0, end=5andstart=5on a prefix. just lintpassed.uv run --env-file .env pytest -n 4 tests/pyathena/filesystem/: 296 passed.
| # a range. | ||
| raise FileNotFoundError(path) | ||
| # S3 would return the whole object for an empty range. | ||
| range_start, range_end, _ = slice(start, end).indices(info.get("size", 0)) |
There was a problem hiding this comment.
Self-review round two (claims, callers, operations). Base aa0fc9146f4e683d6cdf96b894f963f9dd8f7abe, head 2452e4f912b75d9bfd2fdafba6f219793d6e598d. Result: FINDINGS (1 PR-body claim, corrected).
Claims checked:
- S3 range behavior (WHY, the
_get_object()Raises:text, the_fetch_range()docstring, the inline comment): raw GetObject on live S3 returned 200 with the whole object forbytes=5-4,10-9,12-9and0--1,InvalidRangefor12-19and10-19, and 206 for5-100. cat_ranges()andAioS3FileSystem._cat_file()share the behavior: fsspec 2026.9.0AbstractFileSystem.cat_rangescallsself.cat_fileper range, andAsyncFileSystem._cat_rangescallsself._cat_file, which delegates to the synccat_file(). Neither filesystem overridescat_ranges.- "fsspec caches request empty ranges":
FirstChunkCache._fetchcalls the fetcher with(size, size)at the end of the file,MMapCache._fetchrequests(32, 32)for an empty last block, andAbstractBufferedFile.read(n)passesloc + nunclamped. - Slice semantics as the contract: fsspec's
MemoryFileSystem.cat_filereturnsdata[start:end]for all the issue's cases.LocalFileSystemraisesValueErrorfor reversed ranges. s3fs sends the unclamped range, so it has the same whole-object behavior this PR fixes.
Correction: the release-note line said that a start at or past the end used to raise InvalidRange. On master that holds only when end > start; with end=None (for example (10, None) or (12, None)) S3 returned the whole object. The PR body now states the condition.
Callers and operations: signatures and return types are unchanged. _get_object()'s new ValueError is reachable only by direct callers of the private method, and both internal callers guard against it. Empty ranges now send no GetObject request, and no retry paths changed. The info() call for ranged cat_file() already existed.
Deferred: cat_file() takes the size from info(), which can come from the listing cache. If another writer grows the object after it was listed, cat_file(p, start, end) with start at or past the cached size now returns b"", where master would send the range. Master already resolved end=None and negative offsets against the same cached size, so the size source is unchanged; it is the issue's proposed approach.
No docs mention ranged reads (searched docs/*.md).
| version_id = kwargs.pop("version_id", None) | ||
| if path_version_id: | ||
| version_id = path_version_id | ||
| if (start is not None and start < 0) or (end is not None and end < 0): |
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 01a100c9-5b8a-7750-aaa5-336762281447. It ran on a detached snapshot of head 2452e4f912b75d9bfd2fdafba6f219793d6e598d against base aa0fc9146f4e683d6cdf96b894f963f9dd8f7abe. The prompt contained the literal diff and the repository conventions; the PR text, the commit messages and prior findings were withheld. Constraints: no edits, builds, tests, network or GitHub. This is a static review (the reviewer read fsspec 2026.9.0 from the installed .venv). After the run, the snapshot and the PR worktree were unchanged at that head. Result: FINDINGS.
Reviewer output, verbatim:
Covered the exact base→head diff, cat_file/cat_ranges, async delegation, every _get_object caller, range splitting/merging, version selection, prefixes, buckets, empty objects, docstrings, and test fakes.
I inspected installed fsspec 2026.9.0 in the main checkout’s .venv: none/None, bytes, readahead, adaptive, first, mmap, blockcache, background, all, and parts. The s3fs comparison uses contract knowledge. No files changed, tests/builds run, or network accessed.
FINDINGS
Introduced regressions:
-
P2 — Literal keys ending in
/can become unreadable.
pyathena/filesystem/s3.py:1480
Given an objectdir/containingb"abc"and no objectdir,cat_file("s3://bucket/dir/", start=0, end=1)now raisesFileNotFoundError.info()strips the trailing slash, checksdir, then identifiesdir/as a prefix. Previously the bounded GetObject used the correctly parsed keydir/and returnedb"a". The new directory rejection must distinguish an actual object from a synthetic prefix using the exact key. -
P2 — Cached sizes now suppress valid positive ranges.
pyathena/filesystem/s3.py:1485
Cache metadata for a 10-byte object, then overwrite it externally with 20 bytes.cat_file(path, start=10, end=20)now clamps both bounds to 10 and returnsb""without GetObject. Previously it requested bytes 10–19 and returned the current contents. Metadata caching predates this change, but using stale size to cap explicit positive bounds is new. Revalidate the metadata before using it to declare EOF. -
P2 — The early return makes prefix-only paths appear as empty files with additional caches.
pyathena/filesystem/s3.py:2614
Givenprefix/childbut no objectprefix, openingprefixwithcache_type="all"now succeeds and readsb"". The constructor accepts directory metadata with size zero;AllBytescalls_fetch_range(0, 0), which now returns immediately. Previously that fetch reached GetObject and failed because the object was absent.cache_type="first"withread(1)has the same regression. Reject directory metadata before constructing the file cache. -
P3 — The new exception documentation overstates bucket handling.
pyathena/filesystem/s3.py:1472
cat_file("s3://bucket")without bounds bypasses the directory check and passesKey=Noneto botocore, producingParamValidationError, not the documentedFileNotFoundError. The runtime behavior is pre-existing; the inaccurate promise is new.
Pre-existing distinctions: directory admission already allowed empty reads with the default bytes cache at s3.py:2321. Likewise, trailing-slash normalization and stale metadata already affected size-dependent reads. Findings 1–3 identify the additional failures introduced here.
The fake’s range assertions do catch the targeted invalid requests: they reject reversed ranges and starts beyond EOF while correctly allowing oversized ends. Those cases cannot pass merely because Python slicing returns empty bytes. However, mocking info() to return fixed metadata hides the metadata/path failures above; the added buffered-read cases also omit prefix-only paths and zero-length objects.
Author verification on live S3 (master aa0fc914 vs 2452e4f9):
- 1 (object
slash/,cat_file(start=0, end=1)): masterb'a', branchFileNotFoundError. Confirmed regression. - 2 (cached size 10, object grown externally to 20,
cat_file(start=10, end=20)): masterb'abcdefghij', branchb''. Confirmed regression. - 3 (
open(prefix, cache_type="all"/"first").read(1)): masterFileNotFoundError, branchb''. Confirmed regression. With"bytes", both returnb'', which is pre-existing. - 4: Confirmed. Without a range,
cat_file("s3://bucket")raises botocore'sParamValidationError.
There was a problem hiding this comment.
Repaired in 20d0913. The maintainer chose the approach: leave size-dependent range edges to S3, and reject prefixes in open().
- 1 and 2:
cat_file()no longer callsinfo()for non-negative offsets. It returnsb""forstart >= endwithout a request, and otherwise sends the range as given (open-endedbytes=N-whenendisNone). S3 clamps an end past the object itself. TheInvalidRangeanswer for a start at or past the end becomesb"", identified by the S3 error code of theClientErrorcause and only for ranged requests. Only negative offsets are resolved againstinfo(), as on master; a prefix there raisesFileNotFoundError. - 3:
S3File.__init__raisesFileNotFoundErrorin read mode wheninfo()reports a directory, for every cache type. This also fixes the pre-existingb""with"bytes"(release note).get_file()now opens the remote file before the local one, so a failed open leaves no empty local file. - 4: the
Raises:text now reads "If the key does not exist". _format_ranges()and_get_object()accept an open end ((start, None)).
Validation on 20d0913:
just lint: passed.uv run --env-file .env pytest -n 4 tests/pyathena/filesystem/: 302 passed.- The new offline tests fail on master (24 cases), and 16 of them fail on
2452e4f9.test_cat_file_range_stale_sizeandtest_open_directory[all|first]cover findings 2 and 3, andtest_cat_file_rangeasserts thatinfo()is called only for negative offsets. - Live S3, finding 1:
b'a'. Finding 2:b'abcdefghij'. Finding 3:FileNotFoundErrorforall,firstandbytes. - Live S3, the issue's cases: slice results. A 0-byte object returns
b''for all ranges and cache types. - Live S3, recursive
get()of a prefix: fails on master (NotADirectoryError) and on this branch (FileNotFoundError). That is Recursive get(), copy() and mv() mishandle directory entries #974.
Limitation, pre-existing: info() strips the trailing /, so an object whose key ends in / is still not addressable through negative offsets or open(). open("…/slash/") now raises FileNotFoundError, where master read b"".
| if ( | ||
| ranges | ||
| and isinstance(e.__cause__, botocore.exceptions.ClientError) | ||
| and S3ClientError(e.__cause__).code == "InvalidRange" |
There was a problem hiding this comment.
Self-review round one, after the independent-review repair (implementation behavior). This is a full pass, because the repair expanded the contract. Base aa0fc9146f4e683d6cdf96b894f963f9dd8f7abe, head 20d09135c4edef805e879eb718ff33afe64e48bb. Result: FINDINGS (1 simplification, repaired in 02f8ebdde0c337ac59d6bc71f6d479eea1da0c6c).
Covered:
cat_file(): non-negative, negative andNoneoffsets; open-ended ranges; a 0-byte object; prefixes; keys ending in/; version IDs (non-negative reads never usedinfo()for the version sent to GetObject).- InvalidRange detection:
_call()raisesS3ClientError(e).os_error from e, so__cause__is theClientError.RetryConfigretries onlyTHROTTLING_ERROR_CODES, so the 416 is not retried. Only ranged requests map it tob"". _fetch_range(): it clamps to the size pinned at open, and reads are sent withIfMatchon the ETag, so the object cannot change underneath.S3File.__init__: the directory check sits before version pinning, and a bucket path still raisesValueErrorfirst.AioS3Fileinherits it.get_file(): the open order changed, and the callback and size use are unchanged.- The test fake is at the client level and keeps the real
_call(), so the error translation is exercised.
Finding: the condition 0 <= start >= end >= 0 in test_cat_file_range was hard to read. It is now 0 <= end <= start with no change in meaning.
Out of scope: recursive get() of a prefix fails on master and on this branch (#974); only the error type differs.
| # otherwise take the size from the latest version of the object. | ||
| info = fs.info(path, version_id=self.version_id) | ||
| if info.get("type") == S3ObjectType.S3_OBJECT_TYPE_DIRECTORY: | ||
| # A prefix has no object to read. |
There was a problem hiding this comment.
Self-review round two, after the independent-review repair (claims, callers, operations). This is a full pass. Base aa0fc9146f4e683d6cdf96b894f963f9dd8f7abe, head 02f8ebdde0c337ac59d6bc71f6d479eea1da0c6c. Result: FINDINGS (PR-body wording, corrected).
Claims checked:
- "S3 clamps an end past the object": live
bytes=5-100returned 206 withb"56789". - "InvalidRange (HTTP 416)": live
bytes=12-19andbytes=10-19returnedInvalidRangewith status 416. - "A 0-byte object returns
b''": live,(0, None),(0, 5)and(-5, None), and thebytes,all,firstandmmapcaches. - Stale size, keys ending in
/,open(prefix)andget_file(prefix): live runs as stated in the PR body. - The docstrings of
cat_file(),_get_object(),_format_ranges(),_fetch_range()andS3File.__init__(Raises) match the code.
Corrections to the PR body:
- The
open(prefix)release note said that"all"/"first"readb""before. On master, they raisedFileNotFoundErrorfrom GetObject when the data was fetched. The note now says so. - The
info()cost note now includes ListObjectsV2 for a missing key.
Callers and operations:
- Non-negative ranged reads send one GetObject and no
info(), which is fewer requests than master.cat_rangesover many ranges, as parquet readers do, benefits. open()of a prefix now raises; on master it silently readb"". This is a release note.get_file()no longer leaves an empty local file when the remote open fails, including for a missing key, which also left one on master.
Imprecision not rewritten: commit 20d0913 says that the synthesized size "made reads of keys ending in '/' empty". That describes 0b0e150; at 2452e4f, the same read raised FileNotFoundError. The branch is not rewritten for a commit-message nuance.
| if path_version_id: | ||
| version_id = path_version_id | ||
| if (start is not None and start < 0) or ( | ||
| end is not None and (end < 0 or (start or 0) >= end) |
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 01a100df-9260-77a2-907b-ca289cf98428. Full review of the expanded scope on a detached snapshot of head 02f8ebdde0c337ac59d6bc71f6d479eea1da0c6c against base aa0fc9146f4e683d6cdf96b894f963f9dd8f7abe. 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 at that head. Result: FINDINGS.
Reviewer output, verbatim:
Covered the supplied diff, synchronous and async cat_file/cat_ranges/get_file, _get_object callers, version handling, prefixes, buckets, empty objects, error translation/retries, docstrings, and tests. Traced all ten built-in fsspec caches: none, bytes, readahead, adaptive, first, mmap, blockcache, background, all, and parts.
Used installed fsspec 2026.9.0 source from the main checkout’s .venv. The s3fs comparison uses API knowledge; s3fs was not installed.
FINDINGS
-
P2 — Regression: empty ranges suppress missing-object errors.
pyathena/filesystem/s3.py:1491With an uncached, nonexistent key,
fs.cat_file("s3://bucket/missing", start=0, end=0)now returnsb""without checking existence. Previously,info()raisedFileNotFoundError. A prefix without an object or a nonexistent version similarly returns success. This also propagates throughcat_rangesand the async wrappers, contradicting the newRaisesdocumentation and fsspec’s open-before-read behavior.Validate the target before this early return. The new tests miss this combination: missing-key coverage uses a nonempty range, and directory coverage uses negative offsets.
-
P3 — New comment overstates protection against a pre-existing download failure.
pyathena/filesystem/s3.py:1577Cache metadata for a nonempty object, delete it externally, then call
get_file()with the default cache.open()succeeds using cached metadata; the destination is created or truncated; the first read raisesFileNotFoundError, leaving an empty local file. Thus the new comment’s unconditional “no local file is left behind” claim is false.The underlying behavior predates this change. Narrow the comment to failures detected during
open(). The directory test does not exercise a failure deferred until the first read.
The fake deliberately rejects malformed ranges instead of returning S3’s whole-object response. Static tracing shows this still detects the targeted EOF defects; it does not falsely pass them. However, it does not model missing keys/versions or conditional-request failures.
No files changed; no builds, tests, GitHub access, or network access performed.
Author verification:
- 1: Confirmed regression. On
02f8ebdd, a missing key, a prefix, and a missing version with(0, 0),(5, 3)or(None, 0)returnedb""without any lookup. On master,info()raisedFileNotFoundError. - 2: Confirmed. The comment overstated the effect. The empty local file left when an object is deleted after its metadata was cached is pre-existing:
open()succeeds from the cache and the first read fails.
There was a problem hiding this comment.
Repaired in 97ce192:
- 1: an empty range (
endis notNoneand(start or 0) >= end) now looks up the object withinfo(), as negative offsets do. A missing object or version raisesFileNotFoundErrorfrominfo(), and a prefix raises it through the directory check. Neither sends a GetObject request. - 2: the
get_file()comment now says that no local file is created "when open() finds no object at the path". The failure deferred to the first read stays as before; it is pre-existing and not changed here.
Validation on 97ce192:
just lint: passed.uv run --env-file .env pytest -n 4 tests/pyathena/filesystem/: 306 passed.- New
test_cat_file_empty_range_missing((0, 0),(5, 3),(None, 0)) andtest_cat_file_range_directory[5-5]fail on02f8ebdd. test_cat_file_rangenow asserts thatinfo()is called exactly for negative offsets and empty ranges.- Live S3: a missing key and a prefix raise
FileNotFoundErrorfor all three empty ranges, and an existing object's(5, 5)returnsb"".
Self-review of this repair:
- Round one (behavior): the repair adds an
info()call only for empty ranges. Master calledinfo()for every ranged read, so this restores master's existence check. Non-empty non-negative ranges still skipinfo(). - Round two (claims): the
cat_file()docstring now says thatinfo()also checks existence for an empty range. The PR body is updated. The existence check may use cached metadata, as master'sinfo()did.
| if path_version_id: | ||
| version_id = path_version_id | ||
| if (start is not None and start < 0) or ( | ||
| end is not None and (end < 0 or (start or 0) >= end) |
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 01a100ea-f792-7363-93c9-920de153308c. The scope was the repair delta 02f8ebdde0c337ac59d6bc71f6d479eea1da0c6c..97ce19220f328e9eced2384b1c674d0de571c5b6 (the base aa0fc9146f4e683d6cdf96b894f963f9dd8f7abe did not move), with the affected contracts listed and the cumulative diff readable. This is a static review. Afterwards, the snapshot and the PR worktree were unchanged at 97ce1922. Result: CLEAN.
Reviewer output, verbatim:
CLEAN — no actionable regressions found in 02f8ebdd..97ce1922.
Surfaces covered: cat_file(), version resolution, info() and its cache/HEAD/prefix paths, S3 error translation, _get_object(), fsspec 2026.9.0 callers (cat, cat_ranges, read_bytes), async delegation, and get_file() → open() → S3File.
- pyathena/filesystem/s3.py:1481: Empty non-negative ranges now perform the version-aware metadata lookup. Missing keys/versions propagate
FileNotFoundError; directory results are explicitly rejected. Existing objects returnb""before anyGetObject. Non-empty non-negative ranges still skipinfo(), and negative-offset behavior is unchanged. - pyathena/filesystem/s3.py:1583: The revised comment accurately describes evaluation order: an exception from remote
open()prevents opening the local destination.
Static comparison of the changed tests predicts seven distinguishing cases:
| Tests | Against 02f8ebdd |
Against 97ce1922 |
|---|---|---|
Range tests:645: (0,0), (5,5), (7,3) |
Fail the expected info() call assertion |
Pass; return empty without GET |
| Missing-object tests:688: all three cases | Fail because no exception is raised | Pass by propagating the lookup failure |
Directory test:698: (5,5) |
Fails because no exception is raised | Passes through directory rejection |
The other parameterized cases retain their previous behavior. Missing-version handling was traced through source; the new missing-object tests mock info().
Pre-existing limitation: lookup freshness still follows info()’s cache semantics. This patch does not change that behavior.
Static review only; no tests, builds, writes, or network access. HEAD remained 97ce1922, with a clean worktree.
S3 ignores a Range whose last byte precedes its first byte and returns the whole object, and rejects a range starting past the end with InvalidRange. cat_file() now resolves start/end like a slice and returns b"" for an empty range without a request. S3File._fetch_range() clamps the end to the object size before splitting the range for parallel requests and returns b"" for an empty range. _get_object() rejects an empty range instead of sending it. Fixes #970. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
info() reports a prefix with size 0, so the clamped range was empty and cat_file() returned b"" instead of the FileNotFoundError that GetObject raises for it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Resolving every range against info() misjudged reads: a cached size hid bytes appended since it was cached, a prefix's synthesized size of 0 made reads of keys ending in "/" empty, and an empty range returned early let open() of a prefix read b"" with the all and first caches. cat_file() now sends non-negative offsets as they are, returning b"" for start >= end without a request and for the InvalidRange error that S3 answers when the range starts at or past the end. Only negative offsets are resolved against info(), which raises FileNotFoundError for a prefix. S3File raises FileNotFoundError when opening a prefix for reading, and get_file() opens the remote file before the local one so that a failed open leaves no local file. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
An empty range returned b"" without a request, so a missing key, a prefix or a missing version read as empty instead of raising FileNotFoundError as master did through info(). The empty range now looks up the object with info() as negative offsets do. The get_file() comment is narrowed to failures that open() detects. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
97ce192 to
fd97af3
Compare
| # otherwise take the size from the latest version of the object. | ||
| info = fs.info(path, version_id=self.version_id) | ||
| if info.get("type") == S3ObjectType.S3_OBJECT_TYPE_DIRECTORY: | ||
| # A prefix has no object to read. |
There was a problem hiding this comment.
Self-review after rebase (rounds one and two, narrow). Old aa0fc9146f4e683d6cdf96b894f963f9dd8f7abe..97ce19220f328e9eced2384b1c674d0de571c5b6, new 6f258501a56124f6e29c6d530ce429b702ed2ab3..fd97af364bdbce5ece2c015124da686acaa0f582. Both old objects were verified with git cat-file -e. Result: CLEAN.
git range-diff: commits 2, 4 and 5 are identical. In commits 1 and 3, only the context changed: theimport re/import sysconflict intest_s3.py, and theS3File.__init__Raises:text from Wait for running parts before aborting multipart uploads, and validate S3File before init #992.- Upstream contracts that this patch touches:
- Wait for running parts before aborting multipart uploads, and validate S3File before init #992 moved argument validation and object lookups before the executor and the base class initializer, so that a failing file is never opened. The prefix check in the read branch sits in that section, so
open(prefix)raises before an executor is created orsuper().__init__runs. This matches Wait for running parts before aborting multipart uploads, and validate S3File before init #992's intent. - The append branch now looks up
info()/cat()before init. It reads the whole object without a range, so this patch does not affect it. - The executor and multipart-abort changes do not touch
_fetch_range()orcat_file(). - Split SQLAlchemy type rendering into Hive DDL and Trino DML compilers #961 is SQLAlchemy only.
- Wait for running parts before aborting multipart uploads, and validate S3File before init #992 moved argument validation and object lookups before the executor and the base class initializer, so that a failing file is never opened. The prefix check in the read branch sits in that section, so
- Claims: the PR body's descriptions stay accurate; only the test counts change.
- Validation on
fd97af36:just lint: passed.uv run --env-file .env pytest -n 4 tests/pyathena/filesystem/: 331 passed.- The live S3 scripts (the issue's cases, Codex findings 1–3, and empty ranges on missing keys and prefixes) give the same results as before the rebase.
| # otherwise take the size from the latest version of the object. | ||
| info = fs.info(path, version_id=self.version_id) | ||
| if info.get("type") == S3ObjectType.S3_OBJECT_TYPE_DIRECTORY: | ||
| # A prefix has no object to read. |
There was a problem hiding this comment.
Independent rebase follow-up review (relayed): Codex CLI 0.160.0, model gpt-6-astra, reasoning effort max, codex exec -s read-only, session 01a100f8-c56d-7a01-bdd5-9741469b28c2. The scope was the git range-diff from aa0fc9146f4e683d6cdf96b894f963f9dd8f7abe..97ce19220f328e9eced2384b1c674d0de571c5b6 to 6f258501a56124f6e29c6d530ce429b702ed2ab3..fd97af364bdbce5ece2c015124da686acaa0f582, plus the upstream filesystem changes between the two bases. This is a static review. Afterwards, the snapshot and the PR worktree were unchanged at fd97af36. Result: CLEAN.
Reviewer output, verbatim:
CLEAN — no actionable findings in the rebase or the inspected upstream interactions.
Surfaces covered:
- Conflict resolutions: The local range-diff matches the supplied output. Differences are confined to import and docstring context; the reviewed implementation and tests are preserved.
cat_file()and range helpers: Negative offsets, empty/reversed ranges, EOF handling, version selection, missing objects/prefixes, and selectiveInvalidRangehandling remain consistent. Multipart-copy range formatting is unchanged.S3File.__init__: Prefix rejection remains before fsspec initialization. Write-size validation leaves small read blocks valid. Append preloading, version/ETag handling, and executor initialization remain compatible with the series._fetch_range()and executors: Clamping precedes request splitting. Checked fsspec 2026.9.0’snone,first,mmap,bytes, and eagerallcache paths. The executor changes preserve future completion and ordered result assembly.- Multipart cleanup: Cancellation and waiting before abort introduce no identified interaction with the read contracts.
get_file()and tests: Remote opening still precedes local creation/truncation. The new range, prefix, version, and cache tests remain consistent with the rebased implementation and fixtures.
Regressions: None found.
Additional pre-existing issues: None identified within the inspected scope.
Static review only; no edits, builds, tests, GitHub, or network access. HEAD remains fd97af364bdbce5ece2c015124da686acaa0f582, with a clean worktree.
#987 pinned get_file() of a prefix to raise FileNotFoundError, so that no empty local file is written. As in fsspec's get_file(), which recursive get() relies on, the prefix now creates the local directory; test_get_file_directory covers it and that no file is written. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
WHAT
Byte ranges follow slice semantics without sending S3 a range that it would answer with the whole object or an error.
S3FileSystem.cat_file():start >= end(both non-negative) returnsb""without a GetObject request, afterinfo()confirms that the object exists (a missing object or a prefix raisesFileNotFoundError, as on master).bytes=N-whenendisNone. S3 clamps an end past the object itself, and itsInvalidRangeanswer for a range that starts at or past the end becomesb"". Only that S3 error code is treated this way, and only for ranged requests.info()as before, now with slice clamping (slice(start, end).indices(size)). A prefix (whichinfo()reports as a directory of size 0) raisesFileNotFoundError.info(), so a cached size cannot hide bytes added since, and a key ending in/is read by its exact key.cat_ranges()(fsspec's default, which callscat_file()) andAioS3FileSystem._cat_file()(which delegates tocat_file()) behave the same.S3File._fetch_range()clamps the end to the size pinned at open and returnsb""for an empty range, before the range is split for parallel requests.S3FileraisesFileNotFoundErrorwhen a prefix is opened for reading, whatever the cache type.get_file()opens the remote file before the local one, so a failure detected byopen()creates no empty local file.S3FileSystem._get_object()raisesValueErrorfor an empty range instead of sendingbytes=n-(n-1). It andS3File._format_ranges()accept an open end ((start, None))._get_object(),_fetch_range()and_format_ranges(). Thecat_file()andS3Filedocstrings state the range semantics and theFileNotFoundError.Behavior changes for the release notes:
cat_file()andcat_ranges()returnb""for an empty, reversed or past-the-end range instead of the whole object.cat_file(path, start, end)withstartat or past the end of the object andend > startreturnsb""instead of raisingOSError: [Errno 22] InvalidRange.cat_file()with a non-empty non-negative range no longer looks upinfo(). Before, every ranged read calledinfo(), which sends HeadObject (and ListObjectsV2 for a missing key) unless the path is cached.open(prefix, "rb")raisesFileNotFoundErrorwhen opening. Before, the default"bytes"cache opened it and readb""; with"all"and"first", GetObject raisedFileNotFoundErrorwhen the data was fetched.get_file()of a prefix raisesFileNotFoundErrorinstead of writing an empty file.cache_type="first","mmap", or"none"withmax_workers > 1no longer return duplicated data, raiseIndexError, or raiseInvalidRangeat the end of the object.WHY
Fixes #970.
S3 ignores a
Rangewhose last byte precedes its first byte and answers with HTTP 200 and the whole object, and answers a range starting at or past the end withInvalidRange.fsspec caches request empty ranges at the end of a file, so
cache_type="first"returned the data twice,"mmap"raisedIndexError: mmap slice assignment is wrong size, and"none"withmax_workers > 1sent a part starting past the end.The issue proposed clamping every range against the object size. The independent review showed that the size from
info()can be stale (listing cache), synthesized for a prefix, or looked up without a trailing/. So size-dependent edges are left to S3, and the size is used only where negative offsets need it.TEST
Tested commit: fd97af3 (Python 3.13.1, fsspec 2026.9.0).
just lint: passed.uv run --env-file .env pytest -n 4 tests/pyathena/filesystem/: 331 passed (rebased onto6f258501).TestS3FileSystem. The client-level GetObject fake keeps the real_call()error translation. It answersInvalidRange(HTTP 416) for a start at or past the end and fails on a range that S3 would answer with the whole object.test_cat_file_range: 14(start, end)pairs on a 10-byte object compare withdata[start:end]. The test asserts thatinfo()is called exactly for negative offsets and empty ranges, and that an empty range sends no GetObject request.test_cat_file_range_stale_size: a cached size of 10 for a 20-byte object still reads[10:20]and[15:].test_cat_file_range_errors:NoSuchKeywith a range raisesFileNotFoundError;InvalidRangewithout a range is raised.test_cat_file_range_directory: negative offsets and an empty range on a prefix raiseFileNotFoundErrorwithout a request.test_cat_file_empty_range_missing:(0, 0),(5, 3)and(None, 0)on a missing key raiseFileNotFoundError.test_cat_ranges_range,test_get_object_empty_range, andtest_format_ranges(open end).test_read_to_end:first(10 bytes),mmap(32 bytes,block_size=16) andnonewithmax_workers=4(40 bytes,block_size=16), reading up to and past the end.test_open_directory(bytes,all,first) andtest_get_file_directory(no local file left).aa0fc914).test_cat_file_version_idnow uses a negative range, because non-negative ranges no longer callinfo().bytes=5-4,bytes=10-9,bytes=12-9andbytes=0--1returned 200 with the whole object;bytes=12-19andbytes=10-19returnedInvalidRange;bytes=5-100returned 206 withb"56789".cat_file()cases,cat_ranges(),cache_type="first"(read()andget_file()),"mmap"and"none"withmax_workers=4returned the slice results.b""for(0, None),(0, 5),(-5, None)and the whole object, and for thebytes,all,firstandmmapcaches./readb"a"for[0:1]. An object grown afterinfo()was cached read the new bytes.open(prefix)raisedFileNotFoundErrorwithbytes,allandfirst, andget_file(prefix)raised without leaving a local file.FileNotFoundErrorfor(0, 0),(5, 3)and(None, 0); an existing object's(5, 5)returnsb"".get()of a prefix fails on master and on this branch, with a different error; that is Recursive get(), copy() and mv() mishandle directory entries #974.Not covered:
AioS3FileSystem._cat_file()has no separate test; it delegates tocat_file().info()strips a trailing/, so keys ending in/are still not addressable through negative offsets oropen()(Objects whose keys end in '/' are listed as files but cannot be opened or read with negative offsets #994).🤖 Generated with Claude Code