rambleraptor commented on code in PR #3775:
URL: https://github.com/apache/iceberg-python/pull/3775#discussion_r3752872062
##########
pyiceberg/table/update/snapshot.py:
##########
@@ -595,8 +596,15 @@ def _existing_manifests(self) -> list[ManifestFile]:
manifest_evaluators: dict[int, Callable[[ManifestFile], bool]] =
KeyDefaultDict(self._build_manifest_evaluator)
if snapshot :=
self._transaction.table_metadata.snapshot_by_name(name=self._target_branch):
for manifest_file in snapshot.manifests(io=self._io):
+ spec = self.spec(manifest_file.partition_spec_id)
+
+ # Optimization: only use manifest evaluator pruning for
identity-only transforms
+ # to avoid false negatives with non-identity transforms
(#3758).
+ # For non-identity transforms, fall back to always checking
exact file identity.
+ spec_is_identity_only = all(isinstance(field.transform,
IdentityTransform) for field in spec.fields)
+
# Manifest does not contain rows that match the files to
delete partitions
- if not
manifest_evaluators[manifest_file.partition_spec_id](manifest_file):
+ if spec_is_identity_only and not
manifest_evaluators[manifest_file.partition_spec_id](manifest_file):
Review Comment:
Agree on this. `spec_is_identity_only` additionally raises questions about
what happens when there's a mixture of identity / non-identity transforms.
--
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]
