Implement write.metadata.delete-after-commit.enabled to clean up old metadata files

[kaushiksrini]

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]

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]

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

[kevinjqliu]

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

[kevinjqliu]

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

[kevinjqliu]

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]

@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.

[kevinjqliu]

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,

[kevinjqliu]

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

[kaushiksrini]

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)

[kevinjqliu]

LGTM! Thanks for following up

[Fokko]

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

[kaushiksrini]

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

[kevinjqliu]

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