You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "kaushiksrini (via GitHub)" <gi...@apache.org> on 2025/05/28 01:48:06 UTC
[PR] Feat/validate added data files [iceberg-python]
kaushiksrini opened a new pull request, #2050:
URL: https://github.com/apache/iceberg-python/pull/2050
<!--
Thanks for opening a pull request!
-->
<!-- In the case this PR will resolve an issue, please replace ${GITHUB_ISSUE_ID} below with the actual Github issue id. -->
<!-- Closes #${GITHUB_ISSUE_ID} -->
Closes #1929
# Rationale for this change
- Since we want to support snapshot write compatibility (#1772) and is part of the following parent issue #819
# Are these changes tested?
Yes
# Are there any user-facing changes?
No
<!-- In the case of user-facing changes, please add the changelog label. -->
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] validate added data files for snapshot compatibility [iceberg-python]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on PR #2050:
URL: https://github.com/apache/iceberg-python/pull/2050#issuecomment-2932019390
@kaushiksrini Could you resolve the conflicts?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] validate added data files for snapshot compatibility [iceberg-python]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #2050:
URL: https://github.com/apache/iceberg-python/pull/2050#discussion_r2164616244
##########
pyiceberg/table/update/validate.py:
##########
@@ -150,3 +151,54 @@ def _validate_deleted_data_files(
if any(conflicting_entries):
conflicting_snapshots = {entry.snapshot_id for entry in conflicting_entries}
raise ValidationException(f"Deleted data files were found matching the filter for snapshots {conflicting_snapshots}!")
+
+
+def _added_data_files(
+ table: Table,
+ starting_snapshot: Snapshot,
+ data_filter: Optional[BooleanExpression],
+ partition_set: Optional[dict[int, set[Record]]],
+ parent_snapshot: Optional[Snapshot],
+) -> Iterator[ManifestEntry]:
+ if parent_snapshot is None:
Review Comment:
I don't think this `if` makes sense, why not make the `parent_snapshot` non-optional?
##########
pyiceberg/table/update/validate.py:
##########
@@ -77,6 +79,47 @@ def _validation_history(
return manifests_files, snapshots
+def _filter_manifest_entries(
+ entry: ManifestEntry,
+ snapshot_ids: set[int],
+ data_filter: Optional[BooleanExpression],
+ partition_set: Optional[dict[int, set[Record]]],
+ entry_status: Optional[ManifestEntryStatus],
+ schema: Schema,
+) -> bool:
+ """Filter manifest entries based on data filter and partition set.
+
+ Args:
+ entry: Manifest entry to filter
+ snapshot_ids: set of snapshot ids to match data files
+ data_filter: Optional filter to match data files
+ partition_set: Optional set of partitions to match data files
+ status: Optional status to match data files
Review Comment:
```suggestion
entry_status: Optional status to match data files
```
##########
pyiceberg/table/update/validate.py:
##########
@@ -77,6 +79,47 @@ def _validation_history(
return manifests_files, snapshots
+def _filter_manifest_entries(
+ entry: ManifestEntry,
+ snapshot_ids: set[int],
+ data_filter: Optional[BooleanExpression],
+ partition_set: Optional[dict[int, set[Record]]],
+ entry_status: Optional[ManifestEntryStatus],
+ schema: Schema,
+) -> bool:
+ """Filter manifest entries based on data filter and partition set.
+
+ Args:
+ entry: Manifest entry to filter
+ snapshot_ids: set of snapshot ids to match data files
+ data_filter: Optional filter to match data files
+ partition_set: Optional set of partitions to match data files
+ status: Optional status to match data files
+ table: Table containing the schema for filtering
Review Comment:
```suggestion
schema: schema for filtering
```
##########
pyiceberg/table/update/validate.py:
##########
@@ -108,27 +151,12 @@ def _deleted_data_files(
ManifestContent.DATA,
)
- if data_filter is not None:
- evaluator = _InclusiveMetricsEvaluator(table.schema(), data_filter).eval
-
for manifest in manifests:
for entry in manifest.fetch_manifest_entry(table.io, discard_deleted=False):
- if entry.snapshot_id not in snapshot_ids:
- continue
-
- if entry.status != ManifestEntryStatus.DELETED:
- continue
-
- if data_filter is not None and evaluator(entry.data_file) is ROWS_CANNOT_MATCH:
- continue
-
- if partition_set is not None:
- spec_id = entry.data_file.spec_id
- partition = entry.data_file.partition
- if spec_id not in partition_set or partition not in partition_set[spec_id]:
- continue
-
- yield entry
+ if _filter_manifest_entries(
+ entry, snapshot_ids, data_filter, partition_set, ManifestEntryStatus.DELETED, table.schema()
+ ):
+ yield entry
Review Comment:
Ignore this comment, but a missed opportunity for some syntactic sugar:
```python
for manifest in manifests:
yield from (
entry
for entry in manifest.fetch_manifest_entry(table.io, discard_deleted=False)
if _filter_manifest_entries(
entry, snapshot_ids, data_filter, partition_set, ManifestEntryStatus.DELETED, table.schema()
)
)
```
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] validate added data files for snapshot compatibility [iceberg-python]
Posted by "jayceslesar (via GitHub)" <gi...@apache.org>.
jayceslesar commented on code in PR #2050:
URL: https://github.com/apache/iceberg-python/pull/2050#discussion_r2164660804
##########
pyiceberg/table/update/validate.py:
##########
@@ -150,3 +151,54 @@ def _validate_deleted_data_files(
if any(conflicting_entries):
conflicting_snapshots = {entry.snapshot_id for entry in conflicting_entries}
raise ValidationException(f"Deleted data files were found matching the filter for snapshots {conflicting_snapshots}!")
+
+
+def _added_data_files(
+ table: Table,
+ starting_snapshot: Snapshot,
+ data_filter: Optional[BooleanExpression],
+ partition_set: Optional[dict[int, set[Record]]],
+ parent_snapshot: Optional[Snapshot],
+) -> Iterator[ManifestEntry]:
+ if parent_snapshot is None:
Review Comment:
Its what the java implementation does right? https://github.com/apache/iceberg/blob/3a29199e73f2e9ae0f8f92a1a0732a338c66aa0d/core/src/main/java/org/apache/iceberg/MergingSnapshotProducer.java#L685
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] validate added data files for snapshot compatibility [iceberg-python]
Posted by "jayceslesar (via GitHub)" <gi...@apache.org>.
jayceslesar commented on code in PR #2050:
URL: https://github.com/apache/iceberg-python/pull/2050#discussion_r2147011898
##########
pyiceberg/table/update/validate.py:
##########
@@ -150,3 +178,60 @@ def _validate_deleted_data_files(
if any(conflicting_entries):
conflicting_snapshots = {entry.snapshot_id for entry in conflicting_entries}
raise ValidationException(f"Deleted data files were found matching the filter for snapshots {conflicting_snapshots}!")
+
+
+def _added_data_files(
+ table: Table,
+ starting_snapshot: Snapshot,
+ data_filter: Optional[BooleanExpression],
+ partition_set: Optional[dict[int, set[Record]]],
+ parent_snapshot: Optional[Snapshot],
+) -> Iterator[ManifestEntry]:
+ """Return manifest entries for data files added between the starting snapshot and parent snapshot.
+
+ Args:
+ table: Table to get the history from
+ starting_snapshot: Starting snapshot to get the history from
+ data_filter: Optional filter to match data files
+ partition_set: Optional set of partitions to match data files
+ parent_snapshot: Parent snapshot to get the history from
+
+ Returns:
+ Iterator of manifest entries for added data files matching the conditions
+ """
+ if parent_snapshot is None:
+ return
+
+ manifests, snapshot_ids = validation_history(
Review Comment:
```suggestion
manifests, snapshot_ids = _validation_history(
```
This was made private here https://github.com/apache/iceberg-python/pull/2054
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] validate added data files for snapshot compatibility [iceberg-python]
Posted by "kaushiksrini (via GitHub)" <gi...@apache.org>.
kaushiksrini commented on code in PR #2050:
URL: https://github.com/apache/iceberg-python/pull/2050#discussion_r2153431445
##########
pyiceberg/table/update/validate.py:
##########
@@ -150,3 +178,60 @@ def _validate_deleted_data_files(
if any(conflicting_entries):
conflicting_snapshots = {entry.snapshot_id for entry in conflicting_entries}
raise ValidationException(f"Deleted data files were found matching the filter for snapshots {conflicting_snapshots}!")
+
+
+def _added_data_files(
+ table: Table,
+ starting_snapshot: Snapshot,
+ data_filter: Optional[BooleanExpression],
+ partition_set: Optional[dict[int, set[Record]]],
+ parent_snapshot: Optional[Snapshot],
+) -> Iterator[ManifestEntry]:
+ """Return manifest entries for data files added between the starting snapshot and parent snapshot.
+
+ Args:
+ table: Table to get the history from
+ starting_snapshot: Starting snapshot to get the history from
+ data_filter: Optional filter to match data files
+ partition_set: Optional set of partitions to match data files
+ parent_snapshot: Parent snapshot to get the history from
+
+ Returns:
+ Iterator of manifest entries for added data files matching the conditions
+ """
+ if parent_snapshot is None:
+ return
+
+ manifests, snapshot_ids = validation_history(
Review Comment:
good catch! missed it in the merge
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] validate added data files for snapshot compatibility [iceberg-python]
Posted by "kaushiksrini (via GitHub)" <gi...@apache.org>.
kaushiksrini commented on code in PR #2050:
URL: https://github.com/apache/iceberg-python/pull/2050#discussion_r2119396683
##########
pyiceberg/table/update/validate.py:
##########
@@ -150,3 +151,54 @@ def _validate_deleted_data_files(
if any(conflicting_entries):
conflicting_snapshots = {entry.snapshot_id for entry in conflicting_entries}
raise ValidationException(f"Deleted data files were found matching the filter for snapshots {conflicting_snapshots}!")
+
+
+def _added_data_files(
+ table: Table,
+ starting_snapshot: Snapshot,
+ data_filter: Optional[BooleanExpression],
+ partition_set: Optional[dict[int, set[Record]]],
+ parent_snapshot: Optional[Snapshot],
+) -> Iterator[ManifestEntry]:
+ if parent_snapshot is None:
+ return
+
+ manifests, snapshot_ids = validation_history(
+ table,
+ parent_snapshot,
+ starting_snapshot,
+ VALIDATE_ADDED_FILES_OPERATIONS,
+ ManifestContent.DATA,
+ )
+
+ if data_filter is not None:
+ evaluator = _InclusiveMetricsEvaluator(table.schema(), data_filter)
+
+ for manifest in manifests:
+ for entry in manifest.fetch_manifest_entry(table.io):
+ if entry.snapshot_id not in snapshot_ids:
+ continue
+
+ if data_filter and evaluator.eval(entry.data_file) is ROWS_CANNOT_MATCH:
Review Comment:
good point! made some changes @sungwy
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] validate added data files for snapshot compatibility [iceberg-python]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on PR #2050:
URL: https://github.com/apache/iceberg-python/pull/2050#issuecomment-3001667967
This looks great @kaushiksrini, let's move this forward 🚀 Thanks for the review @sungwy and @jayceslesar 🙌
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] validate added data files for snapshot compatibility [iceberg-python]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on PR #2050:
URL: https://github.com/apache/iceberg-python/pull/2050#issuecomment-2932020981
@kaushiksrini could you resolve the conflicts? Thanks!
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] validate added data files for snapshot compatibility [iceberg-python]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko merged PR #2050:
URL: https://github.com/apache/iceberg-python/pull/2050
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] validate added data files for snapshot compatibility [iceberg-python]
Posted by "sungwy (via GitHub)" <gi...@apache.org>.
sungwy commented on code in PR #2050:
URL: https://github.com/apache/iceberg-python/pull/2050#discussion_r2112948288
##########
pyiceberg/table/update/validate.py:
##########
@@ -24,7 +24,8 @@
from pyiceberg.table.snapshots import Operation, Snapshot, ancestors_between
from pyiceberg.typedef import Record
-VALIDATE_DATA_FILES_EXIST_OPERATIONS = {Operation.OVERWRITE, Operation.REPLACE, Operation.DELETE}
+VALIDATE_DATA_FILES_EXIST_OPERATIONS: Set[Operation] = {Operation.OVERWRITE, Operation.REPLACE, Operation.DELETE}
+VALIDATE_ADDED_FILES_OPERATIONS: Set[Operation] = {Operation.APPEND, Operation.OVERWRITE}
Review Comment:
I know Java uses this name, but I'd like to make an argument for us to be more specific about the files we are referring to since we are naming this constant for the first time in PyIceberg:
```suggestion
VALIDATE_ADDED_DATA_FILES_OPERATIONS: Set[Operation] = {Operation.APPEND, Operation.OVERWRITE}
```
##########
pyiceberg/table/update/validate.py:
##########
@@ -150,3 +151,54 @@ def _validate_deleted_data_files(
if any(conflicting_entries):
conflicting_snapshots = {entry.snapshot_id for entry in conflicting_entries}
raise ValidationException(f"Deleted data files were found matching the filter for snapshots {conflicting_snapshots}!")
+
+
+def _added_data_files(
+ table: Table,
+ starting_snapshot: Snapshot,
+ data_filter: Optional[BooleanExpression],
+ partition_set: Optional[dict[int, set[Record]]],
+ parent_snapshot: Optional[Snapshot],
+) -> Iterator[ManifestEntry]:
+ if parent_snapshot is None:
+ return
+
+ manifests, snapshot_ids = validation_history(
+ table,
+ parent_snapshot,
+ starting_snapshot,
+ VALIDATE_ADDED_FILES_OPERATIONS,
+ ManifestContent.DATA,
+ )
+
+ if data_filter is not None:
+ evaluator = _InclusiveMetricsEvaluator(table.schema(), data_filter)
+
+ for manifest in manifests:
+ for entry in manifest.fetch_manifest_entry(table.io):
+ if entry.snapshot_id not in snapshot_ids:
+ continue
+
+ if data_filter and evaluator.eval(entry.data_file) is ROWS_CANNOT_MATCH:
Review Comment:
nit: there's a lot of duplicated code between `_deleted_data_files` and `_added_data_files`. Is there a way we could refactor the code block here to prevent that?
##########
pyiceberg/table/update/validate.py:
##########
@@ -150,3 +151,54 @@ def _validate_deleted_data_files(
if any(conflicting_entries):
conflicting_snapshots = {entry.snapshot_id for entry in conflicting_entries}
raise ValidationException(f"Deleted data files were found matching the filter for snapshots {conflicting_snapshots}!")
+
+
+def _added_data_files(
+ table: Table,
+ starting_snapshot: Snapshot,
+ data_filter: Optional[BooleanExpression],
+ partition_set: Optional[dict[int, set[Record]]],
+ parent_snapshot: Optional[Snapshot],
+) -> Iterator[ManifestEntry]:
+ if parent_snapshot is None:
Review Comment:
Could we introduce a doc string for each function?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org