gaborkaszab commented on code in PR #18503:
URL: https://github.com/apache/iceberg/pull/18503#discussion_r4460661998
##########
format/spec.md:
##########
@@ -1107,11 +1107,13 @@ A snapshot consists of the following fields:
| _required_ | **`sequence-number`** | A monotonically increasing
long that tracks the order of changes to a table |
| _required_ | **`timestamp-ms`** | A timestamp when the
snapshot was created, used for garbage collection and table inspection |
| _required_ | **`root-manifest`** | The location of the root
manifest for this snapshot |
+ | _required_ | **`root-manifest-size-in-bytes`** | Size of the root
manifest file in bytes |
Review Comment:
I think I initially got @ggershinsky 's comment wrong, and this field is not
encryption related. Moved to a [separate
PR](https://github.com/apache/iceberg/pull/18780).
##########
format/spec.md:
##########
@@ -1241,7 +1241,7 @@ A snapshot consists of the following fields:
| _optional_ | _optional_ | _optional_ | **`schema-id`** | ID
of the table's current schema when the snapshot was created |
| | | _required_ | **`first-row-id`** |
The first `_row_id` assigned to the first row in the first data file in the
first manifest, see [Row Lineage](#row-lineage) |
| | | _required_ | **`added-rows`** |
The upper bound of the number of rows with assigned row IDs, see [Row
Lineage](#row-lineage) |
- | | | _optional_ | **`key-id`** | ID
of the encryption key that encrypts the manifest list key metadata |
+ | | | _optional_ | **`key-id`** | ID
of the encryption key that encrypts the manifest list |
Review Comment:
See my comment below on the V4 table: I think the current description is
incorrect, I'm fixing it together for both versions. Separate PR seems overkill
because we touch encryption with the current one.
##########
format/spec.md:
##########
@@ -1269,6 +1271,8 @@ The snapshot summary's `operation` field is used by some
operations, like snapsh
For other optional snapshot summary fields, see [Appendix
F](#optional-snapshot-summary-fields).
+For encrypted v4 tables, the encryption key for the snapshot root file may be
referenced by `key-id` or stored directly in the `encryption-key` field.
Review Comment:
Removed
##########
format/spec.md:
##########
@@ -1608,18 +1612,19 @@ If a table has no deletes or only deletion vectors,
implementations are encourag
#### Encryption Keys
-Keys used for table encryption can be tracked in table metadata as a list
named `encryption-keys`. The schema of each key is a struct with the following
fields:
-=== "v1 - v3"
- | v1 | v2 | v3 | Field name | Type
| Description |
-
|----|----|------------|-------------------------------|-----------------------|-------------|
- | | | _required_ | **`key-id`** | `string`
| ID of the encryption key |
- | | | _required_ | **`encrypted-key-metadata`** | `string`
| Encrypted key and metadata, base64 encoded [1] |
- | | | _optional_ | **`encrypted-by-id`** | `string`
| Optional ID of the key used to encrypt or wrap `key-metadata` |
- | | | _optional_ | **`properties`** | `map<string,
string>` | A string to string map of additional metadata used by the table's
encryption scheme |
+Keys used for table encryption can be tracked in table metadata as a list
named `encryption-keys`, or embedded directly in the metadata object that
references the encrypted file, such as the `encryption-key` field of a v4
snapshot. The schema of each key is a struct with the following fields:
+=== "v1 - v4"
+ | v1 and v2 | v3 | v4 | Field name |
Type | Description |
+
|-----------|------------|------------|-------------------------------|-----------------------|-------------|
+ | | _required_ | _optional_ | **`key-id`** |
`string` | ID of the encryption key [2] |
Review Comment:
Reverted the changes here
##########
format/spec.md:
##########
@@ -1251,11 +1251,13 @@ A snapshot consists of the following fields:
| _required_ | **`sequence-number`** | A monotonically increasing
long that tracks the order of changes to a table |
| _required_ | **`timestamp-ms`** | A timestamp when the
snapshot was created, used for garbage collection and table inspection |
| _required_ | **`root-manifest`** | The location of the root
manifest for this snapshot |
+ | _required_ | **`root-manifest-size-in-bytes`** | Size of the root
manifest file in bytes |
| _required_ | **`summary`** | A string map that summarizes
the snapshot changes, including `operation` as a _required_ field (see below) |
| _optional_ | **`schema-id`** | ID of the table's current
schema when the snapshot was created |
| _required_ | **`first-row-id`** | The first `_row_id` assigned
to the first row in the first data file in the first manifest, see [Row
Lineage](#row-lineage) |
| _required_ | **`added-rows`** | The upper bound of the
number of rows with assigned row IDs, see [Row Lineage](#row-lineage) |
- | _optional_ | **`key-id`** | ID of the encryption key
that encrypts the root manifest key metadata |
+ | _optional_ | **`key-id`** | ID of the encryption key
that encrypts the root manifest |
Review Comment:
@rdblue No, the key that encrypts the snapshot's key metadata is the KEK,
but we don't refer to that here. We refer to the snapshot's (encrypted) key
metadata itself here, that will in turn reference the KEK.
I don't think the original description is correct, also seems wrong in the
V3 table, hence the same change there.
##########
format/spec.md:
##########
@@ -1251,11 +1251,13 @@ A snapshot consists of the following fields:
| _required_ | **`sequence-number`** | A monotonically increasing
long that tracks the order of changes to a table |
| _required_ | **`timestamp-ms`** | A timestamp when the
snapshot was created, used for garbage collection and table inspection |
| _required_ | **`root-manifest`** | The location of the root
manifest for this snapshot |
+ | _required_ | **`root-manifest-size-in-bytes`** | Size of the root
manifest file in bytes |
| _required_ | **`summary`** | A string map that summarizes
the snapshot changes, including `operation` as a _required_ field (see below) |
| _optional_ | **`schema-id`** | ID of the table's current
schema when the snapshot was created |
| _required_ | **`first-row-id`** | The first `_row_id` assigned
to the first row in the first data file in the first manifest, see [Row
Lineage](#row-lineage) |
| _required_ | **`added-rows`** | The upper bound of the
number of rows with assigned row IDs, see [Row Lineage](#row-lineage) |
- | _optional_ | **`key-id`** | ID of the encryption key
that encrypts the root manifest key metadata |
+ | _optional_ | **`key-id`** | ID of the encryption key
that encrypts the root manifest |
+ | _optional_ | **`encryption-key`** | The [encryption
key](#encryption-keys) that encrypts the root manifest |
Review Comment:
Yes, this is the encryption key struct.
I just double checked that `properties` from that structure is only used for
KEKs and not the keys encrypting the snapshot root file. However, since it's
there in the encryption key structure, shouldn't we account for implementations
that already populate that not just for KEKs? They would loose that option.
Anyway, my hesitance here is that if we add `encrypted-key-metadata` and
`encrypted-by-id` here, together with the existing `key-id` field we have 3
encryption related fields, where 2 of them are coupled together while the third
is independent. Might cause confusion later on.
Applied this change to show how it looks, but still hesitant how intuitive
this is, and worried if custom implementations loose their `properties` field.
Added a note below about to add some explanation.
--
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]
