feat: `validate_deleted_data_files` by jayceslesar · Pull Request #1938 · apache/iceberg-python

jayceslesar · 2025-04-19T19:42:19Z

Closes #1928

Rationale for this change

Add validate_deleted_data_files which depends on #1935

Are these changes tested?

Added a test!

References

Java deletedDataFiles impl:
https://github.com/apache/iceberg/blob/3a29199e73f2e9ae0f8f92a1a0732a338c66aa0d/core/src/main/java/org/apache/iceberg/MergingSnapshotProducer.java#L678

Java ManifestGroup.entries impl:
https://github.com/apache/iceberg/blob/3a29199e73f2e9ae0f8f92a1a0732a338c66aa0d/core/src/main/java/org/apache/iceberg/ManifestGroup.java#L242

…_history`

jayceslesar · 2025-04-23T21:42:42Z

+            if entry.snapshot_id not in new_snapshot_ids:
+                continue
+
+            if entry.status != ManifestEntryStatus.DELETED:
+                continue
+
+            if data_filter is not None and not evaluator(entry.data_file):
+                continue
+
+            if partition_set is not None and (entry.data_file.spec_id, entry.data_file.partition) not in partition_set:
+                continue


probably cleaner to put these each on a line and wrap in an any call

sungwy · 2025-05-02T01:19:39Z

+    parent_snapshot: Optional[Snapshot],
+    partition_set: Optional[set[Record]],


This looks out of order:

Suggested change

parent_snapshot: Optional[Snapshot],

partition_set: Optional[set[Record]],

partition_set: Optional[set[Record]],

parent_snapshot: Optional[Snapshot],

It looks like the function call in validate_deleted_data_files is assuming that parent_snapshot is the last argument

conflicting_entries = deleted_data_files(table, starting_snapshot, data_filter, None, parent_snapshot)

I think this needs some more work. Next to @sungwy's comment, we in the code below:

(entry.data_file.spec_id, entry.data_file.partition) not in partition_set

Where it checks if a tuple is in a partition_set, but the partition_set only contains the Record according to the signature.

This triggered me, because if you do:

ALTER TABLE prod.db.taxis REPLACE PARTITION FIELD pickup_timestamp WITH day(pickup_timestamp); -- and then ALTER TABLE prod.db.taxis REPLACE PARTITION FIELD pickup_timestamp WITH day(dropoff_timestamp);

Both of the partitioning strategies will produce a Record[int] because it will contain the number of days since epoch. But the meaning is completely different.

sungwy · 2025-05-02T01:24:15Z

+        starting_snapshot,
+        parent_snapshot,


This looks out of order to, because validation_history has to_snapshot as the second argument, and from_snapshot as the third argument.

Suggested change

starting_snapshot,

parent_snapshot,

starting_snapshot,

parent_snapshot,

Do you think it would be better to update validation_history function to use the following function signature instead? I think it's a lot more expected to have from_snapshot then to_snapshot

def validation_history( table: Table, from_snapshot: Snapshot, to_snapshot: Snapshot, matching_operations: set[Operation], manifest_content_filter: ManifestContent, )

Could you clarify a little more? Are you saying we should move to the terms starting_snapshot (which is from_snapshot) and parent_snapshot (to_snapshot)

sungwy · 2025-05-02T01:42:51Z

+    )
+
+    if data_filter is not None:
+        evaluator = _StrictMetricsEvaluator(table.schema(), data_filter).eval


I'm not too sure if this is correct, because ManifestGroup.entries seems to be using inclusive projection.

Should we be using inclusive_projection here instead?

Summoning @Fokko for a second review

Yes, I agree that this should be inclusive projection, since we want to know if there are any matches. Inclusive projection returns rows_might_match and rows_cannot_match. If they cannot be matched, then we can skip it :)

Fokko

Thanks for following up on the previous PR @jayceslesar. This looks like a great start, but there are some missing elements. It would be good to add some more tests as well, since this is pretty critical code since it might affect correctness.

Fokko · 2025-05-02T08:29:34Z

+    parent_snapshot: Optional[Snapshot],
+    partition_set: Optional[set[Record]],


I think this needs some more work. Next to @sungwy's comment, we in the code below:

(entry.data_file.spec_id, entry.data_file.partition) not in partition_set

Where it checks if a tuple is in a partition_set, but the partition_set only contains the Record according to the signature.

This triggered me, because if you do:

ALTER TABLE prod.db.taxis REPLACE PARTITION FIELD pickup_timestamp WITH day(pickup_timestamp); -- and then ALTER TABLE prod.db.taxis REPLACE PARTITION FIELD pickup_timestamp WITH day(dropoff_timestamp);

Both of the partitioning strategies will produce a Record[int] because it will contain the number of days since epoch. But the meaning is completely different.

Fokko · 2025-05-02T08:58:08Z

+    )
+
+    if data_filter is not None:
+        evaluator = _StrictMetricsEvaluator(table.schema(), data_filter).eval


Yes, I agree that this should be inclusive projection, since we want to know if there are any matches. Inclusive projection returns rows_might_match and rows_cannot_match. If they cannot be matched, then we can skip it :)

Co-authored-by: Fokko Driesprong <fokko@apache.org>

jayceslesar · 2025-05-10T18:00:07Z

@sungwy @Fokko thank you for reviews! Late responses here are my bad -- was focusing on the orphaned files PR

sungwy

Hi @jayceslesar - thanks for adopting all of the review feedback!

I think this is good to merge, but I'd prefer to make validate_deleted_data_files a private function

sungwy · 2025-05-15T22:50:54Z

Waiting for the CI to pass

jayceslesar · 2025-05-17T22:27:59Z

@sungwy @Fokko thank you for the review! Learned a bunch and hopefully I can get the other 3 sub tasks in #819 along quickly!

Fokko · 2025-05-20T19:38:30Z

+        parent_snapshot: Ending snapshot on the branch being validated
+
+    """
+    conflicting_entries = _deleted_data_files(table, starting_snapshot, data_filter, None, parent_snapshot)


nit, I think style wise it is more elegant to switch over to keyword-arguments:

Suggested change

conflicting_entries = _deleted_data_files(table, starting_snapshot, data_filter, None, parent_snapshot)

conflicting_entries = _deleted_data_files(table, starting_snapshot, data_filter, parent_snapshot=parent_snapshot)

Fokko

Looks good, thanks @jayceslesar for working on this, and thanks @sungwy for the review 🙌

Closes apache#1928 # Rationale for this change Add `validate_deleted_data_files` which depends on apache#1935 # Are these changes tested? Added a test! # References Java `deletedDataFiles` impl: https://github.com/apache/iceberg/blob/3a29199e73f2e9ae0f8f92a1a0732a338c66aa0d/core/src/main/java/org/apache/iceberg/MergingSnapshotProducer.java#L678 Java `ManifestGroup.entries` impl: https://github.com/apache/iceberg/blob/3a29199e73f2e9ae0f8f92a1a0732a338c66aa0d/core/src/main/java/org/apache/iceberg/ManifestGroup.java#L242 --------- Co-authored-by: Fokko Driesprong <fokko@apache.org>

jayceslesar added 15 commits April 18, 2025 15:53

feat: validation history

beff92b

format

c369720

almost a working test

41bb8a4

allow content_filter in snapshot.manifests

763e9f4

simplify order of arguments to validation_history

f200beb

simplify return in snapshot.manifests

7f6bf9d

tests passing

c63cc55

correct ancestors_between

f2f3a88

fix to/from logic and allow optional to_snapshot arg in `validation…

