Skip to content

Commit df077b6

Browse files
wangzeyu03claude
andcommitted
[python] Move commit dedup parameters from table options to a runtime argument
The three commit.dedup-* options were read from CoreOptions, which flow through table.copy() and are inherited by every writer built from that table. The dedup bound describes one writer at one instant, so putting it in table properties causes cross-task contamination in the actor mode: a base_snapshot pinned for task 0 would be inherited by tasks 1..N as though they had also had their bounds validated. Replace them with a CommitDedup value object passed as a keyword-only argument to new_commit(dedup=...). The object is consumed by FileStoreCommit and never written to any property store. The three CoreOptions entries and their accessors are removed. Also log a WARNING when first-attempt dedup is requested on a table with commit.native.enabled=true: the native committer returns before the Python duplicate-commit scan is reached, so a replayed commit on that path can publish the same files twice. The scan is not implemented in the native path; the warning makes the hazard visible without blocking the write. Co-Authored-By: Claude Code <noreply@anthropic.com>
1 parent 695414d commit df077b6

7 files changed

Lines changed: 139 additions & 96 deletions

File tree

‎paimon-python/pypaimon/common/options/core_options.py‎

Lines changed: 0 additions & 55 deletions
Original file line numberDiff line numberDiff line change
@@ -804,52 +804,6 @@ class CoreOptions:
804804
.with_description("Maximum wait time between commit retries (e.g., '1s', '10s').")
805805
)
806806

807-
COMMIT_DEDUP_ON_FIRST_ATTEMPT: ConfigOption[bool] = (
808-
ConfigOptions.key("commit.dedup-on-first-attempt")
809-
.boolean_type()
810-
.default_value(False)
811-
.with_description(
812-
"Also run the duplicate-commit scan on the first commit attempt, not "
813-
"only on optimistic-lock retries. Enable when the same logical commit "
814-
"may be re-issued with a stable commit_user (e.g. a Ray committer-actor "
815-
"task retried after the actor died mid-commit), so an already-landed "
816-
"duplicate is skipped. Off by default: per-worker/two-phase commits use "
817-
"a fresh commit_user per builder, so a first-attempt scan would never "
818-
"match and would only add a snapshot scan per commit."
819-
)
820-
)
821-
822-
COMMIT_DEDUP_BASE_SNAPSHOT: ConfigOption[int] = (
823-
ConfigOptions.key("commit.dedup-base-snapshot")
824-
.int_type()
825-
.no_default_value()
826-
.with_description(
827-
"Lower bound for the first-attempt duplicate-commit scan: only snapshots "
828-
"after this id are checked. Should be the latest snapshot id the writer "
829-
"observed before issuing its first commit, so the scan is bounded to "
830-
"snapshots produced by this job rather than the table's whole history. "
831-
"Only used when commit.dedup-on-first-attempt is enabled; unset means "
832-
"scan from the first snapshot."
833-
)
834-
)
835-
836-
COMMIT_DEDUP_BASE_FILE_MILLIS: ConfigOption[int] = (
837-
ConfigOptions.key("commit.dedup-base-file-millis")
838-
.int_type()
839-
.no_default_value()
840-
.with_description(
841-
"Filesystem mtime, in epoch millis, of the snapshot file named by "
842-
"commit.dedup-base-snapshot when that bound was taken. Validates the "
843-
"bound: snapshot ids are re-used after a rollback, so a bare id can "
844-
"point at a different snapshot by the time the scan runs, and the "
845-
"scan would then start above this write's own commit and miss the "
846-
"duplicate. When the mtime no longer matches, the scan walks back "
847-
"from the current latest instead of trusting the id. Only used when "
848-
"commit.dedup-on-first-attempt is enabled; unset means the bound "
849-
"cannot be validated, so the whole history is scanned."
850-
)
851-
)
852-
853807
# Keys under this prefix are copied into the committed snapshot's properties
854808
# (re-prefixed with SNAPSHOT_COMMIT_PREFIX), letting a caller tag one commit
855809
# with its own metadata without the table schema having to know about it.
@@ -1782,15 +1736,6 @@ def commit_max_retry_wait(self) -> int:
17821736
wait = self.options.get(CoreOptions.COMMIT_MAX_RETRY_WAIT)
17831737
return int(wait.total_seconds() * 1000)
17841738

