-
Notifications
You must be signed in to change notification settings - Fork 3k
Core: add key_metadata in ManifestFile #2520
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -62,14 +62,16 @@ public interface ManifestFile { | |
| Types.NestedField PARTITION_SUMMARIES = optional(507, "partitions", | ||
| Types.ListType.ofRequired(508, PARTITION_SUMMARY_TYPE), | ||
| "Summary for each partition"); | ||
| // next ID to assign: 519 | ||
| Types.NestedField KEY_METADATA = optional(519, "key_metadata", Types.BinaryType.get(), | ||
| "Encryption key metadata blob"); | ||
| // next ID to assign: 520 | ||
|
|
||
| Schema SCHEMA = new Schema( | ||
| PATH, LENGTH, SPEC_ID, MANIFEST_CONTENT, | ||
| SEQUENCE_NUMBER, MIN_SEQUENCE_NUMBER, SNAPSHOT_ID, | ||
| ADDED_FILES_COUNT, EXISTING_FILES_COUNT, DELETED_FILES_COUNT, | ||
| ADDED_ROWS_COUNT, EXISTING_ROWS_COUNT, DELETED_ROWS_COUNT, | ||
| PARTITION_SUMMARIES); | ||
| PARTITION_SUMMARIES, KEY_METADATA); | ||
|
|
||
| static Schema schema() { | ||
| return SCHEMA; | ||
|
|
@@ -179,6 +181,13 @@ default boolean hasDeletedFiles() { | |
| */ | ||
| List<PartitionFieldSummary> partitions(); | ||
|
|
||
| /** | ||
| * Returns metadata about how this manifest file is encrypted, or null if the file is stored in plain text. | ||
| */ | ||
| default ByteBuffer keyMetadata() { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does keyMetadata have substructure or is it a pure binary buffer? Looks like it will have substructures form the description. Are we going to to define it later or in this patch?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was wondering about this too, should we make this a struct with name like
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The intent here is that an encryption manager can decide what the key metadata holds. It could be an encrypted key or it could be a key reference. There are lots of possibilities and we did it this way to not constrain what the encryption manager can choose to do.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1 to a byte array, serialized by an encryption manager from its structs. Btw, besides encryption keys, we have the AAD prefixes. We can keep them inside the key metadata (because it is convenient and flexible) - or we can add a separate manifest field/column for them (because technically AADs are not used for key retrieval). In both cases, the decision can be made later, when we get to handle end-to-end table integrity. |
||
| return null; | ||
| } | ||
|
|
||
| /** | ||
| * Copies this {@link ManifestFile manifest file}. Readers can reuse manifest file instances; use | ||
| * this method to make defensive copies. | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added this to the spec in #2654.