Core, Data: File Format API interfaces

[pvary]

The interface part of the changes from #12298

Interfaces which have to be implemented by the File Formats:

• ReadBuilder - Builder for reading data from data files
• AppenderBuilder - Builder for writing data to data files
• ObjectModel - Providing ReadBuilders, and AppenderBuilders for the specific data file format and object model pair

Interfaces which are used by the actual readers/writers:

• AppenderBuilder - Builder for writing a file
• DataWriterBuilder - Builder for generating a data file
• PositionDeleteWriterBuilder - Builder for generating a position delete file
• EqualityDeleteWriterBuilder - Builder for generating an equality delete file
• No ReadBuilder here - the file format reader builder is reused

Implementation classes tying them together:

• WriterBuilder class which implements the AppenderBuilder/DataWriterBuilder/PositionDeleteWriterBuilder/EqualityDeleteWriterBuilder interfaces using the AppenderBuilder provided by the File Format itself
• ObjectModelRegistry which stores the available ObjectModels and users could request the readers (ReadBuilder) and writers (AppenderBuilder/DataWriterBuilder/PositionDeleteWriterBuilder/EqualityDeleteWriterBuilder) from.

[liurenjie1024]

Thanks @pvary for this pr, left some comments, genearlly looks great!

[liurenjie1024]

Thanks @pvary for this pr, LGTM!

[liurenjie1024]

LGTM, just some nits!

[liurenjie1024]

Thanks @pvary !

[stevenzwu]

left some initial comments on the interfaces. will still need to take a look at the other bigger PR to understand more on the work as a whole.

[RussellSpitzer]

@huaxingao, @pvary Could you take a look from a comet prospective? I know you have some custom code that would be using this as well

[pvary]

@huaxingao, @pvary Could you take a look from a comet prospective? I know you have some custom code that would be using this as well

Originally I thought that the comet could be just another FileAccessFactory to register, but based on @rdblue's suggestion I have merged it back to the spark-vectorized/Parquet factory. If the current API is not working for the custom code, that direction could be something which might be worth to explore.

[stevenzwu]

this is the drawback of not having generic parameters.

@rdblue Should we revisit @pvary 's earlier attempt to define the new interfaces with generic types.

To avoid breaking the source compatibility and keep the change scope small, the existing implementations (Avro, Orc, Parquet) would use WriteBuilder<Object, Object>, which means the generic parameters don't bring any materialized benefits.

but at least the new interfaces are properly typed. Who knows. In the future, the implementations may be updated. Also if we add a new file format support in the future, the new implementation can be properly typed.

[liurenjie1024]

I agree with @stevenzwu generic types would be better. There are several benefits here:

1. Better readability.
2. More consistent with other apis such as DataFileWriterBuilder.

In the future, the implementations may be updated. Also if we add a new file format support in the future, the new implementation can be properly typed.

+1 for this. One of the goals of this effort is to make adopting new file formats easier. Generic types would benefit other file formats.

[rdblue]

I'm not sure why this is Object. I thought that the builders would be parameterized by this type. Unlike the Parquet and Avro builders, the DataWriteBuilder and delete builders do not expose the writerFunc method of building a writer tree. Instead, those builders accept the Iceberg schema and engine schema and configure the Avro and Parquet builders by calling writerFunc. So the new builders should have a type parameter for this, but the wrapped format-specific builders should not.

[stevenzwu]

Great that we have consensus that the new WriteBuilder interface can have generic parameters.

The Parquet.WriteBuilder would implement this WriteBuilder interface.

