Implement update for `remove-snapshots` action by grihabor · Pull Request #1561 · apache/iceberg-python

grihabor · 2025-01-22T16:23:29Z

No description provided.

kevinjqliu

Thanks! This was one of the missing update functions mentioned in #952

Do you mind also including some tests? Similar to

Lines 663 to 682 in 2cd4e78

deftest_apply_remove_properties_update(table_v2: Table) ->None: 
base_metadata=update_table_metadata( 
table_v2.metadata, 
 (SetPropertiesUpdate(updates={"test_a": "test_a", "test_b": "test_b", "test_c": "test_c", "test_d": "test_d"}),), 
 ) 
new_metadata_no_removal=update_table_metadata(base_metadata, (RemovePropertiesUpdate(removals=[]),)) 
assertbase_metadata==new_metadata_no_removal
new_metadata=update_table_metadata(base_metadata, (RemovePropertiesUpdate(removals=["test_a", "test_c"]),)) 
assertbase_metadata.properties== { 
"read.split.target.size": "134217728", 
"test_a": "test_a", 
"test_b": "test_b", 
"test_c": "test_c", 
"test_d": "test_d", 
 } 
assertnew_metadata.properties== {"read.split.target.size": "134217728", "test_b": "test_b", "test_d": "test_d"} 

grihabor · 2025-01-22T16:31:25Z

Sure! Thanks for the fast review

kevinjqliu

left some comments, looks like theres also a linter error, do you mind running make lint locally?

kevinjqliu · 2025-01-22T21:17:35Z



+@_apply_table_update.register(RemoveSnapshotsUpdate)
+def _(update: RemoveSnapshotsUpdate, base_metadata: TableMetadata, context: _TableMetadataUpdateContext) -> TableMetadata:


this is the java reference
https://github.com/apache/iceberg/blob/be6e9daf8901cdee63197e77fcf95624bb694f39/core/src/main/java/org/apache/iceberg/TableMetadata.java#L1396-L1426

kevinjqliu · 2025-01-22T21:18:09Z

+def _(update: RemoveSnapshotsUpdate, base_metadata: TableMetadata, context: _TableMetadataUpdateContext) -> TableMetadata:
+ for remove_snapshot_id in update.snapshot_ids:
+ if remove_snapshot_id == base_metadata.current_snapshot_id:
+ raise ValueError(f"Can't remove current snapshot id {remove_snapshot_id}")


should we block the current snapshot?

I'm not an expert in iceberg spec, but it's not clear what should happen if you try to remove the current snapshot.
I'm also not sure if I should update parent_snapshot_id in every snapshot that was referencing removed snapshots

Decided to set parent_snapshot_id to None if the parent is gone

it's not clear what should happen if you try to remove the current snapshot.
im looking at the java implementation for answers, i think you can just remove the current snapshot... because you can have an empty table with no snapshots

Created a separate pr for remove-snapshot-ref and added a unit test there #1598

grihabor · 2025-01-23T21:50:03Z

Hey @kevinjqliu, ready for another review round. I had to cherry pick the changes from #822 to reuse the code that removes refs

grihabor · 2025-01-27T13:19:05Z

Fixed the linters

grihabor · 2025-01-30T12:42:14Z

Removed corresponding statistics files and entries from snapshot log

kevinjqliu

I added a few comments to the PR. I think we might need some integrations tests to make sure the behavior aligns with the java implementation.

Also, looks like remove_tag and remove_branch are unrelated to this PR, perhaps we can move them to a separate PR.

kevinjqliu · 2025-01-30T01:45:40Z

+def _(update: RemoveSnapshotsUpdate, base_metadata: TableMetadata, context: _TableMetadataUpdateContext) -> TableMetadata:
+ for remove_snapshot_id in update.snapshot_ids:
+ if remove_snapshot_id == base_metadata.current_snapshot_id:
+ raise ValueError(f"Can't remove current snapshot id {remove_snapshot_id}")


