amogh-jahagirdar commented on code in PR #15911:
URL: https://github.com/apache/iceberg/pull/15911#discussion_r3375284321
##########
core/src/main/java/org/apache/iceberg/deletes/BaseDVFileWriter.java:
##########
@@ -52,20 +57,31 @@ public class BaseDVFileWriter implements DVFileWriter {
private static final String REFERENCED_DATA_FILE_KEY =
"referenced-data-file";
private static final String CARDINALITY_KEY = "cardinality";
- private final Supplier<OutputFile> dvOutputFile;
+ private final Supplier<EncryptedOutputFile> dvOutputFile;
private final Function<String, PositionDeleteIndex> loadPreviousDeletes;
private final Map<String, Deletes> deletesByPath = Maps.newHashMap();
private final Map<String, BlobMetadata> blobsByPath = Maps.newHashMap();
+ private EncryptionKeyMetadata keyMetadata = null;
Review Comment:
Why does this need to be saved as a field? It's a bit suspect, considering
we already have an EncryptedOutputFile to work with
##########
data/src/test/java/org/apache/iceberg/io/TestDVWriters.java:
##########
@@ -354,6 +400,69 @@ private DeleteFile writePositionDeletes(
return writer.toDeleteFile();
}
+ private static long decode(ByteBuffer buffer) {
+ return buffer.duplicate().order(ByteOrder.LITTLE_ENDIAN).getLong();
+ }
+
+ private record TestEncryptedOutputFile(OutputFile outputFile,
EncryptionKeyMetadata keyMetadata)
+ implements EncryptedOutputFile {
+
+ @Override
+ public OutputFile encryptingOutputFile() {
+ return outputFile;
+ }
+ }
+
+ private record TestEncryptionKeyMetadata(ByteBuffer buffer) implements
EncryptionKeyMetadata {
Review Comment:
Same as above, I don't think we need this class, I think we can just pass in
the bytebuffer directly to encryptedOutput
##########
data/src/test/java/org/apache/iceberg/io/TestDVWriters.java:
##########
@@ -354,6 +400,69 @@ private DeleteFile writePositionDeletes(
return writer.toDeleteFile();
}
+ private static long decode(ByteBuffer buffer) {
+ return buffer.duplicate().order(ByteOrder.LITTLE_ENDIAN).getLong();
+ }
+
+ private record TestEncryptedOutputFile(OutputFile outputFile,
EncryptionKeyMetadata keyMetadata)
Review Comment:
I don't think we need this class, why can't we just use `
EncryptedFiles.encryptedOutput`?
##########
core/src/main/java/org/apache/iceberg/deletes/BaseDVFileWriter.java:
##########
@@ -52,20 +57,31 @@ public class BaseDVFileWriter implements DVFileWriter {
private static final String REFERENCED_DATA_FILE_KEY =
"referenced-data-file";
private static final String CARDINALITY_KEY = "cardinality";
- private final Supplier<OutputFile> dvOutputFile;
+ private final Supplier<EncryptedOutputFile> dvOutputFile;
private final Function<String, PositionDeleteIndex> loadPreviousDeletes;
private final Map<String, Deletes> deletesByPath = Maps.newHashMap();
private final Map<String, BlobMetadata> blobsByPath = Maps.newHashMap();
+ private EncryptionKeyMetadata keyMetadata = null;
Review Comment:
Looks like this is just state shared between `newWriter` and `createDV` and
both are called via `close`. Probably just inline the contents of `newWriter`,
and capture the EncrytpionKeyMEtadata within close and pass it through to the
createDV call.
--
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]
