Read a negative cat_file() start without an end as a suffix range

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

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(

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):