Add support for table encryption in Iceberg#28905
Conversation
|
I simply restored #28354. This PR isn't ready for reviews for now. |
ebyhr
force-pushed
the
ebi/iceberg-encryption
branch
from
0ebc277 to
9227282
Compare
|
cc @sopel39 |
|
Can we use ConnectorTableCredentials to carry over these encryption keys? |
Co-Authored-By: kamijin_fanta <kamijin@live.jp>
ebyhr
force-pushed
the
ebi/iceberg-encryption
branch
from
9227282 to
eb017f4
Compare
| } | ||
| } | ||
|
|
||
| private static Map<String, String> fileIoProperties(Table table) |
| @Override | ||
| public Map<String, String> fileIoProperties() | ||
| { | ||
| return fileIo.properties(); |
There was a problem hiding this comment.
Why not io().properties()? Some comment would be useful.
| private static TrinoInputFile newInputFile(TrinoFileSystem fileSystem, DeleteFile deleteFile, Optional<EncryptionManager> encryptionManager) | ||
| { | ||
| if (encryptionManager.isPresent() && deleteFile.keyMetadata() != null) { | ||
| return fileSystem.newInputFile(Location.of(deleteFile.location())); |
There was a problem hiding this comment.
nit: add comment why this branch doesn't have file size in bytes
| DeleteFile deleteFile, | ||
| Optional<EncryptionManager> encryptionManager) | ||
| { | ||
| if (encryptionManager.isEmpty() || deleteFile.keyMetadata() == null) { |
There was a problem hiding this comment.
better to throw if key medata is not null, but encryption manager is empty. Otherwise, weird serde errors might occur
|
|
||
| OptionalLong rowPositionLowerBound = lowerBoundPosition == null ? | ||
| OptionalLong.empty() : OptionalLong.of(Conversions.fromByteBuffer(DELETE_FILE_POS.type(), lowerBoundPosition)); | ||
|
|
| { | ||
| return INSTANCE_SIZE | ||
| + estimatedSizeOf(path) | ||
| + encryptionKeyMetadata.map(value -> sizeOf(value)).orElse(0L) |
There was a problem hiding this comment.
optional still takes space, should use io.airlift.slice.SizeOf#OPTIONAL_INSTANCE_SIZE
| writeDeletes(rowsToDelete); | ||
| writer.commit(); | ||
|
|
||
| long fileSizeInBytes = writer.getWrittenBytes(); |
There was a problem hiding this comment.
writer might be IcebergParquetFileWriter. However Trino native parquet writer doesn't support encryption. Should we skip encrypted insert path for now?
| import static io.trino.spi.StandardErrorCode.NOT_SUPPORTED; | ||
| import static java.util.Objects.requireNonNull; | ||
|
|
||
| public class IcebergEncryptionManagerFactory |
There was a problem hiding this comment.
Let's make it an interface. It will be easier to adopt in different environments.
| return catalogKmsClient; | ||
| } | ||
| Map<String, String> properties = new HashMap<>(kmsProperties); | ||
| // Iceberg 1.10.x does not support encryption.kms-type yet, so set the KMS impl explicitly. |
| import static java.util.Objects.checkFromIndexSize; | ||
| import static java.util.Objects.requireNonNull; | ||
|
|
||
| public class EncryptedTrinoInput |
There was a problem hiding this comment.
This is only required for non-parquet encrypted files, right?
| .orElse(toCompressionCodec(hiveCompressionOption)); | ||
|
|
||
| return new IcebergParquetFileWriter( | ||
| IcebergFileWriter writer = new IcebergParquetFileWriter( |
There was a problem hiding this comment.
native Parquet writer doesn't support encryption.
|
Superseded by #28389 |
4 participants
Description
Fixes #28204
Release notes