Skip to content

Commit 631b53d

Browse files
moomindaniIsaac
andcommitted
Keep concurrent commits on rewrite retry and check the rewrite keeps every active file
A rewrite that raced an append failed its retry on the serializable added-files check, even though the retry replans against the new head. Skip that check, as Java does, and instead refuse a rewrite whose new manifests do not hold the same number of active files as the ones they replace. Co-authored-by: Isaac <[email protected]>
1 parent cadafef commit 631b53d

2 files changed

Lines changed: 63 additions & 1 deletion

File tree

‎pyiceberg/table/update/snapshot.py‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1314,6 +1314,15 @@ def older_than(self, dt: datetime) -> ExpireSnapshots:
13141314
return self
13151315

13161316

1317+
def _active_files_count(manifests: list[ManifestFile]) -> int:
1318+
count = 0
1319+
for manifest in manifests:
1320+
if manifest.added_files_count is None or manifest.existing_files_count is None:
1321+
raise ValueError(f"Missing file counts in {manifest.manifest_path}")
1322+
count += manifest.added_files_count + manifest.existing_files_count
1323+
return count
1324+
1325+
13171326
class RewriteManifests(_SnapshotProducer["RewriteManifests"]):
13181327
"""Rewrite the current snapshot's data manifests without changing data.
13191328
@@ -1370,6 +1379,9 @@ def rewrite_if(self, predicate: Callable[[ManifestFile], bool]) -> RewriteManife
13701379
self._manifest_predicate = predicate
13711380
return self
13721381

1382+
def _validate_concurrency(self) -> None:
1383+
"""Skip validation; a retry replans against the refreshed branch head, so concurrent commits are kept."""
1384+
13731385
def _deleted_entries(self) -> list[ManifestEntry]:
13741386
return []
13751387

@@ -1419,6 +1431,7 @@ def _existing_manifests(self) -> list[ManifestFile]:
14191431
kept_manifests.append(manifest)
14201432

14211433
new_manifests: list[ManifestFile] = []
1434+
rewritten_manifests: list[ManifestFile] = []
14221435
for spec_id, manifests in data_manifests_by_spec.items():
14231436
for group in self._group_by_target_size(manifests):
14241437
if len(group) == 1 and self._manifest_predicate is None:
@@ -1438,9 +1451,18 @@ def _existing_manifests(self) -> list[ManifestFile]:
14381451
self._entries_processed += 1
14391452

14401453
new_manifests.append(writer.to_manifest_file())
1454+
rewritten_manifests.extend(group)
14411455
self._created_count += 1
14421456
self._rewritten_count += len(group)
14431457

1458+
created_files_count = _active_files_count(new_manifests)
1459+
replaced_files_count = _active_files_count(rewritten_manifests)
1460+
if created_files_count != replaced_files_count:
1461+
raise ValidationException(
1462+
"Replaced and created manifests must have the same number of active files: "
1463+
f"{created_files_count} (new), {replaced_files_count} (old)"
1464+
)
1465+
14441466
self._kept_count = len(kept_manifests)
14451467
self.snapshot_properties = {
14461468
**self.snapshot_properties,

‎tests/table/test_rewrite_manifests.py‎

Lines changed: 41 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,13 +15,15 @@
1515
# specific language governing permissions and limitations
1616
# under the License.
1717
from pathlib import Path
18+
from typing import Any
1819

1920
import pyarrow as pa
2021
import pytest
2122

2223
from pyiceberg.catalog import Catalog
2324
from pyiceberg.catalog.memory import InMemoryCatalog
24-
from pyiceberg.manifest import ManifestContent, ManifestFile
25+
from pyiceberg.exceptions import ValidationException
26+
from pyiceberg.manifest import ManifestContent, ManifestEntry, ManifestFile
2527
from pyiceberg.table import Table
2628
from pyiceberg.table.snapshots import Operation
2729

@@ -303,3 +305,41 @@ def test_rewrites_needed_is_false_when_every_group_is_a_single_manifest(catalog:
303305
table = catalog.load_table("default.test_rewrite")
304306
assert table.current_snapshot() == snapshot_before
305307
assert len(_data_manifests(table)) == 3
308+
309+
310+
def test_rewrite_manifests_replans_after_concurrent_append(catalog: Catalog) -> None:
311+
_create_table_with_appends(catalog, appends=3)
312+
rewrite = catalog.load_table("default.test_rewrite").maintenance.rewrite_manifests()
313+
314+
# another writer commits between planning and committing the rewrite
315+
catalog.load_table("default.test_rewrite").append(_arrow_table(offset=9))
316+
317+
rewrite.commit()
318+
319+
table = catalog.load_table("default.test_rewrite")
320+
snapshot = table.current_snapshot()
321+
assert snapshot is not None
322+
assert snapshot.summary is not None
323+
assert snapshot.summary.operation == Operation.REPLACE
324+
# the concurrently appended file is merged in, not dropped
325+
assert sorted(table.scan().to_arrow()["id"].to_pylist()) == list(range(1, 13))
326+
assert len(_data_manifests(table)) == 1
327+
assert snapshot.summary["manifests-replaced"] == "4"
328+
329+
330+
def test_rewrite_manifests_rejects_rewrite_that_drops_files(catalog: Catalog, monkeypatch: pytest.MonkeyPatch) -> None:
331+
table = _create_table_with_appends(catalog, appends=3)
332+
snapshot_before = table.current_snapshot()
333+
lost_manifest_path = _data_manifests(table)[0].manifest_path
334+
fetch_manifest_entry = ManifestFile.fetch_manifest_entry
335+
336+
def fetch_losing_one_manifest(self: ManifestFile, *args: Any, **kwargs: Any) -> list[ManifestEntry]:
337+
entries = fetch_manifest_entry(self, *args, **kwargs)
338+
return [] if self.manifest_path == lost_manifest_path else entries
339+
340+
monkeypatch.setattr(ManifestFile, "fetch_manifest_entry", fetch_losing_one_manifest)
341+
342+
with pytest.raises(ValidationException, match="same number of active files: 2 \\(new\\), 3 \\(old\\)"):
343+
table.maintenance.rewrite_manifests().commit()
344+
345+
assert catalog.load_table("default.test_rewrite").current_snapshot() == snapshot_before

0 commit comments

Comments
 (0)