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..bded546e356d 100644 --- a/paimon-python/pypaimon/ray/data_evolution_merge_into.py +++ b/paimon-python/pypaimon/ray/data_evolution_merge_into.py @@ -35,6 +35,7 @@ distributed_delete_apply, distributed_update_apply, distributed_write_collect_msgs, + raise_group_apply_error, ) from pypaimon.ray.data_evolution_merge_transform import ( LiteralValue, @@ -281,6 +282,10 @@ def _build_datasets( # snapshot the caller observed; otherwise concurrent commits in between # would mix data from different snapshots. base_snapshot_id = base_snapshot.id if base_snapshot is not None else None + # An existing but empty snapshot (0 rows) is still an empty target: matching + # against it joins a zero-block dataset (ArrowInvalid on some Arrow versions). + target_is_empty = ( + base_snapshot is None or base_snapshot.total_record_count == 0) update_ds = None delete_ds = None @@ -288,7 +293,7 @@ def _build_datasets( update_cols_union: List[str] = [] if ctx.is_self_merge: - if matched_specs and base_snapshot is not None: + if matched_specs and not target_is_empty: update_cols_union = _union_update_cols(matched_specs) if update_cols_union: update_ds = build_self_merge_update_ds( @@ -317,7 +322,7 @@ def _build_datasets( # Mirror Spark: matched/not-matched run as two independent joins # (inner / left_anti). One unified left_outer join would force # joined.materialize() to feed both branches, which can OOM on large merges. - if matched_specs and base_snapshot is not None: + if matched_specs and not target_is_empty: update_cols_union = _union_update_cols(matched_specs) if update_cols_union: update_ds = build_matched_update_ds( @@ -362,7 +367,7 @@ def _build_datasets( catalog_options=ctx.catalog_options, num_partitions=num_partitions, snapshot_id=base_snapshot_id, - target_empty=base_snapshot is None, + target_empty=target_is_empty, ray_remote_args=ray_remote_args, ) @@ -388,7 +393,8 @@ def _execute_and_commit( try: if update_ds is not None: - update_msgs, num_updated, update_row_ids = distributed_update_apply( + (update_msgs, num_updated, update_row_ids, + update_group_error) = distributed_update_apply( update_ds, table, update_cols_union, num_partitions=num_partitions, ray_remote_args=ray_remote_args, @@ -398,7 +404,12 @@ def _execute_and_commit( ), collect_row_ids=collect_action_row_ids, ) + # Keep the successful groups' files in pending_msgs so the except + # below aborts them: merge_into commits atomically, so a failed + # group must abort the whole batch rather than commit part of it. pending_msgs.extend(update_msgs) + if update_group_error is not None: + raise_group_apply_error(update_group_error) if delete_ds is not None: delete_msgs, num_deleted, delete_row_ids = distributed_delete_apply( @@ -522,14 +533,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..cc8cb72c3607 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 @@ -32,6 +32,39 @@ ) +class GroupApplyError(RuntimeError): + """A target file group failed while applying a distributed update. Groups + that succeeded are already committed/aborted by the time this is raised.""" + + +def _encode_group_error(exc: BaseException) -> Dict[str, str]: + """Worker-side exception as a text-only payload (type/message/traceback). + + Never carries the instance: dumps succeeding on the worker does not mean + loads succeeds on the driver, and a failed unpickle there would drop the + groups that succeeded. A dict of strings always round-trips. + """ + import traceback as _traceback + return { + "type": type(exc).__name__, + "message": str(exc), + "traceback": "".join(_traceback.format_exception( + type(exc), exc, exc.__traceback__)), + } + + +def raise_group_apply_error(payload: Dict[str, Any]) -> None: + """Reconstruct and raise the failure carried by a ``group_error`` payload.""" + raise GroupApplyError( + "update_by_row_id file group failed: " + "{type}: {message}\n{traceback}".format( + type=payload.get("type", "Exception"), + message=payload.get("message", ""), + traceback=payload.get("traceback", ""), + ) + ) + + def _map_kwargs( ray_remote_args: Optional[Dict[str, Any]], ) -> Dict[str, Any]: @@ -445,7 +478,20 @@ def distributed_update_apply( ray_remote_args: Optional[Dict[str, Any]] = None, base_snapshot_id: Optional[int] = None, collect_row_ids: bool = False, -) -> Tuple[list, int, list]: + on_group_result: Optional[Callable[[list, int, list], None]] = None, +) -> Tuple[list, int, list, Optional[dict]]: + """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. + + Returns ``(msgs, num_updated, row_ids, group_error)``. ``group_error`` is + ``None`` on success, else a payload for the first failed group (see + ``raise_group_apply_error``); returned rather than raised so the caller can + flush/abort the successful groups before surfacing it. + """ import numpy as np import pickle import uuid @@ -481,7 +527,8 @@ def distributed_update_apply( ) sorted_first_row_ids = list(planner.first_row_ids) if not sorted_first_row_ids: - return [], 0, [] + # No target file groups (e.g. empty snapshot): keep the 4-tuple contract. + return [], 0, [], None # Pin commit-time conflict check to the snapshot the join was built on, # so concurrent commits between read and planner are detected. @@ -547,50 +594,60 @@ def _assign_frid(batch: pa.Table) -> pa.Table: captured_table = table captured_cols = cols + def _group_result(msgs_blob, n_updated, row_ids_blob, error_blob): + return pa.Table.from_pydict({ + "msgs_blob": pa.array([msgs_blob], type=pa.binary()), + "n_updated": pa.array([n_updated], type=pa.int64()), + "row_ids_blob": pa.array([row_ids_blob], type=pa.binary()), + "error_blob": pa.array([error_blob], type=pa.binary()), + }) + def _apply_group(group: pa.Table) -> pa.Table: if group.num_rows == 0: return pa.Table.from_pydict({ "msgs_blob": pa.array([], type=pa.binary()), "n_updated": pa.array([], type=pa.int64()), "row_ids_blob": pa.array([], type=pa.binary()), + "error_blob": pa.array([], type=pa.binary()), }) - if ( - pc.count_distinct(group.column(row_id_name)).as_py() - != group.num_rows - ): - raise ValueError( - "MERGE matched multiple source rows to the same " - "target _ROW_ID. Deduplicate the source before " - "merging." - ) - - for_update = group.drop_columns([frid_col]) - row_ids = ( - for_update.column(row_id_name).to_pylist() - if collect_row_ids else [] - ) - worker = TableUpdateByRowId( - captured_table, - "_merge_into_shard_" + uuid.uuid4().hex[:8], - BATCH_COMMIT_IDENTIFIER, - _precomputed_files_info=ray.get(precomputed_info_ref), - ) - msgs = worker.update_columns(for_update, list(captured_cols)) - return pa.Table.from_pydict({ - "msgs_blob": [pickle.dumps(msgs)], - "n_updated": pa.array( - [for_update.num_rows], type=pa.int64() - ), - "row_ids_blob": pa.array( - [pickle.dumps(row_ids)], type=pa.binary() - ), - }) + try: + if ( + pc.count_distinct(group.column(row_id_name)).as_py() + != group.num_rows + ): + raise ValueError( + "MERGE matched multiple source rows to the same " + "target _ROW_ID. Deduplicate the source before " + "merging." + ) - # One group per target data file; bounded by file count and num_partitions. - group_partitions = max( - 1, min(len(captured_sorted), num_partitions) - ) + for_update = group.drop_columns([frid_col]) + row_ids = ( + for_update.column(row_id_name).to_pylist() + if collect_row_ids else [] + ) + worker = TableUpdateByRowId( + captured_table, + "_merge_into_shard_" + uuid.uuid4().hex[:8], + BATCH_COMMIT_IDENTIFIER, + _precomputed_files_info=ray.get(precomputed_info_ref), + ) + msgs = worker.update_columns(for_update, list(captured_cols)) + except Exception as group_error: + # Return the failure as data, not raise: a raised error kills the + # whole Ray task and discards its other (completed) groups. Strings + # only, so the payload always unpickles on the driver. + return _group_result( + b"", 0, b"", pickle.dumps(_encode_group_error(group_error))) + + return _group_result( + pickle.dumps(msgs), for_update.num_rows, + pickle.dumps(row_ids), None) + + # Bound the shuffle to num_partitions so a sparse source over a huge table + # does not spawn one partition per untouched file group. + group_partitions = max(1, min(len(captured_sorted), num_partitions)) msgs_ds = with_frid.groupby( frid_col, num_partitions=group_partitions ).map_groups(_apply_group, **map_kwargs) @@ -598,15 +655,42 @@ 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(): + group_error_payload = None + iter_batches_kwargs = {"batch_format": "pyarrow"} + if on_group_result is not None: + # Deliver each group as soon as its block lands so it commits early. + # (Failure isolation comes from _apply_group, not from batch_size.) + 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() + error_blobs = batch.column("error_blob").to_pylist() + row_id_blobs = ( + batch.column("row_ids_blob").to_pylist() + if collect_row_ids else [None] * len(message_blobs) + ) + for blob, n, err_blob, row_ids_blob in zip( + message_blobs, updated_counts, error_blobs, row_id_blobs): + if err_blob is not None: + # Report the first failure only after all successes are delivered. + if group_error_payload is None: + group_error_payload = pickle.loads(err_blob) + continue + 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)) - return all_msgs, num_updated, action_row_ids + if collect_row_ids: + action_row_ids.extend(group_row_ids) + # Failure is returned, not raised: the caller flushes/aborts the successful + # groups first, then surfaces it. See raise_group_apply_error. + return all_msgs, num_updated, action_row_ids, group_error_payload def _read_output_schema(table, read_cols: Sequence[str]) -> "pa.Schema": diff --git a/paimon-python/pypaimon/ray/update_by_row_id.py b/paimon-python/pypaimon/ray/update_by_row_id.py index 41b968c4fdc8..2611a2077707 100644 --- a/paimon-python/pypaimon/ray/update_by_row_id.py +++ b/paimon-python/pypaimon/ray/update_by_row_id.py @@ -34,9 +34,13 @@ _require_ray_join, _resolve_num_partitions, ) -from pypaimon.ray.data_evolution_merge_join import distributed_update_apply +from pypaimon.ray.data_evolution_merge_join import ( + distributed_update_apply, + raise_group_apply_error, +) 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 +60,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 +70,18 @@ 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. + A failing group cannot discard groups that already succeeded: its exception + is returned as data, not raised, so groups sharing its Ray task still commit + before the failure surfaces. 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 +91,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 +161,261 @@ 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 + ) + group_error_payload = None try: - 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, + 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, _, group_error_payload = distributed_update_apply( + update_ds, table, update_cols, **apply_kwargs ) + if incremental_committer is not None: + # Flush successful sub-window groups; a deferred commit error raised + # here keeps priority over the group error surfaced below. + 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 group_error_payload is not None: + # Successful groups are now durable (incremental) or about to be aborted + # (atomic, all-or-nothing). Surface the first group failure. + if incremental_committer is None and msgs: + _abort_pending_update_messages(table, msgs) + raise_group_apply_error(group_error_payload) + 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() + if (self._table_commit is not None + and self._base_snapshot_id is not None): + # Final guard: catch an overwrite even if no window follows. + self._table_commit.validate_protected_row_id_scope() + 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() + # Fail fast: don't stack a window on a scope an overwrite invalidated + # (only meaningful once a base snapshot is pinned). + if self._base_snapshot_id is not None: + self._table_commit.validate_protected_row_id_scope() + 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() + # Extend the protected scope to this window and re-check for + # overwrites (_last_committed_snapshot needs a pinned base). + if self._base_snapshot_id is not None: + self._table_commit.protect_committed_row_id_scope( + committed_messages, self._last_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 +437,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..bf6b751abe38 100644 --- a/paimon-python/pypaimon/tests/file_store_commit_test.py +++ b/paimon-python/pypaimon/tests/file_store_commit_test.py @@ -19,12 +19,28 @@ 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, + DeterministicCommitRejectionError, + FileStoreCommit, + RetryResult, + SuccessResult, +) @patch('pypaimon.write.file_store_commit.ManifestFileManager') @@ -54,6 +70,31 @@ 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 + # close() is driven through __exit__ so the lifecycle contract is kept; + # simulate a close failure on __exit__ (its base impl calls close()). + 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 +489,703 @@ 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._clean_up_reuse_tmp_manifests = Mock() + file_store_commit._clean_up_no_reuse_tmp_manifests = Mock() + file_store_commit.snapshot_manager.get_latest_snapshot.return_value = None + + # Surfaced as a safe-to-abort CommitConflictError (so the + # message-owning layer aborts the staged files), original cause + # preserved. + with self.assertRaises( + DeterministicCommitRejectionError) as raised: + file_store_commit._try_commit( + "APPEND", 1, lambda _snapshot: [commit_entry]) + + self.assertIsInstance(raised.exception, CommitConflictError) + self.assertIs(atomic_error, raised.exception.__cause__) + snapshot_commit.commit.assert_called_once() + file_store_commit._commit_retry_wait.assert_not_called() + file_store_commit._clean_up_reuse_tmp_manifests.assert_called_once() + file_store_commit._clean_up_no_reuse_tmp_manifests.assert_called_once() + + 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 _commit_once(self, file_store_commit, commit_entry): + return file_store_commit._try_commit_once( + retry_result=None, commit_kind="APPEND", + commit_entries=[commit_entry], changelog_entries=[], + commit_identifier=1, latest_snapshot=None) + + def test_snapshot_commit_lifecycle_is_preserved( + self, mock_manifest_list_manager, mock_manifest_file_manager): + # The __enter__/__exit__ contract must run (e.g. an extension's lock). + fsc, snapshot_commit, entry = self._prepare_atomic_attempt() + result = self._commit_once(fsc, entry) + self.assertTrue(result.is_success()) + snapshot_commit.__enter__.assert_called_once() + snapshot_commit.__exit__.assert_called_once() + self.assertEqual( + (None, None, None), snapshot_commit.__exit__.call_args.args) + + def test_commit_exception_is_passed_to_exit( + self, mock_manifest_list_manager, mock_manifest_file_manager): + # __exit__ sees the real commit exception triple (for rollback). + atomic_error = RuntimeError("atomic commit failed") + fsc, snapshot_commit, entry = self._prepare_atomic_attempt( + atomic_error=atomic_error) + result = self._commit_once(fsc, entry) + self.assertFalse(result.is_success()) + exit_args = snapshot_commit.__exit__.call_args.args + self.assertIs(RuntimeError, exit_args[0]) + self.assertIs(atomic_error, exit_args[1]) + + def test_enter_or_close_failure_never_overrides_outcome( + self, mock_manifest_list_manager, mock_manifest_file_manager): + # __enter__/__exit__ failures never override commit()'s outcome, and a + # non-success never cleans up the (possibly-committed) files. + identifier = Identifier.create("default", "test_table") + enter_error = RuntimeError("lock acquisition failed") + close_error = TableNoPermissionException(identifier) + lost_error = RESTException( + "response lost", cause=TimeoutError("read timed out")) + cases = [ + # label, atomic_error, enter_error, close_error, success, exception + ("enter_failure", None, enter_error, None, False, enter_error), + ("close_after_success", None, None, close_error, True, None), + ("lost_response_then_close", lost_error, None, close_error, + False, lost_error), + ] + for label, atomic_error, enter_err, close_err, success, exc in cases: + with self.subTest(label): + fsc, snapshot_commit, entry = self._prepare_atomic_attempt( + atomic_error=atomic_error, close_error=close_err) + if enter_err is not None: + snapshot_commit.__enter__.side_effect = enter_err + fsc._clean_up_reuse_tmp_manifests = Mock() + fsc._clean_up_no_reuse_tmp_manifests = Mock() + result = self._commit_once(fsc, entry) + self.assertEqual(success, result.is_success()) + if exc is not None: + self.assertIs(exc, result.exception) + self.assertTrue(result.outcome_unknown) + if enter_err is not None: + # A failed __enter__ skips commit() and __exit__. + snapshot_commit.commit.assert_not_called() + snapshot_commit.__exit__.assert_not_called() + fsc._clean_up_reuse_tmp_manifests.assert_not_called() + fsc._clean_up_no_reuse_tmp_manifests.assert_not_called() + + def test_deterministic_rejection_survives_ordinary_close_failure( + self, mock_manifest_list_manager, mock_manifest_file_manager): + # Reverse: commit() deterministically rejected, close() raised an + # ordinary error: not downgraded to unknown; files cleaned, error raised. + atomic_error = TableNoPermissionException( + Identifier.create("default", "test_table")) + close_error = RuntimeError("close failed") + fsc, _, entry = self._prepare_atomic_attempt( + atomic_error=atomic_error, close_error=close_error) + fsc._clean_up_reuse_tmp_manifests = Mock() + fsc._clean_up_no_reuse_tmp_manifests = Mock() + with self.assertRaises(DeterministicCommitRejectionError) as raised: + self._commit_once(fsc, entry) + self.assertIs(atomic_error, raised.exception.__cause__) + fsc._clean_up_reuse_tmp_manifests.assert_called_once() + fsc._clean_up_no_reuse_tmp_manifests.assert_called_once() + + def test_false_commit_close_failure_is_deterministic( + self, mock_manifest_list_manager, mock_manifest_file_manager): + close_error = RuntimeError("close failed") + file_store_commit, snapshot_commit, commit_entry = ( + self._prepare_atomic_attempt(close_error=close_error)) + snapshot_commit.commit.return_value = False + file_store_commit._clean_up_reuse_tmp_manifests = Mock() + file_store_commit._clean_up_no_reuse_tmp_manifests = Mock() + + 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.assertFalse(result.outcome_unknown) + self.assertIsNone(result.exception) + file_store_commit._clean_up_reuse_tmp_manifests.assert_called_once() + file_store_commit._clean_up_no_reuse_tmp_manifests.assert_called_once() + + 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(CommitConflictError) as raised: + file_store_commit._try_commit( + "APPEND", 1, lambda _snapshot: [Mock()]) + + self.assertNotIsInstance( + raised.exception, CommitOutcomeUnknownError) + + def test_false_atomic_commit_cleans_attempt_manifests( + self, mock_manifest_list_manager, mock_manifest_file_manager): + file_store_commit, snapshot_commit, commit_entry = ( + self._prepare_atomic_attempt()) + snapshot_commit.commit.return_value = False + file_store_commit._clean_up_reuse_tmp_manifests = Mock() + file_store_commit._clean_up_no_reuse_tmp_manifests = Mock() + + 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.assertFalse(result.outcome_unknown) + file_store_commit._clean_up_reuse_tmp_manifests.assert_called_once() + file_store_commit._clean_up_no_reuse_tmp_manifests.assert_called_once() + + 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_index_only_message_keeps_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, + ) + update = CommitMessage( + partition=(), + bucket=0, + new_files=[Mock()], + check_from_snapshot=7, + row_id_base_files=[base_file], + row_id_base_snapshot_identity=identity, + ) + row_delete = CommitMessage( + partition=("other",), + bucket=0, + new_files=[], + check_from_snapshot=7, + index_adds=[Mock()], + ) + + entries, snapshot = file_store_commit._collect_row_id_base_entries( + [update, row_delete]) + + 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_data_evolution_merge_into_test.py b/paimon-python/pypaimon/tests/ray_data_evolution_merge_into_test.py index cddb11ac37b1..aaae31aad5f2 100644 --- a/paimon-python/pypaimon/tests/ray_data_evolution_merge_into_test.py +++ b/paimon-python/pypaimon/tests/ray_data_evolution_merge_into_test.py @@ -505,6 +505,37 @@ def test_insert_into_empty_target(self): self.assertEqual(out['name'], ['a', 'b', 'c']) self.assertEqual(out['age'], [10, 20, 30]) + def test_update_and_insert_into_existing_empty_snapshot(self): + # Existing but empty snapshot: the planner finds no target file groups, + # so distributed_update_apply must return the 4-tuple. NOT MATCHED + # inserts must still land instead of crashing on tuple unpacking. + target = self._create_table() + self._write(target, self._source(ids=(1,))) + wb = self.catalog.get_table(target).new_batch_write_builder().overwrite() + w = wb.new_write() + w.write_arrow(pa.Table.from_pydict( + {'id': pa.array([], type=pa.int32()), + 'name': pa.array([], type=pa.string()), + 'age': pa.array([], type=pa.int32())}, + schema=self.pa_schema)) + wb.new_commit().commit(w.prepare_commit()) + w.close() + + metrics = merge_into( + target=target, + source=self._source(ids=(1, 2)), + catalog_options=self.catalog_options, + on=['id'], + when_matched=[WhenMatched.update('*')], + when_not_matched=[WhenNotMatched(insert='*')], + num_partitions=_TEST_NUM_PARTITIONS, + ) + + out = self._read_sorted(target) + self.assertEqual(out['id'], [1, 2]) + self.assertEqual(metrics, + {'num_matched': 0, 'num_inserted': 2, 'num_unchanged': 0}) + def test_multi_source_match_raises_by_default(self): # One target row matched by several source rows: the winning value is # undefined (Spark DE's checkCardinality=false), so we refuse by default. 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..fe837b467970 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 @@ -33,6 +34,13 @@ from pypaimon.ray import update_by_row_id +class _UndeserializableError(Exception): + # Module-level so pickle.dumps succeeds; two required args make loads fail. + def __init__(self, a, b): + super().__init__(f"unpicklable boom {a}/{b}") + self.a, self.b = a, b + + class RayUpdateByRowIdTest(unittest.TestCase): """Distributed row-id update: rewrite only the files owning the given row ids, without reading or joining the whole target (unlike merge_into(on=_ROW_ID)).""" @@ -88,6 +96,36 @@ def _rowid_by_id(self, target): tab = self._read(target, ["_ROW_ID", "id"]) return dict(zip(tab.column("id").to_pylist(), tab.column("_ROW_ID").to_pylist())) + _UPDATE_SCHEMA = pa.schema([("_ROW_ID", pa.int64()), ("age", pa.int32())]) + + def _de_table_with_files(self, n): + """A DE table with n single-row files (one file group each).""" + target = self._create() + for row_id in range(1, n + 1): + self._write(target, pa.Table.from_pydict( + {"id": [row_id], "name": ["a"], "age": [0]}, + schema=self.pa_schema)) + return target, self._rowid_by_id(target) + + def _dup_source(self, row_ids): + """Source where id=1's group is valid and id=2's has a duplicate + _ROW_ID, so that group fails inside the worker.""" + return pa.table( + {"_ROW_ID": [row_ids[1], row_ids[2], row_ids[2]], + "age": [111, 222, 333]}, + schema=self._UPDATE_SCHEMA) + + def _ages(self, target): + return dict(zip(self._read(target)["id"].to_pylist(), + self._read(target)["age"].to_pylist())) + + def _full_source(self, row_ids, n): + """Update ids 1..n to age i*100.""" + return pa.table( + {"_ROW_ID": [row_ids[i] for i in range(1, n + 1)], + "age": [i * 100 for i in range(1, n + 1)]}, + schema=self._UPDATE_SCHEMA) + def test_update_by_row_id_basic(self): target = self._create() self._write(target, pa.Table.from_pydict( @@ -130,6 +168,1093 @@ 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_group_failure_preserves_earlier_successes(self): + # A failing group (duplicate _ROW_ID) sharing a Ray task (num_partitions=1) + # must not discard the healthy group, and the error must still surface. + # max_groups_per_commit=1 is a full window; =5 is a not-yet-flushed one. + for max_groups in (1, 5): + with self.subTest(max_groups_per_commit=max_groups): + target, row_ids = self._de_table_with_files(2) + with self.assertRaisesRegex(RuntimeError, "Deduplicate"): + update_by_row_id( + target, ray.data.from_arrow(self._dup_source(row_ids)), + self.catalog_options, update_cols=["age"], + num_partitions=1, max_groups_per_commit=max_groups) + ages = self._ages(target) + self.assertEqual(111, ages[1]) # healthy group committed + self.assertEqual(0, ages[2]) # failed group untouched + + def test_atomic_group_failure_commits_nothing(self): + # Atomic mode is all-or-nothing: a group failure aborts the healthy files. + target, row_ids = self._de_table_with_files(2) + base = self.catalog.get_table( + target).snapshot_manager().get_latest_snapshot().id + with self.assertRaisesRegex(RuntimeError, "Deduplicate"): + update_by_row_id( + target, ray.data.from_arrow(self._dup_source(row_ids)), + self.catalog_options, update_cols=["age"]) + self.assertEqual(base, self.catalog.get_table( + target).snapshot_manager().get_latest_snapshot().id) + self.assertEqual( + [0, 0], self._read(target).sort_by("id")["age"].to_pylist()) + + def test_group_error_channel_is_string_only_and_round_trips(self): + # The worker->driver channel carries strings only: an exception whose + # __init__ needs extra args can dump on the worker yet fail to load on + # the driver, which would drop the successful groups. + import importlib + import pickle + m = importlib.import_module("pypaimon.ray.data_evolution_merge_join") + + try: + raise _UndeserializableError("x", "y") + except _UndeserializableError as exc: + payload = m._encode_group_error(exc) + dumped_instance = pickle.dumps(exc) # exc is cleared after the block + + with self.assertRaises(Exception): # instance fails at load time + pickle.loads(dumped_instance) + self.assertEqual(payload, pickle.loads(pickle.dumps(payload))) + self.assertTrue(all(isinstance(v, str) for v in payload.values())) + self.assertEqual("_UndeserializableError", payload["type"]) + with self.assertRaisesRegex(m.GroupApplyError, "unpicklable boom"): + m.raise_group_apply_error(payload) + + def test_sparse_source_keeps_partitions_bounded(self): + # Partitions follow num_partitions, not the table's file count. + target, row_ids = self._de_table_with_files(5) + source = pa.table( + {"_ROW_ID": [row_ids[1]], "age": [111]}, schema=self._UPDATE_SCHEMA) + + captured = [] + original_groupby = ray.data.Dataset.groupby + + def recording_groupby(dataset, *args, **kwargs): + captured.append(kwargs.get("num_partitions")) + return original_groupby(dataset, *args, **kwargs) + + with mock.patch.object( + ray.data.Dataset, "groupby", recording_groupby): + update_by_row_id( + target, ray.data.from_arrow(source), self.catalog_options, + update_cols=["age"], num_partitions=2, max_groups_per_commit=1) + self.assertEqual([2], captured) # capped at 2, not the five-file count + self.assertEqual(111, self._ages(target)[1]) + + 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_commit_rejects_overwrite_of_committed_window(self): + # A concurrent overwrite of an already-committed window must be caught: + # the protected scope covers earlier windows, not just the current one. + # Commit window 1, overwrite the table, then let window 2 fail closed. + target, row_ids = self._de_table_with_files(2) + table = self.catalog.get_table(target) + source = self._full_source(row_ids, 2) + + original_iter_batches = ray.data.Dataset.iter_batches + first_window_committed = threading.Event() + resume_iteration = threading.Event() + overwrite_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 overwrite") + + def overwrite_first_window(): + try: + if not first_window_committed.wait(30): + raise TimeoutError("first commit was not observed") + wb = table.new_batch_write_builder().overwrite() + w = wb.new_write() + w.write_arrow(pa.Table.from_pydict( + {"id": [9], "name": ["z"], "age": [9]}, + schema=self.pa_schema)) + wb.new_commit().commit(w.prepare_commit()) + w.close() + except Exception as error: + overwrite_error.append(error) + finally: + resume_iteration.set() + + overwrite_thread = threading.Thread(target=overwrite_first_window) + overwrite_thread.start() + try: + with mock.patch.object( + ray.data.Dataset, "iter_batches", pausing_iter_batches): + with self.assertRaisesRegex( + RuntimeError, "already-committed update_by_row_id"): + update_by_row_id( + target, + ray.data.from_arrow(source), + self.catalog_options, + update_cols=["age"], + num_partitions=1, + max_groups_per_commit=1, + ) + finally: + resume_iteration.set() + overwrite_thread.join(30) + + self.assertEqual([], overwrite_error) + self.assertFalse(overwrite_thread.is_alive()) + + def test_protected_scope_not_rescanned_without_external_writer(self): + # Without a concurrent writer, the protected-scope check short-circuits: + # no manifest scan (keeps N windows O(N), not O(N^2)). + import pypaimon.write.commit.commit_scanner as cs + target, row_ids = self._de_table_with_files(3) + source = self._full_source(row_ids, 3) + + scan_calls = [] + original = cs.CommitScanner.read_entries_for_row_id_scope + + def spy(self, *args, **kwargs): + scan_calls.append(1) + return original(self, *args, **kwargs) + + with mock.patch.object( + cs.CommitScanner, "read_entries_for_row_id_scope", spy): + update_by_row_id( + target, ray.data.from_arrow(source), self.catalog_options, + update_cols=["age"], num_partitions=1, max_groups_per_commit=1) + + self.assertEqual(0, len(scan_calls)) + self.assertEqual( + [100, 200, 300], + self._read(target).sort_by("id")["age"].to_pylist()) + + def test_own_snapshots_state_stays_bounded_across_windows(self): + # _protected_own_snapshots is pruned as the checkpoint advances, so it + # stays bounded regardless of how many windows commit. + import importlib + uix = importlib.import_module("pypaimon.ray.update_by_row_id") + target, row_ids = self._de_table_with_files(5) + source = self._full_source(row_ids, 5) + + committers = [] + original_init = uix._IncrementalUpdateCommitter.__init__ + + def recording_init(self, *args, **kwargs): + original_init(self, *args, **kwargs) + committers.append(self) + + with mock.patch.object( + uix._IncrementalUpdateCommitter, "__init__", recording_init): + update_by_row_id( + target, ray.data.from_arrow(source), self.catalog_options, + update_cols=["age"], num_partitions=1, max_groups_per_commit=1) + + conflict_detection = ( + committers[0]._table_commit.file_store_commit.conflict_detection) + self.assertLessEqual(len(conflict_detection._protected_own_snapshots), 1) + self.assertEqual( + [100, 200, 300, 400, 500], + 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. @@ -149,7 +1274,7 @@ def test_pins_base_snapshot_for_conflict_detection(self): def fake_apply(update_ds, table, cols, *, num_partitions, ray_remote_args=None, base_snapshot_id=None): captured["base_snapshot_id"] = base_snapshot_id - return [], 0, [] + return [], 0, [], None with mock.patch.object(m, "distributed_update_apply", fake_apply): update_by_row_id(target, src, self.catalog_options, update_cols=["age"]) @@ -169,7 +1294,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 +1304,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") @@ -425,7 +1595,7 @@ def map_batches(self, fn, batch_format=None): ("age", pa.int32()), ])), \ mock.patch.object(m, "distributed_update_apply", - return_value=(recorder["msgs"], 3, [])): + return_value=(recorder["msgs"], 3, [], None)): recorder["result"] = m.update_by_row_id( "default.fake", FakeSource(), 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..16c67d34b735 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,24 @@ 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, + DeterministicCommitRejectionError, +) class TestRESTCatalogCommitSnapshot(unittest.TestCase): @@ -138,6 +148,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 +353,57 @@ 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: + # Safe-to-abort rejection (cause = mapped exception); + # staged files aborted by the commit layer. + with self.assertRaises( + DeterministicCommitRejectionError) as raised: + table_commit.commit(commit_messages) + + self.assertIsInstance( + raised.exception.__cause__, expected_error) + rest_commit.assert_called_once() + 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 +431,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 +443,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_commit_test.py b/paimon-python/pypaimon/tests/table_commit_test.py index d0b9d62762df..0e57603eece9 100644 --- a/paimon-python/pypaimon/tests/table_commit_test.py +++ b/paimon-python/pypaimon/tests/table_commit_test.py @@ -100,3 +100,30 @@ def test_stream_commit_overwrite_empty_messages(self): commit_messages=[], commit_identifier=42, ) + + def test_deterministic_rejection_aborts_staged_files(self): + # A deterministic rejection is aborted by the commit layer; no manual + # abort() needed. + from pypaimon.write.file_store_commit import ( + DeterministicCommitRejectionError) + + commit, mock_fsc = self._create_commit( + BatchTableCommit, overwrite_partition=None) + messages = [CommitMessage(partition=(), bucket=0, new_files=[Mock()])] + mock_fsc.commit.side_effect = DeterministicCommitRejectionError( + "rejected") + + with self.assertRaises(DeterministicCommitRejectionError): + commit.commit(messages) + mock_fsc.abort.assert_called_once_with(messages) + + def test_generic_commit_failure_does_not_abort(self): + # A generic (uncertain) failure must not abort: the commit may have landed. + commit, mock_fsc = self._create_commit( + BatchTableCommit, overwrite_partition=None) + messages = [CommitMessage(partition=(), bucket=0, new_files=[Mock()])] + mock_fsc.commit.side_effect = RuntimeError("response lost") + + with self.assertRaises(RuntimeError): + commit.commit(messages) + mock_fsc.abort.assert_not_called() diff --git a/paimon-python/pypaimon/tests/table_merge_into_test.py b/paimon-python/pypaimon/tests/table_merge_into_test.py index 5ad3a405f38c..3d4b573499f0 100644 --- a/paimon-python/pypaimon/tests/table_merge_into_test.py +++ b/paimon-python/pypaimon/tests/table_merge_into_test.py @@ -107,7 +107,7 @@ def test_ray_execute_validates_mixed_update_delete_duplicate_row_ids(self): with patch.object( ray_merge, "distributed_update_apply", - return_value=([update_msg], 1, [7]) + return_value=([update_msg], 1, [7], None) ), patch.object( ray_merge, "distributed_delete_apply", @@ -137,7 +137,7 @@ def test_ray_execute_aborts_prepared_messages_on_later_branch_failure(self): with patch.object( ray_merge, "distributed_update_apply", - return_value=([update_msg], 1, []) + return_value=([update_msg], 1, [], None) ), patch.object( ray_merge, "distributed_delete_apply", @@ -168,7 +168,7 @@ def test_ray_execute_aborts_prepared_messages_on_insert_failure(self): with patch.object( ray_merge, "distributed_update_apply", - return_value=([update_msg], 1, []) + return_value=([update_msg], 1, [], None) ), patch.object( ray_merge, "distributed_write_collect_msgs", @@ -200,7 +200,7 @@ def test_ray_execute_does_not_abort_after_commit_starts(self): with patch.object( ray_merge, "distributed_update_apply", - return_value=([update_msg], 1, []) + return_value=([update_msg], 1, [], None) ): with self.assertRaisesRegex(RuntimeError, "commit failed"): ray_merge._execute_and_commit( diff --git a/paimon-python/pypaimon/tests/table_update_test.py b/paimon-python/pypaimon/tests/table_update_test.py index 40ceaf2489f6..6deb31013776 100644 --- a/paimon-python/pypaimon/tests/table_update_test.py +++ b/paimon-python/pypaimon/tests/table_update_test.py @@ -24,11 +24,15 @@ import pyarrow as pa +from pypaimon.catalog.catalog_exception import TableNoPermissionException +from pypaimon.write.file_store_commit import DeterministicCommitRejectionError +from pypaimon.common.identifier import Identifier from pypaimon.tests.data_evolution_test_helpers import ( BatchModeMixin, DataEvolutionTestBase, StreamModeMixin, ) +from pypaimon.write.commit.conflict_detection import CommitConflictError # ====================================================================== @@ -181,6 +185,14 @@ def _create_seeded_deletion_vector_table(self, partition_keys=None): ) return table + @staticmethod + def _list_table_files(table): + return { + os.path.relpath(os.path.join(root, name), table.table_path) + for root, _dirs, files in os.walk(table.table_path) + for name in files + } + # ================================================================== # Shared tests (run under both batch and stream modes) # ================================================================== @@ -603,6 +615,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 +672,572 @@ 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_false_atomic_commit_aborts_files_and_manifests(self): + table = self._create_seeded_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())) + before_files = self._list_table_files(table) + + wb = self._make_write_builder(table) + update = wb.new_update().with_update_type(['age']) + commit_id = self._next_commit_id() + 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, + ) + + commit = wb.new_commit() + commit.file_store_commit.commit_max_retries = 0 + with mock.patch.object( + commit.file_store_commit.snapshot_commit, + 'commit', return_value=False): + with self.assertRaises(CommitConflictError): + self._apply_commit(commit, messages, commit_id) + commit.close() + + self.assertEqual(before_files, self._list_table_files(table)) + + def test_deterministic_atomic_rejection_cleans_manifests(self): + table = self._create_seeded_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())) + before_files = self._list_table_files(table) + + wb = self._make_write_builder(table) + commit_id = self._next_commit_id() + messages = self._apply_update( + wb.new_update().with_update_type(['age']), + pa.Table.from_pydict({ + '_ROW_ID': [row_ids[1]], + 'age': [99], + }, schema=pa.schema([ + ('_ROW_ID', pa.int64()), + ('age', pa.int32()), + ])), + commit_id, + ) + + commit = wb.new_commit() + error = TableNoPermissionException( + Identifier.create('default', 'test')) + with mock.patch.object( + commit.file_store_commit.snapshot_commit, + 'commit', side_effect=error): + # Surfaced as a safe-to-abort rejection; the commit layer aborts the + # staged files, so no manual abort() is needed here. + with self.assertRaises( + DeterministicCommitRejectionError) as raised: + self._apply_commit(commit, messages, commit_id) + self.assertIsInstance( + raised.exception.__cause__, TableNoPermissionException) + commit.close() + + self.assertEqual(before_files, self._list_table_files(table)) + + def test_false_atomic_commit_close_failure_cleans_files(self): + class RejectingCommit: + + def __enter__(self): + return self + + def __exit__(self, _exc_type, _exc_val, _exc_tb): + raise RuntimeError("close failed") + + def commit(self, _snapshot, _statistics): + return False + + def close(self): + pass + + table = self._create_seeded_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())) + before_files = self._list_table_files(table) + + wb = self._make_write_builder(table) + commit_id = self._next_commit_id() + messages = self._apply_update( + wb.new_update().with_update_type(['age']), + pa.Table.from_pydict({ + '_ROW_ID': [row_ids[1]], + 'age': [99], + }, schema=pa.schema([ + ('_ROW_ID', pa.int64()), + ('age', pa.int32()), + ])), + commit_id, + ) + + commit = wb.new_commit() + commit.file_store_commit.commit_max_retries = 0 + commit.file_store_commit.snapshot_commit = RejectingCommit() + with self.assertRaises(CommitConflictError): + self._apply_commit(commit, messages, commit_id) + commit.close() + + self.assertEqual(before_files, self._list_table_files(table)) + + 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, "deletion vectors"): + 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_row_delete_allows_disjoint_concurrent_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) + commit_id = self._next_commit_id() + update_messages = self._apply_update( + update_wb.new_update().with_update_type(['age']), + 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_messages = self._apply_delete_by_row_id( + delete_wb.new_update(), + [row_ids[2]], + commit_id, + ) + + self._do_delete_by_row_id(table, [row_ids[3]]) + + commit = update_wb.new_commit() + self._apply_commit( + commit, update_messages + delete_messages, commit_id) + 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) + self.assertNotIn(3, ages) + + 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_blob_update_allows_dv_delete_before_updated_row(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())) + + wb = self._make_write_builder(table) + commit_id = self._next_commit_id() + messages = self._apply_update( + wb.new_update().with_update_type(['picture']), + pa.Table.from_pydict({ + '_ROW_ID': [row_ids[9]], + 'picture': [b'updated'], + }, schema=pa.schema([ + ('_ROW_ID', pa.int64()), + ('picture', pa.large_binary()), + ])), + commit_id, + ) + blob_file = next( + file + for message in messages + for file in message.new_files + if file.file_name.endswith('.blob') + ) + self.assertEqual(row_ids[0], blob_file.first_row_id) + self.assertEqual(10, blob_file.row_count) + self.assertEqual( + [(row_ids[9], row_ids[9])], + messages[0].row_id_update_ranges, + ) + + self._do_delete_by_row_id(table, [row_ids[0]]) + + commit = wb.new_commit() + self._apply_commit(commit, messages, commit_id) + commit.close() + + result = self._read_all(table).sort_by('id').to_pydict() + self.assertEqual(list(range(1, 10)), result['id']) + self.assertEqual(b'updated', result['picture'][-1]) + 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 +1854,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) @@ -1339,14 +1982,6 @@ def fail_after_prepare_commit( self.assertEqual(before_files, self._list_table_files(table)) - @staticmethod - def _list_table_files(table): - return { - os.path.relpath(os.path.join(root, name), table.table_path) - for root, _dirs, files in os.walk(table.table_path) - for name in files - } - class TableUpdateStreamTest(_StreamModeMixin, _TableUpdateTestBase, unittest.TestCase): """All shared update tests under stream (``StreamWriteBuilder``) semantics, 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..0ed9a0320adc 100644 --- a/paimon-python/pypaimon/tests/write/conflict_detection_test.py +++ b/paimon-python/pypaimon/tests/write/conflict_detection_test.py @@ -26,6 +26,11 @@ 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.utils.range import Range +from pypaimon.write.commit.commit_scanner import ( + CommitScanner, + _ConflictEntryScope, +) from pypaimon.write.commit.conflict_detection import ( ConflictDetection, RowIdColumnConflictChecker, @@ -56,10 +61,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 +138,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 +335,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 +363,518 @@ 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_dv_targets_use_exact_update_ranges(self): + detection = self._make_detection( + [_FakeSnapshot(1, "APPEND")], + _FakeBaseEntryScanner({}, {}), + ) + base = [_make_entry( + "base.parquet", first_row_id=100, row_count=10)] + window = [_make_entry( + "update.blob", first_row_id=100, row_count=10)] + + targets = detection._row_id_deletion_vector_targets( + base, + window, + {((), 0): [Range(102, 102), Range(109, 109)]}, + ) + + self.assertEqual( + {((), 0, "base.parquet"): [Range(2, 2), Range(9, 9)]}, + targets, + ) + + 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 +892,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 +912,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 +975,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): @@ -542,5 +1263,52 @@ def test_cross_schema_field_id_resolution(self): self.assertFalse(checker.conflicts_with(committed_diff_field)) +class TestConflictEntryScopeFromRanges(unittest.TestCase): + """The compact row-id scope used to protect committed incremental windows + must isolate by (partition, bucket) even though the early bucket filter can + over-read; matches_entry is the exact filter.""" + + def test_scope_isolates_by_partition_and_bucket(self): + # Overlapping range but a different partition or bucket must not match. + scope = _ConflictEntryScope.from_ranges({(("p1",), 0): [Range(0, 99)]}) + target = _make_entry( + "f1", bucket=0, first_row_id=0, row_count=100, partition=["p1"]) + self.assertTrue(scope.matches_entry(target)) + for label, entry in [ + ("different partition", _make_entry( + "f2", bucket=0, first_row_id=0, row_count=100, partition=["p2"])), + ("different bucket", _make_entry( + "f3", bucket=1, first_row_id=0, row_count=100, partition=["p1"])), + ]: + with self.subTest(label): + self.assertFalse(scope.matches_entry(entry)) + + def test_overlapping_and_adjacent_ranges_merge(self): + scope = _ConflictEntryScope.from_ranges({ + (("p1",), 0): [Range(0, 49), Range(50, 99), Range(90, 120)], + }) + # Overlapping + adjacent ranges collapse to a single [0, 120] span, + # giving stable, comparable signatures for checkpoint vs latest scans. + merged = scope._ranges[(("p1",), 0)] + self.assertEqual(1, len(merged)) + self.assertEqual((0, 120), (merged[0].from_, merged[0].to)) + inside = _make_entry( + "f1", bucket=0, first_row_id=100, row_count=10, partition=["p1"]) + outside = _make_entry( + "f2", bucket=0, first_row_id=200, row_count=10, partition=["p1"]) + self.assertTrue(scope.matches_entry(inside)) + self.assertFalse(scope.matches_entry(outside)) + + def test_empty_scope_reads_nothing_without_partition_fallback(self): + # An empty protected scope must return [] -- never fall back to scanning + # changed partitions the way read_conflict_entries does for an empty + # commit window. + self.assertTrue(_ConflictEntryScope.from_ranges({}).is_empty()) + scanner = CommitScanner(None, None) + self.assertEqual([], scanner.read_entries_for_row_id_scope(None, {})) + self.assertEqual( + [], scanner.read_entries_for_row_id_scope(object(), {})) + + if __name__ == '__main__': unittest.main() 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..1613c0087996 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,142 @@ 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._init_from_ranges(ranges, file_names) + + @classmethod + def from_ranges(cls, ranges_by_key): + """Build a scope from a compact ``{(partition, bucket): [Range]}`` map, + so a cumulative row-id scope can be kept as ranges only (no resident + ManifestEntry/DataFileMeta). No file-name-only entries.""" + scope = cls.__new__(cls) + scope._init_from_ranges( + {key: list(values) for key, values in ranges_by_key.items()}, {}) + return scope + + def _init_from_ranges(self, ranges, file_names): + 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(self._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 +181,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 +214,62 @@ 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) + + partition_filter = self._build_partition_filter_from_changes( + commit_entries, index_entries) + return self._read_entries_for_scope( + latest_snapshot, scope, partition_filter) + + def read_entries_for_row_id_scope(self, latest_snapshot: Optional[Snapshot], + ranges_by_key): + """Read live entries within a compact row-id scope + (``{(partition, bucket): [Range]}``). Unlike :meth:`read_conflict_entries`, + an empty scope returns ``[]`` (never scans changed partitions) -- callers + must fail closed rather than register an empty scope. + """ + if latest_snapshot is None: + return [] + scope = _ConflictEntryScope.from_ranges(ranges_by_key) + if scope.is_empty(): + return [] + # Partition filter straight from the scope keys (unpartitioned -> None). + partition_filter = self._build_partition_filter_from_values( + {key[0] for key in ranges_by_key}) + return self._read_entries_for_scope( + latest_snapshot, scope, partition_filter) + + def _read_entries_for_scope(self, latest_snapshot, scope, partition_filter): + """Shared read path for a non-empty scope. Early filters may over-read by + bucket, but ``scope.matches_entry`` is the exact (partition, bucket)+range + filter, so distinct partitions/buckets stay isolated.""" + 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) + + 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 +317,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 +360,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 +370,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 +409,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..59dd065460c8 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,7 +170,26 @@ 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 + # Protection for already-committed incremental windows: merged full + # file-group ranges by (partition, bucket) + the last confirmed window's + # snapshot as a checkpoint. Survives reset_row_id_history(). + self._protected_row_id_scope = {} + self._protected_checkpoint_snapshot = None + self._protected_checkpoint_identity = None + # Our committed windows (snapshot id -> identity), to tell our own + # disjoint commits from an external overwrite and skip the scan. + self._protected_own_snapshots = {} def should_be_overwrite_commit(self, append_file_entries=None, append_index_files=None): for entry in append_file_entries or []: @@ -177,6 +203,521 @@ 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 protect_committed_row_id_scope(self, committed_snapshot, messages): + """Register a just-committed window for protection: validate the current + scope, merge this window's full file-group ranges, advance the + checkpoint, then re-validate. Fails closed on any anomaly. Call only for + a known-committed window (never a rejected/unknown commit). + """ + # Register our own snapshot first so validation treats this commit as + # ours, not as an external overwrite to re-scan. + self._protected_own_snapshots[committed_snapshot.id] = ( + self._snapshot_identity(committed_snapshot)) + self._validate_protected_scope(self.snapshot_manager.get_latest_snapshot()) + + window_ranges = self._row_id_ranges_from_messages(messages) + if not window_ranges: + # A committed window must not silently drop out of protection. + raise RowIdPlanningConflictError( + "Cannot protect a committed update_by_row_id window without " + "row ID ranges.") + self._merge_protected_ranges(window_ranges) + + self._protected_checkpoint_snapshot = committed_snapshot + self._protected_checkpoint_identity = self._snapshot_identity( + committed_snapshot) + # Drop ids <= checkpoint (never queried again) so the map stays bounded. + self._protected_own_snapshots = { + snapshot_id: identity + for snapshot_id, identity in self._protected_own_snapshots.items() + if snapshot_id > committed_snapshot.id + } + + self._validate_protected_scope(self.snapshot_manager.get_latest_snapshot()) + + def validate_protected_row_id_scope(self): + """Re-validate the protected scope against latest (used pre-commit and + at finish() so an overwrite is caught even if no further window commits).""" + self._validate_protected_scope(self.snapshot_manager.get_latest_snapshot()) + + def _validate_protected_scope(self, latest_snapshot): + if (not self._protected_row_id_scope + or self._protected_checkpoint_snapshot is None): + return + + checkpoint = self._protected_checkpoint_snapshot + current_checkpoint = self.snapshot_manager.get_snapshot_by_id( + checkpoint.id) + # Rollback / expiration / reused-id show up as a missing or changed + # snapshot at the checkpoint id -- fail closed. + if (current_checkpoint is None + or self._snapshot_identity(current_checkpoint) + != self._protected_checkpoint_identity): + raise RowIdPlanningConflictError( + "Protected update_by_row_id checkpoint snapshot is no longer " + "in the current lineage.") + if latest_snapshot is None or latest_snapshot.id < checkpoint.id: + raise RowIdPlanningConflictError( + "Protected update_by_row_id checkpoint snapshot is no longer " + "in the current lineage.") + + if not self._external_snapshot_since_checkpoint( + checkpoint.id, latest_snapshot): + # Only our own disjoint APPEND windows since the checkpoint -> no + # protected file group changed. Skip the scan (keeps N windows O(N)). + return + + checkpoint_signatures = self._row_id_entry_signatures( + self.commit_scanner.read_entries_for_row_id_scope( + checkpoint, self._protected_row_id_scope)) + latest_signatures = self._row_id_entry_signatures( + self.commit_scanner.read_entries_for_row_id_scope( + latest_snapshot, self._protected_row_id_scope)) + if checkpoint_signatures != latest_signatures: + raise RowIdPlanningConflictError( + "Concurrent overwrite changed row ID files for an " + "already-committed update_by_row_id window.") + + def _external_snapshot_since_checkpoint(self, checkpoint_id, latest_snapshot): + """Whether any snapshot in ``(checkpoint_id, latest_snapshot]`` is not + one of our own committed windows. A missing (expired) snapshot in the + range counts as external so we fall back to the full scan rather than + assume it was ours.""" + for snapshot_id in range(checkpoint_id + 1, latest_snapshot.id + 1): + if snapshot_id == latest_snapshot.id: + snapshot = latest_snapshot + else: + snapshot = self.snapshot_manager.get_snapshot_by_id(snapshot_id) + own_identity = self._protected_own_snapshots.get(snapshot_id) + if (snapshot is None + or own_identity is None + or self._snapshot_identity(snapshot) != own_identity): + return True + return False + + def _merge_protected_ranges(self, window_ranges): + for key, ranges in window_ranges.items(): + merged = self._protected_row_id_scope.get(key, []) + list(ranges) + self._protected_row_id_scope[key] = Range.sort_and_merge_overlap( + merged, True, True) + + @staticmethod + def _row_id_ranges_from_messages(messages): + """Full file-group row-id ranges by (partition, bucket): each committed + file's whole range (not a partial row_id_update_ranges subset).""" + ranges = {} + for message in messages or []: + key = (tuple(message.partition), message.bucket) + for file in message.new_files: + row_range = file.row_id_range() + if row_range is None: + continue + ranges.setdefault(key, []).append(row_range) + return { + key: Range.sort_and_merge_overlap(values, True, True) + for key, values in ranges.items() + } + + def read_row_id_base_entries(self, latest_snapshot, commit_entries, + index_entries=None, planned_base_entries=None, + planned_base_snapshot_identity=None, + planned_row_id_update_ranges=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, planned_row_id_update_ranges) + 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, planned_row_id_update_ranges) + 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, row_id_update_ranges=None): + if (self._row_id_index_overwrite_seen + and self._row_id_deletion_vectors_changed( + base_snapshot, latest_snapshot, base_entries, + commit_entries, row_id_update_ranges)): + 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, row_id_update_ranges=None): + targets = self._row_id_deletion_vector_targets( + base_entries, commit_entries, row_id_update_ranges) + 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, + row_id_update_ranges=None): + update_ranges = row_id_update_ranges + if update_ranges is None: + 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)) @@ -363,6 +904,20 @@ def check_deletion_vector_index_conflicts(self, for data_file_name in self._deletion_vector_data_file_names(add.index_file): affected_files.append((add.partition, add.bucket, data_file_name)) + missing_files = { + (tuple(partition.values), bucket, data_file_name) + for partition, bucket, data_file_name in affected_files + }.difference(existing_data_files) + if missing_files and latest_snapshot is not None: + current = self.commit_scanner.read_conflict_entries( + latest_snapshot, [], add_entries) + existing_data_files.update( + (tuple(entry.partition.values), entry.bucket, + entry.file.file_name) + for entry in current + if entry.kind == 0 + ) + for partition, bucket, data_file_name in affected_files: data_file_key = (tuple(partition.values), bucket, data_file_name) if data_file_key not in existing_data_files: @@ -490,13 +1045,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 +1219,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 + (self._file_kind(f.file_name), r.from_, r.to)) - 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 - - 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 +1263,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 +1289,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..02a0a3fbb483 100644 --- a/paimon-python/pypaimon/write/commit_message.py +++ b/paimon-python/pypaimon/write/commit_message.py @@ -35,6 +35,9 @@ 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 + row_id_update_ranges: List[Tuple[int, int]] = field(default_factory=list) 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..bb939907a92f 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 @@ -36,11 +47,13 @@ SnapshotCommit) 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_rollback import CommitRollback from pypaimon.write.commit.commit_scanner import CommitScanner 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 +63,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 +101,15 @@ def is_success(self) -> bool: raise NotImplementedError +class CommitOutcomeUnknownError(RuntimeError): + """The commit may already be visible.""" + + +class DeterministicCommitRejectionError(CommitConflictError): + """A deterministically rejected commit; a CommitConflictError so the caller + aborts the staged files. Original cause preserved.""" + + class SuccessResult(CommitResult): """Result indicating successful commit.""" @@ -68,9 +120,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 @@ -126,6 +180,17 @@ def __init__(self, snapshot_commit: SnapshotCommit, table, commit_user: str, table_rollback = table.catalog_environment.catalog_table_rollback() self.rollback = CommitRollback(table_rollback) if table_rollback is not None else None + def protect_committed_row_id_scope(self, commit_messages, committed_snapshot): + """Register a just-committed incremental row-id window for cumulative + overwrite protection (see ConflictDetection).""" + self.conflict_detection.protect_committed_row_id_scope( + committed_snapshot, commit_messages) + + def validate_protected_row_id_scope(self): + """Re-validate that no concurrent overwrite touched an already-committed + incremental row-id window.""" + self.conflict_detection.validate_protected_row_id_scope() + def commit(self, commit_messages: List[CommitMessage], commit_identifier: int): """Commit the given commit messages in normal append mode.""" if not commit_messages: @@ -135,7 +200,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 +210,10 @@ 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)) + row_id_update_ranges = self._collect_row_id_update_ranges( + commit_messages) changelog_entries = self._collect_changelog_entries(commit_messages) logger.info("Finished collecting changes, including: %d entries, %d changelog entries", @@ -203,7 +274,11 @@ 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), + row_id_update_ranges=row_id_update_ranges) def overwrite(self, overwrite_partition, commit_messages: List[CommitMessage], commit_identifier: int): """Commit the given commit messages in overwrite mode.""" @@ -356,78 +431,123 @@ 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, + row_id_update_ranges=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), + row_id_update_ranges=row_id_update_ranges, + 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 CommitConflictError ) - 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 +558,15 @@ 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, + row_id_update_ranges=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 +581,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 +596,73 @@ 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, + row_id_update_ranges, + ) + ) + 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,26 +764,82 @@ 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 + def clean_up_rejected_commit(): + try: + self._clean_up_reuse_tmp_manifests( + delta_manifest_list, + changelog_manifest_list_name, + new_index_manifest, + ) + self._clean_up_no_reuse_tmp_manifests( + base_manifest_list, merge_new_files) + except Exception: + logger.warning( + "Failed to clean up rejected commit manifests.", + exc_info=True, + ) + + # Mirror ``with`` manually so only commit() decides the outcome: an + # __enter__ failure is classified below (not bypassed), and __exit__'s + # own failure is only logged. + success = None + commit_exc = None + entered = False try: - with self.snapshot_commit: - success = self.snapshot_commit.commit(snapshot_data, statistics) - if not success: - commit_time_s = (int(time.time() * 1000) - start_millis) / 1000 + self.snapshot_commit.__enter__() + entered = True + success = self.snapshot_commit.commit(snapshot_data, statistics) + except Exception as e: + commit_exc = e + finally: + # No __exit__ without a successful __enter__ (mirrors ``with``). + if entered: + exit_exc_info = ( + (type(commit_exc), commit_exc, commit_exc.__traceback__) + if commit_exc is not None else (None, None, None) + ) + try: + self.snapshot_commit.__exit__(*exit_exc_info) + except Exception: logger.warning( - "Atomic commit failed for snapshot #%d by user %s " - "with identifier %s and kind %s after %.0f seconds. Try again.", - new_snapshot_id, - self.commit_user, - commit_identifier, - commit_kind, - commit_time_s, + "Failed to close snapshot commit; ignoring because it " + "must not override the commit outcome.", + exc_info=True, ) - return RetryResult(latest_snapshot, None, base_data_files=base_data_files) - except Exception as e: + + if commit_exc is not None: + if _is_deterministic_atomic_commit_failure(commit_exc): + logger.warning( + "Atomic commit was rejected deterministically; do not retry.", + exc_info=commit_exc, + ) + clean_up_rejected_commit() + # CommitConflictError so the caller aborts the staged files. + raise DeterministicCommitRejectionError( + "Atomic commit was rejected deterministically.") from commit_exc # 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) + logger.warning("Retry commit for exception.", exc_info=commit_exc) + return RetryResult( + latest_snapshot, + commit_exc, + base_data_files=base_data_files, + outcome_unknown=True, + ) + + if not success: + commit_time_s = (int(time.time() * 1000) - start_millis) / 1000 + logger.warning( + "Atomic commit failed for snapshot #%d by user %s " + "with identifier %s and kind %s after %.0f seconds. Try again.", + new_snapshot_id, + self.commit_user, + commit_identifier, + commit_kind, + commit_time_s, + ) + clean_up_rejected_commit() + return RetryResult( + latest_snapshot, None, base_data_files=base_data_files) logger.info( "Successfully commit snapshot %d to table %s by user %s " @@ -649,8 +857,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 +988,67 @@ def _collect_manifest_entries(self, commit_messages: List[CommitMessage]) -> Lis )) return commit_entries + def _collect_row_id_base_entries(self, commit_messages): + messages = self._row_id_data_messages(commit_messages) + 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 _collect_row_id_update_ranges(self, commit_messages): + messages = self._row_id_data_messages(commit_messages) + if (not messages + or any(not getattr(msg, "row_id_update_ranges", None) + for msg in messages)): + return None + + ranges = {} + for msg in messages: + key = (tuple(msg.partition), msg.bucket) + ranges.setdefault(key, []).extend( + Range(from_, to) + for from_, to in msg.row_id_update_ranges + ) + return { + key: Range.sort_and_merge_overlap(values, True, True) + for key, values in ranges.items() + } + + @staticmethod + def _row_id_data_messages(commit_messages): + return [ + msg for msg in commit_messages + if msg.check_from_snapshot != -1 + and (msg.new_files or msg.deleted_files) + ] + 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..f0d2603ee2e1 100644 --- a/paimon-python/pypaimon/write/table_commit.py +++ b/paimon-python/pypaimon/write/table_commit.py @@ -106,6 +106,27 @@ 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 protect_committed_row_id_scope( + self, commit_messages: List[CommitMessage], committed_snapshot) -> None: + """Protect an already-committed incremental row-id window against a + later concurrent overwrite of its file groups.""" + self.file_store_commit.protect_committed_row_id_scope( + commit_messages, committed_snapshot) + + def validate_protected_row_id_scope(self) -> None: + """Re-check that no concurrent overwrite touched an already-committed + incremental row-id window.""" + self.file_store_commit.validate_protected_row_id_scope() + 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..0a0614f88e69 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, @@ -589,12 +622,21 @@ def _write_group( if new_files: self._assign_update_file_metadata( new_files, first_row_id, column_names, blob_columns) + update_ranges = [ + (range_.from_, range_.to) + for range_ in Range.to_ranges( + data[SpecialFields.ROW_ID.name].to_pylist()) + ] self.commit_messages.append( CommitMessage( partition=partition_tuple, 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), + row_id_update_ranges=update_ranges, ) ) success = True