Skip to content

Commit 34c8949

Browse files
authored
Reorder validation_history parameters to be more idiomatic (#1959)
<!-- 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} --> # Rationale for this change Nit: the current order (`to_snapshot` before `from_snapshot`) is unexpected and [error-prone](https://github.com/apache/iceberg-python/pull/1938/files#r2070985497). This PR proposes to reorder these parameters in `validation_history` and `ancestors_between` so that it is more idiomatic. # Are these changes tested? Yes, through integration and unit tests # Are there any user-facing changes? Yes, but these functions were just introduced (no releases were made yet with these functions) <!-- In the case of user-facing changes, please add the changelog label. -->
1 parent e6f6018 commit 34c8949

File tree

4 files changed

+8
-8
lines changed

4 files changed

+8
-8
lines changed

pyiceberg/table/snapshots.py

+1-1
Original file line numberDiff line numberDiff line change
@@ -438,7 +438,7 @@ def ancestors_of(current_snapshot: Optional[Snapshot], table_metadata: TableMeta
438438

439439

440440
def ancestors_between(
441-
to_snapshot: Snapshot, from_snapshot: Optional[Snapshot], table_metadata: TableMetadata
441+
from_snapshot: Optional[Snapshot], to_snapshot: Snapshot, table_metadata: TableMetadata
442442
) -> Iterable[Snapshot]:
443443
"""Get the ancestors of and including the given snapshot between the to and from snapshots."""
444444
if from_snapshot is not None:

pyiceberg/table/update/validate.py

+3-3
Original file line numberDiff line numberDiff line change
@@ -23,17 +23,17 @@
2323

2424
def validation_history(
2525
table: Table,
26-
to_snapshot: Snapshot,
2726
from_snapshot: Snapshot,
27+
to_snapshot: Snapshot,
2828
matching_operations: set[Operation],
2929
manifest_content_filter: ManifestContent,
3030
) -> tuple[list[ManifestFile], set[int]]:
3131
"""Return newly added manifests and snapshot IDs between the starting snapshot and parent snapshot.
3232
3333
Args:
3434
table: Table to get the history from
35-
to_snapshot: Starting snapshot
3635
from_snapshot: Parent snapshot to get the history from
36+
to_snapshot: Starting snapshot
3737
matching_operations: Operations to match on
3838
manifest_content_filter: Manifest content type to filter
3939
@@ -47,7 +47,7 @@ def validation_history(
4747
snapshots: set[int] = set()
4848

4949
last_snapshot = None
50-
for snapshot in ancestors_between(to_snapshot, from_snapshot, table.metadata):
50+
for snapshot in ancestors_between(from_snapshot, to_snapshot, table.metadata):
5151
last_snapshot = snapshot
5252
summary = snapshot.summary
5353
if summary is None:

tests/table/test_snapshots.py

+1-1
Original file line numberDiff line numberDiff line change
@@ -426,8 +426,8 @@ def test_ancestors_between(table_v2_with_extensive_snapshots: Table) -> None:
426426
len(
427427
list(
428428
ancestors_between(
429-
current_snapshot,
430429
oldest_snapshot,
430+
current_snapshot,
431431
table_v2_with_extensive_snapshots.metadata,
432432
)
433433
)

tests/table/test_validate.py

+3-3
Original file line numberDiff line numberDiff line change
@@ -71,8 +71,8 @@ def mock_read_manifest_side_effect(self: Snapshot, io: FileIO) -> list[ManifestF
7171
with patch("pyiceberg.table.snapshots.Snapshot.manifests", new=mock_read_manifest_side_effect):
7272
manifests, snapshots = validation_history(
7373
table,
74-
newest_snapshot,
7574
oldest_snapshot,
75+
newest_snapshot,
7676
{Operation.APPEND},
7777
ManifestContent.DATA,
7878
)
@@ -101,8 +101,8 @@ def test_validation_history_fails_on_snapshot_with_no_summary(
101101
with pytest.raises(ValidationException):
102102
validation_history(
103103
table,
104-
newest_snapshot,
105104
oldest_snapshot,
105+
newest_snapshot,
106106
{Operation.APPEND},
107107
ManifestContent.DATA,
108108
)
@@ -131,8 +131,8 @@ def mock_read_manifest_side_effect(self: Snapshot, io: FileIO) -> list[ManifestF
131131
with pytest.raises(ValidationException):
132132
validation_history(
133133
table,
134-
newest_snapshot,
135134
oldest_snapshot,
135+
newest_snapshot,
136136
{Operation.APPEND},
137137
ManifestContent.DATA,
138138
)

0 commit comments

Comments
 (0)