Fix empty object write for small files in fsspec transactions

[laughingman7743]

WHAT

Fixes #719 — small files (smaller than the block size, default 5 MiB) written inside an fsspec transaction (autocommit=False) were committed to S3 as empty objects.

The fix in pyathena/filesystem/s3.py is a one-liner (twice): S3File._upload_chunk now returns not final instead of True. commit() and __init__ are unchanged.

WHY

S3File.close() triggers fsspec's flush(force=True), and fsspec resets self.buffer after the upload only when _upload_chunk returns a value other than False:

# fsspec/spec.py
if self._upload_chunk(final=force) is not False:
    self.offset += self.buffer.seek(0, 2)
    self.buffer = io.BytesIO()   # buffer reset

_upload_chunk previously returned True unconditionally. For a small file the actual upload is deferred to commit():

• autocommit (normal open()): commit() runs synchronously inside _upload_chunk, before the reset — works.
• transaction (autocommit=False): commit() is deferred to Transaction.complete(), which runs after fsspec reset the buffer, so it reads an empty buffer and sends body=b"" to PutObject.

Returning not final fixes this:

• Mid-stream chunks (final=False) → True: fsspec clears the already-uploaded buffer between multipart parts (PyAthena delegates mid-stream buffer management to fsspec).
• Final flush (final=True) → False: fsspec leaves the buffer intact, so the deferred commit() reads the real bytes.

This mirrors how s3fs solves the same problem — s3fs returns False from _upload_chunk to keep the buffer, and commit() reads from it. (s3fs returns False unconditionally because it does its own mid-stream buffer bookkeeping; PyAthena uses not final because it relies on fsspec for that.) The large-file/multipart path's commit() completes the upload from the already-uploaded parts and never reads the buffer.

AioS3File inherits _upload_chunk/commit from S3File, so the async filesystem benefits from the same fix.

This keeps PyAthena's custom S3FileSystem (which intentionally avoids the s3fs/aiobotocore dependency) and supports fsspec transactions properly rather than handing filesystem ownership back to s3fs.

Tests

Unit (no AWS) — tests/pyathena/filesystem/test_s3.py::TestS3File

Drive S3File._upload_chunk/commit/discard with a mocked filesystem (no AWS), asserting the fsspec return-value contract. Parametrized to cover the full matrix of autocommit (transaction vs not) × single vs multipart × commit vs rollback:

Test	autocommit / transaction	upload	path
test_upload_chunk_small_file[True]	autocommit (no txn)	single	one-shot PutObject, committed in-line
test_upload_chunk_small_file[False]	transaction	single	GH-719 regression — deferred commit reads the preserved buffer
test_upload_chunk_multipart[True]	autocommit (no txn)	multipart	CompleteMultipartUpload in-line
test_upload_chunk_multipart[False]	transaction	multipart	CompleteMultipartUpload deferred to commit
test_discard[False]	transaction (rollback)	single	no-op (object never created)
test_discard[True]	transaction (rollback)	multipart	AbortMultipartUpload
test_upload_chunk_empty_file_touches	autocommit	empty	touch (never PutObject)

The multipart tests also assert the mid-stream invariant (non-final → True so fsspec resets the buffer between parts; final → False), which guards against a future "simplification" to a plain return False that would corrupt multipart uploads.

Integration (real AWS) — sync (test_s3.py) and async (test_s3_async.py)

test_write_transaction / test_write_transaction_rollback, parametrized over a small (one-shot PutObject) and a 10 MiB (multipart) size:

• write inside with fs.transaction: round-trips with the real content;
• raising inside the transaction leaves no object (CompleteMultipartUpload vs AbortMultipartUpload paths exercised end-to-end, incl. the async executor).

AWS cost. The large (10 MiB) cases are bounded to three uploads total (sync write, sync rollback, async write; the async rollback stays small since AioS3File inherits discard()). These live at the filesystem layer, so they run once each and are not multiplied across cursor types (pandas/arrow/polars). No multi-GiB cases are added.

Verification

• make lint — passes (ruff + format check + mypy).
• 21 unit + 7 integration transaction cases pass against real AWS.
• Confirmed the [False] unit cases fail when the source fix is reverted (regression guard), and the multipart mid-stream assertion fails under a plain return False (anti-simplification guard).

🤖 Generated with Claude Code