JingsongLi commented on code in PR #7873:
URL: https://github.com/apache/paimon/pull/7873#discussion_r3460597817
##########
paimon-python/pypaimon/write/file_store_commit.py:
##########
@@ -189,6 +191,116 @@ def commit(self, commit_messages: List[CommitMessage],
commit_identifier: int):
allow_rollback=allow_rollback,
index_deletes=index_deletes)
+ def commit_compact(self, commit_messages: List[CommitMessage],
commit_identifier: int):
+ """Commit compaction-only messages.
+
+ Each message must carry no new_files; compact_before → DELETE entries,
+ compact_after → ADD entries; snapshot kind = COMPACT. Skips row-id
+ assignment and conflict detection (a compaction never produces new
rows).
+ """
+ if not commit_messages:
+ return
+
+ for msg in commit_messages:
+ if msg.new_files:
Review Comment:
`commit_compact()` should reject any non-empty `data_increment`, not only
`new_files`. Today a message such as `CommitMessage(...,
data_increment=DataIncrement(changelog_files=[...]))` passes this check,
`_build_commit_entries()` deliberately produces no delta entries for data-side
changelog files, and the `if not commit_entries and not index_deletes` branch
below returns without committing or raising. That silently drops a non-empty
commit message. Please change this guard to `if not
msg.data_increment.is_empty(): raise ...` (or explicitly route data-side
changelog entries through `commit()`), and add a regression test for a
data-increment changelog-only compact message.
##########
paimon-python/pypaimon/write/file_store_commit.py:
##########
@@ -189,6 +191,116 @@ def commit(self, commit_messages: List[CommitMessage],
commit_identifier: int):
allow_rollback=allow_rollback,
index_deletes=index_deletes)
+ def commit_compact(self, commit_messages: List[CommitMessage],
commit_identifier: int):
+ """Commit compaction-only messages.
+
+ Each message must carry no new_files; compact_before → DELETE entries,
+ compact_after → ADD entries; snapshot kind = COMPACT. Skips row-id
Review Comment:
Once `commit_compact()` accepts `compact_after` files, the abort path needs
to clean them as well. `FileStoreCommit.abort()` still iterates only over
`message.new_files + message.changelog_files`, so a failed/cancelled compaction
would leave the rewritten `compact_after` files (and any future
`compact_changelog_files`) behind, unlike the Java abort path which includes
`compactIncrement().compactAfter()` and compact changelog files. Please extend
`abort()` and add a regression test for aborting a compact message.
--
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]