public static class WriteBuilder
      implements InternalData.WriteBuilder, org.apache.iceberg.formats.WriteBuilder<Object, Object> {

Peter's earlier attempt is to use Object for the generic params to avoid breaking the source compatibility for the existing Parquet.WriteBuilder. Don't know if we have agreed on how to move forward for that?

[aihuaxu]

Seems this should return WriteBuilder<S>. If it's Parquet format, then it would be WriteBuilder<MessageType>?

[pvary]

We definitely don't want to expose the internal file format types handled by the FileFormats themselves. We might be able to expose the accepted data types, and the engine specific schemas if we decide to do so. See: #12774 (comment)

Based on the decision there, it could be:

• For Spark: WriteBuilder<InternalRow, SchemaType>, WriteBuilder<InternalRow>, or WriteBuilder
• For Flink: WriteBuilder<RowData, RowType>, WriteBuilder<RowData>, or WriteBuilder
• For Generic: WriteBuilder<Record, Schema>, WriteBuilder<Record>, or WriteBuilder

[pvary]

EncryptedOutputFile and OutputFile are mostly interchangeable—but not everywhere.
The FileWriterFactory API expects an EncryptedOutputFile as input. In FormatModelRegistry, we can obtain an OutputFile using EncryptedOutputFile.encryptingOutputFile(). However, this means we can’t pass the original EncryptedOutputFile down to Parquet.write, which looks like this:

iceberg/parquet/src/main/java/org/apache/iceberg/parquet/Parquet.java

Lines 148 to 157 in 4e68ff0

	public static WriteBuilder write(EncryptedOutputFile file) {
	if (file instanceof NativeEncryptionOutputFile) {
	NativeEncryptionOutputFile nativeFile = (NativeEncryptionOutputFile) file;
	return write(nativeFile.plainOutputFile())
	.withFileEncryptionKey(nativeFile.keyMetadata().encryptionKey())
	.withAADPrefix(nativeFile.keyMetadata().aadPrefix());
	} else {
	return write(file.encryptingOutputFile());
	}
	}

If we pass the result of EncryptedOutputFile.encryptingOutputFile(), the following code path is used:

iceberg/parquet/src/main/java/org/apache/iceberg/parquet/Parquet.java

Lines 140 to 146 in 4e68ff0

	public static WriteBuilder write(OutputFile file) {
	if (file instanceof EncryptedOutputFile) {
	return write((EncryptedOutputFile) file);
	}
	return new WriteBuilder(file);
	}

Since EncryptedOutputFile.encryptingOutputFile() returns a plain OutputFile, the file will not be encrypted.

To make the FormatModel API symmetric, we would need to change:

ReadBuilder<D, S> readBuilder(InputFile inputFile);

to

ReadBuilder<D, S> readBuilder(EncryptedInputFile inputFile);

But that feels unnecessary and doesn’t add real value.

@ggershinsky: What's your take? Will we need to push EncryptedInputFile to the file format readers sometime in the future?

[ggershinsky]

Sure, I'll have a look

[stevenzwu]

I thought the EncryptedOutputFile. encryptingOutputFile () would return an encryption wrapped `OutputFile

iceberg/core/src/main/java/org/apache/iceberg/encryption/StandardEncryptionManager.java

Line 291 in ebc6b66

public OutputFile encryptingOutputFile() {

[pvary]

I followed exactly what @stevenzwu suggested.

Unfortunately, there are some hidden requirements in the Parquet implementation for encryption to work properly.
Parquet encryption was only functioning when the target was a HadoopOutputFile. Using StandardEncryptedOutputFile.encryptingOutputFile() produced an AesGcmOutputFile, which resulted in corrupt files.
Since Parquet only needed the Hadoop Configuration object from HadoopOutputFile, I addressed this by introducing:

• HasConfiguration interface – to expose the Hadoop configuration (similar to HadoopConfigurable).
• HadoopAesGcmOutputFile – which extends AesGcmOutputFile, but also stores the Hadoop configuration.

With these new classes, I made the following changes:

• Implemented HasConfiguration in both HadoopAesGcmOutputFile and HadoopOutputFile.
• Updated the check in the Parquet.WriteBuilder constructor to look for HasConfiguration instead of HadoopOutputFile when extracting the configuration.
• Modified ParquetIO.file to check for HasConfiguration instead of HadoopOutputFile when creating the parquet.HadoopOutputFile. - This one seems a bit risky for me, I would like to hear your thoughts

You can review the changes here

[pvary]

Created a PR with a fix: #14528

[ggershinsky]

There are two types of file encryption in Iceberg,

1. Native Parquet encryption (PME), part of the Parquet spec/impl. Later, it will be leveraged for columnar encryption. Possibly, ORC native encryption will be used too.
2. AES GCM Stream, part of the Iceberg spec, for encrypting the non-columnar Avro format (manifest, manifest list and Avro data files).

For Parquet write, we need to pass it the original plain output file (see NativeEncryptionOutputFile.plainOutputFile()) and the encryption key plus AAD prefix (file ID) - see the NativeEncryptionKeyMetadata methods.
For Parquet read, we need to pass it the original input file (via StandardDecryptedInputFile.encryptedInputFile() that does not activate AES GCM Stream decryption) and the encryption key plus AAD prefix (file ID) - see NativeEncryptionKeyMetadata methods.

Parquet encryption/decryption does not use (depend on) Hadoop config or input/output files.

For AES GCM Stream write, we take the StandardEncryptedOutputFile.encryptingOutputFile() that produces an AesGcmOutputFile.
For AES GCM Stream read, we use the StandardDecryptedInputFile as an InputFile object which produces an AesGcmInputFile upon eg. newStream call in StandardDecryptedInputFile)

[pvary]

@ggershinsky: Could you please help me with unit tests which execute the different encryption/decryption methods, so I can check what is happening?

I suspect that the Parquet.write expects and handles NativeEncryptingOutputFile for writes, as there explicit implements checks in the Parquet.java

StandardEncryptedOutputFile also implements NativeEncryptingOutputFile. I guess this is why it is handled correctly.

[ggershinsky]

@pvary sure, the PME unitests are here; some of the AES GCM Stream unitests can found in #3231, #9436 and #8252

[pvary]

@ggershinsky: The File Format API is designed to handle data and delete file writes. From what I understand, in this case native Parquet encryption is being used, which means the input is a NativeEncryptingOutputFile.

Given that, the File Format API should only accept OutputFile as input—not EncryptingOutputFile—since data and delete files are always natively encrypted.

What I don’t understand is why FileWriterFactory expects EncryptedOutputFile:

public interface FileWriterFactory<T> {
  DataWriter<T> newDataWriter(EncryptedOutputFile file, PartitionSpec spec, StructLike partition);
  EqualityDeleteWriter<T> newEqualityDeleteWriter(EncryptedOutputFile file, PartitionSpec spec, StructLike partition);
  PositionDeleteWriter<T> newPositionDeleteWriter(EncryptedOutputFile file, PartitionSpec spec, StructLike partition);
}

All of these should have either NativeEncryptingOutputFile or OutputFile as an input.

[stevenzwu]

LGTM.

Left a nit comment for a log