diff --git a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergFileWriterFactory.java b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergFileWriterFactory.java index 332b3117ecdc..68b23b38685b 100644 --- a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergFileWriterFactory.java +++ b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergFileWriterFactory.java @@ -87,14 +87,12 @@ import static io.trino.spi.StandardErrorCode.NOT_SUPPORTED; import static java.lang.String.format; import static java.util.Objects.requireNonNull; -import static org.apache.iceberg.TableProperties.DEFAULT_WRITE_METRICS_MODE; import static org.apache.iceberg.io.DeleteSchemaUtil.pathPosSchema; import static org.apache.iceberg.parquet.ParquetSchemaUtil.convert; public class IcebergFileWriterFactory { private static final Schema POSITION_DELETE_SCHEMA = pathPosSchema(); - private static final MetricsConfig FULL_METRICS_CONFIG = MetricsConfig.fromProperties(ImmutableMap.of(DEFAULT_WRITE_METRICS_MODE, "full")); private static final Splitter COLUMN_NAMES_SPLITTER = Splitter.on(',').trimResults().omitEmptyStrings(); private final TypeManager typeManager; @@ -151,8 +149,8 @@ public IcebergFileWriter createPositionDeleteWriter( Map storageProperties) { return switch (fileFormat) { - case PARQUET -> createParquetWriter(FULL_METRICS_CONFIG, fileSystem, outputPath, POSITION_DELETE_SCHEMA, session, storageProperties); - case ORC -> createOrcWriter(FULL_METRICS_CONFIG, fileSystem, outputPath, POSITION_DELETE_SCHEMA, session, storageProperties, DataSize.ofBytes(Integer.MAX_VALUE)); + case PARQUET -> createParquetWriter(MetricsConfig.forPositionDelete(), fileSystem, outputPath, POSITION_DELETE_SCHEMA, session, storageProperties); + case ORC -> createOrcWriter(MetricsConfig.forPositionDelete(), fileSystem, outputPath, POSITION_DELETE_SCHEMA, session, storageProperties, DataSize.ofBytes(Integer.MAX_VALUE)); case AVRO -> createAvroWriter(fileSystem, outputPath, POSITION_DELETE_SCHEMA, storageProperties); }; } diff --git a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergMetadata.java b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergMetadata.java index fe94a28f4dcd..001adf64ef01 100644 --- a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergMetadata.java +++ b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergMetadata.java @@ -3632,7 +3632,7 @@ private void finishWrite(ConnectorSession session, IcebergTableHandle table, Col switch (task.content()) { case DATA -> dataTasks.add(task); case POSITION_DELETES -> deleteTasks.add(task); - case EQUALITY_DELETES -> throw new UnsupportedOperationException("Unsupported task content: " + task.content()); + case EQUALITY_DELETES, DATA_MANIFEST, DELETE_MANIFEST -> throw new UnsupportedOperationException("Unsupported task content: " + task.content()); } } diff --git a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergSplitSource.java b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergSplitSource.java index d5e50213e495..24329350a1e0 100644 --- a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergSplitSource.java +++ b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergSplitSource.java @@ -361,7 +361,7 @@ private synchronized Iterator prepareFileTasksIterator(L } yield isUnconstrainedPathAndTimeDomain(); } - case DATA -> throw new IllegalStateException("Unexpected delete file: " + deleteFile); + case DATA, DATA_MANIFEST, DELETE_MANIFEST -> throw new IllegalStateException("Unexpected delete file: " + deleteFile); }) .collect(toImmutableList()); scannedFiles.add(new DataFileWithDeleteFiles(wholeFileTask.file(), fullyAppliedDeletes)); diff --git a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/rest/TrinoIcebergRestCatalogFactory.java b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/rest/TrinoIcebergRestCatalogFactory.java index 321597effc74..eaedc9eae6f9 100644 --- a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/rest/TrinoIcebergRestCatalogFactory.java +++ b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/rest/TrinoIcebergRestCatalogFactory.java @@ -35,6 +35,7 @@ import org.apache.iceberg.catalog.Namespace; import org.apache.iceberg.catalog.TableIdentifier; import org.apache.iceberg.rest.HTTPClient; +import org.apache.iceberg.rest.RESTCatalogProperties; import org.apache.iceberg.rest.RESTSessionCatalog; import org.apache.iceberg.rest.RESTUtil; @@ -129,7 +130,7 @@ public synchronized TrinoCatalog create(ConnectorIdentity identity) properties.put(CatalogProperties.URI, serverUri.toString()); warehouse.ifPresent(location -> properties.put(CatalogProperties.WAREHOUSE_LOCATION, location)); prefix.ifPresent(prefix -> properties.put("prefix", prefix)); - properties.put("view-endpoints-supported", Boolean.toString(viewEndpointsEnabled)); + properties.put(RESTCatalogProperties.VIEW_ENDPOINTS_SUPPORTED, Boolean.toString(viewEndpointsEnabled)); properties.put("trino-version", trinoVersion); properties.put(AUTH_SESSION_TIMEOUT_MS, String.valueOf(sessionTimeout.toMillis())); connectionTimeout.ifPresent(duration -> properties.put("rest.client.connection-timeout-ms", String.valueOf(duration.toMillis()))); @@ -148,8 +149,13 @@ public synchronized TrinoCatalog create(ConnectorIdentity identity) (context, config) -> { ConnectorIdentity currentIdentity = (context.wrappedIdentity() != null) ? ((ConnectorIdentity) context.wrappedIdentity()) - : ConnectorIdentity.ofUser("fake"); - return fileIoFactory.create(fileSystemFactory.create(currentIdentity, config), true, config); + : ConnectorIdentity.ofUser("trino"); + return fileIoFactory.create( + fileSystemFactory.create(currentIdentity, config), + true, + config, + fileSystemFactory, + currentIdentity); }); icebergCatalogInstance.initialize(catalogName.toString(), properties.buildOrThrow()); diff --git a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/delete/DeleteManager.java b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/delete/DeleteManager.java index 3fcc2d94d12a..7823f3a60865 100644 --- a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/delete/DeleteManager.java +++ b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/delete/DeleteManager.java @@ -85,7 +85,7 @@ public Optional getDeletePredicate( } } case EQUALITY_DELETES -> equalityDeleteFiles.add(deleteFile); - case DATA -> throw new VerifyException("DATA is not delete file type"); + case DATA, DATA_MANIFEST, DELETE_MANIFEST -> throw new VerifyException("DATA is not delete file type"); } } diff --git a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/fileio/ForwardingFileIo.java b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/fileio/ForwardingFileIo.java index 81e58b6b9c2d..3789d5ef894f 100644 --- a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/fileio/ForwardingFileIo.java +++ b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/fileio/ForwardingFileIo.java @@ -14,22 +14,30 @@ package io.trino.plugin.iceberg.fileio; import com.google.common.annotations.VisibleForTesting; +import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.Iterables; import com.google.common.collect.Streams; import io.trino.filesystem.Location; import io.trino.filesystem.TrinoFileSystem; +import io.trino.plugin.iceberg.IcebergFileSystemFactory; import io.trino.spi.TrinoException; +import io.trino.spi.security.ConnectorIdentity; import org.apache.iceberg.DataFile; import org.apache.iceberg.DeleteFile; import org.apache.iceberg.ManifestFile; +import org.apache.iceberg.ManifestListFile; import org.apache.iceberg.io.BulkDeletionFailureException; import org.apache.iceberg.io.InputFile; import org.apache.iceberg.io.OutputFile; +import org.apache.iceberg.io.StorageCredential; import org.apache.iceberg.io.SupportsBulkOperations; +import org.apache.iceberg.io.SupportsStorageCredentials; import java.io.IOException; import java.io.UncheckedIOException; +import java.util.ArrayList; +import java.util.IdentityHashMap; import java.util.List; import java.util.Map; import java.util.concurrent.Callable; @@ -45,7 +53,7 @@ import static java.util.stream.Collectors.joining; public class ForwardingFileIo - implements SupportsBulkOperations + implements SupportsBulkOperations, SupportsStorageCredentials { private static final int DELETE_BATCH_SIZE = 1000; private static final int BATCH_DELETE_PATHS_MESSAGE_LIMIT = 5; @@ -54,6 +62,11 @@ public class ForwardingFileIo private final Map properties; private final boolean useFileSizeFromMetadata; private final ExecutorService deleteExecutor; + private final IcebergFileSystemFactory storageCredentialFileSystemFactory; + private final ConnectorIdentity storageCredentialIdentity; + + private volatile List storageCredentials = ImmutableList.of(); + private volatile Map prefixedFileSystems = ImmutableMap.of(); @VisibleForTesting public ForwardingFileIo(TrinoFileSystem fileSystem, boolean useFileSizeFromMetadata) @@ -62,22 +75,36 @@ public ForwardingFileIo(TrinoFileSystem fileSystem, boolean useFileSizeFromMetad } public ForwardingFileIo(TrinoFileSystem fileSystem, Map properties, boolean useFileSizeFromMetadata, ExecutorService deleteExecutor) + { + this(fileSystem, properties, useFileSizeFromMetadata, deleteExecutor, null, null); + } + + public ForwardingFileIo( + TrinoFileSystem fileSystem, + Map properties, + boolean useFileSizeFromMetadata, + ExecutorService deleteExecutor, + IcebergFileSystemFactory storageCredentialFileSystemFactory, + ConnectorIdentity storageCredentialIdentity) { this.fileSystem = requireNonNull(fileSystem, "fileSystem is null"); this.deleteExecutor = requireNonNull(deleteExecutor, "executorService is null"); this.properties = ImmutableMap.copyOf(requireNonNull(properties, "properties is null")); this.useFileSizeFromMetadata = useFileSizeFromMetadata; + this.storageCredentialFileSystemFactory = storageCredentialFileSystemFactory; + this.storageCredentialIdentity = storageCredentialIdentity; } @Override public InputFile newInputFile(String path) { - return new ForwardingInputFile(fileSystem.newInputFile(Location.of(path))); + return new ForwardingInputFile(fileSystemForPath(path).newInputFile(Location.of(path))); } @Override public InputFile newInputFile(String path, long length) { + TrinoFileSystem fileSystem = fileSystemForPath(path); if (!useFileSizeFromMetadata) { return new ForwardingInputFile(fileSystem.newInputFile(Location.of(path))); } @@ -88,14 +115,14 @@ public InputFile newInputFile(String path, long length) @Override public OutputFile newOutputFile(String path) { - return new ForwardingOutputFile(fileSystem, Location.of(path)); + return new ForwardingOutputFile(fileSystemForPath(path), Location.of(path)); } @Override public void deleteFile(String path) { try { - fileSystem.deleteFile(Location.of(path)); + fileSystemForPath(path).deleteFile(Location.of(path)); } catch (IOException e) { throw new UncheckedIOException("Failed to delete file: " + path, e); @@ -149,10 +176,24 @@ public InputFile newInputFile(DeleteFile file) return SupportsBulkOperations.super.newInputFile(file); } + @Override + public InputFile newInputFile(ManifestListFile manifestList) + { + return SupportsBulkOperations.super.newInputFile(manifestList); + } + private void deleteBatch(List filesToDelete) { try { - fileSystem.deleteFiles(filesToDelete.stream().map(Location::of).toList()); + Map> locationsByFileSystem = new IdentityHashMap<>(); + for (String path : filesToDelete) { + TrinoFileSystem fileSystem = fileSystemForPath(path); + locationsByFileSystem.computeIfAbsent(fileSystem, ignored -> new ArrayList<>()) + .add(Location.of(path)); + } + for (Map.Entry> entry : locationsByFileSystem.entrySet()) { + entry.getKey().deleteFiles(entry.getValue()); + } } catch (IOException e) { throw new UncheckedIOException( @@ -172,6 +213,51 @@ public Map properties() return properties; } + @Override + public void setCredentials(List credentials) + { + storageCredentials = ImmutableList.copyOf(requireNonNull(credentials, "credentials is null")); + rebuildPrefixedFileSystems(); + } + + @Override + public List credentials() + { + return storageCredentials; + } + + private TrinoFileSystem fileSystemForPath(String path) + { + TrinoFileSystem matchingFileSystem = fileSystem; + int matchingPrefixLength = -1; + for (Map.Entry prefixedFileSystem : prefixedFileSystems.entrySet()) { + String prefix = prefixedFileSystem.getKey(); + if (path.startsWith(prefix) && prefix.length() > matchingPrefixLength) { + matchingPrefixLength = prefix.length(); + matchingFileSystem = prefixedFileSystem.getValue(); + } + } + return matchingFileSystem; + } + + private void rebuildPrefixedFileSystems() + { + if (storageCredentials.isEmpty() || storageCredentialFileSystemFactory == null || storageCredentialIdentity == null) { + prefixedFileSystems = ImmutableMap.of(); + return; + } + + ImmutableMap.Builder rebuiltFileSystems = ImmutableMap.builder(); + for (StorageCredential storageCredential : storageCredentials) { + Map mergedProperties = ImmutableMap.builder() + .putAll(properties) + .putAll(storageCredential.config()) + .buildKeepingLast(); + rebuiltFileSystems.put(storageCredential.prefix(), storageCredentialFileSystemFactory.create(storageCredentialIdentity, mergedProperties)); + } + prefixedFileSystems = rebuiltFileSystems.buildKeepingLast(); + } + @Override public void initialize(Map properties) { diff --git a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/fileio/ForwardingFileIoFactory.java b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/fileio/ForwardingFileIoFactory.java index 5d19b479c84a..7ba2fbd51dfd 100644 --- a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/fileio/ForwardingFileIoFactory.java +++ b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/fileio/ForwardingFileIoFactory.java @@ -17,6 +17,8 @@ import com.google.inject.Inject; import io.trino.filesystem.TrinoFileSystem; import io.trino.plugin.iceberg.ForIcebergFileDelete; +import io.trino.plugin.iceberg.IcebergFileSystemFactory; +import io.trino.spi.security.ConnectorIdentity; import org.apache.iceberg.io.FileIO; import java.util.Map; @@ -48,4 +50,20 @@ public FileIO create(TrinoFileSystem fileSystem, boolean useFileSizeFromMetadata { return new ForwardingFileIo(fileSystem, properties, useFileSizeFromMetadata, deleteExecutor); } + + public FileIO create( + TrinoFileSystem fileSystem, + boolean useFileSizeFromMetadata, + Map properties, + IcebergFileSystemFactory storageCredentialFileSystemFactory, + ConnectorIdentity storageCredentialIdentity) + { + return new ForwardingFileIo( + fileSystem, + properties, + useFileSizeFromMetadata, + deleteExecutor, + storageCredentialFileSystemFactory, + storageCredentialIdentity); + } } diff --git a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/BaseIcebergConnectorTest.java b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/BaseIcebergConnectorTest.java index d7be3c34682e..16aaf665a3ca 100644 --- a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/BaseIcebergConnectorTest.java +++ b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/BaseIcebergConnectorTest.java @@ -7927,6 +7927,77 @@ public void testPreparedStatementWithParameterizedVersionedTable() assertQueryFails(session, "DESCRIBE OUTPUT my_query", ".* DESCRIBE is not supported if a versioned table uses parameters"); } + @Test + public void testTimeTravelWithFilterOnRenamedColumn() + { + testTimeTravelWithFilterOnRenamedColumn(false); + testTimeTravelWithFilterOnRenamedColumn(true); + } + + private void testTimeTravelWithFilterOnRenamedColumn(boolean partitioned) + { + String partition = partitioned ? "WITH (partitioning = ARRAY['part'])" : ""; + try (TestTable table = newTrinoTable("time_travel_with_filter_on_rename_", "(x int, y int, part int)" + partition)) { + assertUpdate("INSERT INTO " + table.getName() + " VALUES (1, 1, 1), (1, 2, 2), (2, 2, 2)", 3); + assertThat(query("SELECT * FROM " + table.getName())) + .matches("VALUES (1, 1, 1), (1, 2, 2), (2, 2, 2)"); + long firstSnapshotId = getCurrentSnapshotId(table.getName()); + + assertUpdate("ALTER TABLE " + table.getName() + " RENAME COLUMN x TO renamed_x"); + + // generate a new version + assertUpdate("INSERT INTO " + table.getName() + " VALUES (1, 2, 3)", 1); + + assertThat(query("SELECT * FROM " + table.getName() + " FOR VERSION AS OF " + firstSnapshotId + " WHERE x = 1")) + .matches("VALUES (1, 1, 1), (1, 2, 2)"); + } + } + + @Test + public void testTimeTravelWithFilterOnDroppedColumn() + { + testTimeTravelWithFilterOnDroppedColumn(false); + testTimeTravelWithFilterOnDroppedColumn(true); + } + + private void testTimeTravelWithFilterOnDroppedColumn(boolean partitioned) + { + String partition = partitioned ? "WITH (partitioning = ARRAY['part'])" : ""; + try (TestTable table = newTrinoTable("time_travel_with_filter_on_drop_", "(x int, y int, part int)" + partition)) { + assertUpdate("INSERT INTO " + table.getName() + " VALUES (1, 1, 1), (1, 2, 2), (2, 2, 2)", 3); + assertThat(query("SELECT * FROM " + table.getName())) + .matches("VALUES (1, 1, 1), (1, 2, 2), (2, 2, 2)"); + long firstSnapshotId = getCurrentSnapshotId(table.getName()); + + assertUpdate("ALTER TABLE " + table.getName() + " DROP COLUMN x"); + + // generate a new version + assertUpdate("INSERT INTO " + table.getName() + " VALUES (1, 2)", 1); + + assertThat(query("SELECT * FROM " + table.getName() + " FOR VERSION AS OF " + firstSnapshotId + " WHERE x = 1")) + .matches("VALUES (1, 1, 1), (1, 2, 2)"); + } + } + + @Test + public void testTimeTravelWithFilterOnRenamedPartitionColumn() + { + try (TestTable table = newTrinoTable("time_travel_with_filter_on_drop_", "(x int, part1 int, part2 int) WITH (partitioning = ARRAY['part1', 'part2'])")) { + assertUpdate("INSERT INTO " + table.getName() + " VALUES (1, 1, 1), (1, 1, 2), (2, 2, 2)", 3); + assertThat(query("SELECT * FROM " + table.getName())) + .matches("VALUES (1, 1, 1), (1, 1, 2), (2, 2, 2)"); + long firstSnapshotId = getCurrentSnapshotId(table.getName()); + + assertUpdate("ALTER TABLE " + table.getName() + " RENAME COLUMN part1 TO renamed_part"); + + // generate a new version + assertUpdate("INSERT INTO " + table.getName() + " VALUES (1, 1, 3)", 1); + + assertThat(query("SELECT * FROM " + table.getName() + " FOR VERSION AS OF " + firstSnapshotId + " WHERE part1 = 1")) + .matches("VALUES (1, 1, 1), (1, 1, 2)"); + } + } + @Test public void testDeleteRetainsTableHistory() { diff --git a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergV2.java b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergV2.java index c240fd7ea3a6..4cc1973bcedc 100644 --- a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergV2.java +++ b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergV2.java @@ -1754,6 +1754,45 @@ void testAnalyzeNoSnapshot() catalog.dropTable(SESSION, schemaTableName); } + @Test // regression test for https://github.com/trinodb/trino/issues/20511 + void testRequiredField() + { + testRequiredField(true); + testRequiredField(false); + } + + private void testRequiredField(boolean projectionPushdown) + { + Session projectionPushdownEnabled = Session.builder(getSession()) + .setCatalogSessionProperty("iceberg", "projection_pushdown_enabled", Boolean.toString(projectionPushdown)) + .build(); + + String table = "test_required_field" + randomNameSuffix(); + SchemaTableName schemaTableName = new SchemaTableName("tpch", table); + + catalog.newCreateTableTransaction( + SESSION, + schemaTableName, + new Schema( + Types.NestedField.optional(1, "id", Types.IntegerType.get()), + Types.NestedField.optional(2, "struct", Types.StructType.of( + Types.NestedField.required(3, "field", Types.IntegerType.get())))), + PartitionSpec.unpartitioned(), + SortOrder.unsorted(), + Optional.ofNullable(catalog.defaultTableLocation(SESSION, schemaTableName)), + ImmutableMap.of()) + .commitTransaction(); + + assertUpdate("INSERT INTO " + table + " VALUES (1, row(10)), (2, NULL)", 2); + + assertThat(query(projectionPushdownEnabled, "SELECT id FROM " + table + " WHERE struct.field IS NOT NULL")) + .matches("VALUES 1"); + assertThat(query(projectionPushdownEnabled, "SELECT id FROM " + table + " WHERE struct.field IS NULL")) + .matches("VALUES 2"); + + catalog.dropTable(SESSION, schemaTableName); + } + private void testHighlyNestedFieldPartitioningWithTimestampTransform(String partitioning, String partitionDirectoryRegex, Set expectedPartitionDirectories) { String tableName = "test_highly_nested_field_partitioning_with_timestamp_transform_" + randomNameSuffix(); diff --git a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/rest/TestIcebergPolarisCatalogConnectorSmokeTest.java b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/rest/TestIcebergPolarisCatalogConnectorSmokeTest.java index 6257a4b7aa70..7291415f393b 100644 --- a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/rest/TestIcebergPolarisCatalogConnectorSmokeTest.java +++ b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/rest/TestIcebergPolarisCatalogConnectorSmokeTest.java @@ -218,7 +218,7 @@ public void testCreateTableWithTrailingSpaceInLocation() public void testDropTableWithMissingMetadataFile() { assertThatThrownBy(super::testDropTableWithMissingMetadataFile) - .hasMessageMatching(".* Table '.*' does not exist"); + .hasMessageMatching("Failed to load table: (.*)"); } @Test @@ -242,7 +242,7 @@ public void testDropTableWithMissingManifestListFile() public void testDropTableWithNonExistentTableLocation() { assertThatThrownBy(super::testDropTableWithNonExistentTableLocation) - .hasMessageMatching(".* Table '.*' does not exist"); + .hasMessageMatching("Failed to load table: (.*)"); } @Test diff --git a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/rest/TestIcebergRestCatalogNestedNamespaceConnectorSmokeTest.java b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/rest/TestIcebergRestCatalogNestedNamespaceConnectorSmokeTest.java index e98f0e2daea4..61e5a40f13be 100644 --- a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/rest/TestIcebergRestCatalogNestedNamespaceConnectorSmokeTest.java +++ b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/rest/TestIcebergRestCatalogNestedNamespaceConnectorSmokeTest.java @@ -234,7 +234,7 @@ public void testDropTableWithMissingSnapshotFile() assertThatThrownBy(super::testDropTableWithMissingSnapshotFile) .isInstanceOf(QueryFailedException.class) .cause() - .hasMessageContaining("Failed to drop table") + .hasMessageMatching("Failed to open input stream for file: .*avro") .hasNoCause(); } diff --git a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/rest/TestIcebergS3TablesConnectorSmokeTest.java b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/rest/TestIcebergS3TablesConnectorSmokeTest.java index 469ccbf13d8c..9f5401ed5f12 100644 --- a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/rest/TestIcebergS3TablesConnectorSmokeTest.java +++ b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/rest/TestIcebergS3TablesConnectorSmokeTest.java @@ -180,7 +180,7 @@ public void testCreateTableWithTrailingSpaceInLocation() public void testRenameTable() { assertThatThrownBy(super::testRenameTable) - .hasStackTraceContaining("Unable to process: RenameTable endpoint is not supported for Glue Catalog"); + .hasStackTraceContaining("RenameTable endpoint is not supported for Glue Catalog"); } @Test diff --git a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/rest/TestIcebergS3VendingRestCatalogConnectorSmokeTest.java b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/rest/TestIcebergS3VendingRestCatalogConnectorSmokeTest.java index bb8b94e45e47..d3e4443e5c1b 100644 --- a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/rest/TestIcebergS3VendingRestCatalogConnectorSmokeTest.java +++ b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/rest/TestIcebergS3VendingRestCatalogConnectorSmokeTest.java @@ -30,6 +30,7 @@ import io.trino.testing.minio.MinioClient; import org.apache.iceberg.BaseTable; import org.apache.iceberg.CatalogProperties; +import org.apache.iceberg.aws.s3.S3FileIOProperties; import org.apache.iceberg.catalog.SessionCatalog; import org.apache.iceberg.catalog.TableIdentifier; import org.apache.iceberg.rest.RESTSessionCatalog; @@ -46,6 +47,9 @@ import java.io.IOException; import java.net.URI; +import java.net.http.HttpClient; +import java.net.http.HttpRequest; +import java.net.http.HttpResponse; import java.nio.file.Files; import java.nio.file.Path; import java.util.Optional; @@ -311,6 +315,33 @@ public void testDropTableWithNonExistentTableLocation() .hasMessageMatching("Failed to load table: (.*)"); } + @Test + public void testRestFixtureVendsCredentialsViaConfigMap() + throws Exception + { + String tableName = "config_credentials_" + randomNameSuffix(); + assertUpdate("CREATE TABLE " + tableName + " AS SELECT 1 value", 1); + try { + String schemaName = getSession().getSchema().orElseThrow(); + HttpRequest request = HttpRequest.newBuilder() + .uri(URI.create("http://" + restCatalogBackendContainer.getRestCatalogEndpoint() + "/v1/namespaces/" + schemaName + "/tables/" + tableName)) + .GET() + .build(); + + HttpResponse response = HttpClient.newHttpClient().send(request, HttpResponse.BodyHandlers.ofString()); + assertThat(response.statusCode()).isEqualTo(200); + assertThat(response.body()) + .contains("\"config\"") + .contains("\"" + S3FileIOProperties.ACCESS_KEY_ID + "\"") + .contains("\"" + S3FileIOProperties.SECRET_ACCESS_KEY + "\"") + .contains("\"" + S3FileIOProperties.SESSION_TOKEN + "\"") + .doesNotContain("\"storage-credentials\""); + } + finally { + assertUpdate("DROP TABLE " + tableName); + } + } + @Override protected boolean isFileSorted(Location path, String sortColumnName) { diff --git a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/rest/TestIcebergStorageCredentialsRestCatalog.java b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/rest/TestIcebergStorageCredentialsRestCatalog.java new file mode 100644 index 000000000000..57eebce4f630 --- /dev/null +++ b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/rest/TestIcebergStorageCredentialsRestCatalog.java @@ -0,0 +1,91 @@ +/* + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package io.trino.plugin.iceberg.catalog.rest; + +import com.google.common.collect.ImmutableMap; +import io.airlift.http.server.testing.TestingHttpServer; +import io.trino.plugin.iceberg.IcebergQueryRunner; +import io.trino.testing.AbstractTestQueryFramework; +import io.trino.testing.QueryRunner; +import org.apache.iceberg.aws.s3.S3FileIOProperties; +import org.apache.iceberg.rest.DelegatingRestSessionCatalog; +import org.apache.iceberg.rest.credentials.ImmutableCredential; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.TestInstance; + +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.List; +import java.util.Optional; + +import static com.google.common.io.MoreFiles.deleteRecursively; +import static com.google.common.io.RecursiveDeleteOption.ALLOW_INSECURE; +import static io.trino.plugin.iceberg.catalog.rest.RestCatalogTestUtils.backendCatalog; +import static org.assertj.core.api.Assertions.assertThat; +import static org.junit.jupiter.api.TestInstance.Lifecycle.PER_CLASS; + +@TestInstance(PER_CLASS) +public class TestIcebergStorageCredentialsRestCatalog + extends AbstractTestQueryFramework +{ + private Path warehouseLocation; + + @Override + protected QueryRunner createQueryRunner() + throws Exception + { + warehouseLocation = Files.createTempDirectory(null); + closeAfterClass(() -> deleteRecursively(warehouseLocation, ALLOW_INSECURE)); + + DelegatingRestSessionCatalog delegatingCatalog = DelegatingRestSessionCatalog.builder() + .delegate(backendCatalog(warehouseLocation)) + .addAllCredentials(List.of( + ImmutableCredential.builder() + .prefix("file://") + .config(ImmutableMap.of( + S3FileIOProperties.ACCESS_KEY_ID, "test-vended-access-key", + S3FileIOProperties.SECRET_ACCESS_KEY, "test-vended-secret-key", + S3FileIOProperties.SESSION_TOKEN, "test-vended-session-token")) + .build())) + .build(); + + TestingHttpServer testServer = delegatingCatalog.testServer(); + testServer.start(); + closeAfterClass(testServer::stop); + + return IcebergQueryRunner.builder() + .setBaseDataDir(Optional.of(warehouseLocation)) + .setIcebergProperties( + ImmutableMap.builder() + .put("iceberg.catalog.type", "rest") + .put("iceberg.rest-catalog.uri", testServer.getBaseUrl().toString()) + .put("iceberg.rest-catalog.vended-credentials-enabled", "true") + .buildOrThrow()) + .build(); + } + + @Test + public void testCreateAndReadTableWithStorageCredentials() + { + assertUpdate("CREATE SCHEMA test_storage_creds"); + assertUpdate("CREATE TABLE test_storage_creds.test_table (id INTEGER, name VARCHAR)"); + assertUpdate("INSERT INTO test_storage_creds.test_table VALUES (1, 'alice'), (2, 'bob')", 2); + + assertThat(query("SELECT * FROM test_storage_creds.test_table")) + .matches("VALUES (1, VARCHAR 'alice'), (2, VARCHAR 'bob')"); + + assertUpdate("DROP TABLE test_storage_creds.test_table"); + assertUpdate("DROP SCHEMA test_storage_creds"); + } +} diff --git a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/rest/TestIcebergTrinoRestCatalogConnectorSmokeTest.java b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/rest/TestIcebergTrinoRestCatalogConnectorSmokeTest.java index ccc9fa724fc5..b3557784d931 100644 --- a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/rest/TestIcebergTrinoRestCatalogConnectorSmokeTest.java +++ b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/rest/TestIcebergTrinoRestCatalogConnectorSmokeTest.java @@ -230,7 +230,7 @@ public void testDropTableWithMissingSnapshotFile() assertThatThrownBy(super::testDropTableWithMissingSnapshotFile) .isInstanceOf(QueryFailedException.class) .cause() - .hasMessageContaining("Failed to drop table") + .hasMessageMatching("Failed to open input stream for file: .*avro") .hasNoCause(); } diff --git a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/fileio/TestForwardingFileIo.java b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/fileio/TestForwardingFileIo.java index 47c33066c469..907046e9478c 100644 --- a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/fileio/TestForwardingFileIo.java +++ b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/fileio/TestForwardingFileIo.java @@ -13,17 +13,27 @@ */ package io.trino.plugin.iceberg.fileio; +import com.google.common.collect.ImmutableMap; import io.trino.filesystem.TrinoFileSystem; import io.trino.filesystem.local.LocalFileSystemFactory; +import io.trino.plugin.iceberg.IcebergFileSystemFactory; +import io.trino.spi.security.ConnectorIdentity; +import org.apache.iceberg.aws.s3.S3FileIOProperties; import org.apache.iceberg.io.FileIO; +import org.apache.iceberg.io.StorageCredential; import org.apache.iceberg.io.SupportsBulkOperations; +import org.apache.iceberg.io.SupportsStorageCredentials; import org.junit.jupiter.api.Test; import java.nio.file.Files; import java.nio.file.Path; +import java.util.ArrayList; +import java.util.List; +import java.util.Map; import static com.google.common.io.MoreFiles.deleteRecursively; import static com.google.common.io.RecursiveDeleteOption.ALLOW_INSECURE; +import static com.google.common.util.concurrent.MoreExecutors.newDirectExecutorService; import static io.trino.testing.InterfaceTestUtils.assertAllMethodsOverridden; import static io.trino.testing.TestingConnectorSession.SESSION; import static org.assertj.core.api.Assertions.assertThat; @@ -35,6 +45,7 @@ public void testEverythingImplemented() { assertAllMethodsOverridden(FileIO.class, ForwardingFileIo.class); assertAllMethodsOverridden(SupportsBulkOperations.class, ForwardingFileIo.class); + assertAllMethodsOverridden(SupportsStorageCredentials.class, ForwardingFileIo.class); } @Test @@ -61,4 +72,153 @@ public void testUseFileSizeFromMetadata() } deleteRecursively(tempDir, ALLOW_INSECURE); } + + @Test + public void testStorageCredentialsRemainSeparateFromProperties() + throws Exception + { + Path tempDir = Files.createTempDirectory("test_forwarding_fileio_storage_creds"); + List> seenProperties = new ArrayList<>(); + LocalFileSystemFactory localFactory = new LocalFileSystemFactory(tempDir); + IcebergFileSystemFactory capturingFactory = (identity, fileIoProperties) -> { + seenProperties.add(ImmutableMap.copyOf(fileIoProperties)); + return localFactory.create(SESSION); + }; + + try (ForwardingFileIo fileIo = new ForwardingFileIo( + localFactory.create(SESSION), + ImmutableMap.of("base.key", "base-value"), + true, + newDirectExecutorService(), + capturingFactory, + ConnectorIdentity.ofUser("test-user"))) { + fileIo.setCredentials(List.of( + StorageCredential.create("s3://bucket-a/", ImmutableMap.of( + S3FileIOProperties.ACCESS_KEY_ID, "access-a", + S3FileIOProperties.SECRET_ACCESS_KEY, "secret-a", + S3FileIOProperties.SESSION_TOKEN, "token-a")), + StorageCredential.create("s3://bucket-a/warehouse/", ImmutableMap.of( + S3FileIOProperties.ACCESS_KEY_ID, "access-b", + S3FileIOProperties.SECRET_ACCESS_KEY, "secret-b", + S3FileIOProperties.SESSION_TOKEN, "token-b")))); + + assertThat(fileIo.properties()).containsExactlyEntriesOf(ImmutableMap.of("base.key", "base-value")); + assertThat(fileIo.credentials()).hasSize(2); + assertThat(seenProperties).hasSize(2); + assertThat(seenProperties).contains(ImmutableMap.of( + "base.key", "base-value", + S3FileIOProperties.ACCESS_KEY_ID, "access-a", + S3FileIOProperties.SECRET_ACCESS_KEY, "secret-a", + S3FileIOProperties.SESSION_TOKEN, "token-a")); + assertThat(seenProperties).contains(ImmutableMap.of( + "base.key", "base-value", + S3FileIOProperties.ACCESS_KEY_ID, "access-b", + S3FileIOProperties.SECRET_ACCESS_KEY, "secret-b", + S3FileIOProperties.SESSION_TOKEN, "token-b")); + } + finally { + deleteRecursively(tempDir, ALLOW_INSECURE); + } + } + + @Test + public void testSetCredentialsAfterInitializeRebuildsPrefixFileSystems() + throws Exception + { + Path tempDir = Files.createTempDirectory("test_forwarding_fileio_refresh_creds"); + List> seenProperties = new ArrayList<>(); + LocalFileSystemFactory localFactory = new LocalFileSystemFactory(tempDir); + IcebergFileSystemFactory capturingFactory = (identity, fileIoProperties) -> { + seenProperties.add(ImmutableMap.copyOf(fileIoProperties)); + return localFactory.create(SESSION); + }; + + try (ForwardingFileIo fileIo = new ForwardingFileIo( + localFactory.create(SESSION), + ImmutableMap.of("base.key", "base-value"), + true, + newDirectExecutorService(), + capturingFactory, + ConnectorIdentity.ofUser("test-user"))) { + fileIo.setCredentials(List.of( + StorageCredential.create("s3://bucket-a/", ImmutableMap.of( + S3FileIOProperties.ACCESS_KEY_ID, "access-a", + S3FileIOProperties.SECRET_ACCESS_KEY, "secret-a", + S3FileIOProperties.SESSION_TOKEN, "token-a")))); + assertThat(seenProperties).hasSize(1); + assertThat(seenProperties.get(0)).containsEntry(S3FileIOProperties.ACCESS_KEY_ID, "access-a"); + + fileIo.setCredentials(List.of( + StorageCredential.create("s3://bucket-b/", ImmutableMap.of( + S3FileIOProperties.ACCESS_KEY_ID, "access-b", + S3FileIOProperties.SECRET_ACCESS_KEY, "secret-b", + S3FileIOProperties.SESSION_TOKEN, "token-b")))); + + assertThat(seenProperties).hasSize(2); + assertThat(seenProperties.get(1)).containsEntry(S3FileIOProperties.ACCESS_KEY_ID, "access-b"); + } + finally { + deleteRecursively(tempDir, ALLOW_INSECURE); + } + } + + @Test + public void testLongestPrefixRoutingAndFallback() + throws Exception + { + Path tempDir = Files.createTempDirectory("test_forwarding_fileio_prefix_routing"); + Path baseRoot = tempDir.resolve("base"); + Path prefixARoot = tempDir.resolve("prefix-a"); + Path prefixBRoot = tempDir.resolve("prefix-b"); + Files.createDirectories(baseRoot); + Files.createDirectories(prefixARoot); + Files.createDirectories(prefixBRoot); + + Files.createDirectories(baseRoot.resolve("other")); + Files.writeString(baseRoot.resolve("other/fallback.txt"), "ccc"); + Files.createDirectories(prefixARoot.resolve("bucket")); + Files.writeString(prefixARoot.resolve("bucket/data.txt"), "a"); + Files.createDirectories(prefixBRoot.resolve("bucket/warehouse")); + Files.writeString(prefixBRoot.resolve("bucket/warehouse/data.txt"), "bb"); + + LocalFileSystemFactory baseFactory = new LocalFileSystemFactory(baseRoot); + LocalFileSystemFactory prefixAFactory = new LocalFileSystemFactory(prefixARoot); + LocalFileSystemFactory prefixBFactory = new LocalFileSystemFactory(prefixBRoot); + IcebergFileSystemFactory routingFactory = (identity, fileIoProperties) -> { + String accessKey = fileIoProperties.get(S3FileIOProperties.ACCESS_KEY_ID); + if ("access-a".equals(accessKey)) { + return prefixAFactory.create(SESSION); + } + if ("access-b".equals(accessKey)) { + return prefixBFactory.create(SESSION); + } + throw new IllegalArgumentException("Unexpected access key: " + accessKey); + }; + + try (ForwardingFileIo fileIo = new ForwardingFileIo( + baseFactory.create(SESSION), + ImmutableMap.of("base.key", "base-value"), + true, + newDirectExecutorService(), + routingFactory, + ConnectorIdentity.ofUser("test-user"))) { + fileIo.setCredentials(List.of( + StorageCredential.create("file:///bucket/", ImmutableMap.of( + S3FileIOProperties.ACCESS_KEY_ID, "access-a", + S3FileIOProperties.SECRET_ACCESS_KEY, "secret-a", + S3FileIOProperties.SESSION_TOKEN, "token-a")), + StorageCredential.create("file:///bucket/warehouse/", ImmutableMap.of( + S3FileIOProperties.ACCESS_KEY_ID, "access-b", + S3FileIOProperties.SECRET_ACCESS_KEY, "secret-b", + S3FileIOProperties.SESSION_TOKEN, "token-b")))); + + // Longest matching prefix should select the more specific credential filesystem. + assertThat(fileIo.newInputFile("file:///bucket/warehouse/data.txt").getLength()).isEqualTo(2); + assertThat(fileIo.newInputFile("file:///bucket/data.txt").getLength()).isEqualTo(1); + assertThat(fileIo.newInputFile("file:///other/fallback.txt").getLength()).isEqualTo(3); + } + finally { + deleteRecursively(tempDir, ALLOW_INSECURE); + } + } } diff --git a/plugin/trino-iceberg/src/test/java/org/apache/iceberg/rest/DelegatingRestSessionCatalog.java b/plugin/trino-iceberg/src/test/java/org/apache/iceberg/rest/DelegatingRestSessionCatalog.java index c4feecc0b5c0..28e82e5bfcfe 100644 --- a/plugin/trino-iceberg/src/test/java/org/apache/iceberg/rest/DelegatingRestSessionCatalog.java +++ b/plugin/trino-iceberg/src/test/java/org/apache/iceberg/rest/DelegatingRestSessionCatalog.java @@ -19,9 +19,15 @@ import io.airlift.http.server.testing.TestingHttpServer; import io.airlift.node.NodeInfo; import org.apache.iceberg.catalog.Catalog; +import org.apache.iceberg.rest.credentials.Credential; +import org.apache.iceberg.rest.responses.LoadTableResponse; import java.io.Closeable; import java.io.IOException; +import java.util.ArrayList; +import java.util.List; +import java.util.Map; +import java.util.function.Consumer; import static java.util.Objects.requireNonNull; @@ -81,6 +87,7 @@ public static Builder builder() public static class Builder { private Catalog delegate; + private final List credentials = new ArrayList<>(); public Builder delegate(Catalog delegate) { @@ -88,11 +95,54 @@ public Builder delegate(Catalog delegate) return this; } + public Builder addAllCredentials(List credentials) + { + this.credentials.addAll(credentials); + return this; + } + public DelegatingRestSessionCatalog build() { requireNonNull(delegate, "Delegate must be set"); - return new DelegatingRestSessionCatalog(new RESTCatalogAdapter(delegate), delegate); + RESTCatalogAdapter adapter = credentials.isEmpty() + ? new RESTCatalogAdapter(delegate) + : new CredentialInjectingAdapter(delegate, credentials); + return new DelegatingRestSessionCatalog(adapter, delegate); + } + } + + private static class CredentialInjectingAdapter + extends RESTCatalogAdapter + { + private final List credentials; + + CredentialInjectingAdapter(Catalog catalog, List credentials) + { + super(catalog); + this.credentials = List.copyOf(credentials); + } + + @Override + public T handleRequest( + Route route, + Map vars, + HTTPRequest request, + Class responseType, + Consumer> responseHeaders) + { + T response = super.handleRequest(route, vars, request, responseType, responseHeaders); + if (response instanceof LoadTableResponse loadTableResponse) { + LoadTableResponse.Builder builder = LoadTableResponse.builder() + .withTableMetadata(loadTableResponse.tableMetadata()) + .addAllConfig(loadTableResponse.config()) + .addAllCredentials(loadTableResponse.credentials()) + .addAllCredentials(credentials); + @SuppressWarnings("unchecked") + T result = (T) builder.build(); + return result; + } + return response; } } } diff --git a/plugin/trino-iceberg/src/test/java/org/apache/iceberg/rest/RestCatalogServlet.java b/plugin/trino-iceberg/src/test/java/org/apache/iceberg/rest/RestCatalogServlet.java index e50c3fb58709..0f38e5ab0711 100644 --- a/plugin/trino-iceberg/src/test/java/org/apache/iceberg/rest/RestCatalogServlet.java +++ b/plugin/trino-iceberg/src/test/java/org/apache/iceberg/rest/RestCatalogServlet.java @@ -23,7 +23,6 @@ import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap; import org.apache.iceberg.relocated.com.google.common.io.CharStreams; import org.apache.iceberg.rest.HTTPRequest.HTTPMethod; -import org.apache.iceberg.rest.RESTCatalogAdapter.Route; import org.apache.iceberg.rest.responses.ErrorResponse; import org.apache.iceberg.util.Pair; diff --git a/pom.xml b/pom.xml index bab49244c09e..8444bf339f44 100644 --- a/pom.xml +++ b/pom.xml @@ -198,7 +198,7 @@ v24.12.0 11.7.0 5.4.2 - 1.10.1 + 1.11.0-SNAPSHOT 5.18.1 0.13.0 1.20.0 @@ -2369,6 +2369,13 @@ + + + apache-snapshots + https://repository.apache.org/content/repositories/snapshots + + +