it's not clear what should happen if you try to remove the current snapshot.
im looking at the java implementation for answers, i think you can just remove the current snapshot... because you can have an empty table with no snapshots

kevinjqliu · 2025-02-01T00:39:52Z

+ assert len(table_v2.metadata.snapshots) == 2
+ assert len(table_v2.metadata.snapshot_log) == 2
+ assert len(table_v2.metadata.refs) == 2
+ update = RemoveSnapshotsUpdate(snapshot_ids=[3051729675574597004])


nit can you make 3051729675574597004 a constant for readability?

Added constants REMOVE_SNAPSHOT and KEEP_SNAPSHOT

kevinjqliu · 2025-02-01T00:41:35Z

+ with pytest.raises(ValueError, match="Can't remove current snapshot id 3055729675574597004"):
+ update_table_metadata(table_v2.metadata, (update,))
+
+


lets also add some tests for RemoveSnapshotRefUpdate

Created a separate pr for remove-snapshot-ref and added a unit test there #1598

grihabor

Thank you for review! Could you explain which kind of integration tests you want? Like pyspark integration with expire_snapshots call?

grihabor · 2025-02-01T09:29:37Z

+def _(update: RemoveSnapshotsUpdate, base_metadata: TableMetadata, context: _TableMetadataUpdateContext) -> TableMetadata:
+ for remove_snapshot_id in update.snapshot_ids:
+ if remove_snapshot_id == base_metadata.current_snapshot_id:
+ raise ValueError(f"Can't remove current snapshot id {remove_snapshot_id}")


Created a separate pr for remove-snapshot-ref and added a unit test there #1598

grihabor · 2025-02-01T09:30:06Z

+ with pytest.raises(ValueError, match="Can't remove current snapshot id 3055729675574597004"):
+ update_table_metadata(table_v2.metadata, (update,))
+
+


Created a separate pr for remove-snapshot-ref and added a unit test there #1598

grihabor · 2025-02-01T09:34:39Z

+ assert len(table_v2.metadata.snapshots) == 2
+ assert len(table_v2.metadata.snapshot_log) == 2
+ assert len(table_v2.metadata.refs) == 2
+ update = RemoveSnapshotsUpdate(snapshot_ids=[3051729675574597004])


Added constants REMOVE_SNAPSHOT and KEEP_SNAPSHOT

Fokko

Left some small comments, but this looks great @grihabor

Fokko · 2025-02-03T11:14:50Z

