qzyu999 commented on code in PR #3131:
URL: https://github.com/apache/iceberg-python/pull/3131#discussion_r3090758831
##########
pyiceberg/table/update/snapshot.py:
##########
@@ -667,6 +667,96 @@ def _get_entries(manifest: ManifestFile) ->
list[ManifestEntry]:
return []
+class _RewriteFiles(_SnapshotProducer["_RewriteFiles"]):
+ """A snapshot producer that rewrites data files."""
+
+ def __init__(self, operation: Operation, transaction: Transaction, io:
FileIO, snapshot_properties: dict[str, str]):
+ super().__init__(operation, transaction, io,
snapshot_properties=snapshot_properties)
+
+ def _commit(self) -> UpdatesAndRequirements:
+ # Only produce a commit when there is something to rewrite
+ if self._deleted_data_files or self._added_data_files:
+ # Grab the entries that we actually found in the table's manifests
+ deleted_entries = self._deleted_entries()
+ found_deleted_files = {entry.data_file for entry in
deleted_entries}
+
+ # If the user asked to delete files that aren't in the table,
abort.
+ if len(found_deleted_files) != len(self._deleted_data_files):
+ raise ValueError("Cannot delete files that are not present in
the table")
+
+ added_records = sum(f.record_count for f in self._added_data_files)
+ deleted_records = sum(entry.data_file.record_count for entry in
deleted_entries)
+
+ if added_records > deleted_records:
Review Comment:
Hi @geruh, thanks for flagging this, you're right that this is a safety
guard, but it doesn't yet factor in future changes when adding delete file
rewriting. Should we add something like this?
```python
# Note: This physical record count invariant is a sanity guard for data file
# compaction to ensure no data is accidentally duplicated or invented.
# TODO: This will need to be evolved into a logical record count validation
# once PyIceberg supports rewriting delete files (Merge-on-Read).
added_records = sum(f.record_count for f in self._added_data_files)
deleted_records = sum(entry.data_file.record_count for entry in
deleted_entries)
if added_records > deleted_records:
raise ValueError(f"Invalid replace: records added ({added_records})
exceeds records removed ({deleted_records})")
```
This logical record count validation would involve something like having the
`_commit` method to do the following, which the codebase currently cannot do:
- Identify associated Delete Files: For every `DataFile` you are deleting,
you would need to find every Position Delete or Equality Delete file that
points to it.
- Calculate the "Subtraction": You would need to subtract those delete row
counts from the physical record_count of the old files to find the Old Logical
Count.
- Compare: You would then verify that Old Logical Count == New Logical Count.
The current `_RewriteFiles` implementation is "blind" to deletes. It only
tracks `_added_data_files` and `_deleted_data_files`.
I believe this can be part of a full MoR implementation, something that I
would love to work on after finishing these maintenance tasks.
Otherwise, I can also remove it from `_RerwriteFiles` and move forward, WDYT?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
