Skip to content

Count S3FileSystem.find() maxdepth levels like fsspec #956

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
laughingman7743 merged 3 commits into from
Oct 3, 2026
Merged

Count S3FileSystem.find() maxdepth levels like fsspec #956

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8f29beb
Count S3FileSystem.find() maxdepth levels like fsspec
laughingman7743 Oct 3, 2026
c1292e0
Compare find() results without depending on listing order
laughingman7743 Oct 3, 2026
54660a4
Drop the object-path fallback from find() with maxdepth
laughingman7743 Oct 3, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 25 additions & 2 deletions pyathena/filesystem/s3.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -717,6 +717,24 @@ def _find(
withdirs: bool | None = None,
**kwargs,
) -> list[S3Object]:
"""List the objects below a path, as described in ``find``.

Args:
path: S3 path to search under.
maxdepth: Maximum number of levels to descend, at least 1
(None for unlimited).
withdirs: Whether to include directories in the result.
**kwargs: Additional arguments including ``prefix`` and
``refresh``, as described in ``find``.

Returns:
The objects found, and the directories if ``withdirs`` is True.

Raises:
ValueError: If ``maxdepth`` is less than 1 or the path is the root.
"""
if maxdepth is not None and maxdepth < 1:
raise ValueError("maxdepth must be at least 1")
path = self._strip_protocol(path)
if path in ["", "/"]:
raise ValueError("Cannot traverse all files in S3.")
Expand All @@ -742,7 +760,7 @@ def _find(
result.append(item)

# Recursively explore subdirectory if depth allows
if maxdepth > 0:
if maxdepth > 1:

@laughingman7743 laughingman7743 Oct 3, 2026

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): FINDINGS (repaired)

Base e0e85da09435257a18c8b8832b3e10ba7840b697, head 8f29bebed998ae97316a24f28a15fd6f6a1a3a89; full diff (pyathena/filesystem/s3.py, tests/pyathena/filesystem/test_s3.py, tests/pyathena/filesystem/test_s3_async.py).

Covered:

  • Level counting against fsspec 2026.9.0 walk()/find(): maxdepth=1 lists only the current level, and each recursion passes maxdepth - 1 >= 1, so the new < 1 check never fires inside the recursion.
  • Callers that pass maxdepth through find(): fsspec glob() (its computed depth is at least 1 whenever maxdepth >= 1), expand_path() (used by S3FileSystem.rm() and fsspec copy()/get()/put()), du(), and the async _glob()/_expand_path()/_du() through AioS3FileSystem._find(). A ValueError from the sync _find() propagates through asyncio.to_thread.
  • Object-path fallback: it runs only when the current level lists nothing and the path has a key, and it returns only a file. A directory result from info() therefore cannot make the loop recurse into the same path. A subdirectory that becomes empty between listings costs one extra HEAD request and returns []. With prefix, it behaves the same as the unlimited branch (see the updated docstring).
  • Tests: the new unit tests fail on master and pass here; test_find_refresh_bypasses_cached_listings still reaches the subdirectory listing with maxdepth=2.

Findings:

  1. The new unit test compared find() results in listing order, which comes from _ls_dirs (common prefixes before contents) and is not a contract. Repaired in c1292e0: the multi-entry assertions compare sorted lists.
  2. Pre-existing, out of scope: AioS3FileSystem._rm() (pyathena/filesystem/s3_async.py:178) calls expand_path(p, recursive=recursive) without maxdepth, so maxdepth is ignored and _rm(path, recursive=True, maxdepth=1) deletes everything under the path. This PR does not change it; to be filed separately.
  3. PR text: the rm() consequence applies to the sync S3FileSystem.rm() only (see finding 2). The PR body will be corrected in round two's claim audit.

@laughingman7743 laughingman7743 Oct 3, 2026

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.

Round one on the repair (54660a4): CLEAN. git range-diff e0e85da0..c1292e03 e0e85da0..54660a49 shows the two reviewed commits unchanged plus one commit. The maxdepth branch is again master's listing loop, with only the < 1 check and the > 1 recursion guard changed. key is still used by the unlimited branch. Removing the tests drops only the fallback coverage; test_find_maxdepth_counts_levels_like_fsspec and the live test_find_maxdepth still fail against the old numbering.