+
+ snapshots = [
+ (s.model_copy(update={"parent_snapshot_id": None}) if s.parent_snapshot_id in update.snapshot_ids else s)
+ for s in base_metadata.snapshots


Can we make this a little more verbose:
Suggested change
forsinbase_metadata.snapshots
forsnapshotinbase_metadata.snapshots

Sure fb8f350

Part of #1561Closes#822 --------- Co-authored-by: Fokko Driesprong <fokko@apache.org>

kevinjqliu · 2025-02-16T20:41:37Z

now that #1598 is merged, @grihabor could you rebase main?

…date

grihabor · 2025-02-17T11:56:36Z

@kevinjqliu Sure, merged wth upstream main

kevinjqliu

LGTM!

Implement update for remove-snapshot action
b6ba319

grihabor changed the title ~~Implement update for remove-snapshot action~~Implement update for remove-snapshots actionJan 22, 2025

kevinjqliu reviewed Jan 22, 2025
View reviewed changes

grihabor added 2 commits January 22, 2025 20:19

Add tests
88064c0

Make sure parent_snapshot_id is consistent
818f771

kevinjqliu reviewed Jan 22, 2025
View reviewed changes

grihabor added 3 commits January 23, 2025 15:38

Set parent_snapshot_id to None
9b31690

cherry-pick: Support Remove Branch or Tag APIs (apache#822)
c5cbfe4

Make sure refs are removed when removing snapshots
29feaf7

Fix ruff and pydocstyle
9e3f7c7

grihabor added 3 commits January 30, 2025 12:52

Remove statistics
b9c912d

Fix name collision
d515e74

Remove entries from snapshot log
c7481fe

kevinjqliu reviewed Feb 1, 2025
View reviewed changes

grihabor mentioned this pull request Feb 1, 2025
Implement update for remove-snapshot-ref action #1598
Merged

grihabor added 2 commits February 1, 2025 10:34

Introduce constants for test readability
da4db9f

Allow to remove main branch
19e17f0

grihabor force-pushed the remove-snapshots-update branch from 28c6657 to 19e17f0Compare February 1, 2025 09:45

Revert changes (moved toapache#1598)
32e1e85

grihabor force-pushed the remove-snapshots-update branch from c8c63ea to 32e1e85Compare February 1, 2025 09:53

grihabor commented Feb 1, 2025
View reviewed changes

Fokko reviewed Feb 3, 2025
View reviewed changes

Fokko added a commit that referenced this pull request Feb 14, 2025
Implement update forremove-snapshot-refaction (#1598)
dd58ac1
Part of #1561Closes#822 --------- Co-authored-by: Fokko Driesprong <fokko@apache.org>

grihabor added 2 commits February 17, 2025 12:53

Merge remote-tracking branch 'upstream/main' into remove-snapshots-up…
e68dbb9
…date

Use verbose variable names
fb8f350

Fokko approved these changes Feb 17, 2025
View reviewed changes

kevinjqliu approved these changes Feb 17, 2025
View reviewed changes

kevinjqliu merged commit 19148d3 into apache:mainFeb 17, 2025

grihabor mentioned this pull request Feb 19, 2025
Add test to ensure every table update has corresponding _apply_table_update function #952
Closed

	deftest_apply_remove_properties_update(table_v2: Table) ->None:
	base_metadata=update_table_metadata(
	table_v2.metadata,
	(SetPropertiesUpdate(updates={"test_a": "test_a", "test_b": "test_b", "test_c": "test_c", "test_d": "test_d"}),),
	)

	new_metadata_no_removal=update_table_metadata(base_metadata, (RemovePropertiesUpdate(removals=[]),))

	assertbase_metadata==new_metadata_no_removal

	new_metadata=update_table_metadata(base_metadata, (RemovePropertiesUpdate(removals=["test_a", "test_c"]),))

	assertbase_metadata.properties== {
	"read.split.target.size": "134217728",
	"test_a": "test_a",
	"test_b": "test_b",
	"test_c": "test_c",
	"test_d": "test_d",
	}
	assertnew_metadata.properties== {"read.split.target.size": "134217728", "test_b": "test_b", "test_d": "test_d"}



		@_apply_table_update.register(RemoveSnapshotsUpdate)
		def _(update: RemoveSnapshotsUpdate, base_metadata: TableMetadata, context: _TableMetadataUpdateContext) -> TableMetadata:

		with pytest.raises(ValueError, match="Can't remove current snapshot id 3055729675574597004"):
		update_table_metadata(table_v2.metadata, (update,))

	forsinbase_metadata.snapshots
	forsnapshotinbase_metadata.snapshots

Uh oh!

Conversation

grihabor commented Jan 22, 2025

Uh oh!

kevinjqliu left a comment

Choose a reason for hiding this comment

Uh oh!

grihabor commented Jan 22, 2025

Uh oh!

kevinjqliu left a comment

Choose a reason for hiding this comment

Uh oh!

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!

grihabor commented Jan 23, 2025

Uh oh!

grihabor commented Jan 27, 2025

Uh oh!

grihabor commented Jan 30, 2025

Uh oh!

kevinjqliu left a comment

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!

Uh oh!

grihabor left a comment

Choose a reason for hiding this comment

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kevinjqliu commented Feb 16, 2025

Uh oh!

grihabor commented Feb 17, 2025

Uh oh!

kevinjqliu left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants