feat: delete orphaned files #1958

jayceslesar · 2025-04-29T22:42:05Z

Closes #1200

Rationale for this change

Ability to do more table maintenance from pyiceberg (iceberg-python?)

Are these changes tested?

Added a test!

Are there any user-facing changes?

Yes, this is a new method on the Table class.

pyiceberg/table/__init__.py

pyiceberg/table/inspect.py

pyiceberg/table/__init__.py

Fokko

Thanks for working on this @jayceslesar, sorry for the late review.

I think this is a great start, I left some comments, let me know what you think!

pyiceberg/table/__init__.py

pyiceberg/table/inspect.py

smaheshwar-pltr

Thanks for the PR @jayceslesar, using InpsectTable to get orphaned files to submit to the executor pool is a nice idea! Just some concerns / suggestions / debugging help 😄

pyiceberg/table/inspect.py

smaheshwar-pltr · 2025-05-03T16:44:25Z

pyiceberg/table/inspect.py

+
+        from pyiceberg.io.pyarrow import _fs_from_file_path
+
+        all_known_files = set()


We also want to have manifest list files here (I don't see them now). Otherwise, they'll be removed by the procedure and the table will be "corrupted".

(Related: when looking at Java tests, I noticed apache/iceberg#12957)

The same goes for the current metadata JSON file, and I think to match Java behaviour we want to include all files in the metadata log of the current metadata file too.

I think there are more files we might be missing - I think tests would be nice to make sure we're not missing something! (Perhaps inspiration can be taken from the Java ones)

I see! I just pushed a change that will capture those, as well as the statistic file paths

pyiceberg/table/inspect.py

kevinjqliu

Thanks for the PR! I added a few comments. ptal :)

pyiceberg/table/__init__.py

kevinjqliu · 2025-05-04T00:43:20Z

pyiceberg/table/__init__.py

+                deletes = executor.map(_delete, orphaned_files)
+                # exhaust
+                list(deletes)
+                logger.info(f"Deleted {len(orphaned_files)} orphaned files at {location}!")


nit: this might not necessary be always true, esp when _delete errors are suppressed.

what we do count the number of successfully deletes here? maybe _delete can return True/False for whether the delete was successful.

the spark procedure outputs the orphan_file_location which are all the files set to be deleted. this is pretty useful for logging
https://iceberg.apache.org/docs/nightly/spark-procedures/#output_7

Just modified, let me know!

pyiceberg/table/inspect.py

pyiceberg/table/__init__.py

kevinjqliu · 2025-05-04T00:51:06Z

pyiceberg/table/inspect.py

+
+        return _all_known_files
+
+    def orphaned_files(self, location: str, older_than: Optional[timedelta] = timedelta(days=3)) -> Set[str]:


nit: should we expose this as a public function given that there's no equivalent from java/spark side? we modeled the inspect tables based on java's metadata tables.
maybe we can change this to _orphaned_files for now

Does this still need to be addressed now that this is under a new namespace?

Yes :) The less we expose publicly, the easier it is to evolve the API in the future (otherwise we have to go through a deprecation cycle).

pyiceberg/table/inspect.py

kevinjqliu · 2025-05-04T01:21:41Z

a meta question, wydt of moving the orphan file function to its own file/namespace, similar to how to use .inspect.

i like the idea of having all the table maintenance functions together, similar to delta table's optimize

jayceslesar · 2025-05-04T16:53:12Z

a meta question, wydt of moving the orphan file function to its own file/namespace, similar to how to use .inspect.

i like the idea of having all the table maintenance functions together, similar to delta table's optimize

I think that makes sense -- would #1880 end up there too?

Also ideally there is a CLI that exposes all the maintenance actions too right?

I think moving things to a new OptimizeTable class in a new namespace optimize.py makes a lot of sense, can be modeled very similar to the InspectTable and generally makes things cleaner -- I think it still makes sense to have the all_known_files inside of inspect though, and can still use that in the new OptimizeTable

Fokko · 2025-05-13T14:42:37Z

i like the idea of having all the table maintenance functions together, similar to delta table's optimize

That's a good point. However, I think we should be able to either run them separate as well. For example, delete orphan files won't affect the speed of the table, so it is more of a maintenance feature to reduce object storage costs. Delete orphan files can also be pretty costly because of the list operation, ideally you would delegate this to the catalog that uses, for example, s3 inventory.

Fokko · 2025-05-16T11:28:59Z

pyiceberg/table/__init__.py

