Skip to content

Clamp byte ranges to the object in cat_file() and S3File reads - #987

Merged
laughingman7743 merged 5 commits into
masterfrom
fix/970-range-clamping
Oct 3, 2026
Merged

laughingman7743 merged 5 commits into
masterfrom
fix/970-range-clamping

Conversation

@laughingman7743

@laughingman7743 laughingman7743 commented Oct 3, 2026 •

Copy link
Copy Markdown
Member

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) returns b"" without a GetObject request, after info() confirms that the object exists (a missing object or a prefix raises FileNotFoundError, as on master).
    • Other non-negative offsets are sent to S3 as they are, with an open-ended bytes=N- when end is None. S3 clamps an end past the object itself, and its InvalidRange answer for a range that starts at or past the end becomes b"". Only that S3 error code is treated this way, and only for ranged requests.
    • Negative offsets are resolved against the size from info() as before, now with slice clamping (slice(start, end).indices(size)). A prefix (which info() reports as a directory of size 0) raises FileNotFoundError.
    • Non-empty non-negative ranges no longer call 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 calls cat_file()) and AioS3FileSystem._cat_file() (which delegates to cat_file()) behave the same.
  • S3File._fetch_range() clamps the end to the size pinned at open and returns b"" for an empty range, before the range is split for parallel requests.
  • S3File raises FileNotFoundError when a prefix is opened for reading, whatever the cache type.
  • get_file() opens the remote file before the local one, so a failure detected by open() creates no empty local file.
  • S3FileSystem._get_object() raises ValueError for an empty range instead of sending bytes=n-(n-1). It and S3File._format_ranges() accept an open end ((start, None)).
  • Docstrings are added to _get_object(), _fetch_range() and _format_ranges(). The cat_file() and S3File docstrings state the range semantics and the FileNotFoundError.

Behavior changes for the release notes:

  • cat_file() and cat_ranges() return b"" for an empty, reversed or past-the-end range instead of the whole object.
  • cat_file(path, start, end) with start at or past the end of the object and end > start returns b"" instead of raising OSError: [Errno 22] InvalidRange.
  • cat_file() with a non-empty non-negative range no longer looks up info(). Before, every ranged read called info(), which sends HeadObject (and ListObjectsV2 for a missing key) unless the path is cached.
  • open(prefix, "rb") raises FileNotFoundError when opening. Before, the default "bytes" cache opened it and read b""; with "all" and "first", GetObject raised FileNotFoundError when the data was fetched. get_file() of a prefix raises FileNotFoundError instead of writing an empty file.
  • Reads with cache_type="first", "mmap", or "none" with max_workers > 1 no longer return duplicated data, raise IndexError, or raise InvalidRange at the end of the object.

WHY

