Repository navigation
Separate an S3 core from the fsspec adapter in pyathena.filesystem #1053
Description
Use case
pyathena/filesystem/ implements S3 operations inside the shape of fsspec's AbstractFileSystem/AbstractBufferedFile API, inherited from the s3fs design it started from. fsspec's conventions (string names, glob semantics, the DirCache, the buffered-file lifecycle) leak into the S3 logic, and each S3 concept that does not fit them becomes another override or workaround. #979 is a current example: a version-qualified path is globbed because fsspec's copy()/get() call glob.has_magic() on the source string and name the destinations after the source strings, so supporting versions means overriding fsspec's pairing.
Inventory of the fsspec-shaped code on master 28ec68d (s3.py unless noted):
- Paths and names. Two scheme parsers (
PATTERN_PATHand fsspec's_strip_protocol(), 10 calls). Therstrip("/")in_strip_protocol()is whydir/anddirmeet ininfo()/open(). Cache keys are built with fsspec's_parent()(:466, :828, :2735, :2880). Directory entries strip trailing slashes to match fsspec names (:577, :653, :797, :829). Model S3 paths with one value class instead of parsing strings at each call site #1049 adds theS3Pathmodel; the names themselves are still fsspec's. - Listing and cache. The whole cache is fsspec's
DirCache, holding layouts fsspec never uses: single objects under a path, listings under(path, delimiter), buckets under"", and per-parameter lookups under(path, "lookups")with their own expiry (:65-67, :556-596, :2796-2824)._ls_from_cache()reimplements fsspec's (:2849-2899),_evict_cache()works around aDirCache.pop()race (:2737-2751), andinvalidate_cache()walks parents with_parent()withoutsuper()(:2692-2735).ls()returns names or objects, and a file path falls back to HeadObject (:599-639). - Globbing and expansion.
find()/_find()/_find_levels()/_extract_parent_directories()(:832-1009) follow fsspec'sfind()semantics: synthesized directories, the path itself included "as in fsspec", fsspec's maxdepth numbering, and dict vs list returns.glob/expand_path/walk/duare inherited and treat?as a wildcard._expand_delete_paths()routes versioned paths aroundexpand_path()(:1102-1136). - Copy, move and delete pairing.
_move_paths()repeats fsspeccopy()'s pairing line by line (expand_path,trailing_sep/isdir,has_magic,other_paths; :1505-1585) before adding S3 rules (nullversions, overlap checks).cp_file()takes unused fsspec arguments (:1606-1635), and_copy_file()skips the directories that recursivecopy()passes (:1669-1673). fsspec'sonerrortypo is popped in both sync and async (:1652-1658,s3_async.py:425-427). - File objects.
S3File.__init__()validates beforesuper().__init__()because fsspec's__del__would commit a half-built file. It also sets the details and size so that the base class does not look up the latest version (:3140-3310). Multipart logic lives in_initiate_upload()/_upload_chunk()/commit()/discard(), shaped by fsspec'sflush()contract (:3354-3551)._close_without_commit(),_write_and_close()and_write_file_and_close()exist because awithblock on an fsspec file commits on error._fetch_range()clamps the ranges fsspec caches request (:3608-3650). - Transactions and the async bridge.
pipe_file()reroutes throughopen()in a transaction (:1922-1929).AioS3FileSystemkeeps its own transaction copies ofpipe_file/put_file(s3_async.py:168-280), wraps the sync filesystem withasyncio.to_thread, and hand-writes the sync wrappers that fsspec does not mirror (mv,rmdir,touch).S3AioExecutorexists so that the sync buffered file can run parts on the loop. - Results.
S3Objectis aMutableMappingso that fsspec can readinfo()["size"]/["name"]/["type"]._directory_object()and the root and bucket objects are synthesized fsspec entries (:322-337, :391-403, :502-515, :748-760).
Parts that are already S3-native and could form a core: _get_object, _put_object, the multipart helpers (:2937-3089), _finish_multipart_upload, the DeleteObjects batching (:1168-1298), _get_copy_ranges, _get_operation_kwargs/_call, and the typed response classes in s3_object.py.
Proposed change
Restructure the filesystem as a typed S3 core wrapped by a thin fsspec adapter, so that fsspec's conventions stay at the edge:
- Core (no fsspec imports):
S3Path(Model S3 paths with one value class instead of parsing strings at each call site #1049); typed entries for objects, prefixes, versions and buckets (instead of fsspec-shaped dicts); operations that takeS3Pathand return typed results, such as head, list (by prefix and delimiter, by level, all versions), get, put, multipart upload/copy with abort on failure, copy, delete in batches, tags, ACL and metadata; and a cache keyed byS3Pathand(bucket, prefix, delimiter), with its own invalidation and expiry. - Adapter:
S3FileSystem,AioS3FileSystemandS3Filekeep their public API (see below). They translate fsspec strings intoS3Path, own the fsspec semantics (glob andexpand_path,find's directory synthesis and depth numbering, copy/move/get destination pairing,info()dicts,detail=True/False, theDirCacheinterface, transactions, callbacks), and drive the core's multipart writer from the buffered-file lifecycle.
The public surface the adapter must keep:
register_s3_filesystem(), and the constructors with their s3fs-compatible arguments (version_aware,s3_additional_kwargs,default_block_size, ...);- every method documented in
docs/filesystem.mdand the API reference, includingls(versions=True),object_version_info(), multipart listing, tags/xattrs/ACLs, transactions and the async_xmethods; parse_path()/PATTERN_PATH, and the mapping and attribute access of theS3Objects thatinfo()/ls(detail=True)return;- the internal users in
pyathena/s3fs/,pyathena/pandas/,pyathena/aio/s3fs/and Polars through the fsspec registration.
A possible order, one reviewable PR per step, each without a behavior change unless agreed in its own issue:
S3Path(Model S3 paths with one value class instead of parsing strings at each call site #1049).- Typed listing entries and a core listing/head API; the adapter builds
S3Objects and names from them. - A core copy/move/delete API that takes pairs of
S3Path. The adapter alone does fsspec's pairing, one shared implementation forcopy(),get()andmv()in sync and async (this would also absorb Keys containing '?' are rejected, and pinned or listed versions are not addressable #979's pairing fix). - A core multipart writer;
S3Filedelegates to it. - The cache behind a core interface, still stored in
DirCachefor fsspec compatibility.
Open questions:
- Does the core become public API (for example
pyathena.filesystem.core), or stay private until it settles? - Should the async adapter call an async core, or keep running the sync core in threads as it does today?
- Should the cache keep living in fsspec's
DirCache(which fsspec users configure withuse_listings_cache/listings_expiry_time), or move into the core with those options mapped?
Out of scope: new S3 features and behavior changes, which go through their own issues.
Validation plan (if implementing)
Each step keeps the existing offline and S3 integration tests in tests/pyathena/filesystem/ passing unchanged, and adds unit tests for the core API it introduces. Steps that move behavior between layers are checked against the resolved fsspec version and the documented behavior in docs/filesystem.md.
Activity
laughingman7743 commentedon Oct 4, 2026
Decisions on the open questions (maintainer, 2026-10-04):
S3Path(Model S3 paths with an S3Path value class #1052) is the first public piece. The steps keep their own issues, and each issue states the public surface it adds.S3FileSystemwith its executor, andAioS3FileSystemwithasyncio.to_threadandasyncio.gather, as it does today.DirCache. The filesystem is based on the fsspec spec and extends it where S3 needs more. Souse_listings_cache/listings_expiry_time/max_paths,fs.dircache,invalidate_cache()and the cache shared between the sync and aio filesystems keep working as they do. Step 5 is reduced to building the cache keys fromS3Pathin one place, and only if a later step needs it.laughingman7743 commentedon Oct 4, 2026
Step 2 is tracked in #1059 (typed listing entries and core list/HEAD operations).
laughingman7743 commentedon Oct 4, 2026
Step 2 (#1059) is merged in #1060 (8bbf8cb): the public
S3Core(pyathena.filesystem.s3_core) with typed listing entries and list/HEAD operations,S3FileSystem.core/AioS3FileSystem.core, and theS3Object.from_*()factories. The private helpers added by the earlier PRs, such asS3FileSystem._split_version_paths()and_copy_paths(), are to be revisited in the step that owns their responsibility (pairing in step 3).laughingman7743 commentedon Oct 4, 2026
Step 3 is tracked in #1063 (copy, multipart and delete requests in
S3Core; pairing inS3PathPairing). Its design was reviewed with claude-fable-5-1 before it was filed.laughingman7743 commentedon Oct 4, 2026
Step 3 (#1063) is complete. The copy, multipart and delete requests and their S3 rules are now in the public
S3Core, and the path pairing has a public owner:S3DeleteBatch,S3DeleteResult,S3DeleteError,S3Core.delete_object/delete_objects).S3MultipartUploadthat the creation returned.copy_object,plan_multipart_copy/S3MultipartCopyPlan, the annotation operations). It also fixed An interrupted multipart copy leaves the upload behind when CreateMultipartUpload is in flight #1076.S3PathPairing, a pure frozen value of the paths of one copy, get or move (delete_paths()forrm()), andS3Path.target.S3FileSystemmakes the lookups, andAioS3FileSystemruns them in a thread.Found along the way and filed: #1077 (an interrupted
S3Filewrite leaves its upload behind; step 4 territory) and #1083 (mv()of a noncurrentnullversion in a versioning-enabled bucket). The design decisions of each sub-step are recorded on #1063.laughingman7743 commentedon Oct 4, 2026
Step 4 is complete in #1085 (
855d4a7b):S3Filedelegates multipart requests and part planning to the public synchronousS3MultipartWriter, including cleanup when upload creation is interrupted.The remaining extraction is tracked by #1086, linked as a native sub-issue. It covers object GET/PUT, tags/ACLs, metadata updates, multipart upload listing, signing, bucket request primitives, and the remaining multipart size rule, plus a final audit of adapter-owned request construction.
The cache decision remains unchanged:
DirCachestays in the adapter, and step 5 centralizes keys only if a later extraction needs it. The new sub-issue records the outcome before completion.Keep this parent open until the new work is implemented, reviewed, validated and merged. Then verify the live sub-issue hierarchy, record the completed scope, and close the remaining sub-issue and this parent. The separately tracked behavior defect #1083 remains outside this extraction.
laughingman7743 commentedon Oct 5, 2026
All steps of the core/adapter separation are complete:
S3Path: Model S3 paths with one value class instead of parsing strings at each call site #1049 (Model S3 paths with an S3Path value class #1052).S3Core, and pairing inS3PathPairing: Move copy, multipart and delete requests into S3Core (step 3 of #1053) #1063 (Move the delete requests into S3Core (step 3.1 of #1063) #1064, Move the multipart upload requests into S3Core (step 3.2 of #1063) #1069, Use multipart upload objects and fix checksum completion and cleanup races #1072, Move the copy requests and the multipart copy plan into S3Core, and abort uploads interrupted during creation (step 3.3 of #1063) #1074, Add the pure S3PathPairing planner and S3Path.target (step 3.4 of #1063) #1081).S3Path: not needed.DirCachestays in the adapters with unchanged storage, options, invalidation and sync/aio sharing.The remaining request construction was extracted by #1086 (#1089, #1090, #1091, #1098). The final audit is in #1086 (comment): no S3 request is built in the adapters any more, and each remaining SDK use is listed there with its reason.
All sub-issues are closed, so I am closing this design issue.