feat: `validation_history` and `ancestors_between` #1935

jayceslesar · 2025-04-18T20:33:41Z

Rationale for this change

Adds validation_history that will be used in support of #819

Also adds ancestors_between.

Based off of the java implementation but we will likely want changes that feel more like python :). Already took a few liberties there

Are these changes tested?

Added tests

References:

java implementation of validation_history:
https://github.com/apache/iceberg/blob/main/core/src/main/java/org/apache/iceberg/MergingSnapshotProducer.java#L859

java implementation of ancestors_between:
https://github.com/apache/iceberg/blob/main/core/src/main/java/org/apache/iceberg/util/SnapshotUtil.java#L213

pyiceberg/table/snapshots.py

sungwy

Hi @jayceslesar thanks for working on this PR! I think having these helper functions will be wonderful in setting up the rest of the subtasks for success. I've left a few comments around the nullability of the snapshots, and the required behavior of ancestors_between function

sungwy · 2025-04-19T02:05:34Z

pyiceberg/table/snapshots.py

+
+
+def ancestors_between(
+    current_snapshot: Optional[Snapshot], oldest_snapshot: Snapshot, table_metadata: TableMetadata


This doesn't looks correct: the current_snapshot is a required field oldest_snapshot could be None and in that case, we also should provide the entire list of ancestors of the current_snapshot

I think the implementation that's in progress on this PR is closer to what we'd want: https://github.com/apache/iceberg-python/pull/533/files

this should now be resolved, thank you for the link to that

pyiceberg/table/update/validate.py

…_history`

Fokko

Thanks for working on this @jayceslesar

pyiceberg/table/update/validate.py

Fokko · 2025-04-22T11:29:25Z

pyiceberg/table/update/validate.py

+def validation_history(
+    table: Table,
+    from_snapshot: Snapshot,
+    to_snapshot: Optional[Snapshot],


What do you think of making the API a bit more strict and simplify code along the way. I don't think this needs to be Optional:

Suggested change

to_snapshot: Optional[Snapshot],

to_snapshot: Snapshot,

@Fokko I think the existing behavior of the Spark/Java API is for to check all snapshots if the from_snapshot is Optional.

https://github.com/apache/iceberg/blob/main/spark/v3.5/spark/src/main/java/org/apache/iceberg/spark/SparkWriteConf.java#L443-L448

Should we mirror that behavior in PyIceberg, or just require that the validate_from_snapshot_id be specified if isolation_level is not None?

My initial thought was that we could always widen the API and make it optional.

Looking at it more closely, only the SparkWriteConf is optional, we only use the value if is != null:

https://github.com/apache/iceberg/blob/d7f8e5400519f84cd2af82d7769713b24e916809/spark/v3.5/spark/src/main/java/org/apache/iceberg/spark/source/SparkWrite.java#L317-L321

https://github.com/apache/iceberg/blob/d7f8e5400519f84cd2af82d7769713b24e916809/spark/v3.5/spark/src/main/java/org/apache/iceberg/spark/source/SparkWrite.java#L361-L366

Fokko · 2025-04-22T11:31:00Z

pyiceberg/table/update/validate.py

+    for snapshot in ancestors_between(from_snapshot, to_snapshot, table.metadata):
+        last_snapshot = snapshot
+        summary = snapshot.summary
+        if summary is None or summary.operation not in matching_operations:


This looks dangerous to me, I don't think we want to skip over this if the summary is None

I think the right thing here is to assume that it is an Overwrite operation

What is a case where summary would ever be None? I can't seem to find any checks against that in the codebase except for https://github.com/apache/iceberg-python/blob/main/pyiceberg/table/snapshots.py#L367 which seems to be checking against an empty dict (well mapping) and should also be checking the or is None case imo

It now defaults to Operation.OVERWRITE if summary is determined to be None, but still wondering about above

summary can be None if the Table format version is v1 according to the Iceberg Spec: https://iceberg.apache.org/spec/#snapshots

Instead of setting it as Operation.OVERWRITE I think an alternative approach is to just throw an exception if summary field is None with a helpful error message that explains that validation_history cannot be generated in the absence of summary fields. Wdyt @Fokko @jayceslesar ?

@sungwy That would also work for me. We can always see if there are ways to make it work for V1 tables later, however, I'm not sure how big the problem is because most implementations also produce an operation for V1 tables. So, I think we're good with the approach that @sungwy suggests 👍

Sounds good to me, implemented and added to the existing test to assure that we raise an error when that happens. Planning on cleaning up tests in the next MR when I will likely need to make some fixtures & parameterize

Fokko · 2025-04-22T11:33:52Z

pyiceberg/table/update/validate.py

+        List of manifest files and set of snapshots matching conditions
+    """
+    manifests_files: list[ManifestFile] = []
+    snapshots: set[Snapshot] = set()


Java just returns ints, we can return Snapshot's as well, but with the set we lose the order.

great, I am happy to remove this being a set, I was just attempting to copy what made sense from java

This now returns a unique list, happy to modify that to allow duplicates if wanted

Yes, or return Set[int], similar to Java :)

__hash__ dunder on Snapshot ?

nah, will just return the Set[int]

__hash__ dunder on Snapshot ?

I like that as well, but let's do that in a separate PR 👍

Now we're back at a set, we can remove the in check below:

if snapshot not in snapshots: snapshots.add(snapshot.snapshot_id)

…uired in `validation_history`

…s determined to be `None`

… set

pyiceberg/table/update/validate.py

Fokko · 2025-04-24T19:22:41Z

pyiceberg/table/update/validate.py

+    if last_snapshot is None or last_snapshot.snapshot_id == from_snapshot.snapshot_id:
+        raise ValidationException("No matching snapshot found.")


We can leave it like this for now, but it seems odd to throw here, since there is no history to validate, which in turn is valid.

Possibly me just not translating this java correctly at all https://github.com/apache/iceberg/blob/main/core/src/main/java/org/apache/iceberg/MergingSnapshotProducer.java#L894

pyiceberg/table/update/validate.py

Co-authored-by: Fokko Driesprong <[email protected]>

sungwy · 2025-04-27T01:43:52Z

pyiceberg/table/update/validate.py

+            ]
+        )
+
+    if last_snapshot is None or last_snapshot.snapshot_id == from_snapshot.snapshot_id:


Suggested change

if last_snapshot is None or last_snapshot.snapshot_id == from_snapshot.snapshot_id:

if last_snapshot is not None and last_snapshot.snapshot_id != from_snapshot.snapshot_id:

`if not (last_snapshot is not None and last_snapshot.snapshot_id != from_snapshot.snapshot_id):`

The ValidationCheck in java throws an Exception with the provided error message if the boolean condition in doesn't hold. So we'd actually be throwing in the inverse of this condition.

https://github.com/apache/iceberg/blob/22d194f5d685fdf5bec17c6bcc92a69db4ae4957/api/src/main/java/org/apache/iceberg/exceptions/ValidationException.java#L47-L50

Made this change

Awesome, changing this to be correct also caught an issue with ancestors_between that is not fixed!

sungwy · 2025-04-27T01:45:03Z

tests/table/test_validate.py

+    with patch("pyiceberg.table.update.validate.ancestors_between", return_value=[snapshot_with_no_summary]):
+        with pytest.raises(ValidationException):
+            validation_history(
+                table_v2_with_extensive_snapshots,
+                newest_snapshot,
+                oldest_snapshot,
+                {Operation.APPEND},
+                ManifestContent.DATA,
+            )


I think it would be best to create a separate test function that tests this failure case, like test_validation_history_fails_on_snapshot_with_no_summary

sungwy · 2025-04-27T01:46:01Z

tests/table/test_validate.py

+from pyiceberg.table.update.validate import validation_history
+
+
+def test_validation_history(table_v2_with_extensive_snapshots: Table) -> None:


Could we add another test which tests the expected failure when from_snapshot value doesn't match a snapshot in the history of the table?

sungwy · 2025-04-27T01:47:15Z

Hi @jayceslesar ! Thank you for pressing on with this PR! This looks almost good to merge. I share the same concerns with @Fokko regarding the validation check - I fear that it's currently not checking the right condition. Let's get that fix in, and merge this PR in soon! 🥳

pyiceberg/table/update/validate.py

jayceslesar added 3 commits April 18, 2025 15:53

feat: validation history

beff92b

format

c369720

almost a working test

41bb8a4

jayceslesar commented Apr 18, 2025

View reviewed changes

pyiceberg/table/snapshots.py Outdated Show resolved Hide resolved

jayceslesar added 2 commits April 18, 2025 16:55

allow content_filter in snapshot.manifests

763e9f4

simplify order of arguments to validation_history

f200beb

jayceslesar changed the title ~~feat: validation history~~ [wip] validation history Apr 18, 2025

jayceslesar changed the title ~~[wip] validation history~~ [wip] validation_history and ancestors_between Apr 18, 2025

jayceslesar added 2 commits April 18, 2025 17:07

simplify return in snapshot.manifests

7f6bf9d

tests passing

c63cc55

jayceslesar changed the title ~~[wip] validation_history and ancestors_between~~ validation_history and ancestors_between Apr 19, 2025

jayceslesar marked this pull request as ready for review April 19, 2025 01:18

jayceslesar mentioned this pull request Apr 19, 2025

Support IsolationLevels and Concurrency Safety Validation Checks #819

Open

sungwy reviewed Apr 19, 2025

View reviewed changes

jayceslesar added 5 commits April 19, 2025 10:25

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

jayceslesar mentioned this pull request Apr 19, 2025

[wip] feat: validate_deleted_data_files #1938

Draft

jayceslesar changed the title ~~validation_history and ancestors_between~~ feat: validation_history and ancestors_between Apr 19, 2025

Merge branch 'main' into feat/validation-history

b10a0bf

jayceslesar requested a review from sungwy April 21, 2025 13:00

Fokko reviewed Apr 22, 2025

View reviewed changes

jayceslesar added 3 commits April 22, 2025 17:31

move ValidationException to exceptions.py, make to_snapshot req…

d57133e

…uired in `validation_history`

default to Operation.OVERWRITE in validation_history if summary i…

6ef1aa2

…s determined to be `None`

preserve order of snapshots by changing response to a list instead of…

fce608f

… set

jayceslesar requested a review from Fokko April 22, 2025 21:41

jayceslesar added 2 commits April 23, 2025 17:26

validation_history and from_ancestor argument alignment

a6624d9

raise error on summary operation being none in validation_history

0763056

jayceslesar added 2 commits April 23, 2025 17:41

Merge branch 'main' into feat/validation-history

cd0146f

Merge branch 'main' into feat/validation-history

d7c7088

Fokko reviewed Apr 24, 2025

View reviewed changes

pyiceberg/table/update/validate.py Outdated Show resolved Hide resolved

Fokko approved these changes Apr 24, 2025

View reviewed changes

Fokko reviewed Apr 24, 2025

View reviewed changes

pyiceberg/table/update/validate.py Show resolved Hide resolved

jayceslesar and others added 3 commits April 24, 2025 16:18

Update pyiceberg/table/update/validate.py

fe8d103

Co-authored-by: Fokko Driesprong <[email protected]>

Update pyiceberg/table/update/validate.py

f8c5fee

Co-authored-by: Fokko Driesprong <[email protected]>

formatting

d95e6ed

sungwy reviewed Apr 27, 2025

View reviewed changes

pyiceberg/table/update/validate.py Outdated Show resolved Hide resolved

jayceslesar added 5 commits April 27, 2025 11:37

remove emoji (lol)

87ee601

vix validation exception check

c16a3e6

fix bug in ancestors_between

e272b26

break into separate test

727845b

Merge branch 'main' into feat/validation-history

e2bba3f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: `validation_history` and `ancestors_between` #1935

feat: `validation_history` and `ancestors_between` #1935

jayceslesar commented Apr 18, 2025 •

edited

Loading

sungwy left a comment

sungwy Apr 19, 2025

jayceslesar Apr 19, 2025

Fokko left a comment

Fokko Apr 22, 2025

jayceslesar Apr 22, 2025

sungwy Apr 23, 2025

Fokko Apr 24, 2025

Fokko Apr 22, 2025

Fokko Apr 22, 2025

jayceslesar Apr 22, 2025

jayceslesar Apr 22, 2025

sungwy Apr 23, 2025

Fokko Apr 23, 2025

jayceslesar Apr 23, 2025

Fokko Apr 22, 2025

jayceslesar Apr 22, 2025

jayceslesar Apr 22, 2025

Fokko Apr 24, 2025

jayceslesar Apr 24, 2025

jayceslesar Apr 24, 2025

Fokko Apr 24, 2025

Fokko Apr 24, 2025

jayceslesar Apr 24, 2025

sungwy Apr 27, 2025

jayceslesar Apr 27, 2025

jayceslesar Apr 27, 2025

sungwy Apr 27, 2025

jayceslesar Apr 27, 2025

sungwy Apr 27, 2025

sungwy commented Apr 27, 2025 •

edited

Loading



		def ancestors_between(
		current_snapshot: Optional[Snapshot], oldest_snapshot: Snapshot, table_metadata: TableMetadata

		if last_snapshot is None or last_snapshot.snapshot_id == from_snapshot.snapshot_id:
		raise ValidationException("No matching snapshot found.")

	if last_snapshot is None or last_snapshot.snapshot_id == from_snapshot.snapshot_id:
	if last_snapshot is not None and last_snapshot.snapshot_id != from_snapshot.snapshot_id:

		from pyiceberg.table.update.validate import validation_history


		def test_validation_history(table_v2_with_extensive_snapshots: Table) -> None:

feat: validation_history and ancestors_between #1935

Are you sure you want to change the base?

feat: validation_history and ancestors_between #1935

Conversation

jayceslesar commented Apr 18, 2025 • edited Loading

Rationale for this change

Are these changes tested?

References:

sungwy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fokko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sungwy commented Apr 27, 2025 • edited Loading

feat: `validation_history` and `ancestors_between` #1935

feat: `validation_history` and `ancestors_between` #1935

jayceslesar commented Apr 18, 2025 •

edited

Loading

sungwy commented Apr 27, 2025 •

edited

Loading