From a906f0d5ccbc2d22a16977245cee56885eb9afcd Mon Sep 17 00:00:00 2001 From: laughingman7743 Date: Sat, 3 Oct 2026 16:56:47 +0900 Subject: [PATCH 1/5] Clamp byte ranges to the object in cat_file() and S3File reads 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 --- pyathena/filesystem/s3.py | 59 +++++++++++++---- tests/pyathena/filesystem/test_s3.py | 95 ++++++++++++++++++++++++++++ 2 files changed, 140 insertions(+), 14 deletions(-) diff --git a/pyathena/filesystem/s3.py b/pyathena/filesystem/s3.py index 2bbda039c..4f482849d 100644 --- a/pyathena/filesystem/s3.py +++ b/pyathena/filesystem/s3.py @@ -1458,6 +1458,10 @@ def cat_file( ) -> bytes: """Read the contents of an S3 object with GetObject. + ``start`` and ``end`` select bytes like a slice: they are clamped to + the object, and an empty range returns ``b""`` without a GetObject + request. + Args: path: S3 path (s3://bucket/key) of the object. start: Byte offset to start reading at. A negative value counts @@ -1477,20 +1481,10 @@ def cat_file( version_id = path_version_id if start is not None or end is not None: size = self.info(path, version_id=version_id).get("size", 0) - if start is None: - range_start = 0 - elif start < 0: - range_start = size + start - else: - range_start = start - - if end is None: - range_end = size - elif end < 0: - range_end = size + end - else: - range_end = end - + # S3 would return the whole object for an empty range. + range_start, range_end, _ = slice(start, end).indices(size) + if range_start >= range_end: + return b"" ranges = (range_start, range_end) else: ranges = None @@ -2083,8 +2077,28 @@ def _get_object( version_id: str | None = None, **kwargs, ) -> tuple[int, bytes]: + """Read an object or a byte range of it with GetObject. + + Args: + bucket: The bucket name. + key: The object key. + ranges: The ``(start, end)`` byte range to read, with an exclusive + end, or ``None`` to read the whole object. + version_id: The version ID to read, or ``None`` for the latest. + **kwargs: Additional parameters passed to the GetObject API. + + Returns: + Tuple of the start of the range (0 for the whole object) and the + bytes read. + + Raises: + ValueError: If the range is empty. S3 ignores a range whose last + byte precedes its first byte and returns the whole object. + """ request = {"Bucket": bucket, "Key": key} if ranges: + if ranges[0] >= ranges[1]: + raise ValueError(f"Invalid empty range: {ranges}.") range_ = S3File._format_ranges(ranges) request.update({"Range": range_}) else: @@ -2602,6 +2616,23 @@ def setxattr(self, copy_kwargs: dict[str, Any] | None = None, **kwargs) -> None: self.fs.setxattr(self.path, copy_kwargs=copy_kwargs, **kwargs) def _fetch_range(self, start: int, end: int) -> bytes: + """Read a byte range of the object for the fsspec cache. + + The range is clamped to the size of the object, since fsspec caches + may request a range that is empty or reaches past the end of the + object. S3 would answer the former with the whole object and a range + starting past the end with an ``InvalidRange`` error. + + Args: + start: The offset of the first byte to read. + end: The offset to stop reading at (exclusive). + + Returns: + The bytes read, empty if the clamped range is empty. + """ + end = min(end, self.size) + if start >= end: + return b"" ranges = self._get_ranges( start, end, max_workers=self.max_workers, worker_block_size=self.blocksize ) diff --git a/tests/pyathena/filesystem/test_s3.py b/tests/pyathena/filesystem/test_s3.py index 95b47d016..8c29cc46b 100644 --- a/tests/pyathena/filesystem/test_s3.py +++ b/tests/pyathena/filesystem/test_s3.py @@ -3,6 +3,7 @@ import gc import io import os +import re import sys import tempfile import threading @@ -651,6 +652,100 @@ def test_cat_file_version_id(self, path, expected): VersionId=expected, ) + def _make_object_fs(self, data): + # A filesystem holding one object at s3://bucket/key that answers + # GetObject like S3, failing on a range that S3 would not answer with + # exactly the requested bytes: S3 returns the whole object when the + # last byte precedes the first one, and InvalidRange when the first + # byte is past the end. The requested ranges are recorded. + fs = self._make_fs() + fs.default_cache_type = "bytes" + fs.info = mock.MagicMock(return_value=self._file_object("key")) + fs.info.return_value.size = len(data) + ranges = [] + + def call(method, **request): + assert method is fs._client.get_object + range_ = request.get("Range") + ranges.append(range_) + if range_ is None: + return {"Body": io.BytesIO(data)} + match = re.fullmatch(r"bytes=(\d+)-(\d+)", range_) + assert match, range_ + first, last = int(match[1]), int(match[2]) + assert first <= last, range_ + assert first < len(data), range_ + return {"Body": io.BytesIO(data[first : last + 1])} + + fs._call.side_effect = call + return fs, ranges + + @pytest.mark.parametrize( + ("start", "end"), + [ + (None, None), + (None, 5), + (5, None), + (1, -1), + (-5, None), + (5, 100), + (-100, 5), + # Empty ranges. + (0, 0), + (5, 5), + (7, 3), + (10, None), + (12, None), + (12, 20), + (None, -20), + ], + ) + def test_cat_file_range(self, start, end): + data = b"0123456789" + fs, ranges = self._make_object_fs(data) + + # The range selects bytes like a slice. + assert fs.cat_file("s3://bucket/key", start=start, end=end) == data[start:end] + if not data[start:end]: + assert ranges == [] + + def test_cat_ranges_range(self): + fs, ranges = self._make_object_fs(b"0123456789") + + assert fs.cat_ranges(["s3://bucket/key"] * 3, [5, 0, -100], [5, 3, 5]) == [ + b"", + b"012", + b"01234", + ] + assert sorted(ranges) == ["bytes=0-2", "bytes=0-4"] + + def test_get_object_empty_range(self): + fs = self._make_fs() + + with pytest.raises(ValueError, match="empty range"): + fs._get_object("bucket", "key", ranges=(5, 5)) + fs._call.assert_not_called() + + @pytest.mark.parametrize( + ("size", "offset", "open_kwargs"), + [ + # FirstChunkCache fetches an empty range at the end of the object. + (10, 10, {"cache_type": "first"}), + # MMapCache fetches an empty last block. + (32, 16, {"cache_type": "mmap", "block_size": 16}), + # A read past the end is split into ranges for parallel requests. + (40, 20, {"cache_type": "none", "block_size": 16, "max_workers": 4}), + ], + ) + def test_read_to_end(self, size, offset, open_kwargs): + data = bytes(range(size)) + fs, _ = self._make_object_fs(data) + + with fs.open("s3://bucket/key", "rb", **open_kwargs) as f: + assert f.read(offset) == data[:offset] + assert f.read(size) == data[offset:] + assert f.read(size) == b"" + def test_finish_multipart_upload(self): fs = self._make_fs() fs._complete_multipart_upload = mock.MagicMock() From 793565c86301f214212e4ad596e7365c1f966f06 Mon Sep 17 00:00:00 2001 From: laughingman7743 Date: Sat, 3 Oct 2026 17:00:16 +0900 Subject: [PATCH 2/5] Keep FileNotFoundError for a ranged cat_file() of a prefix 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 --- pyathena/filesystem/s3.py | 11 +++++++++-- tests/pyathena/filesystem/test_s3.py | 10 ++++++++++ 2 files changed, 19 insertions(+), 2 deletions(-) diff --git a/pyathena/filesystem/s3.py b/pyathena/filesystem/s3.py index 4f482849d..7fefcb593 100644 --- a/pyathena/filesystem/s3.py +++ b/pyathena/filesystem/s3.py @@ -1474,15 +1474,22 @@ def cat_file( Returns: The bytes read from the object. + + Raises: + FileNotFoundError: If the path is not an object. """ bucket, key, path_version_id = self.parse_path(path) version_id = kwargs.pop("version_id", None) if path_version_id: version_id = path_version_id if start is not None or end is not None: - size = self.info(path, version_id=version_id).get("size", 0) + info = self.info(path, version_id=version_id) + if info.get("type") == S3ObjectType.S3_OBJECT_TYPE_DIRECTORY: + # There is no object to read, as GetObject reports without + # a range. + raise FileNotFoundError(path) # S3 would return the whole object for an empty range. - range_start, range_end, _ = slice(start, end).indices(size) + range_start, range_end, _ = slice(start, end).indices(info.get("size", 0)) if range_start >= range_end: return b"" ranges = (range_start, range_end) diff --git a/tests/pyathena/filesystem/test_s3.py b/tests/pyathena/filesystem/test_s3.py index 8c29cc46b..324244f7e 100644 --- a/tests/pyathena/filesystem/test_s3.py +++ b/tests/pyathena/filesystem/test_s3.py @@ -709,6 +709,16 @@ def test_cat_file_range(self, start, end): if not data[start:end]: assert ranges == [] + @pytest.mark.parametrize(("start", "end"), [(0, 5), (5, None)]) + def test_cat_file_range_directory(self, start, end): + fs = self._make_fs() + fs.info = mock.MagicMock(return_value=S3FileSystem._directory_object("bucket", "dir")) + + # A prefix is not read as an empty object. + with pytest.raises(FileNotFoundError): + fs.cat_file("s3://bucket/dir", start=start, end=end) + fs._call.assert_not_called() + def test_cat_ranges_range(self): fs, ranges = self._make_object_fs(b"0123456789") From 721a0e282856e8790c4882b27093f3a76033683e Mon Sep 17 00:00:00 2001 From: laughingman7743 Date: Sat, 3 Oct 2026 17:24:33 +0900 Subject: [PATCH 3/5] Leave size-dependent range edges to S3 and reject prefixes in open() 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 --- pyathena/filesystem/s3.py | 85 +++++++++++++++------- tests/pyathena/filesystem/test_s3.py | 101 ++++++++++++++++++++++----- 2 files changed, 140 insertions(+), 46 deletions(-) diff --git a/pyathena/filesystem/s3.py b/pyathena/filesystem/s3.py index 7fefcb593..bdf43744b 100644 --- a/pyathena/filesystem/s3.py +++ b/pyathena/filesystem/s3.py @@ -1458,9 +1458,11 @@ def cat_file( ) -> bytes: """Read the contents of an S3 object with GetObject. - ``start`` and ``end`` select bytes like a slice: they are clamped to - the object, and an empty range returns ``b""`` without a GetObject - request. + ``start`` and ``end`` select bytes like a slice of the object: an + empty range, or one that starts at or past the end of the object, + returns ``b""``, and an end past the object reads up to its end. + Non-negative offsets are sent to S3 as they are; a negative offset is + resolved against the size from :meth:`info`. Args: path: S3 path (s3://bucket/key) of the object. @@ -1476,33 +1478,44 @@ def cat_file( The bytes read from the object. Raises: - FileNotFoundError: If the path is not an object. + FileNotFoundError: If the key does not exist. """ bucket, key, path_version_id = self.parse_path(path) version_id = kwargs.pop("version_id", None) if path_version_id: version_id = path_version_id - if start is not None or end is not None: + if (start is not None and start < 0) or (end is not None and end < 0): info = self.info(path, version_id=version_id) if info.get("type") == S3ObjectType.S3_OBJECT_TYPE_DIRECTORY: - # There is no object to read, as GetObject reports without - # a range. + # There is no object to read, as GetObject reports for the + # other ranges. 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)) - if range_start >= range_end: - return b"" - ranges = (range_start, range_end) - else: - ranges = None + start, end, _ = slice(start, end).indices(info.get("size", 0)) - return self._get_object( - bucket=bucket, - key=cast(str, key), - ranges=ranges, - version_id=version_id, - **kwargs, - )[1] + ranges: tuple[int, int | None] | None = None + if start is not None or end is not None: + start = start or 0 + if end is not None and start >= end: + # S3 would return the whole object for an empty range. + return b"" + ranges = (start, end) + try: + return self._get_object( + bucket=bucket, + key=cast(str, key), + ranges=ranges, + version_id=version_id, + **kwargs, + )[1] + except OSError as e: + if ( + ranges + and isinstance(e.__cause__, botocore.exceptions.ClientError) + and S3ClientError(e.__cause__).code == "InvalidRange" + ): + # The range starts at or past the end of the object. + return b"" + raise def put_file(self, lpath: str, rpath: str, callback=_DEFAULT_CALLBACK, **kwargs): """Upload a local file to S3. @@ -1568,7 +1581,9 @@ def get_file(self, rpath: str, lpath: str, callback=_DEFAULT_CALLBACK, outfile=N if os.path.isdir(lpath): return - with open(lpath, "wb") as local, self.open(rpath, "rb", **kwargs) as remote: + # The remote file is opened first so that no local file is left + # behind when it does not exist. + with self.open(rpath, "rb", **kwargs) as remote, open(lpath, "wb") as local: callback.set_size(remote.size) while data := remote.read(remote.blocksize): local.write(data) @@ -2080,7 +2095,7 @@ def _get_object( self, bucket: str, key: str, - ranges: tuple[int, int] | None = None, + ranges: tuple[int, int | None] | None = None, version_id: str | None = None, **kwargs, ) -> tuple[int, bytes]: @@ -2090,7 +2105,8 @@ def _get_object( bucket: The bucket name. key: The object key. ranges: The ``(start, end)`` byte range to read, with an exclusive - end, or ``None`` to read the whole object. + end or ``None`` to read to the end of the object, or ``None`` + to read the whole object. version_id: The version ID to read, or ``None`` for the latest. **kwargs: Additional parameters passed to the GetObject API. @@ -2104,7 +2120,7 @@ def _get_object( """ request = {"Bucket": bucket, "Key": key} if ranges: - if ranges[0] >= ranges[1]: + if ranges[1] is not None and ranges[0] >= ranges[1]: raise ValueError(f"Invalid empty range: {ranges}.") range_ = S3File._format_ranges(ranges) request.update({"Range": range_}) @@ -2291,6 +2307,8 @@ def __init__( **kwargs: Accepted for compatibility; not used. Raises: + FileNotFoundError: If no object exists at the path when reading, + including when the path is a prefix. ValueError: If the path has no key, the version IDs do not match, a version is given for writing, or the block size is not between ``MULTIPART_UPLOAD_MIN_PART_SIZE`` and @@ -2342,6 +2360,9 @@ def __init__( # Looked up before the base class initializer, which would # 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. + raise FileNotFoundError(path) if fs.version_aware and not self.version_id: # Pin the version observed at open time so that reads are # consistent even if the object is overwritten. info() heads @@ -2667,8 +2688,18 @@ def _fetch_range(self, start: int, end: int) -> bytes: return object_ @staticmethod - def _format_ranges(ranges: tuple[int, int]): - return f"bytes={ranges[0]}-{ranges[1] - 1}" + def _format_ranges(ranges: tuple[int, int | None]) -> str: + """Format a byte range as the value of an HTTP ``Range`` header. + + Args: + ranges: The ``(start, end)`` byte range, with an exclusive end or + ``None`` for the end of the object. + + Returns: + The range, such as ``bytes=0-99`` or ``bytes=100-``. + """ + start, end = ranges + return f"bytes={start}-" if end is None else f"bytes={start}-{end - 1}" @staticmethod def _get_ranges( diff --git a/tests/pyathena/filesystem/test_s3.py b/tests/pyathena/filesystem/test_s3.py index 324244f7e..008c53a2c 100644 --- a/tests/pyathena/filesystem/test_s3.py +++ b/tests/pyathena/filesystem/test_s3.py @@ -18,6 +18,7 @@ from types import SimpleNamespace from unittest import mock +import botocore.exceptions import pytest from fsspec import Callback @@ -639,10 +640,10 @@ def test_cat_file_version_id(self, path, expected): fs._client.get_object, Bucket="bucket", Key="key", VersionId=expected ) - # A range is resolved against the size of the same version. + # A negative offset is resolved against the size of the same version. fs._call.reset_mock() fs._call.return_value = {"Body": io.BytesIO(b"ta")} - assert fs.cat_file(path, start=2, end=4, version_id="v1") == b"ta" + assert fs.cat_file(path, start=-8, end=-6, version_id="v1") == b"ta" fs.info.assert_called_once_with(path, version_id=expected) fs._call.assert_called_once_with( fs._client.get_object, @@ -653,37 +654,45 @@ def test_cat_file_version_id(self, path, expected): ) def _make_object_fs(self, data): - # A filesystem holding one object at s3://bucket/key that answers - # GetObject like S3, failing on a range that S3 would not answer with - # exactly the requested bytes: S3 returns the whole object when the - # last byte precedes the first one, and InvalidRange when the first - # byte is past the end. The requested ranges are recorded. + # A filesystem holding one object at s3://bucket/key whose client + # answers GetObject like S3: InvalidRange when the range starts at + # or past the end of the object, and a failure on a range that S3 + # would answer with the whole object (last byte before the first). + # info() reports the given size, and the requested ranges are + # recorded. fs = self._make_fs() fs.default_cache_type = "bytes" + fs._call = functools.partial(S3FileSystem._call, fs) fs.info = mock.MagicMock(return_value=self._file_object("key")) fs.info.return_value.size = len(data) ranges = [] - def call(method, **request): - assert method is fs._client.get_object + def get_object(**request): range_ = request.get("Range") ranges.append(range_) if range_ is None: return {"Body": io.BytesIO(data)} - match = re.fullmatch(r"bytes=(\d+)-(\d+)", range_) + match = re.fullmatch(r"bytes=(\d+)-(\d*)", range_) assert match, range_ - first, last = int(match[1]), int(match[2]) - assert first <= last, range_ - assert first < len(data), range_ + first = int(match[1]) + last = int(match[2]) if match[2] else len(data) - 1 + assert not match[2] or first <= last, range_ + if first >= len(data): + raise botocore.exceptions.ClientError( + { + "Error": {"Code": "InvalidRange", "Message": "Not satisfiable"}, + "ResponseMetadata": {"HTTPStatusCode": 416}, + }, + "GetObject", + ) return {"Body": io.BytesIO(data[first : last + 1])} - fs._call.side_effect = call + fs._client.get_object.side_effect = get_object return fs, ranges @pytest.mark.parametrize( ("start", "end"), [ - (None, None), (None, 5), (5, None), (1, -1), @@ -698,6 +707,7 @@ def call(method, **request): (12, None), (12, 20), (None, -20), + (-3, -5), ], ) def test_cat_file_range(self, start, end): @@ -706,10 +716,42 @@ def test_cat_file_range(self, start, end): # The range selects bytes like a slice. assert fs.cat_file("s3://bucket/key", start=start, end=end) == data[start:end] - if not data[start:end]: + # Only a negative offset needs the size of the object. + assert fs.info.called == ((start or 0) < 0 or (end or 0) < 0) + if start is not None and end is not None and 0 <= start >= end >= 0: assert ranges == [] - @pytest.mark.parametrize(("start", "end"), [(0, 5), (5, None)]) + def test_cat_file_range_stale_size(self): + fs, ranges = self._make_object_fs(b"0123456789abcdefghij") + # A cached entry from before the object grew. + fs.info.return_value.size = 10 + + assert fs.cat_file("s3://bucket/key", start=10, end=20) == b"abcdefghij" + assert fs.cat_file("s3://bucket/key", start=15) == b"fghij" + assert ranges == ["bytes=10-19", "bytes=15-"] + + def test_cat_file_range_errors(self): + fs = self._make_fs() + fs._call = functools.partial(S3FileSystem._call, fs) + fs._client.get_object.side_effect = botocore.exceptions.ClientError( + {"Error": {"Code": "NoSuchKey", "Message": "No such key"}}, "GetObject" + ) + + # Errors other than InvalidRange are raised. + with pytest.raises(FileNotFoundError): + fs.cat_file("s3://bucket/dir", start=0, end=5) + fs._client.get_object.side_effect = botocore.exceptions.ClientError( + { + "Error": {"Code": "InvalidRange", "Message": "Not satisfiable"}, + "ResponseMetadata": {"HTTPStatusCode": 416}, + }, + "GetObject", + ) + # InvalidRange without a range is not taken as an empty read. + with pytest.raises(OSError, match="Not satisfiable"): + fs.cat_file("s3://bucket/key") + + @pytest.mark.parametrize(("start", "end"), [(-5, None), (0, -1)]) def test_cat_file_range_directory(self, start, end): fs = self._make_fs() fs.info = mock.MagicMock(return_value=S3FileSystem._directory_object("bucket", "dir")) @@ -719,15 +761,25 @@ def test_cat_file_range_directory(self, start, end): fs.cat_file("s3://bucket/dir", start=start, end=end) fs._call.assert_not_called() + def test_get_file_directory(self, tmp_path): + fs = self._make_fs() + fs.default_cache_type = "bytes" + fs.info = mock.MagicMock(return_value=S3FileSystem._directory_object("bucket", "dir")) + + with pytest.raises(FileNotFoundError): + fs.get_file("s3://bucket/dir", str(tmp_path / "dir")) + assert list(tmp_path.iterdir()) == [] + def test_cat_ranges_range(self): fs, ranges = self._make_object_fs(b"0123456789") - assert fs.cat_ranges(["s3://bucket/key"] * 3, [5, 0, -100], [5, 3, 5]) == [ + assert fs.cat_ranges(["s3://bucket/key"] * 4, [5, 0, -100, 12], [5, 3, 5, 20]) == [ b"", b"012", b"01234", + b"", ] - assert sorted(ranges) == ["bytes=0-2", "bytes=0-4"] + assert sorted(ranges) == ["bytes=0-2", "bytes=0-4", "bytes=12-19"] def test_get_object_empty_range(self): fs = self._make_fs() @@ -756,6 +808,16 @@ def test_read_to_end(self, size, offset, open_kwargs): assert f.read(size) == data[offset:] assert f.read(size) == b"" + @pytest.mark.parametrize("cache_type", ["bytes", "all", "first"]) + def test_open_directory(self, cache_type): + fs = self._make_fs() + fs.info = mock.MagicMock(return_value=S3FileSystem._directory_object("bucket", "dir")) + + # A prefix is not read as an empty object. + with pytest.raises(FileNotFoundError): + fs.open("s3://bucket/dir", "rb", cache_type=cache_type) + fs._call.assert_not_called() + def test_finish_multipart_upload(self): fs = self._make_fs() fs._complete_multipart_upload = mock.MagicMock() @@ -2527,6 +2589,7 @@ def test_get_ranges(self, start, end, max_workers, worker_block_size, ranges): def test_format_ranges(self): assert S3File._format_ranges((0, 100)) == "bytes=0-99" + assert S3File._format_ranges((100, None)) == "bytes=100-" @pytest.mark.parametrize("autocommit", [True, False]) def test_upload_chunk_small_file(self, autocommit): From ee4f5ee7056b188fc5967a7e1347594ac6bb517f Mon Sep 17 00:00:00 2001 From: laughingman7743 Date: Sat, 3 Oct 2026 17:26:13 +0900 Subject: [PATCH 4/5] Simplify the no-request condition in test_cat_file_range Co-Authored-By: Claude Opus 5.5 --- tests/pyathena/filesystem/test_s3.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/pyathena/filesystem/test_s3.py b/tests/pyathena/filesystem/test_s3.py index 008c53a2c..d477d7544 100644 --- a/tests/pyathena/filesystem/test_s3.py +++ b/tests/pyathena/filesystem/test_s3.py @@ -718,7 +718,7 @@ def test_cat_file_range(self, start, end): assert fs.cat_file("s3://bucket/key", start=start, end=end) == data[start:end] # Only a negative offset needs the size of the object. assert fs.info.called == ((start or 0) < 0 or (end or 0) < 0) - if start is not None and end is not None and 0 <= start >= end >= 0: + if start is not None and end is not None and 0 <= end <= start: assert ranges == [] def test_cat_file_range_stale_size(self): From fd97af364bdbce5ece2c015124da686acaa0f582 Mon Sep 17 00:00:00 2001 From: laughingman7743 Date: Sat, 3 Oct 2026 17:38:32 +0900 Subject: [PATCH 5/5] Check that the object exists for an empty cat_file() range 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 --- pyathena/filesystem/s3.py | 14 ++++++++++---- tests/pyathena/filesystem/test_s3.py | 20 ++++++++++++++++---- 2 files changed, 26 insertions(+), 8 deletions(-) diff --git a/pyathena/filesystem/s3.py b/pyathena/filesystem/s3.py index bdf43744b..de6dadb10 100644 --- a/pyathena/filesystem/s3.py +++ b/pyathena/filesystem/s3.py @@ -1462,7 +1462,8 @@ def cat_file( empty range, or one that starts at or past the end of the object, returns ``b""``, and an end past the object reads up to its end. Non-negative offsets are sent to S3 as they are; a negative offset is - resolved against the size from :meth:`info`. + resolved against the size from :meth:`info`, which also checks that + the object exists for an empty range. Args: path: S3 path (s3://bucket/key) of the object. @@ -1484,7 +1485,12 @@ def cat_file( 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): + if (start is not None and start < 0) or ( + end is not None and (end < 0 or (start or 0) >= end) + ): + # A negative offset needs the size of the object, and an 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: # There is no object to read, as GetObject reports for the @@ -1581,8 +1587,8 @@ def get_file(self, rpath: str, lpath: str, callback=_DEFAULT_CALLBACK, outfile=N if os.path.isdir(lpath): return - # The remote file is opened first so that no local file is left - # behind when it does not exist. + # The remote file is opened first so that no local file is created + # when open() finds no object at the path. with self.open(rpath, "rb", **kwargs) as remote, open(lpath, "wb") as local: callback.set_size(remote.size) while data := remote.read(remote.blocksize): diff --git a/tests/pyathena/filesystem/test_s3.py b/tests/pyathena/filesystem/test_s3.py index d477d7544..f73177eff 100644 --- a/tests/pyathena/filesystem/test_s3.py +++ b/tests/pyathena/filesystem/test_s3.py @@ -716,9 +716,11 @@ def test_cat_file_range(self, start, end): # The range selects bytes like a slice. assert fs.cat_file("s3://bucket/key", start=start, end=end) == data[start:end] - # Only a negative offset needs the size of the object. - assert fs.info.called == ((start or 0) < 0 or (end or 0) < 0) - if start is not None and end is not None and 0 <= end <= start: + negative = (start or 0) < 0 or (end or 0) < 0 + empty = end is not None and 0 <= end <= (start or 0) + # Only a negative offset or an empty range looks up the object. + assert fs.info.called == (negative or empty) + if empty: assert ranges == [] def test_cat_file_range_stale_size(self): @@ -751,7 +753,17 @@ def test_cat_file_range_errors(self): with pytest.raises(OSError, match="Not satisfiable"): fs.cat_file("s3://bucket/key") - @pytest.mark.parametrize(("start", "end"), [(-5, None), (0, -1)]) + @pytest.mark.parametrize(("start", "end"), [(0, 0), (5, 3), (None, 0)]) + def test_cat_file_empty_range_missing(self, start, end): + fs = self._make_fs() + fs.info = mock.MagicMock(side_effect=FileNotFoundError("bucket/missing")) + + # An empty range of a missing object is not read as empty. + with pytest.raises(FileNotFoundError): + fs.cat_file("s3://bucket/missing", start=start, end=end) + fs._call.assert_not_called() + + @pytest.mark.parametrize(("start", "end"), [(-5, None), (0, -1), (5, 5)]) def test_cat_file_range_directory(self, start, end): fs = self._make_fs() fs.info = mock.MagicMock(return_value=S3FileSystem._directory_object("bucket", "dir"))