JingsongLi commented on code in PR #10114:
URL: https://github.com/apache/paimon/pull/10114#discussion_r4255149350
##########
paimon-python/pypaimon/write/file_store_commit.py:
##########
@@ -1023,22 +1039,36 @@ def abort(self, commit_messages: List[CommitMessage]):
paths must preserve prepared files instead of calling this method.
"""
for message in commit_messages:
+ preserve_blob_files = message.preserve_blob_files_on_abort
for file in (list(message.new_files) +
list(message.changelog_files)
+ list(message.compact_after)
+ list(message.compact_changelog_files)):
+ paths = []
path = None
+ data_path = None
try:
if not file.physical_path():
file.file_path = canonical_data_file_path(
self.table, tuple(message.partition),
message.bucket, file.file_name)
+ data_path = file.physical_path()
for path in file.collect_files():
- self.table.file_io.delete_quietly(path)
+ if preserve_blob_files and
_is_preserved_blob_pack(path):
+ continue
Review Comment:
[P2] Keep explicit commit abort consistent with Java
This skips cleanup of every `.blob` file whenever a `BlobConsumer` was
installed, even if the consumer only requests flushing and does not retain any
descriptor. With `with_blob_consumer(...) -> write -> prepare_commit() ->
commit.abort(messages)`, explicitly abandoned, uncommitted BLOB payloads remain
on disk.
Java's consumer preservation applies to writer cleanup
(`BlobFormatWriter.deleteFileUponAbort`); after preparation,
[`FileStoreCommitImpl.abort`](https://github.com/apache/paimon/blob/ca7dbebf00b2d802196ece55f9373a5c5f0cc60f/paimon-core/src/main/java/org/apache/paimon/operation/FileStoreCommitImpl.java#L697-L714)
deletes all new files in the messages without a consumer exception. The
existing Python README also documents this explicit-abort contract.
I reproduced this with a real append/data-evolution table: the exact base
deletes both Parquet and BLOB files, while this head leaves the BLOB file. A
Java v14 serialization round trip drops the flag and makes the same abort
delete all files again, so cleanup also depends on how the message is
transported.
Please retain the existing consumer protection for writer cleanup, but
remove the automatic preservation from explicit commit abort, along with the
flag and its native eligibility restriction. Add a real `prepare_commit() ->
commit.abort(messages)` regression test with a flush-only consumer asserting
that all newly written files are deleted.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
