diff --git a/docs/filesystem.md b/docs/filesystem.md index 0096ee03b..fd2439ead 100644 --- a/docs/filesystem.md +++ b/docs/filesystem.md @@ -98,6 +98,17 @@ the existing object also count toward the limit. A write with `open` that reache limit raises `ValueError` and aborts its multipart upload. Multipart copies with `cp` use parts large enough to stay within the limit. +Paths are normalized as in fsspec, which drops a trailing slash, so `info`, `isfile`, +and `open` treat `s3://YOUR_S3_BUCKET/dir/` as `s3://YOUR_S3_BUCKET/dir`: the object +`dir` if it exists, and otherwise the directory `dir`. An object whose key ends in a +slash, such as a folder marker, is therefore not a file for these methods. Opening +`dir/` for reading reads the object `dir` or raises `FileNotFoundError`, and opening it +for writing writes the object `dir`. A path with a `?versionId=` suffix keeps the slash +and refers to the object. `find`, and `ls` of the directory, list the object as a file +entry. `cat_file` uses the key as written. Without a `?versionId=` suffix, it reads +such an object without a range, with a non-empty range of non-negative offsets, or with +a negative `start` and no `end`, and raises `FileNotFoundError` for other ranges. + ## Error translation S3 error responses are translated into standard Python exceptions, so filesystem diff --git a/pyathena/filesystem/s3.py b/pyathena/filesystem/s3.py index 2eb0ced6a..d8293caf3 100644 --- a/pyathena/filesystem/s3.py +++ b/pyathena/filesystem/s3.py @@ -1497,9 +1497,11 @@ def cat_file( ``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`, which also checks that - the object exists for an empty range. + Non-negative offsets are sent to S3 as they are, and so is a negative + ``start`` without an ``end``, as a suffix range of the last bytes. + Other negative offsets are 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. @@ -1515,36 +1517,44 @@ def cat_file( The bytes read from the object. Raises: - FileNotFoundError: If the key does not exist. + FileNotFoundError: If the path has no key or the key does not + exist. """ bucket, key, path_version_id = self.parse_path(path) + if not key: + raise FileNotFoundError(path) 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 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 - # other ranges. - raise FileNotFoundError(path) - start, end, _ = slice(start, end).indices(info.get("size", 0)) - 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) + if start is not None and start < 0 and end is None: + # S3 returns the last bytes, or the whole object when it is + # shorter, without the size of the object. + ranges = (start, None) + else: + 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 or info.key != key: + # There is no object to read, as GetObject reports for + # the other ranges, or info() describes the key without + # the trailing slash of this one. + raise FileNotFoundError(path) + start, end, _ = slice(start, end).indices(info.get("size", 0)) + 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), + key=key, ranges=ranges, version_id=version_id, **kwargs, @@ -2154,14 +2164,15 @@ 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 to the end of the object, or ``None`` - to read the whole object. + end or ``None`` to read to the end of the object (the last + ``-start`` bytes for a negative start), 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. + Tuple of the start of the range as given (0 for the whole + object) and the bytes read. Raises: ValueError: If the range is empty. S3 ignores a range whose last @@ -2775,13 +2786,16 @@ def _format_ranges(ranges: tuple[int, int | None]) -> str: Args: ranges: The ``(start, end)`` byte range, with an exclusive end or - ``None`` for the end of the object. + ``None`` for the end of the object. A negative start with no + end selects the last ``-start`` bytes. Returns: - The range, such as ``bytes=0-99`` or ``bytes=100-``. + The range, such as ``bytes=0-99``, ``bytes=100-`` or ``bytes=-8``. """ start, end = ranges - return f"bytes={start}-" if end is None else f"bytes={start}-{end - 1}" + if end is None: + return f"bytes={start}" if start < 0 else f"bytes={start}-" + return 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 9c07424a0..058b67725 100644 --- a/tests/pyathena/filesystem/test_s3.py +++ b/tests/pyathena/filesystem/test_s3.py @@ -743,9 +743,10 @@ def test_cat_file_version_id(self, path, expected): def _make_object_fs(self, data): # 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). + # answers GetObject like S3: the last bytes for a suffix range, + # 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() @@ -760,6 +761,8 @@ def get_object(**request): ranges.append(range_) if range_ is None: return {"Body": io.BytesIO(data)} + if suffix := re.fullmatch(r"bytes=-(\d+)", range_): + return {"Body": io.BytesIO(data[-int(suffix[1]) :])} match = re.fullmatch(r"bytes=(\d+)-(\d*)", range_) assert match, range_ first = int(match[1]) @@ -785,6 +788,8 @@ def get_object(**request): (5, None), (1, -1), (-5, None), + (-10, None), + (-100, None), (5, 100), (-100, 5), # Empty ranges. @@ -804,10 +809,14 @@ 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] + suffix = (start or 0) < 0 and end is None 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) + # Only a negative offset with an end, a negative end, or an empty + # range looks up the object. + assert fs.info.called == ((negative and not suffix) or empty) + if suffix: + assert ranges == [f"bytes={start}"] if empty: assert ranges == [] @@ -818,7 +827,17 @@ def test_cat_file_range_stale_size(self): 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-"] + assert fs.cat_file("s3://bucket/key", start=-5) == b"fghij" + assert ranges == ["bytes=10-19", "bytes=15-", "bytes=-5"] + + def test_cat_file_suffix_range_key_ending_in_slash(self): + fs, _ = self._make_object_fs(b"abc") + # info() reports a key ending in "/" as a directory. + fs.info.return_value = S3FileSystem._directory_object("bucket", "dir") + + assert fs.cat_file("s3://bucket/dir/", start=-2) == b"bc" + fs._client.get_object.assert_called_once_with(Bucket="bucket", Key="dir/", Range="bytes=-2") + fs.info.assert_not_called() def test_cat_file_range_errors(self): fs = self._make_fs() @@ -851,7 +870,7 @@ def test_cat_file_empty_range_missing(self, start, end): 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)]) + @pytest.mark.parametrize(("start", "end"), [(-5, 3), (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")) @@ -861,6 +880,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() + @pytest.mark.parametrize(("start", "end"), [(None, None), (-1, None), (0, 5)]) + def test_cat_file_bucket(self, start, end): + fs = self._make_fs() + + with pytest.raises(FileNotFoundError): + fs.cat_file("s3://bucket/", start=start, end=end) + fs._call.assert_not_called() + + @pytest.mark.parametrize(("start", "end"), [(-2, -1), (0, -1), (5, 5)]) + def test_cat_file_range_key_ending_in_slash(self, start, end): + fs, ranges = self._make_object_fs(b"0123456789") + # info() of "dir/" describes the object "dir" when both exist. + fs.info.return_value = self._file_object("dir") + + # The size of "dir" is not used for the range of "dir/". + with pytest.raises(FileNotFoundError): + fs.cat_file("s3://bucket/dir/", start=start, end=end) + assert ranges == [] + def test_get_file_directory(self, tmp_path): fs = self._make_fs() fs.default_cache_type = "bytes" @@ -2837,6 +2875,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-" + assert S3File._format_ranges((-8, None)) == "bytes=-8" @pytest.mark.parametrize("autocommit", [True, False]) def test_upload_chunk_small_file(self, autocommit):