API, Core: Add table metadata keys for encryption #12927
Conversation
| * | ||
| * @return a string key ID | ||
| */ | ||
| default String keyId() { |
There was a problem hiding this comment.
would it make sense to name this encryptionKeyId as keyId might not be immediately obvious that it relates to encryption when browsing/reading the code
There was a problem hiding this comment.
didn't we call this encryptingKeyId in the spec proposal?
NVM we have this one as key-id
There was a problem hiding this comment.
I don't think that this is unclear and needs to be longer, but I'm open to making it encryptionKeyId if others think that it is.
There was a problem hiding this comment.
I don't want to change it unless we change it in the spec as well
core/src/main/java/org/apache/iceberg/RewriteTablePathUtil.java
Outdated
Show resolved
Hide resolved
| @@ -201,6 +201,14 @@ public static ByteBuffer getByteBufferOrNull(String property, JsonNode node) { | |||
| BaseEncoding.base16().decode(pNode.textValue().toUpperCase(Locale.ROOT))); | |||
| } | |||
|
|
|||
| public static Map<String, String> getStringMapOrNull(String property, JsonNode node) { | |||
There was a problem hiding this comment.
would be good to test these new methods in TestJsonUtil
rdblue
force-pushed
the
encryption-add-table-metadata-keys
branch
from
e09c590 to
cc162e8
Compare
|
Updated for review comments and to add tests that I didn't add in the initial push. |
|
Thanks for reviewing, @nastra and @RussellSpitzer! |
This implements the spec changes from #12162.
It adds the
encryption-keyslist toTableMetadatathat stores anEncryptedKey. TheTableMetadata.Builderis updated withaddEncryptionKeyandremoveEncryptionKeymethods, along with corresponding REST protocol metadata updates. This also addskey-idto snapshot metadata.