Move the multipart upload requests into S3Core (step 3.2 of #1063)

[laughingman7743]

WHAT

Sub-step 2 of #1063 (step 3 of #1053): the multipart upload requests move into S3Core.

Core (pyathena/filesystem/s3_core.py, public):

• create_multipart_upload(path, **params), upload_part(path, upload_id, part_number, body, **params), upload_part_copy(path, upload_id, part_number, source, range_=None, **params), complete_multipart_upload(path, upload_id, parts, **params) and abort_multipart_upload(path, upload_id, **params), one request each. They return the existing S3MultipartUpload, S3MultipartUploadPart and S3CompleteMultipartUpload.
  • As in the replaced helpers, the fields that the arguments set take precedence over inherited parameters of the same name (Route S3 request parameters to the operations that accept them #1000).
  • upload_part_copy() takes the source as an S3Path (its version ID, if any, goes into CopySource) and the range as (start, end) with an exclusive end.
  • complete_multipart_upload() takes the typed parts instead of {"ETag", "PartNumber"} dicts.
  • abort_multipart_upload() raises. Logging and swallowing a failed abort stays in the filesystem's _abort_multipart_upload(), and S3File.discard() still lets the error propagate.
• create_multipart_upload() raises ValueError for a destination with a version ID instead of ignoring it, as the filesystem already does for a versioned write (Accept version_id in S3FileSystem.open() and cat_file() #958); no filesystem path passes one.
• part_ranges(size, block_size) is the former _get_copy_ranges(), used by the multipart copies and by an append.
• MULTIPART_UPLOAD_MIN_PART_SIZE, MULTIPART_UPLOAD_MAX_PART_SIZE and MULTIPART_UPLOAD_MAX_PARTS are S3Core attributes.

Adapters:

• S3File (write, append, discard), pipe_file(), _check_multipart_upload_size(), clear_multipart_uploads(), _finish_multipart_upload(), _abort_multipart_upload() and the sync and aio multipart copies call fs.core.* and read the limits from the core. The aio cancellation handling (the failed flag, the semaphore, the cleanup tasks, abort unless the completion succeeded) is unchanged.
• Removed private helpers: _create_multipart_upload, _upload_part, _upload_part_copy, _complete_multipart_upload, _get_copy_ranges.
• Breaking (4.0.0, maintainer decision recorded on Move copy, multipart and delete requests into S3Core (step 3 of #1053) #1063): S3FileSystem.MULTIPART_UPLOAD_MIN_PART_SIZE, MULTIPART_UPLOAD_MAX_PART_SIZE and MULTIPART_UPLOAD_MAX_PARTS are removed; the S3Core attributes are the only limits. Code that reads them from a filesystem gets AttributeError, and a subclass that sets them no longer changes the limits.
• Docs: the multipart operations and part_ranges() in docs/filesystem.md; the note that requests sent through fs.core do not invalidate the filesystem's cache now covers every operation.

Request changes: none on the wire, with one edge case. An append used to send CopySource="bucket/key" as a string and now sends {"Bucket", "Key"}. botocore encodes both the same way, including spaces, +, %, non-ASCII characters and ? (checked with botocore.handlers.handle_copy_source_param). The exception is a key such as a?versionId=v1?b, which S3Path treats as an unversioned, writable key: botocore read the string form as version v1?b of a, while the dict form copies the key itself.

Known pre-existing defects, unchanged here and tracked in #1070: a multipart upload created with ChecksumAlgorithm cannot complete (no part checksums in CompleteMultipartUpload), and clear_multipart_uploads() fails on an upload that is completed or aborted after the listing.

WHY

#1063 / #1053: S3 requests and S3 rules belong in the typed core; scheduling, cache invalidation and fsspec semantics stay in the adapters. This is the second of the four PRs (delete #1064 → multipart primitives → copy → pairing).

TEST

Tested commit: e8f2853 (lint; offline suite on 6e2fb2e, which differs only by a docstring); the live runs were on f730dd1, which differs only by the versioned-destination check, docstrings, building the S3File upload path once, and one offline test

• just lint: passed. just docs lint: passed.
• Offline: pytest --noconftest -n 8 tests/pyathena/filesystem with dummy credentials: 664 passed, 119 failed. The failure set is identical to origin/master 911492c (652 passed, 119 failed); all 119 need AWS.
• New Stubber tests in tests/pyathena/filesystem/test_s3_core.py for each primitive: the request shape, the precedence of the arguments over **params, the versioned CopySource and CopySourceRange, a whole-source part copy, the typed parts of the completion, an abort that raises (NoSuchUpload → FileNotFoundError, also measured live), keyless paths and a versioned destination. The _get_copy_ranges tests moved there as part_ranges tests.
• test_finish_multipart_upload_abort_failure_does_not_mask_the_original_error now injects the abort failure through the core and asserts the abort and its log (checked by mutation: it fails if _abort_multipart_upload stops swallowing the error).
• The mocked tests in test_s3.py/test_s3_async.py now mock fs.core.* and set the limits on fs.core; their assertions changed only in call shape (S3Path instead of bucket=/key=, typed parts, the client method instead of the method name in _call).
• Live AWS: uv run --env-file .env pytest -n 4 tests/pyathena/filesystem/test_s3.py tests/pyathena/filesystem/test_s3_async.py -k "multipart or append or cp_file or copy or write or pipe or put_file or mv": 225 passed, 1 failed on a stale fs._create_multipart_upload(bucket=, key=) call left in test_list_and_clear_multipart_uploads; fixed before the commit and re-run (-k test_list_and_clear_multipart_uploads: 4 passed).
• Not run locally: the full AWS suite (runs in CI when Ready).

🤖 Generated with Claude Code

[laughingman7743]

Self-review round one (implementation behavior). Base 911492c3e63ae6ba8c8892be2543fd1b51a75447, head 3825953dfc51b842229f1eb563931176f3797880; findings made on f730dd1, repaired in 3825953. Sources: my own pass, /code-review (high) and /simplify (reuse, simplification, efficiency, altitude).

Covered: the five core primitives, part_ranges() and the limits; S3File write/append/discard; pipe_file(), _check_multipart_upload_size(), clear_multipart_uploads(), _finish_multipart_upload(), _abort_multipart_upload(); the sync and aio multipart copies (cancellation logic unchanged); the test rewiring and docs. The kwargs that reach the primitives all pass operation_params() (CamelCase API names), so the new argument names cannot collide with them, unlike the rm_file case in #1064. The request precedence {**params, **request} and S3Core.call's request_kwargs merge are as before.

FINDING (repaired): the public create_multipart_upload() ignored a version ID on the destination, so core.create_multipart_upload(S3Path('b', 'k', 'v1')) + parts + complete overwrote the current object at k, where the filesystem raises ValueError (#958). It now raises ValueError; no adapter passes a version there. The other primitives work on the upload ID, so the entry point is the one place to check.

FINDING (docs, repaired): abort_multipart_upload() now documents FileNotFoundError for a missing upload (measured live: NoSuchUpload → FileNotFoundError), and docs/filesystem.md says that part_ranges() sends no request.

FINDING (simplification, repaired): the append path built two identical S3Paths per copied range; _initiate_upload() builds it once.

[laughingman7743]

Self-review round one — pre-existing defects found by /code-review, verified, and split into issues per the maintainer's choice (this PR stays behavior-preserving):

1. CompleteMultipartUpload sends only ETag/PartNumber, so an upload created with ChecksumAlgorithm cannot complete. Measured live with SHA256 and CRC32: S3 answers InvalidRequest ("The complete request must include the checksum for each part"). Same request as master's _finish_multipart_upload/aio copy.
2. clear_multipart_uploads() raises FileNotFoundError when a listed upload is completed or aborted before its abort (abort of a missing upload measured live → FileNotFoundError); unchanged by this PR.

Reasoned deferrals: the hard-coded "5 MiB"/"5 GiB" in the block-size messages only disagree when tests override the limits; a subclass overriding the removed MULTIPART_UPLOAD_* attributes is silently ignored, which follows from the removal decision (release note); CopySourceRange formatting duplicates S3File._format_ranges in one line because the core cannot import s3.py; storing one S3Path on S3File, S3Path parameters for _finish_multipart_upload/_abort_multipart_upload, merging the two append branches and the duplicated block-size validation belong to sub-steps 3 and 4.

[laughingman7743]

Filed with the maintainer's approval: (1) checksum fields in CompleteMultipartUpload → #1070; (2) clear_multipart_uploads() race → #1071. Both are left unchanged here.

[laughingman7743]

Update: both defects are now tracked in one issue, #1070 (#1071 closed as merged into it).

[laughingman7743]

Self-review round two (claims, callers, AWS operation, docs). Base 911492c3e63ae6ba8c8892be2543fd1b51a75447, head 3825953dfc51b842229f1eb563931176f3797880. Result: CLEAN after PR-text corrections.

Claims checked:

• "No request change on the wire except one edge case": every primitive keeps {**params, **request} and S3Core.call's request_kwargs merge; discard() and clear_multipart_uploads() send the same AbortMultipartUpload fields. The append's CopySource changed from "bucket/key" to a dict: botocore.handlers.handle_copy_source_param (botocore 1.43.102) encodes both identically for keys with spaces, +, %, non-ASCII and ?; only a key like a?versionId=v1?b differs, where the string form was misread as a version. Stated in the PR.
• Docs: abort_multipart_upload of a missing upload → FileNotFoundError (measured live); part_ranges() sends no request; the cache note now covers all core requests. The other filesystem docs (5 MiB, 5 GiB, 10,000 parts) still hold.
• Existing callers: the removed MULTIPART_UPLOAD_* attributes raise AttributeError when read and are ignored when a subclass sets them — added to the PR's breaking note. The removed private helpers have no remaining references in pyathena/, tests/, docs/ or benchmarks/.
• AWS operation: retries and error translation still go through S3Core.call; the aio copy keeps to_thread + wait/shield and its cleanup unchanged. Pre-existing Multipart uploads with ChecksumAlgorithm cannot complete, and clear_multipart_uploads() fails on finished uploads #1070 (checksum completion, clear race) measured live and left unchanged.
• Evidence: offline failure set identical to master at 3825953 (664 passed / 119 AWS-dependent failed); the live runs were on f730dd1 (PR body corrected to say so). Full AWS suite pending CI on Ready.

[laughingman7743]

Independent review (relayed) — static, read-only review of git diff 911492c3e63ae6ba8c8892be2543fd1b51a75447..3825953dfc51b842229f1eb563931176f3797880 on a clean detached snapshot; the reviewer ran no builds or tests. The prompt stated the intended behavior but not the PR number, description, commit messages or prior findings.

Reviewer: Codex CLI (codex exec -s read-only, model gpt-6-astra, session 01a10545-fc2c-7690-ac30-16508c3b7e2d). Covered: core multipart APIs, limits, ranges, precedence, response types, error translation; sync/aio scheduling, cancellation, completion/abort, cache invalidation, S3File write/append/commit/discard; pipe_file, put_file, clear_multipart_uploads, tests, docs. Result: FINDINGS (3).

1. P2 — Multipart copy changes keyword-collision exceptions (here and s3_async.py:640). For a source larger than 5 GiB, fs.cp_file(src, dst, key="other") previously raised TypeError (collision with the helper's explicit argument); it now reaches botocore and raises ParamValidationError. Conversely an extra path keyword now collides with the core argument.
2. P2 — The abort-failure test no longer injects an abort failure (test_s3.py:3311): it replaces only fs._call, but the abort now uses fs.core.call, so the test passes without exercising the failed-abort handling.
3. P3 — range_=None does not guarantee copying the whole source (s3_core.py:671): an inherited CopySourceRange in the params stays; document that None supplies no range override.

Author verification:

1. Rejected, reasoned: _get_multipart_copy_kwargs forwards unknown names for botocore to reject; on both base and head such a call fails client-side before CreateMultipartUpload is sent, so nothing is written. Only the exception class differs for meaningless lowercase kwargs, unlike Move the delete requests into S3Core (step 3.1 of #1063) #1064's rm_file, where the change deleted a different version. Fable reached the same conclusion ("informational, same class of garbage input").
2. CONFIRMED (coverage regression introduced here): only this test replaced _call alone (the two others at :2965/:3057 set core.call first). Repaired in 6e2fb2e.
3. CONFIRMED (docstring): repaired in 6e2fb2e.

[laughingman7743]

Repairs: (2) 6e2fb2e sets the abort failure on the mock shared by fs._call and fs.core.call and asserts the abort request and its log; checked by mutation (the test fails when _abort_multipart_upload stops swallowing). (3) 6e2fb2e + e8f2853: range_=None sends no range of its own; a CopySourceRange from params or the core's request_kwargs still applies. Lint passed; offline failure set identical to master (664 passed / 119 AWS-dependent failed). Both self-review perspectives re-applied (test now load-bearing; docstring claims traced to {**params, **request} and S3Core.call's merge).

Independent follow-ups (relayed): Codex CLI read-only, gpt-6-astra, session 01a10550-09b7-7b41-853e-f506bc92e507, on 3825953d..6e2fb2e0: one P3 — the docstring still omitted request_kwargs as a source of CopySourceRange → fixed in e8f2853; Codex on 6e2fb2e0..e8f28532: CLEAN ("accurately describes range_=None").

[laughingman7743]

Independent review (relayed) — static, read-only review of git diff 911492c3e63ae6ba8c8892be2543fd1b51a75447..3825953dfc51b842229f1eb563931176f3797880 on a clean detached snapshot; the reviewer ran no builds or tests. The prompt stated the intended behavior but not the PR number, description, commit messages or prior findings.

Reviewer: Claude claude-fable-5-1 (Agent tool, model override fable). Covered: all new core methods vs the deleted helpers and inline aborts; S3Core.call; the sync/aio copies, _finish_multipart_upload, _abort_multipart_upload, clear_multipart_uploads, _check_multipart_upload_size, pipe_file, S3File init/append/chunk/commit/discard; botocore handle_copy_source_param for the append CopySource; the test rewiring and docs. Result: FINDINGS (1, low; no behavior regressions found).

Low (docs/docstring accuracy) — docs/filesystem.md :282-285 and the S3Core class docstring said an operation raises FileNotFoundError for a missing bucket or a missing object/version it reads; upload_part, upload_part_copy, complete_multipart_upload and abort_multipart_upload on a stale upload ID also raise it (NoSuchUpload). Suggest adding "or a missing multipart upload".

Also noted as behavior-preserving: request precedence, CopySourceRange formatting, source versions, the append CopySource encoding (and, incidentally, a directly constructed S3File(fs, "s3://bucket/key", "ab") used to send the scheme in CopySource; the dict form works), part numbering, abort semantics, the aio cast, typing.

Author verification: CONFIRMED (NoSuchUpload → FileNotFoundError measured live for the abort); repaired in 6e2fb2e.

[laughingman7743]

Repaired in 6e2fb2e: the S3Core class docstring and docs/filesystem.md now say that a missing bucket or multipart upload raises FileNotFoundError.

Independent follow-up (relayed): claude-fable-5-1 (Agent tool, model override fable) on 3825953d..6e2fb2e0: CLEAN — the test injects through _abort_multipart_upload → S3Core.abort_multipart_upload → S3Core.call and would fail if the abort error stopped being swallowed; the docstrings match NoSuchUpload → FileNotFoundError and the range construction. Non-actionable nits recorded: a short rewrapped docs line, and upload_part/upload_part_copy/complete_multipart_upload not listing FileNotFoundError individually (the class docstring covers it).