Add the pure S3PathPairing planner and S3Path.target (step 3.4 of #1063)

[laughingman7743]

WHAT

Sub-step 4 of #1063 (step 3 of #1053), the last one: the path pairing of copy(), get(), mv() and rm() gets a public owner, S3PathPairing, and the null-version rule moves onto S3Path.

• New public S3PathPairing in pyathena/filesystem/s3_path_pairing.py: a frozen value of the paths that one copy(), get() or mv() pairs, S3PathPairing(path1, path2, recursive=False, maxdepth=None). It holds no filesystem.
  • Questions (properties and one method): expands, skips_directories, looks_up_destination and conflict_candidates(pairs).
  • Answers: copy_pairs(sources, destination_is_dir) -> list[tuple[str, str]] and move_pairs(pairs, missing) -> list[tuple[str, str]].
  • The classmethod delete_paths(path) -> (versioned, unversioned) splits the paths of an rm(), which has one path.
  • A lookup that a rule needs and that is not passed raises ValueError, instead of defaulting to "not a directory" or "no object".
• New S3Path.target: the object that a write to the path replaces. A null version names its key; any other path is its own target. It replaces _move_target().
• Adapters: S3FileSystem builds one pairing per operation and makes the lookups with its requests and cache, in private _copy_pairs(pairing, isdir=None), _move_pairs() and _delete_paths(). AioS3FileSystem calls these in one asyncio.to_thread, as master ran the old helpers.
  • They replace _copy_paths(), _move_paths() and _expand_delete_paths().
  • The destination is looked up only when it decides the pairing, and HeadObject is sent only for the conflict candidates, as before.
• _split_version_paths() stays on S3FileSystem as the expansion rule.
• Docs: a "Path pairing" section in docs/filesystem.md, and S3PathPairing in the API reference.

The pairing results and the S3 requests are unchanged; see TEST.

The design was revised twice in review:

• A first version stored an S3PathPairing on the filesystem that referenced the filesystem. That reference cycle kept S3FileSystem from being freed by reference counting (measured), and the public class called the private _head_object(). After a comparison with claude-fable-5-1 and Codex, the planner became pure.
• At the maintainer's request, its static functions, which took the same paths again and again, then became one frozen value per operation.
• An aio orchestration with fsspec's async _expand_path() was tried and dropped. Its glob listing has a prefix and is not cached, so the directory filter of a non-recursive _mv() sent a HeadObject per matched path. aio now runs the sync orchestration in a thread.

The decisions are recorded on #1063.

WHY

Closes #1063.

#1063 / #1053: the S3 requests and rules live in the core; the adapters keep fsspec's semantics, the cache and the scheduling. The pairing rules now have one public, I/O-free owner, and one orchestration that both filesystems use. S3PathPairing stays out of s3_core because it follows fsspec's conventions, while the core does not depend on fsspec.

Related: #1083 (filed in this review). A null version moved onto its key in a bucket with versioning enabled is left in place by the target rule. This behavior predates this PR; S3Path.target documents it.

TEST

Tested commit: 1a73556 (lint, offline suite, equivalence, live run). 41cdf8b merged origin/master 0c19c8d (#1072, #1080).

• just lint and just docs lint: passed. A Sphinx build on 05f37db had 205 warnings, the same count as origin/master, and the API page linked the cross-module references to S3PathPairing.copy_pairs and S3Path.target.
• Equivalence: 26 inputs were run through the old private helpers on origin/master and through the new sync orchestration, which aio now runs in a thread, with the same served keys.
  • The results were identical, errors included, and the S3 requests were the same multiset. A move that lists the same directory source twice sends the same HeadObject requests as master, one per pair (checked directly).
  • Only one recursive glob listed its subdirectories in a different order. That order comes from fsspec iterating a set, and it varies between runs.
  • The inputs covered recursive and non-recursive directories, maxdepth, versions with file, directory and trailing-slash destinations, list/list and list/str pairing, globs (including one without matches), a key with ?, a local destination with a given isdir, null-version moves, both move conflicts, a directory without an object in a conflict, and deletes with versions, a bucket and a missing path.
• Offline, with dummy credentials: pytest --noconftest -n 8 tests/pyathena/filesystem gave 814 passed, 161 failed. The failure set is identical to origin/master 0c19c8d (770 passed, 161 failed); all 161 need AWS.
• New tests:
  • test_s3_path_pairing.py: pure tests of every rule, with no filesystem. They cover the ValueError for missing sources, destination_is_dir or missing, missing given in any form or as a string (TypeError), and a null version of a missing directory key still counting as a writer (from the independent review; it fails on 594d94d).
  • test_s3_path.py: S3Path.target.
  • test_s3.py:
    • which lookups _copy_pairs/_move_pairs make: no destination lookup for a trailing slash, and HEAD only for the conflict candidate;
    • test_freed_by_reference_counting, which fails on the first design (b09d4cb) and passes now.
  • test_s3_async.py is unchanged from master, and its _rm/_mv/_copy/_get tests pass.
• Live AWS: uv run --env-file .env pytest -n 4 tests/pyathena/filesystem/test_s3.py tests/pyathena/filesystem/test_s3_async.py -k "copy or cp_file or mv or move or rm or get or version or expand": 231 passed on 1a73556 (235 on 9cb261e, which still had the aio orchestration tests).
• Not run locally: the full AWS suite, which runs in CI once the PR is Ready.

🤖 Generated with Claude Code

[laughingman7743]

Self-review round 1 (implementation behavior, public contracts, simplicity, regression coverage; plus /code-review high twice and /simplify), base 200762088e7b0bac45054f22e32a16aa0ce95dbb, head 4794aa0b95dbbed432b537e8638096d9edb13228. Initial head b09d4cbb reviewed in full, then redesigned in 422c3ad0, and that design reviewed in full again; repairs in 05f37dbe and 4794aa0b.

Inventory: S3PathPairing (new module), S3Path.target, the sync and aio _copy_pairs/_move_pairs/_delete_paths and their callers (copy/get/mv/rm, _copy/_get/_mv/_rm), the removed helpers, the docs, and the tests.

Design finding (b09d4cb): S3FileSystem stored an S3PathPairing that referenced the filesystem.

• Measured: S3FileSystem(skip_instance_cache=True) was no longer freed by reference counting, while master freed it at once.
• The public class also called the private _head_object(), and aio ran the sync pairing in a thread.

The maintainer called the design fundamentally wrong. After consulting claude-fable-5-1 and Codex (both recommended a pure planner), the planner became pure and static, and the adapters own the lookups. The decision is recorded on #1063. test_freed_by_reference_counting fails on b09d4cb and passes now.

Equivalence evidence: 26 inputs through the old helpers (origin/master), the new sync orchestration and the new aio orchestration gave identical results, errors included. The sync requests were the same multiset. aio listed globs under the literal prefix in 2 cases (fsspec's async _expand_path()), with the same results.

Result: FINDINGS, repaired (see the following comments).

[laughingman7743]

Round 1, /code-review on 422c3ad0 (no correctness regressions; contract, efficiency and docs findings):

• Repaired in 05f37dbe:
  • copy_pairs() without sources silently returned []; it now raises ValueError, unlike an empty expansion.
  • move_pairs(missing=...) now accepts any form of the path and normalizes it through S3Path.target.
  • Each move path is parsed once per call.
  • The public cross-module references are qualified. The rendered API page now links them; Sphinx has 205 warnings, the same as master.
• Deferred: the sync/aio orchestration duplication, which follows from the chosen design.

/simplify (4 angles) on 05f37dbe, repaired in 4794aa0b:

• copy_pairs() reuses looks_up_destination() for the exists rule (one copy of the rule).
• The move analysis is one private _moves() returning (named, sources, candidates), and skipped is a set.
• The _delete_paths() guard comments say why the guard exists.
• The test uses _stubbed_fs().
• The docs name expands() and say that copy()/get() use the pairing only when a source has a version ID.
• S3Path.target's summary no longer claims to be the replaced object in every bucket.
• Skipped: the repeated isinstance (mypy narrowing); the reference-counting test (it guards this review's regression); docs/docstring overlap (repository style); the second _moves() call per move (inherent to the stateless API; about 0.4 s at 100k pairs, against 100k copy requests); s3_object naming through target (out of scope).

[laughingman7743]

Round 1, aio efficiency (measured by the /simplify efficiency review):

• After the first /code-review, the per-source asyncio.gather(self._isdir(p)) cost about 4.5 s at 100k paths, against 0.02 s for one to_thread. It is back to one thread with the cached sync isdir, as on master.
• The pure planning of a 100k-key _mv() took about 1 s of CPU on the event loop. copy_pairs/conflict_candidates/move_pairs now run in to_thread, as master ran the whole pairing.
• The destination lookup stays await self._isdir(), and a given local isdir runs in a thread. The conflict HEADs are gathered; they are deduplicated and few.

[laughingman7743]

Self-review round 2 (claims, callers, AWS operation, docs, evidence), base 200762088e7b0bac45054f22e32a16aa0ce95dbb, head 594d94d752a20bd44c3e0c7d7fda0aeaa76d48e8. Full claim audit of the PR body, the docstrings of S3PathPairing/S3Path.target/the adapters, docs/filesystem.md and the commit messages.

Claims corrected:

• This docs paragraph said that copy()/get() use the pairing "when a source has a version ID". The code also requires a single destination path (copy(): isinstance(path2, str); get(): str/PathLike). It now says so (594d94d7).
• The PR body described aio as using _isdir() for every lookup, but since 4794aa0b the sources are checked in one to_thread. The body now describes the current lookups and threads, its test counts and tested commits are updated, and the Sphinx claim states the commit it was measured on.
• S3Path.target (round 1): its summary no longer presents the move convention as an S3 fact; versioning-enabled buckets are mv() of a noncurrent null version onto its key does nothing in a versioning-enabled bucket #1083.

Claims verified:

• The destination is looked up only when it decides the pairing, and HEAD is sent only for the conflict candidates (adapter tests, sync and aio).
• The pairing results are unchanged (26-input equivalence, old/sync/aio).
• The sync requests are unchanged; aio's two narrower glob listings are documented.
• The docs example pairs without destination_is_dir (both paths end with a slash), checked offline.
• The removed private helpers and the never-released pairing property have no callers.

Callers and operators: the public signatures of copy/get/mv/rm are unchanged; no new requests in sync; retries go through the same S3Core.call; aio no longer blocks the event loop on pairing CPU or local isdir.

Limits:

• The equivalence ran on a fake store.
• The live subset (224 passed) ran on 422c3ad0 and is being repeated on this head once the active Test run of another PR finishes.
• The full AWS suite runs in CI on Ready.

Result: FINDINGS (claims), corrected.

[laughingman7743]

Independent review (relayed): Codex CLI gpt-6.1-sol, session 01a10636-6328-77e2-8d4f-3782e5e0971c, codex exec -s read-only on a detached snapshot at 594d94d7, diff 20076208..594d94d7. The prompt gave the intended behavior only. Static review.

Coverage (reviewer): the exact diff and the base implementations; the copy/get/mv/rm pairing, lookups, errors and ordering; sync/aio parity, threading, concurrency and cancellation; reference ownership; docstrings, both docs pages, tests.

Result: FINDINGS, three regressions:

1. P1: missing suppressed an explicit null version that shares its target with a missing directory key. For example, src/d (no object) and src/d?versionId=null both moved to dst/out passed the checks, so the null version could be overwritten and then deleted. The base raised. Repaired in 26b39957: a versioned row always counts as a writer. test_move_pairs_version_of_missing_directory_key fails on 594d94d7 and passes now.
2. P2: the aio delete_paths() and the list/list copy_pairs() ran on the event loop. Repaired: both run in to_thread.
3. P2: the gathered conflict HEADs raised whichever error came first in time; the base raised the first candidate's. Repaired: return_exceptions=True, and the first candidate's error is raised in order. test_move_pairs_raises_the_first_lookup_error fails on 594d94d7 and passes now.

After the repairs: lint passed; the offline suite matches master's failure set (736 passed); the 26-input equivalence still holds for sync and aio; the live subset gave 225 passed on 26b39957.

[laughingman7743]

Independent follow-up of 594d94d7..26b39957 (relayed): Codex gpt-6.1-sol, session 01a10640-1435-7b41-bfb3-b4388f4b33b4, read-only snapshot at 26b39957, full review of the repair. Static review.

Coverage (reviewer): versioned writer accounting, missing-directory exemptions, no-op handling and the sync/aio move callers against the base _move_paths; aio thread dispatch, completion of the conflict HEADs, candidate-order errors and None detection; both regression tests; the docs wording. Result: CLEAN.

origin/master (with #1072 and #1080) was then merged in 41cdf8b4. The conflicts were additive:

• docs/filesystem.md: Use multipart upload objects and fix checksum completion and cleanup races #1072's multipart paragraph stays in "Typed S3 operations", followed by "Path pairing"; one line rewrapped.
• test_s3.py: imports.
  Use multipart upload objects and fix checksum completion and cleanup races #1072 changed the multipart primitives and copy scheduling, which the pairing does not touch.

After the merge, against a fresh origin/master 0c19c8d baseline:

• lint and docs lint passed;
• offline: identical failure set (161, all AWS), 815 passed;
• 26-input equivalence unchanged;
• live subset: 235 passed on 41cdf8b4.

[laughingman7743]

Independent review (relayed): Claude claude-fable-5-1 (Agent tool, model fable) on the same snapshot 594d94d7, same prompt. Static review.

Coverage (reviewer): the whole planner traced line by line against the base _copy_paths/_move_paths/_move_target/_expand_delete_paths (including the exists rule, the error order and the deduplicated HEADs), the sync and aio adapters, fsspec's installed async _expand_path/_isdir, S3Path.target, the docs and all changed tests.

Result: CLEAN, with low-severity nits:

1. aio used if not object_ where sync uses is None. Repaired in 26b39957 (is None).
2. The aio delete_paths() ran on the loop. Repaired, as Codex's finding 2.
3. A NamedTuple for the _moves() rows: not taken (simplicity).
4. This sentence said that rm() pairs its paths. Repaired: mv() pairs, rm() expands.

Both snapshots are unchanged after the reviews.

[laughingman7743]

Independent follow-up of 594d94d7..26b39957 (relayed): Claude claude-fable-5-1, same snapshot, full review of the repair. Static review.

Coverage (reviewer): move_pairs' writer filter and _moves' candidate rules row by row against the base _move_paths, including one divergence it checked and found to have an identical outcome. It also covered every caller of the planner (sync and aio), the aio gather(return_exceptions=True) order, is None parity, and the to_thread dispatch. It traced both new tests by hand and found them deterministic, with the earlier tests consistent, and checked the docs wording. Result: CLEAN.

[laughingman7743]

Self-review, both rounds, of the value-object change (41cdf8b4..30e48052). The maintainer asked for S3PathPairing to become an object of the paths instead of static functions that took them again (decision recorded on #1063).

Round 1 (/code-review high and /simplify):

• Repaired in 97aea57d:
  • a string passed as missing was split into characters; it now raises TypeError;
  • aio sent all conflict HEADs at once, wasting requests after the first failure; it now looks them up in order;
  • aio used two different isdir mechanisms;
  • the docs/docstrings now say exactly which lookups raise ValueError (the caller leaves out the directories) and that the dataclass keeps the given lists.
• Repaired in 30e48052 (efficiency review, a request regression): the aio orchestration with fsspec's async _expand_path() lists globs with a prefix, which is not cached. The directory filter of a non-recursive _mv() therefore sent one HeadObject per matched path. My earlier equivalence summary had called this "narrower listings"; case 11 in fact had the extra HEAD. The maintainer chose to run the sync orchestration in one thread from aio, as master did. That also removes the sync/aio duplication that /code-review flagged, and test_s3_async.py is back to master's, original test_rm_maxdepth included.
• /simplify repairs in the same commit: the exists rule in fsspec's shape, self._moves(), and one aio code path.
• Skipped: _moves() twice per move; the delete_paths/_split_version_paths overlap; the pair unzip; missing normalization (already tested); docs/docstring overlap; the reference-counting test is kept as the guard for this review's regression.

Round 2 (claims): the PR body, the #1063 decisions and the docs were re-audited. The body no longer claims aio-native lookups or narrower aio listings, its test counts and tested commit are current, and "the S3 requests are unchanged" is now true for sync and aio. Evidence on 30e48052: lint; offline suite equal to master's failure set (812 passed); 26-input equivalence with the same request multiset (one fsspec set-order difference); live subset 231 passed.

[laughingman7743]

Independent review of the value-object change (relayed): Codex CLI gpt-6.1-sol, session 01a1065c-d638-7e80-a0d7-d0540099ca9f, codex exec -s read-only on a detached snapshot at 30e48052, diff 41cdf8b4..30e48052 plus the end state against 0c19c8da. Static review.

Coverage (reviewer): the full incremental diff, compared with 0c19c8da for pairing results, errors, the expansion/isdir/HEAD order, the sync/aio callers, the value-object API, reference ownership, docstrings, both docs and the named tests. test_s3_async.py matches master exactly.

Result: FINDINGS. One P2, present since 41cdf8b4: conflict_candidates() deduplicated, so mv(["b/d", "b/d", "b/d/x"], ["b/out", "b/out", "b/x"]) sent one HEAD for b/d where master sent one per pair. A missing HEAD is not cached, so the actual S3 requests changed.

Repaired in 1a73556c: one candidate per pair.

• test_conflict_candidates_one_per_pair pins it.
• A direct check of that scenario shows the same HEAD keys on master and on this head: ['d', 'd', 'd', 'd', 'd/x'].
• The offline suite matches master's failure set (814 passed), and the 26-input equivalence holds.

[laughingman7743]

Follow-up of 30e48052..1a73556c (relayed): Codex gpt-6.1-sol, session 01a10664-0026-7742-a536-45568e3eb9ba, read-only snapshot at 1a73556c. It covered duplicate candidates and pair order against 0c19c8da, _move_pairs lookups and exemptions including versioned sources, the comment, and both new tests. Result: CLEAN; the per-pair _head_object calls are restored as in master. Static review.

Live subset on 1a73556c: 231 passed.

[laughingman7743]

Independent review of the value-object change (relayed): Claude claude-fable-5-1 on the same snapshot 30e48052, same prompt. Static review.

Disclosure from the reviewer: a command fell through to uv run, which created the gitignored .venv in the snapshot. git status stayed clean, and nothing else ran.

Coverage (reviewer):

• copy_pairs' exists against master's short-circuit, including when destination_is_dir is supplied;
• the lookup order, the _moves candidates and writers (including the versioned or clause), the raise order, _delete_paths, and the aio to_thread delegation;
• S3Path.target against _move_target, the absence of a reference cycle, and all tests;
• test_s3_async.py equal to master, no stale names, and the docs.

Result: CLEAN. It noted the same HEAD deduplication as a benign reduction, now repaired above. Nits:

1. delete_paths is a classmethod with cls unused. Kept as decided on Move copy, multipart and delete requests into S3Core (step 3 of #1053) #1063.
2. This comment missed the list-source case. Repaired in 1a73556c.
3. Frozen with list fields makes hash() fail. Known, documented.
4. No unit case for a list source with a string destination. Added in 1a73556c.

[laughingman7743]

Follow-up of 30e48052..1a73556c (relayed): Claude claude-fable-5-1, same snapshot; the prompt also barred running uv, pip and python. Coverage (reviewer):

• the candidates against master's predicate, with network parity in both directions (cached hits, re-sent misses);
• move_pairs with duplicate candidates and versioned pairs;
• the comment's accuracy in every looks_up_destination == False case;
• both new tests evaluated by hand against fsspec's other_paths, and the unchanged tests.

Result: CLEAN. Static review.