1785-
def commit_dedup_on_first_attempt(self) -> bool:
1786-
return self.options.get(CoreOptions.COMMIT_DEDUP_ON_FIRST_ATTEMPT)
1787-
1788-
def commit_dedup_base_snapshot(self, default=None) -> Optional[int]:
1789-
return self.options.get(CoreOptions.COMMIT_DEDUP_BASE_SNAPSHOT, default)
1790-
1791-
def commit_dedup_base_file_millis(self, default=None) -> Optional[int]:
1792-
return self.options.get(CoreOptions.COMMIT_DEDUP_BASE_FILE_MILLIS, default)
1793-
17941739
def global_index_enabled(self, default=None):
17951740
return self.options.get(CoreOptions.GLOBAL_INDEX_ENABLED, default)
17961741

‎paimon-python/pypaimon/tests/file_store_commit_test.py‎

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -142,9 +142,6 @@ def setUp(self):
142142
self.mock_table.table_path = '/test/table/path'
143143
self.mock_table.file_io = Mock()
144144
self.mock_table.options.manifest_target_size.return_value = 8 * 1024 * 1024
145-
self.mock_table.options.commit_dedup_on_first_attempt.return_value = False
146-
self.mock_table.options.commit_dedup_base_snapshot.return_value = None
147-
self.mock_table.options.commit_dedup_base_file_millis.return_value = None
148145
self.mock_table.options.options.to_map.return_value = {}
149146
self.mock_snapshot_commit = Mock()
150147

@@ -372,9 +369,6 @@ def setUp(self):
372369
self.mock_table.table_path = '/test/table/path'
373370
self.mock_table.file_io = Mock()
374371
self.mock_table.options.manifest_target_size.return_value = 8 * 1024 * 1024
375-
self.mock_table.options.commit_dedup_on_first_attempt.return_value = False
376-
self.mock_table.options.commit_dedup_base_snapshot.return_value = None
377-
self.mock_table.options.commit_dedup_base_file_millis.return_value = None
378372
self.mock_table.options.options.to_map.return_value = {}
379373

380374
# Mock snapshot commit

‎paimon-python/pypaimon/tests/table_commit_test.py‎

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -252,3 +252,45 @@ def test_stream_commit_forwards_snapshot_properties(self):
252252
commit_identifier=42,
253253
snapshot_properties={"checkpoint": "42"},
254254
)
255+
256+
def test_native_commit_warns_when_first_attempt_dedup_requested(self):
257+
"""The native committer has no dedup scan, so a replayed commit would
258+
publish the same files twice. The commit still goes ahead -- the scan
259+
is not implemented on that path -- but the caller is told."""
260+
from pypaimon.write.file_store_commit import CommitDedup
261+
262+
table = Mock()
263+
table.identifier = 'default.test_table'
264+
table.new_snapshot_commit.return_value = Mock()
265+
table.options.native_commit_enabled.return_value = True
266+
267+
with self.assertLogs(
268+
'pypaimon.write.table_commit', level='WARNING') as logs:
269+
commit = BatchTableCommit(
270+
table, 'stable-user', None,
271+
dedup=CommitDedup(on_first_attempt=True))
272+
273+
self.assertEqual(commit.file_store_commit.table, table)
274+
self.assertTrue(any(
275+
'commit.native.enabled' in line and 'stable-user' in line
276+
for line in logs.output), logs.output)
277+
278+
def test_native_commit_does_not_warn_without_first_attempt_dedup(self):
279+
import logging as _logging
280+
from unittest.mock import patch
281+
from pypaimon.write.file_store_commit import CommitDedup
282+
283+
table = Mock()
284+
table.identifier = 'default.test_table'
285+
table.new_snapshot_commit.return_value = Mock()
286+
table.options.native_commit_enabled.return_value = True
287+
288+
# assertNoLogs is 3.10+; patch warning explicitly so older
289+
# interpreters keep working too.
290+
logger = _logging.getLogger('pypaimon.write.table_commit')
291+
with patch.object(logger, 'warning') as warn:
292+
BatchTableCommit(
293+
table, 'stable-user', None,
294+
dedup=CommitDedup(on_first_attempt=False))
295+
BatchTableCommit(table, 'stable-user', None, dedup=None)
296+
warn.assert_not_called()

