Rewrite manifests #1661

amitgilad3 · 2025-02-13T20:21:24Z

This is an initial implementation of rewrite manifests, aiming to mimic the Java implementation as closely as possible. I’ve tried to follow the same structure and logic, but there are still some areas that might need refinement.

I’m looking for feedback and suggestions on:
• Whether the approach aligns well with the existing design.
• Any gaps or optimizations that could improve performance.
• How best to proceed with completing this feature.

Would love any insights or guidance on the next steps! Thanks in advance for the review! 🙌

Fokko

Sorry for the many comments, most of them are easy to resolve. Thanks @amitgilad3 for working on this, I like it how you reuse the _ManifestMergeManager 👍

Fokko · 2025-03-10T19:57:33Z

pyiceberg/manifest.py

@@ -861,6 +865,7 @@ def existing(self, entry: ManifestEntry) -> ManifestWriter:
                entry.snapshot_id, entry.sequence_number, entry.file_sequence_number, entry.data_file
            )
        )
+        self._existing_files += 1


I believe we already do this here:

iceberg-python/pyiceberg/manifest.py

Line 823 in 7648803

self._existing_files += 1

Fokko · 2025-03-10T19:59:01Z

pyiceberg/table/update/snapshot.py

+    rewritten_manifests: List[ManifestFile]
+    added_manifests: List[ManifestFile]


How about slapping on some default factories: https://docs.python.org/3/library/dataclasses.html#dataclasses.field

Suggested change

rewritten_manifests: List[ManifestFile]

added_manifests: List[ManifestFile]

rewritten_manifests: List[ManifestFile] = field(default_factory=list)

added_manifests: List[ManifestFile] = field(default_factory=list)

Fokko · 2025-03-10T20:00:25Z

pyiceberg/table/update/snapshot.py

@@ -477,6 +478,20 @@ def _deleted_entries(self) -> List[ManifestEntry]:
        return []


+@dataclass(init=False)


What do you think of making this Frozen? This gives some nice benefits like being hashable: https://docs.python.org/3/library/dataclasses.html#frozen-instances

Using the default_factory's, we can also drop the init.

Suggested change

@dataclass(init=False)

@dataclass(frozen=True)

pyiceberg/table/update/snapshot.py

Fokko · 2025-03-10T20:04:26Z

pyiceberg/table/update/snapshot.py

+        _table: Table
+        _spec: PartitionSpec


I don't think these are used.

Fokko · 2025-03-10T20:27:49Z

pyiceberg/table/update/snapshot.py