Fixes #970.
S3 ignores a Range whose 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 with InvalidRange.
fsspec caches request empty ranges at the end of a file, so cache_type="first" returned the data twice, "mmap" raised IndexError: mmap slice assignment is wrong size, and "none" with max_workers > 1 sent 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 onto 6f258501).
  • New offline tests in TestS3FileSystem. The client-level GetObject fake keeps the real _call() error translation. It answers InvalidRange (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 with data[start:end]. The test asserts that info() 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: NoSuchKey with a range raises FileNotFoundError; InvalidRange without a range is raised.
    • test_cat_file_range_directory: negative offsets and an empty range on a prefix raise FileNotFoundError without a request.
    • test_cat_file_empty_range_missing: (0, 0), (5, 3) and (None, 0) on a missing key raise FileNotFoundError.
    • test_cat_ranges_range, test_get_object_empty_range, and test_format_ranges (open end).
    • test_read_to_end: first (10 bytes), mmap (32 bytes, block_size=16) and none with max_workers=4 (40 bytes, block_size=16), reading up to and past the end.
    • test_open_directory (bytes, all, first) and test_get_file_directory (no local file left).
    • 24 of these cases fail on master (aa0fc914).
    • test_cat_file_version_id now uses a negative range, because non-negative ranges no longer call info().
  • Live S3 checks with scripts outside the repository (objects created and removed in the staging bucket):
    • Raw GetObject on a 10-byte object: bytes=5-4, bytes=10-9, bytes=12-9 and bytes=0--1 returned 200 with the whole object; bytes=12-19 and bytes=10-19 returned InvalidRange; bytes=5-100 returned 206 with b"56789".
    • The issue's cat_file() cases, cat_ranges(), cache_type="first" (read() and get_file()), "mmap" and "none" with max_workers=4 returned the slice results.
    • A 0-byte object returned b"" for (0, None), (0, 5), (-5, None) and the whole object, and for the bytes, all, first and mmap caches.
    • A key ending in / read b"a" for [0:1]. An object grown after info() was cached read the new bytes. open(prefix) raised FileNotFoundError with bytes, all and first, and get_file(prefix) raised without leaving a local file.
    • A missing key and a prefix raise FileNotFoundError for (0, 0), (5, 3) and (None, 0); an existing object's (5, 5) returns b"".
    • Recursive 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.
    • These checks are not part of the test suite, to keep S3 integration tests cheap.

Not covered:

🤖 Generated with Claude Code

Comment thread pyathena/filesystem/s3.py
range_end = end

info = self.info(path, version_id=version_id)
if info.get("type") == S3ObjectType.S3_OBJECT_TYPE_DIRECTORY:

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_directory covers (0, 5) and (5, None).
  • The live S3 check returns FileNotFoundError for {}, start=0, end=5 and start=5 on a prefix.
  • just lint passed.
  • uv run --env-file .env pytest -n 4 tests/pyathena/filesystem/: 296 passed.

Comment thread pyathena/filesystem/s3.py Outdated
# 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))

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 for bytes=5-4, 10-9, 12-9 and 0--1, InvalidRange for 12-19 and 10-19, and 206 for 5-100.
  • cat_ranges() and AioS3FileSystem._cat_file() share the behavior: fsspec 2026.9.0 AbstractFileSystem.cat_ranges calls self.cat_file per range, and AsyncFileSystem._cat_ranges calls self._cat_file, which delegates to the sync cat_file(). Neither filesystem overrides cat_ranges.
  • "fsspec caches request empty ranges": FirstChunkCache._fetch calls the fetcher with (size, size) at the end of the file, MMapCache._fetch requests (32, 32) for an empty last block, and AbstractBufferedFile.read(n) passes loc + n unclamped.
  • Slice semantics as the contract: fsspec's MemoryFileSystem.cat_file returns data[start:end] for all the issue's cases. LocalFileSystem raises ValueError for 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).

Comment thread pyathena/filesystem/s3.py Outdated
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):

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. P2 — Literal keys ending in / can become unreadable.
    pyathena/filesystem/s3.py:1480
    Given an object dir/ containing b"abc" and no object dir, cat_file("s3://bucket/dir/", start=0, end=1) now raises FileNotFoundError. info() strips the trailing slash, checks dir, then identifies dir/ as a prefix. Previously the bounded GetObject used the correctly parsed key dir/ and returned b"a". The new directory rejection must distinguish an actual object from a synthetic prefix using the exact key.

  2. 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 returns b"" 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.

  3. P2 — The early return makes prefix-only paths appear as empty files with additional caches.
    pyathena/filesystem/s3.py:2614
    Given prefix/child but no object prefix, opening prefix with cache_type="all" now succeeds and reads b"". The constructor accepts directory metadata with size zero; AllBytes calls _fetch_range(0, 0), which now returns immediately. Previously that fetch reached GetObject and failed because the object was absent. cache_type="first" with read(1) has the same regression. Reject directory metadata before constructing the file cache.

  4. P3 — The new exception documentation overstates bucket handling.
    pyathena/filesystem/s3.py:1472
    cat_file("s3://bucket") without bounds bypasses the directory check and passes Key=None to botocore, producing ParamValidationError, not the documented FileNotFoundError. 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)): master b'a', branch FileNotFoundError. Confirmed regression.
  • 2 (cached size 10, object grown externally to 20, cat_file(start=10, end=20)): master b'abcdefghij', branch b''. Confirmed regression.
  • 3 (open(prefix, cache_type="all"/"first").read(1)): master FileNotFoundError, branch b''. Confirmed regression. With "bytes", both return b'', which is pre-existing.
  • 4: Confirmed. Without a range, cat_file("s3://bucket") raises botocore's ParamValidationError.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 calls info() for non-negative offsets. It returns b"" for start >= end without a request, and otherwise sends the range as given (open-ended bytes=N- when end is None). S3 clamps an end past the object itself. The InvalidRange answer for a start at or past the end becomes b"", identified by the S3 error code of the ClientError cause and only for ranged requests. Only negative offsets are resolved against info(), as on master; a prefix there raises FileNotFoundError.
  • 3: S3File.__init__ raises FileNotFoundError in read mode when info() reports a directory, for every cache type. This also fixes the pre-existing b"" 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_size and test_open_directory[all|first] cover findings 2 and 3, and test_cat_file_range asserts that info() is called only for negative offsets.
  • Live S3, finding 1: b'a'. Finding 2: b'abcdefghij'. Finding 3: FileNotFoundError for all, first and bytes.
  • 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"".