@@ -1023,6 +1024,15 @@ def inspect(self) -> InspectTable:
        """
        return InspectTable(self)

+    @property
+    def optimize(self) -> OptimizeTable:


As mentioned before, I don't think deleting orphan files will optimize the table. Since Iceberg does not list any prefixes, there won't be any improvements (such as listing fewer objects). How about changing this to maintenance?

Fokko · 2025-05-16T11:32:00Z

pyiceberg/table/inspect.py

+        # coerce into snapshot objects if users passes in snapshot ids
+        if snapshots is not None:
+            if isinstance(snapshots[0], int):
+                snapshots = cast(list[Snapshot], [self.tbl.metadata.snapshot_by_id(snapshot_id) for snapshot_id in snapshots])


I don't think this cast is the correct way to fix this, since it returns a List[Optional[Snapshot]], so this might cause issues when you push snapshot-IDs that do not exist. Instead we should filter out the None's.

Fokko · 2025-05-16T11:33:53Z

pyiceberg/table/inspect.py

@@ -678,6 +685,28 @@ def all_manifests(self) -> "pa.Table":
        )
        return pa.concat_tables(manifests_by_snapshots)

+    def _all_known_files(self) -> dict[str, set[str]]:


Do we still need this now we have all_files?

I dont think that all_files includes manifest lists or statistics files right?

It should reference deletes, but thinking of it, this is probably much more efficient because it does not pull in all the other data.

Fokko · 2025-05-16T11:37:12Z

pyiceberg/table/optimize.py

+        except ModuleNotFoundError as e:
+            raise ModuleNotFoundError("For metadata operations PyArrow needs to be installed") from e
+
+    def orphaned_files(self, location: str, older_than: timedelta = timedelta(days=3)) -> Set[str]:


Do we want to expose both oprhaned_files and remove_ophaned_files to the public? How about making this one private:

Suggested change

def orphaned_files(self, location: str, older_than: timedelta = timedelta(days=3)) -> Set[str]:

def _orphaned_files(self, location: str, older_than: timedelta = timedelta(days=3)) -> Set[str]:

jayceslesar and others added 3 commits April 29, 2025 16:58

feat: delete orphaned files

9dcb580

simpler and a test

e43505c

remove

eed5ea8

jayceslesar commented Apr 29, 2025

View reviewed changes

pyiceberg/table/__init__.py Outdated Show resolved Hide resolved

jayceslesar commented Apr 29, 2025

View reviewed changes

pyiceberg/table/inspect.py Outdated Show resolved Hide resolved

jayceslesar commented Apr 29, 2025

View reviewed changes

pyiceberg/table/__init__.py Outdated Show resolved Hide resolved

Fokko reviewed May 2, 2025

View reviewed changes

jayceslesar added 3 commits May 2, 2025 17:22

updates from review!

8cca600

include dry run and older than

75b1240

add case for dry run

6379480

smaheshwar-pltr suggested changes May 3, 2025

View reviewed changes

jayceslesar added 7 commits May 3, 2025 14:16

use .path so we get paths pack

0c2822e

actually pass in iterable

aaf8fc2

capture manifest_list files

b09641b

refactor into all_known_files

beec233

fix type in docstring

b888c56

mildly more readable

ff461ed

beef up tests

3b3b10e

kevinjqliu reviewed May 4, 2025

View reviewed changes

jayceslesar added 4 commits May 4, 2025 12:54

make older_than required

a62c8cf

move under optimize namespace

07cbf1b

add some better logging about what was/was not deleted

54e1e00

Merge branch 'main' into feat/orphan-files

7c780d3

Fokko mentioned this pull request May 13, 2025

Add all filles metadata tables #1626

Merged

Merge branch 'main' into feat/orphan-files

9b6c9ed

Fokko reviewed May 16, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: delete orphaned files #1958

feat: delete orphaned files #1958

jayceslesar commented Apr 29, 2025

Fokko left a comment

smaheshwar-pltr left a comment

smaheshwar-pltr May 3, 2025

smaheshwar-pltr May 3, 2025

jayceslesar May 3, 2025

kevinjqliu left a comment

kevinjqliu May 4, 2025

kevinjqliu May 4, 2025

jayceslesar May 4, 2025

kevinjqliu May 4, 2025

jayceslesar May 4, 2025

Fokko May 16, 2025 •

edited

Loading

kevinjqliu commented May 4, 2025

jayceslesar commented May 4, 2025 •

edited

Loading

Fokko commented May 13, 2025

Fokko May 16, 2025

Fokko May 16, 2025 •

edited

Loading

Fokko May 16, 2025

jayceslesar May 16, 2025

Fokko May 16, 2025 •

edited

Loading

Fokko May 16, 2025


		from pyiceberg.io.pyarrow import _fs_from_file_path

		all_known_files = set()


		return _all_known_files

		def orphaned_files(self, location: str, older_than: Optional[timedelta] = timedelta(days=3)) -> Set[str]:

	def orphaned_files(self, location: str, older_than: timedelta = timedelta(days=3)) -> Set[str]:
	def _orphaned_files(self, location: str, older_than: timedelta = timedelta(days=3)) -> Set[str]:

feat: delete orphaned files #1958

Are you sure you want to change the base?

feat: delete orphaned files #1958

Conversation

jayceslesar commented Apr 29, 2025

Rationale for this change

Are these changes tested?

Are there any user-facing changes?

Fokko left a comment

Choose a reason for hiding this comment

smaheshwar-pltr 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

kevinjqliu 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

Fokko May 16, 2025 • edited Loading

Choose a reason for hiding this comment

kevinjqliu commented May 4, 2025

jayceslesar commented May 4, 2025 • edited Loading

Fokko commented May 13, 2025

Choose a reason for hiding this comment

Fokko May 16, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fokko May 16, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fokko May 16, 2025 •

edited

Loading

jayceslesar commented May 4, 2025 •

edited

Loading

Fokko May 16, 2025 •

edited

Loading

Fokko May 16, 2025 •

edited

Loading