Implement `write.metadata.delete-after-commit.enabled` to clean up old metadata files by kaushiksrini · Pull Request #1607 · apache/iceberg-python

kaushiksrini · 2025-02-04T14:17:43Z

Implements property write.metadata.delete-after-commit.enabled from https://iceberg.apache.org/docs/1.5.1/maintenance/#remove-old-metadata-files.

Closes #1199

Fokko

Thanks for adding this @kaushiksrini 🙌 This looks great.

I have one minor style comment. Could you also add this to the docs? Thanks!

Fokko · 2025-02-07T17:20:42Z

tests/catalog/test_sql.py

+        transaction = table.transaction()
+        update = transaction.update_schema()
+        update.add_column(path=f"new_column_{i}", field_type=IntegerType())
+        update.commit()
+        transaction.commit_transaction()


Could you use the context managers? I think that's easier to read

Suggested change

transaction = table.transaction()

update = transaction.update_schema()

update.add_column(path=f"new_column_{i}", field_type=IntegerType())

update.commit()

transaction.commit_transaction()

with table.transaction() as transaction:

with transaction.update_schema() as update:

update.add_column(path=f"new_column_{i}", field_type=IntegerType())

kaushiksrini · 2025-02-07T21:40:54Z

@Fokko thanks! used context managers and added to the documentation

kevinjqliu · 2025-02-08T18:21:54Z

mkdocs/docs/configuration.md

 | `write.parquet.page-row-limit`           | Number of rows                     | 20000                      | Set a target threshold for the maximum number of rows within a column chunk                                                                          |
 | `write.parquet.dict-size-bytes`          | Size in bytes                      | 2MB                        | Set the dictionary page size limit per row group                                                                                                     |
 | `write.metadata.previous-versions-max`   | Integer                            | 100                        | The max number of previous version metadata files to keep before deleting after commit.                                                              |
+| `write.metadata.delete-after-commit.enabled` | Boolean                        | False                      | Whether to automatically delete old *tracked* metadata files after each table commit. It will retain a number of the most recent metadata files, which can be set using property `write.metadata.previous-versions-max`. |


👍
https://iceberg.apache.org/docs/1.6.0/maintenance/#remove-old-metadata-files

kevinjqliu · 2025-02-08T18:25:39Z

pyiceberg/catalog/__init__.py

+        if current_table is not None:
+            self._delete_old_metadata(io, current_table.metadata, updated_metadata)


is this the right place for this operation?
looking at the java implementation, changes are committed to the table before old metadata files are deleted
https://github.com/apache/iceberg/blob/d935460bbed5da0ac205a67912b70fa682ee84df/core/src/main/java/org/apache/iceberg/BaseMetastoreTableOperations.java#L125-L126

At this point, changes are only applied to the table metadata but is not yet committed

the java implementation uses deleteAfterCommit

https://github.com/apache/iceberg/blob/d935460bbed5da0ac205a67912b70fa682ee84df/core/src/main/java/org/apache/iceberg/CatalogUtil.java#L533

kevinjqliu

_delete_old_metadata should be after table is committed

kaushiksrini · 2025-02-09T17:19:01Z

@kevinjqliu thanks for the review! I moved the logic after the table is committed

kevinjqliu

Generally LGTM! Thanks for the PR. I think _do_commit is the right place to add this.

pyiceberg/catalog/__init__.py

kevinjqliu · 2025-02-09T17:32:44Z

pyiceberg/table/__init__.py

+
+        # https://github.com/apache/iceberg/blob/f6faa58/core/src/main/java/org/apache/iceberg/CatalogUtil.java#L527
+        # delete old metadata if METADATA_DELETE_AFTER_COMMIT_ENABLED is set to true
+        self.catalog._delete_old_metadata(self.io, self.metadata, response.metadata)


similar to https://github.com/apache/iceberg/blob/f6faa58dac57e03be6e02a43937ac7c15c770225/core/src/main/java/org/apache/iceberg/CatalogUtil.java#L539-L544

can we add a comment here explaining how METADATA_PREVIOUS_VERSIONS_MAX is taken into account?

iceberg-python/pyiceberg/table/update/__init__.py

Line 576 in 826a006

TableProperties.METADATA_PREVIOUS_VERSIONS_MAX,

also maybe we want to wrap this in try/catch and throw a warning as to not block the commit process

added a try-catch, but the deleteFiles already has a warning for actually deleting the files. I could only see an exception arise from _delete_old_metadata's operations.

iceberg-python/pyiceberg/catalog/__init__.py

Lines 254 to 269 in dd175aa

def delete_files(io: FileIO, files_to_delete: Set[str], file_type: str) -> None:

"""Delete files.

Log warnings if failing to delete any file.

Args:

io: The FileIO used to delete the object.

files_to_delete: A set of file paths to be deleted.

file_type: The type of the file.

"""

