diff --git a/docs/docs/pypaimon/ray-data.md b/docs/docs/pypaimon/ray-data.md index 88c0a7143ac4..6d01b6aa8197 100644 --- a/docs/docs/pypaimon/ray-data.md +++ b/docs/docs/pypaimon/ray-data.md @@ -524,6 +524,7 @@ metrics = update_by_row_id( source=ray_dataset, # ray.data.Dataset / pa.Table / pandas, carrying _ROW_ID catalog_options={"warehouse": "/path/to/warehouse"}, update_cols=["feature"], # non-blob columns to overwrite + max_groups_per_commit=64, # optional incremental commit window ) print(metrics) # {"num_updated": 50} ``` @@ -537,6 +538,9 @@ print(metrics) # {"num_updated": 50} - `num_partitions`: parallelism for grouping the update rows by target file; defaults to `max(1, cluster_cpus * 2)`. - `ray_remote_args`: Ray remote options applied to the update tasks. +- `max_groups_per_commit`: optional positive number of completed target file groups + per commit. By default, all groups are committed together after every worker + finishes. For example, `64` commits after each 64 `_FIRST_ROW_ID` file groups. **Returns:** `{"num_updated": }`. @@ -548,6 +552,11 @@ print(metrics) # {"num_updated": 50} - Partition columns cannot be updated (in-place rewrite can't move a row across partitions). - Deletion-vectors-enabled tables are not supported yet: a DV-deleted row still lives in its data file, so it can't be told apart from a live row without reading the target. +- Incremental commit mode overlaps later worker writes with earlier commits, but is not + atomic across the whole operation. If a later group fails, earlier committed windows + remain visible and the table is partially updated. Inspect or reconcile that state, + then rerun the intended update as appropriate. It stops on concurrent commits, + except overwrites outside the current row-ID scope. ## Read By Row Id diff --git a/paimon-python/pypaimon/ray/data_evolution_merge_into.py b/paimon-python/pypaimon/ray/data_evolution_merge_into.py index d43ac21f36bc..13fa25755f13 100644 --- a/paimon-python/pypaimon/ray/data_evolution_merge_into.py +++ b/paimon-python/pypaimon/ray/data_evolution_merge_into.py @@ -522,14 +522,27 @@ def _require_ray_join() -> None: def _reraise_inner(err: BaseException) -> None: """Unwrap Ray's RayTaskError so callers see the worker-side exception.""" + try: + from ray.exceptions import RayTaskError + except ImportError: + raise err + + if not isinstance(err, RayTaskError): + raise err + inner = err - cause = getattr(err, "cause", None) or getattr(err, "__cause__", None) - while cause is not None: + seen = {id(err)} + cause = getattr(err, "cause", None) + while cause is not None and id(cause) not in seen: + seen.add(id(cause)) inner = cause - cause = getattr(inner, "cause", None) or getattr(inner, "__cause__", None) + cause = ( + getattr(inner, "cause", None) + if isinstance(inner, RayTaskError) else None + ) if inner is err: raise err - raise inner from err + raise inner from None def _validate_disjoint_action_row_ids(update_row_ids, delete_row_ids) -> None: diff --git a/paimon-python/pypaimon/ray/data_evolution_merge_join.py b/paimon-python/pypaimon/ray/data_evolution_merge_join.py index 359af8e6efa2..c300a1cd87fe 100644 --- a/paimon-python/pypaimon/ray/data_evolution_merge_join.py +++ b/paimon-python/pypaimon/ray/data_evolution_merge_join.py @@ -16,7 +16,7 @@ # limitations under the License. ################################################################################ -from typing import Any, Dict, List, Optional, Sequence, Tuple +from typing import Any, Callable, Dict, List, Optional, Sequence, Tuple import pyarrow as pa @@ -445,7 +445,15 @@ def distributed_update_apply( ray_remote_args: Optional[Dict[str, Any]] = None, base_snapshot_id: Optional[int] = None, collect_row_ids: bool = False, + on_group_result: Optional[Callable[[list, int, list], None]] = None, ) -> Tuple[list, int, list]: + """Apply updates grouped by target file and collect their commit messages. + + When ``on_group_result`` is set, it is called on the driver once for every + completed ``_FIRST_ROW_ID`` group. Commit messages are delivered to the + callback instead of being retained in the returned list, which lets callers + commit completed file groups incrementally while the remaining groups run. + """ import numpy as np import pickle import uuid @@ -598,14 +606,31 @@ def _apply_group(group: pa.Table) -> pa.Table: all_msgs: list = [] num_updated = 0 action_row_ids = [] - for batch in msgs_ds.iter_batches(batch_format="pyarrow"): - for blob in batch.column("msgs_blob").to_pylist(): - all_msgs.extend(pickle.loads(blob)) - for n in batch.column("n_updated").to_pylist(): + iter_batches_kwargs = {"batch_format": "pyarrow"} + if on_group_result is not None: + # Yield each group immediately. + iter_batches_kwargs["batch_size"] = 1 + iter_batches_kwargs["prefetch_batches"] = 0 + for batch in msgs_ds.iter_batches(**iter_batches_kwargs): + message_blobs = batch.column("msgs_blob").to_pylist() + updated_counts = batch.column("n_updated").to_pylist() + row_id_blobs = ( + batch.column("row_ids_blob").to_pylist() + if collect_row_ids else [None] * len(message_blobs) + ) + for blob, n, row_ids_blob in zip( + message_blobs, updated_counts, row_id_blobs): + group_msgs = pickle.loads(blob) + group_row_ids = ( + pickle.loads(row_ids_blob) if collect_row_ids else [] + ) + if on_group_result is None: + all_msgs.extend(group_msgs) + else: + on_group_result(group_msgs, n, group_row_ids) num_updated += n - if collect_row_ids: - for blob in batch.column("row_ids_blob").to_pylist(): - action_row_ids.extend(pickle.loads(blob)) + if collect_row_ids: + action_row_ids.extend(group_row_ids) return all_msgs, num_updated, action_row_ids diff --git a/paimon-python/pypaimon/ray/update_by_row_id.py b/paimon-python/pypaimon/ray/update_by_row_id.py index 41b968c4fdc8..0fdd1034c1a1 100644 --- a/paimon-python/pypaimon/ray/update_by_row_id.py +++ b/paimon-python/pypaimon/ray/update_by_row_id.py @@ -37,6 +37,7 @@ from pypaimon.ray.data_evolution_merge_join import distributed_update_apply from pypaimon.ray.data_evolution_merge_transform import build_update_schema from pypaimon.schema.data_types import is_blob_file_field +from pypaimon.write.file_store_commit import CommitOutcomeUnknownError __all__ = ["update_by_row_id"] @@ -56,6 +57,7 @@ def update_by_row_id( update_cols: List[str], num_partitions: Optional[int] = None, ray_remote_args: Optional[Dict[str, Any]] = None, + max_groups_per_commit: Optional[int] = None, ) -> Dict[str, int]: """Update ``update_cols`` of a data-evolution table by ``_ROW_ID``. @@ -65,6 +67,16 @@ def update_by_row_id( the target is never fully read and there is no join against it. Requires ``ray >= 2.50`` and a target with ``data-evolution.enabled`` + ``row-tracking.enabled``. + By default all file groups are committed atomically after every worker + finishes. Set ``max_groups_per_commit`` to incrementally commit each completed + window of target file groups. Incremental mode can leave earlier windows + committed if a later window fails. The table may therefore be partially + updated; callers must reconcile the result or rerun the intended update. + Incremental mode stops on concurrent commits, except overwrites outside + the current row-ID scope. + Concurrent snapshot expiration can also invalidate a committed-window + lineage check; the operation then stops with earlier windows still visible. + Returns ``{"num_updated": }``. """ from pypaimon.catalog.catalog_factory import CatalogFactory @@ -74,6 +86,11 @@ def update_by_row_id( _require_ray_join() if not update_cols: raise ValueError("update_cols must be non-empty.") + if max_groups_per_commit is not None: + if (isinstance(max_groups_per_commit, bool) + or not isinstance(max_groups_per_commit, int) + or max_groups_per_commit <= 0): + raise ValueError("max_groups_per_commit must be a positive integer.") update_cols = list(dict.fromkeys(update_cols)) # de-dup, keep order num_partitions = _resolve_num_partitions(num_partitions) @@ -139,22 +156,239 @@ def _project_cast(batch: pa.Table) -> pa.Table: raise ValueError( f"target '{target}' has no rows; every _ROW_ID in the source is foreign.") return {"num_updated": 0} + incremental_committer = ( + _IncrementalUpdateCommitter( + table, max_groups_per_commit, base.id + ) + if max_groups_per_commit is not None else None + ) try: + apply_kwargs = { + "num_partitions": num_partitions, + "ray_remote_args": ray_remote_args, + "base_snapshot_id": base.id, + } + if incremental_committer is not None: + apply_kwargs["on_group_result"] = incremental_committer.add_group msgs, num_updated, _ = distributed_update_apply( - update_ds, table, update_cols, - num_partitions=num_partitions, - ray_remote_args=ray_remote_args, - base_snapshot_id=base.id, + update_ds, table, update_cols, **apply_kwargs ) + if incremental_committer is not None: + incremental_committer.finish() except Exception as e: - _reraise_inner(e) - raise # _reraise_inner always raises; keeps msgs/num_updated defined for linters - - if msgs: + if incremental_committer is not None: + incremental_committer.abort_pending() + try: + _reraise_inner(e) + except Exception as worker_error: + deferred_error = ( + incremental_committer._deferred_error + if incremental_committer is not None else None + ) + # A commit error can be deferred while the remaining Ray groups drain. + # Preserve that earlier failure if a later worker fails, but do not + # chain an error to itself when finish() raised the deferred error. + if (deferred_error is not None + and deferred_error is not worker_error): + if deferred_error.__cause__ is not None: + # Python has only one explicit cause. Keep the original commit + # cause and report the later update failure separately. + logger.warning( + "A later update error occurred after an earlier incremental " + "commit error; preserving the original commit error: %s", + worker_error, + exc_info=( + type(worker_error), + worker_error, + worker_error.__traceback__, + ), + ) + raise deferred_error + raise deferred_error from worker_error + raise + finally: + if incremental_committer is not None: + incremental_committer.close() + if incremental_committer is None and msgs: _commit_update_messages(table, msgs) return {"num_updated": num_updated} +class _IncrementalUpdateCommitter: + + def __init__(self, table, max_groups_per_commit: int, + base_snapshot_id: Optional[int] = None): + self._table = table + self._max_groups_per_commit = max_groups_per_commit + self._base_snapshot_id = base_snapshot_id + self._pending_messages: list = [] + self._pending_groups = 0 + self._table_commit = None + self._next_commit_identifier = 1 + self._deferred_error = None + # Never abort messages whose commit outcome is unknown: their files may + # already be referenced by a snapshot. If not, orphan cleanup reclaims them. + self._uncertain_messages: list = [] + self._last_committed_snapshot = None + + def add_group(self, commit_messages, _num_updated, _row_ids) -> None: + if self._deferred_error is not None: + # Drain workers without retaining later uncommitted files. + self._abort_messages(commit_messages) + return + + self._pending_messages.extend(commit_messages) + self._pending_groups += 1 + if self._pending_groups >= self._max_groups_per_commit: + self._commit_pending() + + def finish(self) -> None: + if self._deferred_error is None: + self._commit_pending() + if self._deferred_error is None: + try: + self._validate_committed_snapshot() + except Exception as error: + self._deferred_error = error + if self._deferred_error is not None: + raise self._deferred_error + + def _commit_pending(self) -> None: + if self._pending_groups == 0: + return + if not self._pending_messages: + self._pending_groups = 0 + return + + if self._table_commit is None: + try: + self._table_commit = ( + self._table.new_stream_write_builder().new_commit() + ) + self._table_commit.enable_bounded_row_id_conflict_state() + except Exception as error: + # Safe to abort before commit starts. + self._deferred_error = error + return + + group_count = self._pending_groups + commit_identifier = self._next_commit_identifier + try: + self._validate_committed_snapshot() + except Exception as error: + self._deferred_error = error + return + try: + self._table_commit.commit( + self._pending_messages, commit_identifier + ) + except Exception as error: + if isinstance(error, CommitOutcomeUnknownError): + self._uncertain_messages.extend(self._pending_messages) + self._pending_messages = [] + self._pending_groups = 0 + self._deferred_error = error + return + + committed_messages = self._pending_messages + self._pending_messages = [] + self._pending_groups = 0 + self._next_commit_identifier += 1 + try: + self._record_committed_snapshot(commit_identifier) + # Later windows are disjoint by _FIRST_ROW_ID. + self._table_commit.ignore_row_id_conflict_for_commit( + commit_identifier) + self._validate_committed_snapshot() + except Exception as error: + self._uncertain_messages.extend(committed_messages) + self._deferred_error = error + return + logger.info( + "Incrementally committed %d update_by_row_id file groups.", + group_count, + ) + + def _record_committed_snapshot(self, commit_identifier: int) -> None: + if self._base_snapshot_id is None: + return + + self._validate_committed_snapshot() + snapshot_manager = self._table.snapshot_manager() + latest = snapshot_manager.get_latest_snapshot() + start_id = ( + self._last_committed_snapshot.id + 1 + if self._last_committed_snapshot is not None + else self._base_snapshot_id + 1 + ) + if latest is not None: + for snapshot_id in range(latest.id, start_id - 1, -1): + snapshot = snapshot_manager.get_snapshot_by_id(snapshot_id) + if (snapshot is not None + and snapshot.commit_user == self._table_commit.commit_user + and snapshot.commit_identifier == commit_identifier + and snapshot.commit_kind == "APPEND"): + self._last_committed_snapshot = snapshot + return + raise RuntimeError( + "Incremental update_by_row_id snapshot is no longer in the " + "current lineage." + ) + + def _validate_committed_snapshot(self) -> None: + committed = self._last_committed_snapshot + if committed is None: + return + + snapshot_manager = self._table.snapshot_manager() + latest = snapshot_manager.get_latest_snapshot() + current = snapshot_manager.get_snapshot_by_id(committed.id) + if (latest is None or latest.id < committed.id + or current != committed): + raise RuntimeError( + "Incremental update_by_row_id snapshot is no longer in the " + "current lineage." + ) + + def abort_pending(self) -> None: + if not self._pending_messages: + return + + pending_messages = self._pending_messages + self._pending_messages = [] + self._pending_groups = 0 + self._abort_messages(pending_messages) + + def _abort_messages(self, messages) -> None: + if not messages: + return + + if self._table_commit is None: + _abort_pending_update_messages(self._table, messages) + return + + try: + self._table_commit.abort(messages) + except Exception as abort_error: + logger.warning( + "Failed to abort pending incremental update_by_row_id messages: %s", + abort_error, + exc_info=abort_error, + ) + + def close(self) -> None: + if self._table_commit is None: + return + try: + self._table_commit.close() + except Exception as close_error: + logger.warning( + "Failed to close incremental update_by_row_id commit: %s", + close_error, + exc_info=close_error, + ) + + def _commit_update_messages(table, commit_messages) -> None: pending_msgs: list = list(commit_messages) commit_started = False @@ -176,7 +410,8 @@ def _commit_update_messages(table, commit_messages) -> None: exc_info=close_error, ) except Exception as e: - if not commit_started: + if (not commit_started + or not isinstance(e, CommitOutcomeUnknownError)): _abort_pending_update_messages(table, pending_msgs) _reraise_inner(e) diff --git a/paimon-python/pypaimon/read/scanner/file_scanner.py b/paimon-python/pypaimon/read/scanner/file_scanner.py index c2fcff26a9c9..7647bbd84e32 100755 --- a/paimon-python/pypaimon/read/scanner/file_scanner.py +++ b/paimon-python/pypaimon/read/scanner/file_scanner.py @@ -290,6 +290,10 @@ def __init__( self.table.table_schema.id ) + @property + def scanned_snapshot(self): + return self._scanned_snapshot + def _schema_fields(self, schema_id: int): """Resolve schema fields, short-circuiting current table schema id to avoid filesystem access (REST catalog would get 403). @@ -857,7 +861,8 @@ def _scan_dv_index(self, snapshot, buckets: Set[tuple]) -> Dict[tuple, Dict[str, # Convert to deletion files deletion_files = self._to_deletion_files(entry) if deletion_files: - result[partition_bucket] = deletion_files + result.setdefault(partition_bucket, {}).update( + deletion_files) return result diff --git a/paimon-python/pypaimon/tests/file_store_commit_test.py b/paimon-python/pypaimon/tests/file_store_commit_test.py index 98c8867fe646..7eb68002e001 100644 --- a/paimon-python/pypaimon/tests/file_store_commit_test.py +++ b/paimon-python/pypaimon/tests/file_store_commit_test.py @@ -19,12 +19,27 @@ from datetime import datetime from unittest.mock import MagicMock, Mock, patch +from pypaimon.api.rest_exception import BadRequestException, RESTException +from pypaimon.catalog.catalog_exception import ( + TableNoPermissionException, + TableNotExistException, +) +from pypaimon.common.identifier import Identifier from pypaimon.manifest.schema.data_file_meta import DataFileMeta from pypaimon.manifest.schema.manifest_entry import ManifestEntry from pypaimon.snapshot.snapshot_commit import PartitionStatistics from pypaimon.table.row.generic_row import GenericRow +from pypaimon.write.commit.conflict_detection import ( + CommitConflictError, + RowIdPlanningConflictError, +) from pypaimon.write.commit_message import CommitMessage -from pypaimon.write.file_store_commit import FileStoreCommit +from pypaimon.write.file_store_commit import ( + CommitOutcomeUnknownError, + FileStoreCommit, + RetryResult, + SuccessResult, +) @patch('pypaimon.write.file_store_commit.ManifestFileManager') @@ -54,6 +69,29 @@ def _create_file_store_commit(self): commit_user='test_user' ) + def _prepare_atomic_attempt(self, atomic_error=None, close_error=None): + file_store_commit = self._create_file_store_commit() + self.mock_table.identifier = 'default.test_table' + self.mock_table.table_schema = Mock(id=7) + self.mock_table.options.row_tracking_enabled.return_value = False + + snapshot_commit = MagicMock() + snapshot_commit.__enter__.return_value = snapshot_commit + snapshot_commit.__exit__.return_value = False + snapshot_commit.__exit__.side_effect = close_error + if atomic_error is None: + snapshot_commit.commit.return_value = True + else: + snapshot_commit.commit.side_effect = atomic_error + file_store_commit.snapshot_commit = snapshot_commit + file_store_commit._write_manifest_files = Mock(return_value=[Mock()]) + file_store_commit._generate_partition_statistics = Mock(return_value=[]) + file_store_commit.manifest_list_manager.read_all.return_value = [] + + commit_entry = Mock(kind=0) + commit_entry.file = Mock(row_count=1) + return file_store_commit, snapshot_commit, commit_entry + def test_generate_partition_statistics_single_partition_single_file( self, mock_manifest_list_manager, mock_manifest_file_manager): """Test partition statistics generation with single partition and single file.""" @@ -448,6 +486,551 @@ def test_append_commit_inherits_index_manifest( snapshot_commit.commit.call_args[0][0].index_manifest ) + def test_atomic_exception_has_unknown_outcome( + self, mock_manifest_list_manager, mock_manifest_file_manager): + file_store_commit = self._create_file_store_commit() + file_store_commit.commit_max_retries = 0 + file_store_commit.commit_timeout = 1000 + file_store_commit.snapshot_manager.get_latest_snapshot.return_value = None + atomic_error = RuntimeError("atomic commit failed") + file_store_commit._try_commit_once = Mock(return_value=RetryResult( + None, atomic_error, outcome_unknown=True)) + + with self.assertRaises(CommitOutcomeUnknownError) as raised: + file_store_commit._try_commit( + "APPEND", 1, lambda _snapshot: [Mock()]) + + self.assertIs(atomic_error, raised.exception.__cause__) + + def test_atomic_commit_exception_marks_retry_unknown( + self, mock_manifest_list_manager, mock_manifest_file_manager): + file_store_commit = self._create_file_store_commit() + self.mock_table.identifier = 'default.test_table' + self.mock_table.table_schema = Mock(id=7) + self.mock_table.options.row_tracking_enabled.return_value = False + atomic_error = RuntimeError("atomic commit failed") + snapshot_commit = MagicMock() + snapshot_commit.__enter__.return_value = snapshot_commit + snapshot_commit.__exit__.return_value = False + snapshot_commit.commit.side_effect = atomic_error + file_store_commit.snapshot_commit = snapshot_commit + file_store_commit._write_manifest_files = Mock(return_value=[Mock()]) + file_store_commit._generate_partition_statistics = Mock(return_value=[]) + file_store_commit.manifest_list_manager.read_all.return_value = [] + commit_entry = Mock(kind=0) + commit_entry.file = Mock(row_count=1) + + result = file_store_commit._try_commit_once( + retry_result=None, + commit_kind="APPEND", + commit_entries=[commit_entry], + changelog_entries=[], + commit_identifier=1, + latest_snapshot=None, + ) + + self.assertFalse(result.is_success()) + self.assertTrue(result.outcome_unknown) + self.assertIs(atomic_error, result.exception) + + def test_deterministic_atomic_rejections_are_not_retried( + self, mock_manifest_list_manager, mock_manifest_file_manager): + identifier = Identifier.create("default", "test_table") + wrapped_bad_request = RuntimeError("Failed to commit snapshot") + wrapped_bad_request.__cause__ = BadRequestException("bad request") + errors = [ + TableNotExistException(identifier), + TableNoPermissionException(identifier), + wrapped_bad_request, + ] + + for atomic_error in errors: + with self.subTest(error=type(atomic_error).__name__): + file_store_commit, snapshot_commit, commit_entry = ( + self._prepare_atomic_attempt(atomic_error=atomic_error) + ) + file_store_commit.commit_max_retries = 3 + file_store_commit.commit_timeout = 10_000 + file_store_commit._commit_retry_wait = Mock() + file_store_commit.snapshot_manager.get_latest_snapshot.return_value = None + + with self.assertRaises(type(atomic_error)) as raised: + file_store_commit._try_commit( + "APPEND", 1, lambda _snapshot: [commit_entry]) + + self.assertIs(atomic_error, raised.exception) + snapshot_commit.commit.assert_called_once() + file_store_commit._commit_retry_wait.assert_not_called() + + def test_atomic_transport_exception_remains_unknown( + self, mock_manifest_list_manager, mock_manifest_file_manager): + transport_error = RESTException( + "response lost", cause=TimeoutError("read timed out")) + file_store_commit, _, commit_entry = self._prepare_atomic_attempt( + atomic_error=transport_error) + + result = file_store_commit._try_commit_once( + retry_result=None, + commit_kind="APPEND", + commit_entries=[commit_entry], + changelog_entries=[], + commit_identifier=1, + latest_snapshot=None, + ) + + self.assertFalse(result.is_success()) + self.assertTrue(result.outcome_unknown) + self.assertIs(transport_error, result.exception) + + def test_exception_after_atomic_call_returns_remains_unknown( + self, mock_manifest_list_manager, mock_manifest_file_manager): + close_error = TableNoPermissionException( + Identifier.create("default", "test_table")) + file_store_commit, _, commit_entry = self._prepare_atomic_attempt( + close_error=close_error) + + result = file_store_commit._try_commit_once( + retry_result=None, + commit_kind="APPEND", + commit_entries=[commit_entry], + changelog_entries=[], + commit_identifier=1, + latest_snapshot=None, + ) + + self.assertFalse(result.is_success()) + self.assertTrue(result.outcome_unknown) + self.assertIs(close_error, result.exception) + + def test_false_commit_has_deterministic_outcome( + self, mock_manifest_list_manager, mock_manifest_file_manager): + file_store_commit = self._create_file_store_commit() + file_store_commit.commit_max_retries = 0 + file_store_commit.commit_timeout = 1000 + file_store_commit.snapshot_manager.get_latest_snapshot.return_value = None + file_store_commit._try_commit_once = Mock( + return_value=RetryResult(None)) + + with self.assertRaises(RuntimeError) as raised: + file_store_commit._try_commit( + "APPEND", 1, lambda _snapshot: [Mock()]) + + self.assertNotIsInstance( + raised.exception, CommitOutcomeUnknownError) + + def test_unknown_outcome_survives_later_failure( + self, mock_manifest_list_manager, mock_manifest_file_manager): + file_store_commit = self._create_file_store_commit() + file_store_commit.commit_max_retries = 2 + file_store_commit.commit_timeout = 1000 + file_store_commit.snapshot_manager.get_latest_snapshot.return_value = None + file_store_commit._commit_retry_wait = Mock() + atomic_error = RuntimeError("atomic commit failed") + file_store_commit._try_commit_once = Mock(side_effect=[ + RetryResult(None, atomic_error, outcome_unknown=True), + RuntimeError("conflict"), + ]) + + with self.assertRaises(CommitOutcomeUnknownError) as raised: + file_store_commit._try_commit( + "APPEND", 1, lambda _snapshot: [Mock()]) + + self.assertIs(atomic_error, raised.exception.__cause__) + + def test_atomic_failure_remains_unknown( + self, mock_manifest_list_manager, mock_manifest_file_manager): + file_store_commit = self._create_file_store_commit() + file_store_commit.commit_max_retries = 0 + file_store_commit.commit_timeout = 1000 + file_store_commit.snapshot_manager.get_latest_snapshot.return_value = None + atomic_error = RuntimeError("response lost") + file_store_commit._try_commit_once = Mock(return_value=RetryResult( + None, atomic_error, outcome_unknown=True)) + + with self.assertRaises(CommitOutcomeUnknownError) as raised: + file_store_commit._try_commit( + "APPEND", 1, lambda _snapshot: [Mock()]) + + self.assertIs(atomic_error, raised.exception.__cause__) + + def test_unknown_duplicate_resolves_to_success( + self, mock_manifest_list_manager, mock_manifest_file_manager): + file_store_commit = self._create_file_store_commit() + atomic_error = RuntimeError("response lost") + retry_result = RetryResult( + None, atomic_error, outcome_unknown=True) + latest_snapshot = Mock( + id=1, + commit_user="test_user", + commit_identifier=1, + commit_kind="APPEND", + ) + file_store_commit.snapshot_manager.get_snapshot_by_id.return_value = ( + latest_snapshot + ) + + result = file_store_commit._try_commit_once( + retry_result=retry_result, + commit_kind="APPEND", + commit_entries=[Mock()], + changelog_entries=[], + commit_identifier=1, + latest_snapshot=latest_snapshot, + ) + + self.assertTrue(result.is_success()) + + def test_unknown_retry_resolves_when_duplicate_is_found( + self, mock_manifest_list_manager, mock_manifest_file_manager): + file_store_commit = self._create_file_store_commit() + file_store_commit.commit_max_retries = 1 + file_store_commit.commit_timeout = 1000 + file_store_commit._commit_retry_wait = Mock() + file_store_commit.snapshot_manager.get_latest_snapshot.side_effect = [ + None, + Mock(id=1), + ] + atomic_error = RuntimeError("response lost") + file_store_commit._try_commit_once = Mock(side_effect=[ + RetryResult(None, atomic_error, outcome_unknown=True), + SuccessResult(), + ]) + + file_store_commit._try_commit( + "APPEND", 1, lambda _snapshot: [Mock()]) + + self.assertEqual(2, file_store_commit._try_commit_once.call_count) + + def test_unknown_retry_resolves_duplicate_before_replanning( + self, mock_manifest_list_manager, mock_manifest_file_manager): + file_store_commit = self._create_file_store_commit() + file_store_commit.commit_max_retries = 1 + file_store_commit.commit_timeout = 1000 + file_store_commit._commit_retry_wait = Mock() + committed_snapshot = Mock( + id=1, + commit_user="test_user", + commit_identifier=1, + commit_kind="OVERWRITE", + ) + file_store_commit.snapshot_manager.get_latest_snapshot.side_effect = [ + None, + committed_snapshot, + ] + file_store_commit.snapshot_manager.get_snapshot_by_id.return_value = ( + committed_snapshot + ) + atomic_error = RuntimeError("response lost") + file_store_commit._try_commit_once = Mock(return_value=RetryResult( + None, atomic_error, outcome_unknown=True)) + commit_entries_plan = Mock(side_effect=[[Mock()], []]) + + file_store_commit._try_commit( + "OVERWRITE", 1, commit_entries_plan) + + self.assertEqual(1, file_store_commit._try_commit_once.call_count) + self.assertEqual(1, commit_entries_plan.call_count) + get_snapshot_by_id = ( + file_store_commit.snapshot_manager.get_snapshot_by_id + ) + get_snapshot_by_id.assert_called_once_with(1) + + def test_unknown_retry_with_empty_plan_and_no_duplicate_remains_unknown( + self, mock_manifest_list_manager, mock_manifest_file_manager): + file_store_commit = self._create_file_store_commit() + file_store_commit.commit_max_retries = 1 + file_store_commit.commit_timeout = 1000 + file_store_commit._commit_retry_wait = Mock() + other_snapshot = Mock( + id=1, + commit_user="other_user", + commit_identifier=1, + commit_kind="OVERWRITE", + ) + file_store_commit.snapshot_manager.get_latest_snapshot.side_effect = [ + None, + other_snapshot, + ] + file_store_commit.snapshot_manager.get_snapshot_by_id.return_value = ( + other_snapshot + ) + atomic_error = RuntimeError("response lost") + file_store_commit._try_commit_once = Mock(return_value=RetryResult( + None, atomic_error, outcome_unknown=True)) + commit_entries_plan = Mock(side_effect=[[Mock()], []]) + + with self.assertRaises(CommitOutcomeUnknownError) as raised: + file_store_commit._try_commit( + "OVERWRITE", 1, commit_entries_plan) + + self.assertIs(atomic_error, raised.exception.__cause__) + self.assertEqual(1, file_store_commit._try_commit_once.call_count) + self.assertEqual(2, commit_entries_plan.call_count) + + def test_empty_plan_without_retry_remains_no_op( + self, mock_manifest_list_manager, mock_manifest_file_manager): + file_store_commit = self._create_file_store_commit() + file_store_commit._try_commit_once = Mock() + file_store_commit._is_duplicate_commit = Mock() + + file_store_commit._try_commit( + "OVERWRITE", 1, lambda _snapshot: []) + + file_store_commit._try_commit_once.assert_not_called() + file_store_commit._is_duplicate_commit.assert_not_called() + + def test_collects_row_id_window_base_entries( + self, mock_manifest_list_manager, mock_manifest_file_manager): + file_store_commit = self._create_file_store_commit() + self.mock_table.partition_keys_fields = [] + self.mock_table.total_buckets = 1 + base_file = Mock( + file_name="base.parquet", + level=0, + extra_files=[], + embedded_index=None, + external_path=None, + ) + identity = (7, "user", 1, "APPEND", 1, "base", "delta", None, None) + message = CommitMessage( + partition=(), + bucket=0, + new_files=[Mock()], + check_from_snapshot=7, + row_id_base_files=[base_file], + row_id_base_snapshot_identity=identity, + ) + + entries, snapshot = file_store_commit._collect_row_id_base_entries( + [message]) + + self.assertEqual(identity, snapshot) + self.assertEqual([base_file], [entry.file for entry in entries]) + + def test_first_row_id_planning_conflict_is_safe_to_abort( + self, mock_manifest_list_manager, mock_manifest_file_manager): + file_store_commit = self._create_file_store_commit() + conflict = RowIdPlanningConflictError("row ID planning conflict") + file_store_commit.conflict_detection.has_row_id_check_from_snapshot = ( + Mock(return_value=True)) + file_store_commit.conflict_detection.read_row_id_base_entries = Mock( + side_effect=conflict) + clear_window = Mock() + file_store_commit.conflict_detection.clear_row_id_window_changes = ( + clear_window) + + with self.assertRaises(CommitConflictError) as raised: + file_store_commit._try_commit_once( + retry_result=None, + commit_kind="APPEND", + commit_entries=[Mock()], + changelog_entries=[], + commit_identifier=1, + latest_snapshot=Mock(id=1), + detect_conflicts=True, + ) + + self.assertIs(conflict, raised.exception.__cause__) + clear_window.assert_called_once() + + def test_retried_row_id_planning_conflict_preserves_error( + self, mock_manifest_list_manager, mock_manifest_file_manager): + file_store_commit = self._create_file_store_commit() + conflict = RowIdPlanningConflictError("row ID planning conflict") + file_store_commit.conflict_detection.has_row_id_check_from_snapshot = ( + Mock(return_value=True)) + file_store_commit.conflict_detection.read_row_id_base_entries = Mock( + side_effect=conflict) + clear_window = Mock() + file_store_commit.conflict_detection.clear_row_id_window_changes = ( + clear_window) + + with self.assertRaises(RuntimeError) as raised: + file_store_commit._try_commit_once( + retry_result=RetryResult(Mock(id=0), outcome_unknown=True), + commit_kind="APPEND", + commit_entries=[Mock()], + changelog_entries=[], + commit_identifier=1, + latest_snapshot=Mock(id=1), + detect_conflicts=True, + ) + + self.assertIs(conflict, raised.exception) + self.assertNotIsInstance(raised.exception, CommitConflictError) + clear_window.assert_called_once() + + def test_false_retry_row_id_planning_conflict_is_safe_to_abort( + self, mock_manifest_list_manager, mock_manifest_file_manager): + file_store_commit = self._create_file_store_commit() + conflict = RowIdPlanningConflictError("row ID planning conflict") + file_store_commit.conflict_detection.has_row_id_check_from_snapshot = ( + Mock(return_value=True)) + file_store_commit.conflict_detection.read_row_id_base_entries = Mock( + side_effect=conflict) + + with self.assertRaises(CommitConflictError) as raised: + file_store_commit._try_commit_once( + retry_result=RetryResult(Mock(id=0), outcome_unknown=False), + commit_kind="APPEND", + commit_entries=[Mock()], + changelog_entries=[], + commit_identifier=1, + latest_snapshot=Mock(id=1), + detect_conflicts=True, + ) + + self.assertIs(conflict, raised.exception.__cause__) + + def test_cumulative_unknown_preserves_row_id_planning_conflict( + self, mock_manifest_list_manager, mock_manifest_file_manager): + file_store_commit = self._create_file_store_commit() + conflict = RowIdPlanningConflictError("row ID planning conflict") + file_store_commit.conflict_detection.has_row_id_check_from_snapshot = ( + Mock(return_value=True)) + file_store_commit.conflict_detection.read_row_id_base_entries = Mock( + side_effect=conflict) + + with self.assertRaises(RowIdPlanningConflictError) as raised: + file_store_commit._try_commit_once( + retry_result=RetryResult(Mock(id=0), outcome_unknown=False), + commit_kind="APPEND", + commit_entries=[Mock()], + changelog_entries=[], + commit_identifier=1, + latest_snapshot=Mock(id=1), + detect_conflicts=True, + previous_outcome_unknown=True, + ) + + self.assertIs(conflict, raised.exception) + + def test_false_retry_generic_conflict_is_safe_to_abort( + self, mock_manifest_list_manager, mock_manifest_file_manager): + file_store_commit = self._create_file_store_commit() + conflict = RuntimeError("commit conflict") + file_store_commit.conflict_detection.has_row_id_check_from_snapshot = ( + Mock(return_value=False)) + file_store_commit.commit_scanner.read_incremental_changes = Mock( + return_value=[]) + file_store_commit.conflict_detection.check_conflicts = Mock( + return_value=conflict) + + with self.assertRaises(CommitConflictError) as raised: + file_store_commit._try_commit_once( + retry_result=RetryResult( + Mock(id=0), base_data_files=[], outcome_unknown=False), + commit_kind="APPEND", + commit_entries=[Mock()], + changelog_entries=[], + commit_identifier=1, + latest_snapshot=Mock(id=1), + detect_conflicts=True, + ) + + self.assertIs(conflict, raised.exception.__cause__) + + def test_false_retry_hash_conflict_is_safe_to_abort( + self, mock_manifest_list_manager, mock_manifest_file_manager): + file_store_commit = self._create_file_store_commit() + + with self.assertRaises(CommitConflictError) as raised: + file_store_commit._try_commit_once( + retry_result=RetryResult(Mock(id=0), outcome_unknown=False), + commit_kind="APPEND", + commit_entries=[Mock()], + changelog_entries=[], + commit_identifier=1, + latest_snapshot=Mock(id=1), + hash_index_base_snapshot=0, + ) + + self.assertIsInstance(raised.exception.__cause__, RuntimeError) + + def test_row_id_planning_io_error_preserves_error( + self, mock_manifest_list_manager, mock_manifest_file_manager): + file_store_commit = self._create_file_store_commit() + error = OSError("row ID planning failed") + file_store_commit.conflict_detection.has_row_id_check_from_snapshot = ( + Mock(return_value=True)) + file_store_commit.conflict_detection.read_row_id_base_entries = Mock( + side_effect=error) + clear_window = Mock() + file_store_commit.conflict_detection.clear_row_id_window_changes = ( + clear_window) + + with self.assertRaises(OSError) as raised: + file_store_commit._try_commit_once( + retry_result=None, + commit_kind="APPEND", + commit_entries=[Mock()], + changelog_entries=[], + commit_identifier=1, + latest_snapshot=Mock(id=1), + detect_conflicts=True, + ) + + self.assertIs(error, raised.exception) + clear_window.assert_called_once() + + def test_mixed_messages_keep_row_id_base_evidence( + self, mock_manifest_list_manager, mock_manifest_file_manager): + file_store_commit = self._create_file_store_commit() + self.mock_table.partition_keys_fields = [] + self.mock_table.total_buckets = 1 + identity = (7, "user", 1, "APPEND", 1, "base", "delta", None, None) + base_file = Mock( + file_name="base.parquet", + level=0, + extra_files=[], + embedded_index=None, + external_path=None, + ) + complete = CommitMessage( + partition=(), + bucket=0, + new_files=[Mock()], + check_from_snapshot=7, + row_id_base_files=[base_file], + row_id_base_snapshot_identity=identity, + ) + legacy = CommitMessage( + partition=(), bucket=0, new_files=[Mock()]) + + entries, snapshot = file_store_commit._collect_row_id_base_entries( + [complete, legacy]) + + self.assertEqual(identity, snapshot) + self.assertEqual([base_file], [entry.file for entry in entries]) + + def test_incomplete_row_id_base_evidence_falls_back( + self, mock_manifest_list_manager, mock_manifest_file_manager): + file_store_commit = self._create_file_store_commit() + message = CommitMessage( + partition=(), bucket=0, new_files=[Mock()], + check_from_snapshot=7) + + self.assertEqual( + (None, None), + file_store_commit._collect_row_id_base_entries([message]), + ) + + def test_abort_does_not_delete_row_id_base_files( + self, mock_manifest_list_manager, mock_manifest_file_manager): + file_store_commit = self._create_file_store_commit() + new_file = Mock(external_path="/new", file_path=None) + base_file = Mock(external_path="/base", file_path=None) + message = CommitMessage( + partition=(), + bucket=0, + new_files=[new_file], + row_id_base_files=[base_file], + ) + + file_store_commit.abort([message]) + + self.mock_table.file_io.delete_quietly.assert_called_once_with("/new") + def test_null_partition_value( self, mock_manifest_list_manager, mock_manifest_file_manager): from pypaimon.data.timestamp import Timestamp diff --git a/paimon-python/pypaimon/tests/overwrite_commit_conflict_test.py b/paimon-python/pypaimon/tests/overwrite_commit_conflict_test.py index 738d455b8513..bb12169f4915 100644 --- a/paimon-python/pypaimon/tests/overwrite_commit_conflict_test.py +++ b/paimon-python/pypaimon/tests/overwrite_commit_conflict_test.py @@ -271,15 +271,15 @@ def patched_cas(snapshot, statistics): self.assertIn(None, incr_results) # incremental bailed on missing self.assertEqual(full_scans['n'], 2) # first attempt + fallback - def test_incremental_merge_across_non_append_snapshot(self): + def test_retry_base_matches_full_scan_across_overwrite_snapshot(self): self._assert_merge_equals_full_scan(self._overwrite_target) def test_incremental_merge_across_compact_snapshot(self): self._assert_merge_equals_full_scan(self._compact_target) def _assert_merge_equals_full_scan(self, concurrent_fn): - # A non-APPEND snapshot (OVERWRITE/COMPACT, delta = ADD+DELETE) lands - # between retries; the merged base must still equal a fresh full scan. + # A non-APPEND snapshot lands between retries. OVERWRITE falls back to + # a full scan; COMPACT can still merge deltas. Either base must be fresh. K = 2 wb = self.table.new_batch_write_builder().overwrite({'f0': 1}) diff --git a/paimon-python/pypaimon/tests/partition_predicate_test.py b/paimon-python/pypaimon/tests/partition_predicate_test.py index c362def4aa55..6c4d85ff60a1 100644 --- a/paimon-python/pypaimon/tests/partition_predicate_test.py +++ b/paimon-python/pypaimon/tests/partition_predicate_test.py @@ -29,6 +29,7 @@ from pypaimon.schema.data_types import AtomicType, DataField from pypaimon.table.row.generic_row import GenericRow from pypaimon.table.row.offset_row import OffsetRow +from pypaimon.utils.range import Range from pypaimon.write.commit.commit_scanner import CommitScanner from pypaimon.write.commit_message import CommitMessage from pypaimon.write.file_store_commit import FileStoreCommit @@ -419,6 +420,84 @@ def test_filter_none_without_partition_keys(self): [_manifest_entry(['2024-01-15', 'us-east-1'])]) self.assertIsNone(pred) + def test_conflict_scope_manifest_pruning(self): + partition = GenericRow( + ['2024-01-15', 'us-east-1'], PARTITION_FIELDS) + + def data_entry(file_name): + file = Mock(file_name=file_name) + file.row_id_range.return_value = Range(10, 19) + return ManifestEntry(0, partition, 0, 1, file) + + normal = CommitScanner.conflict_entry_scope([data_entry('data.parquet')]) + blob = CommitScanner.conflict_entry_scope([data_entry('delta.blob')]) + dv_file = IndexFileMeta( + IndexManifestFile.DELETION_VECTORS_INDEX, + 'dv-index', 1, 1, dv_ranges={'data.parquet': Mock()}, + ) + dv = CommitScanner.conflict_entry_scope([], [ + IndexManifestEntry(0, partition, 0, dv_file), + ]) + global_file = IndexFileMeta( + 'btree', 'global-index', 1, 1, + global_index_meta=Mock(row_range_start=10, row_range_end=19), + ) + global_index = CommitScanner.conflict_entry_scope([], [ + IndexManifestEntry(0, partition, 0, global_file), + ]) + + self.assertTrue(normal.can_prune_manifest_files()) + self.assertTrue(blob.can_prune_manifest_files()) + self.assertFalse(dv.can_prune_manifest_files()) + self.assertTrue(global_index.can_prune_manifest_files()) + + @patch('pypaimon.write.commit.commit_scanner.ManifestFileManager') + def test_conflict_entries_apply_partition_and_overlapping_range(self, mock_mfm_cls): + target_partition = GenericRow( + ['2024-01-15', 'us-east-1'], PARTITION_FIELDS) + other_partition = GenericRow( + ['2024-01-16', 'us-west-2'], PARTITION_FIELDS) + + def entry(partition, first_row_id, row_count=10): + file = Mock(file_name='f-{}-{}.parquet'.format( + first_row_id, row_count)) + file.row_id_range.return_value = Range( + first_row_id, first_row_id + row_count - 1) + return ManifestEntry(0, partition, 0, 1, file) + + target = entry(target_partition, 10) + widened = entry(target_partition, 5, 20) + split_left = entry(target_partition, 10, 5) + split_right = entry(target_partition, 15, 5) + same_range_other_partition = entry(other_partition, 10) + unrelated = entry(target_partition, 100) + + def filter_entries(_manifests, manifest_entry_filter=None, **kwargs): + partition_filter = kwargs.get('partition_filter') + return [ + candidate + for candidate in [ + target, widened, split_left, split_right, + same_range_other_partition, unrelated] + if (partition_filter is None + or partition_filter.test(candidate.partition)) + and (manifest_entry_filter is None + or manifest_entry_filter(candidate)) + ] + + mock_mfm_cls.return_value.read_entries_parallel.side_effect = filter_entries + scanner = self._scanner() + scanner.manifest_list_manager.read_all.return_value = [ + Mock(min_row_id=None, max_row_id=None)] + window = entry(target_partition, 10) + + result = scanner.read_conflict_entries(Mock(), [window]) + + self.assertEqual([target, widened, split_left, split_right], result) + kwargs = mock_mfm_cls.return_value.read_entries_parallel.call_args[1] + self.assertIsNotNone(kwargs['early_record_filter']) + self.assertIsNotNone(kwargs['partition_filter']) + @patch('pypaimon.write.commit.commit_scanner.FileScanner') def test_passes_partition_predicate_to_file_scanner(self, mock_scanner_cls): mock_scanner_cls.return_value.read_manifest_entries.return_value = [] @@ -474,3 +553,61 @@ def test_raw_entries_filter_unmatched_partition(self, mock_mfm_cls): self.assertEqual(len(result), 1) self.assertEqual(tuple(result[0].partition.values), ('p1', 'us')) + + @patch('pypaimon.write.commit.commit_scanner.ManifestFileManager') + def test_scoped_raw_entries_include_overlapping_ranges(self, mock_mfm_cls): + partition = GenericRow( + ['2024-01-15', 'us-east-1'], PARTITION_FIELDS) + + def entry(kind, first_row_id, row_count): + file = Mock(file_name='f-{}.parquet'.format(first_row_id)) + file.row_id_range.return_value = Range( + first_row_id, first_row_id + row_count - 1) + return ManifestEntry(kind, partition, 0, 1, file) + + overlap = entry(1, 0, 20) + unrelated = entry(0, 100, 10) + + def read(_name, manifest_entry_filter=None, **_kwargs): + return [ + candidate for candidate in [overlap, unrelated] + if manifest_entry_filter(candidate) + ] + + mock_mfm_cls.return_value.read.side_effect = read + scanner = self._scanner() + scanner.manifest_list_manager.read_delta.return_value = [ + Mock(file_name='m1', min_row_id=None, max_row_id=None)] + window = entry(0, 10, 10) + + result = scanner.read_incremental_raw_entries_for_scope( + Mock(), [window]) + + self.assertEqual([overlap], result) + kwargs = mock_mfm_cls.return_value.read.call_args[1] + self.assertIsNotNone(kwargs['early_record_filter']) + self.assertIsNotNone(kwargs['partition_filter']) + + def test_incremental_changes_falls_back_on_overwrite(self): + append = Mock(id=2, commit_kind='APPEND') + overwrite = Mock(id=3, commit_kind='OVERWRITE') + after_overwrite = Mock(id=4, commit_kind='APPEND') + snapshots = {2: append, 3: overwrite, 4: after_overwrite} + + table = _mock_table() + snapshot_manager = table.snapshot_manager.return_value + snapshot_manager.get_snapshot_by_id.side_effect = snapshots.get + scanner = CommitScanner(table, Mock()) + scanner.read_incremental_raw_entries_from_changed_partitions = Mock( + return_value=[]) + + result = scanner.read_incremental_changes( + Mock(id=1), after_overwrite, []) + + self.assertIsNone(result) + self.assertEqual( + [call[0][0] + for call in snapshot_manager.get_snapshot_by_id.call_args_list], + [2, 3], + ) + scanner.read_incremental_raw_entries_from_changed_partitions.assert_called_once() diff --git a/paimon-python/pypaimon/tests/ray_update_by_row_id_test.py b/paimon-python/pypaimon/tests/ray_update_by_row_id_test.py index 613fa50d0319..6a3559136555 100644 --- a/paimon-python/pypaimon/tests/ray_update_by_row_id_test.py +++ b/paimon-python/pypaimon/tests/ray_update_by_row_id_test.py @@ -18,6 +18,7 @@ import os import shutil import tempfile +import threading import types import unittest import uuid @@ -130,6 +131,907 @@ def test_updates_correct_row_across_files(self): self.assertEqual(got[21], 999) self.assertTrue(all(v == 0 for k, v in got.items() if k != 21)) + def test_incrementally_commits_file_group_windows(self): + from pypaimon.write.commit.commit_scanner import CommitScanner + from pypaimon.write.commit.conflict_detection import ConflictDetection + + target = self._create() + chunks = [ + [group_start, group_start + 1] + for group_start in range(10, 90, 10) + ] + for chunk in chunks: + self._write(target, pa.Table.from_pydict( + {"id": chunk, "name": ["x"] * len(chunk), "age": [0] * len(chunk)}, + schema=self.pa_schema, + )) + + table = self.catalog.get_table(target) + base_snapshot_id = table.snapshot_manager().get_latest_snapshot().id + rid = self._rowid_by_id(target) + updated_ids = [chunk[0] for chunk in chunks] + src = pa.table( + { + "_ROW_ID": [rid[row_id] for row_id in updated_ids], + "age": updated_ids, + }, + schema=pa.schema([("_ROW_ID", pa.int64()), ("age", pa.int32())]), + ) + + original_iter_batches = ray.data.Dataset.iter_batches + iter_batch_options = [] + first_window_committed = threading.Event() + resume_iteration = threading.Event() + observed = {} + scan_counts = {"fallback": 0, "incremental": 0} + query_sizes = [] + original_fallback_scan = CommitScanner.read_conflict_entries + original_incremental_scan = ( + CommitScanner.read_incremental_raw_entries_for_scope + ) + original_base_read = ConflictDetection.read_row_id_base_entries + + def counting_fallback_scan(scanner, *args, **kwargs): + scan_counts["fallback"] += 1 + return original_fallback_scan(scanner, *args, **kwargs) + + def counting_incremental_scan(scanner, *args, **kwargs): + scan_counts["incremental"] += 1 + return original_incremental_scan(scanner, *args, **kwargs) + + def counting_base_read(detection, *args, **kwargs): + result = original_base_read(detection, *args, **kwargs) + query_sizes.append(len(result)) + return result + + def pausing_iter_batches(dataset, *args, **kwargs): + iter_batch_options.append(( + kwargs.get("batch_size"), + kwargs.get("prefetch_batches"), + )) + for batch_number, batch in enumerate( + original_iter_batches(dataset, *args, **kwargs), 1): + yield batch + if batch_number == 3: + first_window_committed.set() + if not resume_iteration.wait(30): + raise TimeoutError("timed out waiting to resume iteration") + + def observe_first_window(): + if not first_window_committed.wait(30): + observed["error"] = "first commit was not observed" + else: + observed["snapshot_id"] = ( + table.snapshot_manager().get_latest_snapshot().id + ) + resume_iteration.set() + + observer = threading.Thread(target=observe_first_window) + observer.start() + + try: + with mock.patch.object( + ray.data.Dataset, "iter_batches", pausing_iter_batches): + with mock.patch.object( + CommitScanner, + "read_conflict_entries", + counting_fallback_scan, + ), mock.patch.object( + CommitScanner, + "read_incremental_raw_entries_for_scope", + counting_incremental_scan, + ), mock.patch.object( + ConflictDetection, + "read_row_id_base_entries", + counting_base_read, + ): + stats = update_by_row_id( + target, + ray.data.from_arrow(src).repartition(8), + self.catalog_options, + update_cols=["age"], + num_partitions=4, + max_groups_per_commit=3, + ) + finally: + resume_iteration.set() + observer.join(30) + + self.assertEqual({"num_updated": 8}, stats) + self.assertEqual([(1, 0)], iter_batch_options) + self.assertNotIn("error", observed) + self.assertEqual(base_snapshot_id + 1, observed["snapshot_id"]) + self.assertEqual({"fallback": 0, "incremental": 0}, scan_counts) + self.assertEqual([2, 3, 3], sorted(query_sizes)) + latest = table.snapshot_manager().get_latest_snapshot() + self.assertEqual(base_snapshot_id + 3, latest.id) + incremental_snapshots = [ + table.snapshot_manager().get_snapshot_by_id(base_snapshot_id + offset) + for offset in range(1, 4) + ] + self.assertEqual(1, len({ + snapshot.commit_user for snapshot in incremental_snapshots + })) + self.assertEqual( + [1, 2, 3], + [snapshot.commit_identifier for snapshot in incremental_snapshots], + ) + + back = self._read(target).sort_by("id").to_pydict() + got = dict(zip(back["id"], back["age"])) + self.assertEqual( + {row_id: row_id for row_id in updated_ids}, + {row_id: got[row_id] for row_id in updated_ids}, + ) + for chunk in chunks: + self.assertEqual(0, got[chunk[1]]) + + def test_incremental_commit_failure_drains_later_groups(self): + import importlib + from pypaimon.write.file_store_commit import CommitOutcomeUnknownError + from pypaimon.write.table_commit import StreamTableCommit + + m = importlib.import_module("pypaimon.ray.update_by_row_id") + target = self._create() + chunks = [[start, start + 1] for start in range(10, 50, 10)] + for chunk in chunks: + self._write(target, pa.Table.from_pydict( + {"id": chunk, "name": ["x", "x"], "age": [0, 0]}, + schema=self.pa_schema, + )) + + rid = self._rowid_by_id(target) + src = pa.table( + { + "_ROW_ID": [rid[chunk[0]] for chunk in chunks], + "age": [chunk[0] for chunk in chunks], + }, + schema=pa.schema([("_ROW_ID", pa.int64()), ("age", pa.int32())]), + ) + seen_groups = [] + aborted = [] + commit_calls = [] + retry_error = ValueError("commit retry failed") + commit_error = CommitOutcomeUnknownError("commit failed") + original_add_group = m._IncrementalUpdateCommitter.add_group + original_abort = StreamTableCommit.abort + + def record_group(committer, messages, num_updated, row_ids): + seen_groups.append(list(messages)) + return original_add_group( + committer, messages, num_updated, row_ids) + + def fail_commit(_commit, _messages, commit_identifier): + commit_calls.append(commit_identifier) + raise commit_error from retry_error + + def record_abort(commit, messages): + aborted.append(list(messages)) + return original_abort(commit, messages) + + with mock.patch.object( + m._IncrementalUpdateCommitter, "add_group", record_group), \ + mock.patch.object(StreamTableCommit, "commit", fail_commit), \ + mock.patch.object(StreamTableCommit, "abort", record_abort): + with self.assertRaises(CommitOutcomeUnknownError) as raised: + update_by_row_id( + target, + ray.data.from_arrow(src).repartition(4), + self.catalog_options, + update_cols=["age"], + num_partitions=4, + max_groups_per_commit=2, + ) + + self.assertIs(commit_error, raised.exception) + self.assertIs(retry_error, raised.exception.__cause__) + self.assertEqual(4, len(seen_groups)) + self.assertEqual([1], commit_calls) + self.assertEqual( + [list(group) for group in seen_groups[2:]], + aborted, + ) + + def test_incremental_commit_error_precedes_later_worker_failure(self): + import importlib + + from ray.exceptions import RayTaskError + + from pypaimon.write.file_store_commit import CommitOutcomeUnknownError + from pypaimon.write.table_commit import StreamTableCommit + + m = importlib.import_module("pypaimon.ray.update_by_row_id") + target = self._create() + self._write(target, pa.Table.from_pydict( + {"id": [1], "name": ["x"], "age": [0]}, + schema=self.pa_schema, + )) + src = pa.table( + {"_ROW_ID": [0], "age": [1]}, + schema=pa.schema([("_ROW_ID", pa.int64()), ("age", pa.int32())]), + ) + commit_error = CommitOutcomeUnknownError("commit outcome is uncertain") + worker_error = ValueError("worker failed") + aborted = [] + close_calls = [] + + def fail_commit(_commit, _messages, _commit_identifier): + raise commit_error + + def record_abort(_commit, messages): + aborted.append(list(messages)) + + def record_close(_commit): + close_calls.append(True) + + def fail_after_commit(_update_ds, _table, _update_cols, **kwargs): + on_group_result = kwargs["on_group_result"] + on_group_result(["uncertain-1"], 1, []) + on_group_result(["uncertain-2"], 1, []) + on_group_result(["pending"], 1, []) + raise RayTaskError("worker", "trace", worker_error) + + with mock.patch.object( + m, "distributed_update_apply", fail_after_commit), \ + mock.patch.object(StreamTableCommit, "commit", fail_commit), \ + mock.patch.object(StreamTableCommit, "abort", record_abort), \ + mock.patch.object(StreamTableCommit, "close", record_close): + with self.assertRaises(CommitOutcomeUnknownError) as raised: + update_by_row_id( + target, + src, + self.catalog_options, + update_cols=["age"], + max_groups_per_commit=2, + ) + + self.assertIs(commit_error, raised.exception) + self.assertIs(worker_error, raised.exception.__cause__) + self.assertEqual([["pending"]], aborted) + self.assertEqual([True], close_calls) + + def test_incremental_commit_preserves_cause_before_worker_failure(self): + import importlib + + from ray.exceptions import RayTaskError + + from pypaimon.write.file_store_commit import CommitOutcomeUnknownError + from pypaimon.write.table_commit import StreamTableCommit + + m = importlib.import_module("pypaimon.ray.update_by_row_id") + target = self._create() + self._write(target, pa.Table.from_pydict( + {"id": [1], "name": ["x"], "age": [0]}, + schema=self.pa_schema, + )) + src = pa.table( + {"_ROW_ID": [0], "age": [1]}, + schema=pa.schema([("_ROW_ID", pa.int64()), ("age", pa.int32())]), + ) + commit_cause = ValueError("commit retry failed") + commit_error = CommitOutcomeUnknownError("commit outcome is uncertain") + worker_error = ValueError("worker failed") + aborted = [] + close_calls = [] + + def fail_commit(_commit, _messages, _commit_identifier): + raise commit_error from commit_cause + + def record_abort(_commit, messages): + aborted.append(list(messages)) + + def record_close(_commit): + close_calls.append(True) + + def fail_after_commit(_update_ds, _table, _update_cols, **kwargs): + on_group_result = kwargs["on_group_result"] + on_group_result(["uncertain-1"], 1, []) + on_group_result(["uncertain-2"], 1, []) + on_group_result(["pending"], 1, []) + raise RayTaskError("worker", "trace", worker_error) + + with mock.patch.object( + m, "distributed_update_apply", fail_after_commit), \ + mock.patch.object(StreamTableCommit, "commit", fail_commit), \ + mock.patch.object(StreamTableCommit, "abort", record_abort), \ + mock.patch.object(StreamTableCommit, "close", record_close), \ + self.assertLogs( + "pypaimon.ray.update_by_row_id", level="WARNING") as logs: + with self.assertRaises(CommitOutcomeUnknownError) as raised: + update_by_row_id( + target, + src, + self.catalog_options, + update_cols=["age"], + max_groups_per_commit=2, + ) + + self.assertIs(commit_error, raised.exception) + self.assertIs(commit_cause, raised.exception.__cause__) + self.assertIs(worker_error, raised.exception.__context__) + self.assertIn( + "preserving the original commit error: worker failed", + "\n".join(logs.output), + ) + self.assertEqual([["pending"]], aborted) + self.assertEqual([True], close_calls) + + def test_incremental_conflict_aborts_output_files(self): + from pypaimon.snapshot.snapshot import BATCH_COMMIT_IDENTIFIER + from pypaimon.write.table_commit import StreamTableCommit + from pypaimon.write.table_update_by_row_id import TableUpdateByRowId + + target = self._create() + self._write(target, pa.Table.from_pydict( + {"id": [1], "name": ["a"], "age": [0]}, + schema=self.pa_schema, + )) + table = self.catalog.get_table(target) + row_id = self._rowid_by_id(target)[1] + update_schema = pa.schema([ + ("_ROW_ID", pa.int64()), + ("age", pa.int32()), + ]) + source = pa.table( + {"_ROW_ID": [row_id], "age": [300]}, + schema=update_schema, + ) + output_paths = [] + original_commit = StreamTableCommit.commit + + def commit_after_concurrent_update( + stream_commit, messages, commit_identifier): + output_paths.extend( + file.file_path + for message in messages + for file in message.new_files + ) + updater = TableUpdateByRowId( + table, "concurrent", BATCH_COMMIT_IDENTIFIER) + concurrent_messages = updater.update_columns( + pa.table( + {"_ROW_ID": [row_id], "age": [999]}, + schema=update_schema, + ), + ["age"], + ) + concurrent_commit = table.new_batch_write_builder().new_commit() + try: + concurrent_commit.commit(concurrent_messages) + finally: + concurrent_commit.close() + return original_commit( + stream_commit, messages, commit_identifier) + + with mock.patch.object( + StreamTableCommit, "commit", commit_after_concurrent_update): + with self.assertRaisesRegex( + RuntimeError, "Concurrent commit detected"): + update_by_row_id( + target, + ray.data.from_arrow(source), + self.catalog_options, + update_cols=["age"], + max_groups_per_commit=1, + ) + + self.assertTrue(output_paths) + self.assertTrue(all( + not table.file_io.exists(path) for path in output_paths + )) + self.assertEqual([999], self._read(target)["age"].to_pylist()) + + def test_rollback_reused_snapshot_id_detects_conflict(self): + from pypaimon.snapshot.snapshot import BATCH_COMMIT_IDENTIFIER + from pypaimon.write.table_commit import StreamTableCommit + from pypaimon.write.table_update_by_row_id import TableUpdateByRowId + + target = self._create() + for row_id in range(1, 4): + self._write(target, pa.Table.from_pydict( + {"id": [row_id], "name": ["a"], "age": [0]}, + schema=self.pa_schema, + )) + table = self.catalog.get_table(target) + base_snapshot_id = table.snapshot_manager().get_latest_snapshot().id + row_ids = self._rowid_by_id(target) + update_schema = pa.schema([ + ("_ROW_ID", pa.int64()), + ("age", pa.int32()), + ]) + source = pa.table( + { + "_ROW_ID": [row_ids[row_id] for row_id in range(1, 4)], + "age": [100, 200, 300], + }, + schema=update_schema, + ) + original_commit = StreamTableCommit.commit + commit_calls = [] + + def commit_then_rebuild_history( + stream_commit, messages, commit_identifier): + commit_calls.append(commit_identifier) + result = original_commit( + stream_commit, messages, commit_identifier) + if commit_identifier == 2: + table.rollback_to(base_snapshot_id) + updater = TableUpdateByRowId( + table, "concurrent", BATCH_COMMIT_IDENTIFIER) + concurrent_messages = updater.update_columns( + pa.table( + { + "_ROW_ID": [ + row_ids[row_id] for row_id in range(1, 4) + ], + "age": [999, 999, 999], + }, + schema=update_schema, + ), + ["age"], + ) + concurrent_commit = ( + table.new_batch_write_builder().new_commit() + ) + try: + concurrent_commit.commit(concurrent_messages) + finally: + concurrent_commit.close() + return result + + with mock.patch.object( + StreamTableCommit, "commit", commit_then_rebuild_history): + with self.assertRaisesRegex( + RuntimeError, "no longer in the current lineage"): + update_by_row_id( + target, + ray.data.from_arrow(source).repartition(3), + self.catalog_options, + update_cols=["age"], + num_partitions=3, + max_groups_per_commit=1, + ) + + self.assertEqual([1, 2], commit_calls) + self.assertEqual( + [999, 999, 999], + self._read(target).sort_by("id")["age"].to_pylist(), + ) + + def test_incremental_commit_fails_after_external_rollback(self): + target = self._create() + for row_id in range(1, 3): + self._write(target, pa.Table.from_pydict( + {"id": [row_id], "name": ["a"], "age": [0]}, + schema=self.pa_schema, + )) + table = self.catalog.get_table(target) + base_snapshot_id = table.snapshot_manager().get_latest_snapshot().id + row_ids = self._rowid_by_id(target) + source = pa.table( + { + "_ROW_ID": [row_ids[1], row_ids[2]], + "age": [100, 200], + }, + schema=pa.schema([ + ("_ROW_ID", pa.int64()), + ("age", pa.int32()), + ]), + ) + + original_iter_batches = ray.data.Dataset.iter_batches + first_window_committed = threading.Event() + resume_iteration = threading.Event() + rollback_error = [] + + def pausing_iter_batches(dataset, *args, **kwargs): + for batch_number, batch in enumerate( + original_iter_batches(dataset, *args, **kwargs), 1): + yield batch + if batch_number == 1: + first_window_committed.set() + if not resume_iteration.wait(30): + raise TimeoutError("timed out waiting for rollback") + + def rollback_first_window(): + try: + if not first_window_committed.wait(30): + raise TimeoutError("first commit was not observed") + table.rollback_to(base_snapshot_id) + except Exception as error: + rollback_error.append(error) + finally: + resume_iteration.set() + + rollback_thread = threading.Thread(target=rollback_first_window) + rollback_thread.start() + try: + with mock.patch.object( + ray.data.Dataset, "iter_batches", pausing_iter_batches): + with self.assertRaisesRegex( + RuntimeError, "no longer in the current lineage"): + update_by_row_id( + target, + ray.data.from_arrow(source).repartition(2), + self.catalog_options, + update_cols=["age"], + num_partitions=2, + max_groups_per_commit=1, + ) + finally: + resume_iteration.set() + rollback_thread.join(30) + + self.assertEqual([], rollback_error) + self.assertFalse(rollback_thread.is_alive()) + self.assertEqual( + base_snapshot_id, + table.snapshot_manager().get_latest_snapshot().id, + ) + self.assertEqual( + [0, 0], + self._read(target).sort_by("id")["age"].to_pylist(), + ) + + def test_incremental_committer_batches_complete_groups(self): + import importlib + m = importlib.import_module("pypaimon.ray.update_by_row_id") + recorder = { + "commits": [], + "ignored": [], + "aborts": [], + "close_calls": 0, + "bounded": 0, + } + + class FakeCommit: + def enable_bounded_row_id_conflict_state(self): + recorder["bounded"] += 1 + + def commit(self, msgs, commit_identifier): + recorder["commits"].append((list(msgs), commit_identifier)) + + def abort(self, msgs): + recorder["aborts"].append(list(msgs)) + + def ignore_row_id_conflict_for_commit(self, commit_identifier): + recorder["ignored"].append(commit_identifier) + + def close(self): + recorder["close_calls"] += 1 + + class FakeBuilder: + def new_commit(self): + return FakeCommit() + + class FakeTable: + def new_stream_write_builder(self): + return FakeBuilder() + + committer = m._IncrementalUpdateCommitter(FakeTable(), 2) + committer.add_group(["group-1"], 1, []) + self.assertEqual([], recorder["commits"]) + committer.add_group(["group-2"], 1, []) + committer.add_group(["group-3"], 1, []) + committer.finish() + committer.close() + + self.assertEqual([ + (["group-1", "group-2"], 1), + (["group-3"], 2), + ], recorder["commits"]) + self.assertEqual([1, 2], recorder["ignored"]) + self.assertEqual([], recorder["aborts"]) + self.assertEqual(1, recorder["close_calls"]) + self.assertEqual(1, recorder["bounded"]) + + def test_incremental_committer_aborts_only_uncommitted_groups(self): + import importlib + m = importlib.import_module("pypaimon.ray.update_by_row_id") + recorder = {"commits": [], "ignored": [], "aborts": []} + + class FakeCommit: + def enable_bounded_row_id_conflict_state(self): + pass + + def commit(self, msgs, commit_identifier): + recorder["commits"].append((list(msgs), commit_identifier)) + + def abort(self, msgs): + recorder["aborts"].append(list(msgs)) + + def ignore_row_id_conflict_for_commit(self, commit_identifier): + recorder["ignored"].append(commit_identifier) + + def close(self): + pass + + class FakeBuilder: + def new_commit(self): + return FakeCommit() + + class FakeTable: + def new_stream_write_builder(self): + return FakeBuilder() + + committer = m._IncrementalUpdateCommitter(FakeTable(), 2) + committer.add_group(["committed-1"], 1, []) + committer.add_group(["committed-2"], 1, []) + committer.add_group(["pending"], 1, []) + committer.abort_pending() + committer.close() + + self.assertEqual([ + (["committed-1", "committed-2"], 1), + ], recorder["commits"]) + self.assertEqual([1], recorder["ignored"]) + self.assertEqual([["pending"]], recorder["aborts"]) + + def test_incremental_committer_drains_after_uncertain_commit(self): + import importlib + from pypaimon.write.file_store_commit import CommitOutcomeUnknownError + + m = importlib.import_module("pypaimon.ray.update_by_row_id") + recorder = {"commit_calls": 0, "ignored": [], "aborts": []} + + class FakeCommit: + def enable_bounded_row_id_conflict_state(self): + pass + + def commit(self, msgs, commit_identifier): + recorder["commit_calls"] += 1 + raise CommitOutcomeUnknownError( + "commit outcome is uncertain") + + def abort(self, msgs): + recorder["aborts"].append(list(msgs)) + + def ignore_row_id_conflict_for_commit(self, commit_identifier): + recorder["ignored"].append(commit_identifier) + + def close(self): + pass + + class FakeBuilder: + def new_commit(self): + return FakeCommit() + + class FakeTable: + def new_stream_write_builder(self): + return FakeBuilder() + + committer = m._IncrementalUpdateCommitter(FakeTable(), 2) + committer.add_group(["uncertain-1"], 1, []) + committer.add_group(["uncertain-2"], 1, []) + self.assertEqual([], committer._pending_messages) + self.assertEqual( + ["uncertain-1", "uncertain-2"], + committer._uncertain_messages, + ) + for group in ("pending-1", "pending-2"): + committer.add_group([group], 1, []) + self.assertEqual([], committer._pending_messages) + self.assertEqual(2, len(committer._uncertain_messages)) + with self.assertRaisesRegex(RuntimeError, "outcome is uncertain"): + committer.finish() + committer.abort_pending() + committer.close() + + self.assertEqual(1, recorder["commit_calls"]) + self.assertEqual([], recorder["ignored"]) + self.assertEqual( + [["pending-1"], ["pending-2"]], + recorder["aborts"], + ) + + def test_incremental_committer_aborts_deterministic_failure(self): + import importlib + + m = importlib.import_module("pypaimon.ray.update_by_row_id") + recorder = {"commit_calls": 0, "aborts": []} + + class FakeCommit: + def enable_bounded_row_id_conflict_state(self): + pass + + def commit(self, msgs, commit_identifier): + recorder["commit_calls"] += 1 + raise RuntimeError("conflict") + + def abort(self, msgs): + recorder["aborts"].append(list(msgs)) + + def close(self): + pass + + class FakeBuilder: + def new_commit(self): + return FakeCommit() + + class FakeTable: + def new_stream_write_builder(self): + return FakeBuilder() + + committer = m._IncrementalUpdateCommitter(FakeTable(), 2) + for group in ("failed-1", "failed-2"): + committer.add_group([group], 1, []) + self.assertEqual( + ["failed-1", "failed-2"], committer._pending_messages) + for group in ("pending-1", "pending-2"): + committer.add_group([group], 1, []) + self.assertEqual( + ["failed-1", "failed-2"], committer._pending_messages) + with self.assertRaisesRegex(RuntimeError, "conflict"): + committer.finish() + committer.abort_pending() + committer.close() + + self.assertEqual(1, recorder["commit_calls"]) + self.assertEqual([ + ["pending-1"], + ["pending-2"], + ["failed-1", "failed-2"], + ], recorder["aborts"]) + + def test_incremental_committer_releases_groups_while_draining(self): + import gc + import importlib + import weakref + + from pypaimon.write.file_store_commit import CommitOutcomeUnknownError + + m = importlib.import_module("pypaimon.ray.update_by_row_id") + recorder = {"aborted": []} + + class Message: + def __init__(self, value): + self.value = value + + class FakeCommit: + def enable_bounded_row_id_conflict_state(self): + pass + + def commit(self, _msgs, _commit_identifier): + raise CommitOutcomeUnknownError("outcome is uncertain") + + def abort(self, msgs): + recorder["aborted"].extend(msg.value for msg in msgs) + + def close(self): + pass + + class FakeBuilder: + def new_commit(self): + return FakeCommit() + + class FakeTable: + def new_stream_write_builder(self): + return FakeBuilder() + + committer = m._IncrementalUpdateCommitter(FakeTable(), 2) + uncertain = [Message("uncertain-1"), Message("uncertain-2")] + committer.add_group([uncertain[0]], 1, []) + committer.add_group([uncertain[1]], 1, []) + + drained_refs = [] + for value in range(100): + message = Message(value) + drained_refs.append(weakref.ref(message)) + committer.add_group([message], 1, []) + del message + + gc.collect() + self.assertTrue(all(ref() is None for ref in drained_refs)) + self.assertEqual(list(range(100)), recorder["aborted"]) + self.assertEqual([], committer._pending_messages) + self.assertEqual(uncertain, committer._uncertain_messages) + + with self.assertRaisesRegex(RuntimeError, "outcome is uncertain"): + committer.finish() + committer.abort_pending() + committer.close() + + def test_incremental_committer_protects_committed_window(self): + import importlib + + m = importlib.import_module("pypaimon.ray.update_by_row_id") + recorder = {"commits": [], "aborts": []} + + class FakeCommit: + def enable_bounded_row_id_conflict_state(self): + pass + + def commit(self, msgs, commit_identifier): + recorder["commits"].append(list(msgs)) + + def abort(self, msgs): + recorder["aborts"].append(list(msgs)) + + def ignore_row_id_conflict_for_commit(self, commit_identifier): + raise RuntimeError("ignore failed") + + def close(self): + pass + + class FakeBuilder: + def new_commit(self): + return FakeCommit() + + class FakeTable: + def new_stream_write_builder(self): + return FakeBuilder() + + committer = m._IncrementalUpdateCommitter(FakeTable(), 2) + for group in ("committed-1", "committed-2", "pending-1", "pending-2"): + committer.add_group([group], 1, []) + with self.assertRaisesRegex(RuntimeError, "ignore failed"): + committer.finish() + committer.abort_pending() + committer.close() + + self.assertEqual([[ + "committed-1", "committed-2" + ]], recorder["commits"]) + self.assertEqual( + [["pending-1"], ["pending-2"]], + recorder["aborts"], + ) + + def test_incremental_committer_aborts_all_after_new_commit_failure(self): + import importlib + m = importlib.import_module("pypaimon.ray.update_by_row_id") + recorder = {"aborts": [], "close_calls": 0} + + class AbortCommit: + def abort(self, msgs): + recorder["aborts"].append(list(msgs)) + + def close(self): + recorder["close_calls"] += 1 + + class FailingBuilder: + def new_commit(self): + raise RuntimeError("new_commit failed") + + class AbortBuilder: + def new_commit(self): + return AbortCommit() + + class FakeTable: + def new_stream_write_builder(self): + return FailingBuilder() + + def new_batch_write_builder(self): + return AbortBuilder() + + committer = m._IncrementalUpdateCommitter(FakeTable(), 2) + for group in ("group-1", "group-2", "group-3"): + committer.add_group([group], 1, []) + with self.assertRaisesRegex(RuntimeError, "new_commit failed"): + committer.finish() + committer.abort_pending() + + self.assertEqual( + [["group-3"], ["group-1", "group-2"]], + recorder["aborts"], + ) + self.assertEqual(2, recorder["close_calls"]) + + def test_rejects_invalid_max_groups_per_commit(self): + target = self._create() + src = pa.table({"_ROW_ID": pa.array([], pa.int64()), + "age": pa.array([], pa.int32())}) + for value in (0, -1, True, 1.5): + with self.assertRaisesRegex( + ValueError, "max_groups_per_commit must be a positive integer"): + update_by_row_id( + target, + src, + self.catalog_options, + update_cols=["age"], + max_groups_per_commit=value, + ) + def test_pins_base_snapshot_for_conflict_detection(self): # The update pins its base snapshot and threads it to distributed_update_apply, # which uses it for commit-time conflict detection against concurrent writers. @@ -169,7 +1071,7 @@ def test_new_commit_failure_aborts_pending_messages(self): self.assertEqual(recorder["abort_calls"], 1) self.assertEqual(recorder["abort_msgs"], recorder["msgs"]) - def test_commit_failure_does_not_abort_after_commit_started(self): + def test_deterministic_commit_failure_aborts_pending_messages(self): err = RuntimeError("commit failed") recorder = {} @@ -179,10 +1081,55 @@ def test_commit_failure_does_not_abort_after_commit_started(self): commit_error=err, ) + self.assertEqual(recorder["commit_calls"], 1) + self.assertEqual(recorder["abort_calls"], 1) + self.assertEqual(recorder["abort_msgs"], recorder["msgs"]) + self.assertEqual(recorder["close_calls"], 2) + + def test_unknown_commit_failure_does_not_abort_pending_messages(self): + from pypaimon.write.file_store_commit import CommitOutcomeUnknownError + + recorder = {} + with self.assertRaises(CommitOutcomeUnknownError): + self._run_with_fake_commit( + recorder=recorder, + commit_error=CommitOutcomeUnknownError("commit failed"), + ) + self.assertEqual(recorder["commit_calls"], 1) self.assertEqual(recorder["abort_calls"], 0) self.assertEqual(recorder["close_calls"], 1) + def test_driver_commit_failure_is_not_unwrapped(self): + from pypaimon.write.file_store_commit import CommitOutcomeUnknownError + + retry_error = ValueError("retry failed") + for error_type in (RuntimeError, CommitOutcomeUnknownError): + try: + raise error_type("commit failed") from retry_error + except error_type as commit_error: + chained_error = commit_error + + with self.assertRaises(error_type) as raised: + self._run_with_fake_commit(commit_error=chained_error) + + self.assertIs(chained_error, raised.exception) + self.assertIs(retry_error, raised.exception.__cause__) + + def test_ray_task_error_is_unwrapped(self): + from ray.exceptions import RayTaskError + + from pypaimon.ray.data_evolution_merge_into import _reraise_inner + + worker_error = ValueError("worker failed") + ray_error = RayTaskError("worker", "trace", worker_error) + + with self.assertRaises(ValueError) as raised: + _reraise_inner(ray_error) + + self.assertIs(worker_error, raised.exception) + self.assertIsNone(raised.exception.__cause__) + def test_close_failure_after_success_warns_and_returns_stats(self): close_error = RuntimeError("close failed") diff --git a/paimon-python/pypaimon/tests/rest/rest_catalog_commit_snapshot_test.py b/paimon-python/pypaimon/tests/rest/rest_catalog_commit_snapshot_test.py index 975c6f643bc2..f4b809bfd1a8 100644 --- a/paimon-python/pypaimon/tests/rest/rest_catalog_commit_snapshot_test.py +++ b/paimon-python/pypaimon/tests/rest/rest_catalog_commit_snapshot_test.py @@ -25,14 +25,21 @@ from pypaimon import Schema from pypaimon.api.api_response import CommitTableResponse from pypaimon.common.options import Options -from pypaimon.api.rest_exception import NoSuchResourceException +from pypaimon.api.rest_exception import ( + ForbiddenException, + NoSuchResourceException, +) from pypaimon.catalog.catalog_context import CatalogContext -from pypaimon.catalog.catalog_exception import TableNotExistException +from pypaimon.catalog.catalog_exception import ( + TableNoPermissionException, + TableNotExistException, +) from pypaimon.catalog.rest.rest_catalog import RESTCatalog from pypaimon.common.identifier import Identifier from pypaimon.snapshot.snapshot import Snapshot from pypaimon.snapshot.snapshot_commit import PartitionStatistics from pypaimon.tests.rest.rest_base_test import RESTBaseTest +from pypaimon.write.file_store_commit import CommitOutcomeUnknownError class TestRESTCatalogCommitSnapshot(unittest.TestCase): @@ -138,6 +145,24 @@ def test_rest_catalog_commit_snapshot_table_not_exist(self): self.test_statistics ) + def test_rest_catalog_commit_snapshot_no_permission(self): + with patch('pypaimon.catalog.rest.rest_catalog.RESTApi') as mock_rest_api: + mock_api_instance = Mock() + mock_api_instance.options = self.test_options + mock_api_instance.commit_snapshot.side_effect = ForbiddenException( + "forbidden") + mock_rest_api.return_value = mock_api_instance + + catalog = RESTCatalog(self.catalog_context) + + with self.assertRaises(TableNoPermissionException): + catalog.commit_snapshot( + self.identifier, + "test-uuid", + self.test_snapshot, + self.test_statistics + ) + def test_rest_catalog_commit_snapshot_api_error(self): """Test snapshot commit with API error.""" with patch('pypaimon.catalog.rest.rest_catalog.RESTApi') as mock_rest_api: @@ -325,6 +350,53 @@ def test_multiple_row_tracking_commits_preserve_all_rows(self): self.assertEqual( sorted(actual.column('id').to_pylist()), [1, 2, 3, 4, 5, 6]) + def test_explicit_rest_commit_rejections_are_not_retried(self): + pa_schema = pa.schema([('id', pa.int32())]) + cases = [ + ( + "missing", + NoSuchResourceException("Table", "missing", "not found"), + TableNotExistException, + ), + ( + "forbidden", + ForbiddenException("forbidden"), + TableNoPermissionException, + ), + ] + + for suffix, rest_error, expected_error in cases: + with self.subTest(error=suffix): + table_name = f'default.test_deterministic_commit_{suffix}' + schema = Schema.from_pyarrow_schema( + pa_schema, + options={ + 'commit.max-retries': '3', + 'commit.min-retry-wait': '1ms', + 'commit.max-retry-wait': '1ms', + 'commit.timeout': '10s', + }, + ) + self.rest_catalog.create_table(table_name, schema, False) + table = self.rest_catalog.get_table(table_name) + table_write = table.new_batch_write_builder().new_write() + table_commit = table.new_batch_write_builder().new_commit() + table_write.write_arrow(pa.table({'id': [1]}, schema=pa_schema)) + commit_messages = table_write.prepare_commit() + + try: + with patch( + 'pypaimon.api.rest_api.RESTApi.commit_snapshot', + side_effect=rest_error) as rest_commit: + with self.assertRaises(expected_error): + table_commit.commit(commit_messages) + + rest_commit.assert_called_once() + table_commit.abort(commit_messages) + finally: + table_write.close() + table_commit.close() + def test_commit_succeeded_on_server_but_client_fails(self): pa_schema = pa.schema([('id', pa.int32()), ('name', pa.string())]) opts = { @@ -352,7 +424,7 @@ def commit_then_raise(sn, st): raise RuntimeError("simulated") with patch.object(tc.file_store_commit.snapshot_commit, 'commit', side_effect=commit_then_raise): - with self.assertRaises(RuntimeError): + with self.assertRaises(CommitOutcomeUnknownError): tc.commit(cm) tw.close() tc.close() @@ -364,6 +436,165 @@ def commit_then_raise(sn, st): self.assertEqual(actual.column('id').to_pylist(), [1, 2, 3]) self.assertEqual(actual.column('name').to_pylist(), ['a', 'b', 'c']) + def test_lost_commit_response_resolves_duplicate_as_success(self): + pa_schema = pa.schema([('id', pa.int32()), ('name', pa.string())]) + opts = { + 'bucket': '1', + 'file.format': 'parquet', + 'commit.max-retries': '1', + 'commit.min-retry-wait': '1ms', + 'commit.max-retry-wait': '1ms', + 'commit.timeout': '10s', + } + schema = Schema.from_pyarrow_schema(pa_schema, options=opts) + table_name = 'default.test_duplicate_commit_success' + self.rest_catalog.create_table(table_name, schema, False) + table = self.rest_catalog.get_table(table_name) + + write_builder = table.new_batch_write_builder() + table_write = write_builder.new_write() + table_commit = write_builder.new_commit() + data = pa.Table.from_pydict( + {'id': [1, 2, 3], 'name': ['a', 'b', 'c']}, schema=pa_schema) + table_write.write_arrow(data) + commit_messages = table_write.prepare_commit() + + real_commit = table_commit.file_store_commit.snapshot_commit.commit + commit_calls = [] + + def commit_then_lose_response(snapshot, statistics): + commit_calls.append(snapshot.id) + real_commit(snapshot, statistics) + raise RuntimeError("simulated lost response") + + with patch.object( + table_commit.file_store_commit.snapshot_commit, + 'commit', + side_effect=commit_then_lose_response): + table_commit.commit(commit_messages) + + table_write.close() + table_commit.close() + + self.assertEqual([1], commit_calls) + self.assertEqual(1, table.snapshot_manager().get_latest_snapshot().id) + read_builder = table.new_read_builder() + actual = read_builder.new_read().to_arrow( + read_builder.new_scan().plan().splits()) + self.assertEqual(actual.num_rows, 3) + self.assertEqual(actual.column('id').to_pylist(), [1, 2, 3]) + self.assertEqual(actual.column('name').to_pylist(), ['a', 'b', 'c']) + + def test_lost_truncate_response_resolves_duplicate_as_success(self): + pa_schema = pa.schema([('id', pa.int32()), ('name', pa.string())]) + opts = { + 'bucket': '1', + 'file.format': 'parquet', + 'commit.max-retries': '1', + 'commit.min-retry-wait': '1ms', + 'commit.max-retry-wait': '1ms', + 'commit.timeout': '10s', + } + schema = Schema.from_pyarrow_schema(pa_schema, options=opts) + table_name = 'default.test_duplicate_truncate_success' + self.rest_catalog.create_table(table_name, schema, False) + table = self.rest_catalog.get_table(table_name) + + write_builder = table.new_batch_write_builder() + table_write = write_builder.new_write() + table_commit = write_builder.new_commit() + data = pa.Table.from_pydict( + {'id': [1, 2, 3], 'name': ['a', 'b', 'c']}, schema=pa_schema) + table_write.write_arrow(data) + table_commit.commit(table_write.prepare_commit()) + table_write.close() + table_commit.close() + + truncate_commit = table.new_batch_write_builder().new_commit() + real_commit = truncate_commit.file_store_commit.snapshot_commit.commit + commit_calls = [] + + def commit_then_lose_response(snapshot, statistics): + commit_calls.append(snapshot.id) + real_commit(snapshot, statistics) + raise RuntimeError("simulated lost response") + + with patch.object( + truncate_commit.file_store_commit.snapshot_commit, + 'commit', + side_effect=commit_then_lose_response): + truncate_commit.truncate_table() + truncate_commit.close() + + self.assertEqual([2], commit_calls) + latest_snapshot = table.snapshot_manager().get_latest_snapshot() + self.assertEqual(2, latest_snapshot.id) + self.assertEqual('OVERWRITE', latest_snapshot.commit_kind) + read_builder = table.new_read_builder() + actual = read_builder.new_read().to_arrow( + read_builder.new_scan().plan().splits()) + self.assertEqual(0, actual.num_rows) + + def test_lost_drop_partitions_response_resolves_duplicate_as_success(self): + pa_schema = pa.schema([ + ('id', pa.int32()), + ('name', pa.string()), + ('dt', pa.string()), + ]) + opts = { + 'bucket': '1', + 'file.format': 'parquet', + 'commit.max-retries': '1', + 'commit.min-retry-wait': '1ms', + 'commit.max-retry-wait': '1ms', + 'commit.timeout': '10s', + } + schema = Schema.from_pyarrow_schema( + pa_schema, partition_keys=['dt'], options=opts) + table_name = 'default.test_duplicate_drop_partitions_success' + self.rest_catalog.create_table(table_name, schema, False) + table = self.rest_catalog.get_table(table_name) + + write_builder = table.new_batch_write_builder() + table_write = write_builder.new_write() + table_commit = write_builder.new_commit() + data = pa.Table.from_pydict({ + 'id': [1, 2], + 'name': ['drop', 'keep'], + 'dt': ['p1', 'p2'], + }, schema=pa_schema) + table_write.write_arrow(data) + table_commit.commit(table_write.prepare_commit()) + table_write.close() + table_commit.close() + + partition_commit = table.new_batch_write_builder().new_commit() + real_commit = partition_commit.file_store_commit.snapshot_commit.commit + commit_calls = [] + + def commit_then_lose_response(snapshot, statistics): + commit_calls.append(snapshot.id) + real_commit(snapshot, statistics) + raise RuntimeError("simulated lost response") + + with patch.object( + partition_commit.file_store_commit.snapshot_commit, + 'commit', + side_effect=commit_then_lose_response): + partition_commit.truncate_partitions([{'dt': 'p1'}]) + partition_commit.close() + + self.assertEqual([2], commit_calls) + latest_snapshot = table.snapshot_manager().get_latest_snapshot() + self.assertEqual(2, latest_snapshot.id) + self.assertEqual('OVERWRITE', latest_snapshot.commit_kind) + read_builder = table.new_read_builder() + actual = read_builder.new_read().to_arrow( + read_builder.new_scan().plan().splits()) + self.assertEqual([2], actual.column('id').to_pylist()) + self.assertEqual(['keep'], actual.column('name').to_pylist()) + self.assertEqual(['p2'], actual.column('dt').to_pylist()) + if __name__ == '__main__': unittest.main() diff --git a/paimon-python/pypaimon/tests/table_update_test.py b/paimon-python/pypaimon/tests/table_update_test.py index 40ceaf2489f6..7f59f32ef810 100644 --- a/paimon-python/pypaimon/tests/table_update_test.py +++ b/paimon-python/pypaimon/tests/table_update_test.py @@ -603,6 +603,28 @@ def test_concurrent_row_level_deletes_conflict_on_same_dv_file(self): result = self._read_all(table).sort_by('id') self.assertEqual([1, 3, 4, 5], result['id'].to_pylist()) + def test_concurrent_deletes_on_different_files_are_preserved(self): + table = self._create_seeded_deletion_vector_table() + rb = table.new_read_builder().with_projection(['id', '_ROW_ID']) + rows = rb.new_read().to_arrow(rb.new_scan().plan().splits()) + row_ids = dict(zip( + rows['id'].to_pylist(), rows['_ROW_ID'].to_pylist())) + + first_wb = self._make_write_builder(table) + first_update = first_wb.new_update() + first_cid = self._next_commit_id() + first_messages = self._apply_delete_by_row_id( + first_update, [row_ids[3]], first_cid) + + self._do_delete_by_row_id(table, [row_ids[1]]) + + first_commit = first_wb.new_commit() + self._apply_commit(first_commit, first_messages, first_cid) + first_commit.close() + + result = self._read_all(table).sort_by('id') + self.assertEqual([2, 4, 5], result['id'].to_pylist()) + def test_row_level_delete_conflicts_when_target_file_removed(self): table = self._create_seeded_deletion_vector_table(partition_keys=['city']) pb = table.new_read_builder().new_predicate_builder() @@ -638,6 +660,360 @@ def test_row_level_delete_conflicts_when_target_file_removed(self): result = self._read_all(table).sort_by('id') self.assertEqual([1, 3, 4, 5, 6], result['id'].to_pylist()) + def test_row_id_update_allows_disjoint_partition_overwrite(self): + table = self._create_seeded_table(partition_keys=['city']) + rb = table.new_read_builder().with_projection(['id', '_ROW_ID']) + rows = rb.new_read().to_arrow(rb.new_scan().plan().splits()) + row_ids = dict(zip( + rows['id'].to_pylist(), rows['_ROW_ID'].to_pylist())) + + update_wb = self._make_write_builder(table) + update = update_wb.new_update().with_update_type(['age']) + update_cid = self._next_commit_id() + update_messages = self._apply_update( + update, + pa.Table.from_pydict({ + '_ROW_ID': [row_ids[1]], + 'age': [99], + }, schema=pa.schema([ + ('_ROW_ID', pa.int64()), + ('age', pa.int32()), + ])), + update_cid, + ) + + overwrite_wb = table.new_batch_write_builder().overwrite({'city': 'LA'}) + overwrite_write = overwrite_wb.new_write() + overwrite_commit = overwrite_wb.new_commit() + overwrite_write.write_arrow(pa.Table.from_pydict({ + 'id': [6], + 'name': ['Frank'], + 'age': [28], + 'city': ['LA'], + }, schema=self.pa_schema)) + overwrite_commit.commit(overwrite_write.prepare_commit()) + overwrite_write.close() + overwrite_commit.close() + + update_commit = update_wb.new_commit() + self._apply_commit(update_commit, update_messages, update_cid) + update_commit.close() + + result = self._read_all(table).to_pydict() + ages = dict(zip(result['id'], result['age'])) + self.assertEqual({1, 3, 4, 5, 6}, set(result['id'])) + self.assertEqual(99, ages[1]) + + def test_row_id_update_conflicts_with_concurrent_dv_delete(self): + table = self._create_seeded_deletion_vector_table() + rb = table.new_read_builder().with_projection(['id', '_ROW_ID']) + rows = rb.new_read().to_arrow(rb.new_scan().plan().splits()) + row_ids = dict(zip( + rows['id'].to_pylist(), rows['_ROW_ID'].to_pylist())) + + update_wb = self._make_write_builder(table) + update = update_wb.new_update().with_update_type(['age']) + update_cid = self._next_commit_id() + update_messages = self._apply_update( + update, + pa.Table.from_pydict({ + '_ROW_ID': [row_ids[1]], + 'age': [99], + }, schema=pa.schema([ + ('_ROW_ID', pa.int64()), + ('age', pa.int32()), + ])), + update_cid, + ) + staged_paths = [ + file.external_path or file.file_path + for message in update_messages + for file in message.new_files + ] + self.assertTrue(staged_paths) + for path in staged_paths: + self.assertTrue(table.file_io.exists(path)) + + self._do_delete_by_row_id(table, [row_ids[1]]) + + update_commit = update_wb.new_commit() + with self.assertRaisesRegex(RuntimeError, "deletion vectors"): + self._apply_commit(update_commit, update_messages, update_cid) + update_commit.close() + for path in staged_paths: + self.assertFalse(table.file_io.exists(path)) + + result = self._read_all(table).to_pydict() + self.assertNotIn(1, result['id']) + + def test_false_retry_dv_conflict_aborts_staged_files(self): + table = self._create_seeded_deletion_vector_table() + rb = table.new_read_builder().with_projection(['id', '_ROW_ID']) + rows = rb.new_read().to_arrow(rb.new_scan().plan().splits()) + row_ids = dict(zip( + rows['id'].to_pylist(), rows['_ROW_ID'].to_pylist())) + + update_wb = self._make_write_builder(table) + update = update_wb.new_update().with_update_type(['age']) + commit_id = self._next_commit_id() + update_messages = self._apply_update( + update, + pa.Table.from_pydict({ + '_ROW_ID': [row_ids[1]], + 'age': [99], + }, schema=pa.schema([ + ('_ROW_ID', pa.int64()), + ('age', pa.int32()), + ])), + commit_id, + ) + staged_paths = [ + file.external_path or file.file_path + for message in update_messages + for file in message.new_files + ] + self.assertTrue(staged_paths) + + update_commit = update_wb.new_commit() + calls = 0 + + def reject_first_commit(_snapshot, _statistics): + nonlocal calls + calls += 1 + self._do_delete_by_row_id(table, [row_ids[1]]) + return False + + with mock.patch.object( + update_commit.file_store_commit.snapshot_commit, + 'commit', side_effect=reject_first_commit), mock.patch.object( + update_commit.file_store_commit, + '_commit_retry_wait'): + with self.assertRaisesRegex(RuntimeError, "deletion vectors"): + self._apply_commit( + update_commit, update_messages, commit_id) + update_commit.close() + + self.assertEqual(1, calls) + for path in staged_paths: + self.assertFalse(table.file_io.exists(path)) + self.assertNotIn(1, self._read_all(table)['id'].to_pylist()) + + def test_row_id_update_allows_disjoint_partition_dv_delete(self): + table = self._create_seeded_deletion_vector_table( + partition_keys=['city']) + rb = table.new_read_builder().with_projection(['id', '_ROW_ID']) + rows = rb.new_read().to_arrow(rb.new_scan().plan().splits()) + row_ids = dict(zip( + rows['id'].to_pylist(), rows['_ROW_ID'].to_pylist())) + + update_wb = self._make_write_builder(table) + update = update_wb.new_update().with_update_type(['age']) + update_cid = self._next_commit_id() + update_messages = self._apply_update( + update, + pa.Table.from_pydict({ + '_ROW_ID': [row_ids[1]], + 'age': [99], + }, schema=pa.schema([ + ('_ROW_ID', pa.int64()), + ('age', pa.int32()), + ])), + update_cid, + ) + + self._do_delete_by_row_id(table, [row_ids[2]]) + + update_commit = update_wb.new_commit() + self._apply_commit(update_commit, update_messages, update_cid) + update_commit.close() + + result = self._read_all(table).to_pydict() + ages = dict(zip(result['id'], result['age'])) + self.assertEqual(99, ages[1]) + self.assertNotIn(2, ages) + + def test_row_id_update_allows_other_file_dv_delete(self): + options = dict(self.table_options) + options['deletion-vectors.enabled'] = 'true' + table = self._create_table( + partition_keys=['city'], options=options) + self._write_arrow(table, pa.Table.from_pydict({ + 'id': [1], + 'name': ['Alice'], + 'age': [25], + 'city': ['NYC'], + }, schema=self.pa_schema)) + self._write_arrow(table, pa.Table.from_pydict({ + 'id': [2], + 'name': ['Bob'], + 'age': [30], + 'city': ['NYC'], + }, schema=self.pa_schema)) + rb = table.new_read_builder().with_projection(['id', '_ROW_ID']) + rows = rb.new_read().to_arrow(rb.new_scan().plan().splits()) + row_ids = dict(zip( + rows['id'].to_pylist(), rows['_ROW_ID'].to_pylist())) + + update_wb = self._make_write_builder(table) + update = update_wb.new_update().with_update_type(['age']) + update_cid = self._next_commit_id() + update_messages = self._apply_update( + update, + pa.Table.from_pydict({ + '_ROW_ID': [row_ids[1]], + 'age': [99], + }, schema=pa.schema([ + ('_ROW_ID', pa.int64()), + ('age', pa.int32()), + ])), + update_cid, + ) + + self._do_delete_by_row_id(table, [row_ids[2]]) + + update_commit = update_wb.new_commit() + self._apply_commit(update_commit, update_messages, update_cid) + update_commit.close() + + result = self._read_all(table).to_pydict() + ages = dict(zip(result['id'], result['age'])) + self.assertEqual({1: 99}, ages) + + def test_mixed_index_message_detects_concurrent_dv_delete(self): + table = self._create_seeded_deletion_vector_table( + partition_keys=['city']) + rb = table.new_read_builder().with_projection(['id', '_ROW_ID']) + rows = rb.new_read().to_arrow(rb.new_scan().plan().splits()) + row_ids = dict(zip( + rows['id'].to_pylist(), rows['_ROW_ID'].to_pylist())) + + update_wb = self._make_write_builder(table) + update = update_wb.new_update().with_update_type(['age']) + commit_id = self._next_commit_id() + update_messages = self._apply_update( + update, + pa.Table.from_pydict({ + '_ROW_ID': [row_ids[1]], + 'age': [99], + }, schema=pa.schema([ + ('_ROW_ID', pa.int64()), + ('age', pa.int32()), + ])), + commit_id, + ) + + delete_wb = self._make_write_builder(table) + delete_update = delete_wb.new_update() + delete_messages = self._apply_delete_by_row_id( + delete_update, [row_ids[2]], commit_id) + + self._do_delete_by_row_id(table, [row_ids[1]]) + + update_commit = update_wb.new_commit() + with self.assertRaisesRegex(RuntimeError, "index-changing overwrite"): + self._apply_commit( + update_commit, + update_messages + delete_messages, + commit_id, + ) + update_commit.close() + + result = self._read_all(table).to_pydict() + self.assertNotIn(1, result['id']) + self.assertIn(2, result['id']) + + def test_mixed_insert_detects_concurrent_dv_delete(self): + table = self._create_seeded_deletion_vector_table( + partition_keys=['city']) + rb = table.new_read_builder().with_projection(['id', '_ROW_ID']) + rows = rb.new_read().to_arrow(rb.new_scan().plan().splits()) + row_ids = dict(zip( + rows['id'].to_pylist(), rows['_ROW_ID'].to_pylist())) + + update_wb = self._make_write_builder(table) + update = update_wb.new_update().with_update_type(['age']) + commit_id = self._next_commit_id() + update_messages = self._apply_update( + update, + pa.Table.from_pydict({ + '_ROW_ID': [row_ids[1]], + 'age': [99], + }, schema=pa.schema([ + ('_ROW_ID', pa.int64()), + ('age', pa.int32()), + ])), + commit_id, + ) + + insert_wb = self._make_write_builder(table) + insert_write = insert_wb.new_write() + insert_write.write_arrow(pa.Table.from_pydict({ + 'id': [6], + 'name': ['Frank'], + 'age': [28], + 'city': ['SF'], + }, schema=self.pa_schema)) + insert_messages = self._prepare_write_commit( + insert_write, commit_id) + + self._do_delete_by_row_id(table, [row_ids[1]]) + + update_commit = update_wb.new_commit() + with self.assertRaisesRegex(RuntimeError, "deletion vectors"): + self._apply_commit( + update_commit, + update_messages + insert_messages, + commit_id, + ) + update_commit.close() + insert_write.close() + + result = self._read_all(table).to_pydict() + self.assertNotIn(1, result['id']) + self.assertNotIn(6, result['id']) + + def test_blob_update_allows_dv_delete_outside_delta_range(self): + table_schema = pa.schema([ + ('id', pa.int32()), + ('picture', pa.large_binary()), + ]) + options = dict(self.table_options) + options['deletion-vectors.enabled'] = 'true' + table = self._create_table(pa_schema=table_schema, options=options) + self._write_arrow(table, pa.Table.from_pydict({ + 'id': list(range(10)), + 'picture': [f'blob-{i}'.encode() for i in range(10)], + }, schema=table_schema)) + rb = table.new_read_builder().with_projection(['id', '_ROW_ID']) + rows = rb.new_read().to_arrow(rb.new_scan().plan().splits()) + row_ids = dict(zip( + rows['id'].to_pylist(), rows['_ROW_ID'].to_pylist())) + + update_wb = self._make_write_builder(table) + update = update_wb.new_update().with_update_type(['picture']) + commit_id = self._next_commit_id() + update_messages = self._apply_update( + update, + pa.Table.from_pydict({ + '_ROW_ID': [row_ids[0]], + 'picture': [b'updated'], + }, schema=pa.schema([ + ('_ROW_ID', pa.int64()), + ('picture', pa.large_binary()), + ])), + commit_id, + ) + + self._do_delete_by_row_id(table, [row_ids[9]]) + + update_commit = update_wb.new_commit() + self._apply_commit(update_commit, update_messages, commit_id) + update_commit.close() + + result = self._read_all(table).sort_by('id').to_pydict() + self.assertEqual(list(range(9)), result['id']) + self.assertEqual(b'updated', result['picture'][0]) + def test_delete_by_partition_predicate_drops_partition_without_dv(self): table = self._create_seeded_table(partition_keys=['city']) pb = table.new_read_builder().new_predicate_builder() @@ -1254,6 +1630,49 @@ def test_update_list_and_map_columns(self): # Mode-specific mixins (add the ``update_by_arrow_with_row_id`` primitive) # ====================================================================== + +class TableUpdateByRowIdPlanningTest(unittest.TestCase): + + def test_uses_snapshot_loaded_by_planner(self): + from pypaimon.utils.range import Range + from pypaimon.write.table_update_by_row_id import TableUpdateByRowId + + snapshot = mock.Mock( + id=7, + commit_user="user", + commit_identifier=1, + commit_kind="APPEND", + time_millis=1, + base_manifest_list="base", + delta_manifest_list="delta", + changelog_manifest_list=None, + index_manifest=None, + ) + file = mock.Mock( + file_name="data.parquet", + first_row_id=0, + ) + file.row_id_range.return_value = Range(0, 9) + split = mock.Mock(files=[file]) + plan = mock.Mock(snapshot_id=7) + plan.splits.return_value = [split] + scan = mock.Mock() + scan.plan_for_write.return_value = plan + scan.file_scanner.scanned_snapshot = snapshot + table = mock.Mock() + table.new_read_builder.return_value.new_scan.return_value = scan + updater = TableUpdateByRowId.__new__(TableUpdateByRowId) + updater.table = table + + info = updater._load_existing_files_info() + + self.assertEqual( + TableUpdateByRowId._snapshot_fingerprint(snapshot), + info.snapshot_identity, + ) + table.snapshot_manager.assert_not_called() + + class _BatchModeMixin(BatchModeMixin): def _apply_update(self, table_update, data, cid): return table_update.update_by_arrow_with_row_id(data) diff --git a/paimon-python/pypaimon/tests/write/commit_callback_test.py b/paimon-python/pypaimon/tests/write/commit_callback_test.py index 4e02bf32f67a..5013d40c8026 100644 --- a/paimon-python/pypaimon/tests/write/commit_callback_test.py +++ b/paimon-python/pypaimon/tests/write/commit_callback_test.py @@ -24,6 +24,7 @@ from pypaimon import CatalogFactory, Schema from pypaimon.write.commit_callback import CommitCallback, CommitCallbackContext +from pypaimon.write.file_store_commit import CommitOutcomeUnknownError class RecordingCallback(CommitCallback): @@ -169,6 +170,31 @@ def test_callback_not_invoked_when_no_data(self): table_write.close() table_commit.close() + def test_callback_failure_keeps_committed_snapshot(self): + class FailingCallback(CommitCallback): + def call(self, context): + raise RuntimeError("callback failed") + + table = self._create_table('test_callback_failure') + write_builder = table.new_batch_write_builder() + table_write = write_builder.new_write() + table_commit = write_builder.new_commit() + table_commit.add_commit_callback(FailingCallback()) + table_write.write_arrow(pa.Table.from_pydict({ + 'id': [1], + 'name': ['a'], + 'dt': ['p1'], + }, schema=self.pa_schema)) + + with self.assertRaisesRegex( + CommitOutcomeUnknownError, "callback failed"): + table_commit.commit(table_write.prepare_commit()) + + self.assertEqual( + 1, table.snapshot_manager().get_latest_snapshot().id) + table_write.close() + table_commit.close() + def test_stream_commit_callback_multiple_rounds(self): table = self._create_table('test_stream_callback') write_builder = table.new_stream_write_builder() diff --git a/paimon-python/pypaimon/tests/write/conflict_detection_test.py b/paimon-python/pypaimon/tests/write/conflict_detection_test.py index 62889370f56d..e22c35d24e1a 100644 --- a/paimon-python/pypaimon/tests/write/conflict_detection_test.py +++ b/paimon-python/pypaimon/tests/write/conflict_detection_test.py @@ -26,6 +26,7 @@ from pypaimon.manifest.schema.manifest_entry import ManifestEntry from pypaimon.schema.data_types import AtomicType, DataField from pypaimon.table.row.generic_row import GenericRow +from pypaimon.write.commit.commit_scanner import CommitScanner from pypaimon.write.commit.conflict_detection import ( ConflictDetection, RowIdColumnConflictChecker, @@ -56,10 +57,13 @@ def _make_file(file_name, row_count=100, first_row_id=None, def _make_entry(file_name, kind=0, bucket=0, first_row_id=None, - row_count=100, write_cols=None, schema_id=0): + row_count=100, write_cols=None, schema_id=0, + partition=None): return ManifestEntry( kind=kind, - partition=_EMPTY_PARTITION, + partition=( + _EMPTY_PARTITION + if partition is None else GenericRow(list(partition), [])), bucket=bucket, total_buckets=1, file=_make_file(file_name, row_count=row_count, @@ -130,6 +134,19 @@ def test_conflict_when_base_file_rewritten(self): self.assertIsNotNone(result) self.assertIn("Row ID existence conflict", str(result)) + def test_dedicated_file_is_not_a_normal_file_anchor(self): + detection = self._make_detection() + delta = [_make_entry("p1", kind=0, first_row_id=0, row_count=100)] + + for file_name in ("p0.blob", "p0.vector.parquet"): + with self.subTest(file_name=file_name): + base = [_make_entry( + file_name, kind=0, first_row_id=0, row_count=100)] + result = detection.check_row_id_existence( + base, delta, next_row_id=200) + self.assertIsNotNone(result) + self.assertIn("Row ID existence conflict", str(result)) + def test_no_conflict_when_blob_file_range_is_covered(self): detection = self._make_detection() base = [_make_entry("f1", kind=0, first_row_id=0, row_count=100)] @@ -314,18 +331,26 @@ def test_delete_entry_missing_from_base_conflicts(self): class _FakeSnapshot: - def __init__(self, snapshot_id, commit_kind, next_row_id=None): + def __init__(self, snapshot_id, commit_kind, next_row_id=None, + commit_user=None, commit_identifier=None, + delta_manifest_list=None, index_manifest=None): self.id = snapshot_id self.commit_kind = commit_kind self.next_row_id = next_row_id + self.commit_user = commit_user + self.commit_identifier = commit_identifier + self.delta_manifest_list = delta_manifest_list + self.index_manifest = index_manifest class _FakeSnapshotManager: def __init__(self, snapshots): self._by_id = {s.id: s for s in snapshots} + self.requests = [] def get_snapshot_by_id(self, snapshot_id): + self.requests.append(snapshot_id) return self._by_id.get(snapshot_id) @@ -334,20 +359,497 @@ class _FakeCommitScanner: def __init__(self, entries_by_snapshot_id, raw_entries_by_snapshot_id=None): self._by_id = entries_by_snapshot_id self._raw_by_id = raw_entries_by_snapshot_id or {} + self.entry_calls = [] + self.raw_entry_calls = [] def read_incremental_entries_from_changed_partitions(self, snapshot, _): - return self._by_id.get(snapshot.id, []) + self.entry_calls.append(snapshot.id) + return self._by_id.get( + snapshot.id, self._raw_by_id.get(snapshot.id, [])) def read_incremental_raw_entries_from_changed_partitions(self, snapshot, _): + self.raw_entry_calls.append(snapshot.id) + return self._raw_by_id.get(snapshot.id, self._by_id.get(snapshot.id, [])) + + def read_incremental_raw_entries_for_scope( + self, snapshot, _entries, _index_entries=None): + self.entry_calls.append(snapshot.id) + self.raw_entry_calls.append(snapshot.id) return self._raw_by_id.get(snapshot.id, self._by_id.get(snapshot.id, [])) +class _FakeBaseEntryScanner: + + def __init__(self, _full_entries, incremental_entries, fallback_entries=None): + self._incremental_entries = incremental_entries + self._fallback_entries = fallback_entries or [] + self.fallback_calls = 0 + self.scoped_raw_calls = [] + + def read_conflict_entries(self, snapshot, entries, index_entries=None): + self.fallback_calls += 1 + scope = CommitScanner.conflict_entry_scope(entries, index_entries) + if scope.is_empty(): + return list(self._fallback_entries) + return [ + entry for entry in self._fallback_entries + if scope.matches_entry(entry) + ] + + def read_incremental_entries_from_changed_partitions( + self, snapshot, _entries): + return self._incremental_entries.get(snapshot.id, []) + + def read_incremental_raw_entries_from_changed_partitions( + self, snapshot, _entries): + return self._incremental_entries.get(snapshot.id, []) + + def read_incremental_raw_entries_for_scope( + self, snapshot, entries, index_entries=None): + self.scoped_raw_calls.append(snapshot.id) + scope = CommitScanner.conflict_entry_scope(entries, index_entries) + return [ + entry for entry in self._incremental_entries.get(snapshot.id, []) + if scope.matches_entry(entry) + ] + + class _FakeTable: def __init__(self, schema_manager): self.schema_manager = schema_manager +class TestRowIdWindowBaseEntries(unittest.TestCase): + + def _make_detection(self, snapshots, scanner): + detection = ConflictDetection( + data_evolution_enabled=True, + snapshot_manager=_FakeSnapshotManager(snapshots), + manifest_list_manager=None, + table=_FakeTable(_FakeSchemaManager([_DEFAULT_SCHEMA])), + commit_scanner=scanner, + ) + detection.set_row_id_check_from_snapshot(1) + return detection + + def test_applies_external_snapshot_deltas(self): + base = _FakeSnapshot(1, "APPEND", delta_manifest_list="base") + latest = _FakeSnapshot( + 2, "APPEND", commit_user="external", commit_identifier=1, + delta_manifest_list="latest") + base_entry = _make_entry( + "base.parquet", first_row_id=0, row_count=100) + added_entry = _make_entry( + "added.parquet", first_row_id=0, row_count=100) + window = [_make_entry( + "window.parquet", first_row_id=0, row_count=100)] + scanner = _FakeBaseEntryScanner( + {"base": [base_entry]}, {2: [added_entry]}) + detection = self._make_detection([base, latest], scanner) + + self.assertEqual( + [base_entry, added_entry], + detection.read_row_id_base_entries( + latest, + window, + planned_base_entries=[base_entry], + planned_base_snapshot_identity=( + detection._snapshot_identity(base)), + ), + ) + self.assertEqual([2], scanner.scoped_raw_calls) + self.assertEqual(0, scanner.fallback_calls) + + def test_fallback_rejects_index_changing_overwrite(self): + base = _FakeSnapshot( + 1, "APPEND", next_row_id=100, + delta_manifest_list="base", index_manifest="old-index") + overwrite = _FakeSnapshot( + 2, "OVERWRITE", next_row_id=100, + commit_user="external", commit_identifier=1, + delta_manifest_list="overwrite", index_manifest="new-index") + base_entry = _make_entry( + "base.parquet", first_row_id=0, row_count=100) + window = [_make_entry( + "window.parquet", first_row_id=0, row_count=100)] + scanner = _FakeBaseEntryScanner( + {"base": [base_entry]}, {2: []}, + fallback_entries=[base_entry]) + detection = self._make_detection([base, overwrite], scanner) + + with self.assertRaisesRegex(RuntimeError, "index-changing overwrite"): + detection.read_row_id_base_entries(overwrite, window) + + self.assertEqual(0, scanner.fallback_calls) + + def test_disjoint_partition_overwrite_uses_current_state(self): + base = _FakeSnapshot( + 1, "APPEND", next_row_id=200, delta_manifest_list="base") + overwrite = _FakeSnapshot( + 2, "OVERWRITE", next_row_id=200, + commit_user="external", commit_identifier=1, + delta_manifest_list="overwrite") + old_entry = _make_entry( + "old.parquet", first_row_id=0, row_count=100, + partition=("a",)) + overwritten = _make_entry( + "other.parquet", first_row_id=0, row_count=100, + partition=("b",)) + window = [_make_entry( + "window.parquet", first_row_id=0, row_count=100, + partition=("a",))] + + for bounded in (False, True): + with self.subTest(bounded=bounded): + scanner = _FakeBaseEntryScanner( + {"base": [old_entry]}, + {2: [overwritten]}, + fallback_entries=[old_entry, overwritten], + ) + detection = self._make_detection( + [base, overwrite], scanner) + if bounded: + detection.enable_bounded_row_id_conflict_state() + + current = detection.read_row_id_base_entries( + overwrite, + window, + planned_base_entries=[old_entry], + planned_base_snapshot_identity=( + detection._snapshot_identity(base)), + ) + result = detection.check_conflicts( + overwrite, current, window, "APPEND") + + self.assertIsNone(result) + self.assertEqual([old_entry], current) + self.assertEqual(1, scanner.fallback_calls) + self.assertEqual([] if bounded else [2], + scanner.scoped_raw_calls) + + def test_overlapping_overwrite_still_conflicts(self): + base = _FakeSnapshot( + 1, "APPEND", next_row_id=200, delta_manifest_list="base") + overwrite = _FakeSnapshot( + 2, "OVERWRITE", next_row_id=200, + commit_user="external", commit_identifier=1, + delta_manifest_list="overwrite") + old_entry = _make_entry( + "old.parquet", first_row_id=0, row_count=100, + partition=("a",), write_cols=["col_a"]) + replacement = _make_entry( + "replacement.parquet", first_row_id=0, row_count=100, + partition=("a",), write_cols=["col_a"]) + window = [_make_entry( + "window.parquet", first_row_id=0, row_count=100, + partition=("a",), write_cols=["col_b"])] + scanner = _FakeBaseEntryScanner( + {"base": [old_entry]}, + {2: []}, + fallback_entries=[replacement], + ) + detection = self._make_detection([base, overwrite], scanner) + detection.enable_bounded_row_id_conflict_state() + + with self.assertRaisesRegex( + RuntimeError, "Concurrent overwrite changed row ID files"): + detection.read_row_id_base_entries( + overwrite, + window, + planned_base_entries=[old_entry], + planned_base_snapshot_identity=( + detection._snapshot_identity(base)), + ) + + def test_index_changing_overwrite_checks_deletion_vectors(self): + base = _FakeSnapshot( + 1, "APPEND", next_row_id=100, + delta_manifest_list="base", index_manifest="old-index") + overwrite = _FakeSnapshot( + 2, "OVERWRITE", next_row_id=100, + commit_user="external", commit_identifier=1, + delta_manifest_list="overwrite", index_manifest="new-index") + base_entry = _make_entry( + "base.parquet", first_row_id=0, row_count=100) + window = [_make_entry( + "window.parquet", first_row_id=0, row_count=100)] + + for bounded in (False, True): + for changed in (False, True): + with self.subTest(bounded=bounded, changed=changed): + scanner = _FakeBaseEntryScanner( + {"base": [base_entry]}, {2: []}, + fallback_entries=[base_entry]) + detection = self._make_detection( + [base, overwrite], scanner) + detection._row_id_deletion_vectors_changed = ( + lambda *_args, changed=changed: changed) + if bounded: + detection.enable_bounded_row_id_conflict_state() + + if not changed: + self.assertEqual( + [base_entry], + detection.read_row_id_base_entries( + overwrite, + window, + planned_base_entries=[base_entry], + planned_base_snapshot_identity=( + detection._snapshot_identity(base)), + ), + ) + continue + + with self.assertRaisesRegex( + RuntimeError, "deletion vectors"): + detection.read_row_id_base_entries( + overwrite, + window, + planned_base_entries=[base_entry], + planned_base_snapshot_identity=( + detection._snapshot_identity(base)), + ) + + self.assertEqual(0, scanner.fallback_calls) + + def test_missing_normal_dv_target_fails_closed(self): + base = _FakeSnapshot(1, "APPEND") + latest = _FakeSnapshot(2, "OVERWRITE") + window = [_make_entry( + "window.blob", first_row_id=0, row_count=1)] + detection = self._make_detection( + [base, latest], _FakeBaseEntryScanner({}, {})) + + self.assertTrue(detection._row_id_deletion_vectors_changed( + base, latest, [], window)) + + def test_bounded_overwrite_is_checked_for_later_windows(self): + base = _FakeSnapshot( + 1, "APPEND", next_row_id=200, delta_manifest_list="base") + overwrite = _FakeSnapshot( + 2, "OVERWRITE", next_row_id=200, + commit_user="external", commit_identifier=1, + delta_manifest_list="overwrite") + old_a = _make_entry( + "old-a.parquet", first_row_id=0, row_count=100, + partition=("a",)) + old_b = _make_entry( + "old-b.parquet", first_row_id=100, row_count=100, + partition=("b",)) + replacement_b = _make_entry( + "replacement-b.parquet", first_row_id=100, row_count=100, + partition=("b",)) + scanner = _FakeBaseEntryScanner( + {"base": [old_a, old_b]}, + {2: []}, + fallback_entries=[old_a, replacement_b], + ) + detection = self._make_detection([base, overwrite], scanner) + detection.enable_bounded_row_id_conflict_state() + window_a = [_make_entry( + "window-a.parquet", first_row_id=0, row_count=100, + partition=("a",))] + window_b = [_make_entry( + "window-b.parquet", first_row_id=100, row_count=100, + partition=("b",))] + + current = detection.read_row_id_base_entries( + overwrite, + window_a, + planned_base_entries=[old_a], + planned_base_snapshot_identity=detection._snapshot_identity(base), + ) + self.assertIsNone(detection.check_conflicts( + overwrite, current, window_a, "APPEND")) + + with self.assertRaisesRegex( + RuntimeError, "Concurrent overwrite changed row ID files"): + detection.read_row_id_base_entries( + overwrite, + window_b, + planned_base_entries=[old_b], + planned_base_snapshot_identity=( + detection._snapshot_identity(base)), + ) + + self.assertEqual(2, scanner.fallback_calls) + self.assertEqual([], detection._row_id_external_snapshots) + + def test_bounded_state_rejects_external_snapshot(self): + base = _FakeSnapshot(1, "APPEND", delta_manifest_list="base") + latest = _FakeSnapshot( + 2, "APPEND", commit_user="external", commit_identifier=1, + delta_manifest_list="latest") + base_entry = _make_entry( + "base.parquet", first_row_id=0, row_count=100) + window = [_make_entry( + "window.parquet", first_row_id=0, row_count=100)] + scanner = _FakeBaseEntryScanner({"base": [base_entry]}, {}) + detection = self._make_detection([base, latest], scanner) + detection.enable_bounded_row_id_conflict_state() + + with self.assertRaisesRegex(RuntimeError, "Concurrent commit detected"): + detection.read_row_id_base_entries( + latest, + window, + planned_base_entries=[base_entry], + planned_base_snapshot_identity=( + detection._snapshot_identity(base)), + ) + + self.assertEqual([], detection._row_id_external_snapshots) + self.assertEqual([], scanner.scoped_raw_calls) + + def test_planned_windows_do_not_scan_base_manifests(self): + snapshot = _FakeSnapshot(1, "APPEND", delta_manifest_list="base") + planned = [ + _make_entry( + "p{}.parquet".format(i), first_row_id=i * 100, + row_count=100, partition=("p{}".format(i),)) + for i in range(16) + ] + scanner = _FakeBaseEntryScanner({"base": planned}, {}) + detection = self._make_detection([snapshot], scanner) + + for entry in planned: + window = [_make_entry( + "update-{}".format(entry.file.file_name), + first_row_id=entry.file.first_row_id, + row_count=entry.file.row_count, + partition=tuple(entry.partition.values))] + self.assertEqual( + [entry], + detection.read_row_id_base_entries( + snapshot, + window, + planned_base_entries=[entry], + planned_base_snapshot_identity=( + detection._snapshot_identity(snapshot)), + ), + ) + + self.assertEqual(0, scanner.fallback_calls) + self.assertEqual([], scanner.scoped_raw_calls) + + def test_rollback_before_base_fails_closed(self): + base = _FakeSnapshot(1, "APPEND", delta_manifest_list="base") + latest = _FakeSnapshot(2, "APPEND", delta_manifest_list="latest") + scanner = _FakeBaseEntryScanner({}, {}) + detection = self._make_detection([base, latest], scanner) + detection.set_row_id_check_from_snapshot(2) + window = [_make_entry( + "window.parquet", first_row_id=0, row_count=100)] + + with self.assertRaisesRegex(RuntimeError, "no longer available"): + detection.read_row_id_base_entries( + base, + window, + planned_base_entries=window, + planned_base_snapshot_identity=( + detection._snapshot_identity(latest)), + ) + + def test_missing_snapshot_fails_closed(self): + base = _FakeSnapshot(1, "APPEND", delta_manifest_list="base") + latest = _FakeSnapshot(3, "APPEND", delta_manifest_list="latest") + scanner = _FakeBaseEntryScanner({}, {}) + detection = self._make_detection([base, latest], scanner) + window = [_make_entry( + "window.parquet", first_row_id=0, row_count=100)] + + with self.assertRaisesRegex(RuntimeError, "snapshot 2"): + detection.read_row_id_base_entries( + latest, + window, + planned_base_entries=window, + planned_base_snapshot_identity=( + detection._snapshot_identity(base)), + ) + + def test_replaced_base_snapshot_fails_closed(self): + base = _FakeSnapshot(1, "APPEND", delta_manifest_list="base") + replacement = _FakeSnapshot( + 1, "APPEND", commit_user="external", commit_identifier=1, + delta_manifest_list="replacement") + scanner = _FakeBaseEntryScanner({}, {}) + detection = self._make_detection([base], scanner) + fingerprint = detection._snapshot_identity(base) + detection.snapshot_manager._by_id[1] = replacement + window = [_make_entry( + "window.parquet", first_row_id=0, row_count=100)] + + with self.assertRaisesRegex(RuntimeError, "base snapshot 1 changed"): + detection.read_row_id_base_entries( + replacement, + window, + planned_base_entries=window, + planned_base_snapshot_identity=fingerprint, + ) + + def test_missing_planned_base_uses_scoped_scan(self): + snapshot = _FakeSnapshot(1, "APPEND", delta_manifest_list="base") + anchor = _make_entry( + "base.parquet", first_row_id=0, row_count=100) + scanner = _FakeBaseEntryScanner( + {"base": [anchor]}, {}, fallback_entries=[anchor]) + detection = self._make_detection([snapshot], scanner) + window = [_make_entry( + "delta.parquet", first_row_id=20, row_count=10)] + + self.assertEqual( + [anchor], detection.read_row_id_base_entries(snapshot, window)) + self.assertEqual(1, scanner.fallback_calls) + + def test_compaction_overlap_cannot_be_hidden_by_blob_anchor(self): + base = _FakeSnapshot( + 1, "APPEND", next_row_id=200, delta_manifest_list="base") + compact = _FakeSnapshot( + 2, "COMPACT", next_row_id=200, delta_manifest_list="compact") + normal_0 = _make_entry( + "normal-0.parquet", first_row_id=0, row_count=100, + write_cols=["col_a"]) + normal_100 = _make_entry( + "normal-100.parquet", first_row_id=100, row_count=100, + write_cols=["col_a"]) + blob = _make_entry( + "payload.blob", first_row_id=0, row_count=100, + write_cols=["col_c"]) + compact_entries = [ + _make_entry( + "normal-0.parquet", kind=1, first_row_id=0, row_count=100, + write_cols=["col_a"]), + _make_entry( + "normal-100.parquet", kind=1, first_row_id=100, + row_count=100, write_cols=["col_a"]), + _make_entry( + "merged.parquet", first_row_id=0, row_count=200, + write_cols=["col_a"]), + ] + staged = [_make_entry( + "update.parquet", first_row_id=0, row_count=100, + write_cols=["col_b"])] + scanner = _FakeBaseEntryScanner( + {"base": [normal_0, normal_100, blob]}, + {2: compact_entries}, + ) + detection = self._make_detection([base, compact], scanner) + + current = detection.read_row_id_base_entries( + compact, + staged, + planned_base_entries=[normal_0, normal_100, blob], + planned_base_snapshot_identity=detection._snapshot_identity(base), + ) + result = detection.check_conflicts( + compact, current, staged, "APPEND") + + self.assertIsNotNone(result) + self.assertIn("Row ID existence conflict", str(result)) + self.assertEqual([2], scanner.scoped_raw_calls) + self.assertEqual(0, scanner.fallback_calls) + + class TestCheckRowIdFromSnapshot(unittest.TestCase): def _make_detection(self, snapshots, raw_entries_by_snapshot_id): @@ -365,6 +867,11 @@ def _blob_delta(self): return [_make_entry("d.blob", first_row_id=0, row_count=51, write_cols=["col_a"])] + def _vector_delta(self): + return [_make_entry( + "d.vector.parquet", first_row_id=0, row_count=51, + write_cols=["col_a"])] + def test_compact_blob_delete_raises_at_first_match(self): check_snap = _FakeSnapshot(1, "APPEND", next_row_id=200) compact1 = _FakeSnapshot(2, "COMPACT", next_row_id=200) @@ -380,17 +887,43 @@ def test_compact_blob_delete_raises_at_first_match(self): self.assertIn("snapshot 2", str(result)) self.assertIn("COMPACT", str(result)) + def test_compact_vector_delete_raises(self): + check_snap = _FakeSnapshot(1, "APPEND", next_row_id=200) + compact_snap = _FakeSnapshot(2, "COMPACT", next_row_id=200) + entries = {2: [_make_entry( + "old.vector.parquet", kind=1, first_row_id=0, row_count=100)]} + detection = self._make_detection( + [check_snap, compact_snap], entries) + + result = detection.check_row_id_from_snapshot( + compact_snap, self._vector_delta()) + + self.assertIsNotNone(result) + self.assertIn("COMPACT", str(result)) + def test_compact_other_file_type_does_not_raise(self): check_snap = _FakeSnapshot(1, "APPEND", next_row_id=200) compact_snap = _FakeSnapshot(2, "COMPACT", next_row_id=200) - compact_entries = [ - _make_entry("old.parquet", kind=1, first_row_id=0, row_count=100), - _make_entry("merged.parquet", kind=0, first_row_id=0, row_count=200), + cases = [ + (self._blob_delta(), "old.parquet"), + (self._vector_delta(), "old.parquet"), + (self._vector_delta(), "old.blob"), ] - detection = self._make_detection( - [check_snap, compact_snap], {2: compact_entries}) - self.assertIsNone( - detection.check_row_id_from_snapshot(compact_snap, self._blob_delta())) + for delta, deleted_file in cases: + with self.subTest( + delta=delta[0].file.file_name, + deleted_file=deleted_file): + compact_entries = [ + _make_entry( + deleted_file, kind=1, first_row_id=0, row_count=100), + _make_entry( + "merged.parquet", kind=0, first_row_id=0, + row_count=200), + ] + detection = self._make_detection( + [check_snap, compact_snap], {2: compact_entries}) + self.assertIsNone( + detection.check_row_id_from_snapshot(compact_snap, delta)) def test_compact_no_conflict_when_no_matching_delete(self): check_snap = _FakeSnapshot(1, "APPEND", next_row_id=400) @@ -417,6 +950,169 @@ def test_compact_no_conflict_when_no_matching_delete(self): self.assertIsNone( detection.check_row_id_from_snapshot(compact_snap, delta)) + def test_own_history_is_not_scanned(self): + base = _FakeSnapshot(1, "APPEND", next_row_id=1000) + own = [ + _FakeSnapshot( + snapshot_id, + "APPEND", + next_row_id=1000, + commit_user="incremental", + commit_identifier=snapshot_id - 1, + ) + for snapshot_id in range(2, 9) + ] + manager = _FakeSnapshotManager([base] + own) + scanner = _FakeCommitScanner({}) + detection = ConflictDetection( + data_evolution_enabled=True, + snapshot_manager=manager, + manifest_list_manager=None, + table=_FakeTable(_FakeSchemaManager([_DEFAULT_SCHEMA])), + commit_scanner=scanner, + ) + detection.set_row_id_check_from_snapshot(1) + detection.enable_bounded_row_id_conflict_state() + for snapshot in own: + detection.ignore_row_id_commit( + snapshot.commit_user, snapshot.commit_identifier) + + delta = self._blob_delta() + for latest in own: + self.assertIsNone( + detection.check_row_id_from_snapshot(latest, delta)) + + self.assertEqual([], scanner.entry_calls) + self.assertEqual( + {"incremental": 7}, + detection._row_id_ignored_commit_high_watermarks, + ) + for snapshot_id in range(2, 8): + self.assertEqual(2, manager.requests.count(snapshot_id)) + self.assertEqual(1, manager.requests.count(8)) + + def test_cached_external_history_checks_later_windows(self): + base = _FakeSnapshot(1, "APPEND", next_row_id=1000) + external = _FakeSnapshot( + 2, "APPEND", next_row_id=1000, + commit_user="external", commit_identifier=1) + own = _FakeSnapshot( + 3, "APPEND", next_row_id=1000, + commit_user="incremental", commit_identifier=1) + external_entry = _make_entry( + "external.parquet", + first_row_id=100, + row_count=50, + write_cols=["col_a"], + ) + manager = _FakeSnapshotManager([base, external, own]) + scanner = _FakeCommitScanner({2: [external_entry]}) + detection = ConflictDetection( + data_evolution_enabled=True, + snapshot_manager=manager, + manifest_list_manager=None, + table=_FakeTable(_FakeSchemaManager([_DEFAULT_SCHEMA])), + commit_scanner=scanner, + ) + detection.set_row_id_check_from_snapshot(1) + detection.ignore_row_id_commit("incremental", 1) + + self.assertIsNone( + detection.check_row_id_from_snapshot(own, self._blob_delta())) + later_delta = [_make_entry( + "later.blob", + first_row_id=100, + row_count=50, + write_cols=["col_a"], + )] + result = detection.check_row_id_from_snapshot(own, later_delta) + + self.assertIsNotNone(result) + self.assertIn("updating the same file", str(result)) + self.assertEqual([2, 2], scanner.entry_calls) + + def test_reused_snapshot_id_invalidates_cached_history(self): + base = _FakeSnapshot(1, "APPEND", next_row_id=1000) + own = _FakeSnapshot( + 2, "APPEND", next_row_id=1000, + commit_user="incremental", commit_identifier=1, + delta_manifest_list="own-manifest") + manager = _FakeSnapshotManager([base, own]) + scanner = _FakeCommitScanner({}) + detection = ConflictDetection( + data_evolution_enabled=True, + snapshot_manager=manager, + manifest_list_manager=None, + table=_FakeTable(_FakeSchemaManager([_DEFAULT_SCHEMA])), + commit_scanner=scanner, + ) + detection.set_row_id_check_from_snapshot(1) + detection.ignore_row_id_commit("incremental", 1) + + delta = self._blob_delta() + self.assertIsNone(detection.check_row_id_from_snapshot(own, delta)) + + replacement = _FakeSnapshot( + 2, "APPEND", next_row_id=1000, + commit_user="external", commit_identifier=1, + delta_manifest_list="external-manifest") + manager._by_id[2] = replacement + scanner._by_id[2] = [_make_entry( + "external.parquet", + first_row_id=0, + row_count=51, + write_cols=["col_a"], + )] + + result = detection.check_row_id_from_snapshot(replacement, delta) + + self.assertIsNotNone(result) + self.assertIn("updating the same file", str(result)) + self.assertEqual([2], scanner.entry_calls) + + def test_rebuilt_history_past_cursor_invalidates_cache(self): + base = _FakeSnapshot(1, "APPEND", next_row_id=1000) + own = _FakeSnapshot( + 2, "APPEND", next_row_id=1000, + commit_user="incremental", commit_identifier=1, + delta_manifest_list="own-manifest") + manager = _FakeSnapshotManager([base, own]) + scanner = _FakeCommitScanner({}) + detection = ConflictDetection( + data_evolution_enabled=True, + snapshot_manager=manager, + manifest_list_manager=None, + table=_FakeTable(_FakeSchemaManager([_DEFAULT_SCHEMA])), + commit_scanner=scanner, + ) + detection.set_row_id_check_from_snapshot(1) + detection.ignore_row_id_commit("incremental", 1) + + delta = self._blob_delta() + self.assertIsNone(detection.check_row_id_from_snapshot(own, delta)) + + replacement = _FakeSnapshot( + 2, "APPEND", next_row_id=1000, + commit_user="external", commit_identifier=1, + delta_manifest_list="external-manifest") + latest = _FakeSnapshot( + 3, "APPEND", next_row_id=1000, + commit_user="external", commit_identifier=2, + delta_manifest_list="latest-manifest") + manager._by_id.update({2: replacement, 3: latest}) + scanner._by_id[2] = [_make_entry( + "external.parquet", + first_row_id=0, + row_count=51, + write_cols=["col_a"], + )] + + result = detection.check_row_id_from_snapshot(latest, delta) + + self.assertIsNotNone(result) + self.assertIn("updating the same file", str(result)) + self.assertEqual([2], scanner.entry_calls) + class TestRowIdColumnConflictChecker(unittest.TestCase): diff --git a/paimon-python/pypaimon/tests/write/dynamic_bucket_test.py b/paimon-python/pypaimon/tests/write/dynamic_bucket_test.py index 30d2dcaa8580..0909c042e902 100644 --- a/paimon-python/pypaimon/tests/write/dynamic_bucket_test.py +++ b/paimon-python/pypaimon/tests/write/dynamic_bucket_test.py @@ -602,7 +602,7 @@ def test_data_only_upsert_conflicts_after_overwrite_remaps_key(self): stale_writer.close() stale_commit.close() - def test_retry_then_hash_index_conflict_preserves_prepared_files(self): + def test_retry_then_hash_index_conflict_aborts_prepared_files(self): with tempfile.TemporaryDirectory() as root: table = self._create_table(root, 'retry_hash_conflict') writer, commit, messages = self._prepare_indexed_write(table, [1]) @@ -647,7 +647,7 @@ def lose_first_compare_and_set(snapshot, statistics): self.assertEqual(1, calls) self.assertTrue(all( - table.file_io.exists(path) for path in prepared_paths + not table.file_io.exists(path) for path in prepared_paths )) writer.close() commit.close() diff --git a/paimon-python/pypaimon/write/commit/commit_scanner.py b/paimon-python/pypaimon/write/commit/commit_scanner.py index 4612054e6c5c..091edc2e154a 100644 --- a/paimon-python/pypaimon/write/commit/commit_scanner.py +++ b/paimon-python/pypaimon/write/commit/commit_scanner.py @@ -18,6 +18,8 @@ """ Manifest entries scanner for commit operations. """ +import bisect +import os from typing import Optional, List from pypaimon.common.predicate_builder import PredicateBuilder @@ -25,8 +27,129 @@ from pypaimon.manifest.manifest_file_manager import ManifestFileManager from pypaimon.manifest.manifest_list_manager import ManifestListManager from pypaimon.manifest.schema.manifest_entry import ManifestEntry -from pypaimon.read.scanner.file_scanner import FileScanner +from pypaimon.read.scanner.file_scanner import ( + FileScanner, + _filter_manifest_files_by_row_ranges, +) from pypaimon.snapshot.snapshot import Snapshot +from pypaimon.utils.range import Range + + +class _ConflictEntryScope: + """Manifest entries which can interact with one commit window.""" + + def __init__(self, entries, index_entries=None): + ranges = {} + file_names = {} + + for entry in entries or []: + key = self._entry_key(entry) + row_range = entry.file.row_id_range() + if row_range is None: + file_names.setdefault(key, set()).add(entry.file.file_name) + else: + ranges.setdefault(key, []).append(row_range) + + for entry in index_entries or []: + if entry.kind != 0: + continue + key = self._entry_key(entry) + index_file = entry.index_file + if index_file.index_type == IndexManifestFile.DELETION_VECTORS_INDEX: + file_names.setdefault(key, set()).update( + (index_file.dv_ranges or {}).keys()) + global_index = index_file.global_index_meta + if global_index is not None: + ranges.setdefault(key, []).append(Range( + global_index.row_range_start, + global_index.row_range_end, + )) + + self._ranges = { + key: Range.sort_and_merge_overlap(values, True, True) + for key, values in ranges.items() + } + self._range_ends = { + key: [row_range.to for row_range in values] + for key, values in self._ranges.items() + } + self._file_names = file_names + self._buckets = {key[1] for key in set(ranges) | set(file_names)} + + ranges_by_bucket = {} + for (_, bucket), values in self._ranges.items(): + ranges_by_bucket.setdefault(bucket, []).extend(values) + self._ranges_by_bucket = { + bucket: Range.sort_and_merge_overlap(values, True, True) + for bucket, values in ranges_by_bucket.items() + } + self._range_ends_by_bucket = { + bucket: [row_range.to for row_range in values] + for bucket, values in self._ranges_by_bucket.items() + } + + names_by_bucket = {} + for (_, bucket), values in self._file_names.items(): + names_by_bucket.setdefault(bucket, set()).update(values) + self._names_by_bucket = names_by_bucket + + self.row_ranges = Range.sort_and_merge_overlap( + [row_range for values in self._ranges.values() for row_range in values], + True, + True, + ) + + def is_empty(self): + return not self._ranges and not self._file_names + + def can_prune_manifest_files(self): + return bool(self.row_ranges) and not self._file_names + + def matches_bucket(self, bucket, _total_buckets=None): + return bucket in self._buckets + + def matches_record(self, record): + bucket = record.get('_BUCKET') + if bucket not in self._buckets: + return False + file_dict = record.get('_FILE') + if file_dict is None: + return False + if file_dict.get('_FILE_NAME') in self._names_by_bucket.get(bucket, ()): + return True + first_row_id = file_dict.get('_FIRST_ROW_ID') + row_count = file_dict.get('_ROW_COUNT') + if first_row_id is None or row_count is None: + return False + return self._matches_range( + self._ranges_by_bucket.get(bucket, ()), + self._range_ends_by_bucket.get(bucket, ()), + int(first_row_id), + int(first_row_id) + int(row_count) - 1, + ) + + def matches_entry(self, entry): + key = self._entry_key(entry) + if entry.file.file_name in self._file_names.get(key, ()): + return True + row_range = entry.file.row_id_range() + if row_range is None: + return False + return self._matches_range( + self._ranges.get(key, ()), + self._range_ends.get(key, ()), + row_range.from_, + row_range.to, + ) + + @staticmethod + def _matches_range(ranges, range_ends, from_, to): + index = bisect.bisect_left(range_ends, from_) + return index < len(ranges) and ranges[index].from_ <= to + + @staticmethod + def _entry_key(entry): + return tuple(entry.partition.values), entry.bucket class CommitScanner: @@ -45,6 +168,10 @@ def __init__(self, table, manifest_list_manager: ManifestListManager): self.table = table self.manifest_list_manager = manifest_list_manager + @staticmethod + def conflict_entry_scope(commit_entries, index_entries=None): + return _ConflictEntryScope(commit_entries, index_entries) + def read_all_entries_from_changed_partitions(self, latest_snapshot: Optional[Snapshot], commit_entries: List[ManifestEntry], @@ -74,6 +201,37 @@ def read_all_entries_from_changed_partitions(self, self.table, lambda: ([], None), partition_predicate=partition_filter ).read_manifest_entries(all_manifests) + def read_conflict_entries(self, + latest_snapshot: Optional[Snapshot], + commit_entries: List[ManifestEntry], + index_entries=None): + """Read only live entries which can interact with this commit window.""" + if latest_snapshot is None: + return [] + + scope = _ConflictEntryScope(commit_entries, index_entries) + if scope.is_empty(): + return self.read_all_entries_from_changed_partitions( + latest_snapshot, commit_entries, index_entries) + + manifest_files = self.manifest_list_manager.read_all(latest_snapshot) + if scope.can_prune_manifest_files(): + manifest_files = _filter_manifest_files_by_row_ranges( + manifest_files, scope.row_ranges) + + partition_filter = self._build_partition_filter_from_changes( + commit_entries, index_entries) + max_workers = self.table.options.scan_manifest_parallelism( + os.cpu_count() or 8) + return ManifestFileManager(self.table).read_entries_parallel( + manifest_files, + manifest_entry_filter=scope.matches_entry, + max_workers=max_workers, + early_entry_filter=scope.matches_bucket, + early_record_filter=scope.matches_record, + partition_filter=partition_filter, + ) + def read_incremental_entries_from_changed_partitions(self, snapshot: Snapshot, commit_entries: List[ManifestEntry], @@ -121,12 +279,42 @@ def read_incremental_raw_entries_from_changed_partitions(self, snapshot: Snapsho mfm = ManifestFileManager(self.table) entries = [] for mf in delta_manifests: - for entry in mfm.read(mf.file_name): - if partition_filter is not None and not partition_filter.test(entry.partition): + for entry in mfm.read( + mf.file_name, partition_filter=partition_filter): + if (partition_filter is not None + and not partition_filter.test(entry.partition)): continue entries.append(entry) return entries + def read_incremental_raw_entries_for_scope( + self, snapshot, commit_entries, index_entries=None): + delta_manifests = self.manifest_list_manager.read_delta(snapshot) + if not delta_manifests: + return [] + + scope = _ConflictEntryScope(commit_entries, index_entries) + if scope.is_empty(): + return self.read_incremental_raw_entries_from_changed_partitions( + snapshot, commit_entries, index_entries=index_entries) + if scope.can_prune_manifest_files(): + delta_manifests = _filter_manifest_files_by_row_ranges( + delta_manifests, scope.row_ranges) + + partition_filter = self._build_partition_filter_from_changes( + commit_entries, index_entries) + mfm = ManifestFileManager(self.table) + entries = [] + for manifest in delta_manifests: + entries.extend(mfm.read( + manifest.file_name, + manifest_entry_filter=scope.matches_entry, + early_entry_filter=scope.matches_bucket, + early_record_filter=scope.matches_record, + partition_filter=partition_filter, + )) + return entries + def read_incremental_changes(self, from_snapshot: Snapshot, to_snapshot: Snapshot, @@ -134,8 +322,9 @@ def read_incremental_changes(self, index_entries=None) -> Optional[List[ManifestEntry]]: """Delta entries (incl. DELETEs) in ``(from_snapshot, to_snapshot]``, changed-partition filtered, so a retry can reuse the prior base and read - only the changes since. Returns None on a missing snapshot (caller then - full-scans). Mirrors Java ``CommitScanner#readIncrementalChanges``. + only the changes since. Returns None on a missing or OVERWRITE snapshot + (caller then full-scans). An OVERWRITE may replace the base manifest + without fully describing the replacement in its delta manifest. """ snapshot_manager = self.table.snapshot_manager() partition_filter = self._build_partition_filter_from_changes( @@ -143,13 +332,26 @@ def read_incremental_changes(self, entries = [] for snapshot_id in range(from_snapshot.id + 1, to_snapshot.id + 1): snapshot = snapshot_manager.get_snapshot_by_id(snapshot_id) - if snapshot is None: + if snapshot is None or snapshot.commit_kind == "OVERWRITE": return None entries.extend( self.read_incremental_raw_entries_from_changed_partitions( snapshot, commit_entries, partition_filter)) return entries + def changed_partition_signature(self, entries, index_entries=None): + """Return the changed partitions used by conflict scans.""" + if not self.table.partition_keys: + return None + + changed_partitions = set() + for entry in entries or []: + changed_partitions.add(tuple(entry.partition.values)) + for entry in index_entries or []: + if self._index_entry_changes_partition(entry): + changed_partitions.add(tuple(entry.partition.values)) + return frozenset(changed_partitions) or None + def _build_partition_filter_from_entries(self, entries: List[ManifestEntry]): return self._build_partition_filter_from_changes(entries) @@ -169,16 +371,19 @@ def _build_partition_filter_from_changes(self, entries, index_entries=None): if not partition_keys: return None - changed_partitions = set() - for entry in entries or []: - changed_partitions.add(tuple(entry.partition.values)) - for entry in index_entries or []: - if self._index_entry_changes_partition(entry): - changed_partitions.add(tuple(entry.partition.values)) + changed_partitions = self.changed_partition_signature( + entries, index_entries) if not changed_partitions: return None + return self._build_partition_filter_from_values(changed_partitions) + + def _build_partition_filter_from_values(self, changed_partitions): + partition_keys = self.table.partition_keys + if not partition_keys: + return None + predicate_builder = PredicateBuilder(self.table.partition_keys_fields) partition_predicates = [] for partition_values in changed_partitions: diff --git a/paimon-python/pypaimon/write/commit/conflict_detection.py b/paimon-python/pypaimon/write/commit/conflict_detection.py index 3534960c1c39..582b76437dd1 100644 --- a/paimon-python/pypaimon/write/commit/conflict_detection.py +++ b/paimon-python/pypaimon/write/commit/conflict_detection.py @@ -21,13 +21,16 @@ import bisect +from pypaimon.deletionvectors.deletion_vector import DeletionVector from pypaimon.manifest.manifest_list_manager import ManifestListManager from pypaimon.manifest.index_manifest_file import IndexManifestFile from pypaimon.manifest.schema.data_file_meta import DataFileMeta from pypaimon.manifest.schema.file_entry import FileEntry from pypaimon.table.special_fields import SpecialFields +from pypaimon.table.source.deletion_file import DeletionFile from pypaimon.utils.range import Range from pypaimon.utils.range_helper import RangeHelper +from pypaimon.utils.roaring_bitmap import RoaringBitmap from pypaimon.write.commit.commit_scanner import CommitScanner @@ -153,6 +156,10 @@ class CommitConflictError(RuntimeError): """A deterministic pre-snapshot conflict which is safe to abort.""" +class RowIdPlanningConflictError(RuntimeError): + """A row-id conflict detected before snapshot creation.""" + + class ConflictDetection: """Detects conflicts between base and delta files during commit.""" @@ -163,6 +170,16 @@ def __init__(self, data_evolution_enabled, snapshot_manager, self.manifest_list_manager = manifest_list_manager self.table = table self._row_id_check_from_snapshot = None + self._row_id_ignored_commit_high_watermarks = {} + self._row_id_history_base_snapshot = None + self._row_id_history_cursor = None + self._row_id_history_cursor_identity = None + self._row_id_external_snapshots = [] + self._row_id_window_changes = None + self._row_id_overwrite_seen = False + self._row_id_index_overwrite_seen = False + self._row_id_index_manifest_cache = {} + self._bounded_row_id_conflict_state = False self.commit_scanner = commit_scanner def should_be_overwrite_commit(self, append_file_entries=None, append_index_files=None): @@ -177,6 +194,402 @@ def should_be_overwrite_commit(self, append_file_entries=None, append_index_file def has_row_id_check_from_snapshot(self): return self._row_id_check_from_snapshot is not None + def set_row_id_check_from_snapshot(self, snapshot_id): + if self._row_id_check_from_snapshot == snapshot_id: + return + self._row_id_check_from_snapshot = snapshot_id + self.reset_row_id_history() + + def reset_row_id_history(self): + """Clear cached row-id history.""" + self._row_id_history_base_snapshot = None + self._row_id_history_cursor = None + self._row_id_history_cursor_identity = None + self._row_id_external_snapshots = [] + self._row_id_window_changes = None + self._row_id_overwrite_seen = False + self._row_id_index_overwrite_seen = False + self._row_id_index_manifest_cache = {} + + def clear_row_id_window_changes(self): + self._row_id_window_changes = None + + def enable_bounded_row_id_conflict_state(self): + self._bounded_row_id_conflict_state = True + + def ignore_row_id_commit(self, commit_user, commit_identifier): + """Skip a disjoint commit in later checks.""" + current = self._row_id_ignored_commit_high_watermarks.get(commit_user) + if current is None or commit_identifier > current: + self._row_id_ignored_commit_high_watermarks[commit_user] = commit_identifier + + def read_row_id_base_entries(self, latest_snapshot, commit_entries, + index_entries=None, planned_base_entries=None, + planned_base_snapshot_identity=None): + """Read the current entries relevant to one row-id commit window.""" + base_snapshot_id = self._row_id_check_from_snapshot + if base_snapshot_id is None: + return self.commit_scanner.read_conflict_entries( + latest_snapshot, commit_entries, index_entries) + base_snapshot = self.snapshot_manager.get_snapshot_by_id( + base_snapshot_id) + if base_snapshot is None or latest_snapshot.id < base_snapshot_id: + raise RowIdPlanningConflictError( + "Row ID conflict check base snapshot {} is no longer " + "available.".format(base_snapshot_id)) + complete_evidence = ( + planned_base_entries is not None + and planned_base_snapshot_identity is not None + ) + if (complete_evidence + and self._snapshot_identity(base_snapshot) + != planned_base_snapshot_identity): + raise RowIdPlanningConflictError( + "Row ID conflict check base snapshot {} changed.".format( + base_snapshot_id)) + + self._row_id_window_changes = None + if not complete_evidence: + self._row_id_history_snapshots(latest_snapshot) + has_row_id_additions = any( + entry.kind == 0 and entry.file.row_id_range() is not None + for entry in commit_entries + ) + if (self._row_id_index_overwrite_seen + and has_row_id_additions): + raise RowIdPlanningConflictError( + "Cannot validate an index-changing overwrite without " + "planned row ID base files.") + if (self._bounded_row_id_conflict_state + and self._row_id_overwrite_seen): + raise RowIdPlanningConflictError( + "Cannot validate a concurrent overwrite without " + "planned row ID base files.") + return self.commit_scanner.read_conflict_entries( + latest_snapshot, commit_entries, index_entries) + + if self._bounded_row_id_conflict_state: + self._row_id_history_snapshots(latest_snapshot) + self._validate_row_id_deletion_vectors( + base_snapshot, latest_snapshot, planned_base_entries, + commit_entries) + if self._row_id_overwrite_seen: + return self._read_current_row_id_entries( + latest_snapshot, + commit_entries, + index_entries, + planned_base_entries, + ) + + entries = list(planned_base_entries) + changes = self._row_id_changes( + latest_snapshot, commit_entries, index_entries, cache_result=True) + self._validate_row_id_deletion_vectors( + base_snapshot, latest_snapshot, planned_base_entries, + commit_entries) + for snapshot, raw_entries in changes: + if snapshot.commit_kind == "OVERWRITE": + return self.commit_scanner.read_conflict_entries( + latest_snapshot, commit_entries, index_entries) + entries.extend(raw_entries) + try: + return FileEntry.merge_entries(entries) + except RuntimeError as error: + raise RowIdPlanningConflictError(str(error)) from error + + def _read_current_row_id_entries( + self, latest_snapshot, commit_entries, index_entries, + planned_base_entries): + current = self.commit_scanner.read_conflict_entries( + latest_snapshot, commit_entries, index_entries) + scope = CommitScanner.conflict_entry_scope( + commit_entries, index_entries) + planned = ( + list(planned_base_entries) + if scope.is_empty() + else [ + entry for entry in planned_base_entries + if scope.matches_entry(entry) + ] + ) + if (self._row_id_entry_signatures(planned) + != self._row_id_entry_signatures(current)): + raise RowIdPlanningConflictError( + "Concurrent overwrite changed row ID files for the current " + "update window.") + + self._row_id_external_snapshots = [] + self._row_id_window_changes = ( + self._row_id_change_key(latest_snapshot, commit_entries), []) + return current + + @staticmethod + def _row_id_entry_signatures(entries): + return { + ( + entry.identifier(), + entry.file.first_row_id, + entry.file.row_count, + entry.file.schema_id, + tuple(entry.file.write_cols) + if entry.file.write_cols is not None else None, + ) + for entry in entries + if entry.kind == 0 + } + + def _row_id_changes(self, latest_snapshot, commit_entries, + index_entries=None, cache_result=False): + key = self._row_id_change_key(latest_snapshot, commit_entries) + cached = self._row_id_window_changes + if cached is not None and cached[0] == key: + return cached[1] + + def changes(): + for snapshot in self._row_id_history_snapshots(latest_snapshot): + yield ( + snapshot, + self.commit_scanner.read_incremental_raw_entries_for_scope( + snapshot, commit_entries, index_entries), + ) + + if not cache_result: + return changes() + result = list(changes()) + self._row_id_window_changes = (key, result) + return result + + def _row_id_change_key(self, latest_snapshot, commit_entries): + return ( + self._snapshot_identity(latest_snapshot), + tuple( + ( + tuple(entry.partition.values), + entry.bucket, + entry.kind, + entry.file.file_name, + entry.file.first_row_id, + entry.file.row_count, + ) + for entry in commit_entries + ), + ) + + def _row_id_history_snapshots(self, latest_snapshot): + """Cache history except disjoint commits.""" + base_snapshot = self._row_id_check_from_snapshot + reset_history = ( + self._row_id_history_base_snapshot != base_snapshot + or self._row_id_history_cursor is None + or latest_snapshot.id < self._row_id_history_cursor + ) + if not reset_history: + cursor_snapshot = ( + latest_snapshot + if latest_snapshot.id == self._row_id_history_cursor + else self.snapshot_manager.get_snapshot_by_id( + self._row_id_history_cursor) + ) + reset_history = ( + self._snapshot_identity(cursor_snapshot) + != self._row_id_history_cursor_identity + ) + + if reset_history: + self._row_id_history_base_snapshot = base_snapshot + self._row_id_history_cursor = base_snapshot + self._row_id_history_cursor_identity = None + self._row_id_external_snapshots = [] + self._row_id_overwrite_seen = False + self._row_id_index_overwrite_seen = False + self._row_id_index_manifest_cache = {} + + # Snapshot files below the cursor are immutable. A rollback replaces the + # whole tail, including the cursor, whose identity check above resets cache. + for snapshot_id in range( + self._row_id_history_cursor + 1, + latest_snapshot.id + 1): + snapshot = self.snapshot_manager.get_snapshot_by_id(snapshot_id) + if snapshot is None: + raise RowIdPlanningConflictError( + "Row ID conflict check snapshot {} is no longer " + "available.".format(snapshot_id)) + commit_user = getattr(snapshot, "commit_user", None) + commit_identifier = getattr(snapshot, "commit_identifier", None) + ignored_through = self._row_id_ignored_commit_high_watermarks.get( + commit_user) + if (ignored_through is None + or commit_identifier is None + or commit_identifier > ignored_through): + if (snapshot.commit_kind == "OVERWRITE" + and self._index_manifest_changed(snapshot)): + self._row_id_index_overwrite_seen = True + if self._bounded_row_id_conflict_state: + if snapshot.commit_kind == "OVERWRITE": + self._row_id_overwrite_seen = True + else: + raise RowIdPlanningConflictError( + "Concurrent commit detected during incremental " + "update_by_row_id.") + else: + self._row_id_external_snapshots.append(snapshot) + + self._row_id_history_cursor = latest_snapshot.id + self._row_id_history_cursor_identity = self._snapshot_identity( + latest_snapshot) + return self._row_id_external_snapshots + + def _index_manifest_changed(self, snapshot): + previous = self.snapshot_manager.get_snapshot_by_id(snapshot.id - 1) + if previous is None: + return True + return (getattr(previous, "index_manifest", None) + != getattr(snapshot, "index_manifest", None)) + + def _validate_row_id_deletion_vectors( + self, base_snapshot, latest_snapshot, base_entries, + commit_entries): + if (self._row_id_index_overwrite_seen + and self._row_id_deletion_vectors_changed( + base_snapshot, latest_snapshot, base_entries, + commit_entries)): + raise RowIdPlanningConflictError( + "Concurrent overwrite changed deletion vectors for the " + "current update window.") + + def _row_id_deletion_vectors_changed( + self, base_snapshot, latest_snapshot, base_entries, + commit_entries): + targets = self._row_id_deletion_vector_targets( + base_entries, commit_entries) + if not targets: + return any( + entry.kind == 0 and entry.file.row_id_range() is not None + for entry in commit_entries + ) + + base_files = self._deletion_vector_files(base_snapshot) + latest_files = self._deletion_vector_files(latest_snapshot) + manifest_names = { + getattr(snapshot, "index_manifest", None) + for snapshot in (base_snapshot, latest_snapshot) + } + self._row_id_index_manifest_cache = { + name: self._row_id_index_manifest_cache[name] + for name in manifest_names + if name in self._row_id_index_manifest_cache + } + + for target, ranges in targets.items(): + base_file = base_files.get(target) + latest_file = latest_files.get(target) + if base_file == latest_file: + continue + scope = RoaringBitmap() + for range_ in ranges: + scope.add_range(range_.from_, range_.to) + base_bitmap = ( + RoaringBitmap() + if base_file is None + else DeletionVector.read( + self.table.file_io, base_file).bit_map() + ) + latest_bitmap = ( + RoaringBitmap() + if latest_file is None + else DeletionVector.read( + self.table.file_io, latest_file).bit_map() + ) + if (RoaringBitmap.and_(base_bitmap, scope) + != RoaringBitmap.and_(latest_bitmap, scope)): + return True + return False + + def _row_id_deletion_vector_targets( + self, base_entries, commit_entries): + update_ranges = {} + for entry in commit_entries: + row_range = entry.file.row_id_range() + if entry.kind != 0 or row_range is None: + continue + key = (tuple(entry.partition.values), entry.bucket) + update_ranges.setdefault(key, []).append(row_range) + update_ranges = { + key: Range.sort_and_merge_overlap(ranges, True, True) + for key, ranges in update_ranges.items() + } + + targets = {} + for entry in base_entries: + if (entry.kind != 0 + or self._is_dedicated_file(entry.file.file_name)): + continue + base_range = entry.file.row_id_range() + if base_range is None: + continue + key = (tuple(entry.partition.values), entry.bucket) + for update_range in update_ranges.get(key, ()): + if not base_range.overlaps(update_range): + continue + target = key + (entry.file.file_name,) + targets.setdefault(target, []).append(Range( + max(base_range.from_, update_range.from_) + - base_range.from_, + min(base_range.to, update_range.to) + - base_range.from_, + )) + return { + key: Range.sort_and_merge_overlap(ranges, True, True) + for key, ranges in targets.items() + } + + def _deletion_vector_files(self, snapshot): + manifest_name = getattr(snapshot, "index_manifest", None) + if manifest_name is None: + return {} + if manifest_name in self._row_id_index_manifest_cache: + return self._row_id_index_manifest_cache[manifest_name] + + index_path = self.table.path_factory().index_path() + result = {} + for entry in IndexManifestFile(self.table).read(manifest_name): + index_file = entry.index_file + if (entry.kind != 0 + or index_file.index_type + != IndexManifestFile.DELETION_VECTORS_INDEX): + continue + path = index_file.external_path or ( + f"{index_path}/{index_file.file_name}") + for data_file_name, meta in (index_file.dv_ranges or {}).items(): + result[( + tuple(entry.partition.values), + entry.bucket, + data_file_name, + )] = DeletionFile( + dv_index_path=path, + offset=meta.offset, + length=meta.length, + cardinality=meta.cardinality, + ) + self._row_id_index_manifest_cache[manifest_name] = result + return result + + @staticmethod + def _snapshot_identity(snapshot): + if snapshot is None: + return None + return ( + snapshot.id, + getattr(snapshot, "commit_user", None), + getattr(snapshot, "commit_identifier", None), + getattr(snapshot, "commit_kind", None), + getattr(snapshot, "time_millis", None), + getattr(snapshot, "base_manifest_list", None), + getattr(snapshot, "delta_manifest_list", None), + getattr(snapshot, "changelog_manifest_list", None), + getattr(snapshot, "index_manifest", None), + ) + @staticmethod def has_global_index_additions(index_entries=None): return bool(ConflictDetection.global_index_file_additions(index_entries)) @@ -490,13 +903,15 @@ def check_row_id_existence(self, base_entries, delta_entries, next_row_id=None): existing_index = set() existing_ranges = {} for base in base_entries: - if base.file.first_row_id is not None: - existing_index.add(( - base.partition, base.bucket, - base.file.first_row_id, base.file.row_count)) - if not self._is_dedicated_file(base.file.file_name): - existing_ranges.setdefault((base.partition, base.bucket), []).append( - base.file.row_id_range()) + if (base.kind != 0 + or base.file.first_row_id is None + or self._is_dedicated_file(base.file.file_name)): + continue + existing_index.add(( + base.partition, base.bucket, + base.file.first_row_id, base.file.row_count)) + existing_ranges.setdefault((base.partition, base.bucket), []).append( + base.file.row_id_range()) existing_ranges = { key: Range.sort_and_merge_overlap(ranges, True, True) @@ -662,40 +1077,38 @@ def check_row_id_from_snapshot(self, latest_snapshot, commit_entries): r = f.row_id_range() if r is not None: delta_signatures.append( - (DataFileMeta.is_blob_file(f.file_name), r.from_, r.to)) - - for snapshot_id in range( - self._row_id_check_from_snapshot + 1, - latest_snapshot.id + 1): - snapshot = self.snapshot_manager.get_snapshot_by_id(snapshot_id) - if snapshot is None: - continue - - if snapshot.commit_kind == "COMPACT": - err = self._compact_conflicts_with_delta( - snapshot, delta_signatures, column_checker, commit_entries) - if err is not None: - return err - continue + (self._file_kind(f.file_name), r.from_, r.to)) - incremental_entries = self.commit_scanner.read_incremental_entries_from_changed_partitions( - snapshot, commit_entries) - for entry in incremental_entries: - file_range = entry.file.row_id_range() - if file_range is None: + try: + for snapshot, raw_entries in self._row_id_changes( + latest_snapshot, commit_entries): + if snapshot.commit_kind == "COMPACT": + err = self._compact_conflicts_with_delta( + snapshot, delta_signatures, column_checker, raw_entries) + if err is not None: + return err continue - if file_range.from_ < check_next_row_id: - if column_checker.conflicts_with(entry.file): + + for entry in FileEntry.merge_entries(raw_entries): + if entry.kind != 0: + continue + file_range = entry.file.row_id_range() + if file_range is None: + continue + if (file_range.from_ < check_next_row_id + and column_checker.conflicts_with(entry.file)): return RuntimeError( "For Data Evolution table, multiple 'MERGE INTO' " "operations have encountered conflicts, updating " "the same file, which can render some updates " "ineffective.") - return None + return None + finally: + self._row_id_window_changes = None def _compact_conflicts_with_delta(self, snapshot, delta_signatures, - column_checker, commit_entries): + column_checker, raw_entries): """Return RuntimeError if a COMPACT snapshot deleted a same-kind anchor file whose row-id range AND write columns overlap any staged delta; otherwise None. @@ -708,17 +1121,15 @@ def _compact_conflicts_with_delta(self, snapshot, delta_signatures, """ if not delta_signatures: return None - raw_entries = self.commit_scanner.read_incremental_raw_entries_from_changed_partitions( - snapshot, commit_entries) for entry in raw_entries: if entry.kind != 1: continue file_range = entry.file.row_id_range() if file_range is None: continue - deleted_is_blob = DataFileMeta.is_blob_file(entry.file.file_name) - for delta_is_blob, from_, to in delta_signatures: - if delta_is_blob != deleted_is_blob: + deleted_kind = self._file_kind(entry.file.file_name) + for delta_kind, from_, to in delta_signatures: + if delta_kind != deleted_kind: continue if file_range.from_ > to or from_ > file_range.to: continue @@ -736,3 +1147,11 @@ def _compact_conflicts_with_delta(self, snapshot, delta_signatures, df=from_, dt=to)) return None + + @staticmethod + def _file_kind(file_name): + if DataFileMeta.is_blob_file(file_name): + return "blob" + if DataFileMeta.is_vector_file(file_name): + return "vector" + return "normal" diff --git a/paimon-python/pypaimon/write/commit_message.py b/paimon-python/pypaimon/write/commit_message.py index 3076b014b16d..643060a0a0f7 100644 --- a/paimon-python/pypaimon/write/commit_message.py +++ b/paimon-python/pypaimon/write/commit_message.py @@ -35,6 +35,8 @@ class CommitMessage: index_deletes: List['IndexManifestEntry'] = field(default_factory=list) changelog_files: List[DataFileMeta] = field(default_factory=list) hash_index_base_snapshot: Optional[int] = None + row_id_base_files: List[DataFileMeta] = field(default_factory=list) + row_id_base_snapshot_identity: Optional[Tuple] = None def is_empty(self): return ( diff --git a/paimon-python/pypaimon/write/file_store_commit.py b/paimon-python/pypaimon/write/file_store_commit.py index 801771bf14ac..a15e2397ebab 100644 --- a/paimon-python/pypaimon/write/file_store_commit.py +++ b/paimon-python/pypaimon/write/file_store_commit.py @@ -21,6 +21,17 @@ import uuid from typing import Dict, List, Optional +from pypaimon.api.rest_exception import ( + BadRequestException, + ForbiddenException, + NoSuchResourceException, + NotAuthorizedException, + NotImplementedException, +) +from pypaimon.catalog.catalog_exception import ( + TableNoPermissionException, + TableNotExistException, +) from pypaimon.common.options.core_options import CoreOptions from pypaimon.common.predicate_builder import PredicateBuilder from pypaimon.manifest.manifest_file_manager import ManifestFileManager @@ -41,6 +52,7 @@ from pypaimon.write.commit.conflict_detection import ( CommitConflictError, ConflictDetection, + RowIdPlanningConflictError, ) from pypaimon.write.commit.overwrite_changes_provider import OverwriteChangesProvider from pypaimon.table.special_fields import SpecialFields @@ -50,6 +62,36 @@ logger = logging.getLogger(__name__) +_DETERMINISTIC_REST_COMMIT_EXCEPTIONS = ( + BadRequestException, + NotAuthorizedException, + ForbiddenException, + NoSuchResourceException, + NotImplementedException, +) + +_DETERMINISTIC_CATALOG_COMMIT_EXCEPTIONS = ( + TableNotExistException, + TableNoPermissionException, +) + + +def _is_deterministic_atomic_commit_failure(error: Exception) -> bool: + """Whether ``error`` proves that the atomic commit was rejected.""" + current = error + seen = set() + while current is not None and id(current) not in seen: + seen.add(id(current)) + if isinstance( + current, + _DETERMINISTIC_CATALOG_COMMIT_EXCEPTIONS + + (NotImplementedError,) + + _DETERMINISTIC_REST_COMMIT_EXCEPTIONS): + return True + current = current.__cause__ + return False + + class CommitResult: """Base class for commit results.""" @@ -58,6 +100,10 @@ def is_success(self) -> bool: raise NotImplementedError +class CommitOutcomeUnknownError(RuntimeError): + """The commit may already be visible.""" + + class SuccessResult(CommitResult): """Result indicating successful commit.""" @@ -68,9 +114,11 @@ def is_success(self) -> bool: class RetryResult(CommitResult): def __init__(self, latest_snapshot, exception: Optional[Exception] = None, - base_data_files: Optional[List[ManifestEntry]] = None): + base_data_files: Optional[List[ManifestEntry]] = None, + outcome_unknown: bool = False): self.latest_snapshot = latest_snapshot self.exception = exception + self.outcome_unknown = outcome_unknown # Base entries as of latest_snapshot, carried so the next attempt reuses # them and reads only the incremental changes. self.base_data_files = base_data_files @@ -135,7 +183,9 @@ def commit(self, commit_messages: List[CommitMessage], commit_identifier: int): valid_snapshots = [msg.check_from_snapshot for msg in commit_messages if msg.check_from_snapshot != -1] if valid_snapshots: - self.conflict_detection._row_id_check_from_snapshot = min(valid_snapshots) + self.conflict_detection.set_row_id_check_from_snapshot( + min(valid_snapshots) + ) logger.info( "Ready to commit to table %s, number of commit messages: %d", @@ -143,6 +193,8 @@ def commit(self, commit_messages: List[CommitMessage], commit_identifier: int): len(commit_messages), ) commit_entries = self._collect_manifest_entries(commit_messages) + row_id_base_entries, row_id_base_snapshot_identity = ( + self._collect_row_id_base_entries(commit_messages)) changelog_entries = self._collect_changelog_entries(commit_messages) logger.info("Finished collecting changes, including: %d entries, %d changelog entries", @@ -203,7 +255,10 @@ def commit(self, commit_messages: List[CommitMessage], commit_identifier: int): allow_rollback=allow_rollback, index_deletes=index_deletes, index_adds=index_adds, - hash_index_base_snapshot=hash_index_base_snapshot) + hash_index_base_snapshot=hash_index_base_snapshot, + row_id_base_entries=row_id_base_entries, + row_id_base_snapshot_identity=( + row_id_base_snapshot_identity)) def overwrite(self, overwrite_partition, commit_messages: List[CommitMessage], commit_identifier: int): """Commit the given commit messages in overwrite mode.""" @@ -356,78 +411,121 @@ def truncate_table(self, commit_identifier: int) -> None: def _try_commit(self, commit_kind, commit_identifier, commit_entries_plan, detect_conflicts=False, allow_rollback=False, index_deletes=None, index_adds=None, changelog_entries=None, - hash_index_base_snapshot=None): + hash_index_base_snapshot=None, + row_id_base_entries=None, + row_id_base_snapshot_identity=None): retry_count = 0 retry_result = None + outcome_unknown = False + outcome_unknown_cause = None start_time_ms = int(time.time() * 1000) while True: - latest_snapshot = self.snapshot_manager.get_latest_snapshot() - commit_entries = commit_entries_plan(latest_snapshot) - - # No entries to commit (e.g. drop_partitions with no matching data): skip commit - # to avoid creating manifest/snapshot with empty partition_stats (causes read errors). - if not commit_entries and not index_deletes and not index_adds: - break - - result = self._try_commit_once( - retry_result=retry_result, - commit_kind=commit_kind, - commit_entries=commit_entries, - changelog_entries=changelog_entries or [], - commit_identifier=commit_identifier, - latest_snapshot=latest_snapshot, - detect_conflicts=detect_conflicts, - allow_rollback=allow_rollback, - index_deletes=index_deletes, - index_adds=index_adds, - hash_index_base_snapshot=hash_index_base_snapshot, - ) + try: + latest_snapshot = self.snapshot_manager.get_latest_snapshot() + # A successful attempt whose response was lost can make an + # overwrite/truncate retry plan empty. Resolve it before replanning. + if (outcome_unknown + and self._is_duplicate_commit( + retry_result, + latest_snapshot, + commit_identifier, + commit_kind)): + break + + commit_entries = commit_entries_plan(latest_snapshot) + + if not commit_entries and not index_deletes and not index_adds: + if outcome_unknown: + raise CommitOutcomeUnknownError( + "Atomic commit outcome is unknown." + ) from outcome_unknown_cause + break + + result = self._try_commit_once( + retry_result=retry_result, + commit_kind=commit_kind, + commit_entries=commit_entries, + changelog_entries=changelog_entries or [], + commit_identifier=commit_identifier, + latest_snapshot=latest_snapshot, + detect_conflicts=detect_conflicts, + allow_rollback=allow_rollback, + index_deletes=index_deletes, + index_adds=index_adds, + hash_index_base_snapshot=hash_index_base_snapshot, + row_id_base_entries=row_id_base_entries, + row_id_base_snapshot_identity=( + row_id_base_snapshot_identity), + previous_outcome_unknown=outcome_unknown, + ) - if result.is_success(): - commit_duration_ms = int(time.time() * 1000) - start_time_ms - if commit_kind == "OVERWRITE": - logger.info( - "Finished overwrite to table %s, duration %d ms", - self.table.identifier, - commit_duration_ms, + if result.is_success(): + commit_duration_ms = int(time.time() * 1000) - start_time_ms + if commit_kind == "OVERWRITE": + logger.info( + "Finished overwrite to table %s, duration %d ms", + self.table.identifier, + commit_duration_ms, + ) + else: + logger.info( + "Finished commit to table %s, duration %d ms", + self.table.identifier, + commit_duration_ms, + ) + break + + retry_result = result + outcome_unknown = outcome_unknown or result.outcome_unknown + if result.outcome_unknown and outcome_unknown_cause is None: + outcome_unknown_cause = result.exception + + elapsed_ms = int(time.time() * 1000) - start_time_ms + if (elapsed_ms > self.commit_timeout + or retry_count >= self.commit_max_retries): + if commit_kind == "OVERWRITE": + logger.info( + "Finished (Uncertain of success) overwrite to " + "table %s, duration %d ms", + self.table.identifier, + elapsed_ms, + ) + else: + logger.info( + "Finished (Uncertain of success) commit to table " + "%s, duration %d ms", + self.table.identifier, + elapsed_ms, + ) + error_msg = ( + f"Commit failed " + f"{latest_snapshot.id + 1 if latest_snapshot else 1} " + f"after {elapsed_ms} millis with {retry_count} retries, " + "there maybe exist commit conflicts between multiple jobs." ) - else: - logger.info( - "Finished commit to table %s, duration %d ms", - self.table.identifier, - commit_duration_ms, + error_type = ( + CommitOutcomeUnknownError + if outcome_unknown else RuntimeError ) - break - - retry_result = result - - elapsed_ms = int(time.time() * 1000) - start_time_ms - if elapsed_ms > self.commit_timeout or retry_count >= self.commit_max_retries: - if commit_kind == "OVERWRITE": - logger.info( - "Finished (Uncertain of success) overwrite to table %s, duration %d ms", - self.table.identifier, - elapsed_ms, + error_cause = ( + outcome_unknown_cause + if outcome_unknown else retry_result.exception ) - else: - logger.info( - "Finished (Uncertain of success) commit to table %s, duration %d ms", - self.table.identifier, - elapsed_ms, + if error_cause: + raise error_type(error_msg) from error_cause + raise error_type(error_msg) + + self._commit_retry_wait(retry_count) + retry_count += 1 + except CommitOutcomeUnknownError: + raise + except Exception as error: + if outcome_unknown: + raise CommitOutcomeUnknownError(str(error)) from ( + outcome_unknown_cause or error ) - error_msg = ( - f"Commit failed {latest_snapshot.id + 1 if latest_snapshot else 1} " - f"after {elapsed_ms} millis with {retry_count} retries, " - f"there maybe exist commit conflicts between multiple jobs." - ) - if retry_result.exception: - raise RuntimeError(error_msg) from retry_result.exception - else: - raise RuntimeError(error_msg) - - self._commit_retry_wait(retry_count) - retry_count += 1 + raise def _try_commit_once(self, retry_result: Optional[RetryResult], commit_kind: str, commit_entries: List[ManifestEntry], @@ -438,8 +536,14 @@ def _try_commit_once(self, retry_result: Optional[RetryResult], commit_kind: str allow_rollback: bool = False, index_deletes=None, index_adds=None, - hash_index_base_snapshot=None) -> CommitResult: + hash_index_base_snapshot=None, + row_id_base_entries=None, + row_id_base_snapshot_identity=None, + previous_outcome_unknown=None) -> CommitResult: start_millis = int(time.time() * 1000) + if previous_outcome_unknown is None: + previous_outcome_unknown = bool( + retry_result is not None and retry_result.outcome_unknown) if self._is_duplicate_commit(retry_result, latest_snapshot, commit_identifier, commit_kind): return SuccessResult() @@ -454,7 +558,7 @@ def _try_commit_once(self, retry_result: Optional[RetryResult], commit_kind: str hash_index_base_snapshot, latest_snapshot_id ) ) - if retry_result is None: + if not previous_outcome_unknown: raise CommitConflictError(str(conflict)) from conflict raise conflict @@ -469,48 +573,72 @@ def _try_commit_once(self, retry_result: Optional[RetryResult], commit_kind: str new_snapshot_id = latest_snapshot.id + 1 if latest_snapshot else 1 index_entries = (index_deletes or []) + (index_adds or []) - # Base entries for conflict detection. On retry, reuse the previous - # attempt's base + read only the incremental changes (mirrors Java). + # Reuse base entries and apply only new snapshot deltas. base_data_files = None if detect_conflicts: - incremental = None if (latest_snapshot is not None - and retry_result is not None - and retry_result.latest_snapshot is not None - and retry_result.base_data_files is not None): - incremental = self.commit_scanner.read_incremental_changes( - retry_result.latest_snapshot, - latest_snapshot, - commit_entries, - index_entries) - if incremental is not None: - base_data_files = list(retry_result.base_data_files) - if incremental: - base_data_files.extend(incremental) - base_data_files = FileEntry.merge_entries(base_data_files) - elif latest_snapshot is not None: - # First attempt, or incremental could not be built (missing - # snapshot): scan the changed partitions in full. - base_data_files = self.commit_scanner.read_all_entries_from_changed_partitions( - latest_snapshot, commit_entries, index_entries) + and self.conflict_detection.has_row_id_check_from_snapshot()): + try: + base_data_files = ( + self.conflict_detection.read_row_id_base_entries( + latest_snapshot, + commit_entries, + index_entries, + row_id_base_entries, + row_id_base_snapshot_identity, + ) + ) + except RowIdPlanningConflictError as error: + self.conflict_detection.clear_row_id_window_changes() + if not previous_outcome_unknown: + raise CommitConflictError(str(error)) from error + raise + except Exception: + self.conflict_detection.clear_row_id_window_changes() + raise else: - base_data_files = [] + incremental = None + if (latest_snapshot is not None + and retry_result is not None + and retry_result.latest_snapshot is not None + and retry_result.base_data_files is not None): + incremental = self.commit_scanner.read_incremental_changes( + retry_result.latest_snapshot, + latest_snapshot, + commit_entries, + index_entries) + if incremental is not None: + base_data_files = list(retry_result.base_data_files) + if incremental: + base_data_files.extend(incremental) + base_data_files = FileEntry.merge_entries(base_data_files) + elif latest_snapshot is not None: + base_data_files = ( + self.commit_scanner.read_all_entries_from_changed_partitions( + latest_snapshot, commit_entries, index_entries) + ) + else: + base_data_files = [] - conflict_exception = self.conflict_detection.check_conflicts( - latest_snapshot, - base_data_files, - commit_entries, - commit_kind, - index_entries, - ) + try: + conflict_exception = self.conflict_detection.check_conflicts( + latest_snapshot, + base_data_files, + commit_entries, + commit_kind, + index_entries, + ) + finally: + self.conflict_detection.clear_row_id_window_changes() if conflict_exception is not None: if allow_rollback and self.rollback is not None: if self.rollback.try_to_rollback(latest_snapshot): # Rolled back: base/snapshot no longer valid; next attempt # re-scans from scratch (matches Java RollbackRetryResult). + self.conflict_detection.reset_row_id_history() return RetryResult(None, conflict_exception) - if retry_result is None: + if not previous_outcome_unknown: raise CommitConflictError( str(conflict_exception) ) from conflict_exception @@ -612,10 +740,12 @@ def _try_commit_once(self, retry_result: Optional[RetryResult], commit_kind: str logger.warning(f"Exception occurs when preparing snapshot: {e}", exc_info=True) raise RuntimeError(f"Failed to prepare snapshot: {e}") - # Use SnapshotCommit for atomic commit + # A failure after commit() returned is still outcome-unknown. + atomic_call_returned = False try: with self.snapshot_commit: success = self.snapshot_commit.commit(snapshot_data, statistics) + atomic_call_returned = True if not success: commit_time_s = (int(time.time() * 1000) - start_millis) / 1000 logger.warning( @@ -629,9 +759,21 @@ def _try_commit_once(self, retry_result: Optional[RetryResult], commit_kind: str ) return RetryResult(latest_snapshot, None, base_data_files=base_data_files) except Exception as e: + if (not atomic_call_returned + and _is_deterministic_atomic_commit_failure(e)): + logger.warning( + "Atomic commit was rejected deterministically; do not retry.", + exc_info=True, + ) + raise # Commit exception, not sure about the situation and should not clean up the files logger.warning("Retry commit for exception.", exc_info=True) - return RetryResult(latest_snapshot, e, base_data_files=base_data_files) + return RetryResult( + latest_snapshot, + e, + base_data_files=base_data_files, + outcome_unknown=True, + ) logger.info( "Successfully commit snapshot %d to table %s by user %s " @@ -649,8 +791,13 @@ def _try_commit_once(self, retry_result: Optional[RetryResult], commit_kind: str commit_entries=commit_entries, identifier=commit_identifier, ) - for callback in self.commit_callbacks: - callback.call(context) + try: + for callback in self.commit_callbacks: + callback.call(context) + except Exception as error: + raise CommitOutcomeUnknownError( + "Snapshot committed, but a commit callback failed." + ) from error return SuccessResult() @@ -775,6 +922,43 @@ def _collect_manifest_entries(self, commit_messages: List[CommitMessage]) -> Lis )) return commit_entries + def _collect_row_id_base_entries(self, commit_messages): + messages = [ + msg for msg in commit_messages + if msg.check_from_snapshot != -1 + ] + if (not messages + or any(not getattr(msg, "row_id_base_files", None) + or getattr( + msg, "row_id_base_snapshot_identity", None) is None + for msg in messages)): + return None, None + + snapshots = { + msg.row_id_base_snapshot_identity for msg in messages + } + check_snapshots = {msg.check_from_snapshot for msg in messages} + if len(snapshots) != 1 or len(check_snapshots) != 1: + return None, None + snapshot = next(iter(snapshots)) + if not snapshot or snapshot[0] != next(iter(check_snapshots)): + return None, None + + entries = {} + for msg in messages: + partition = GenericRow( + list(msg.partition), self.table.partition_keys_fields) + for file in msg.row_id_base_files: + entry = ManifestEntry( + kind=0, + partition=partition, + bucket=msg.bucket, + total_buckets=self.table.total_buckets, + file=file, + ) + entries[entry.identifier()] = entry + return list(entries.values()), snapshot + def _clean_up_reuse_tmp_manifests( self, delta_manifest_list: Optional[str], diff --git a/paimon-python/pypaimon/write/table_commit.py b/paimon-python/pypaimon/write/table_commit.py index 2f4d8e60a0ce..26e6835264c5 100644 --- a/paimon-python/pypaimon/write/table_commit.py +++ b/paimon-python/pypaimon/write/table_commit.py @@ -106,6 +106,15 @@ def _commit(self, commit_messages: List[CommitMessage], commit_identifier: int = def abort(self, commit_messages: List[CommitMessage]): self.file_store_commit.abort(commit_messages) + def ignore_row_id_conflict_for_commit(self, commit_identifier: int) -> None: + """Skip a disjoint commit in later row-id checks.""" + self.file_store_commit.conflict_detection.ignore_row_id_commit( + self.commit_user, commit_identifier + ) + + def enable_bounded_row_id_conflict_state(self) -> None: + self.file_store_commit.conflict_detection.enable_bounded_row_id_conflict_state() + def close(self): self.file_store_commit.close() diff --git a/paimon-python/pypaimon/write/table_update_by_row_id.py b/paimon-python/pypaimon/write/table_update_by_row_id.py index 8963bf4abcdc..749f99710ed0 100644 --- a/paimon-python/pypaimon/write/table_update_by_row_id.py +++ b/paimon-python/pypaimon/write/table_update_by_row_id.py @@ -60,6 +60,7 @@ class _FilesInfo: field(default_factory=dict) ) valid_row_id_ranges: List[Range] = field(default_factory=list) + snapshot_identity: Optional[Tuple] = None class TableUpdateByRowId: @@ -93,6 +94,7 @@ def __init__( self.first_row_ids = info.first_row_ids self._first_row_id_index = info.first_row_id_index self.valid_row_id_ranges = info.valid_row_id_ranges + self._base_snapshot_identity = info.snapshot_identity self.commit_messages: List[CommitMessage] = [] @@ -103,6 +105,7 @@ def _snapshot_files_info(self) -> _FilesInfo: first_row_ids=self.first_row_ids, first_row_id_index=self._first_row_id_index, valid_row_id_ranges=self.valid_row_id_ranges, + snapshot_identity=self._base_snapshot_identity, ) def _load_existing_files_info(self) -> _FilesInfo: @@ -125,10 +128,11 @@ def _load_existing_files_info(self) -> _FilesInfo: ] data_files = [ file for file in files_with_row_id - if not DataFileMeta.is_blob_file(file.file_name) + if not self._is_dedicated_file(file.file_name) ] for file in split.files: - if file.first_row_id is None or DataFileMeta.is_blob_file(file.file_name): + if (file.first_row_id is None + or self._is_dedicated_file(file.file_name)): continue row_id_ranges.append(file.row_id_range()) for file in data_files: @@ -156,17 +160,42 @@ def _load_existing_files_info(self) -> _FilesInfo: merged = [] snapshot_id = plan.snapshot_id if plan.snapshot_id is not None else -1 + snapshot = scan.file_scanner.scanned_snapshot + if snapshot is not None and snapshot.id != snapshot_id: + raise RuntimeError("Planned snapshot changed during row-id routing.") return _FilesInfo( snapshot_id=snapshot_id, first_row_ids=sorted(index.keys()), first_row_id_index=index, valid_row_id_ranges=merged, + snapshot_identity=self._snapshot_fingerprint(snapshot), ) @staticmethod def _overlaps(left: Range, right: Range) -> bool: return left.from_ <= right.to and right.from_ <= left.to + @staticmethod + def _is_dedicated_file(file_name: str) -> bool: + return (DataFileMeta.is_blob_file(file_name) + or DataFileMeta.is_vector_file(file_name)) + + @staticmethod + def _snapshot_fingerprint(snapshot) -> Optional[Tuple]: + if snapshot is None: + return None + return ( + snapshot.id, + getattr(snapshot, "commit_user", None), + getattr(snapshot, "commit_identifier", None), + getattr(snapshot, "commit_kind", None), + getattr(snapshot, "time_millis", None), + getattr(snapshot, "base_manifest_list", None), + getattr(snapshot, "delta_manifest_list", None), + getattr(snapshot, "changelog_manifest_list", None), + getattr(snapshot, "index_manifest", None), + ) + def update_columns(self, data: pa.Table, column_names: List[str]) -> List[CommitMessage]: """ Add or update columns in the table. @@ -546,6 +575,10 @@ def _write_group( Reads the original file data, merges in the update values, and writes a single output file (rolling disabled) for the group. """ + entry = self._first_row_id_index.get(first_row_id) + if entry is None: + raise ValueError(f"No file found for first_row_id {first_row_id}") + base_files = entry[1] original_data = self._read_original_file_data(first_row_id, column_names) merged_data, blob_columns = self._merge_update_with_original( original_data, @@ -595,6 +628,9 @@ def _write_group( bucket=0, new_files=new_files, check_from_snapshot=self.snapshot_id, + row_id_base_files=list(base_files), + row_id_base_snapshot_identity=( + self._base_snapshot_identity), ) ) success = True