Comment thread pyathena/filesystem/s3.py
if (
ranges
and isinstance(e.__cause__, botocore.exceptions.ClientError)
and S3ClientError(e.__cause__).code == "InvalidRange"

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 and None offsets; open-ended ranges; a 0-byte object; prefixes; keys ending in /; version IDs (non-negative reads never used info() for the version sent to GetObject).
  • InvalidRange detection: _call() raises S3ClientError(e).os_error from e, so __cause__ is the ClientError. RetryConfig retries only THROTTLING_ERROR_CODES, so the 416 is not retried. Only ranged requests map it to b"".
  • _fetch_range(): it clamps to the size pinned at open, and reads are sent with IfMatch on the ETag, so the object cannot change underneath.
  • S3File.__init__: the directory check sits before version pinning, and a bucket path still raises ValueError first. AioS3File inherits 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.

Comment thread pyathena/filesystem/s3.py
# 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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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-100 returned 206 with b"56789".
  • "InvalidRange (HTTP 416)": live bytes=12-19 and bytes=10-19 returned InvalidRange with status 416.
  • "A 0-byte object returns b''": live, (0, None), (0, 5) and (-5, None), and the bytes, all, first and mmap caches.
  • Stale size, keys ending in /, open(prefix) and get_file(prefix): live runs as stated in the PR body.
  • The docstrings of cat_file(), _get_object(), _format_ranges(), _fetch_range() and S3File.__init__ (Raises) match the code.

Corrections to the PR body:

  • The open(prefix) release note said that "all"/"first" read b"" before. On master, they raised FileNotFoundError from 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_ranges over many ranges, as parquet readers do, benefits.
  • open() of a prefix now raises; on master it silently read b"". 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.

Comment thread pyathena/filesystem/s3.py
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)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. P2 — Regression: empty ranges suppress missing-object errors.
    pyathena/filesystem/s3.py:1491

    With an uncached, nonexistent key, fs.cat_file("s3://bucket/missing", start=0, end=0) now returns b"" without checking existence. Previously, info() raised FileNotFoundError. A prefix without an object or a nonexistent version similarly returns success. This also propagates through cat_ranges and the async wrappers, contradicting the new Raises documentation 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.

  2. P3 — New comment overstates protection against a pre-existing download failure.
    pyathena/filesystem/s3.py:1577

    Cache 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 raises FileNotFoundError, 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) returned b"" without any lookup. On master, info() raised FileNotFoundError.
  • 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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Repaired in 97ce192:

  • 1: an empty range (end is not None and (start or 0) >= end) now looks up the object with info(), as negative offsets do. A missing object or version raises FileNotFoundError from info(), 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)) and test_cat_file_range_directory[5-5] fail on 02f8ebdd.
  • test_cat_file_range now asserts that info() is called exactly for negative offsets and empty ranges.
  • Live S3: a missing key and a prefix raise FileNotFoundError for all three empty ranges, and an existing object's (5, 5) returns b"".

Self-review of this repair:

  • Round one (behavior): the repair adds an info() call only for empty ranges. Master called info() for every ranged read, so this restores master's existence check. Non-empty non-negative ranges still skip info().
  • Round two (claims): the cat_file() docstring now says that info() also checks existence for an empty range. The PR body is updated. The existence check may use cached metadata, as master's info() did.

Comment thread pyathena/filesystem/s3.py
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)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 return b"" before any GetObject. Non-empty non-negative ranges still skip info(), 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.

@laughingman7743
laughingman7743 marked this pull request as ready for review October 3, 2026 08:42
@laughingman7743
laughingman7743 marked this pull request as draft October 3, 2026 08:51
laughingman7743 and others added 5 commits October 3, 2026 17:51
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>
Comment thread pyathena/filesystem/s3.py
# 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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread pyathena/filesystem/s3.py
# 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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 selective InvalidRange handling 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’s none, first, mmap, bytes, and eager all cache 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.

@laughingman7743
laughingman7743 marked this pull request as ready for review October 3, 2026 08:57
@laughingman7743
laughingman7743 merged commit e162d7f into master Oct 3, 2026
12 checks passed
@laughingman7743
laughingman7743 deleted the fix/970-range-clamping branch October 3, 2026 09:11
laughingman7743 added a commit that referenced this pull request Oct 3, 2026
#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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

S3File reads and cat_file() return the whole object for empty or out-of-bounds ranges

1 participant