gaborkaszab commented on code in PR #17523:
URL: https://github.com/apache/iceberg/pull/17523#discussion_r3729393240
##########
api/src/main/java/org/apache/iceberg/encryption/EncryptingFileIO.java:
##########
@@ -120,15 +121,24 @@ public InputFile newInputFile(ManifestFile manifest) {
}
@Override
- public InputFile newInputFile(ManifestListFile manifestList) {
- if (manifestList.encryptionKeyID() != null) {
- ByteBuffer keyMetadata = manifestList.decryptKeyMetadata(em);
- return newDecryptingInputFile(manifestList.location(), keyMetadata);
+ public InputFile newInputFile(SnapshotFile snapshotFile) {
Review Comment:
I was poking this code for `StatisticsFile` encryption recently and I had
the impression that these `newInputFile` function parameters are maybe too
narrow.
The things we need here aren't really specific to manifest list or snapshot
file or any other file:
1) What we need is the `encryptionKeyId`, the `location` and raw
`key-metadata`.
2) calling `snapshotFile.decryptKeyMetadata` is somewhat misleading because
there is nothing specific happens internally to SnapshotFile or
BaseManifestListFile.
I'm wondering if we want to scratch this question within this PR, but might
be the right time to introduce a more general way of implementing
`newInputFile(location, encryptionKeyId, key_metadata)`
##########
core/src/main/java/org/apache/iceberg/SnapshotsTable.java:
##########
@@ -37,7 +37,8 @@ public class SnapshotsTable extends BaseMetadataTable {
Types.NestedField.optional(
6,
"summary",
- Types.MapType.ofRequired(7, 8, Types.StringType.get(),
Types.StringType.get())));
+ Types.MapType.ofRequired(7, 8, Types.StringType.get(),
Types.StringType.get())),
+ Types.NestedField.optional(9, "snapshot_file",
Types.StringType.get()));
Review Comment:
Do we want to change the snapshots metadata table with this PR? I think
metadata tables are orthogonal and can be taken care separately. This PR is to
introduce another level of abstraction, but how to visualize it in the snapshot
table is a different topic IMO.
##########
api/src/main/java/org/apache/iceberg/encryption/EncryptingFileIO.java:
##########
@@ -120,15 +121,24 @@ public InputFile newInputFile(ManifestFile manifest) {
}
@Override
- public InputFile newInputFile(ManifestListFile manifestList) {
- if (manifestList.encryptionKeyID() != null) {
- ByteBuffer keyMetadata = manifestList.decryptKeyMetadata(em);
- return newDecryptingInputFile(manifestList.location(), keyMetadata);
+ public InputFile newInputFile(SnapshotFile snapshotFile) {
+ if (snapshotFile.encryptionKeyID() != null) {
+ ByteBuffer keyMetadata = snapshotFile.decryptKeyMetadata(em);
+ return newDecryptingInputFile(snapshotFile.location(), keyMetadata);
} else {
- return newInputFile(manifestList.location());
+ return newInputFile(snapshotFile.location());
}
}
+ /**
+ * @deprecated since 1.13.0; use {@link #newInputFile(SnapshotFile)}.
Review Comment:
2 comments:
1) Next release is 1.12.0. Shouldn't we target that with the deprecation?
2) Shouldn't we also articulate when it will be dropped? Since it's in
`api/`, `2.0.0`, I guess
This goes for all deprecation in this PR, except that in `core/` we can drop
earlier, in `1.13.0`
##########
api/src/main/java/org/apache/iceberg/SnapshotFile.java:
##########
@@ -0,0 +1,39 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg;
+
+import java.nio.ByteBuffer;
+import org.apache.iceberg.encryption.EncryptionManager;
+
+/**
+ * The top-level file that a {@link Snapshot} points at. For v3 and earlier
this is a manifest list
+ * (see {@link ManifestListFile}); for v4+ it is a root manifest carrying a
mix of data-file entries
Review Comment:
I'm wondering if we want to take this one step further and making even more
general: I was examining encryption/decryption for `StatisticsFile` and
`PartitionStatistics` recently, and I found that we have many specializations
for the relevant `EncryptiongFileIO`, `FileIO` and `EncryptionUtil` functions
for `ManifestListFile`, but in fact that contains nothing that is manifest list
specific.
I think all this boils down to having the necessary fields to create either
a regular Input/Output file or the encrypting/decryption versions. The code for
this is general: you need a location, an encryption key ID that refers to some
encryption key in `TableMetadata.encryptionKeys` and a mechanism to decrypt the
`key-metadata` referred by the key id. Even this mechanism is not specific to
manifest lists or V4 snapshot files.
My point is that I think whatever structure we introduce now to serve a
general purpose than `ManifestListFile`, it should be even more general than
`SnapshotFile` and could cover all file types that are keeping their encryption
keys within `TableMetadata.encryptionKeys`. I'm in trouble with the naming of
such a common interface, though.
Just for the record, [here is the
PR](https://github.com/apache/iceberg/pull/17417) where I introduce the same
for `StatisticsFile`.
If you want, I can explore the more general approach in a separate PR, but
it has a direct effect on this one too.
##########
core/src/main/java/org/apache/iceberg/encryption/EncryptionUtil.java:
##########
@@ -134,25 +135,44 @@ public static ByteBuffer setFileLength(ByteBuffer
keyMetadata, long fileLength)
return
StandardKeyMetadata.parse(keyMetadata).copyWithLength(fileLength).buffer();
}
+ /**
+ * Decrypt the key metadata for the top-level file that a snapshot points at
(a v3 manifest list
+ * or a v4+ root manifest).
+ *
+ * @param snapshotFile a SnapshotFile
+ * @param em the table's EncryptionManager
+ * @return a decrypted key metadata buffer
+ */
+ public static ByteBuffer decryptSnapshotFileKeyMetadata(
+ SnapshotFile snapshotFile, EncryptionManager em) {
+ return decryptSnapshotFileKeyMetadata(snapshotFile.encryptionKeyID(), em);
+ }
+
/**
* Decrypt the key metadata for a manifest list.
*
* @param manifestList a ManifestListFile
* @param em the table's EncryptionManager
* @return a decrypted key metadata buffer
+ * @deprecated since 1.13.0; use {@link
#decryptSnapshotFileKeyMetadata(SnapshotFile,
+ * EncryptionManager)}.
*/
+ @Deprecated
public static ByteBuffer decryptManifestListKeyMetadata(
ManifestListFile manifestList, EncryptionManager em) {
+ return decryptSnapshotFileKeyMetadata(manifestList.encryptionKeyID(), em);
+ }
+
+ private static ByteBuffer decryptSnapshotFileKeyMetadata(String keyId,
EncryptionManager em) {
Review Comment:
This doesn't have to be snapshot file specific. The algorithm is pretty
general to all files referenced in table metadata. [See
here](https://github.com/apache/iceberg/pull/17417/changes#diff-d5672d217e4ba33b04085c007604bd32ebf266837e713988d69b6967940be72fR205).
##########
api/src/main/java/org/apache/iceberg/ManifestFile.java:
##########
@@ -29,6 +29,12 @@
public interface ManifestFile {
int PARTITION_SUMMARIES_ELEMENT_ID = 508;
+ /**
+ * Sentinel returned by {@link Snapshot#formatVersion()} for snapshots that
do not report a format
+ * version — e.g. v3-and-earlier snapshots whose top-level file is a
manifest list.
+ */
+ int LEGACY_FORMAT_VERSION = 0;
Review Comment:
In case this PR gets merged earlier: I prefer the shorter comment as it is
in the other PR:
`/** Format version for pre-v4 manifest files. */`
Also, shouldn't this be static final? (true for the linked PR as well)
--
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]
