amogh-jahagirdar commented on code in PR #15911:
URL: https://github.com/apache/iceberg/pull/15911#discussion_r3268226271
##########
core/src/main/java/org/apache/iceberg/MergingSnapshotProducer.java:
##########
Review Comment:
This doesn't need to change right?
##########
core/src/main/java/org/apache/iceberg/deletes/BaseDVFileWriter.java:
##########
@@ -166,8 +180,23 @@ private void write(PuffinWriter writer, Deletes deletes) {
}
private PuffinWriter newWriter() {
- OutputFile outputFile = dvOutputFile.get();
- return
Puffin.write(outputFile).createdBy(IcebergBuild.fullVersion()).build();
+ EncryptedOutputFile outputFile = dvOutputFile.get();
+ this.keyMetadata = outputFile.keyMetadata();
+ return Puffin.write(outputFile.encryptingOutputFile())
+ .createdBy(IcebergBuild.fullVersion())
+ .build();
+ }
+
+ private ByteBuffer keyMetadata(long fileSizeInBytes) {
+ if (keyMetadata != null) {
Review Comment:
Do we need the null check at this level in the abstraction? It's always set
in newWriter() anyways.
Feel like:
```
private ByteBuffer keyMetadata(long fileSizeInBytes) {
if (keyMetadata instanceof NativeEncryptionKeyMetadata) {
return ((NativeEncryptionKeyMetadata)
keyMetadata).copyWithLength(fileSizeInBytes).buffer();
} else {
return keyMetadata.buffer();
}
}
```
should be all that's needed but let me know if i'm missing something
##########
core/src/main/java/org/apache/iceberg/deletes/BaseDVFileWriter.java:
##########
@@ -166,8 +180,23 @@ private void write(PuffinWriter writer, Deletes deletes) {
}
private PuffinWriter newWriter() {
- OutputFile outputFile = dvOutputFile.get();
- return
Puffin.write(outputFile).createdBy(IcebergBuild.fullVersion()).build();
+ EncryptedOutputFile outputFile = dvOutputFile.get();
+ this.keyMetadata = outputFile.keyMetadata();
+ return Puffin.write(outputFile.encryptingOutputFile())
+ .createdBy(IcebergBuild.fullVersion())
+ .build();
+ }
+
+ private ByteBuffer keyMetadata(long fileSizeInBytes) {
+ if (keyMetadata != null) {
+ if (keyMetadata instanceof NativeEncryptionKeyMetadata) {
+ return ((NativeEncryptionKeyMetadata)
keyMetadata).copyWithLength(fileSizeInBytes).buffer();
+ } else {
+ return ByteBuffers.copy(keyMetadata.buffer());
Review Comment:
I don't think we need to copy
##########
core/src/test/java/org/apache/iceberg/deletes/TestBaseDVFileWriter.java:
##########
@@ -0,0 +1,187 @@
+/*
+ * 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.deletes;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.io.IOException;
+import java.nio.ByteBuffer;
+import java.nio.ByteOrder;
+import java.nio.file.Path;
+import org.apache.iceberg.DeleteFile;
+import org.apache.iceberg.FileFormat;
+import org.apache.iceberg.FileMetadata;
+import org.apache.iceberg.Files;
+import org.apache.iceberg.PartitionSpec;
+import org.apache.iceberg.encryption.EncryptedOutputFile;
+import org.apache.iceberg.encryption.EncryptionKeyMetadata;
+import org.apache.iceberg.encryption.NativeEncryptionKeyMetadata;
+import org.apache.iceberg.io.OutputFile;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList;
+import org.apache.iceberg.relocated.com.google.common.collect.Iterables;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.io.TempDir;
+
+class TestBaseDVFileWriter {
Review Comment:
Do we need a separate class for all these tests? There's already a
TestDVWriters. I'd also recommend just some simple tests which just verify the
encryption portion for the fix, it looks like some of the tests in this class
test more than that.
##########
core/src/main/java/org/apache/iceberg/DVUtil.java:
##########
@@ -188,8 +190,8 @@ private static List<DeleteFile> writeDVs(
FileIO fileIO,
String dvOutputLocation,
Map<String, Pair<PartitionSpec, StructLike>> partitions) {
- OutputFile dvOutputFile = fileIO.newOutputFile(dvOutputLocation);
- try (DVFileWriter dvFileWriter = new BaseDVFileWriter(() -> dvOutputFile,
path -> null)) {
+ EncryptedOutputFile dvOutputFile = newDVOutputFile(fileIO,
dvOutputLocation);
Review Comment:
Minor: Like the helper method but maybe we should just inline the call when
iniitializing the writer, up to you
##########
core/src/main/java/org/apache/iceberg/DVUtil.java:
##########
@@ -203,4 +205,12 @@ private static List<DeleteFile> writeDVs(
throw new UncheckedIOException(e);
}
}
+
+ private static EncryptedOutputFile newDVOutputFile(FileIO fileIO, String
location) {
+ if (fileIO instanceof EncryptingFileIO encryptingFileIO) {
+ return encryptingFileIO.newEncryptingOutputFile(location);
+ } else {
+ return
EncryptedFiles.plainAsEncryptedOutput(fileIO.newOutputFile(location));
+ }
Review Comment:
Minor: This is one of those cases where I'd just leave out the explicit
else, and just fall through to returning plainText.
--
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]
