mbutrovich commented on code in PR #4159:
URL: https://github.com/apache/iceberg-python/pull/4159#discussion_r4448329352
##########
tests/table/test_init.py:
##########
@@ -2047,6 +2047,39 @@ def test_serialize_encryption_key_updates() -> None:
assert TypeAdapter(TableUpdate).validate_json(remove.model_dump_json()) ==
remove
+def test_encryption_manager_of_unencrypted_table(table_v3: Table) -> None:
+ assert table_v3.encryption_manager is None
+
+
+def
test_encryption_manager_requires_kms_client(example_table_metadata_v3_encrypted:
dict[str, Any]) -> None:
+ table_metadata =
TableMetadataUtil.parse_obj(example_table_metadata_v3_encrypted)
+ table = Table(("database", "table"), table_metadata,
"s3://bucket/metadata.json", load_file_io(), NoopCatalog("noop"))
+
+ with pytest.raises(ValueError, match="no key management client was
provided"):
+ table.scan()
+
+
+def test_encryption_manager_follows_current_metadata(table_v3_encrypted:
Table) -> None:
+ assert table_v3_encrypted.encryption_manager is not None
+
+ # Catalogs must not let encryption.key-id be removed, but removing it
shows the manager follows the current metadata
+ table_v3_encrypted.metadata =
table_v3_encrypted.metadata.model_copy(update={"properties": {}})
+
+ assert table_v3_encrypted.encryption_manager is None
+
+
+def test_scan_encrypted_table(table_v3_encrypted: Table) -> None:
+ """The manifest list was encrypted by iceberg-rust, and lists no
manifests."""
+ assert list(table_v3_encrypted.scan().plan_files()) == []
+ assert list(table_v3_encrypted.incremental_append_scan().plan_files()) ==
[]
+ assert table_v3_encrypted.scan().to_arrow().num_rows == 0
+
+
+def test_inspect_encrypted_table(table_v3_encrypted: Table) -> None:
+ assert table_v3_encrypted.inspect.manifests().num_rows == 0
+ assert table_v3_encrypted.inspect.all_manifests().num_rows == 0
Review Comment:
Could this also cover `entries()` and `partitions()`? Both read the manifest
list through the paths this PR changes, and `partitions()` would have caught
the missing manager above.
```suggestion
def test_inspect_encrypted_table(table_v3_encrypted: Table) -> None:
assert table_v3_encrypted.inspect.manifests().num_rows == 0
assert table_v3_encrypted.inspect.all_manifests().num_rows == 0
assert table_v3_encrypted.inspect.entries().num_rows == 0
assert table_v3_encrypted.inspect.partitions().num_rows == 0
```
##########
pyiceberg/table/inspect.py:
##########
@@ -383,7 +383,7 @@ def partitions(
)
snapshot = self._get_snapshot(snapshot_id)
- spec_ids = {manifest.partition_spec_id for manifest in
snapshot.manifests(self.tbl.io)}
+ spec_ids = {manifest.partition_spec_id for manifest in
snapshot.manifests(self.tbl.io, self.tbl.encryption_manager)}
Review Comment:
Should the `DataScan` a few lines below
([L401-L407](https://github.com/apache/iceberg-python/blob/0ad7405099ec4b5a33d39d0e05c40a8fe46a4644/pyiceberg/table/inspect.py#L401-L407))
also get `encryption_manager=self.tbl.encryption_manager`? This line decrypts
the manifest list, but that scan is built without the manager, so
`_plan_manifest_entries` calls `snapshot.manifests(io, None)`. On the head
commit, `table_v3_encrypted.inspect.partitions()` raises `ValueError: Snapshot
1 is encrypted, but no encryption manager was provided`. With the argument
added, it returns an empty table.
As I read it, this is the main cost of passing the manager separately, which
bears on the design question in the description. Every caller of
`Snapshot.manifests` and every scan constructor has to remember it, and a
missed one fails only on encrypted tables. In iceberg-rust,
`ManifestListReader::new` is `pub(crate)`, and callers get a reader from
[`Table::manifest_list_reader`](https://github.com/apache/iceberg-rust/blob/896f9d6c50ff52fa9e43aec170d3f14bfb40b512/crates/iceberg/src/table.rs#L309-L317),
which supplies the manager, so they can't leave it out.
##########
pyiceberg/table/__init__.py:
##########
@@ -1340,13 +1342,26 @@ def __init__(
io: FileIO,
catalog: Catalog,
config: dict[str, str] = EMPTY_DICT,
+ kms_client: KeyManagementClient | None = None,
) -> None:
self._identifier = identifier
self.metadata = metadata
self.metadata_location = metadata_location
self.io = io
self.catalog = catalog
self.config = config
+ self._kms_client = kms_client
+
+ @property
+ def encryption_manager(self) -> EncryptionManager | None:
+ """The table's encryption manager, or None if the table is not
encrypted.
+
+ Raises:
+ ValueError: If the table is encrypted but no key management client
was provided.
+ """
+ from pyiceberg.encryption.manager import EncryptionManager
+
+ return EncryptionManager.from_table_metadata(self._kms_client,
self.metadata)
Review Comment:
Could we build the manager once per metadata version instead of on every
access? `EncryptionManager` caches unwrapped KEKs for an hour
([`_unwrap_kek`](https://github.com/apache/iceberg-python/blob/e3564a54c10960307147377c578680276865ed18/pyiceberg/encryption/manager.py#L161-L170)),
but each access here creates a new manager with an empty cache, so every
manifest list read calls `KeyManagementClient.unwrap_key` again. I wrapped the
test KMS in `table_v3_encrypted` with a call counter. Three
`scan().plan_files()` calls made three `unwrap_key` calls.
`inspect.all_manifests()` over five snapshots made five, because
`_generate_manifests_table` reads the property once per snapshot.
`Table.metadata` is reassigned on commit and refresh, so caching the manager
next to the metadata it came from keeps
`test_encryption_manager_follows_current_metadata` passing:
```python
if self._encryption_manager is None or self._encryption_manager[0] is not
self.metadata:
self._encryption_manager = (self.metadata,
EncryptionManager.from_table_metadata(self._kms_client, self.metadata))
return self._encryption_manager[1]
```
Here `self._encryption_manager` starts as `None` in `__init__`. With that
change, three scans followed by `manifests()`, `entries()`, and
`all_manifests()` made one `unwrap_key` call.
--
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]