74d5569

…_history`

remove a level of nesting with smarter clause

167f9e4

fix bad accessor

efe50b4

fix docstring

0793713

[wip] feat: validate_deleted_data_files

89120aa

first dummy test case

a39abd2

first working tests (needs cleanup)

cf5b061

jayceslesar commented Apr 23, 2025

View reviewed changes

jayceslesar added 3 commits May 1, 2025 09:07

Merge branch 'main' into feat/validate-deleted-data-files

645e8df

bring back all code from silly merge

caf7e36

last tweaks

a52c422

jayceslesar changed the title ~~[wip] feat: validate_deleted_data_files~~ feat: validate_deleted_data_files May 1, 2025

jayceslesar marked this pull request as ready for review May 2, 2025 00:40

sungwy reviewed May 2, 2025

View reviewed changes

Fokko reviewed May 2, 2025

View reviewed changes

jayceslesar and others added 7 commits May 10, 2025 11:16

Merge branch 'main' into feat/validate-deleted-data-files

d3df4a7

fix order of args in deleted_data_files

9eca29f

make deleted_data_files private

fc83d42

Update pyiceberg/table/update/validate.py

ee3959a

Co-authored-by: Fokko Driesprong <fokko@apache.org>

ise inclusive metrics evaluator and add rows cannot match check

a51c2da

show what snapshot IDs conflict

ec86695

maybe correct partition_spec impl?

0a6b781

fix ordering to match

a96da01

sungwy approved these changes May 15, 2025

View reviewed changes

Comment thread pyiceberg/table/update/validate.py Outdated

jayceslesar added 2 commits May 15, 2025 17:38

make private and add test

03b2913

Merge branch 'main' into feat/validate-deleted-data-files

aae22e6

Fokko reviewed May 20, 2025

View reviewed changes

Fokko approved these changes May 20, 2025

View reviewed changes

Fokko merged commit e98f685 into apache:main May 20, 2025
10 checks passed

smaheshwar-pltr mentioned this pull request Jul 22, 2025

Incremental Append Scan #2031

Closed

This was referenced May 2, 2026

Add commit retry with data conflict validation #3319

Open

Support IsolationLevels and Concurrency Safety Validation Checks #819

Open

Add commit retry and concurrency validation for writes #3320

Open

		parent_snapshot: Optional[Snapshot],
		partition_set: Optional[set[Record]],

	conflicting_entries = _deleted_data_files(table, starting_snapshot, data_filter, None, parent_snapshot)
	conflicting_entries = _deleted_data_files(table, starting_snapshot, data_filter, parent_snapshot=parent_snapshot)

Conversation

jayceslesar commented Apr 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rationale for this change

Are these changes tested?

References

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Fokko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jayceslesar commented May 10, 2025

Uh oh!

sungwy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sungwy commented May 15, 2025

Uh oh!

jayceslesar commented May 17, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Fokko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jayceslesar commented Apr 19, 2025 •

edited

Loading