for file in files_to_delete:

try:

io.delete(file)

except OSError as exc:

logger.warning(msg=f"Failed to delete {file_type} file {file}", exc_info=exc)

tests/catalog/test_sql.py

kevinjqliu

LGTM! Thanks for following up

tests/catalog/test_sql.py

Fokko

I've tested this locally, and it works great. Thanks @kaushiksrini for working on this, and thanks @kevinjqliu for the review 👍

tests/catalog/test_sql.py

kaushiksrini · 2025-02-12T04:27:22Z

@Fokko thanks for the review! and @kevinjqliu thanks for making the changes + review!

kevinjqliu · 2025-02-12T04:38:41Z

Thanks @kaushiksrini I applied the simple test changes via github. Thanks @Fokko for the review

kaushiksrini added 4 commits February 4, 2025 01:29

feat: remove old metadata files and add setting

0985972

add test for delete after commit

d244ffd

feat: delete old metadata after commit table

4807040

reformatting

352b972

Fokko reviewed Feb 7, 2025

View reviewed changes

kaushiksrini added 4 commits February 7, 2025 16:24

use context managers

a375e1c

add docs to configuration

1f631c2

reorder comment

7e4c4b3

Merge branch 'main' into remove-old-metadata

d1ed69c

kevinjqliu reviewed Feb 8, 2025

View reviewed changes

kevinjqliu requested changes Feb 8, 2025

View reviewed changes

kaushiksrini added 4 commits February 9, 2025 11:07

remove call from catalog; convert to static method

f440ac1

call after commit

4b36bfc

remove call from test

f7b7c03

few lint fixes

d30bbaf

kevinjqliu reviewed Feb 9, 2025

View reviewed changes

kaushiksrini added 4 commits February 9, 2025 14:56

add comment and remove parameterized test

01fb388

reset io change in catalog

462f3d4

add comments, wrap around exception and warning

45516dc

remove additional new line

e8277c8

kevinjqliu approved these changes Feb 9, 2025

View reviewed changes

remove table identifier from test function

ed7a705

kevinjqliu reviewed Feb 11, 2025

View reviewed changes

tests/catalog/test_sql.py Outdated Show resolved Hide resolved

Fokko changed the title ~~Remove old metadata~~ Clean up old metadata Feb 11, 2025

Fokko approved these changes Feb 11, 2025

View reviewed changes

kevinjqliu reviewed Feb 12, 2025

View reviewed changes

tests/catalog/test_sql.py Outdated Show resolved Hide resolved

Apply suggestions from code review

ec3de23

kevinjqliu merged commit d6dce6d into apache:main Feb 12, 2025
8 checks passed

kevinjqliu changed the title ~~Clean up old metadata~~ Implement write.metadata.delete-after-commit.enabled to clean up old metadata files Feb 12, 2025

kevinjqliu changed the title ~~Implement write.metadata.delete-after-commit.enabled to clean up old metadata files~~ Implement write.metadata.delete-after-commit.enabled to clean up old metadata files Feb 12, 2025

		if current_table is not None:
		self._delete_old_metadata(io, current_table.metadata, updated_metadata)

	def delete_files(io: FileIO, files_to_delete: Set[str], file_type: str) -> None:
	"""Delete files.

	Log warnings if failing to delete any file.

	Args:
	io: The FileIO used to delete the object.
	files_to_delete: A set of file paths to be deleted.
	file_type: The type of the file.
	"""
	for file in files_to_delete:
	try:
	io.delete(file)
	except OSError as exc:
	logger.warning(msg=f"Failed to delete {file_type} file {file}", exc_info=exc)

Conversation

kaushiksrini commented Feb 4, 2025

Uh oh!

Fokko left a comment

Choose a reason for hiding this comment

Uh oh!

Fokko Feb 7, 2025

Choose a reason for hiding this comment

Uh oh!

kaushiksrini commented Feb 7, 2025

Uh oh!

kevinjqliu Feb 8, 2025

Choose a reason for hiding this comment

Uh oh!

kevinjqliu Feb 8, 2025

Choose a reason for hiding this comment

Uh oh!

kevinjqliu Feb 8, 2025

Choose a reason for hiding this comment

Uh oh!

kevinjqliu left a comment

Choose a reason for hiding this comment

Uh oh!

kaushiksrini commented Feb 9, 2025

Uh oh!

kevinjqliu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kevinjqliu Feb 9, 2025

Choose a reason for hiding this comment

Uh oh!

kevinjqliu Feb 9, 2025

Choose a reason for hiding this comment

Uh oh!

kaushiksrini Feb 9, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kevinjqliu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Fokko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kaushiksrini commented Feb 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

kevinjqliu commented Feb 12, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kaushiksrini commented Feb 12, 2025 •

edited

Loading