+    def _deleted_entries(self) -> List[ManifestEntry]:
+        """To determine if we need to record any deleted manifest entries.
+
+        In case of an append, nothing is deleted.


Copy paste :)

Fokko · 2025-03-10T20:31:02Z

pyiceberg/table/update/snapshot.py

+        self.rewritten_manifests.extend(deletes_result.rewritten_manifests)
+        self.added_manifests.extend(deletes_result.added_manifests)
+
+        if not self.rewritten_manifests:


nit, just for clarity that it is a list:

Suggested change

if not self.rewritten_manifests:

if len(self.rewritten_manifests) == 0:

Fokko · 2025-03-10T20:34:44Z

pyiceberg/table/update/snapshot.py

+            min_count_to_merge=2,
+            merge_enabled=True,


I think in Java we set the properties from the table properties:

https://github.com/apache/iceberg/blob/6e8718113c08aebf76d8e79a9e2534c89c73407a/core/src/main/java/org/apache/iceberg/MergingSnapshotProducer.java#L113-L119

Fokko · 2025-03-10T20:35:46Z

tests/integration/test_writes/test_rewrite_manifests.py

+
+    tbl.append(arrow_table_with_null)
+
+    assert tbl.format_version == 2, f"Expected v2, got: v{tbl.format_version}"


What are we testing here? I would love to see if we could rewrite the V1 and V2 manifest into a V2 one.

Fokko · 2025-03-10T20:37:02Z

tests/integration/test_writes/test_rewrite_manifests.py

+    assert len(result.rewritten_manifests) == 4, "Action should rewrite 4 manifests"
+    assert len(result.added_manifests) == 2, "Action should add 2 manifests"
+
+    new_manifests = tbl.current_snapshot().manifests(tbl.io)


Nothing wrong with this, but tbl.inspect might also be helpful to assert the manifests.

Fokko · 2025-04-17T13:39:59Z

@amitgilad3 gentle ping, are you still interested in working on this?

amitgilad3 · 2025-04-17T14:04:39Z

Hey @Fokko , yes i am very interested in finishing this, must of missed this (sorry) , will look at this later today :)

2. write tests for rewrite manifests

Fokko · 2025-05-16T21:16:58Z

Looks like the CI is sad 😞

tests/integration/test_writes/test_rewrite_manifests.py:154: error: "rewrite_manifests" of "Table" does not return a value (it only ever returns None)  [func-returns-value]
tests/integration/test_writes/test_rewrite_manifests.py:227: error: "rewrite_manifests" of "Table" does not return a value (it only ever returns None)  [func-returns-value]

Fokko · 2025-05-16T21:19:26Z

pyiceberg/table/update/snapshot.py

+        ssc = SnapshotSummaryCollector()
+        partition_summary_limit = int(
+            self._transaction.table_metadata.properties.get(
+                TableProperties.WRITE_PARTITION_SUMMARY_LIMIT, TableProperties.WRITE_PARTITION_SUMMARY_LIMIT_DEFAULT
+            )
+        )
+        ssc.set_partition_summary_limit(partition_summary_limit)


Suggested change

ssc = SnapshotSummaryCollector()

partition_summary_limit = int(

self._transaction.table_metadata.properties.get(

TableProperties.WRITE_PARTITION_SUMMARY_LIMIT, TableProperties.WRITE_PARTITION_SUMMARY_LIMIT_DEFAULT

)

)

ssc.set_partition_summary_limit(partition_summary_limit)

ssc = SnapshotSummaryCollector(int(

self._transaction.table_metadata.properties.get(

TableProperties.WRITE_PARTITION_SUMMARY_LIMIT, TableProperties.WRITE_PARTITION_SUMMARY_LIMIT_DEFAULT

)

))

amitgilad3 marked this pull request as draft February 13, 2025 20:21

Fokko self-requested a review February 18, 2025 15:10

Fokko approved these changes Mar 10, 2025

View reviewed changes

kevinjqliu mentioned this pull request Mar 11, 2025

Support metadata compaction #270

Open

amitgilad3 and others added 8 commits May 15, 2025 22:34

first attempt at init_manifests

14c27cf

initial implementation of rewrite_manifests

ee0392d

fix lint

7b82638

remove unused code

15a8f6a

commit

443ffda

1. support returning rewrite results

10affa8

2. write tests for rewrite manifests

add test

82d0d3a

fixes according to pr

6913f09

amitgilad3 force-pushed the rewrite_manifests branch from abbb01c to 6913f09 Compare May 15, 2025 19:35

more fixes

74e42e0

Fokko marked this pull request as ready for review May 16, 2025 21:11

Fokko reviewed May 16, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rewrite manifests #1661

Rewrite manifests #1661

amitgilad3 commented Feb 13, 2025 •

edited

Loading

Fokko left a comment

Fokko Mar 10, 2025

Fokko Mar 10, 2025

Fokko Mar 10, 2025

Fokko Mar 10, 2025

Fokko Mar 10, 2025

Fokko Mar 10, 2025

Fokko Mar 10, 2025

Fokko Mar 10, 2025

Fokko Mar 10, 2025

Fokko commented Apr 17, 2025

amitgilad3 commented Apr 17, 2025

Fokko commented May 16, 2025

Fokko May 16, 2025

		rewritten_manifests: List[ManifestFile]
		added_manifests: List[ManifestFile]

		@@ -477,6 +478,20 @@ def _deleted_entries(self) -> List[ManifestEntry]:
		return []


		@dataclass(init=False)

	if not self.rewritten_manifests:
	if len(self.rewritten_manifests) == 0:


		tbl.append(arrow_table_with_null)

		assert tbl.format_version == 2, f"Expected v2, got: v{tbl.format_version}"

Rewrite manifests #1661

Are you sure you want to change the base?

Rewrite manifests #1661

Conversation

amitgilad3 commented Feb 13, 2025 • edited Loading

Fokko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fokko commented Apr 17, 2025

amitgilad3 commented Apr 17, 2025

Fokko commented May 16, 2025

Choose a reason for hiding this comment

amitgilad3 commented Feb 13, 2025 •

edited

Loading