smaheshwar-pltr commented on code in PR #2118:
URL: https://github.com/apache/iceberg-python/pull/2118#discussion_r2225407017
##########
tests/table/test_snapshots.py:
##########
@@ -139,15 +139,15 @@ def
test_deserialize_snapshot_with_properties(snapshot_with_properties: Snapshot
def test_snapshot_repr(snapshot: Snapshot) -> None:
assert (
repr(snapshot)
- == """Snapshot(snapshot_id=25, parent_snapshot_id=19,
sequence_number=200, timestamp_ms=1602638573590,
manifest_list='s3:/a/b/c.avro', summary=Summary(Operation.APPEND),
schema_id=3)"""
+ == """Snapshot(snapshot_id=25, parent_snapshot_id=19,
sequence_number=200, timestamp_ms=1602638573590,
manifest_list='s3:/a/b/c.avro', summary=Summary(Operation.APPEND), schema_id=3,
key_id=None)"""
Review Comment:
Perhaps we want to omit `key_id` for < V3 tables instead of serialising it.
The spec says:
<img width="772" height="91" alt="image"
src="https://github.com/user-attachments/assets/185c5dee-2a0b-49fa-82d8-7b9d10111dfd";
/>
with blanks for format versions 1 and 2.
https://github.com/apache/iceberg-python/pull/2146#discussion_r2193411814 then
makes me think we shouldn't write this field for those versions. WDYT?
##########
pyiceberg/table/update/__init__.py:
##########
@@ -598,6 +611,30 @@ def _(update: RemoveStatisticsUpdate, base_metadata:
TableMetadata, context: _Ta
return base_metadata.model_copy(update={"statistics": statistics})
+@_apply_table_update.register(AddEncryptedKeyUpdate)
+def _(update: AddEncryptedKeyUpdate, base_metadata: TableMetadata, context:
_TableMetadataUpdateContext) -> TableMetadata:
+ context.add_update(update)
+
+ if base_metadata.format_version <= 2:
+ raise ValueError("Cannot add encryption keys to Iceberg v1 or v2
tables")
+
+ return base_metadata.model_copy(update={"encryption_keys":
base_metadata.encryption_keys + [update.key]})
+
+
+@_apply_table_update.register(RemoveEncryptedKeyUpdate)
+def _(update: RemoveEncryptedKeyUpdate, base_metadata: TableMetadata, context:
_TableMetadataUpdateContext) -> TableMetadata:
+ context.add_update(update)
+
+ if base_metadata.format_version <= 2:
+ raise ValueError("Cannot add encryption keys to Iceberg v1 or v2
tables")
Review Comment:
Tiny: error message here should be about removal?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
