stevenzwu commented on code in PR #18503:
URL: https://github.com/apache/iceberg/pull/18503#discussion_r4291455213
##########
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:
Is my understanding and phrasing below accurate?
> V4 writers must store the root manifest file's encryption key directly in
the snapshot's `encryption-key` field and must not write `key-id`.
##########
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:
is `root-manifest-size-in-bytes` more clear? I know it is a bit longer
##########
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:
> `key-id` and `encryption-key` are mutually exclusive;
I would probably remove the above part, and only keep the later part. Then
the two bullet points together essentially cover the mutually exclusive point.
--
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]
