gaborkaszab commented on code in PR #18503:
URL: https://github.com/apache/iceberg/pull/18503#discussion_r4294158556
##########
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_ | **`file-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 manifest list key metadata (**Deprecated**: use
`encryption-key` instead) |
Review Comment:
I wasn't entirely sure what it means to drop such a field in V4. I wanted to
avoid expressing that when we upgrade the table to V4 we have to rewrite
`key-id` into `encryption-key`.
With the current setup don't we say that `key-id` might be set from the V3
version of the table, but post upgrade to V4 we should write the new snapshots
with `encryption-key`? At least this was my intention.
If I got it wrong, I'll remove the `key-id` field.
##########
format/spec.md:
##########
@@ -1470,9 +1474,18 @@ Keys used for table encryption can be tracked in table
metadata as a list named
| | | _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 |
+=== "v4"
Review Comment:
V3 manifest lists use `key-id`, V4 root manifests don't. I'm not entirely
sure we can drop the field from the V4 tab because of the same reason.
I recall I did drop the `key-metadata` field from statistics replacing it
with `key-id` in V4, and I got a comment that we can't do that because that
would mean we can't carry forward V3 written stats and we have to rewrite them
to the new V4 format when upgrading.
([link](https://lists.apache.org/thread.html/s2649vlgw3xxr2bjptszvv5cgslrgm0p))
Isn't it the same case here? V3 encryption keys could still exist after
upgrade, so In V4 we could have these objects either set or unset `key-id`.
Hence I figured this should be optional and not dropped.
Let me know if I get this wrong!
##########
format/spec.md:
##########
@@ -1470,9 +1474,18 @@ Keys used for table encryption can be tracked in table
metadata as a list named
| | | _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 |
+=== "v4"
+ | v4 | Field name | Type |
Description |
+ | ----------
|-------------------------------|-----------------------|-------------|
+ | _optional_ | **`key-id`** | `string` | ID
of the encryption key [2] |
+ | _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 |
+
Notes:
1. The format of encrypted key metadata is determined by the table's
encryption scheme and can be a wrapped format specific to the table's KMS
provider.
+2. `key-id` is required when a key is stored indirectly from the file it
encrypts, such as in the table metadata `encryption-keys` list. It is optional
when the key is stored directly on the encrypted file's metadata.
Review Comment:
I deliberately didn't say it's stored in the snapshot metadata, because that
won't be true for the statistics file being covered later on.
I rephrased this. Let me know if it's better.
##########
format/spec.md:
##########
@@ -2104,6 +2117,11 @@ Snapshot timestamp changes:
* When committed while the table is at version 4 or higher, a snapshot's
`timestamp-ms` must be strictly greater than its parent snapshot's
`timestamp-ms` on the same branch.
* Snapshots with no parent and snapshots that existed before a table was
upgraded to v4 are not constrained.
+Snapshot encryption key changes:
+
+* Writers must store a snapshot's encryption key directly in the snapshot's
`encryption-key` field and must not write `key-id`
Review Comment:
I wouldn't explicitly say "V4 writers" because this is the part of the spec
to include the format version changes for V4. I take the other part of your
suggestion, thanks!
##########
format/spec.md:
##########
@@ -2104,6 +2117,11 @@ Snapshot timestamp changes:
* When committed while the table is at version 4 or higher, a snapshot's
`timestamp-ms` must be strictly greater than its parent snapshot's
`timestamp-ms` on the same branch.
* Snapshots with no parent and snapshots that existed before a table was
upgraded to v4 are not constrained.
+Snapshot encryption key changes:
+
+* Writers must store a snapshot's encryption key directly in the snapshot's
`encryption-key` field and must not write `key-id`
+* `key-id` and `encryption-key` are mutually exclusive; snapshots that existed
before a table was upgraded to v4 may continue to use `key-id`
Review Comment:
Sure, removed
##########
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_ | **`file-size-in-bytes`** | Size of the root manifest
file in bytes |
Review Comment:
yes, renamed
--
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]