‎paimon-python/pypaimon/write/file_store_commit.py‎

Lines changed: 40 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -69,6 +69,38 @@ class DedupEvidenceMissing(RuntimeError):
6969
"""
7070

7171

72+
class CommitDedup:
73+
"""Duplicate-commit scan parameters for one committer.
74+
75+
Passed to :class:`FileStoreCommit` (via ``WriteBuilder.new_commit``) rather
76+
than read from table options: the bound describes *this* writer at *this*
77+
moment -- the branch's latest snapshot when the write started -- and would be
78+
wrong for any other writer inheriting it from the table schema.
79+
80+
``base_snapshot_id`` and ``base_file_millis`` are only meaningful together:
81+
the id bounds the scan and the mtime proves the id still names the snapshot
82+
it named when the bound was taken. A caller with no bound leaves both None.
83+
84+
Plain class rather than a dataclass, which pypaimon still supports Python
85+
3.6 without.
86+
"""
87+
88+
__slots__ = ("on_first_attempt", "base_snapshot_id", "base_file_millis")
89+
90+
def __init__(self, on_first_attempt: bool = False,
91+
base_snapshot_id: Optional[int] = None,
92+
base_file_millis: Optional[int] = None):
93+
self.on_first_attempt = on_first_attempt
94+
self.base_snapshot_id = base_snapshot_id
95+
self.base_file_millis = base_file_millis
96+
97+
def __repr__(self) -> str:
98+
return ("CommitDedup(on_first_attempt=%r, base_snapshot_id=%r, "
99+
"base_file_millis=%r)" % (
100+
self.on_first_attempt, self.base_snapshot_id,
101+
self.base_file_millis))
102+
103+
72104
def _row_id_check_from_messages(messages: List[CommitMessage]) -> Optional[int]:
73105
"""Check conflicts from the earliest baseline, matching the native committer."""
74106
check_from_snapshot = None
@@ -161,13 +193,15 @@ class FileStoreCommit:
161193
"""
162194

163195
def __init__(self, snapshot_commit: SnapshotCommit, table, commit_user: str,
164-
commit_callbacks: Optional[List[CommitCallback]] = None):
196+
commit_callbacks: Optional[List[CommitCallback]] = None,
197+
dedup: Optional[CommitDedup] = None):
165198
from pypaimon.table.file_store_table import FileStoreTable
166199

167200
self.snapshot_commit = snapshot_commit
168201
self.table: FileStoreTable = table
169202
self.commit_user = commit_user
170203
self.commit_callbacks: List[CommitCallback] = commit_callbacks if commit_callbacks is not None else []
204+
self.dedup = dedup if dedup is not None else CommitDedup()
171205

