XiaoHongbo-Hope commented on code in PR #8985:
URL: https://github.com/apache/paimon/pull/8985#discussion_r3704145933
##########
paimon-python/pypaimon/write/commit/overwrite_changes_provider.py:
##########
@@ -132,12 +132,17 @@ def _build_result(self, existing_entries:
List[ManifestEntry]) -> List[ManifestE
# New files being written by this overwrite.
for msg in self.commit_messages:
partition = GenericRow(list(msg.partition),
self.table.partition_keys_fields)
+ total_buckets = (
+ msg.total_buckets
+ if msg.total_buckets is not None
+ else self.table.total_buckets
+ )
for file in msg.new_files:
entries.append(ManifestEntry(
kind=0,
partition=partition,
bucket=msg.bucket,
- total_buckets=self.table.total_buckets,
+ total_buckets=total_buckets,
Review Comment:
> [P2] Preserve the replacement bucket count in partition statistics. This
list emits DELETE entries first with the old `total_buckets`, then ADD entries
with the new value. `_generate_partition_statistics` records only the first
value it sees for a partition, so an overwrite from postpone mode (`-2`) to `N`
leaves catalog partition statistics at `-2` even though the new manifest
entries use `N`. Java's `PartitionEntry.merge` takes the later entry's
`totalBuckets`. Please make the statistics aggregation last-wins (and add a
catalog-statistics assertion for `-2 -> N`, ideally also `N -> M`).
Fixed
##########
paimon-python/pypaimon/write/file_store_commit.py:
##########
@@ -211,6 +211,10 @@ def commit(self, commit_messages: List[CommitMessage],
commit_identifier: int):
if self.conflict_detection.has_hash_index_changes(
index_adds + index_deletes):
detect_conflicts = True
+ if any(message.total_buckets is not None
+ for message in commit_messages):
+ # Detect concurrent bucket-count changes in postpone APPENDs.
+ detect_conflicts = True
Review Comment:
> [P2] Keep deterministic CAS failures on the abortable conflict path.
Enabling conflict detection here exposes a cleanup gap: when `atomic_commit`
returns `False`, `_try_commit_once` creates `RetryResult(exception=None)`
because the snapshot definitely was not committed. If the retry then detects a
bucket-count conflict, `_try_commit` converts it to a generic exception solely
because `retry_result` is non-null. `TableCommit` aborts files only for
`CommitConflictError`, so the losing writer's files can be orphaned. Please
distinguish `retry_result.exception is None` from an uncertain commit exception
and retain `CommitConflictError` for the former; add a concurrent
different-plan test that also asserts cleanup.
Fixed
--
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]
