You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "amogh-jahagirdar (via GitHub)" <gi...@apache.org> on 2026/06/08 18:00:22 UTC
Re: [PR] Core: Preserve DV encryption metadata in merges [iceberg]
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org