kbendick commented on a change in pull request #3049:
URL: https://github.com/apache/iceberg/pull/3049#discussion_r698066215
##########
File path: core/src/main/java/org/apache/iceberg/SnapshotParser.java
##########
@@ -146,19 +151,31 @@ static Snapshot fromJson(FileIO io, JsonNode node) {
}
Integer schemaId = JsonUtil.getIntOrNull(SCHEMA_ID, node);
+ String keyMetadataLocation =
JsonUtil.getStringOrNull(KEY_METADATA_LOCATION, node);
if (node.has(MANIFEST_LIST)) {
// the manifest list is stored in a manifest list file
String manifestList = JsonUtil.getString(MANIFEST_LIST, node);
- return new BaseSnapshot(
- io, sequenceNumber, snapshotId, parentId, timestamp, operation,
summary, schemaId, manifestList);
+ return new BaseSnapshot(io, sequenceNumber, snapshotId, parentId,
timestamp, operation, summary, schemaId,
+ manifestList, keyMetadataLocation);
} else {
// fall back to an embedded manifest list. pass in the manifest's
InputFile so length can be
// loaded lazily, if it is needed
List<ManifestFile> manifests =
Lists.transform(JsonUtil.getStringList(MANIFESTS, node),
location -> new GenericManifestFile(io.newInputFile(location), 0));
- return new BaseSnapshot(io, snapshotId, parentId, timestamp, operation,
summary, schemaId, manifests);
+
+ ByteBuffer keyMetadata = null;
+ if (keyMetadataLocation != null) {
+ try (InputStream stream =
io.newInputFile(keyMetadataLocation).newStream()) {
+ keyMetadata = ByteBuffer.wrap(ByteStreams.toByteArray(stream));
+ } catch (IOException e) {
+ throw new UncheckedIOException("Failed to read key metadata at
location " + keyMetadataLocation, e);
+ }
+ }
Review comment:
Nit: This seems to be repeated. Would it make sense to put this into a
`EncryptionUtils` or `EncKeyUtils` or similar?
Might be premature as I'm not sure of all of the possible cases and I don't
_love_ when an overly generic utility class is introduced that has the
potential to get relatively large. Though that might not be the case here.
##########
File path: core/src/test/java/org/apache/iceberg/TestTableMetadata.java
##########
@@ -411,11 +411,11 @@ public void testAddPreviousMetadataRemoveMultiple() {
long previousSnapshotId = System.currentTimeMillis() - new
Random(1234).nextInt(3600);
Snapshot previousSnapshot = new BaseSnapshot(
ops.io(), previousSnapshotId, null, previousSnapshotId, null, null,
null, ImmutableList.of(
- new GenericManifestFile(localInput("file:/tmp/manfiest.1.avro"),
SPEC_5.specId())));
+ new GenericManifestFile(localInput("file:/tmp/manfiest.1.avro"),
SPEC_5.specId())), null);
long currentSnapshotId = System.currentTimeMillis();
Snapshot currentSnapshot = new BaseSnapshot(
ops.io(), currentSnapshotId, previousSnapshotId, currentSnapshotId,
null, null, null, ImmutableList.of(
- new GenericManifestFile(localInput("file:/tmp/manfiest.2.avro"),
SPEC_5.specId())));
+ new GenericManifestFile(localInput("file:/tmp/manfiest.2.avro"),
SPEC_5.specId())), null);
Review comment:
Nit: Is it possible to avoid having to pass all of these `null`s in?
##########
File path: api/src/main/java/org/apache/iceberg/Snapshot.java
##########
@@ -135,4 +136,20 @@
default Integer schemaId() {
return null;
}
+
+ /**
+ * Returns the location of the manifest list's encryption key metadata file
location,
+ * or null if there is no key metadata.
+ */
+ default String keyMetadataLocation() {
+ return null;
+ }
+
+ /**
+ * Returns metadata about how the manifest list in the snapshot is encrypted,
+ * or null if the file is stored in plain text.
+ */
+ default ByteBuffer keyMetadata() {
+ return null;
+ }
Review comment:
Open question (non-blocking): Should we consider adding in a
`VoidKeyMetadata` or similar `null` type?
My thinking is similar to the `VoidTransform` for tables that were
previously partitioned and then now are not. We use the `VoidTransform` (if I'm
not mistaken) to help indicate that.
If users were to encrypt their table, and then decide to decrypt the table,
would we need a way of expressing that?
Also, I'm just not a big fan of using `null` as a default (though I
recognize that it's fine and valid here).
--
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]