sub_path = f"s3://{bucket}/{item.key}"
sub_results = self._find(
sub_path, maxdepth=maxdepth - 1, withdirs=withdirs, **kwargs
Expand Down Expand Up @@ -786,7 +804,9 @@ def find(

Args:
path: S3 path to search under (e.g., "s3://bucket/prefix").
maxdepth: Maximum depth to recurse (None for unlimited).
maxdepth: Maximum number of levels to descend, at least 1

@laughingman7743 laughingman7743 Oct 3, 2026

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): FINDINGS (PR text corrected)

Base e0e85da09435257a18c8b8832b3e10ba7840b697, head c1292e030417ed8a289f9df333ccb676aa3a668c; full pass over the PR body, commit messages, and changed docstrings.

Claims checked:

  • Old numbering (0 gave the current level, n gave n + 1 levels, negative values behaved like 0): reproduced with a mocked three-level listing on master; maxdepth > 0 guarded the recursion.
  • fsspec 2026.9.0 documents maxdepth as "the maximum number of levels to descend" (find), and walk(), glob(), and expand_path() raise ValueError("maxdepth must be at least 1"): confirmed in fsspec/spec.py.
  • ValueError before any request: the unit test asserts _call was not called.
  • Object path returned with maxdepth, as fsspec find() does (if not out and self.isfile(path)) and as the unlimited branch already did: confirmed. The prefix docstring no longer limits this to calls without maxdepth.
  • Introduced in v3.15.0 (681e749), and before that maxdepth was ignored (# TODO: Support maxdepth and withdirs, full listing): confirmed with git tag --contains and the parent revision.
  • maxdepth reaches find() from S3FileSystem.rm() (via expand_path), fsspec glob()/du()/copy()/get(), and the async _glob()/_du()/_expand_path()/_copy()/_get(): confirmed. No other PyAthena module passes maxdepth or calls find() with it.

Corrections to the PR body:

  1. put() was listed among the callers, but fsspec put() expands the local path with LocalFileSystem.expand_path(), so S3FileSystem.find() is not involved. Removed.
  2. The rm() consequence holds for the sync S3FileSystem.rm() only; AioS3FileSystem._rm() ignores maxdepth (pre-existing, s3_async.py:178). Qualified, and recorded under "Not changed".
  3. "Tested at" now separates the live run (8f29beb) from the assertion-only repair (c1292e0, offline tests rerun).

Caller/operator effects: callers that passed maxdepth=0 now get ValueError, and callers relying on the old numbering need maxdepth + 1; both are in the release note. S3 requests drop by one listing level per call. The object-path fallback adds at most one info() (HEAD, then a 1-key listing on 404) when the top-level listing is empty, the same cost as the unlimited branch.

Evidence limits: live S3 coverage is the targeted -n 1 run (28 tests, sync and async) at 8f29beb; the full filesystem and other suites run in CI after Ready.

@laughingman7743 laughingman7743 Oct 3, 2026

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.

Round two on the repair (54660a4): FINDINGS (PR text corrected). The PR body no longer claims the object-path behavior and lists #962-#966 under "Not changed". It states the 4.0.0-only target (milestone set on #933 and this PR), and "Tested at" now names 54660a4 with the 27-test live run. Request-count claim: back to master's single listing for a no-match prefixed maxdepth search (mocked measurement). Commit 8f29beb's message still mentions the fallback; 54660a4's message records its removal, and the history is not rewritten.

(None for unlimited). With 1, only the entries directly under
the path are listed.
withdirs: Whether to include directories in results (None = default behavior).
detail: If True, return dict of {path: S3Object}; if False, return list of paths.
**kwargs: Additional arguments including:
Expand All @@ -799,6 +819,9 @@ def find(
Dictionary mapping paths to S3Objects (if detail=True) or
list of paths (if detail=False).

Raises:
ValueError: If ``maxdepth`` is less than 1 or the path is the root.

Example:
>>> fs = S3FileSystem()
>>> fs.find("s3://bucket/data/", maxdepth=2) # Limit depth
Expand Down
52 changes: 45 additions & 7 deletions tests/pyathena/filesystem/test_s3.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -279,7 +279,41 @@ def test_find_refresh_bypasses_cached_listings(self):

assert fs.find("s3://bucket/dir", refresh=True) == ["bucket/dir/sub/new"]
# The subdirectory listings of maxdepth are refreshed as well.
assert fs.find("s3://bucket/dir", maxdepth=1, refresh=True) == ["bucket/dir/sub/new"]
assert fs.find("s3://bucket/dir", maxdepth=2, refresh=True) == ["bucket/dir/sub/new"]

def test_find_maxdepth_counts_levels_like_fsspec(self):
fs = self._make_fs()
responses = {
"dir/": {
"Contents": [{"Key": "dir/direct"}],
"CommonPrefixes": [{"Prefix": "dir/sub/"}],
},
"dir/sub/": {
"Contents": [{"Key": "dir/sub/nested"}],
"CommonPrefixes": [{"Prefix": "dir/sub/deep/"}],
},
"dir/sub/deep/": {"Contents": [{"Key": "dir/sub/deep/file"}]},
}
fs._call.side_effect = lambda method, **kwargs: responses[kwargs["Prefix"]]

with pytest.raises(ValueError, match="maxdepth must be at least 1"):
fs.find("s3://bucket/dir", maxdepth=0)
fs._call.assert_not_called()

assert fs.find("s3://bucket/dir", maxdepth=1) == ["bucket/dir/direct"]
assert sorted(fs.find("s3://bucket/dir", maxdepth=1, withdirs=True)) == [
"bucket/dir/direct",
"bucket/dir/sub",
]
assert sorted(fs.find("s3://bucket/dir", maxdepth=2)) == [
"bucket/dir/direct",
"bucket/dir/sub/nested",
]
assert sorted(fs.find("s3://bucket/dir", maxdepth=3)) == [
"bucket/dir/direct",
"bucket/dir/sub/deep/file",
"bucket/dir/sub/nested",
]

def test_refresh_evicts_cached_object_and_bucket_not_found(self):
fs = self._make_fs()
Expand Down Expand Up @@ -1089,19 +1123,23 @@ def test_find_maxdepth(self, fs):
fs.touch(f"{dir_}/level1/level2/file2.txt")
fs.touch(f"{dir_}/level1/level2/level3/file3.txt")

# Test maxdepth=0 (only files in the root)
result = fs.find(dir_, maxdepth=0)
# maxdepth must be at least 1, as in fsspec
with pytest.raises(ValueError, match="maxdepth must be at least 1"):
fs.find(dir_, maxdepth=0)

# Test maxdepth=1 (only files in the root)
result = fs.find(dir_, maxdepth=1)
assert len(result) == 1
assert fs._strip_protocol(f"{dir_}/file0.txt") in result

# Test maxdepth=1 (files in root and level1)
result = fs.find(dir_, maxdepth=1)
# Test maxdepth=2 (files in root and level1)
result = fs.find(dir_, maxdepth=2)
assert len(result) == 2
assert fs._strip_protocol(f"{dir_}/file0.txt") in result
assert fs._strip_protocol(f"{dir_}/level1/file1.txt") in result

# Test maxdepth=2 (files in root, level1, and level2)
result = fs.find(dir_, maxdepth=2)
# Test maxdepth=3 (files in root, level1, and level2)
result = fs.find(dir_, maxdepth=3)
assert len(result) == 3
assert fs._strip_protocol(f"{dir_}/level1/level2/file2.txt") in result

Expand Down
16 changes: 10 additions & 6 deletions tests/pyathena/filesystem/test_s3_async.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -442,19 +442,23 @@ async def test_find_maxdepth(self, fs):
await fs._touch(f"{dir_}/level1/level2/file2.txt")
await fs._touch(f"{dir_}/level1/level2/level3/file3.txt")

# Test maxdepth=0 (only files in the root)
result = await fs._find(dir_, maxdepth=0)
# maxdepth must be at least 1, as in fsspec
with pytest.raises(ValueError, match="maxdepth must be at least 1"):
await fs._find(dir_, maxdepth=0)

# Test maxdepth=1 (only files in the root)
result = await fs._find(dir_, maxdepth=1)
assert len(result) == 1
assert fs._strip_protocol(f"{dir_}/file0.txt") in result

# Test maxdepth=1 (files in root and level1)
result = await fs._find(dir_, maxdepth=1)
# Test maxdepth=2 (files in root and level1)
result = await fs._find(dir_, maxdepth=2)
assert len(result) == 2
assert fs._strip_protocol(f"{dir_}/file0.txt") in result
assert fs._strip_protocol(f"{dir_}/level1/file1.txt") in result

# Test maxdepth=2 (files in root, level1, and level2)
result = await fs._find(dir_, maxdepth=2)
# Test maxdepth=3 (files in root, level1, and level2)
result = await fs._find(dir_, maxdepth=3)
assert len(result) == 3
assert fs._strip_protocol(f"{dir_}/level1/level2/file2.txt") in result

Expand Down
Loading