172206
self.snapshot_manager = table.snapshot_manager()
173207
self.manifest_file_manager = ManifestFileManager(table)
@@ -179,9 +213,6 @@ def __init__(self, snapshot_commit: SnapshotCommit, table, commit_user: str,
179213
self.commit_timeout = table.options.commit_timeout()
180214
self.commit_min_retry_wait = table.options.commit_min_retry_wait()
181215
self.commit_max_retry_wait = table.options.commit_max_retry_wait()
182-
self.commit_dedup_on_first_attempt = table.options.commit_dedup_on_first_attempt()
183-
self.commit_dedup_base_snapshot = table.options.commit_dedup_base_snapshot()
184-
self.commit_dedup_base_file_millis = table.options.commit_dedup_base_file_millis()
185216

186217
self.row_id_conflict_rewrite_max_size = (
187218
table.options.data_evolution_row_id_conflict_rewrite_max_size()
@@ -874,7 +905,7 @@ def _dedup_start_after_rollback(self, end_id: int) -> Tuple[int, bool]:
874905
older than the write's start therefore finds the lowest id this write
875906
could have reached.
876907
"""
877-
base_millis = self.commit_dedup_base_file_millis
908+
base_millis = self.dedup.base_file_millis
878909
for snapshot_id in range(end_id, 0, -1):
879910
file_millis = self.snapshot_manager.snapshot_file_millis(snapshot_id)
880911
if file_millis is None:
@@ -904,20 +935,20 @@ def _dedup_scan_window(self, retry_result, end_id: int) -> Tuple[int, bool]:
904935
return retry_result.latest_snapshot.id + 1, True
905936
return 1, True # Snapshot.FIRST_SNAPSHOT_ID
906937

907-
base_id = self.commit_dedup_base_snapshot
938+
base_id = self.dedup.base_snapshot_id
908939
if base_id is None:
909940
return 1, True
910941
if base_id == 0:
911942
# The branch had no snapshot when this write started, so every
912943
# snapshot on it is a candidate and none is missing from below.
913944
return 1, True
914-
if self.commit_dedup_base_file_millis is None:
945+
if self.dedup.base_file_millis is None:
915946
# Bound without the mtime that validates it: cannot tell a normal id
916947
# line from a rolled-back one, so scan everything.
917948
return 1, base_id <= end_id
918949

919950
base_millis = self.snapshot_manager.snapshot_file_millis(base_id)
920-
if base_millis == self.commit_dedup_base_file_millis:
951+
if base_millis == self.dedup.base_file_millis:
921952
# The bound still names the same file, so ids above it were handed
922953
# out normally and our commit, if any, is among them. Expiry trims
923954
# the oldest snapshots first, so a surviving base also proves the
@@ -1018,7 +1049,7 @@ def _is_duplicate_commit(
10181049
commit_kind,
10191050
notify_callbacks=False) -> bool:
10201051
if (not isinstance(retry_result, CommitFailRetryResult)
1021-
and not self.commit_dedup_on_first_attempt):
1052+
and not self.dedup.on_first_attempt):
10221053
# First attempt of a fresh commit_user commit: it cannot be a
10231054
# duplicate, so skip the scan entirely (per-worker/two-phase
10241055
# default).

‎paimon-python/pypaimon/write/ray_datasink.py‎

Lines changed: 17 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@
3535
import pyarrow as pa
3636

3737
from pypaimon.write.commit_callback import CommitCallback
38+
from pypaimon.write.file_store_commit import CommitDedup
3839

3940
if TYPE_CHECKING:
4041
from pypaimon.table.table import Table
@@ -725,9 +726,9 @@ class PaimonCommitterActor:
725726
(``{job_id}-{write_uuid}-{task_idx}``) and sends it, with the
726727
driver-pinned ``base_snapshot_id``, in the RPC args. If the actor dies
727728
mid-commit, Ray re-issues the *same* actor task with the *same* args, so
728-
the replayed commit carries the same ``commit_user``; the injected
729-
``commit.dedup-on-first-attempt`` scan then finds the already-landed
730-
snapshot (same ``commit_user`` + ``commit_identifier`` + ``commit_kind``)
729+
the replayed commit carries the same ``commit_user``; the first-attempt
730+
dedup scan then finds the already-landed snapshot (same ``commit_user`` +
731+
``commit_identifier`` + ``commit_kind``)
731732
and skips it instead of committing the same files twice. Without this the
732733
fresh builder's random commit_user would make every replay a duplicate.
733734
This does not extend across jobs -- see :class:`PaimonActorDatasink`.
@@ -864,18 +865,21 @@ def commit(
864865
and commit_user is not None
865866
and commit_user not in self._attempted_users
866867
)
867-
options['commit.dedup-on-first-attempt'] = 'false' if can_vouch else 'true'
868-
if dedup_base is not None:
869-
base_id = dedup_base.get("snapshot_id")
870-
if base_id is not None:
871-
options['commit.dedup-base-snapshot'] = str(base_id)
872-
file_millis = dedup_base.get("file_millis")
873-
if file_millis is not None:
874-
options['commit.dedup-base-file-millis'] = str(file_millis)
868+
# A runtime argument to new_commit, not an option: it describes this one
869+
# commit attempt, whereas options reach the committer through
870+
# table.copy() and would be inherited by every other writer built from
871+
# that table.
872+
dedup = CommitDedup(
873+
on_first_attempt=not can_vouch,
874+
base_snapshot_id=(
875+
dedup_base.get("snapshot_id") if dedup_base else None),
876+
base_file_millis=(
877+
dedup_base.get("file_millis") if dedup_base else None),
878+
)
875879

876880
builder = self._table.new_batch_write_builder()
877881
# with_options returns a new builder; discarding it silently drops the
878-
# options (commit.metadata.* and the dedup options would never land).
882+
# options (commit.metadata.* would never land).
879883
builder = builder.with_options(options)
880884
if commit_user is not None:
881885
# with_options rebuilt the builder (regenerating a random
@@ -884,7 +888,7 @@ def commit(
884888
builder.commit_user = commit_user
885889

886890
# Create commit and pass message via snapshot_properties if provided
887-
table_commit = builder.new_commit()
891+
table_commit = builder.new_commit(dedup=dedup)
888892
snapshot_properties = {}
889893
if message:
890894
snapshot_properties['commit.message'] = message

‎paimon-python/pypaimon/write/table_commit.py‎

Lines changed: 25 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -18,10 +18,11 @@
1818
import logging
1919
from typing import Dict, List, Optional
2020

21+
from pypaimon.common.options.core_options import CoreOptions
2122
from pypaimon.snapshot.snapshot import BATCH_COMMIT_IDENTIFIER
2223
from pypaimon.write.commit_callback import CommitCallback
2324
from pypaimon.write.commit_message import CommitMessage
24-
from pypaimon.write.file_store_commit import FileStoreCommit
25+
from pypaimon.write.file_store_commit import CommitDedup, FileStoreCommit
2526

2627
logger = logging.getLogger(__name__)
2728

@@ -39,7 +40,8 @@ class TableCommit:
3940
on every call and may be reused for many commits.
4041
"""
4142

42-
def __init__(self, table, commit_user: str, static_partition: Optional[dict]):
43+
def __init__(self, table, commit_user: str, static_partition: Optional[dict],
44+
*, dedup: Optional[CommitDedup] = None):
4345
from pypaimon.table.file_store_table import FileStoreTable
4446

4547
self.table: FileStoreTable = table
@@ -56,7 +58,21 @@ def __init__(self, table, commit_user: str, static_partition: Optional[dict]):
5658
self._native_callbacks_registered = False
5759
self.file_store_commit = FileStoreCommit(
5860
snapshot_commit, table, commit_user,
59-
commit_callbacks=self._commit_callbacks)
61+
commit_callbacks=self._commit_callbacks, dedup=dedup)
62+
if (dedup is not None and dedup.on_first_attempt
63+
and table.options.native_commit_enabled()):
64+
# The native committer has no duplicate-commit scan, and _commit
65+
# returns as soon as it publishes, so the Python dedup below is
66+
# never reached. A caller only asks for first-attempt dedup because
67+
# it may re-issue this commit with the same commit_user, and on the
68+
# native path that re-issue commits the files a second time.
69+
logger.warning(
70+
"Table %s has %s enabled while this commit requests "
71+
"first-attempt duplicate detection. The native committer does "
72+
"not implement that scan, so a re-issued commit with "
73+
"commit_user %s can publish the same files twice.",
74+
table.identifier, CoreOptions.COMMIT_NATIVE_ENABLED.key(),
75+
commit_user)
6076

6177
def add_commit_callback(self, callback: CommitCallback) -> None:
6278
"""Register a callback to be invoked after each successful commit."""
@@ -167,8 +183,9 @@ def close(self):
167183
class BatchTableCommit(TableCommit):
168184
"""Batch-mode commit; supports at most one commit per instance."""
169185

170-
def __init__(self, table, commit_user: str, static_partition: Optional[dict]):
171-
super().__init__(table, commit_user, static_partition)
186+
def __init__(self, table, commit_user: str, static_partition: Optional[dict],
187+
*, dedup: Optional[CommitDedup] = None):
188+
super().__init__(table, commit_user, static_partition, dedup=dedup)
172189
self.batch_committed = False
173190

174191
def commit(
@@ -205,8 +222,9 @@ class StreamTableCommit(TableCommit):
205222
:meth:`StreamTableWrite.prepare_commit`.
206223
"""
207224

208-
def __init__(self, table, commit_user: str):
209-
super().__init__(table, commit_user, None)
225+
def __init__(self, table, commit_user: str,
226+
*, dedup: Optional[CommitDedup] = None):
227+
super().__init__(table, commit_user, None, dedup=dedup)
210228

211229
def commit(
212230
self,

0 commit comments

Comments
 (0)