Skip to content

API, Core: Introduce a generic abstraction to replace ManifestListFile #17545

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
gaborkaszab wants to merge 1 commit into
base: main
Choose a base branch
from
Open

API, Core: Introduce a generic abstraction to replace ManifestListFile #17545

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions api/src/main/java/org/apache/iceberg/ManifestListFile.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,12 @@
import java.nio.ByteBuffer;
import org.apache.iceberg.encryption.EncryptionManager;

/**
* @deprecated since 1.12.0. Will be removed in 2.0.0; Use {@link
* org.apache.iceberg.io.FileIO#newInputFile(String, String)} and {@link
* EncryptionManager#decryptKeyMetadata(String)} providing location and keyId.
*/
@Deprecated
public interface ManifestListFile {

/** Location of manifest list file. */
Expand Down
15 changes: 15 additions & 0 deletions api/src/main/java/org/apache/iceberg/encryption/EncryptingFileIO.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,11 @@ public InputFile newInputFile(ManifestFile manifest) {
}
}

/**
* @deprecated since 1.12.0. Will be removed in 2.0.0; use {@link #newInputFile(String, String)}
* instead.
*/
@Deprecated
@Override
public InputFile newInputFile(ManifestListFile manifestList) {
if (manifestList.encryptionKeyID() != null) {
Expand All @@ -140,6 +145,16 @@ public InputFile newInputFile(ManifestListFile manifestList) {
}
}

@Override
public InputFile newInputFile(String location, String keyId) {
if (keyId != null) {

@RussellSpitzer RussellSpitzer Sep 18, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I probably would change this to require that keyId is nonNull.

I don't think we want folks calling this method if they don't expect to be using a key

@RussellSpitzer RussellSpitzer Sep 18, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Precondition keyId.notNull

@gaborkaszab gaborkaszab Sep 18, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think such a precondition would break the contract of EncryptionFileIO. If I'm not mistaken, EncryptingFileIO theoretically can be created even if the table is not encrypted, and other functions here also seem to branch on key_metadata being null or not, where null defaults to the non-encrypting case. I think for consistency we should follow that pattern with the keyId variation of newFileIO functions.
WDYT @RussellSpitzer ?

@RussellSpitzer RussellSpitzer Sep 18, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I follow. In this PR we now provide 2 APIs

new inputFile(String location)

and

new InputFile(String location, String keyID)

Usually I would expect the first to call the second. But here we have the second call the first.

The question is what is the point of the first API if I have the second one and can pass String keyID) as null. My assumption would be that if I pass through a keyId I am attempting to have that used by the fileIO. If I have passed through "null" I'm probably not doing something right because I should have called "new inputFile(location)".

We also should consider the integration of this API with the "Length" passthrough

Let me jump down to default definition to add some more comments

@RussellSpitzer RussellSpitzer Sep 18, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think overall we are still stuck a bit too much mimicking the logic that was here before for ManifestLists without thinking about how we would add this API by first principals.

We are essentially adding a new API that a FileIO user can call if they have key metadata. We know they also will have a path and may have a length. So it probably makes sense to build this up similar to the other methods in FileIO.

Right now we have

  /** Get a {@link InputFile} instance to read bytes from the file at the given path. */
  InputFile newInputFile(String path);

  /**
   * Get a {@link InputFile} instance to read bytes from the file at the given path, with a known
   * file length.
   */
  default InputFile newInputFile(String path, long length) {
    return newInputFile(path);
  }

So we probably also need

 /**
   * Get a {@link InputFile} instance to read bytes from the file at the given path, with a known
   * file length.
   */
  default InputFile newInputFile(String path, long length, String keyMetadata) {
    throws UnsupportedOperationException(can't use without encrypting file io)
  }

Then the question would be if you are in EncryptingFileIO should the implementation fail if keyMetadata is null? Because the library user has expressly used a method which implies keyMetadata is important, so is it OK to just fall back to non-ecryption behavior? I don't feel comfortable with that but others may have a different opinion.

@gaborkaszab gaborkaszab Sep 19, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think having a length param could make sense as you described above to be consistent with other function and to have a length available in the "fallback to unencrypted read" case.
My understanding might not be that strong here, but I think for manifests list files we are in a weird situation because we don't directly keep the length of the physical encrypted file. We do keep the location and keyId on the snapshot, and in turn we can get the keyMetadata based on the keyId. When it comes to reading we assume that the length is integrated into keyMetadata. So for manifest list files I don't think there is a straightforward way to use the proposed newInputFile(path, length, keyMetadata or keyId).

Just a general comment, that with the proposed new function, we could technically wipe out all the variations of newInputFile(DataFile), newInputFile(DeleteFile), newInputFile(ManifestFile) because all they need is a path, a length and keyMetadata. Unfortunately, we keep no length for manifest list files as described above.
Should we proposed keeping the length in Snapshot for the new V4 root manifest since we are actively working on it and then we can avoid relying on length being embedded into keyMetadata and could use length as a param for newInputFile? cc @amogh-jahagirdar @stevenzwu

On the question of should we fail if keyMetadata is null or fallback to the unencrypted path, I think the latter is cleaner on the caller side. See my example on the other comment.

ByteBuffer keyMetadata = em.decryptKeyMetadata(keyId);
return newDecryptingInputFile(location, keyMetadata);
} else {
return newInputFile(location);
}
}

public InputFile newDecryptingInputFile(String path, ByteBuffer buffer) {
return em.decrypt(wrap(io.newInputFile(path), buffer));
}
Expand Down
12 changes: 12 additions & 0 deletions api/src/main/java/org/apache/iceberg/encryption/EncryptionManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
package org.apache.iceberg.encryption;

import java.io.Serializable;
import java.nio.ByteBuffer;
import org.apache.iceberg.io.InputFile;
import org.apache.iceberg.io.OutputFile;
import org.apache.iceberg.relocated.com.google.common.collect.Iterables;
Expand Down Expand Up @@ -51,6 +52,17 @@ default Iterable<InputFile> decrypt(Iterable<EncryptedInputFile> encrypted) {
return Iterables.transform(encrypted, this::decrypt);
}

/**
* Decrypt an encrypted key metadata referred by a key id.
*
* @param keyId the encryption key ID
* @return the decrypted key metadata buffer
*/
default ByteBuffer decryptKeyMetadata(String keyId) {
throw new UnsupportedOperationException(
this.getClass().getName() + " does not support key metadata decryption");
}

/**
* Given a handle on an {@link OutputFile} that writes raw bytes to the underlying file system,
* return a bundle of an {@link EncryptedOutputFile#encryptingOutputFile()} that writes encrypted
Expand Down
12 changes: 12 additions & 0 deletions api/src/main/java/org/apache/iceberg/io/FileIO.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,11 @@ default InputFile newInputFile(ManifestFile manifest) {
return newInputFile(manifest.path(), manifest.length());
}

/**
* @deprecated since 1.12.0. Will be removed in 2.0.0; use {@link #newInputFile(String, String)}
* instead.
*/
@Deprecated
default InputFile newInputFile(ManifestListFile manifestList) {

@gaborkaszab gaborkaszab Aug 7, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dropped BaseManifestListFile because that was package private. Now, there is no implementation in the library that can call this function, still I don't think we can drop this, because it'd break API for users that happen to implement their own ManifestListFile. Not likely, but technically feasible.

Preconditions.checkArgument(
manifestList.encryptionKeyID() == null,
Expand All @@ -80,6 +85,13 @@ default InputFile newInputFile(ManifestListFile manifestList) {
return newInputFile(manifestList.location());
}

default InputFile newInputFile(String location, String keyId) {

@RussellSpitzer RussellSpitzer Sep 18, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While this made sense for "manifest List" i'm not sure it makes sense for a generic path. The caller of this method has control over whether or not they are passing through a String. If they choose to pass through a string shouldn't we be using it?

Do we have other examples of API's where an argument can be ignored if it is null and fall back to another polymorphism?

@gaborkaszab gaborkaszab Sep 19, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the general case what I have in mind is that we don't want the callers to branch on whether keyId is null or not. It's a cleaner implementation to call newInputFile(location, keyId) unconditionally for files that might be encrypted with the key-id based mechanism and then internally we can decide which path to go.
One such implementation would look like this:

StatisticsFile statFile = // get the stats file from table metadata
EncryptingFileIO io = EncryptingFileIO.combine(table.io(), table.encryption());
InputFile inputFile = io.newInputFile(statFile.path(), statFile.keyId());

This way the caller code is clean enough, no need to branch on keyId being null, we can fallback to the unencrypted case inside newInputFile.
This is what BaseSnapshot.cacheManifests() -> ManifestLists.newInputFile() -> io.newInputFile()` path does now.

About other such APIs, I'm not sure about other APIs, but the keyMetadata based ones seem to do the same here, and for me this seems to give a nice flexibility: even if we have an EncryptionFileIO there might be unencrypted files without keyMetadata or keyId and then we can silently fallback to the unencrypted case without making the caller to make this decision.

Preconditions.checkArgument(
keyId == null, "Cannot decrypt file: %s (use EncryptingFileIO)", location);
// cannot pass length because it is not tracked outside of key metadata
return newInputFile(location);
}

/** Get a {@link OutputFile} instance to write bytes to the file at the given path. */
OutputFile newOutputFile(String path);

Expand Down
27 changes: 17 additions & 10 deletions core/src/main/java/org/apache/iceberg/AllManifestsTable.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,8 @@ protected CloseableIterable<FileScanTask> doPlanFiles() {
io,
schema(),
specs,
new BaseManifestListFile(snap.manifestListLocation(), snap.keyId()),
snap.manifestListLocation(),
snap.keyId(),
residual,
snap.snapshotId());
} else {
Expand All @@ -165,7 +166,8 @@ static class ManifestListReadTask implements DataTask {
private final FileIO io;
private final Schema schema;
private final Map<Integer, PartitionSpec> specs;
private final ManifestListFile manifestList;
private final String location;
private final String encryptionKeyId;
private final Expression residual;
private final long referenceSnapshotId;
private DataFile lazyDataFile = null;
Expand All @@ -175,14 +177,16 @@ static class ManifestListReadTask implements DataTask {
FileIO io,
Schema schema,
Map<Integer, PartitionSpec> specs,
ManifestListFile manifestList,
String location,
String encryptionKeyId,
Expression residual,
long referenceSnapshotId) {
this.dataTableSchema = dataTableSchema;
this.io = io;
this.schema = schema;
this.specs = specs;
this.manifestList = manifestList;
this.location = location;
this.encryptionKeyId = encryptionKeyId;
this.residual = residual;
this.referenceSnapshotId = referenceSnapshotId;
}
Expand All @@ -195,7 +199,7 @@ public List<DeleteFile> deletes() {
@Override
public CloseableIterable<StructLike> rows() {
try (CloseableIterable<ManifestFile> manifests =
InternalData.read(FileFormat.AVRO, io.newInputFile(manifestList))
InternalData.read(FileFormat.AVRO, io.newInputFile(location, encryptionKeyId))
.setRootType(GenericManifestFile.class)
.setCustomType(
ManifestFile.PARTITION_SUMMARIES_ELEMENT_ID, GenericPartitionFieldSummary.class)
Expand All @@ -213,8 +217,7 @@ public CloseableIterable<StructLike> rows() {
return CloseableIterable.transform(rowIterable, projection::wrap);

} catch (IOException e) {
throw new RuntimeIOException(
e, "Cannot read manifest list file: %s", manifestList.location());
throw new RuntimeIOException(e, "Cannot read manifest list file: %s", location);
}
}

Expand All @@ -223,7 +226,7 @@ public DataFile file() {
if (lazyDataFile == null) {
this.lazyDataFile =
DataFiles.builder(PartitionSpec.unpartitioned())
.withInputFile(io.newInputFile(manifestList))
.withInputFile(io.newInputFile(location, encryptionKeyId))
.withRecordCount(1)
.withFormat(FileFormat.AVRO)
.build();
Expand Down Expand Up @@ -276,8 +279,12 @@ Map<Integer, PartitionSpec> specsById() {
return specs;
}

ManifestListFile manifestList() {
return manifestList;
String location() {
return location;
}

String encryptionKeyId() {
return encryptionKeyId;
}

long referenceSnapshotId() {
Expand Down
9 changes: 5 additions & 4 deletions core/src/main/java/org/apache/iceberg/AllManifestsTableTaskParser.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,9 +64,9 @@ static void toJson(AllManifestsTable.ManifestListReadTask task, JsonGenerator ge

generator.writeEndArray();

generator.writeStringField(MANIFEST_LIST_LOCATION, task.manifestList().location());
if (task.manifestList().encryptionKeyID() != null) {
generator.writeStringField(MANIFEST_LIST_KEY_ID, task.manifestList().encryptionKeyID());
generator.writeStringField(MANIFEST_LIST_LOCATION, task.location());
if (task.encryptionKeyId() != null) {
generator.writeStringField(MANIFEST_LIST_KEY_ID, task.encryptionKeyId());
}

generator.writeFieldName(RESIDUAL);
Expand Down Expand Up @@ -105,7 +105,8 @@ static AllManifestsTable.ManifestListReadTask fromJson(JsonNode jsonNode) {
fileIO,
schema,
specsById,
new BaseManifestListFile(manifestListLocation, manifestListKeyId),
manifestListLocation,
manifestListKeyId,
residualFilter,
referenceSnapshotId);
}
Expand Down
49 changes: 0 additions & 49 deletions core/src/main/java/org/apache/iceberg/BaseManifestListFile.java
View file
Open in desktop

This file was deleted.

4 changes: 1 addition & 3 deletions core/src/main/java/org/apache/iceberg/BaseSnapshot.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,9 +183,7 @@ private void cacheManifests(FileIO fileIO) {
if (allManifests == null) {
// if manifests isn't set, then the snapshotFile is set and should be read to get the list
this.allManifests =
ManifestLists.read(
ManifestLists.newInputFile(
fileIO, new BaseManifestListFile(manifestListLocation, keyId)));
ManifestLists.read(ManifestLists.newInputFile(fileIO, manifestListLocation, keyId));
}

if (dataManifests == null || deleteManifests == null) {
Expand Down
4 changes: 2 additions & 2 deletions core/src/main/java/org/apache/iceberg/ManifestLists.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,8 @@
class ManifestLists {
private ManifestLists() {}

static InputFile newInputFile(FileIO io, ManifestListFile manifestList) {
InputFile input = io.newInputFile(manifestList);
static InputFile newInputFile(FileIO io, String location, String keyId) {
InputFile input = io.newInputFile(location, keyId);
if (ManifestFiles.cachingEnabled(io)) {
return ManifestFiles.contentCache(io).tryCache(input);
}
Expand Down
37 changes: 25 additions & 12 deletions core/src/main/java/org/apache/iceberg/encryption/EncryptionUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,19 +140,32 @@ public static ByteBuffer setFileLength(ByteBuffer keyMetadata, long fileLength)
* @param manifestList a ManifestListFile
* @param em the table's EncryptionManager
* @return a decrypted key metadata buffer
* @deprecated since 1.12.0. Will be removed in 2.0.0; use {@link
* EncryptionManager#decryptKeyMetadata(String)} instead.
*/
@Deprecated
public static ByteBuffer decryptManifestListKeyMetadata(
ManifestListFile manifestList, EncryptionManager em) {
return decryptKeyMetadata(manifestList.encryptionKeyID(), em);
}

/**
* Decrypt the key metadata referred by key ID using an encryption manager.
*
* @param encryptionKeyId the key ID of the encrypted key metadata
* @param em the table's EncryptionManager
* @return a decrypted key metadata buffer
*/
static ByteBuffer decryptKeyMetadata(String encryptionKeyId, EncryptionManager em) {
Preconditions.checkState(
em instanceof StandardEncryptionManager,
"Snapshot key metadata encryption requires a StandardEncryptionManager");
"Key metadata decryption requires a StandardEncryptionManager");
StandardEncryptionManager sem = (StandardEncryptionManager) em;
String manifestListKeyId = manifestList.encryptionKeyID();
Map<String, EncryptedKey> encryptionKeys = sem.encryptionKeys();
EncryptedKey manifestListKey = encryptionKeys.get(manifestListKeyId);
ByteBuffer encryptedKeyMetadata = manifestListKey.encryptedKeyMetadata();
String keyEncryptionKeyID = manifestListKey.encryptedById();
ByteBuffer keyEncryptionKey = sem.encryptedByKey(manifestListKeyId);
EncryptedKey encryptionKey = encryptionKeys.get(encryptionKeyId);
ByteBuffer encryptedKeyMetadata = encryptionKey.encryptedKeyMetadata();
String keyEncryptionKeyID = encryptionKey.encryptedById();
ByteBuffer keyEncryptionKey = sem.encryptedByKey(encryptionKeyId);
String keyEncryptionKeyTimestamp =
encryptionKeys
.get(keyEncryptionKeyID)
Expand Down Expand Up @@ -182,22 +195,22 @@ public static Map<String, EncryptedKey> encryptionKeys(EncryptionManager em) {
}

/**
* Encrypts the key metadata for a manifest list.
* Encrypts an encryption key metadata.
*
* @param key key encryption key bytes
* @param keyTimestamp timestamp of the key encryption key
* @param mlkMetadata manifest list key metadata
* @param keyMetadata key metadata
* @return encrypted key metadata
*/
static ByteBuffer encryptManifestListKeyMetadata(
ByteBuffer key, String keyTimestamp, EncryptionKeyMetadata mlkMetadata) {
static ByteBuffer encryptKeyMetadata(
ByteBuffer key, String keyTimestamp, EncryptionKeyMetadata keyMetadata) {
Ciphers.AesGcmEncryptor encryptor = new Ciphers.AesGcmEncryptor(ByteBuffers.toByteArray(key));
byte[] mlkMetadataBytes = ByteBuffers.toByteArray(mlkMetadata.buffer());
byte[] keyMetadataBytes = ByteBuffers.toByteArray(keyMetadata.buffer());

// Use key encryption key timestamp as AES GCM signature (AAD) of encryption - in order to
// prevent timestamp tampering attacks
byte[] encryptedKeyMetadata =
encryptor.encrypt(mlkMetadataBytes, keyTimestamp.getBytes(StandardCharsets.UTF_8));
encryptor.encrypt(keyMetadataBytes, keyTimestamp.getBytes(StandardCharsets.UTF_8));

return ByteBuffer.wrap(encryptedKeyMetadata);
}
Expand Down
17 changes: 10 additions & 7 deletions core/src/main/java/org/apache/iceberg/encryption/StandardEncryptionManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,11 @@ public Iterable<InputFile> decrypt(Iterable<EncryptedInputFile> encrypted) {
return Iterables.transform(encrypted, this::decrypt);
}

@Override
public ByteBuffer decryptKeyMetadata(String keyId) {
return EncryptionUtil.decryptKeyMetadata(keyId, this);
}

private LoadingCache<String, ByteBuffer> unwrappedKeyCache() {
if (this.unwrappedKeyCache == null) {
this.unwrappedKeyCache =
Expand Down Expand Up @@ -168,18 +173,16 @@ private long currentTimeMillis() {
return System.currentTimeMillis() + testTimeShift;
}

ByteBuffer encryptedByKey(String manifestListKeyID) {
EncryptedKey encryptedKeyMetadata = encryptionKeys.get(manifestListKeyID);
ByteBuffer encryptedByKey(String keyId) {
EncryptedKey encryptedKeyMetadata = encryptionKeys.get(keyId);

Preconditions.checkState(
encryptedKeyMetadata != null,
"Cannot find manifest list key metadata with id %s",
manifestListKeyID);
encryptedKeyMetadata != null, "Cannot find manifest list key metadata with id %s", keyId);

Preconditions.checkArgument(
!encryptedKeyMetadata.encryptedById().equals(tableKeyId),
"%s is a key encryption key, not manifest list key metadata",
manifestListKeyID);
keyId);

return unwrappedKeyCache().get(encryptedKeyMetadata.encryptedById());
}
Expand Down Expand Up @@ -207,7 +210,7 @@ public FileEncryptionKeys registerKeyMetadata(NativeEncryptionKeyMetadata keyMet
EncryptedKey keyEncryptionKey = encryptionKeys.get(keyEncryptionKeyID);
String keyEncryptionKeyTimestamp = keyEncryptionKey.properties().get(KEY_TIMESTAMP);
ByteBuffer encryptedKeyMetadata =
EncryptionUtil.encryptManifestListKeyMetadata(
EncryptionUtil.encryptKeyMetadata(
unwrappedKeyCache().get(keyEncryptionKeyID), keyEncryptionKeyTimestamp, keyMetadata);
BaseEncryptedKey key =
new BaseEncryptedKey(fileKeyID, encryptedKeyMetadata, keyEncryptionKeyID, null);
Expand Down
Loading
Loading